Set index routes for signing tasks based on build jobs.

RESOLVED FIXED in mozilla54

Status

Taskcluster
Task Configuration
RESOLVED FIXED
a year ago
11 months ago

People

(Reporter: Callek, Assigned: Callek)

Tracking

unspecified
mozilla54
Dependency tree / graph

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments, 2 obsolete attachments)

(Assignee)

Description

a year ago
Created attachment 8831270 [details] [diff] [review]
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
Attachment #8831270 - Flags: review?(mshal)
Comment hidden (mozreview-request)
(Assignee)

Updated

a year ago
Assignee: nobody → bugspam.Callek

Comment 2

a year ago
mozreview-review
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.
Attachment #8831272 - Flags: review?(aki) → review+
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?
Attachment #8831270 - Flags: review?(mshal) → review-
(Assignee)

Comment 4

a year ago
(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.
Blocks: 1335370
Comment hidden (mozreview-request)
(Assignee)

Comment 6

a year ago
Created attachment 8833022 [details] [diff] [review]
Route Diff (take 2)
Attachment #8831270 - Attachment is obsolete: true
Attachment #8833022 - Flags: review?(mshal)
(Assignee)

Comment 7

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

Please re-review
Attachment #8831272 - Flags: review+ → review?(aki)

Comment 8

a year ago
mozreview-review
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?
Attachment #8831272 - Flags: review?(aki) → review+
(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 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".
Attachment #8833022 - Flags: review?(mshal) → review+
(Assignee)

Comment 11

a year ago
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 hidden (mozreview-request)
(Assignee)

Comment 13

a year ago
Created attachment 8833832 [details] [diff] [review]
Route Diff (take 3)
Attachment #8833022 - Attachment is obsolete: true
Attachment #8833832 - Flags: review?(mshal)
Comment on attachment 8833832 [details] [diff] [review]
Route Diff (take 3)

This looks good to me. Thanks!
Attachment #8833832 - Flags: review?(mshal) → review+

Comment 15

11 months ago
Pushed by Callek@gmail.com:
https://hg.mozilla.org/integration/autoland/rev/051a846e1094
Set index routes for signing tasks based on build jobs. r=aki

Comment 16

11 months ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/051a846e1094
Status: NEW → RESOLVED
Last Resolved: 11 months ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
(Assignee)

Comment 17

11 months ago
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.
Attachment #8831272 - Flags: approval-mozilla-aurora?
(Assignee)

Comment 18

11 months ago
n-i to :rail,

incase this bug gets a+ after I leave for PTO, so we can ensure to land it.
Flags: needinfo?(rail)
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!
Attachment #8831272 - Flags: approval-mozilla-aurora? → approval-mozilla-aurora+

Updated

11 months ago
Blocks: 1338527

Updated

11 months ago
Blocks: 1328263
You need to log in before you can comment on or make changes to this bug.