should not change relative order of envs (#2088)

Co-authored-by: Yecheng Fu <[email protected]>
pingcap · Mar 31, 2020 · f7583a1 · f7583a1
1 parent 1ac8f7f
commit f7583a1
Show file tree

Hide file tree

Showing 7 changed files with 36 additions and 34 deletions.
diff --git a/pkg/manager/member/pd_member_manager.go b/pkg/manager/member/pd_member_manager.go
@@ -622,7 +622,7 @@ func getNewPDSetForTidbCluster(tc *v1alpha1.TidbCluster, cm *corev1.ConfigMap) (
 			},
 		})
 	}
-	pdContainer.Env = util.MergeEnv(basePDSpec.Env(), env)
+	pdContainer.Env = util.AppendEnv(env, basePDSpec.Env())
 	podSpec.Volumes = vols
 	podSpec.Containers = []corev1.Container{pdContainer}
 

diff --git a/pkg/manager/member/pd_member_manager_test.go b/pkg/manager/member/pd_member_manager_test.go
@@ -1081,17 +1081,6 @@ func TestGetNewPDSetForTidbCluster(t *testing.T) {
 				},
 			},
 			testSts: testPDContainerEnv(t, []corev1.EnvVar{
-				{
-					Name: "DASHBOARD_SESSION_SECRET",
-					ValueFrom: &corev1.EnvVarSource{
-						SecretKeyRef: &corev1.SecretKeySelector{
-							LocalObjectReference: corev1.LocalObjectReference{
-								Name: "dashboard-session-secret",
-							},
-							Key: "encryption_key",
-						},
-					},
-				},
 				{
 					Name: "NAMESPACE",
 					ValueFrom: &corev1.EnvVarSource{
@@ -1115,6 +1104,17 @@ func TestGetNewPDSetForTidbCluster(t *testing.T) {
 				{
 					Name: "TZ",
 				},
+				{
+					Name: "DASHBOARD_SESSION_SECRET",
+					ValueFrom: &corev1.EnvVarSource{
+						SecretKeyRef: &corev1.SecretKeySelector{
+							LocalObjectReference: corev1.LocalObjectReference{
+								Name: "dashboard-session-secret",
+							},
+							Key: "encryption_key",
+						},
+					},
+				},
 			}),
 		},
 		// TODO add more tests

diff --git a/pkg/manager/member/pump_member_manager.go b/pkg/manager/member/pump_member_manager.go
@@ -362,7 +362,7 @@ func getNewPumpStatefulSet(tc *v1alpha1.TidbCluster, cm *corev1.ConfigMap) (*app
 				ContainerPort: 8250,
 			}},
 			Resources:    controller.ContainerResource(tc.Spec.Pump.ResourceRequirements),
-			Env:          util.MergeEnv(spec.Env(), envs),
+			Env:          util.AppendEnv(envs, spec.Env()),
 			VolumeMounts: volumeMounts,
 		},
 	}

diff --git a/pkg/manager/member/tidb_member_manager.go b/pkg/manager/member/tidb_member_manager.go
@@ -654,7 +654,7 @@ func getNewTiDBSetForTidbCluster(tc *v1alpha1.TidbCluster, cm *corev1.ConfigMap)
 		},
 		VolumeMounts: volMounts,
 		Resources:    controller.ContainerResource(tc.Spec.TiDB.ResourceRequirements),
-		Env:          util.MergeEnv(baseTiDBSpec.Env(), envs),
+		Env:          util.AppendEnv(envs, baseTiDBSpec.Env()),
 		ReadinessProbe: &corev1.Probe{
 			Handler: corev1.Handler{
 				HTTPGet: &corev1.HTTPGetAction{

diff --git a/pkg/manager/member/tikv_member_manager.go b/pkg/manager/member/tikv_member_manager.go
@@ -454,7 +454,7 @@ func getNewTiKVSetForTidbCluster(tc *v1alpha1.TidbCluster, cm *corev1.ConfigMap)
 			},
 		})
 	}
-	tikvContainer.Env = util.MergeEnv(baseTiKVSpec.Env(), env)
+	tikvContainer.Env = util.AppendEnv(env, baseTiKVSpec.Env())
 	podSpec.Volumes = vols
 	podSpec.SecurityContext = podSecurityContext
 	podSpec.InitContainers = initContainers

diff --git a/pkg/util/util.go b/pkg/util/util.go
@@ -16,7 +16,6 @@ package util
 import (
 	"encoding/json"
 	"fmt"
-	"sort"
 	"strconv"
 	"strings"
 
@@ -196,19 +195,18 @@ func (e SortEnvByName) Less(i, j int) bool {
 	return e[i].Name < e[j].Name
 }
 
-// MergeEnv merges env in `b` to `a` and overrides env that has the same name.
-func MergeEnv(a []corev1.EnvVar, b []corev1.EnvVar) []corev1.EnvVar {
-	tmpEnv := make(map[string]corev1.EnvVar)
+// AppendEnv appends envs `b` into `a` ignoring envs whose names already exist
+// in `b`.
+// Note that this will not change relative order of envs.
+func AppendEnv(a []corev1.EnvVar, b []corev1.EnvVar) []corev1.EnvVar {
+	aMap := make(map[string]corev1.EnvVar)
 	for _, e := range a {
-		tmpEnv[e.Name] = e
+		aMap[e.Name] = e
 	}
 	for _, e := range b {
-		tmpEnv[e.Name] = e
-	}
-	c := make([]corev1.EnvVar, 0)
-	for _, e := range tmpEnv {
-		c = append(c, e)
+		if _, ok := aMap[e.Name]; !ok {
+			a = append(a, e)
+		}
 	}
-	sort.Sort(SortEnvByName(c))
-	return c
+	return a
 }
diff --git a/pkg/util/utils_test.go b/pkg/util/utils_test.go
@@ -128,15 +128,15 @@ func TestGetPodOrdinals(t *testing.T) {
 	}
 }
 
-func TestMergeEnv(t *testing.T) {
+func TestAppendEnv(t *testing.T) {
 	tests := []struct {
 		name string
 		a    []corev1.EnvVar
 		b    []corev1.EnvVar
 		want []corev1.EnvVar
 	}{
 		{
-			name: "b overrides a with the same name",
+			name: "envs whose names exist are ignored",
 			a: []corev1.EnvVar{
 				{
 					Name:  "foo",
@@ -156,27 +156,31 @@ func TestMergeEnv(t *testing.T) {
 					Name:  "new",
 					Value: "bar",
 				},
+				{
+					Name:  "xxx",
+					Value: "yyy",
+				},
 			},
 			want: []corev1.EnvVar{
 				{
 					Name:  "foo",
-					Value: "barbar",
-				},
-				{
-					Name:  "new",
 					Value: "bar",
 				},
 				{
 					Name:  "xxx",
 					Value: "xxx",
 				},
+				{
+					Name:  "new",
+					Value: "bar",
+				},
 			},
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			got := MergeEnv(tt.a, tt.b)
+			got := AppendEnv(tt.a, tt.b)
 			if diff := cmp.Diff(tt.want, got); diff != "" {
 				t.Errorf("unwant (-want, +got): %s", diff)
 			}