Fix error for APE check #1524

aarifullin · 2024-11-27T12:57:03Z

aarifullin commented

2024-11-27 12:57:03 +00:00

Common APE checking logic was moved into a common package but CheckAPE has been returning the error wrapped into apistatus. That leads to this effect:

$ frostfs-cli container list-objects -w wallet.json -r localhost:8080 --cid CVoGbSRn4CJFReL2mDVTma7
MuRg1pmDDZmMYwRXVndF2 | head
Enter password >
rpc error: read object list: status: code = 2048 message = access to object operation denied: ape denied request: status: code = 2048 message = access to object operation denied

So:

The error is not wrapped to apistatus in common package
Tree service wraps it on its own
Object service already does this on its own - no change is required

Common APE checking logic was moved into a common package but `CheckAPE` has been returning the error wrapped into `apistatus`. That leads to this effect: ```bash $ frostfs-cli container list-objects -w wallet.json -r localhost:8080 --cid CVoGbSRn4CJFReL2mDVTma7 MuRg1pmDDZmMYwRXVndF2 | head Enter password > rpc error: read object list: status: code = 2048 message = access to object operation denied: ape denied request: status: code = 2048 message = access to object operation denied ``` So: - The error is not wrapped to `apistatus` in `common` package - Tree service wraps it on its own - Object service already does this on its own - no change is required

aarifullin force-pushed fix/ape/error from cba53ccdfc to d831ffc695

2024-11-27 12:58:02 +00:00

Compare

requested reviews from storage-core-committers, storage-core-developers

2024-11-27 12:58:18 +00:00

dstepanov-yadro approved these changes 2024-11-27 13:08:47 +00:00

acid-ant approved these changes 2024-11-27 13:08:50 +00:00

elebedeva approved these changes 2024-11-27 14:47:27 +00:00

fyrchik reviewed 2024-11-28 05:49:41 +00:00

pkg/services/common/ape/checker.go

					
				@ -112,3 +106,1 @@

					errAccessDenied := &apistatus.ObjectAccessDenied{}

					errAccessDenied.WriteReason(err.Error())

					return errAccessDenied

					return fmt.Errorf("access to operation %s is denied by access policy engine: %s", prm.Request.Operation(), status.String())

fyrchik commented

2024-11-28 05:49:29 +00:00

This file has not been touched by the recent refactoring.
Why change it instead of fixing what has changed there?

This file has not been touched by the recent refactoring. Why change it instead of fixing what has changed there?

aarifullin commented

2024-11-28 08:30:56 +00:00

It seems in PR #1362 I unnecessarily moved apeErr to common package because I was thinking rather about tree service than object service. It's better if the common package just returns error without statuses and each service wraps into status on its own

Why change it instead of fixing what has changed there?

Let's have a look at the ape middleware of object service. Any failure is turned to object access denied by toStatusErr invocation

func toStatusErr(err error) error {
	errAccessDenied := &apistatus.ObjectAccessDenied{}
	errAccessDenied.WriteReason("ape denied request: " + err.Error())
	return errAccessDenied
}

So, I suppose the middleware wraps an error on its own and I don't need to check statuses within

It seems in PR #1362 I unnecessarily [moved](https://git.frostfs.info/TrueCloudLab/frostfs-node/pulls/1362/files#diff-96aa001a23f215fa6c13e999754461bfa7dbd606) `apeErr` to common package because I was thinking rather about `tree` service than `object` service. It's better if the common package just returns error without statuses and each service wraps into status on its own > Why change it instead of fixing what has changed there? Let's have a look at the ape middleware of object service. Any failure is turned to `object access denied` by `toStatusErr` invocation ```go func toStatusErr(err error) error { errAccessDenied := &apistatus.ObjectAccessDenied{} errAccessDenied.WriteReason("ape denied request: " + err.Error()) return errAccessDenied } ``` So, I suppose the middleware wraps an error on its own and I don't need to check statuses within