Merge branch 'po-reenable-git-v2-feature-flag' into 'master'

Reenable git wire protocol v2 behind feature flag

See merge request gitlab-org/gitaly!1797

11 files changed, 88 insertions, 64 deletions

diff --git a/changelogs/unreleased/po-reenable-git-v2-feature-flag.yml b/changelogs/unreleased/po-reenable-git-v2-feature-flag.yml
new file mode 100644
index 000000000..16776bfe9
--- /dev/null
+++ b/changelogs/unreleased/po-reenable-git-v2-feature-flag.yml
@@ -0,0 +1,5 @@
+---
+title: Reenable git wire protocol v2 behind feature flag
+merge_request: 1797
+author:
+type: other
diff --git a/cmd/gitaly-ssh/receive_pack.go b/cmd/gitaly-ssh/receive_pack.go
index 97ffee280..a91bc4c91 100644
--- a/cmd/gitaly-ssh/receive_pack.go
+++ b/cmd/gitaly-ssh/receive_pack.go
@@ -7,6 +7,8 @@ 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"
 )
@@ -20,5 +22,9 @@ 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.ContextWithFeatureFlag(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 954e47a6b..d6ef54687 100644
--- a/cmd/gitaly-ssh/upload_pack.go
+++ b/cmd/gitaly-ssh/upload_pack.go
@@ -7,6 +7,8 @@ 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"
 )
@@ -29,5 +31,9 @@ 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.ContextWithFeatureFlag(ctx, featureflag.UseGitProtocolV2)
+	}
+
 	return client.UploadPack(ctx, conn, os.Stdin, os.Stdout, os.Stderr, &request)
 }
