Make git-upload-pack use gitaly-hooks for pack-objects

10 files changed, 220 insertions, 21 deletions

diff --git a/cmd/gitaly-hooks/.gitignore b/cmd/gitaly-hooks/.gitignore
new file mode 100644
index 000000000..d383c56ff
--- /dev/null
+++ b/cmd/gitaly-hooks/.gitignore
@@ -0,0 +1 @@
+testdata
diff --git a/cmd/gitaly-hooks/hooks.go b/cmd/gitaly-hooks/hooks.go
index 4aad5bd9d..2db3f2832 100644
--- a/cmd/gitaly-hooks/hooks.go
+++ b/cmd/gitaly-hooks/hooks.go
@@ -7,7 +7,9 @@ import (
 	"io"
 	"log"
 	"os"
+	"os/exec"
 	"strconv"
+	"strings"
 
 	"github.com/sirupsen/logrus"
 	gitalyauth "gitlab.com/gitlab-org/gitaly/auth"
@@ -23,8 +25,10 @@ import (
 	"google.golang.org/grpc"
 )
 
+var logger *gitalylog.HookLogger
+
 func main() {
-	var logger = gitalylog.NewHookLogger()
+	logger = gitalylog.NewHookLogger()
 
 	if len(os.Args) < 2 {
 		logger.Fatalf("requires hook name. args: %v", os.Args)
@@ -190,6 +194,16 @@ func main() {
 		}, f, os.Stdout, os.Stderr); err != nil {
 			logger.Fatalf("error when receiving data for %q: %v", subCmd, err)
 		}
+	case "git":
+		var args []string
+		for _, a := range os.Args[2:] {
+			args = append(args, fixFilterQuoteBug(a))
+		}
+
+		hookStatus = 0
+		if fallBackGit(args) != nil {
+			hookStatus = 1
+		}
 	default:
 		logger.Fatalf("subcommand name invalid: %q", subCmd)
 	}
@@ -255,3 +269,46 @@ func check(configPath string) (*hook.CheckInfo, error) {
 
 	return hook.NewManager(config.NewLocator(cfg), gitlabAPI, cfg).Check(context.TODO())
 }
+
+// This is a workaround for a bug in Git:
+// https://gitlab.com/gitlab-org/git/-/issues/82. Once that bug is fixed
+// we should no longer need this. The fix function is harmless if the bug
+// is not present.
+func fixFilterQuoteBug(arg string) string {
+	const prefix = "--filter='"
+
+	if !(strings.HasPrefix(arg, prefix) && strings.HasSuffix(arg, "'")) {
+		return arg
+	}
+
+	filterSpec := arg[len(prefix) : len(arg)-1]
+
+	// Perform the inverse of sq_quote_buf() in quote.c. The surrounding quotes
+	// are already gone, we now need to undo escaping of ! and '. The escape
+	// patterns are '\!' and '\'' respectively.
+	filterSpec = strings.ReplaceAll(filterSpec, `'\!'`, `!`)
+	filterSpec = strings.ReplaceAll(filterSpec, `'\''`, `'`)
+
+	return "--filter=" + filterSpec
+}
+
+func fallBackGit(args []string) error {
+	gitCmd := exec.Command(os.Getenv("GITALY_GIT_BIN_PATH"), args...)
+	gitCmd.Stdin = os.Stdin
+	gitCmd.Stdout = os.Stdout
+	gitCmd.Stderr = os.Stderr
+
+	entry := logger.Logger().WithFields(logrus.Fields{
+		"args": args,
+	})
+	const message = "local git command"
+
+	err := gitCmd.Run()
+	if err != nil {
+		entry.WithError(err).Error(message)
+	} else {
+		entry.Info(message)
+	}
+
+	return err
+}
diff --git a/cmd/gitaly-hooks/hooks_test.go b/cmd/gitaly-hooks/hooks_test.go
index b14e5e7ae..8a5060992 100644
--- a/cmd/gitaly-hooks/hooks_test.go
+++ b/cmd/gitaly-hooks/hooks_test.go
@@ -653,3 +653,71 @@ func requireContainsOnce(t *testing.T, s string, contains string) {
 	matches := r.FindAllStringIndex(s, -1)
 	require.Equal(t, 1, len(matches))
 }
+
+func TestFixFilterQuoteBug(t *testing.T) {
+	testCases := []struct{ in, out string }{
+		{"foo bar", "foo bar"},
+		{"--filter=blob:none", "--filter=blob:none"},
+		{"--filter='blob:none'", "--filter=blob:none"},
+		{`--filter='blob'\'':none'`, `--filter=blob':none`},
+		{`--filter='blob'\!':none'`, `--filter=blob!:none`},
+		{`--filter='blob'\'':none'\!''`, `--filter=blob':none!`},
+	}
+
+	for i, tc := range testCases {
+		t.Run(fmt.Sprintf("%d-%s", i, tc.in), func(t *testing.T) {
+			require.Equal(t, tc.out, fixFilterQuoteBug(tc.in))
+		})
+	}
+}
+
+func TestGitalyHooksPackObjects(t *testing.T) {
+	defer func(cfg config.Cfg) {
+		config.Config = cfg
+	}(config.Config)
+
+	testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t)
+	defer cleanupFn()
+
+	logDir, err := filepath.Abs("testdata")
+	require.NoError(t, err)
+	require.NoError(t, os.MkdirAll(logDir, 0755))
+
+	env := append(
+		envForHooks(t, logDir, testRepo, glHookValues{}, proxyValues{}),
+		"GITALY_GIT_BIN_PATH=git",
+	)
+
+	baseArgs := []string{
+		config.Config.Git.BinPath,
+		"clone",
+		"-u",
+		"git -c uploadpack.allowFilter -c uploadpack.packObjectsHook=" + config.Config.BinDir + "/gitaly-hooks upload-pack",
+		"--no-local",
+		"--bare",
+	}
+
+	testCases := []struct {
+		desc      string
+		extraArgs []string
+	}{
+		{desc: "regular clone"},
+		{desc: "shallow clone", extraArgs: []string{"--depth=1"}},
+		{desc: "partial clone", extraArgs: []string{"--filter=blob:none"}},
+	}
+
+	for _, tc := range testCases {
+		t.Run(tc.desc, func(t *testing.T) {
+			tempDir, cleanTempDir := testhelper.TempDir(t)
+			defer cleanTempDir()
+
+			args := append(baseArgs[1:], tc.extraArgs...)
+			args = append(args, testRepoPath, tempDir)
+			cmd := exec.Command(baseArgs[0], args...)
+			cmd.Env = env
+			cmd.Stderr = os.Stderr
+
+			require.NoError(t, cmd.Run())
+		})
+	}
+}
diff --git a/internal/git/hooks_options.go b/internal/git/hooks_options.go
index d18bf8ab0..c7131df07 100644
--- a/internal/git/hooks_options.go
+++ b/internal/git/hooks_options.go
@@ -38,6 +38,30 @@ func WithRefTxHook(ctx context.Context, repo *gitalypb.Repository, cfg config.Cf
 	}
 }
 
