zuul
/
zuul
Code Issues Proposed changes

Perform late validation of secrets 

This moves validation of the existence of secrets to after the
Job object has been constructed.  In the future, this will let
us re-validate Job objects if, for example, only the set of
secret objects has changed, without having to create new Job
objects.

We currently late-validate job parents; this is updated to use
the same new, more generic, mechanism as secrets.

This does incur a memory penalty in that we will be carrying around
start_mark objects which contain the original text of each
configuration entry attached to their resulting objects.  That will
increase the amount of memory used in the near term, but in the long
term, this will enable us to have fewer objects overall, once we
are able to re-use the same objects between different layouts.

The noop job needs a more correct fake playbook in order for it to
pass the new validation check.

The secret_not_found test is updated because the old version had
a configuration error (job not found) which, due to the later validation
of secrets, would have been propogated first.  This updates that
configuration so we proceed far enough to hit the secret not found
error.

Change-Id: I5bb673839ecf09939e4e57e7d548ef3fd96f767f
This commit is contained in:
James E. Blair 2018-03-14 10:15:51 -07:00
parent fd8896958f
commit 305459a9eb
3 changed files with 97 additions and 44 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

3
tests/unit/test_v3.py
View File

@ -1543,7 +1543,8 @@ class TestInRepoConfig(ZuulTestCase):
in_repo_conf = textwrap.dedent(
"""
- job:
name: test
name: project-test1
run: playbooks/project-test1.yaml
secrets: does-not-exist
""")

102
zuul/configloader.py
View File

