clientv3:get AuthToken gracefully without extra connection. #12165
Conversation
|
/cc @mitake can you take a look? |
There was a problem hiding this comment.
Could you update the log of the 2nd commit to something like etcdserver: check authinfo if it is not InternalAuthenticateRequest for the style check?
The overall direction seems to be fine. I'll take a look at failed tests sometime this week. It's great if you can also take a look.
| r.Header.Username = authInfo.Username | ||
| r.Header.AuthRevision = authInfo.Revision | ||
| // check authinfo if it is not InternalAuthenticateRequest | ||
| if r.Authenticate == nil { |
There was a problem hiding this comment.
This is a nice idea, thanks!
|
tests failed in https://travis-ci.com/github/etcd-io/etcd/jobs/364568672#L2153 I don't understand this logs . @mitake Can you tell me the reason? and I'll continue to debug it. |
I saw the same error in https://travis-ci.com/github/etcd-io/etcd/jobs/366031014#L2114 , is it means that i can ignore this error? |
|
@cfc4n the error message means the test introduced leaked goroutines even after cleaning up resources. I tried to reproduce the error on my local machine but couldn't: I guess it would be non deterministic failure, but am not fully sure yet. Also travis shows some failed test cases: I'll look at the failed test cases, but if you can help it's great. |
| resp, err := c.Auth.Authenticate(ctx, c.Username, c.Password) | ||
| if err != nil { | ||
| if err == rpctypes.ErrAuthNotEnabled { | ||
| return nil |
There was a problem hiding this comment.
Do we have a reason to hide this error code?
There was a problem hiding this comment.
That means etcd cluster do not enable auth. so return nil .
ref #10428
There was a problem hiding this comment.
If we return nil, clients continue retrying forever like this when they issue Authenticate() request:
~/g/s/g/etcd [get_authtoken_gracefully]× Â» bin/etcdctl get k1 --user u1:p -130- 18:08:51
{"level":"warn","ts":"2020-08-15T18:08:51.760+0900","caller":"clientv3/retry_interceptor.go:62","msg":"retrying of unary invoker failed","target":"endpoint://client-3a425fc6-a534-496d-872b-9c074bc66927/127.0.0.1:2379","attempt":0,"error":"rpc error: code = FailedPrecondition desc = etcdserver: authentication is not enabled"}
{"level":"warn","ts":"2020-08-15T18:08:51.762+0900","caller":"clientv3/retry_interceptor.go:62","msg":"retrying of unary invoker failed","target":"endpoint://client-3a425fc6-a534-496d-872b-9c074bc66927/127.0.0.1:2379","attempt":0,"error":"rpc error: code = Unauthenticated desc = etcdserver: invalid auth token"}
{"level":"warn","ts":"2020-08-15T18:08:51.764+0900","caller":"clientv3/retry_interceptor.go:62","msg":"retrying of unary invoker failed","target":"endpoint://client-3a425fc6-a534-496d-872b-9c074bc66927/127.0.0.1:2379","attempt":0,"error":"rpc error: code = FailedPrecondition desc = etcdserver: authentication is not enabled"}
...
The behavior is different from the original one. I think it would be related to the failed test cases, let me check.
There was a problem hiding this comment.
It wasn't related to the failed test cases. I'm checking its effect.
Test passed in my cloud server with 8 Core\32G memory ,and CPU |
I have a machine with similar resources but can reproduce the failed test cases. I couldn't allocate a time for this last week. Hopefully I can work on the issue this week. Sorry for keeping you waiting. |
|
@cfc4n The direct cause of the failed test cases is that this PR changes behavior of etcd server with auth disabled. Typical failed request/response sequences would be like below:
The older version (current master branch) doesn't use interceptor for I think this commit fixes the problem, could you pick the commit in your PR? mitake@747cc70 |
|
On my local machine |
OK,I'll test this pr for all TESTS later,thank you. |
|
@mitake thanks, I reproduced it, I will try to fix it. |
|
Codecov ReportAttention: Patch coverage is
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## master #12165 +/- ##
==========================================
- Coverage 64.00% 64.00% -0.01%
==========================================
Files 403 403
Lines 37427 37516 +89
==========================================
+ Hits 23957 24012 +55
- Misses 11955 11979 +24
- Partials 1515 1525 +10 ☔ View full report in Codecov by Sentry. |
|
@gyuho Please review this PR,and merge it if passed. |
|
I restarted the build to check the goroutine leak happens again or not. |
|
It seems that the goroutine leak is a deterministic problem, let me diagnose (it might need some time though...). |
|
At least |
|
@cfc4n I think I found the cause of the goroutine leak issue. Could you pick this commit into your branch? mitake@9f1dfa2 We need to close client object when getToken() fails. On my local env the goroutine issue is resolved and integration pass can finish successfully. |
|
lgtm |
Old etcdserver which have not apply pr of etcd-io#12165 will check auth token even if the request is a Authenticate request. If the client has a invalid auth token, it will not able to update it's token, since the Authenticate has a invalid auth token. This fix clear the auth token when encounter an ErrInvalidAuthToken to talk with old version etcd servers. Fix etcd-io#12385 with etcd-io#12165 and etcd-io#12264
Old etcdserver which have not apply pr of etcd-io#12165 will check auth token even if the request is a Authenticate request. If the client has a invalid auth token, it will not able to update it's token, since the Authenticate has a invalid auth token. This fix clear the auth token when encounter an ErrInvalidAuthToken to talk with old version etcd servers. Fix etcd-io#12385 with etcd-io#12165 and etcd-io#12264
Old etcdserver which have not apply pr of etcd-io#12165 will check auth token even if the request is an Authenticate request. If the client has a invalid auth token, it will not able to update it's token, since the Authenticate has a invalid auth token. This fix clear the auth token when encounter an ErrInvalidAuthToken to talk with old version etcd servers. Fix etcd-io#12385 with etcd-io#12165 and etcd-io#12264
Old etcdserver which have not apply pr of etcd-io#12165 will check auth token even if the request is an Authenticate request. If the client has a invalid auth token, it will not able to update it's token, since the Authenticate has a invalid auth token. This fix clear the auth token when encounter an ErrInvalidAuthToken to talk with old version etcd servers. Fix etcd-io#12385 with etcd-io#12165 and etcd-io#12264
Old etcdserver which have not apply pr of etcd-io#12165 will check auth token even if the request is an Authenticate request. If the client has a invalid auth token, it will not able to update it's token, since the Authenticate has a invalid auth token. This fix clear the auth token when encounter an ErrInvalidAuthToken to talk with old version etcd servers. Fix etcd-io#12385 with etcd-io#12165 and etcd-io#12264
clientv3: get AuthToken gracefully without extra connection.
ignore AuthToken check when InternalRequest was
AuthenticateRequestalsoetcdserverpb.Auth/Authenticate.