1485680 - Convert release-promotion to an action hook

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

The actual change to accomplish this is tiny, similar to
  https://reviewboard.mozilla.org/r/253236/diff/4

However, the implications are larger, and probably security sensitive, and also relate to scripts that start off releases.  So, I'd like to work with someone from the release team to get this right..  Rail, is that you?

Comment 1

[Aki Sasaki (not active)]

Does converting to a hook disable non-hook action triggers? If so, we'd have to change and test our various processes more thoroughly before cutting over. If not, we could just do a quick staging release test on maple before landing.

Security-wise, I *think* this should be an overall improvement: we would be able to more easily restrict the release scopes to the hook, rather than to all level 3 roles. Do you have any specific concerns here? Am I missing anything?

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

I think your first question is the concern -- actions.json would contain {kind: "hook", ..}, and not {kind: "task", ..}, so if there's automation looking for the latter, it will fail.

For treeherder and TC tools, we have implemented support for both kinds, meaning that the kind change can ride the trains (or be uplifted).  Maybe that's a good choice here, too?

And, yes, I think there's a good chance for this to be a big security improvement.  We can issue various scopes to *only* this action -- not crontasks or pushes to the repo.  And we can limit who has access to call triggerHook. And, we can write a schema for the hook payload to limit the allowed inputs.  I don't really have the bandwidth to figure out what those should be, but given that information I can definitely get them configured.

Comment 3

[Aki Sasaki (not active)]

(In reply to Dustin J. Mitchell [:dustin] pronoun: he from comment #2)
> I think your first question is the concern -- actions.json would contain
> {kind: "hook", ..}, and not {kind: "task", ..}, so if there's automation
> looking for the latter, it will fail.
> 
> For treeherder and TC tools, we have implemented support for both kinds,
> meaning that the kind change can ride the trains (or be uplifted).  Maybe
> that's a good choice here, too?

Makes sense. Looks like we should try this on maple, and either verify it works or add support for hooks where needed before rolling out.

> And, yes, I think there's a good chance for this to be a big security
> improvement.  We can issue various scopes to *only* this action -- not
> crontasks or pushes to the repo.  And we can limit who has access to call
> triggerHook. And, we can write a schema for the hook payload to limit the
> allowed inputs.  I don't really have the bandwidth to figure out what those
> should be, but given that information I can definitely get them configured.

+1, thanks for the offer! We'll need to compile that list.

Comment 4

[Aki Sasaki (not active)]

Attachment: input.yml for maple 62.0b14 build 5 hook

Comment 5

[Aki Sasaki (not active)]

Current status:

- enabled relpro-hook on the maple `relpro-hook` branch. [1]  This is visible on treeherder here [2].

- I was able to trigger a promotion graph off that revision. action task: [3] graph: [4].  The input.yml is here [5].

    a) this Just Works from treeherder (no scopes issues), so hopefully it's straightforward to trigger from ship-it.

    b) it looks like switching to the hook breaks CoT verification on the action task. This is because the treeherder symbol is currently set to "${input.release_promotion_flavor}"; we'll need to have that expand to its actual value during cot verification.

- Once I applied this patch [6] locally, `verify_cot --task-type action -- PV3gwVnWSgaGzUQuqVr41w` works.  This means we have a working solution.  We can find a better solution or go with this one.

Next:

- Rail was going to look at implementing action hook handling in ship-it.  The implementation details here will guide how we roll out the changes to the release branches.  (E.g., if we can support both hooks and non-hooks simultaneously, we have the option of riding the trains; if it requires a hard cutover, we'll need to uplift everywhere.)
- We should roll out a CoT fix in scriptworker.
- Once we're ready, we can uplift the in-tree patch to the various release branches.
- At that point, we can look at restricting scopes to the hook.  We will likely need at least 3 sets of scopes: one for nightly cron, one for staging releases on level 1 trees, and one for real releases.  If we're ambitious, we can split that last one into esr vs beta vs release (vs nightly promotion, once shippable builds is a thing).


[1] https://hg.mozilla.org/projects/maple/rev/358b78517a800f02e025a9020ffbf0f8516c204c
[2] https://treeherder.mozilla.org/#/jobs?repo=maple&selectedJob=198189192
[3] https://tools.taskcluster.net/groups/E7w_SFN8TouLJXQ-xNVbhw/tasks/PV3gwVnWSgaGzUQuqVr41w/details
[4] https://tools.taskcluster.net/groups/PV3gwVnWSgaGzUQuqVr41w
[5] https://bugzilla.mozilla.org/attachment.cgi?id=9007401
[6] https://github.com/mozilla-releng/scriptworker/pull/256

Comment 6

[Aki Sasaki (not active)]

Attachment: bug 1485680 - make the relpro action a hook.

make the relpro action a hook.

Comment 7

[Aki Sasaki (not active)]

Attachment: GitHub Pull Request

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 9008177 [details]
bug 1485680 - make the relpro action a hook.

Dustin J. Mitchell [:dustin] pronoun: he has approved the revision.

Comment 9

[[e-mail] (:bhearsum)]

I think this has become higher priority since bug 1491186 landed -- release promotion is currently busted on Maple with:
[task 2018-09-25T00:18:01.880Z] + ./mach --log-no-times taskgraph action-callback
[task 2018-09-25T00:18:02.404Z] loading config from `taskcluster/ci/config.yml`
[task 2018-09-25T00:18:03.236Z] Traceback (most recent call last):
[task 2018-09-25T00:18:03.236Z]   File "/builds/worker/checkouts/gecko/taskcluster/mach_commands.py", line 252, in action_callback
[task 2018-09-25T00:18:03.236Z]     test=False)
[task 2018-09-25T00:18:03.236Z]   File "/builds/worker/checkouts/gecko/taskcluster/taskgraph/actions/registry.py", line 353, in trigger_action_callback
[task 2018-09-25T00:18:03.236Z]     sanity_check_task_scope(callback, parameters, graph_config)
[task 2018-09-25T00:18:03.236Z]   File "/builds/worker/checkouts/gecko/taskcluster/taskgraph/actions/registry.py", line 332, in sanity_check_task_scope
[task 2018-09-25T00:18:03.236Z]     raise Exception('Expected task scope {} for this action'.format(expected_scope))
[task 2018-09-25T00:18:03.236Z] Exception: Expected task scope assume:repo:hg.mozilla.org/projects/maple:action:generic for this action

