From efe46743b1b17f85b32f1ae0081c8ad55107492e Mon Sep 17 00:00:00 2001 From: James Fargher Date: Thu, 29 Jul 2021 16:22:19 +1200 Subject: [PATCH 1/2] Extract FetchInternalObject There are several RPCs that need to fetch a specific SHA from a remote repo over gitaly internal ssh. This extraction is intended to centralise this code. --- internal/git/localrepo/internal.go | 74 ++++++++++++++++++++++++++++++ 1 file changed, 74 insertions(+) create mode 100644 internal/git/localrepo/internal.go diff --git a/internal/git/localrepo/internal.go b/internal/git/localrepo/internal.go new file mode 100644 index 00000000000..eb27ab731cd --- /dev/null +++ b/internal/git/localrepo/internal.go @@ -0,0 +1,74 @@ +package localrepo + +import ( + "bytes" + "context" + "fmt" + + "gitlab.com/gitlab-org/gitaly/v14/internal/git" + "gitlab.com/gitlab-org/gitaly/v14/internal/gitalyssh" + "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" +) + +// FetchError records an error during a `git fetch` +type FetchError struct { + source error + stderr []byte +} + +func (e FetchError) Error() string { + if len(e.stderr) == 0 { + return fmt.Sprintf("fetch: %s", e.source.Error()) + } + return fmt.Sprintf("fetch: %s, stderr: %q", e.source.Error(), e.stderr) +} + +// Unwrap satisfies `errors.Unwrap` in order to determine the source of the failure +func (e FetchError) Unwrap() error { + return e.source +} + +// FetchInternalObject fetches a remote object from gitaly internal SSH +func (repo *Repo) FetchInternalObject(ctx context.Context, remoteRepo *gitalypb.Repository, oid git.ObjectID, opts FetchOpts) error { + env, err := gitalyssh.UploadPackEnv(ctx, repo.cfg, &gitalypb.SSHUploadPackRequest{ + Repository: remoteRepo, + GitConfigOptions: []string{"uploadpack.allowAnySHA1InWant=true"}, + }) + if err != nil { + return fmt.Errorf("fetch internal object: upload pack env: %w", err) + } + + env = append(env, opts.Env...) + + var stderr bytes.Buffer + if opts.Stderr == nil { + opts.Stderr = &stderr + } + + commandOptions := []git.CmdOpt{ + git.WithEnv(env...), + git.WithStderr(opts.Stderr), + git.WithRefTxHook(ctx, repo, repo.cfg), + } + commandOptions = append(commandOptions, opts.CommandOptions...) + + cmd, err := repo.Exec(ctx, git.SubCmd{ + Name: "fetch", + Flags: opts.buildFlags(), + Args: []string{gitalyssh.GitalyInternalURL, oid.String()}, + }, + commandOptions..., + ) + if err != nil { + return fmt.Errorf("fetch internal object: %w", err) + } + + if err := cmd.Wait(); err != nil { + return FetchError{ + source: err, + stderr: stderr.Bytes(), + } + } + + return nil +} -- GitLab From 2f65ebbe057b2b6e066b251516c3274e22e45ed1 Mon Sep 17 00:00:00 2001 From: James Fargher Date: Thu, 29 Jul 2021 16:24:25 +1200 Subject: [PATCH 2/2] Replace bespoke fetch implementations with FetchInternalObject --- .../service/conflicts/resolve_conflicts.go | 25 ++--------- .../gitaly/service/operations/commit_files.go | 43 ++----------------- internal/gitaly/service/operations/revert.go | 5 ++- internal/gitaly/service/repository/fetch.go | 34 +++++---------- 4 files changed, 22 insertions(+), 85 deletions(-) diff --git a/internal/gitaly/service/conflicts/resolve_conflicts.go b/internal/gitaly/service/conflicts/resolve_conflicts.go index 155d641be24..908278b871f 100644 --- a/internal/gitaly/service/conflicts/resolve_conflicts.go +++ b/internal/gitaly/service/conflicts/resolve_conflicts.go @@ -20,7 +20,6 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/git/remoterepo" "gitlab.com/gitlab-org/gitaly/v14/internal/git/repository" "gitlab.com/gitlab-org/gitaly/v14/internal/git2go" - "gitlab.com/gitlab-org/gitaly/v14/internal/gitalyssh" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" @@ -292,26 +291,10 @@ func (s *server) repoWithBranchCommit(ctx context.Context, sourceRepo *localrepo return nil } - env, err := gitalyssh.UploadPackEnv(ctx, s.cfg, &gitalypb.SSHUploadPackRequest{ - Repository: targetRepo.Repository, - GitConfigOptions: []string{"uploadpack.allowAnySHA1InWant=true"}, - }) - if err != nil { - return err - } - - var stderr bytes.Buffer - if err := sourceRepo.ExecAndWait(ctx, - git.SubCmd{ - Name: "fetch", - Flags: []git.Option{git.Flag{Name: "--no-tags"}}, - Args: []string{gitalyssh.GitalyInternalURL, oid.String()}, - }, - git.WithStderr(&stderr), - git.WithEnv(env...), - git.WithRefTxHook(ctx, sourceRepo, s.cfg), - ); err != nil { - return fmt.Errorf("could not fetch target commit: %w, stderr: %q", err, stderr.String()) + if err := sourceRepo.FetchInternalObject(ctx, targetRepo.Repository, oid, localrepo.FetchOpts{ + Tags: localrepo.FetchOptsTagsNone, + }); err != nil { + return fmt.Errorf("could not fetch target commit: %w", err) } return nil diff --git a/internal/gitaly/service/operations/commit_files.go b/internal/gitaly/service/operations/commit_files.go index 8e76b7377b5..f859976ec06 100644 --- a/internal/gitaly/service/operations/commit_files.go +++ b/internal/gitaly/service/operations/commit_files.go @@ -17,21 +17,12 @@ import ( "gitlab.com/gitlab-org/gitaly/v14/internal/git/updateref" "gitlab.com/gitlab-org/gitaly/v14/internal/git2go" "gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/storage" - "gitlab.com/gitlab-org/gitaly/v14/internal/gitalyssh" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) -func errorWithStderr(err error, stderr *bytes.Buffer) error { - if stderr.Len() == 0 { - return fmt.Errorf("%w", err) - } - - return fmt.Errorf("%w, stderr: %q", err, stderr) -} - // UserCommitFiles allows for committing from a set of actions. See the protobuf documentation // for details. func (s *Server) UserCommitFiles(stream gitalypb.OperationService_UserCommitFilesServer) error { @@ -411,7 +402,9 @@ func (s *Server) fetchMissingCommit( return fmt.Errorf("lookup parent commit: %w", err) } - if err := s.fetchRemoteObject(ctx, localRepo, remoteRepo, commit); err != nil { + if err := localRepo.FetchInternalObject(ctx, remoteRepo, commit, localrepo.FetchOpts{ + Tags: localrepo.FetchOptsTagsNone, + }); err != nil { return fmt.Errorf("fetch parent commit: %w", err) } } @@ -419,36 +412,6 @@ func (s *Server) fetchMissingCommit( return nil } -func (s *Server) fetchRemoteObject( - ctx context.Context, - localRepo *localrepo.Repo, - remoteRepo *gitalypb.Repository, - oid git.ObjectID, -) error { - env, err := gitalyssh.UploadPackEnv(ctx, s.cfg, &gitalypb.SSHUploadPackRequest{ - Repository: remoteRepo, - GitConfigOptions: []string{"uploadpack.allowAnySHA1InWant=true"}, - }) - if err != nil { - return fmt.Errorf("upload pack env: %w", err) - } - - stderr := &bytes.Buffer{} - if err := localRepo.ExecAndWait(ctx, git.SubCmd{ - Name: "fetch", - Flags: []git.Option{git.Flag{Name: "--no-tags"}}, - Args: []string{"ssh://gitaly/internal.git", oid.String()}, - }, - git.WithEnv(env...), - git.WithStderr(stderr), - git.WithRefTxHook(ctx, localRepo, s.cfg), - ); err != nil { - return errorWithStderr(err, stderr) - } - - return nil -} - func validateUserCommitFilesHeader(header *gitalypb.UserCommitFilesRequestHeader) error { if header.GetRepository() == nil { return fmt.Errorf("empty Repository") diff --git a/internal/gitaly/service/operations/revert.go b/internal/gitaly/service/operations/revert.go index 543b45717d2..3a5c91a0a0f 100644 --- a/internal/gitaly/service/operations/revert.go +++ b/internal/gitaly/service/operations/revert.go @@ -6,6 +6,7 @@ import ( "fmt" "gitlab.com/gitlab-org/gitaly/v14/internal/git" + "gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v14/internal/git/remoterepo" "gitlab.com/gitlab-org/gitaly/v14/internal/git/updateref" "gitlab.com/gitlab-org/gitaly/v14/internal/git2go" @@ -154,7 +155,9 @@ func (s *Server) fetchStartRevision(ctx context.Context, req requestFetchingStar _, err = localRepo.ResolveRevision(ctx, startRevision.Revision()+"^{commit}") if errors.Is(err, git.ErrReferenceNotFound) { - if err := s.fetchRemoteObject(ctx, localRepo, req.GetStartRepository(), startRevision); err != nil { + if err := localRepo.FetchInternalObject(ctx, req.GetStartRepository(), startRevision, localrepo.FetchOpts{ + Tags: localrepo.FetchOptsTagsNone, + }); err != nil { return "", helper.ErrInternalf("fetch start: %w", err) } } else if err != nil { diff --git a/internal/gitaly/service/repository/fetch.go b/internal/gitaly/service/repository/fetch.go index 6c6138e5fa0..d7b9528a1b2 100644 --- a/internal/gitaly/service/repository/fetch.go +++ b/internal/gitaly/service/repository/fetch.go @@ -6,8 +6,8 @@ import ( "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v14/internal/git" + "gitlab.com/gitlab-org/gitaly/v14/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/v14/internal/git/remoterepo" - "gitlab.com/gitlab-org/gitaly/v14/internal/gitalyssh" "gitlab.com/gitlab-org/gitaly/v14/internal/helper" "gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb" ) @@ -70,30 +70,18 @@ func (s *server) FetchSourceBranch(ctx context.Context, req *gitalypb.FetchSourc // There's no need to perform the fetch if we already have the object // available. if !containsObject { - env, err := gitalyssh.UploadPackEnv(ctx, s.cfg, &gitalypb.SSHUploadPackRequest{Repository: req.SourceRepository}) - if err != nil { - return nil, err - } - - cmd, err := s.gitCmdFactory.New(ctx, req.Repository, - git.SubCmd{ - Name: "fetch", - Args: []string{gitalyssh.GitalyInternalURL, sourceOid.String()}, - Flags: []git.Option{git.Flag{Name: "--no-tags"}, git.Flag{Name: "--quiet"}}, - }, - git.WithEnv(env...), - git.WithRefTxHook(ctx, req.Repository, s.cfg), - ) - if err != nil { + if err := targetRepo.FetchInternalObject(ctx, req.GetSourceRepository(), sourceOid, localrepo.FetchOpts{ + Tags: localrepo.FetchOptsTagsNone, + }); err != nil { + if errors.As(err, &localrepo.FetchError{}) { + // Design quirk: if the fetch fails, this RPC returns Result: false, but no error. + ctxlogrus.Extract(ctx). + WithField("oid", sourceOid.String()). + WithError(err).Warn("git fetch failed") + return &gitalypb.FetchSourceBranchResponse{Result: false}, nil + } return nil, err } - if err := cmd.Wait(); err != nil { - // Design quirk: if the fetch fails, this RPC returns Result: false, but no error. - ctxlogrus.Extract(ctx). - WithField("oid", sourceOid.String()). - WithError(err).Warn("git fetch failed") - return &gitalypb.FetchSourceBranchResponse{Result: false}, nil - } } if err := targetRepo.UpdateRef(ctx, git.ReferenceName(req.GetTargetRef()), sourceOid, ""); err != nil { -- GitLab