Reorder environment variables (#570)

* reorder environment variables * fixes
solo-io · Jun 20, 2024 · 1583cb7 · 1583cb7
1 parent 47eb19b
commit 1583cb7
Show file tree

Hide file tree

Showing 20 changed files with 389 additions and 103 deletions.
diff --git a/changelog/v0.40.4/env-priority.yaml b/changelog/v0.40.4/env-priority.yaml
@@ -0,0 +1,8 @@
+changelog:
+  - type: NON_USER_FACING
+    description: >
+      Reorder the priority of the environment variables to be loaded in the following order:
+      1. Templated environment variables
+      2. Environment variables
+      3. Extra environment variables
+    skipCI: "false"
diff --git a/codegen/cmd_test.go b/codegen/cmd_test.go
@@ -41,6 +41,66 @@ var _ = Describe("Cmd", func() {
 	skv2Imports.External["github.com/solo-io/cue"] = []string{
 		"encoding/protobuf/cue/cue.proto",
 	}
+	It("env variable priority", func() {
+		cmd := &Command{
+			Chart: &Chart{
+				Data: Data{
+					ApiVersion:  "v1",
+					Description: "",
+					Name:        "Painting Operator",
+					Version:     "v0.0.1",
+					Home:        "https://docs.solo.io/skv2/latest",
+					Sources: []string{
+						"https://github.com/solo-io/skv2",
+					},
+				},
+				Operators: []Operator{{
+					Name: "painter",
+					Deployment: Deployment{
+						Container: Container{
+							Image: Image{Repository: "painter", Tag: "v0.0.1"},
+							Env:   []v1.EnvVar{{Name: "ENV_VAR", Value: "default"}},
+							TemplateEnvVars: []TemplateEnvVar{
+								{
+									Condition: "$.Values.secret",
+									Name:      "ENV_VAR",
+									Value:     "templated",
+								},
+							},
+						},
+					},
+				}},
+			},
+			ManifestRoot: "codegen/test/chart/env-priority",
+		}
+		Expect(cmd.Execute()).NotTo(HaveOccurred(), "failed to execute command")
+
+		manifests := helmTemplate("./test/chart/env-priority", map[string]any{"painter": map[string]any{"enabled": true}, "secret": true})
+		var renderedDeployment *appsv1.Deployment
+		decoder := kubeyaml.NewYAMLOrJSONDecoder(bytes.NewBuffer(manifests), 4096)
+		for {
+			obj := &unstructured.Unstructured{}
+			err := decoder.Decode(obj)
+			if err != nil {
+				break
+			}
+			if obj.GetName() != "painter" || obj.GetKind() != "Deployment" {
+				continue
+			}
+
+			bytes, err := obj.MarshalJSON()
+			Expect(err).NotTo(HaveOccurred())
+			renderedDeployment = &appsv1.Deployment{}
+			err = json.Unmarshal(bytes, renderedDeployment)
+			Expect(err).NotTo(HaveOccurred())
+		}
+		Expect(renderedDeployment).NotTo(BeNil())
+
+		Expect(renderedDeployment.Spec.Template.Spec.Containers[0].Env).To(HaveLen(2))
+		Expect(renderedDeployment.Spec.Template.Spec.Containers[0].Env[0]).To(Equal(v1.EnvVar{Name: "ENV_VAR", Value: "templated"}))
+		Expect(renderedDeployment.Spec.Template.Spec.Containers[0].Env[1]).To(Equal(v1.EnvVar{Name: "ENV_VAR", Value: "default"}))
+	})
+
 	It("install conditional sidecars", func() {
 		agentConditional := "and ($.Values.glooAgent.enabled) ($.Values.glooAgent.runAsSidecar)"
 
@@ -2255,7 +2315,9 @@ roleRef:
 						Value: "{{ $.Values.featureGates.Foo | quote }}",
 					},
 				},
-				nil),
+				[]v1.EnvVar{
+					{Name: "FEATURE_ENABLE_FOO", Value: "true"},
+				}),
 			Entry("when Env and TemplateEnvVar are specified, true value",
 				map[string]string{"Foo": "true"},
 				[]v1.EnvVar{

diff --git a/codegen/templates/chart/operator-deployment.yamltmpl b/codegen/templates/chart/operator-deployment.yamltmpl
@@ -140,30 +140,34 @@ spec:
           containerPort: [[ $port.Port ]]
         [[- end ]]
 [[- end ]]
-{{- if [[ $containerVar ]].env }}
+[[- if or $container.Env $container.TemplateEnvVars ]]
+        env:
+[[- else ]]
+  {{- if or [[ $containerVar ]].env [[ $containerVar ]].extraEnvs }}
         env:
-{{ toYaml [[ $containerVar ]].env | indent 10 }}
+  {{- end }}
+[[- end ]]
 [[- range $f := $container.TemplateEnvVars ]]
-[[- if $f.Condition ]]
-{{- if [[ $f.Condition ]] }}
-[[- end]]
-          [[- if $f.Value ]]
+  [[- if $f.Condition ]]
+  {{- if [[ $f.Condition ]] }}
+  [[- end ]]
+        [[- if $f.Value ]]
           - name: [[ $f.Name ]]
             value: [[ $f.Value ]]
-          [[- else if $f.ValueFrom ]]
+        [[- else if $f.ValueFrom ]]
           - name: [[ $f.Name ]]
             valueFrom: [[ $f.ValueFrom | toYaml | nindent 14 ]]
-          [[- end ]]
-[[- if $f.Condition ]]
-{{- end }}
-[[- end ]] [[/* end Condition */]]
-[[- end ]] [[/* end TemplateEnvVars */]]
-{{- else if [[ $containerVar ]].extraEnvs }}
-        env:
+        [[- end ]]
+  [[- if $f.Condition ]]
+  {{- end }}
+  [[- end ]]
+[[- end ]]
+{{- if [[ $containerVar ]].env }}
+{{- toYaml [[ $containerVar ]].env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := [[ $containerVar ]].extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
 [[- if $container.VolumeMounts ]]
         volumeMounts:

diff --git a/codegen/test/chart-conditional-deployment-strategy/templates/deployment.yaml b/codegen/test/chart-conditional-deployment-strategy/templates/deployment.yaml
@@ -48,15 +48,15 @@ spec:
       - name: painter
         image: {{ $painterImage.registry }}/{{ $painterImage.repository }}:{{ $painterImage.tag }}
         imagePullPolicy: {{ $painterImage.pullPolicy }}
-{{- if $painter.env }}
-        env:
-{{ toYaml $painter.env | indent 10 }} 
-{{- else if $painter.extraEnvs }}
+  {{- if or $painter.env $painter.extraEnvs }}
         env:
+  {{- end }}
+{{- if $painter.env }}
+{{- toYaml $painter.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $painter.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         resources:
 {{- if $painter.resources }}

diff --git a/codegen/test/chart-deployment-strategy/templates/deployment.yaml b/codegen/test/chart-deployment-strategy/templates/deployment.yaml
@@ -44,15 +44,15 @@ spec:
       - name: painter
         image: {{ $painterImage.registry }}/{{ $painterImage.repository }}:{{ $painterImage.tag }}
         imagePullPolicy: {{ $painterImage.pullPolicy }}
-{{- if $painter.env }}
-        env:
-{{ toYaml $painter.env | indent 10 }} 
-{{- else if $painter.extraEnvs }}
+  {{- if or $painter.env $painter.extraEnvs }}
         env:
+  {{- end }}
+{{- if $painter.env }}
+{{- toYaml $painter.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $painter.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         resources:
 {{- if $painter.resources }}

diff --git a/codegen/test/chart-envvars/templates/deployment.yaml b/codegen/test/chart-envvars/templates/deployment.yaml
@@ -40,15 +40,15 @@ spec:
       - name: painter
         image: {{ $painterImage.registry }}/{{ $painterImage.repository }}:{{ $painterImage.tag }}
         imagePullPolicy: {{ $painterImage.pullPolicy }}
-{{- if $painter.env }}
-        env:
-{{ toYaml $painter.env | indent 10 }} 
-{{- else if $painter.extraEnvs }}
+  {{- if or $painter.env $painter.extraEnvs }}
         env:
+  {{- end }}
+{{- if $painter.env }}
+{{- toYaml $painter.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $painter.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         resources:
 {{- if $painter.resources }}

diff --git a/codegen/test/chart-no-desc/templates/deployment.yaml b/codegen/test/chart-no-desc/templates/deployment.yaml
@@ -50,15 +50,13 @@ spec:
         imagePullPolicy: {{ $painterImage.pullPolicy }}
         args:
         - foo
-{{- if $painter.env }}
-        env:
-{{ toYaml $painter.env | indent 10 }} 
-{{- else if $painter.extraEnvs }}
         env:
+{{- if $painter.env }}
+{{- toYaml $painter.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $painter.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         resources:
 {{- if $painter.resources }}
@@ -105,15 +103,15 @@ spec:
         args:
         - bar
         - baz
-{{- if $palette.env }}
-        env:
-{{ toYaml $palette.env | indent 10 }} 
-{{- else if $palette.extraEnvs }}
+  {{- if or $palette.env $palette.extraEnvs }}
         env:
+  {{- end }}
+{{- if $palette.env }}
+{{- toYaml $palette.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $palette.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         volumeMounts:
         - mountPath: /etc/paint

diff --git a/codegen/test/chart-pod-security-context/templates/deployment.yaml b/codegen/test/chart-pod-security-context/templates/deployment.yaml
@@ -43,15 +43,15 @@ spec:
       - name: painter
         image: {{ $painterImage.registry }}/{{ $painterImage.repository }}:{{ $painterImage.tag }}
         imagePullPolicy: {{ $painterImage.pullPolicy }}
-{{- if $painter.env }}
-        env:
-{{ toYaml $painter.env | indent 10 }} 
-{{- else if $painter.extraEnvs }}
+  {{- if or $painter.env $painter.extraEnvs }}
         env:
+  {{- end }}
+{{- if $painter.env }}
+{{- toYaml $painter.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $painter.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         resources:
 {{- if $painter.resources }}

diff --git a/codegen/test/chart-readiness/templates/deployment.yaml b/codegen/test/chart-readiness/templates/deployment.yaml
@@ -40,15 +40,15 @@ spec:
       - name: painter
         image: {{ $painterImage.registry }}/{{ $painterImage.repository }}:{{ $painterImage.tag }}
         imagePullPolicy: {{ $painterImage.pullPolicy }}
-{{- if $painter.env }}
-        env:
-{{ toYaml $painter.env | indent 10 }} 
-{{- else if $painter.extraEnvs }}
+  {{- if or $painter.env $painter.extraEnvs }}
         env:
+  {{- end }}
+{{- if $painter.env }}
+{{- toYaml $painter.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $painter.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         resources:
 {{- if $painter.resources }}

diff --git a/codegen/test/chart-sidecar-svcport/templates/deployment.yaml b/codegen/test/chart-sidecar-svcport/templates/deployment.yaml
@@ -40,15 +40,15 @@ spec:
       - name: painter
         image: {{ $painterImage.registry }}/{{ $painterImage.repository }}:{{ $painterImage.tag }}
         imagePullPolicy: {{ $painterImage.pullPolicy }}
-{{- if $painter.env }}
-        env:
-{{ toYaml $painter.env | indent 10 }} 
-{{- else if $painter.extraEnvs }}
+  {{- if or $painter.env $painter.extraEnvs }}
         env:
+  {{- end }}
+{{- if $painter.env }}
+{{- toYaml $painter.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $painter.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         resources:
 {{- if $painter.resources }}
@@ -86,15 +86,15 @@ spec:
       - name: sidecar-painter
         image: {{ $sidecarPainterImage.registry }}/{{ $sidecarPainterImage.repository }}:{{ $sidecarPainterImage.tag }}
         imagePullPolicy: {{ $sidecarPainterImage.pullPolicy }}
-{{- if $sidecarPainter.env }}
-        env:
-{{ toYaml $sidecarPainter.env | indent 10 }} 
-{{- else if $sidecarPainter.extraEnvs }}
+  {{- if or $sidecarPainter.env $sidecarPainter.extraEnvs }}
         env:
+  {{- end }}
+{{- if $sidecarPainter.env }}
+{{- toYaml $sidecarPainter.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $sidecarPainter.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         resources:
 {{- if $sidecarPainter.resources }}

diff --git a/codegen/test/chart-sidecar/templates/deployment.yaml b/codegen/test/chart-sidecar/templates/deployment.yaml
@@ -40,15 +40,15 @@ spec:
       - name: painter
         image: {{ $painterImage.registry }}/{{ $painterImage.repository }}:{{ $painterImage.tag }}
         imagePullPolicy: {{ $painterImage.pullPolicy }}
-{{- if $painter.env }}
-        env:
-{{ toYaml $painter.env | indent 10 }} 
-{{- else if $painter.extraEnvs }}
+  {{- if or $painter.env $painter.extraEnvs }}
         env:
+  {{- end }}
+{{- if $painter.env }}
+{{- toYaml $painter.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $painter.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         resources:
 {{- if $painter.resources }}
@@ -85,15 +85,15 @@ spec:
       - name: foo-bar
         image: {{ $fooBarImage.registry }}/{{ $fooBarImage.repository }}:{{ $fooBarImage.tag }}
         imagePullPolicy: {{ $fooBarImage.pullPolicy }}
-{{- if $fooBar.env }}
-        env:
-{{ toYaml $fooBar.env | indent 10 }} 
-{{- else if $fooBar.extraEnvs }}
+  {{- if or $fooBar.env $fooBar.extraEnvs }}
         env:
+  {{- end }}
+{{- if $fooBar.env }}
+{{- toYaml $fooBar.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $fooBar.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         resources:
 {{- if $fooBar.resources }}

diff --git a/codegen/test/chart-svcport/templates/deployment.yaml b/codegen/test/chart-svcport/templates/deployment.yaml
@@ -40,15 +40,15 @@ spec:
       - name: painter
         image: {{ $painterImage.registry }}/{{ $painterImage.repository }}:{{ $painterImage.tag }}
         imagePullPolicy: {{ $painterImage.pullPolicy }}
-{{- if $painter.env }}
-        env:
-{{ toYaml $painter.env | indent 10 }} 
-{{- else if $painter.extraEnvs }}
+  {{- if or $painter.env $painter.extraEnvs }}
         env:
+  {{- end }}
+{{- if $painter.env }}
+{{- toYaml $painter.env | nindent 10 }}
 {{- end }}
 {{- range $name, $item := $painter.extraEnvs }}
           - name: {{ $name }}
-{{- $item | toYaml | nindent 12 }}
+  {{- $item | toYaml | nindent 12 }}
 {{- end }}
         resources:
 {{- if $painter.resources }}