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) err.WriteReason(reason)
return err return err
} }
func ErrAPEManagerInvalidArgument(msg string) error {
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 { func (s *Service) validateContainerTargetRequest(ctx context.Context, cid string, pubKey *keys.PublicKey) error {
var cidSDK cidSDK.ID var cidSDK cidSDK.ID
if err := cidSDK.DecodeString(cid); err != nil { 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))
} }
isOwner, err := s.isActorContainerOwner(ctx, cidSDK, pubKey) isOwner, err := s.isActorContainerOwner(ctx, cidSDK, pubKey)
if err != nil { 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()) chain, err := decodeAndValidateChain(req.GetBody().GetChain().GetKind().(*apeV2.ChainRaw).GetRaw())
if err != nil { if err != nil {
return nil, err return nil, apemanager_errors.ErrAPEManagerInvalidArgument(err.Error())
} }
if len(chain.ID) == 0 { if len(chain.ID) == 0 {
const randomIDLength = 10 const randomIDLength = 10
@ -122,7 +122,7 @@ func (s *Service) AddChain(ctx context.Context, req *apemanagerV2.AddChainReques
} }
target = policy_engine.ContainerTarget(reqCID) target = policy_engine.ContainerTarget(reqCID)
default: 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) 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) target = policy_engine.ContainerTarget(reqCID)
default: 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()) 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) target = policy_engine.ContainerTarget(reqCID)
default: 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) chs, err := s.contractStorage.ListMorphRuleChains(apechain.Ingress, target)
@ -227,11 +227,11 @@ func getSignaturePublicKey(vh *session.RequestVerificationHeader) (*keys.PublicK
} }
sig := vh.GetBodySignature() sig := vh.GetBodySignature()
if sig == nil { if sig == nil {
return nil, errEmptyBodySignature return nil, apemanager_errors.ErrAPEManagerInvalidArgument(errEmptyBodySignature.Error())
} }
key, err := keys.NewPublicKeyFromBytes(sig.GetKey(), elliptic.P256()) key, err := keys.NewPublicKeyFromBytes(sig.GetKey(), elliptic.P256())
if err != nil { 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 return key, nil