Skip to content

Commit 7b51a48

Browse files
alanprotalanprot
authored
Changing return code from storegateway for CMK errors (cortexproject#5442)
* Changing store gateway error code from ResourceExhausted to PermissionDenied Signed-off-by: Alan Protasio <[email protected]> * using cause on the mock error Signed-off-by: Alan Protasio <[email protected]> * chnagelog Signed-off-by: Alan Protasio <[email protected]> * lint Signed-off-by: Alan Protasio <[email protected]> * Addressing comments Signed-off-by: Alan Protasio <[email protected]> --------- Signed-off-by: Alan Protasio <[email protected]>
1 parent b87ac49 commit 7b51a48

File tree

10 files changed

+277
-68
lines changed

10 files changed

+277
-68
lines changed

‎CHANGELOG.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
* [ENHANCEMENT] Add jitter to lifecycler heartbeat. #5404
3737
* [ENHANCEMENT] Store Gateway: Add config `estimated_max_series_size_bytes` and `estimated_max_chunk_size_bytes` to address data overfetch. #5401
3838
* [ENHANCEMENT] Distributor/Ingester: Add experimental `-distributor.sign_write_requests` flag to sign the write requests. #5430
39+
* [ENHANCEMENT] Store Gateway/Querier/Compactor: Handling CMK Access Denied errors. #5420 #5442
3940
* [BUGFIX] Ruler: Validate if rule group can be safely converted back to rule group yaml from protobuf message #5265
4041
* [BUGFIX] Querier: Convert gRPC `ResourceExhausted` status code from store gateway to 422 limit error. #5286
4142
* [BUGFIX] Alertmanager: Route web-ui requests to the alertmanager distributor when sharding is enabled. #5293

‎pkg/querier/blocks_store_queryable.go

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -648,6 +648,10 @@ func (q *blocksStoreQuerier) fetchSeriesFromStores(
648648
if s.Code() == codes.ResourceExhausted {
649649
return validation.LimitError(s.Message())
650650
}
651+
652+
if s.Code() == codes.PermissionDenied {
653+
return validation.AccessDeniedError(s.Message())
654+
}
651655
}
652656
return errors.Wrapf(err, "failed to receive series from %s", c.RemoteAddress())
653657
}
@@ -816,6 +820,10 @@ func (q *blocksStoreQuerier) fetchLabelNamesFromStore(
816820
return validation.LimitError(s.Message())
817821
}
818822
}
823+
824+
if s.Code() == codes.PermissionDenied {
825+
return validation.AccessDeniedError(s.Message())
826+
}
819827
return errors.Wrapf(err, "failed to fetch label names from %s", c.RemoteAddress())
820828
}
821829

@@ -907,6 +915,10 @@ func (q *blocksStoreQuerier) fetchLabelValuesFromStore(
907915
if s.Code() == codes.ResourceExhausted {
908916
return validation.LimitError(s.Message())
909917
}
918+
919+
if s.Code() == codes.PermissionDenied {
920+
return validation.AccessDeniedError(s.Message())
921+
}
910922
}
911923
return errors.Wrapf(err, "failed to fetch label values from %s", c.RemoteAddress())
912924
}

‎pkg/querier/blocks_store_queryable_test.go

