Merge branch 'sh-fix-remote-mirrors-wildcard-branches' into 'master'

Fix wildcard protected branches not working with remote mirrors

Closes #1435

See merge request gitlab-org/gitaly!1006

7 files changed, 294 insertions, 18 deletions

diff --git a/changelogs/unreleased/sh-fix-remote-mirrors-wildcard-branches.yml b/changelogs/unreleased/sh-fix-remote-mirrors-wildcard-branches.yml
new file mode 100644
index 000000000..0c91bf5a3
--- /dev/null
+++ b/changelogs/unreleased/sh-fix-remote-mirrors-wildcard-branches.yml
@@ -0,0 +1,5 @@
+---
+title: Fix wildcard protected branches not working with remote mirrors
+merge_request: 1006
+author:
+type: fixed
diff --git a/internal/service/remote/update_remote_mirror_test.go b/internal/service/remote/update_remote_mirror_test.go
index cd08bb08d..dfa173829 100644
--- a/internal/service/remote/update_remote_mirror_test.go
+++ b/internal/service/remote/update_remote_mirror_test.go
@@ -26,23 +26,29 @@ func TestSuccessfulUpdateRemoteMirrorRequest(t *testing.T) {
 
 	remoteName := "remote_mirror_1"
 
-	// Preconditions
 	testhelper.MustRunCommand(t, nil, "git", "-C", mirrorPath, "tag", "v0.0.1", "master") // I needed another tag for the tests
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "remote", "add", remoteName, mirrorPath)
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "fetch", remoteName)
 
