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
Return better errors for unavailable error codes #2336
Conversation
If we can determine that the error from login is due to a connect unavailable error, we should return more details to aid in troubleshooting. Update wrapError to return a better error message in case of a TLS certificate error (could be a MITM attack or an untrusted certificate).
When testing locally, the behavior is: Before:
After:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
if connectErr := new(connect.Error); errors.As(err, &connectErr) && connectErr.Code() == connect.CodeUnavailable { | ||
return connectErr | ||
} | ||
// We don't want to use the default error from wrapError here if the error | ||
// an unauthenticated error. | ||
return errors.New("invalid token provided") |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I thought you were going to invoke wrapError
here.
"strings" | ||
) | ||
|
||
// wrappedTLSError returns an unwrapped TLS error or nil if the error is another type of error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd like to see a bit more documentation here as to the motivation for this, so that in the future we understand why this is here in case we need to debug
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok - this is just a temporary thing until Go 1.21 is released and we can remove and just use errors.As which will be a lot more readable. I'll update to add more details though.
If we can determine that the error from login is due to a connect unavailable error, we should return more details to aid in troubleshooting. Update wrapError to return a better error message in case of a TLS certificate error (could be a MITM attack or an untrusted certificate).