Closed Bug 1340609 Opened 3 years ago Closed 3 years ago

Enable signing for linux CI builds on jamun

Categories

(Release Engineering :: Release Automation: Other, defect, P2)

defect

Tracking

(Not tracked)

RESOLVED FIXED

People

(Reporter: rail, Assigned: aki)

References

Details

Attachments

(6 files)

We need these promotable, so they should be signed.
This is essentially making the default CI graph for linux the release graph on Jamun, including scopes changes.
Assignee: nobody → aki
Keywords: leave-open
Attached patch beta-hack.diffSplinter Review
Dustin:

1) I asked for review (via mozreview) on the scopes-on-demand patch, which I think is ready.  This elevates our beta+release scopes for release promotion.  We probably want this to be even more customizable later, but this works for now. This also makes things like nightly graphs on try and project branches possible without breaking chain of trust verification, though we still have some ways to go to green up nightly-graphs-on-try.  Once balrog and beetmover can handle different scope levels, we can update the various scopes in taskcluster.taskgraph.utils.scriptworker.  Let me know if this approach works for you?

2) For on-push graphs on jamun (later mozilla-beta), we need to a) trigger the linux nightly graph minus beetmover and balrog, and b) trigger the android CI graph.  Attached is my attempt at it, but it's not working so far.  Do you have any suggestions?
Attachment #8838966 - Flags: feedback?(dustin)
Comment on attachment 8838965 [details]
bug 1340609 - toggle nightly scopes on-demand.  a=release

https://reviewboard.mozilla.org/r/113722/#review115598

::: taskcluster/taskgraph/transforms/balrog.py:105
(Diff revision 1)
>              'worker-type': 'scriptworker-prov-v1/balrogworker-v1',
>              'worker': {
>                  'implementation': 'balrog',
>                  'upstream-artifacts': upstream_artifacts,
>              },
>              # bump this to nightly / release when applicable+permitted

is this comment still relevant?

::: taskcluster/taskgraph/transforms/checksums_signing.py:88
(Diff revision 1)
>              'worker-type': "scriptworker-prov-v1/signing-linux-v1",
>              'worker': {'implementation': 'scriptworker-signing',
>                         'upstream-artifacts': upstream_artifacts,
>                         'max-run-time': 3600},
>              'scopes': [
> -                "project:releng:signing:cert:nightly-signing",
> +                signing_cert_scope,

Is this how the signing worker figures out which cert to use, by examining task.scopes?  What would happen if a task had `project:releng:signing:cert:*`, or multiple scopes?  

From a design perspective, I try to avoid pattern-matching on scopes, preferring to specify something explicitly elsewhere, and then just verify that the corresponding scope exists.  Here that would mean something like `task.payload.cert = "nightly-signing"`, with a scope-satisfaction check in the scriptworker for `project:releng:signing:cert:<cert>`.

Not blocking this patch, just something I noticed here.

::: taskcluster/taskgraph/util/scriptworker.py:66
(Diff revision 1)
> +def get_scope_from_project(level_to_project_map, alias_to_scope_map, config):
> +    for alias, projects in level_to_project_map:

I'm confused by the terminology here, and in particular `for alias, projects in level_to_project_map`.  Is it an alias or a level?  In general, a bit more comments in this file would be good, as I'm not even sure I understand it after staring at it for a while, and a logic error here might lead to using the wrong key or bucket.

::: taskcluster/taskgraph/util/scriptworker.py:66
(Diff revision 1)
> +def get_scope_from_project(level_to_project_map, alias_to_scope_map, config):
> +    for alias, projects in level_to_project_map:
Attachment #8838965 - Flags: review?(dustin) → review+
Comment on attachment 8838966 [details] [diff] [review]
beta-hack.diff

Review of attachment 8838966 [details] [diff] [review]:
-----------------------------------------------------------------

::: .taskcluster.yml
@@ +110,5 @@
>                --base-repository='https://hg.mozilla.org/mozilla-central'
>                --head-repository='{{{url}}}'
>                --head-ref='{{revision}}'
>                --head-rev='{{revision}}'
> +              --target-tasks-method='beta_tasks'

This line will mean that every push is handled with `beta_tasks`, regardless of tree -- that doesn't seem like what you want?

I suspect that instead you want to set the default parameter for the `mozilla-beta` project (in PER_PROJECT_PARAMETERS in taskcluster/taskgraph/decision.py)

::: taskcluster/taskgraph/target_tasks.py
@@ +213,5 @@
> +                            'linux-nightly'):
> +            return False
> +        if platform in ('linux64-nightly', 'linux-nightly'):
> +            for skip in ('beetmover', 'checksum', 'balrog'):
> +                if skip in task.label:

