Allow upload pack filters with feature flag

8 files changed, 163 insertions, 19 deletions

diff --git a/changelogs/unreleased/jc-upload-pack-filter.yml b/changelogs/unreleased/jc-upload-pack-filter.yml
new file mode 100644
index 000000000..435b052ea
--- /dev/null
+++ b/changelogs/unreleased/jc-upload-pack-filter.yml
@@ -0,0 +1,5 @@
+---
+title: Allow upload pack filters with feature flag
+merge_request: 1411
+author:
+type: performance
diff --git a/internal/metadata/featureflag/featureflags.go b/internal/metadata/featureflag/feature_flags.go
index 8ddd15d26..9c86fba62 100644
--- a/internal/metadata/featureflag/featureflags.go
+++ b/internal/metadata/featureflag/feature_flags.go
@@ -1,6 +1,9 @@
 package featureflag
 
 const (
+	// UploadPackFilter enables partial clones by sending uploadpack.allowFilter and uploadpack.allowAnySHA1InWant
+	// to upload-pack
+	UploadPackFilter = "upload_pack_filter"
 	// GetAllLFSPointersGo will cause the GetAllLFSPointers RPC to use the go implementation when set
 	GetAllLFSPointersGo = "get_all_lfs_pointers_go"
 )
diff --git a/internal/metadata/featureflag/test_utils.go b/internal/metadata/featureflag/test_utils.go
index 574ca94cc..c0711a92d 100644
--- a/internal/metadata/featureflag/test_utils.go
+++ b/internal/metadata/featureflag/test_utils.go
@@ -6,8 +6,8 @@ import (
 	"google.golang.org/grpc/metadata"
 )
 
