zuul/zuul
Code Issues Proposed changes

Honor independent pipeline requirements for non-live changes

Browse Source 
Independent pipelines ignore requirements for non-live changes
because they are not actually executed.  However, a user might
configure an independent pipeline that requires code review and
expect a positive code-review pipeline requirement to be enforced.
To ignore it risks executing unreviewed code via dependencies.

To correct this, we now enforce pipeline requirements in independent
pipelines in the same way as dependent ones.

This also adds a new "allow-other-connections" pipeline configuration
option which permits users to specify exhaustive pipeline requirements.

Change-Id: I6c006f9e63a888f83494e575455395bd534b955f
Story: 2010515
This commit is contained in:
James E. Blair 2023-01-10 15:04:49 -08:00
parent 8f6a421b37
commit 3f3101216e
15 changed files with 167 additions and 12 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

11
doc/source/config/pipeline.rst
View File

@ -266,6 +266,17 @@ success, the pipeline reports back to Gerrit with ``Verified`` vote of
type of the connection will dictate which options are available.
See :ref:`drivers`.
.. attr:: allow-other-connections
:default: true
If this is set to `false` then any change enqueued into the
pipeline (whether it is enqueued to run jobs or merely as a
dependency) must be from one of the connections specified in the
pipeline configuration (this includes any trigger, reporter, or
source requirement). When used in conjuctions with
:attr:`pipeline.require`, this can ensure that pipeline
requirements are exhaustive.
.. attr:: supercedes
The name of a pipeline, or a list of names, that this pipeline

29
releasenotes/notes/non-live-pipeline-requirements-aa173bd86b332e63.yaml Normal file
View File

@ -0,0 +1,29 @@
---
features:
- |
A new pipeline attribute,
:attr:`pipeline.allow-other-connections`, has been added
to ensure that only changes from connections which
are mentioned in the pipeline configuration (such as triggers,
reporters, or pipeline requirements) are enqueued.
security:
- |
Non-live items are now subject to pipeline requirements for
independent pipelines.
Previously, an optimization for independent pipelines skipped
checking that a change met the pipeline requirements. If an
independent pipeline is intended only to run reviewed code, this
could allow running unreviewed code by updating dependent changes.
Now both non-live and live items are subject to pipeline
requirements in all pipeline managers.
- |
The new `allow-other-connections` pipeline configuration option
may now be used to ensure that only changes from connections which
are mentioned in the pipeline configuration (such as triggers,
reporters, or pipeline requirements) are enqueued. This allows
the construction of a pipeline where, for example, code review
requirements are strictly enforced, even for dependencies which
are not normally directly enqueued.

2
tests/fixtures/config/requirements/trusted-check/git/common-config/playbooks/base.yaml vendored Normal file
View File

@ -0,0 +1,2 @@
- hosts: all
tasks: []

41
tests/fixtures/config/requirements/trusted-check/git/common-config/zuul.yaml vendored Normal file
View File

@ -0,0 +1,41 @@
- pipeline:
name: trusted-check
manager: independent
allow-other-connections: false
require:
gerrit:
approval:
- Code-Review: 2
trigger:
gerrit:
- event: patchset-created
success:
gerrit:
Verified: 1
failure:
gerrit:
Verified: -1
- job:
name: base
parent: null
run: playbooks/base.yaml
nodeset:
nodes:
- label: ubuntu-xenial
name: controller
- job:
name: check-job
- project:
name: org/project
trusted-check:
jobs:
- check-job
- project:
name: gh/project
trusted-check:
jobs:
- check-job

1
tests/fixtures/config/requirements/trusted-check/git/gh_project/README vendored Normal file
View File

@ -0,0 +1 @@
test

1
tests/fixtures/config/requirements/trusted-check/git/org_project/README vendored Normal file
View File

@ -0,0 +1 @@
test

11
tests/fixtures/config/requirements/trusted-check/main.yaml vendored Normal file
View File

@ -0,0 +1,11 @@
- tenant:
name: tenant-one
source:
gerrit:
config-projects:
- common-config
untrusted-projects:
- org/project
github:
untrusted-projects:
- gh/project

