From 4375dd24091d171d48e32629f221b0361923212d Mon Sep 17 00:00:00 2001 From: Jorge Marey Date: Wed, 9 Feb 2022 17:46:42 +0100 Subject: [PATCH 1/5] Avoid raft change when no config is provided on CAmanager - This avoids a change to the raft store when no roots or config are provided to persistNewRootAndConfig --- agent/consul/leader_connect_ca.go | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/agent/consul/leader_connect_ca.go b/agent/consul/leader_connect_ca.go index 35b4f343eb..ed5587ff57 100644 --- a/agent/consul/leader_connect_ca.go +++ b/agent/consul/leader_connect_ca.go @@ -693,7 +693,7 @@ func (c *CAManager) persistNewRootAndConfig(provider ca.Provider, newActiveRoot 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) { + if !shouldPersistNewRootAndConfig(newActiveRoot, storedConfig, config) { return nil } @@ -758,6 +758,17 @@ func (c *CAManager) persistNewRootAndConfig(provider ca.Provider, newActiveRoot return nil } +func shouldPersistNewRootAndConfig(newActiveRoot *structs.CARoot, oldConfig, newConfig *structs.CAConfiguration) bool { + if newActiveRoot != nil { + return true + } + + if newConfig == nil { + return false + } + return newConfig.Provider == oldConfig.Provider && reflect.DeepEqual(newConfig.Config, oldConfig.Config) +} + func (c *CAManager) UpdateConfiguration(args *structs.CARequest) (reterr error) { // Attempt to update the state first. oldState, err := c.setState(caStateReconfig, true) From 91ca4555627c5d93d7d2147ef449b8846342988d Mon Sep 17 00:00:00 2001 From: Jorge Marey Date: Thu, 10 Feb 2022 09:16:25 +0100 Subject: [PATCH 2/5] Add changelog file --- .changelog/12298.txt | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/12298.txt diff --git a/.changelog/12298.txt b/.changelog/12298.txt new file mode 100644 index 0000000000..b413e6ffa7 --- /dev/null +++ b/.changelog/12298.txt @@ -0,0 +1,3 @@ +```release-note:improvement +connect: reduce raft apply on CA configuration when no change is performed +``` From 1a0baf40242b85269ab86c2eaa906e86c32f1616 Mon Sep 17 00:00:00 2001 From: Jorge Marey Date: Thu, 10 Feb 2022 11:16:36 +0100 Subject: [PATCH 3/5] Add test case to verify #12298 --- agent/consul/leader_connect_ca_test.go | 80 ++++++++++++++++++++++++++ 1 file changed, 80 insertions(+) diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 73787700aa..2a55639b63 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -693,6 +693,86 @@ func TestCAManager_Initialize_Vault_WithIntermediateAsPrimaryCA(t *testing.T) { }) } +func TestCAManager_Verify_NoChangeToSecondaryConfig(t *testing.T) { + if testing.Short() { + t.Skip("too slow for testing.Short") + } + ca.SkipIfVaultNotPresent(t) + + vault := ca.NewTestVaultServer(t) + vclient := vault.Client() + generateExternalRootCA(t, vclient) + + meshRootPath := "pki-root" + primaryCert := setupPrimaryCA(t, vclient, meshRootPath) + + _, s1 := testServerWithConfig(t, func(c *Config) { + c.CAConfig = &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vault.RootToken, + "RootPKIPath": meshRootPath, + "IntermediatePKIPath": "pki-intermediate/", + // TODO: there are failures to init the CA system if these are not set + // to the values of the already initialized CA. + "PrivateKeyType": "ec", + "PrivateKeyBits": 256, + }, + } + }) + defer s1.Shutdown() + + roots := structs.IndexedCARoots{} + runStep(t, "check primary DC", func(t *testing.T) { + testrpc.WaitForTestAgent(t, s1.RPC, "dc1") + + codec := rpcClient(t, s1) + err := msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) + require.NoError(t, err) + require.Len(t, roots.Roots, 1) + require.Equal(t, primaryCert, roots.Roots[0].RootCert) + + leafCertPEM := getLeafCert(t, codec, roots.TrustDomain, "dc1") + verifyLeafCert(t, roots.Roots[0], leafCertPEM) + }) + + runStep(t, "run secondary DC and force update", func(t *testing.T) { + _, sDC2 := testServerWithConfig(t, func(c *Config) { + c.Datacenter = "dc2" + c.PrimaryDatacenter = "dc1" + c.CAConfig = &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vault.RootToken, + "RootPKIPath": meshRootPath, + "IntermediatePKIPath": "pki-secondary/", + // TODO: there are failures to init the CA system if these are not set + // to the values of the already initialized CA. + "PrivateKeyType": "ec", + "PrivateKeyBits": 256, + }, + } + }) + defer sDC2.Shutdown() + joinWAN(t, sDC2, s1) + testrpc.WaitForActiveCARoot(t, sDC2.RPC, "dc2", nil) + + codec := rpcClient(t, sDC2) + var configBefore structs.CAConfiguration + err := msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", &structs.DCSpecificRequest{}, &configBefore) + require.NoError(t, err) + + sDC2.caManager.secondaryUpdateRoots(roots) + + var configAfter structs.CAConfiguration + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", &structs.DCSpecificRequest{}, &configAfter) + require.NoError(t, err) + require.EqualValues(t, configBefore.ModifyIndex, configAfter.ModifyIndex) + }) +} + func getLeafCert(t *testing.T, codec rpc.ClientCodec, trustDomain string, dc string) string { pk, _, err := connect.GeneratePrivateKey() require.NoError(t, err) From f429c1a5d9ff8aab7f8898510b97fe849993b54a Mon Sep 17 00:00:00 2001 From: Jorge Marey Date: Tue, 1 Mar 2022 10:20:00 +0100 Subject: [PATCH 4/5] Fix vault test with suggested changes --- agent/consul/leader_connect_ca_test.go | 85 +++++++++----------------- 1 file changed, 29 insertions(+), 56 deletions(-) diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index 2a55639b63..ad92ef2054 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -693,84 +693,57 @@ func TestCAManager_Initialize_Vault_WithIntermediateAsPrimaryCA(t *testing.T) { }) } -func TestCAManager_Verify_NoChangeToSecondaryConfig(t *testing.T) { +func TestCAManager_Verify_Vault_NoChangeToSecondaryConfig(t *testing.T) { if testing.Short() { t.Skip("too slow for testing.Short") } ca.SkipIfVaultNotPresent(t) vault := ca.NewTestVaultServer(t) - vclient := vault.Client() - generateExternalRootCA(t, vclient) - meshRootPath := "pki-root" - primaryCert := setupPrimaryCA(t, vclient, meshRootPath) - - _, s1 := testServerWithConfig(t, func(c *Config) { + _, sDC1 := testServerWithConfig(t, func(c *Config) { c.CAConfig = &structs.CAConfiguration{ Provider: "vault", Config: map[string]interface{}{ "Address": vault.Addr, "Token": vault.RootToken, - "RootPKIPath": meshRootPath, + "RootPKIPath": "pki-root/", "IntermediatePKIPath": "pki-intermediate/", - // TODO: there are failures to init the CA system if these are not set - // to the values of the already initialized CA. - "PrivateKeyType": "ec", - "PrivateKeyBits": 256, }, } }) - defer s1.Shutdown() + defer sDC1.Shutdown() + testrpc.WaitForActiveCARoot(t, sDC1.RPC, "dc1", nil) - roots := structs.IndexedCARoots{} - runStep(t, "check primary DC", func(t *testing.T) { - testrpc.WaitForTestAgent(t, s1.RPC, "dc1") - - codec := rpcClient(t, s1) - err := msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots) - require.NoError(t, err) - require.Len(t, roots.Roots, 1) - require.Equal(t, primaryCert, roots.Roots[0].RootCert) - - leafCertPEM := getLeafCert(t, codec, roots.TrustDomain, "dc1") - verifyLeafCert(t, roots.Roots[0], leafCertPEM) + _, sDC2 := testServerWithConfig(t, func(c *Config) { + c.Datacenter = "dc2" + c.PrimaryDatacenter = "dc1" + c.CAConfig = &structs.CAConfiguration{ + Provider: "vault", + Config: map[string]interface{}{ + "Address": vault.Addr, + "Token": vault.RootToken, + "RootPKIPath": "pki-root/", + "IntermediatePKIPath": "pki-intermediate/", + }, + } }) + defer sDC2.Shutdown() + joinWAN(t, sDC2, sDC1) + testrpc.WaitForActiveCARoot(t, sDC2.RPC, "dc2", nil) - runStep(t, "run secondary DC and force update", func(t *testing.T) { - _, sDC2 := testServerWithConfig(t, func(c *Config) { - c.Datacenter = "dc2" - c.PrimaryDatacenter = "dc1" - c.CAConfig = &structs.CAConfiguration{ - Provider: "vault", - Config: map[string]interface{}{ - "Address": vault.Addr, - "Token": vault.RootToken, - "RootPKIPath": meshRootPath, - "IntermediatePKIPath": "pki-secondary/", - // TODO: there are failures to init the CA system if these are not set - // to the values of the already initialized CA. - "PrivateKeyType": "ec", - "PrivateKeyBits": 256, - }, - } - }) - defer sDC2.Shutdown() - joinWAN(t, sDC2, s1) - testrpc.WaitForActiveCARoot(t, sDC2.RPC, "dc2", nil) + codec := rpcClient(t, sDC2) + var configBefore structs.CAConfiguration + err := msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", &structs.DCSpecificRequest{}, &configBefore) + require.NoError(t, err) - codec := rpcClient(t, sDC2) - var configBefore structs.CAConfiguration - err := msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", &structs.DCSpecificRequest{}, &configBefore) - require.NoError(t, err) + renewLeafSigningCert(t, sDC1.caManager, sDC1.caManager.primaryRenewIntermediate) - sDC2.caManager.secondaryUpdateRoots(roots) + var configAfter structs.CAConfiguration + err = msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", &structs.DCSpecificRequest{}, &configAfter) + require.NoError(t, err) - var configAfter structs.CAConfiguration - err = msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", &structs.DCSpecificRequest{}, &configAfter) - require.NoError(t, err) - require.EqualValues(t, configBefore.ModifyIndex, configAfter.ModifyIndex) - }) + require.EqualValues(t, configBefore.ModifyIndex, configAfter.ModifyIndex) } func getLeafCert(t *testing.T, codec rpc.ClientCodec, trustDomain string, dc string) string { From 161206e24d32820289a3bc065e20bd70d7f0a2ab Mon Sep 17 00:00:00 2001 From: Daniel Nephin Date: Wed, 2 Mar 2022 16:34:28 -0500 Subject: [PATCH 5/5] ca: make sure the test fails without the fix Also change the path used for the secondary so that both primary and secondary do not overwrite each other. --- agent/consul/leader_connect_ca_test.go | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/agent/consul/leader_connect_ca_test.go b/agent/consul/leader_connect_ca_test.go index ad92ef2054..1b4eaf38fd 100644 --- a/agent/consul/leader_connect_ca_test.go +++ b/agent/consul/leader_connect_ca_test.go @@ -724,7 +724,7 @@ func TestCAManager_Verify_Vault_NoChangeToSecondaryConfig(t *testing.T) { "Address": vault.Addr, "Token": vault.RootToken, "RootPKIPath": "pki-root/", - "IntermediatePKIPath": "pki-intermediate/", + "IntermediatePKIPath": "pki-intermediate-2/", }, } }) @@ -739,6 +739,9 @@ func TestCAManager_Verify_Vault_NoChangeToSecondaryConfig(t *testing.T) { renewLeafSigningCert(t, sDC1.caManager, sDC1.caManager.primaryRenewIntermediate) + // Give the secondary some time to notice the update + time.Sleep(100 * time.Millisecond) + var configAfter structs.CAConfiguration err = msgpackrpc.CallWithCodec(codec, "ConnectCA.ConfigurationGet", &structs.DCSpecificRequest{}, &configAfter) require.NoError(t, err)