Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BasicACLErr and eACLErr errors are created incorrectly #2465

Closed
Ayrtat opened this issue Aug 1, 2023 · 4 comments
Closed

BasicACLErr and eACLErr errors are created incorrectly #2465

Ayrtat opened this issue Aug 1, 2023 · 4 comments
Milestone

Comments

@Ayrtat
Copy link

Ayrtat commented Aug 1, 2023

Expected Behavior

An error created by the method basicACLErr or eACLErr must be able to be casted to

var errAccessDenied *apistatus.ObjectAccessDenied

Current Behavior

It seems getObject method for object service works incorrectly here because the casting errors.As(err, &errAccessDenied) doesn't work out

Possible Solution

https://github.com/nspcc-dev/neofs-node/blob/master/pkg/services/object/acl/v2/errors.go#L28
https://github.com/nspcc-dev/neofs-node/blob/master/pkg/services/object/acl/v2/errors.go#L35
var errAccessDenied apistatus.ObjectAccessDenied should be replaced with errAccessDenied := &apistatus.ObjectAccessDenied{}

Steps to Reproduce (for bugs)

You can check it with unit-test

func TestBasicACLErr(t *testing.T) {
	var reqInfo RequestInfo
	err := basicACLErr(reqInfo)

	var errAccessDenied *apistatus.ObjectAccessDenied

	require.ErrorAs(t, err, &errAccessDenied,
		"basicACLErr must be able to be casted to apistatus.ObjectAccessDenied")
}
@Ayrtat Ayrtat added the triage label Aug 1, 2023
@cthulhu-rider
Copy link
Contributor

these code spaces are executed on different processes - this on one node-as-client, this on another node-as-server. So, type mismatch must not affect behavior

@roman-khimov
Copy link
Member

I think we can still check our error.As uses and try converting them to error.Is using new SDK types and instances, those work fine both for simple instances and pointers. And the code will likely be a bit more readable this way.

@Ayrtat
Copy link
Author

Ayrtat commented Aug 2, 2023

these code spaces are executed on different processes - this on one node-as-client, this on another node-as-server. So, type mismatch must not affect behavior

Sorry, I didn't get the point. The client code invokes PayloadRange that gets a response from server side. And server returns wrapped basicACLErr error that is failed to get casted to apistatus.ObjectAccessDenied

@cthulhu-rider
Copy link
Contributor

cthulhu-rider commented Aug 2, 2023

@Ayrtat i mean this calls client method and it must handle client's claimed return no matter what the server returns

as @roman-khimov said, we just inaccurately updated to a NeoFS SDK RC-9 that changed the documentation of returned errors: actual instead of previous

i suppose there may be other problem cases related to status error handling, lets resolve all of them in this issue

@roman-khimov roman-khimov added this to the v0.39.0 milestone Aug 2, 2023
cthulhu-rider added a commit to cthulhu-rider/neofs-node that referenced this issue Aug 3, 2023
Currently used SDK revision changed/improved docs and UX of error
handling. Storage node missed these changes on SDK upgrade, so we have
to adjust to them.

Use `errors.Is` when exact error structure is not needed. Actualize docs
of errors returned by internal client.

Fixes nspcc-dev#2465.

Signed-off-by: Leonard Lyubich <leonard@morphbits.io>
@roman-khimov roman-khimov modified the milestones: v0.39.0, v0.38.0 Aug 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants