bugfix: merge env wrong when create container

Signed-off-by: Michael Wan <zirenwan@gmail.com>

daemon/mgr/container.go

@@ -1036,42 +1036,11 @@ func (mgr *ContainerManager) Update(ctx context.Context, name string, config *ty
 	}
 
 	// Update Env
-	if len(config.Env) > 0 {
-		newEnvMap, err := opts.ParseEnv(config.Env)
-		if err != nil {
-			return errors.Wrapf(err, "failed to parse new env")
-		}
-
-		oldEnvMap, err := opts.ParseEnv(c.Config.Env)
-		if err != nil {
-			return 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)
-			}
-		}
-
-		newEnvSlice := []string{}
-		for k, v := range oldEnvMap {
-			newEnvSlice = append(newEnvSlice, fmt.Sprintf("%s=%s", k, v))
-		}
-
-		c.Config.Env = newEnvSlice
+	newEnvSlice, err := mergeEnvSlice(config.Env, c.Config.Env)
+	if err != nil {
+		return err
 	}
+	c.Config.Env = newEnvSlice
 
 	// If container is not running, update container metadata struct is enough,
 	// resources will be updated when the container is started again,

daemon/mgr/container_types.go

@@ -287,11 +287,13 @@ func (c *Container) merge(getconfig func() (v1.ImageConfig, error)) error {
 		}
 		c.Config.Entrypoint = config.Entrypoint
 	}
-	if c.Config.Env == nil {
-		c.Config.Env = config.Env
-	} else {
-		c.Config.Env = append(c.Config.Env, config.Env...)
+
+	// ContainerConfig.Env is new, and the ImageConfig.Env is old
+	newEnvSlice, err := mergeEnvSlice(c.Config.Env, config.Env)
+	if err != nil {
+		return err
 	}
+	c.Config.Env = newEnvSlice
 	if c.Config.WorkingDir == "" {
 		c.Config.WorkingDir = config.WorkingDir
 	}

daemon/mgr/container_utils.go

@@ -6,6 +6,7 @@ import (
 	"strconv"
 	"strings"
 
+	"github.com/alibaba/pouch/apis/opts"
 	"github.com/alibaba/pouch/apis/types"
 	networktypes "github.com/alibaba/pouch/network/types"
 	"github.com/alibaba/pouch/pkg/errtypes"
@@ -256,3 +257,45 @@ func amendContainerSettings(config *types.ContainerConfig, hostConfig *types.Hos
 		r.MemorySwap = 2 * r.Memory
 	}
 }
+
+func mergeEnvSlice(newEnv, oldEnv []string) ([]string, error) {
+	// if newEnv is empty, return old env slice
+	if len(newEnv) == 0 {
+		return oldEnv, nil
+	}
+
+	newEnvMap, err := opts.ParseEnv(newEnv)
+	if err != nil {
+		return nil, errors.Wrapf(err, "failed to parse new env")
+	}
+
+	oldEnvMap, err := opts.ParseEnv(oldEnv)
+	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)
+		}
+	}
+
+	newEnvSlice := []string{}
+	for k, v := range oldEnvMap {
+		newEnvSlice = append(newEnvSlice, fmt.Sprintf("%s=%s", k, v))
+	}
+
+	return newEnvSlice, nil
+}

daemon/mgr/container_utils_test.go

@@ -187,3 +187,84 @@ func Test_parsePSOutput(t *testing.T) {
 		})
 	}
 }
+
+func Test_mergeEnvSlice(t *testing.T) {
+	type args struct {
+		newEnv []string
+		oldEnv []string
+	}
+	tests := []struct {
+		name    string
+		args    args
+		want    []string
+		wantErr bool
+	}{
+		// TODO: Add test cases.
+		{
+			name: "test1",
+			args: args{
+				newEnv: []string{"a=b"},
+				oldEnv: []string{"c=d"},
+			},
+			want:    []string{"c=d", "a=b"},
+			wantErr: false,
+		},
+		{
+			name: "test2",
+			args: args{
+				newEnv: []string{"test=false"},
+				oldEnv: []string{"test=true"},
+			},
+			want:    []string{"test=false"},
+			wantErr: false,
+		},
+		{
+			name: "test3",
+			args: args{
+				newEnv: []string{"wrong-format"},
+				oldEnv: []string{"c=d"},
+			},
+			want:    nil,
+			wantErr: true,
+		},
+		{
+			name: "test4",
+			args: args{
+				newEnv: []string{},
+				oldEnv: []string{"c=d"},
+			},
+			want:    []string{"c=d"},
+			wantErr: false,
+		},
+		{
+			name: "test5",
+			args: args{
+				newEnv: []string{"a=b"},
+				oldEnv: []string{},
+			},
+			want:    []string{"a=b"},
+			wantErr: false,
+		},
+		{
+			name: "test6",
+			args: args{
+				newEnv: []string{"a="},
+				oldEnv: []string{"a=b"},
+			},
+			want:    []string{},
+			wantErr: false,
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := mergeEnvSlice(tt.args.newEnv, tt.args.oldEnv)
+			if (err != nil) != tt.wantErr {
+				t.Errorf("mergeEnvSlice() error = %v, wantErr %v", err, tt.wantErr)
+				return
+			}
+			if !reflect.DeepEqual(got, tt.want) {
+				t.Errorf("mergeEnvSlice() = %v, want %v", got, tt.want)
+			}
+		})
+	}
+}