Repository: GoogleCloudPlatform/cloudsql-proxy
Pull request #521 (2020-10-30 21:55:48)

fix: refreshCfg no longer caches error over valid cert
## Change Description

The caching logic in `refreshCfg` makes it possible to replace a perfectly valid cert with an error. This can cause production issues that are unnecessary, in my opinion. 

If an error gets cached, it's not possible to attempt to retry the refresh for a full minute. If we had never cached the error in the first place, we could wait a minute to retry the refresh without having to return any errors because we still have a valid cert from the last successful call to refreshCfg.

## Checklist

- [ ] https://github.com/GoogleCloudPlatform/cloudsql-proxy/issues/519

## Relevant issues:

- Fixes https://github.com/GoogleCloudPlatform/cloudsql-proxy/issues/519

kurtisvg: @troylelandshields Thanks for taking the time to fix this!

Patch:
diff --git a/proxy/proxy/client.go b/proxy/proxy/client.go
index a309850..c02f6a6 100644
--- a/proxy/proxy/client.go
+++ b/proxy/proxy/client.go
@@ -32,14 +32,14 @@ import (
 const (
 	DefaultRefreshCfgThrottle = time.Minute
 	keepAlivePeriod           = time.Minute
+	defaultRefreshCfgBuffer   = 5 * time.Minute
 )
 
 var (
 	// errNotCached is returned when the instance was not found in the Client's
 	// cache. It is an internal detail and is not actually ever returned to the
 	// user.
-	errNotCached      = errors.New("instance was not found in cache")
-	refreshCertBuffer = 30 * time.Second
+	errNotCached = errors.New("instance was not found in cache")
 )
 
 // Conn represents a connection from a client to a specific instance.
@@ -76,13 +76,6 @@ type Client struct {
 	// Dialer should return a new connection to the provided address. It will be used only if ContextDialer is nil.
 	Dialer func(net, addr string) (net.Conn, error)
 
-	// RefreshCfgThrottle is the amount of time to wait between configuration
-	// refreshes. If not set, it defaults to 1 minute.
-	//
-	// This is to prevent quota exhaustion in the case of client-side
-	// malfunction.
-	RefreshCfgThrottle time.Duration
-
 	// The cfgCache holds the most recent connection configuration keyed by
 	// instance. Relevant functions are refreshCfg and cachedCfg. It is
 	// protected by cacheL.
@@ -92,6 +85,17 @@ type Client struct {
 	// refreshCfgL prevents multiple goroutines from contacting the Cloud SQL API at once.
 	refreshCfgL sync.Mutex
 
+	// RefreshCfgThrottle is the amount of time to wait between configuration
+	// refreshes. If not set, it defaults to 1 minute.
+	//
+	// This is to prevent quota exhaustion in the case of client-side
+	// malfunction.
+	RefreshCfgThrottle time.Duration
+
+	// RefreshCertBuffer is the amount of time before the configuration expires to
+	// attempt to refresh it. If not set, it defaults to 5 minutes.
+	RefreshCfgBuffer time.Duration
+
 	// MaxConnections is the maximum number of connections to establish
 	// before refusing new connections. 0 means no limit.
 	MaxConnections uint64
@@ -166,6 +170,11 @@ func (c *Client) refreshCfg(instance string) (addr string, cfg *tls.Config, vers
 		throttle = DefaultRefreshCfgThrottle
 	}
 
+	refreshCfgBuffer := c.RefreshCfgBuffer
+	if refreshCfgBuffer == 0 {
+		refreshCfgBuffer = defaultRefreshCfgBuffer
+	}
+
 	c.cacheL.Lock()
 	if c.cfgCache == nil {
 		c.cfgCache = make(map[string]cacheEntry)
@@ -179,6 +188,12 @@ func (c *Client) refreshCfg(instance string) (addr string, cfg *tls.Config, vers
 	}
 
 	defer func() {
+		// if we failed to refresh cfg do not throw out potentially valid one
+		if err != nil && !isExpired(old.cfg) {
+			logging.Errorf("failed to refresh the ephemeral certificate for %s, returning previous cert instead: %v", instance, err)
+			addr, cfg, version, err = old.addr, old.cfg, old.version, old.err
+		}
+
 		c.cacheL.Lock()
 		c.cfgCache[instance] = cacheEntry{
 			lastRefreshed: time.Now(),
@@ -219,7 +234,7 @@ func (c *Client) refreshCfg(instance string) (addr string, cfg *tls.Config, vers
 
 	expire := mycert.Leaf.NotAfter
 	now := time.Now()
-	timeToRefresh := expire.Sub(now) - refreshCertBuffer
+	timeToRefresh := expire.Sub(now) - refreshCfgBuffer
 	if timeToRefresh <= 0 {
 		err = fmt.Errorf("new ephemeral certificate expires too soon: current time: %v, certificate expires: %v", expire, now)
 		logging.Errorf("ephemeral certificate (%+v) error: %v", mycert, err)
@@ -266,6 +281,9 @@ func genVerifyPeerCertificateFunc(instanceName string, pool *x509.CertPool) func
 }
 
 func isExpired(cfg *tls.Config) bool {
+	if cfg == nil {
+		return true
+	}
 	return time.Now().After(cfg.Certificates[0].Leaf.NotAfter)
 }
 


Test patch:
diff --git a/proxy/proxy/client_test.go b/proxy/proxy/client_test.go
index ebe1237..03f9d91 100644
--- a/proxy/proxy/client_test.go
+++ b/proxy/proxy/client_test.go
@@ -246,12 +246,6 @@ func TestShutdownTerminatesEarly(t *testing.T) {
 }
 
 func TestRefreshTimer(t *testing.T) {
-	oldRefreshCertBuffer := refreshCertBuffer
-	defer func() {
-		refreshCertBuffer = oldRefreshCertBuffer
-	}()
-	refreshCertBuffer = time.Second
-
 	timeToExpire := 5 * time.Second
 	b := &fakeCerts{}
 	certCreated := time.Now()
@@ -266,6 +260,7 @@ func TestRefreshTimer(t *testing.T) {
 			return nil, errFakeDial
 		},
 		RefreshCfgThrottle: 20 * time.Millisecond,
+		RefreshCfgBuffer:   time.Second,
 	}
 	// Call Dial to cache the cert.
 	if _, err := c.Dial(instance); err != errFakeDial {