From 0445d03542d66bbe2e337d4dad3a267331de6156 Mon Sep 17 00:00:00 2001 From: Tobias Henkel Date: Thu, 24 May 2018 08:34:00 -0700 Subject: [PATCH] Status branch protection checking for github The github provider was doing a very naive check for whether a PR was able to be merged, simply checking if there was a current merge conflict. We also want to make sure that if there are any branch protection requirements these are also checked because if zuul tries to merge a patch via github that doesn't meet the branch protection it will be rejected and cause zuul to report a failure to users. Change-Id: I66d54c2603c462cb029510dd4e37fc89afeb200d Signed-off-by: Jamie Lennox Co-authored-by: Tobias Henkel --- .../branch-protection-f79d97c4e6c0b05f.yaml | 7 ++ tests/fakegithub.py | 94 ++++++++++++++++++- tests/fixtures/layouts/gate-github.yaml | 37 ++++++++ tests/unit/test_github_driver.py | 26 +++++ zuul/configloader.py | 2 +- zuul/driver/__init__.py | 4 +- zuul/driver/gerrit/__init__.py | 2 +- zuul/driver/github/__init__.py | 9 +- zuul/driver/github/githubconnection.py | 90 ++++++++++++++++-- zuul/driver/github/githubreporter.py | 24 ++++- zuul/driver/mqtt/__init__.py | 2 +- zuul/driver/smtp/__init__.py | 2 +- zuul/driver/sql/__init__.py | 2 +- zuul/lib/connections.py | 4 +- 14 files changed, 274 insertions(+), 31 deletions(-) create mode 100644 releasenotes/notes/branch-protection-f79d97c4e6c0b05f.yaml create mode 100644 tests/fixtures/layouts/gate-github.yaml diff --git a/releasenotes/notes/branch-protection-f79d97c4e6c0b05f.yaml b/releasenotes/notes/branch-protection-f79d97c4e6c0b05f.yaml new file mode 100644 index 0000000000..479473db5e --- /dev/null +++ b/releasenotes/notes/branch-protection-f79d97c4e6c0b05f.yaml @@ -0,0 +1,7 @@ +--- +features: + - | + The GitHub driver can determine the required status checks of pull requests + which are needed for entering a gate pipeline. This eliminates the need to + hard code required status checks in the gate pipeline and makes + interoperation with other GitHub apps much more flexible. diff --git a/tests/fakegithub.py b/tests/fakegithub.py index 2afb2ba9e5..ce14026a3a 100644 --- a/tests/fakegithub.py +++ b/tests/fakegithub.py @@ -16,6 +16,8 @@ import re +FAKE_BASE_URL = 'https://example.com/api/v3/' + class FakeUser(object): def __init__(self, login): @@ -31,18 +33,18 @@ class FakeBranch(object): class FakeStatus(object): def __init__(self, state, url, description, context, user): - self._state = state + self.state = state + self.context = context self._url = url self._description = description - self._context = context self._user = user def as_dict(self): return { - 'state': self._state, + 'state': self.state, 'url': self._url, 'description': self._description, - 'context': self._context, + 'context': self.context, 'creator': { 'login': self._user } @@ -67,6 +69,7 @@ class FakeCommit(object): class FakeRepository(object): def __init__(self, name, data): + self._api = FAKE_BASE_URL self._branches = [FakeBranch()] self._commits = {} self.data = data @@ -78,6 +81,16 @@ class FakeRepository(object): return [] return self._branches + def _build_url(self, *args, **kwargs): + path_args = ['repos', self.name] + path_args.extend(args) + fakepath = '/'.join(path_args) + return FAKE_BASE_URL + fakepath + + def _get(self, url, headers=None): + client = FakeGithubClient(self.data) + return client.session.get(url, headers) + def create_status(self, sha, state, url, description, context, user='zuul'): # Since we're bypassing github API, which would require a user, we @@ -95,6 +108,31 @@ class FakeRepository(object): self._commits[sha] = commit return commit + def get_url(self, path): + entity, request = path.split('/', 1) + + if entity == 'branches': + return self.get_url_branch(request) + else: + return None + + def get_url_branch(self, path): + branch, entity = path.split('/') + + if entity == 'protection': + return self.get_url_protection(branch) + else: + return None + + def get_url_protection(self, branch): + contexts = self.data.required_contexts.get((self.name, branch), []) + data = { + 'required_status_checks': { + 'contexts': contexts + } + } + return FakeResponse(data) + def pull_requests(self, state=None): pulls = [] for pull in self.data.pull_requests.values(): @@ -145,6 +183,11 @@ class FakePull(object): repo = client.repo_from_project(self._fake_pull_request.project) return repo.commit(self._fake_pull_request.head_sha) + def commits(self): + # since we don't know all commits of a pr we just return here a list + # with the head_sha as the only commit + return [self.head] + def as_dict(self): pr = self._fake_pull_request connection = pr.github @@ -180,16 +223,59 @@ class FakeIssueSearchResult(object): self.issue = issue +class FakeResponse(object): + def __init__(self, data): + self.status_code = 200 + self.data = data + + def json(self): + return self.data + + +class FakeGithubSession(object): + + def __init__(self, data): + self._data = data + + def build_url(self, *args): + fakepath = '/'.join(args) + return FAKE_BASE_URL + fakepath + + def get(self, url, headers=None): + request = url + if request.startswith(FAKE_BASE_URL): + request = request[len(FAKE_BASE_URL):] + + entity, request = request.split('/', 1) + + if entity == 'repos': + return self.get_repo(request) + else: + # unknown entity to process + return None + + def get_repo(self, request): + org, project, request = request.split('/', 2) + project_name = '{}/{}'.format(org, project) + + client = FakeGithubClient(self._data) + repo = client.repo_from_project(project_name) + + return repo.get_url(request) + + class FakeGithubData(object): def __init__(self, pull_requests): self.pull_requests = pull_requests self.repos = {} + self.required_contexts = {} class FakeGithubClient(object): def __init__(self, data, inst_id=None): self._data = data self._inst_id = inst_id + self.session = FakeGithubSession(data) def user(self, login): return FakeUser(login) diff --git a/tests/fixtures/layouts/gate-github.yaml b/tests/fixtures/layouts/gate-github.yaml new file mode 100644 index 0000000000..2946be179d --- /dev/null +++ b/tests/fixtures/layouts/gate-github.yaml @@ -0,0 +1,37 @@ +- pipeline: + name: gate + manager: dependent + trigger: + github: + - event: pull_request + action: + - opened + - changed + - reopened + branch: ^master$ + success: + github: + status: success + merge: true + failure: + github: {} + +- job: + name: base + parent: null + run: playbooks/base.yaml + +- job: + name: project-test1 + run: playbooks/project-test1.yaml + +- job: + name: project-test2 + run: playbooks/project-test2.yaml + +- project: + name: org/project + gate: + jobs: + - project-test1 + - project-test2 diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py index d48b685ecd..acc8010c5d 100644 --- a/tests/unit/test_github_driver.py +++ b/tests/unit/test_github_driver.py @@ -771,6 +771,32 @@ class TestGithubDriver(ZuulTestCase): ('ping', pevent), ) + @simple_layout('layouts/gate-github.yaml', driver='github') + def test_status_checks(self): + github = self.fake_github.getGithubClient() + github._data.required_contexts[('org/project', 'master')] = [ + 'tenant-one/check', + 'tenant-one/gate'] + + A = self.fake_github.openFakePullRequest('org/project', 'master', 'A') + self.fake_github.emitEvent(A.getPullRequestOpenedEvent()) + self.waitUntilSettled() + + # since the required status 'tenant-one/check' is not fulfilled no + # job is expected + self.assertEqual(0, len(self.history)) + + # now set the required status 'tenant-one/check' + repo = github.repo_from_project('org/project') + repo.create_status(A.head_sha, 'success', 'example.com', 'description', + 'tenant-one/check') + + self.fake_github.emitEvent(A.getPullRequestOpenedEvent()) + self.waitUntilSettled() + + # the change should have entered the gate + self.assertEqual(2, len(self.history)) + class TestGithubUnprotectedBranches(ZuulTestCase): config_file = 'zuul-github-driver.conf' diff --git a/zuul/configloader.py b/zuul/configloader.py index bf6dbe4bf1..c7498cc326 100644 --- a/zuul/configloader.py +++ b/zuul/configloader.py @@ -1088,7 +1088,7 @@ class PipelineParser(object): for reporter_name, params \ in conf.get(conf_key).items(): reporter = self.pcontext.connections.getReporter( - reporter_name, params) + reporter_name, pipeline, params) reporter.setAction(conf_key) reporter_set.append(reporter) setattr(pipeline, action, reporter_set) diff --git a/zuul/driver/__init__.py b/zuul/driver/__init__.py index 682b251abe..8b99c7f071 100644 --- a/zuul/driver/__init__.py +++ b/zuul/driver/__init__.py @@ -219,7 +219,7 @@ class ReporterInterface(object, metaclass=abc.ABCMeta): """ @abc.abstractmethod - def getReporter(self, connection, config=None): + def getReporter(self, connection, pipeline, config=None): """Create and return a new Reporter object. This method is required by the interface. @@ -227,6 +227,8 @@ class ReporterInterface(object, metaclass=abc.ABCMeta): :arg Connection connection: The Connection object associated with the reporter (as previously returned by getConnection) or None. + :arg Pipeline pipeline: The pipeline object associated with the + reporter. :arg dict config: The configuration information supplied along with the reporter in the layout. diff --git a/zuul/driver/gerrit/__init__.py b/zuul/driver/gerrit/__init__.py index 76ab5b7671..c720ba6a34 100644 --- a/zuul/driver/gerrit/__init__.py +++ b/zuul/driver/gerrit/__init__.py @@ -33,7 +33,7 @@ class GerritDriver(Driver, ConnectionInterface, TriggerInterface, def getSource(self, connection): return gerritsource.GerritSource(self, connection) - def getReporter(self, connection, config=None): + def getReporter(self, connection, pipeline, config=None): return gerritreporter.GerritReporter(self, connection, config) def getTriggerSchema(self): diff --git a/zuul/driver/github/__init__.py b/zuul/driver/github/__init__.py index f75e907209..919a521115 100644 --- a/zuul/driver/github/__init__.py +++ b/zuul/driver/github/__init__.py @@ -13,7 +13,7 @@ # under the License. from zuul.driver import Driver, ConnectionInterface, TriggerInterface -from zuul.driver import SourceInterface +from zuul.driver import SourceInterface, ReporterInterface from zuul.driver.github import githubconnection from zuul.driver.github import githubtrigger from zuul.driver.github import githubsource @@ -21,7 +21,7 @@ from zuul.driver.github import githubreporter class GithubDriver(Driver, ConnectionInterface, TriggerInterface, - SourceInterface): + SourceInterface, ReporterInterface): name = 'github' def getConnection(self, name, config): @@ -33,8 +33,9 @@ class GithubDriver(Driver, ConnectionInterface, TriggerInterface, def getSource(self, connection): return githubsource.GithubSource(self, connection) - def getReporter(self, connection, config=None): - return githubreporter.GithubReporter(self, connection, config) + def getReporter(self, connection, pipeline, config=None): + return githubreporter.GithubReporter( + self, connection, pipeline, config) def getTriggerSchema(self): return githubtrigger.getSchema() diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py index a872e077bb..ab779576f8 100644 --- a/zuul/driver/github/githubconnection.py +++ b/zuul/driver/github/githubconnection.py @@ -937,16 +937,35 @@ class GithubConnection(BaseConnection): return pr def canMerge(self, change, allow_needs): - # This API call may get a false (null) while GitHub is calculating - # if it can merge. The github3.py library will just return that as - # false. This could lead to false negatives. - # Additionally, this only checks if the PR code could merge - # cleanly to the target branch. It does not evaluate any branch - # protection merge requirements (such as reviews and status states) - # At some point in the future this may be available through the API - # or we can fetch the branch protection settings and evaluate within - # Zuul whether or not those protections have been met - # For now, just send back a True value. + # NOTE: The mergeable call may get a false (null) while GitHub is + # calculating if it can merge. The github3.py library will just return + # that as false. This could lead to false negatives. So don't do this + # call here and only evaluate branch protection settings. Any merge + # conflicts which would block merging finally will be detected by + # the zuul-mergers anyway. + + github = self.getGithubClient(change.project.name) + owner, proj = change.project.name.split('/') + pull = github.pull_request(owner, proj, change.number) + + protection = self._getBranchProtection( + change.project.name, change.branch) + + if not self._hasRequiredStatusChecks(allow_needs, protection, pull): + return False + + required_reviews = protection.get( + 'required_pull_request_reviews') + if required_reviews: + if required_reviews.get('require_code_owner_reviews'): + # we need to process the reviews using code owners + # TODO(tobiash): not implemented yet + pass + else: + # we need to process the review using access rights + # TODO(tobiash): not implemented yet + pass + return True def getPullBySha(self, sha, project): @@ -1020,6 +1039,57 @@ class GithubConnection(BaseConnection): return reviews.values() + def _getBranchProtection(self, project_name: str, branch: str): + github = self.getGithubClient(project_name) + url = github.session.build_url('repos', project_name, + 'branches', branch, + 'protection') + + headers = {'Accept': 'application/vnd.github.loki-preview+json'} + resp = github.session.get(url, headers=headers) + + if resp.status_code == 404: + return None + + return resp.json() + + def _hasRequiredStatusChecks(self, allow_needs, protection, pull): + if not protection: + # There are no protection settings -> ok by definition + return True + + required_contexts = protection.get( + 'required_status_checks', {}).get('contexts') + + if not required_contexts: + # There are no required contexts -> ok by definition + return True + + # Strip allow_needs as we will set this in the gate ourselves + required_contexts = set( + [x for x in required_contexts if x not in allow_needs]) + + # NOTE(tobiash): We cannot just take the last commit in the list + # because it is not sorted that the head is the last one in every case. + # E.g. when doing a re-merge from the target the PR head can be + # somewhere in the middle of the commit list. Thus we need to search + # the whole commit list for the PR head commit which has the statuses + # attached. + commits = list(pull.commits()) + commit = None + for c in commits: + if c.sha == pull.head.sha: + commit = c + break + + # Get successful statuses + successful = set( + [s.context for s in commit.statuses() if s.state == 'success']) + + # Required contexts must be a subset of the successful contexts as + # we allow additional successful status contexts we don't care about. + return required_contexts.issubset(successful) + def _getPullReviews(self, owner, project, number): # make a list out of the reviews so that we complete our # API transaction diff --git a/zuul/driver/github/githubreporter.py b/zuul/driver/github/githubreporter.py index 57b594b1f4..46de7f0f11 100644 --- a/zuul/driver/github/githubreporter.py +++ b/zuul/driver/github/githubreporter.py @@ -28,7 +28,7 @@ class GithubReporter(BaseReporter): name = 'github' log = logging.getLogger("zuul.GithubReporter") - def __init__(self, driver, connection, config=None): + def __init__(self, driver, connection, pipeline, config=None): super(GithubReporter, self).__init__(driver, connection, config) self._commit_status = self.config.get('status', None) self._create_comment = self.config.get('comment', True) @@ -39,6 +39,7 @@ class GithubReporter(BaseReporter): self._unlabels = self.config.get('unlabel', []) if not isinstance(self._unlabels, list): self._unlabels = [self._unlabels] + self.context = "{}/{}".format(pipeline.tenant_name, pipeline.name) def report(self, item): """Report on an event.""" @@ -98,8 +99,6 @@ class GithubReporter(BaseReporter): sha = item.change.patchset elif hasattr(item.change, 'newrev'): sha = item.change.newrev - context = '%s/%s' % (item.pipeline.layout.tenant.name, - item.pipeline.name) state = self._commit_status url_pattern = self.config.get('status-url') @@ -123,10 +122,10 @@ class GithubReporter(BaseReporter): 'Reporting change %s, params %s, ' 'context: %s, state: %s, description: %s, url: %s' % (item.change, self.config, - context, state, description, url)) + self.context, state, description, url)) self.connection.setCommitStatus( - project, sha, state, url, description, context) + project, sha, state, url, description, self.context) def mergePull(self, item): project = item.change.project.name @@ -192,6 +191,21 @@ class GithubReporter(BaseReporter): return message + def getSubmitAllowNeeds(self): + """Get a list of code review labels that are allowed to be + "needed" in the submit records for a change, with respect + to this queue. In other words, the list of review labels + this reporter itself is likely to set before submitting. + """ + + # check if we report a status, if not we can return an empty list + status = self.config.get('status') + if not status: + return [] + + # we return a status so return the status we report to github + return [self.context] + def getSchema(): github_reporter = v.Schema({ diff --git a/zuul/driver/mqtt/__init__.py b/zuul/driver/mqtt/__init__.py index cfe9f7514c..5111b15bda 100644 --- a/zuul/driver/mqtt/__init__.py +++ b/zuul/driver/mqtt/__init__.py @@ -23,7 +23,7 @@ class MQTTDriver(Driver, ConnectionInterface, ReporterInterface): def getConnection(self, name, config): return mqttconnection.MQTTConnection(self, name, config) - def getReporter(self, connection, config=None): + def getReporter(self, connection, pipeline, config=None): return mqttreporter.MQTTReporter(self, connection, config) def getReporterSchema(self): diff --git a/zuul/driver/smtp/__init__.py b/zuul/driver/smtp/__init__.py index b914c81ea2..693c5a33e1 100644 --- a/zuul/driver/smtp/__init__.py +++ b/zuul/driver/smtp/__init__.py @@ -23,7 +23,7 @@ class SMTPDriver(Driver, ConnectionInterface, ReporterInterface): def getConnection(self, name, config): return smtpconnection.SMTPConnection(self, name, config) - def getReporter(self, connection, config=None): + def getReporter(self, connection, pipeline, config=None): return smtpreporter.SMTPReporter(self, connection, config) def getReporterSchema(self): diff --git a/zuul/driver/sql/__init__.py b/zuul/driver/sql/__init__.py index dbb70ea254..3c91b52248 100644 --- a/zuul/driver/sql/__init__.py +++ b/zuul/driver/sql/__init__.py @@ -47,7 +47,7 @@ class SQLDriver(Driver, ConnectionInterface, ReporterInterface): def getConnection(self, name, config): return sqlconnection.SQLConnection(self, name, config) - def getReporter(self, connection, config=None): + def getReporter(self, connection, pipeline, config=None): return sqlreporter.SQLReporter(self, connection, config) def getReporterSchema(self): diff --git a/zuul/lib/connections.py b/zuul/lib/connections.py index 29eb03ff21..1320e4cf15 100644 --- a/zuul/lib/connections.py +++ b/zuul/lib/connections.py @@ -158,9 +158,9 @@ class ConnectionRegistry(object): sources.append(connection.driver.getSource(connection)) return sources - def getReporter(self, connection_name, config=None): + def getReporter(self, connection_name, pipeline, config=None): connection = self.connections[connection_name] - return connection.driver.getReporter(connection, config) + return connection.driver.getReporter(connection, pipeline, config) def getTrigger(self, connection_name, config=None): connection = self.connections[connection_name]