Merge branch 'po-remove-protocol-v2-ff' into 'master'

Remove protocol v2 feature flag

See merge request gitlab-org/gitaly!1841

10 files changed, 18 insertions, 57 deletions

diff --git a/changelogs/unreleased/po-remove-protocol-v2-ff.yml b/changelogs/unreleased/po-remove-protocol-v2-ff.yml
new file mode 100644
index 000000000..f2a6daf1b
--- /dev/null
+++ b/changelogs/unreleased/po-remove-protocol-v2-ff.yml
@@ -0,0 +1,5 @@
+---
+title: Remove protocol v2 feature flag
+merge_request: 1841
+author:
+type: other
diff --git a/cmd/gitaly-ssh/receive_pack.go b/cmd/gitaly-ssh/receive_pack.go
index b0f44326b..97ffee280 100644
--- a/cmd/gitaly-ssh/receive_pack.go
+++ b/cmd/gitaly-ssh/receive_pack.go
@@ -7,8 +7,6 @@ import (
 
 	"github.com/golang/protobuf/jsonpb"
 	"gitlab.com/gitlab-org/gitaly/client"
-	"gitlab.com/gitlab-org/gitaly/internal/git"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc"
 )
@@ -22,9 +20,5 @@ func receivePack(ctx context.Context, conn *grpc.ClientConn, req string) (int32,
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 
-	if request.GetGitProtocol() == git.ProtocolV2 {
-		ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, featureflag.UseGitProtocolV2)
-	}
-
 	return client.ReceivePack(ctx, conn, os.Stdin, os.Stdout, os.Stderr, &request)
 }
diff --git a/cmd/gitaly-ssh/upload_pack.go b/cmd/gitaly-ssh/upload_pack.go
index 37a62f64b..954e47a6b 100644
--- a/cmd/gitaly-ssh/upload_pack.go
+++ b/cmd/gitaly-ssh/upload_pack.go
@@ -7,8 +7,6 @@ import (
 
 	"github.com/golang/protobuf/jsonpb"
 	"gitlab.com/gitlab-org/gitaly/client"
-	"gitlab.com/gitlab-org/gitaly/internal/git"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc"
 )
@@ -31,9 +29,5 @@ func uploadPack(ctx context.Context, conn *grpc.ClientConn, req string) (int32,
 	ctx, cancel := context.WithCancel(ctx)
 	defer cancel()
 
-	if request.GetGitProtocol() == git.ProtocolV2 {
-		ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, featureflag.UseGitProtocolV2)
-	}
-
 	return client.UploadPack(ctx, conn, os.Stdin, os.Stdout, os.Stderr, &request)
 }
