feature: make env support inputting empty value when creation

Signed-off-by: Allen Sun <[email protected]>

Author: allencloud

apis/opts/env.go

@@ -5,20 +5,60 @@ import (
 	"strings"
 )
 
-// ParseEnv parses the env param of container.
-func ParseEnv(env []string) (map[string]string, error) {
-	results := make(map[string]string)
-	for _, e := range env {
-		fields := strings.SplitN(e, "=", 2)
-		if len(fields) != 2 {
-			return nil, fmt.Errorf("invalid env %s: env must be in format of key=value", e)
+// ParseEnvs parses the env slice in container's config.
+func ParseEnvs(envs []string) ([]string, error) {
+	results := []string{}
+	for _, env := range envs {
+		result, err := parseEnv(env)
+		if err != nil {
+			return nil, err
 		}
-		results[fields[0]] = fields[1]
+		results = append(results, result)
 	}
 
 	return results, nil
 }
 
+// parseEnv parses single elements of env slice.
+func parseEnv(env string) (string, error) {
+	env = strings.TrimSpace(env)
+	if len(env) == 0 {
+		return "", fmt.Errorf("invalid env: env cannot be empty")
+	}
+
+	arr := strings.SplitN(env, "=", 2)
+	if arr[0] == "" {
+		return "", fmt.Errorf("invalid env %s: key of env cannot be empty", env)
+	}
+
+	if len(arr) == 1 {
+		// no matter it is "KEY=" or just "KEY", both are valid.
+		return env, nil
+	}
+
+	return fmt.Sprintf("%s=%s", arr[0], arr[1]), nil
+}
+
+// ValidSliceEnvsToMap converts slice envs to be map
+// assuming that the input are always valid with a char of '='.
+func ValidSliceEnvsToMap(envs []string) map[string]string {
+	results := make(map[string]string)
+	for _, env := range envs {
+		arr := strings.SplitN(env, "=", 2)
+		results[arr[0]] = arr[1]
+	}
+	return results
+}
+
+// ValidMapEnvsToSlice converts valid map envs to slice.
+func ValidMapEnvsToSlice(envs map[string]string) []string {
+	results := make([]string, 0)
+	for key, value := range envs {
+		results = append(results, fmt.Sprintf("%s=%s", key, value))
+	}
+	return results
+}
+
 // ValidateEnv verifies the correct of env
 func ValidateEnv(map[string]string) error {
 	// TODO

apis/opts/env_test.go

@@ -6,32 +6,29 @@ import (
 )
 
 func TestParseEnv(t *testing.T) {
-	type args struct {
-		env []string
-	}
 	tests := []struct {
 		name    string
-		args    args
-		want    map[string]string
+		args    string
+		want    string
 		wantErr bool
 	}{
-		{"test single env", args{env: []string{"foo=bar"}}, map[string]string{"foo": "bar"}, false},
-		{"test error env format", args{env: []string{"ErrorInfo"}}, nil, true},
-		{"test multiple envs", args{env: []string{"foo=bar", "A=a"}}, map[string]string{"foo": "bar", "A": "a"}, false},
-		{"test multiple '=' envs", args{env: []string{"A=1=2"}}, map[string]string{"A": "1=2"}, false},
-		{"test nil env", args{env: nil}, map[string]string{}, false}, // empty map
-		{"test empty env", args{env: []string{""}}, nil, true},
-		{"test empty blank", args{env: []string{"  "}}, nil, true},
+		{"test single env", "foo=bar", "foo=bar", false},
+		{"test str with no =", "NOEQUALMARK", "NOEQUALMARK", false},
+		{"test multiple '=' envs", "A=1=2", "A=1=2", false},
+		{"test empty blank in value", "A=B C", "A=B C", false},
+		{"test only =", "=", "", true},
+		{"test empty env", "", "", true}, // empty map
+		{"test empty blank", "  ", "", true},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got, err := ParseEnv(tt.args.env)
+			got, err := parseEnv(tt.args)
 			if (err != nil) != tt.wantErr {
-				t.Errorf("ParseEnv() error = %v, wantErr %v", err, tt.wantErr)
+				t.Errorf("parseEnv() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
 			if !reflect.DeepEqual(got, tt.want) {
-				t.Errorf("ParseEnv() = %v, want %v", got, tt.want)
+				t.Errorf("parseEnv() = %v, want %v", got, tt.want)
 			}
 		})
 	}

apis/swagger.yml

@@ -2149,7 +2149,9 @@ definitions:
         x-nullable: false
       Env:
         description: |
-          A list of environment variables to set inside the container in the form `["VAR=value", ...]`. A variable without `=` is removed from the environment, rather than to have an empty value.
+          A list of environment variables to set inside the container in the form `["VAR=value", ...]`. 
+          A variable like "A=" means setting env A in container to be empty value.
+          And a variable without `=` is removed from the environment, rather than to have an empty value.
         type: "array"
         items:
           type: "string"
@@ -2505,7 +2507,9 @@ definitions:
             $ref: "#/definitions/RestartPolicy"
           Env:
             description: |
-              A list of environment variables to set inside the container in the form `["VAR=value", ...]`. A variable without `=` is removed from the environment, rather than to have an empty value.
+              A list of environment variables to set inside the container in the form `["VAR=value", ...]`.
+              A variable like "A=" means updating env A in container to be empty value.
+              A variable without `=` is removed from the environment, rather than to have an empty value.
             type: "array"
             items:
               type: "string"

cli/common_flags.go

@@ -39,7 +39,7 @@ func addCommonFlags(flagSet *pflag.FlagSet) *container {
 
 	flagSet.BoolVar(&c.enableLxcfs, "enableLxcfs", false, "Enable lxcfs for the container, only effective when enable-lxcfs switched on in Pouchd")
 	flagSet.StringVar(&c.entrypoint, "entrypoint", "", "Overwrite the default ENTRYPOINT of the image")
-	flagSet.StringArrayVarP(&c.env, "env", "e", nil, "Set environment variables for container")
+	flagSet.StringArrayVarP(&c.env, "env", "e", nil, "Set environment variables for container('--env A=' means setting env A to empty, '--env B' means removing env B from container env inherited from image)")
 	flagSet.StringVar(&c.hostname, "hostname", "", "Set container's hostname")
 	flagSet.BoolVar(&c.disableNetworkFiles, "disable-network-files", false, "Disable the generation of network files(/etc/hostname, /etc/hosts and /etc/resolv.conf) for container. If true, no network files will be generated. Default false")
 

cli/update.go

@@ -51,8 +51,8 @@ func (uc *UpdateCommand) addFlags() {
 	flagSet.StringVar(&uc.cpusetmems, "cpuset-mems", "", "MEMs in cpuset which to allow execution (0-3, 0, 1)")
 	flagSet.StringVarP(&uc.memory, "memory", "m", "", "Container memory limit")
 	flagSet.StringVar(&uc.memorySwap, "memory-swap", "", "Container swap limit")
-	flagSet.StringSliceVarP(&uc.env, "env", "e", nil, "Set environment variables for container")
-	flagSet.StringSliceVarP(&uc.labels, "label", "l", nil, "Set label for container")
+	flagSet.StringSliceVarP(&uc.env, "env", "e", nil, "Update environment variables for container('--env A=' means updating env A to be empty and '--env A' means removing env A)")
+	flagSet.StringSliceVarP(&uc.labels, "label", "l", nil, "Update labels for container")
 	flagSet.StringVar(&uc.restartPolicy, "restart", "", "Restart policy to apply when container exits")
 	flagSet.StringSliceVar(&uc.diskQuota, "disk-quota", nil, "Update disk quota for container(/=10g)")
 	flagSet.StringSliceVar(&uc.specAnnotation, "annotation", nil, "Update annotation for runtime spec")

daemon/mgr/container_utils.go

@@ -269,46 +269,49 @@ func amendContainerSettings(config *types.ContainerConfig, hostConfig *types.Hos
 	}
 }
 
-func mergeEnvSlice(newEnv, oldEnv []string) ([]string, error) {
+// mergeEnvSlice merges two parts into a singe one.
+// Here are some cases:
+// 1. container creation needs to merge user input envs and envs inherited from image;
+// 2. update action with env needs to merge original envs and the user input envs;
+// 3. exec action needs to merge container's original envs and user input envs.
+func mergeEnvSlice(new, old []string) ([]string, error) {
 	// if newEnv is empty, return old env slice
-	if len(newEnv) == 0 {
-		return oldEnv, nil
+	if len(new) == 0 {
+		return old, nil
 	}
 
-	newEnvMap, err := opts.ParseEnv(newEnv)
+	newEnvs, err := opts.ParseEnvs(new)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to parse new env")
 	}
 
-	oldEnvMap, err := opts.ParseEnv(oldEnv)
+	// TODO: do we need to valid the old one?
+	oldEnvs, err := opts.ParseEnvs(old)
 	if err != nil {
 		return nil, errors.Wrapf(err, "failed to parse old env")
 	}
 
-	for k, v := range newEnvMap {
-		// key should not be empty
-		if k == "" {
-			continue
-		}
-
-		// add or change an env
-		if v != "" {
-			oldEnvMap[k] = v
-			continue
-		}
-
-		// value is empty, we need delete the env
-		if _, exists := oldEnvMap[k]; exists {
-			delete(oldEnvMap, k)
+	oldEnvsMap := opts.ValidSliceEnvsToMap(oldEnvs)
+
+	for _, env := range newEnvs {
+		arr := strings.SplitN(env, "=", 2)
+		if len(arr) == 1 {
+			// there are two cases, the first is 'KEY=', the second is just 'KEY'
+			if len(env) == len(arr[0]) {
+				// the case of 'KEY'. It is valid to remove the env from original one.
+				if _, exits := oldEnvsMap[arr[0]]; exits {
+					delete(oldEnvsMap, arr[0])
+				}
+			} else {
+				// the case of 'KEY='. It is valid to set env value empty.
+				oldEnvsMap[arr[0]] = ""
+			}
+		} else {
+			oldEnvsMap[arr[0]] = arr[1]
 		}
 	}
 
-	newEnvSlice := []string{}
-	for k, v := range oldEnvMap {
-		newEnvSlice = append(newEnvSlice, fmt.Sprintf("%s=%s", k, v))
-	}
-
-	return newEnvSlice, nil
+	return opts.ValidMapEnvsToSlice(oldEnvsMap), nil
 }
 
 func mergeAnnotation(newAnnotation, oldAnnotation map[string]string) map[string]string {

daemon/mgr/container_utils_test.go

@@ -200,9 +200,8 @@ func Test_mergeEnvSlice(t *testing.T) {
 		want    []string
 		wantErr bool
 	}{
-		// TODO: Add test cases.
 		{
-			name: "test1",
+			name: "normal merge with adding ones",
 			args: args{
 				newEnv: []string{"a=b"},
 				oldEnv: []string{"c=d"},
@@ -211,7 +210,7 @@ func Test_mergeEnvSlice(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "test2",
+			name: "normal merge with updating existing one",
 			args: args{
 				newEnv: []string{"test=false"},
 				oldEnv: []string{"test=true"},
@@ -220,16 +219,16 @@ func Test_mergeEnvSlice(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "test3",
+			name: "normal merge with removing one",
 			args: args{
-				newEnv: []string{"wrong-format"},
-				oldEnv: []string{"c=d"},
+				newEnv: []string{"JUST_KEY"},
+				oldEnv: []string{"c=d", "JUST_KEY=VALUE"},
 			},
-			want:    nil,
-			wantErr: true,
+			want:    []string{"c=d"},
+			wantErr: false,
 		},
 		{
-			name: "test4",
+			name: "normal merge with nothing in new",
 			args: args{
 				newEnv: []string{},
 				oldEnv: []string{"c=d"},
@@ -238,23 +237,41 @@ func Test_mergeEnvSlice(t *testing.T) {
 			wantErr: false,
 		},
 		{
-			name: "test5",
+			name: "normal merge with updating existing key to empty",
 			args: args{
-				newEnv: []string{"a=b"},
-				oldEnv: []string{},
+				newEnv: []string{"a="},
+				oldEnv: []string{"a=b"},
 			},
-			want:    []string{"a=b"},
+			want:    []string{"a="},
 			wantErr: false,
 		},
 		{
-			name: "test6",
+			name: "normal merge with adding env with whitespace in value",
 			args: args{
-				newEnv: []string{"a="},
-				oldEnv: []string{"a=b"},
+				newEnv: []string{"a=b c d "},
+				oldEnv: []string{"c=b"},
 			},
-			want:    []string{},
+			want:    []string{"a=b c d", "c=b"},
 			wantErr: false,
 		},
+		{
+			name: "illegal merge with invalid in new ones",
+			args: args{
+				newEnv: []string{"="},
+				oldEnv: []string{"a=b"},
+			},
+			want:    nil,
+			wantErr: true,
+		},
+		{
+			name: "illegal merge with empty element in new ones",
+			args: args{
+				newEnv: []string{"", ""},
+				oldEnv: []string{"a=b"},
+			},
+			want:    nil,
+			wantErr: true,
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {