Fix race related to PR with changed base branch

Some people use a workflow that's known as "stacked pull requests" in
order to split a change into more reviewable chunks.

In this workflow the first PR in the stack targets a protected branch
(e.g. master) and all other PRs target the unprotected branch of the
next item in the stack.

    E.g. master <- feature-A (PR#1) <- feature-B (PR#2) <- ...

Now, when the first PR in the stack is merged Github will automatically
change the target branch of dependent PRs. For the above example this
would look like the following after PR#1 is merged:

    master <- feature-B (PR#2) <- ...

The problem here is that we might still process events for a PR before
the base branch change, but the Github API already returns the info
about the updated target branch.

The problem with this is, that we used the target branch name from the
event (outdated branch name) and and the information from the change
object (new target branch) whether or not the target branch is protected
to determine if a branch was configured as protected.

In the above example Zuul might wrongly conclude that the "feature-A"
branch (taken from the event) is now protected.

In the related incident we also observed that this triggered a
reconfiguration with the wrong state of now two protected branches
(masters + feature-A). Since the project in question previously had only
one branch this lead to a change in branch matching behavior for jobs
defined in that repository.

Change-Id: Ia037e3070aaecb05c062865a6bb0479b86e4dcde

2 files changed, 40 insertions, 1 deletions

diff --git a/tests/unit/test_github_driver.py b/tests/unit/test_github_driver.py
index fb46aa7d1..47e84ca7f 100644
--- a/tests/unit/test_github_driver.py
+++ b/tests/unit/test_github_driver.py
@@ -1741,6 +1741,41 @@ class TestGithubUnprotectedBranches(ZuulTestCase):
         # branch
         self.assertLess(old, new)
 
+    def test_base_branch_updated(self):
+        self.create_branch('org/project2', 'feature')
+        github = self.fake_github.getGithubClient()
+        repo = github.repo_from_project('org/project2')
+        repo._set_branch_protection('master', True)
+
+        # Make sure Zuul picked up and cached the configured branches
+        self.scheds.execute(lambda app: app.sched.reconfigure(app.config))
+        self.waitUntilSettled()
+
+        github_connection = self.scheds.first.connections.connections['github']
+        tenant = self.scheds.first.sched.abide.tenants.get('tenant-one')
+        project = github_connection.source.getProject('org/project2')
+
+        # Verify that only the master branch is considered protected
+        branches = github_connection.getProjectBranches(project, tenant)
+        self.assertEqual(branches, ["master"])
+
+        A = self.fake_github.openFakePullRequest('org/project2', 'master',
+                                                 'A')
+        # Fake an event from a pull-request that changed the base
+        # branch from "feature" to "master". The PR is already
+        # using "master" as base, but the event still references
+        # the old "feature" branch.
+        event = A.getPullRequestOpenedEvent()
+        event[1]["pull_request"]["base"]["ref"] = "feature"
+
+        self.fake_github.emitEvent(event)
+        self.waitUntilSettled()
+
+        # Make sure we are still only considering "master" to be
+        # protected.
+        branches = github_connection.getProjectBranches(project, tenant)
+        self.assertEqual(branches, ["master"])
+
     # This test verifies that a PR is considered in case it was created for
     # a branch just has been set to protected before a tenant reconfiguration
     # took place.
diff --git a/zuul/driver/github/githubconnection.py b/zuul/driver/github/githubconnection.py
index 182c83bae..cffbd6769 100644
--- a/zuul/driver/github/githubconnection.py
+++ b/zuul/driver/github/githubconnection.py
@@ -439,7 +439,11 @@ class GithubEventProcessor(object):
                 # branch is now protected.
                 if hasattr(event, "branch") and event.branch:
                     protected = None
-                    if change:
+                    # Only use the `branch_protected` flag if the
+                    # target branch of change and event are the same.
+                    # The base branch could have changed in the
+                    # meantime.
+                    if change and change.branch == event.branch:
                         # PR based events already have the information if the
                         # target branch is protected so take the information
                         # from there.