Merge pull request #5905 from fiam/alberto/grpc-preserve-unknown-types

feat(grpc): preserve unknown detail types when decoding gRPC errors

Author: tonistiigi

util/grpcerrors/grpcerrors.go

@@ -45,7 +45,7 @@ func ToGRPC(ctx context.Context, err error) error {
 
 	// If the original error was wrapped with more context than the GRPCStatus error,
 	// copy the original message to the GRPCStatus error
-	if err.Error() != st.Message() {
+	if errorHasMoreContext(err, st) {
 		pb := st.Proto()
 		pb.Message = err.Error()
 		st = status.FromProto(pb)
@@ -72,6 +72,21 @@ func ToGRPC(ctx context.Context, err error) error {
 	return st.Err()
 }
 
+// errorHasMoreContext checks if the original error provides more context by having
+// a different message or additional details than the Status.
+func errorHasMoreContext(err error, st *status.Status) bool {
+	if errMessage := err.Error(); len(errMessage) > len(st.Message()) {
+		// check if the longer message in errMessage is only due to
+		// prepending with the status code
+		var grpcStatusError *grpcStatusError
+		if errors.As(err, &grpcStatusError) {
+			return st.Code() != grpcStatusError.st.Code() || st.Message() != grpcStatusError.st.Message()
+		}
+		return true
+	}
+	return false
+}
+
 func withDetails(ctx context.Context, s *status.Status, details ...proto.Message) (*status.Status, error) {
 	if s.Code() == codes.OK {
 		return nil, errors.New("no error details for status with code OK")
@@ -172,6 +187,8 @@ func FromGRPC(err error) error {
 	for _, d := range pb.Details {
 		m, err := typeurl.UnmarshalAny(d)
 		if err != nil {
+			bklog.L.Debugf("failed to unmarshal error detail with type %q: %v", d.GetTypeUrl(), err)
+			n.Details = append(n.Details, d)
 			continue
 		}
 
@@ -181,6 +198,7 @@ func FromGRPC(err error) error {
 		case TypedErrorProto:
 			details = append(details, v)
 		default:
+			bklog.L.Debugf("unknown detail with type %T", v)
 			n.Details = append(n.Details, d)
 		}
 	}

util/grpcerrors/grpcerrors_test.go

@@ -0,0 +1,88 @@
+package grpcerrors_test
+
+import (
+	"context"
+	"fmt"
+	"strings"
+	"testing"
+
+	"github.com/moby/buildkit/util/grpcerrors"
+	"github.com/pkg/errors"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+	spb "google.golang.org/genproto/googleapis/rpc/status"
+	"google.golang.org/grpc/codes"
+	"google.golang.org/grpc/status"
+	"google.golang.org/protobuf/types/known/anypb"
+)
+
+func TestFromGRPCPreserveUnknownTypes(t *testing.T) {
+	t.Parallel()
+	const (
+		unknownType  = "type.googleapis.com/unknown.Type"
+		unknownValue = "unknown value"
+
+		errMessage = "something failed"
+		errCode    = codes.Internal
+	)
+	raw := &anypb.Any{
+		TypeUrl: unknownType,
+		Value:   []byte(unknownValue),
+	}
+
+	pb := &spb.Status{
+		Code:    int32(errCode),
+		Message: errMessage,
+		Details: []*anypb.Any{raw},
+	}
+
+	assertErrorProperties := func(t *testing.T, err error) {
+		require.Error(t, err)
+		assert.Equal(t, fmt.Sprintf("%s: %s", codes.Code(errCode), errMessage), err.Error())
+
+		st, ok := status.FromError(err)
+		require.True(t, ok)
+
+		details := st.Proto().Details
+		require.Len(t, details, 1)
+		assert.Equal(t, unknownType, details[0].TypeUrl)
+		assert.Equal(t, []byte(unknownValue), details[0].Value)
+	}
+
+	t.Run("encode", func(t *testing.T) {
+		t.Parallel()
+		err := grpcerrors.FromGRPC(status.FromProto(pb).Err())
+		assertErrorProperties(t, err)
+	})
+
+	t.Run("roundtrip", func(t *testing.T) {
+		t.Parallel()
+
+		decodedErr := grpcerrors.FromGRPC(status.FromProto(pb).Err())
+
+		reEncodedErr := grpcerrors.ToGRPC(context.TODO(), decodedErr)
+
+		reDecodedErr := grpcerrors.FromGRPC(reEncodedErr)
+		assertErrorProperties(t, reDecodedErr)
+	})
+}
+
+func TestToGRPCMessage(t *testing.T) {
+	t.Parallel()
+	t.Run("avoid prepending grpc status code", func(t *testing.T) {
+		t.Parallel()
+		err := errors.New("something")
+		decoded := grpcerrors.FromGRPC(grpcerrors.ToGRPC(context.TODO(), err))
+		assert.Equal(t, err.Error(), decoded.Error())
+	})
+	t.Run("keep extra context", func(t *testing.T) {
+		t.Parallel()
+		err := errors.New("something")
+		wrapped := errors.Wrap(grpcerrors.ToGRPC(context.TODO(), err), "extra context")
+		// Check that wrapped.Error() starts with "extra context"
+		assert.True(t, strings.HasPrefix(wrapped.Error(), "extra context"), "expected wrapped error to start with 'extra context'")
+		encoded := grpcerrors.ToGRPC(context.TODO(), wrapped)
+		decoded := grpcerrors.FromGRPC(encoded)
+		assert.Equal(t, wrapped.Error(), decoded.Error())
+	})
+}