If docker swarm ca is not called with the --rotate flag, warn if other flags are passed - #207
Conversation
| if !opts.rotate { | ||
| if flags.Changed(flagCACert) || flags.Changed(flagCAKey) || flags.Changed(flagCertExpiry) { | ||
| fmt.Fprintln(dockerCli.Out(), "Warning: because the `--rotate` flag was not provided, all other flags will be ignored") | ||
| } |
There was a problem hiding this comment.
Any reason not to make this an error instead of a warning?
If this remains a warning it should be on stderr (dockerCli.Err()).
If it remains a warning maybe this could be split out into a separate function and we can print a warning for each flag? "all other flags" isn't really accurate because -d and -q are allowed, and we could always add other flags later.
Something like
"WARNING: Flag --%s ignored because, you must --rotate to update the CA"
There was a problem hiding this comment.
I notice flagExternalCA is not being checked. Is that right?
There was a problem hiding this comment.
@dnephin It just didn't particularly seem like an error to me, although I don't feel very strongly.
I notice
flagExternalCAis not being checked. Is that right?
Ah sorry, that is also being addressed by mergeSwarmSpec. I'll add that to the comment.
There was a problem hiding this comment.
As a user, if I run a command like
docker swarm ca --ca-cert foo
I think I'm expecting an update, but the command acts as basically a "print the current value". That feels like an error to me.
There was a problem hiding this comment.
I see, sure, I'll make that update.
Codecov Report
@@ Coverage Diff @@
## master #207 +/- ##
==========================================
- Coverage 45.75% 45.66% -0.09%
==========================================
Files 171 171
Lines 11527 11516 -11
==========================================
- Hits 5274 5259 -15
- Misses 5946 5950 +4
Partials 307 307 |
| for _, f := range []string{flagCACert, flagCAKey, flagCACert, flagExternalCA} { | ||
| if flags.Changed(f) { | ||
| return fmt.Errorf("`--%s` flag ignored because you must `--rotate` to update the CA", f) | ||
| } |
There was a problem hiding this comment.
I guess now that it's an error it's not ignored anymore. So maybe "--%s flag requires --rotate to update the CA" ?
flags, including cert expiry, will be ignored, so warn if a user attempts to use `docker swarm ca --cert-expiry` or something. Signed-off-by: Ying Li <ying.li@docker.com>
| } | ||
|
|
||
| if !opts.rotate { | ||
| for _, f := range []string{flagCACert, flagCAKey, flagCACert, flagExternalCA} { |
There was a problem hiding this comment.
@cyli flagCACert appears twice here. Is that correct?
There was a problem hiding this comment.
Ah you're right, it's duplicated. Will update.
7 participants
If
docker swarm cais not called with the--rotateflag, the other flags, including cert expiry, will be ignored, so warn if a user attempts to usedocker swarm ca --cert-expiryor something.See #48 (comment).