917485 - Limit the set of people who can make changes to PuppetAgain

Status: RESOLVED FIXED

(Infrastructure & Operations :: RelOps: Puppet, task)

Description

[Dustin J. Mitchell [:dustin] (he/him)]

Currently, anyone with L2 access can push a change to PuppetAgain.  That's a big bunch of people.

Ideally we'd be able to limit the set to an arbitrary group, LDAP or otherwise.

While we're at it, we'll also want the ability for different orgs to have different repositories and different lists of people who can put changes into production.

I don't have any great ideas of how to do this, and I'm open to suggestions.

Comment 1

[Dustin J. Mitchell [:dustin] (he/him)]

One option may be to have each distinguished master hold an hg repository from which the other masters in the cluster pull, and not automatically update that repository from hg.m.o.  Then deployment would mean landing a change in hg.m.o and running a command on the DM to perform the pull.  We could have some sort of nag email for when the two get out of sync.

Comment 2

[Dustin J. Mitchell [:dustin] (he/him)]

Source-control gods: can we limit access to http://hg.mozilla.org/build/puppet to  cn=releng,ou=groups,dc=mozilla?  Similarly, can we create http://hg.mozilla.org/qa/puppet and limit access to it to cn=qa,ou=groups,dc=mozilla?

This "can we" is both a policy and technical question, so if it's a matter of policy that every hg repo be limited only to a committer level, I will be happy to find another solution.

P.S. Henrik, we'd have to add you and anyone else who wants to push to puppet to the QA group to make the above work, but hopefully that will be OK.

Comment 3

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Dustin J. Mitchell [:dustin] from comment #2)
> P.S. Henrik, we'd have to add you and anyone else who wants to push to
> puppet to the QA group to make the above work, but hopefully that will be OK.

No problem. I think the people who need permissions would be Dave, Rob, myself, and Clint as last fallback.

Comment 4

[Shyam Mani [:fox2mike]]

(In reply to Dustin J. Mitchell [:dustin] from comment #2)

> This "can we" is both a policy and technical question, so if it's a matter
> of policy that every hg repo be limited only to a committer level, I will be
> happy to find another solution.

I think this is the case, but will leave it to fubar and bkero to confirm. I don't think we do per group ACLs, it's all about commit access levels.

Comment 5

[Kendall Libby [:fubar] (he/him)]

I'm pretty sure that as long as it's a posix ldap group it'll work; at the very least we can give it a try!

Comment 6

[Dustin J. Mitchell [:dustin] (he/him)]

I don't think cn=releng,ou=groups,dc=mozilla is such a thing, but with a little help figuring out what that *would* look like, we could easiliy create build-puppet and qa-puppet groups, for example.

Comment 7

[Dustin J. Mitchell [:dustin] (he/him)]

OK, per irc this would be difficult technically, and also a very unusual config.

So, back to the drawing board.  The goals here are twofold:
 1. Allow each org to control timing of deployment of changes separately
 2. Prevent deployment of changes not made by a trusted person (where the set of trusted people is much smaller than SCM level 2).

I think we can achieve both at once.

Comment 8

[Dustin J. Mitchell [:dustin] (he/him)]

Callek, you were going to set up a meeting for this a week ago..

Comment 9

[Dustin J. Mitchell [:dustin] (he/him)]

We met.

Aside from the two goals in comment 7:
 1. Allow each org to control timing of deployment of changes separately
 2. Prevent deployment of changes not made by a trusted person (where the set of trusted people is much smaller than SCM level 2).

We have:
 3. Releng would like to use the default/production branch model used for mozharness and a few other repos, where changesets can land on default at any time, but are only deployed when they are merged to production (usually by buildduty).
 4. Seamonkey would like to automatically follow releng's PuppetAgain, even at the risk of breakage to SeaMonkey automation.

We'll meet #1 by creating a separate hg repo for each org that wishes to remain separate.  SeaMonkey can keep using hgmo/build/puppet, which addresses #4.

With the addition of code on the puppetmasters to set up and pull from a configurable branch, releng can achieve #3 while other orgs can stick with a one-branch repository.

We'll meet #2 with an hg hook that verifies the pusher's identity is on a whitelist, with the hook itself defined in http://hg.mozilla.org/hgcustom/hghooks.  It'd be great if that can talk to LDAP and whitelist group members, but that's not required.

Comment 10

[Dustin J. Mitchell [:dustin] (he/him)]

Done:
 * create a "production" branch on http://hg.mozilla.org/build/puppet
 * configure all masters (moco, relabs, servo, qa, seamonkey) to follow that branch
   (running 'hg up -r production' in /etc/puppet/production - this may not be enough!)
 * emailed tools-puppetagain

To Do:
 * add a hook to limit push access to the production branch
 * add a http://hg.mozilla.org/qa/puppet repository
 * add puppet config to verify which branch a master is running on

Comment 11

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug917485.patch

This defaults to the 'production' branch of build/puppet for everyone, which is probably better than defaulting to 'default'!

Comment 12

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 827459 [details] [diff] [review]
bug917485.patch

>commit 11b43a8e9ea159ef1740721777027d325f13a238
>Author: Dustin J. Mitchell <[e-mail]>
>Date:   Tue Nov 5 11:59:32 2013 -0500
>
>    Bug 917485: enforce repo branch in the update script
>
>diff --git a/modules/config/manifests/base.pp b/modules/config/manifests/base.pp
>index b8e1998..4e11dd4 100644
>--- a/modules/config/manifests/base.pp
>+++ b/modules/config/manifests/base.pp
>@@ -23,16 +23,17 @@ class config::base {
>     $puppetmaster_upstream_rsync_args = ''
>     $puppetmaster_public_mirror_hosts = []
>     $builder_username = 'cltbld'
>     $signer_username = 'cltsign'
>     $nrpe_allowed_hosts = '127.0.0.1'
>     $ntp_server = "pool.ntp.org"
>     $relay_domains = "smtp.mozilla.org"
>     $puppet_again_repo = "http://hg.mozilla.org/build/puppet"
>+    $puppet_again_branch = "production"
>     $puppet_server_reports = "tagmail"
>     $puppet_server_reporturl = "http://localhost:3000/reports/upload"
>     $user_python_repositories = [ "http://pypi.pub.build.mozilla.org/pub" ]
>     $master_json = "https://hg.mozilla.org/build/tools/raw-file/default/buildfarm/maintenance/production-masters.json"
>     $buildbot_tools_hg_repo = "https://hg.mozilla.org/build/tools"
>     $buildbot_configs_hg_repo = "https://hg.mozilla.org/build/buildbot-configs"
>     $buildbot_configs_branch = "production"
>     $buildbotcustom_branch = "production-0.8"
>diff --git a/modules/puppetmaster/templates/update.sh.erb b/modules/puppetmaster/templates/update.sh.erb
>index 63e98c2..50cc62e 100755
>--- a/modules/puppetmaster/templates/update.sh.erb
>+++ b/modules/puppetmaster/templates/update.sh.erb
>@@ -2,16 +2,17 @@
> 
> # This Source Code Form is subject to the terms of the Mozilla Public
> # License, v. 2.0. If a copy of the MPL was not distributed with this
> # file, You can obtain one at http://mozilla.org/MPL/2.0/. 
> 
> errormailto="<%= scope.lookupvar('::config::puppet_notif_email') %>"
> changemailto="<%= scope.lookupvar('::config::puppet_notif_email') %>"
> repo="<%= scope.lookupvar('::config::puppet_again_repo') %>"
>+branch="<%= scope.lookupvar('::config::puppet_again_branch') %>"
> repodir="/etc/puppet/production"
> 
> hostname=<%= scope.lookupvar('::fqdn') %>
> cd $repodir
> 
> # check for uncommitted changes
> output=`/usr/bin/hg stat`
> if [ -n "$output" ]; then
>@@ -20,28 +21,29 @@ if [ -n "$output" ]; then
>         echo ''
>         echo $output
>     ) | mail -s "[PuppetAgain Errors] Uncommitted local changes in $hostname:$repodir" $errormailto
>     exit 1
> fi
> 
> try_update() {
>     rev_before=`/usr/bin/hg ident -i`
>-    rev_current=`/usr/bin/hg ident -i $repo`
>+    rev_current=`/usr/bin/hg ident -i $repo -r $branch`
>     if [ $? -ne 0 ] || [ -z "$rev_current" ]; then
>         return 1
>     fi
> 
>     if [ $rev_before == $rev_current ]; then
>         # nothing to do
>         return 0
>     fi
> 
>     # update (and pass on the exit status)
>-    /usr/bin/hg pull -u $repo
>+    # (hg pull can't do -u and -r at the same time)
>+    /usr/bin/hg pull $repo && /usr/bin/hg up -r $branch
> }
> 
> 
> # retry that five times
> LOGFILE=$(mktemp)
> ok=false
> for try in {1..5}; do
>     if try_update >> $LOGFILE 2>&1; then

Comment 13

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug917485-releng-whitelist.patch

A first stab at the hook.  This enforces the whitelist if any of the changesets in the push are to the production branch.

I don't really want to try to hook this up to LDAP - it just seems too complex and involved.  But if you have alternative suggestions for where to keep the whitelist, that'd be great.  One idea would be to keep it in a file in the production branch, but I'm not sure how I'd get access to that file from the hook.  Pointers appreciated.

Comment 14

[Justin Wood (:Callek)]

Comment on attachment 828245 [details] [diff] [review]
bug917485-releng-whitelist.patch

AIUI we need full LDAP here, and please include jwood@m.c *and* Callek@gmail.c for said LDAP (would be useful to see user of people as they are in the pushlog)

Also, I suspect gps may also be willing to give hook-writing-insight

Comment 15

[Gregory Szorc [:gps]]

Comment on attachment 828245 [details] [diff] [review]
bug917485-releng-whitelist.patch

Review of attachment 828245 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozhghooks/whitelist_releng.py
@@ +16,5 @@
> +# along with this program; if not, write to the Free Software
> +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA  02110-1301, USA.
> +
> +# This script implements a Mercurial hook to log the date and time when
> +# changesets are pushed into a repository, and the user that pushed them.

No it doesn't.

@@ +23,5 @@
> +# and then modify the repository's hgrc as per example-hgrc.
> +
> +import os
> +
> +ALLOWED_USERS = [

It is faster to compare membership against sets instead of lists. Likely doesn't matter for something this small. Just keep it in mind.

@@ +49,5 @@
> +    'raliiev',
> +    'sbruno',
> +]
> +
> +def hook(ui, repo, node, **kwargs):

node should be a keyword argument, not positional.

@@ +52,5 @@
> +
> +def hook(ui, repo, node, **kwargs):
> +    rev = repo.changectx(node).rev()
> +    tip = repo.changectx('tip').rev()
> +    branches = set(repo.changectx(i).branch() for i in range(rev, tip+1))

repo[x] is the same as repo.changectx(x).

You can also iterate over changesets by doing range(rev, len(repo)). There is a lock on the repo while this is running, so len() won't change. You may need a +1 or -1 in there (I can't remember boundary conditions!). If you are up for some light magic, you should also be able to do |for i in repo.revs('%s::tip' % node)|.