4
tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml vendored
View File

@ -4,7 +4,7 @@
require:
gerrit:
approval:
- Verified: -1
- email: for-check@example.com
trigger:
gerrit:
- event: patchset-created
@ -24,7 +24,7 @@
require:
gerrit:
approval:
- Verified: 1
- email: for-gate@example.com
trigger:
gerrit:
- event: comment-added

37
tests/unit/test_requirements.py
View File

@ -453,3 +453,40 @@ class TestRequirementsReject(ZuulTestCase):
self.fake_gerrit.addEvent(comment)
self.waitUntilSettled()
self.assertEqual(len(self.history), 3)
class TestRequirementsTrustedCheck(ZuulTestCase):
config_file = "zuul-gerrit-github.conf"
tenant_config_file = "config/requirements/trusted-check/main.yaml"
def test_non_live_requirements(self):
# Test that pipeline requirements are applied to non-live
# changes.
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A')
B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B')
B.setDependsOn(A, 1)
B.addApproval('Code-Review', 2)
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertHistory([])
self.fake_gerrit.addEvent(A.addApproval('Code-Review', 2))
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertHistory([
dict(name='check-job', result='SUCCESS', changes='1,1 2,1')],
ordered=False)
def test_other_connections(self):
# Test allow-other-connections: False
A = self.fake_github.openFakePullRequest("gh/project", "master", "A")
B = self.fake_gerrit.addFakeChange('org/project', 'master', 'B')
B.data["commitMessage"] = "{}\n\nDepends-On: {}\n".format(
B.subject, A.url,
)
B.addApproval('Code-Review', 2)
self.fake_gerrit.addEvent(B.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertHistory([])

1
tests/unit/test_scheduler.py
View File

@ -2807,6 +2807,7 @@ class TestScheduler(ZuulTestCase):
B = self.fake_gerrit.addFakeChange('org/project1', 'master', 'B')
B.data['commitMessage'] = '%s\n\nDepends-On: %s\n' % (
B.subject, A.data['url'])
A.addApproval('Code-Review', 2)
B.addApproval('Code-Review', 2)
self.executor_server.hold_jobs_in_build = True

12
tests/unit/test_zuultrigger.py
View File

@ -35,9 +35,10 @@ class TestZuulTriggerParentChangeEnqueued(ZuulTestCase):
A.addApproval('Code-Review', 2)
B1.addApproval('Code-Review', 2)
B2.addApproval('Code-Review', 2)
A.addApproval('Verified', 1) # required by gate
B1.addApproval('Verified', -1) # should go to check
B2.addApproval('Verified', 1) # should go to gate
A.addApproval('Verified', 1, username="for-check") # reqd by check
A.addApproval('Verified', 1, username="for-gate") # reqd by gate
B1.addApproval('Verified', 1, username="for-check") # go to check
B2.addApproval('Verified', 1, username="for-gate") # go to gate
B1.addApproval('Approved', 1)
B2.addApproval('Approved', 1)
B1.setDependsOn(A, 1)
@ -75,9 +76,9 @@ class TestZuulTriggerParentChangeEnqueued(ZuulTestCase):
self.scheds.first.sched, "addTriggerEvent", addTriggerEvent
):
C = self.fake_gerrit.addFakeChange('org/project', 'master', 'C')
C.addApproval('Verified', -1)
C.addApproval('Verified', 1, username="for-check")
D = self.fake_gerrit.addFakeChange('org/project', 'master', 'D')
D.addApproval('Verified', -1)
D.addApproval('Verified', 1, username="for-check")
D.setDependsOn(C, 1)
self.fake_gerrit.addEvent(C.getPatchsetCreatedEvent(1))
@ -108,6 +109,7 @@ class TestZuulTriggerParentChangeEnqueuedGithub(ZuulGithubAppTestCase):
B1.addReview('derp', 'APPROVED')
B2.addReview('derp', 'APPROVED')
A.addLabel('for-gate') # required by gate
A.addLabel('for-check') # required by check
B1.addLabel('for-check') # should go to check
B2.addLabel('for-gate') # should go to gate

