zuul/zuul
Code Issues Proposed changes

Merge "Detect errors with non-permitted parent jobs"

Browse Source
This commit is contained in:
Zuul 2022-11-15 14:03:38 +00:00 committed by Gerrit Code Review
commit 4d555ca675
5 changed files with 155 additions and 41 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

12
releasenotes/notes/non-final-parents-f7a2226862738507.yaml Normal file
View File

@ -0,0 +1,12 @@
---
upgrade:
- |
Additional syntax checking is performed on job configuration
changes so that Zuul will report an error if a job inherits from a
non-permitted parent (such as a final job, intermediate job when
the child is not abstract, or a protected job in another project).
Previously, these situations might only be discovered when
freezing a job graph.
If any jobs currently in this situation exist, they will be
reported as configuration errors.

10
tests/fixtures/config/protected/git/common-config/zuul.yaml vendored
View File

@ -14,3 +14,13 @@
- job:
name: base
parent: null
- project:
name: org/project
check:
jobs: []
- project:
name: org/project1
check:
jobs: []

30
tests/fixtures/layouts/protected-parent.yaml vendored Normal file
View File

@ -0,0 +1,30 @@
- pipeline:
name: check
manager: independent
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: protected-job
protected: true
- project:
name: org/project
check:
jobs: []

113
tests/unit/test_v3.py
View File

@ -185,9 +185,8 @@ class TestProtected(ZuulTestCase):
self.assertEqual(A.reported, 1)
self.assertEqual(A.patchsets[-1]['approvals'][0]['value'], '-1')
self.assertIn(
"which is defined in review.example.com/org/project is protected "
"and cannot be inherited from other projects.", A.messages[0])
self.assertIn("is a protected job in a different project",
A.messages[0])
class TestAbstract(ZuulTestCase):
@ -256,8 +255,7 @@ class TestIntermediate(ZuulTestCase):
self.assertEqual(A.reported, 1)
self.assertEqual(A.patchsets[-1]['approvals'][0]['value'], '-1')
self.assertIn('may only inherit to another abstract job',
A.messages[0])
self.assertIn('is not abstract', A.messages[0])
def test_intermediate_config_fail(self):
# an intermediate job must also be abstract
@ -354,41 +352,6 @@ class TestFinal(ZuulTestCase):
self.assertEqual(A.patchsets[-1]['approvals'][0]['value'], '-1')
self.assertIn('Unable to modify final job', A.messages[0])
def test_final_inheritance(self):
# test misuse of final parent job
in_repo_conf = textwrap.dedent(
"""
- job:
name: project-test
parent: job-final
run: playbooks/project-test.yaml
- project:
name: org/project
check:
jobs:
- project-test
""")
in_repo_playbook = textwrap.dedent(
"""
- hosts: all
tasks: []
""")
file_dict = {'.zuul.yaml': in_repo_conf,
'playbooks/project-test.yaml': in_repo_playbook}
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A',
files=file_dict)
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
# The second patch tried to override some variables.
# Thus it should fail.
self.assertEqual(A.reported, 1)
self.assertEqual(A.patchsets[-1]['approvals'][0]['value'], '-1')
self.assertIn('Unable to modify final job', A.messages[0])
class TestBranchCreation(ZuulTestCase):
tenant_config_file = 'config/one-project/main.yaml'
@ -3063,6 +3026,76 @@ class TestInRepoConfig(ZuulTestCase):
dict(name='test-job', result='SUCCESS', changes='3,1'),
], ordered=True)
def test_final_parent(self):
# If all variants of the parent are final, it is an error.
# This doesn't catch all possibilities (that is handled during
# job freezing) but this may catch most errors earlier.
in_repo_conf = textwrap.dedent(
"""
- job:
name: parent
final: true
- job:
name: project-test1
parent: parent
""")
file_dict = {'.zuul.yaml': in_repo_conf}
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A',
files=file_dict)
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertEqual(A.reported, 1)
self.assertIn('is final and can not act as a parent', A.messages[0])
def test_intermediate_parent(self):
# If all variants of the parent are intermediate and this job
# is not abstract, it is an error.
# This doesn't catch all possibilities (that is handled during
# job freezing) but this may catch most errors earlier.
in_repo_conf = textwrap.dedent(
"""
- job:
name: parent
intermediate: true
abstract: true
- job:
name: project-test1
parent: parent
""")
file_dict = {'.zuul.yaml': in_repo_conf}
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A',
files=file_dict)
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertEqual(A.reported, 1)
self.assertIn('is not abstract', A.messages[0])
@simple_layout('layouts/protected-parent.yaml')
def test_protected_parent(self):
# If a parent is protected, it may only be used by a child in
# the same project.
# This doesn't catch all possibilities (that is handled during
# job freezing) but this may catch most errors earlier.
in_repo_conf = textwrap.dedent(
"""
- job:
name: project-test1
parent: protected-job
""")
file_dict = {'.zuul.yaml': in_repo_conf}
A = self.fake_gerrit.addFakeChange('org/project', 'master', 'A',
files=file_dict)
self.fake_gerrit.addEvent(A.getPatchsetCreatedEvent(1))
self.waitUntilSettled()
self.assertEqual(A.reported, 1)
self.assertIn('protected job in a different project', A.messages[0])
class TestInRepoConfigSOS(ZuulTestCase):
config_file = 'zuul-connections-gerrit-and-github.conf'

31
zuul/model.py
View File

@ -2935,7 +2935,36 @@ class Job(ConfigObject):
# Verify that references to other objects in the layout are
# valid.
if not self.isBase() and self.parent:
layout.getJob(self.parent)
parents = layout.getJobs(self.parent)
if not parents:
raise Exception("Job %s not defined" % (self.parent,))
# For the following checks, we allow some leeway to
# account for the possibility that there may be
# nonconforming job variants that would not match and
# therefore not trigger an error during job graph
# freezing. We only raise an error here if there is no
# possibility of success, which may help prevent errors in
# most cases. If we don't raise an error here, the
# possibility of later failure still remains.
nonfinal_parents = [p for p in parents if not p.final]
if not nonfinal_parents:
raise Exception(
f'The parent of job "{self.name}", "{self.parent}" '
'is final and can not act as a parent')
nonintermediate_parents = [
p for p in parents if not p.intermediate]
if not nonintermediate_parents and not self.abstract:
raise Exception(
f'The parent of job "{self.name}", "{self.parent}" '
f'is intermediate but "{self.name}" is not abstract')
nonprotected_parents = [
p for p in parents if not p.protected]
if (not nonprotected_parents and
parents[0].source_context.project_canonical_name !=
self.source_context.project_canonical_name):
raise Exception(
f'The parent of job "{self.name}", "{self.parent}" '
f'is a protected job in a different project')
for ns in self.flattenNodesetAlternatives(layout):
if layout.tenant.max_nodes_per_job != -1 and \