Merge pull request #1671 from thaJeztah/fix_labels_expanding_env_vars

Fix labels copying value from environment variables

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

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/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())
 	}

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

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 {
+	arr := strings.SplitN(val, "=", 2)
-		return "", fmt.Errorf("bad attribute format: %s", val)
+	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
 }

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" {
+	tests := []struct {
-		t.Fatalf("Expected an error [bad attribute format: label], go %v", err)
+		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)
+	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
 			}
-	// Validate it's working with more than one =
+			assert.NilError(t, err)
-	if actual, err := ValidateLabel("key1=value1=value2"); err != nil {
+			assert.Equal(t, val, tc.value)
-		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)
 	}
 }