Skip to content
This repository has been archived by the owner. It is now read-only.

Bug 1380829: Added binary transparency test and template #254

Merged
merged 12 commits into from Jul 26, 2017

Conversation

@BrandonTang
Copy link
Contributor

BrandonTang commented Jul 19, 2017

No description provided.

Copy link
Contributor

MihaiTabara left a comment

Skimmed this at first glance and looks overall good. You need to rebase this againast latest HEAD as you're a bit behind. Let me know when this is final and I can look again. Nice work so far, btw 👍

reruns: 5
task:
provisionerId: "scriptworker-prov-v1"
workerType: "dummy-worker-transpar"

This comment has been minimized.

Copy link
@MihaiTabara

MihaiTabara Jul 19, 2017

Contributor

Are you gonna switch these to real workerTypes in a follow-up commit are you relying on these dummy workers for now and address these later as you rollback binarytransparency fully productional?

This comment has been minimized.

Copy link
@BrandonTang

BrandonTang Jul 19, 2017

Author Contributor

Yup, the dummy workers are for now and will be addressed as binary transparency is rolled out.

@rail

This comment has been minimized.

Copy link
Member

rail commented Jul 19, 2017

I'd copy one of the EN_US_CONFIGs to match the new introduced variables there.

source: https://github.com/mozilla/releasetasks

extra:
{{ task_notifications("{} {} binary transparency".format(product, branch), completed=["releasetasks"], failed=["releasetasks"], exception=["releasetasks"]) | indent(12) }}

This comment has been minimized.

Copy link
@nthomas-mozilla

nthomas-mozilla Jul 20, 2017

Contributor

Brandon, please update this in the style of #252.

@nthomas-mozilla

This comment has been minimized.

Copy link
Contributor

nthomas-mozilla commented Jul 20, 2017

@MihaiTabara do you have any other concerns ?

Copy link
Contributor

MihaiTabara left a comment

@BrandonTang @nthomas-mozilla
Looks good! 👍 However, not sure what your plans are now.

  1. If you want to land this now but keep the binary transparency job off, and later on follow-up with fixes for the workerType, etc - you're good to go. The only thing left to add is that existence check suggested in releasetasks/templates/desktop/release_graph.yml.tmpl

  2. Otherwise, if you want to turn this on, you might wanna consider a couple more things:

a) add binary_transparency_enabled in the release configs too - that is, all config files related to firefox (not fennec) from https://github.com/mozilla-releng/releasetasks/tree/master/releasetasks/release_configs. If we don't do this, then the graph will break when we'll ship the Release Candidate (tl;dr - when we ship RC, we relay on aforementioned configs, and not ones from bb-configs + tools).

b) add binary_transparency_enabled within https://hg.mozilla.org/build/buildbot-configs/file/tip/mozilla/config.py for all channels we intend to have this turned on. You can see a similar example by looking for uptake_monitoring_enabled.

c) once you have it in b), you need to add within release-runner under tools, in three different places:

c1) in the release runner kwargs https://hg.mozilla.org/build/tools/file/tip/buildfarm/release/release-runner.py#l431

c2) in the (manual generative) release runner too - https://hg.mozilla.org/build/tools/file/tip/buildfarm/release/releasetasks_graph_gen.py#l36

c3) in the sanity kwargs checker - https://hg.mozilla.org/build/tools/file/tip/lib/python/kickoff/__init__.py#l292

Please let me know if this doesn't make sense and I can follow-up with more details.

-
taskId: "{{ stableSlugId(buildername) }}"
requires:
- "{{ stableSlugId('release-foo-{}_chcksms'.format(product)) }}"

This comment has been minimized.

Copy link
@MihaiTabara

MihaiTabara Jul 21, 2017

Contributor

Not sure when you want to turn this on but you might want to change this to "release-{}-{}_chcksms".format(branch, product) to chain it to the real checksums task, when rolling this out in production.

priority: "high"
retries: 5

# Add scopes when using in production

This comment has been minimized.

Copy link
@MihaiTabara

MihaiTabara Jul 21, 2017

Contributor

friendly reminder to add scopes here when rolling this out.

@@ -126,6 +126,13 @@ tasks:
{% include "checksums.yml.tmpl" %}
{% endmacro %}
{{ checksums_tasks()|indent(4) }}

{% if binary_transparency_enabled %}

This comment has been minimized.

Copy link
@MihaiTabara

MihaiTabara Jul 21, 2017

Contributor

Might wanna rewrite this to {% if binary_transparency_enabled is defined and binary_transparency_enabled %}. I'll go into more details in the sum-up comment of the review.

@nthomas-mozilla

This comment has been minimized.

Copy link
Contributor

nthomas-mozilla commented Jul 24, 2017

Thanks for the thorough review Mihai. I'd like to propose we turn on binary transparency only on Firefox desktop betas at first, producing an artifact that we don't publish into firefox/releases/, and still in a staging mode (ie the certs are do not look like production). That way the task doesn't block the push to releases, and doesn't force the pace of the security team's work, while still maintaining steady progress for us. We have on our TODO list to convert to proper scopes (and update the scriptworker) prior to moving out of staging mode with the certificate issuing, but I'm wondering if you'd prefer that was completed before we enabled the task.

@MihaiTabara

This comment has been minimized.

Copy link
Contributor

MihaiTabara commented Jul 26, 2017

@nthomas-mozilla @BrandonTang

Sorry for delays in answering. So sounds like we're very close 👍
To first answer Nick's comment:

i) turning this on for Firefox beta only in yet a "staging mode" sounds good to me. As far as I can tell, we'll be adding a new task that runs after checksums job that creates an artifact. Without beetmover chained to this job, we won't be sending anything to ~candidates directory so there's no way we actually publish anything to ~releases so I think we're safe. And even if we did, the push-candidates-to-releases script has a very comprehensive list of excludes that we can always enrich at https://hg.mozilla.org/mozilla-central/file/tip/testing/mozharness/scripts/release/push-candidate-to-releases.py.

ii) updating later on the scopes & workerType sounds good to me.

iii) if you really want to test this through in proper staging, we can land this on bm83 first and see how it behaves there. We are going to run a staging release anyway these days for tcmigration post-win32/win64 flip in central. But as I said, I think we're good to chain this to production too.

Now - strictly related to this PR, there is only one more thing left to be done:

2fbd309 is good but not enough. I'm afraid we need this variable flipped for all firefox config files from https://github.com/mozilla-releng/releasetasks/tree/master/releasetasks/release_configs. That is:

dev_mozilla-esr45_firefox_rc_graph_2.yml
dev_mozilla-release_firefox_rc_graph_2.yml
prod_mozilla-esr45_firefox_rc_graph_2.yml
prod_mozilla-esr52_firefox_rc_graph_2.yml
prod_mozilla-release_firefox_rc_graph_2.yml

If we don't do this, on 7th of August we'll hit this on esr52 build second-graph we'll manually generate as https://hg.mozilla.org/build/tools/file/tip/buildfarm/release/releasetasks_graph_gen.py will fail for not knowing who binary_transparency_enabled is.

Speaking of which, this reminds me I need to follow-up with a cleanup PR to kill esr45 configs with fire! 😈

Now, suppose we're done with this change as well, the following things need to happen in order:

  1. we land bb-configs patch first since it doesn't have any dependency and it's harmless. https://bug1380829.bmoattachments.org/attachment.cgi?id=8889965 + we do a reconfig.

  2. we land the tools patch from https://bug1380829.bmoattachments.org/attachment.cgi?id=8890083 second to consume the bb-configs patch from 1.

Ideally (but not necessarily needed) - for tools, to avoid any linter errors or whatever alike, you can fork https://github.com/mozilla/build-tools and:

a) either tox and run unit tests locally to make sure they pass
b) create a PR against it and wait for Travis to run those for you. You cannot push/merge your PR to https://github.com/mozilla/build-tools as it's just a mirror or http://hg.mozilla.org/build/tools/. I usually close the PR right away.

  1. we merge this PR and pull latest head on bm85.

We can coordinate for a common timezone ground here if you want these days to do this together.

@MihaiTabara

This comment has been minimized.

Copy link
Contributor

MihaiTabara commented Jul 26, 2017

Make sure to rebase first ;-) I killed esr45 release-configs with fire 😈 in #256

@nthomas-mozilla

This comment has been minimized.

Copy link
Contributor

nthomas-mozilla commented Jul 26, 2017

2fbd309 handles one of the configs but the change to releasetasks_graph_gen.py requires it defined in all configs. You could modify them all, or use a release_config.get(....) in releasetasks_graph_gen.py so that it's not mandatory.

@MihaiTabara

This comment has been minimized.

Copy link
Contributor

MihaiTabara commented Jul 26, 2017

@nthomas-mozilla is right. Either way works; probably it's cleaner to use .get() in releasetasks_graph_gen.py as you did in release-runner.py. If you go down this path, r+ in advance for that tools change, no need to repush patch after refactoring.

@nthomas-mozilla nthomas-mozilla merged commit 546ea6d into mozilla-releng:master Jul 26, 2017
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
4 participants
You can’t perform that action at this time.