From 01f4f2e80a2dc1709bc576d005d9181696adfbd7 Mon Sep 17 00:00:00 2001 From: Drew Erny Date: Wed, 27 Mar 2019 15:44:32 -0500 Subject: [PATCH] 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 --- cli/command/service/create.go | 51 ++++++++++++++--- cli/command/service/opts.go | 13 ++++- cli/command/service/opts_test.go | 7 +-- cli/command/service/parse.go | 35 ++++++++++++ cli/command/service/update.go | 98 +++++++++++++++++++++++++++++++- 5 files changed, 188 insertions(+), 16 deletions(-) diff --git a/cli/command/service/create.go b/cli/command/service/create.go index fb8f26c995..aff8d46eb0 100644 --- a/cli/command/service/create.go +++ b/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() - if len(specifiedConfigs) > 0 { - // parse and validate configs - configs, err := ParseConfigs(apiClient, specifiedConfigs) - if err != nil { - return err - } - service.TaskTemplate.ContainerSpec.Configs = configs - } + setConfigs(apiClient, &service, opts) 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 +} diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index ce9f39c842..d76687337a 100644 --- a/cli/command/service/opts.go +++ b/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(), } diff --git a/cli/command/service/opts_test.go b/cli/command/service/opts_test.go index 9f26daa3bf..6b9b83d186 100644 --- a/cli/command/service/opts_test.go +++ b/cli/command/service/opts_test.go @@ -22,10 +22,9 @@ func TestCredentialSpecOpt(t *testing.T) { expectedErr string }{ { - name: "empty", - in: "", - value: swarm.CredentialSpec{}, - expectedErr: `invalid credential spec: value must be prefixed with "config://", "file://", or "registry://"`, + name: "empty", + in: "", + value: swarm.CredentialSpec{}, }, { name: "no-prefix", diff --git a/cli/command/service/parse.go b/cli/command/service/parse.go index 254107071f..b3d35f551a 100644 --- a/cli/command/service/parse.go +++ b/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 } diff --git a/cli/command/service/update.go b/cli/command/service/update.go index 8269adcd00..7371a28bc1 100644 --- a/cli/command/service/update.go +++ b/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 }