-
Notifications
You must be signed in to change notification settings - Fork 36
AUT-293: add stage formats for all dev/fake-prod signingscript formats #1085
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
057b3e1 to
fd3008c
Compare
|
This PR is still in draft while we wait for all of the necessary keys to be populated in autograph stage. (I'll probably also do some sanity checking with scriptworker dev instances before having this reviewed and landed.) |
13807e0 to
9d5fb66
Compare
3e3a933 to
fd4de3a
Compare
5026ac2 to
82e5660
Compare
14a51ba to
3a041b4
Compare
jcristau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oops, I only looked at the first commit initially.
456ff73 to
d3eee3e
Compare
|
https://treeherder.mozilla.org/jobs?repo=try&revision=09c0318c363c2043d61db910ba924de5c2a68e7c&searchStr=sign has my latest tests with this patch + up-to-date sops + cloudops-infra. I've also prepared the |
|
I'm going to fix up the things you mentioned, I also want to note here that I'm looking at a strange failure in this linux l10n signing job. In just this one chunk, we ended up with: Normally I'd write this off as network gremlins, but the fact that it's trying to connect to autograph prod is very surprising, as it's configured only to use stage formats. I've pushed an additional change with some logging improvements to help diagnose this, and see if in fact all of the chunks are doing some signing against prod. |
a763a35 to
67a2eef
Compare
This turned out to be another case of a format being hardcoded deep in the bowels of signingscript. I've fixed it, and found a bunch more along the way. I've also added some logging on the success cases. I'm kicking off some widespread testing again, and I'll verify that a) things continue to work, and b) everything is actually using stage when it should. Regardless, this will need review again, as the changes I've made are not trivial. |
02420a5 to
1b79253
Compare
|
My latest try push is still finishing up, but it's already done every type of signing AFAICT, and I'm quite confident it will turn out fine. (The scattered failures are DNS resolution issues, which I've reported back to the Autograph team about.) |
jcristau
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, just a couple of logging nits.
| fmt = None | ||
| base_filename = os.path.basename(filename) | ||
| if base_filename not in _WIDEVINE_BLESSED_FILENAMES and base_filename not in _WIDEVINE_NONBLESSED_FILENAMES: | ||
| log.debug(f"{filename} is not a widevine signing file") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is going to be quite noisy...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, that's fair. It's probably not necessary to add this in the end; we already log the other path.
| log.debug("{} is already signed! Skipping...".format(filename)) | ||
| blessed = True | ||
|
|
||
| log.debug("Found {} to sign {}".format(filename, blessed)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"Found firefox/firefox-bin to sign False" is a bit of an odd message
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You're right, this could be improved.
|
Another failure on the try push is https://firefox-ci-tc.services.mozilla.com/tasks/FIpZBU_ZSUqWZ5KbKncx1g, where iscript doesn't know about stage_autograph_langpack; probably something to handle (or not) separately from this PR. |
We need this to easily allow dev and fake-prod scriptworkers to opt into testing against Autograph stage. Rather than duplicating all of the formats, we add some simple fallback to the non-stage_ version when selecting signing function. (Note that we must pass through the original format to allow `get_autograph_config` to find the correct server configuration deeper down the stack.)
These variants don't necessarily use the same certs as the autograph prod versions, but they are similar enough that they allow us to verify that Autograph works from a functional point of view.
Yeah. I did some best effort testing of stage with iscript, but that configuration and set-up is a whole other beast that I'm not prepared to tackle at the moment. (I did do enough testing and set-up to verify that iscript machines can route to the new autograph envs, and that the other types of signing they do with autograph works.) |
1b2393a to
eba63fc
Compare
This is part 1 of the plan from this document. The latest version of this has been tested on dev scriptworkers in https://treeherder.mozilla.org/jobs?repo=try&revision=3856381ba7e5d539896bc5dca31aa7a4df43c7ed. There are a handful of failures that we can ignore:
stage_autograph_langpackon the dev mac signing worker. I could, theoretically, do this and rerun, but I honestly don't think it's worth the effort. All of the non ja-jP-mac signing worked fine, because we don't do langpacks for them. I have no reason to believe this will fail in fake-prod or prod.