1334624 - Set index routes for signing tasks based on build jobs.

Status: RESOLVED FIXED

(Firefox Build System :: Task Configuration, task)

Description

[Justin Wood (:Callek)]

Attachment: Route Diff

This is in service of the need for pushapk worker to fetch a signed copy of the multi locale build, for publishing to google play.

There is likely a need for other consumers of this too.

I'm explicitly NOT doing this for l10n at the moment, due to Bug 1333234 and would block rolling this out for l10n, on either a strong need/desire from someone or Bug 1333255 being done.

========

On the route difference attachment: r? to mshal to validate the actual naming/expansion of these routes.

This was generated with the to-be-attached-patch like:

$ hg up -r central
$ ./mach taskgraph full -p ./parameters-desktop-aurora.yml --json > ../jobs_test1.json 
$ cat ../jobs_test1.json | jq '[. | to_entries[] | select(.key) | .value = .value.task.routes ] | from_entries' > routes_before.json
$ hg up -r <patch>
$  ./mach taskgraph full -p ./parameters-desktop-aurora.yml --json > ../jobs_test1.json 
$ cat ../jobs_test1.json | jq '[. | to_entries[] | select(.key) | .value = .value.task.routes ] | from_entries' > routes_after.json
$ diff -U10 ./routes_before.json ./routes_after.json

Comment 1

[Justin Wood (:Callek)]

Attachment: Bug 1334624 - Set index routes for signing tasks based on build jobs.

Review commit: https://reviewboard.mozilla.org/r/107832/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/107832/

Comment 2

[Aki Sasaki (not active)]

Comment on attachment 8831272 [details]
Bug 1334624 - Set index routes for signing tasks based on build jobs.

https://reviewboard.mozilla.org/r/107832/#review109048

lgtm, provided mshal approves the naming.

Comment 3

[Michael Shal [:mshal]]

Comment on attachment 8831270 [details] [diff] [review]
Route Diff

I think it is confusing to have a route that is both a task and a namespace - it's like having a file that also operates as a directory. Specifically, the route:

index.gecko.v2.mozilla-aurora.nightly.latest.mobile.android-api-15-opt

Will have a task associated with it, and the same route will also have sub-namespaces associated with it (the ".signed" route). I think we should move the signed name elsewhere in the route so that we can maintain only having tasks as leaf nodes. Some possibilities might be:

gecko.v2-signed.mozilla-aurora...
gecko-signed.v2.mozilla-aurora...
gecko.v2.mozilla-aurora.latest.mobile-l10n-signed...

Or even just a new top-level namespace:

signed.gecko.v2.mozilla-aurora...

Your thoughts? Are there other concerns you have that wouldn't be addressed by moving it?

Comment 4

[Justin Wood (:Callek)]

(In reply to Michael Shal [:mshal] from comment #3)
> Comment on attachment 8831270 [details] [diff] [review]
> Route Diff
> 
...
> Your thoughts? Are there other concerns you have that wouldn't be addressed
> by moving it?

In a convo in #releng ( started at http://logs.glob.uno/?c=mozilla%23releng&s=30+Jan+2017&e=30+Jan+2017#c280761 ), mshal and myself discussed the options here, and with dustin and catlee joining briefly we have settled on:

gecko.v2.<branch>.signed.*  with unsigned staying the same.

I'll put forward a new patch and a new route diff within the next day or so.

Comment 5

[Justin Wood (:Callek)]

Comment on attachment 8831272 [details]
Bug 1334624 - Set index routes for signing tasks based on build jobs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107832/diff/1-2/

Comment 6

[Justin Wood (:Callek)]

Attachment: Route Diff (take 2)

Comment 7

[Justin Wood (:Callek)]

Comment on attachment 8831272 [details]
Bug 1334624 - Set index routes for signing tasks based on build jobs.

Please re-review

Comment 8

[Aki Sasaki (not active)]

Comment on attachment 8831272 [details]
Bug 1334624 - Set index routes for signing tasks based on build jobs.

https://reviewboard.mozilla.org/r/107832/#review110384

Hm.  I'm not 100% sure about this, but it makes sense.  We may end up having multiple levels of 'signed'.  For instance, if we sign with the dep or nightly key, and then re-sign with the release key for relpromo, we might have dep-, nightly-, and/or release-signed bits, so '.signed' may or may not help us differentiate which bits these are.  Having said that, we don't have plans laid in stone for that, just a want-to, so maybe we rename if and when that happens?

Comment 9

[Michael Shal [:mshal]]

(In reply to Aki Sasaki [:aki] from comment #8)
> Hm.  I'm not 100% sure about this, but it makes sense.  We may end up having
> multiple levels of 'signed'.  For instance, if we sign with the dep or
> nightly key, and then re-sign with the release key for relpromo, we might
> have dep-, nightly-, and/or release-signed bits, so '.signed' may or may not
> help us differentiate which bits these are.  Having said that, we don't have
> plans laid in stone for that, just a want-to, so maybe we rename if and when
> that happens?

Would it seem reasonable to have '.signed.[keyname]' then? Callek, how hard would that be to add now? IMO, transitioning from .signed now to .signed.[keyname] in the future would be more annoying, since you'd have all the "old" routes still listed under the .signed namespace. Or is there some other way we'd differentiate between these keys?

Comment 10

[Michael Shal [:mshal]]

Comment on attachment 8833022 [details] [diff] [review]
Route Diff (take 2)

This looks great! Thanks for making the adjustments.

Even though this is r+, I think we need to address aki's concerns in #c8 before landing. I'd prefer we avoid trying to shoehorn that in later. So consider this "r+ assuming we only ever need one level of signing in the index".

Comment 11

[Justin Wood (:Callek)]

To document in-bug:

tl;dr we're going to use

`gecko.v2.<branch>.signed-nightly.*` where * is the stuff currently after `gecko.v2.<branch>` and leave the unsigned bits at `gecko.v2.<branch>.*`

This to facilitate a likely alternate type of signing involved (as in release signing, etc)

====
12:57 PM <Callek> mshal: sooo, re: signing routes, I can envision ` dep signing` and `release signing` in the future easily. right now its only nightly....
12:57 PM <Callek> mshal: do you want me to do `.signing.<type>.*` instead?
12:57 PM <mshal> Callek: how hard is that to add? Do you know the type already when the task is created?
12:58 PM <mshal> if you know already that we're going to need to distinguish between them, I think it makes sense to make space for it now
12:58 PM <Callek> mshal: yea the type of signing key is embedded in the scopes, so we do know it ahead of time
12:58 PM <mshal> oh, cool
12:58 PM <Callek> I'm not 100% sure that we need to, but I can easily see a strong likelihood
12:58 PM <Callek> (e.g. if we sign every-ci-job with a `dep` key)
12:59 PM <Callek> (and if we do multiple signings on the same rev for e.g. release)
12:59 PM <mshal> ok - sounds like it makes sense to add it now
12:59 PM <Callek> mshal: need a full new pass, or shall I modify and land?
12:59 PM <mshal> so it would be like "signed.dep.blah", "signed.release.blah", "signed.nightly.blah" ?
1:00 PM rail-lunch → rail
1:02 PM <Callek> yea
1:02 PM <mshal> sounds good to me, I'm not sure if the code changes would warrant re-review though
1:02 PM <Callek> for this I'm only hardcoding `.nightly` for now, until we have code to support another key
1:02 PM <Callek> :-)
1:03 PM <mshal> hmm, one sec
1:04 PM <mshal> so that would make the route for some of them "index.gecko.v2.mozilla-aurora.signed.nightly.nightly...", right?
1:04 PM <mshal> IOW, nightly twice in a row
1:04 PM <mshal> is there another name you could use to make it clear that the "nightly" under signed means a nightly signing key rather than a nightly build?
1:05 PM <Callek> mshal: could do signed-nightly and signed-dep and signed-release if it suits you?
1:06 PM <Callek> but yea, it would make it signed.nightly.nightly.... in current thoughts
1:06 PM <Callek> not sure how else to differentiate `nightly` though
1:07 PM kmoir → kmoir-afk
1:08 PM <mshal> Callek: I think I like signed-foo a bit better. If we knew about this ahead of time, we could've then had "unsigned" at the same level as "signed-nightly", "signed-dep", etc, which makes sense to me
1:08 PM <mshal> instead of having one level for unsigned and two levels for signed things ("unsigned" vs "signed.key")
1:09 PM <mshal> Callek: do you mind generating the diff with the dash version? Sometimes it helps just to see it all spelled out
1:09 PM <Callek> mshal: sure.... -- is that diff a "you want to review+" it anew, or is that a "you just want to see it anyway"
1:10 PM <mshal> I can review it real quick... mostly just want to see if anything looks weird that I'm missing

Comment 12

[Justin Wood (:Callek)]

Comment on attachment 8831272 [details]
Bug 1334624 - Set index routes for signing tasks based on build jobs.

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/107832/diff/2-3/

Comment 13

[Justin Wood (:Callek)]

Attachment: Route Diff (take 3)

Comment 14

[Michael Shal [:mshal]]

Comment on attachment 8833832 [details] [diff] [review]
Route Diff (take 3)

This looks good to me. Thanks!

Comment 15

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/051a846e1094
Set index routes for signing tasks based on build jobs. r=aki

Comment 16

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/051a846e1094

Comment 17

[Justin Wood (:Callek)]

Comment on attachment 8831272 [details]
Bug 1334624 - Set index routes for signing tasks based on build jobs.

Approval Request Comment

This is necessary on aurora to have an easy way to get at signed artifacts. It's needed for pushapk at the least, and we'll need this (or a slightly modified version of this) for beta builds based on taskcluster as well.

Comment 18

[Justin Wood (:Callek)]

n-i to :rail,

incase this bug gets a+ after I leave for PTO, so we can ensure to land it.

Comment 19

[Liz Henry (:lizzard) (relman/hg->git project)]

Comment on attachment 8831272 [details]
Bug 1334624 - Set index routes for signing tasks based on build jobs.

We need this to upload new aurora apks, let's take it!

Comment 20

[Justin Wood (:Callek)]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2e0af7243d08369ef93638aa306f6cc2056c28b9