Changing store gateway error code from ResourceExhausted to Permissio…

…nDenied
cortexproject · Jul 5, 2023 · 9bc4569 · 9bc4569
1 parent d328ba2
commit 9bc4569
Show file tree

Hide file tree

Showing 7 changed files with 242 additions and 66 deletions.
diff --git a/pkg/querier/blocks_store_queryable.go b/pkg/querier/blocks_store_queryable.go
@@ -648,6 +648,10 @@ func (q *blocksStoreQuerier) fetchSeriesFromStores(
 						if s.Code() == codes.ResourceExhausted {
 							return validation.LimitError(s.Message())
 						}
+
+						if s.Code() == codes.PermissionDenied {
+							return validation.AccessDeniedError(s.Message())
+						}
 					}
 					return errors.Wrapf(err, "failed to receive series from %s", c.RemoteAddress())
 				}
@@ -816,6 +820,10 @@ func (q *blocksStoreQuerier) fetchLabelNamesFromStore(
 						return validation.LimitError(s.Message())
 					}
 				}
+
+				if s.Code() == codes.PermissionDenied {
+					return validation.AccessDeniedError(s.Message())
+				}
 				return errors.Wrapf(err, "failed to fetch label names from %s", c.RemoteAddress())
 			}
 
@@ -907,6 +915,10 @@ func (q *blocksStoreQuerier) fetchLabelValuesFromStore(
 					if s.Code() == codes.ResourceExhausted {
 						return validation.LimitError(s.Message())
 					}
+
+					if s.Code() == codes.PermissionDenied {
+						return validation.AccessDeniedError(s.Message())
+					}
 				}
 				return errors.Wrapf(err, "failed to fetch label values from %s", c.RemoteAddress())
 			}

diff --git a/pkg/querier/blocks_store_queryable_test.go b/pkg/querier/blocks_store_queryable_test.go
@@ -638,6 +638,44 @@ func TestBlocksStoreQuerier_Select(t *testing.T) {
 				},
 			},
 		},
