Closed Bug 1334624 Opened 8 years ago Closed 8 years ago

Set index routes for signing tasks based on build jobs.

Categories

(Firefox Build System :: Task Configuration, task)

task
Not set
normal

Tracking

(Not tracked)

RESOLVED FIXED
mozilla54

People

(Reporter: Callek, Assigned: Callek)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached patch Route Diff (obsolete) (deleted) — Splinter Review
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)
Assignee: nobody → bugspam.Callek
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-
(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.
Attached patch Route Diff (take 2) (obsolete) (deleted) — Splinter Review
Attachment #8831270 - Attachment is obsolete: true
Attachment #8833022 - Flags: review?(mshal)
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 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+
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
Attached patch Route Diff (take 3) (deleted) — Splinter Review
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+
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
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla54
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?
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+
Product: TaskCluster → Firefox Build System
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: