yellow team mailing list archive

[Gary Poster <gary.poster@xxxxxxxxxxxxx>]

Gary Poster has proposed merging lp:~gary/charms/oneiric/buildbot-master/run-as-buildbot into lp:~yellow/charms/oneiric/buildbot-master/trunk.

Requested reviews:
  Launchpad Yellow Squad (yellow)

For more details, see:
https://code.launchpad.net/~gary/charms/oneiric/buildbot-master/run-as-buildbot/+merge/92399

This branch makes the buildbot master run as the buildbot user.  This necessarily changes some directories, which is all for the better IMO, at least in terms of matching Debian's expectations.

This also sets up helper functions for the buildslave.  I'm not really in love with the buildbot/buildslave OSError try except dances: please feel free to encourage me or give me another approach.
-- 
https://code.launchpad.net/~gary/charms/oneiric/buildbot-master/run-as-buildbot/+merge/92399
Your team Launchpad Yellow Squad is requested to review the proposed merge of lp:~gary/charms/oneiric/buildbot-master/run-as-buildbot into lp:~yellow/charms/oneiric/buildbot-master/trunk.

=== modified file 'config.yaml'
--- config.yaml	2012-02-09 14:33:51 +0000
+++ config.yaml	2012-02-10 01:15:26 +0000
@@ -10,7 +10,7 @@
     description: |
       The directory where the Buildbot master will be installed.
     type: string
-    default: /tmp/buildbot
+    default: /var/lib/buildbot/masters/master
   config-file:
     description: |
         An encoded master.cfg file.  Use of this configuration is

=== modified file 'examples/lpbuildbot.yaml'
--- examples/lpbuildbot.yaml	2012-02-02 19:38:58 +0000
+++ examples/lpbuildbot.yaml	2012-02-10 01:15:26 +0000
@@ -3,5 +3,3 @@
   config-url: http://bazaar.launchpad.net/~launchpad/lpbuildbot/public
   extra-repository: deb http://us.archive.ubuntu.com/ubuntu/ lucid main universe
   buildbot-pkg: buildbot/lucid
-  installdir: /var/lib/buildbot/masters/lpbuildbot
-

=== modified file 'hooks/buildbot-relation-changed'
--- hooks/buildbot-relation-changed	2012-02-08 20:23:41 +0000
+++ hooks/buildbot-relation-changed	2012-02-10 01:15:26 +0000
@@ -11,6 +11,7 @@
     log_exit,
     relation_get,
     relation_set,
+    su,
     )
 from local import (
     buildbot_reconfig,
@@ -30,9 +31,10 @@
 
 
 def update_slave_json(name, passwd, builders):
-    slave_info = slave_json.get()
-    slave_info[name] = (passwd, builders)
-    slave_json.set(slave_info)
+    with su('buildbot'):
+        slave_info = slave_json.get()
+        slave_info[name] = (passwd, builders)
+        slave_json.set(slave_info)
 
 
 def main():

=== modified file 'hooks/config-changed'
--- hooks/config-changed	2012-02-08 15:58:32 +0000
+++ hooks/config-changed	2012-02-10 01:15:26 +0000
@@ -16,11 +16,13 @@
     command,
     DictDiffer,
     get_config,
+    get_user_ids,
     install_extra_repository,
     log,
     log_entry,
     log_exit,
     run,
+    su,
     )
 from local import (
     buildbot_create,
@@ -82,7 +84,7 @@
 
 
 def configure_buildbot(config, diff):
-    buildbot_dir =  config.get('installdir')
+    buildbot_dir = config.get('installdir')
     config_file = config.get('config-file')
     master_cfg_path = os.path.join(buildbot_dir, 'master.cfg')
     config_transport = config.get('config-transport')
@@ -97,7 +99,7 @@
         buildbot_create(buildbot_dir)
         # This file will be moved to master.cfg.original in
         # initialize_buildbot().
-        with open(master_cfg_path, 'w') as f:
+        with su('buildbot'), open(master_cfg_path, 'w') as f:
             base64.decode(StringIO.StringIO(config_file), f)
         log('config_file decoded and written.')
         restart_required = True
@@ -107,20 +109,29 @@
         # If the branch is private then more setup needs to be done.  The
         # gpg-agent needs to send the key over and the bzr launchpad-login
         # needs to be set.
-        lp_id = config.get('config-user')
-        if lp_id:
-            bzr('launchpad-login', lp_id)
-
-        private_key = config.get('config-private-key')
-        if private_key:
-            # Set up the .ssh directory.
-            ssh_dir = os.expanduser('~/.ssh')
-            os.mkdir(ssh_dir)
-            os.chmod(ssh_dir, 0700)
-            with open(os.path.join(ssh_dir, 'lp_key', 'w')) as f:
-                f.write(base64.decode(private_key))
-        bzr('branch', '--use-existing-dir', config_url, buildbot_dir)
+        with su('buildbot'):
+            lp_id = config.get('config-usr')
+            if lp_id:
+                bzr('launchpad-login', lp_id)
+            private_key = config.get('config-private-key')
+            if private_key:
+                # Set up the .ssh directory.
+                ssh_dir = os.expanduser('~/.ssh')
+                os.mkdir(ssh_dir)
+                os.chmod(ssh_dir, 0700)
+                with open(os.path.join(ssh_dir, 'lp_key', 'w')) as f:
+                    f.write(base64.decode(private_key))
+        # bzr branch does not work well with seteuid (so the "su"
+        # contextmanager won't work).  We need to use a real su.
+        # Make buildbot user have a shell by editing /etc/passwd.
+        # Otherwise you cannot act as this user in the su.
+        run('usermod', '-s', '/bin/sh', 'buildbot')
+        run('su', '-', 'buildbot', '-c',
+            'bzr branch --use-existing-dir {} "{}"'.format(
+                config_url, buildbot_dir))
         log('configuration fetched from {}'.format(config_url))
+        # This should generally only create the necessary .tac file.
+        buildbot_create(buildbot_dir)
         restart_required = True
     return restart_required
 
@@ -132,15 +143,18 @@
     master_cfg_path = os.path.join(buildbot_dir, 'master.cfg')
     shutil.move(master_cfg_path, master_cfg_path + '.original')
     shutil.copy(
-        os.path.join(os.path.dirname(__file__), 'master.cfg'), master_cfg_path)
-    # Initialise the slave JSON if it doesn't exist.
-    if not slave_json.exists():
-        slave_json.set({})
-    placeholder_path = os.path.join(buildbot_dir, 'placeholder.json')
-    if not os.path.exists(placeholder_path):
-        with open(placeholder_path, 'w') as f:
-            json.dump(generate_string("temporary-placeholder-"), f)
-    run('chown', '-R', 'ubuntu:ubuntu', buildbot_dir)
+        os.path.join(os.path.dirname(__file__), 'master.cfg'),
+        master_cfg_path)
+    uid, gid = get_user_ids('buildbot')
+    os.chown(master_cfg_path, uid, gid)
+    with su('buildbot'):
+        # Initialise the slave JSON if it doesn't exist.
+        if not slave_json.exists():
+            slave_json.set({})
+        placeholder_path = os.path.join(buildbot_dir, 'placeholder.json')
+        if not os.path.exists(placeholder_path):
+            with open(placeholder_path, 'w') as f:
+                json.dump(generate_string("temporary-placeholder-"), f)
     buildbot_reconfig()
 
 
