Update CredentialSpec code to allow using configs

Updates the CredentialSpec handling code for services to allow using
swarm Configs.

Additionally, fixes a bug where the `--credential-spec` flag would not
be respected on service updates.

Signed-off-by: Drew Erny <drew.erny@docker.com>

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,15 +97,7 @@ func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, opts *serviceOptions
 		service.TaskTemplate.ContainerSpec.Secrets = secrets
 	}
 
-	specifiedConfigs := opts.configs.Value()
+	setConfigs(apiClient, &service, opts)
-	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 := resolveServiceImageDigestContentTrust(dockerCli, &service); err != nil {
 		return err
@@ -141,3 +135,42 @@ 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
+				}
+			}
+		}
+	}
+
+	return nil
+}

cli/command/service/opts.go

@@ -332,11 +332,22 @@ func (c *credentialSpecOpt) Set(value string) error {
 	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 with "config://", "file://", or "registry://"`)
 	}
@@ -665,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(),
 		}

cli/command/service/opts_test.go

@@ -25,7 +25,6 @@ func TestCredentialSpecOpt(t *testing.T) {
 			name:  "empty",
 			in:    "",
 			value: swarm.CredentialSpec{},
-			expectedErr: `invalid credential spec: value must be prefixed with "config://", "file://", or "registry://"`,
 		},
 		{
 			name:        "no-prefix",

cli/command/service/parse.go

@@ -70,10 +70,29 @@ func ParseConfigs(client client.ConfigAPIClient, requestedConfigs []*swarmtypes.
 		return []*swarmtypes.ConfigReference{}, nil
 	}
 
+	// the configRefs map has two purposes: it prevents duplication of config
+	// target filenames, and it it used to get all configs so we can resolve
+	// their IDs. unfortunately, there are other targets for ConfigReferences,
+	// besides just a File; specifically, the Runtime target, which is used for
+	// CredentialSpecs. Therefore, we need to have a list of ConfigReferences
+	// that are not File targets as well. at this time of writing, the only use
+	// for Runtime targets is CredentialSpecs. However, to future-proof this
+	// functionality, we should handle the case where multiple Runtime targets
+	// are in use for the same Config, and we should deduplicate
+	// such ConfigReferences, as no matter how many times the Config is used,
+	// it is only needed to be referenced once.
 	configRefs := make(map[string]*swarmtypes.ConfigReference)
+	runtimeRefs := make(map[string]*swarmtypes.ConfigReference)
 	ctx := context.Background()
 
 	for _, config := range requestedConfigs {
+		if config.Runtime != nil {
+			// by assigning to a map based on ConfigName, if the same Config
+			// is required as a Runtime target for multiple purposes, we only
+			// include it once in the final set of configs.
+			runtimeRefs[config.ConfigName] = config
+		}
+
 		if _, exists := configRefs[config.File.Name]; exists {
 			return nil, errors.Errorf("duplicate config target for %s not allowed", config.ConfigName)
 		}
@@ -87,6 +106,9 @@ func ParseConfigs(client client.ConfigAPIClient, requestedConfigs []*swarmtypes.
 	for _, s := range configRefs {
 		args.Add("name", s.ConfigName)
 	}
+	for _, s := range runtimeRefs {
+		args.Add("name", s.ConfigName)
+	}
 
 	configs, err := client.ConfigList(ctx, types.ConfigListOptions{
 		Filters: args,
@@ -114,5 +136,18 @@ func ParseConfigs(client client.ConfigAPIClient, requestedConfigs []*swarmtypes.
 		addedConfigs = append(addedConfigs, ref)
 	}
 
+	// unfortunately, because the key of configRefs and runtimeRefs is different
+	// values that may collide, we can't just do some fancy trickery to
+	// concat maps, we need to do two separate loops
+	for _, ref := range runtimeRefs {
+		id, ok := foundConfigs[ref.ConfigName]
+		if !ok {
+			return nil, errors.Errorf("config not found: %s", ref.ConfigName)
+		}
+
+		ref.ConfigID = id
+		addedConfigs = append(addedConfigs, ref)
+	}
+
 	return addedConfigs, nil
 }

cli/command/service/update.go

@@ -201,6 +201,48 @@ func runUpdate(dockerCli command.Cli, flags *pflag.FlagSet, options *serviceOpti
 
 	spec.TaskTemplate.ContainerSpec.Configs = updatedConfigs
 
+	// set the credential spec value after get the updated configs, because we
+	// might need the updated configs to set the correct value of the
+	// CredentialSpec.
+	if flags.Changed(flagCredentialSpec) {
+		containerSpec := spec.TaskTemplate.ContainerSpec
+		credSpecOpt := flags.Lookup(flagCredentialSpec)
+		// if the flag has changed, and the value is empty string, then we
+		// should remove any credential spec that might be present
+		if credSpecOpt.Value.String() == "" {
+			if containerSpec.Privileges != nil {
+				containerSpec.Privileges.CredentialSpec = nil
+			}
+			return nil
+		}
+
+		// otherwise, set the credential spec to be the parsed value
+		credSpec := credSpecOpt.Value.(*credentialSpecOpt).Value()
+
+		// if this is a Config credential spec, we we still need to replace the
+		// value of credSpec.Config with the config ID instead of Name.
+		if credSpec.Config != "" {
+			for _, config := range updatedConfigs {
+				// if the config name matches, then set the config ID. we do
+				// not need to worry about if this is a Runtime target or not.
+				// even if it is not a Runtime target, getUpdatedConfigs
+				// ensures that a Runtime target for this config exists, and
+				// the Name is unique so the ID is correct no matter the
+				// target.
+				if config.ConfigName == credSpec.Config {
+					credSpec.Config = config.ConfigID
+					break
+				}
+			}
+		}
+
+		if containerSpec.Privileges == nil {
+			containerSpec.Privileges = &swarm.Privileges{}
+		}
+
+		containerSpec.Privileges.CredentialSpec = credSpec
+	}
+
 	// only send auth if flag was set
 	sendAuth, err := flags.GetBool(flagRegistryAuth)
 	if err != nil {
@@ -734,17 +776,69 @@ func getUpdatedSecrets(apiClient client.SecretAPIClient, flags *pflag.FlagSet, s
 func getUpdatedConfigs(apiClient client.ConfigAPIClient, flags *pflag.FlagSet, configs []*swarm.ConfigReference) ([]*swarm.ConfigReference, error) {
 	newConfigs := []*swarm.ConfigReference{}
 
+	// resolveConfigs is a slice of any new configs that need to have the ID
+	// resolved
+	resolveConfigs := []*swarm.ConfigReference{}
+
+	// credSpecConfig is used for two things, and may be a bit confusing.
+	// First, it's used to store the temporary value of the ConfigReference for
+	// a credential spec config. This signals to the loop that removes configs
+	// that a credential spec config is needed. If a Runtime target Config with
+	// this name is found in the existing configs, then that loop will nil this
+	// variable. then, before we go looking up all the new configs, if this
+	// variable is non-nil, we'll add it to resolveConfigs and fill in its ID.
+	var credSpecConfig *swarm.ConfigReference
+
+	if flags.Changed(flagCredentialSpec) {
+		credSpec := flags.Lookup(flagCredentialSpec).Value.(*credentialSpecOpt).Value()
+		if credSpec.Config != "" {
+			credSpecConfig = &swarm.ConfigReference{
+				ConfigName: credSpec.Config,
+				Runtime:    &swarm.ConfigReferenceRuntimeTarget{},
+			}
+		}
+	}
+
 	toRemove := buildToRemoveSet(flags, flagConfigRemove)
 	for _, config := range configs {
+		// if the config is a Runtime target, make sure it's still in use right
+		// now, the only use for Runtime target is credential specs.  if, in
+		// the future, more uses are added, then this check will need to be
+		// made more intelligent.
+		if config.Runtime != nil {
+			// for now, just check if the credential spec flag has changed, and
+			// if it has, that the credSpecConfig exists and has the same name.
+			// if the credential spec flag has changed, and it's either no
+			// longer a config or its a different config, then we do not add
+			// this config to newConfigs
+			if flags.Changed(flagCredentialSpec) && credSpecConfig != nil {
+				if credSpecConfig.ConfigName == config.ConfigName {
+					newConfigs = append(newConfigs, config)
+					credSpecConfig = nil
+				}
+			}
+			// continue the loop, to skip the part where we check if the config
+			// is in toRemove.
+			continue
+		}
+
 		if _, exists := toRemove[config.ConfigName]; !exists {
 			newConfigs = append(newConfigs, config)
 		}
 	}
 
 	if flags.Changed(flagConfigAdd) {
-		values := flags.Lookup(flagConfigAdd).Value.(*opts.ConfigOpt).Value()
+		resolveConfigs = append(resolveConfigs, flags.Lookup(flagConfigAdd).Value.(*opts.ConfigOpt).Value()...)
+	}
 
-		addConfigs, err := ParseConfigs(apiClient, values)
+	// if credSpecConfig is non-nil at this point, it means its a new config,
+	// and we need to resolve its ID accordingly.
+	if credSpecConfig != nil {
+		resolveConfigs = append(resolveConfigs, credSpecConfig)
+	}
+
+	if len(resolveConfigs) > 0 {
+		addConfigs, err := ParseConfigs(apiClient, resolveConfigs)
 		if err != nil {
 			return nil, err
 		}