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 added 2 commits 2024-11-27 12:57:04 +00:00

[#XX] ape: Make APE checker return error without status 64582215c9

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>

[#xx] tree: Make check APE error get wrapped to api status

Tests and linters / gopls check (pull_request) Failing after 50s

Details

Pre-commit hooks / Pre-commit (pull_request) Failing after 1m10s

Details

Tests and linters / Tests (pull_request) Failing after 1m11s

Details

Vulncheck / Vulncheck (pull_request) Failing after 1m49s

Details

Build / Build Components (pull_request) Failing after 2m4s

Details

Tests and linters / Lint (pull_request) Failing after 2m0s

Details

DCO action / DCO (pull_request) Failing after 2m2s

Details

Tests and linters / Tests with -race (pull_request) Failing after 2m1s

Details

Tests and linters / Run gofumpt (pull_request) Failing after 2m10s

Details

Tests and linters / Staticcheck (pull_request) Failing after 2m12s

Details

cba53ccdfc

Signed-off-by: Airat Arifullin <a.arifullin@yadro.com>

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

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

Compare

aarifullin requested review from storage-core-committers 2024-11-27 12:58:11 +00:00

aarifullin requested review from 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