mirror of
https://github.com/fullstorydev/grpcurl.git
synced 2026-07-25 15:06:05 +03:00
Fix flaky TLS test by waiting for dial outcome before closing connection (#572)
* Fix flaky TLS test by waiting for dial outcome before closing connection PRs #564 and #567 introduced errSignalingConn to surface TLS errors (especially TLS 1.3 post-handshake alerts) to BlockingDial callers. This caused a flaky test failure (reported in: https://github.com/fullstorydev/grpcurl/pull/564#issuecomment-4916344087): ``` TestBrokenTLS_RequireClientCertButNonePresented: expecting a TLS certificate error, got: read tcp ...: use of closed network connection ``` The race works as follows: 1. With TLS 1.3, the server rejects the connection *after* the handshake completes (e.g. missing client cert) by sending a TLS alert, then closing the connection. 2. Simultaneously, the client's Write fails (broken pipe) because the server already closed its side. 3. grpc-go's `NewHTTP2Client` returns the Write error and calls `t.Close()` to tear down the transport, which calls `errSignalingConn.Close()` -> `c.Conn.Close()`, closing the connection. 4. The reader goroutine's pending Read (which should have returned the TLS alert (e.g. "certificate required")) instead returns `use of closed network connection` because the connection was closed underneath it. 5. `writeResult` surfaces the "closed network connection" error to the caller instead of the meaningful TLS alert. The fix: delay Close on the errSignalingConn until the outcome of the dial is known, with a 50ms timeout. This gives the reader a chance to surface the TLS alert. Demonstrating this by stressing the test: **Without the fix** ``` alasia :: github/fullstorydev/grpcurl ‹master› » go test -c -o /tmp/grpcurl.test . && GOMAXPROCS=2 $HOME/go/bin/stress -p 16 -count 5000 -ignore 'assign requested address|context deadline exceeded' /tmp/grpcurl.test -test.run 'TestBrokenTLS' /var/folders/7l/0tdjxwg912j9gqh446yk3ll40000gn/T/go-stress-20260724T113423-1710148011 --- FAIL: TestBrokenTLS_RequireClientCertButNonePresented (0.00s) tls_settings_test.go:332: expecting a TLS certificate error, got: read tcp 127.0.0.1:52850->127.0.0.1:52848: use of closed network connection FAIL ERROR: exit status 1 [...] 5s: 1580 runs so far, 5 failures (0.32%), 13 active ``` **With the fix** ``` alasia :: github/fullstorydev/grpcurl ‹surface-post-dial-conn-errors› » go test -c -o /tmp/grpcurl.test . && GOMAXPROCS=2 $HOME/go/bin/stress -p 16 -count 5000 -ignore 'assign requested address|context deadline exceeded' /tmp/grpcurl.test -test.run 'TestBrokenTLS' 5s: 1594 runs so far, 0 failures, 16 active 10s: 3386 runs so far, 0 failures, 16 active 15s: 4999 runs so far, 0 failures, 1 active 15s: 5000 runs total, 0 failures ``` * Implement review comment
This commit is contained in:
+42
-5
@@ -20,6 +20,8 @@ import (
|
|||||||
"slices"
|
"slices"
|
||||||
"sort"
|
"sort"
|
||||||
"strings"
|
"strings"
|
||||||
|
"sync"
|
||||||
|
"time"
|
||||||
|
|
||||||
"github.com/golang/protobuf/proto" //lint:ignore SA1019 we have to import these because some of their types appear in exported API
|
"github.com/golang/protobuf/proto" //lint:ignore SA1019 we have to import these because some of their types appear in exported API
|
||||||
"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above
|
"github.com/jhump/protoreflect/desc" //lint:ignore SA1019 same as above
|
||||||
@@ -629,7 +631,16 @@ func BlockingDial(ctx context.Context, network, address string, creds credential
|
|||||||
// custom dialer that can provide that info. That means we manage the TLS handshake.
|
// custom dialer that can provide that info. That means we manage the TLS handshake.
|
||||||
result := make(chan interface{}, 1)
|
result := make(chan interface{}, 1)
|
||||||
|
|
||||||
|
// dialCompleted is closed once the outcome of the dial (ready connection
|
||||||
|
// or error) is known, so that connection teardown no longer needs to wait
|
||||||
|
// for a pending TLS alert to be read.
|
||||||
|
dialCompleted := make(chan struct{})
|
||||||
|
var dialCompletedOnce sync.Once
|
||||||
|
completeDial := func() { dialCompletedOnce.Do(func() { close(dialCompleted) }) }
|
||||||
|
defer completeDial()
|
||||||
|
|
||||||
writeResult := func(res interface{}) {
|
writeResult := func(res interface{}) {
|
||||||
|
completeDial()
|
||||||
// non-blocking write: we only need the first result
|
// non-blocking write: we only need the first result
|
||||||
select {
|
select {
|
||||||
case result <- res:
|
case result <- res:
|
||||||
@@ -642,6 +653,7 @@ func BlockingDial(ctx context.Context, network, address string, creds credential
|
|||||||
creds = &errSignalingCreds{
|
creds = &errSignalingCreds{
|
||||||
TransportCredentials: creds,
|
TransportCredentials: creds,
|
||||||
writeResult: writeResult,
|
writeResult: writeResult,
|
||||||
|
dialCompleted: dialCompleted,
|
||||||
}
|
}
|
||||||
|
|
||||||
switch network {
|
switch network {
|
||||||
@@ -724,7 +736,8 @@ func BlockingDial(ctx context.Context, network, address string, creds credential
|
|||||||
// it will use the writeResult function to notify on error.
|
// it will use the writeResult function to notify on error.
|
||||||
type errSignalingCreds struct {
|
type errSignalingCreds struct {
|
||||||
credentials.TransportCredentials
|
credentials.TransportCredentials
|
||||||
writeResult func(res interface{})
|
writeResult func(res interface{})
|
||||||
|
dialCompleted <-chan struct{}
|
||||||
}
|
}
|
||||||
|
|
||||||
func (c *errSignalingCreds) ClientHandshake(ctx context.Context, addr string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) {
|
func (c *errSignalingCreds) ClientHandshake(ctx context.Context, addr string, rawConn net.Conn) (net.Conn, credentials.AuthInfo, error) {
|
||||||
@@ -736,14 +749,23 @@ func (c *errSignalingCreds) ClientHandshake(ctx context.Context, addr string, ra
|
|||||||
// Wrap the connection to capture post-handshake errors, e.g.:
|
// Wrap the connection to capture post-handshake errors, e.g.:
|
||||||
// - TLS 1.3 client cert rejection (server sends alert after handshake)
|
// - TLS 1.3 client cert rejection (server sends alert after handshake)
|
||||||
// - Plaintext client to TLS server (server closes conn immediately)
|
// - Plaintext client to TLS server (server closes conn immediately)
|
||||||
return &errSignalingConn{Conn: conn, writeResult: c.writeResult}, auth, nil
|
return &errSignalingConn{Conn: conn, writeResult: c.writeResult, dialCompleted: c.dialCompleted}, auth, nil
|
||||||
}
|
}
|
||||||
|
|
||||||
// errSignalingConn wraps a net.Conn to capture the first read error and
|
// maxTLSAlertWait is an upper bound on how long a failing dial's connection
|
||||||
// report it via writeResult.
|
// teardown waits for a pending TLS alert to be read. It is a heuristic: long
|
||||||
|
// enough for the reader goroutine to be scheduled even on a loaded machine,
|
||||||
|
// short enough to not noticeably delay a failing dial.
|
||||||
|
const maxTLSAlertWait = 50 * time.Millisecond
|
||||||
|
|
||||||
|
// errSignalingConn wraps a net.Conn to capture read errors and report
|
||||||
|
// them via writeResult. Close is delayed until the dial has completed to
|
||||||
|
// give the reader a chance to read a pending TLS alert before the
|
||||||
|
// connection is closed.
|
||||||
type errSignalingConn struct {
|
type errSignalingConn struct {
|
||||||
net.Conn
|
net.Conn
|
||||||
writeResult func(res interface{})
|
writeResult func(res interface{})
|
||||||
|
dialCompleted <-chan struct{}
|
||||||
}
|
}
|
||||||
|
|
||||||
func (c *errSignalingConn) Read(b []byte) (int, error) {
|
func (c *errSignalingConn) Read(b []byte) (int, error) {
|
||||||
@@ -754,6 +776,21 @@ func (c *errSignalingConn) Read(b []byte) (int, error) {
|
|||||||
return n, err
|
return n, err
|
||||||
}
|
}
|
||||||
|
|
||||||
|
func (c *errSignalingConn) Close() error {
|
||||||
|
// With TLS 1.3, the server may reject the connection *after* the
|
||||||
|
// handshake completes (e.g. a missing client cert), by sending an alert.
|
||||||
|
// If the transport closes the connection before that alert is read, the
|
||||||
|
// caller sees a less useful error (e.g. "use of closed network
|
||||||
|
// connection") instead of the TLS alert. So while the outcome of the
|
||||||
|
// dial is still undecided, give the reader a brief window to read a
|
||||||
|
// pending alert before closing the connection.
|
||||||
|
select {
|
||||||
|
case <-c.dialCompleted:
|
||||||
|
case <-time.After(maxTLSAlertWait):
|
||||||
|
}
|
||||||
|
return c.Conn.Close()
|
||||||
|
}
|
||||||
|
|
||||||
// UsesXDS forwards the optional UsesXDS marker of the wrapped credentials. The
|
// UsesXDS forwards the optional UsesXDS marker of the wrapped credentials. The
|
||||||
// xDS credentials returned for "xds://" targets implement this method, and
|
// xDS credentials returned for "xds://" targets implement this method, and
|
||||||
// grpc-go's cds balancer relies on a type assertion for it to decide whether to
|
// grpc-go's cds balancer relies on a type assertion for it to decide whether to
|
||||||
|
|||||||
Reference in New Issue
Block a user