Please use an an attribute (maybe just attributes["kind"]) to identify these tasks.  Substring matching on labels is risky.  In this case, I can imagine scenarios where someone adds an unrelated task that includes the string "checksum", which then causes issues when it hits beta.
Attachment #8838966 - Flags: feedback?(dustin) → feedback-
(In reply to Dustin J. Mitchell [:dustin] from comment #9)
> Comment on attachment 8838966 [details] [diff] [review]
> beta-hack.diff
> 
> Review of attachment 8838966 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: .taskcluster.yml
> @@ +110,5 @@
> >                --base-repository='https://hg.mozilla.org/mozilla-central'
> >                --head-repository='{{{url}}}'
> >                --head-ref='{{revision}}'
> >                --head-rev='{{revision}}'
> > +              --target-tasks-method='beta_tasks'
> 
> This line will mean that every push is handled with `beta_tasks`, regardless
> of tree -- that doesn't seem like what you want?

I was going to land it on m-b, so m-c and m-a would never see this line.  I can certainly do this in a more train-friendly way; landing on m-b was going to be the quick fix for fx53.

> I suspect that instead you want to set the default parameter for the
> `mozilla-beta` project (in PER_PROJECT_PARAMETERS in
> taskcluster/taskgraph/decision.py)

I'll try that, thanks!

> ::: taskcluster/taskgraph/target_tasks.py
> @@ +213,5 @@
> > +                            'linux-nightly'):
> > +            return False
> > +        if platform in ('linux64-nightly', 'linux-nightly'):
> > +            for skip in ('beetmover', 'checksum', 'balrog'):
> > +                if skip in task.label:
> 
> Please use an an attribute (maybe just attributes["kind"]) to identify these
> tasks.  Substring matching on labels is risky.  In this case, I can imagine
> scenarios where someone adds an unrelated task that includes the string
> "checksum", which then causes issues when it hits beta.

