-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Synchronous CLI command for root CA rotation #48
Conversation
Have re-vendored the latest version of |
Looking at this again, I wonder if;
Happy to hear thoughts @aaronlehmann @diogomonica @dnephin |
I originally thought about just doing The one argument I'd have for a separate command is that
Some of this info is currently on
Happy to do that as well. |
I have a slight preference for having the ca subcommand |
LGTM |
Updated the |
LGTM |
cli/command/swarm/ca.go
Outdated
go func() { | ||
for { | ||
var buf [1024]byte | ||
if _, err := pipeReader.Read(buf[:]); err != nil { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: this could be simpler with ioutil.Discard
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Updated with io.Copy(ioutil.Discard, pipeReader)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need a for
or function wrapper anymore.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left one comment, LGTM otherwise
Args: cli.NoArgs, | ||
RunE: func(cmd *cobra.Command, args []string) error { | ||
return runRotateCA(dockerCli, cmd.Flags(), opts) | ||
}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add
Tags: map[string]string{"version": "1.30"},
like we have on cli/command/system/prune.go#L31, so that the command is hidden when talking to an older daemon, and produces an error that it requires a newer daemon
…t certificate. Signed-off-by: Ying Li <ying.li@docker.com>
Signed-off-by: Ying Li <ying.li@docker.com>
Signed-off-by: Aaron Lehmann <aaron.lehmann@docker.com>
@tonistiigi: Pushed a commit that simplifies the use of |
LGTM |
Thanks @aaronlehmann! |
Hi folks - Training wants to highlight this feature in our workshops, but I'm a bit confused; |
@BillMills Ah, apologies, that was intended to be used with the Wondering if we should update the flags, if used by themselves, to specify that they are ignored unless |
A warning would be great. We might even want to consider it an error. |
Provide command line tool to view and rotate swarm's currently CA root certificate.
Signed-off-by: Ying Li ying.li@docker.com
This code was originally in moby/moby#32993.
Sample progress bar here, copied styling from the synchronous service create progress bar: https://asciinema.org/a/2em3i7y1u50ajeut13es5m6uh
Note: I originally thought about just doing
docker swarm update --rotate-ca cert=/path/...,key=/path/...
but I wasn't sure how to also do tell the CLI to just generate a cert and key - it'd probably have to look likedocker swarm update --rotate-ca ""
which seems weird, hence the new command.Also a suggestion from @NathanMcCauley: if we have service identities, and those could be rotated, do we want to use the same CLI command to view/rotate those as well?