Merge pull request #213 from dnephin/improve-swarm-ca-cmd

Refactor and UI changes to `swarm ca` command

cli/command/swarm/ca.go

@@ -3,23 +3,22 @@ package swarm
 import (
 	"fmt"
 	"io"
-	"strings"
-
-	"golang.org/x/net/context"
-
 	"io/ioutil"
+	"strings"
 
 	"github.com/docker/cli/cli"
 	"github.com/docker/cli/cli/command"
 	"github.com/docker/cli/cli/command/swarm/progress"
 	"github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/pkg/jsonmessage"
+	"github.com/pkg/errors"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
+	"golang.org/x/net/context"
 )
 
 type caOptions struct {
-	swarmOptions
+	swarmCAOptions
 	rootCACert PEMFile
 	rootCAKey  PEMFile
 	rotate     bool
@@ -27,21 +26,21 @@ type caOptions struct {
 	quiet      bool
 }
 
-func newRotateCACommand(dockerCli command.Cli) *cobra.Command {
+func newCACommand(dockerCli command.Cli) *cobra.Command {
 	opts := caOptions{}
 
 	cmd := &cobra.Command{
 		Use:   "ca [OPTIONS]",
-		Short: "Manage root CA",
+		Short: "Display and rotate the root CA",
 		Args:  cli.NoArgs,
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return runRotateCA(dockerCli, cmd.Flags(), opts)
+			return runCA(dockerCli, cmd.Flags(), opts)
 		},
 		Tags: map[string]string{"version": "1.30"},
 	}
 
 	flags := cmd.Flags()
-	addSwarmCAFlags(flags, &opts.swarmOptions)
+	addSwarmCAFlags(flags, &opts.swarmCAOptions)
 	flags.BoolVar(&opts.rotate, flagRotate, false, "Rotate the swarm CA - if no certificate or key are provided, new ones will be generated")
 	flags.Var(&opts.rootCACert, flagCACert, "Path to the PEM-formatted root CA certificate to use for the new cluster")
 	flags.Var(&opts.rootCAKey, flagCAKey, "Path to the PEM-formatted root CA key to use for the new cluster")
@@ -51,7 +50,7 @@ func newRotateCACommand(dockerCli command.Cli) *cobra.Command {
 	return cmd
 }
 
-func runRotateCA(dockerCli command.Cli, flags *pflag.FlagSet, opts caOptions) error {
+func runCA(dockerCli command.Cli, flags *pflag.FlagSet, opts caOptions) error {
 	client := dockerCli.Client()
 	ctx := context.Background()
 
@@ -66,31 +65,10 @@ func runRotateCA(dockerCli command.Cli, flags *pflag.FlagSet, opts caOptions) er
 				return fmt.Errorf("`--%s` flag requires the `--rotate` flag to update the CA", f)
 			}
 		}
-		if swarmInspect.ClusterInfo.TLSInfo.TrustRoot == "" {
+		return displayTrustRoot(dockerCli.Out(), swarmInspect)
-			fmt.Fprintln(dockerCli.Out(), "No CA information available")
-		} else {
-			fmt.Fprintln(dockerCli.Out(), strings.TrimSpace(swarmInspect.ClusterInfo.TLSInfo.TrustRoot))
-		}
-		return nil
-	}
-
-	genRootCA := true
-	spec := &swarmInspect.Spec
-	opts.mergeSwarmSpec(spec, flags) // updates the spec given the cert expiry or external CA flag
-	if flags.Changed(flagCACert) {
-		spec.CAConfig.SigningCACert = opts.rootCACert.Contents()
-		genRootCA = false
-	}
-	if flags.Changed(flagCAKey) {
-		spec.CAConfig.SigningCAKey = opts.rootCAKey.Contents()
-		genRootCA = false
-	}
-	if genRootCA {
-		spec.CAConfig.ForceRotate++
-		spec.CAConfig.SigningCACert = ""
-		spec.CAConfig.SigningCAKey = ""
 	}
 
+	updateSwarmSpec(&swarmInspect.Spec, flags, opts)
 	if err := client.SwarmUpdate(ctx, swarmInspect.Version, swarmInspect.Spec, swarm.UpdateFlags{}); err != nil {
 		return err
 	}
@@ -98,7 +76,29 @@ func runRotateCA(dockerCli command.Cli, flags *pflag.FlagSet, opts caOptions) er
 	if opts.detach {
 		return nil
 	}
+	return attach(ctx, dockerCli, opts)
+}
 
