Merge branch 'pks-smarthttp-upload-pack-filter' into 'master'

Allow filters when advertising refs

See merge request gitlab-org/gitaly!1894

8 files changed, 162 insertions, 63 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/cmd/gitaly-debug/analyzehttp.go b/cmd/gitaly-debug/analyzehttp.go
index 61cf0ce2e..bb2492d17 100644
--- a/cmd/gitaly-debug/analyzehttp.go
+++ b/cmd/gitaly-debug/analyzehttp.go
@@ -22,7 +22,7 @@ func analyzeHTTPClone(cloneURL string) {
 		{"response body time", st.Get.ResponseBody()},
 		{"payload size", st.Get.PayloadSize()},
 		{"Git packets received", st.Get.Packets()},
-		{"refs advertised", st.Get.RefsAdvertised()},
+		{"refs advertised", len(st.Get.Refs())},
 		{"wanted refs", st.RefsWanted()},
 	} {
 		entry.print()
diff --git a/internal/git/stats/analyzehttp.go b/internal/git/stats/analyzehttp.go
index 400839a49..dcbb2ae9a 100644
--- a/internal/git/stats/analyzehttp.go
+++ b/internal/git/stats/analyzehttp.go
@@ -6,6 +6,7 @@ import (
 	"context"
 	"errors"
 	"fmt"
+	"io"
 	"net/http"
 	"os"
 	"strings"
@@ -47,6 +48,10 @@ func ctxErr(ctx context.Context, err error) error {
 	return err
 }
 
+type Reference struct {
+	Oid, Name string
+}
+
 type Get struct {
 	start          time.Time
 	responseHeader time.Duration
@@ -55,7 +60,8 @@ type Get struct {
 	responseBody   time.Duration
 	payloadSize    int64
 	packets        int
-	refs           int
+	refs           []Reference
+	caps           []string
 }
 
 func (g *Get) ResponseHeader() time.Duration { return g.responseHeader }
@@ -64,7 +70,94 @@ func (g *Get) FirstGitPacket() time.Duration { return g.firstGitPacket }
 func (g *Get) ResponseBody() time.Duration   { return g.responseBody }
 func (g *Get) PayloadSize() int64            { return g.payloadSize }
 func (g *Get) Packets() int                  { return g.packets }
-func (g *Get) RefsAdvertised() int           { return g.refs }
+func (g *Get) Refs() []Reference             { return g.refs }
+func (g *Get) Caps() []string                { return g.caps }
+
+type uploadPackState int
+
+const (
+	uploadPackExpectService uploadPackState = iota
+	uploadPackExpectFlush
+	uploadPackExpectRefWithCaps
+	uploadPackExpectRef
+	uploadPackExpectEnd
+)
+
+// Expected response:
+// - "# service=git-upload-pack\n"
+// - FLUSH
+// - "<OID> <ref>\x00<capabilities>\n"
+// - "<OID> <ref>\n"
+// - ...
+// - FLUSH
+func (g *Get) Parse(body io.Reader) error {
+	state := uploadPackExpectService
+	scanner := pktline.NewScanner(body)
+
+	for ; scanner.Scan(); g.packets++ {
+		pkt := scanner.Bytes()
+		data := pktline.Data(pkt)
+		g.payloadSize += int64(len(data))
+
+		switch state {
+		case uploadPackExpectService:
+			g.firstGitPacket = time.Since(g.start)
+			header := string(data)
+			if header != "# service=git-upload-pack\n" {
+				return fmt.Errorf("unexpected header %q", header)
+			}
+
+			state = uploadPackExpectFlush
+		case uploadPackExpectFlush:
+			if !pktline.IsFlush(pkt) {
+				return errors.New("missing flush after service announcement")
+			}
+
+			state = uploadPackExpectRefWithCaps
+		case uploadPackExpectRefWithCaps:
+			split := bytes.Split(data, []byte{0})
+			if len(split) != 2 {
+				return errors.New("invalid first reference line")
+			}
+			g.caps = strings.Split(string(split[1]), " ")
+
+			ref := strings.SplitN(string(split[0]), " ", 2)
+			if len(ref) != 2 {
+				continue
+			}
+			g.refs = append(g.refs, Reference{Oid: ref[0], Name: ref[1]})
+
+			state = uploadPackExpectRef
+		case uploadPackExpectRef:
+			if pktline.IsFlush(pkt) {
+				state = uploadPackExpectEnd
+				continue
+			}
+
+			split := strings.SplitN(string(data), " ", 2)
+			if len(split) != 2 {
+				continue
+			}
+			g.refs = append(g.refs, Reference{Oid: split[0], Name: split[1]})
+		case uploadPackExpectEnd:
+			return errors.New("received packet after flush")
+		}
+	}
+
+	if err := scanner.Err(); err != nil {
+		return err
+	}
+	if len(g.refs) == 0 {
+		return errors.New("received no references")
+	}
+	if len(g.caps) == 0 {
+		return errors.New("received no capabilities")
+	}
+
+	g.responseBody = time.Since(g.start)
+
+	return nil
+}
 
 func (cl *Clone) doGet(ctx context.Context) error {
 	req, err := http.NewRequest("GET", cl.URL+"/info/refs?service=git-upload-pack", nil)
@@ -110,65 +203,15 @@ func (cl *Clone) doGet(ctx context.Context) error {
 		}
 	}
 
-	// Expected response:
-	// - "# service=git-upload-pack\n"
-	// - FLUSH
-	// - "<OID> <ref> <capabilities>\n"
-	// - "<OID> <ref>\n"
-	// - ...
-	// - FLUSH
-	//
-	seenFlush := false
-	scanner := pktline.NewScanner(body)
-	for ; scanner.Scan(); cl.Get.packets++ {
-		if seenFlush {
-			return errors.New("received packet after flush")
-		}
-
-		data := string(pktline.Data(scanner.Bytes()))
-		cl.Get.payloadSize += int64(len(data))
-		switch cl.Get.packets {
-		case 0:
-			cl.Get.firstGitPacket = time.Since(cl.Get.start)
-
-			if data != "# service=git-upload-pack\n" {
-				return fmt.Errorf("unexpected header %q", data)
-			}
-		case 1:
-			if !pktline.IsFlush(scanner.Bytes()) {
-				return errors.New("missing flush after service announcement")
-			}
-		case 2:
-			if !strings.Contains(data, " side-band-64k") {
-				return fmt.Errorf("missing side-band-64k capability in %q", data)
-			}
-
-			fallthrough
-		default:
-			if pktline.IsFlush(scanner.Bytes()) {
-				seenFlush = true
-				continue
-			}
-
-			split := strings.SplitN(data, " ", 2)
-			if len(split) != 2 {
-				continue
-			}
-			cl.Get.refs++
-
-			if strings.HasPrefix(split[1], "refs/heads/") || strings.HasPrefix(split[1], "refs/tags/") {
-				cl.wants = append(cl.wants, split[0])
-			}
-		}
-	}
-	if err := scanner.Err(); err != nil {
+	if err := cl.Get.Parse(body); err != nil {
 		return err
 	}
-	if !seenFlush {
-		return errors.New("missing flush in response")
-	}
 
-	cl.Get.responseBody = time.Since(cl.Get.start)
+	for _, ref := range cl.Get.Refs() {
+		if strings.HasPrefix(ref.Name, "refs/heads/") || strings.HasPrefix(ref.Name, "refs/tags/") {
+			cl.wants = append(cl.wants, ref.Oid)
+		}
+	}
 
 	return nil
 }
diff --git a/internal/git/stats/analyzehttp_test.go b/internal/git/stats/analyzehttp_test.go
index f8d4c4490..75b27eb12 100644
--- a/internal/git/stats/analyzehttp_test.go
+++ b/internal/git/stats/analyzehttp_test.go
@@ -30,6 +30,7 @@ func TestClone(t *testing.T) {
 	require.Equal(t, 200, clone.Get.HTTPStatus(), "get status")
 	require.Greater(t, clone.Get.Packets(), 0, "number of get packets")
 	require.Greater(t, clone.Get.PayloadSize(), int64(0), "get payload size")
+	require.Greater(t, len(clone.Get.Caps()), 10, "get capabilities")
 
 	previousValue := time.Duration(0)
 	for _, m := range []struct {
diff --git a/internal/git/uploadpack.go b/internal/git/uploadpack.go
new file mode 100644
index 000000000..9481e6e16
--- /dev/null
+++ b/internal/git/uploadpack.go
@@ -0,0 +1,10 @@
+package git
+
+// UploadPackFilterConfig confins config options that are required to allow
+// partial-clone filters.
+func UploadPackFilterConfig() []Option {
+	return []Option{
+		ValueFlag{"-c", "uploadpack.allowFilter=true"},
+		ValueFlag{"-c", "uploadpack.allowAnySHA1InWant=true"},
+	}
+}
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()
diff --git a/internal/service/smarthttp/upload_pack.go b/internal/service/smarthttp/upload_pack.go
index 6d20eb0cd..7ed467281 100644
--- a/internal/service/smarthttp/upload_pack.go
+++ b/internal/service/smarthttp/upload_pack.go
@@ -82,10 +82,7 @@ func (s *server) PostUploadPack(stream gitalypb.SmartHTTPService_PostUploadPackS
 
 	var globalOpts []git.Option
 	if featureflag.IsEnabled(ctx, featureflag.UploadPackFilter) {
-		globalOpts = append(globalOpts,
-			git.ValueFlag{"-c", "uploadpack.allowFilter=true"},
-			git.ValueFlag{"-c", "uploadpack.allowAnySHA1InWant=true"},
-		)
+		globalOpts = append(globalOpts, git.UploadPackFilterConfig()...)
 	}
 
 	for _, o := range req.GitConfigOptions {