Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

WIP: [cli] Interactive root rotation #2104

Closed
wants to merge 5 commits into from
Closed

Conversation

cyli
Copy link
Contributor

@cyli cyli commented Apr 7, 2017

This is based on moby/moby#31144.

Todo:

  • what should the command line flags for accepting a cert and key, and accepting a cert + external CAs look like?
  • should we be adding the progress bar to cluster inspect or add a new command to resume checking root CA progress?

Basic root rotation: https://asciinema.org/a/ddsnrnzob16v3ampmbqjf3j55
Root rotation when there's another root rotation: https://asciinema.org/a/19ah16sg29it0fyeellflpe68

This is stacked on top of #2100

cc @diogomonica @aaronlehmann

cyli added 5 commits April 6, 2017 17:53
…es all the nodes

to have an IssuanceStateRotate to trigger all the nodes to get new certificates.

When all the nodes have rotated their certificates to be signed by the desired issuer,
complete root rotation.

Signed-off-by: cyli <ying.li@docker.com>
Signed-off-by: cyli <ying.li@docker.com>
…ng the store for

all nodes at intervals, rely on the cluster and node watches to update an in-memory mapping
of the current nodes.  At regular intervals, update the store to tell a throttled number
of the unconverged nodes to rotate their certificates.

Also, remove the leader rotation part of the root rotation integration test, as that
takes a very long time. There are server tests to ensure that multiple CA servers
running reconciliation loops, and starting a CA server from a stopped state, does not
break root reconciliation.

Signed-off-by: cyli <ying.li@docker.com>
Signed-off-by: cyli <ying.li@docker.com>
@aaronlehmann
Copy link
Collaborator

This is neat but I'm not sure about putting fancy features into swarmctl. Shouldn't we be focusing our energy on the docker CLI?

I'm a bit confused about the long-term future of swarmctl. I know there are efforts to make it work for "stand alone swarmd", but it seems strange to fragment our efforts across two CLIs. What do we gain by having two CLIs?

cc @aluzzardi

@cyli
Copy link
Contributor Author

cyli commented Apr 7, 2017

@aaronlehmann Makes sense - I interpreted swarmctl as more of a 'trial run for the docker CLI'. Happy to just close this and add it to docker instead.

@thaJeztah
Copy link
Member

From a UX perspective looking at the example recordings ;

  • I missed feedback why the progressbar "hangs" (node unavailable, other rotation in progress). Without this, we can expect bugreports like "bug: ca rotation hangs"
  • What happens if you cancel the action (^C)? (e.g. in the example where one node was down); does it roll back?

@aaronlehmann
Copy link
Collaborator

I think it is worth having a --rotate-ca option in swarmctl, and displaying the TLS info in inspect, but I'd vote against having interactive progress in swarmctl, at least for now. Among other things it looks like it would involve vendoring some Docker CLI stuff that I don't really want to bring in.

IMHO the priority should be getting this working end-to-end in the Docker CLI. Hopefully the progress indicator can be ported over there without too much trouble.

@cyli
Copy link
Contributor Author

cyli commented Apr 7, 2017

What happens if you cancel the action (^C)? (e.g. in the example where one node was down); does it roll back?

@thaJeztah No, it detaches from waiting, and the root rotation goes on (or not) as it was.

@cyli
Copy link
Contributor Author

cyli commented Apr 7, 2017

If the priority is just the docker CLI, I'll abandon this one and just open the docker one, if that's ok. We can come back and add stuff to swarmctl later.

@cyli cyli mentioned this pull request Apr 10, 2017
10 tasks
@diogomonica
Copy link
Contributor

SGTM

@cyli cyli closed this Apr 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants