TrueCloudLab/frostfs-node
20
1
Fork 27
Code Issues 105 Pull requests 9 Projects Releases 2 Activity Actions

Introduce ChainRouterError error type and wrap only these errors with ObjectAccessDenied status #1563

Merged
fyrchik merged 3 commits from aarifullin/frostfs-node:fix/ape_logicalerr into master 2024-12-17 08:24:32 +00:00
Conversation 1 Commits 3 Files changed 4 +9
1 changed files with 9 additions and 0 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 1a091ea7bb - Show all commits

9
pkg/services/object/ape/errors.go
View file

@ -1,10 +1,19 @@
package ape
import (
"errors"
checkercore "git.frostfs.info/TrueCloudLab/frostfs-node/pkg/services/common/ape"
apistatus "git.frostfs.info/TrueCloudLab/frostfs-sdk-go/client/status"
)
func toStatusErr(err error) error {
var chRouterErr *checkercore.ChainRouterError
if !errors.As(err, &chRouterErr) {
errServerInternal := &apistatus.ServerInternal{}
fyrchik marked this conversation as resolved Outdated
fyrchik commented 2024-12-16 11:33:59 +00:00
Outdated
Review
Copy link

Where do we do wrapping to internal status (if we have any)?
I am wondering whether these changes may break client code (see PrmInit.ResolveFrostFSFailures() in SDK), because we will get an error instead of status.

Where do we do wrapping to internal status (if we have any)? I am wondering whether these changes may break client code (see `PrmInit.ResolveFrostFSFailures()` in SDK), because we will get an error instead of status.
aarifullin commented 2024-12-16 12:02:27 +00:00
Outdated
Review
Copy link

We don't do that for all errors, but they are implicitly (from the point of this middleware) wrapped to internalServerError anyway. Aren't they?

func setStatusV2(resp ResponseMessage, err error) {
	// unwrap error
	for e := errors.Unwrap(err); e != nil; e = errors.Unwrap(err) {
		err = e
	}

	session.SetStatus(resp, apistatus.ToStatusV2(apistatus.ErrToStatus(err)))
}

func ToStatusV2(st Status) *status.Status {
	if v, ok := st.(StatusV2); ok {
		return v.ToStatusV2()
	}

	if IsSuccessful(st) {
		return newStatusV2WithLocalCode(status.OK, status.GlobalizeSuccess)
	}

	internalErrorStatus := newStatusV2WithLocalCode(status.Internal, status.GlobalizeCommonFail)
	internalErrorStatus.SetMessage(st.(error).Error()) // type cast never panics because IsSuccessful() checks cast

	return internalErrorStatus
}

Although that's not true for tree service

We don't do that for all errors, but they are implicitly (from the point of this middleware) wrapped to `internalServerError` anyway. Aren't they? ```go func setStatusV2(resp ResponseMessage, err error) { // unwrap error for e := errors.Unwrap(err); e != nil; e = errors.Unwrap(err) { err = e } session.SetStatus(resp, apistatus.ToStatusV2(apistatus.ErrToStatus(err))) } ``` ```go func ToStatusV2(st Status) *status.Status { if v, ok := st.(StatusV2); ok { return v.ToStatusV2() } if IsSuccessful(st) { return newStatusV2WithLocalCode(status.OK, status.GlobalizeSuccess) } internalErrorStatus := newStatusV2WithLocalCode(status.Internal, status.GlobalizeCommonFail) internalErrorStatus.SetMessage(st.(error).Error()) // type cast never panics because IsSuccessful() checks cast return internalErrorStatus } ``` Although that's not true for `tree` service
aarifullin commented 2024-12-16 12:18:10 +00:00
Outdated
Review
Copy link

For now non-chainRouterError errors are explicitly wrapped with ServerInternal status for the all goodness sake

For now non-`chainRouterError` errors are explicitly wrapped with ServerInternal status for the all goodness sake
apistatus.WriteInternalServerErr(errServerInternal, err)
return errServerInternal
}
errAccessDenied := &apistatus.ObjectAccessDenied{}
errAccessDenied.WriteReason("ape denied request: " + err.Error())
return errAccessDenied