From 087a59fa14ff3dc7cc9aff86c0aa693eb761c570 Mon Sep 17 00:00:00 2001 From: Eno Compton Date: Mon, 10 Jan 2022 15:11:46 -0700 Subject: [PATCH 1/6] fix: invalidated config should retain error When a TLS handshake fails, the client would invalidate the configuration without recording the associated error. When the liveness check runs, it would panic when trying to print the invalidated configuration's error because the error was nil. This commit ensures that when the proxy invalidates a configuration, it includes the error. Fixes #1065. --- proxy/proxy/client.go | 3 ++- proxy/proxy/client_test.go | 22 ++++++++++++++++++++++ proxy/proxy/connect_tls_117.go | 2 +- proxy/proxy/connect_tls_other.go | 2 +- 4 files changed, 26 insertions(+), 3 deletions(-) diff --git a/proxy/proxy/client.go b/proxy/proxy/client.go index 84c992b2e..e5565e7f3 100644 --- a/proxy/proxy/client.go +++ b/proxy/proxy/client.go @@ -526,7 +526,7 @@ func (c *Client) selectDialer() func(context.Context, string, string) (net.Conn, } } -func (c *Client) invalidateCfg(cfg *tls.Config, instance string) { +func (c *Client) invalidateCfg(cfg *tls.Config, instance string, err error) { c.cacheL.RLock() e := c.cfgCache[instance] c.cacheL.RUnlock() @@ -541,6 +541,7 @@ func (c *Client) invalidateCfg(cfg *tls.Config, instance string) { return } c.cfgCache[instance] = cacheEntry{ + err: err, done: e.done, lastRefreshed: e.lastRefreshed, } diff --git a/proxy/proxy/client_test.go b/proxy/proxy/client_test.go index 0d380d60c..a5b3bd515 100644 --- a/proxy/proxy/client_test.go +++ b/proxy/proxy/client_test.go @@ -513,7 +513,29 @@ func TestClientHandshakeCanceled(t *testing.T) { _, err := c.DialContext(ctx, instance) validateError(t, err) }) + }) + + t.Run("when liveness check is called on invalidated config", func(t *testing.T) { + withTestHarness(t, func(port int) { + c := newClient(port) + + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + _, err := c.DialContext(ctx, instance) + if err == nil { + t.Fatal("expected DialContext to fail, got no error") + } + + invalid := c.InvalidInstances() + if gotLen := len(invalid); gotLen != 1 { + t.Fatalf("invalid instance want = 1, got = %v", gotLen) + } + i := invalid[0] + if want := "deadline exceeded"; !strings.Contains(i.err.Error(), want) { + t.Fatalf("invalid instance error want = %v, got = %v", want, i.Error()) + } + }) }) // Makes it to Handshake. diff --git a/proxy/proxy/connect_tls_117.go b/proxy/proxy/connect_tls_117.go index fcba5906f..a1d580217 100644 --- a/proxy/proxy/connect_tls_117.go +++ b/proxy/proxy/connect_tls_117.go @@ -38,7 +38,7 @@ func (c *Client) connectTLS( // this file is conditionally compiled on only Go versions >= 1.17. if err := ret.HandshakeContext(ctx); err != nil { _ = ret.Close() - c.invalidateCfg(cfg, instance) + c.invalidateCfg(cfg, instance, err) return nil, err } return ret, nil diff --git a/proxy/proxy/connect_tls_other.go b/proxy/proxy/connect_tls_other.go index 94c5cb3f5..574bcd564 100644 --- a/proxy/proxy/connect_tls_other.go +++ b/proxy/proxy/connect_tls_other.go @@ -106,7 +106,7 @@ func (c *Client) connectTLS( ret := tls.Client(conn, cfg) if err := ret.Handshake(); err != nil { _ = ret.Close() - c.invalidateCfg(cfg, instance) + c.invalidateCfg(cfg, instance, err) return nil, err } return ret, nil From 7db7142059f12cd29ab48572212b268d4c209277 Mon Sep 17 00:00:00 2001 From: Eno Compton Date: Thu, 13 Jan 2022 10:15:15 -0700 Subject: [PATCH 2/6] Assert error is not nil --- proxy/proxy/client_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/proxy/proxy/client_test.go b/proxy/proxy/client_test.go index a5b3bd515..b1fe7f972 100644 --- a/proxy/proxy/client_test.go +++ b/proxy/proxy/client_test.go @@ -531,9 +531,9 @@ func TestClientHandshakeCanceled(t *testing.T) { if gotLen := len(invalid); gotLen != 1 { t.Fatalf("invalid instance want = 1, got = %v", gotLen) } - i := invalid[0] - if want := "deadline exceeded"; !strings.Contains(i.err.Error(), want) { - t.Fatalf("invalid instance error want = %v, got = %v", want, i.Error()) + got := invalid[0] + if got.err == nil { + t.Fatal("want invalid instance error, got nil") } }) }) From a08ccb5fc5dd1b675f5db94850ebfe12380ff99a Mon Sep 17 00:00:00 2001 From: Eno Compton Date: Tue, 18 Jan 2022 15:47:53 -0700 Subject: [PATCH 3/6] Wrap handshake error with additional context --- proxy/proxy/connect_tls_117.go | 2 ++ proxy/proxy/connect_tls_other.go | 1 + 2 files changed, 3 insertions(+) diff --git a/proxy/proxy/connect_tls_117.go b/proxy/proxy/connect_tls_117.go index a1d580217..2ecff8814 100644 --- a/proxy/proxy/connect_tls_117.go +++ b/proxy/proxy/connect_tls_117.go @@ -20,6 +20,7 @@ package proxy import ( "context" "crypto/tls" + "fmt" "net" ) @@ -38,6 +39,7 @@ func (c *Client) connectTLS( // this file is conditionally compiled on only Go versions >= 1.17. if err := ret.HandshakeContext(ctx); err != nil { _ = ret.Close() + err = fmt.Errorf("config invalidated after TLS handshake failed, error = %w", err) c.invalidateCfg(cfg, instance, err) return nil, err } diff --git a/proxy/proxy/connect_tls_other.go b/proxy/proxy/connect_tls_other.go index 574bcd564..c2f2be949 100644 --- a/proxy/proxy/connect_tls_other.go +++ b/proxy/proxy/connect_tls_other.go @@ -106,6 +106,7 @@ func (c *Client) connectTLS( ret := tls.Client(conn, cfg) if err := ret.Handshake(); err != nil { _ = ret.Close() + err = fmt.Errorf("config invalidated after TLS handshake failed, error = %w", err) c.invalidateCfg(cfg, instance, err) return nil, err } From 757204b51fa8547ca6b9c7309a262bfdbdd3cbdd Mon Sep 17 00:00:00 2001 From: Eno Compton Date: Tue, 18 Jan 2022 15:58:36 -0700 Subject: [PATCH 4/6] Fix build --- proxy/proxy/connect_tls_other.go | 1 + 1 file changed, 1 insertion(+) diff --git a/proxy/proxy/connect_tls_other.go b/proxy/proxy/connect_tls_other.go index c2f2be949..fa2c19903 100644 --- a/proxy/proxy/connect_tls_other.go +++ b/proxy/proxy/connect_tls_other.go @@ -20,6 +20,7 @@ package proxy import ( "context" "crypto/tls" + "fmt" "net" "sync" "time" From 0e96e03368b3d374899548d3db8eba22787563d8 Mon Sep 17 00:00:00 2001 From: Eno Compton Date: Thu, 20 Jan 2022 12:09:12 -0700 Subject: [PATCH 5/6] Consildate error wrapping into one location --- proxy/proxy/client.go | 1 + proxy/proxy/connect_tls_117.go | 2 -- proxy/proxy/connect_tls_other.go | 1 - 3 files changed, 1 insertion(+), 3 deletions(-) diff --git a/proxy/proxy/client.go b/proxy/proxy/client.go index b438f0b81..6a913afb9 100644 --- a/proxy/proxy/client.go +++ b/proxy/proxy/client.go @@ -540,6 +540,7 @@ func (c *Client) invalidateCfg(cfg *tls.Config, instance string, err error) { if e.cfg != cfg { return } + err = fmt.Errorf("config invalidated after TLS handshake failed, error = %w", err) c.cfgCache[instance] = cacheEntry{ err: err, done: e.done, diff --git a/proxy/proxy/connect_tls_117.go b/proxy/proxy/connect_tls_117.go index 2ecff8814..a1d580217 100644 --- a/proxy/proxy/connect_tls_117.go +++ b/proxy/proxy/connect_tls_117.go @@ -20,7 +20,6 @@ package proxy import ( "context" "crypto/tls" - "fmt" "net" ) @@ -39,7 +38,6 @@ func (c *Client) connectTLS( // this file is conditionally compiled on only Go versions >= 1.17. if err := ret.HandshakeContext(ctx); err != nil { _ = ret.Close() - err = fmt.Errorf("config invalidated after TLS handshake failed, error = %w", err) c.invalidateCfg(cfg, instance, err) return nil, err } diff --git a/proxy/proxy/connect_tls_other.go b/proxy/proxy/connect_tls_other.go index fa2c19903..0774ca3f7 100644 --- a/proxy/proxy/connect_tls_other.go +++ b/proxy/proxy/connect_tls_other.go @@ -107,7 +107,6 @@ func (c *Client) connectTLS( ret := tls.Client(conn, cfg) if err := ret.Handshake(); err != nil { _ = ret.Close() - err = fmt.Errorf("config invalidated after TLS handshake failed, error = %w", err) c.invalidateCfg(cfg, instance, err) return nil, err } From 1041a5a01f6bb92d409fd98665373bab4a241d9f Mon Sep 17 00:00:00 2001 From: Eno Compton Date: Thu, 20 Jan 2022 12:15:52 -0700 Subject: [PATCH 6/6] Remove unused import --- proxy/proxy/connect_tls_other.go | 1 - 1 file changed, 1 deletion(-) diff --git a/proxy/proxy/connect_tls_other.go b/proxy/proxy/connect_tls_other.go index 0774ca3f7..574bcd564 100644 --- a/proxy/proxy/connect_tls_other.go +++ b/proxy/proxy/connect_tls_other.go @@ -20,7 +20,6 @@ package proxy import ( "context" "crypto/tls" - "fmt" "net" "sync" "time"