From 13c31ccfce50bb2d34d01cfdf3331690ecc4e33b Mon Sep 17 00:00:00 2001
From: Kyle Havlovitz <kylehav@gmail.com>
Date: Fri, 20 Nov 2020 15:54:44 -0800
Subject: [PATCH] Clean up the logic in persistNewRootAndConfig

---
 agent/consul/leader_connect_ca.go      | 39 +++++++++++++++++---------
 agent/consul/leader_connect_ca_test.go |  1 +
 agent/consul/leader_connect_test.go    | 16 -----------
 3 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go
index 5e42b5f9d0..cf010b353b 100644
--- a/agent/consul/leader_connect_ca.go
+++ b/agent/consul/leader_connect_ca.go
@@ -535,6 +535,7 @@ func (c *CAManager) persistNewRootAndConfig(provider ca.Provider, newActiveRoot
 		return err
 	}
 
+	// Look up the existing CA config if a new one wasn't provided.
 	var newConf structs.CAConfiguration
 	_, storedConfig, err := state.CAConfig(nil)
 	if err != nil {
@@ -543,14 +544,28 @@ func (c *CAManager) persistNewRootAndConfig(provider ca.Provider, newActiveRoot
 	if storedConfig == nil {
 		return fmt.Errorf("local CA not initialized yet")
 	}
+	// Exit early if the change is a no-op.
+	if newActiveRoot == nil && config != nil && config.Provider == storedConfig.Provider && reflect.DeepEqual(config.Config, storedConfig.Config) {
+		return nil
+	}
+
 	if config != nil {
 		newConf = *config
 	} else {
 		newConf = *storedConfig
 	}
+
+	// Update the trust domain for the config if there's a new root, or keep the old
+	// one if the root isn't being updated.
 	newConf.ModifyIndex = storedConfig.ModifyIndex
 	if newActiveRoot != nil {
 		newConf.ClusterID = newActiveRoot.ExternalTrustDomain
+	} else {
+		_, activeRoot, err := state.CARootActive(nil)
+		if err != nil {
+			return err
+		}
+		newConf.ClusterID = activeRoot.ExternalTrustDomain
 	}
 
 	// Persist any state the provider needs us to
@@ -562,21 +577,19 @@ func (c *CAManager) persistNewRootAndConfig(provider ca.Provider, newActiveRoot
 	// If there's a new active root, copy the root list and append it, updating
 	// the old root with the time it was rotated out.
 	var newRoots structs.CARoots
-	if newActiveRoot != nil {
-		for _, r := range oldRoots {
-			newRoot := *r
-			if newRoot.Active {
-				newRoot.Active = false
-				newRoot.RotatedOutAt = time.Now()
-			}
-			if newRoot.ExternalTrustDomain == "" {
-				newRoot.ExternalTrustDomain = newConf.ClusterID
-			}
-			newRoots = append(newRoots, &newRoot)
+	for _, r := range oldRoots {
+		newRoot := *r
+		if newRoot.Active {
+			newRoot.Active = false
+			newRoot.RotatedOutAt = time.Now()
 		}
+		if newRoot.ExternalTrustDomain == "" {
+			newRoot.ExternalTrustDomain = newConf.ClusterID
+		}
+		newRoots = append(newRoots, &newRoot)
+	}
+	if newActiveRoot != nil {
 		newRoots = append(newRoots, newActiveRoot)
-	} else {
-		newRoots = oldRoots
 	}
 
 	args := &structs.CARequest{
diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go
index a582f90ba1..25708bc964 100644
--- a/agent/consul/leader_connect_ca_test.go
+++ b/agent/consul/leader_connect_ca_test.go
@@ -100,6 +100,7 @@ func (m *mockCAServerDelegate) raftApply(t structs.MessageType, msg interface{})
 
 		act, err = m.store.CACheckAndSetConfig(1, req.Config.ModifyIndex, req.Config)
 		require.NoError(m.t, err)
+		require.True(m.t, act)
 	}
 	m.callbackCh <- fmt.Sprintf("raftApply/%s", t)
 	return nil, nil
diff --git a/agent/consul/leader_connect_test.go b/agent/consul/leader_connect_test.go
index 5beca904bd..cf7ddff421 100644
--- a/agent/consul/leader_connect_test.go
+++ b/agent/consul/leader_connect_test.go
@@ -700,22 +700,6 @@ func TestLeader_SecondaryCA_TransitionFromPrimary(t *testing.T) {
 	require.NoError(t, s2.RPC("ConnectCA.Roots", &args, &dc2PrimaryRoots))
 	require.Len(t, dc2PrimaryRoots.Roots, 1)
 
-	// Set the ExternalTrustDomain to a blank string to simulate an old version (pre-1.4.0)
-	// it's fine to change the roots struct directly here because the RPC endpoint already
-	// makes a copy to return.
-	dc2PrimaryRoots.Roots[0].ExternalTrustDomain = ""
-	rootSetArgs := structs.CARequest{
-		Op:         structs.CAOpSetRoots,
-		Datacenter: "dc2",
-		Index:      dc2PrimaryRoots.Index,
-		Roots:      dc2PrimaryRoots.Roots,
-	}
-	resp, err := s2.raftApply(structs.ConnectCARequestType, rootSetArgs)
-	require.NoError(t, err)
-	if respErr, ok := resp.(error); ok {
-		t.Fatal(respErr)
-	}
-
 	// Shutdown s2 and restart it with the dc1 as the primary
 	s2.Shutdown()
 	dir3, s3 := testServerWithConfig(t, func(c *Config) {