-	// Updates
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "new-branch", "60ecb67744cb56576c30214ff52294f8ce2def98")                  // Add branch
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "ignored-branch", "60ecb67744cb56576c30214ff52294f8ce2def98")              // Add branch not matching branch list
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "update-ref", "refs/heads/empty-branch", "0b4bc9a49b562e85de7cc9e834518ea6828729b9") // Update branch
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-D", "not-merged-branch")                                                 // Delete branch
-
-	// Scoped to the project, so will be removed after
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "config", "user.email", "gitalytest@example.com") // Delete branch
+	setupCommands := [][]string{
+		// Preconditions
+		{"config", "user.email", "gitalytest@example.com"},
+		{"remote", "add", remoteName, mirrorPath},
+		{"fetch", remoteName},
+		// Updates
+		{"branch", "new-branch", "60ecb67744cb56576c30214ff52294f8ce2def98"},                  // Add branch
+		{"branch", "ignored-branch", "60ecb67744cb56576c30214ff52294f8ce2def98"},              // Add branch not matching branch list
+		{"update-ref", "refs/heads/empty-branch", "0b4bc9a49b562e85de7cc9e834518ea6828729b9"}, // Update branch
+		{"branch", "-D", "not-merged-branch"},                                                 // Delete branch
+		// Scoped to the project, so will be removed after
+		{"tag", "new-tag", "60ecb67744cb56576c30214ff52294f8ce2def98"},                          // Add tag
+		{"tag", "-fam", "Overriding tag", "v1.0.0", "0b4bc9a49b562e85de7cc9e834518ea6828729b9"}, // Update tag
+		{"tag", "-d", "v0.0.1"}, // Delete tag
+	}
 
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "new-tag", "60ecb67744cb56576c30214ff52294f8ce2def98")                          // Add tag
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-fam", "Overriding tag", "v1.0.0", "0b4bc9a49b562e85de7cc9e834518ea6828729b9") // Update tag
-	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", "-d", "v0.0.1")                                                                 // Delete tag
+	for _, args := range setupCommands {
+		gitArgs := []string{"-C", testRepoPath}
+		gitArgs = append(gitArgs, args...)
+		testhelper.MustRunCommand(t, nil, "git", gitArgs...)
+	}
 
 	newTagOid := string(testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", "v1.0.0"))
 	newTagOid = strings.TrimSpace(newTagOid)
@@ -83,6 +89,81 @@ func TestSuccessfulUpdateRemoteMirrorRequest(t *testing.T) {
 	require.NotContains(t, mirrorRefs, "refs/tags/v0.0.1")
 }
 
+func TestSuccessfulUpdateRemoteMirrorRequestWithWildcards(t *testing.T) {
+	server, serverSocketPath := runRemoteServiceServer(t)
+	defer server.Stop()
+
+	client, conn := NewRemoteClient(t, serverSocketPath)
+	defer conn.Close()
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	_, mirrorPath, mirrorCleanupFn := testhelper.NewTestRepo(t)
+	defer mirrorCleanupFn()
+
+	remoteName := "remote_mirror_2"
+
+	setupCommands := [][]string{
+		// Preconditions
+		{"config", "user.email", "gitalytest@example.com"},
+		{"remote", "add", remoteName, mirrorPath},
+		{"fetch", remoteName},
+		// Updates
+		{"branch", "11-0-stable", "60ecb67744cb56576c30214ff52294f8ce2def98"},
+		{"branch", "11-1-stable", "60ecb67744cb56576c30214ff52294f8ce2def98"},                // Add branch
+		{"branch", "ignored-branch", "60ecb67744cb56576c30214ff52294f8ce2def98"},             // Add branch not matching branch list
+		{"update-ref", "refs/heads/some-branch", "0b4bc9a49b562e85de7cc9e834518ea6828729b9"}, // Update branch
+		{"update-ref", "refs/heads/feature", "0b4bc9a49b562e85de7cc9e834518ea6828729b9"},     // Update branch
+		// Scoped to the project, so will be removed after
+		{"branch", "-D", "not-merged-branch"},                                                   // Delete branch
+		{"tag", "new-tag", "60ecb67744cb56576c30214ff52294f8ce2def98"},                          // Add tag
+		{"tag", "-fam", "Overriding tag", "v1.0.0", "0b4bc9a49b562e85de7cc9e834518ea6828729b9"}, // Update tag
+	}
+
+	for _, args := range setupCommands {
+		gitArgs := []string{"-C", testRepoPath}
+		gitArgs = append(gitArgs, args...)
+		testhelper.MustRunCommand(t, nil, "git", gitArgs...)
+	}
+
+	// Workaround for https://gitlab.com/gitlab-org/gitaly/issues/1439
+	// Create a tag on the remote to ensure it gets deleted later
+	testhelper.MustRunCommand(t, nil, "git", "-C", mirrorPath, "tag", "v1.2.0", "master")
+
+	newTagOid := string(testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", "v1.0.0"))
+	newTagOid = strings.TrimSpace(newTagOid)
+	require.NotEqual(t, newTagOid, "f4e6814c3e4e7a0de82a9e7cd20c626cc963a2f8") // Sanity check that the tag did in fact change
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	firstRequest := &gitalypb.UpdateRemoteMirrorRequest{
+		Repository:           testRepo,
+		RefName:              remoteName,
+		OnlyBranchesMatching: [][]byte{[]byte("*-stable"), []byte("feature")},
+	}
+
+	stream, err := client.UpdateRemoteMirror(ctx)
+	require.NoError(t, err)
+	require.NoError(t, stream.Send(firstRequest))
+
+	_, err = stream.CloseAndRecv()
+	require.NoError(t, err)
+
+	mirrorRefs := string(testhelper.MustRunCommand(t, nil, "git", "-C", mirrorPath, "for-each-ref"))
+
+	require.Contains(t, mirrorRefs, "60ecb67744cb56576c30214ff52294f8ce2def98 commit\trefs/heads/11-0-stable")
+	require.Contains(t, mirrorRefs, "60ecb67744cb56576c30214ff52294f8ce2def98 commit\trefs/heads/11-1-stable")
+	require.Contains(t, mirrorRefs, "0b4bc9a49b562e85de7cc9e834518ea6828729b9 commit\trefs/heads/feature")
+	require.NotContains(t, mirrorRefs, "refs/heads/ignored-branch")
+	require.NotContains(t, mirrorRefs, "refs/heads/some-branch")
+	require.Contains(t, mirrorRefs, "refs/heads/not-merged-branch")
+	require.Contains(t, mirrorRefs, "60ecb67744cb56576c30214ff52294f8ce2def98 commit\trefs/tags/new-tag")
+	require.Contains(t, mirrorRefs, newTagOid+" tag\trefs/tags/v1.0.0")
+	require.NotContains(t, mirrorRefs, "refs/tags/v1.2.0")
+}
+
 func TestFailedUpdateRemoteMirrorRequestDueToValidation(t *testing.T) {
 	server, serverSocketPath := runRemoteServiceServer(t)
 	defer server.Stop()
diff --git a/ruby/lib/gitlab/git.rb b/ruby/lib/gitlab/git.rb
index ce7fc4b2c..6970655a2 100644
--- a/ruby/lib/gitlab/git.rb
+++ b/ruby/lib/gitlab/git.rb
@@ -23,6 +23,7 @@ dir = __dir__
 # Some later requires are order-sensitive. Manually require whatever we need.
 require_relative "#{dir}/encoding_helper.rb"
 require_relative "#{dir}/utils/strong_memoize.rb"
+require_relative "#{dir}/ref_matcher.rb"
 require_relative "#{dir}/git/remote_repository.rb"
 require_relative "#{dir}/git/popen.rb"
 require_relative "#{dir}/git/repository_mirroring.rb"
diff --git a/ruby/lib/gitlab/git/remote_mirror.rb b/ruby/lib/gitlab/git/remote_mirror.rb
index 9d520aca9..9b5cf3180 100644
--- a/ruby/lib/gitlab/git/remote_mirror.rb
+++ b/ruby/lib/gitlab/git/remote_mirror.rb
@@ -27,23 +27,29 @@ module Gitlab
 
       private
 
+      def ref_matchers
+        @ref_matchers ||= only_branches_matching.map do |ref|
+          GitLab::RefMatcher.new(ref)
+        end
+      end
+
       def local_branches
         @local_branches ||= refs_obj(
           repository.local_branches,
-          only_refs_matching: only_branches_matching
+          match_refs: true
         )
       end
 
       def remote_branches
         @remote_branches ||= refs_obj(
           repository.remote_branches(ref_name),
-          only_refs_matching: only_branches_matching
+          match_refs: true
         )
       end
 
-      def refs_obj(refs, only_refs_matching: [])
+      def refs_obj(refs, match_refs: false)
         refs.each_with_object({}) do |ref, refs|
-          next if only_refs_matching.present? && !only_refs_matching.include?(ref.name)
+          next if match_refs && !include_ref?(ref.name)
 
           refs[ref.name] = ref
         end
@@ -93,6 +99,12 @@ module Gitlab
           remote_ref_id && repository.ancestor?(remote_ref_id, default_branch_id)
         end
       end
+
+      def include_ref?(ref_name)
+        return true unless ref_matchers.present?
+
+        ref_matchers.any? { |matcher| matcher.matches?(ref_name) }
+      end
     end
   end
 end
diff --git a/ruby/lib/gitlab/ref_matcher.rb b/ruby/lib/gitlab/ref_matcher.rb
new file mode 100644
index 000000000..111bd2d50
--- /dev/null
+++ b/ruby/lib/gitlab/ref_matcher.rb
@@ -0,0 +1,41 @@
+# frozen_string_literal: true
+
+module GitLab
+  class RefMatcher
+    def initialize(ref_name_or_pattern)
+      @ref_name_or_pattern = ref_name_or_pattern
+    end
+
+    # Checks if the protected ref matches the given ref name.
+    def matches?(ref_name)
+      return false if @ref_name_or_pattern.blank?
+
+      exact_match?(ref_name) || wildcard_match?(ref_name)
+    end
+
+    private
+
+    # Checks if this protected ref contains a wildcard
+    def wildcard?
+      @ref_name_or_pattern&.include?('*')
+    end
+
+    def exact_match?(ref_name)
+      @ref_name_or_pattern == ref_name
+    end
+
+    def wildcard_match?(ref_name)
+      return false unless wildcard?
+
+      ref_name.match(wildcard_regex).present?
+    end
+
+    def wildcard_regex
+      @wildcard_regex ||= begin
+                            split = @ref_name_or_pattern.split('*', -1) # Use -1 to correctly handle trailing '*'
+                            quoted_segments = split.map { |segment| Regexp.quote(segment) }
+                            quoted_segments.join('.*?')
+                          end
+    end
+  end
+end
diff --git a/ruby/spec/lib/gitlab/git/remote_mirror_spec.rb b/ruby/spec/lib/gitlab/git/remote_mirror_spec.rb
index a009a2f92..34503d0b0 100644
--- a/ruby/spec/lib/gitlab/git/remote_mirror_spec.rb
+++ b/ruby/spec/lib/gitlab/git/remote_mirror_spec.rb
@@ -27,6 +27,54 @@ describe Gitlab::Git::RemoteMirror do
   end
 
   describe '#update' do
+    context 'with wildcard protected branches' do
+      subject(:remote_mirror) do
+        described_class.new(
+          repository,
+          ref_name,
+          ssh_auth: ssh_auth,
+          only_branches_matching: ['master', '*-stable']
+        )
+      end
+
+      it 'updates the remote repository' do
+        # Stub this check so we try to delete the obsolete tag
+        allow(repository).to receive(:ancestor?).and_return(true)
+
+        expect(repository).to receive(:local_branches).and_return([ref('master'), ref('11-5-stable'), ref('unprotected')])
+        expect(repository).to receive(:remote_branches)
+          .with(ref_name)
+          .and_return([ref('master'), ref('obsolete-branch')])
+
+        expect(repository).to receive(:tags).and_return([ref('v1.0.0'), ref('new-tag')])
+        expect(repository).to receive(:remote_tags)
+          .with(ref_name, env: env)
+          .and_return([ref('v1.0.0'), ref('obsolete-tag')])
+
+        expect(gitlab_projects)
+          .to receive(:push_branches)
+          .with(ref_name, gl_projects_timeout, gl_projects_force, ['master', '11-5-stable'], env: env)
+          .and_return(true)
+
+        expect(gitlab_projects)
+          .to receive(:push_branches)
+          .with(ref_name, gl_projects_timeout, gl_projects_force, ['v1.0.0', 'new-tag'], env: env)
+          .and_return(true)
+
+        # Leave remote branches that do not match the protected branch filter
+        expect(gitlab_projects)
+          .not_to receive(:delete_remote_branches)
+          .with(ref_name, ['obsolete-branch'], env: env)
+
+        expect(gitlab_projects)
+          .to receive(:delete_remote_branches)
+          .with(ref_name, ['obsolete-tag'], env: env)
+          .and_return(true)
+
+        remote_mirror.update
+      end
+    end
+
     it 'updates the remote repository' do
       # Stub this check so we try to delete the obsolete refs
       allow(repository).to receive(:ancestor?).and_return(true)
diff --git a/ruby/spec/lib/gitlab/ref_matcher_spec.rb b/ruby/spec/lib/gitlab/ref_matcher_spec.rb
new file mode 100644
index 000000000..00e26328b
--- /dev/null
+++ b/ruby/spec/lib/gitlab/ref_matcher_spec.rb
@@ -0,0 +1,88 @@
+require 'spec_helper'
+
+describe GitLab::RefMatcher do
+  subject { described_class.new(pattern) }
+
+  describe "#matches?" do
+    context "when the ref pattern is not a wildcard" do
+      let(:pattern) { 'production/some-branch' }
+
+      it "returns true for branch names that are an exact match" do
+        expect(subject.matches?('production/some-branch')).to be true
+      end
+
+      it "returns false for branch names that are not an exact match" do
+        expect(subject.matches?("staging/some-branch")).to be false
+      end
+    end
+
+    context "when the ref pattern  name contains wildcard(s)" do
+      context "when there is a single '*'" do
+        let(:pattern) { 'production/*' }
+
+        it "returns true for branch names matching the wildcard" do
+          expect(subject.matches?("production/some-branch")).to be true
+          expect(subject.matches?("production/")).to be true
+        end
+
+        it "returns false for branch names not matching the wildcard" do
+          expect(subject.matches?("staging/some-branch")).to be false
+          expect(subject.matches?("production")).to be false
+        end
+      end
+
+      context "when the wildcard contains regex symbols other than a '*'" do
+        let(:pattern) { "pro.duc.tion/*" }
+
+        it "returns true for branch names matching the wildcard" do
+          expect(subject.matches?("pro.duc.tion/some-branch")).to be true
+        end
+
+        it "returns false for branch names not matching the wildcard" do
+          expect(subject.matches?("production/some-branch")).to be false
+          expect(subject.matches?("proXducYtion/some-branch")).to be false
+        end
+      end
+
+      context "when there are '*'s at either end" do
+        let(:pattern) { "*/production/*" }
+
+        it "returns true for branch names matching the wildcard" do
+          expect(subject.matches?("gitlab/production/some-branch")).to be true
+          expect(subject.matches?("/production/some-branch")).to be true
+          expect(subject.matches?("gitlab/production/")).to be true
+          expect(subject.matches?("/production/")).to be true
+        end
+
+        it "returns false for branch names not matching the wildcard" do
+          expect(subject.matches?("gitlabproductionsome-branch")).to be false
+          expect(subject.matches?("production/some-branch")).to be false
+          expect(subject.matches?("gitlab/production")).to be false
+          expect(subject.matches?("production")).to be false
+        end
+      end
+
+      context "when there are arbitrarily placed '*'s" do
+        let(:pattern) { "pro*duction/*/gitlab/*" }
+
+        it "returns true for branch names matching the wildcard" do
+          expect(subject.matches?("production/some-branch/gitlab/second-branch")).to be true
+          expect(subject.matches?("proXYZduction/some-branch/gitlab/second-branch")).to be true
+          expect(subject.matches?("proXYZduction/gitlab/gitlab/gitlab")).to be true
+          expect(subject.matches?("proXYZduction//gitlab/")).to be true
+          expect(subject.matches?("proXYZduction/some-branch/gitlab/")).to be true
+          expect(subject.matches?("proXYZduction//gitlab/some-branch")).to be true
+        end
+
+        it "returns false for branch names not matching the wildcard" do
+          expect(subject.matches?("production/some-branch/not-gitlab/second-branch")).to be false
+          expect(subject.matches?("prodXYZuction/some-branch/gitlab/second-branch")).to be false
+          expect(subject.matches?("proXYZduction/gitlab/some-branch/gitlab")).to be false
+          expect(subject.matches?("proXYZduction/gitlab//")).to be false
+          expect(subject.matches?("proXYZduction/gitlab/")).to be false
+          expect(subject.matches?("proXYZduction//some-branch/gitlab")).to be false
+        end
+      end
+    end
+  end
+end