(From https://tools.taskcluster.net/groups/PC32PCpzQy2U_aK7vpxKpA/tasks/bE30WPeoT_i5me8Pp-5ZIw/details)

Comment 10

[Aki Sasaki (not active)]

I have two patches that fix the CoT issue. One's close; we could defer the ownTaskId issue if we wanted it sooner. However, we don't currently have anyone working on the ship-it v2 side, so that may be the long pole.

Comment 11

[Aki Sasaki (not active)]

Attachment: Bug 1485680 - add actionPerm to actions.json for hooks. r=dustin

Comment 12

[Aki Sasaki (not active)]

Attachment: GitHub Pull Request

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/24bb83f24599
add actionPerm to actions.json for hooks. r=dustin

Comment 14

[Aki Sasaki (not active)]

Status:

X actionPerm:
  X review & land https://bugzilla.mozilla.org/attachment.cgi?id=9032313
_ relpro hook cot:
  _ review & land https://github.com/mozilla-releng/scriptworker/pull/286
  _ scriptworker release
  _ roll out via puppet
_ shipit v2 relpro hook support
_ make relpro a hook
  _ land https://bugzilla.mozilla.org/attachment.cgi?id=9008177

Comment 15

[Razvan Maries]

https://hg.mozilla.org/mozilla-central/rev/24bb83f24599

Comment 16

[Aki Sasaki (not active)]

Attachment: Bug 1485680 - if we modify the action hook task logic, we need to update scriptworker. r=dustin

Comment 17

[Aki Sasaki (not active)]

Status:

X actionPerm:
  X review & land https://bugzilla.mozilla.org/attachment.cgi?id=9032313
X relpro hook cot:
  X review & land https://github.com/mozilla-releng/scriptworker/pull/286
  X scriptworker release
  X roll out via puppet
_ add a comment in ci-admin to update scriptworker if action hook logic changes
_ shipit v2 relpro hook support
_ make relpro a hook
  _ land https://bugzilla.mozilla.org/attachment.cgi?id=9008177
_ fix up scopes

Comment 18

[Rail Aliiev [:rail]]

I pushed these to maple in order to test the ship-it work

remote:   https://hg.mozilla.org/projects/maple/rev/87d8647c98ec0a971cf0e0798f9ac705b73bc091
remote:   https://hg.mozilla.org/projects/maple/rev/7eab4a4ea5ff0b8496896a623d060e7514ee90ce

Comment 19

[Tom Prince [:tomprince]]

https://hg.mozilla.org/releases/mozilla-esr60/rev/725ac683a79c

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/2f95100eef22
make the relpro action a hook. r=dustin

Comment 21

[Daniel Varga [:dvarga]]

https://hg.mozilla.org/mozilla-central/rev/2f95100eef22

Comment 22

[Aki Sasaki (not active)]

I believe we need to uplift the final patch to esr60, beta, and release to make relpro a hook everywhere.
We may want bug 1491371 to land first, so we don't break TB.

Comment 23

[Tom Prince [:tomprince]]

(In reply to Aki Sasaki [:aki] from comment #22)

I believe we need to uplift the final patch to esr60, beta, and release to make relpro a hook everywhere.
We may want bug 1491371 to land first, so we don't break TB.

I concur with all of this.

Comment 24

[Tom Prince [:tomprince]]

https://hg.mozilla.org/releases/mozilla-beta/rev/37273deb744f

Comment 25

[Aki Sasaki (not active)]

https://hg.mozilla.org/releases/mozilla-release/rev/2e051d69d6e4

Comment 26

[Aki Sasaki (not active)]

https://hg.mozilla.org/releases/mozilla-esr60/rev/f5043009e5ef

Comment 27

[Rail Aliiev [:rail]]

\o/ \o/ \o/ \o/ \o/ \o/ \o/