ca: all managers will update their security configs when cluster root CA updated by cyli · Pull Request #1984 · moby/swarmkit

cyli · 2017-02-24T05:23:45Z

Rather than just the leader that is running the CA server. This way on root rotation, all managers will get root changes right away.

#1983 will actually update the TLS creds with the new root pool when the RootCA is updated

codecov-io · 2017-02-24T05:33:01Z

Codecov Report

Merging #1984 into master will increase coverage by 0.11%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master    #1984      +/-   ##
==========================================
+ Coverage   53.61%   53.72%   +0.11%     
==========================================
  Files         109      109              
  Lines       18919    18923       +4     
==========================================
+ Hits        10144    10167      +23     
+ Misses       7541     7520      -21     
- Partials     1234     1236       +2

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 46bbd41...4d0ba73. Read the comment docs.

aaronlehmann · 2017-02-24T18:30:57Z

 	<-done
 }
+
+// If the root CA materia is updated in the memory store, a manager will update its own


material

(I don't get any pleasure out of pointing out typos, but if I don't do it, someone else will open a PR, and I prefer to preempt those.)

I appreciate it! I tend to misspell a bunch of stuff. I wonder if we should add github.com/client9/misspell/cmd/misspell to the linter?

I don't know what the false positive rate is, but I guess it can't hurt to try.

Looks like it wouldn't have caught this one anyway, I guess because materia is a real word, although latin).

aaronlehmann · 2017-02-24T19:09:51Z


+	// skipAutoUpdateRootCA, if set to true, causes the CA server to skip automatically updating
+	// the signing credentials based on changes in the memory store
+	skipAutoUpdateRootCA bool


The auto-update functionality seems like it's only used for test cases. I'm curious how difficult/ugly it would be to completely remove this.

We can add a cluster watch functionality to the test CA server. I put this in mainly because it seemed nice for the CA server to a be completely self-sufficient server, if necessary, if someone wanted to run one independently of the manager (which I agree, we are only only for tests, but the CA server would not be as usable otherwise).

I'll trust your judgement on this. I didn't like having an option that's never enabled by the actual program, but it's basically enabling or disabling a function call, and the alternative is having the test harness do significantly more.

Another option I was considering was to have a separate startable goroutine on the server that would watch the memory store and auto-update the RootCA. Then whomever is responsible for starting it would have to cancel it as well. Would this be more preferable? But again, this would only be used for the tests.

I still think it's better to move the s.UpdateRootCA call out of the CA server code, but I won't block the PR on this.

Ok, I don't feel super strongly about my position - it's more of a preference, so happy to address this.

aaronlehmann · 2017-02-24T21:11:51Z

Another option I was considering was to have a separate startable goroutine on the server that would watch the memory store and auto-update the RootCA. Then whomever is responsible for starting it would have to cancel it as well. Would this be more preferable? But again, this would only be used for the tests.

Not to me.

…heir SecurityConfig's RootCA upon a change in the cluster's RootCA configuration. Signed-off-by: cyli <ying.li@docker.com>

aaronlehmann · 2017-02-28T00:29:42Z

+	if err := ca.UpdateSecurityConfigFromCluster(ctx, m.config.SecurityConfig, cluster); err != nil {
+		log.G(ctx).WithError(err).Error("updating Root CA failed")
+	}
+	m.caserver.UpdateJoinTokens(ctx, cluster)


It seems weird for a function called updateRootCA to be updating join tokens.

Would a rename to something more appropriate be ok, or would you suggest having 2 watches on the cluster, one to update the root CA and one to update the join tokens? My preference is for the first, to make sure that both are updated in sync.

Just want a rename

aaronlehmann · 2017-02-28T00:31:57Z


+// UpdateSecurityConfigFromCluster takes an api.Cluster object and updates the security config based on the changes
+// in the RootCA and ExternalURLs from that cluster object.
+func UpdateSecurityConfigFromCluster(ctx context.Context, s *SecurityConfig, cluster *api.Cluster) error {


Maybe just UpdateSecurityConfig?

Would it make sense to make it a method on (*SecurityConfig)? and call it Update?

…entirely Signed-off-by: cyli <ying.li@docker.com>

aaronlehmann · 2017-02-28T02:12:03Z

LGTM

aaronlehmann · 2017-03-01T00:01:56Z

ping @diogomonica

cyli force-pushed the managers-update-root-ca branch from 36ee1c9 to f66156e Compare February 24, 2017 05:25

cyli changed the title ~~ca: all managers will update their security configs when cluster root CA updated~~ WIP: ca: all managers will update their security configs when cluster root CA updated Feb 24, 2017

cyli force-pushed the managers-update-root-ca branch from f66156e to 5e0ea96 Compare February 24, 2017 18:23

cyli changed the title ~~WIP: ca: all managers will update their security configs when cluster root CA updated~~ ca: all managers will update their security configs when cluster root CA updated Feb 24, 2017

aaronlehmann reviewed Feb 24, 2017

View reviewed changes

cyli force-pushed the managers-update-root-ca branch 2 times, most recently from 143ceac to 747ed31 Compare February 27, 2017 22:43

All managers, not just the current leader running a CA, will update t…

dfa04bb

…heir SecurityConfig's RootCA upon a change in the cluster's RootCA configuration. Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the managers-update-root-ca branch from 747ed31 to 7d8f54f Compare February 27, 2017 22:43

cyli mentioned this pull request Feb 28, 2017

[feature] CA Root rotation #1990

Closed

10 tasks

aaronlehmann reviewed Feb 28, 2017

View reviewed changes

Move the cluster watch that updates the root CA out of the CA server …

4d0ba73

…entirely Signed-off-by: cyli <ying.li@docker.com>

cyli force-pushed the managers-update-root-ca branch from 7d8f54f to 4d0ba73 Compare February 28, 2017 00:59

diogomonica merged commit 25387c0 into moby:master Mar 1, 2017

cyli deleted the managers-update-root-ca branch March 2, 2017 07:16

Conversation

cyli commented Feb 24, 2017

Uh oh!

codecov-io commented Feb 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Feb 24, 2017 via email

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented Feb 28, 2017

Uh oh!

aaronlehmann commented Mar 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

codecov-io commented Feb 24, 2017 •

edited

Loading