Run playbooks with only those roles defined thus far

So that a job lower in the inheritance hierarchy does not alter
the behavior of playbooks defined higher in the hierarchy, run
each playbook with only the roles that were present on the job
at the point in the inheritance hierarchy that playbook was
defined.

Change-Id: I06f4aff5340f48a09dae2cd95180531fa572b85e

doc/source/user/config.rst

@@ -697,8 +697,19 @@ unless otherwise specified:
   roles: a Galaxy role, which is simply a role that is installed from
   Ansible Galaxy, or a Zuul role, which is a role provided by a
   project managed by Zuul.  Zuul roles are able to benefit from
-  speculative merging and cross-project dependencies when used by jobs
+  speculative merging and cross-project dependencies when used by
-  in untrusted projects.
+  playbooks in untrusted projects.
+
+  In the case of job inheritance or variance, the roles used for each
+  of the playbooks run by the job will be only those which were
+  defined along with that playbook.  If a child job inherits from a
+  parent which defines a pre and post playbook, then the pre and post
+  playbooks it inherits from the parent job will run only with the
+  roles that were defined on the parent.  If the child adds its own
+  pre and post playbooks, then any roles added by the child will be
+  available to the child's playbooks.  This is so that a job which
+  inherits from a parent does not inadvertantly alter the behavior of
+  the parent's playbooks by the addition of conflicting roles.
 
   A project which supplies a role may be structured in one of two
   configurations: a bare role (in which the role exists at the root of

tests/base.py

@@ -1359,12 +1359,12 @@ class RecordingAnsibleJob(zuul.executor.server.AnsibleJob):
         self.recordResult(result)
         return result
 
-    def runAnsible(self, cmd, timeout, trusted=False):
+    def runAnsible(self, cmd, timeout, config_file, trusted):
         build = self.executor_server.job_builds[self.job.unique]
 
         if self.executor_server._run_ansible:
             result = super(RecordingAnsibleJob, self).runAnsible(
-                cmd, timeout, trusted=trusted)
+                cmd, timeout, config_file, trusted)
         else:
             result = build.run()
         return result

tests/unit/test_model.py

@@ -95,9 +95,9 @@ class TestJob(BaseTestCase):
     def test_job_inheritance(self):
         # This is standard job inheritance.
 
-        base_pre = model.PlaybookContext(self.context, 'base-pre')
+        base_pre = model.PlaybookContext(self.context, 'base-pre', [])
-        base_run = model.PlaybookContext(self.context, 'base-run')
+        base_run = model.PlaybookContext(self.context, 'base-run', [])
-        base_post = model.PlaybookContext(self.context, 'base-post')
+        base_post = model.PlaybookContext(self.context, 'base-post', [])
 
         base = model.Job('base')
         base.timeout = 30
@@ -121,9 +121,9 @@ class TestJob(BaseTestCase):
     def test_job_variants(self):
         # This simulates freezing a job.
 
-        py27_pre = model.PlaybookContext(self.context, 'py27-pre')
+        py27_pre = model.PlaybookContext(self.context, 'py27-pre', [])
-        py27_run = model.PlaybookContext(self.context, 'py27-run')
+        py27_run = model.PlaybookContext(self.context, 'py27-run', [])
-        py27_post = model.PlaybookContext(self.context, 'py27-post')
+        py27_post = model.PlaybookContext(self.context, 'py27-post', [])
 
         py27 = model.Job('py27')
         py27.timeout = 30

zuul/configloader.py

@@ -400,21 +400,34 @@ class JobParser(object):
             parent = layout.getJob(conf['parent'])
             job.inheritFrom(parent)
 
+        # Roles are part of the playbook context so we must establish
+        # them earlier than playbooks.
+        if 'roles' in conf:
+            roles = []
+            for role in conf.get('roles', []):
+                if 'zuul' in role:
+                    r = JobParser._makeZuulRole(tenant, job, role)
+                    if r:
+                        roles.append(r)
+            job.addRoles(roles)
+
         for pre_run_name in as_list(conf.get('pre-run')):
             pre_run = model.PlaybookContext(job.source_context,
-                                            pre_run_name)
+                                            pre_run_name, job.roles)
             job.pre_run = job.pre_run + (pre_run,)
         for post_run_name in as_list(conf.get('post-run')):
             post_run = model.PlaybookContext(job.source_context,
-                                             post_run_name)
+                                             post_run_name, job.roles)
             job.post_run = (post_run,) + job.post_run
         if 'run' in conf:
-            run = model.PlaybookContext(job.source_context, conf['run'])
+            run = model.PlaybookContext(job.source_context, conf['run'],
+                                        job.roles)
             job.run = (run,)
         else:
             if not project_pipeline:
                 run_name = os.path.join('playbooks', job.name)
-                run = model.PlaybookContext(job.source_context, run_name)
+                run = model.PlaybookContext(job.source_context, run_name,
+                                            job.roles)
                 job.implied_run = (run,) + job.implied_run
 
         for k in JobParser.simple_attributes:
@@ -460,15 +473,6 @@ class JobParser(object):
 
         job.dependencies = frozenset(as_list(conf.get('dependencies')))
 
-        if 'roles' in conf:
-            roles = []
-            for role in conf.get('roles', []):
-                if 'zuul' in role:
-                    r = JobParser._makeZuulRole(tenant, job, role)
-                    if r:
-                        roles.append(r)
-            job.addRoles(roles)
-
         variables = conf.get('vars', None)
         if variables:
             job.updateVariables(variables)

zuul/executor/client.py

@@ -237,7 +237,6 @@ class ExecutorClient(object):
             params['playbooks'] = [x.toDict() for x in job.run]
             params['pre_playbooks'] = [x.toDict() for x in job.pre_run]
             params['post_playbooks'] = [x.toDict() for x in job.post_run]
-            params['roles'] = [x.toDict() for x in job.roles]
 
         nodeset = item.current_build_set.getJobNodeSet(job.name)
         nodes = []

zuul/executor/server.py

@@ -72,13 +72,6 @@ class Watchdog(object):
         self._running = False
 
 
-class JobDirPlaybook(object):
-    def __init__(self, root):
-        self.root = root
-        self.trusted = None
-        self.path = None
-
-
 class SshAgent(object):
     log = logging.getLogger("zuul.ExecutorServer")
 
@@ -151,6 +144,24 @@ class SshAgent(object):
         return result
 
 
+class JobDirPlaybook(object):
+    def __init__(self, root):
+        self.root = root
+        self.trusted = None
+        self.path = None
+        self.roles = []
+        self.roles_path = []
+        self.ansible_config = os.path.join(self.root, 'ansible.cfg')
+        self.project_link = os.path.join(self.root, 'project')
+
+    def addRole(self):
+        count = len(self.roles)
+        root = os.path.join(self.root, 'role_%i' % (count,))
+        os.makedirs(root)
+        self.roles.append(root)
+        return root
+
+
 class JobDir(object):
     def __init__(self, root, keep, build_uuid):
         '''
@@ -163,11 +174,23 @@ class JobDir(object):
         '''
         # root
         #   ansible
-        #     trusted.cfg
+        #     inventory.yaml
-        #     untrusted.cfg
+        #   playbook_0
+        #     project -> ../trusted/project_0/...
+        #     role_0 -> ../trusted/project_0/...
+        #   trusted
+        #     project_0
+        #       <git.example.com>
+        #         <project>
         #   work
+        #     .ssh
+        #       known_hosts
         #     src
+        #       <git.example.com>
+        #         <project>
         #     logs
+        #       job-output.txt
+        #     results.json
         self.keep = keep
         if root:
             tmpdir = root
@@ -175,16 +198,16 @@ class JobDir(object):
             tmpdir = tempfile.gettempdir()
         self.root = os.path.join(tmpdir, build_uuid)
         os.mkdir(self.root, 0o700)
-        # Work
         self.work_root = os.path.join(self.root, 'work')
         os.makedirs(self.work_root)
         self.src_root = os.path.join(self.work_root, 'src')
         os.makedirs(self.src_root)
         self.log_root = os.path.join(self.work_root, 'logs')
         os.makedirs(self.log_root)
-        # Ansible
         self.ansible_root = os.path.join(self.root, 'ansible')
         os.makedirs(self.ansible_root)
+        self.trusted_root = os.path.join(self.root, 'trusted')
+        os.makedirs(self.trusted_root)
         ssh_dir = os.path.join(self.work_root, '.ssh')
         os.mkdir(ssh_dir, 0o700)
         self.result_data_file = os.path.join(self.work_root, 'results.json')
