Merge branch 'pks-remote-support-sha256-object-format' into 'master'

remote: Support for SHA256 object format

See merge request https://gitlab.com/gitlab-org/gitaly/-/merge_requests/5368

Merged-by: Pavlo Strokov <pstrokov@gitlab.com>
Approved-by: Pavlo Strokov <pstrokov@gitlab.com>
Reviewed-by: Patrick Steinhardt <psteinhardt@gitlab.com>
Reviewed-by: Pavlo Strokov <pstrokov@gitlab.com>
Co-authored-by: Patrick Steinhardt <psteinhardt@gitlab.com>

6 files changed, 195 insertions, 166 deletions

diff --git a/internal/gitaly/service/remote/find_remote_repository.go b/internal/gitaly/service/remote/find_remote_repository.go
index bd6f54e82..c4b018a64 100644
--- a/internal/gitaly/service/remote/find_remote_repository.go
+++ b/internal/gitaly/service/remote/find_remote_repository.go
@@ -36,11 +36,27 @@ func (s *server) FindRemoteRepository(ctx context.Context, req *gitalypb.FindRem
 		return &gitalypb.FindRemoteRepositoryResponse{Exists: false}, nil
 	}
 
-	// The output of a successful command is structured like
-	// Regexp would've read better, but this is faster
-	// 58fbff2e0d3b620f591a748c158799ead87b51cd	HEAD
-	fields := bytes.Fields(output)
-	match := len(fields) == 2 && len(fields[0]) == 40 && string(fields[1]) == "HEAD"
+	// The output of git-ls-remote is expected to be of the format:
+	//
+	//	58fbff2e0d3b620f591a748c158799ead87b51cd	HEAD\n
+	objectID, refname, ok := bytes.Cut(output, []byte("\t"))
+	if !ok {
+		return &gitalypb.FindRemoteRepositoryResponse{Exists: false}, nil
+	}
 
-	return &gitalypb.FindRemoteRepositoryResponse{Exists: match}, nil
+	// We've asked for HEAD, so the refname should match that.
+	if !bytes.Equal(refname, []byte("HEAD\n")) {
+		return &gitalypb.FindRemoteRepositoryResponse{Exists: false}, nil
+	}
+
+	// We have no way to ask the remote's object format via git-ls-remote(1), so all we can do
+	// is to verify that the object hash matches something we know.
+	switch len(objectID) {
+	case git.ObjectHashSHA1.EncodedLen():
+		return &gitalypb.FindRemoteRepositoryResponse{Exists: true}, nil
+	case git.ObjectHashSHA256.EncodedLen():
+		return &gitalypb.FindRemoteRepositoryResponse{Exists: true}, nil
+	default:
+		return &gitalypb.FindRemoteRepositoryResponse{Exists: false}, nil
+	}
 }
diff --git a/internal/gitaly/service/remote/find_remote_repository_test.go b/internal/gitaly/service/remote/find_remote_repository_test.go
index 9279648b0..f54383702 100644
--- a/internal/gitaly/service/remote/find_remote_repository_test.go
+++ b/internal/gitaly/service/remote/find_remote_repository_test.go
@@ -1,65 +1,94 @@
-//go:build !gitaly_test_sha256
-
 package remote
 
 import (
-	"bytes"
-	"io"
+	"fmt"
 	"net/http"
 	"net/http/httptest"
+	"path/filepath"
 	"testing"
 
-	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
+	"gitlab.com/gitlab-org/gitaly/v15/internal/structerr"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
-	"google.golang.org/grpc/codes"
 )
 
 func TestFindRemoteRepository(t *testing.T) {
 	t.Parallel()
 
 	ctx := testhelper.Context(t)
-	_, repo, _, client := setupRemoteService(t, ctx)
+	cfg, client := setupRemoteService(t, ctx)
+	gitCmdFactory := gittest.NewCommandFactory(t, cfg)
 
-	ts := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) {
-		infoRefs := testhelper.MustReadFile(t, "testdata/lsremotedata.txt")
-		w.Header().Set("Content-Type", "application/x-git-upload-pack-advertisement")
-		_, err := io.Copy(w, bytes.NewReader(infoRefs))
-		require.NoError(t, err)
-	}))
-	defer ts.Close()
+	type setupData struct {
+		request          *gitalypb.FindRemoteRepositoryRequest
+		expectedErr      error
+		expectedResponse *gitalypb.FindRemoteRepositoryResponse
+	}
 