+		"all store-gateways return PermissionDenied": {
+			finderResult: bucketindex.Blocks{
+				{ID: block1},
+			},
+			expectedErr: validation.AccessDeniedError("PermissionDenied"),
+			storeSetResponses: []interface{}{
+				map[BlocksStoreClient][]ulid.ULID{
+					&storeGatewayClientMock{
+						remoteAddr: "1.1.1.1",
+						mockedSeriesResponses: []*storepb.SeriesResponse{
+							mockSeriesResponse(labels.Labels{metricNameLabel, series1Label}, minT, 2),
+							mockHintsResponse(block1),
+						},
+						mockedSeriesStreamErr: status.Error(codes.PermissionDenied, "PermissionDenied"),
+					}: {block1},
+				},
+				map[BlocksStoreClient][]ulid.ULID{
+					&storeGatewayClientMock{
+						remoteAddr: "2.2.2.2",
+						mockedSeriesResponses: []*storepb.SeriesResponse{
+							mockSeriesResponse(labels.Labels{metricNameLabel, series1Label}, minT, 2),
+							mockHintsResponse(block1),
+						},
+						mockedSeriesStreamErr: status.Error(codes.PermissionDenied, "PermissionDenied"),
+					}: {block1},
+				},
+			},
+			limits:       &blocksStoreLimitsMock{},
+			queryLimiter: noOpQueryLimiter,
+			expectedSeries: []seriesResult{
+				{
+					lbls: labels.New(metricNameLabel, series1Label),
+					values: []valueResult{
+						{t: minT, v: 2},
+					},
+				},
+			},
+		},
 		"multiple store-gateways has the block, but one of them fails to return on stream": {
 			finderResult: bucketindex.Blocks{
 				{ID: block1},

diff --git a/pkg/storage/bucket/sse_bucket_client.go b/pkg/storage/bucket/sse_bucket_client.go
@@ -4,10 +4,13 @@ import (
 	"context"
 	"io"
 
+	cortex_errors "github.com/cortexproject/cortex/pkg/util/errors"
+	"github.com/gogo/status"
 	"github.com/minio/minio-go/v7/pkg/encrypt"
 	"github.com/pkg/errors"
 	"github.com/thanos-io/objstore"
 	"github.com/thanos-io/objstore/providers/s3"
+	"google.golang.org/grpc/codes"
 
 	cortex_s3 "github.com/cortexproject/cortex/pkg/storage/bucket/s3"
 )
@@ -101,12 +104,23 @@ func (b *SSEBucketClient) Iter(ctx context.Context, dir string, f func(string) e
 
 // Get implements objstore.Bucket.
 func (b *SSEBucketClient) Get(ctx context.Context, name string) (io.ReadCloser, error) {
-	return b.bucket.Get(ctx, name)
+	r, err := b.bucket.Get(ctx, name)
+
+	if err != nil && b.bucket.IsCustomerManagedKeyError(err) {
+		return nil, cortex_errors.WithCause(err, status.Error(codes.PermissionDenied, err.Error()))
+	}
+
+	return r, err
 }
 
 // GetRange implements objstore.Bucket.
 func (b *SSEBucketClient) GetRange(ctx context.Context, name string, off, length int64) (io.ReadCloser, error) {
-	return b.bucket.GetRange(ctx, name, off, length)
+	r, err := b.bucket.GetRange(ctx, name, off, length)
+	if err != nil && b.bucket.IsCustomerManagedKeyError(err) {
+		return nil, cortex_errors.WithCause(err, status.Error(codes.PermissionDenied, err.Error()))
+	}
+
+	return r, err
 }
 
 // Exists implements objstore.Bucket.

diff --git a/pkg/storage/tsdb/testutil/objstore.go b/pkg/storage/tsdb/testutil/objstore.go
@@ -8,12 +8,11 @@ import (
 	"sync/atomic"
 	"testing"
 
+	"github.com/cortexproject/cortex/pkg/util"
 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/require"
 	"github.com/thanos-io/objstore"
 
-	"github.com/cortexproject/cortex/pkg/util"
-
 	"github.com/cortexproject/cortex/pkg/storage/bucket/filesystem"
 )
 
@@ -51,6 +50,20 @@ func (m *MockBucketFailure) Delete(ctx context.Context, name string) error {
 	return m.Bucket.Delete(ctx, name)
 }
 
+func (m *MockBucketFailure) GetRange(ctx context.Context, name string, off, length int64) (io.ReadCloser, error) {
+	m.GetCalls.Add(1)
+	for prefix, err := range m.GetFailures {
+		if strings.HasPrefix(name, prefix) {
+			return nil, err
+		}
+	}
+	if e, ok := m.GetFailures[name]; ok {
+		return nil, e
+	}
+
+	return m.Bucket.GetRange(ctx, name, off, length)
+}
+
 func (m *MockBucketFailure) Get(ctx context.Context, name string) (io.ReadCloser, error) {
 	m.GetCalls.Add(1)
 	for prefix, err := range m.GetFailures {

diff --git a/pkg/storegateway/bucket_stores.go b/pkg/storegateway/bucket_stores.go
@@ -306,18 +306,14 @@ func (u *BucketStores) Series(req *storepb.SeriesRequest, srv storepb.Store_Seri
 	err := u.getStoreError(userID)
 
 	if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
-		return httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
+		return httpgrpc.Errorf(int(codes.PermissionDenied), "store error: %s", err)
 	}
 
 	err = store.Series(req, spanSeriesServer{
 		Store_SeriesServer: srv,
 		ctx:                spanCtx,
 	})
 
-	if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
-		return httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
-	}
-
 	return err
 }
 
@@ -339,15 +335,11 @@ func (u *BucketStores) LabelNames(ctx context.Context, req *storepb.LabelNamesRe
 	err := u.getStoreError(userID)
 
 	if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
-		return nil, httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
+		return nil, httpgrpc.Errorf(int(codes.PermissionDenied), "store error: %s", err)
 	}
 
 	resp, err := store.LabelNames(ctx, req)
 
-	if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
-		return resp, httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
-	}
-
 	return resp, err
 }
 
@@ -369,15 +361,11 @@ func (u *BucketStores) LabelValues(ctx context.Context, req *storepb.LabelValues
 	err := u.getStoreError(userID)
 
 	if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
-		return nil, httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
+		return nil, httpgrpc.Errorf(int(codes.PermissionDenied), "store error: %s", err)
 	}
 
 	resp, err := store.LabelValues(ctx, req)
 
-	if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
-		return resp, httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
-	}
-
 	return resp, err
 }