Closed
Bug 1343002
Opened 7 years ago
Closed 7 years ago
Treeherder should stop using pulse_actions for anything to do with taskcluster
Categories
(Tree Management :: Treeherder, defect)
Tree Management
Treeherder
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: bstack, Assigned: bstack)
References
Details
Attachments
(1 file)
Now that tc credentials work from the th ui directly (barring any new bugs we discover), we have no reason for pulse_actions to be handling any tc specific tasks. It is still needed for bb jobs, but those aren't particularly a problem. The issue with pulse_actions is that it is a deputy for users and we end up with weird/wrong permissions checks in there to use pulse_action's very powerful tc creds. This creates two issues: 1. (very bad) if the hacky permission check is too loose, this is a major privilege escalation 2. (normal bad) if the checks are too strict, people are prevented from doing things they should be allowed to do. As it stands, issue 2 is happening. There is a check in pulse_actions that only allows jobs to be added to pushes in try pushes due to security concerns. The fix for this is to move add-new-job functionality into th ui directly. Afaict, the only two remaining things to port are add-new-jobs and retrigger.
Assignee | ||
Comment 1•7 years ago
|
||
Specifically, the porting from pulse_actions to treeherder ui isn't a terribly hard thing to do, particularly if you have proper scopes that allow you to try the action out. An example of doing this is where I've done so with add-talos-jobs https://github.com/mozilla/treeherder/pull/2092/files#diff-1dcf92fc8473c8a5e650f4b7a8220c65L196 The diff shows a move from logic that only fired off a pulse message for pulse_actions to process, to one that creates a tc client and creates an action task in tc directly, and also fires off a pulse action so that it can do its thing with bb jobs as well.
Comment 2•7 years ago
|
||
I am not sure if there are treeherder fixes needed here, but I added camd and rwood to this bug so they could chime in or help get to the right solution.
Comment 3•7 years ago
|
||
Assignee | ||
Updated•7 years ago
|
Assignee: nobody → bstack
Status: NEW → ASSIGNED
Assignee | ||
Comment 4•7 years ago
|
||
Comment on attachment 8841813 [details] [review] [treeherder] imbstack:bug-1343002 > mozilla:master Note: I just used the old action.yml instead of the new-cool action.json because it was already written for add-tasks and I just wanted to get this out of the way asap.
Attachment #8841813 -
Flags: review?(wlachance)
Comment 5•7 years ago
|
||
Comment on attachment 8841813 [details] [review] [treeherder] imbstack:bug-1343002 > mozilla:master This pretty much looks fine to me. A few small nits. Let me know when you've addressed those and I'll land it!
Attachment #8841813 -
Flags: review?(wlachance) → review+
Comment 6•7 years ago
|
||
Commit pushed to master at https://github.com/mozilla/treeherder https://github.com/mozilla/treeherder/commit/2e80b9d6d86896f100c3d9417482e04f7ecf6e6a Bug 1343002 - Make add-jobs go direct to tc for tc jobs (#2208)
Comment 7•7 years ago
|
||
Can this be marked as fixed, or is there more to do? :-)
Flags: needinfo?(bstack)
Assignee | ||
Comment 8•7 years ago
|
||
I believe this is fixed now. A couple of things are still using mozilla-tc instead of directly posting, but we can fix that in another bug.
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Flags: needinfo?(bstack)
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•