-	resp, err := client.FindRemoteRepository(ctx, &gitalypb.FindRemoteRepositoryRequest{Remote: ts.URL, StorageName: repo.GetStorageName()})
-	require.NoError(t, err)
+	for _, tc := range []struct {
+		desc  string
+		setup func(t *testing.T) setupData
+	}{
+		{
+			desc: "empty remote",
+			setup: func(t *testing.T) setupData {
+				return setupData{
+					request: &gitalypb.FindRemoteRepositoryRequest{
+						Remote:      "",
+						StorageName: cfg.Storages[0].Name,
+					},
+					expectedErr: structerr.NewInvalidArgument("empty remote can't be checked."),
+				}
+			},
+		},
+		{
+			desc: "nonexistent remote repository",
+			setup: func(t *testing.T) setupData {
+				server := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+					w.WriteHeader(404)
+				}))
+				t.Cleanup(server.Close)
 
-	require.True(t, resp.Exists)
-}
+				return setupData{
+					request: &gitalypb.FindRemoteRepositoryRequest{
+						Remote:      server.URL + "/does-not-exist.git",
+						StorageName: cfg.Storages[0].Name,
+					},
+					expectedResponse: &gitalypb.FindRemoteRepositoryResponse{},
+				}
+			},
+		},
+		{
+			desc: "successful",
+			setup: func(t *testing.T) setupData {
+				_, remoteRepoPath := gittest.CreateRepository(t, ctx, cfg)
+				gittest.WriteCommit(t, cfg, remoteRepoPath, gittest.WithBranch("main"))
 
-func TestFailedFindRemoteRepository(t *testing.T) {
-	t.Parallel()
+				port := gittest.HTTPServer(t, ctx, gitCmdFactory, remoteRepoPath, nil)
 
-	ctx := testhelper.Context(t)
-	_, repo, _, client := setupRemoteService(t, ctx)
+				return setupData{
+					request: &gitalypb.FindRemoteRepositoryRequest{
+						Remote:      fmt.Sprintf("http://127.0.0.1:%d/%s", port, filepath.Base(remoteRepoPath)),
+						StorageName: cfg.Storages[0].Name,
+					},
+					expectedResponse: &gitalypb.FindRemoteRepositoryResponse{
+						Exists: true,
+					},
+				}
+			},
+		},
+	} {
+		tc := tc
 
-	testCases := []struct {
-		description string
-		remote      string
-		exists      bool
-		code        codes.Code
-	}{
-		{"non existing remote", "http://example.com/test.git", false, codes.OK},
-		{"empty remote", "", false, codes.InvalidArgument},
-	}
+		t.Run(tc.desc, func(t *testing.T) {
+			t.Parallel()
 
-	for _, tc := range testCases {
-		resp, err := client.FindRemoteRepository(ctx, &gitalypb.FindRemoteRepositoryRequest{Remote: tc.remote, StorageName: repo.GetStorageName()})
-		if tc.code == codes.OK {
-			require.NoError(t, err)
-		} else {
-			testhelper.RequireGrpcCode(t, err, tc.code)
-			continue
-		}
+			setup := tc.setup(t)
 
-		require.Equal(t, tc.exists, resp.GetExists(), tc.description)
+			response, err := client.FindRemoteRepository(ctx, setup.request)
+			testhelper.RequireGrpcError(t, setup.expectedErr, err)
+			testhelper.ProtoEqual(t, setup.expectedResponse, response)
+		})
 	}
 }
diff --git a/internal/gitaly/service/remote/find_remote_root_ref_test.go b/internal/gitaly/service/remote/find_remote_root_ref_test.go
index 61d5f82bb..42e20dcfb 100644
--- a/internal/gitaly/service/remote/find_remote_root_ref_test.go
+++ b/internal/gitaly/service/remote/find_remote_root_ref_test.go
@@ -1,5 +1,3 @@
-//go:build !gitaly_test_sha256
-
 package remote
 
 import (
@@ -18,129 +16,132 @@ import (
 	"google.golang.org/grpc/status"
 )
 
-func TestFindRemoteRootRefSuccess(t *testing.T) {
+func TestFindRemoteRootRef(t *testing.T) {
 	t.Parallel()
 
 	ctx := testhelper.Context(t)
-	cfg, repo, repoPath, client := setupRemoteService(t, ctx)
+	cfg, client := setupRemoteService(t, ctx)
 	gitCmdFactory := gittest.NewCommandFactory(t, cfg)
 
-	const (
-		host   = "example.com"
-		secret = "mysecret"
-	)
-
-	port := gittest.HTTPServer(t, ctx, gitCmdFactory, repoPath, newGitRequestValidationMiddleware(host, secret))
+	// Even though FindRemoteRootRef does theoretically not require a local repository it is
+	// still bound to one right now. We thus create an empty repository up front that we can
+	// reuse.
+	localRepo, _ := gittest.CreateRepository(t, ctx, cfg)
 
-	originURL := fmt.Sprintf("http://127.0.0.1:%d/%s", port, filepath.Base(repoPath))
+	type setupData struct {
+		request          *gitalypb.FindRemoteRootRefRequest
+		expectedErr      error
+		expectedResponse *gitalypb.FindRemoteRootRefResponse
+	}
 
 	for _, tc := range []struct {
-		desc    string
-		request *gitalypb.FindRemoteRootRefRequest
+		desc  string
+		setup func(t *testing.T) setupData
 	}{
 		{
-			desc: "with remote URL",
-			request: &gitalypb.FindRemoteRootRefRequest{
-				Repository:              repo,
-				RemoteUrl:               originURL,
-				HttpAuthorizationHeader: secret,
-				HttpHost:                host,
+			desc: "invalid repository",
+			setup: func(t *testing.T) setupData {
+				return setupData{
+					request: &gitalypb.FindRemoteRootRefRequest{
+						Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"},
+						RemoteUrl:  "remote-url",
+					},
+					expectedErr: structerr.NewInvalidArgument(testhelper.GitalyOrPraefect(
+						`GetStorageByName: no such storage: "fake"`,
+						"repo scoped: invalid Repository",
+					)),
+				}
 			},
 		},
-	} {
-		t.Run(tc.desc, func(t *testing.T) {
-			response, err := client.FindRemoteRootRef(ctx, tc.request)
-			require.NoError(t, err)
-			require.Equal(t, "master", response.Ref)
-		})
-	}
-}
-
-func TestFindRemoteRootRefWithUnbornRemoteHead(t *testing.T) {
-	t.Parallel()
-
-	ctx := testhelper.Context(t)
-	cfg, remoteRepo, remoteRepoPath, client := setupRemoteService(t, ctx)
-
-	// We're creating an empty repository. Empty repositories do have a HEAD set up, but they
-	// point to an unborn branch because the default branch hasn't yet been created.
-	_, clientRepoPath := gittest.CreateRepository(t, ctx, cfg)
-	gittest.Exec(t, cfg, "-C", remoteRepoPath, "remote", "add", "foo", "file://"+clientRepoPath)
-	response, err := client.FindRemoteRootRef(ctx, &gitalypb.FindRemoteRootRefRequest{
-		Repository: remoteRepo,
-		RemoteUrl:  "file://" + clientRepoPath,
-	},
-	)
-	testhelper.RequireGrpcError(t, status.Error(codes.NotFound, "no remote HEAD found"), err)
-	require.Nil(t, response)
-}
-
-func TestFindRemoteRootRefFailedDueToValidation(t *testing.T) {
-	t.Parallel()
-
-	ctx := testhelper.Context(t)
-	_, repo, _, client := setupRemoteService(t, ctx)
-
-	testCases := []struct {
-		desc        string
-		request     *gitalypb.FindRemoteRootRefRequest
-		expectedErr error
-	}{
 		{
-			desc: "Invalid repository",
-			request: &gitalypb.FindRemoteRootRefRequest{
-				Repository: &gitalypb.Repository{StorageName: "fake", RelativePath: "path"},
-				RemoteUrl:  "remote-url",
+			desc: "missing repository",
+			setup: func(t *testing.T) setupData {
+				return setupData{
+					request: &gitalypb.FindRemoteRootRefRequest{
+						RemoteUrl: "remote-url",
+					},
+					expectedErr: structerr.NewInvalidArgument(testhelper.GitalyOrPraefect(
+						"empty Repository",
+						"repo scoped: empty Repository",
+					)),
+				}
 			},
-			expectedErr: structerr.NewInvalidArgument(testhelper.GitalyOrPraefect(
-				`GetStorageByName: no such storage: "fake"`,
-				"repo scoped: invalid Repository",
-			)),
 		},
 		{
-			desc: "Repository is nil",
-			request: &gitalypb.FindRemoteRootRefRequest{
-				RemoteUrl: "remote-url",
+			desc: "missing remote URL",
+			setup: func(t *testing.T) setupData {
+				return setupData{
+					request: &gitalypb.FindRemoteRootRefRequest{
+						Repository: localRepo,
+					},
+					expectedErr: structerr.NewInvalidArgument("missing remote URL"),
+				}
 			},
-			expectedErr: structerr.NewInvalidArgument(testhelper.GitalyOrPraefect(
-				"empty Repository",
-				"repo scoped: empty Repository",
-			)),
 		},
 		{
-			desc: "Remote URL is empty",
-			request: &gitalypb.FindRemoteRootRefRequest{
-				Repository: repo,
+			desc: "successful",
+			setup: func(t *testing.T) setupData {
+				host := "example.com"
+				secret := "mysecret"
+
+				_, remoteRepoPath := gittest.CreateRepository(t, ctx, cfg)
+				gittest.WriteCommit(t, cfg, remoteRepoPath, gittest.WithBranch("main"))
+
+				port := gittest.HTTPServer(t, ctx, gitCmdFactory, remoteRepoPath, newGitRequestValidationMiddleware(host, secret))
+				originURL := fmt.Sprintf("http://127.0.0.1:%d/%s", port, filepath.Base(remoteRepoPath))
+
+				return setupData{
+					request: &gitalypb.FindRemoteRootRefRequest{
+						Repository:              localRepo,
+						RemoteUrl:               originURL,
+						HttpAuthorizationHeader: secret,
+						HttpHost:                host,
+					},
+					expectedResponse: &gitalypb.FindRemoteRootRefResponse{
+						Ref: "main",
+					},
+				}
 			},
-			expectedErr: structerr.NewInvalidArgument("missing remote URL"),
 		},
-	}
-
-	for _, testCase := range testCases {
-		t.Run(testCase.desc, func(t *testing.T) {
-			_, err := client.FindRemoteRootRef(ctx, testCase.request)
-			testhelper.RequireGrpcError(t, testCase.expectedErr, err)
-		})
-	}
-}
-
-func TestFindRemoteRootRefFailedDueToInvalidRemote(t *testing.T) {
-	t.Parallel()
-
-	ctx := testhelper.Context(t)
-	_, repo, _, client := setupRemoteService(t, ctx)
+		{
+			desc: "unborn HEAD",
+			setup: func(t *testing.T) setupData {
+				_, remoteRepoPath := gittest.CreateRepository(t, ctx, cfg)
+
+				return setupData{
+					request: &gitalypb.FindRemoteRootRefRequest{
+						Repository: localRepo,
+						RemoteUrl:  "file://" + remoteRepoPath,
+					},
+					expectedErr: status.Error(codes.NotFound, "no remote HEAD found"),
+				}
+			},
+		},
+		{
+			desc: "invalid remote URL",
+			setup: func(t *testing.T) setupData {
+				return setupData{
+					request: &gitalypb.FindRemoteRootRefRequest{
+						Repository: localRepo,
+						RemoteUrl:  "file://" + testhelper.TempDir(t),
+					},
+					expectedErr: structerr.New("exit status 128"),
+				}
+			},
+		},
+	} {
+		tc := tc
 
-	t.Run("invalid remote URL", func(t *testing.T) {
-		fakeRepoDir := testhelper.TempDir(t)
+		t.Run(tc.desc, func(t *testing.T) {
+			t.Parallel()
 
-		// We're using a nonexistent filepath remote URL so we avoid hitting the internet.
-		request := &gitalypb.FindRemoteRootRefRequest{
-			Repository: repo, RemoteUrl: "file://" + fakeRepoDir,
-		}
+			setup := tc.setup(t)
 
-		_, err := client.FindRemoteRootRef(ctx, request)
-		testhelper.RequireGrpcCode(t, err, codes.Internal)
-	})
+			response, err := client.FindRemoteRootRef(ctx, setup.request)
+			testhelper.RequireGrpcError(t, setup.expectedErr, err)
+			testhelper.ProtoEqual(t, setup.expectedResponse, response)
+		})
+	}
 }
 
 func newGitRequestValidationMiddleware(host, secret string) func(http.ResponseWriter, *http.Request, http.Handler) {
diff --git a/internal/gitaly/service/remote/testdata/lsremotedata.txt b/internal/gitaly/service/remote/testdata/lsremotedata.txt
deleted file mode 100644
index 197c975d1..000000000
--- a/internal/gitaly/service/remote/testdata/lsremotedata.txt
+++ /dev/null
diff --git a/internal/gitaly/service/remote/testhelper_test.go b/internal/gitaly/service/remote/testhelper_test.go
index 3172d9984..fec38a447 100644
--- a/internal/gitaly/service/remote/testhelper_test.go
+++ b/internal/gitaly/service/remote/testhelper_test.go
@@ -1,12 +1,9 @@
-//go:build !gitaly_test_sha256
-
 package remote
 
 import (
 	"context"
 	"testing"
 
-	"gitlab.com/gitlab-org/gitaly/v15/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/config"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/gitaly/service/repository"
@@ -22,7 +19,7 @@ func TestMain(m *testing.M) {
 	testhelper.Run(m)
 }
 
-func setupRemoteServiceWithoutRepo(t *testing.T, ctx context.Context, opts ...testserver.GitalyServerOpt) (config.Cfg, gitalypb.RemoteServiceClient) {
+func setupRemoteService(t *testing.T, ctx context.Context, opts ...testserver.GitalyServerOpt) (config.Cfg, gitalypb.RemoteServiceClient) {
 	t.Helper()
 
 	cfg := testcfg.Build(t)
@@ -55,18 +52,6 @@ func setupRemoteServiceWithoutRepo(t *testing.T, ctx context.Context, opts ...te
 	return cfg, client
 }
 
-func setupRemoteService(t *testing.T, ctx context.Context, opts ...testserver.GitalyServerOpt) (config.Cfg, *gitalypb.Repository, string, gitalypb.RemoteServiceClient) {
-	t.Helper()
-
-	cfg, client := setupRemoteServiceWithoutRepo(t, ctx, opts...)
-
-	repo, repoPath := gittest.CreateRepository(t, ctx, cfg, gittest.CreateRepositoryConfig{
-		Seed: gittest.SeedGitLabTest,
-	})
-
-	return cfg, repo, repoPath, client
-}
-
 func newRemoteClient(t *testing.T, serverSocketPath string) (gitalypb.RemoteServiceClient, *grpc.ClientConn) {
 	t.Helper()
 
diff --git a/internal/gitaly/service/remote/update_remote_mirror_test.go b/internal/gitaly/service/remote/update_remote_mirror_test.go
index 39b9ca09e..a564d52b6 100644
--- a/internal/gitaly/service/remote/update_remote_mirror_test.go
+++ b/internal/gitaly/service/remote/update_remote_mirror_test.go
@@ -1,5 +1,3 @@
-//go:build !gitaly_test_sha256
-
 package remote
 
 import (
@@ -721,7 +719,7 @@ func TestUpdateRemoteMirror_Validations(t *testing.T) {
 	t.Parallel()
 
 	ctx := testhelper.Context(t)
-	cfg, client := setupRemoteServiceWithoutRepo(t, ctx)
+	cfg, client := setupRemoteService(t, ctx)
 
 	testCases := []struct {
 		expectedErr error