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

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