Don't match branch protection rule patterns locally

Branch protection rules in github are fn patterns which are currently
matched locally in zuul. This is error prone and can lead in edge
cases to wrong matches resulting in wrong enqueue decisions into gate
pipelines. When requesting branch protection rules in github we also
can request the matching refs along with the rules. This is much safer
since we can plain text match them against the change branch.

Change-Id: Ic995d4b2e16a5d741f0209fa9236959d8f4d10b9

6 files changed, 51 insertions, 12 deletions

diff --git a/requirements.txt b/requirements.txt
index 019bd352a..20778a1fd 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -32,7 +32,6 @@ paho-mqtt
 cherrypy
 ws4py
 routes
-pathspec
 jsonpath-rw
 urllib3!=1.25.4,!=1.25.5  # https://github.com/urllib3/urllib3/pull/1684
 cheroot!=8.1.*,!=8.2.*,!=8.3.0 # https://github.com/cherrypy/cheroot/issues/263
diff --git a/tests/fake_graphql.py b/tests/fake_graphql.py
index 6eaa965ba..9e86f81ad 100644
--- a/tests/fake_graphql.py
+++ b/tests/fake_graphql.py
@@ -26,11 +26,28 @@ class FakePageInfo(ObjectType):
         return False
 
 
+class FakeMatchingRef(ObjectType):
+    name = String()
+
+    def resolve_name(parent, info):
+        return parent
+
+
+class FakeMatchingRefs(ObjectType):
+    nodes = List(FakeMatchingRef)
+
+    def resolve_nodes(parent, info):
+        # To simplify tests just return the pattern and a bogus ref that should
+        # not disturb zuul.
+        return [parent.pattern, 'bogus-ref']
+
+
 class FakeBranchProtectionRule(ObjectType):
     pattern = String()
     requiredStatusCheckContexts = List(String)
     requiresApprovingReviews = Boolean()
     requiresCodeOwnerReviews = Boolean()
+    matchingRefs = Field(FakeMatchingRefs, first=Int())
 
     def resolve_pattern(parent, info):
         return parent.pattern
@@ -44,6 +61,9 @@ class FakeBranchProtectionRule(ObjectType):
     def resolve_requiresCodeOwnerReviews(parent, info):
         return parent.require_codeowners_review
 
+    def resolve_matchingRefs(parent, info, first=None):
+        return parent
+
 
 class FakeBranchProtectionRules(ObjectType):
     nodes = List(FakeBranchProtectionRule)
diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py
index df4c013d4..4290f7337 100644
--- a/zuul/driver/github/githubconnection.py
+++ b/zuul/driver/github/githubconnection.py
@@ -1635,7 +1635,8 @@ class GithubConnection(BaseConnection):
 
         # For performance reasons fetch all needed data for canMerge upfront
         # using a single graphql call.
-        canmerge_data = self.graphql_client.fetch_canmerge(github, change)
+        canmerge_data = self.graphql_client.fetch_canmerge(
+            github, change, zuul_event_id=event)
 
         # If the PR is a draft it cannot be merged.
         if canmerge_data.get('isDraft', False):
diff --git a/zuul/driver/github/graphql/__init__.py b/zuul/driver/github/graphql/__init__.py
index 191c8e1e7..1704d7efc 100644
--- a/zuul/driver/github/graphql/__init__.py
+++ b/zuul/driver/github/graphql/__init__.py
@@ -13,9 +13,10 @@
 # under the License.
 import logging
 
-import pathspec
 from pkg_resources import resource_string
 
+from zuul.lib.logutil import get_annotated_logger
+
 
 def nested_get(d, *keys, default=None):
     temp = d
@@ -70,7 +71,8 @@ class GraphQLClient:
         response = github.session.post(self.url, json=query)
         return response.json()
 
-    def fetch_canmerge(self, github, change):
+    def fetch_canmerge(self, github, change, zuul_event_id=None):
+        log = get_annotated_logger(self.log, zuul_event_id)
         owner, repo = change.project.name.split('/')
 
         data = self._fetch_canmerge(github, owner, repo, change.number,
@@ -81,14 +83,21 @@ class GraphQLClient:
         # Find corresponding rule to our branch
         rules = nested_get(repository, 'branchProtectionRules', 'nodes',
                            default=[])
-        matching_rule = None
-        for rule in rules:
-            pattern = pathspec.patterns.GitWildMatchPattern(
-                rule.get('pattern'))
-            match = pathspec.match_files([pattern], [change.branch])
-            if match:
-                matching_rule = rule
-                break
+
+        # Filter branch protection rules for the one matching the change.
+        matching_rules = [
+            rule for rule in rules
+            for ref in nested_get(rule, 'matchingRefs', 'nodes', default=[])
+            if ref.get('name') == change.branch
+        ]
+        if len(matching_rules) > 1:
+            log.warn('More than one branch protection rules match change %s',
+                     change)
+            return result
+        elif len(matching_rules) == 1:
+            matching_rule = matching_rules[0]
+        else:
+            matching_rule = None
 
         # If there is a matching rule, get required status checks
         if matching_rule:
diff --git a/zuul/driver/github/graphql/canmerge-legacy.graphql b/zuul/driver/github/graphql/canmerge-legacy.graphql
index 4bddd73dc..edc1b0fcd 100644
--- a/zuul/driver/github/graphql/canmerge-legacy.graphql
+++ b/zuul/driver/github/graphql/canmerge-legacy.graphql
@@ -11,6 +11,11 @@ query canMergeData(
         requiredStatusCheckContexts
         requiresApprovingReviews
         requiresCodeOwnerReviews
+        matchingRefs(first: 100) {
+          nodes {
+            name
+          }
+        }
       }
     }
     pullRequest(number: $pull) {
diff --git a/zuul/driver/github/graphql/canmerge.graphql b/zuul/driver/github/graphql/canmerge.graphql
index ee3cd8fd3..baa4a9f81 100644
--- a/zuul/driver/github/graphql/canmerge.graphql
+++ b/zuul/driver/github/graphql/canmerge.graphql
@@ -11,6 +11,11 @@ query canMergeData(
         requiredStatusCheckContexts
         requiresApprovingReviews
         requiresCodeOwnerReviews
+        matchingRefs(first: 100) {
+          nodes {
+            name
+          }
+        }
       }
     }
     pullRequest(number: $pull) {