[release-4.9] Bug 2071689: lib/resourcemerge: handle container env var deletions

lib/resourcemerge/core.go

@@ -100,7 +100,10 @@ func ensureContainer(modified *bool, existing *corev1.Container, required corev1
 	setStringIfSet(modified, &existing.WorkingDir, required.WorkingDir)
 
 	// also sync the env vars here, added to handle proxy
+	// use a map to keep track of removed vars, with empty values
+	requiredVars := make(map[string]struct{})
 	for _, required := range required.Env {
+		requiredVars[required.Name] = struct{}{}
 		var existingCurr *corev1.EnvVar
 		for j, curr := range existing.Env {
 			if curr.Name == required.Name {
@@ -116,6 +119,14 @@ func ensureContainer(modified *bool, existing *corev1.Container, required corev1
 		ensureEnvVar(modified, existingCurr, required)
 	}
 
+	// any env vars we don't have anymore should be removed
+	for i := len(existing.Env) - 1; i >= 0; i-- {
+		if _, ok := requiredVars[existing.Env[i].Name]; !ok {
+			*modified = true
+			existing.Env = append(existing.Env[:i], existing.Env[i+1:]...)
+		}
+	}
+
 	// any port we specify, we require
 	for _, required := range required.Ports {
 		var existingCurr *corev1.ContainerPort

lib/resourcemerge/core_test.go

@@ -0,0 +1,133 @@
+package resourcemerge
+
+import (
+	"fmt"
+	"testing"
+
+	corev1 "k8s.io/api/core/v1"
+	"k8s.io/apimachinery/pkg/api/equality"
+)
+
+// Test added for now to test ensureContainer (mostly so we don't break Env Vars - proxy)
+func TestEnsureContainer(t *testing.T) {
+	tests := []struct {
+		existing corev1.Container
+		input    corev1.Container
+
+		expectedModified bool
+		expected         corev1.Container
+	}{
+		// Change in proxy
+		{
+			existing: corev1.Container{
+				Env: []corev1.EnvVar{
+					{
+						Name:  "Proxy",
+						Value: "Proxy1",
+					},
+				},
+			},
+			input: corev1.Container{
+				Env: []corev1.EnvVar{
+					{
+						Name:  "Proxy",
+						Value: "Proxy2",
+					},
+				},
+			},
+			expectedModified: true,
+			expected: corev1.Container{
+				Env: []corev1.EnvVar{
+					{
+						Name:  "Proxy",
+						Value: "Proxy2",
+					},
+				},
+			},
+		},
+		// Add Proxy
+		{
+			existing: corev1.Container{
+				Env: []corev1.EnvVar{},
+			},
+			input: corev1.Container{
+				Env: []corev1.EnvVar{
+					{
+						Name:  "Proxy",
+						Value: "Proxy1",
+					},
+				},
+			},
+			expectedModified: true,
+			expected: corev1.Container{
+				Env: []corev1.EnvVar{
+					{
+						Name:  "Proxy",
+						Value: "Proxy1",
+					},
+				},
+			},
+		},
+		// Remove Proxy
+		{
+			existing: corev1.Container{
+				Env: []corev1.EnvVar{
+					{
+						Name:  "Proxy",
+						Value: "Proxy1",
+					},
+				},
+			},
+			input: corev1.Container{
+				Env: []corev1.EnvVar{},
+			},
+			expectedModified: true,
+			expected: corev1.Container{
+				Env: []corev1.EnvVar{},
+			},
+		},
+		// No change
+		{
+			existing: corev1.Container{
+				Env: []corev1.EnvVar{
+					{
+						Name:  "Proxy",
+						Value: "Proxy1",
+					},
+				},
+			},
+			input: corev1.Container{
+				Env: []corev1.EnvVar{
+					{
+						Name:  "Proxy",
+						Value: "Proxy1",
+					},
+				},
+			},
+			expectedModified: false,
+			expected: corev1.Container{
+				Env: []corev1.EnvVar{
+					{
+						Name:  "Proxy",
+						Value: "Proxy1",
+					},
+				},
+			},
+		},
+	}
+
+	for idx, test := range tests {
+		t.Run(fmt.Sprintf("test#%d", idx), func(t *testing.T) {
+			modified := BoolPtr(false)
+			ensureContainer(modified, &test.existing, test.input)
+
+			if *modified != test.expectedModified {
+				t.Fatalf("mismatch container got: %v want: %v", *modified, test.expectedModified)
+			}
+
+			if !equality.Semantic.DeepEqual(test.existing, test.expected) {
+				t.Fatalf("mismatch container got: %v want: %v", test.existing, test.expected)
+			}
+		})
+	}
+}