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 <jamielennox@gmail.com>
Co-authored-by: Tobias Henkel <tobias.henkel@bmw.de>

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.

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)

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

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'

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)

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.
 

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):

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()

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

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({

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):

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):

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):

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]