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.

Sign up for GitHub

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
Open

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

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
732305f
rgw/admin: Add ParseError function to extract RGW API error models
raman-vhd Jan 29, 2025
d0bb699
rgw/admin: Add statusError field comment for better clarity
raman-vhd Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 17 additions & 0 deletions rgw/admin/errors.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -127,3 +127,20 @@ func (e statusError) Is(target error) bool { return target == errorReason(e.Code

// Error returns non-empty string if there was an error.
func (e statusError) Error() string { return fmt.Sprintf("%s %s %s", e.Code, e.RequestID, e.HostID) }

// ParseError parses an error returned by the RGW API and attempts to
// extract error model. It returns the extracted error model and a boolean
// indicating whether the error was successfully parsed.
//
// Does not handle internal client errors such as "missing user ID"
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
}
Comment on lines +136 to +146
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 •
edited
Loading

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 •
edited
Loading

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.

61 changes: 61 additions & 0 deletions rgw/admin/errors_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@ package admin

import (
"errors"
"fmt"
"testing"

"github.com/stretchr/testify/assert"
Expand All @@ -21,3 +22,63 @@ func TestHandleStatusError(t *testing.T) {
assert.Error(t, err)
assert.True(t, errors.Is(err, ErrNoSuchSubUser), err)
}
func TestParseError(t *testing.T) {
var (
nilError error = nil
nilErrorExpected = statusError{}

nonValidStatusError = errors.New("missing user ID")
nonValidStatusErrorExpected = statusError{}

getUserError error = statusError{
Code: "NoSuchUser",
RequestID: "tx0000000000000000005a9-00608957a2-10496-my-store",
HostID: "10496-my-store-my-store",
}
getUserErrorExpected = statusError{
Code: "NoSuchUser",
RequestID: "tx0000000000000000005a9-00608957a2-10496-my-store",
HostID: "10496-my-store-my-store",
}

getSubUserError error = statusError{
Code: "NoSuchSubUser",
RequestID: "tx0000000000000000005a9-00608957a2-10496-my-store",
HostID: "10496-my-store-my-store",
}
getSubUserErrorExpected = statusError{
Code: "NoSuchSubUser",
RequestID: "tx0000000000000000005a9-00608957a2-10496-my-store",
HostID: "10496-my-store-my-store",
}
)

// Nil error
statusError, ok := ParseError(nilError)
assert.True(t, ok)
assert.Equal(t, nilErrorExpected, statusError)

// Non-valid error
statusError, ok = ParseError(nonValidStatusError)
assert.False(t, ok)
assert.Equal(t, nonValidStatusErrorExpected, statusError)

// Get user error
statusError, ok = ParseError(getUserError)
assert.True(t, ok)
assert.EqualExportedValues(t, getUserErrorExpected, statusError)
assert.True(t, statusError.Is(ErrNoSuchUser))

// Get sub-user error
statusError, ok = ParseError(getSubUserError)
assert.True(t, ok)
assert.EqualExportedValues(t, getSubUserErrorExpected, statusError)
assert.True(t, statusError.Is(ErrNoSuchSubUser))

// Wrapped error
wrappedError := fmt.Errorf("additional note: %w", getUserError)
statusError, ok = ParseError(wrappedError)
assert.True(t, ok)
assert.EqualExportedValues(t, getUserErrorExpected, statusError)
assert.True(t, statusError.Is(ErrNoSuchUser))
}