-
Notifications
You must be signed in to change notification settings - Fork 9.8k
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
*: implement a retry logic for auth old revision in the client #13308
Conversation
client/v3/retry_interceptor.go
Outdated
@@ -157,7 +157,8 @@ func (c *Client) shouldRefreshToken(err error, callOpts *options) bool { | |||
// which is possible when the client token is cleared somehow | |||
return c.authTokenBundle != nil // equal to c.Username != "" && c.Password != "" | |||
} | |||
return callOpts.retryAuth && rpctypes.Error(err) == rpctypes.ErrInvalidAuthToken | |||
|
|||
return callOpts.retryAuth && (errors.Is(err, rpctypes.ErrInvalidAuthToken) || errors.Is(err, rpctypes.ErrGRPCAuthOldRevision)) |
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.
@r-ashish could you check this path? I think we should use errors.Is()
instead of ==
here.
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.
also line 155 would need change too. I'll add a unit test to the file for ensuring that the error handling logic is correct
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.
yeah agree, looks okay.
@mitake Seems to recover nicely! Thoughts on when this could get pushed out? |
Actually, one thing I did find is that tokens are not renewed at expire time:
|
@davissp14 thanks for checking! I’ll fix the renew issue. Probably I can finalize this PR this weekend (or sometime next week), is this ok for you? |
@davissp14 the issue should be fixed now, could you try again? |
I think the change is ready to be reviewed, could you take a look @spzala @serathius ? |
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.
@mitake lgtm overall. Great job adding test :) Thanks!
@spzala thanks! I'm merging this PR then. |
@davissp14 did this MR solve the issue on your env? I'll also make PRs for backporting this PR (and related ones) to stable releases. |
@mitake Was this change included in 3.5.1? |
@davissp14 if you would like to raise the PR to backport to |
fix #13300
@davissp14 could you try this one? I tried the change with https://github.com/mitake/etcd-things/tree/master/auth-failover and it worked