From 0ccd2d02fb61f1584dd7f48b3cca5d745fa7cf9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 1 Dec 2020 09:46:52 +0100 Subject: [PATCH 1/6] helper: move "Unimplemented" variable to its only consumer The only consumer of the "Unimplemented" variable is this one response, let's just move it over there. --- internal/gitaly/service/repository/repository.go | 2 +- internal/helper/error.go | 3 --- 2 files changed, 1 insertion(+), 4 deletions(-) diff --git a/internal/gitaly/service/repository/repository.go b/internal/gitaly/service/repository/repository.go index 6ceb3a101c5..3cb68bdd606 100644 --- a/internal/gitaly/service/repository/repository.go +++ b/internal/gitaly/service/repository/repository.go @@ -11,7 +11,7 @@ import ( // Deprecated func (s *server) Exists(ctx context.Context, in *gitalypb.RepositoryExistsRequest) (*gitalypb.RepositoryExistsResponse, error) { - return nil, helper.Unimplemented + return nil, status.Error(codes.Unimplemented, "this rpc is not implemented") } func (s *server) RepositoryExists(ctx context.Context, in *gitalypb.RepositoryExistsRequest) (*gitalypb.RepositoryExistsResponse, error) { diff --git a/internal/helper/error.go b/internal/helper/error.go index 664989a58c6..904a34228e9 100644 --- a/internal/helper/error.go +++ b/internal/helper/error.go @@ -7,9 +7,6 @@ import ( "google.golang.org/grpc/status" ) -// Unimplemented is a Go error with gRPC error code 'Unimplemented' -var Unimplemented = status.Error(codes.Unimplemented, "this rpc is not implemented") - type statusWrapper struct { error status *status.Status -- GitLab From ba86137a12c8dd65bc1678efff782f1d43861e82 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 1 Dec 2020 09:52:06 +0100 Subject: [PATCH 2/6] helper: get rid of helper.DecorateError use Change this code added in dca53eabd (SearchFilesBy{Content,Name} Server Implementation, 2018-05-04). I'm seeing if I can get us rid of DecorateError magic. In this case we're re-throwing our own errors, which we can just more explicitly cast here. --- internal/gitaly/service/repository/search_files.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/internal/gitaly/service/repository/search_files.go b/internal/gitaly/service/repository/search_files.go index 95888009723..7f254017716 100644 --- a/internal/gitaly/service/repository/search_files.go +++ b/internal/gitaly/service/repository/search_files.go @@ -30,7 +30,7 @@ var contentDelimiter = []byte("--\n") func (s *server) SearchFilesByContent(req *gitalypb.SearchFilesByContentRequest, stream gitalypb.RepositoryService_SearchFilesByContentServer) error { if err := validateSearchFilesRequest(req); err != nil { - return helper.DecorateError(codes.InvalidArgument, err) + return status.Errorf(codes.InvalidArgument, err.Error()) } repo := req.GetRepository() @@ -106,7 +106,7 @@ func sendSearchFilesResultChunked(cmd *command.Command, stream gitalypb.Reposito func (s *server) SearchFilesByName(req *gitalypb.SearchFilesByNameRequest, stream gitalypb.RepositoryService_SearchFilesByNameServer) error { if err := validateSearchFilesRequest(req); err != nil { - return helper.DecorateError(codes.InvalidArgument, err) + return status.Errorf(codes.InvalidArgument, err.Error()) } var filter *regexp.Regexp -- GitLab From d9725dfd8c15b96a14536b05e8593672f153f612 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 1 Dec 2020 09:54:05 +0100 Subject: [PATCH 3/6] WIP: try to get rid of DecorateError() I don't like this for the reasons noted in https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2857#note_457436954 Let's try to get rid of it, by making this die very noisily. Then I can look at the CI fail whale at my leisure and fix any issues if that seems to make sense, as I did for one file in a commit leading up to this. The rest of ErrInvalidArgument() etc. helpers just become a plain wrapper for the equivalent status/code API use, which (if they don't fail) we can eventually just use those APIs directly instead. --- internal/gitaly/service/repository/create.go | 8 +++--- .../service/repository/create_bundle.go | 2 +- internal/gitaly/service/repository/midx.go | 2 +- .../gitaly/service/repository/repository.go | 2 ++ .../gitaly/service/repository/snapshot.go | 2 +- internal/helper/error.go | 28 +++++++------------ 6 files changed, 19 insertions(+), 25 deletions(-) diff --git a/internal/gitaly/service/repository/create.go b/internal/gitaly/service/repository/create.go index b9fc42d3a27..16daec9e5a0 100644 --- a/internal/gitaly/service/repository/create.go +++ b/internal/gitaly/service/repository/create.go @@ -13,11 +13,11 @@ import ( func (s *server) CreateRepository(ctx context.Context, req *gitalypb.CreateRepositoryRequest) (*gitalypb.CreateRepositoryResponse, error) { diskPath, err := s.locator.GetPath(req.GetRepository()) if err != nil { - return nil, helper.ErrInvalidArgumentf("locate repository: %w", err) + return nil, helper.ErrInvalidArgumentf("locate repository: %v", err) } if err := os.MkdirAll(diskPath, 0770); err != nil { - return nil, helper.ErrInternalf("create directories: %w", err) + return nil, helper.ErrInternalf("create directories: %v", err) } stderr := &bytes.Buffer{} @@ -32,11 +32,11 @@ func (s *server) CreateRepository(ctx context.Context, req *gitalypb.CreateRepos }, ) if err != nil { - return nil, helper.ErrInternalf("create git init: %w", err) + return nil, helper.ErrInternalf("create git init: %v", err) } if err := cmd.Wait(); err != nil { - return nil, helper.ErrInternalf("git init stderr: %q, err: %w", stderr, err) + return nil, helper.ErrInternalf("git init stderr: %q, err: %v", stderr, err) } return &gitalypb.CreateRepositoryResponse{}, nil diff --git a/internal/gitaly/service/repository/create_bundle.go b/internal/gitaly/service/repository/create_bundle.go index 4f2d631c5e8..2f57ddbc2c8 100644 --- a/internal/gitaly/service/repository/create_bundle.go +++ b/internal/gitaly/service/repository/create_bundle.go @@ -20,7 +20,7 @@ func (s *server) CreateBundle(req *gitalypb.CreateBundleRequest, stream gitalypb ctx := stream.Context() if _, err := s.Cleanup(ctx, &gitalypb.CleanupRequest{Repository: req.GetRepository()}); err != nil { - return helper.ErrInternalf("running Cleanup on repository: %w", err) + return helper.ErrInternalf("running Cleanup on repository: %v", err) } cmd, err := git.SafeCmd(ctx, repo, nil, git.SubCmd{ diff --git a/internal/gitaly/service/repository/midx.go b/internal/gitaly/service/repository/midx.go index ec04e3dc641..299a229ed79 100644 --- a/internal/gitaly/service/repository/midx.go +++ b/internal/gitaly/service/repository/midx.go @@ -30,7 +30,7 @@ func (s *server) MidxRepack(ctx context.Context, in *gitalypb.MidxRepackRequest) for _, cmd := range []midxSubCommand{midxWrite, midxExpire, s.midxRepack} { if err := s.safeMidxCommand(ctx, repo, cmd); err != nil { if git.IsInvalidArgErr(err) { - return nil, helper.ErrInvalidArgumentf("MidxRepack: %w", err) + return nil, helper.ErrInvalidArgumentf("MidxRepack: %v", err) } return nil, helper.ErrInternal(fmt.Errorf("...%v", err)) diff --git a/internal/gitaly/service/repository/repository.go b/internal/gitaly/service/repository/repository.go index 3cb68bdd606..30e696b2dc2 100644 --- a/internal/gitaly/service/repository/repository.go +++ b/internal/gitaly/service/repository/repository.go @@ -7,6 +7,8 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/storage" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) // Deprecated diff --git a/internal/gitaly/service/repository/snapshot.go b/internal/gitaly/service/repository/snapshot.go index 87a5634792a..d628b53a279 100644 --- a/internal/gitaly/service/repository/snapshot.go +++ b/internal/gitaly/service/repository/snapshot.go @@ -67,7 +67,7 @@ func (s *server) GetSnapshot(in *gitalypb.GetSnapshotRequest, stream gitalypb.Re builder.FileIfExist("shallow") if err := s.addAlternateFiles(stream.Context(), in.GetRepository(), builder); err != nil { - return helper.ErrInternalf("add alternates: %w", err) + return helper.ErrInternalf("add alternates: %v", err) } if err := builder.Close(); err != nil { diff --git a/internal/helper/error.go b/internal/helper/error.go index 904a34228e9..a9f3730ccba 100644 --- a/internal/helper/error.go +++ b/internal/helper/error.go @@ -1,8 +1,6 @@ package helper import ( - "fmt" - "google.golang.org/grpc/codes" "google.golang.org/grpc/status" ) @@ -24,37 +22,31 @@ func (sw statusWrapper) Unwrap() error { // If given nil it will return nil. func DecorateError(code codes.Code, err error) error { if err != nil && GrpcCode(err) == codes.Unknown { + panic("we should not be relying on wrapped errors!") return statusWrapper{err, status.New(code, err.Error())} } return err } -// ErrInternal wraps err with codes.Internal, unless err is already a grpc error -func ErrInternal(err error) error { return DecorateError(codes.Internal, err) } - -// ErrInternalf wrapps a formatted error with codes.Internal, unless err is already a grpc error +func ErrInternal(err error) error { return status.Errorf(codes.Internal, "%s", err.Error()) } func ErrInternalf(format string, a ...interface{}) error { - return ErrInternal(fmt.Errorf(format, a...)) + return status.Errorf(codes.Internal, format, a...) } -// ErrInvalidArgument wraps err with codes.InvalidArgument, unless err is already a grpc error -func ErrInvalidArgument(err error) error { return DecorateError(codes.InvalidArgument, err) } - -// ErrInvalidArgumentf wraps a formatted error with codes.InvalidArgument, unless err is already a grpc error +func ErrInvalidArgument(err error) error { return status.Errorf(codes.InvalidArgument, err.Error()) } func ErrInvalidArgumentf(format string, a ...interface{}) error { - return ErrInvalidArgument(fmt.Errorf(format, a...)) + return status.Errorf(codes.InvalidArgument, format, a...) } -// ErrPreconditionFailed wraps err with codes.FailedPrecondition, unless err is already a grpc error -func ErrPreconditionFailed(err error) error { return DecorateError(codes.FailedPrecondition, err) } +func ErrPreconditionFailed(err error) error { + return status.Errorf(codes.FailedPrecondition, "%s", err.Error()) +} -// ErrPreconditionFailedf wraps a formatted error with codes.FailedPrecondition, unless err is already a grpc error func ErrPreconditionFailedf(format string, a ...interface{}) error { - return ErrPreconditionFailed(fmt.Errorf(format, a...)) + return status.Errorf(codes.FailedPrecondition, format, a...) } -// ErrNotFound wraps error with codes.NotFound, unless err is already a grpc error -func ErrNotFound(err error) error { return DecorateError(codes.NotFound, err) } +func ErrNotFound(err error) error { return status.Errorf(codes.NotFound, "%s", err.Error()) } // GrpcCode emulates the old grpc.Code function: it translates errors into codes.Code values. func GrpcCode(err error) codes.Code { -- GitLab From bb59c08ec9de2b55ca5da533cef4fb68d067d1ba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 1 Dec 2020 11:19:29 +0100 Subject: [PATCH 4/6] Fixes a test failure, just return the existing GRPC error up the stack --- internal/gitaly/service/repository/raw_changes.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gitaly/service/repository/raw_changes.go b/internal/gitaly/service/repository/raw_changes.go index dab1d1fb1d2..136de3e6809 100644 --- a/internal/gitaly/service/repository/raw_changes.go +++ b/internal/gitaly/service/repository/raw_changes.go @@ -20,7 +20,7 @@ func (s *server) GetRawChanges(req *gitalypb.GetRawChangesRequest, stream gitaly repo := req.Repository batch, err := catfile.New(stream.Context(), repo) if err != nil { - return helper.ErrInternal(err) + return err } if err := validateRawChangesRequest(req, batch); err != nil { -- GitLab From 0d6712526a6f63126ba69076c8d9efc05d165804 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 1 Dec 2020 11:23:38 +0100 Subject: [PATCH 5/6] fix a bug in error wrapping --- internal/gitaly/service/repository/raw_changes.go | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/internal/gitaly/service/repository/raw_changes.go b/internal/gitaly/service/repository/raw_changes.go index 136de3e6809..95229d1f5ff 100644 --- a/internal/gitaly/service/repository/raw_changes.go +++ b/internal/gitaly/service/repository/raw_changes.go @@ -14,12 +14,18 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/helper/chunk" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) func (s *server) GetRawChanges(req *gitalypb.GetRawChangesRequest, stream gitalypb.RepositoryService_GetRawChangesServer) error { repo := req.Repository batch, err := catfile.New(stream.Context(), repo) if err != nil { + _, ok := status.FromError(err) + if !ok { + return status.Errorf(codes.Internal, "%s", err.Error()) + } return err } -- GitLab From e7ba030843ff4d6e12764e49134a0529a4bc1314 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 1 Dec 2020 11:25:57 +0100 Subject: [PATCH 6/6] another explicit wrapping --- internal/gitaly/service/repository/replicate.go | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/internal/gitaly/service/repository/replicate.go b/internal/gitaly/service/repository/replicate.go index 992398fb56d..230356b95e2 100644 --- a/internal/gitaly/service/repository/replicate.go +++ b/internal/gitaly/service/repository/replicate.go @@ -34,7 +34,11 @@ func (s *server) ReplicateRepository(ctx context.Context, in *gitalypb.Replicate repoPath, err := s.locator.GetPath(in.GetRepository()) if err != nil { - return nil, helper.ErrInternal(err) + _, ok := status.FromError(err) + if !ok { + return nil, status.Errorf(codes.Internal, "%s", err.Error()) + } + return nil, err } if !storage.IsGitDirectory(repoPath) { -- GitLab