CLOUDP-330561: Moves unauth error check to common errors #4043
Conversation
| type AuthRequiredRoundTripper struct { | ||
| Base http.RoundTripper | ||
| } | ||
|
|
There was a problem hiding this comment.
transport wrapper no longer needed since its primary purpose, to check for unauth, is now done after the command executes
| @@ -49,6 +54,13 @@ func Check(err error) error { | |||
| return err | |||
| } | |||
|
|
|||
| func CheckHTTPErrors(err error) error { | |||
| if strings.Contains(err.Error(), "401") && strings.Contains(err.Error(), "Unauthorized") { | |||
There was a problem hiding this comment.
I don't think this is the proper way to check the error code coming from the API.
I think you'll have to use:
admin.AsError(err)I was first going to recommend using the errorcode: UNAUTHORIZED, but it looks like we have so many variations.
It's better to check the HTTP status code instead (through the error).
Unfortunately I don't fully remember how to get the status code from the SDK, let me know if you can't find it and want to look into it together
There was a problem hiding this comment.
Unfortunately I don't fully remember how to get the status code from the SDK, let me know if you can't find it and want to look into it together
AsError returns the ApiError struct which contains the error code as param
https://github.com/mongodb/atlas-sdk-go/blob/main/admin/model_api_error.go#L6
There was a problem hiding this comment.
Yes i tried this initially, but these unauth errors aren't GenericOpenAPIError types so admin.AsError(err) returns !ok.
There was a problem hiding this comment.
I think we need a custom error, perhaps should we introduce error handling on transport and create an ErrUnauthorized? at least a generic http error that has code as a property
| if errMsg != nil { | ||
| fmt.Println(errMsg) | ||
| } |
There was a problem hiding this comment.
shouldn't we print this to stderr?
There was a problem hiding this comment.
I think this prints it twice because cobra prints it
There was a problem hiding this comment.
If we are moving the error check here we should remove each individual usage of it and move it here
Proposed changes
Issues
Initial work to move unauth check from builder.go to transport.go (#4034) neglected digest and access token transport clients. This meant that if either of these transport methods received a 401 response, the error would not be wrapped with any additional messaging. For example, when a user's access token expires.
Solution
To fix this, I have moved the unauth check to atlas.go.
The flow now is:
Note that the raw transport error message will be printed to terminal before the command is fully complete. This means we cannot wrap the error but can only print additional messaging. In practice this looks much the same as the previous implementation:
Jira ticket: CLOUDP-330561
Checklist
make fmtand formatted my code