@@ -157,12 +171,14 @@
         log("No configuration changes, exiting.")
         sys.exit(0)
 
+    restart_required = handle_config_changes(config, diff)
+
     # Ensure the install directory exists.
     buildbot_dir = config.get('installdir')
     if not os.path.exists(buildbot_dir):
-        os.makedirs(buildbot_dir)
+        with su('buildbot'):
+            os.makedirs(buildbot_dir)
 
-    restart_required = handle_config_changes(config, diff)
     restart_required |= configure_buildbot(config, diff)
 
     master_cfg_path = os.path.join(buildbot_dir, 'master.cfg')

=== modified file 'hooks/helpers.py'
--- hooks/helpers.py	2012-02-09 15:48:36 +0000
+++ hooks/helpers.py	2012-02-10 01:15:26 +0000
@@ -6,9 +6,12 @@
 __metaclass__ = type
 __all__ = [
     'apt_get_install',
+    'cd',
     'command',
     'DictDiffer',
     'get_config',
+    'get_user_ids',
+    'get_user_home',
     'get_value_from_line',
     'grep',
     'install_extra_repository',
@@ -18,21 +21,28 @@
     'run',
     'relation_get',
     'relation_set',
+    'su',
     'unit_info',
     ]
 
-from textwrap import dedent
 import base64
+from collections import namedtuple
+from contextlib import contextmanager
 import json
 import os
+import pwd
 import re
 import subprocess
 import sys
+from textwrap import dedent
 import time
 import urllib2
 import yaml
 
 