diff --git a/internal/config/config.go b/internal/config/config.go
index 037c96027..0f041f2a9 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -65,17 +65,6 @@ type GitlabShell struct {
 type Git struct {
 	BinPath string `toml:"bin_path"`
 
-	// ProtocolV2Enabled can be set to true to enable the newer Git protocol
-	// version. This should not be enabled until GitLab *either* stops
-	// using transfer.hideRefs for security purposes, *or* Git protocol v2
-	// respects this setting:
-	//
-	// https://public-inbox.org/git/20181213155817.27666-1-avarab@gmail.com/T/
-	//
-	// This is not user-configurable. Once a new Git version has been released,
-	// we can add code to enable it if the detected git binary is new enough
-	ProtocolV2Enabled bool
-
 	CatfileCacheSize int `toml:"catfile_cache_size"`
 }
 
diff --git a/internal/git/protocol.go b/internal/git/protocol.go
index ac86873b8..9c8192a08 100644
--- a/internal/git/protocol.go
+++ b/internal/git/protocol.go
@@ -7,7 +7,7 @@ import (
 
 	grpc_ctxtags "github.com/grpc-ecosystem/go-grpc-middleware/tags"
 	"github.com/prometheus/client_golang/prometheus"
-	"gitlab.com/gitlab-org/gitaly/internal/config"
+	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 )
 
 const (
@@ -37,20 +37,21 @@ 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 {
-	service, method := methodFromContext(ctx)
-
-	if req.GetGitProtocol() == ProtocolV2 {
-		if config.Config.Git.ProtocolV2Enabled {
-			env = append(env, fmt.Sprintf("GIT_PROTOCOL=%s", ProtocolV2))
+	reqIsV2 := req.GetGitProtocol() == ProtocolV2
+	ffEnabled := featureflag.IsEnabled(ctx, featureflag.UseGitProtocolV2)
+	protocol := "v0"
 
-			gitProtocolRequests.WithLabelValues(service, method, "v2").Inc()
-		} else {
-			gitProtocolRequests.WithLabelValues(service, method, "v2-rejected").Inc()
-		}
-	} else {
-		gitProtocolRequests.WithLabelValues(service, method, "v0").Inc()
+	switch {
+	case reqIsV2 && ffEnabled:
+		env = append(env, fmt.Sprintf("GIT_PROTOCOL=%s", ProtocolV2))
+		protocol = "v2"
+	case reqIsV2 && !ffEnabled:
+		protocol = "v2-rejected"
 	}
 
+	service, method := methodFromContext(ctx)
+	gitProtocolRequests.WithLabelValues(service, method, protocol).Inc()
+
 	return env
 }
 
diff --git a/internal/git/protocol_test.go b/internal/git/protocol_test.go
index 925a4ff74..99993a135 100644
--- a/internal/git/protocol_test.go
+++ b/internal/git/protocol_test.go
@@ -5,7 +5,8 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
-	"gitlab.com/gitlab-org/gitaly/internal/config"
+	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
+	"google.golang.org/grpc/metadata"
 )
 
 type fakeProtocolMessage struct {
@@ -16,42 +17,55 @@ func (f fakeProtocolMessage) GetGitProtocol() string {
 	return f.protocol
 }
 
-func setGitProtocolV2Support(value bool) func() {
-	orig := config.Config.Git.ProtocolV2Enabled
-	config.Config.Git.ProtocolV2Enabled = value
+func enableProtocolV2(ctx context.Context) context.Context {
+	// TODO: replace this implementation with a helper function that deals
+	// with the incoming context
+	flag := featureflag.UseGitProtocolV2
 
-	return func() {
-		config.Config.Git.ProtocolV2Enabled = orig
+	md, ok := metadata.FromIncomingContext(ctx)
+	if !ok {
+		md = metadata.New(map[string]string{featureflag.HeaderKey(flag): "true"})
 	}
-}
-
-func TestAddGitProtocolEnvRespectsConfigEnabled(t *testing.T) {
-	restore := setGitProtocolV2Support(true)
-	defer restore()
+	md.Set(featureflag.HeaderKey(flag), "true")
 
-	env := []string{"fake=value"}
-	msg := fakeProtocolMessage{protocol: "version=2"}
-	value := AddGitProtocolEnv(context.Background(), msg, env)
-
-	require.Equal(t, value, append(env, "GIT_PROTOCOL=version=2"))
+	return metadata.NewIncomingContext(ctx, md)
 }
 
-func TestAddGitProtocolEnvWhenV2NotRequested(t *testing.T) {
-	restore := setGitProtocolV2Support(true)
-	defer restore()
-
+func TestAddGitProtocolEnv(t *testing.T) {
 	env := []string{"fake=value"}
-	msg := fakeProtocolMessage{protocol: ""}
-	value := AddGitProtocolEnv(context.Background(), msg, env)
-	require.Equal(t, value, env)
-}
-
-func TestAddGitProtocolEnvRespectsConfigDisabled(t *testing.T) {
-	restore := setGitProtocolV2Support(false)
-	defer restore()
 
-	env := []string{"fake=value"}
-	msg := fakeProtocolMessage{protocol: "GIT_PROTOCOL=version=2"}
-	value := AddGitProtocolEnv(context.Background(), msg, env)
-	require.Equal(t, value, env)
+	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()),
+			env:  env,
+		},
+		{
+			desc: "feature flag with V2 request",
+			ctx:  enableProtocolV2(context.Background()),
+			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)
+			require.Equal(t, tt.env, actual)
+		})
+	}
 }
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index f9555a85d..35aa7984b 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -18,6 +18,8 @@ const (
 	CommitWithoutBatchCheck = "commit_without_batch_check"
 	// UseCoreDeltaIslands enables support of core delta islands for 'repack'.
 	UseCoreDeltaIslands = "use_core_delta_islands"
+	// 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 9cbb1fcaa..d633da2cf 100644
--- a/internal/service/smarthttp/inforefs_test.go
+++ b/internal/service/smarthttp/inforefs_test.go
@@ -80,6 +80,8 @@ func TestSuccessfulInfoRefsUploadPackWithGitProtocol(t *testing.T) {
 	ctx, cancel := context.WithCancel(context.Background())
 	defer cancel()
 
+	ctx = featureflag.ContextWithFeatureFlag(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 ffd778f5a..7f0eb50ed 100644
--- a/internal/service/smarthttp/receive_pack_test.go
+++ b/internal/service/smarthttp/receive_pack_test.go
@@ -17,6 +17,7 @@ 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"
@@ -84,6 +85,8 @@ func TestSuccessfulReceivePackRequestWithGitProtocol(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
+	ctx = featureflag.ContextWithFeatureFlag(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 e7c051d79..05ffbd7a4 100644
--- a/internal/service/smarthttp/upload_pack_test.go
+++ b/internal/service/smarthttp/upload_pack_test.go
@@ -173,6 +173,8 @@ func TestUploadPackRequestWithGitProtocol(t *testing.T) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
+	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.UseGitProtocolV2)
+
 	testRepo := testhelper.TestRepository()
 	testRepoPath, err := helper.GetRepoPath(testRepo)
 	require.NoError(t, err)
diff --git a/internal/testhelper/git_protocol.go b/internal/testhelper/git_protocol.go
index 5619739cb..ab0377551 100644
--- a/internal/testhelper/git_protocol.go
+++ b/internal/testhelper/git_protocol.go
@@ -4,23 +4,17 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 )
 
-// EnableGitProtocolV2Support ensures that Git protocol v2 support is enabled,
-// and replaces the git binary in config with an `env_git` wrapper that allows
-// the protocol to be tested. It returns a function that restores the given
-// settings.
+// EnableGitProtocolV2Support replaces the git binary in config with an
+// `env_git` wrapper that allows the protocol to be tested. It returns a
+// function that restores the given settings.
 //
 // Because we don't know how to get to that wrapper script from the current
 // working directory, callers must create a symbolic link to the `env_git` file
 // in their own `testdata` directories.
 func EnableGitProtocolV2Support() func() {
 	oldGitBinPath := config.Config.Git.BinPath
-	oldGitProtocolV2Enabled := config.Config.Git.ProtocolV2Enabled
-
 	config.Config.Git.BinPath = "testdata/env_git"
-	config.Config.Git.ProtocolV2Enabled = true
-
 	return func() {
 		config.Config.Git.BinPath = oldGitBinPath
-		config.Config.Git.ProtocolV2Enabled = oldGitProtocolV2Enabled
 	}
 }