helper: Implement helper to create gRPC errors with details

The gRPC error model allows for two different usages: first the standard
error model where one can return a simple error message associated with
a code. And then the richer error model, where structured data can be
added to errors such that details about the failure mode can passed to
the caller [1]. In Gitaly, we always use the simple error model.

This simple error model is quite limiting though. For example in the
operations service, we have resorted to pass error information to the
caller by embedding error details into the normal response. While not
elegant, it has served us alright in the past. But this design is
awkward, has issues with Praefect if it inspects proxied errors, and is
just bad design in general.

To fix this, this commit implements a new helper function which will
enrich a gRPC status error with additional details. These details can be
an arbitrary protobuf message. Like this, we can pass up arbitrary
structured data to the caller, which will allow us to fix these
shortcomings.

[1]: https://www.grpc.io/docs/guides/error/

2 files changed, 127 insertions, 0 deletions

diff --git a/internal/helper/error.go b/internal/helper/error.go
index 62724f16b..10551c013 100644
--- a/internal/helper/error.go
+++ b/internal/helper/error.go
@@ -6,6 +6,8 @@ import (
 
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/proto"
+	"google.golang.org/protobuf/types/known/anypb"
 )
 
 type statusWrapper struct {
@@ -102,6 +104,30 @@ func formatError(code codes.Code, format string, a ...interface{}) error {
 	return statusWrapper{err, status.New(code, err.Error())}
 }
 
+// ErrWithDetails adds the given details to the error if it is a gRPC status whose code is not OK.
+func ErrWithDetails(err error, details ...proto.Message) (error, error) {
+	if GrpcCode(err) == codes.OK {
+		return nil, fmt.Errorf("no error given")
+	}
+
+	st, ok := status.FromError(err)
+	if !ok {
+		return nil, fmt.Errorf("error is not a gRPC status")
+	}
+
+	proto := st.Proto()
+	for _, detail := range details {
+		marshaled, err := anypb.New(detail)
+		if err != nil {
+			return nil, err
+		}
+
+		proto.Details = append(proto.Details, marshaled)
+	}
+
+	return statusWrapper{err, status.FromProto(proto)}, nil
+}
+
 // GrpcCode emulates the old grpc.Code function: it translates errors into codes.Code values.
 func GrpcCode(err error) codes.Code {
 	if err == nil {
diff --git a/internal/helper/error_test.go b/internal/helper/error_test.go
index 614b7b8b6..65f7bab91 100644
--- a/internal/helper/error_test.go
+++ b/internal/helper/error_test.go
@@ -7,8 +7,11 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/v14/internal/testhelper/testassert"
+	"gitlab.com/gitlab-org/gitaly/v14/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/proto"
 )
 
 func TestError(t *testing.T) {
@@ -161,3 +164,101 @@ func testErrorfFormat(t *testing.T, errorFormat, errorFormatEqual string) {
 		})
 	}
 }
+
+func TestErrWithDetails(t *testing.T) {
+	for _, tc := range []struct {
+		desc            string
+		err             error
+		details         []proto.Message
+		expectedErr     error
+		expectedMessage string
+		expectedDetails []proto.Message
+		expectedCode    codes.Code
+	}{
+		{
+			desc:        "no error",
+			expectedErr: errors.New("no error given"),
+		},
+		{
+			desc:        "status with OK code",
+			err:         status.Error(codes.OK, "message"),
+			expectedErr: errors.New("no error given"),
+		},
+		{
+			desc:        "normal error",
+			err:         errors.New("message"),
+			expectedErr: errors.New("error is not a gRPC status"),
+		},
+		{
+			desc:            "status",
+			err:             status.Error(codes.FailedPrecondition, "message"),
+			expectedMessage: "rpc error: code = FailedPrecondition desc = message",
+			expectedCode:    codes.FailedPrecondition,
+		},
+		{
+			desc: "status with details",
+			err:  status.Error(codes.FailedPrecondition, "message"),
+			details: []proto.Message{
+				&gitalypb.Repository{},
+			},
+			expectedMessage: "rpc error: code = FailedPrecondition desc = message",
+			expectedCode:    codes.FailedPrecondition,
+			expectedDetails: []proto.Message{
+				&gitalypb.Repository{},
+			},
+		},
+		{
+			desc: "status with multiple details",
+			err:  status.Error(codes.FailedPrecondition, "message"),
+			details: []proto.Message{
+				&gitalypb.Repository{RelativePath: "a"},
+				&gitalypb.Repository{RelativePath: "b"},
+			},
+			expectedMessage: "rpc error: code = FailedPrecondition desc = message",
+			expectedCode:    codes.FailedPrecondition,
+			expectedDetails: []proto.Message{
+				&gitalypb.Repository{RelativePath: "a"},
+				&gitalypb.Repository{RelativePath: "b"},
+			},
+		},
+		{
+			desc: "status with mixed type details",
+			err:  status.Error(codes.FailedPrecondition, "message"),
+			details: []proto.Message{
+				&gitalypb.Repository{},
+				&gitalypb.GitCommit{},
+			},
+			expectedMessage: "rpc error: code = FailedPrecondition desc = message",
+			expectedCode:    codes.FailedPrecondition,
+			expectedDetails: []proto.Message{
+				&gitalypb.Repository{},
+				&gitalypb.GitCommit{},
+			},
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			detailedErr, err := ErrWithDetails(tc.err, tc.details...)
+			require.Equal(t, tc.expectedErr, err)
+			if err != nil {
+				return
+			}
+
+			require.Equal(t, tc.expectedMessage, detailedErr.Error())
+
+			st, ok := status.FromError(detailedErr)
+			require.True(t, ok, "error should be a status")
+			require.Equal(t, tc.expectedCode, st.Code())
+
+			statusProto := st.Proto()
+			require.NotNil(t, statusProto)
+
+			var details []proto.Message
+			for _, detail := range statusProto.GetDetails() {
+				detailProto, err := detail.UnmarshalNew()
+				require.NoError(t, err)
+				details = append(details, detailProto)
+			}
+			testassert.ProtoEqual(t, tc.expectedDetails, details)
+		})
+	}
+}