zuul/zuul
Code Issues Proposed changes

Detect errors with non-permitted parent jobs

Browse Source 
We currently only detect some errors with job parents when freezing
the job graph.  This is due to the vagaries of job variants, where
it is possible for a variant on one branch to be okay while one on
another branch is an error.  If the erroneous job doesn't match,
then there is no harm.

However, in the typical case where there is only one variant or
multiple variants are identical, it is possible for us to detect
during config loading a situation where we know the job graph
generation will later fail.  This change adds that analysis and
raises errors early.

This can save users quite a bit of time, and since variants are
typically added one at a time, may even prevent users from getting
into abiguous situations which could only be detected when freezing
the job graph.

Change-Id: Ie8b9ee7758c94788ee7bc05947ddd97d9fa8e075
This commit is contained in:
James E. Blair 2022-10-19 14:55:20 -07:00
parent 3968de9abd
commit bc16efbecc
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'
@ -2910,6 +2873,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

@ -2848,7 +2848,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 \