ape: Improve error messages in ape service #919

aarifullin · 2024-01-18T11:39:16Z

aarifullin commented

2024-01-18 11:39:16 +00:00

Wrap all APE middleware errors in apeErr that
makes errors more explicit with status AccessDenied.
Use denyingRuleErr for denying status from chain router.

* Wrap all APE middleware errors in apeErr that makes errors more explicit with status AccessDenied. * Use denyingRuleErr for denying status from chain router.

aarifullin requested review from storage-core-committers 2024-01-18 11:40:05 +00:00

aarifullin requested review from storage-core-developers 2024-01-18 11:40:38 +00:00

fyrchik approved these changes 2024-01-18 14:38:55 +00:00

pkg/services/object/ape/errors.go Outdated

													
				@ -0,0 +7,4 @@

					"git.frostfs.info/TrueCloudLab/policy-engine/pkg/chain"

				)

				func apeErr(err error) error {

fyrchik commented

2024-01-18 14:34:36 +00:00

Minor sugession: APE err is actually an argument, what we do here is add API status, how about toStatus() or wrapStatus or at least toStatusErr?

Minor sugession: APE err is actually an argument, what we do here is add API status, how about `toStatus()` or `wrapStatus` or at least `toStatusErr`?

pkg/services/object/ape/errors.go Outdated

													
				@ -0,0 +13,4 @@

					return errAccessDenied

				}

				type denyingRuleErr struct {

fyrchik commented

2024-01-18 14:38:50 +00:00

The only thing we do with this type is translate it to string in apeErr anyway, why do we need it? We do not use %w, so we might as well have old wrapper and a new one. This way we won't have 2 fmt.Sprintf.

The only thing we do with this type is translate it to string in `apeErr` anyway, why do we need it? We do not use `%w`, so we might as well have old wrapper and a new one. This way we won't have 2 `fmt.Sprintf`.

aarifullin commented

2024-01-19 08:03:21 +00:00

Poster

Initally that was for unit-test sake: I want a test to expect only this kind of error (with require.ErrorAs) but I think you're right - this does not make a big deal. I removed it and apeErr -> toStatusErr

Initally that was for unit-test sake: I want a test to expect only this kind of error (with `require.ErrorAs`) but I think you're right - this does not make a big deal. I removed it and `apeErr` -> `toStatusErr`

pkg/services/object/ape/errors.go Outdated

													
				@ -0,0 +28,4 @@

				}

				func (d *denyingRuleErr) Error() string {

					return fmt.Sprintf("found denying rule for %s: %s", d.method, d.status.String())