+func updateSwarmSpec(spec *swarm.Spec, flags *pflag.FlagSet, opts caOptions) {
+	opts.mergeSwarmSpecCAFlags(spec, flags)
+	caCert := opts.rootCACert.Contents()
+	caKey := opts.rootCAKey.Contents()
+
+	if caCert != "" {
+		spec.CAConfig.SigningCACert = caCert
+	}
+	if caKey != "" {
+		spec.CAConfig.SigningCAKey = caKey
+	}
+	if caKey == "" && caCert == "" {
+		spec.CAConfig.ForceRotate++
+		spec.CAConfig.SigningCACert = ""
+		spec.CAConfig.SigningCAKey = ""
+	}
+}
+
+func attach(ctx context.Context, dockerCli command.Cli, opts caOptions) error {
+	client := dockerCli.Client()
 	errChan := make(chan error, 1)
 	pipeReader, pipeWriter := io.Pipe()
 
@@ -111,7 +111,7 @@ func runRotateCA(dockerCli command.Cli, flags *pflag.FlagSet, opts caOptions) er
 		return <-errChan
 	}
 
-	err = jsonmessage.DisplayJSONMessagesToStream(pipeReader, dockerCli.Out(), nil)
+	err := jsonmessage.DisplayJSONMessagesToStream(pipeReader, dockerCli.Out(), nil)
 	if err == nil {
 		err = <-errChan
 	}
@@ -119,15 +119,17 @@ func runRotateCA(dockerCli command.Cli, flags *pflag.FlagSet, opts caOptions) er
 		return err
 	}
 
-	swarmInspect, err = client.SwarmInspect(ctx)
+	swarmInspect, err := client.SwarmInspect(ctx)
 	if err != nil {
 		return err
 	}
+	return displayTrustRoot(dockerCli.Out(), swarmInspect)
+}
 
