1445684 - test bouncer alias' after updating them

Status: VERIFIED FIXED

(Release Engineering :: Release Automation: Other, enhancement, P1)

Description

[[e-mail] (:bhearsum)]

In bug 1445672 we updated the wrong bouncer alias' when shipping Firefox 60.0b3. We changed the release channel alias' instead of the Beta ones.

Catlee points out that we should actually verify bouncer alias changes when we make them. This will need to go in the ship phase, and depend on the bouncer alias' task

Comment 1

[Mihai Tabara [:mtabara]⌚️GMT]

I can work on this. I suppose we can add a release-bouncer-aliases-verification in-tree task and have the same bouncerworker test? I'll sketch some idea soon.

Comment 2

[Mihai Tabara [:mtabara]⌚️GMT]

mtabara> bhearsum: moving the conversationa about bouncer tests to avoid disrupting a.ki and jlore.nzo talking about in-tree fix
17:25:54 <mtabara> re: 1445684 
17:26:00 <mtabara> should that be a separate task altogether? 
17:26:08 <mtabara> like release-bouncer-aliases-verification
17:26:34 <mtabara> kind of like the same in-tree definition + a new behavior in bouncerscript
17:26:36 <aki> either we'd have to have hardcodes somewhere, or the test's task definition would also be passed in through the graph, no?
17:27:00 <aki> if we have tests in bouncerscript, it could have checks before it submits the aliases
17:27:04 <aki> and it would be the same task
17:27:14 <mtabara> yeah, that's what I was thinking too
17:27:23 <aki> :) gmta
17:27:24 <mtabara> not sure why we need a follow-up task, since we can do it all in the same task
17:27:36 <mtabara> but I don't get one thjing
17:27:44 <aki> ?
17:28:06 <mtabara> what aliases we'd check? all of them basically? making sure that the one we're adding (product X) is not among any of the other aliases except the current payload
17:28:07 <mtabara> ?
17:28:18 <aki> i think so
17:28:33 <aki> all of them. somehow we'd have to know that beta products shouldn't go in non-beta aliases
17:28:53 <aki> probably esr too
17:30:57 <mtabara> should we hardcode that list in bouncerscript constants or have it in-tree and pass it along bouncer aliases task definition?
17:31:46 <bhearsum> not sure if it's useful to us, but mbrandt pointed me at the tests that caught this issue: https://github.com/mozilla-services/go-bouncer/tree/master/tests/e2e
17:33:08 <aki> our current approach is hardcoding in bouncerscript or the puppet config template. if we passed it in from in-tree, i'd want the rules to be in a flat file rather than dynamically created, or we could make the test go false-green with the same bad in-tree patch that changes the task
17:33:51 <aki> i suppose it would still be subject to transforms, so we could still break the test in-tree
17:34:06 <aki> so i'm leaning towards puppet config template
17:34:14 <mtabara> hm, I like the puppet config template idea
17:34:26 <mtabara> the aliases are not changing as often, do they? 
17:34:36 <aki> not sure
17:34:49 <mtabara> except "sha1" cases and dawn, not sure the set suffered any modifications
17:35:24 <aki> i'm leaning towards regexes. someone should probably stop me
17:35:34 <mtabara> bhearsum: ah, thanks for poiting me out to that. I'll glance at that and come up with some approach in the bug. so far I'm leaning towards puppet config templates
17:35:35 <mtabara> hahah
17:35:45 <mtabara> come on, it's regexes, what can go wrong? 
17:35:52 <mtabara> (TM)
17:36:38 <aki> it could be something like "firefox-latest-ssl": "release", "firefox-latest-beta-ssl": "beta", and something on the other end makes sure that the product looks like a release or beta product
17:37:09 <aki> up to us how we behave if there's a new alias. we can either inspect it to see if it looks like esr or beta, or we can break
17:37:30 <aki> the downside of the former is the test may be wrong. the downside of the latter is manually fixing up the bouncer aliases
17:37:38 <aki> i suppose puppet patch and rerun
17:39:00 <aki> i suppose "firefox-latest-ssl": "Firefox-[\d.]+-SSL" isn't so bad
17:39:11 <aki> assuming that regex is right =\
17:42:16 <aki> i wonder if `"latest-beta": "[\d.]+b\d+"` would work. we'd have to order the string replacement, and then compare the alias versus the string-replaced product.lower()
17:42:18 <mtabara> puppet patch + rerun or manual is far better than in-tree patch anyway
17:42:32 <mtabara> ino
17:42:37 <mtabara> imo
New messages since you tabbed out
17:42:50 <aki> yeah

Comment 3

[Mihai Tabara [:mtabara]⌚️GMT]

Attachment: Add preflight checks for bouncer aliases job

* add check for bouncer aliases job (via puppet config)
* some simplifying in the tests

Comment 4

[Mihai Tabara [:mtabara]⌚️GMT]

https://tools.taskcluster.net/groups/RVZZfhFbQNOIi2BM2DoOTA/tasks/VB7Bq0HfTrKzLsp1FV5ICQ/details was the initial bouncer aliases job that was green but wrongly updated the aliases in staging. It fails now in sanity check 
See more https://tools.taskcluster.net/groups/RVZZfhFbQNOIi2BM2DoOTA/tasks/WKW4OvfAQTuIa0D1pOnY0w/runs/0/logs/public%2Flogs%2Flive_backing.log
automation >> human attention span. automation FTW!

Comment 5

[Mihai Tabara [:mtabara]⌚️GMT]

Attachment: Bug 1445684 - add bouncer aliases preflight check regexes.

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

Comment 6

[Aki Sasaki (not active)]

(In reply to Mihai Tabara [:mtabara]⌚️GMT from comment #4)
> https://tools.taskcluster.net/groups/RVZZfhFbQNOIi2BM2DoOTA/tasks/
> VB7Bq0HfTrKzLsp1FV5ICQ/details was the initial bouncer aliases job that was
> green but wrongly updated the aliases in staging. It fails now in sanity
> check 
> See more
> https://tools.taskcluster.net/groups/RVZZfhFbQNOIi2BM2DoOTA/tasks/
> WKW4OvfAQTuIa0D1pOnY0w/runs/0/logs/public%2Flogs%2Flive_backing.log
> automation >> human attention span. automation FTW!

\o/

Comment 7

[Aki Sasaki (not active)]

Comment on attachment 8960808 [details]
Bug 1445684 - add bouncer aliases preflight check regexes.

https://reviewboard.mozilla.org/r/229536/#review235286

::: modules/bouncer_scriptworker/manifests/settings.pp:107
(Diff revision 1)
>              submission => "${root}/lib/python3.5/site-packages/bouncerscript/data/bouncer_submission_task_schema.json",
>              aliases    => "${root}/lib/python3.5/site-packages/bouncerscript/data/bouncer_aliases_task_schema.json",
>          },
>          verbose            => $verbose_logging,
>          bouncer_config     => $_env_config['bouncer_instances'],
> +        aliases_regexes    => $all_regexes

We probably need to make sure this creates the file properly (with appropriate escaping).

Thanks!

Comment 8

[Mihai Tabara [:mtabara]⌚️GMT]

(In reply to Aki Sasaki [:aki] from comment #7)
> Comment on attachment 8960808 [details]
> Bug 1445684 - add bouncer aliases preflight check regexes.
> 
> https://reviewboard.mozilla.org/r/229536/#review235286
> 
> ::: modules/bouncer_scriptworker/manifests/settings.pp:107
> (Diff revision 1)
> >              submission => "${root}/lib/python3.5/site-packages/bouncerscript/data/bouncer_submission_task_schema.json",
> >              aliases    => "${root}/lib/python3.5/site-packages/bouncerscript/data/bouncer_aliases_task_schema.json",
> >          },
> >          verbose            => $verbose_logging,
> >          bouncer_config     => $_env_config['bouncer_instances'],
> > +        aliases_regexes    => $all_regexes
> 
> We probably need to make sure this creates the file properly (with
> appropriate escaping).
> 
> Thanks!

I initially added the strings as they were in the bouncerscript PR[1] but then I noticed puppet was double-escaping everything "\\\\d". So I switched back to the initial form of them, seems like `pp` files are doing these for us when they transition the content of the `script_config.json`.

[1]: https://github.com/mozilla-releng/bouncerscript/pull/16/files#diff-3d6e28e1693286ec7fe31e724b9b9cf0R19

Comment 9

[Mihai Tabara [:mtabara]⌚️GMT]

Works smoothly in puppet, I pinned the bouncer-dev to my environment and tested both the puppet patch + bouncerscript and work good. I'll try to successfully ship a staging release tomorrow to make sure things work properly and then I'll roll-out to production.

Comment 10

[Aki Sasaki (not active)]

Awesome, thanks you! Let me know if you need a hand.

Comment 11

[Johan Lorenzo [:jlorenzo]]

Comment on attachment 8960725 [details] [review]
Add preflight checks for bouncer aliases job

Thank you for these checks. It's great to be safeguarded by them! I think we can make the overall config less error-prone. I left more details at: https://github.com/mozilla-releng/bouncerscript/pull/16#pullrequestreview-105654320

Comment 12

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8960725 [details] [review]
Add preflight checks for bouncer aliases job

https://github.com/mozilla-releng/bouncerscript/commit/4c78f3ff13d679b562efeeb707bbe29b7f20f4cd

Rolled-out 1.2.1 to master branch.

Comment 13

[Mihai Tabara [:mtabara]⌚️GMT]

Comment on attachment 8960808 [details]
Bug 1445684 - add bouncer aliases preflight check regexes.

Trimmed the regexes part and kept solely the version bump (1.2.1)

https://hg.mozilla.org/build/puppet/rev/9dee0cf03e462b34c91382f51092cc86a9d98812

Comment 14

[Mihai Tabara [:mtabara]⌚️GMT]

Rolled-out 1.2.1 to production and dev bouncer workers. This should increase our checks and security for upcoming bouncer aliases jobs. 

Closing this now.

Comment 15

[Mihai Tabara [:mtabara]⌚️GMT]

https://taskcluster-artifacts.net/OgCFskaKSaGl5BwnJTfBBA/0/public/logs/live_backing.log
https://taskcluster-artifacts.net/Pu2JDGByTLKihDXwwOK0eA/0/public/logs/live_backing.log