From bc17ab5e47a99cf79520e2a7df7fedbcfed4118c Mon Sep 17 00:00:00 2001 From: Nikita Zinkevich Date: Fri, 27 Sep 2024 12:26:08 +0300 Subject: [PATCH] [#488] middleware/policy: Add frostfs-to-s3 error transformation Signed-off-by: Nikita Zinkevich --- api/errors/errors.go | 27 ++++++++++++-- api/errors/errors_test.go | 58 ++++++++++++++++++++++++++++++ api/handler/util.go | 23 +----------- api/handler/util_test.go | 64 ---------------------------------- api/layer/layer.go | 4 +-- api/layer/tree/tree_service.go | 4 +-- api/middleware/auth.go | 1 - api/middleware/policy.go | 3 +- api/router_mock_test.go | 30 +++++++++------- api/router_test.go | 40 +++++++++++++++++++-- 10 files changed, 143 insertions(+), 111 deletions(-) diff --git a/api/errors/errors.go b/api/errors/errors.go index 6f839b8..775add3 100644 --- a/api/errors/errors.go +++ b/api/errors/errors.go @@ -1,10 +1,13 @@ package errors import ( + "errors" "fmt" "net/http" - frosterrors "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/frostfs/errors" + "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/frostfs" + "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/tree" + frosterr "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/frostfs/errors" ) type ( @@ -1765,7 +1768,7 @@ var errorCodes = errorCodeMap{ // IsS3Error checks if the provided error is a specific s3 error. func IsS3Error(err error, code ErrorCode) bool { - err = frosterrors.UnwrapErr(err) + err = frosterr.UnwrapErr(err) e, ok := err.(Error) return ok && e.ErrCode == code } @@ -1802,6 +1805,26 @@ func GetAPIErrorWithError(code ErrorCode, err error) Error { return errorCodes.toAPIErrWithErr(code, err) } +// TransformToS3Error converts FrostFS error to the corresponding S3 error type. +func TransformToS3Error(err error) error { + err = frosterr.UnwrapErr(err) // this wouldn't work with errors.Join + var s3err Error + if errors.As(err, &s3err) { + return err + } + + if errors.Is(err, frostfs.ErrAccessDenied) || + errors.Is(err, tree.ErrNodeAccessDenied) { + return GetAPIError(ErrAccessDenied) + } + + if errors.Is(err, frostfs.ErrGatewayTimeout) { + return GetAPIError(ErrGatewayTimeout) + } + + return GetAPIError(ErrInternalError) +} + // ObjectError -- error that is linked to a specific object. type ObjectError struct { Err error diff --git a/api/errors/errors_test.go b/api/errors/errors_test.go index 71fa4c8..15f3f0f 100644 --- a/api/errors/errors_test.go +++ b/api/errors/errors_test.go @@ -2,7 +2,12 @@ package errors import ( "errors" + "fmt" "testing" + + "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/frostfs" + "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/tree" + "github.com/stretchr/testify/require" ) func BenchmarkErrCode(b *testing.B) { @@ -24,3 +29,56 @@ func BenchmarkErrorsIs(b *testing.B) { } } } + +func TestTransformS3Errors(t *testing.T) { + for _, tc := range []struct { + name string + err error + expected ErrorCode + }{ + { + name: "simple std error to internal error", + err: errors.New("some error"), + expected: ErrInternalError, + }, + { + name: "layer access denied error to s3 access denied error", + err: frostfs.ErrAccessDenied, + expected: ErrAccessDenied, + }, + { + name: "wrapped layer access denied error to s3 access denied error", + err: fmt.Errorf("wrap: %w", frostfs.ErrAccessDenied), + expected: ErrAccessDenied, + }, + { + name: "layer node access denied error to s3 access denied error", + err: tree.ErrNodeAccessDenied, + expected: ErrAccessDenied, + }, + { + name: "layer gateway timeout error to s3 gateway timeout error", + err: frostfs.ErrGatewayTimeout, + expected: ErrGatewayTimeout, + }, + { + name: "s3 error to s3 error", + err: GetAPIError(ErrInvalidPart), + expected: ErrInvalidPart, + }, + { + name: "wrapped s3 error to s3 error", + err: fmt.Errorf("wrap: %w", GetAPIError(ErrInvalidPart)), + expected: ErrInvalidPart, + }, + } { + t.Run(tc.name, func(t *testing.T) { + err := TransformToS3Error(tc.err) + s3err, ok := err.(Error) + require.True(t, ok, "error must be s3 error") + require.Equalf(t, tc.expected, s3err.ErrCode, + "expected: '%s', got: '%s'", + GetAPIError(tc.expected).Code, GetAPIError(s3err.ErrCode).Code) + }) + } +} diff --git a/api/handler/util.go b/api/handler/util.go index 7b675c8..65b94af 100644 --- a/api/handler/util.go +++ b/api/handler/util.go @@ -12,10 +12,7 @@ import ( "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/data" apierr "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/errors" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer" - "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/frostfs" - "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/tree" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware" - frosterrors "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/frostfs/errors" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/logs" cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id" "go.opentelemetry.io/otel/trace" @@ -32,7 +29,7 @@ func (h *handler) reqLogger(ctx context.Context) *zap.Logger { func (h *handler) logAndSendError(w http.ResponseWriter, logText string, reqInfo *middleware.ReqInfo, err error, additional ...zap.Field) { err = handleDeleteMarker(w, err) - if code, wrErr := middleware.WriteErrorResponse(w, reqInfo, transformToS3Error(err)); wrErr != nil { + if code, wrErr := middleware.WriteErrorResponse(w, reqInfo, apierr.TransformToS3Error(err)); wrErr != nil { additional = append(additional, zap.NamedError("write_response_error", wrErr)) } else { additional = append(additional, zap.Int("status", code)) @@ -62,24 +59,6 @@ func handleDeleteMarker(w http.ResponseWriter, err error) error { return fmt.Errorf("%w: %s", apierr.GetAPIError(target.ErrorCode), err) } -func transformToS3Error(err error) error { - err = frosterrors.UnwrapErr(err) // this wouldn't work with errors.Join - if _, ok := err.(apierr.Error); ok { - return err - } - - if errors.Is(err, frostfs.ErrAccessDenied) || - errors.Is(err, tree.ErrNodeAccessDenied) { - return apierr.GetAPIError(apierr.ErrAccessDenied) - } - - if errors.Is(err, frostfs.ErrGatewayTimeout) { - return apierr.GetAPIError(apierr.ErrGatewayTimeout) - } - - return apierr.GetAPIError(apierr.ErrInternalError) -} - func (h *handler) ResolveBucket(ctx context.Context, bucket string) (*data.BucketInfo, error) { return h.obj.GetBucketInfo(ctx, bucket) } diff --git a/api/handler/util_test.go b/api/handler/util_test.go index f0eb4e1..abeebd1 100644 --- a/api/handler/util_test.go +++ b/api/handler/util_test.go @@ -1,65 +1 @@ package handler - -import ( - "errors" - "fmt" - "testing" - - s3errors "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/errors" - "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/frostfs" - "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/tree" - "github.com/stretchr/testify/require" -) - -func TestTransformS3Errors(t *testing.T) { - for _, tc := range []struct { - name string - err error - expected s3errors.ErrorCode - }{ - { - name: "simple std error to internal error", - err: errors.New("some error"), - expected: s3errors.ErrInternalError, - }, - { - name: "layer access denied error to s3 access denied error", - err: frostfs.ErrAccessDenied, - expected: s3errors.ErrAccessDenied, - }, - { - name: "wrapped layer access denied error to s3 access denied error", - err: fmt.Errorf("wrap: %w", frostfs.ErrAccessDenied), - expected: s3errors.ErrAccessDenied, - }, - { - name: "layer node access denied error to s3 access denied error", - err: tree.ErrNodeAccessDenied, - expected: s3errors.ErrAccessDenied, - }, - { - name: "layer gateway timeout error to s3 gateway timeout error", - err: frostfs.ErrGatewayTimeout, - expected: s3errors.ErrGatewayTimeout, - }, - { - name: "s3 error to s3 error", - err: s3errors.GetAPIError(s3errors.ErrInvalidPart), - expected: s3errors.ErrInvalidPart, - }, - { - name: "wrapped s3 error to s3 error", - err: fmt.Errorf("wrap: %w", s3errors.GetAPIError(s3errors.ErrInvalidPart)), - expected: s3errors.ErrInvalidPart, - }, - } { - t.Run(tc.name, func(t *testing.T) { - err := transformToS3Error(tc.err) - s3err, ok := err.(s3errors.Error) - require.True(t, ok, "error must be s3 error") - require.Equalf(t, tc.expected, s3err.ErrCode, - "expected: '%s', got: '%s'", - s3errors.GetAPIError(tc.expected).Code, s3errors.GetAPIError(s3err.ErrCode).Code) - }) - } -} diff --git a/api/layer/layer.go b/api/layer/layer.go index 17646c9..0768c83 100644 --- a/api/layer/layer.go +++ b/api/layer/layer.go @@ -53,7 +53,7 @@ type ( anonKey AnonymousKey resolver BucketResolver cache *Cache - treeService tree.TreeService + treeService tree.Service features FeatureSettings gateKey *keys.PrivateKey corsCnrInfo *data.BucketInfo @@ -66,7 +66,7 @@ type ( Cache *Cache AnonKey AnonymousKey Resolver BucketResolver - TreeService tree.TreeService + TreeService tree.Service Features FeatureSettings GateKey *keys.PrivateKey CORSCnrInfo *data.BucketInfo diff --git a/api/layer/tree/tree_service.go b/api/layer/tree/tree_service.go index ab658d9..db079b1 100644 --- a/api/layer/tree/tree_service.go +++ b/api/layer/tree/tree_service.go @@ -8,8 +8,8 @@ import ( oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id" ) -// TreeService provide interface to interact with tree service using s3 data models. -type TreeService interface { +// Service provide interface to interact with tree service using s3 data models. +type Service interface { // PutSettingsNode update or create new settings node in tree service. PutSettingsNode(ctx context.Context, bktInfo *data.BucketInfo, settings *data.BucketSettings) error diff --git a/api/middleware/auth.go b/api/middleware/auth.go index 998f3a1..cbdceb2 100644 --- a/api/middleware/auth.go +++ b/api/middleware/auth.go @@ -58,7 +58,6 @@ func Auth(center Center, log *zap.Logger) Func { } else { reqLogOrDefault(ctx, log).Error(logs.FailedToPassAuthentication, zap.Error(err)) err = frosterr.UnwrapErr(err) - log.Info("error", zap.Error(err), zap.String("code", err.(apierr.Error).Code), zap.String("description", err.(apierr.Error).Description)) if _, ok := err.(apierr.Error); !ok { err = apierr.GetAPIError(apierr.ErrAccessDenied) } diff --git a/api/middleware/policy.go b/api/middleware/policy.go index 109089a..2c07c3f 100644 --- a/api/middleware/policy.go +++ b/api/middleware/policy.go @@ -12,7 +12,6 @@ import ( "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/data" apierr "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/errors" - frostfsErrors "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/frostfs/errors" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/internal/logs" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain" "git.frostfs.info/TrueCloudLab/policy-engine/pkg/engine" @@ -85,7 +84,7 @@ func PolicyCheck(cfg PolicyConfig) Func { ctx := r.Context() if err := policyCheck(r, cfg); err != nil { reqLogOrDefault(ctx, cfg.Log).Error(logs.PolicyValidationFailed, zap.Error(err)) - err = frostfsErrors.UnwrapErr(err) + err = apierr.TransformToS3Error(err) if _, wrErr := WriteErrorResponse(w, GetReqInfo(ctx), err); wrErr != nil { reqLogOrDefault(ctx, cfg.Log).Error(logs.FailedToWriteResponse, zap.Error(wrErr)) } diff --git a/api/router_mock_test.go b/api/router_mock_test.go index 817b0d4..4f237f9 100644 --- a/api/router_mock_test.go +++ b/api/router_mock_test.go @@ -40,8 +40,9 @@ type centerMock struct { t *testing.T anon bool noAuthHeader bool - isError bool + err error attrs []object.Attribute + key *keys.PrivateKey } func (c *centerMock) Authenticate(*http.Request) (*middleware.Box, error) { @@ -49,8 +50,8 @@ func (c *centerMock) Authenticate(*http.Request) (*middleware.Box, error) { return nil, middleware.ErrNoAuthorizationHeader } - if c.isError { - return nil, fmt.Errorf("some error") + if c.err != nil { + return nil, c.err } var token *bearer.Token @@ -58,8 +59,12 @@ func (c *centerMock) Authenticate(*http.Request) (*middleware.Box, error) { if !c.anon { bt := bearertest.Token() token = &bt - key, err := keys.NewPrivateKey() - require.NoError(c.t, err) + key := c.key + if key == nil { + var err error + key, err = keys.NewPrivateKey() + require.NoError(c.t, err) + } require.NoError(c.t, token.Sign(key.PrivateKey)) } @@ -151,22 +156,21 @@ func (m *xmlMock) NewXMLDecoder(r io.Reader) *xml.Decoder { } type resourceTaggingMock struct { - bucketTags map[string]string - objectTags map[string]string - noSuchObjectKey bool - noSuchBucketKey bool + bucketTags map[string]string + objectTags map[string]string + err error } func (m *resourceTaggingMock) GetBucketTagging(context.Context, *data.BucketInfo) (map[string]string, error) { - if m.noSuchBucketKey { - return nil, apierr.GetAPIError(apierr.ErrNoSuchKey) + if m.err != nil { + return nil, m.err } return m.bucketTags, nil } func (m *resourceTaggingMock) GetObjectTagging(context.Context, *data.GetObjectTaggingParams) (string, map[string]string, error) { - if m.noSuchObjectKey { - return "", nil, apierr.GetAPIError(apierr.ErrNoSuchKey) + if m.err != nil { + return "", nil, m.err } return "", m.objectTags, nil } diff --git a/api/router_test.go b/api/router_test.go index c4a524f..a219109 100644 --- a/api/router_test.go +++ b/api/router_test.go @@ -15,6 +15,7 @@ import ( "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/data" apierr "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/errors" + "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/layer/frostfs" s3middleware "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/api/middleware" "git.frostfs.info/TrueCloudLab/frostfs-s3-gw/metrics" "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object" @@ -26,6 +27,7 @@ import ( "git.frostfs.info/TrueCloudLab/policy-engine/schema/s3" "github.com/go-chi/chi/v5" "github.com/go-chi/chi/v5/middleware" + "github.com/nspcc-dev/neo-go/pkg/crypto/keys" "github.com/prometheus/client_golang/prometheus" "github.com/stretchr/testify/require" "go.uber.org/zap/zaptest" @@ -274,6 +276,36 @@ func TestPolicyCheckerReqTypeDetermination(t *testing.T) { }) } +func TestPolicyCheckFrostfsErrors(t *testing.T) { + chiRouter := prepareRouter(t) + ns1, bktName1, objName1 := "", "bucket", "object" + + createBucket(chiRouter, ns1, bktName1) + key, err := keys.NewPrivateKey() + require.NoError(t, err) + chiRouter.cfg.Center.(*centerMock).key = key + chiRouter.cfg.MiddlewareSettings.(*middlewareSettingsMock).denyByDefault = true + + ruleChain := &chain.Chain{ + ID: chain.ID("id"), + Rules: []chain.Rule{{ + Status: chain.Allow, + Actions: chain.Actions{Names: []string{"*"}}, + Resources: chain.Resources{Names: []string{fmt.Sprintf(s3.ResourceFormatS3BucketObjects, bktName1)}}, + }}, + } + + _, _, err = chiRouter.policyChecker.MorphRuleChainStorage().AddMorphRuleChain(chain.S3, engine.UserTarget(ns1+":"+key.Address()), ruleChain) + require.NoError(t, err) + + // check we can access 'bucket' in default namespace + putObject(chiRouter, ns1, bktName1, objName1, nil) + + chiRouter.cfg.Center.(*centerMock).anon = true + chiRouter.cfg.Tagging.(*resourceTaggingMock).err = frostfs.ErrAccessDenied + getObjectErr(chiRouter, ns1, bktName1, objName1, apierr.ErrAccessDenied) +} + func TestDefaultBehaviorPolicyChecker(t *testing.T) { chiRouter := prepareRouter(t) ns, bktName := "", "bucket" @@ -524,11 +556,10 @@ func TestResourceTagsCheck(t *testing.T) { listObjectsV1Err(router, ns, bktName, "", "", "", apierr.ErrNoSuchBucket) - router.cfg.Tagging.(*resourceTaggingMock).noSuchBucketKey = true + router.cfg.Tagging.(*resourceTaggingMock).err = apierr.GetAPIError(apierr.ErrNoSuchKey) createBucket(router, ns, bktName) getBucketErr(router, ns, bktName, apierr.ErrNoSuchKey) - router.cfg.Tagging.(*resourceTaggingMock).noSuchObjectKey = true createBucket(router, ns, bktName) getObjectErr(router, ns, bktName, objName, apierr.ErrNoSuchKey) }) @@ -826,8 +857,11 @@ func TestAuthenticate(t *testing.T) { createBucket(chiRouter, "", "bkt-2") chiRouter = prepareRouter(t) - chiRouter.cfg.Center.(*centerMock).isError = true + chiRouter.cfg.Center.(*centerMock).err = apierr.GetAPIError(apierr.ErrAccessDenied) createBucketErr(chiRouter, "", "bkt-3", nil, apierr.ErrAccessDenied) + + chiRouter.cfg.Center.(*centerMock).err = frostfs.ErrGatewayTimeout + createBucketErr(chiRouter, "", "bkt-3", nil, apierr.ErrGatewayTimeout) } func TestFrostFSIDValidation(t *testing.T) {