@ -117,15 +117,6 @@ class TemplateNotFoundError(Exception):
super(TemplateNotFoundError, self).__init__(message)
class SecretNotFoundError(Exception):
def __init__(self, secret):
message = textwrap.dedent("""\
The secret "{secret}" was not found.
""")
message = textwrap.fill(message.format(secret=secret))
super(SecretNotFoundError, self).__init__(message)
class NodesetNotFoundError(Exception):
def __init__(self, nodeset):
message = textwrap.dedent("""\
@ -251,6 +242,39 @@ def configuration_exceptions(stanza, conf):
raise ConfigurationSyntaxError(m)
@contextmanager
def reference_exceptions(stanza, context, start_mark):
try:
yield
except ConfigurationSyntaxError:
raise
except Exception as e:
intro = textwrap.fill(textwrap.dedent("""\
Zuul encountered a syntax error while parsing its configuration in the
repo {repo} on branch {branch}. The error was:""".format(
repo=context.project.name,
branch=context.branch,
)))
m = textwrap.dedent("""\
{intro}
{error}
The error appears in the following {stanza} stanza:
{content}
{start_mark}""")
m = m.format(intro=intro,
error=indent(str(e)),
stanza=stanza,
content=indent(start_mark.snippet.rstrip()),
start_mark=str(start_mark))
raise ConfigurationSyntaxError(m)
class ZuulMark(object):
# The yaml mark class differs between the C and python versions.
# The C version does not provide a snippet, and also appears to
@ -595,7 +619,7 @@ class JobParser(object):
job = model.Job(name)
job.description = conf.get('description')
job.source_context = conf.get('_source_context')
job.source_line = conf.get('_start_mark').line + 1
job.start_mark = conf.get('_start_mark')
job.variant_description = conf.get(
'variant-description', " ".join(as_list(conf.get('branches'))))
@ -618,24 +642,11 @@ class JobParser(object):
for secret_config in as_list(conf.get('secrets', [])):
if isinstance(secret_config, str):
secret_name = secret_config
secret = self.pcontext.layout.secrets.get(secret_name)
secret_alias = secret_config
else:
secret_name = secret_config['name']
secret = self.pcontext.layout.secrets.get(
secret_config['secret'])
if secret is None:
raise SecretNotFoundError(secret_name)
if secret_name == 'zuul' or secret_name == 'nodepool':
raise Exception("Secrets named 'zuul' or 'nodepool' "
"are not allowed.")
if not secret.source_context.isSameProject(job.source_context):
raise Exception(
"Unable to use secret %s. Secrets must be "
"defined in the same project in which they "
"are used" % secret_name)
# 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))
secret_name = secret_config['secret']
secret_alias = secret_config['name']
secrets.append((secret_name, secret_alias))
# A job in an untrusted repo that uses secrets requires
# special care. We must note this, and carry this flag
@ -875,11 +886,6 @@ class ProjectTemplateParser(object):
attrs['_source_context'] = source_context
attrs['_start_mark'] = start_mark
# validate that the job is existing
with configuration_exceptions('project or project-template',
attrs):
self.pcontext.layout.getJob(jobname)
job_list.addJob(self.pcontext.job_parser.fromYaml(
attrs, project_pipeline=True,
name=jobname, validate=False))
@ -1662,15 +1668,13 @@ class TenantParser(object):
"Skipped adding job %s which shadows an existing job" %
(job,))
# Now that all the jobs are loaded, verify their parents exist
for config_job in data.jobs:
classes = self._getLoadClasses(tenant, config_job)
if 'job' not in classes:
continue
with configuration_exceptions('job', config_job):
parent = config_job.get('parent')
if parent:
layout.getJob(parent)
# Now that all the jobs are loaded, verify references to other
# config objects.
for jobs in layout.jobs.values():
for job in jobs:
with reference_exceptions('job', job.source_context,
job.start_mark):
job.validateReferences(layout)
if skip_semaphores:
# We should not actually update the layout with new
@ -1720,6 +1724,22 @@ class TenantParser(object):
layout.addProjectConfig(pcontext.project_parser.fromYaml(
filtered_projects))
# Now that all the project pipelines are loaded, verify
# references to other config objects.
for project_config in layout.project_configs.values():
for project_pipeline_config in project_config.pipelines.values():
for jobs in project_pipeline_config.job_list.jobs.values():
for job in jobs:
with reference_exceptions(
'project or project-template',
job.source_context,
job.start_mark):
# validate that the job exists on its own (an
# additional requirement for project-pipeline
# jobs)
layout.getJob(job.name)
job.validateReferences(layout)
def _flattenProjects(self, projects, tenant):
# Group together all of the project stanzas for each project.
result_projects = {}

36
zuul/model.py
View File

@ -723,6 +723,27 @@ class PlaybookContext(object):
self.secrets)
return r
def validateReferences(self, layout):
# Verify that references to other objects in the layout are
# valid.
for (secret_name, secret_alias) in self.secrets:
secret = layout.secrets.get(secret_name)
if secret is None:
raise Exception(
'The secret "{name}" was not found.'.format(
name=secret_name))
if secret_alias == 'zuul' or secret_alias == 'nodepool':
raise Exception('Secrets named "zuul" or "nodepool" '
'are not allowed.')
if not secret.source_context.isSameProject(self.source_context):
raise Exception(
"Unable to use secret {name}. Secrets must be "
"defined in the same project in which they "
"are used".format(
name=secret_name))
# Decrypt a copy of the secret to verify it can be done
secret.decrypt(self.source_context.project.private_key)
def freezeSecrets(self, layout):
secrets = []
for (secret_name, secret_alias) in self.secrets:
@ -874,7 +895,7 @@ class Job(object):
self.other_attributes = dict(
name=None,
source_context=None,
source_line=None,
start_mark=None,
inheritance_path=(),
parent_data=None,
description=None,
@ -913,11 +934,14 @@ class Job(object):
return self.name
def __repr__(self):
ln = 0
if self.start_mark:
ln = self.start_mark.line
return '<Job %s branches: %s source: %s#%s>' % (
self.name,
self.branch_matcher,
self.source_context,
self.source_line)
ln)
def __getattr__(self, name):
v = self.__dict__.get(name)
@ -943,6 +967,14 @@ class Job(object):
if self._get('post_run') is not None:
self.post_run = self.freezePlaybooks(self.post_run, layout)
def validateReferences(self, layout):
# Verify that references to other objects in the layout are
# valid.
if not self.isBase() and self.parent:
layout.getJob(self.parent)
for pb in self.pre_run + self.run + self.post_run:
pb.validateReferences(layout)
def addRoles(self, roles):
newroles = []
# Start with a copy of the existing roles, but if any of them