-
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
clientv3: refresh authToken when permission deny #12135
Conversation
|
341272f
to
e1664e1
Compare
e1664e1
to
7ae4bf9
Compare
return | ||
} | ||
s.client.lg.Info("refresh auth token and resend watch request") | ||
if err = s.client.getToken(s.ctx); err != nil { |
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.
etcd supports multiple authentication methods. If users use other authentication methods, such as certificates. have you tested this scenario(cert has no permission)?
It seems that we need to distinguish token authentication. for example, in unaryClientInterceptor.
if callOpts.retryAuth && rpctypes.Error(lastErr) == rpctypes.ErrInvalidAuthToken {
gterr := c.getToken(ctx)
if gterr != nil {
logger.Warn(
"retrying of unary invoker failed to fetch new auth token",
zap.String("target", cc.Target()),
zap.Error(gterr),
)
return gterr // lastErr must be invalid auth token
}
continue
}
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.
In the case of CN based auth, users won't supply username and password, then getToken()
will be nop so it will be fine. But caring about different auth mechanisms are good point.
@jschwinger23 could you add e2e test cases for making sure this refreshing mechanism works and it won't break other auth mechanisms? If you are busy adding in other PRs is fine.
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.
It seems that it cannot distinguish different auth mechanisms here. I agree with @xkeyideal solution in issue #12157.
/cc @mitake can you take a look. thanks. |
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.
callOpts: callOpts, | ||
ctx: ctx, | ||
streamerCall: func(ctx context.Context) (grpc.ClientStream, 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.
remove empty line here.
fixed by #12264, close |
clientv3: bring watch interceptor to re-fetch auth token
watch interceptor will work only when ClientStream.RecvMsg fulfill 2 requirements:
on other situations the interceptor is transparent and insignificant.
fix:
#11954
#11121
#11381