Will do.
(In reply to Dustin J. Mitchell [:dustin] from comment #8)
> ::: taskcluster/taskgraph/transforms/checksums_signing.py:88
> (Diff revision 1)
> >              'worker-type': "scriptworker-prov-v1/signing-linux-v1",
> >              'worker': {'implementation': 'scriptworker-signing',
> >                         'upstream-artifacts': upstream_artifacts,
> >                         'max-run-time': 3600},
> >              'scopes': [
> > -                "project:releng:signing:cert:nightly-signing",
> > +                signing_cert_scope,
> 
> Is this how the signing worker figures out which cert to use, by examining
> task.scopes?  What would happen if a task had
> `project:releng:signing:cert:*`, or multiple scopes?

Yes, this specifies which level of certs to use (essentially which signing server to talk to).

Multiple project:releng:signing:cert: scopes will die: https://github.com/mozilla-releng/signingscript/blob/7519e23645f89933ba81eda8b2c0407563bc39dc/signingscript/task.py#L27

project:releng:signing:cert:* won't match anything in passwords.json and will die: https://hg.mozilla.org/build/puppet/file/94bb8a3a6c7d/modules/signing_scriptworker/templates/passwords.json.erb#l2
https://github.com/mozilla-releng/signingscript/blob/7519e23645f89933ba81eda8b2c0407563bc39dc/signingscript/task.py#L45

Specifying nightly-signing or release-signing on non-allowlisted branches will die in cot verification:
https://github.com/mozilla-releng/scriptworker/blob/dbb8ae3dc3b970f10162cc4f219937ed9522a890/scriptworker/constants.py#L182-L187
https://github.com/mozilla-releng/scriptworker/blob/dbb8ae3dc3b970f10162cc4f219937ed9522a890/scriptworker/cot/verify.py#L1136-L1144

> From a design perspective, I try to avoid pattern-matching on scopes,
> preferring to specify something explicitly elsewhere, and then just verify
> that the corresponding scope exists.  Here that would mean something like
> `task.payload.cert = "nightly-signing"`, with a scope-satisfaction check in
> the scriptworker for `project:releng:signing:cert:<cert>`.
> 
> Not blocking this patch, just something I noticed here.

I'm following the funsize signingworker model:
https://github.com/mozilla-releng/signingworker/blob/master/signingworker/task.py#L24

We're also following this model for beetmover and balrog scriptworkers for fx53b1.

We can certainly adjust, but I think our current scope checking is sufficient.  By hardcoding behavior to the scope, we make sure we restrict the ability to request certain behaviors to scopes, and by only allowing certain scopes on certain branches, and by dynamically switching those scopes in the decision task, I think it's user friendly.  The only thing that isn't user friendly is trying to hack the graph without the decision task, which chain of trust is trying to discourage anyway.

If we think this isn't the ideal approach, let's make a followup fix after we get 53b1 squared away.  I'm not convinced avoiding scope pattern matching is the right approach here, but we can discuss further if this still bothers you.

> ::: taskcluster/taskgraph/util/scriptworker.py:66
> (Diff revision 1)
> > +def get_scope_from_project(level_to_project_map, alias_to_scope_map, config):
> > +    for alias, projects in level_to_project_map:
> 
> I'm confused by the terminology here, and in particular `for alias, projects
> in level_to_project_map`.  Is it an alias or a level?  In general, a bit
> more comments in this file would be good, as I'm not even sure I understand
> it after staring at it for a while, and a logic error here might lead to
> using the wrong key or bucket.

I did call it level earlier, and renamed it to alias because a) it's not necessarily 1-2-3, since aliases like "all-nightly-branches" are a superset of branches that are allowlisted to run nightly signing rather than just m-c, and b) we use level for commit level, which is also something different.

I can certainly rename it level, and add more comments.
The scope thing definitely doesn't have to be fixed here.  The idea is that scopes shouldn't communicate information about what the task should do; they should just represent permission to do it.

I agree with your reasons to call it "alias", so maybe use that throughout?
Priority: -- → P2
Comment on attachment 8840047 [details]
bug 1340609 - promotable beta linux builds on push.  a=release

https://reviewboard.mozilla.org/r/114598/#review116102
Attachment #8840047 - Flags: review?(dustin) → review+
Attached patch fix-scopes.diffSplinter Review
balrog and beetmover scripts aren't ready for the new scopes yet.
Attachment #8840205 - Flags: review?(mtabara)
Attachment #8840205 - Flags: review?(mtabara) → review+
This patch:

1) updates balrog, beetmover, and signing scriptworkers to scriptworker 2.3.0, which updates the restricted scopes.

2) removes the scriptworker_worker_types from scriptworker.yaml.  If we don't specify here, we'll fall back to the constants, which are more up to date: https://github.com/mozilla-releng/scriptworker/blob/master/scriptworker/constants.py#L131

Nothing's broken here; we also check for the provisioner:
https://github.com/mozilla-releng/scriptworker/blob/master/scriptworker/cot/verify.py#L231-L234
This part of the patch is just to clean things up.
Attachment #8840215 - Flags: review?(mtabara)
Comment on attachment 8840215 [details] [diff] [review]
scriptworker-2.3.0.diff

lgtm! - we're backward compatible with restricted scopes so it shouldn't affect current beetmoverworkers.
Attachment #8840215 - Flags: review?(mtabara) → review+
https://hg.mozilla.org/build/puppet/rev/f9d2577ebe3cb1877125b3daa0355750d76e55e3
bug 1340609 - scriptworker 2.3.0 for balrog, beetmover, signing scriptworkers. r=mtabara
Attachment #8840160 - Flags: review?(rail) → review+
Uplifted to aurora yesterday.  I think we're done here.
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Removing leave-open keyword from resolved bugs, per :sylvestre.
Keywords: leave-open
You need to log in before you can comment on or make changes to this bug.