@@ -196,13 +219,23 @@ class JobDir(object):
         self.playbook = None  # A pointer to the candidate we have chosen
         self.pre_playbooks = []
         self.post_playbooks = []
-        self.roles = []
-        self.trusted_roles_path = []
-        self.untrusted_roles_path = []
-        self.untrusted_config = os.path.join(
-            self.ansible_root, 'untrusted.cfg')
-        self.trusted_config = os.path.join(self.ansible_root, 'trusted.cfg')
         self.job_output_file = os.path.join(self.log_root, 'job-output.txt')
+        self.trusted_projects = []
+        self.trusted_project_index = {}
+
+    def addTrustedProject(self, canonical_name, branch):
+        # Trusted projects are placed in their own directories so that
+        # we can support using different branches of the same project
+        # in different playbooks.
+        count = len(self.trusted_projects)
+        root = os.path.join(self.trusted_root, 'project_%i' % (count,))
+        os.makedirs(root)
+        self.trusted_projects.append(root)
+        self.trusted_project_index[(canonical_name, branch)] = root
+        return root
+
+    def getTrustedProject(self, canonical_name, branch):
+        return self.trusted_project_index.get((canonical_name, branch))
 
     def addPrePlaybook(self):
         count = len(self.pre_playbooks)
@@ -228,17 +261,6 @@ class JobDir(object):
         self.playbooks.append(playbook)
         return playbook
 
-    def addRole(self):
-        count = len(self.roles)
-        root = os.path.join(self.ansible_root, 'role_%i' % (count,))
-        os.makedirs(root)
-        trusted = os.path.join(root, 'trusted')
-        os.makedirs(trusted)
-        untrusted = os.path.join(root, 'untrusted')
-        os.makedirs(untrusted)
-        self.roles.append(root)
-        return root
-
     def cleanup(self):
         if not self.keep:
             shutil.rmtree(self.root)
@@ -760,8 +782,13 @@ class AnsibleJob(object):
             projects.add((project['connection'], project['name']))
 
         # ...as well as all playbook and role projects.
-        repos = (args['pre_playbooks'] + args['playbooks'] +
+        repos = []
-                 args['post_playbooks'] + args['roles'])
+        playbooks = (args['pre_playbooks'] + args['playbooks'] +
+                     args['post_playbooks'])
+        for playbook in playbooks:
+            repos.append(playbook)
+            repos += playbook['roles']
+
         for repo in repos:
             self.log.debug("Job %s: updating playbook or role %s" %
                            (self.job.unique, repo))
@@ -806,12 +833,9 @@ class AnsibleJob(object):
         for repo in repos.values():
             repo.deleteRemote('origin')
 
-        # is the playbook in a repo that we have already prepared?
+        # This prepares each playbook and the roles needed for each.
-        trusted, untrusted = self.preparePlaybookRepos(args)
+        self.preparePlaybooks(args)
 