+// 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 repo == nil {
+			return fmt.Errorf("missing repo: %w", ErrInvalidArg)
+		}
+
+		payload, err := NewHooksPayload(cfg, repo, nil, nil, nil).Env()
+		if err != nil {
+			return err
+		}
+
+		cc.env = append(
+			cc.env,
+			payload,
+			"GITALY_BIN_DIR="+cfg.BinDir,
+			"GITALY_GIT_BIN_PATH="+cfg.Git.BinPath,
+			fmt.Sprintf("%s=%s", log.GitalyLogDirEnvKey, cfg.Logging.Dir),
+		)
+
+		return nil
+	}
+}
+
 // configureHooks updates the command configuration to include all environment
 // variables required by the reference transaction hook and any other needed
 // options to successfully execute hooks.
diff --git a/internal/gitaly/service/remote/fetch_internal_remote_test.go b/internal/gitaly/service/remote/fetch_internal_remote_test.go
index 3b4f8214a..c5be1ce96 100644
--- a/internal/gitaly/service/remote/fetch_internal_remote_test.go
+++ b/internal/gitaly/service/remote/fetch_internal_remote_test.go
@@ -46,6 +46,8 @@ func TestSuccessfulFetchInternalRemote(t *testing.T) {
 		},
 	}...)
 
+	testhelper.ConfigureGitalyHooksBinary()
+
 	locator := config.NewLocator(config.Config)
 	gitaly0Server := testhelper.NewServer(t, nil, nil, testhelper.WithStorages([]string{"gitaly-0"}))
 	gitalypb.RegisterSSHServiceServer(gitaly0Server.GrpcServer(), ssh.NewServer(config.Config, locator))
diff --git a/internal/gitaly/service/smarthttp/upload_pack.go b/internal/gitaly/service/smarthttp/upload_pack.go
index ec124549a..9e4097b67 100644
--- a/internal/gitaly/service/smarthttp/upload_pack.go
+++ b/internal/gitaly/service/smarthttp/upload_pack.go
@@ -4,12 +4,14 @@ import (
 	"crypto/sha1"
 	"fmt"
 	"io"
+	"path/filepath"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/service/inspect"
+	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
 	"google.golang.org/grpc/codes"
@@ -77,11 +79,21 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 		globalOpts[i] = git.ValueFlag{"-c", o}
 	}
 
