diff --git a/apis/opts/env.go b/apis/opts/env.go index 6064b1e31..6945bd60b 100644 --- a/apis/opts/env.go +++ b/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 diff --git a/apis/opts/env_test.go b/apis/opts/env_test.go index deeff4f49..af7ed2b5a 100644 --- a/apis/opts/env_test.go +++ b/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) } }) } diff --git a/apis/swagger.yml b/apis/swagger.yml index 505fca798..9dea23df4 100644 --- a/apis/swagger.yml +++ b/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" diff --git a/cli/common_flags.go b/cli/common_flags.go index a34ee99e8..ac62319c8 100644 --- a/cli/common_flags.go +++ b/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") diff --git a/cli/update.go b/cli/update.go index 98808ce8e..cac886d25 100644 --- a/cli/update.go +++ b/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") diff --git a/daemon/mgr/container_utils.go b/daemon/mgr/container_utils.go index 9f12c1fe6..53c1ee192 100644 --- a/daemon/mgr/container_utils.go +++ b/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 { diff --git a/daemon/mgr/container_utils_test.go b/daemon/mgr/container_utils_test.go index 78ca305b0..24726b25b 100644 --- a/daemon/mgr/container_utils_test.go +++ b/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) { diff --git a/test/cli_create_test.go b/test/cli_create_test.go index dacf8c221..548258eaf 100644 --- a/test/cli_create_test.go +++ b/test/cli_create_test.go @@ -300,25 +300,64 @@ func (suite *PouchCreateSuite) TestCreateEnableLxcfs(c *check.C) { func (suite *PouchCreateSuite) TestCreateWithEnv(c *check.C) { name := "TestCreateWithEnv" - res := command.PouchRun("create", "--name", name, "-e TEST=true", busyboxImage) + env1 := "TEST1=true" + env2 := "TEST2=" // should be in inspect result as TEST2=, and still in container's real env as TEST2= + env3 := "TEST3" // should not in container's real env + env4 := "TEST4=a b" // valid + env5 := "TEST5=a=b" // valid + res := command.PouchRun("run", "-d", + "--name", name, + "-e", env1, + "-e", env2, + "-e", env3, + "-e", env4, + "-e", env5, + busyboxImage, + "top") defer DelContainerForceMultyTime(c, name) res.Assert(c, icmd.Success) - output := command.PouchRun("inspect", name).Stdout() + envs, err := inspectFilter(name, ".Config.Env") + c.Assert(err, check.IsNil) - result := []types.ContainerJSON{} - if err := json.Unmarshal([]byte(output), &result); err != nil { - c.Errorf("failed to decode inspect output: %v", err) + // check if these envs are in inspect result of container. + if !strings.Contains(envs, env1) { + c.Fatalf("container env in inspect result should have %s in %s while no\n", env1, envs) + } + if !strings.Contains(envs, env2) { + c.Fatalf("container env in inspect result should have %s in %s while no\n", env2, envs) + } + if strings.Contains(envs, env3) { + c.Fatalf("container env in inspect result should not have %s in %s, while it has\n", env3, envs) + } + if !strings.Contains(envs, env4) { + c.Fatalf("container env in inspect result should have %s in %s while no\n", env4, envs) + } + if !strings.Contains(envs, env5) { + c.Fatalf("container env in inspect result should have %s in %s while no\n", env5, envs) } - ok := false - for _, v := range result[0].Config.Env { - if strings.Contains(v, "TEST=true") { - ok = true - } + // check if these envs are in the real container envs + ret := command.PouchRun("exec", name, "env") + envs = ret.Stdout() + + if !strings.Contains(envs, env1) { + c.Fatalf("container's runtime env should have %s in %s while no\n", env1, envs) + } + if !strings.Contains(envs, env2) { + c.Fatalf("container's runtime env should have %s in %s while no\n", env2, envs) + } + // container's runtime env should not have env3 + if strings.Contains(envs, env3) { + c.Fatalf("container's runtime env should not have %s in %s while it is there\n", env3, envs) + } + if !strings.Contains(envs, env4) { + c.Fatalf("container's runtime env should have %s in %s while no\n", env4, envs) + } + if !strings.Contains(envs, env5) { + c.Fatalf("container's runtime env should have %s in %s while no\n", env5, envs) } - c.Assert(ok, check.Equals, true) } // TestCreateWithWorkDir tests creating container with a workdir works. diff --git a/test/cli_update_test.go b/test/cli_update_test.go index d5707deed..7b8380691 100644 --- a/test/cli_update_test.go +++ b/test/cli_update_test.go @@ -51,6 +51,9 @@ func (suite *PouchUpdateSuite) TestUpdateContainerNormalOption(c *check.C) { //"--cpuset-cpus", "0", //"--cpuset-mems", "0", // memory related options + "--env", "TEST1=foo1", + "--env", "TEST2=foo2", + "--env", "TEST3=foo3", "-m", "300M", busyboxImage, "top") @@ -71,8 +74,10 @@ func (suite *PouchUpdateSuite) TestUpdateContainerNormalOption(c *check.C) { // memory related update "-m", "500M", // env related update - // adding a new env - "--env", "foo=bar", + "--env", "TEST1=bar1", // updating env to new non-empty value + "--env", "TEST2=", // update env to empty + "--env", "TEST3", // update to remove the env + "--env", "TEST4=bar4", // adding a new env // label related update "--label", "foo=bar", name, @@ -192,31 +197,46 @@ func (suite *PouchUpdateSuite) TestUpdateContainerNormalOption(c *check.C) { c.Assert(cpuQuota, check.Equals, "524288000") } + // test value check about env { - // test value check about env and label - output := command.PouchRun("inspect", name).Stdout() - result := []types.ContainerJSON{} - if err := json.Unmarshal([]byte(output), &result); err != nil { - c.Errorf("failed to decode inspect output: %v", err) - } + envs, err := inspectFilter(name, ".Config.Env") + c.Assert(err, check.IsNil) - // test env - { - if !utils.StringInSlice(result[0].Config.Env, "foo=bar") { - c.Fatalf("expect 'foo=bar' in container env, but got: %v", result[0].Config.Env) - } + if !strings.Contains(envs, "TEST1=bar1") { + c.Fatalf("expect 'TEST1=bar1' in container env, but got: %s", envs) + } + if !strings.Contains(envs, "TEST2=") { + c.Fatalf("expect 'TEST2=' in container env, but got: %s", envs) + } + if strings.Contains(envs, "TEST3") { + c.Fatalf("expect no 'TEST3' in container env since using '--env TEST#' to remove, but got: %s", envs) + } + if !strings.Contains(envs, "TEST4=bar4") { + c.Fatalf("expect 'TEST4=bar4' in container env, but got: %s", envs) + } - output = command.PouchRun("exec", name, "env").Stdout() - if !strings.Contains(output, "foo=bar") { - c.Fatalf("Update running container env not worked") - } + output := command.PouchRun("exec", name, "env").Stdout() + if !strings.Contains(output, "TEST1=bar1") { + c.Fatalf("env TEST1=bar1 should be in container runtime env, while get: %v", output) + } + if !strings.Contains(output, "TEST2=") { + c.Fatalf("env TEST2= should be in container runtime env, while get: %v", output) } + if strings.Contains(output, "TEST3") { + c.Fatalf("env TEST3 should not be in container runtime env, while get: %v", output) + } + if !strings.Contains(output, "TEST4=bar4") { + c.Fatalf("env TEST4=bar4 should be in container runtime env, while get: %v", output) + } + } + + // test value check aboudoct labels + { + labels, err := inspectFilter(name, ".Config.Labels") + c.Assert(err, check.IsNil) - // test labels - { - if v, ok := result[0].Config.Labels["foo"]; !ok || v != "bar" { - c.Fatalf("expect 'foo=bar' in Labels, got: %v", result[0].Config.Labels) - } + if !strings.Contains(labels, "foo:bar") { + c.Fatalf("expect 'foo:bar' in Labels, got: %s", labels) } } }