Merge branch 'jv-always-use-sidechannel' into 'main'

Always use Gitaly sidechannel connections

See merge request gitlab-org/gitlab-shell!567

12 files changed, 41 insertions, 175 deletions

diff --git a/internal/command/receivepack/gitalycall_test.go b/internal/command/receivepack/gitalycall_test.go
index 4665366..cab2d80 100644
--- a/internal/command/receivepack/gitalycall_test.go
+++ b/internal/command/receivepack/gitalycall_test.go
@@ -57,8 +57,10 @@ func TestReceivePack(t *testing.T) {
 			args.GitlabKeyId = tc.keyId
 		}
 
+		cfg := &config.Config{GitlabUrl: url}
+		cfg.GitalyClient.InitSidechannelRegistry(context.Background())
 		cmd := &Command{
-			Config:     &config.Config{GitlabUrl: url},
+			Config:     cfg,
 			Args:       args,
 			ReadWriter: &readwriter.ReadWriter{ErrOut: output, Out: output, In: input},
 		}
diff --git a/internal/command/uploadarchive/gitalycall_test.go b/internal/command/uploadarchive/gitalycall_test.go
index 302b949..a0f4ce5 100644
--- a/internal/command/uploadarchive/gitalycall_test.go
+++ b/internal/command/uploadarchive/gitalycall_test.go
@@ -41,8 +41,10 @@ func TestUploadArchive(t *testing.T) {
 		Env:         env,
 	}
 
+	cfg := &config.Config{GitlabUrl: url}
+	cfg.GitalyClient.InitSidechannelRegistry(context.Background())
 	cmd := &Command{
-		Config:     &config.Config{GitlabUrl: url},
+		Config:     cfg,
 		Args:       args,
 		ReadWriter: &readwriter.ReadWriter{ErrOut: output, Out: output, In: input},
 	}
