Merge pull request #1781 from dperny/swarm-credentialspec

Support using swarm Configs as CredentialSpecs in Services.

cli/command/service/create.go

@@ -8,7 +8,9 @@ import (
 	"github.com/docker/cli/cli/command"
 	cliopts "github.com/docker/cli/opts"
 	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/swarm"
 	"github.com/docker/docker/api/types/versions"
+	"github.com/docker/docker/client"
 	"github.com/spf13/cobra"
 	"github.com/spf13/pflag"
 )
@@ -95,14 +97,8 @@ func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, opts *serviceOptions
 		service.TaskTemplate.ContainerSpec.Secrets = secrets
 	}
 
-	specifiedConfigs := opts.configs.Value()
-	if len(specifiedConfigs) > 0 {
-		// parse and validate configs
-		configs, err := ParseConfigs(apiClient, specifiedConfigs)
-		if err != nil {
-			return err
-		}
-		service.TaskTemplate.ContainerSpec.Configs = configs
+	if err := setConfigs(apiClient, &service, opts); err != nil {
+		return err
 	}
 
 	if err := resolveServiceImageDigestContentTrust(dockerCli, &service); err != nil {
@@ -141,3 +137,45 @@ func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, opts *serviceOptions
 
 	return waitOnService(ctx, dockerCli, response.ID, opts.quiet)
 }
+
+// setConfigs does double duty: it both sets the ConfigReferences of the
+// service, and it sets the service CredentialSpec. This is because there is an
+// interplay between the CredentialSpec and the Config it depends on.
+func setConfigs(apiClient client.ConfigAPIClient, service *swarm.ServiceSpec, opts *serviceOptions) error {
+	specifiedConfigs := opts.configs.Value()
+	// if the user has requested to use a Config, for the CredentialSpec add it
+	// to the specifiedConfigs as a RuntimeTarget.
+	if cs := opts.credentialSpec.Value(); cs != nil && cs.Config != "" {
+		specifiedConfigs = append(specifiedConfigs, &swarm.ConfigReference{
+			ConfigName: cs.Config,
+			Runtime:    &swarm.ConfigReferenceRuntimeTarget{},
+		})
+	}
+	if len(specifiedConfigs) > 0 {
+		// parse and validate configs
+		configs, err := ParseConfigs(apiClient, specifiedConfigs)
+		if err != nil {
+			return err
+		}
+		service.TaskTemplate.ContainerSpec.Configs = configs
+		// if we have a CredentialSpec Config, find its ID and rewrite the
+		// field on the spec
+		//
+		// we check the opts instead of the service directly because there are
+		// a few layers of nullable objects in the service, which is a PITA
+		// to traverse, but the existence of the option implies that those are
+		// non-null.
+		if cs := opts.credentialSpec.Value(); cs != nil && cs.Config != "" {
+			for _, config := range configs {
+				if config.ConfigName == cs.Config {
+					service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config = config.ConfigID
+					// we've found the right config, no need to keep iterating
+					// through the rest of them.
+					break
+				}
+			}
+		}
+	}
+
+	return nil
+}

cli/command/service/create_test.go

@@ -0,0 +1,271 @@
+package service
+
+import (
+	"context"
+	"testing"
+
+	"github.com/docker/docker/api/types"
+	"github.com/docker/docker/api/types/swarm"
+	"gotest.tools/assert"
+	is "gotest.tools/assert/cmp"
+
+	cliopts "github.com/docker/cli/opts"
+)
+
+// fakeConfigAPIClientList is used to let us pass a closure as a
+// ConfigAPIClient, to use as ConfigList. for all the other methods in the
+// interface, it does nothing, not even return an error, so don't use them
+type fakeConfigAPIClientList func(context.Context, types.ConfigListOptions) ([]swarm.Config, error)
+
+func (f fakeConfigAPIClientList) ConfigList(ctx context.Context, opts types.ConfigListOptions) ([]swarm.Config, error) {
+	return f(ctx, opts)
+}
+
+func (f fakeConfigAPIClientList) ConfigCreate(_ context.Context, _ swarm.ConfigSpec) (types.ConfigCreateResponse, error) {
+	return types.ConfigCreateResponse{}, nil
+}
+
+func (f fakeConfigAPIClientList) ConfigRemove(_ context.Context, _ string) error {
+	return nil
+}
+
+func (f fakeConfigAPIClientList) ConfigInspectWithRaw(_ context.Context, _ string) (swarm.Config, []byte, error) {
+	return swarm.Config{}, nil, nil
+}
+
+func (f fakeConfigAPIClientList) ConfigUpdate(_ context.Context, _ string, _ swarm.Version, _ swarm.ConfigSpec) error {
+	return nil
+}
+
+// TestSetConfigsWithCredSpecAndConfigs tests that the setConfigs function for
+// create correctly looks up the right configs, and correctly handles the
+// credentialSpec
+func TestSetConfigsWithCredSpecAndConfigs(t *testing.T) {
+	// we can't directly access the internal fields of the ConfigOpt struct, so
+	// we need to let it do the parsing
+	configOpt := &cliopts.ConfigOpt{}
+	configOpt.Set("bar")
+	opts := &serviceOptions{
+		credentialSpec: credentialSpecOpt{
+			value: &swarm.CredentialSpec{
+				Config: "foo",
+			},
+			source: "config://foo",
+		},
+		configs: *configOpt,
+	}
+
+	// create a service spec. we need to be sure to fill in the nullable
+	// fields, like the code expects
+	service := &swarm.ServiceSpec{
+		TaskTemplate: swarm.TaskSpec{
+			ContainerSpec: &swarm.ContainerSpec{
+				Privileges: &swarm.Privileges{
+					CredentialSpec: opts.credentialSpec.value,
+				},
+			},
+		},
+	}
+
+	// set up a function to use as the list function
+	var fakeClient fakeConfigAPIClientList = func(_ context.Context, opts types.ConfigListOptions) ([]swarm.Config, error) {
+		f := opts.Filters
+
+		// we're expecting the filter to have names "foo" and "bar"
+		names := f.Get("name")
+		assert.Equal(t, len(names), 2)
+		assert.Assert(t, is.Contains(names, "foo"))
+		assert.Assert(t, is.Contains(names, "bar"))
+
+		return []swarm.Config{
+			{
+				ID: "fooID",
+				Spec: swarm.ConfigSpec{
+					Annotations: swarm.Annotations{
+						Name: "foo",
+					},
+				},
+			}, {
+				ID: "barID",
+				Spec: swarm.ConfigSpec{
+					Annotations: swarm.Annotations{
+						Name: "bar",
+					},
+				},
+			},
+		}, nil
+	}
+
+	// now call setConfigs
+	err := setConfigs(fakeClient, service, opts)
+	// verify no error is returned
+	assert.NilError(t, err)
+
+	credSpecConfigValue := service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config
+	assert.Equal(t, credSpecConfigValue, "fooID")
+
+	configRefs := service.TaskTemplate.ContainerSpec.Configs
+	assert.Assert(t, is.Contains(configRefs, &swarm.ConfigReference{
+		ConfigID:   "fooID",
+		ConfigName: "foo",
+		Runtime:    &swarm.ConfigReferenceRuntimeTarget{},
+	}), "expected configRefs to contain foo config")
+	assert.Assert(t, is.Contains(configRefs, &swarm.ConfigReference{
+		ConfigID:   "barID",
+		ConfigName: "bar",
+		File: &swarm.ConfigReferenceFileTarget{
+			Name: "bar",
+			// these are the default field values
+			UID:  "0",
+			GID:  "0",
+			Mode: 0444,
+		},
+	}), "expected configRefs to contain bar config")
+}
+
+// TestSetConfigsOnlyCredSpec tests that even if a CredentialSpec is the only
+// config needed, setConfigs still works
+func TestSetConfigsOnlyCredSpec(t *testing.T) {
+	opts := &serviceOptions{
+		credentialSpec: credentialSpecOpt{
+			value: &swarm.CredentialSpec{
+				Config: "foo",
+			},
+			source: "config://foo",
+		},
+	}
+
+	service := &swarm.ServiceSpec{
+		TaskTemplate: swarm.TaskSpec{
+			ContainerSpec: &swarm.ContainerSpec{
+				Privileges: &swarm.Privileges{
+					CredentialSpec: opts.credentialSpec.value,
+				},
+			},
+		},
+	}
+
+	// set up a function to use as the list function
+	var fakeClient fakeConfigAPIClientList = func(_ context.Context, opts types.ConfigListOptions) ([]swarm.Config, error) {
+		f := opts.Filters
+
+		names := f.Get("name")
+		assert.Equal(t, len(names), 1)
+		assert.Assert(t, is.Contains(names, "foo"))
+
+		return []swarm.Config{
+			{
+				ID: "fooID",
+				Spec: swarm.ConfigSpec{
+					Annotations: swarm.Annotations{
+						Name: "foo",
+					},
+				},
+			},
+		}, nil
+	}
+
+	// now call setConfigs
+	err := setConfigs(fakeClient, service, opts)
+	// verify no error is returned
+	assert.NilError(t, err)
+
+	credSpecConfigValue := service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config
+	assert.Equal(t, credSpecConfigValue, "fooID")
+
+	configRefs := service.TaskTemplate.ContainerSpec.Configs
+	assert.Assert(t, is.Contains(configRefs, &swarm.ConfigReference{
+		ConfigID:   "fooID",
+		ConfigName: "foo",
+		Runtime:    &swarm.ConfigReferenceRuntimeTarget{},
+	}))
+}
+
+// TestSetConfigsOnlyConfigs verifies setConfigs when only configs (and not a
+// CredentialSpec) is needed.
+func TestSetConfigsOnlyConfigs(t *testing.T) {
+	configOpt := &cliopts.ConfigOpt{}
+	configOpt.Set("bar")
+	opts := &serviceOptions{
+		configs: *configOpt,
+	}
+
+	service := &swarm.ServiceSpec{
+		TaskTemplate: swarm.TaskSpec{
+			ContainerSpec: &swarm.ContainerSpec{},
+		},
+	}
+
+	var fakeClient fakeConfigAPIClientList = func(_ context.Context, opts types.ConfigListOptions) ([]swarm.Config, error) {
+		f := opts.Filters
+
+		names := f.Get("name")
+		assert.Equal(t, len(names), 1)
+		assert.Assert(t, is.Contains(names, "bar"))
+
+		return []swarm.Config{
+			{
+				ID: "barID",
+				Spec: swarm.ConfigSpec{
+					Annotations: swarm.Annotations{
+						Name: "bar",
+					},
+				},
+			},
+		}, nil
+	}
+
+	// now call setConfigs
+	err := setConfigs(fakeClient, service, opts)
+	// verify no error is returned
+	assert.NilError(t, err)
+
+	configRefs := service.TaskTemplate.ContainerSpec.Configs
+	assert.Assert(t, is.Contains(configRefs, &swarm.ConfigReference{
+		ConfigID:   "barID",
+		ConfigName: "bar",
+		File: &swarm.ConfigReferenceFileTarget{
+			Name: "bar",
+			// these are the default field values
+			UID:  "0",
+			GID:  "0",
+			Mode: 0444,
+		},
+	}))
+}
+
+// TestSetConfigsNoConfigs checks that setConfigs works when there are no
+// configs of any kind needed
+func TestSetConfigsNoConfigs(t *testing.T) {
+	// add a credentialSpec that isn't a config
+	opts := &serviceOptions{
+		credentialSpec: credentialSpecOpt{
+			value: &swarm.CredentialSpec{
+				File: "foo",
+			},
+			source: "file://foo",
+		},
+	}
+	service := &swarm.ServiceSpec{
+		TaskTemplate: swarm.TaskSpec{
+			ContainerSpec: &swarm.ContainerSpec{
+				Privileges: &swarm.Privileges{
+					CredentialSpec: opts.credentialSpec.value,
+				},
+			},
+		},
+	}
+
+	var fakeClient fakeConfigAPIClientList = func(_ context.Context, opts types.ConfigListOptions) ([]swarm.Config, error) {
+		// assert false -- we should never call this function
+		assert.Assert(t, false, "we should not be listing configs")
+		return nil, nil
+	}
+
+	err := setConfigs(fakeClient, service, opts)
+	assert.NilError(t, err)
+
+	// ensure that the value of the credentialspec has not changed
+	assert.Equal(t, service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.File, "foo")
+	assert.Equal(t, service.TaskTemplate.ContainerSpec.Privileges.CredentialSpec.Config, "")
+}

cli/command/service/opts.go

@@ -331,12 +331,25 @@ func (c *credentialSpecOpt) Set(value string) error {
 	c.source = value
 	c.value = &swarm.CredentialSpec{}
 	switch {
+	case strings.HasPrefix(value, "config://"):
+		// NOTE(dperny): we allow the user to specify the value of
+		// CredentialSpec Config using the Name of the config, but the API
+		// requires the ID of the config. For simplicity, we will parse
+		// whatever value is provided into the "Config" field, but before
+		// making API calls, we may need to swap the Config Name for the ID.
+		// Therefore, this isn't the definitive location for the value of
+		// Config that is passed to the API.
+		c.value.Config = strings.TrimPrefix(value, "config://")
 	case strings.HasPrefix(value, "file://"):
 		c.value.File = strings.TrimPrefix(value, "file://")
 	case strings.HasPrefix(value, "registry://"):
 		c.value.Registry = strings.TrimPrefix(value, "registry://")
+	case value == "":
+		// if the value of the flag is an empty string, that means there is no
+		// CredentialSpec needed. This is useful for removing a CredentialSpec
+		// during a service update.
 	default:
-		return errors.New("Invalid credential spec - value must be prefixed file:// or registry:// followed by a value")
+		return errors.New(`invalid credential spec: value must be prefixed with "config://", "file://", or "registry://"`)
 	}
 
 	return nil
@@ -663,7 +676,7 @@ func (options *serviceOptions) ToService(ctx context.Context, apiClient client.N
 		EndpointSpec:   options.endpoint.ToEndpointSpec(),
 	}
 
-	if options.credentialSpec.Value() != nil {
+	if options.credentialSpec.String() != "" && options.credentialSpec.Value() != nil {
 		service.TaskTemplate.ContainerSpec.Privileges = &swarm.Privileges{
 			CredentialSpec: options.credentialSpec.Value(),
 		}