From f2424bd3753af96ab2be741ae55af11841a8f0bd Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Wed, 13 Feb 2019 16:47:30 +0100 Subject: [PATCH] Fix labels copying value from environment variables This patch fixes a bug where labels use the same behavior as `--env`, resulting in a value to be copied from environment variables with the same name as the label if no value is set (i.e. a simple key, no `=` sign, no value). An earlier pull request addressed similar cases for `docker run`; 2b17f4c8a8caad552025edb05a73db683fb8a5c6, but this did not address the same situation for (e.g.) `docker service create`. Digging in history for this bug, I found that use of the `ValidateEnv` function for labels was added in the original implementation of the labels feature in https://github.com/docker/docker/commit/abb5e9a0777469e64fe2c7ecfa66ea01083d2071#diff-ae476143d40e21ac0918630f7365ed3cR34 However, the design never intended it to expand environment variables, and use of this function was either due to either a "copy/paste" of the equivalent `--env` flags, or a misunderstanding (the name `ValidateEnv` does not communicate that it also expands environment variables), and the existing `ValidateLabel` was designed for _engine_ labels (which required a value to be set). Following the initial implementation, other parts of the code followed the same (incorrect) approach, therefore leading the bug to be introduced in services as well. This patch: - updates the `ValidateLabel` to match the expected validation rules (this function is no longer used since 31dc5c0a9a8bdc11c7ad335aebb753ed527caa5a), and the daemon has its own implementation) - corrects various locations in the code where `ValidateEnv` was used instead of `ValidateLabel`. Before this patch: ```bash export SOME_ENV_VAR=I_AM_SOME_ENV_VAR docker service create --label SOME_ENV_VAR --tty --name test busybox docker service inspect --format '{{json .Spec.Labels}}' test {"SOME_ENV_VAR":"I_AM_SOME_ENV_VAR"} ``` After this patch: ```bash export SOME_ENV_VAR=I_AM_SOME_ENV_VAR docker service create --label SOME_ENV_VAR --tty --name test busybox docker container inspect --format '{{json .Config.Labels}}' test {"SOME_ENV_VAR":""} ``` Signed-off-by: Sebastiaan van Stijn --- cli/command/config/create.go | 2 +- cli/command/image/build.go | 2 +- cli/command/network/create.go | 2 +- cli/command/secret/create.go | 2 +- cli/command/service/opts.go | 4 +- cli/command/volume/create.go | 2 +- opts/file.go | 2 +- opts/opts.go | 20 +++++- opts/opts_test.go | 111 ++++++++++++++++++++++++++++++---- 9 files changed, 124 insertions(+), 23 deletions(-) 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/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/file.go b/opts/file.go index 1721a46ef9..96c0c7b84e 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 { diff --git a/opts/opts.go b/opts/opts.go index 6485e309e5..7e10a0c4f5 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("empty label name: '%s'", val) + } + if strings.ContainsAny(key, whiteSpaces) { + return "", fmt.Errorf("label '%s' has white spaces", key) } return val, nil } diff --git a/opts/opts_test.go b/opts/opts_test.go index 723a212ea7..664c1010d2 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: `empty label name: ''`, + }, + { + name: "whitespace only ", + value: " ", + expectedErr: `empty label name: ' '`, + }, + { + name: "whitespace around equal-sign", + value: " = ", + expectedErr: `empty label 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' has white spaces`, + }, + { + name: "whitespaces in key", + value: "this is a label=value", + expectedErr: `label 'this is a label' has white spaces`, + }, + { + 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: `empty label name: '=label'`, + }, + { + 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) + }) } }