-	if swarmInspect.ClusterInfo.TLSInfo.TrustRoot == "" {
+func displayTrustRoot(out io.Writer, info swarm.Swarm) error {
-		fmt.Fprintln(dockerCli.Out(), "No CA information available")
+	if info.ClusterInfo.TLSInfo.TrustRoot == "" {
-	} else {
+		return errors.New("No CA information available")
-		fmt.Fprintln(dockerCli.Out(), strings.TrimSpace(swarmInspect.ClusterInfo.TLSInfo.TrustRoot))
 	}
+	fmt.Fprintln(out, strings.TrimSpace(info.ClusterInfo.TLSInfo.TrustRoot))
 	return nil
 }

cli/command/swarm/ca_test.go

@@ -0,0 +1,88 @@
+package swarm
+
+import (
+	"bytes"
+	"testing"
+	"time"
+
+	"github.com/docker/docker/api/types/swarm"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+func swarmSpecWithFullCAConfig() *swarm.Spec {
+	return &swarm.Spec{
+		CAConfig: swarm.CAConfig{
+			SigningCACert:  "cacert",
+			SigningCAKey:   "cakey",
+			ForceRotate:    1,
+			NodeCertExpiry: time.Duration(200),
+			ExternalCAs: []*swarm.ExternalCA{
+				{
+					URL:      "https://example.com/ca",
+					Protocol: swarm.ExternalCAProtocolCFSSL,
+					CACert:   "excacert",
+				},
+			},
+		},
+	}
+}
+
+func TestDisplayTrustRootNoRoot(t *testing.T) {
+	buffer := new(bytes.Buffer)
+	err := displayTrustRoot(buffer, swarm.Swarm{})
+	assert.EqualError(t, err, "No CA information available")
+}
+
+func TestDisplayTrustRoot(t *testing.T) {
+	buffer := new(bytes.Buffer)
+	trustRoot := "trustme"
+	err := displayTrustRoot(buffer, swarm.Swarm{
+		ClusterInfo: swarm.ClusterInfo{
+			TLSInfo: swarm.TLSInfo{TrustRoot: trustRoot},
+		},
+	})
+	require.NoError(t, err)
+	assert.Equal(t, trustRoot+"\n", buffer.String())
+}
+
+func TestUpdateSwarmSpecDefaultRotate(t *testing.T) {
+	spec := swarmSpecWithFullCAConfig()
+	flags := newCACommand(nil).Flags()
+	updateSwarmSpec(spec, flags, caOptions{})
+
+	expected := swarmSpecWithFullCAConfig()
+	expected.CAConfig.ForceRotate = 2
+	expected.CAConfig.SigningCACert = ""
+	expected.CAConfig.SigningCAKey = ""
+	assert.Equal(t, expected, spec)
+}
+
+func TestUpdateSwarmSpecPartial(t *testing.T) {
+	spec := swarmSpecWithFullCAConfig()
+	flags := newCACommand(nil).Flags()
+	updateSwarmSpec(spec, flags, caOptions{
+		rootCACert: PEMFile{contents: "cacert"},
+	})
+
+	expected := swarmSpecWithFullCAConfig()
+	expected.CAConfig.SigningCACert = "cacert"
+	assert.Equal(t, expected, spec)
+}
+
+func TestUpdateSwarmSpecFullFlags(t *testing.T) {
+	flags := newCACommand(nil).Flags()
+	flags.Lookup(flagCertExpiry).Changed = true
+	spec := swarmSpecWithFullCAConfig()
+	updateSwarmSpec(spec, flags, caOptions{
+		rootCACert:     PEMFile{contents: "cacert"},
+		rootCAKey:      PEMFile{contents: "cakey"},
+		swarmCAOptions: swarmCAOptions{nodeCertExpiry: 3 * time.Minute},
+	})
+
+	expected := swarmSpecWithFullCAConfig()
+	expected.CAConfig.SigningCACert = "cacert"
+	expected.CAConfig.SigningCAKey = "cakey"
+	expected.CAConfig.NodeCertExpiry = 3 * time.Minute
+	assert.Equal(t, expected, spec)
+}

cli/command/swarm/cmd.go

@@ -25,7 +25,7 @@ func NewSwarmCommand(dockerCli command.Cli) *cobra.Command {
 		newUpdateCommand(dockerCli),
 		newLeaveCommand(dockerCli),
 		newUnlockCommand(dockerCli),
-		newRotateCACommand(dockerCli),
+		newCACommand(dockerCli),
 	)
 	return cmd
 }

cli/command/swarm/opts.go

@@ -36,10 +36,9 @@ const (
 )
 
 type swarmOptions struct {
+	swarmCAOptions
 	taskHistoryLimit    int64
 	dispatcherHeartbeat time.Duration
-	nodeCertExpiry      time.Duration
-	externalCA          ExternalCAOption
 	maxSnapshots        uint64
 	snapshotInterval    uint64
 	autolock            bool
@@ -216,7 +215,7 @@ func parseExternalCA(caSpec string) (*swarm.ExternalCA, error) {
 	return &externalCA, nil
 }
 
-func addSwarmCAFlags(flags *pflag.FlagSet, opts *swarmOptions) {
+func addSwarmCAFlags(flags *pflag.FlagSet, opts *swarmCAOptions) {
 	flags.DurationVar(&opts.nodeCertExpiry, flagCertExpiry, 90*24*time.Hour, "Validity period for node certificates (ns|us|ms|s|m|h)")
 	flags.Var(&opts.externalCA, flagExternalCA, "Specifications of one or more certificate signing endpoints")
 }
@@ -228,7 +227,7 @@ func addSwarmFlags(flags *pflag.FlagSet, opts *swarmOptions) {
 	flags.SetAnnotation(flagMaxSnapshots, "version", []string{"1.25"})
 	flags.Uint64Var(&opts.snapshotInterval, flagSnapshotInterval, 10000, "Number of log entries between Raft snapshots")
 	flags.SetAnnotation(flagSnapshotInterval, "version", []string{"1.25"})
-	addSwarmCAFlags(flags, opts)
+	addSwarmCAFlags(flags, &opts.swarmCAOptions)
 }
 
 func (opts *swarmOptions) mergeSwarmSpec(spec *swarm.Spec, flags *pflag.FlagSet) {
@@ -238,12 +237,6 @@ func (opts *swarmOptions) mergeSwarmSpec(spec *swarm.Spec, flags *pflag.FlagSet)
 	if flags.Changed(flagDispatcherHeartbeat) {
 		spec.Dispatcher.HeartbeatPeriod = opts.dispatcherHeartbeat
 	}
-	if flags.Changed(flagCertExpiry) {
-		spec.CAConfig.NodeCertExpiry = opts.nodeCertExpiry
-	}
-	if flags.Changed(flagExternalCA) {
-		spec.CAConfig.ExternalCAs = opts.externalCA.Value()
-	}
 	if flags.Changed(flagMaxSnapshots) {
 		spec.Raft.KeepOldSnapshots = &opts.maxSnapshots
 	}
@@ -253,6 +246,21 @@ func (opts *swarmOptions) mergeSwarmSpec(spec *swarm.Spec, flags *pflag.FlagSet)
 	if flags.Changed(flagAutolock) {
 		spec.EncryptionConfig.AutoLockManagers = opts.autolock
 	}
+	opts.mergeSwarmSpecCAFlags(spec, flags)
+}
+
+type swarmCAOptions struct {
+	nodeCertExpiry time.Duration
+	externalCA     ExternalCAOption
+}
+
+func (opts *swarmCAOptions) mergeSwarmSpecCAFlags(spec *swarm.Spec, flags *pflag.FlagSet) {
+	if flags.Changed(flagCertExpiry) {
+		spec.CAConfig.NodeCertExpiry = opts.nodeCertExpiry
+	}
+	if flags.Changed(flagExternalCA) {
+		spec.CAConfig.ExternalCAs = opts.externalCA.Value()
+	}
 }
 
 func (opts *swarmOptions) ToSpec(flags *pflag.FlagSet) swarm.Spec {