From f37568cc102760d7b0946e1a907bccf6171cfcb6 Mon Sep 17 00:00:00 2001 From: "James E. Blair" Date: Tue, 20 Feb 2018 14:57:25 -0800 Subject: [PATCH] Don't store references to secret objects from jobs This removes direct references to secret objects, instead resolving them at run time. They are also resolved and decrypted (but not stored) at configuration time for validation purposes. This is part of a change series to re-use configuration objects between layouts to save memory. Change-Id: I004f381d9df8c165b04ba540527d42ed74ee8fe1 --- tests/unit/test_model.py | 6 +++--- zuul/configloader.py | 9 +++------ zuul/model.py | 31 +++++++++++++++++++++---------- 3 files changed, 27 insertions(+), 19 deletions(-) diff --git a/tests/unit/test_model.py b/tests/unit/test_model.py index 6ec5232704..3fecb26c37 100644 --- a/tests/unit/test_model.py +++ b/tests/unit/test_model.py @@ -121,7 +121,7 @@ class TestJob(BaseTestCase): # Apply the diablo variant diablo = model.Job('py27') diablo.timeout = 40 - job.applyVariant(diablo) + job.applyVariant(diablo, self.layout) self.assertEqual(40, job.timeout) self.assertEqual(['py27-pre'], @@ -140,7 +140,7 @@ class TestJob(BaseTestCase): good_final = model.Job('py27') good_final.voting = False - job.applyVariant(good_final) + job.applyVariant(good_final, self.layout) self.assertFalse(job.voting) bad_final = model.Job('py27') @@ -148,7 +148,7 @@ class TestJob(BaseTestCase): with testtools.ExpectedException( Exception, "Unable to modify final job"): - job.applyVariant(bad_final) + job.applyVariant(bad_final, self.layout) def test_job_inheritance_job_tree(self): pipeline = model.Pipeline('gate', self.layout) diff --git a/zuul/configloader.py b/zuul/configloader.py index d3f3236d40..cab59499d5 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -629,12 +629,9 @@ class JobParser(object): "Unable to use secret %s. Secrets must be " "defined in the same project in which they " "are used" % secret_name) - # If the secret declares a different name, set it on the decrypted - # copy of the secret object - decrypted_secret = secret.decrypt( - job.source_context.project.private_key) - decrypted_secret.name = secret_name - secrets.append(decrypted_secret) + # Decrypt a copy of the secret to verify it can be done + secret.decrypt(job.source_context.project.private_key) + secrets.append((secret.name, secret_name)) # A job in an untrusted repo that uses secrets requires # special care. We must note this, and carry this flag diff --git a/zuul/model.py b/zuul/model.py index 44e8d06f05..37d173e05f 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -698,6 +698,7 @@ class PlaybookContext(object): self.path = path self.roles = roles self.secrets = secrets + self.decrypted_secrets = [] def __repr__(self): return '' % (self.source_context, @@ -721,12 +722,21 @@ class PlaybookContext(object): self.secrets) return r + def freezeSecrets(self, layout): + secrets = [] + for (secret_name, secret_alias) in self.secrets: + secret = layout.secrets.get(secret_name) + decrypted_secret = secret.decrypt( + self.source_context.project.private_key) + decrypted_secret.name = secret_alias + secrets.append(decrypted_secret) + self.decrypted_secrets = secrets + def toDict(self): # Render to a dict to use in passing json to the executor secrets = {} - for secret in self.secrets: - secret_data = copy.deepcopy(secret.secret_data) - secrets[secret.name] = secret_data + for secret in self.decrypted_secrets: + secrets[secret.name] = secret.secret_data return dict( connection=self.source_context.project.connection_name, project=self.source_context.project.name, @@ -1036,7 +1046,7 @@ class Job(object): setattr(job, k, copy.deepcopy(self._get(k))) return job - def freezePlaybooks(self, pblist): + def freezePlaybooks(self, pblist, layout): """Take a list of playbooks, and return a copy of it updated with this job's roles. @@ -1046,10 +1056,11 @@ class Job(object): for old_pb in pblist: pb = old_pb.copy() pb.roles = self.roles + pb.freezeSecrets(layout) ret.append(pb) return tuple(ret) - def applyVariant(self, other): + def applyVariant(self, other, layout): """Copy the attributes which have been set on the other job to this job.""" if not isinstance(other, Job): @@ -1107,13 +1118,13 @@ class Job(object): self.addRoles(other.roles) if other._get('run') is not None: - other_run = self.freezePlaybooks(other.run) + other_run = self.freezePlaybooks(other.run, layout) self.run = other_run if other._get('pre_run') is not None: - other_pre_run = self.freezePlaybooks(other.pre_run) + other_pre_run = self.freezePlaybooks(other.pre_run, layout) self.pre_run = self.pre_run + other_pre_run if other._get('post_run') is not None: - other_post_run = self.freezePlaybooks(other.post_run) + other_post_run = self.freezePlaybooks(other.post_run, layout) self.post_run = other_post_run + self.post_run self.updateVariables(other.variables, other.host_variables, other.group_variables) @@ -2820,7 +2831,7 @@ class Layout(object): frozen_job = variant.copy() frozen_job.setBase() else: - frozen_job.applyVariant(variant) + frozen_job.applyVariant(variant, item.layout) frozen_job.name = variant.name frozen_job.name = jobname # Whether the change matches any of the project pipeline @@ -2828,7 +2839,7 @@ class Layout(object): matched = False for variant in job_list.jobs[jobname]: if variant.changeMatches(change): - frozen_job.applyVariant(variant) + frozen_job.applyVariant(variant, item.layout) matched = True self.log.debug("Pipeline variant %s matched %s", repr(variant), change)