TrueCloudLab/frostfs-node
20
1
Fork 28
Code Issues 106 Pull requests 4 Projects Releases 2 Activity Actions

Wrap some errors with InvalidArgument status in apemanager service #1651

Merged
fyrchik merged 1 commit from aarifullin/frostfs-node:feat/improve_apemngr_errors into master 2025-02-27 18:25:10 +00:00
Conversation 0 Commits 1 Files changed 2 +13 -7
2 changed files with 13 additions and 7 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

6
pkg/services/apemanager/errors/errors.go
View file

@ -9,3 +9,9 @@ func ErrAPEManagerAccessDenied(reason string) error {
err.WriteReason(reason)
return err
}
func ErrAPEManagerInvalidArgument(msg string) error {
dstepanov-yadro marked this conversation as resolved Outdated
dstepanov-yadro commented 2025-02-21 06:40:36 +00:00
Outdated
Review
Copy link

Please write down the motivation for this change. In particular, I am interested in using status.Error(codes.InvalidArgument...) vs apemanager_errors.ErrAPEManagerInvalidArgument

Please write down the motivation for this change. In particular, I am interested in using `status.Error(codes.InvalidArgument...)` vs `apemanager_errors.ErrAPEManagerInvalidArgument`
aarifullin commented 2025-02-21 11:21:27 +00:00
Outdated
Review
Copy link

status.Error(codes.InvalidArgument...) - probably, you're talking about the status from grpc-package (used in control services).
ErrAPEManagerInvalidArgument wraps into frostfs-api status

`status.Error(codes.InvalidArgument...)` - probably, you're talking about the status from `grpc`-package (used in control services). `ErrAPEManagerInvalidArgument` wraps into frostfs-api status
dstepanov-yadro commented 2025-02-21 11:46:22 +00:00
Outdated
Review
Copy link

Got it, thanks

Got it, thanks
err := new(apistatus.InvalidArgument)
err.SetMessage(msg)
return err
}

14
pkg/services/apemanager/executor.go
View file