diff --git a/internal/git/protocol.go b/internal/git/protocol.go
index 9c8192a08..ba8f6609e 100644
--- a/internal/git/protocol.go
+++ b/internal/git/protocol.go
@@ -7,7 +7,6 @@ import (
 
 	grpc_ctxtags "github.com/grpc-ecosystem/go-grpc-middleware/tags"
 	"github.com/prometheus/client_golang/prometheus"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 )
 
 const (
@@ -37,16 +36,11 @@ type RequestWithGitProtocol interface {
 // AddGitProtocolEnv checks whether the request has Git protocol v2
 // and sets this in the environment.
 func AddGitProtocolEnv(ctx context.Context, req RequestWithGitProtocol, env []string) []string {
-	reqIsV2 := req.GetGitProtocol() == ProtocolV2
-	ffEnabled := featureflag.IsEnabled(ctx, featureflag.UseGitProtocolV2)
 	protocol := "v0"
 
-	switch {
-	case reqIsV2 && ffEnabled:
+	if req.GetGitProtocol() == ProtocolV2 {
 		env = append(env, fmt.Sprintf("GIT_PROTOCOL=%s", ProtocolV2))
 		protocol = "v2"
-	case reqIsV2 && !ffEnabled:
-		protocol = "v2-rejected"
 	}
 
 	service, method := methodFromContext(ctx)
diff --git a/internal/git/protocol_test.go b/internal/git/protocol_test.go
index 18583b381..c0314dfa3 100644
--- a/internal/git/protocol_test.go
+++ b/internal/git/protocol_test.go
@@ -5,7 +5,6 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 )
 
 type fakeProtocolMessage struct {
@@ -16,44 +15,26 @@ func (f fakeProtocolMessage) GetGitProtocol() string {
 	return f.protocol
 }
 
-func enableProtocolV2(ctx context.Context) context.Context {
-	return featureflag.IncomingCtxWithFeatureFlag(ctx, featureflag.UseGitProtocolV2)
-}
-
 func TestAddGitProtocolEnv(t *testing.T) {
 	env := []string{"fake=value"}
 
 	for _, tt := range []struct {
 		desc string
-		ctx  context.Context
 		msg  fakeProtocolMessage
 		env  []string
 	}{
 		{
-			desc: "no feature flag with no V2 request",
-			ctx:  context.Background(),
-			env:  env,
-		},
-		{
-			desc: "feature flag with no V2 request",
-			ctx:  enableProtocolV2(context.Background()),
+			desc: "no V2 request",
 			env:  env,
 		},
 		{
-			desc: "feature flag with V2 request",
-			ctx:  enableProtocolV2(context.Background()),
+			desc: "V2 request",
 			msg:  fakeProtocolMessage{protocol: "version=2"},
 			env:  append(env, "GIT_PROTOCOL=version=2"),
 		},
-		{
-			desc: "no feature flag with V2 request",
-			ctx:  context.Background(),
-			msg:  fakeProtocolMessage{protocol: "version=2"},
-			env:  env,
-		},
 	} {
 		t.Run(tt.desc, func(t *testing.T) {
-			actual := AddGitProtocolEnv(tt.ctx, tt.msg, env)
+			actual := AddGitProtocolEnv(context.Background(), tt.msg, env)
 			require.Equal(t, tt.env, actual)
 		})
 	}
diff --git a/internal/metadata/featureflag/context_test.go b/internal/metadata/featureflag/context_test.go
index 3422aae59..9cfa63438 100644
--- a/internal/metadata/featureflag/context_test.go
+++ b/internal/metadata/featureflag/context_test.go
@@ -8,20 +8,22 @@ import (
 	"google.golang.org/grpc/metadata"
 )
 
+const mockFeatureFlag = "turn meow on"
+
 func TestIncomingCtxWithFeatureFlag(t *testing.T) {
 	ctx := context.Background()
-	require.False(t, IsEnabled(ctx, UseGitProtocolV2))
+	require.False(t, IsEnabled(ctx, mockFeatureFlag))
 
-	ctx = IncomingCtxWithFeatureFlag(ctx, UseGitProtocolV2)
-	require.True(t, IsEnabled(ctx, UseGitProtocolV2))
+	ctx = IncomingCtxWithFeatureFlag(ctx, mockFeatureFlag)
+	require.True(t, IsEnabled(ctx, mockFeatureFlag))
 }
 
 func TestOutgoingCtxWithFeatureFlag(t *testing.T) {
 	ctx := context.Background()
-	require.False(t, IsEnabled(ctx, UseGitProtocolV2))
+	require.False(t, IsEnabled(ctx, mockFeatureFlag))
 
-	ctx = OutgoingCtxWithFeatureFlag(ctx, UseGitProtocolV2)
-	require.False(t, IsEnabled(ctx, UseGitProtocolV2))
+	ctx = OutgoingCtxWithFeatureFlag(ctx, mockFeatureFlag)
+	require.False(t, IsEnabled(ctx, mockFeatureFlag))
 
 	// simulate an outgoing context leaving the process boundary and then
 	// becoming an incoming context in a new process boundary
@@ -29,5 +31,5 @@ func TestOutgoingCtxWithFeatureFlag(t *testing.T) {
 	require.True(t, ok)
 
 	ctx = metadata.NewIncomingContext(context.Background(), md)
-	require.True(t, IsEnabled(ctx, UseGitProtocolV2))
+	require.True(t, IsEnabled(ctx, mockFeatureFlag))
 }
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index c4e3540e7..c2178e554 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -12,8 +12,6 @@ const (
 	// annotations (i.e. accessor and mutator RPC's). This enables cache
 	// invalidation by changing state when the repo is modified.
 	CacheInvalidator = "cache_invalidator"
-	// UseGitProtocolV2 enables support for git wire protocol v2
-	UseGitProtocolV2 = "use_git_protocol_v2"
 )
 
 const (
diff --git a/internal/service/smarthttp/inforefs_test.go b/internal/service/smarthttp/inforefs_test.go
index 64ebbe6a9..9cbb1fcaa 100644
--- a/internal/service/smarthttp/inforefs_test.go
+++ b/internal/service/smarthttp/inforefs_test.go
@@ -80,8 +80,6 @@ func TestSuccessfulInfoRefsUploadPackWithGitProtocol(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
-	ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, featureflag.UseGitProtocolV2)
-
 	c, err := client.InfoRefsUploadPack(ctx, rpcRequest)
 
 	for {
diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go
index ba9f658a7..ea8465630 100644
--- a/internal/service/smarthttp/receive_pack_test.go
+++ b/internal/service/smarthttp/receive_pack_test.go
@@ -18,7 +18,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
-	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
@@ -86,8 +85,6 @@ func TestSuccessfulReceivePackRequestWithGitProtocol(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, featureflag.UseGitProtocolV2)
-
 	stream, err := client.PostReceivePack(ctx)
 	require.NoError(t, err)
 
diff --git a/internal/service/smarthttp/upload_pack_test.go b/internal/service/smarthttp/upload_pack_test.go
index a7777e595..808ace220 100644
--- a/internal/service/smarthttp/upload_pack_test.go
+++ b/internal/service/smarthttp/upload_pack_test.go
@@ -173,8 +173,6 @@ func TestUploadPackRequestWithGitProtocol(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, featureflag.UseGitProtocolV2)
-
 	testRepo := testhelper.TestRepository()
 	testRepoPath, err := helper.GetRepoPath(testRepo)
 	require.NoError(t, err)