Merge branch '14-3-sh-pack-objects-hook-cfg' into 'master'

Only activate Git pack-objects hook if cache is enabled

See merge request gitlab-org/gitaly!3816

4 files changed, 96 insertions, 35 deletions

diff --git a/internal/git/hooks_options.go b/internal/git/hooks_options.go
index 7ad3ff9f7..8b03060c7 100644
--- a/internal/git/hooks_options.go
+++ b/internal/git/hooks_options.go
@@ -53,6 +53,10 @@ func WithRefTxHook(ctx context.Context, repo repository.GitRepo, cfg config.Cfg)
 // WithPackObjectsHookEnv provides metadata for gitaly-hooks so it can act as a pack-objects hook.
 func WithPackObjectsHookEnv(ctx context.Context, repo *gitalypb.Repository, cfg config.Cfg) CmdOpt {
 	return func(cc *cmdCfg) error {
+		if !cfg.PackObjectsCache.Enabled {
+			return nil
+		}
+
 		if repo == nil {
 			return fmt.Errorf("missing repo: %w", ErrInvalidArg)
 		}
diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go
index 9d814a25b..8785a2c0e 100644
--- a/internal/gitaly/service/smarthttp/upload_pack_test.go
+++ b/internal/gitaly/service/smarthttp/upload_pack_test.go
@@ -27,11 +27,25 @@ const (
 	clientCapabilities = `multi_ack_detailed no-done side-band-64k thin-pack include-tag ofs-delta deepen-since deepen-not filter agent=git/2.18.0`
 )
 
-func TestServer_PostUploadPack(t *testing.T) {
+func runTestWithAndWithoutConfigOptions(t *testing.T, tf func(t *testing.T, ctx context.Context, opts ...testcfg.Option), opts ...testcfg.Option) {
 	ctx, cancel := testhelper.Context()
 	defer cancel()
 
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
+	t.Run("no config options", func(t *testing.T) { tf(t, ctx) })
+
+	if len(opts) > 0 {
+		t.Run("with config options", func(t *testing.T) {
+			tf(t, ctx, opts...)
+		})
+	}
+}
+
+func TestServer_PostUpload(t *testing.T) {
+	runTestWithAndWithoutConfigOptions(t, testServerPostUpload, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testServerPostUpload(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg, repo, repoPath := testcfg.BuildWithRepo(t, opts...)
 	_, localRepoPath := gittest.CloneRepo(t, cfg, cfg.Storages[0])
 
 	testhelper.BuildGitalyHooks(t, cfg)
@@ -67,10 +81,11 @@ func TestServer_PostUploadPack(t *testing.T) {
 }
 
 func TestServer_PostUploadPack_gitConfigOptions(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testServerPostUploadPackGitConfigOptions, testcfg.WithPackObjectsCacheEnabled())
+}
 
+func testServerPostUploadPackGitConfigOptions(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg, repo, repoPath := testcfg.BuildWithRepo(t, opts...)
 	testhelper.BuildGitalyHooks(t, cfg)
 
 	serverSocketPath := runSmartHTTPServer(t, cfg)
@@ -116,10 +131,11 @@ func TestServer_PostUploadPack_gitConfigOptions(t *testing.T) {
 }
 
 func TestServer_PostUploadPack_gitProtocol(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
+	runTestWithAndWithoutConfigOptions(t, testServerPostUploadPackGitProtocol, testcfg.WithPackObjectsCacheEnabled())
+}
 
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+func testServerPostUploadPackGitProtocol(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 	readProto, cfg := gittest.EnableGitProtocolV2Support(t, cfg)
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
@@ -147,10 +163,11 @@ func TestServer_PostUploadPack_gitProtocol(t *testing.T) {
 // on 'deepen' requests even though the request is being handled just
 // fine from the client perspective.
 func TestServer_PostUploadPack_suppressDeepenExitError(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testServerPostUploadPackSuppressDeepenExitError, testcfg.WithPackObjectsCacheEnabled())
+}
 
+func testServerPostUploadPackSuppressDeepenExitError(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
 	requestBody := &bytes.Buffer{}
@@ -167,7 +184,7 @@ func TestServer_PostUploadPack_suppressDeepenExitError(t *testing.T) {
 }
 
 func TestServer_PostUploadPack_usesPackObjectsHook(t *testing.T) {
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
+	cfg, repo, repoPath := testcfg.BuildWithRepo(t, testcfg.WithPackObjectsCacheEnabled())
 	cfg.BinDir = testhelper.TempDir(t)
 
 	outputPath := filepath.Join(cfg.BinDir, "output")
@@ -206,10 +223,11 @@ func TestServer_PostUploadPack_usesPackObjectsHook(t *testing.T) {
 }
 
 func TestServer_PostUploadPack_validation(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
+	runTestWithAndWithoutConfigOptions(t, testServerPostUploadPackValidation, testcfg.WithPackObjectsCacheEnabled())
+}
 
-	cfg := testcfg.Build(t)
+func testServerPostUploadPackValidation(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg := testcfg.Build(t, opts...)
 	serverSocketPath := runSmartHTTPServer(t, cfg)
 
 	rpcRequests := []*gitalypb.PostUploadPackRequest{
@@ -295,10 +313,11 @@ func extractPackDataFromResponse(t *testing.T, buf *bytes.Buffer) ([]byte, int,
 }
 
 func TestServer_PostUploadPack_partialClone(t *testing.T) {
-	ctx, cancel := testhelper.Context()
-	defer cancel()
+	runTestWithAndWithoutConfigOptions(t, testServerPostUploadPackPartialClone, testcfg.WithPackObjectsCacheEnabled())
+}
 
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
+func testServerPostUploadPackPartialClone(t *testing.T, ctx context.Context, opts ...testcfg.Option) {
+	cfg, repo, repoPath := testcfg.BuildWithRepo(t, opts...)
 	_, localRepoPath := gittest.InitRepo(t, cfg, cfg.Storages[0])
 
 	testhelper.BuildGitalyHooks(t, cfg)
diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go
index d6107e5de..bdc7de33d 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -36,6 +36,16 @@ type cloneCommand struct {
 	cfg          config.Cfg
 }
 
+func runTestWithAndWithoutConfigOptions(t *testing.T, tf func(t *testing.T, opts ...testcfg.Option), opts ...testcfg.Option) {
+	t.Run("no config options", func(t *testing.T) { tf(t) })
+
+	if len(opts) > 0 {
+		t.Run("with config options", func(t *testing.T) {
+			tf(t, opts...)
+		})
+	}
+}
+
 func (cmd cloneCommand) execute(t *testing.T) error {
 	req := &gitalypb.SSHUploadPackRequest{
 		Repository:  cmd.repository,
@@ -89,7 +99,11 @@ func (cmd cloneCommand) test(t *testing.T, cfg config.Cfg, repoPath string, loca
 }
 
 func TestFailedUploadPackRequestDueToTimeout(t *testing.T) {
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testFailedUploadPackRequestDueToTimeout, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testFailedUploadPackRequestDueToTimeout(t *testing.T, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 
 	serverSocketPath := runSSHServerWithOptions(t, cfg, []ServerOpt{WithUploadPackRequestTimeout(10 * time.Microsecond)})
 
@@ -195,7 +209,11 @@ func TestFailedUploadPackRequestDueToValidationError(t *testing.T) {
 }
 
 func TestUploadPackCloneSuccess(t *testing.T) {
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testUploadPackCloneSuccess, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testUploadPackCloneSuccess(t *testing.T, opts ...testcfg.Option) {
+	cfg, repo, repoPath := testcfg.BuildWithRepo(t, opts...)
 
 	testhelper.BuildGitalyHooks(t, cfg)
 	testhelper.BuildGitalySSH(t, cfg)
@@ -207,10 +225,9 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 	localRepoPath := testhelper.TempDir(t)
 
 	tests := []struct {
-		cmd          *exec.Cmd
-		desc         string
-		deepen       float64
-		featureFlags []string
+		cmd    *exec.Cmd
+		desc   string
+		deepen float64
 	}{
 		{
 			cmd:    exec.Command(cfg.Git.BinPath, "clone", "git@localhost:test/test.git", localRepoPath),
@@ -229,11 +246,10 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 			negotiationMetrics.Reset()
 
 			cmd := cloneCommand{
-				repository:   repo,
-				command:      tc.cmd,
-				featureFlags: tc.featureFlags,
-				server:       serverSocketPath,
-				cfg:          cfg,
+				repository: repo,
+				command:    tc.cmd,
+				server:     serverSocketPath,
+				cfg:        cfg,
 			}
 			lHead, rHead, _, _ := cmd.test(t, cfg, repoPath, localRepoPath)
 			require.Equal(t, lHead, rHead, "local and remote head not equal")
@@ -246,7 +262,7 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 }
 
 func TestUploadPackWithPackObjectsHook(t *testing.T) {
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	cfg, repo, _ := testcfg.BuildWithRepo(t, testcfg.WithPackObjectsCacheEnabled())
 
 	filterDir := testhelper.TempDir(t)
 	outputPath := filepath.Join(filterDir, "output")
@@ -286,7 +302,11 @@ exec '%s' "$@"
 }
 
 func TestUploadPackWithoutSideband(t *testing.T) {
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testUploadPackWithoutSideband, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testUploadPackWithoutSideband(t *testing.T, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 
 	testhelper.BuildGitalySSH(t, cfg)
 	testhelper.BuildGitalyHooks(t, cfg)
@@ -329,7 +349,11 @@ func TestUploadPackWithoutSideband(t *testing.T) {
 }
 
 func TestUploadPackCloneWithPartialCloneFilter(t *testing.T) {
-	cfg, repo, _ := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testUploadPackCloneWithPartialCloneFilter, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testUploadPackCloneWithPartialCloneFilter(t *testing.T, opts ...testcfg.Option) {
+	cfg, repo, _ := testcfg.BuildWithRepo(t, opts...)
 
 	testhelper.BuildGitalySSH(t, cfg)
 	testhelper.BuildGitalyHooks(t, cfg)
@@ -386,7 +410,11 @@ func TestUploadPackCloneWithPartialCloneFilter(t *testing.T) {
 }
 
 func TestUploadPackCloneSuccessWithGitProtocol(t *testing.T) {
-	cfg, repo, repoPath := testcfg.BuildWithRepo(t)
+	runTestWithAndWithoutConfigOptions(t, testUploadPackCloneSuccessWithGitProtocol, testcfg.WithPackObjectsCacheEnabled())
+}
+
+func testUploadPackCloneSuccessWithGitProtocol(t *testing.T, opts ...testcfg.Option) {
+	cfg, repo, repoPath := testcfg.BuildWithRepo(t, opts...)
 
 	testhelper.BuildGitalySSH(t, cfg)
 	testhelper.BuildGitalyHooks(t, cfg)
diff --git a/internal/testhelper/testcfg/gitaly_builder.go b/internal/testhelper/testcfg/gitaly_builder.go
index cd21b8442..6d65556a2 100644
--- a/internal/testhelper/testcfg/gitaly_builder.go
+++ b/internal/testhelper/testcfg/gitaly_builder.go
@@ -41,6 +41,13 @@ func WithRealLinguist() Option {
 	}
 }
 
+// WithPackObjectsCacheEnabled enables the pack object cache.
+func WithPackObjectsCacheEnabled() Option {
+	return func(builder *GitalyCfgBuilder) {
+		builder.packObjectsCacheEnabled = true
+	}
+}
+
 // NewGitalyCfgBuilder returns gitaly configuration builder with configured set of options.
 func NewGitalyCfgBuilder(opts ...Option) GitalyCfgBuilder {
 	cfgBuilder := GitalyCfgBuilder{}
@@ -56,8 +63,9 @@ func NewGitalyCfgBuilder(opts ...Option) GitalyCfgBuilder {
 type GitalyCfgBuilder struct {
 	cfg config.Cfg
 
-	storages     []string
-	realLinguist bool
+	storages                []string
+	realLinguist            bool
+	packObjectsCacheEnabled bool
 }
 
 // Build setups required filesystem structure, creates and returns configuration of the gitaly service.
@@ -121,6 +129,8 @@ func (gc *GitalyCfgBuilder) Build(t testing.TB) config.Cfg {
 		}
 	}
 
+	cfg.PackObjectsCache.Enabled = gc.packObjectsCacheEnabled
+
 	require.NoError(t, testhelper.ConfigureRuby(&cfg))
 	require.NoError(t, cfg.Validate())