-// EnableFeatureFlag is used in tests to enablea a feature flag in the context metadata
-func EnableFeatureFlag(ctx context.Context, flag string) context.Context {
+// ContextWithFeatureFlag is used in tests to enablea a feature flag in the context metadata
+func ContextWithFeatureFlag(ctx context.Context, flag string) context.Context {
 	md, ok := metadata.FromIncomingContext(ctx)
 	if !ok {
 		md = metadata.New(map[string]string{HeaderKey(flag): "true"})
diff --git a/internal/service/blob/lfs_pointers_test.go b/internal/service/blob/lfs_pointers_test.go
index a9036300c..6b86ce7b1 100644
--- a/internal/service/blob/lfs_pointers_test.go
+++ b/internal/service/blob/lfs_pointers_test.go
@@ -409,7 +409,7 @@ func TestSuccessfulGetAllLFSPointersRequest(t *testing.T) {
 
 	// test with go implementation
 	// TODO: remove once feature flag is removed
-	c, err = client.GetAllLFSPointers(featureflag.EnableFeatureFlag(ctx, featureflag.GetAllLFSPointersGo), request)
+	c, err = client.GetAllLFSPointers(featureflag.ContextWithFeatureFlag(ctx, featureflag.GetAllLFSPointersGo), request)
 	require.NoError(t, err)
 
 	require.ElementsMatch(t, expectedLFSPointers, getAllPointers(t, c))
diff --git a/internal/service/repository/rename_test.go b/internal/service/repository/rename_test.go
index de025d938..0b98259f0 100644
--- a/internal/service/repository/rename_test.go
+++ b/internal/service/repository/rename_test.go
@@ -41,7 +41,7 @@ func TestRenameRepositorySuccess(t *testing.T) {
 	require.True(t, helper.IsGitDirectory(newDirectory), "moved Git repository has been corrupted")
 
 	// ensure the git directory that got renamed contains a sha in the seed repo
-	testhelper.MustReachGitObject(t, newDirectory, "913c66a37b4a45b9769037c55c2d238bd0942d2e")
+	testhelper.GitObjectMustExist(t, newDirectory, "913c66a37b4a45b9769037c55c2d238bd0942d2e")
 }
 
 func TestRenameRepositoryDestinationExists(t *testing.T) {
@@ -68,7 +68,7 @@ func TestRenameRepositoryDestinationExists(t *testing.T) {
 	testhelper.RequireGrpcError(t, err, codes.FailedPrecondition)
 
 	// ensure the git directory that already existed didn't get overwritten
-	testhelper.MustReachGitObject(t, destinationRepoPath, sha)
+	testhelper.GitObjectMustExist(t, destinationRepoPath, sha)
 }
 
 func TestRenameRepositoryInvalidRequest(t *testing.T) {
diff --git a/internal/service/smarthttp/upload_pack.go b/internal/service/smarthttp/upload_pack.go
index 14fac54e1..6d1232f76 100644
--- a/internal/service/smarthttp/upload_pack.go
+++ b/internal/service/smarthttp/upload_pack.go
@@ -7,6 +7,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"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"
 	"google.golang.org/grpc/codes"
@@ -62,6 +63,10 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 	}
 
 	args := []string{}
+	if featureflag.IsEnabled(ctx, featureflag.UploadPackFilter) {
+		args = append(args, "-c", "uploadpack.allowFilter=true", "-c", "uploadpack.allowAnySHA1InWant=true")
+	}
+
 	for _, params := range req.GitConfigOptions {
 		args = append(args, "-c", params)
 	}
@@ -83,6 +88,7 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 			deepenCount.Inc()
 			return nil
 		}
+
 		return status.Errorf(codes.Unavailable, "PostUploadPack: %v", err)
 	}
 
diff --git a/internal/service/smarthttp/upload_pack_test.go b/internal/service/smarthttp/upload_pack_test.go
index c45fde676..e7c051d79 100644
--- a/internal/service/smarthttp/upload_pack_test.go
+++ b/internal/service/smarthttp/upload_pack_test.go
@@ -17,6 +17,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
+	"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"
@@ -24,13 +25,16 @@ import (
 )
 
 const (
-	clientCapabilities = `multi_ack_detailed no-done side-band-64k thin-pack include-tag ofs-delta deepen-since deepen-not agent=git/2.18.0`
+	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 TestSuccessfulUploadPackRequest(t *testing.T) {
 	server, serverSocketPath := runSmartHTTPServer(t)
 	defer server.Stop()
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testRepo := testhelper.TestRepository()
 	testRepoPath, err := helper.GetRepoPath(testRepo)
 	require.NoError(t, err)
@@ -76,7 +80,7 @@ func TestSuccessfulUploadPackRequest(t *testing.T) {
 			RelativePath: path.Join(remoteRepoRelativePath, ".git"),
 		},
 	}
-	responseBuffer, err := makePostUploadPackRequest(t, serverSocketPath, req, requestBuffer)
+	responseBuffer, err := makePostUploadPackRequest(ctx, t, serverSocketPath, req, requestBuffer)
 	require.NoError(t, err)
 
 	// There's no git command we can pass it this response and do the work for us (extracting pack file, ...),
@@ -94,6 +98,9 @@ func TestUploadPackRequestWithGitConfigOptions(t *testing.T) {
 	server, serverSocketPath := runSmartHTTPServer(t)
 	defer server.Stop()
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testRepo := testhelper.TestRepository()
 	testRepoPath, err := helper.GetRepoPath(testRepo)
 	require.NoError(t, err)
@@ -133,7 +140,7 @@ func TestUploadPackRequestWithGitConfigOptions(t *testing.T) {
 	}
 
 	// The ref is successfully requested as it is not hidden
-	response, err := makePostUploadPackRequest(t, serverSocketPath, rpcRequest, requestBody)
+	response, err := makePostUploadPackRequest(ctx, t, serverSocketPath, rpcRequest, requestBody)
 	require.NoError(t, err)
 	_, _, count := extractPackDataFromResponse(t, response)
 	assert.Equal(t, 5, count, "pack should have 5 entries")
@@ -141,8 +148,10 @@ func TestUploadPackRequestWithGitConfigOptions(t *testing.T) {
 	// Now the ref is hidden, no packfile will be received. The git process
 	// dies with an error message: `git upload-pack: not our ref ...` but the
 	// client just sees a grpc unavailable error
-	rpcRequest.GitConfigOptions = []string{"uploadpack.hideRefs=refs/hidden"}
-	response, err = makePostUploadPackRequest(t, serverSocketPath, rpcRequest, requestBodyCopy)
+	// we need to set uploadpack.allowAnySHA1InWant=false, because if it's true then we won't encounter an error from setting
+	// uploadpack.hideRefs=refs/hidden. We are setting uploadpack.allowAnySHA1InWant=true in the RPC to enable partial clones
+	rpcRequest.GitConfigOptions = []string{"uploadpack.hideRefs=refs/hidden", "uploadpack.allowAnySHA1InWant=false"}
+	response, err = makePostUploadPackRequest(ctx, t, serverSocketPath, rpcRequest, requestBodyCopy)
 	testhelper.RequireGrpcError(t, err, codes.Unavailable)
 
 	// Remove the if clause if support is dropped for Git versions before 2.22
@@ -161,6 +170,9 @@ func TestUploadPackRequestWithGitProtocol(t *testing.T) {
 	server, serverSocketPath := runSmartHTTPServer(t)
 	defer server.Stop()
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testRepo := testhelper.TestRepository()
 	testRepoPath, err := helper.GetRepoPath(testRepo)
 	require.NoError(t, err)
@@ -188,7 +200,7 @@ func TestUploadPackRequestWithGitProtocol(t *testing.T) {
 	}
 
 	// The ref is successfully requested as it is not hidden
-	_, err = makePostUploadPackRequest(t, serverSocketPath, rpcRequest, requestBody)
+	_, err = makePostUploadPackRequest(ctx, t, serverSocketPath, rpcRequest, requestBody)
 	require.NoError(t, err)
 
 	envData, err := testhelper.GetGitEnvData()
@@ -204,6 +216,9 @@ func TestSuccessfulUploadPackDeepenRequest(t *testing.T) {
 	server, serverSocketPath := runSmartHTTPServer(t)
 	defer server.Stop()
 
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
 	testRepo := testhelper.TestRepository()
 
 	requestBody := &bytes.Buffer{}
@@ -212,7 +227,7 @@ func TestSuccessfulUploadPackDeepenRequest(t *testing.T) {
 	pktline.WriteFlush(requestBody)
 
 	rpcRequest := &gitalypb.PostUploadPackRequest{Repository: testRepo}
-	response, err := makePostUploadPackRequest(t, serverSocketPath, rpcRequest, requestBody)
+	response, err := makePostUploadPackRequest(ctx, t, serverSocketPath, rpcRequest, requestBody)
 
 	// This assertion is the main reason this test exists.
 	assert.NoError(t, err)
@@ -231,17 +246,19 @@ func TestFailedUploadPackRequestDueToValidationError(t *testing.T) {
 
 	for _, rpcRequest := range rpcRequests {
 		t.Run(fmt.Sprintf("%v", rpcRequest), func(t *testing.T) {
-			_, err := makePostUploadPackRequest(t, serverSocketPath, &rpcRequest, bytes.NewBuffer(nil))
+			ctx, cancel := testhelper.Context()
+			defer cancel()
+
+			_, err := makePostUploadPackRequest(ctx, t, serverSocketPath, &rpcRequest, bytes.NewBuffer(nil))
 			testhelper.RequireGrpcError(t, err, codes.InvalidArgument)
 		})
 	}
 }
 
-func makePostUploadPackRequest(t *testing.T, serverSocketPath string, in *gitalypb.PostUploadPackRequest, body io.Reader) (*bytes.Buffer, error) {
+func makePostUploadPackRequest(ctx context.Context, t *testing.T, serverSocketPath string, in *gitalypb.PostUploadPackRequest, body io.Reader) (*bytes.Buffer, error) {
 	client, conn := newSmartHTTPClient(t, serverSocketPath)
 	defer conn.Close()
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
+
 	stream, err := client.PostUploadPack(ctx)
 	require.NoError(t, err)
 
@@ -289,7 +306,9 @@ func extractPackDataFromResponse(t *testing.T, buf *bytes.Buffer) ([]byte, int,
 			pack = append(pack, data[1:]...)
 		}
 	}
+
 	require.NoError(t, scanner.Err())
+	require.NotEmpty(t, pack, "pack data should not be empty")
 
 	// The packet is structured as follows:
 	// 4 bytes for signature, here it's "PACK"
@@ -303,3 +322,100 @@ func extractPackDataFromResponse(t *testing.T, buf *bytes.Buffer) ([]byte, int,
 
 	return pack, version, entries
 }
+
+func TestUploadPackRequestForPartialCloneSuccess(t *testing.T) {
+	server, serverSocketPath := runSmartHTTPServer(t)
+	defer server.Stop()
+
+	testRepo := testhelper.TestRepository()
+	testRepoPath, err := helper.GetRepoPath(testRepo)
+	require.NoError(t, err)
+
+	storagePath := testhelper.GitlabTestStoragePath()
+	remoteRepoRelativePath := "gitlab-test-remote"
+	localRepoRelativePath := "gitlab-test-local"
+	remoteRepoPath := path.Join(storagePath, remoteRepoRelativePath)
+	localRepoPath := path.Join(storagePath, localRepoRelativePath)
+	// Make a non-bare clone of the test repo to act as a remote one
+	testhelper.MustRunCommand(t, nil, "git", "clone", testRepoPath, remoteRepoPath)
+	// Make a bare clone of the test repo to act as a local one and to leave the original repo intact for other tests
+	testhelper.MustRunCommand(t, nil, "git", "init", "--bare", localRepoPath)
+
+	defer os.RemoveAll(localRepoPath)
+	defer os.RemoveAll(remoteRepoPath)
+
+	commitMsg := fmt.Sprintf("Testing UploadPack RPC around %d", time.Now().Unix())
+	committerName := "Scrooge McDuck"
+	committerEmail := "scrooge@mcduck.com"
+
+	testhelper.MustRunCommand(t, nil, "git", "-C", remoteRepoPath,
+		"-c", fmt.Sprintf("user.name=%s", committerName),
+		"-c", fmt.Sprintf("user.email=%s", committerEmail),
+		"commit", "--allow-empty", "-m", commitMsg)
+
+	// The commit ID we want to pull from the remote repo
+	newHead := bytes.TrimSpace(testhelper.MustRunCommand(t, nil, "git", "-C", remoteRepoPath, "rev-parse", "master"))
+	// The commit ID we want to pull from the remote repo
+
+	// UploadPack request is a "want" packet line followed by a packet flush, then many "have" packets followed by a packet flush.
+	// This is explained a bit in https://git-scm.com/book/en/v2/Git-Internals-Transfer-Protocols#_downloading_data
+
+	var requestBuffer, requestBufferForFailed bytes.Buffer
+	pktline.WriteString(&requestBuffer, fmt.Sprintf("want %s %s\n", newHead, clientCapabilities))
+	pktline.WriteString(&requestBuffer, fmt.Sprintf("filter %s\n", "blob:limit=200"))
+	pktline.WriteFlush(&requestBuffer)
+	pktline.WriteString(&requestBuffer, "done\n")
+	pktline.WriteFlush(&requestBuffer)
+
+	req := &gitalypb.PostUploadPackRequest{
+		Repository: &gitalypb.Repository{
+			StorageName:  "default",
+			RelativePath: path.Join(remoteRepoRelativePath, ".git"),
+		},
+	}
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	requestBufferForFailed = requestBuffer
+
+	_, err = makePostUploadPackRequest(ctx, t, serverSocketPath, req, &requestBufferForFailed)
+	require.Error(t, err, "trying to use filters without the feature flag should result in an error")
+
+	ctx = featureflag.ContextWithFeatureFlag(ctx, featureflag.UploadPackFilter)
+
+	responseBuffer, err := makePostUploadPackRequest(ctx, t, serverSocketPath, req, &requestBuffer)
+	require.NoError(t, err)
+
+	pack, version, entries := extractPackDataFromResponse(t, responseBuffer)
+	require.NotNil(t, pack, "Expected to find a pack file in response, found none")
+
+	testhelper.MustRunCommand(t, bytes.NewReader(pack), "git", "-C", localRepoPath, "unpack-objects", fmt.Sprintf("--pack_header=%d,%d", version, entries))
+
+	// a4a132b1b0d6720ca9254440a7ba8a6b9bbd69ec is README.md, which is a small file
+	blobLessThanLimit := "a4a132b1b0d6720ca9254440a7ba8a6b9bbd69ec"
+
+	// c1788657b95998a2f177a4f86d68a60f2a80117f is CONTRIBUTING.md, which is > 200 bytese
+	blobGreaterThanLimit := "c1788657b95998a2f177a4f86d68a60f2a80117f"
+
+	testhelper.GitObjectMustExist(t, localRepoPath, blobLessThanLimit)
+	testhelper.GitObjectMustExist(t, remoteRepoPath, blobGreaterThanLimit)
+	testhelper.GitObjectMustNotExist(t, localRepoPath, blobGreaterThanLimit)
+
+	newBranch := "new-branch"
+	newHead = []byte(testhelper.CreateCommit(t, remoteRepoPath, newBranch, &testhelper.CreateCommitOpts{
+		Message: commitMsg,
+	}))
+
+	// after we delete the branch, we have a dangling commit
+	testhelper.MustRunCommand(t, nil, "git", "-C", remoteRepoPath, "branch", "-D", newBranch)
+
+	requestBuffer.Reset()
+	pktline.WriteString(&requestBuffer, fmt.Sprintf("want %s %s\n", string(newHead), clientCapabilities))
+	// add filtering
+	pktline.WriteFlush(&requestBuffer)
+	pktline.WriteFlush(&requestBuffer)
+
+	_, err = makePostUploadPackRequest(ctx, t, serverSocketPath, req, &requestBuffer)
+	require.NoError(t, err)
+}
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 54e41ce3a..f7809b2ee 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -502,7 +502,21 @@ func TempDir(t *testing.T, dir, prefix string) (string, func() error) {
 	}
 }
 
-// MustReachGitObject is a test assertion that fails unless the git repo in repoPath contains sha
-func MustReachGitObject(t testing.TB, repoPath, sha string) {
-	MustRunCommand(t, nil, "git", "-C", repoPath, "cat-file", "-e", sha)
+// GitObjectMustExist is a test assertion that fails unless the git repo in repoPath contains sha
+func GitObjectMustExist(t testing.TB, repoPath, sha string) {
+	gitObjectExists(t, repoPath, sha, true)
+}
+
+// GitObjectMustNotExist is a test assertion that fails unless the git repo in repoPath contains sha
+func GitObjectMustNotExist(t testing.TB, repoPath, sha string) {
+	gitObjectExists(t, repoPath, sha, false)
+}
+
+func gitObjectExists(t testing.TB, repoPath, sha string, exists bool) {
+	cmd := exec.Command("git", "-C", repoPath, "cat-file", "-e", sha)
+	if exists {
+		require.NoError(t, cmd.Run(), "checking for object should succeed")
+		return
+	}
+	require.Error(t, cmd.Run(), "checking for object should fail")
 }