From f2f3294fc3e827ba8faecdf6ab121ba5cbe85231 Mon Sep 17 00:00:00 2001 From: Airat Arifullin Date: Thu, 18 Jan 2024 14:35:52 +0300 Subject: [PATCH] [#919] ape: Improve error messages in ape service * Wrap all APE middleware errors in apeErr that makes errors more explicit with status AccessDenied. * Use denyingRuleErr for denying status from chain router. Signed-off-by: Airat Arifullin --- pkg/services/object/ape/checker.go | 11 +-------- pkg/services/object/ape/checker_test.go | 6 ++--- pkg/services/object/ape/errors.go | 13 +++++++++++ pkg/services/object/ape/service.go | 30 ++++++++++++------------- 4 files changed, 32 insertions(+), 28 deletions(-) create mode 100644 pkg/services/object/ape/errors.go diff --git a/pkg/services/object/ape/checker.go b/pkg/services/object/ape/checker.go index fdaa9df85..ccd23bf06 100644 --- a/pkg/services/object/ape/checker.go +++ b/pkg/services/object/ape/checker.go @@ -5,7 +5,6 @@ import ( "fmt" objectV2 "git.frostfs.info/TrueCloudLab/frostfs-api-go/v2/object" - apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status" cid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/container/id" oid "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/object/id" apechain "git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain" @@ -70,13 +69,5 @@ func (c *checkerImpl) CheckAPE(ctx context.Context, prm Prm) error { return nil } - return apeErr(prm.Method, status) -} - -const accessDeniedAPEReasonFmt = "access to operation %s is denied by access policy engine: %s" - -func apeErr(op string, status apechain.Status) error { - errAccessDenied := &apistatus.ObjectAccessDenied{} - errAccessDenied.WriteReason(fmt.Sprintf(accessDeniedAPEReasonFmt, op, status.String())) - return errAccessDenied + return fmt.Errorf("found denying rule for %s: %s", prm.Method, status) } diff --git a/pkg/services/object/ape/checker_test.go b/pkg/services/object/ape/checker_test.go index 9d548480b..08d097907 100644 --- a/pkg/services/object/ape/checker_test.go +++ b/pkg/services/object/ape/checker_test.go @@ -308,7 +308,8 @@ func TestAPECheck(t *testing.T) { ms := inmemory.NewInmemoryMorphRuleChainStorage() ls.AddOverride(chain.Ingress, policyengine.ContainerTarget(test.container), &chain.Chain{ - Rules: test.containerRules, + Rules: test.containerRules, + MatchType: chain.MatchTypeFirstMatch, }) router := policyengine.NewDefaultChainRouterWithLocalOverrides(ms, ls) @@ -336,8 +337,7 @@ func TestAPECheck(t *testing.T) { err := checker.CheckAPE(context.Background(), prm) if test.expectAPEErr { - aErr := apeErr(method, chain.AccessDenied) - require.ErrorAs(t, err, &aErr) + require.Error(t, err) } else { require.NoError(t, err) } diff --git a/pkg/services/object/ape/errors.go b/pkg/services/object/ape/errors.go new file mode 100644 index 000000000..7c2a82dfd --- /dev/null +++ b/pkg/services/object/ape/errors.go @@ -0,0 +1,13 @@ +package ape + +import ( + "fmt" + + apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status" +) + +func toStatusErr(err error) error { + errAccessDenied := &apistatus.ObjectAccessDenied{} + errAccessDenied.WriteReason(fmt.Sprintf("ape denied request: %s", err.Error())) + return errAccessDenied +} diff --git a/pkg/services/object/ape/service.go b/pkg/services/object/ape/service.go index d28ad4aeb..781f9df4b 100644 --- a/pkg/services/object/ape/service.go +++ b/pkg/services/object/ape/service.go @@ -69,7 +69,7 @@ func (g *getStreamBasicChecker) Send(resp *objectV2.GetResponse) error { if partInit, ok := resp.GetBody().GetObjectPart().(*objectV2.GetObjectPartInit); ok { cnrID, objID, err := getAddressParamsSDK(partInit.GetHeader().GetContainerID(), partInit.GetObjectID()) if err != nil { - return err + return toStatusErr(err) } prm := Prm{ @@ -82,7 +82,7 @@ func (g *getStreamBasicChecker) Send(resp *objectV2.GetResponse) error { } if err := g.apeChecker.CheckAPE(g.Context(), prm); err != nil { - return err + return toStatusErr(err) } } return g.GetObjectStream.Send(resp) @@ -103,12 +103,12 @@ func requestContext(ctx context.Context) (*objectSvc.RequestContext, error) { func (c *Service) Get(request *objectV2.GetRequest, stream objectSvc.GetObjectStream) error { cnrID, objID, err := getAddressParamsSDK(request.GetBody().GetAddress().GetContainerID(), request.GetBody().GetAddress().GetObjectID()) if err != nil { - return err + return toStatusErr(err) } reqCtx, err := requestContext(stream.Context()) if err != nil { - return err + return toStatusErr(err) } err = c.apeChecker.CheckAPE(stream.Context(), Prm{ @@ -120,7 +120,7 @@ func (c *Service) Get(request *objectV2.GetRequest, stream objectSvc.GetObjectSt SenderKey: hex.EncodeToString(reqCtx.SenderKey), }) if err != nil { - return err + return toStatusErr(err) } return c.next.Get(request, &getStreamBasicChecker{ @@ -139,12 +139,12 @@ func (p *putStreamBasicChecker) Send(ctx context.Context, request *objectV2.PutR if partInit, ok := request.GetBody().GetObjectPart().(*objectV2.PutObjectPartInit); ok { reqCtx, err := requestContext(ctx) if err != nil { - return err + return toStatusErr(err) } cnrID, objID, err := getAddressParamsSDK(partInit.GetHeader().GetContainerID(), partInit.GetObjectID()) if err != nil { - return err + return toStatusErr(err) } prm := Prm{ @@ -158,7 +158,7 @@ func (p *putStreamBasicChecker) Send(ctx context.Context, request *objectV2.PutR } if err := p.apeChecker.CheckAPE(ctx, prm); err != nil { - return err + return toStatusErr(err) } } @@ -244,13 +244,13 @@ func (c *Service) Search(request *objectV2.SearchRequest, stream objectSvc.Searc var cnrID cid.ID if cnrV2 := request.GetBody().GetContainerID(); cnrV2 != nil { if err := cnrID.ReadFromV2(*cnrV2); err != nil { - return err + return toStatusErr(err) } } reqCtx, err := requestContext(stream.Context()) if err != nil { - return err + return toStatusErr(err) } err = c.apeChecker.CheckAPE(stream.Context(), Prm{ @@ -261,7 +261,7 @@ func (c *Service) Search(request *objectV2.SearchRequest, stream objectSvc.Searc SenderKey: hex.EncodeToString(reqCtx.SenderKey), }) if err != nil { - return err + return toStatusErr(err) } return c.next.Search(request, stream) @@ -301,12 +301,12 @@ func (c *Service) Delete(ctx context.Context, request *objectV2.DeleteRequest) ( func (c *Service) GetRange(request *objectV2.GetRangeRequest, stream objectSvc.GetObjectRangeStream) error { cnrID, objID, err := getAddressParamsSDK(request.GetBody().GetAddress().GetContainerID(), request.GetBody().GetAddress().GetObjectID()) if err != nil { - return err + return toStatusErr(err) } reqCtx, err := requestContext(stream.Context()) if err != nil { - return err + return toStatusErr(err) } err = c.apeChecker.CheckAPE(stream.Context(), Prm{ @@ -318,7 +318,7 @@ func (c *Service) GetRange(request *objectV2.GetRangeRequest, stream objectSvc.G SenderKey: hex.EncodeToString(reqCtx.SenderKey), }) if err != nil { - return err + return toStatusErr(err) } return c.next.GetRange(request, stream) @@ -381,7 +381,7 @@ func (c *Service) PutSingle(ctx context.Context, request *objectV2.PutSingleRequ } if err = c.apeChecker.CheckAPE(ctx, prm); err != nil { - return nil, err + return nil, toStatusErr(err) } return c.next.PutSingle(ctx, request)