From 2326606e06b5853b64268905fc91cd03843bf214 Mon Sep 17 00:00:00 2001 From: Toon Claes Date: Thu, 21 Dec 2023 11:42:35 +0100 Subject: smarthttp: Fix upload-pack using bundle-URI with missing backup When `bundleuri.UploadPackGitConfig()` returns an empty slice, we see the `PostUploadPackWithSidechannel` RPC fail with `FailedPrecondition`. The git-upload-pack(1) process started here quits with the error: fatal: invalid command 'bundle-uri' Looking at the source code of Git, we see when git-upload-pack(1) receives the 'bundle-uri' command, it checks if the that capability should be advertised. To query for this, Git checks if the config `uploadpack.advertiseBundleURIs` is set to `true`. When Gitaly could not build a bundle URI to pass to the git-upload-pack(1), this config was not set, and Git did not accept this command. For the SmartHTTP protocol Gitaly uses separate processes to handle 'GET info/refs' and 'POST upload-pack', using option `--stateless-rpc`. Due to this, the server advertised to the client it supports 'bundle-uri' in the 'GET info/refs' response, but in the consecutive requests we might spawn git-upload-pack(1) without this config, and Git will not accept the 'bundle-uri' command. Inject the config `uploadpack.advertiseBundleURIs=true` into all calls to git-upload-pack(1), even when the bundle-URI cannot be built. Label: bug::availability --- internal/bundleuri/git_config.go | 20 ++++++------- internal/bundleuri/git_config_test.go | 42 +++++++++++++++++++++++++++ internal/gitaly/service/smarthttp/inforefs.go | 2 +- 3 files changed, 53 insertions(+), 11 deletions(-) diff --git a/internal/bundleuri/git_config.go b/internal/bundleuri/git_config.go index 747a99bf2..5eb809480 100644 --- a/internal/bundleuri/git_config.go +++ b/internal/bundleuri/git_config.go @@ -11,10 +11,12 @@ import ( "gitlab.com/gitlab-org/gitaly/v16/internal/log" ) -// InfoRefsGitConfig return a slice of git.ConfigPairs you can inject into the -// call to git-upload-pack(1) --advertise-refs, to advertise the use of -// bundle-URI to the client who clones/fetches from the repository. -func InfoRefsGitConfig(ctx context.Context) []git.ConfigPair { +// CapabilitiesGitConfig returns a slice of git.ConfigPairs that can be injected +// into the Git config to make it aware the bundle-URI capabilities are +// supported. +// This can be used when spawning git-upload-pack(1) --advertise-refs in +// response to the GET /info/refs request. +func CapabilitiesGitConfig(ctx context.Context) []git.ConfigPair { if featureflag.BundleURI.IsDisabled(ctx) { return []git.ConfigPair{} } @@ -30,8 +32,6 @@ func InfoRefsGitConfig(ctx context.Context) []git.ConfigPair { // UploadPackGitConfig return a slice of git.ConfigPairs you can inject into the // call to git-upload-pack(1) to advertise the available bundle to the client // who clones/fetches from the repository. -// In case no backups could be found or something else goes wrong, an empty -// slice is returned without error. func UploadPackGitConfig( ctx context.Context, backupLocator backup.Locator, @@ -39,12 +39,12 @@ func UploadPackGitConfig( repo storage.Repository, ) []git.ConfigPair { if backupLocator == nil || backupSink == nil || featureflag.BundleURI.IsDisabled(ctx) { - return []git.ConfigPair{} + return CapabilitiesGitConfig(ctx) } theBackup, err := backupLocator.FindLatest(ctx, repo) if err != nil { - return []git.ConfigPair{} + return CapabilitiesGitConfig(ctx) } for _, step := range theBackup.Steps { @@ -55,7 +55,7 @@ func UploadPackGitConfig( uri, err := backupSink.SignedURL(ctx, step.BundlePath, 10*time.Minute) if err != nil { - return []git.ConfigPair{} + return CapabilitiesGitConfig(ctx) } log.AddFields(ctx, log.Fields{"bundle_uri": true}) @@ -80,5 +80,5 @@ func UploadPackGitConfig( } } - return []git.ConfigPair{} + return CapabilitiesGitConfig(ctx) } diff --git a/internal/bundleuri/git_config_test.go b/internal/bundleuri/git_config_test.go index c14c3625b..57427fe41 100644 --- a/internal/bundleuri/git_config_test.go +++ b/internal/bundleuri/git_config_test.go @@ -40,18 +40,36 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) { }{ { desc: "no backup locator nor sink", + expectedConfig: []git.ConfigPair{ + { + Key: "uploadpack.advertiseBundleURIs", + Value: "true", + }, + }, }, { desc: "no backup locator", signedURLFunc: func(context.Context, string, time.Duration) (string, error) { return "", structerr.NewNotFound("not signed") }, + expectedConfig: []git.ConfigPair{ + { + Key: "uploadpack.advertiseBundleURIs", + Value: "true", + }, + }, }, { desc: "no backup sink", findLatestFunc: func(context.Context, storage.Repository) (*backup.Backup, error) { return nil, structerr.NewNotFound("no backup found") }, + expectedConfig: []git.ConfigPair{ + { + Key: "uploadpack.advertiseBundleURIs", + Value: "true", + }, + }, }, { desc: "no backup found", @@ -61,6 +79,12 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) { signedURLFunc: func(context.Context, string, time.Duration) (string, error) { return "", structerr.NewNotFound("not signed") }, + expectedConfig: []git.ConfigPair{ + { + Key: "uploadpack.advertiseBundleURIs", + Value: "true", + }, + }, }, { desc: "backup has no steps", @@ -70,6 +94,12 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) { signedURLFunc: func(context.Context, string, time.Duration) (string, error) { return "", structerr.NewNotFound("not signed") }, + expectedConfig: []git.ConfigPair{ + { + Key: "uploadpack.advertiseBundleURIs", + Value: "true", + }, + }, }, { desc: "backup step is incremental", @@ -81,6 +111,12 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) { signedURLFunc: func(context.Context, string, time.Duration) (string, error) { return "", structerr.NewNotFound("not signed") }, + expectedConfig: []git.ConfigPair{ + { + Key: "uploadpack.advertiseBundleURIs", + Value: "true", + }, + }, }, { desc: "sign failed", @@ -92,6 +128,12 @@ func testUploadPackGitConfig(t *testing.T, ctx context.Context) { signedURLFunc: func(context.Context, string, time.Duration) (string, error) { return "", structerr.NewNotFound("not signed") }, + expectedConfig: []git.ConfigPair{ + { + Key: "uploadpack.advertiseBundleURIs", + Value: "true", + }, + }, }, { desc: "success", diff --git a/internal/gitaly/service/smarthttp/inforefs.go b/internal/gitaly/service/smarthttp/inforefs.go index 63be73f0f..2818741de 100644 --- a/internal/gitaly/service/smarthttp/inforefs.go +++ b/internal/gitaly/service/smarthttp/inforefs.go @@ -67,7 +67,7 @@ func (s *server) handleInfoRefs(ctx context.Context, service, repoPath string, r if err != nil { return err } - gitConfig = append(gitConfig, bundleuri.InfoRefsGitConfig(ctx)...) + gitConfig = append(gitConfig, bundleuri.CapabilitiesGitConfig(ctx)...) cmdOpts = append(cmdOpts, git.WithConfig(gitConfig...)) -- cgit v1.2.3