Add support for TLS 1.3#564
Conversation
This PR allows TLS 1.3, by removing the MaxVersion in the client config. This would silently swallow errors, so e.g. a client without cert dialing a server that requires client certs would lead to an error which gets ignored, leading to retries until timeout. In this PR, we wrap the connection and if an error occurs we send it to the existing `result` channel. I think this matches @jhump's comment in fullstorydev#387 (comment) **Testing** ```console # Start the test server (in another tab) go run ./internal/testing/cmd/testserver \ -cert internal/testing/tls/server.crt \ -key internal/testing/tls/server.key \ -cacert internal/testing/tls/ca.crt \ -requirecert -p 9999 # Old behavior $ grpcurl -cacert internal/testing/tls/ca.crt \ localhost:9999 list Failed to dial target host "localhost:9999": context deadline exceeded # New behavior $ go run ./cmd/grpcurl -cacert internal/testing/tls/ca.crt \ localhost:9999 list Failed to dial target host "localhost:9999": remote error: tls: certificate required exit status 1 ``` The old behavior is to hang until we hit the deadline. The new behavior is to return immediately with an error. Fixes fullstorydev#563
| type errSignalingConn struct { | ||
| net.Conn | ||
| writeResult func(res interface{}) | ||
| once sync.Once |
There was a problem hiding this comment.
I don't think we need the sync.Once, writeResult() has to be idempotent. If we're worried about spamming that select statement too many times, we should add an atomic CompareAndSwap guard inside writeResult(), and then unconditionally push and close the channel
Removing unnecessary sync.Once, as the channel will already drop repeated calls.
|
Thanks for the review! I've implemented your suggestion. I'm also happy to apply the same connection-error handling logic to apply to plaintext connections, if you'd like (by getting rid of this if statement). If that sounds good, I can do it as a follow-up PR, as some tests need updating if I make that change. |
|
I think it's fine to just support TLS here |
|
But feel free to follow up if you can make better error reporting on plain text happen! |
Avoid hanging on connection errors; fail fast by propagating connection errors. This also speeds up tests a lot (from >20s to about 3s), since the tests included some connection timeouts. This expands on fullstorydev#564 ; it does the same for plaintext connections. Fixes fullstorydev#387
|
FYI: we've been getting test flakes recently-- It's either this PR, or #567 - unsure. Would you mind taking a look @bcleenders ? |
2 participants
This PR allows TLS 1.3, by removing the MaxVersion in the client config.
This would silently swallow errors, so e.g. a client without cert dialing a server that requires client certs would lead to an error which gets ignored, leading to retries until timeout.
In this PR, we wrap the connection and if an error occurs we send it to the existing
resultchannel.I think this matches @jhump's comment in #387 (comment) (but for this PR, I scoped the change to TLS only to avoid dropping a huge patch)
Testing
Start a test server that requires client certs
Old behavior
New behavior
The old behavior is to hang until we hit the deadline. The new behavior is to return immediately with an error.
Fixes #563