diff --git a/internal/command/uploadpack/gitalycall.go b/internal/command/uploadpack/gitalycall.go
index 96dd823..2ba5f1d 100644
--- a/internal/command/uploadpack/gitalycall.go
+++ b/internal/command/uploadpack/gitalycall.go
@@ -15,24 +15,7 @@ import (
 func (c *Command) performGitalyCall(ctx context.Context, response *accessverifier.Response) error {
 	gc := handler.NewGitalyCommand(c.Config, string(commandargs.UploadPack), response)
 
-	if response.Gitaly.UseSidechannel {
-		request := &pb.SSHUploadPackWithSidechannelRequest{
-			Repository:       &response.Gitaly.Repo,
-			GitProtocol:      c.Args.Env.GitProtocolVersion,
-			GitConfigOptions: response.GitConfigOptions,
-		}
-
-		return gc.RunGitalyCommand(ctx, func(ctx context.Context, conn *grpc.ClientConn) (int32, error) {
-			ctx, cancel := gc.PrepareContext(ctx, request.Repository, c.Args.Env)
-			defer cancel()
-
-			registry := c.Config.GitalyClient.SidechannelRegistry
-			rw := c.ReadWriter
-			return client.UploadPackWithSidechannel(ctx, conn, registry, rw.In, rw.Out, rw.ErrOut, request)
-		})
-	}
-
-	request := &pb.SSHUploadPackRequest{
+	request := &pb.SSHUploadPackWithSidechannelRequest{
 		Repository:       &response.Gitaly.Repo,
 		GitProtocol:      c.Args.Env.GitProtocolVersion,
 		GitConfigOptions: response.GitConfigOptions,
@@ -42,7 +25,8 @@ func (c *Command) performGitalyCall(ctx context.Context, response *accessverifie
 		ctx, cancel := gc.PrepareContext(ctx, request.Repository, c.Args.Env)
 		defer cancel()
 
+		registry := c.Config.GitalyClient.SidechannelRegistry
 		rw := c.ReadWriter
-		return client.UploadPack(ctx, conn, rw.In, rw.Out, rw.ErrOut, request)
+		return client.UploadPackWithSidechannel(ctx, conn, registry, rw.In, rw.Out, rw.ErrOut, request)
 	})
 }
diff --git a/internal/command/uploadpack/gitalycall_test.go b/internal/command/uploadpack/gitalycall_test.go
index e0a15ee..3245a65 100644
--- a/internal/command/uploadpack/gitalycall_test.go
+++ b/internal/command/uploadpack/gitalycall_test.go
@@ -41,62 +41,6 @@ func TestUploadPack(t *testing.T) {
 		Env:         env,
 	}
 
-	cmd := &Command{
-		Config:     &config.Config{GitlabUrl: url},
-		Args:       args,
-		ReadWriter: &readwriter.ReadWriter{ErrOut: output, Out: output, In: input},
-	}
-
-	ctx := correlation.ContextWithCorrelation(context.Background(), "a-correlation-id")
-	ctx = correlation.ContextWithClientName(ctx, "gitlab-shell-tests")
-
-	err := cmd.Execute(ctx)
-	require.NoError(t, err)
-
-	require.Equal(t, "UploadPack: "+repo, output.String())
-
-	for k, v := range map[string]string{
-		"gitaly-feature-cache_invalidator":        "true",
-		"gitaly-feature-inforef_uploadpack_cache": "false",
-		"x-gitlab-client-name":                    "gitlab-shell-tests-git-upload-pack",
-		"key_id":                                  "123",
-		"user_id":                                 "1",
-		"remote_ip":                               "127.0.0.1",
-		"key_type":                                "key",
-	} {
-		actual := testServer.ReceivedMD[k]
-		require.Len(t, actual, 1)
-		require.Equal(t, v, actual[0])
-	}
-	require.Empty(t, testServer.ReceivedMD["some-other-ff"])
-	require.Equal(t, testServer.ReceivedMD["x-gitlab-correlation-id"][0], "a-correlation-id")
-}
-
-func TestUploadPack_withSidechannel(t *testing.T) {
-	gitalyAddress, testServer := testserver.StartGitalyServer(t)
-
-	requests := requesthandlers.BuildAllowedWithGitalyHandlersWithSidechannel(t, gitalyAddress)
-	url := testserver.StartHttpServer(t, requests)
-
-	output := &bytes.Buffer{}
-	input := &bytes.Buffer{}
-
-	userId := "1"
-	repo := "group/repo"
-
-	env := sshenv.Env{
-		IsSSHConnection: true,
-		OriginalCommand: "git-upload-pack " + repo,
-		RemoteAddr:      "127.0.0.1",
-	}
-
-	args := &commandargs.Shell{
-		GitlabKeyId: userId,
-		CommandType: commandargs.UploadPack,
-		SshArgs:     []string{"git-upload-pack", repo},
-		Env:         env,
-	}
-
 	ctx := correlation.ContextWithCorrelation(context.Background(), "a-correlation-id")
 	ctx = correlation.ContextWithClientName(ctx, "gitlab-shell-tests")
 
diff --git a/internal/gitaly/gitaly.go b/internal/gitaly/gitaly.go
index a6d8128..9f73661 100644
--- a/internal/gitaly/gitaly.go
+++ b/internal/gitaly/gitaly.go
@@ -21,10 +21,9 @@ import (
 )
 
 type Command struct {
-	ServiceName     string
-	Address         string
-	Token           string
-	DialSidechannel bool
+	ServiceName string
+	Address     string
+	Token       string
 }
 
 type connectionsCache struct {
@@ -125,9 +124,5 @@ func (c *Client) newConnection(ctx context.Context, cmd Command) (conn *grpc.Cli
 		)
 	}
 
-	if cmd.DialSidechannel {
-		return client.DialSidechannel(ctx, cmd.Address, c.SidechannelRegistry, connOpts)
-	}
-
-	return client.DialContext(ctx, cmd.Address, connOpts)
+	return client.DialSidechannel(ctx, cmd.Address, c.SidechannelRegistry, connOpts)
 }
diff --git a/internal/gitaly/gitaly_test.go b/internal/gitaly/gitaly_test.go
index 9e1ee1a..abfb764 100644
--- a/internal/gitaly/gitaly_test.go
+++ b/internal/gitaly/gitaly_test.go
@@ -13,7 +13,7 @@ import (
 func TestPrometheusMetrics(t *testing.T) {
 	metrics.GitalyConnectionsTotal.Reset()
 
-	c := &Client{}
+	c := newClient()
 
 	cmd := Command{ServiceName: "git-upload-pack", Address: "tcp://localhost:9999"}
 	c.newConnection(context.Background(), cmd)
@@ -31,7 +31,7 @@ func TestPrometheusMetrics(t *testing.T) {
 }
 
 func TestCachedConnections(t *testing.T) {
-	c := &Client{}
+	c := newClient()
 
 	require.Len(t, c.cache.connections, 0)
 
@@ -51,3 +51,9 @@ func TestCachedConnections(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, c.cache.connections, 2)
 }
+
+func newClient() *Client {
+	c := &Client{}
+	c.InitSidechannelRegistry(context.Background())
+	return c
+}
diff --git a/internal/gitlabnet/accessverifier/client.go b/internal/gitlabnet/accessverifier/client.go
index c0d10e3..bce32cf 100644
--- a/internal/gitlabnet/accessverifier/client.go
+++ b/internal/gitlabnet/accessverifier/client.go
@@ -32,11 +32,10 @@ type Request struct {
 }
 
 type Gitaly struct {
-	Repo           pb.Repository     `json:"repository"`
-	Address        string            `json:"address"`
-	Token          string            `json:"token"`
-	Features       map[string]string `json:"features"`
-	UseSidechannel bool              `json:"use_sidechannel"`
+	Repo     pb.Repository     `json:"repository"`
+	Address  string            `json:"address"`
+	Token    string            `json:"token"`
+	Features map[string]string `json:"features"`
 }
 
 type CustomPayloadData struct {
diff --git a/internal/gitlabnet/accessverifier/client_test.go b/internal/gitlabnet/accessverifier/client_test.go
index 60e3f0e..13e2d2c 100644
--- a/internal/gitlabnet/accessverifier/client_test.go
+++ b/internal/gitlabnet/accessverifier/client_test.go
@@ -86,41 +86,6 @@ func TestSuccessfulResponses(t *testing.T) {
 	}
 }
 
-func TestSidechannelFlag(t *testing.T) {
-	okResponse := testResponse{body: responseBody(t, "allowed_sidechannel.json"), status: http.StatusOK}
-	client := setup(t,
-		map[string]testResponse{"first": okResponse},
-		map[string]testResponse{"1": okResponse},
-	)
-
-	testCases := []struct {
-		desc string
-		args *commandargs.Shell
-		who  string
-	}{
-		{
-			desc: "Provide key id within the request",
-			args: &commandargs.Shell{GitlabKeyId: "1"},
-			who:  "key-1",
-		}, {
-			desc: "Provide username within the request",
-			args: &commandargs.Shell{GitlabUsername: "first"},
-			who:  "user-1",
-		},
-	}
-
-	for _, tc := range testCases {
-		t.Run(tc.desc, func(t *testing.T) {
-			result, err := client.Verify(context.Background(), tc.args, uploadPackAction, repo)
-			require.NoError(t, err)
-
-			response := buildExpectedResponse(tc.who)
-			response.Gitaly.UseSidechannel = true
-			require.Equal(t, response, result)
-		})
-	}
-}
-
 func TestGeoPushGetCustomAction(t *testing.T) {
 	client := setup(t, map[string]testResponse{
 		"custom": {
diff --git a/internal/handler/exec.go b/internal/handler/exec.go
index 44b02a7..0870d2b 100644
--- a/internal/handler/exec.go
+++ b/internal/handler/exec.go
@@ -33,10 +33,9 @@ type GitalyCommand struct {
 
 func NewGitalyCommand(cfg *config.Config, serviceName string, response *accessverifier.Response) *GitalyCommand {
 	gc := gitaly.Command{
-		ServiceName:     serviceName,
-		Address:         response.Gitaly.Address,
-		Token:           response.Gitaly.Token,
-		DialSidechannel: response.Gitaly.UseSidechannel,
+		ServiceName: serviceName,
+		Address:     response.Gitaly.Address,
+		Token:       response.Gitaly.Token,
 	}
 
 	return &GitalyCommand{Config: cfg, Response: response, Command: gc}
diff --git a/internal/handler/exec_test.go b/internal/handler/exec_test.go
index 8f1d5b2..791fe79 100644
--- a/internal/handler/exec_test.go
+++ b/internal/handler/exec_test.go
@@ -29,7 +29,7 @@ func makeHandler(t *testing.T, err error) func(context.Context, *grpc.ClientConn
 
 func TestRunGitalyCommand(t *testing.T) {
 	cmd := NewGitalyCommand(
-		&config.Config{},
+		newConfig(),
 		string(commandargs.UploadPack),
 		&accessverifier.Response{
 			Gitaly: accessverifier.Gitaly{Address: "tcp://localhost:9999"},
@@ -46,14 +46,12 @@ func TestRunGitalyCommand(t *testing.T) {
 
 func TestCachingOfGitalyConnections(t *testing.T) {
 	ctx := context.Background()
-	cfg := &config.Config{}
-	cfg.GitalyClient.InitSidechannelRegistry(ctx)
+	cfg := newConfig()
 	response := &accessverifier.Response{
 		Username: "user",
 		Gitaly: accessverifier.Gitaly{
-			Address:        "tcp://localhost:9999",
-			Token:          "token",
-			UseSidechannel: true,
+			Address: "tcp://localhost:9999",
+			Token:   "token",
 		},
 	}
 
@@ -71,7 +69,7 @@ func TestCachingOfGitalyConnections(t *testing.T) {
 }
 
 func TestMissingGitalyAddress(t *testing.T) {
-	cmd := GitalyCommand{Config: &config.Config{}}
+	cmd := GitalyCommand{Config: newConfig()}
 
 	err := cmd.RunGitalyCommand(context.Background(), makeHandler(t, nil))
 	require.EqualError(t, err, "no gitaly_address given")
@@ -79,7 +77,7 @@ func TestMissingGitalyAddress(t *testing.T) {
 
 func TestUnavailableGitalyErr(t *testing.T) {
 	cmd := NewGitalyCommand(
-		&config.Config{},
+		newConfig(),
 		string(commandargs.UploadPack),
 		&accessverifier.Response{
 			Gitaly: accessverifier.Gitaly{Address: "tcp://localhost:9999"},
@@ -101,7 +99,7 @@ func TestRunGitalyCommandMetadata(t *testing.T) {
 		{
 			name: "gitaly_feature_flags",
 			gc: NewGitalyCommand(
-				&config.Config{},
+				newConfig(),
 				string(commandargs.UploadPack),
 				&accessverifier.Response{
 					Gitaly: accessverifier.Gitaly{
@@ -209,3 +207,9 @@ func TestPrepareContext(t *testing.T) {
 		})
 	}
 }
+
+func newConfig() *config.Config {
+	cfg := &config.Config{}
+	cfg.GitalyClient.InitSidechannelRegistry(context.Background())
+	return cfg
+}
diff --git a/internal/testhelper/requesthandlers/requesthandlers.go b/internal/testhelper/requesthandlers/requesthandlers.go
index 2fdaef6..6d501d0 100644
--- a/internal/testhelper/requesthandlers/requesthandlers.go
+++ b/internal/testhelper/requesthandlers/requesthandlers.go
@@ -29,14 +29,6 @@ func BuildDisallowedByApiHandlers(t *testing.T) []testserver.TestRequestHandler
 }
 
 func BuildAllowedWithGitalyHandlers(t *testing.T, gitalyAddress string) []testserver.TestRequestHandler {
-	return buildAllowedWithGitalyHandlers(t, gitalyAddress, false)
-}
-
-func BuildAllowedWithGitalyHandlersWithSidechannel(t *testing.T, gitalyAddress string) []testserver.TestRequestHandler {
-	return buildAllowedWithGitalyHandlers(t, gitalyAddress, true)
-}
-
-func buildAllowedWithGitalyHandlers(t *testing.T, gitalyAddress string, useSidechannel bool) []testserver.TestRequestHandler {
 	requests := []testserver.TestRequestHandler{
 		{
 			Path: "/api/v4/internal/allowed",
@@ -64,9 +56,6 @@ func buildAllowedWithGitalyHandlers(t *testing.T, gitalyAddress string, useSidec
 						},
 					},
 				}
-				if useSidechannel {
-					body["gitaly"].(map[string]interface{})["use_sidechannel"] = true
-				}
 				require.NoError(t, json.NewEncoder(w).Encode(body))
 			},
 		},
diff --git a/internal/testhelper/testdata/testroot/responses/allowed_sidechannel.json b/internal/testhelper/testdata/testroot/responses/allowed_sidechannel.json
deleted file mode 100644
index ca3f165..0000000
--- a/internal/testhelper/testdata/testroot/responses/allowed_sidechannel.json
+++ /dev/null
@@ -1,23 +0,0 @@
-{
-	"status": true,
-	"gl_repository": "project-26",
-	"gl_project_path": "group/private",
-	"gl_id": "user-1",
-	"gl_username": "root",
-	"git_config_options": ["option"],
-	"gitaly": {
-		"repository": {
-			"storage_name": "default",
-			"relative_path": "@hashed/5f/9c/5f9c4ab08cac7457e9111a30e4664920607ea2c115a1433d7be98e97e64244ca.git",
-			"git_object_directory": "path/to/git_object_directory",
-			"git_alternate_object_directories": ["path/to/git_alternate_object_directory"],
-			"gl_repository": "project-26",
-			"gl_project_path": "group/private"
-		},
-		"address": "unix:gitaly.socket",
-		"token": "token",
-               "use_sidechannel": true
-	},
-	"git_protocol": "protocol",
-	"gl_console_messages": ["console", "message"]
-}