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 <github@gone.nl>

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,
+		)
+	}
+}

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

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 {

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))
+		})
+	}
+}