Lines changed: 38 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -638,6 +638,44 @@ func TestBlocksStoreQuerier_Select(t *testing.T) {
638638
},
639639
},
640640
},
641+
"all store-gateways return PermissionDenied": {
642+
finderResult: bucketindex.Blocks{
643+
{ID: block1},
644+
},
645+
expectedErr: validation.AccessDeniedError("PermissionDenied"),
646+
storeSetResponses: []interface{}{
647+
map[BlocksStoreClient][]ulid.ULID{
648+
&storeGatewayClientMock{
649+
remoteAddr: "1.1.1.1",
650+
mockedSeriesResponses: []*storepb.SeriesResponse{
651+
mockSeriesResponse(labels.Labels{metricNameLabel, series1Label}, minT, 2),
652+
mockHintsResponse(block1),
653+
},
654+
mockedSeriesStreamErr: status.Error(codes.PermissionDenied, "PermissionDenied"),
655+
}: {block1},
656+
},
657+
map[BlocksStoreClient][]ulid.ULID{
658+
&storeGatewayClientMock{
659+
remoteAddr: "2.2.2.2",
660+
mockedSeriesResponses: []*storepb.SeriesResponse{
661+
mockSeriesResponse(labels.Labels{metricNameLabel, series1Label}, minT, 2),
662+
mockHintsResponse(block1),
663+
},
664+
mockedSeriesStreamErr: status.Error(codes.PermissionDenied, "PermissionDenied"),
665+
}: {block1},
666+
},
667+
},
668+
limits: &blocksStoreLimitsMock{},
669+
queryLimiter: noOpQueryLimiter,
670+
expectedSeries: []seriesResult{
671+
{
672+
lbls: labels.New(metricNameLabel, series1Label),
673+
values: []valueResult{
674+
{t: minT, v: 2},
675+
},
676+
},
677+
},
678+
},
641679
"multiple store-gateways has the block, but one of them fails to return on stream": {
642680
finderResult: bucketindex.Blocks{
643681
{ID: block1},

‎pkg/storage/bucket/sse_bucket_client.go

Lines changed: 23 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -4,10 +4,14 @@ import (
44
"context"
55
"io"
66

7+
"github.com/gogo/status"
78
"github.com/minio/minio-go/v7/pkg/encrypt"
89
"github.com/pkg/errors"
910
"github.com/thanos-io/objstore"
1011
"github.com/thanos-io/objstore/providers/s3"
12+
"google.golang.org/grpc/codes"
13+
14+
cortex_errors "github.com/cortexproject/cortex/pkg/util/errors"
1115

1216
cortex_s3 "github.com/cortexproject/cortex/pkg/storage/bucket/s3"
1317
)
@@ -101,12 +105,24 @@ func (b *SSEBucketClient) Iter(ctx context.Context, dir string, f func(string) e
101105

102106
// Get implements objstore.Bucket.
103107
func (b *SSEBucketClient) Get(ctx context.Context, name string) (io.ReadCloser, error) {
104-
return b.bucket.Get(ctx, name)
108+
r, err := b.bucket.Get(ctx, name)
109+
110+
if err != nil && b.IsCustomerManagedKeyError(err) {
111+
// Store gateway will return the status if the returned error is an `status.Error`
112+
return nil, cortex_errors.WithCause(err, status.Error(codes.PermissionDenied, err.Error()))
113+
}
114+
115+
return r, err
105116
}
106117

107118
// GetRange implements objstore.Bucket.
108119
func (b *SSEBucketClient) GetRange(ctx context.Context, name string, off, length int64) (io.ReadCloser, error) {
109-
return b.bucket.GetRange(ctx, name, off, length)
120+
r, err := b.bucket.GetRange(ctx, name, off, length)
121+
if err != nil && b.IsCustomerManagedKeyError(err) {
122+
return nil, cortex_errors.WithCause(err, status.Error(codes.PermissionDenied, err.Error()))
123+
}
124+
125+
return r, err
110126
}
111127

112128
// Exists implements objstore.Bucket.
@@ -119,7 +135,12 @@ func (b *SSEBucketClient) IsObjNotFoundErr(err error) bool {
119135
return b.bucket.IsObjNotFoundErr(err)
120136
}
121137

138+
// IsCustomerManagedKeyError implements objstore.Bucket.
122139
func (b *SSEBucketClient) IsCustomerManagedKeyError(err error) bool {
140+
// unwrap error
141+
if se, ok := err.(interface{ Err() error }); ok {
142+
return b.bucket.IsCustomerManagedKeyError(se.Err()) || b.bucket.IsCustomerManagedKeyError(err)
143+
}
123144
return b.bucket.IsCustomerManagedKeyError(err)
124145
}
125146

‎pkg/storage/bucket/sse_bucket_client_test.go

Lines changed: 13 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -106,6 +106,19 @@ func TestSSEBucketClient_Upload_ShouldInjectCustomSSEConfig(t *testing.T) {
106106
}
107107
}
108108

109+
func Test_shouldWrapSSeErrors(t *testing.T) {
110+
cfgProvider := &mockTenantConfigProvider{}
111+
112+
bkt := &ClientMock{}
113+
114+
bkt.MockGet("Test", "someContent", errKeyPermissionDenied)
115+
116+
sseBkt := NewSSEBucketClient("user-1", bkt, cfgProvider)
117+
118+
_, err := sseBkt.Get(context.Background(), "Test")
119+
require.True(t, sseBkt.IsCustomerManagedKeyError(err))
120+
}
121+
109122
type mockTenantConfigProvider struct {
110123
s3SseType string
111124
s3KmsKeyID string

‎pkg/storage/tsdb/testutil/objstore.go

Lines changed: 15 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,20 @@ func (m *MockBucketFailure) Delete(ctx context.Context, name string) error {
5151
return m.Bucket.Delete(ctx, name)
5252
}
5353

54+
func (m *MockBucketFailure) GetRange(ctx context.Context, name string, off, length int64) (io.ReadCloser, error) {
55+
m.GetCalls.Add(1)
56+
for prefix, err := range m.GetFailures {
57+
if strings.HasPrefix(name, prefix) {
58+
return nil, err
59+
}
60+
}
61+
if e, ok := m.GetFailures[name]; ok {
62+
return nil, e
63+
}
64+
65+
return m.Bucket.GetRange(ctx, name, off, length)
66+
}
67+
5468
func (m *MockBucketFailure) Get(ctx context.Context, name string) (io.ReadCloser, error) {
5569
m.GetCalls.Add(1)
5670
for prefix, err := range m.GetFailures {
@@ -96,5 +110,5 @@ func (m *MockBucketFailure) ReaderWithExpectedErrs(expectedFunc objstore.IsOpFai
96110
}
97111

98112
func (m *MockBucketFailure) IsCustomerManagedKeyError(err error) bool {
99-
return errors.Is(err, ErrKeyAccessDeniedError)
113+
return errors.Is(errors.Cause(err), ErrKeyAccessDeniedError)
100114
}

‎pkg/storegateway/bucket_stores.go

Lines changed: 9 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -299,25 +299,22 @@ func (u *BucketStores) Series(req *storepb.SeriesRequest, srv storepb.Store_Seri
299299
}
300300

301301
store := u.getStore(userID)
302+
userBkt := bucket.NewUserBucketClient(userID, u.bucket, u.limits)
302303
if store == nil {
303304
return nil
304305
}
305306

306307
err := u.getStoreError(userID)
307308

308-
if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
309-
return httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
309+
if err != nil && cortex_errors.ErrorIs(err, userBkt.IsCustomerManagedKeyError) {
310+
return httpgrpc.Errorf(int(codes.PermissionDenied), "store error: %s", err)
310311
}
311312

312313
err = store.Series(req, spanSeriesServer{
313314
Store_SeriesServer: srv,
314315
ctx: spanCtx,
315316
})
316317

317-
if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
318-
return httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
319-
}
320-
321318
return err
322319
}
323320

@@ -332,22 +329,19 @@ func (u *BucketStores) LabelNames(ctx context.Context, req *storepb.LabelNamesRe
332329
}
333330

334331
store := u.getStore(userID)
332+
userBkt := bucket.NewUserBucketClient(userID, u.bucket, u.limits)
335333
if store == nil {
336334
return &storepb.LabelNamesResponse{}, nil
337335
}
338336

339337
err := u.getStoreError(userID)
340338

341-
if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
342-
return nil, httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
339+
if err != nil && cortex_errors.ErrorIs(err, userBkt.IsCustomerManagedKeyError) {
340+
return nil, httpgrpc.Errorf(int(codes.PermissionDenied), "store error: %s", err)
343341
}
344342

345343
resp, err := store.LabelNames(ctx, req)
346344

347-
if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
348-
return resp, httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
349-
}
350-
351345
return resp, err
352346
}
353347

@@ -362,22 +356,19 @@ func (u *BucketStores) LabelValues(ctx context.Context, req *storepb.LabelValues
362356
}
363357

364358
store := u.getStore(userID)
359+
userBkt := bucket.NewUserBucketClient(userID, u.bucket, u.limits)
365360
if store == nil {
366361
return &storepb.LabelValuesResponse{}, nil
367362
}
368363

369364
err := u.getStoreError(userID)
370365

371-
if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
372-
return nil, httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
366+
if err != nil && cortex_errors.ErrorIs(err, userBkt.IsCustomerManagedKeyError) {
367+
return nil, httpgrpc.Errorf(int(codes.PermissionDenied), "store error: %s", err)
373368
}
374369

375370
resp, err := store.LabelValues(ctx, req)
376371

377-
if err != nil && cortex_errors.ErrorIs(err, u.bucket.IsCustomerManagedKeyError) {
378-
return resp, httpgrpc.Errorf(int(codes.ResourceExhausted), "store error: %s", err)
379-
}
380-
381372
return resp, err
382373
}
383374

0 commit comments

Comments
 (0)