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

aarifullin · 2024-12-16T11:25:36Z

aarifullin commented

2024-12-16 11:25:36 +00:00

Introduce ChainRouterError in common/ape package
Refactor tree service. Make tree service wrap with ObjectAccessDenied only ChainRouterError
Refactor object service. Make object service wrap with ObjectAccessDenied only ChainRouterError

Since errors can be differentiated logical check errors and server internal errors.

* Introduce `ChainRouterError` in `common/ape` package * Refactor `tree` service. Make `tree` service wrap with `ObjectAccessDenied` only `ChainRouterError` * Refactor `object` service. Make `object` service wrap with `ObjectAccessDenied` only `ChainRouterError` Since errors can be differentiated logical check errors and server internal errors.

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

2024-12-16 11:25:37 +00:00

aarifullin force-pushed fix/ape_logicalerr from 4ead7084a9 to cb63ea028c

2024-12-16 11:26:34 +00:00

Compare

fyrchik reviewed 2024-12-16 11:34:11 +00:00

pkg/services/common/ape/error.go Outdated

					
				@ -0,0 +14,4 @@

				}

				func (e *ChainRouterError) Error() string {

					return fmt.Sprintf("access to operation %s is denied by access policy engine: %s", e.Operation(), e.Status().String())

fyrchik commented

2024-12-16 11:31:32 +00:00

.String() can be omitted for %s.

`.String()` can be omitted for `%s`.

👍 1

aarifullin commented

2024-12-16 12:18:16 +00:00

Fixed

fyrchik marked this conversation as resolved

pkg/services/common/ape/error.go Outdated

					
				@ -0,0 +25,4 @@

					return e.status

				}

				func NewChainRouterError(operation string, status apechain.Status) *ChainRouterError {

fyrchik commented

2024-12-16 11:31:22 +00:00

Do we need to export this?

👍 1

aarifullin commented

2024-12-16 12:18:19 +00:00

Fixed

fyrchik marked this conversation as resolved

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

					
				@ -8,0 +10,4 @@

				func processAPECheckErr(err error) error {

					var chRouterErr *checkercore.ChainRouterError

					if !errors.As(err, &chRouterErr) {

						return err

fyrchik commented

2024-12-16 11:33:59 +00:00

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

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

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

fyrchik marked this conversation as resolved

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

					
				@ -92,3 +92,3 @@

						cnrID, objID, err := getAddressParamsSDK(partInit.GetHeader().GetContainerID(), partInit.GetObjectID())

						if err != nil {

							return toStatusErr(err)

							return processAPECheckErr(err)

fyrchik commented

2024-12-16 11:33:35 +00:00

Why all the renaming?

aarifullin commented

2024-12-16 12:04:39 +00:00

I implied as we don't wrap all errors to status but this can be made after some processing. For now I think we need to wrap errors explicitly and the name is going to be returned

aarifullin commented

2024-12-16 12:17:02 +00:00

Naming have been returned back

fyrchik marked this conversation as resolved

fyrchik approved these changes 2024-12-16 11:34:25 +00:00

Dismissed

aarifullin force-pushed fix/ape_logicalerr from cb63ea028c to 6e82661c35

2024-12-16 12:16:51 +00:00

Compare

aarifullin dismissed fyrchik's review 2024-12-16 12:16:51 +00:00

Reason:

New commits pushed, approval review dismissed automatically according to repository settings