-
Notifications
You must be signed in to change notification settings - Fork 9.7k
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
*: simply ignore ErrAuthNotEnabled in clientv3 if auth is not enabled #7759
Conversation
if err == ctx.Err() && ctx.Err() != c.ctx.Err() { | ||
err = grpc.ErrClientConnTimeout | ||
|
||
err := c.getToken(ctx) |
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.
Could auth return a bogus token from server side instead of expecting the client to know if auth is enabled or not? When auth is turned on the server will reject it and the client will renegotiate. That'll avoid having this extra error handling path.
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.
On the other hand, the server probably shouldn't fake it. Never mind.
clientv3/client.go
Outdated
|
||
err := c.getToken(ctx) | ||
if err != nil { | ||
if err.Error() != rpctypes.ErrAuthNotEnabled.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.
toErr(ctx, err) != rpctypes.ErrAuthNotEnabled
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.
Thanks, I'll modify this line.
@heyitsanthony updated based on your comment, could you take a look? |
lgtm. One question: will the client retry and authenticate if auth is enabled some time after hitting this path? |
clientv3/client.go
Outdated
|
||
err := c.getToken(ctx) | ||
if err != nil { | ||
if toErr(ctx, err).Error() != rpctypes.ErrAuthNotEnabled.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.
wait, why does this still need Error()
? the pointer comparison should work
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.
ah sorry, they must be needless. I'll remove the both Error()
calls
Current implementation of clientv3 won't retry. Because it only retries when the error code is Possibly adding a new test case will be helpful. Should I add it in this PR? I think this is an independent topic so would like to do it in another PR if it is ok. |
@mitake OK, sure, file an issue for the disable/enable case after merging this one so it doesn't get lost in the shuffle? Also remove |
@heyitsanthony sure, I'll remove the unused func and open the issue. Thanks! |
Fix #7724
/cc @raoofm @heyitsanthony