Skip to content

Commit

Permalink
Preserve environment variable order (#2341)
Browse files Browse the repository at this point in the history
  • Loading branch information
charith-elastic authored Jan 6, 2020
1 parent a786e23 commit 32f2a0b
Show file tree
Hide file tree
Showing 7 changed files with 26 additions and 32 deletions.
4 changes: 2 additions & 2 deletions pkg/controller/apmserver/apmserver_controller_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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{
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/apmserver/pod.go
Original file line number Diff line number Diff line change
Expand Up @@ -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{
Expand Down
20 changes: 11 additions & 9 deletions pkg/controller/common/defaults/pod_template.go
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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
}

Expand Down Expand Up @@ -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
}
Expand Down
12 changes: 6 additions & 6 deletions pkg/controller/common/defaults/pod_template_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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",
Expand Down Expand Up @@ -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",
Expand All @@ -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",
Expand All @@ -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,
Expand Down
2 changes: 1 addition & 1 deletion pkg/controller/elasticsearch/initcontainer/prepare_fs.go
Original file line number Diff line number Diff line change
Expand Up @@ -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(),
Expand Down
3 changes: 1 addition & 2 deletions pkg/controller/elasticsearch/nodespec/defaults.go
Original file line number Diff line number Diff line change
Expand Up @@ -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},
Expand Down
15 changes: 4 additions & 11 deletions pkg/controller/elasticsearch/nodespec/podspec_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -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...)
}

Expand Down Expand Up @@ -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{
Expand All @@ -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(),
Expand All @@ -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))
}

0 comments on commit 32f2a0b

Please sign in to comment.