-        self.prepareRoles(args, trusted, untrusted)
-
-        # TODOv3: Ansible the ansible thing here.
         self.prepareAnsibleFiles(args)
 
         data = {
@@ -997,42 +1021,29 @@ class AnsibleJob(object):
             raise Exception("Unable to find playbook %s" % path)
         return None
 
-    def preparePlaybookRepos(self, args):
+    def preparePlaybooks(self, args):
-        trusted = untrusted = False
         for playbook in args['pre_playbooks']:
             jobdir_playbook = self.jobdir.addPrePlaybook()
-            self.preparePlaybookRepo(jobdir_playbook, playbook,
+            self.preparePlaybook(jobdir_playbook, playbook,
-                                     args, required=True)
+                                 args, required=True)
-            if playbook['trusted']:
-                trusted = True
-            else:
-                untrusted = True
 
         for playbook in args['playbooks']:
             jobdir_playbook = self.jobdir.addPlaybook()
-            self.preparePlaybookRepo(jobdir_playbook, playbook,
+            self.preparePlaybook(jobdir_playbook, playbook,
-                                     args, required=False)
+                                 args, required=False)
-            if playbook['trusted']:
-                trusted = True
-            else:
-                untrusted = True
             if jobdir_playbook.path is not None:
                 self.jobdir.playbook = jobdir_playbook
                 break
+
         if self.jobdir.playbook is None:
             raise Exception("No valid playbook found")
 
         for playbook in args['post_playbooks']:
             jobdir_playbook = self.jobdir.addPostPlaybook()
-            self.preparePlaybookRepo(jobdir_playbook, playbook,
+            self.preparePlaybook(jobdir_playbook, playbook,
-                                     args, required=True)
+                                 args, required=True)
-            if playbook['trusted']:
-                trusted = True
-            else:
-                untrusted = True
-        return (trusted, untrusted)
 
-    def preparePlaybookRepo(self, jobdir_playbook, playbook, args, required):
+    def preparePlaybook(self, jobdir_playbook, playbook, args, required):
         self.log.debug("Prepare playbook repo for %s" % (playbook,))
         # Check out the playbook repo if needed and set the path to
         # the playbook that should be run.
@@ -1040,6 +1051,7 @@ class AnsibleJob(object):
         source = self.executor_server.connections.getSource(
             playbook['connection'])
         project = source.getProject(playbook['project'])
+        path = None
         if not playbook['trusted']:
             # This is a project repo, so it is safe to use the already
             # checked out version (from speculative merging) of the
@@ -1052,34 +1064,48 @@ class AnsibleJob(object):
                                         project.canonical_hostname,
                                         project.name,
                                         playbook['path'])
-                    jobdir_playbook.path = self.findPlaybook(
+                    break
-                        path,
+        if not path:
-                        required=required,
+            # The playbook repo is either a config repo, or it isn't in
-                        trusted=playbook['trusted'])
+            # the stack of changes we are testing, so check out the branch
-                    return
+            # tip into a dedicated space.
-        # The playbook repo is either a config repo, or it isn't in
+            path = self.checkoutTrustedProject(project, playbook['branch'])
-        # the stack of changes we are testing, so check out the branch
+            path = os.path.join(path, playbook['path'])
-        # tip into a dedicated space.
 
-        merger = self.executor_server._getMerger(jobdir_playbook.root,
-                                                 self.log)
-        merger.checkoutBranch(playbook['connection'], project.name,
-                              playbook['branch'])
-
-        path = os.path.join(jobdir_playbook.root,
-                            project.canonical_hostname,
-                            project.name,
-                            playbook['path'])
         jobdir_playbook.path = self.findPlaybook(
             path,
             required=required,
             trusted=playbook['trusted'])
 
-    def prepareRoles(self, args, trusted, untrusted):
+        # If this playbook doesn't exist, don't bother preparing
-        for role in args['roles']:
+        # roles.
-            if role['type'] == 'zuul':
+        if not jobdir_playbook.path:
-                root = self.jobdir.addRole()
+            return
-                self.prepareZuulRole(args, role, root, trusted, untrusted)
+
+        for role in playbook['roles']:
+            self.prepareRole(jobdir_playbook, role, args)
+
+        self.writeAnsibleConfig(jobdir_playbook)
+
+    def checkoutTrustedProject(self, project, branch):
+        root = self.jobdir.getTrustedProject(project.canonical_name,
+                                             branch)
+        if not root:
+            root = self.jobdir.addTrustedProject(project.canonical_name,
+                                                 branch)
+            merger = self.executor_server._getMerger(root, self.log)
+            merger.checkoutBranch(project.connection_name, project.name,
+                                  branch)
+
+        path = os.path.join(root,
+                            project.canonical_hostname,
+                            project.name)
+        return path
+
+    def prepareRole(self, jobdir_playbook, role, args):
+        if role['type'] == 'zuul':
+            root = jobdir_playbook.addRole()
+            self.prepareZuulRole(jobdir_playbook, role, args, root)
 
     def findRole(self, path, trusted=False):
         d = os.path.join(path, 'tasks')
@@ -1099,97 +1125,50 @@ class AnsibleJob(object):
         # It is neither a bare role, nor a collection of roles
         raise Exception("Unable to find role in %s" % (path,))
 
-    def prepareZuulRole(self, args, role, root, trusted, untrusted):
+    def prepareZuulRole(self, jobdir_playbook, role, args, root):
         self.log.debug("Prepare zuul role for %s" % (role,))
         # Check out the role repo if needed
         source = self.executor_server.connections.getSource(
             role['connection'])
         project = source.getProject(role['project'])
