smarthttp: inforefs: Allow filters when advertising refs

When fetching via smarthttp with filters enabled, then the client
currently gets a warning about the server-side not knowing any of the
filters that the client provides and as a result all filters are
ignored. Naturally, the result is not what the caller usually intends.

Support for filters has been originally implemented for our smarthttp
service in commit a5595e79 (Allow upload pack filters with feature flag,
2019-08-08), seting up required options for the upload-pack service. The
problem with this is that a clone is in fact a multi-phase process for
smarthttp, most importantly it involves the rev-advertisement and then
upload of the pack generated based on advertised revs. So while filter
feature flag was correctly honored in the upload-pack phase, the rev
advertisement doesn't yet and thus also doesn't set up required config
options for git-upload-pack(1). As a result, the client will notice that
the server doesn't support desired filters in the rev-advertisement
phase and as a result turn it off.

Fix the issue by respecting the UploadPackFilter feature flag in our
inforefs service, which handles `git upload-pack --advertise-refs`
amongst others, adding in required flags. Add a test to verify that the
inforefs service correctly handles required capabilities for filters
depending on the feature flag.

3 files changed, 48 insertions, 0 deletions

diff --git a/changelogs/unreleased/pks-smarthttp-upload-pack-filter.yml b/changelogs/unreleased/pks-smarthttp-upload-pack-filter.yml
new file mode 100644
index 000000000..0b5028d43
--- /dev/null
+++ b/changelogs/unreleased/pks-smarthttp-upload-pack-filter.yml
@@ -0,0 +1,5 @@
+---
+title: Allow filters when advertising refs
+merge_request: 1894
+author:
+type: fixed
diff --git a/internal/service/smarthttp/inforefs.go b/internal/service/smarthttp/inforefs.go
index e56745824..1d3fe52f9 100644
--- a/internal/service/smarthttp/inforefs.go
+++ b/internal/service/smarthttp/inforefs.go
@@ -10,6 +10,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/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/streamio"
 	"google.golang.org/grpc/codes"
@@ -54,6 +55,11 @@ func handleInfoRefs(ctx context.Context, service string, req *gitalypb.InfoRefsR
 	if service == "receive-pack" {
 		globalOpts = append(globalOpts, git.ReceivePackConfig()...)
 	}
+
+	if service == "upload-pack" && featureflag.IsEnabled(ctx, featureflag.UploadPackFilter) {
+		globalOpts = append(globalOpts, git.UploadPackFilterConfig()...)
+	}
+
 	for _, o := range req.GitConfigOptions {
 		globalOpts = append(globalOpts, git.ValueFlag{"-c", o})
 	}
diff --git a/internal/service/smarthttp/inforefs_test.go b/internal/service/smarthttp/inforefs_test.go
index 9cbb1fcaa..46935e9ee 100644
--- a/internal/service/smarthttp/inforefs_test.go
+++ b/internal/service/smarthttp/inforefs_test.go
@@ -1,6 +1,7 @@
 package smarthttp
 
 import (
+	"bytes"
 	"context"
 	"fmt"
 	"io"
@@ -15,6 +16,7 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/objectpool"
+	"gitlab.com/gitlab-org/gitaly/internal/git/stats"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
 	"gitlab.com/gitlab-org/gitaly/internal/tempdir"
@@ -42,6 +44,41 @@ func TestSuccessfulInfoRefsUploadPack(t *testing.T) {
 	})
 }
 
+func TestSuccessfulInfoRefsUploadWithPartialClone(t *testing.T) {
+	server, serverSocketPath := runSmartHTTPServer(t)
+	defer server.Stop()
+
+	ctx, cancel := testhelper.Context()
+	defer cancel()
+
+	testRepo := testhelper.TestRepository()
+
+	request := &gitalypb.InfoRefsRequest{
+		Repository: testRepo,
+	}
+
+	fullResponse, err := makeInfoRefsUploadPackRequest(ctx, t, serverSocketPath, request)
+	require.NoError(t, err)
+	fullRefs := stats.Get{}
+	err = fullRefs.Parse(bytes.NewReader(fullResponse))
+	require.NoError(t, err)
+
+	ctx = featureflag.OutgoingCtxWithFeatureFlag(ctx, featureflag.UploadPackFilter)
+
+	partialResponse, err := makeInfoRefsUploadPackRequest(ctx, t, serverSocketPath, request)
+	require.NoError(t, err)
+	partialRefs := stats.Get{}
+	err = partialRefs.Parse(bytes.NewReader(partialResponse))
+	require.NoError(t, err)
+
+	require.Equal(t, fullRefs.Refs(), partialRefs.Refs())
+
+	for _, c := range []string{"allow-tip-sha1-in-want", "allow-reachable-sha1-in-want", "filter"} {
+		require.Contains(t, partialRefs.Caps(), c)
+		require.NotContains(t, fullRefs.Caps(), c)
+	}
+}
+
 func TestSuccessfulInfoRefsUploadPackWithGitConfigOptions(t *testing.T) {
 	server, serverSocketPath := runSmartHTTPServer(t)
 	defer server.Stop()