-
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: fix secure endpoint failover, refactor with gRPC 1.22 upgrade #10911
Conversation
This is the initial proposal. I will improve the logging and docs around this change. @hexfusion @jingyih @wenjiaswe @spzala Can you take a look as well? This should be the last major change before 3.4 code freeze. |
excellent. thanks @gyuho |
874f1bf
to
010e6d7
Compare
Thanks for implementing the fix. Do we have a test to verify the original bug was fixed? |
See
We can only test it manually, because the fix is made in our TLS SAN field authentication :) I manually confirmed that this fixes the original bug. |
2e3d59b
to
1b4a401
Compare
clientv3/credentials/credentials.go
Outdated
return nil, nil | ||
} | ||
|
||
// transportCredential implements "grpccredentials.PerRPCCredentials" interface. |
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.
nit: "grpccredentials.TransportCredentials"
clientv3/credentials/credentials.go
Outdated
@@ -71,6 +71,19 @@ func newTransportCredential(cfg *tls.Config) *transportCredential { | |||
} | |||
|
|||
func (tc *transportCredential) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (net.Conn, grpccredentials.AuthInfo, error) { | |||
target := rawConn.RemoteAddr().String() | |||
if authority != target { |
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.
do we need to check authority is an IP address rather than a DNS name?
the core idea looks good to me. |
@xiang90 Good point. I made a change to overwrite authority only when the target is IP. Manually tested with SRV records: diff --git a/tests/docker-dns-srv/certs/server-ca-csr.json b/tests/docker-dns-srv/certs/server-ca-csr.json
index 661de3799..f3a23c656 100644
--- a/tests/docker-dns-srv/certs/server-ca-csr.json
+++ b/tests/docker-dns-srv/certs/server-ca-csr.json
@@ -19,8 +19,6 @@
"m4.etcd.local",
"m5.etcd.local",
"m6.etcd.local",
- "etcd.local",
- "127.0.0.1",
- "localhost"
+ "etcd.local"
]
} Before (if we overwrite authority)
After (if we overwrite only when it's an IP) No error |
8c64b9f
to
3b39c57
Compare
Codecov Report
@@ Coverage Diff @@
## master #10911 +/- ##
==========================================
- Coverage 63.9% 63.66% -0.24%
==========================================
Files 400 400
Lines 37688 37415 -273
==========================================
- Hits 24085 23821 -264
- Misses 11966 11976 +10
+ Partials 1637 1618 -19
Continue to review full report at Codecov.
|
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.
Did a first pass review. I'll do a second pass tomorrow to make sure I fully understand some of the details.
clientv3/balancer/balancer.go
Outdated
oldAggrState := bb.currentState | ||
bb.currentState = bb.csEvltr.recordTransition(old, s) | ||
oldAggrState := bb.connectivityRecorder.GetCurrentState() | ||
bb.connectivityRecorder.RecordTransition(old, s) | ||
|
||
// Regenerate picker when one of the following happens: |
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.
Update comment to reflect that we're calling updatedPicker
now instead regeneratePicker
?
rc.mu.RLock() | ||
state = rc.cur | ||
rc.mu.RUnlock() | ||
return state |
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.
nit: Can we just do:
rc.mu.RLock()
defer rc.mu.RUnlock()
return rc.cur
?
rc.numConnecting += updateVal | ||
case connectivity.TransientFailure: | ||
rc.numTransientFailure += updateVal | ||
} |
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.
nit: Log a warn or something if none of the cases match?
clientv3/balancer/picker/picker.go
Outdated
// Error is error picker policy. | ||
Error Policy = iota | ||
|
||
// RoundrobinBalanced balance loads over multiple endpoints |
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.
balances
// Picker implements gRPC picker. | ||
// Leave empty if "Policy" field is not custom. | ||
// TODO: currently custom policy is not supported. | ||
// Picker picker.Picker |
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 comments about picker configuration and Custom
picker enum in this PR and instead open an issue about it? It's a adds quite a bit of noise in this PR, might be better to just leave it out of the code until we decide to implement something. Same for custom balancers.
clientv3/credentials/credentials.go
Outdated
} | ||
|
||
func (tc *transportCredential) Clone() grpccredentials.TransportCredentials { | ||
return tc.gtc.Clone() |
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.
return &transportCredential{
gtc: tc.gtc.Clone()
}
? (I realize we're not using it for anything, but just to future proof..)
clientv3/credentials/credentials.go
Outdated
// perRPCCredential implements "grpccredentials.PerRPCCredentials" interface. | ||
type perRPCCredential struct { | ||
authToken string | ||
authTokenMu *sync.RWMutex |
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.
pointer needed? just do authTokenMu sync.RWMutex
to eliminate authTokenMu: new(sync.RWMutex)
below?
} | ||
} | ||
|
||
func (tc *transportCredential) ClientHandshake(ctx context.Context, authority string, rawConn net.Conn) (net.Conn, grpccredentials.AuthInfo, 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.
This looks right to me.
@wenjiaswe Would you double check this function as well? Docs for ClientHandshake here: https://godoc.org/google.golang.org/grpc/credentials#TransportCredentials
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 checked the function, lgtm, thanks!
The original issue from Kubernetes repo was reported with etcd version 3.2.24. etcd 3.2 and 3.3 do not use the grpc load balancer, but still have the same issue which needs to be fixed (primarily by 13e26a4.) I think we should consider backporting (part of) this. |
@jingyih correct. and as i said before in #10911 (comment) we tried patching up the |
@dims @jpbetz We did something similar for etcd 3.2 (ref. kubernetes/kubernetes#57480) to backport client balancer fixes from 3.3 to 3.2. We can look into backporting this after 3.4 release. |
@gyuho massive thanks for fixing this. |
@frittentheke the fix is in the etcd client code that is vendored into k/k and hence part of api server. So just updating the etcd "server" will not help in any way to mitigate the problem |
@dims urgh, yeah, my bad. But since this solely is a client issue, there is no need for any backporting required, is there? |
@frittentheke "etcd client lib" with the fix in this PR, note that this drags in new grpc as well. Note that this combination has not landed in k/k master, so there is no testing at all at this point :) Also see my note in the k/k issue - kubernetes/kubernetes#72102 (comment) |
We are experiencing the same issue as API Server is crashing out every time the first etcd server goes down. Earlier using etcd v3.3.13 and upgraded this to v3.3.15 after coming across this PR and kubernetes/kubernetes#72102. But I still see that apiserver is crashing out.After spending some time to make sure I'm using the proper versions, decided to take help from other folks here. I believe I might be missing something. Please correct me if so. Thanks! Below are the details:
As you notice here, I expected 'OK' response in the 3rd step as well. Do I need re-generate the certificates in any specific way? the same certs works fine when the first server is up. Please point me in right direction. |
I use a domain name to issue a certificate,the error still exists! {"level":"warn","ts":"2019-09-17T19:23:17.183+0800","caller":"clientv3/retry_interceptor.go:61","msg":"retrying of unary invoker failed","target":"endpoint://client-8728de64-f8ea-4354-8151-4365e42b3acd/e2341-1.etcd.dpool.com.cn:2341","attempt":0,"error":"rpc error: code = DeadlineExceeded desc = latest connection error: connection error: desc = "transport: authentication handshake failed: x509: certificate is valid for e2341-4.etcd.dpool.com.cn, not e2341-1.etcd.dpool.com.cn""} |
Contains an important fix in clientv3 that allows vault to successfully failover to another etcdv3 endpoint in the event that the current active connection becomes unavailable. See also: * etcd-io/etcd#9949 * etcd-io/etcd#10911 * https://github.com/etcd-io/etcd/blob/master/CHANGELOG-3.3.md#v3314-2019-08-16 Fixes hashicorp#4961
Specifically, we are looking for this fix etcd-io/etcd#10911 Which, was included in v3.3.14 according to the changelog: https://github.com/etcd-io/etcd/blob/master/CHANGELOG-3.3.md#v3314-2019-08-16
Fix kubernetes/kubernetes#72102.
Manually tested, and works:
Server 1 certificate:
Server 2 certificate:
Server 3 certificate:
192.168.223.156
,192.168.121.180
,192.168.150.136
192.168.223.156
--endpoints 192.168.223.156:2379,192.168.121.180:2379,192.168.150.136:2379
192.168.223.156
but client balancer should fali-over to other endpointsWithout this patch, it fails:
Action Items
authTokenCredential
with custom credential bundle/cc @jpbetz @xiang90