ca: properly handle the case where the secondary initializes after the primary #11514
Conversation
Previously secondaryInitialize would return nil in this case, which prevented the deferred initialize from happening, and left the CA in an uninitialized state until a config update or root rotation. To fix this I extracted the common parts into the delegate implementation. However looking at this again, it seems like the handling in secondaryUpdateRoots is impossible, because that function should never be called before the secondary is initialzied. I beleive we can remove some of that logic in a follow up.
The secondary must get the clusterID from the primary
dnephin
added
the
theme/certificates
github-actions
bot
removed
the
theme/certificates
| @@ -1110,15 +1109,18 @@ func TestLeader_SecondaryCA_UpgradeBeforePrimary(t *testing.T) { | |||
| require.Equal(r, roots1[0].ID, roots2[0].ID) | |||
| require.Equal(r, roots1[0].RootCert, roots2[0].RootCert) | |||
|
|
|||
| secondaryProvider, _ := getCAProviderWithLock(s2) | |||
There was a problem hiding this comment.
This had to move into the retry loop, otherwise it was pointing at the old not initialized provider that existed before the secondary could be initialized.
| // Allow the ClusterID to change if it's provided by an internal operation, such | ||
| // as a primary datacenter being switched to secondary mode. |
There was a problem hiding this comment.
I deleted this comment since I came across it while tracing this code and it is misleading. We always allow changing the ClusterID. All these next lines do is default it when the new config has it unset.
dnephin
added
pr/no-changelog
Currently getCARoots could return an empty object with an empty trust domain before the CA is initialized. This commit returns an error while there is no CA config or no trust domain. There could be a CA config and no trust domain because the CA config can be created in InitializeCA before initialization succeeds.
There was a problem hiding this comment.
These changes look good - we could probably remove ClusterID from the config altogether in the future (we don't document it as a configurable field) and just persist it in the roots object in the state store. I think having it in two places is definitely a source of unnecessary confusion.
freddygv
added
backport/1.10
and removed
pr/no-changelog
|
🍒 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/496536. |
ca: properly handle the case where the secondary initializes after the primary
Backport of #11514 --------- Fixes #11367 Previously `secondaryInitialize` would return nil in this case, which prevented the deferred initialize from happening, and left the CA in an uninitialized state until a config update or root rotation. To fix this I extracted the common parts into the delegate implementation. However looking at this again, it seems like the handling in secondaryUpdateRoots is impossible, because that function should never be called before the secondary is initialized. I believe we can remove some of that logic in a follow up.
| indexedRoots.TrustDomain = signingID.Host() | ||
| if indexedRoots.TrustDomain == "" { |
There was a problem hiding this comment.
Host() returns clusterID.Domain, and Domain is hardcoded to consul in SpiffeIDSigningForCluster, so I don't think this will ever eventuate to true. Should this only check the ClusterID from config?
cc @freddygv
Fixes #11367
Previously
secondaryInitializewould return nil in this case, which prevented thedeferred initialize from happening, and left the CA in an uninitialized state until a config
update or root rotation.
To fix this I extracted the common parts into the delegate implementation. However looking at this
again, it seems like the handling in secondaryUpdateRoots is impossible, because that function
should never be called before the secondary is initialzied. I beleive we can remove some of that
logic in a follow up.
TODO: