Merge branch 'pks-ff-remove-fetch-internal-with-sidechannel' into 'master'

localrepo: Remove flag to switch to sidechannels for internal fetches

Closes #4065

See merge request gitlab-org/gitaly!4375

9 files changed, 14 insertions, 65 deletions

diff --git a/internal/git/localrepo/remote.go b/internal/git/localrepo/remote.go
index 823372a3f..16a4d4e36 100644
--- a/internal/git/localrepo/remote.go
+++ b/internal/git/localrepo/remote.go
@@ -9,7 +9,6 @@ import (
 	"strings"
 
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 )
 
@@ -137,24 +136,13 @@ func (repo *Repo) FetchInternal(
 		// shouldn't even be included in the negotiation phase, so they aren't going to
 		// matter in the connectivity check either.
 		git.WithConfig(git.ConfigPair{Key: "core.alternateRefsCommand", Value: "exit 0 #"}),
-	}
-
-	if featureflag.FetchInternalWithSidechannel.IsEnabled(ctx) {
-		commandOptions = append(commandOptions, git.WithInternalFetchWithSidechannel(
+		git.WithInternalFetchWithSidechannel(
 			&gitalypb.SSHUploadPackWithSidechannelRequest{
 				Repository:       remoteRepo,
 				GitConfigOptions: []string{"uploadpack.allowAnySHA1InWant=true"},
 				GitProtocol:      git.ProtocolV2,
 			},
-		))
-	} else {
-		commandOptions = append(commandOptions, git.WithInternalFetch(
-			&gitalypb.SSHUploadPackRequest{
-				Repository:       remoteRepo,
-				GitConfigOptions: []string{"uploadpack.allowAnySHA1InWant=true"},
-				GitProtocol:      git.ProtocolV2,
-			},
-		))
+		),
 	}
 
 	if opts.DisableTransactions {
diff --git a/internal/git/localrepo/remote_extra_test.go b/internal/git/localrepo/remote_extra_test.go
index 834fcd261..7f4afd350 100644
--- a/internal/git/localrepo/remote_extra_test.go
+++ b/internal/git/localrepo/remote_extra_test.go
@@ -2,7 +2,6 @@ package localrepo_test
 
 import (
 	"bytes"
-	"context"
 	"path/filepath"
 	"testing"
 
@@ -14,7 +13,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/hook"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/repository"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/service/ssh"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
@@ -24,10 +22,8 @@ import (
 
 func TestRepo_FetchInternal(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testRepoFetchInternal)
-}
 
-func testRepoFetchInternal(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 	gitCmdFactory, readGitProtocol := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
 
diff --git a/internal/gitaly/service/conflicts/resolve_conflicts_test.go b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
index 6c29067ac..0cc778069 100644
--- a/internal/gitaly/service/conflicts/resolve_conflicts_test.go
+++ b/internal/gitaly/service/conflicts/resolve_conflicts_test.go
@@ -18,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/hook"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -200,10 +199,7 @@ func TestSuccessfulResolveConflictsRequestHelper(t *testing.T) {
 func TestResolveConflictsWithRemoteRepo(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testResolveConflictsWithRemoteRepo)
-}
-
-func testResolveConflictsWithRemoteRepo(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	hookManager := hook.NewMockManager(t, hook.NopPreReceive, hook.NopPostReceive, hook.NopUpdate, hook.NopReferenceTransaction)
 	cfg, sourceRepo, sourceRepoPath, client := SetupConflictsService(ctx, t, true, hookManager)
 
@@ -817,10 +813,9 @@ func TestFailedResolveConflictsRequestDueToValidation(t *testing.T) {
 }
 
 func TestResolveConflictsQuarantine(t *testing.T) {
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testResolveConflictsQuarantine)
-}
+	t.Parallel()
 
-func testResolveConflictsQuarantine(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, sourceRepoProto, sourceRepoPath, client := SetupConflictsService(ctx, t, true, nil)
 
 	testcfg.BuildGitalySSH(t, cfg)
diff --git a/internal/gitaly/service/operations/rebase_test.go b/internal/gitaly/service/operations/rebase_test.go
index 901db8214..1d388ded7 100644
--- a/internal/gitaly/service/operations/rebase_test.go
+++ b/internal/gitaly/service/operations/rebase_test.go
@@ -1,7 +1,6 @@
 package operations
 
 import (
-	"context"
 	"fmt"
 	"io"
 	"path/filepath"
@@ -16,7 +15,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testserver"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/transaction/txinfo"
@@ -656,10 +654,8 @@ func TestRebaseRequestWithDeletedFile(t *testing.T) {
 
 func TestRebaseOntoRemoteBranch(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testRebaseOntoRemoteBranch)
-}
 
-func testRebaseOntoRemoteBranch(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	ctx, cfg, repoProto, repoPath, client := setupOperationsService(t, ctx)
 
 	repo := localrepo.NewTestRepo(t, cfg, repoProto)
diff --git a/internal/gitaly/service/operations/revert_test.go b/internal/gitaly/service/operations/revert_test.go
index 27a6cda0d..22e10efa2 100644
--- a/internal/gitaly/service/operations/revert_test.go
+++ b/internal/gitaly/service/operations/revert_test.go
@@ -1,7 +1,6 @@
 package operations
 
 import (
-	"context"
 	"fmt"
 	"path/filepath"
 	"testing"
@@ -11,7 +10,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -275,10 +273,7 @@ func TestServer_UserRevert_stableID(t *testing.T) {
 func TestServer_UserRevert_successfulIntoEmptyRepo(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testServerUserRevertSuccessfulIntoEmptyRepo)
-}
-
-func testServerUserRevertSuccessfulIntoEmptyRepo(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	ctx, cfg, startRepoProto, _, client := setupOperationsService(t, ctx)
 
 	startRepo := localrepo.NewTestRepo(t, cfg, startRepoProto)
diff --git a/internal/gitaly/service/repository/fetch_test.go b/internal/gitaly/service/repository/fetch_test.go
index 30c7b14af..360ced75d 100644
--- a/internal/gitaly/service/repository/fetch_test.go
+++ b/internal/gitaly/service/repository/fetch_test.go
@@ -1,14 +1,12 @@
 package repository
 
 import (
-	"context"
 	"testing"
 
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testcfg"
 	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
@@ -18,10 +16,7 @@ import (
 func TestFetchSourceBranchSourceRepositorySuccess(t *testing.T) {
 	t.Parallel()
 
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testFetchSourceBranchSourceRepositorySuccess)
-}
-
-func testFetchSourceBranchSourceRepositorySuccess(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	cfg, sourceRepo, sourcePath, client := setupRepositoryService(ctx, t)
 
 	md := testcfg.GitalyServersMetadataFromCfg(t, cfg)
diff --git a/internal/gitaly/service/repository/replicate_test.go b/internal/gitaly/service/repository/replicate_test.go
index 7c2231aea..7b87f8923 100644
--- a/internal/gitaly/service/repository/replicate_test.go
+++ b/internal/gitaly/service/repository/replicate_test.go
@@ -43,7 +43,6 @@ import (
 func TestReplicateRepository(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testReplicateRepository)
 }
@@ -121,7 +120,6 @@ func testReplicateRepository(t *testing.T, ctx context.Context) {
 func TestReplicateRepositoryTransactional(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testReplicateRepositoryTransactional)
 }
@@ -307,7 +305,6 @@ func TestReplicateRepositoryInvalidArguments(t *testing.T) {
 func TestReplicateRepository_BadRepository(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testReplicateRepositoryBadRepository)
 }
@@ -395,7 +392,6 @@ func testReplicateRepositoryBadRepository(t *testing.T, ctx context.Context) {
 func TestReplicateRepository_FailedFetchInternalRemote(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testReplicateRepositoryFailedFetchInternalRemote)
 }
@@ -481,7 +477,6 @@ func listenGitalySSHCalls(t *testing.T, conf config.Cfg) func() gitalySSHParams
 func TestFetchInternalRemote_successful(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testFetchInternalRemoteSuccessful)
 }
@@ -573,7 +568,6 @@ func testFetchInternalRemoteSuccessful(t *testing.T, ctx context.Context) {
 func TestFetchInternalRemote_failure(t *testing.T) {
 	t.Parallel()
 	testhelper.NewFeatureSets(
-		featureflag.FetchInternalWithSidechannel,
 		featureflag.TransactionalSymbolicRefUpdates,
 	).Run(t, testFetchInternalRemoteFailure)
 }
diff --git a/internal/metadata/featureflag/ff_fetch_internal_with_sidechannel.go b/internal/metadata/featureflag/ff_fetch_internal_with_sidechannel.go
deleted file mode 100644
index 726aeb0ea..000000000
--- a/internal/metadata/featureflag/ff_fetch_internal_with_sidechannel.go
+++ /dev/null
@@ -1,5 +0,0 @@
-package featureflag
-
-// FetchInternalWithSidechannel enables the use of SSHUploadPackWithSidechannel for internal
-// fetches.
-var FetchInternalWithSidechannel = NewFeatureFlag("fetch_internal_with_sidechannel", false)
diff --git a/internal/praefect/replicator_test.go b/internal/praefect/replicator_test.go
index 7cd48ff3a..6ddb85128 100644
--- a/internal/praefect/replicator_test.go
+++ b/internal/praefect/replicator_test.go
@@ -26,7 +26,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/transaction"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/helper"
-	"gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/middleware/metadatahandler"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/config"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/praefect/datastore"
@@ -52,10 +51,9 @@ func TestMain(m *testing.M) {
 
 func TestReplMgr_ProcessBacklog(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testReplMgrProcessBacklog)
-}
 
-func testReplMgrProcessBacklog(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
+
 	primaryCfg, testRepoProto, testRepoPath := testcfg.BuildWithRepo(t, testcfg.WithStorages("primary"))
 	testRepo := localrepo.NewTestRepo(t, primaryCfg, testRepoProto)
 	primaryCfg.SocketPath = testserver.RunGitalyServer(t, primaryCfg, nil, setup.RegisterAll, testserver.WithDisablePraefect())
@@ -681,10 +679,9 @@ func getChecksumFunc(ctx context.Context, client gitalypb.RepositoryServiceClien
 
 func TestProcessBacklog_FailedJobs(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testProcessBacklogFailedJobs)
-}
 
-func testProcessBacklogFailedJobs(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
+
 	primaryCfg, testRepo, _ := testcfg.BuildWithRepo(t, testcfg.WithStorages("default"))
 	primaryAddr := testserver.RunGitalyServer(t, primaryCfg, nil, setup.RegisterAll, testserver.WithDisablePraefect())
 
@@ -787,10 +784,8 @@ func testProcessBacklogFailedJobs(t *testing.T, ctx context.Context) {
 
 func TestProcessBacklog_Success(t *testing.T) {
 	t.Parallel()
-	testhelper.NewFeatureSets(featureflag.FetchInternalWithSidechannel).Run(t, testProcessBacklogSuccess)
-}
 
-func testProcessBacklogSuccess(t *testing.T, ctx context.Context) {
+	ctx := testhelper.Context(t)
 	ctx, cancel := context.WithCancel(ctx)
 
 	primaryCfg, testRepo, _ := testcfg.BuildWithRepo(t, testcfg.WithStorages("primary"))