-        untrusted_role_repo = None
-        trusted_role_repo = None
-        trusted_root = os.path.join(root, 'trusted')
-        untrusted_root = os.path.join(root, 'untrusted')
         name = role['target_name']
+        path = None
 
-        if untrusted:
+        if not jobdir_playbook.trusted:
-            # There is at least one untrusted playbook.  For that
+            # This playbook is untrested.  Use the already checked out
-            # case, use the already checked out version (from
+            # version (from speculative merging) of the role if it
-            # speculative merging) of the role.
+            # exists.
 
             for i in args['items']:
                 if (i['connection'] == role['connection'] and
                     i['project'] == role['project']):
-                    # We already have this repo prepared;
+                    # We already have this repo prepared; use it.
-                    # copy it into location.
-
                     path = os.path.join(self.jobdir.src_root,
                                         project.canonical_hostname,
                                         project.name)
-                    # The name of the symlink is the requested name of
-                    # the role (which may be the repo name or may be
-                    # something else; this can come into play if this
-                    # is a bare role).
-                    link = os.path.join(untrusted_root, name)
-                    link = os.path.realpath(link)
-                    if not link.startswith(os.path.realpath(untrusted_root)):
-                        raise Exception("Invalid role name %s", name)
-                    os.symlink(path, link)
-                    untrusted_role_repo = link
                     break
 
-        if trusted or not untrusted_role_repo:
+        if not path:
-            # There is at least one trusted playbook which will need a
+            # This is a trusted playbook or the role did not appear
-            # trusted checkout of the role, or the role did not appear
             # in the dependency chain for the change (in which case,
             # there is no existing untrusted checkout of it).  Check
             # out the branch tip into a dedicated space.
-            merger = self.executor_server._getMerger(trusted_root,
+            path = self.checkoutTrustedProject(project, 'master')
-                                                     self.log)
-            merger.checkoutBranch(role['connection'], project.name,
-                                  'master')
-            orig_repo_path = os.path.join(trusted_root,
-                                          project.canonical_hostname,
-                                          project.name)
-            if name != project.name:
-                # The requested name of the role is not the same as
-                # the project name, so rename the git repo as the
-                # requested name.  It is the only item in this
-                # directory, so we don't need to worry about
-                # collisions.
-                target = os.path.join(trusted_root,
-                                      project.canonical_hostname,
-                                      name)
-                target = os.path.realpath(target)
-                if not target.startswith(os.path.realpath(trusted_root)):
-                    raise Exception("Invalid role name %s", name)
-                os.rename(orig_repo_path, target)
-                trusted_role_repo = target
-            else:
-                trusted_role_repo = orig_repo_path
 
-            if not untrusted_role_repo:
+        # The name of the symlink is the requested name of the role
-                # In the case that there was no untrusted checkout,
+        # (which may be the repo name or may be something else; this
-                # use the trusted checkout.
+        # can come into play if this is a bare role).
-                untrusted_role_repo = trusted_role_repo
+        link = os.path.join(root, name)
-                untrusted_root = trusted_root
+        link = os.path.realpath(link)
+        if not link.startswith(os.path.realpath(root)):
+            raise Exception("Invalid role name %s", name)
+        os.symlink(path, link)
 
-        if untrusted:
+        role_path = self.findRole(link, trusted=jobdir_playbook.trusted)
-            untrusted_role_path = self.findRole(untrusted_role_repo,
+        if role_path is None:
-                                                trusted=False)
+            # In the case of a bare role, add the containing directory
-            if untrusted_role_path is None:
+            role_path = root
-                # In the case of a bare role, add the containing directory
+        jobdir_playbook.roles_path.append(role_path)
-                untrusted_role_path = os.path.join(untrusted_root,
-                                                   project.canonical_hostname)
-            self.jobdir.untrusted_roles_path.append(untrusted_role_path)
-
-        if trusted:
-            trusted_role_path = self.findRole(trusted_role_repo,
-                                              trusted=True)
-            if trusted_role_path is None:
-                # In the case of a bare role, add the containing directory
-                trusted_role_path = os.path.join(trusted_root,
-                                                 project.canonical_hostname)
-            self.jobdir.trusted_roles_path.append(trusted_role_path)
 
     def prepareAnsibleFiles(self, args):
         all_vars = dict(args['vars'])
