From eca4620efa07ca1eb8675eba6ec4ab5b6cb35fb2 Mon Sep 17 00:00:00 2001 From: Tobias Henkel Date: Wed, 2 Aug 2017 20:27:10 +0200 Subject: [PATCH] Optionally limit github to protected branches When using a branch and pull model on a shared repository there are usually one or more protected branches which are gated and a dynamic number of temporary personal/feature branches which are the source for the pull requests. These temporary branches while ungated can potentially include broken zuul config and therefore break the global tenant wide configuration. In order to deal with this model add support for excluding unprotected branches. This can be configured on tenant level and overridden per project. Change-Id: I8a45fd41539a3c964a84142f04c1644585c0fdcf --- doc/source/admin/tenants.rst | 18 +++++++++++- tests/base.py | 5 +++- .../tenant-parser/unprotected-branches.yaml | 11 +++++++ .../git/org_common-config/zuul.yaml | 19 ++++++++++++ .../git/org_project1/README | 1 + .../org_project1/playbooks/project-test.yaml | 2 ++ .../git/org_project1/zuul.yaml | 8 +++++ .../git/org_project2/README | 1 + .../git/org_project2/zuul.yaml | 1 + .../config/unprotected-branches/main.yaml | 10 +++++++ tests/unit/test_configloader.py | 24 +++++++++++++++ tests/unit/test_github_driver.py | 17 +++++++++++ zuul/configloader.py | 29 ++++++++++++++----- zuul/driver/gerrit/gerritconnection.py | 2 +- zuul/driver/gerrit/gerritsource.py | 4 +-- zuul/driver/git/gitconnection.py | 2 +- zuul/driver/git/gitsource.py | 4 +-- zuul/driver/github/githubconnection.py | 14 +++++++-- zuul/driver/github/githubsource.py | 4 +-- zuul/driver/timer/__init__.py | 2 +- zuul/model.py | 5 ++++ zuul/source/__init__.py | 2 +- 22 files changed, 163 insertions(+), 22 deletions(-) create mode 100644 tests/fixtures/config/tenant-parser/unprotected-branches.yaml create mode 100644 tests/fixtures/config/unprotected-branches/git/org_common-config/zuul.yaml create mode 100644 tests/fixtures/config/unprotected-branches/git/org_project1/README create mode 100644 tests/fixtures/config/unprotected-branches/git/org_project1/playbooks/project-test.yaml create mode 100644 tests/fixtures/config/unprotected-branches/git/org_project1/zuul.yaml create mode 100644 tests/fixtures/config/unprotected-branches/git/org_project2/README create mode 100644 tests/fixtures/config/unprotected-branches/git/org_project2/zuul.yaml create mode 100644 tests/fixtures/config/unprotected-branches/main.yaml diff --git a/doc/source/admin/tenants.rst b/doc/source/admin/tenants.rst index 1f8f7dbd87..b3b2d9cef9 100644 --- a/doc/source/admin/tenants.rst +++ b/doc/source/admin/tenants.rst @@ -29,6 +29,7 @@ configuration. An example tenant definition is:: - tenant: name: my-tenant max-nodes-per-job: 5 + exclude-unprotected-branches: false source: gerrit: config-projects: @@ -39,7 +40,8 @@ configuration. An example tenant definition is:: - zuul-jobs: shadow: common-config - project1 - - project2 + - project2: + exclude-unprotected-branches: true The following attributes are supported: @@ -53,6 +55,16 @@ The following attributes are supported: The maximum number of nodes a job can request, default to 5. A '-1' value removes the limit. +**exclude-unprotected-branches** (optional) + When using a branch and pull model on a shared github repository there are + usually one or more protected branches which are gated and a dynamic number of + personal/feature branches which are the source for the pull requests. These + branches can potentially include broken zuul config and therefore break the + global tenant wide configuration. In order to deal with this zuul's operations + can be limited to the protected branches which are gated. This is a tenant + wide setting and can be overridden per project. If not specified, defaults + to ``false``. + **source** (required) A dictionary of sources to consult for projects. A tenant may contain projects from multiple sources; each of those sources must @@ -104,6 +116,10 @@ The following attributes are supported: "zuul-jobs" projects, the definition in "common-config" will be used. + **exclude-unprotected-branches** + Define if unprotected github branches should be processed. Defaults to the + tenant wide setting of exclude-unprotected-branches. + The order of the projects listed in a tenant is important. A job which is defined in one project may not be redefined in another project; therefore, once a job appears in one project, a project diff --git a/tests/base.py b/tests/base.py index eca31fe52e..0f188bd2f0 100755 --- a/tests/base.py +++ b/tests/base.py @@ -567,7 +567,10 @@ class FakeGithub(object): def __init__(self): self._branches = [FakeGithub.FakeBranch()] - def branches(self): + def branches(self, protected=False): + if protected: + # simulate there is no protected branch + return [] return self._branches def user(self, login): diff --git a/tests/fixtures/config/tenant-parser/unprotected-branches.yaml b/tests/fixtures/config/tenant-parser/unprotected-branches.yaml new file mode 100644 index 0000000000..bf2feefcd3 --- /dev/null +++ b/tests/fixtures/config/tenant-parser/unprotected-branches.yaml @@ -0,0 +1,11 @@ +- tenant: + name: tenant-one + exclude-unprotected-branches: true + source: + gerrit: + config-projects: + - common-config: + exclude-unprotected-branches: false + untrusted-projects: + - org/project1 + - org/project2 diff --git a/tests/fixtures/config/unprotected-branches/git/org_common-config/zuul.yaml b/tests/fixtures/config/unprotected-branches/git/org_common-config/zuul.yaml new file mode 100644 index 0000000000..c0fbf0d28a --- /dev/null +++ b/tests/fixtures/config/unprotected-branches/git/org_common-config/zuul.yaml @@ -0,0 +1,19 @@ +- pipeline: + name: check + manager: independent + trigger: + github: + - event: pull_request + action: + - opened + - changed + - reopened + success: + github: + status: 'success' + failure: + github: + status: 'failure' + start: + github: + comment: true diff --git a/tests/fixtures/config/unprotected-branches/git/org_project1/README b/tests/fixtures/config/unprotected-branches/git/org_project1/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/unprotected-branches/git/org_project1/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/unprotected-branches/git/org_project1/playbooks/project-test.yaml b/tests/fixtures/config/unprotected-branches/git/org_project1/playbooks/project-test.yaml new file mode 100644 index 0000000000..f679dceaef --- /dev/null +++ b/tests/fixtures/config/unprotected-branches/git/org_project1/playbooks/project-test.yaml @@ -0,0 +1,2 @@ +- hosts: all + tasks: [] diff --git a/tests/fixtures/config/unprotected-branches/git/org_project1/zuul.yaml b/tests/fixtures/config/unprotected-branches/git/org_project1/zuul.yaml new file mode 100644 index 0000000000..31abadf066 --- /dev/null +++ b/tests/fixtures/config/unprotected-branches/git/org_project1/zuul.yaml @@ -0,0 +1,8 @@ +- job: + name: project-test + +- project: + name: org/project1 + check: + jobs: + - project-test diff --git a/tests/fixtures/config/unprotected-branches/git/org_project2/README b/tests/fixtures/config/unprotected-branches/git/org_project2/README new file mode 100644 index 0000000000..9daeafb986 --- /dev/null +++ b/tests/fixtures/config/unprotected-branches/git/org_project2/README @@ -0,0 +1 @@ +test diff --git a/tests/fixtures/config/unprotected-branches/git/org_project2/zuul.yaml b/tests/fixtures/config/unprotected-branches/git/org_project2/zuul.yaml new file mode 100644 index 0000000000..64d316d31d --- /dev/null +++ b/tests/fixtures/config/unprotected-branches/git/org_project2/zuul.yaml @@ -0,0 +1 @@ +This zuul.yaml is intentionally broken and should not be loaded on startup. diff --git a/tests/fixtures/config/unprotected-branches/main.yaml b/tests/fixtures/config/unprotected-branches/main.yaml new file mode 100644 index 0000000000..8078d37326 --- /dev/null +++ b/tests/fixtures/config/unprotected-branches/main.yaml @@ -0,0 +1,10 @@ +- tenant: + name: tenant-one + source: + github: + config-projects: + - org/common-config + untrusted-projects: + - org/project1 + - org/project2: + exclude-unprotected-branches: true diff --git a/tests/unit/test_configloader.py b/tests/unit/test_configloader.py index d08c6a1eda..1ba4ed94be 100644 --- a/tests/unit/test_configloader.py +++ b/tests/unit/test_configloader.py @@ -182,6 +182,7 @@ class TestTenantGroups3(TenantParserTestCase): def test_tenant_groups3(self): tenant = self.sched.abide.tenants.get('tenant-one') + self.assertEqual(False, tenant.exclude_unprotected_branches) self.assertEqual(['common-config'], [x.name for x in tenant.config_projects]) self.assertEqual(['org/project1', 'org/project2'], @@ -212,6 +213,29 @@ class TestTenantGroups3(TenantParserTestCase): project2_config.pipelines['check'].job_list.jobs) +class TestTenantUnprotectedBranches(TenantParserTestCase): + tenant_config_file = 'config/tenant-parser/unprotected-branches.yaml' + + def test_tenant_unprotected_branches(self): + tenant = self.sched.abide.tenants.get('tenant-one') + self.assertEqual(True, tenant.exclude_unprotected_branches) + + self.assertEqual(['common-config'], + [x.name for x in tenant.config_projects]) + self.assertEqual(['org/project1', 'org/project2'], + [x.name for x in tenant.untrusted_projects]) + + tpc = tenant.project_configs + project_name = tenant.config_projects[0].canonical_name + self.assertEqual(False, tpc[project_name].exclude_unprotected_branches) + + project_name = tenant.untrusted_projects[0].canonical_name + self.assertIsNone(tpc[project_name].exclude_unprotected_branches) + + project_name = tenant.untrusted_projects[1].canonical_name + self.assertIsNone(tpc[project_name].exclude_unprotected_branches) + + class TestSplitConfig(ZuulTestCase): tenant_config_file = 'config/split-config/main.yaml' diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index 0e199dfa82..68fbe29cb4 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -683,3 +683,20 @@ class TestGithubDriver(ZuulTestCase): self.fake_github.emitEvent, ('ping', pevent), ) + + +class TestGithubUnprotectedBranches(ZuulTestCase): + config_file = 'zuul-github-driver.conf' + tenant_config_file = 'config/unprotected-branches/main.yaml' + + def test_unprotected_branches(self): + tenant = self.sched.abide.tenants.get('tenant-one') + + project1 = tenant.untrusted_projects[0] + project2 = tenant.untrusted_projects[1] + + # project1 should have parsed master + self.assertIn('master', project1.unparsed_branch_config.keys()) + + # project2 should have no parsed branch + self.assertEqual(0, len(project2.unparsed_branch_config.keys())) diff --git a/zuul/configloader.py b/zuul/configloader.py index 4b9b8a0051..26684dfe2c 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -929,6 +929,7 @@ class TenantParser(object): 'include': to_list(classes), 'exclude': to_list(classes), 'shadow': to_list(str), + 'exclude-unprotected-branches': bool, }} project = vs.Any(str, project_dict) @@ -965,7 +966,9 @@ class TenantParser(object): def getSchema(connections=None): tenant = {vs.Required('name'): str, 'max-nodes-per-job': int, - 'source': TenantParser.validateTenantSources(connections)} + 'source': TenantParser.validateTenantSources(connections), + 'exclude-unprotected-branches': bool, + } return vs.Schema(tenant) @staticmethod @@ -975,6 +978,10 @@ class TenantParser(object): tenant = model.Tenant(conf['name']) if conf.get('max-nodes-per-job') is not None: tenant.max_nodes_per_job = conf['max-nodes-per-job'] + if conf.get('exclude-unprotected-branches') is not None: + tenant.exclude_unprotected_branches = \ + conf['exclude-unprotected-branches'] + tenant.unparsed_config = conf unparsed_config = model.UnparsedTenantConfig() # tpcs is TenantProjectConfigs @@ -993,7 +1000,7 @@ class TenantParser(object): TenantParser._loadTenantInRepoLayouts(merger, connections, tenant.config_projects, tenant.untrusted_projects, - cached) + cached, tenant) unparsed_config.extend(tenant.config_projects_config) unparsed_config.extend(tenant.untrusted_projects_config) tenant.layout = TenantParser._parseLayout(base, tenant, @@ -1065,6 +1072,7 @@ class TenantParser(object): project = source.getProject(conf) project_include = current_include shadow_projects = [] + project_exclude_unprotected_branches = None else: project_name = list(conf.keys())[0] project = source.getProject(project_name) @@ -1078,10 +1086,14 @@ class TenantParser(object): as_list(conf[project_name].get('exclude', []))) if project_exclude: project_include = frozenset(project_include - project_exclude) + project_exclude_unprotected_branches = conf[project_name].get( + 'exclude-unprotected-branches', None) tenant_project_config = model.TenantProjectConfig(project) tenant_project_config.load_classes = frozenset(project_include) tenant_project_config.shadow_projects = shadow_projects + tenant_project_config.exclude_unprotected_branches = \ + project_exclude_unprotected_branches return tenant_project_config @@ -1148,7 +1160,7 @@ class TenantParser(object): @staticmethod def _loadTenantInRepoLayouts(merger, connections, config_projects, - untrusted_projects, cached): + untrusted_projects, cached, tenant): config_projects_config = model.UnparsedTenantConfig() untrusted_projects_config = model.UnparsedTenantConfig() jobs = [] @@ -1196,7 +1208,7 @@ class TenantParser(object): # branch. Remember the branch and then implicitly add a # branch selector to each job there. This makes the # in-repo configuration apply only to that branch. - for branch in project.source.getProjectBranches(project): + for branch in project.source.getProjectBranches(project, tenant): project.unparsed_branch_config[branch] = \ model.UnparsedTenantConfig() job = merger.getFiles( @@ -1416,11 +1428,11 @@ class ConfigLoader(object): new_abide.tenants[tenant.name] = new_tenant return new_abide - def _loadDynamicProjectData(self, config, project, files, trusted): + def _loadDynamicProjectData(self, config, project, files, trusted, tenant): if trusted: branches = ['master'] else: - branches = project.source.getProjectBranches(project) + branches = project.source.getProjectBranches(project, tenant) for branch in branches: fns1 = [] @@ -1472,11 +1484,12 @@ class ConfigLoader(object): if include_config_projects: config = model.UnparsedTenantConfig() for project in tenant.config_projects: - self._loadDynamicProjectData(config, project, files, True) + self._loadDynamicProjectData( + config, project, files, True, tenant) else: config = tenant.config_projects_config.copy() for project in tenant.untrusted_projects: - self._loadDynamicProjectData(config, project, files, False) + self._loadDynamicProjectData(config, project, files, False, tenant) layout = model.Layout(tenant) # NOTE: the actual pipeline objects (complete with queues and diff --git a/zuul/driver/gerrit/gerritconnection.py b/zuul/driver/gerrit/gerritconnection.py index d23857f2bd..6bf43d64f0 100644 --- a/zuul/driver/gerrit/gerritconnection.py +++ b/zuul/driver/gerrit/gerritconnection.py @@ -616,7 +616,7 @@ class GerritConnection(BaseConnection): (record.get('number'),)) return changes - def getProjectBranches(self, project: Project) -> List[str]: + def getProjectBranches(self, project: Project, tenant) -> List[str]: refs = self.getInfoRefs(project) heads = [str(k[len('refs/heads/'):]) for k in refs.keys() if k.startswith('refs/heads/')] diff --git a/zuul/driver/gerrit/gerritsource.py b/zuul/driver/gerrit/gerritsource.py index e41859eb8e..7141080ac0 100644 --- a/zuul/driver/gerrit/gerritsource.py +++ b/zuul/driver/gerrit/gerritsource.py @@ -54,8 +54,8 @@ class GerritSource(BaseSource): def getProjectOpenChanges(self, project): return self.connection.getProjectOpenChanges(project) - def getProjectBranches(self, project): - return self.connection.getProjectBranches(project) + def getProjectBranches(self, project, tenant): + return self.connection.getProjectBranches(project, tenant) def getGitUrl(self, project): return self.connection.getGitUrl(project) diff --git a/zuul/driver/git/gitconnection.py b/zuul/driver/git/gitconnection.py index f4fe7e5efb..0624088634 100644 --- a/zuul/driver/git/gitconnection.py +++ b/zuul/driver/git/gitconnection.py @@ -48,7 +48,7 @@ class GitConnection(BaseConnection): def addProject(self, project): self.projects[project.name] = project - def getProjectBranches(self, project): + def getProjectBranches(self, project, tenant): # TODO(jeblair): implement; this will need to handle local or # remote git urls. raise NotImplemented() diff --git a/zuul/driver/git/gitsource.py b/zuul/driver/git/gitsource.py index 61a328e650..8d85c082fe 100644 --- a/zuul/driver/git/gitsource.py +++ b/zuul/driver/git/gitsource.py @@ -45,8 +45,8 @@ class GitSource(BaseSource): self.connection.addProject(p) return p - def getProjectBranches(self, project): - return self.connection.getProjectBranches(project) + def getProjectBranches(self, project, tenant): + return self.connection.getProjectBranches(project, tenant) def getGitUrl(self, project): return self.connection.getGitUrl(project) diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index 80ac57395e..616e774271 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -698,11 +698,21 @@ class GithubConnection(BaseConnection): def addProject(self, project): self.projects[project.name] = project - def getProjectBranches(self, project): + def getProjectBranches(self, project, tenant): + + # Evaluate if unprotected branches should be excluded or not. The first + # match wins. The order is project -> tenant (default is false). + project_config = tenant.project_configs.get(project.canonical_name) + if project_config.exclude_unprotected_branches is not None: + exclude_unprotected = project_config.exclude_unprotected_branches + else: + exclude_unprotected = tenant.exclude_unprotected_branches + github = self.getGithubClient() owner, proj = project.name.split('/') repository = github.repository(owner, proj) - branches = [branch.name for branch in repository.branches()] + branches = [branch.name for branch in repository.branches( + protected=exclude_unprotected)] log_rate_limit(self.log, github) return branches diff --git a/zuul/driver/github/githubsource.py b/zuul/driver/github/githubsource.py index 1bd280fb71..1e7e07a88f 100644 --- a/zuul/driver/github/githubsource.py +++ b/zuul/driver/github/githubsource.py @@ -68,8 +68,8 @@ class GithubSource(BaseSource): self.connection.addProject(p) return p - def getProjectBranches(self, project): - return self.connection.getProjectBranches(project) + def getProjectBranches(self, project, tenant): + return self.connection.getProjectBranches(project, tenant) def getProjectOpenChanges(self, project): """Get the open changes for a project.""" diff --git a/zuul/driver/timer/__init__.py b/zuul/driver/timer/__init__.py index 4489808f04..69cd508746 100644 --- a/zuul/driver/timer/__init__.py +++ b/zuul/driver/timer/__init__.py @@ -81,7 +81,7 @@ class TimerDriver(Driver, TriggerInterface): def _onTrigger(self, tenant, pipeline_name, timespec): for project_name in tenant.layout.project_configs.keys(): (trusted, project) = tenant.getProject(project_name) - for branch in project.source.getProjectBranches(project): + for branch in project.source.getProjectBranches(project, tenant): event = TimerTriggerEvent() event.type = 'timer' event.timespec = timespec diff --git a/zuul/model.py b/zuul/model.py index 9be8745436..26a79636f4 100644 --- a/zuul/model.py +++ b/zuul/model.py @@ -2091,6 +2091,10 @@ class TenantProjectConfig(object): self.load_classes = set() self.shadow_projects = set() + # The tenant's default setting of exclude_unprotected_branches will + # be overridden by this one if not None. + self.exclude_unprotected_branches = None + class ProjectConfig(object): # Represents a project cofiguration @@ -2451,6 +2455,7 @@ class Tenant(object): def __init__(self, name): self.name = name self.max_nodes_per_job = 5 + self.exclude_unprotected_branches = False self.layout = None # The unparsed configuration from the main zuul config for # this tenant. diff --git a/zuul/source/__init__.py b/zuul/source/__init__.py index b37aeb4d1d..0396aff499 100644 --- a/zuul/source/__init__.py +++ b/zuul/source/__init__.py @@ -64,7 +64,7 @@ class BaseSource(object, metaclass=abc.ABCMeta): """Get a project.""" @abc.abstractmethod - def getProjectBranches(self, project): + def getProjectBranches(self, project, tenant): """Get branches for a project""" @abc.abstractmethod