diff --git a/cli/command/config/create.go b/cli/command/config/create.go index fc19370ccb..17cb4184f3 100644 --- a/cli/command/config/create.go +++ b/cli/command/config/create.go @@ -25,7 +25,7 @@ type CreateOptions struct { func newConfigCreateCommand(dockerCli command.Cli) *cobra.Command { createOpts := CreateOptions{ - Labels: opts.NewListOpts(opts.ValidateEnv), + Labels: opts.NewListOpts(opts.ValidateLabel), } cmd := &cobra.Command{ diff --git a/cli/command/container/opts.go b/cli/command/container/opts.go index c98a26adce..8fe4ded9c8 100644 --- a/cli/command/container/opts.go +++ b/cli/command/container/opts.go @@ -147,7 +147,7 @@ func addFlags(flags *pflag.FlagSet) *containerOptions { expose: opts.NewListOpts(nil), extraHosts: opts.NewListOpts(opts.ValidateExtraHost), groupAdd: opts.NewListOpts(nil), - labels: opts.NewListOpts(nil), + labels: opts.NewListOpts(opts.ValidateLabel), labelsFile: opts.NewListOpts(nil), linkLocalIPs: opts.NewListOpts(nil), links: opts.NewListOpts(opts.ValidateLink), diff --git a/cli/command/image/build.go b/cli/command/image/build.go index 8f925cc0b0..3c67946b29 100644 --- a/cli/command/image/build.go +++ b/cli/command/image/build.go @@ -93,7 +93,7 @@ func newBuildOptions() buildOptions { tags: opts.NewListOpts(validateTag), buildArgs: opts.NewListOpts(opts.ValidateEnv), ulimits: opts.NewUlimitOpt(&ulimits), - labels: opts.NewListOpts(opts.ValidateEnv), + labels: opts.NewListOpts(opts.ValidateLabel), extraHosts: opts.NewListOpts(opts.ValidateExtraHost), } } diff --git a/cli/command/network/create.go b/cli/command/network/create.go index a8dda0a258..bcb3729426 100644 --- a/cli/command/network/create.go +++ b/cli/command/network/create.go @@ -39,7 +39,7 @@ type createOptions struct { func newCreateCommand(dockerCli command.Cli) *cobra.Command { options := createOptions{ driverOpts: *opts.NewMapOpts(nil, nil), - labels: opts.NewListOpts(opts.ValidateEnv), + labels: opts.NewListOpts(opts.ValidateLabel), ipamAux: *opts.NewMapOpts(nil, nil), ipamOpt: *opts.NewMapOpts(nil, nil), } diff --git a/cli/command/secret/create.go b/cli/command/secret/create.go index 1739fefa9c..1f570bcac3 100644 --- a/cli/command/secret/create.go +++ b/cli/command/secret/create.go @@ -25,7 +25,7 @@ type createOptions struct { func newSecretCreateCommand(dockerCli command.Cli) *cobra.Command { options := createOptions{ - labels: opts.NewListOpts(opts.ValidateEnv), + labels: opts.NewListOpts(opts.ValidateLabel), } cmd := &cobra.Command{ diff --git a/cli/command/service/opts.go b/cli/command/service/opts.go index ec4ea403d6..e2620ada35 100644 --- a/cli/command/service/opts.go +++ b/cli/command/service/opts.go @@ -520,9 +520,9 @@ type serviceOptions struct { func newServiceOptions() *serviceOptions { return &serviceOptions{ - labels: opts.NewListOpts(opts.ValidateEnv), + labels: opts.NewListOpts(opts.ValidateLabel), constraints: opts.NewListOpts(nil), - containerLabels: opts.NewListOpts(opts.ValidateEnv), + containerLabels: opts.NewListOpts(opts.ValidateLabel), env: opts.NewListOpts(opts.ValidateEnv), envFile: opts.NewListOpts(nil), groups: opts.NewListOpts(nil), diff --git a/cli/command/volume/create.go b/cli/command/volume/create.go index b25ed155cb..125da44332 100644 --- a/cli/command/volume/create.go +++ b/cli/command/volume/create.go @@ -22,7 +22,7 @@ type createOptions struct { func newCreateCommand(dockerCli command.Cli) *cobra.Command { options := createOptions{ driverOpts: *opts.NewMapOpts(nil, nil), - labels: opts.NewListOpts(opts.ValidateEnv), + labels: opts.NewListOpts(opts.ValidateLabel), } cmd := &cobra.Command{ diff --git a/opts/envfile_test.go b/opts/envfile_test.go index bda9dcb6e7..f875707a5e 100644 --- a/opts/envfile_test.go +++ b/opts/envfile_test.go @@ -100,7 +100,7 @@ func TestParseEnvFileBadlyFormattedFile(t *testing.T) { if _, ok := err.(ErrBadKey); !ok { t.Fatalf("Expected an ErrBadKey, got [%v]", err) } - expectedMessage := "poorly formatted environment: variable 'f ' has white spaces" + expectedMessage := "poorly formatted environment: variable 'f ' contains whitespaces" if err.Error() != expectedMessage { t.Fatalf("Expected [%v], got [%v]", expectedMessage, err.Error()) } @@ -134,7 +134,7 @@ another invalid line` if _, ok := err.(ErrBadKey); !ok { t.Fatalf("Expected an ErrBadKey, got [%v]", err) } - expectedMessage := "poorly formatted environment: variable 'first line' has white spaces" + expectedMessage := "poorly formatted environment: variable 'first line' contains whitespaces" if err.Error() != expectedMessage { t.Fatalf("Expected [%v], got [%v]", expectedMessage, err.Error()) } diff --git a/opts/file.go b/opts/file.go index 1721a46ef9..2346cc1670 100644 --- a/opts/file.go +++ b/opts/file.go @@ -10,7 +10,7 @@ import ( "unicode/utf8" ) -var whiteSpaces = " \t" +const whiteSpaces = " \t" // ErrBadKey typed error for bad environment variable type ErrBadKey struct { @@ -51,7 +51,7 @@ func parseKeyValueFile(filename string, emptyFn func(string) (string, bool)) ([] // trim the front of a variable, but nothing else variable := strings.TrimLeft(data[0], whiteSpaces) if strings.ContainsAny(variable, whiteSpaces) { - return []string{}, ErrBadKey{fmt.Sprintf("variable '%s' has white spaces", variable)} + return []string{}, ErrBadKey{fmt.Sprintf("variable '%s' contains whitespaces", variable)} } if len(variable) == 0 { return []string{}, ErrBadKey{fmt.Sprintf("no variable name on line '%s'", line)} diff --git a/opts/opts.go b/opts/opts.go index 6485e309e5..765548f6b9 100644 --- a/opts/opts.go +++ b/opts/opts.go @@ -267,10 +267,24 @@ func validateDomain(val string) (string, error) { } // ValidateLabel validates that the specified string is a valid label, and returns it. -// Labels are in the form on key=value. +// +// Labels are in the form of key=value; key must be a non-empty string, and not +// contain whitespaces. A value is optional (defaults to an empty string if omitted). +// +// Leading whitespace is removed during validation but values are kept as-is +// otherwise, so any string value is accepted for both, which includes whitespace +// (for values) and quotes (surrounding, or embedded in key or value). +// +// TODO discuss if quotes (and other special characters) should be valid or invalid for keys +// TODO discuss if leading/trailing whitespace in keys should be preserved (and valid) func ValidateLabel(val string) (string, error) { - if strings.Count(val, "=") < 1 { - return "", fmt.Errorf("bad attribute format: %s", val) + arr := strings.SplitN(val, "=", 2) + key := strings.TrimLeft(arr[0], whiteSpaces) + if key == "" { + return "", fmt.Errorf("invalid label '%s': empty name", val) + } + if strings.ContainsAny(key, whiteSpaces) { + return "", fmt.Errorf("label '%s' contains whitespaces", key) } return val, nil } diff --git a/opts/opts_test.go b/opts/opts_test.go index 723a212ea7..4d5ef1749e 100644 --- a/opts/opts_test.go +++ b/opts/opts_test.go @@ -4,6 +4,8 @@ import ( "fmt" "strings" "testing" + + "gotest.tools/assert" ) func TestValidateIPAddress(t *testing.T) { @@ -175,19 +177,104 @@ func TestValidateDNSSearch(t *testing.T) { } func TestValidateLabel(t *testing.T) { - if _, err := ValidateLabel("label"); err == nil || err.Error() != "bad attribute format: label" { - t.Fatalf("Expected an error [bad attribute format: label], go %v", err) + tests := []struct { + name string + value string + expectedErr string + }{ + { + name: "empty", + expectedErr: `invalid label '': empty name`, + }, + { + name: "whitespace only ", + value: " ", + expectedErr: `invalid label ' ': empty name`, + }, + { + name: "whitespace around equal-sign", + value: " = ", + expectedErr: `invalid label ' = ': empty name`, + }, + { + name: "leading whitespace", + value: " label=value", + }, + { + name: "whitespaces in key without value", + value: "this is a label without value", + expectedErr: `label 'this is a label without value' contains whitespaces`, + }, + { + name: "whitespaces in key", + value: "this is a label=value", + expectedErr: `label 'this is a label' contains whitespaces`, + }, + { + name: "whitespaces in value", + value: "label=a value that has whitespace", + }, + { + name: "trailing whitespace in value", + value: "label=value ", + }, + { + name: "leading whitespace in value", + value: "label= value", + }, + { + name: "no value", + value: "label", + }, + { + name: "no key", + value: "=label", + expectedErr: `invalid label '=label': empty name`, + }, + { + name: "empty value", + value: "label=", + }, + { + name: "key value", + value: "key1=value1", + }, + { + name: "double equal-signs", + value: "key1=value1=value2", + }, + { + name: "multiple equal-signs", + value: "key1=value1=value2=value", + }, + { + name: "double quotes in key and value", + value: `key"with"quotes={"hello"}`, + }, + { + name: "double quotes around key and value", + value: `"quoted-label"="quoted value"`, + }, + { + name: "single quotes in key and value", + value: `key'with'quotes=hello'with'quotes`, + }, + { + name: "single quotes around key and value", + value: `'quoted-label'='quoted value''`, + }, } - if actual, err := ValidateLabel("key1=value1"); err != nil || actual != "key1=value1" { - t.Fatalf("Expected [key1=value1], got [%v,%v]", actual, err) - } - // Validate it's working with more than one = - if actual, err := ValidateLabel("key1=value1=value2"); err != nil { - t.Fatalf("Expected [key1=value1=value2], got [%v,%v]", actual, err) - } - // Validate it's working with one more - if actual, err := ValidateLabel("key1=value1=value2=value3"); err != nil { - t.Fatalf("Expected [key1=value1=value2=value2], got [%v,%v]", actual, err) + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + val, err := ValidateLabel(tc.value) + if tc.expectedErr != "" { + assert.Error(t, err, tc.expectedErr) + return + } + assert.NilError(t, err) + assert.Equal(t, val, tc.value) + }) } }