tlsutil: remove the RLock from log

The log method only needed the lock because it accessed version. By using an atomic
instead of a lock, we can remove the risk that the comments call out, making log safer
to use.

Also updates the log name to match the function names, and adds some comments about how
the lock is used.

tlsutil/config.go

@@ -11,6 +11,7 @@ import (
 	"sort"
 	"strings"
 	"sync"
+	"sync/atomic"
 	"time"
 
 	"github.com/hashicorp/go-hclog"
@@ -176,16 +177,20 @@ type manual struct {
 // Configurator holds a Config and is responsible for generating all the
 // *tls.Config necessary for Consul. Except the one in the api package.
 type Configurator struct {
-	// lock synchronizes access to all fields on this struct
+	// lock synchronizes access to all fields on this struct except for logger and version.
 	lock                 sync.RWMutex
 	base                 *Config
 	autoTLS              *autoTLS
 	manual               *manual
 	peerDatacenterUseTLS map[string]bool
+	caPool               *x509.CertPool
 
-	caPool  *x509.CertPool
+	// logger is not protected by a lock. It must never be changed after
-	logger  hclog.Logger
+	// Configurator is created.
-	version int
+	logger hclog.Logger
+	// version is increased each time the Configurator is updated. Must be accessed
+	// using sync/atomic.
+	version uint64
 }
 
 // NewConfigurator creates a new Configurator and sets the provided
@@ -229,8 +234,6 @@ func (c *Configurator) ManualCAPems() []string {
 // This function acquires a write lock because it writes the new config.
 func (c *Configurator) Update(config Config) error {
 	c.lock.Lock()
-	// order of defers matters because log acquires a RLock()
-	defer c.log("Update")
 	defer c.lock.Unlock()
 
 	cert, err := loadKeyPair(config.CertFile, config.KeyFile)
@@ -252,7 +255,8 @@ func (c *Configurator) Update(config Config) error {
 	c.manual.cert = cert
 	c.manual.caPems = pems
 	c.caPool = pool
-	c.version++
+	atomic.AddUint64(&c.version, 1)
+	c.log("Update")
 	return nil
 }
 
@@ -262,8 +266,6 @@ func (c *Configurator) Update(config Config) error {
 // Or it is being called on the client side when CA changes are detected.
 func (c *Configurator) UpdateAutoTLSCA(connectCAPems []string) error {
 	c.lock.Lock()
-	// order of defers matters because log acquires a RLock()
-	defer c.log("UpdateAutoEncryptCA")
 	defer c.lock.Unlock()
 
 	pool, err := pool(append(c.manual.caPems, append(c.autoTLS.manualCAPems, connectCAPems...)...))
@@ -275,14 +277,13 @@ func (c *Configurator) UpdateAutoTLSCA(connectCAPems []string) error {
 	}
 	c.autoTLS.connectCAPems = connectCAPems
 	c.caPool = pool
-	c.version++
+	atomic.AddUint64(&c.version, 1)
+	c.log("UpdateAutoTLSCA")
 	return nil
 }
 
 // UpdateAutoTLSCert
 func (c *Configurator) UpdateAutoTLSCert(pub, priv string) error {
-	// order of defers matters because log acquires a RLock()
-	defer c.log("UpdateAutoEncryptCert")
 	cert, err := tls.X509KeyPair([]byte(pub), []byte(priv))
 	if err != nil {
 		return fmt.Errorf("Failed to load cert/key pair: %v", err)
@@ -292,15 +293,14 @@ func (c *Configurator) UpdateAutoTLSCert(pub, priv string) error {
 	defer c.lock.Unlock()
 
 	c.autoTLS.cert = &cert
-	c.version++
+	atomic.AddUint64(&c.version, 1)
+	c.log("UpdateAutoTLSCert")
 	return nil
 }
 
 // UpdateAutoTLS sets everything under autoEncrypt. This is being called on the
 // client when it received its cert from AutoEncrypt/AutoConfig endpoints.
 func (c *Configurator) UpdateAutoTLS(manualCAPems, connectCAPems []string, pub, priv string, verifyServerHostname bool) error {
-	// order of defers matters because log acquires a RLock()
-	defer c.log("UpdateAutoEncrypt")
 	cert, err := tls.X509KeyPair([]byte(pub), []byte(priv))
 	if err != nil {
 		return fmt.Errorf("Failed to load cert/key pair: %v", err)
@@ -318,14 +318,16 @@ func (c *Configurator) UpdateAutoTLS(manualCAPems, connectCAPems []string, pub,
 	c.autoTLS.cert = &cert
 	c.caPool = pool
 	c.autoTLS.verifyServerHostname = verifyServerHostname
-	c.version++
+	atomic.AddUint64(&c.version, 1)
+	c.log("UpdateAutoTLS")
 	return nil
 }
 
 func (c *Configurator) UpdateAreaPeerDatacenterUseTLS(peerDatacenter string, useTLS bool) {
 	c.lock.Lock()
 	defer c.lock.Unlock()
-	c.version++
+	atomic.AddUint64(&c.version, 1)
+	c.log("UpdateAreaPeerDatacenterUseTLS")
 	c.peerDatacenterUseTLS[peerDatacenter] = useTLS
 }
 
@@ -818,12 +820,9 @@ func (c *Configurator) AutoEncryptCertExpired() bool {
 	return c.AutoEncryptCertNotAfter().Before(time.Now())
 }
 
-// This function acquires a read lock because it reads from the config.
 func (c *Configurator) log(name string) {
-	if c.logger != nil {
+	if c.logger != nil && c.logger.IsTrace() {
-		c.lock.RLock()
+		c.logger.Trace(name, "version", atomic.LoadUint64(&c.version))
-		defer c.lock.RUnlock()
-		c.logger.Trace(name, "version", c.version)
 	}
 }
 

tlsutil/config_test.go

@@ -1026,11 +1026,10 @@ func TestConfigurator_UpdateChecks(t *testing.T) {
 	require.NoError(t, err)
 	require.NoError(t, c.Update(Config{}))
 	require.Error(t, c.Update(Config{VerifyOutgoing: true}))
-	require.Error(t, c.Update(Config{VerifyIncoming: true,
+	require.Error(t, c.Update(Config{VerifyIncoming: true, CAFile: "../test/ca/root.cer"}))
-		CAFile: "../test/ca/root.cer"}))
 	require.False(t, c.base.VerifyIncoming)
 	require.False(t, c.base.VerifyOutgoing)
-	require.Equal(t, c.version, 2)
+	require.Equal(t, uint64(2), c.version)
 }
 
 func TestConfigurator_UpdateSetsStuff(t *testing.T) {
@@ -1039,10 +1038,10 @@ func TestConfigurator_UpdateSetsStuff(t *testing.T) {
 	require.Nil(t, c.caPool)
 	require.Nil(t, c.manual.cert)
 	require.Equal(t, c.base, &Config{})
-	require.Equal(t, 1, c.version)
+	require.Equal(t, uint64(1), c.version)
 
 	require.Error(t, c.Update(Config{VerifyOutgoing: true}))
-	require.Equal(t, c.version, 1)
+	require.Equal(t, uint64(1), c.version)
 
 	config := Config{
 		CAFile:   "../test/ca/root.cer",
@@ -1054,7 +1053,7 @@ func TestConfigurator_UpdateSetsStuff(t *testing.T) {
 	require.Len(t, c.caPool.Subjects(), 1)
 	require.NotNil(t, c.manual.cert)
 	require.Equal(t, c.base, &config)
-	require.Equal(t, 2, c.version)
+	require.Equal(t, uint64(2), c.version)
 }
 
 func TestConfigurator_ServerNameOrNodeName(t *testing.T) {