From 190dac77bd34dd13aea28642c3ef10b53821f8c2 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Mon, 27 Jun 2022 17:16:44 +0200 Subject: [PATCH] container: validate --pull option on create and run Before this change, specifying the `--pull` flag without a value, could result in the flag after it, or the positional argument to be used as value. This patch makes sure that the value is an expected value; docker create --pull --rm hello-world docker: invalid pull option: '--rm': must be one of "always", "missing" or "never". docker run --pull --rm hello-world docker: invalid pull option: '--rm': must be one of "always", "missing" or "never". docker run --pull hello-world docker: invalid pull option: 'hello-world': must be one of "always", "missing" or "never". Signed-off-by: Sebastiaan van Stijn --- cli/command/container/create.go | 20 +++++++++++++++ cli/command/container/create_test.go | 37 ++++++++++++++++++++++++++++ cli/command/container/run.go | 4 +++ cli/command/container/run_test.go | 36 +++++++++++++++++++++++++++ 4 files changed, 97 insertions(+) diff --git a/cli/command/container/create.go b/cli/command/container/create.go index 4f9a71091b..99ef78710c 100644 --- a/cli/command/container/create.go +++ b/cli/command/container/create.go @@ -79,6 +79,10 @@ func NewCreateCommand(dockerCli command.Cli) *cobra.Command { } func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, options *createOptions, copts *containerOptions) error { + if err := validatePullOpt(options.pull); err != nil { + reportError(dockerCli.Err(), "create", err.Error(), true) + return cli.StatusError{StatusCode: 125} + } proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), opts.ConvertKVStringsToMapWithNil(copts.env.GetAll())) newEnv := []string{} for k, v := range proxyConfig { @@ -323,3 +327,19 @@ var localhostIPRegexp = regexp.MustCompile(ipLocalhost) func isLocalhost(ip string) bool { return localhostIPRegexp.MatchString(ip) } + +func validatePullOpt(val string) error { + switch val { + case PullImageAlways, PullImageMissing, PullImageNever, "": + // valid option, but nothing to do yet + return nil + default: + return fmt.Errorf( + "invalid pull option: '%s': must be one of %q, %q or %q", + val, + PullImageAlways, + PullImageMissing, + PullImageNever, + ) + } +} diff --git a/cli/command/container/create_test.go b/cli/command/container/create_test.go index 05c1d87b4b..c74b5d359e 100644 --- a/cli/command/container/create_test.go +++ b/cli/command/container/create_test.go @@ -2,6 +2,7 @@ package container import ( "context" + "errors" "fmt" "io" "os" @@ -10,6 +11,7 @@ import ( "strings" "testing" + "github.com/docker/cli/cli" "github.com/docker/cli/cli/config/configfile" "github.com/docker/cli/internal/test" "github.com/docker/cli/internal/test/notary" @@ -18,6 +20,7 @@ import ( "github.com/docker/docker/api/types/network" "github.com/google/go-cmp/cmp" specs "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/spf13/pflag" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" "gotest.tools/v3/fs" @@ -153,6 +156,40 @@ func TestCreateContainerImagePullPolicy(t *testing.T) { assert.Check(t, is.Equal(c.ExpectedPulls, pullCounter)) } } + +func TestCreateContainerImagePullPolicyInvalid(t *testing.T) { + cases := []struct { + PullPolicy string + ExpectedErrMsg string + }{ + { + PullPolicy: "busybox:latest", + ExpectedErrMsg: `invalid pull option: 'busybox:latest': must be one of "always", "missing" or "never"`, + }, + { + PullPolicy: "--network=foo", + ExpectedErrMsg: `invalid pull option: '--network=foo': must be one of "always", "missing" or "never"`, + }, + } + for _, tc := range cases { + tc := tc + t.Run(tc.PullPolicy, func(t *testing.T) { + dockerCli := test.NewFakeCli(&fakeClient{}) + err := runCreate( + dockerCli, + &pflag.FlagSet{}, + &createOptions{pull: tc.PullPolicy}, + &containerOptions{}, + ) + + statusErr := cli.StatusError{} + assert.Check(t, errors.As(err, &statusErr)) + assert.Equal(t, statusErr.StatusCode, 125) + assert.Check(t, is.Contains(dockerCli.ErrBuffer().String(), tc.ExpectedErrMsg)) + }) + } +} + func TestNewCreateCommandWithContentTrustErrors(t *testing.T) { testCases := []struct { name string diff --git a/cli/command/container/run.go b/cli/command/container/run.go index d851709561..c6b66b4db2 100644 --- a/cli/command/container/run.go +++ b/cli/command/container/run.go @@ -91,6 +91,10 @@ func NewRunCommand(dockerCli command.Cli) *cobra.Command { } func runRun(dockerCli command.Cli, flags *pflag.FlagSet, ropts *runOptions, copts *containerOptions) error { + if err := validatePullOpt(ropts.pull); err != nil { + reportError(dockerCli.Err(), "run", err.Error(), true) + return cli.StatusError{StatusCode: 125} + } proxyConfig := dockerCli.ConfigFile().ParseProxyConfig(dockerCli.Client().DaemonHost(), opts.ConvertKVStringsToMapWithNil(copts.env.GetAll())) newEnv := []string{} for k, v := range proxyConfig { diff --git a/cli/command/container/run_test.go b/cli/command/container/run_test.go index 1ac353fe86..4f3acfa730 100644 --- a/cli/command/container/run_test.go +++ b/cli/command/container/run_test.go @@ -1,15 +1,18 @@ package container import ( + "errors" "fmt" "io" "testing" + "github.com/docker/cli/cli" "github.com/docker/cli/internal/test" "github.com/docker/cli/internal/test/notary" "github.com/docker/docker/api/types/container" "github.com/docker/docker/api/types/network" specs "github.com/opencontainers/image-spec/specs-go/v1" + "github.com/spf13/pflag" "gotest.tools/v3/assert" is "gotest.tools/v3/assert/cmp" ) @@ -74,3 +77,36 @@ func TestRunCommandWithContentTrustErrors(t *testing.T) { assert.Assert(t, is.Contains(cli.ErrBuffer().String(), tc.expectedError)) } } + +func TestRunContainerImagePullPolicyInvalid(t *testing.T) { + cases := []struct { + PullPolicy string + ExpectedErrMsg string + }{ + { + PullPolicy: "busybox:latest", + ExpectedErrMsg: `invalid pull option: 'busybox:latest': must be one of "always", "missing" or "never"`, + }, + { + PullPolicy: "--network=foo", + ExpectedErrMsg: `invalid pull option: '--network=foo': must be one of "always", "missing" or "never"`, + }, + } + for _, tc := range cases { + tc := tc + t.Run(tc.PullPolicy, func(t *testing.T) { + dockerCli := test.NewFakeCli(&fakeClient{}) + err := runRun( + dockerCli, + &pflag.FlagSet{}, + &runOptions{createOptions: createOptions{pull: tc.PullPolicy}}, + &containerOptions{}, + ) + + statusErr := cli.StatusError{} + assert.Check(t, errors.As(err, &statusErr)) + assert.Equal(t, statusErr.StatusCode, 125) + assert.Check(t, is.Contains(dockerCli.ErrBuffer().String(), tc.ExpectedErrMsg)) + }) + } +}