+Env = namedtuple('Env', 'uid gid home')
+
+
 def run(*args):
     """Run the command with the given arguments.
 
@@ -277,3 +287,55 @@
     def set(self, data):
         with open(self.path, 'w') as f:
             self.serialize(data, f)
+
+
+@contextmanager
+def cd(directory):
+    """A context manager to temporary change current working dir, e.g.::
+
+        >>> import os
+        >>> os.chdir('/tmp')
+        >>> with cd('/bin'): print os.getcwd()
+        /bin
+        >>> os.getcwd()
+        '/tmp'
+    """
+    cwd = os.getcwd()
+    os.chdir(directory)
+    yield
+    os.chdir(cwd)
+
+
+def get_user_ids(user):
+    """Return the uid and gid of given `user`, e.g.::
+
+        >>> get_user_ids('root')
+        (0, 0)
+    """
+    userdata = pwd.getpwnam(user)
+    return userdata.pw_uid, userdata.pw_gid
+
+
+def get_user_home(user):
+    """Return the home directory of the given `user`.
+
+        >>> get_user_home('root')
+        '/root'
+    """
+    return pwd.getpwnam(user).pw_dir
+
+
+@contextmanager
+def su(user):
+    """A context manager to temporary run the script as a different user."""
+    uid, gid = get_user_ids(user)
+    os.setegid(gid)
+    os.seteuid(uid)
+    current_home = os.getenv('HOME')
+    home = get_user_home(user)
+    os.environ['HOME'] = home
+    yield Env(uid, gid, home)
+    os.setegid(os.getgid())
+    os.seteuid(os.getuid())
+    if current_home is not None:
+        os.environ['HOME'] = current_home

=== modified file 'hooks/local.py'
--- hooks/local.py	2012-02-08 14:26:58 +0000
+++ hooks/local.py	2012-02-10 01:15:26 +0000
@@ -24,6 +24,7 @@
     log,
     run,
     Serializer,
+    su,
     )
 
 
@@ -43,7 +44,8 @@
 def buildbot_create(buildbot_dir):
     """Create a buildbot instance in `buildbot_dir`."""
     if not os.path.exists(os.path.join(buildbot_dir, 'buildbot.tac')):
-        return run('buildbot', 'create-master', buildbot_dir)
+        with su('buildbot'):
+            return run('buildbot', 'create-master', buildbot_dir)
 
 
 def buildbot_running(buildbot_dir):
@@ -63,8 +65,10 @@
     buildbot_dir = _get_buildbot_dir()
     if buildbot_running(buildbot_dir):
         # Buildbot is running, stop it.
-        log('Stopping buildbot')
-        run('buildbot', 'stop', buildbot_dir)
+        log('--> Stopping buildbot')
+        with su('buildbot'):
+            run('buildbot', 'stop', buildbot_dir)
+        log('<-- Stopping buildbot')
 
 
 def buildbot_reconfig():
@@ -82,19 +86,19 @@
         else:
             # Buildbot is running, reconfigure it.
             log('--> Reconfiguring buildbot')
-            # XXX as root! :-(
-            # reconfig is broken in 0.8.3 (Oneiric)
-            # run('buildbot', 'reconfig', buildbot_dir)
-            run('buildbot', 'stop', buildbot_dir)
-            run('buildbot', 'start', buildbot_dir)
+            with su('buildbot'):
+                # Reconfig is broken in 0.8.3 (Oneiric), so we can't do this:
+                # run('buildbot', 'reconfig', buildbot_dir)
+                run('buildbot', 'stop', buildbot_dir)
+                run('buildbot', 'start', buildbot_dir)
             log('<-- Reconfiguring buildbot')
             running = True
     if not running:
         # Buildbot isn't running so start afresh.
         if os.path.exists(os.path.join(buildbot_dir, 'master.cfg')):
             log('--> Starting buildbot')
-            # XXX as root! :-(
-            run('buildbot', 'start', buildbot_dir)
+            with su('buildbot'):
+                run('buildbot', 'start', buildbot_dir)
             log('<-- Starting buildbot')
 
 
@@ -105,7 +109,14 @@
 def buildslave_stop(buildbot_dir=None):
     if buildbot_dir is None:
         buildbot_dir = _get_buildbot_dir()
-    exit_code = subprocess.call(['buildslave', 'stop', buildbot_dir])
+    with su('buildbot'):
+        # In recent versions of buildbot, it provides a "buildslave" binary.
+        # Earlier versions have the "buildbot" binary only, providing the
+        # same behavior.
+        try:
+            exit_code = subprocess.call(['buildslave', 'stop', buildbot_dir])
+        except OSError:
+            exit_code = subprocess.call(['buildbot', 'stop', buildbot_dir])
     tac_file = _get_tac_filename(buildbot_dir)
     if os.path.exists(tac_file):
         os.remove(tac_file)
@@ -115,16 +126,33 @@
 def buildslave_start(buildbot_dir=None):
     if buildbot_dir is None:
         buildbot_dir = _get_buildbot_dir()
-    return subprocess.call(['buildslave', 'start', buildbot_dir])
+    with su('buildbot'):
+        # In recent versions of buildbot, it provides a "buildslave" binary.
+        # Earlier versions have the "buildbot" binary only, providing the
+        # same behavior.
+        try:
+            return subprocess.call(['buildslave', 'start', buildbot_dir])
+        except OSError:
+            return subprocess.call(['buildbot', 'start', buildbot_dir])
 
 
 def create_slave(name, passwd, host='localhost', buildbot_dir=None):
     if buildbot_dir is None:
         buildbot_dir = _get_buildbot_dir()
-    if not os.path.exists(buildbot_dir):
-        os.makedirs(buildbot_dir)
-    return subprocess.call([
-        'buildslave', 'create-slave', buildbot_dir, host, name, passwd])
+    with su('buildbot'):
+        if not os.path.exists(buildbot_dir):
+            os.makedirs(buildbot_dir)
+        # In recent versions of buildbot, it provides a "buildslave" binary.
+        # Earlier versions have the "buildbot" binary only, providing the
+        # same behavior.
+        try:
+            return subprocess.call(
+                ['buildslave',
+                 'create-slave', buildbot_dir, host, name, passwd])
+        except OSError:
+            return subprocess.call(
+                ['buildbot',
+                 'create-slave', buildbot_dir, host + ":9989", name, passwd])
 
 
 slave_json = Serializer('/tmp/slave_info.json')