Preserve environment variable order (elastic#2341)

pkg/controller/apmserver/apmserver_controller_test.go

@@ -75,7 +75,7 @@ func (tp testParams) withInitContainer() testParams {
 			},
 			Name:  "",
 			Image: "docker.elastic.co/apm/apm-server:1.0",
-			Env:   defaults.PodDownwardEnvVars,
+			Env:   defaults.PodDownwardEnvVars(),
 		},
 	}
 	return tp
@@ -152,7 +152,7 @@ func expectedDeploymentParams() testParams {
 							"-c",
 							"config/config-secret/apm-server.yml",
 						},
-						Env: append(defaults.PodDownwardEnvVars, corev1.EnvVar{
+						Env: defaults.ExtendPodDownwardEnvVars(corev1.EnvVar{
 							Name: "SECRET_TOKEN",
 							ValueFrom: &corev1.EnvVarSource{
 								SecretKeyRef: &corev1.SecretKeySelector{

pkg/controller/apmserver/pod.go

@@ -100,7 +100,7 @@ func newPodSpec(as *apmv1.ApmServer, p PodSpecParams) corev1.PodTemplateSpec {
 		filepath.Join(ConfigVolumePath, "config-secret"),
 	)
 
-	env := append(defaults.PodDownwardEnvVars, corev1.EnvVar{
+	env := defaults.ExtendPodDownwardEnvVars(corev1.EnvVar{
 		Name: "SECRET_TOKEN",
 		ValueFrom: &corev1.EnvVarSource{
 			SecretKeyRef: &corev1.SecretKeySelector{

pkg/controller/common/defaults/pod_template.go

@@ -12,17 +12,23 @@ import (
 	"github.com/elastic/cloud-on-k8s/pkg/controller/elasticsearch/settings"
 )
 
-var (
-	// PodDownwardEnvVars inject the runtime Pod Name and IP as environment variables.
-	PodDownwardEnvVars = []corev1.EnvVar{
+// PodDownwardEnvVars returns default environment variables created from the downward API.
+func PodDownwardEnvVars() []corev1.EnvVar {
+	return []corev1.EnvVar{
 		{Name: settings.EnvPodIP, Value: "", ValueFrom: &corev1.EnvVarSource{
 			FieldRef: &corev1.ObjectFieldSelector{APIVersion: "v1", FieldPath: "status.podIP"},
 		}},
 		{Name: settings.EnvPodName, Value: "", ValueFrom: &corev1.EnvVarSource{
 			FieldRef: &corev1.ObjectFieldSelector{APIVersion: "v1", FieldPath: "metadata.name"},
 		}},
 	}
-)
+}
+
+// ExtendPodDownwardEnvVars creates a new EnvVar array with the default downward API variables prepended to given list.
+func ExtendPodDownwardEnvVars(vars ...corev1.EnvVar) []corev1.EnvVar {
+	podDownwardEnvVars := PodDownwardEnvVars()
+	return append(podDownwardEnvVars, vars...)
+}
 
 // PodTemplateBuilder helps with building a pod template inheriting values
 // from a user-provided pod template. It focuses on building a pod with
@@ -204,10 +210,6 @@ func (b *PodTemplateBuilder) WithEnv(vars ...corev1.EnvVar) *PodTemplateBuilder
 			b.Container.Env = append(b.Container.Env, v)
 		}
 	}
-	// order env vars by name to ensure stable pod spec comparison
-	sort.SliceStable(b.Container.Env, func(i, j int) bool {
-		return b.Container.Env[i].Name < b.Container.Env[j].Name
-	})
 	return b
 }
 
@@ -259,7 +261,7 @@ func (b *PodTemplateBuilder) WithInitContainerDefaults() *PodTemplateBuilder {
 		}
 
 		// append the dynamic pod name and IP env vars
-		c.Env = append(c.Env, PodDownwardEnvVars...)
+		c.Env = append(c.Env, PodDownwardEnvVars()...)
 	}
 	return b
 }

pkg/controller/common/defaults/pod_template_test.go

@@ -597,10 +597,10 @@ func TestPodTemplateBuilder_WithEnv(t *testing.T) {
 			want:        []corev1.EnvVar{{Name: "var1"}, {Name: "var2"}},
 		},
 		{
-			name:        "env vars should be sorted alphabetically",
+			name:        "env var order should be preserved",
 			PodTemplate: corev1.PodTemplateSpec{},
 			vars:        []corev1.EnvVar{{Name: "cc"}, {Name: "aa"}, {Name: "bb"}},
-			want:        []corev1.EnvVar{{Name: "aa"}, {Name: "bb"}, {Name: "cc"}},
+			want:        []corev1.EnvVar{{Name: "cc"}, {Name: "aa"}, {Name: "bb"}},
 		},
 		{
 			name: "append to but don't override user provided env vars",
@@ -754,13 +754,13 @@ func TestPodTemplateBuilder_WithInitContainerDefaults(t *testing.T) {
 				{
 					Name:         "user-init-container1",
 					Image:        "user-image",
-					Env:          PodDownwardEnvVars,
+					Env:          PodDownwardEnvVars(),
 					VolumeMounts: defaultVolumeMounts,
 				},
 				{
 					Name:  "user-init-container2",
 					Image: "default-image",
-					Env:   PodDownwardEnvVars,
+					Env:   PodDownwardEnvVars(),
 					VolumeMounts: []corev1.VolumeMount{{
 						Name:      "foo",
 						MountPath: "/foo",
@@ -770,7 +770,7 @@ func TestPodTemplateBuilder_WithInitContainerDefaults(t *testing.T) {
 				{
 					Name:  "user-init-container3",
 					Image: "default-image",
-					Env:   PodDownwardEnvVars,
+					Env:   PodDownwardEnvVars(),
 					// uses the same mount path as a default mount, so default mount should not be used
 					VolumeMounts: []corev1.VolumeMount{{
 						Name:      "bar",
@@ -780,7 +780,7 @@ func TestPodTemplateBuilder_WithInitContainerDefaults(t *testing.T) {
 				{
 					Name:  "user-init-container4",
 					Image: "default-image",
-					Env:   PodDownwardEnvVars,
+					Env:   PodDownwardEnvVars(),
 					// uses the same name as a default mount, so default mount should not be used
 					VolumeMounts: []corev1.VolumeMount{{
 						Name:      defaultVolumeMount.Name,

pkg/controller/elasticsearch/initcontainer/prepare_fs.go

@@ -121,7 +121,7 @@ func NewPrepareFSInitContainer(
 		SecurityContext: &corev1.SecurityContext{
 			Privileged: &privileged,
 		},
-		Env:     defaults.PodDownwardEnvVars,
+		Env:     defaults.PodDownwardEnvVars(),
 		Command: []string{"bash", "-c", path.Join(esvolume.ScriptsVolumeMountPath, PrepareFsScriptConfigKey)},
 		VolumeMounts: append(
 			PluginVolumes.InitContainerVolumeMounts(),

pkg/controller/elasticsearch/nodespec/defaults.go

@@ -53,8 +53,7 @@ var (
 
 // DefaultEnvVars are environment variables injected into Elasticsearch pods.
 func DefaultEnvVars(httpCfg commonv1.HTTPConfig, headlessServiceName string) []corev1.EnvVar {
-	return append(
-		defaults.PodDownwardEnvVars,
+	return defaults.ExtendPodDownwardEnvVars(
 		[]corev1.EnvVar{
 			{Name: settings.EnvProbePasswordPath, Value: path.Join(esvolume.ProbeUserSecretMountPath, user.InternalProbeUserName)},
 			{Name: settings.EnvProbeUsername, Value: user.InternalProbeUserName},

pkg/controller/elasticsearch/nodespec/podspec_test.go

@@ -117,7 +117,7 @@ func TestBuildPodTemplateSpec(t *testing.T) {
 	require.NoError(t, err)
 	// should be patched with volume and env
 	for i := range initContainers {
-		initContainers[i].Env = append(initContainers[i].Env, defaults.PodDownwardEnvVars...)
+		initContainers[i].Env = append(initContainers[i].Env, defaults.PodDownwardEnvVars()...)
 		initContainers[i].VolumeMounts = append(initContainers[i].VolumeMounts, volumeMounts...)
 	}
 
@@ -158,7 +158,7 @@ func TestBuildPodTemplateSpec(t *testing.T) {
 			InitContainers: append(initContainers, corev1.Container{
 				Name:         "additional-init-container",
 				Image:        "docker.elastic.co/elasticsearch/elasticsearch:7.2.0",
-				Env:          defaults.PodDownwardEnvVars,
+				Env:          defaults.PodDownwardEnvVars(),
 				VolumeMounts: volumeMounts,
 			}),
 			Containers: []corev1.Container{
@@ -173,8 +173,8 @@ func TestBuildPodTemplateSpec(t *testing.T) {
 						{Name: "transport", HostPort: 0, ContainerPort: 9300, Protocol: "TCP", HostIP: ""},
 					},
 					Env: append(
-						DefaultEnvVars(sampleES.Spec.HTTP, HeadlessServiceName(esv1.StatefulSet(sampleES.Name, nodeSet.Name))),
-						corev1.EnvVar{Name: "my-env", Value: "my-value"}),
+						[]corev1.EnvVar{{Name: "my-env", Value: "my-value"}},
+						DefaultEnvVars(sampleES.Spec.HTTP, HeadlessServiceName(esv1.StatefulSet(sampleES.Name, nodeSet.Name)))...),
 					Resources:      DefaultResources,
 					VolumeMounts:   volumeMounts,
 					ReadinessProbe: NewReadinessProbe(),
@@ -189,13 +189,6 @@ func TestBuildPodTemplateSpec(t *testing.T) {
 		},
 	}
 
-	// pods built with BuildPodTemplateSpec sort env vars, so our expected result must be sorted as well.
-	for _, container := range expected.Spec.Containers {
-		sort.SliceStable(container.Env, func(i, j int) bool {
-			return container.Env[i].Name < container.Env[j].Name
-		})
-	}
-
 	deep.MaxDepth = 25
 	require.Nil(t, deep.Equal(expected, actual))
 }