diff options
-rw-r--r-- | doc/source/config/pipeline.rst | 11 | ||||
-rw-r--r-- | releasenotes/notes/non-live-pipeline-requirements-aa173bd86b332e63.yaml | 29 | ||||
-rw-r--r-- | tests/fixtures/config/requirements/trusted-check/git/common-config/playbooks/base.yaml | 2 | ||||
-rw-r--r-- | tests/fixtures/config/requirements/trusted-check/git/common-config/zuul.yaml | 41 | ||||
-rw-r--r-- | tests/fixtures/config/requirements/trusted-check/git/gh_project/README | 1 | ||||
-rw-r--r-- | tests/fixtures/config/requirements/trusted-check/git/org_project/README | 1 | ||||
-rw-r--r-- | tests/fixtures/config/requirements/trusted-check/main.yaml | 11 | ||||
-rw-r--r-- | tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml | 4 | ||||
-rw-r--r-- | tests/unit/test_requirements.py | 37 | ||||
-rw-r--r-- | tests/unit/test_scheduler.py | 1 | ||||
-rw-r--r-- | tests/unit/test_zuultrigger.py | 12 | ||||
-rw-r--r-- | zuul/configloader.py | 9 | ||||
-rw-r--r-- | zuul/manager/__init__.py | 6 | ||||
-rw-r--r-- | zuul/manager/independent.py | 12 | ||||
-rw-r--r-- | zuul/model.py | 2 |
15 files changed, 167 insertions, 12 deletions
diff --git a/doc/source/config/pipeline.rst b/doc/source/config/pipeline.rst index f4d7cce69..41bedfbf2 100644 --- a/doc/source/config/pipeline.rst +++ b/doc/source/config/pipeline.rst @@ -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 diff --git a/releasenotes/notes/non-live-pipeline-requirements-aa173bd86b332e63.yaml b/releasenotes/notes/non-live-pipeline-requirements-aa173bd86b332e63.yaml new file mode 100644 index 000000000..052d5b255 --- /dev/null +++ b/releasenotes/notes/non-live-pipeline-requirements-aa173bd86b332e63.yaml @@ -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. diff --git a/tests/fixtures/config/requirements/trusted-check/git/common-config/playbooks/base.yaml b/tests/fixtures/config/requirements/trusted-check/git/common-config/playbooks/base.yaml new file mode 100644 index 000000000..f679dceae --- /dev/null +++ b/tests/fixtures/config/requirements/trusted-check/git/common-config/playbooks/base.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/requirements/trusted-check/git/common-config/zuul.yaml b/tests/fixtures/config/requirements/trusted-check/git/common-config/zuul.yaml new file mode 100644 index 000000000..494cd3cc7 --- /dev/null +++ b/tests/fixtures/config/requirements/trusted-check/git/common-config/zuul.yaml @@ -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 diff --git a/tests/fixtures/config/requirements/trusted-check/git/gh_project/README b/tests/fixtures/config/requirements/trusted-check/git/gh_project/README new file mode 100644 index 000000000..9daeafb98 --- /dev/null +++ b/tests/fixtures/config/requirements/trusted-check/git/gh_project/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/requirements/trusted-check/git/org_project/README b/tests/fixtures/config/requirements/trusted-check/git/org_project/README new file mode 100644 index 000000000..9daeafb98 --- /dev/null +++ b/tests/fixtures/config/requirements/trusted-check/git/org_project/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/requirements/trusted-check/main.yaml b/tests/fixtures/config/requirements/trusted-check/main.yaml new file mode 100644 index 000000000..c8deb36c1 --- /dev/null +++ b/tests/fixtures/config/requirements/trusted-check/main.yaml @@ -0,0 +1,11 @@ +- tenant: + name: tenant-one + source: + gerrit: + config-projects: + - common-config + untrusted-projects: + - org/project + github: + untrusted-projects: + - gh/project diff --git a/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml b/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml index 3fcc43ce6..975b9713e 100644 --- a/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml +++ b/tests/fixtures/config/zuultrigger/parent-change-enqueued/git/common-config/zuul.yaml @@ -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 diff --git a/tests/unit/test_requirements.py b/tests/unit/test_requirements.py index 9a32e4b21..9f3b87187 100644 --- a/tests/unit/test_requirements.py +++ b/tests/unit/test_requirements.py @@ -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([]) diff --git a/tests/unit/test_scheduler.py b/tests/unit/test_scheduler.py index 5c36e6621..3a6544937 100644 --- a/tests/unit/test_scheduler.py +++ b/tests/unit/test_scheduler.py @@ -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 diff --git a/tests/unit/test_zuultrigger.py b/tests/unit/test_zuultrigger.py index 25bfd4c30..f649f4723 100644 --- a/tests/unit/test_zuultrigger.py +++ b/tests/unit/test_zuultrigger.py @@ -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 diff --git a/zuul/configloader.py b/zuul/configloader.py index 08b517618..7f8346382 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -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 diff --git a/zuul/manager/__init__.py b/zuul/manager/__init__.py index aec996f31..60eb479e0 100644 --- a/zuul/manager/__init__.py +++ b/zuul/manager/__init__.py @@ -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: diff --git a/zuul/manager/independent.py b/zuul/manager/independent.py index aeeff5adc..ca52b37fe 100644 --- a/zuul/manager/independent.py +++ b/zuul/manager/independent.py @@ -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 diff --git a/zuul/model.py b/zuul/model.py index ac38c3705..0d789f976 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -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 |