@ -81,7 +81,7 @@ var _ Server = (*Service)(nil)
func (s *Service) validateContainerTargetRequest(ctx context.Context, cid string, pubKey *keys.PublicKey) error {
var cidSDK cidSDK.ID
if err := cidSDK.DecodeString(cid); err != nil {
return fmt.Errorf("invalid CID format: %w", err)
return apemanager_errors.ErrAPEManagerInvalidArgument(fmt.Sprintf("invalid CID format: %v", err))
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2025-02-25 09:29:38 +00:00
Outdated
Review
Copy link

fmt.Errorf("... %w", err).Error() is equivalent to fmt.Sprintf("... %v", err)
bdcd6d1b65/src/fmt/print.go (L1063)
bdcd6d1b65/src/fmt/errors.go (L31)

For %s it is even easier.

`fmt.Errorf("... %w", err).Error()` is equivalent to `fmt.Sprintf("... %v", err)` https://github.com/golang/go/blob/bdcd6d1b653dd7a5b3eb9a053623f85433ff9e6b/src/fmt/print.go#L1063 https://github.com/golang/go/blob/bdcd6d1b653dd7a5b3eb9a053623f85433ff9e6b/src/fmt/errors.go#L31 For `%s` it is even easier.
aarifullin commented 2025-02-27 08:49:25 +00:00
Outdated
Review
Copy link

fixed to %s

fixed to `%s`
fyrchik commented 2025-02-27 08:58:18 +00:00
Outdated
Review
Copy link

That is not what I wanted to say:

err = fmt.Errorf("...: %w", err)
return apemanager_errors.ErrAPEManagerInvalidArgument(err.Error())

is equivalent to

return apemanager_errors.ErrAPEManagerInvalidArgument(fmt.Sprintf("...: %v", err))

So no intermediate error (via fmt.Errorf) is needed.
The same could be done in multiple other places in this PR

That is not what I wanted to say: ```go err = fmt.Errorf("...: %w", err) return apemanager_errors.ErrAPEManagerInvalidArgument(err.Error()) ``` is equivalent to ```go return apemanager_errors.ErrAPEManagerInvalidArgument(fmt.Sprintf("...: %v", err)) ``` So no intermediate error (via `fmt.Errorf`) is needed. The same could be done in multiple other places in this PR
aarifullin commented 2025-02-27 14:33:14 +00:00
Outdated
Review
Copy link

Ah. Okay. Fixed

Ah. Okay. Fixed
}
isOwner, err := s.isActorContainerOwner(ctx, cidSDK, pubKey)
if err != nil {
@ -101,7 +101,7 @@ func (s *Service) AddChain(ctx context.Context, req *apemanagerV2.AddChainReques
chain, err := decodeAndValidateChain(req.GetBody().GetChain().GetKind().(*apeV2.ChainRaw).GetRaw())
if err != nil {
return nil, err
return nil, apemanager_errors.ErrAPEManagerInvalidArgument(err.Error())
}
if len(chain.ID) == 0 {
const randomIDLength = 10
@ -122,7 +122,7 @@ func (s *Service) AddChain(ctx context.Context, req *apemanagerV2.AddChainReques
}
target = policy_engine.ContainerTarget(reqCID)
default:
return nil, fmt.Errorf("unsupported target type: %s", targetType)
return nil, apemanager_errors.ErrAPEManagerInvalidArgument(fmt.Sprintf("unsupported target type: %s", targetType))
}
txHash, vub, err := s.contractStorage.AddMorphRuleChain(apechain.Ingress, target, &chain)
@ -158,7 +158,7 @@ func (s *Service) RemoveChain(ctx context.Context, req *apemanagerV2.RemoveChain
}
target = policy_engine.ContainerTarget(reqCID)
default:
return nil, fmt.Errorf("unsupported target type: %s", targetType)
return nil, apemanager_errors.ErrAPEManagerInvalidArgument(fmt.Sprintf("unsupported target type: %s", targetType))
}
txHash, vub, err := s.contractStorage.RemoveMorphRuleChain(apechain.Ingress, target, req.GetBody().GetChainID())
@ -193,7 +193,7 @@ func (s *Service) ListChains(ctx context.Context, req *apemanagerV2.ListChainsRe
}
target = policy_engine.ContainerTarget(reqCID)
default:
return nil, fmt.Errorf("unsupported target type: %s", targetType)
return nil, apemanager_errors.ErrAPEManagerInvalidArgument(fmt.Sprintf("unsupported target type: %s", targetType))
fyrchik commented 2025-02-27 14:56:43 +00:00
Review
Copy link

4 lines above we also return an error, but do not wrap it, why?

4 lines above we also return an error, but do not wrap it, why?
aarifullin commented 2025-02-27 15:14:04 +00:00
Review
Copy link

validateContainerTargetRequest?

You can see it already wraps errors within

// validateContainerTargetRequest validates request for the container target.
// It checks if request actor is the owner of the container, otherwise it denies the request.
func (s *Service) validateContainerTargetRequest(ctx context.Context, cid string, pubKey *keys.PublicKey) error {
	var cidSDK cidSDK.ID
	if err := cidSDK.DecodeString(cid); err != nil {
		return apemanager_errors.ErrAPEManagerInvalidArgument(fmt.Sprintf("invalid CID format: %v", err))
	}
	isOwner, err := s.isActorContainerOwner(ctx, cidSDK, pubKey)
	if err != nil {
		return fmt.Errorf("failed to check owner: %w", err)
	}
	if !isOwner {
		return apemanager_errors.ErrAPEManagerAccessDenied("actor must be container owner")
	}
	return nil
}

I know this is not a good approach when you wrap errors within call. But

  1. validateContainerTargetRequest is the internal method used only in ape-manager anyway
  2. If we need to wrap outside, then I need to introduce new error types to return them from invocation. Then categorize them outside of invocation to select proper wrapper ErrAPEManagerInvalidArgument and ErrAPEManagerAccessDenied

In this case I suppose it's not worth it

`validateContainerTargetRequest`? You can see it already wraps errors within ```go // validateContainerTargetRequest validates request for the container target. // It checks if request actor is the owner of the container, otherwise it denies the request. func (s *Service) validateContainerTargetRequest(ctx context.Context, cid string, pubKey *keys.PublicKey) error { var cidSDK cidSDK.ID if err := cidSDK.DecodeString(cid); err != nil { return apemanager_errors.ErrAPEManagerInvalidArgument(fmt.Sprintf("invalid CID format: %v", err)) } isOwner, err := s.isActorContainerOwner(ctx, cidSDK, pubKey) if err != nil { return fmt.Errorf("failed to check owner: %w", err) } if !isOwner { return apemanager_errors.ErrAPEManagerAccessDenied("actor must be container owner") } return nil } ``` I know this is not a good approach when you wrap errors within call. But 1. `validateContainerTargetRequest` is the internal method used only in ape-manager anyway 2. If we need to wrap outside, then I need to introduce new error types to return them from invocation. Then categorize them outside of invocation to select proper wrapper `ErrAPEManagerInvalidArgument` and `ErrAPEManagerAccessDenied` In this case I suppose it's not worth it
}
chs, err := s.contractStorage.ListMorphRuleChains(apechain.Ingress, target)
@ -227,11 +227,11 @@ func getSignaturePublicKey(vh *session.RequestVerificationHeader) (*keys.PublicK
}
sig := vh.GetBodySignature()
if sig == nil {
return nil, errEmptyBodySignature
return nil, apemanager_errors.ErrAPEManagerInvalidArgument(errEmptyBodySignature.Error())
}
key, err := keys.NewPublicKeyFromBytes(sig.GetKey(), elliptic.P256())
if err != nil {
return nil, fmt.Errorf("invalid signature key: %w", err)
return nil, apemanager_errors.ErrAPEManagerInvalidArgument(fmt.Sprintf("invalid signature key: %v", err))
}
return key, nil