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`;
2b17f4c8a8, 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
abb5e9a077 (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 31dc5c0a9a),
  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 <github@gone.nl>

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{

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

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

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{

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

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{

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 {

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
 }

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