structerr: Add interceptors to inject metadata as error details

It is quite hard right now to properly test whether metadata is injected
as expected as it is only used for logging purposes. This is further
complicated by the fact that by default, the metadata is only attached
to log messages that are dispatched asynchronously after we have already
returned the error to the client. This causes flakiness in one of our
tests.

Metadata is really not supposed to be seen by clients at all: they are
intended only for logging purposes and are thus not stable information
that any client may rely on. So it is not immediately obvious how we can
remedy that situation. There are multiple alternatives:

    - Don't test error metadata at all when it crosses the gRPC
      boundary. This is the easiest to implement, but it may make it
      hard to see what's going on.

    - Provide a mechanism to make the logging synchronous. We cannot do
      this in production code as we rely on the `grpc/stats.End` message
      to obtain information on how many bytes we have sent and received,
      and that can only ever be available after we have sent the final
      error.

    - Convert error metadata into error details. This may cause clients
      to start parsing them though and thus rely on implemenation
      details that are subject to change.

This commit opts for the last option, but makes the logic conditional so
that we only convert error metadata when using our test server setup.
It has the significant downside that errors are now different depending
on whether we are testing or not. But the interceptors really only add
additional information, which should hopefully be fine.

The logic is not yet wired up.

7 files changed, 309 insertions, 37 deletions

