ca: improve test coverage for RenewIntermediate #11780

dnephin · 2021-12-08T18:40:03Z

There's a bunch of stuff here, so probably best viewed by individual commit. I'll try to comment on any interesting lines. I've been running the tests with -count=100 in a loop and the flakes appear to be fixed now.

Primarily this PR:

adds a test for using the Vault provider in a secondary PR (previously we mostly only tested with Vault provider in the primary)
improves some existing RenewIntermediate tests to be less flaky (hopefully not flaky at all!) and to be less verbose, so easier to read.

Other changes made:

adds an Active() method to both the IndexedCARoots (the RPC response) and CARoots (the state store result) so that we can stop repeating the "lookup active root" loop in dozens of places.

I'm planning on back porting to reduce conflicts when I back port other fixes.

Fixes #7520 (hopefully for real this time)

You can see a recent flake of this on main here.

dnephin · 2021-12-08T18:41:23Z

agent/consul/leader_connect_ca_test.go

@@ -56,8 +56,6 @@ func TestCAManager_Initialize_Vault_Secondary_SharedVault(t *testing.T) {
 		testrpc.WaitForTestAgent(t, serverDC1.RPC, "dc1")

 		codec := rpcClient(t, serverDC1)
-		defer codec.Close()


The rpcClient function already does this with t.Cleanup

TIL about t.Cleanup. Thanks! 🙌🏻

dnephin · 2021-12-08T18:43:13Z

agent/consul/leader_connect_test.go

-	_, activeRoot, err = store.CARootActive(nil)
+	codec := rpcClient(t, s1)
+	roots := structs.IndexedCARoots{}
+	err = msgpackrpc.CallWithCodec(codec, "ConnectCA.Roots", &structs.DCSpecificRequest{}, &roots)


I changed from a direct state store call to an RPC call because we need the TrustDomain below to create a valid spiffe ID.

I'm attempting to move where we store this value (see #11687) so I prefer fetching it from a public interface instead of reaching into the state store config.

dnephin · 2021-12-08T18:44:43Z

agent/consul/leader_connect_test.go

-	leafCsr, err := connect.ParseCSR(raw)
-	require.NoError(err)
-
-	leafPEM, err := provider.Sign(leafCsr)
-	require.NoError(err)
-
-	cert, err := connect.ParseCert(leafPEM)
-	require.NoError(err)


Replaced the direct call to provider.Sign with an RPC to ConenctCA.Sign because those two things return different data. The RPC call returns a pem that includes intermediate certs, which is required to verify the cert.

In general using public interfaces in tests is preferable, so this felt like a good change.

dnephin · 2021-12-08T18:44:58Z

agent/consul/leader_connect_test.go

-	_, err = cert.Verify(x509.VerifyOptions{
-		Intermediates: intermediatePool,
-		Roots:         rootPool,
-	})


This is now done in verifyLeafCert, which will prevent bugs like #11671 because verifyLeafCert checks the cert with intermediates from both locations (leafCertPEM and Root.IntermediateCerts)

dnephin · 2021-12-08T18:46:47Z

agent/structs/connect_ca.go

+func (r IndexedCARoots) Active() *CARoot {
+	for _, root := range r.Roots {
+		if root.ID == r.ActiveRootID {
+			return root
+		}
+	}
+	return nil
+}


I will be slowly replacing the many places we do this with this new method in future PRs.

dnephin · 2021-12-08T18:54:45Z

agent/consul/leader_connect_test.go

@@ -330,10 +330,8 @@ func TestCAManager_RenewIntermediate_Vault_Primary(t *testing.T) {
 	require := require.New(t)

 	testVault := ca.NewTestVaultServer(t)
-	defer testVault.Stop()


already done by NewTestVaultServer using t.Cleanup.

dnephin · 2021-12-08T18:55:07Z

agent/consul/leader_connect_test.go


-	dir1, s1 := testServerWithConfig(t, func(c *Config) {
-		c.Build = "1.6.0"


I think these were copy/paste from other tests where they were relevant. The build number does not matter for these tests.

dnephin · 2021-12-08T18:56:19Z

agent/consul/leader_connect_test.go

+				"LeafCertTTL":         "2s",
+				"IntermediateCertTTL": "7s",


This was another source of flakes. The leaf cert could expire before we got to checking if it was valid.

Increasing these values does not actually slow down the test at all , because we use a "NotBefore" date for the cert that is a full minute in the past, so we only end up waiting on the "renew interval", which is 200ms.

dnephin · 2021-12-08T18:57:50Z

agent/consul/leader_connect_test.go

-	provider, _ := getCAProviderWithLock(s1)
-	intermediatePEM, err := provider.ActiveIntermediate()


Reading the cert from the provider directly was another possible source of flakes, because that value will be updated before the state store.

I've tried to change these tests to always use either the RPC interface (preferred) or the state store (backs the RPC interface). That should make the tests less flaky.

I think we should make a similar change in other tests, but that is for a future PR.

dnephin · 2021-12-08T18:58:48Z

agent/connect/ca/testing.go

@@ -162,7 +163,7 @@ func runTestVault(t testing.T) (*TestVaultServer, error) {
 	}
 	t.Cleanup(func() {
 		if err := testVault.Stop(); err != nil {
-			t.Log("failed to stop vault server: %w", err)
+			t.Logf("failed to stop vault server: %v", err)


not sure why %w was not working here, but it was not formatting correctly, so %v should be fine. Maybe %w only works in fmt.Errorf ?

that seems right, since %w doesn't show up as a verb in this list, and is only documented alongside Errorf

dnephin · 2021-12-08T23:05:33Z

agent/consul/leader_connect_test.go

-		provider, _ = getCAProviderWithLock(s1)
-		newIntermediatePEM, err := provider.ActiveIntermediate()
+		store := s1.caManager.delegate.State()
+		_, storedRoot, err := store.CARootActive(nil)


This is another cause of flakes. Mentioned in another comment maybe, but it relates to this flake so I'll mention it here as well.

We poll checking the provider, which is updated first, but it is the state store that backs RPC requests, so we need to poll the state store (or RPC) not the provider.

The provider is updated before the state store, which causes a race in tests.

Which will be used in the next commit.

Use the new verifyLearfCert to show the cert verifies with intermediates from both sources. This required using the RPC interface so that the leaf pem was constructed correctly. Add IndexedCARoots.Active since that is a common operation we see in a few places.

I suspect one problem was that we set structs.IntermediateCertRenewInterval to 1ms, which meant that in some cases the intermediate could renew before we stored the original value. Another problem was that the 'wait for intermediate' loop was calling the provider.ActiveIntermediate, but the comparison needs to use the RPC endpoint to accurately represent a user request. So changing the 'wait for' to use the state store ensures we don't race. Also moves the patching into a separate function. Removes the addition of ca.CertificateTimeDriftBuffer as part of calculating halfTime. This was added in a previous commit to attempt to fix the flake, but it did not appear to fix the problem. Adding the time here was making the tests fail when using the shared patch function. It's not clear to me why, but there's no reason we should be including this time in the halfTime calculation.

freddygv · 2021-12-08T21:44:22Z

agent/connect/ca/testing.go

@@ -162,7 +163,7 @@ func runTestVault(t testing.T) (*TestVaultServer, error) {
 	}
 	t.Cleanup(func() {
 		if err := testVault.Stop(); err != nil {
-			t.Log("failed to stop vault server: %w", err)
+			t.Logf("failed to stop vault server: %v", err)


that seems right, since %w doesn't show up as a verb in this list, and is only documented alongside Errorf

acpana · 2021-12-09T03:06:38Z

agent/consul/leader_connect_ca_test.go

+	})
+}
+
+func verifyLeafCert(t *testing.T, root *structs.CARoot, leafCertPEM string) {


this looks super useful -- we don't have to do it with this PR but curious how we can propagate/ export this across server/ agent wherever we deal w certs.

If we find a use for it outside of this package, I generally like to export test helpers in their own internal packages (ref).

So maybe internal/catest or internal/pkitest.

acpana

Thanks for all the comments! It made reviewing easier -- LGTM! 🚀

hc-github-team-consul-core · 2021-12-09T17:30:53Z

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/522605.

hc-github-team-consul-core · 2021-12-09T17:30:54Z

🍒❌ Cherry pick of commit f9647ec onto release/1.11.x failed! Build Log

hc-github-team-consul-core · 2021-12-09T17:30:56Z

🍒❌ Cherry pick of commit f9647ec onto release/1.10.x failed! Build Log

hc-github-team-consul-core · 2021-12-09T17:30:58Z

🍒❌ Cherry pick of commit f9647ec onto release/1.9.x failed! Build Log

dnephin · 2021-12-09T17:31:32Z

I'll batch up a few changes to manually cherry-pick at the same time.

kisunji · 2022-01-11T22:56:39Z

@dnephin Have these been cherry-picked? I think 1.11.x is failing on CA tests related to this change

dnephin · 2022-01-11T23:16:28Z

Nope, was still on my list. I'll do that now along with #11721. Thanks for the reminder!

hc-github-team-consul-core · 2022-01-11T23:21:21Z

🍒 If backport labels were added before merging, cherry-picking will start automatically.

To retroactively trigger a backport after merging, add backport labels and re-run https://circleci.com/gh/hashicorp/consul/549495.

hc-github-team-consul-core · 2022-01-11T23:21:24Z

🍒✅ Cherry pick of commit f9647ec onto release/1.11.x succeeded!

…ondary ca: improve test coverage for RenewIntermediate

hc-github-team-consul-core · 2022-01-11T23:21:26Z

🍒❌ Cherry pick of commit f9647ec onto release/1.10.x failed! Build Log

hc-github-team-consul-core · 2022-01-11T23:21:28Z

🍒❌ Cherry pick of commit f9647ec onto release/1.9.x failed! Build Log

dnephin · 2022-01-11T23:22:12Z

I managed to get this auto-backported into 1.11 by backporting #11721.

I'll do the other two release branches manually now.

…ondary ca: improve test coverage for RenewIntermediate

dnephin added theme/testing Testing, and related enhancements theme/certificates Related to creating, distributing, and rotating certificates in Consul backport/1.9 pr/no-changelog PR does not need a corresponding .changelog entry labels Dec 8, 2021

dnephin requested a review from a team December 8, 2021 18:40

github-actions bot added theme/connect Anything related to Consul Connect, Service Mesh, Side Car Proxies and removed theme/certificates Related to creating, distributing, and rotating certificates in Consul theme/testing Testing, and related enhancements labels Dec 8, 2021

dnephin commented Dec 8, 2021

View reviewed changes

dnephin and others added 7 commits December 8, 2021 18:41

ca: Add CARoots.Active method

a5d9b1d

Which will be used in the next commit.

ca: add a test for Vault in secondary DC

a4ba1f3

ca: remove duplicate WaitFor function

305655a

ca: prune some unnecessary lookups in the tests

968aeff

fix misleading errors on vault shutdown

4116a14

dnephin force-pushed the dnephin/ca-test-vault-in-secondary branch from 6525f13 to 4116a14 Compare December 8, 2021 23:43

vercel bot temporarily deployed to Preview – consul-ui-staging December 8, 2021 23:43 Inactive

vercel bot temporarily deployed to Preview – consul December 8, 2021 23:43 Inactive

freddygv approved these changes Dec 8, 2021

View reviewed changes

dnephin mentioned this pull request Dec 8, 2021

ca: add a test that uses an intermediate CA as the primary CA #11783

Merged

acpana reviewed Dec 9, 2021

View reviewed changes

acpana approved these changes Dec 9, 2021

View reviewed changes

dnephin merged commit f9647ec into main Dec 9, 2021

dnephin deleted the dnephin/ca-test-vault-in-secondary branch December 9, 2021 17:29

hc-github-team-consul-core pushed a commit that referenced this pull request Jan 11, 2022

Merge pull request #11780 from hashicorp/dnephin/ca-test-vault-in-sec…

71c707e

…ondary ca: improve test coverage for RenewIntermediate

dnephin added a commit that referenced this pull request Jan 11, 2022

Merge pull request #11780 from hashicorp/dnephin/ca-test-vault-in-sec…

41e0c54

…ondary ca: improve test coverage for RenewIntermediate

dnephin mentioned this pull request Jan 11, 2022

[1.10.x] Backport CA and test fixes #12037

Merged

dnephin added a commit that referenced this pull request Jan 11, 2022

Merge pull request #11780 from hashicorp/dnephin/ca-test-vault-in-sec…

17b791a

…ondary ca: improve test coverage for RenewIntermediate

dnephin mentioned this pull request Jan 11, 2022

[1.9.x] Backport CA and test fixes #12038

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ca: improve test coverage for RenewIntermediate #11780

ca: improve test coverage for RenewIntermediate #11780

dnephin commented Dec 8, 2021 •

edited

dnephin Dec 8, 2021

boxofrad Dec 9, 2021

dnephin Dec 8, 2021

dnephin Dec 8, 2021

dnephin Dec 8, 2021

dnephin Dec 8, 2021

dnephin Dec 8, 2021

dnephin Dec 8, 2021

dnephin Dec 8, 2021

dnephin Dec 8, 2021

dnephin Dec 8, 2021

freddygv Dec 8, 2021

dnephin Dec 8, 2021

freddygv Dec 8, 2021

acpana Dec 9, 2021

dnephin Dec 9, 2021

acpana left a comment

hc-github-team-consul-core commented Dec 9, 2021

hc-github-team-consul-core commented Dec 9, 2021

hc-github-team-consul-core commented Dec 9, 2021

hc-github-team-consul-core commented Dec 9, 2021

dnephin commented Dec 9, 2021

kisunji commented Jan 11, 2022

dnephin commented Jan 11, 2022

hc-github-team-consul-core commented Jan 11, 2022

hc-github-team-consul-core commented Jan 11, 2022

hc-github-team-consul-core commented Jan 11, 2022

hc-github-team-consul-core commented Jan 11, 2022

dnephin commented Jan 11, 2022


		dir1, s1 := testServerWithConfig(t, func(c *Config) {
		c.Build = "1.6.0"

		provider, _ := getCAProviderWithLock(s1)
		intermediatePEM, err := provider.ActiveIntermediate()

ca: improve test coverage for RenewIntermediate #11780

ca: improve test coverage for RenewIntermediate #11780

Conversation

dnephin commented Dec 8, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

acpana left a comment

Choose a reason for hiding this comment

hc-github-team-consul-core commented Dec 9, 2021

hc-github-team-consul-core commented Dec 9, 2021

hc-github-team-consul-core commented Dec 9, 2021

hc-github-team-consul-core commented Dec 9, 2021

dnephin commented Dec 9, 2021

kisunji commented Jan 11, 2022

dnephin commented Jan 11, 2022

hc-github-team-consul-core commented Jan 11, 2022

hc-github-team-consul-core commented Jan 11, 2022

hc-github-team-consul-core commented Jan 11, 2022

hc-github-team-consul-core commented Jan 11, 2022

dnephin commented Jan 11, 2022

dnephin commented Dec 8, 2021 •

edited