+	if featureflag.IsEnabled(ctx, featureflag.UploadPackGitalyHooks) {
+		hookBin := filepath.Join(s.cfg.BinDir, "gitaly-hooks")
+		globalOpts = append(globalOpts, git.ValueFlag{"-c", "uploadpack.packObjectsHook=" + hookBin})
+	}
+
 	cmd, err := git.NewCommandWithoutRepo(ctx, globalOpts, git.SubCmd{
 		Name:  "upload-pack",
 		Flags: []git.Option{git.Flag{Name: "--stateless-rpc"}},
 		Args:  []string{repoPath},
-	}, git.WithStdin(stdin), git.WithStdout(stdout), git.WithGitProtocol(ctx, req))
+	},
+		git.WithStdin(stdin),
+		git.WithStdout(stdout),
+		git.WithGitProtocol(ctx, req),
+		git.WithPackObjectsHookEnv(ctx, req.Repository, s.cfg),
+	)
 
 	if err != nil {
 		return status.Errorf(codes.Unavailable, "PostUploadPack: cmd: %v", err)
diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go
index a6a9ea205..7b4d418c3 100644
--- a/internal/gitaly/service/smarthttp/upload_pack_test.go
+++ b/internal/gitaly/service/smarthttp/upload_pack_test.go
@@ -17,6 +17,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
+	"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"
@@ -28,6 +29,12 @@ const (
 )
 
 func TestSuccessfulUploadPackRequest(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.UploadPackGitalyHooks,
+	}).Run(t, testSuccessfulUploadPackRequest)
+}
+
+func testSuccessfulUploadPackRequest(t *testing.T, ctx context.Context) {
 	negotiationMetrics := prometheus.NewCounterVec(prometheus.CounterOpts{}, []string{"feature"})
 
 	serverSocketPath, stop := runSmartHTTPServer(
@@ -35,9 +42,6 @@ func TestSuccessfulUploadPackRequest(t *testing.T) {
 	)
 	defer stop()
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
 	_, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
@@ -101,12 +105,15 @@ func TestSuccessfulUploadPackRequest(t *testing.T) {
 }
 
 func TestUploadPackRequestWithGitConfigOptions(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.UploadPackGitalyHooks,
+	}).Run(t, testUploadPackRequestWithGitConfigOptions)
+}
+
+func testUploadPackRequestWithGitConfigOptions(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runSmartHTTPServer(t)
 	defer stop()
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
 	_, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
@@ -164,15 +171,18 @@ func TestUploadPackRequestWithGitConfigOptions(t *testing.T) {
 }
 
 func TestUploadPackRequestWithGitProtocol(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.UploadPackGitalyHooks,
+	}).Run(t, testUploadPackRequestWithGitProtocol)
+}
+
+func testUploadPackRequestWithGitProtocol(t *testing.T, ctx context.Context) {
 	restore := testhelper.EnableGitProtocolV2Support(t)
 	defer restore()
 
 	serverSocketPath, stop := runSmartHTTPServer(t)
 	defer stop()
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
 	_, testRepoPath, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
@@ -210,12 +220,15 @@ func TestUploadPackRequestWithGitProtocol(t *testing.T) {
 // on 'deepen' requests even though the request is being handled just
 // fine from the client perspective.
 func TestSuccessfulUploadPackDeepenRequest(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.UploadPackGitalyHooks,
+	}).Run(t, testSuccessfulUploadPackDeepenRequest)
+}
+
+func testSuccessfulUploadPackDeepenRequest(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runSmartHTTPServer(t)
 	defer stop()
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
 	testRepo, _, cleanup := testhelper.NewTestRepo(t)
 	defer cleanup()
 
@@ -233,6 +246,12 @@ func TestSuccessfulUploadPackDeepenRequest(t *testing.T) {
 }
 
 func TestFailedUploadPackRequestDueToValidationError(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.UploadPackGitalyHooks,
+	}).Run(t, testFailedUploadPackRequestDueToValidationError)
+}
+
+func testFailedUploadPackRequestDueToValidationError(t *testing.T, ctx context.Context) {
 	serverSocketPath, stop := runSmartHTTPServer(t)
 	defer stop()
 
@@ -244,9 +263,6 @@ func TestFailedUploadPackRequestDueToValidationError(t *testing.T) {
 
 	for _, rpcRequest := range rpcRequests {
 		t.Run(fmt.Sprintf("%v", rpcRequest), func(t *testing.T) {
-			ctx, cancel := testhelper.Context()
-			defer cancel()
-
 			_, err := makePostUploadPackRequest(ctx, t, serverSocketPath, &rpcRequest, bytes.NewBuffer(nil))
 			testhelper.RequireGrpcError(t, err, codes.InvalidArgument)
 		})
@@ -322,6 +338,12 @@ func extractPackDataFromResponse(t *testing.T, buf *bytes.Buffer) ([]byte, int,
 }
 
 func TestUploadPackRequestForPartialCloneSuccess(t *testing.T) {
+	testhelper.NewFeatureSets([]featureflag.FeatureFlag{
+		featureflag.UploadPackGitalyHooks,
+	}).Run(t, testUploadPackRequestForPartialCloneSuccess)
+}
+
+func testUploadPackRequestForPartialCloneSuccess(t *testing.T, ctx context.Context) {
 	negotiationMetrics := prometheus.NewCounterVec(prometheus.CounterOpts{}, []string{"feature"})
 
 	serverSocketPath, stop := runSmartHTTPServer(
@@ -375,9 +397,6 @@ func TestUploadPackRequestForPartialCloneSuccess(t *testing.T) {
 		},
 	}
 
-	ctx, cancel := testhelper.Context()
-	defer cancel()
-
 	responseBuffer, err := makePostUploadPackRequest(ctx, t, serverSocketPath, req, &requestBuffer)
 	require.NoError(t, err)
 
diff --git a/internal/gitaly/service/ssh/upload_pack.go b/internal/gitaly/service/ssh/upload_pack.go
index 76c5e50dc..879e6b07f 100644
--- a/internal/gitaly/service/ssh/upload_pack.go
+++ b/internal/gitaly/service/ssh/upload_pack.go
@@ -4,6 +4,7 @@ import (
 	"context"
 	"fmt"
 	"io"
+	"path/filepath"
 	"sync"
 
 	"github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus"
@@ -14,6 +15,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/internal/gitaly/service/inspect"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
 )
@@ -83,6 +85,11 @@ func (s *server) sshUploadPack(stream gitalypb.SSHService_SSHUploadPackServer, r
 		globalOpts[i] = git.ValueFlag{"-c", o}
 	}
 
+	if featureflag.IsEnabled(ctx, featureflag.UploadPackGitalyHooks) {
+		hookBin := filepath.Join(s.cfg.BinDir, "gitaly-hooks")
+		globalOpts = append(globalOpts, git.ValueFlag{"-c", "uploadpack.packObjectsHook=" + hookBin})
+	}
+
 	pr, pw := io.Pipe()
 	defer pw.Close()
 	stdin = io.TeeReader(stdin, pw)
@@ -106,7 +113,10 @@ func (s *server) sshUploadPack(stream gitalypb.SSHService_SSHUploadPackServer, r
 	cmd, monitor, err := monitorStdinCommand(ctx, stdin, stdout, stderr, globalOpts, git.SubCmd{
 		Name: "upload-pack",
 		Args: []string{repoPath},
-	}, git.WithGitProtocol(ctx, req))
+	},
+		git.WithGitProtocol(ctx, req),
+		git.WithPackObjectsHookEnv(ctx, req.Repository, s.cfg),
+	)
 
 	if err != nil {
 		return err
diff --git a/internal/log/hook.go b/internal/log/hook.go
index 80f0a4441..ec4987b54 100644
--- a/internal/log/hook.go
+++ b/internal/log/hook.go
@@ -49,3 +49,6 @@ func (h *HookLogger) Fatalf(format string, a ...interface{}) {
 func (h *HookLogger) Errorf(format string, a ...interface{}) {
 	h.logger.Errorf(format, a...)
 }
+
+// Logger returns the underlying logrus logger
+func (h *HookLogger) Logger() *logrus.Logger { return h.logger }
diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go
index 08ef10e65..f0bfa46a2 100644
--- a/internal/metadata/featureflag/feature_flags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -91,6 +91,8 @@ var (
 	TxWikiUpdatePage = FeatureFlag{Name: "tx_wiki_update_page", OnByDefault: false}
 	// TxWikiWritePage enables transactions for WikiWritePage
 	TxWikiWritePage = FeatureFlag{Name: "tx_wiki_write_page", OnByDefault: false}
+	// UploadPackGitalyHooks makes git-upload-pack use gitaly-hooks to run pack-objects
+	UploadPackGitalyHooks = FeatureFlag{Name: "upload_pack_gitaly_hooks", OnByDefault: false}
 )
 
 // All includes all feature flags.
@@ -135,4 +137,5 @@ var All = []FeatureFlag{
 	TxWikiDeletePage,
 	TxWikiUpdatePage,
 	TxWikiWritePage,
+	UploadPackGitalyHooks,
 }