ca: isolate more of the CA logic in CAManager #10445

dnephin · 2021-06-21T23:46:44Z

Best viewed by individual commit.

This is in preparation for documenting the CA provider flows, and fixing some bugs related to CA provider in secondary DCs.

dnephin · 2021-06-22T00:16:35Z

Looks like there is a lint error and a couple test failures to fix.

dnephin · 2021-06-22T21:48:19Z

agent/consul/leader_connect_ca.go

 	caStateUninitialized     caState = "UNINITIALIZED"
-	caStateInitializing              = "INITIALIZING"
-	caStateInitialized               = "INITIALIZED"
-	caStateRenewIntermediate         = "RENEWING"
-	caStateReconfig                  = "RECONFIGURING"
+	caStateInitializing      caState = "INITIALIZING"
+	caStateInitialized       caState = "INITIALIZED"
+	caStateRenewIntermediate caState = "RENEWING"
+	caStateReconfig          caState = "RECONFIGURING"


Implicit using the type in a const block only works with iota. In blocks like this without iota the type has to be explicit. This was changed so that tests can use require.Equal , instead of require.EqualValue

dnephin · 2021-06-30T18:23:44Z

agent/consul/leader_connect_ca.go

-	c.primaryRoots = structs.IndexedCARoots{}
-	c.actingSecondaryCA = false


TODO: check to see if this should be done in Stop, should primaryRoots and actingSecondarCA be called to zero things out on leader loss.

I checked and I'm pretty sure it's needed to clean up the struc and the state. I added them as part of the stop

How was reset() being used originally? I can't find where it was being called from

It wasn't, which is why 7dae65c was able to remove the method without any other changes. While reviewing this I thought it might be worth checking to see if the Stop method was missing any of this logic, and I guess it was.

I believe an earlier change must have removed the call to reset, but I haven't looked into it.