Comment 16

[Ed Morley [:emorley]]

Comment on attachment 828245 [details] [diff] [review]
bug917485-releng-whitelist.patch

Redirecting review to someone from releng :-)

Comment 17

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 828245 [details] [diff] [review]
bug917485-releng-whitelist.patch

Ed, my r? was based on a skim of the hg log, looking for someone who knew about hooks - gps seems to fit that bill :)

Gps, thanks for the comments - I'll whip up a new version.

Comment 18

[Ed Morley [:emorley]]

(In reply to Dustin J. Mitchell [:dustin] (I read my bugmail; don't needinfo me) from comment #17)
> Ed, my r? was based on a skim of the hg log, looking for someone who knew
> about hooks - gps seems to fit that bill :)

Ah makes sense! I only pretend to know what I am doing wrt hg hooks, gps or ted are much more knowledgeable in those areas :-)

Comment 19

[Kendall Libby [:fubar] (he/him)]

Comment on attachment 828245 [details] [diff] [review]
bug917485-releng-whitelist.patch

I will defer to :gps on hook writing, but I see nothing that makes me sad. I think USER will be username@domain, though.

As far as LDAP goes, it would certainly be possible but the larger issue is managing that list which, afaik, would probably require tweaking the ldap manage code. If we were to use this elsewhere it would probably make sense, but for a small list of users|repos, this should be fine.

Comment 20

[Dustin J. Mitchell [:dustin] (he/him)]

Attachment: bug917485-releng-whitelist-p2.patch

carrying forward the f+

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8336830 [details] [diff] [review]
bug917485-releng-whitelist-p2.patch

Review of attachment 8336830 [details] [diff] [review]:
-----------------------------------------------------------------

::: mozhghooks/whitelist_releng.py
@@ +51,5 @@
> +
> +def hook(ui, repo, node=None, **kwargs):
> +    rev = repo[node].rev()
> +    tip = repo['tip'].rev()
> +    branches = set(repo.changectx(i).branch() for i in range(rev, tip+1))

You could also write this as:

branches = set(repo[i].branch() for i in repo.changelog.revs(repo[node].rev())

Also, len(repo) returns something you may find useful.

Comment 22

[Dustin J. Mitchell [:dustin] (he/him)]

Comment on attachment 8336830 [details] [diff] [review]
bug917485-releng-whitelist-p2.patch

I'll make a followup to correct my LDAP name (dmitchell) and to add Callek's gmail as requested above.

Comment 23

[Dustin J. Mitchell [:dustin] (he/him)]

And it's tested successfully - only folks on the whitelist can merge to prod.