@@ -1213,11 +1192,10 @@ class AnsibleJob(object):
                 for key in node['host_keys']:
                     known_hosts.write('%s\n' % key)
 
-        self.writeAnsibleConfig(self.jobdir.untrusted_config)
+    def writeAnsibleConfig(self, jobdir_playbook):
-        self.writeAnsibleConfig(self.jobdir.trusted_config, trusted=True)
+        trusted = jobdir_playbook.trusted
 
-    def writeAnsibleConfig(self, config_path, trusted=False):
+        with open(jobdir_playbook.ansible_config, 'w') as config:
-        with open(config_path, 'w') as config:
             config.write('[defaults]\n')
             config.write('hostfile = %s\n' % self.jobdir.inventory)
             config.write('local_tmp = %s/.ansible/local_tmp\n' %
@@ -1240,12 +1218,10 @@ class AnsibleJob(object):
                              % self.executor_server.action_dir)
                 config.write('lookup_plugins = %s\n'
                              % self.executor_server.lookup_dir)
-                roles_path = self.jobdir.untrusted_roles_path
-            else:
-                roles_path = self.jobdir.trusted_roles_path
 
-            if roles_path:
+            if jobdir_playbook.roles_path:
-                config.write('roles_path = %s\n' % ':'.join(roles_path))
+                config.write('roles_path = %s\n' % ':'.join(
+                    jobdir_playbook.roles_path))
 
             # On trusted jobs, we want to prevent the printing of args,
             # since trusted jobs might have access to secrets that they may
@@ -1286,7 +1262,7 @@ class AnsibleJob(object):
             except Exception:
                 self.log.exception("Exception while killing ansible process:")
 
-    def runAnsible(self, cmd, timeout, trusted=False):
+    def runAnsible(self, cmd, timeout, config_file, trusted):
         env_copy = os.environ.copy()
         env_copy.update(self.ssh_agent.env)
         env_copy['LOGNAME'] = 'zuul'
@@ -1301,10 +1277,8 @@ class AnsibleJob(object):
         env_copy['PYTHONPATH'] = os.path.pathsep.join(pythonpath)
 
         if trusted:
-            config_file = self.jobdir.trusted_config
             opt_prefix = 'trusted'
         else:
-            config_file = self.jobdir.untrusted_config
             opt_prefix = 'untrusted'
         ro_dirs = get_default(self.executor_server.config, 'executor',
                               '%s_ro_dirs' % opt_prefix)
@@ -1409,7 +1383,9 @@ class AnsibleJob(object):
             cmd.extend(['-e', 'zuul_execution_phase_count=%s' % count])
 
         result, code = self.runAnsible(
-            cmd=cmd, timeout=timeout, trusted=playbook.trusted)
+            cmd=cmd, timeout=timeout,
+            config_file=playbook.ansible_config,
+            trusted=playbook.trusted)
         self.log.debug("Ansible complete, result %s code %s" % (
             self.RESULT_MAP[result], code))
         return result, code

zuul/model.py

@@ -632,11 +632,17 @@ class PlaybookContext(object):
 
     Jobs refer to objects of this class for their main, pre, and post
     playbooks so that we can keep track of which repos and security
-    contexts are needed in order to run them."""
+    contexts are needed in order to run them.
 
-    def __init__(self, source_context, path):
+    We also keep a list of roles so that playbooks only run with the
+    roles which were defined at the point the playbook was defined.
+
+    """
+
+    def __init__(self, source_context, path, roles):
         self.source_context = source_context
         self.path = path
+        self.roles = roles
 
     def __repr__(self):
         return '<PlaybookContext %s %s>' % (self.source_context,
@@ -649,7 +655,8 @@ class PlaybookContext(object):
         if not isinstance(other, PlaybookContext):
             return False
         return (self.source_context == other.source_context and
-                self.path == other.path)
+                self.path == other.path and
+                self.roles == other.roles)
 
     def toDict(self):
         # Render to a dict to use in passing json to the executor
@@ -658,6 +665,7 @@ class PlaybookContext(object):
             project=self.source_context.project.name,
             branch=self.source_context.branch,
             trusted=self.source_context.trusted,
+            roles=[r.toDict() for r in self.roles],
             path=self.path)