reset seems like it was added in 0bfda44 (#9009) but never used.

Got it. I just saw that when reset was added it wasn't being called then either, looks like a stray change.

The important parts of reset were to set the state to uninitialized and set the provider to nil, and those were being handled directly in revokeLeadership.

It's definitely clearer to move those down into caManager.Stop() rather than spread around.

Reducing the coupling between Server and CAManager

This further decouples the CAManager from Server. It reduces the interface between them and removes the need for the SetLogger method on providers.

This method on Server was only used by the caDelegateWithState, so move it there until we can move it entirely into CAManager.

After moving ca.ConsulProviderStateDelegate into the interface we now have the ApplyCARequest method which does the same thing. Use this more specific method instead of raftApply.

and small refactor to getCAProvider so that GoLand is less confused about what it is doing. Previously it was reporting that the for condition was always true, which was not the case.

raftApply was removed so ApplyCARequest needs to handle all the possible operations Also set the providerShim to use the mock provider. other changes are small test improvements that were necessary to debug the failures.

freddygv

This mostly looks good to me, left a couple questions/comments.

The main one being the question about CAManager.reset()

agent/connect/ca/provider_aws_test.go

agent/connect/ca/provider_consul.go

agent/consul/leader_connect_ca.go

freddygv · 2021-07-12T17:11:47Z

agent/consul/server.go

@@ -473,7 +473,7 @@ func NewServer(config *Config, flat Deps) (*Server, error) {
 		return nil, fmt.Errorf("Failed to start Raft: %v", err)
 	}

-	s.caManager = NewCAManager(&caDelegateWithState{s}, s.leaderRoutineManager, s.loggers.Named(logging.Connect), s.config)
+	s.caManager = NewCAManager(&caDelegateWithState{s}, s.leaderRoutineManager, s.logger.ResetNamed("connect.ca"), s.config)


There's a logging.CA constant, "ca", that could be used here. I'm not sure how much value is added by the "connect." prefix.

This was to keep it consistent with the previous logger name.

IMO, the logging constants are not really valuable. I think we should aim to only create a logger with a given name in one place, which makes the name constants in some far away package more of a nuisance than something helpful. If we want a constant it could be un-exported in the local package.

freddygv · 2021-07-12T17:12:48Z

agent/consul/leader_connect_ca.go

-	c.primaryRoots = structs.IndexedCARoots{}
-	c.actingSecondaryCA = false


How was reset() being used originally? I can't find where it was being called from

Adds a contructor for the one provider that did not have one.

dnephin · 2021-07-12T18:05:30Z

Pushed another commit which makes the provider constructors more consistent.

freddygv

LGTM, solid improvement!

hc-github-team-consul-core · 2021-07-12T19:27:10Z

🍒 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/407297.

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 labels Jun 21, 2021

dnephin marked this pull request as draft June 22, 2021 00:16

dnephin force-pushed the dnephin/ca-provider-explore branch from 1d6d6a9 to 1d69937 Compare June 22, 2021 21:42

vercel bot temporarily deployed to Preview – consul-ui-staging June 22, 2021 21:42 Inactive

vercel bot temporarily deployed to Preview – consul June 22, 2021 21:42 Inactive

dnephin commented Jun 22, 2021

View reviewed changes

dnephin marked this pull request as ready for review June 22, 2021 22:31

dnephin mentioned this pull request Jun 23, 2021

ca: move Server.SignIntermediate to CAManager #10479

Merged

dnephin requested a review from a team June 30, 2021 18:05

dnephin commented Jun 30, 2021

View reviewed changes

dnephin mentioned this pull request Jul 5, 2021

connect/ca: ensure edits to the key type/bits for the connect builtin CA will regenerate the roots #10330

Merged

dnephin and others added 7 commits July 12, 2021 09:27

ca-manager: move provider shutdown into CAManager

b187766

Reducing the coupling between Server and CAManager

ca: move provider creation into CAManager

fc14f5a

This further decouples the CAManager from Server. It reduces the interface between them and removes the need for the SetLogger method on providers.

ca: move generateCASignRequest to the delegate

1f4cdde

This method on Server was only used by the caDelegateWithState, so move it there until we can move it entirely into CAManager.

ca: remove raftApply from delegate interface

1960c71

After moving ca.ConsulProviderStateDelegate into the interface we now have the ApplyCARequest method which does the same thing. Use this more specific method instead of raftApply.

ca: remove unused method

7dae65c

and small refactor to getCAProvider so that GoLand is less confused about what it is doing. Previously it was reporting that the for condition was always true, which was not the case.

ca: fix mockCAServerDelegate to work with the new interface

6228c4a

raftApply was removed so ApplyCARequest needs to handle all the possible operations Also set the providerShim to use the mock provider. other changes are small test improvements that were necessary to debug the failures.

add missing state reset when stopping ca manager

0475378

dhiaayachi force-pushed the dnephin/ca-provider-explore branch from 1d69937 to 0475378 Compare July 12, 2021 13:33

vercel bot temporarily deployed to Preview – consul July 12, 2021 13:33 Inactive

vercel bot temporarily deployed to Preview – consul-ui-staging July 12, 2021 13:33 Inactive

freddygv reviewed Jul 12, 2021

View reviewed changes

ca: use provider constructors to be more consistent

bf292cb

Adds a contructor for the one provider that did not have one.

vercel bot temporarily deployed to Preview – consul-ui-staging July 12, 2021 18:04 Inactive

vercel bot temporarily deployed to Preview – consul July 12, 2021 18:04 Inactive

freddygv approved these changes Jul 12, 2021

View reviewed changes

dnephin merged commit b89ec82 into main Jul 12, 2021

dnephin deleted the dnephin/ca-provider-explore branch July 12, 2021 19:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ca: isolate more of the CA logic in CAManager #10445

ca: isolate more of the CA logic in CAManager #10445

dnephin commented Jun 21, 2021

dnephin commented Jun 22, 2021

dnephin Jun 22, 2021 •

edited

Loading

dnephin Jun 30, 2021

dhiaayachi Jul 12, 2021

freddygv Jul 12, 2021

dnephin Jul 12, 2021

dnephin Jul 12, 2021

freddygv Jul 12, 2021 •

edited

Loading

freddygv left a comment

freddygv Jul 12, 2021 •

edited

Loading

dnephin Jul 12, 2021

freddygv Jul 12, 2021

dnephin commented Jul 12, 2021

freddygv left a comment

hc-github-team-consul-core commented Jul 12, 2021

		c.primaryRoots = structs.IndexedCARoots{}
		c.actingSecondaryCA = false

ca: isolate more of the CA logic in CAManager #10445

ca: isolate more of the CA logic in CAManager #10445

Conversation

dnephin commented Jun 21, 2021

dnephin commented Jun 22, 2021

dnephin Jun 22, 2021 • edited Loading

Choose a reason for hiding this comment

dnephin Jun 30, 2021

Choose a reason for hiding this comment

dhiaayachi Jul 12, 2021

Choose a reason for hiding this comment

freddygv Jul 12, 2021

Choose a reason for hiding this comment

dnephin Jul 12, 2021

Choose a reason for hiding this comment

dnephin Jul 12, 2021

Choose a reason for hiding this comment

freddygv Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

freddygv left a comment

Choose a reason for hiding this comment

freddygv Jul 12, 2021 • edited Loading

Choose a reason for hiding this comment

dnephin Jul 12, 2021

Choose a reason for hiding this comment

freddygv Jul 12, 2021

Choose a reason for hiding this comment

dnephin commented Jul 12, 2021

freddygv left a comment

Choose a reason for hiding this comment

hc-github-team-consul-core commented Jul 12, 2021

dnephin Jun 22, 2021 •

edited

Loading

freddygv Jul 12, 2021 •

edited

Loading

freddygv Jul 12, 2021 •

edited

Loading