From fd377cfe665b6f6182c5dc12b8e932e0c96fc18e Mon Sep 17 00:00:00 2001 From: maicon Date: Wed, 19 Feb 2025 15:29:57 -0300 Subject: [PATCH] Unistore: map grpc status to http status (#100942) * Unistore: map grpc status to http status Signed-off-by: Maicon Costa --------- Signed-off-by: Maicon Costa Co-authored-by: Georges Chaudy --- pkg/storage/unified/apistore/store.go | 17 ++++---- pkg/storage/unified/apistore/store_test.go | 49 ++++++++++++++++++++++ pkg/storage/unified/resource/errors.go | 13 +++++- pkg/storage/unified/resource/go.mod | 2 +- 4 files changed, 70 insertions(+), 11 deletions(-) diff --git a/pkg/storage/unified/apistore/store.go b/pkg/storage/unified/apistore/store.go index 472952b2c2f..8d3da4b7c33 100644 --- a/pkg/storage/unified/apistore/store.go +++ b/pkg/storage/unified/apistore/store.go @@ -171,7 +171,7 @@ func (s *Storage) Create(ctx context.Context, key string, obj runtime.Object, ou } rsp, err := s.store.Create(ctx, req) if err != nil { - return err + return resource.GetError(resource.AsErrorResult(err)) } if rsp.Error != nil { if rsp.Error.Code == http.StatusConflict { @@ -249,7 +249,7 @@ func (s *Storage) Delete( } rsp, err := s.store.Delete(ctx, cmd) if err != nil { - return err + return resource.GetError(resource.AsErrorResult(err)) } if rsp.Error != nil { return resource.GetError(rsp.Error) @@ -289,7 +289,8 @@ func (s *Storage) Watch(ctx context.Context, key string, opts storage.ListOption if errors.Is(err, context.Canceled) || errors.Is(err, context.DeadlineExceeded) || errors.Is(err, io.EOF) { return watch.NewEmptyWatch(), nil } - return nil, err + + return nil, resource.GetError(resource.AsErrorResult(err)) } reporter := apierrors.NewClientErrorReporter(500, "WATCH", "") @@ -323,7 +324,7 @@ func (s *Storage) Get(ctx context.Context, key string, opts storage.GetOptions, rsp, err := s.store.Read(ctx, req) if err != nil { - return err + return resource.GetError(resource.AsErrorResult(err)) } if rsp.Error != nil { if rsp.Error.Code == http.StatusNotFound { @@ -361,7 +362,7 @@ func (s *Storage) GetList(ctx context.Context, key string, opts storage.ListOpti rsp, err := s.store.List(ctx, req) if err != nil { - return err + return resource.GetError(resource.AsErrorResult(err)) } if rsp.Error != nil { return resource.GetError(rsp.Error) @@ -469,7 +470,7 @@ func (s *Storage) GuaranteedUpdate( // Read the latest value rsp, err := s.store.Read(ctx, &resource.ReadRequest{Key: req.Key}) if err != nil { - return err + return resource.GetError(resource.AsErrorResult(err)) } if rsp.Error != nil { @@ -554,7 +555,7 @@ func (s *Storage) GuaranteedUpdate( Value: value, }) if err != nil { - return err + return resource.GetError(resource.AsErrorResult(err)) } if rsp2.Error != nil { return resource.GetError(rsp2.Error) @@ -567,7 +568,7 @@ func (s *Storage) GuaranteedUpdate( } rsp2, err := s.store.Update(ctx, req) if err != nil { - return err + return resource.GetError(resource.AsErrorResult(err)) } if rsp2.Error != nil { return resource.GetError(rsp2.Error) diff --git a/pkg/storage/unified/apistore/store_test.go b/pkg/storage/unified/apistore/store_test.go index ec1013de44a..12d9629d311 100644 --- a/pkg/storage/unified/apistore/store_test.go +++ b/pkg/storage/unified/apistore/store_test.go @@ -7,10 +7,17 @@ package apistore_test import ( "context" + "errors" "fmt" + "net/http" "testing" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/grpc" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" + apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/fields" "k8s.io/apimachinery/pkg/labels" @@ -19,10 +26,13 @@ import ( "k8s.io/apiserver/pkg/apis/example" examplev1 "k8s.io/apiserver/pkg/apis/example/v1" "k8s.io/apiserver/pkg/storage" + "k8s.io/apiserver/pkg/storage/storagebackend" claims "github.com/grafana/authlib/types" "github.com/grafana/grafana/pkg/apimachinery/identity" storagetesting "github.com/grafana/grafana/pkg/apiserver/storage/testing" + "github.com/grafana/grafana/pkg/storage/unified/apistore" + "github.com/grafana/grafana/pkg/storage/unified/resource" ) func init() { @@ -142,6 +152,45 @@ func TestDeleteWithSuggestionAndConflict(t *testing.T) { storagetesting.RunTestDeleteWithSuggestionAndConflict(ctx, t, store) } +type resourceClientMock struct { + resource.ResourceStoreClient + resource.ResourceIndexClient + resource.RepositoryIndexClient + resource.BatchStoreClient + resource.BlobStoreClient + resource.DiagnosticsClient +} + +// always return GRPC Unauthenticated code +func (r resourceClientMock) List(ctx context.Context, in *resource.ListRequest, opts ...grpc.CallOption) (*resource.ListResponse, error) { + return &resource.ListResponse{}, status.Error(codes.Unauthenticated, "missing token") +} + +func TestGRPCtoHTTPStatusMapping(t *testing.T) { + t.Run("ensure that GRPC Unauthenticated code gets translated to HTTP StatusUnauthorized", func(t *testing.T) { + s, _, err := apistore.NewStorage( + &storagebackend.ConfigForResource{}, + resourceClientMock{}, + nil, + nil, + nil, + nil, + nil, + nil, + nil, + apistore.StorageOptions{}) + require.NoError(t, err) + + err = s.GetList(context.Background(), "/group/resource.grafana.app/resource/resources/namespace/default", storage.ListOptions{}, nil) + require.Error(t, err) + + var statusError *apierrors.StatusError + ok := errors.As(err, &statusError) + require.Equal(t, true, ok) + require.Equal(t, int(statusError.Status().Code), http.StatusUnauthorized) + }) +} + // TODO: this test relies on update //func TestDeleteWithSuggestionOfDeletedObject(t *testing.T) { // ctx, store, destroyFunc, err := testSetup(t) diff --git a/pkg/storage/unified/resource/errors.go b/pkg/storage/unified/resource/errors.go index 7f1d3a3ca56..48faa27a303 100644 --- a/pkg/storage/unified/resource/errors.go +++ b/pkg/storage/unified/resource/errors.go @@ -7,6 +7,9 @@ import ( apierrors "k8s.io/apimachinery/pkg/api/errors" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/apimachinery/pkg/types" + + "github.com/grpc-ecosystem/grpc-gateway/v2/runtime" + grpcstatus "google.golang.org/grpc/status" ) // Package-level errors. @@ -67,10 +70,16 @@ func AsErrorResult(err error) *ErrorResult { return res } - // TODO... better conversion?? + code := 500 + + st, ok := grpcstatus.FromError(err) + if ok { + code = runtime.HTTPStatusFromCode(st.Code()) + } + return &ErrorResult{ Message: err.Error(), - Code: 500, + Code: int32(code), } } diff --git a/pkg/storage/unified/resource/go.mod b/pkg/storage/unified/resource/go.mod index 82aff1d7974..b245031350a 100644 --- a/pkg/storage/unified/resource/go.mod +++ b/pkg/storage/unified/resource/go.mod @@ -19,6 +19,7 @@ require ( github.com/grafana/grafana/pkg/apimachinery v0.0.0-20250121113133-e747350fee2d github.com/grafana/grafana/pkg/apiserver v0.0.0-20250121113133-e747350fee2d // indirect github.com/grpc-ecosystem/go-grpc-middleware/v2 v2.2.0 + github.com/grpc-ecosystem/grpc-gateway/v2 v2.25.1 github.com/hashicorp/golang-lru/v2 v2.0.7 github.com/prometheus/client_golang v1.20.5 github.com/stretchr/testify v1.10.0 @@ -125,7 +126,6 @@ require ( github.com/grafana/pyroscope-go/godeltaprof v0.1.8 // indirect github.com/grafana/sqlds/v4 v4.1.3 // indirect github.com/grpc-ecosystem/go-grpc-middleware/providers/prometheus v1.0.1 // indirect - github.com/grpc-ecosystem/grpc-gateway/v2 v2.25.1 // indirect github.com/hashicorp/errwrap v1.1.0 // indirect github.com/hashicorp/go-hclog v1.6.3 // indirect github.com/hashicorp/go-immutable-radix v1.3.1 // indirect