From e9bc5ab185f05ae757cc059ad89d54ba005bba91 Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Mon, 25 Jun 2018 17:15:26 +0200 Subject: [PATCH 1/2] Add options to the compose loader - Add the possibility to skip interpolation - Add the possibility to skip schema validation - Allow customizing the substitution function, to add special cases. Signed-off-by: Vincent Demeester Upstream-commit: 9fdd14f399ed175b2d70b9e17f8b7725234c0e5b Component: cli --- .../compose/interpolation/interpolation.go | 7 +- .../cli/cli/compose/loader/interpolate.go | 9 +- components/cli/cli/compose/loader/loader.go | 41 +++++- .../cli/cli/compose/template/template.go | 126 ++++++++++++------ .../cli/cli/compose/template/template_test.go | 24 ++++ 5 files changed, 152 insertions(+), 55 deletions(-) diff --git a/components/cli/cli/compose/interpolation/interpolation.go b/components/cli/cli/compose/interpolation/interpolation.go index a1e7719860..d4f5c4a43f 100644 --- a/components/cli/cli/compose/interpolation/interpolation.go +++ b/components/cli/cli/compose/interpolation/interpolation.go @@ -14,6 +14,8 @@ type Options struct { LookupValue LookupValue // TypeCastMapping maps key paths to functions to cast to a type TypeCastMapping map[Path]Cast + // Substitution function to use + Substitute func(string, template.Mapping) (string, error) } // LookupValue is a function which maps from variable names to values. @@ -33,6 +35,9 @@ func Interpolate(config map[string]interface{}, opts Options) (map[string]interf if opts.TypeCastMapping == nil { opts.TypeCastMapping = make(map[Path]Cast) } + if opts.Substitute == nil { + opts.Substitute = template.Substitute + } out := map[string]interface{}{} @@ -51,7 +56,7 @@ func recursiveInterpolate(value interface{}, path Path, opts Options) (interface switch value := value.(type) { case string: - newValue, err := template.Substitute(value, template.Mapping(opts.LookupValue)) + newValue, err := opts.Substitute(value, template.Mapping(opts.LookupValue)) if err != nil || newValue == value { return value, newPathError(path, err) } diff --git a/components/cli/cli/compose/loader/interpolate.go b/components/cli/cli/compose/loader/interpolate.go index 5c3e1b8b0c..888d29b58c 100644 --- a/components/cli/cli/compose/loader/interpolate.go +++ b/components/cli/cli/compose/loader/interpolate.go @@ -64,11 +64,6 @@ func toBoolean(value string) (interface{}, error) { } } -func interpolateConfig(configDict map[string]interface{}, lookupEnv interp.LookupValue) (map[string]interface{}, error) { - return interp.Interpolate( - configDict, - interp.Options{ - LookupValue: lookupEnv, - TypeCastMapping: interpolateTypeCastMapping, - }) +func interpolateConfig(configDict map[string]interface{}, opts interp.Options) (map[string]interface{}, error) { + return interp.Interpolate(configDict, opts) } diff --git a/components/cli/cli/compose/loader/loader.go b/components/cli/cli/compose/loader/loader.go index 3c138d59ec..f11619b985 100644 --- a/components/cli/cli/compose/loader/loader.go +++ b/components/cli/cli/compose/loader/loader.go @@ -8,6 +8,7 @@ import ( "sort" "strings" + interp "github.com/docker/cli/cli/compose/interpolation" "github.com/docker/cli/cli/compose/schema" "github.com/docker/cli/cli/compose/template" "github.com/docker/cli/cli/compose/types" @@ -22,6 +23,16 @@ import ( yaml "gopkg.in/yaml.v2" ) +// Options supported by Load +type Options struct { + // Skip schema validation + SkipValidation bool + // Skip interpolation + SkipInterpolation bool + // Interpolation options + Interpolate *interp.Options +} + // ParseYAML reads the bytes from a file, parses the bytes into a mapping // structure, and returns it. func ParseYAML(source []byte) (map[string]interface{}, error) { @@ -41,12 +52,25 @@ func ParseYAML(source []byte) (map[string]interface{}, error) { } // Load reads a ConfigDetails and returns a fully loaded configuration -func Load(configDetails types.ConfigDetails) (*types.Config, error) { +func Load(configDetails types.ConfigDetails, options ...func(*Options)) (*types.Config, error) { if len(configDetails.ConfigFiles) < 1 { return nil, errors.Errorf("No files specified") } + opts := &Options{ + Interpolate: &interp.Options{ + Substitute: template.Substitute, + LookupValue: configDetails.LookupEnv, + TypeCastMapping: interpolateTypeCastMapping, + }, + } + + for _, op := range options { + op(opts) + } + configs := []*types.Config{} + var err error for _, file := range configDetails.ConfigFiles { configDict := file.Config @@ -62,14 +86,17 @@ func Load(configDetails types.ConfigDetails) (*types.Config, error) { return nil, err } - var err error - configDict, err = interpolateConfig(configDict, configDetails.LookupEnv) - if err != nil { - return nil, err + if !opts.SkipInterpolation { + configDict, err = interpolateConfig(configDict, *opts.Interpolate) + if err != nil { + return nil, err + } } - if err := schema.Validate(configDict, configDetails.Version); err != nil { - return nil, err + if !opts.SkipValidation { + if err := schema.Validate(configDict, configDetails.Version); err != nil { + return nil, err + } } cfg, err := loadSections(configDict, configDetails) diff --git a/components/cli/cli/compose/template/template.go b/components/cli/cli/compose/template/template.go index 086194b8fe..be5d456053 100644 --- a/components/cli/cli/compose/template/template.go +++ b/components/cli/cli/compose/template/template.go @@ -16,6 +16,14 @@ var patternString = fmt.Sprintf( var pattern = regexp.MustCompile(patternString) +// DefaultSubstituteFuncs contains the default SubstitueFunc used by the docker cli +var DefaultSubstituteFuncs = []SubstituteFunc{ + softDefault, + hardDefault, + requiredNonEmpty, + required, +} + // InvalidTemplateError is returned when a variable template is not in a valid // format type InvalidTemplateError struct { @@ -32,8 +40,14 @@ func (e InvalidTemplateError) Error() string { // and the absence of a value. type Mapping func(string) (string, bool) -// Substitute variables in the string with their values -func Substitute(template string, mapping Mapping) (string, error) { +// SubstituteFunc is a user-supplied function that apply substitution. +// Returns the value as a string, a bool indicating if the function could apply +// the substitution and an error. +type SubstituteFunc func(string, Mapping) (string, bool, error) + +// SubstituteWith subsitute variables in the string with their values. +// It accepts additional substitute function. +func SubstituteWith(template string, mapping Mapping, pattern *regexp.Regexp, subsFuncs ...SubstituteFunc) (string, error) { var err error result := pattern.ReplaceAllStringFunc(template, func(substring string) string { matches := pattern.FindStringSubmatch(substring) @@ -47,49 +61,22 @@ func Substitute(template string, mapping Mapping) (string, error) { substitution = groups["braced"] } - switch { - - case substitution == "": + if substitution == "" { err = &InvalidTemplateError{Template: template} return "" + } - // Soft default (fall back if unset or empty) - case strings.Contains(substitution, ":-"): - name, defaultValue := partition(substitution, ":-") - value, ok := mapping(name) - if !ok || value == "" { - return defaultValue - } - return value - - // Hard default (fall back if-and-only-if empty) - case strings.Contains(substitution, "-"): - name, defaultValue := partition(substitution, "-") - value, ok := mapping(name) - if !ok { - return defaultValue - } - return value - - case strings.Contains(substitution, ":?"): - name, errorMessage := partition(substitution, ":?") - value, ok := mapping(name) - if !ok || value == "" { - err = &InvalidTemplateError{ - Template: fmt.Sprintf("required variable %s is missing a value: %s", name, errorMessage), - } + for _, f := range subsFuncs { + var ( + value string + applied bool + ) + value, applied, err = f(substitution, mapping) + if err != nil { return "" } - return value - - case strings.Contains(substitution, "?"): - name, errorMessage := partition(substitution, "?") - value, ok := mapping(name) - if !ok { - err = &InvalidTemplateError{ - Template: fmt.Sprintf("required variable %s is missing a value: %s", name, errorMessage), - } - return "" + if !applied { + continue } return value } @@ -101,6 +88,65 @@ func Substitute(template string, mapping Mapping) (string, error) { return result, err } +// Substitute variables in the string with their values +func Substitute(template string, mapping Mapping) (string, error) { + return SubstituteWith(template, mapping, pattern, DefaultSubstituteFuncs...) +} + +// Soft default (fall back if unset or empty) +func softDefault(substitution string, mapping Mapping) (string, bool, error) { + if !strings.Contains(substitution, ":-") { + return "", false, nil + } + name, defaultValue := partition(substitution, ":-") + value, ok := mapping(name) + if !ok || value == "" { + return defaultValue, true, nil + } + return value, true, nil +} + +// Hard default (fall back if-and-only-if empty) +func hardDefault(substitution string, mapping Mapping) (string, bool, error) { + if !strings.Contains(substitution, "-") { + return "", false, nil + } + name, defaultValue := partition(substitution, "-") + value, ok := mapping(name) + if !ok { + return defaultValue, true, nil + } + return value, true, nil +} + +func requiredNonEmpty(substitution string, mapping Mapping) (string, bool, error) { + if !strings.Contains(substitution, ":?") { + return "", false, nil + } + name, errorMessage := partition(substitution, ":?") + value, ok := mapping(name) + if !ok || value == "" { + return "", true, &InvalidTemplateError{ + Template: fmt.Sprintf("required variable %s is missing a value: %s", name, errorMessage), + } + } + return value, true, nil +} + +func required(substitution string, mapping Mapping) (string, bool, error) { + if !strings.Contains(substitution, "?") { + return "", false, nil + } + name, errorMessage := partition(substitution, "?") + value, ok := mapping(name) + if !ok { + return "", true, &InvalidTemplateError{ + Template: fmt.Sprintf("required variable %s is missing a value: %s", name, errorMessage), + } + } + return value, true, nil +} + func matchGroups(matches []string) map[string]string { groups := make(map[string]string) for i, name := range pattern.SubexpNames()[1:] { diff --git a/components/cli/cli/compose/template/template_test.go b/components/cli/cli/compose/template/template_test.go index 48d76bb9fc..4d2f030816 100644 --- a/components/cli/cli/compose/template/template_test.go +++ b/components/cli/cli/compose/template/template_test.go @@ -1,6 +1,7 @@ package template import ( + "fmt" "reflect" "testing" @@ -148,3 +149,26 @@ func TestDefaultsForMandatoryVariables(t *testing.T) { assert.Check(t, is.Equal(tc.expected, result)) } } + +func TestSubstituteWithCustomFunc(t *testing.T) { + errIsMissing := func(substitution string, mapping Mapping) (string, bool, error) { + value, found := mapping(substitution) + if !found { + return "", true, &InvalidTemplateError{ + Template: fmt.Sprintf("required variable %s is missing a value", substitution), + } + } + return value, true, nil + } + + result, err := SubstituteWith("ok ${FOO}", defaultMapping, pattern, errIsMissing) + assert.NilError(t, err) + assert.Check(t, is.Equal("ok first", result)) + + result, err = SubstituteWith("ok ${BAR}", defaultMapping, pattern, errIsMissing) + assert.NilError(t, err) + assert.Check(t, is.Equal("ok ", result)) + + _, err = SubstituteWith("ok ${NOTHERE}", defaultMapping, pattern, errIsMissing) + assert.Check(t, is.ErrorContains(err, "required variable")) +} From 1821b4879db25abb544c901a78206a7d601c320b Mon Sep 17 00:00:00 2001 From: Vincent Demeester Date: Tue, 26 Jun 2018 14:07:26 +0200 Subject: [PATCH 2/2] Refactor `stack` command/package MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Handle `bundlefile` directly in the `top-level` command. `bundlefile` is still experimental and will be deprecated in future version — this should make be easier to remove it. - Validate the `stack` name in all cases (i.e. whatever the orchestrator is used) - Load the composefile ahead of choosing the orchestrator. This removes some slight duplication. - Makes `RunDeploy` easier to use from outside packages (like `docker/app`) with a preloaded configuration. Signed-off-by: Vincent Demeester Upstream-commit: 0f9d24f78d9873bc42cfbb128506e9aea294b1e1 Component: cli --- components/cli/cli/command/stack/common.go | 31 ++++++++++ components/cli/cli/command/stack/deploy.go | 60 +++++++++++++++---- .../cli/cli/command/stack/deploy_test.go | 17 ++++++ .../cli/command/stack/kubernetes/deploy.go | 9 +-- components/cli/cli/command/stack/ps.go | 4 ++ components/cli/cli/command/stack/ps_test.go | 8 +++ components/cli/cli/command/stack/remove.go | 4 ++ .../cli/cli/command/stack/remove_test.go | 8 +++ components/cli/cli/command/stack/services.go | 4 ++ .../cli/cli/command/stack/services_test.go | 8 +++ .../cli/cli/command/stack/swarm/common.go | 28 --------- .../cli/cli/command/stack/swarm/deploy.go | 17 +----- .../command/stack/swarm/deploy_bundlefile.go | 6 +- .../command/stack/swarm/deploy_composefile.go | 11 +--- .../cli/command/stack/swarm/deploy_test.go | 10 ---- components/cli/cli/command/stack/swarm/ps.go | 4 -- .../cli/cli/command/stack/swarm/ps_test.go | 18 ------ .../cli/cli/command/stack/swarm/remove.go | 4 -- .../cli/command/stack/swarm/remove_test.go | 18 ------ .../cli/cli/command/stack/swarm/services.go | 3 - .../cli/command/stack/swarm/services_test.go | 18 ------ 21 files changed, 139 insertions(+), 151 deletions(-) create mode 100644 components/cli/cli/command/stack/common.go create mode 100644 components/cli/cli/command/stack/deploy_test.go delete mode 100644 components/cli/cli/command/stack/swarm/ps_test.go delete mode 100644 components/cli/cli/command/stack/swarm/remove_test.go delete mode 100644 components/cli/cli/command/stack/swarm/services_test.go diff --git a/components/cli/cli/command/stack/common.go b/components/cli/cli/command/stack/common.go new file mode 100644 index 0000000000..6de410da84 --- /dev/null +++ b/components/cli/cli/command/stack/common.go @@ -0,0 +1,31 @@ +package stack + +import ( + "fmt" + "strings" + "unicode" +) + +// validateStackName checks if the provided string is a valid stack name (namespace). +// It currently only does a rudimentary check if the string is empty, or consists +// of only whitespace and quoting characters. +func validateStackName(namespace string) error { + v := strings.TrimFunc(namespace, quotesOrWhitespace) + if v == "" { + return fmt.Errorf("invalid stack name: %q", namespace) + } + return nil +} + +func validateStackNames(namespaces []string) error { + for _, ns := range namespaces { + if err := validateStackName(ns); err != nil { + return err + } + } + return nil +} + +func quotesOrWhitespace(r rune) bool { + return unicode.IsSpace(r) || r == '"' || r == '\'' +} diff --git a/components/cli/cli/command/stack/deploy.go b/components/cli/cli/command/stack/deploy.go index 5ca032d758..0165bbda1a 100644 --- a/components/cli/cli/command/stack/deploy.go +++ b/components/cli/cli/command/stack/deploy.go @@ -1,12 +1,18 @@ package stack import ( + "context" + "github.com/docker/cli/cli" "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/stack/kubernetes" + "github.com/docker/cli/cli/command/stack/loader" "github.com/docker/cli/cli/command/stack/options" "github.com/docker/cli/cli/command/stack/swarm" + composetypes "github.com/docker/cli/cli/compose/types" + "github.com/pkg/errors" "github.com/spf13/cobra" + "github.com/spf13/pflag" ) func newDeployCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command { @@ -19,20 +25,32 @@ func newDeployCommand(dockerCli command.Cli, common *commonOptions) *cobra.Comma Args: cli.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.Namespace = args[0] - switch { - case common == nil: // Top level deploy commad - return swarm.RunDeploy(dockerCli, opts) - case common.orchestrator.HasAll(): - return errUnsupportedAllOrchestrator - case common.orchestrator.HasKubernetes(): - kli, err := kubernetes.WrapCli(dockerCli, kubernetes.NewOptions(cmd.Flags(), common.orchestrator)) - if err != nil { - return err - } - return kubernetes.RunDeploy(kli, opts) - default: - return swarm.RunDeploy(dockerCli, opts) + if err := validateStackName(opts.Namespace); err != nil { + return err } + + commonOrchestrator := command.OrchestratorSwarm // default for top-level deploy command + if common != nil { + commonOrchestrator = common.orchestrator + } + + switch { + case opts.Bundlefile == "" && len(opts.Composefiles) == 0: + return errors.Errorf("Please specify either a bundle file (with --bundle-file) or a Compose file (with --compose-file).") + case opts.Bundlefile != "" && len(opts.Composefiles) != 0: + return errors.Errorf("You cannot specify both a bundle file and a Compose file.") + case opts.Bundlefile != "": + if commonOrchestrator != command.OrchestratorSwarm { + return errors.Errorf("bundle files are not supported on another orchestrator than swarm.") + } + return swarm.DeployBundle(context.Background(), dockerCli, opts) + } + + config, err := loader.LoadComposefile(dockerCli, opts) + if err != nil { + return err + } + return RunDeploy(dockerCli, cmd.Flags(), config, commonOrchestrator, opts) }, } @@ -54,3 +72,19 @@ func newDeployCommand(dockerCli command.Cli, common *commonOptions) *cobra.Comma kubernetes.AddNamespaceFlag(flags) return cmd } + +// RunDeploy performs a stack deploy against the specified orchestrator +func RunDeploy(dockerCli command.Cli, flags *pflag.FlagSet, config *composetypes.Config, commonOrchestrator command.Orchestrator, opts options.Deploy) error { + switch { + case commonOrchestrator.HasAll(): + return errUnsupportedAllOrchestrator + case commonOrchestrator.HasKubernetes(): + kli, err := kubernetes.WrapCli(dockerCli, kubernetes.NewOptions(flags, commonOrchestrator)) + if err != nil { + return err + } + return kubernetes.RunDeploy(kli, opts, config) + default: + return swarm.RunDeploy(dockerCli, opts, config) + } +} diff --git a/components/cli/cli/command/stack/deploy_test.go b/components/cli/cli/command/stack/deploy_test.go new file mode 100644 index 0000000000..89dbc6e1b7 --- /dev/null +++ b/components/cli/cli/command/stack/deploy_test.go @@ -0,0 +1,17 @@ +package stack + +import ( + "io/ioutil" + "testing" + + "github.com/docker/cli/internal/test" + "gotest.tools/assert" +) + +func TestDeployWithEmptyName(t *testing.T) { + cmd := newDeployCommand(test.NewFakeCli(&fakeClient{}), nil) + cmd.SetArgs([]string{"' '"}) + cmd.SetOutput(ioutil.Discard) + + assert.ErrorContains(t, cmd.Execute(), `invalid stack name: "' '"`) +} diff --git a/components/cli/cli/command/stack/kubernetes/deploy.go b/components/cli/cli/command/stack/kubernetes/deploy.go index 877cb1ef89..501e0a240b 100644 --- a/components/cli/cli/command/stack/kubernetes/deploy.go +++ b/components/cli/cli/command/stack/kubernetes/deploy.go @@ -5,14 +5,14 @@ import ( "io" "github.com/docker/cli/cli/command" - "github.com/docker/cli/cli/command/stack/loader" "github.com/docker/cli/cli/command/stack/options" + composetypes "github.com/docker/cli/cli/compose/types" "github.com/morikuni/aec" "github.com/pkg/errors" ) // RunDeploy is the kubernetes implementation of docker stack deploy -func RunDeploy(dockerCli *KubeCli, opts options.Deploy) error { +func RunDeploy(dockerCli *KubeCli, opts options.Deploy, cfg *composetypes.Config) error { cmdOut := dockerCli.Out() // Check arguments if len(opts.Composefiles) == 0 { @@ -29,11 +29,6 @@ func RunDeploy(dockerCli *KubeCli, opts options.Deploy) error { return err } - // Parse the compose file - cfg, err := loader.LoadComposefile(dockerCli, opts) - if err != nil { - return err - } stack, err := stacks.FromCompose(dockerCli.Err(), opts.Namespace, cfg) if err != nil { return err diff --git a/components/cli/cli/command/stack/ps.go b/components/cli/cli/command/stack/ps.go index c1c18d204a..4d6215351b 100644 --- a/components/cli/cli/command/stack/ps.go +++ b/components/cli/cli/command/stack/ps.go @@ -19,6 +19,10 @@ func newPsCommand(dockerCli command.Cli, common *commonOptions) *cobra.Command { Args: cli.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.Namespace = args[0] + if err := validateStackName(opts.Namespace); err != nil { + return err + } + switch { case common.orchestrator.HasAll(): return errUnsupportedAllOrchestrator diff --git a/components/cli/cli/command/stack/ps_test.go b/components/cli/cli/command/stack/ps_test.go index c192a7a5ab..39dd3c85fc 100644 --- a/components/cli/cli/command/stack/ps_test.go +++ b/components/cli/cli/command/stack/ps_test.go @@ -51,6 +51,14 @@ func TestStackPsErrors(t *testing.T) { } } +func TestRunPSWithEmptyName(t *testing.T) { + cmd := newPsCommand(test.NewFakeCli(&fakeClient{}), &orchestrator) + cmd.SetArgs([]string{"' '"}) + cmd.SetOutput(ioutil.Discard) + + assert.ErrorContains(t, cmd.Execute(), `invalid stack name: "' '"`) +} + func TestStackPsEmptyStack(t *testing.T) { fakeCli := test.NewFakeCli(&fakeClient{ taskListFunc: func(options types.TaskListOptions) ([]swarm.Task, error) { diff --git a/components/cli/cli/command/stack/remove.go b/components/cli/cli/command/stack/remove.go index 8738e6eafa..737713e48a 100644 --- a/components/cli/cli/command/stack/remove.go +++ b/components/cli/cli/command/stack/remove.go @@ -19,6 +19,10 @@ func newRemoveCommand(dockerCli command.Cli, common *commonOptions) *cobra.Comma Args: cli.RequiresMinArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.Namespaces = args + if err := validateStackNames(opts.Namespaces); err != nil { + return err + } + switch { case common.orchestrator.HasAll(): return errUnsupportedAllOrchestrator diff --git a/components/cli/cli/command/stack/remove_test.go b/components/cli/cli/command/stack/remove_test.go index 61080fbf3d..c7032d845f 100644 --- a/components/cli/cli/command/stack/remove_test.go +++ b/components/cli/cli/command/stack/remove_test.go @@ -41,6 +41,14 @@ func fakeClientForRemoveStackTest(version string) *fakeClient { } } +func TestRemoveWithEmptyName(t *testing.T) { + cmd := newRemoveCommand(test.NewFakeCli(&fakeClient{}), &orchestrator) + cmd.SetArgs([]string{"good", "' '", "alsogood"}) + cmd.SetOutput(ioutil.Discard) + + assert.ErrorContains(t, cmd.Execute(), `invalid stack name: "' '"`) +} + func TestRemoveStackVersion124DoesNotRemoveConfigsOrSecrets(t *testing.T) { client := fakeClientForRemoveStackTest("1.24") cmd := newRemoveCommand(test.NewFakeCli(client), &orchestrator) diff --git a/components/cli/cli/command/stack/services.go b/components/cli/cli/command/stack/services.go index 0379409b31..ca6d42c231 100644 --- a/components/cli/cli/command/stack/services.go +++ b/components/cli/cli/command/stack/services.go @@ -19,6 +19,10 @@ func newServicesCommand(dockerCli command.Cli, common *commonOptions) *cobra.Com Args: cli.ExactArgs(1), RunE: func(cmd *cobra.Command, args []string) error { opts.Namespace = args[0] + if err := validateStackName(opts.Namespace); err != nil { + return err + } + switch { case common.orchestrator.HasAll(): return errUnsupportedAllOrchestrator diff --git a/components/cli/cli/command/stack/services_test.go b/components/cli/cli/command/stack/services_test.go index 0bc8839b64..64a58b9970 100644 --- a/components/cli/cli/command/stack/services_test.go +++ b/components/cli/cli/command/stack/services_test.go @@ -80,6 +80,14 @@ func TestStackServicesErrors(t *testing.T) { } } +func TestRunServicesWithEmptyName(t *testing.T) { + cmd := newServicesCommand(test.NewFakeCli(&fakeClient{}), &orchestrator) + cmd.SetArgs([]string{"' '"}) + cmd.SetOutput(ioutil.Discard) + + assert.ErrorContains(t, cmd.Execute(), `invalid stack name: "' '"`) +} + func TestStackServicesEmptyServiceList(t *testing.T) { fakeCli := test.NewFakeCli(&fakeClient{ serviceListFunc: func(options types.ServiceListOptions) ([]swarm.Service, error) { diff --git a/components/cli/cli/command/stack/swarm/common.go b/components/cli/cli/command/stack/swarm/common.go index 62ff0d9ff5..b4193df360 100644 --- a/components/cli/cli/command/stack/swarm/common.go +++ b/components/cli/cli/command/stack/swarm/common.go @@ -2,9 +2,6 @@ package swarm import ( "context" - "fmt" - "strings" - "unicode" "github.com/docker/cli/cli/compose/convert" "github.com/docker/cli/opts" @@ -51,28 +48,3 @@ func getStackSecrets(ctx context.Context, apiclient client.APIClient, namespace func getStackConfigs(ctx context.Context, apiclient client.APIClient, namespace string) ([]swarm.Config, error) { return apiclient.ConfigList(ctx, types.ConfigListOptions{Filters: getStackFilter(namespace)}) } - -// validateStackName checks if the provided string is a valid stack name (namespace). -// -// It currently only does a rudimentary check if the string is empty, or consists -// of only whitespace and quoting characters. -func validateStackName(namespace string) error { - v := strings.TrimFunc(namespace, quotesOrWhitespace) - if len(v) == 0 { - return fmt.Errorf("invalid stack name: %q", namespace) - } - return nil -} - -func validateStackNames(namespaces []string) error { - for _, ns := range namespaces { - if err := validateStackName(ns); err != nil { - return err - } - } - return nil -} - -func quotesOrWhitespace(r rune) bool { - return unicode.IsSpace(r) || r == '"' || r == '\'' -} diff --git a/components/cli/cli/command/stack/swarm/deploy.go b/components/cli/cli/command/stack/swarm/deploy.go index 0504e3381b..d11c328f3f 100644 --- a/components/cli/cli/command/stack/swarm/deploy.go +++ b/components/cli/cli/command/stack/swarm/deploy.go @@ -7,6 +7,7 @@ import ( "github.com/docker/cli/cli/command" "github.com/docker/cli/cli/command/stack/options" "github.com/docker/cli/cli/compose/convert" + composetypes "github.com/docker/cli/cli/compose/types" "github.com/docker/docker/api/types/swarm" "github.com/docker/docker/api/types/versions" "github.com/pkg/errors" @@ -21,26 +22,14 @@ const ( ) // RunDeploy is the swarm implementation of docker stack deploy -func RunDeploy(dockerCli command.Cli, opts options.Deploy) error { +func RunDeploy(dockerCli command.Cli, opts options.Deploy, cfg *composetypes.Config) error { ctx := context.Background() - if err := validateStackName(opts.Namespace); err != nil { - return err - } if err := validateResolveImageFlag(dockerCli, &opts); err != nil { return err } - switch { - case opts.Bundlefile == "" && len(opts.Composefiles) == 0: - return errors.Errorf("Please specify either a bundle file (with --bundle-file) or a Compose file (with --compose-file).") - case opts.Bundlefile != "" && len(opts.Composefiles) != 0: - return errors.Errorf("You cannot specify both a bundle file and a Compose file.") - case opts.Bundlefile != "": - return deployBundle(ctx, dockerCli, opts) - default: - return deployCompose(ctx, dockerCli, opts) - } + return deployCompose(ctx, dockerCli, opts, cfg) } // validateResolveImageFlag validates the opts.resolveImage command line option diff --git a/components/cli/cli/command/stack/swarm/deploy_bundlefile.go b/components/cli/cli/command/stack/swarm/deploy_bundlefile.go index 7b59126baf..8db6f66b0e 100644 --- a/components/cli/cli/command/stack/swarm/deploy_bundlefile.go +++ b/components/cli/cli/command/stack/swarm/deploy_bundlefile.go @@ -15,10 +15,8 @@ import ( "github.com/pkg/errors" ) -func deployBundle(ctx context.Context, dockerCli command.Cli, opts options.Deploy) error { - if err := validateStackName(opts.Namespace); err != nil { - return err - } +// DeployBundle deploy a bundlefile (dab) on a swarm. +func DeployBundle(ctx context.Context, dockerCli command.Cli, opts options.Deploy) error { bundle, err := loadBundlefile(dockerCli.Err(), opts.Namespace, opts.Bundlefile) if err != nil { return err diff --git a/components/cli/cli/command/stack/swarm/deploy_composefile.go b/components/cli/cli/command/stack/swarm/deploy_composefile.go index a0b8b127d6..e4574fd987 100644 --- a/components/cli/cli/command/stack/swarm/deploy_composefile.go +++ b/components/cli/cli/command/stack/swarm/deploy_composefile.go @@ -5,7 +5,6 @@ import ( "fmt" "github.com/docker/cli/cli/command" - "github.com/docker/cli/cli/command/stack/loader" "github.com/docker/cli/cli/command/stack/options" "github.com/docker/cli/cli/compose/convert" composetypes "github.com/docker/cli/cli/compose/types" @@ -17,15 +16,7 @@ import ( "github.com/pkg/errors" ) -func deployCompose(ctx context.Context, dockerCli command.Cli, opts options.Deploy) error { - if err := validateStackName(opts.Namespace); err != nil { - return err - } - config, err := loader.LoadComposefile(dockerCli, opts) - if err != nil { - return err - } - +func deployCompose(ctx context.Context, dockerCli command.Cli, opts options.Deploy, config *composetypes.Config) error { if err := checkDaemonIsSwarmManager(ctx, dockerCli); err != nil { return err } diff --git a/components/cli/cli/command/stack/swarm/deploy_test.go b/components/cli/cli/command/stack/swarm/deploy_test.go index 64aafec382..b1df60dcba 100644 --- a/components/cli/cli/command/stack/swarm/deploy_test.go +++ b/components/cli/cli/command/stack/swarm/deploy_test.go @@ -4,7 +4,6 @@ import ( "context" "testing" - "github.com/docker/cli/cli/command/stack/options" "github.com/docker/cli/cli/compose/convert" "github.com/docker/cli/internal/test" "github.com/docker/docker/api/types" @@ -27,15 +26,6 @@ func TestPruneServices(t *testing.T) { assert.Check(t, is.DeepEqual(buildObjectIDs([]string{objectName("foo", "remove")}), client.removedServices)) } -func TestDeployWithEmptyName(t *testing.T) { - ctx := context.Background() - client := &fakeClient{} - dockerCli := test.NewFakeCli(client) - - err := deployCompose(ctx, dockerCli, options.Deploy{Namespace: "' '", Prune: true}) - assert.Check(t, is.Error(err, `invalid stack name: "' '"`)) -} - // TestServiceUpdateResolveImageChanged tests that the service's // image digest, and "ForceUpdate" is preserved if the image did not change in // the compose file diff --git a/components/cli/cli/command/stack/swarm/ps.go b/components/cli/cli/command/stack/swarm/ps.go index 79a154a8f2..5b28a39e73 100644 --- a/components/cli/cli/command/stack/swarm/ps.go +++ b/components/cli/cli/command/stack/swarm/ps.go @@ -13,10 +13,6 @@ import ( // RunPS is the swarm implementation of docker stack ps func RunPS(dockerCli command.Cli, opts options.PS) error { - if err := validateStackName(opts.Namespace); err != nil { - return err - } - filter := getStackFilterFromOpt(opts.Namespace, opts.Filter) ctx := context.Background() diff --git a/components/cli/cli/command/stack/swarm/ps_test.go b/components/cli/cli/command/stack/swarm/ps_test.go deleted file mode 100644 index 33401da655..0000000000 --- a/components/cli/cli/command/stack/swarm/ps_test.go +++ /dev/null @@ -1,18 +0,0 @@ -package swarm - -import ( - "testing" - - "github.com/docker/cli/cli/command/stack/options" - "github.com/docker/cli/internal/test" - "gotest.tools/assert" - is "gotest.tools/assert/cmp" -) - -func TestRunPSWithEmptyName(t *testing.T) { - client := &fakeClient{} - dockerCli := test.NewFakeCli(client) - - err := RunPS(dockerCli, options.PS{Namespace: "' '"}) - assert.Check(t, is.Error(err, `invalid stack name: "' '"`)) -} diff --git a/components/cli/cli/command/stack/swarm/remove.go b/components/cli/cli/command/stack/swarm/remove.go index e75597d065..4dedef12f3 100644 --- a/components/cli/cli/command/stack/swarm/remove.go +++ b/components/cli/cli/command/stack/swarm/remove.go @@ -16,10 +16,6 @@ import ( // RunRemove is the swarm implementation of docker stack remove func RunRemove(dockerCli command.Cli, opts options.Remove) error { - if err := validateStackNames(opts.Namespaces); err != nil { - return err - } - client := dockerCli.Client() ctx := context.Background() diff --git a/components/cli/cli/command/stack/swarm/remove_test.go b/components/cli/cli/command/stack/swarm/remove_test.go deleted file mode 100644 index 3d30b288ac..0000000000 --- a/components/cli/cli/command/stack/swarm/remove_test.go +++ /dev/null @@ -1,18 +0,0 @@ -package swarm - -import ( - "testing" - - "github.com/docker/cli/cli/command/stack/options" - "github.com/docker/cli/internal/test" - "gotest.tools/assert" - is "gotest.tools/assert/cmp" -) - -func TestRunRemoveWithEmptyName(t *testing.T) { - client := &fakeClient{} - dockerCli := test.NewFakeCli(client) - - err := RunRemove(dockerCli, options.Remove{Namespaces: []string{"good", "' '", "alsogood"}}) - assert.Check(t, is.Error(err, `invalid stack name: "' '"`)) -} diff --git a/components/cli/cli/command/stack/swarm/services.go b/components/cli/cli/command/stack/swarm/services.go index 0225918678..07b990adc8 100644 --- a/components/cli/cli/command/stack/swarm/services.go +++ b/components/cli/cli/command/stack/swarm/services.go @@ -14,9 +14,6 @@ import ( // RunServices is the swarm implementation of docker stack services func RunServices(dockerCli command.Cli, opts options.Services) error { - if err := validateStackName(opts.Namespace); err != nil { - return err - } ctx := context.Background() client := dockerCli.Client() diff --git a/components/cli/cli/command/stack/swarm/services_test.go b/components/cli/cli/command/stack/swarm/services_test.go deleted file mode 100644 index 9e3ca47f3a..0000000000 --- a/components/cli/cli/command/stack/swarm/services_test.go +++ /dev/null @@ -1,18 +0,0 @@ -package swarm - -import ( - "testing" - - "github.com/docker/cli/cli/command/stack/options" - "github.com/docker/cli/internal/test" - "gotest.tools/assert" - is "gotest.tools/assert/cmp" -) - -func TestRunServicesWithEmptyName(t *testing.T) { - client := &fakeClient{} - dockerCli := test.NewFakeCli(client) - - err := RunServices(dockerCli, options.Services{Namespace: "' '"}) - assert.Check(t, is.Error(err, `invalid stack name: "' '"`)) -}