From 74a82cff6c1234c04a2361813c62a63d35de8935 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Wed, 12 Jul 2017 17:23:08 -0700 Subject: [PATCH] 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 | 15 +- tests/base.py | 4 +- tests/unit/test_model.py | 12 +- zuul/configloader.py | 30 ++-- zuul/executor/client.py | 1 - zuul/executor/server.py | 300 +++++++++++++++++-------------------- zuul/model.py | 14 +- 7 files changed, 187 insertions(+), 189 deletions(-) diff --git a/doc/source/user/config.rst b/doc/source/user/config.rst index 0b2b5d4531..64dc393fec 100644 --- a/doc/source/user/config.rst +++ b/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 - in untrusted projects. + speculative merging and cross-project dependencies when used by + 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 diff --git a/tests/base.py b/tests/base.py index 9709bf783a..1cc9999182 100755 --- a/tests/base.py +++ b/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 diff --git a/tests/unit/test_model.py b/tests/unit/test_model.py index 7fe101e903..23a6f26755 100644 --- a/tests/unit/test_model.py +++ b/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_run = model.PlaybookContext(self.context, 'base-run') - base_post = model.PlaybookContext(self.context, 'base-post') + base_pre = model.PlaybookContext(self.context, 'base-pre', []) + base_run = model.PlaybookContext(self.context, 'base-run', []) + 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_run = model.PlaybookContext(self.context, 'py27-run') - py27_post = model.PlaybookContext(self.context, 'py27-post') + py27_pre = model.PlaybookContext(self.context, 'py27-pre', []) + py27_run = model.PlaybookContext(self.context, 'py27-run', []) + py27_post = model.PlaybookContext(self.context, 'py27-post', []) py27 = model.Job('py27') py27.timeout = 30 diff --git a/zuul/configloader.py b/zuul/configloader.py index 254527adf4..023d01efae 100644 --- a/zuul/configloader.py +++ b/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) diff --git a/zuul/executor/client.py b/zuul/executor/client.py index c36d5694d3..2a205bf335 100644 --- a/zuul/executor/client.py +++ b/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 = [] diff --git a/zuul/executor/server.py b/zuul/executor/server.py index 818c7e2ded..c6b819eb8a 100644 --- a/zuul/executor/server.py +++ b/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 - # untrusted.cfg + # inventory.yaml + # playbook_0 + # project -> ../trusted/project_0/... + # role_0 -> ../trusted/project_0/... + # trusted + # project_0 + # + # # work + # .ssh + # known_hosts # src + # + # # 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'] + - args['post_playbooks'] + args['roles']) + repos = [] + 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? - trusted, untrusted = self.preparePlaybookRepos(args) + # This prepares each playbook and the roles needed for each. + 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): - trusted = untrusted = False + def preparePlaybooks(self, args): for playbook in args['pre_playbooks']: jobdir_playbook = self.jobdir.addPrePlaybook() - self.preparePlaybookRepo(jobdir_playbook, playbook, - args, required=True) - if playbook['trusted']: - trusted = True - else: - untrusted = True + self.preparePlaybook(jobdir_playbook, playbook, + args, required=True) for playbook in args['playbooks']: jobdir_playbook = self.jobdir.addPlaybook() - self.preparePlaybookRepo(jobdir_playbook, playbook, - args, required=False) - if playbook['trusted']: - trusted = True - else: - untrusted = True + self.preparePlaybook(jobdir_playbook, playbook, + args, required=False) 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, - args, required=True) - if playbook['trusted']: - trusted = True - else: - untrusted = True - return (trusted, untrusted) + self.preparePlaybook(jobdir_playbook, playbook, + args, required=True) - 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( - path, - required=required, - trusted=playbook['trusted']) - return - # The playbook repo is either a config repo, or it isn't in - # the stack of changes we are testing, so check out the branch - # tip into a dedicated space. + break + if not path: + # The playbook repo is either a config repo, or it isn't in + # the stack of changes we are testing, so check out the branch + # tip into a dedicated space. + path = self.checkoutTrustedProject(project, playbook['branch']) + path = os.path.join(path, playbook['path']) - 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): - for role in args['roles']: - if role['type'] == 'zuul': - root = self.jobdir.addRole() - self.prepareZuulRole(args, role, root, trusted, untrusted) + # If this playbook doesn't exist, don't bother preparing + # roles. + if not jobdir_playbook.path: + return + + 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: - # There is at least one untrusted playbook. For that - # case, use the already checked out version (from - # speculative merging) of the role. + if not jobdir_playbook.trusted: + # This playbook is untrested. Use the already checked out + # version (from speculative merging) of the role if it + # exists. for i in args['items']: if (i['connection'] == role['connection'] and i['project'] == role['project']): - # We already have this repo prepared; - # copy it into location. - + # We already have this repo prepared; use it. 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: - # There is at least one trusted playbook which will need a - # trusted checkout of the role, or the role did not appear + if not path: + # This is a trusted playbook 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, - 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 + path = self.checkoutTrustedProject(project, 'master') - if not untrusted_role_repo: - # In the case that there was no untrusted checkout, - # use the trusted checkout. - untrusted_role_repo = trusted_role_repo - untrusted_root = trusted_root + # 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(root, name) + 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: - untrusted_role_path = self.findRole(untrusted_role_repo, - trusted=False) - if untrusted_role_path is None: - # In the case of a bare role, add the containing directory - 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) + role_path = self.findRole(link, trusted=jobdir_playbook.trusted) + if role_path is None: + # In the case of a bare role, add the containing directory + role_path = root + jobdir_playbook.roles_path.append(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) - self.writeAnsibleConfig(self.jobdir.trusted_config, trusted=True) + def writeAnsibleConfig(self, jobdir_playbook): + trusted = jobdir_playbook.trusted - def writeAnsibleConfig(self, config_path, trusted=False): - with open(config_path, 'w') as config: + with open(jobdir_playbook.ansible_config, '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: - config.write('roles_path = %s\n' % ':'.join(roles_path)) + if jobdir_playbook.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 diff --git a/zuul/model.py b/zuul/model.py index de733d7720..66c043d5f8 100644 --- a/zuul/model.py +++ b/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 '' % (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)