secrets: use explicit format when using secrets

Signed-off-by: Evan Hazlett <ejhazlett@gmail.com>

cli/command/service/create.go

@@ -39,6 +39,7 @@ func newCreateCommand(dockerCli *command.DockerCli) *cobra.Command {
 	flags.Var(&opts.mounts, flagMount, "Attach a filesystem mount to the service")
 	flags.Var(&opts.constraints, flagConstraint, "Placement constraints")
 	flags.Var(&opts.networks, flagNetwork, "Network attachments")
+	flags.Var(&opts.secrets, flagSecret, "Specify secrets to expose to the service")
 	flags.VarP(&opts.endpoint.ports, flagPublish, "p", "Publish a port as a node port")
 	flags.Var(&opts.groups, flagGroup, "Set one or more supplementary user groups for the container")
 	flags.Var(&opts.dns, flagDNS, "Set custom DNS servers")
@@ -59,7 +60,7 @@ func runCreate(dockerCli *command.DockerCli, opts *serviceOptions) error {
 	}
 
 	// parse and validate secrets
-	secrets, err := parseSecrets(apiClient, opts.secrets)
+	secrets, err := parseSecrets(apiClient, opts.secrets.Value())
 	if err != nil {
 		return err
 	}

cli/command/service/opts.go

@@ -1,7 +1,10 @@
 package service
 
 import (
+	"encoding/csv"
 	"fmt"
+	"os"
+	"path/filepath"
 	"strconv"
 	"strings"
 	"time"
@@ -139,6 +142,98 @@ func (f *floatValue) Value() float32 {
 	return float32(*f)
 }
 
+// SecretRequestSpec is a type for requesting secrets
+type SecretRequestSpec struct {
+	source string
+	target string
+	uid    string
+	gid    string
+	mode   os.FileMode
+}
+
+// SecretOpt is a Value type for parsing secrets
+type SecretOpt struct {
+	values []*SecretRequestSpec
+}
+
+// Set a new secret value
+func (o *SecretOpt) Set(value string) error {
+	csvReader := csv.NewReader(strings.NewReader(value))
+	fields, err := csvReader.Read()
+	if err != nil {
+		return err
+	}
+
+	spec := &SecretRequestSpec{
+		source: "",
+		target: "",
+		uid:    "0",
+		gid:    "0",
+		mode:   0444,
+	}
+
+	for _, field := range fields {
+		parts := strings.SplitN(field, "=", 2)
+		key := strings.ToLower(parts[0])
+
+		if len(parts) != 2 {
+			return fmt.Errorf("invalid field '%s' must be a key=value pair", field)
+		}
+
+		value := parts[1]
+		switch key {
+		case "source":
+			spec.source = value
+		case "target":
+			tDir, _ := filepath.Split(value)
+			if tDir != "" {
+				return fmt.Errorf("target must not have a path")
+			}
+			spec.target = value
+		case "uid":
+			spec.uid = value
+		case "gid":
+			spec.gid = value
+		case "mode":
+			m, err := strconv.ParseUint(value, 0, 32)
+			if err != nil {
+				return fmt.Errorf("invalid mode specified: %v", err)
+			}
+
+			spec.mode = os.FileMode(m)
+		default:
+			return fmt.Errorf("invalid field in secret request: %s", key)
+		}
+	}
+
+	if spec.source == "" {
+		return fmt.Errorf("source is required")
+	}
+
+	o.values = append(o.values, spec)
+	return nil
+}
+
+// Type returns the type of this option
+func (o *SecretOpt) Type() string {
+	return "secret"
+}
+
+// String returns a string repr of this option
+func (o *SecretOpt) String() string {
+	secrets := []string{}
+	for _, secret := range o.values {
+		repr := fmt.Sprintf("%s -> %s", secret.source, secret.target)
+		secrets = append(secrets, repr)
+	}
+	return strings.Join(secrets, ", ")
+}
+
+// Value returns the secret requests
+func (o *SecretOpt) Value() []*SecretRequestSpec {
+	return o.values
+}
+
 type updateOptions struct {
 	parallelism     uint64
 	delay           time.Duration
@@ -337,7 +432,7 @@ type serviceOptions struct {
 	logDriver logDriverOptions
 
 	healthcheck healthCheckOptions
-	secrets     []string
+	secrets     SecretOpt
 }
 
 func newServiceOptions() *serviceOptions {

cli/command/service/opts_test.go

@@ -1,6 +1,7 @@
 package service
 
 import (
+	"os"
 	"reflect"
 	"testing"
 	"time"
@@ -105,3 +106,47 @@ func TestHealthCheckOptionsToHealthConfigConflict(t *testing.T) {
 	_, err := opt.toHealthConfig()
 	assert.Error(t, err, "--no-healthcheck conflicts with --health-* options")
 }
+
+func TestSecretOptionsSimple(t *testing.T) {
+	var opt SecretOpt
+
+	testCase := "source=/foo,target=testing"
+	assert.NilError(t, opt.Set(testCase))
+
+	reqs := opt.Value()
+	assert.Equal(t, len(reqs), 1)
+	req := reqs[0]
+	assert.Equal(t, req.source, "/foo")
+	assert.Equal(t, req.target, "testing")
+}
+
+func TestSecretOptionsCustomUidGid(t *testing.T) {
+	var opt SecretOpt
+
+	testCase := "source=/foo,target=testing,uid=1000,gid=1001"
+	assert.NilError(t, opt.Set(testCase))
+
+	reqs := opt.Value()
+	assert.Equal(t, len(reqs), 1)
+	req := reqs[0]
+	assert.Equal(t, req.source, "/foo")
+	assert.Equal(t, req.target, "testing")
+	assert.Equal(t, req.uid, "1000")
+	assert.Equal(t, req.gid, "1001")
+}
+
+func TestSecretOptionsCustomMode(t *testing.T) {
+	var opt SecretOpt
+
+	testCase := "source=/foo,target=testing,uid=1000,gid=1001,mode=0444"
+	assert.NilError(t, opt.Set(testCase))
+
+	reqs := opt.Value()
+	assert.Equal(t, len(reqs), 1)
+	req := reqs[0]
+	assert.Equal(t, req.source, "/foo")
+	assert.Equal(t, req.target, "testing")
+	assert.Equal(t, req.uid, "1000")
+	assert.Equal(t, req.gid, "1001")
+	assert.Equal(t, req.mode, os.FileMode(0444))
+}

cli/command/service/parse.go

@@ -3,8 +3,6 @@ package service
 import (
 	"context"
 	"fmt"
-	"path/filepath"
-	"strings"
 
 	"github.com/docker/docker/api/types"
 	"github.com/docker/docker/api/types/filters"
@@ -12,61 +10,27 @@ import (
 	"github.com/docker/docker/client"
 )
 
-// parseSecretString parses the requested secret and returns the secret name
-// and target.  Expects format SECRET_NAME:TARGET
-func parseSecretString(secretString string) (string, string, error) {
-	tokens := strings.Split(secretString, ":")
-
-	secretName := strings.TrimSpace(tokens[0])
-	targetName := secretName
-
-	if secretName == "" {
-		return "", "", fmt.Errorf("invalid secret name provided")
-	}
-
-	if len(tokens) > 1 {
-		targetName = strings.TrimSpace(tokens[1])
-		if targetName == "" {
-			return "", "", fmt.Errorf("invalid presentation name provided")
-		}
-	}
-
-	// ensure target is a filename only; no paths allowed
-	tDir, _ := filepath.Split(targetName)
-	if tDir != "" {
-		return "", "", fmt.Errorf("target must not have a path")
-	}
-
-	return secretName, targetName, nil
-}
-
 // parseSecrets retrieves the secrets from the requested names and converts
 // them to secret references to use with the spec
-func parseSecrets(client client.APIClient, requestedSecrets []string) ([]*swarmtypes.SecretReference, error) {
+func parseSecrets(client client.APIClient, requestedSecrets []*SecretRequestSpec) ([]*swarmtypes.SecretReference, error) {
 	secretRefs := make(map[string]*swarmtypes.SecretReference)
 	ctx := context.Background()
 
 	for _, secret := range requestedSecrets {
-		n, t, err := parseSecretString(secret)
-		if err != nil {
-			return nil, err
-		}
-
 		secretRef := &swarmtypes.SecretReference{
-			SecretName: n,
-			// TODO (ehazlett): parse these from cli request
+			SecretName: secret.source,
 			Target: swarmtypes.SecretReferenceFileTarget{
-				Name: t,
-				UID:  "0",
-				GID:  "0",
-				Mode: 0444,
+				Name: secret.target,
+				UID:  secret.uid,
+				GID:  secret.gid,
+				Mode: secret.mode,
 			},
 		}
 
-		if _, exists := secretRefs[t]; exists {
-			return nil, fmt.Errorf("duplicate secret target for %s not allowed", n)
+		if _, exists := secretRefs[secret.target]; exists {
+			return nil, fmt.Errorf("duplicate secret target for %s not allowed", secret.source)
 		}
-		secretRefs[t] = secretRef
+		secretRefs[secret.target] = secretRef
 	}
 
 	args := filters.NewArgs()

cli/command/service/update.go

@@ -56,7 +56,7 @@ func newUpdateCommand(dockerCli *command.DockerCli) *cobra.Command {
 	flags.Var(&opts.containerLabels, flagContainerLabelAdd, "Add or update a container label")
 	flags.Var(&opts.env, flagEnvAdd, "Add or update an environment variable")
 	flags.Var(newListOptsVar(), flagSecretRemove, "Remove a secret")
-	flags.StringSliceVar(&opts.secrets, flagSecretAdd, []string{}, "Add a secret")
+	flags.Var(&opts.secrets, flagSecretAdd, "Add or update a secret on a service")
 	flags.Var(&opts.mounts, flagMountAdd, "Add or update a mount on a service")
 	flags.Var(&opts.constraints, flagConstraintAdd, "Add or update a placement constraint")
 	flags.Var(&opts.endpoint.ports, flagPublishAdd, "Add or update a published port")
@@ -413,10 +413,7 @@ func updateEnvironment(flags *pflag.FlagSet, field *[]string) {
 
 func getUpdatedSecrets(apiClient client.APIClient, flags *pflag.FlagSet, secrets []*swarm.SecretReference) ([]*swarm.SecretReference, error) {
 	if flags.Changed(flagSecretAdd) {
-		values, err := flags.GetStringSlice(flagSecretAdd)
-		if err != nil {
-			return nil, err
-		}
+		values := flags.Lookup(flagSecretAdd).Value.(*SecretOpt).Value()
 
 		addSecrets, err := parseSecrets(apiClient, values)
 		if err != nil {

docs/reference/commandline/service_create.md

@@ -126,7 +126,7 @@ Use the `--secret` flag to give a container access to a
 with two secrets named `ssh-key` and `app-key`:
 
 ```bash
-$ docker service create --name redis --secret ssh-key:ssh --secret app-key:app redis:3.0.6
+$ docker service create --name redis --secret source=ssh-key,target=ssh --secret source=app-key,target=app,uid=1000,gid=1001,mode=0400 redis:3.0.6
 4cdgfyky7ozwh3htjfw0d12qv
 ```
 

docs/reference/commandline/service_update.md

@@ -157,7 +157,7 @@ The following example adds a secret named `ssh-2` and removes `ssh-1`:
 
 ```bash
 $ docker service update \
-    --secret-add ssh-2 \
+    --secret-add source=ssh-2,target=ssh-2 \
     --secret-rm ssh-1 \
     myservice
 ```

integration-cli/docker_cli_service_create_test.go

@@ -59,7 +59,7 @@ func (s *DockerSwarmSuite) TestServiceCreateWithSecret(c *check.C) {
 	c.Assert(id, checker.Not(checker.Equals), "", check.Commentf("secrets: %s", id))
 	testTarget := "testing"
 
-	out, err := d.Cmd("service", "create", "--name", serviceName, "--secret", fmt.Sprintf("%s:%s", testName, testTarget), "busybox", "top")
+	out, err := d.Cmd("service", "create", "--name", serviceName, "--secret", fmt.Sprintf("source=%s,target=%s", testName, testTarget), "busybox", "top")
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 
 	out, err = d.Cmd("service", "inspect", "--format", "{{ json .Spec.TaskTemplate.ContainerSpec.Secrets }}", serviceName)

integration-cli/docker_cli_service_update_test.go

@@ -103,7 +103,7 @@ func (s *DockerSwarmSuite) TestServiceUpdateSecrets(c *check.C) {
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 
 	// add secret
-	out, err = d.Cmd("service", "update", "test", "--secret-add", fmt.Sprintf("%s:%s", testName, testTarget))
+	out, err = d.Cmd("service", "update", "test", "--secret-add", fmt.Sprintf("source=%s,target=%s", testName, testTarget))
 	c.Assert(err, checker.IsNil, check.Commentf(out))
 
 	out, err = d.Cmd("service", "inspect", "--format", "{{ json .Spec.TaskTemplate.ContainerSpec.Secrets }}", serviceName)