Merge branch 'ps-hook-fix' into 'master'

hook: Standardize errors creation

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

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

8 files changed, 25 insertions, 18 deletions

diff --git a/internal/gitaly/hook/sidechannel.go b/internal/gitaly/hook/sidechannel.go
index 14fa5304e..678e19283 100644
--- a/internal/gitaly/hook/sidechannel.go
+++ b/internal/gitaly/hook/sidechannel.go
@@ -21,10 +21,11 @@ const (
 	sidechannelSocket = "sidechannel"
 )
 
-type errInvalidSidechannelAddress struct{ string }
+// ErrInvalidSidechannelAddress is returned by GetSidechannel if improper address is used.
+type ErrInvalidSidechannelAddress struct{ address string }
 
-func (e *errInvalidSidechannelAddress) Error() string {
-	return fmt.Sprintf("invalid side channel address: %q", e.string)
+func (e ErrInvalidSidechannelAddress) Error() string {
+	return fmt.Sprintf("invalid side channel address: %q", e.address)
 }
 
 // GetSidechannel looks for a sidechannel address in an incoming context
@@ -32,7 +33,7 @@ func (e *errInvalidSidechannelAddress) Error() string {
 func GetSidechannel(ctx context.Context) (net.Conn, error) {
 	address := gitaly_metadata.GetValue(ctx, sidechannelHeader)
 	if path.Base(address) != sidechannelSocket {
-		return nil, &errInvalidSidechannelAddress{address}
+		return nil, ErrInvalidSidechannelAddress{address: address}
 	}
 
 	return net.DialTimeout("unix", address, time.Second)
diff --git a/internal/gitaly/hook/sidechannel_test.go b/internal/gitaly/hook/sidechannel_test.go
index 0cba065c9..78d76d96c 100644
--- a/internal/gitaly/hook/sidechannel_test.go
+++ b/internal/gitaly/hook/sidechannel_test.go
@@ -113,7 +113,7 @@ func TestGetSidechannel(t *testing.T) {
 			)
 			_, err := GetSidechannel(ctx)
 			require.Error(t, err)
-			require.Equal(t, &errInvalidSidechannelAddress{tc}, err)
+			require.Equal(t, ErrInvalidSidechannelAddress{tc}, err)
 		})
 	}
 }
diff --git a/internal/gitaly/service/hook/pack_objects.go b/internal/gitaly/service/hook/pack_objects.go
index 1cdbcead7..975adb998 100644
--- a/internal/gitaly/service/hook/pack_objects.go
+++ b/internal/gitaly/service/hook/pack_objects.go
@@ -381,7 +381,10 @@ func (s *server) PackObjectsHookWithSidechannel(ctx context.Context, req *gitaly
 
 	c, err := gitalyhook.GetSidechannel(ctx)
 	if err != nil {
-		return nil, err
+		if errors.As(err, &gitalyhook.ErrInvalidSidechannelAddress{}) {
+			return nil, helper.ErrInvalidArgument(err)
+		}
+		return nil, helper.ErrInternalf("get side-channel: %w", err)
 	}
 	defer c.Close()
 
@@ -392,11 +395,11 @@ func (s *server) PackObjectsHookWithSidechannel(ctx context.Context, req *gitaly
 			// errors caused by the client disconnecting with the Canceled gRPC code.
 			err = helper.ErrCanceled(err)
 		}
-		return nil, err
+		return nil, helper.ErrInternalf("pack objects hook: %w", err)
 	}
 
 	if err := c.Close(); err != nil {
-		return nil, err
+		return nil, helper.ErrInternalf("close side-channel: %w", err)
 	}
 
 	return &gitalypb.PackObjectsHookWithSidechannelResponse{}, nil
diff --git a/internal/gitaly/service/hook/pack_objects_test.go b/internal/gitaly/service/hook/pack_objects_test.go
index da2ff8289..be0098615 100644
--- a/internal/gitaly/service/hook/pack_objects_test.go
+++ b/internal/gitaly/service/hook/pack_objects_test.go
@@ -522,6 +522,11 @@ func TestServer_PackObjectsHookWithSidechannel_invalidArgument(t *testing.T) {
 			req:         &gitalypb.PackObjectsHookWithSidechannelRequest{Repository: repo, Args: []string{"rm", "-rf"}},
 			expectedErr: status.Error(codes.InvalidArgument, "invalid pack-objects command: [rm -rf]: missing pack-objects"),
 		},
+		{
+			desc:        "no side-channel address",
+			req:         &gitalypb.PackObjectsHookWithSidechannelRequest{Repository: repo, Args: []string{"pack-objects", "--revs", "--stdout"}},
+			expectedErr: status.Error(codes.InvalidArgument, `invalid side channel address: ""`),
+		},
 	}
 
 	for _, tc := range testCases {
diff --git a/internal/gitaly/service/hook/post_receive.go b/internal/gitaly/service/hook/post_receive.go
index 9259a07c3..24982ff5f 100644
--- a/internal/gitaly/service/hook/post_receive.go
+++ b/internal/gitaly/service/hook/post_receive.go
@@ -17,7 +17,7 @@ func postReceiveHookResponse(stream gitalypb.HookService_PostReceiveHookServer,
 		ExitStatus: &gitalypb.ExitStatus{Value: code},
 		Stderr:     []byte(stderr),
 	}); err != nil {
-		return helper.ErrInternalf("sending response: %v", err)
+		return helper.ErrInternalf("sending response: %w", err)
 	}
 
 	return nil
diff --git a/internal/gitaly/service/hook/pre_receive.go b/internal/gitaly/service/hook/pre_receive.go
index 596fd6396..67557f1d2 100644
--- a/internal/gitaly/service/hook/pre_receive.go
+++ b/internal/gitaly/service/hook/pre_receive.go
@@ -65,7 +65,7 @@ func preReceiveHookResponse(stream gitalypb.HookService_PreReceiveHookServer, co
 		ExitStatus: &gitalypb.ExitStatus{Value: code},
 		Stderr:     []byte(stderr),
 	}); err != nil {
-		return helper.ErrInternalf("sending response: %v", err)
+		return helper.ErrInternalf("sending response: %w", err)
 	}
 
 	return nil
diff --git a/internal/gitaly/service/hook/reference_transaction.go b/internal/gitaly/service/hook/reference_transaction.go
index 51fbff981..af6ee8b66 100644
--- a/internal/gitaly/service/hook/reference_transaction.go
+++ b/internal/gitaly/service/hook/reference_transaction.go
@@ -9,8 +9,6 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb"
 	"gitlab.com/gitlab-org/gitaly/v15/streamio"
-	"google.golang.org/grpc/codes"
-	"google.golang.org/grpc/status"
 )
 
 func validateReferenceTransactionHookRequest(in *gitalypb.ReferenceTransactionHookRequest) error {
@@ -50,20 +48,20 @@ func (s *server) ReferenceTransactionHook(stream gitalypb.HookService_ReferenceT
 		request.GetEnvironmentVariables(),
 		stdin,
 	); err != nil {
-		code := codes.Internal
 		switch {
 		case errors.Is(err, transaction.ErrTransactionAborted):
-			code = codes.Aborted
+			return helper.ErrAbortedf("reference-transaction hook: %w", err)
 		case errors.Is(err, transaction.ErrTransactionStopped):
-			code = codes.FailedPrecondition
+			return helper.ErrFailedPreconditionf("reference-transaction hook: %w", err)
+		default:
+			return helper.ErrInternalf("reference-transaction hook: %w", err)
 		}
-		return status.Errorf(code, "reference-transaction hook: %v", err)
 	}
 
 	if err := stream.Send(&gitalypb.ReferenceTransactionHookResponse{
 		ExitStatus: &gitalypb.ExitStatus{Value: 0},
 	}); err != nil {
-		return helper.ErrInternalf("sending response: %v", err)
+		return helper.ErrInternalf("sending response: %w", err)
 	}
 
 	return nil
diff --git a/internal/gitaly/service/hook/update.go b/internal/gitaly/service/hook/update.go
index b79029e0d..393dd1a81 100644
--- a/internal/gitaly/service/hook/update.go
+++ b/internal/gitaly/service/hook/update.go
@@ -53,7 +53,7 @@ func updateHookResponse(stream gitalypb.HookService_UpdateHookServer, code int32
 	if err := stream.Send(&gitalypb.UpdateHookResponse{
 		ExitStatus: &gitalypb.ExitStatus{Value: code},
 	}); err != nil {
-		return helper.ErrInternalf("sending response: %v", err)
+		return helper.ErrInternalf("sending response: %w", err)
 	}
 
 	return nil