9
zuul/configloader.py
View File

@ -1292,6 +1292,7 @@ class PipelineParser(object):
pipeline = {vs.Required('name'): str,
vs.Required('manager'): manager,
'allow-other-connections': bool,
'precedence': precedence,
'supercedes': to_list(str),
'description': str,
@ -1331,6 +1332,8 @@ class PipelineParser(object):
pipeline = model.Pipeline(conf['name'], self.pcontext.tenant)
pipeline.source_context = conf['_source_context']
pipeline.start_mark = conf['_start_mark']
pipeline.allow_other_connections = conf.get(
'allow-other-connections', True)
pipeline.description = conf.get('description')
pipeline.supercedes = as_list(conf.get('supercedes', []))
@ -1366,6 +1369,7 @@ class PipelineParser(object):
# Make a copy to manipulate for backwards compat.
conf_copy = conf.copy()
seen_connections = set()
for conf_key, action in self.reporter_actions.items():
reporter_set = []
allowed_reporters = self.pcontext.tenant.allowed_reporters
@ -1379,6 +1383,7 @@ class PipelineParser(object):
reporter_name, pipeline, params)
reporter.setAction(conf_key)
reporter_set.append(reporter)
seen_connections.add(reporter_name)
setattr(pipeline, action, reporter_set)
# If merge-conflict actions aren't explicit, use the failure actions
@ -1423,11 +1428,13 @@ class PipelineParser(object):
source = self.pcontext.connections.getSource(source_name)
manager.ref_filters.extend(
source.getRequireFilters(require_config))
seen_connections.add(source_name)
for source_name, reject_config in conf.get('reject', {}).items():
source = self.pcontext.connections.getSource(source_name)
manager.ref_filters.extend(
source.getRejectFilters(reject_config))
seen_connections.add(source_name)
for connection_name, trigger_config in conf.get('trigger').items():
if self.pcontext.tenant.allowed_triggers is not None and \
@ -1439,7 +1446,9 @@ class PipelineParser(object):
manager.event_filters.extend(
trigger.getEventFilters(connection_name,
conf['trigger'][connection_name]))
seen_connections.add(connection_name)
pipeline.connections = list(seen_connections)
# Pipelines don't get frozen
return pipeline

6
zuul/manager/__init__.py
View File

@ -508,6 +508,12 @@ class PipelineManager(metaclass=ABCMeta):
log.debug("Change %s is already in pipeline, ignoring" % change)
return True
if ((not self.pipeline.allow_other_connections) and
(change.project.connection_name not in self.pipeline.connections)):
log.debug("Change %s is not from a connection known to %s ",
change, self.pipeline)
return False
if not ignore_requirements:
for f in self.ref_filters:
if f.connection_name != change.project.connection_name:

12
zuul/manager/independent.py
View File

@ -61,13 +61,15 @@ class IndependentPipelineManager(PipelineManager):
for needed_change in needed_changes:
# This differs from the dependent pipeline by enqueuing
# changes ahead as "not live", that is, not intended to
# have jobs run. Also, pipeline requirements are always
# ignored (which is safe because the changes are not
# live).
# have jobs run. Pipeline requirements are still in place
# in order to avoid unreviewed code being executed in
# pipelines that require review.
if needed_change not in history:
r = self.addChange(needed_change, event, quiet=True,
ignore_requirements=True, live=False,
change_queue=change_queue, history=history,
ignore_requirements=ignore_requirements,
live=False,
change_queue=change_queue,
history=history,
dependency_graph=dependency_graph)
if not r:
return False

2
zuul/model.py
View File

@ -438,6 +438,8 @@ class Pipeline(object):
# reconfigured). A pipeline requires a tenant in order to
# reach the currently active layout for that tenant.
self.tenant = tenant
self.allow_other_connections = True
self.connections = []
self.source_context = None
self.start_mark = None
self.description = None