diff --git a/internal/structerr/error.go b/internal/structerr/error.go
index 7cff2bb0d..fdaf132df 100644
--- a/internal/structerr/error.go
+++ b/internal/structerr/error.go
@@ -4,6 +4,7 @@ import (
 	"errors"
 	"fmt"
 
+	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb/testproto"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/status"
 	"google.golang.org/protobuf/proto"
@@ -288,6 +289,17 @@ func (e Error) WithMetadata(key string, value any) Error {
 	return e
 }
 
+// WithInterceptedMetadata adds an additional metadata item to the Error in the form of an error
+// detail. Note that this is only intended to be used in the context of tests where we convert error
+// metadata into structured errors via the UnaryInterceptor and StreamInterceptor so that we can
+// test that metadata has been set as expected on the client-side of a gRPC call.
+func (e Error) WithInterceptedMetadata(key string, value any) Error {
+	return e.WithDetail(&testproto.ErrorMetadata{
+		Key:   []byte(key),
+		Value: []byte(fmt.Sprintf("%v", value)),
+	})
+}
+
 // Details returns the chain error details set by this and any wrapped Error. The returned array
 // contains error details ordered from top-level error details to bottom-level error details.
 func (e Error) Details() []proto.Message {
diff --git a/internal/structerr/fields_producer.go b/internal/structerr/fields_producer.go
deleted file mode 100644
index 367f149e7..000000000
--- a/internal/structerr/fields_producer.go
+++ /dev/null
@@ -1,26 +0,0 @@
-package structerr
-
-import (
-	"context"
-	"errors"
-
-	"github.com/sirupsen/logrus"
-)
-
-// FieldsProducer extracts metadata from err if it contains a `structerr.Error` and exposes it as
-// logged fields. This function is supposed to be used with `log.MessageProducer()`.
-func FieldsProducer(_ context.Context, err error) logrus.Fields {
-	var structErr Error
-	if errors.As(err, &structErr) {
-		metadata := structErr.Metadata()
-		if len(metadata) == 0 {
-			return nil
-		}
-
-		return logrus.Fields{
-			"error_metadata": metadata,
-		}
-	}
-
-	return nil
-}
diff --git a/internal/structerr/grpc_server.go b/internal/structerr/grpc_server.go
new file mode 100644
index 000000000..dec2d44d2
--- /dev/null
+++ b/internal/structerr/grpc_server.go
@@ -0,0 +1,70 @@
+package structerr
+
+import (
+	"context"
+	"errors"
+	"sort"
+
+	"github.com/sirupsen/logrus"
+	"google.golang.org/grpc"
+)
+
+// UnaryInterceptor is an interceptor for unary RPC calls that injects error metadata as detailed
+// error. This is only supposed to be used for testing purposes as error metadata is considered to
+// be a server-side detail. No clients should start to rely on it.
+func UnaryInterceptor(ctx context.Context, req interface{}, info *grpc.UnaryServerInfo, handler grpc.UnaryHandler) (interface{}, error) {
+	response, err := handler(ctx, req)
+	return response, interceptedError(err)
+}
+
+// StreamInterceptor is an interceptor for streaming RPC calls that injects error metadata as
+// detailed error. This is only supposed to be used for testing purposes as error metadata is
+// considered to be a server-side detail. No clients should start to rely on it.
+func StreamInterceptor(srv interface{}, stream grpc.ServerStream, info *grpc.StreamServerInfo, handler grpc.StreamHandler) error {
+	return interceptedError(handler(srv, stream))
+}
+
+func interceptedError(err error) error {
+	var structErr Error
+	if errors.As(err, &structErr) {
+		metadata := structErr.Metadata()
+		if len(metadata) == 0 {
+			return err
+		}
+
+		keys := make([]string, 0, len(metadata))
+		for key := range metadata {
+			keys = append(keys, key)
+		}
+		sort.Strings(keys)
+
+		// We need to wrap the top-level error with the expected intercepted metadata, or
+		// otherwise we lose error context.
+		errWithMetadata := New("%w", err)
+		for _, key := range keys {
+			errWithMetadata = errWithMetadata.WithInterceptedMetadata(key, metadata[key])
+		}
+
+		return errWithMetadata
+	}
+
+	return err
+}
+
+// FieldsProducer extracts metadata from err if it contains a `structerr.Error` and exposes it as
+// logged fields. This function is supposed to be used with `log.MessageProducer()`.
+func FieldsProducer(_ context.Context, err error) logrus.Fields {
+	var structErr Error
+	if errors.As(err, &structErr) {
+		metadata := structErr.Metadata()
+		if len(metadata) == 0 {
+			return nil
+		}
+
+		return logrus.Fields{
+			"error_metadata": metadata,
+		}
+	}
+
+	return nil
+}
diff --git a/internal/structerr/fields_producer_test.go b/internal/structerr/grpc_server_test.go
index 27bde9a32..b9c48d6b9 100644
--- a/internal/structerr/fields_producer_test.go
+++ b/internal/structerr/grpc_server_test.go
@@ -3,6 +3,7 @@ package structerr
 import (
 	"context"
 	"errors"
+	"fmt"
 	"net"
 	"testing"
 
@@ -13,6 +14,7 @@ import (
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/log"
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
+	"gitlab.com/gitlab-org/gitaly/v15/proto/go/gitalypb/testproto"
 	"google.golang.org/grpc"
 	"google.golang.org/grpc/codes"
 	"google.golang.org/grpc/credentials/insecure"
@@ -20,6 +22,57 @@ import (
 	"google.golang.org/grpc/test/grpc_testing"
 )
 
+func TestInterceptedError(t *testing.T) {
+	t.Parallel()
+
+	for _, tc := range []struct {
+		desc        string
+		err         error
+		expectedErr error
+	}{
+		{
+			desc:        "normal error",
+			err:         fmt.Errorf("test"),
+			expectedErr: fmt.Errorf("test"),
+		},
+		{
+			desc:        "structured error",
+			err:         NewNotFound("not found"),
+			expectedErr: NewNotFound("not found"),
+		},
+		{
+			desc:        "wrapped structured error",
+			err:         fmt.Errorf("wrapped: %w", NewNotFound("not found")),
+			expectedErr: fmt.Errorf("wrapped: %w", NewNotFound("not found")),
+		},
+		{
+			desc: "metadata",
+			err:  NewNotFound("not found").WithMetadata("key", "value"),
+			expectedErr: NewNotFound("not found").WithDetail(
+				&testproto.ErrorMetadata{
+					Key:   []byte("key"),
+					Value: []byte("value"),
+				},
+			),
+		},
+		{
+			desc: "wrapped error with metadata",
+			err:  fmt.Errorf("wrapped: %w", NewNotFound("not found").WithMetadata("key", "value")),
+			expectedErr: NewNotFound("wrapped: not found").WithDetail(
+				&testproto.ErrorMetadata{
+					Key:   []byte("key"),
+					Value: []byte("value"),
+				},
+			),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			err := interceptedError(tc.err)
+			testhelper.RequireGrpcError(t, tc.expectedErr, err)
+		})
+	}
+}
+
 type mockService struct {
 	grpc_testing.UnimplementedTestServiceServer
 	err error
diff --git a/internal/structerr/testhelper_test.go b/internal/structerr/testhelper_test.go
deleted file mode 100644
index b9934fecc..000000000
--- a/internal/structerr/testhelper_test.go
+++ /dev/null
@@ -1,11 +0,0 @@
-package structerr
-
-import (
-	"testing"
-
-	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
-)
-
-func TestMain(m *testing.M) {
-	testhelper.Run(m)
-}
diff --git a/proto/go/gitalypb/testproto/error_metadata.pb.go b/proto/go/gitalypb/testproto/error_metadata.pb.go
new file mode 100644
index 000000000..56f0b4a17
--- /dev/null
+++ b/proto/go/gitalypb/testproto/error_metadata.pb.go
@@ -0,0 +1,159 @@
+// Code generated by protoc-gen-go. DO NOT EDIT.
+// versions:
+// 	protoc-gen-go v1.28.1
+// 	protoc        v3.21.7
+// source: testproto/error_metadata.proto
+
+package testproto
+
+import (
+	protoreflect "google.golang.org/protobuf/reflect/protoreflect"
+	protoimpl "google.golang.org/protobuf/runtime/protoimpl"
+	reflect "reflect"
+	sync "sync"
+)
+
+const (
+	// Verify that this generated code is sufficiently up-to-date.
+	_ = protoimpl.EnforceVersion(20 - protoimpl.MinVersion)
+	// Verify that runtime/protoimpl is sufficiently up-to-date.
+	_ = protoimpl.EnforceVersion(protoimpl.MaxVersion - 20)
+)
+
+// ErrorMetadata is a key-value metadata item that may be attached to errors. We only use this
+// infrastructure for testing purposes to assert that we add error metadata as expected that would
+// otherwise only get logged.
+type ErrorMetadata struct {
+	state         protoimpl.MessageState
+	sizeCache     protoimpl.SizeCache
+	unknownFields protoimpl.UnknownFields
+
+	// Key is the key of the item.
+	Key []byte `protobuf:"bytes,1,opt,name=key,proto3" json:"key,omitempty"`
+	// Value is the value of the item.
+	Value []byte `protobuf:"bytes,2,opt,name=value,proto3" json:"value,omitempty"`
+}
+
+func (x *ErrorMetadata) Reset() {
+	*x = ErrorMetadata{}
+	if protoimpl.UnsafeEnabled {
+		mi := &file_testproto_error_metadata_proto_msgTypes[0]
+		ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x))
+		ms.StoreMessageInfo(mi)
+	}
+}
+
+func (x *ErrorMetadata) String() string {
+	return protoimpl.X.MessageStringOf(x)
+}
+
+func (*ErrorMetadata) ProtoMessage() {}
+
+func (x *ErrorMetadata) ProtoReflect() protoreflect.Message {
+	mi := &file_testproto_error_metadata_proto_msgTypes[0]
+	if protoimpl.UnsafeEnabled && x != nil {
+		ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x))
+		if ms.LoadMessageInfo() == nil {
+			ms.StoreMessageInfo(mi)
+		}
+		return ms
+	}
+	return mi.MessageOf(x)
+}
+
+// Deprecated: Use ErrorMetadata.ProtoReflect.Descriptor instead.
+func (*ErrorMetadata) Descriptor() ([]byte, []int) {
+	return file_testproto_error_metadata_proto_rawDescGZIP(), []int{0}
+}
+
+func (x *ErrorMetadata) GetKey() []byte {
+	if x != nil {
+		return x.Key
+	}
+	return nil
+}
+
+func (x *ErrorMetadata) GetValue() []byte {
+	if x != nil {
+		return x.Value
+	}
+	return nil
+}
+
+var File_testproto_error_metadata_proto protoreflect.FileDescriptor
+
+var file_testproto_error_metadata_proto_rawDesc = []byte{
+	0x0a, 0x1e, 0x74, 0x65, 0x73, 0x74, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2f, 0x65, 0x72, 0x72, 0x6f,
+	0x72, 0x5f, 0x6d, 0x65, 0x74, 0x61, 0x64, 0x61, 0x74, 0x61, 0x2e, 0x70, 0x72, 0x6f, 0x74, 0x6f,
+	0x12, 0x09, 0x74, 0x65, 0x73, 0x74, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x22, 0x37, 0x0a, 0x0d, 0x45,
+	0x72, 0x72, 0x6f, 0x72, 0x4d, 0x65, 0x74, 0x61, 0x64, 0x61, 0x74, 0x61, 0x12, 0x10, 0x0a, 0x03,
+	0x6b, 0x65, 0x79, 0x18, 0x01, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x03, 0x6b, 0x65, 0x79, 0x12, 0x14,
+	0x0a, 0x05, 0x76, 0x61, 0x6c, 0x75, 0x65, 0x18, 0x02, 0x20, 0x01, 0x28, 0x0c, 0x52, 0x05, 0x76,
+	0x61, 0x6c, 0x75, 0x65, 0x42, 0x32, 0x5a, 0x30, 0x67, 0x69, 0x74, 0x6c, 0x61, 0x62, 0x2e, 0x63,
+	0x6f, 0x6d, 0x2f, 0x67, 0x69, 0x74, 0x6c, 0x61, 0x62, 0x2d, 0x6f, 0x72, 0x67, 0x2f, 0x67, 0x69,
+	0x74, 0x61, 0x6c, 0x79, 0x2f, 0x76, 0x31, 0x35, 0x2f, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x2f, 0x74,
+	0x65, 0x73, 0x74, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x62, 0x06, 0x70, 0x72, 0x6f, 0x74, 0x6f, 0x33,
+}
+
+var (
+	file_testproto_error_metadata_proto_rawDescOnce sync.Once
+	file_testproto_error_metadata_proto_rawDescData = file_testproto_error_metadata_proto_rawDesc
+)
+
+func file_testproto_error_metadata_proto_rawDescGZIP() []byte {
+	file_testproto_error_metadata_proto_rawDescOnce.Do(func() {
+		file_testproto_error_metadata_proto_rawDescData = protoimpl.X.CompressGZIP(file_testproto_error_metadata_proto_rawDescData)
+	})
+	return file_testproto_error_metadata_proto_rawDescData
+}
+
+var file_testproto_error_metadata_proto_msgTypes = make([]protoimpl.MessageInfo, 1)
+var file_testproto_error_metadata_proto_goTypes = []interface{}{
+	(*ErrorMetadata)(nil), // 0: testproto.ErrorMetadata
+}
+var file_testproto_error_metadata_proto_depIdxs = []int32{
+	0, // [0:0] is the sub-list for method output_type
+	0, // [0:0] is the sub-list for method input_type
+	0, // [0:0] is the sub-list for extension type_name
+	0, // [0:0] is the sub-list for extension extendee
+	0, // [0:0] is the sub-list for field type_name
+}
+
+func init() { file_testproto_error_metadata_proto_init() }
+func file_testproto_error_metadata_proto_init() {
+	if File_testproto_error_metadata_proto != nil {
+		return
+	}
+	if !protoimpl.UnsafeEnabled {
+		file_testproto_error_metadata_proto_msgTypes[0].Exporter = func(v interface{}, i int) interface{} {
+			switch v := v.(*ErrorMetadata); i {
+			case 0:
+				return &v.state
+			case 1:
+				return &v.sizeCache
+			case 2:
+				return &v.unknownFields
+			default:
+				return nil
+			}
+		}
+	}
+	type x struct{}
+	out := protoimpl.TypeBuilder{
+		File: protoimpl.DescBuilder{
+			GoPackagePath: reflect.TypeOf(x{}).PkgPath(),
+			RawDescriptor: file_testproto_error_metadata_proto_rawDesc,
+			NumEnums:      0,
+			NumMessages:   1,
+			NumExtensions: 0,
+			NumServices:   0,
+		},
+		GoTypes:           file_testproto_error_metadata_proto_goTypes,
+		DependencyIndexes: file_testproto_error_metadata_proto_depIdxs,
+		MessageInfos:      file_testproto_error_metadata_proto_msgTypes,
+	}.Build()
+	File_testproto_error_metadata_proto = out.File
+	file_testproto_error_metadata_proto_rawDesc = nil
+	file_testproto_error_metadata_proto_goTypes = nil
+	file_testproto_error_metadata_proto_depIdxs = nil
+}
diff --git a/proto/testproto/error_metadata.proto b/proto/testproto/error_metadata.proto
new file mode 100644
index 000000000..e97c9dc0d
--- /dev/null
+++ b/proto/testproto/error_metadata.proto
@@ -0,0 +1,15 @@
+syntax = "proto3";
+
+package testproto;
+
+option go_package = "gitlab.com/gitlab-org/gitaly/v15/proto/testproto";
+
+// ErrorMetadata is a key-value metadata item that may be attached to errors. We only use this
+// infrastructure for testing purposes to assert that we add error metadata as expected that would
+// otherwise only get logged.
+message ErrorMetadata {
+  // Key is the key of the item.
+  bytes key = 1;
+  // Value is the value of the item.
+  bytes value = 2;
+}