Merge pull request GoogleCloudPlatform#158 from GoogleCloudPlatform/revert-142-master

Revert "Automatically refresh SSL certs before they expire"

Author: hfwang

proxy/proxy/client.go

@@ -32,13 +32,10 @@ const (
 	keepAlivePeriod           = 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 = 5 * time.Minute
-)
+// 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.
+var errNotCached = errors.New("instance was not found in cache")
 
 // Conn represents a connection from a client to a specific instance.
 type Conn struct {
@@ -81,12 +78,9 @@ type Client struct {
 
 	// The cfgCache holds the most recent connection configuration keyed by
 	// instance. Relevant functions are refreshCfg and cachedCfg. It is
-	// protected by cacheL.
+	// protected by cfgL.
 	cfgCache map[string]cacheEntry
-	cacheL   sync.Mutex
-
-	// refreshCfgL prevents multiple goroutines from contacting the Cloud SQL API at once.
-	refreshCfgL sync.Mutex
+	cfgL     sync.RWMutex
 
 	// MaxConnections is the maximum number of connections to establish
 	// before refusing new connections. 0 means no limit.
@@ -156,39 +150,32 @@ func (c *Client) handleConn(conn Conn) {
 // address as well as construct a new tls.Config to connect to the instance. It
 // caches the result.
 func (c *Client) refreshCfg(instance string) (addr string, cfg *tls.Config, err error) {
-	c.refreshCfgL.Lock()
-	defer c.refreshCfgL.Unlock()
+	c.cfgL.Lock()
+	defer c.cfgL.Unlock()
 
 	throttle := c.RefreshCfgThrottle
 	if throttle == 0 {
 		throttle = DefaultRefreshCfgThrottle
 	}
 
-	c.cacheL.Lock()
-	if c.cfgCache == nil {
-		c.cfgCache = make(map[string]cacheEntry)
-	}
-	old, oldok := c.cfgCache[instance]
-	c.cacheL.Unlock()
-
-	if oldok && time.Since(old.lastRefreshed) < throttle {
+	if old := c.cfgCache[instance]; time.Since(old.lastRefreshed) < throttle {
 		logging.Errorf("Throttling refreshCfg(%s): it was only called %v ago", instance, time.Since(old.lastRefreshed))
 		// Refresh was called too recently, just reuse the result.
 		return old.addr, old.cfg, old.err
 	}
 
+	if c.cfgCache == nil {
+		c.cfgCache = make(map[string]cacheEntry)
+	}
+
 	defer func() {
-		if err != nil && oldok {
-			return
-		}
-		c.cacheL.Lock()
 		c.cfgCache[instance] = cacheEntry{
 			lastRefreshed: time.Now(),
-			err:           err,
-			addr:          addr,
-			cfg:           cfg,
+
+			err:  err,
+			addr: addr,
+			cfg:  cfg,
 		}
-		c.cacheL.Unlock()
 	}()
 
 	mycert, err := c.Certs.Local(instance)
@@ -208,26 +195,13 @@ func (c *Client) refreshCfg(instance string) (addr string, cfg *tls.Config, err
 		Certificates: []tls.Certificate{mycert},
 		RootCAs:      certs,
 	}
-
-	// Refresh cert 5 minutes before it expires.
-	timeToRefresh := cfg.Certificates[0].Leaf.NotAfter.Sub(time.Now()) - refreshCertBuffer
-	if timeToRefresh > 0 {
-		go func() {
-			<-time.After(timeToRefresh)
-			logging.Verbosef("Cert for instance %s will expire soon, refreshing now.", instance)
-			if _, _, err := c.refreshCfg(instance); err != nil {
-				logging.Errorf("couldn't connect to %q: %v", instance, err)
-			}
-		}()
-	}
-
 	return fmt.Sprintf("%s:%d", addr, c.Port), cfg, nil
 }
 
 func (c *Client) cachedCfg(instance string) (string, *tls.Config) {
-	c.cacheL.Lock()
+	c.cfgL.RLock()
 	ret, ok := c.cfgCache[instance]
-	c.cacheL.Unlock()
+	c.cfgL.RUnlock()
 
 	// Don't waste time returning an expired/invalid cert.
 	if !ok || ret.err != nil || time.Now().After(ret.cfg.Certificates[0].Leaf.NotAfter) {
@@ -251,7 +225,6 @@ func (c *Client) Dial(instance string) (net.Conn, error) {
 	if err != nil {
 		return nil, err
 	}
-
 	return c.tryConnect(addr, cfg)
 }
 

proxy/proxy/client_test.go

@@ -28,19 +28,15 @@ import (
 
 const instance = "instance-name"
 
-var (
-	errFakeDial = errors.New("this error is returned by the dialer")
-	forever     = time.Date(9999, 0, 0, 0, 0, 0, 0, time.UTC)
-)
+var errFakeDial = errors.New("this error is returned by the dialer")
 
 type fakeCerts struct {
 	sync.Mutex
 	called int
 }
 
 type blockingCertSource struct {
-	values     map[string]*fakeCerts
-	validUntil time.Time
+	values map[string]*fakeCerts
 }
 
 func (cs *blockingCertSource) Local(instance string) (tls.Certificate, error) {
@@ -52,10 +48,11 @@ func (cs *blockingCertSource) Local(instance string) (tls.Certificate, error) {
 	v.called++
 	v.Unlock()
 
+	validUntil, _ := time.Parse("2006", "9999")
 	// Returns a cert which is valid forever.
 	return tls.Certificate{
 		Leaf: &x509.Certificate{
-			NotAfter: cs.validUntil,
+			NotAfter: validUntil,
 		},
 	}, nil
 }
@@ -70,9 +67,7 @@ func TestClientCache(t *testing.T) {
 		Certs: &blockingCertSource{
 			map[string]*fakeCerts{
 				instance: b,
-			},
-			forever,
-		},
+			}},
 		Dialer: func(string, string) (net.Conn, error) {
 			return nil, errFakeDial
 		},
@@ -97,9 +92,7 @@ func TestConcurrentRefresh(t *testing.T) {
 		Certs: &blockingCertSource{
 			map[string]*fakeCerts{
 				instance: b,
-			},
-			forever,
-		},
+			}},
 		Dialer: func(string, string) (net.Conn, error) {
 			return nil, errFakeDial
 		},
@@ -138,9 +131,7 @@ func TestMaximumConnectionsCount(t *testing.T) {
 
 	b := &fakeCerts{}
 	certSource := blockingCertSource{
-		map[string]*fakeCerts{},
-		forever,
-	}
+		map[string]*fakeCerts{}}
 	firstDialExited := make(chan struct{})
 	c := &Client{
 		Certs: &certSource,
@@ -192,54 +183,3 @@ func TestMaximumConnectionsCount(t *testing.T) {
 		t.Errorf("client should have dialed exactly the maximum of %d connections (%d connections, %d dials)", maxConnections, numConnections, dials)
 	}
 }
-
-func TestRefreshTimer(t *testing.T) {
-	refreshCertBuffer = time.Millisecond * 10
-	timeToExpire := time.Millisecond * 500
-	b := &fakeCerts{}
-	c := &Client{
-		Certs: &blockingCertSource{
-			map[string]*fakeCerts{
-				instance: b,
-			},
-			time.Now().Add(timeToExpire),
-		},
-		Dialer: func(string, string) (net.Conn, error) {
-			return nil, errFakeDial
-		},
-		RefreshCfgThrottle: 20 * time.Millisecond,
-	}
-
-	// Call Dial to cache the cert.
-	if _, err := c.Dial(instance); err != errFakeDial {
-		t.Errorf("unexpected error: %v", err)
-	}
-
-	c.cacheL.Lock()
-	cached, ok := c.cfgCache[instance]
-	c.cacheL.Unlock()
-	if !ok {
-		t.Error("expected instance to be cached")
-	}
-	waitTil := time.After(timeToExpire + (10 * time.Millisecond))
-loop:
-	for {
-		select {
-		case <-waitTil:
-			break loop
-		default:
-			time.Sleep(100 * time.Millisecond)
-		}
-	}
-
-	// Verify cert was refreshed in the background, without calling Dial again.
-	c.cacheL.Lock()
-	refreshed, ok := c.cfgCache[instance]
-	c.cacheL.Unlock()
-	if !ok {
-		t.Error("expected instance to be cached")
-	}
-	if !refreshed.lastRefreshed.After(cached.lastRefreshed) {
-		t.Error("expected cert to be refreshed.")
-	}
-}