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

rgw/admin: Add Error parser to extract RGW API error models #1065

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

raman-vhd
Copy link

@raman-vhd raman-vhd commented Jan 29, 2025

Add ParseError function to extract RGW API error models.
Essential for handling error reasons in a project.

Checklist

  • Added tests for features and functional changes
  • Public functions and types are documented
  • Standard formatting is applied to Go code
  • Is this a new API? Added a new file that begins with //go:build ceph_preview
  • Ran make api-update to record new APIs

Comment on lines +136 to +146
func ParseError(err error) (statusError, bool) {
statusErr := statusError{}
if err == nil {
return statusErr, true
}
ok := errors.As(err, &statusErr)
if !ok {
return statusErr, false
}
return statusErr, true
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this already possible with the usual standard error returned for different APIs? See few examples from our current tests within rgw/admin package.

        suite.T().Run("info non-existing bucket", func(_ *testing.T) {
                _, err := co.GetBucketInfo(context.Background(), Bucket{Bucket: "foo"})
                assert.Error(suite.T(), err)
                assert.True(suite.T(), errors.Is(err, ErrNoSuchBucket), err)
        })
        suite.T().Run("get policy non-existing bucket", func(_ *testing.T) {
                _, err := co.GetBucketPolicy(context.Background(), Bucket{Bucket: "foo"})
                assert.Error(suite.T(), err)
                assert.True(suite.T(), errors.Is(err, ErrNoSuchKey), err)
        })

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

// call makes request to the RGW Admin Ops API
func (api *API) call(ctx context.Context, httpMethod, path string, args url.Values) (body []byte, err error) {
...
// Handle error in response
if resp.StatusCode >= 300 {
	return nil, handleStatusError(decodedResponse)
}

return decodedResponse, nil
}

API calls are calling call method in radosgw.go & it's handling errors by handleStatusError. Which returns statusError as error response, not errorReason. I need to parse statusErrors error types.

I wonder how the test cases work. Am I wrong?

Copy link
Collaborator

@anoopcs9 anoopcs9 Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

API calls are calling call method in radosgw.go & it's handling errors by handleStatusError. Which returns statusError as error response, not errorReason. I need to parse statusErrors error types.

I wonder how the test cases work. Am I wrong?

If I get it correctly that's why we have the following Is method defined to compare based on type errorReason as per the docs.

func (e statusError) Is(target error) bool { return target == errorReason(e.Code) }

This enables us to use errors.Is() to check against pre-defined errorReason. I hope your requirement is mostly covered.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see, you're correct. But accessing statusError fields are helpful. Here I preferred having parser instead of exporting statusError.

For my scenario, I'm grouping rgw errors by error reason. Error() is concating errorReason, HostID & RequestID, making it impossible to group & manage them.

Copy link
Collaborator

@phlogistonjohn phlogistonjohn Feb 7, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Regardless of how this ends up, you should not be returning a private type from a public function.

My suggestion is to take a page from how the errors are handled elsewhere in go-ceph. For example:

go-ceph/docs/hints.md

Lines 213 to 253 in 2e15e72

In other cases the returned error doesn't match a specific error value but
rather is implemented by a type that may carry additional data. Specifically,
many errors in go-ceph implement an `ErrorCode() int` method. If this is the
case you can use ErrorCode to access a numeric error code provided by calls to
Ceph. Note that the error codes returned by Ceph often match unix/linux
`errno`s - but the exact meaning of the values returned by `ErrorCode()` are
determined by the Ceph APIs and go-ceph is just making them accessible.
Example:
```go
type errorWithCode interface {
ErrorCode() int
}
err := rados.SomeRadosFunc()
if err != nil {
var ec errorWithCode
if errors.As(err, &ec) {
errCode := ec.ErrorCode()
// ... do something with errCode ...
} else {
// ... handle generic error ...
}
}
```
Note that Go allows type definitions inline so you can even write:
```go
err := rados.SomeRadosFunc()
if err != nil {
var ec interface { ErrorCode() int }
if errors.As(err, &ec) {
errCode := ec.ErrorCode()
// ... do something with errCode ...
} else {
// ... handle generic error ...
}
}
```

Consider adding methods like ErrorCode, ErrorRequestID and so forth to the statusError type and then in your own code you can define an interface that has equivalent ErrorCode, ErrorRequestID and so on methods, and then errors.As into that type yourself. And of course, document this :-)

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding to what John mentioned, you can find examples for the most common ErrorCode() function under internal/errutil, rados/striper packages etc. and interface definitions from test files like cephfs/errors_test.go, cephfs/admin/pin_test.go etc. In the current context the only difference would be the return type which is string and not int as we see elsewhere in the code base.

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

Successfully merging this pull request may close these issues.

3 participants