Fix duplicate environment variables in MergeEnvVars

pkg/env/env.go

@@ -53,27 +53,33 @@ func IsForbiddenEnvVar(name string) bool {
 // if overwriteValues is false, this function will return an error if a duplicate EnvVar name is encountered
 // if overwriteValues is true, this function will overwrite the existing value with the new value if a duplicate is encountered
 func MergeEnvVars(from []corev1.EnvVar, into []corev1.EnvVar, overwriteValues bool) ([]corev1.EnvVar, error) {
-	// if from, into, or both are empty, there is no need to run through the processing logic
-	// just quickly return the appropriate value
 	if len(from) == 0 && len(into) == 0 {
 		return []corev1.EnvVar{}, nil
-	} else if len(from) == 0 {
-		return into, nil
-	} else if len(into) == 0 {
-		return from, nil
 	}
 
 	// create a map of the original (into) env vars with the name as the key and
 	// their index as the value so we can do value replacements later if overwriteValues is true
-	originalEnvs := make(map[string]int)
-
-	for i, o := range into {
-		originalEnvs[o.Name] = i
-	}
+	envIndices := make(map[string]int)
 
 	// errs holds a slice of error objects from the merge process
 	var errs []error
 
+	merged := make([]corev1.EnvVar, 0, len(into)+len(from))
+
+	for _, o := range into {
+		index, exists := envIndices[o.Name]
+
+		switch {
+		case exists && overwriteValues:
+			merged[index] = o
+		case exists && !overwriteValues:
+			errs = append(errs, fmt.Errorf("environment variable %q already exists", o.Name))
+		default:
+			envIndices[o.Name] = len(merged)
+			merged = append(merged, o)
+		}
+	}
+
 	// merge the new env vars into the original env vars list following a few simple rules
 	// based on if the name already exists and whether overwriteValues is true or false
 	for _, n := range from {
@@ -82,19 +88,20 @@ func MergeEnvVars(from []corev1.EnvVar, into []corev1.EnvVar, overwriteValues bo
 			continue
 		}
 
-		_, exists := originalEnvs[n.Name]
+		index, exists := envIndices[n.Name]
 
 		switch {
 		case exists && overwriteValues:
-			into[originalEnvs[n.Name]] = n
+			merged[index] = n
 		case exists && !overwriteValues:
 			errs = append(errs, fmt.Errorf("environment variable %q already exists", n.Name))
 		default:
-			into = append(into, n)
+			envIndices[n.Name] = len(merged)
+			merged = append(merged, n)
 		}
 	}
 
 	// kerrors.NewAggregate will return nil if the slice is empty
 	// or an aggregated error otherwise
-	return into, kerrors.NewAggregate(errs)
+	return merged, kerrors.NewAggregate(errs)
 }

pkg/env/env_test.go

@@ -117,6 +117,195 @@ func TestMergeEnvVars(t *testing.T) {
 			},
 			wantErr: false,
 		},
+		{
+			name: "duplicate incoming names should fail with overwriteValues false",
+			args: args{
+				new: []corev1.EnvVar{
+					{Name: "TWO", Value: "first"},
+					{Name: "THREE", Value: "threeValue"},
+					{Name: "TWO", Value: "second"},
+				},
+				into: []corev1.EnvVar{
+					{Name: "ONE", Value: "oneValue"},
+				},
+				overwriteValues: false,
+			},
+			want: []corev1.EnvVar{
+				{Name: "ONE", Value: "oneValue"},
+				{Name: "TWO", Value: "first"},
+				{Name: "THREE", Value: "threeValue"},
+			},
+			wantErr: true,
+		},
+		{
+			name: "duplicate incoming names should overwrite without changing order",
+			args: args{
+				new: []corev1.EnvVar{
+					{Name: "TWO", Value: "first"},
+					{Name: "THREE", Value: "threeValue"},
+					{Name: "TWO", Value: "second"},
+				},
+				into: []corev1.EnvVar{
+					{Name: "ONE", Value: "oneValue"},
+				},
+				overwriteValues: true,
+			},
+			want: []corev1.EnvVar{
+				{Name: "ONE", Value: "oneValue"},
+				{Name: "TWO", Value: "second"},
+				{Name: "THREE", Value: "threeValue"},
+			},
+		},
+		{
+			name: "duplicate incoming names should fail with nil into",
+			args: args{
+				new: []corev1.EnvVar{
+					{Name: "ONE", Value: "first"},
+					{Name: "ONE", Value: "second"},
+				},
+				into:            nil,
+				overwriteValues: false,
+			},
+			want: []corev1.EnvVar{
+				{Name: "ONE", Value: "first"},
+			},
+			wantErr: true,
+		},
+		{
+			name: "duplicate incoming names should overwrite with empty into",
+			args: args{
+				new: []corev1.EnvVar{
+					{Name: "ONE", Value: "first"},
+					{Name: "ONE", Value: "second"},
+				},
+				into:            []corev1.EnvVar{},
+				overwriteValues: true,
+			},
+			want: []corev1.EnvVar{
+				{Name: "ONE", Value: "second"},
+			},
+		},
+		{
+			name: "duplicate destination names should fail with overwriteValues false and empty from",
+			args: args{
+				new: []corev1.EnvVar{},
+				into: []corev1.EnvVar{
+					{Name: "ONE", Value: "first"},
+					{Name: "ONE", Value: "second"},
+				},
+				overwriteValues: false,
+			},
+			want: []corev1.EnvVar{
+				{Name: "ONE", Value: "first"},
+			},
+			wantErr: true,
+		},
+		{
+			name: "duplicate destination names should overwrite with empty from",
+			args: args{
+				new: []corev1.EnvVar{},
+				into: []corev1.EnvVar{
+					{Name: "ONE", Value: "first"},
+					{Name: "TWO", Value: "twoValue"},
+					{Name: "ONE", Value: "second"},
+				},
+				overwriteValues: true,
+			},
+			want: []corev1.EnvVar{
+				{Name: "ONE", Value: "second"},
+				{Name: "TWO", Value: "twoValue"},
+			},
+		},
+		{
+			name: "duplicate destination names should fail with overwriteValues false",
+			args: args{
+				new: []corev1.EnvVar{
+					{Name: "THREE", Value: "threeValue"},
+				},
+				into: []corev1.EnvVar{
+					{Name: "ONE", Value: "first"},
+					{Name: "ONE", Value: "second"},
+				},
+				overwriteValues: false,
+			},
+			want: []corev1.EnvVar{
+				{Name: "ONE", Value: "first"},
+				{Name: "THREE", Value: "threeValue"},
+			},
+			wantErr: true,
+		},
+		{
+			name: "duplicate destination names should be overwritten by incoming values",
+			args: args{
+				new: []corev1.EnvVar{
+					{Name: "ONE", Value: "third"},
+				},
+				into: []corev1.EnvVar{
+					{Name: "ONE", Value: "first"},
+					{Name: "ONE", Value: "second"},
+				},
+				overwriteValues: true,
+			},
+			want: []corev1.EnvVar{
+				{Name: "ONE", Value: "third"},
+			},
+		},
+		{
+			name: "duplicate incoming valueFrom should replace value",
+			args: args{
+				new: []corev1.EnvVar{
+					{Name: "ONE", Value: "first"},
+					{
+						Name: "ONE",
+						ValueFrom: &corev1.EnvVarSource{
+							FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"},
+						},
+					},
+				},
+				overwriteValues: true,
+			},
+			want: []corev1.EnvVar{
+				{
+					Name: "ONE",
+					ValueFrom: &corev1.EnvVarSource{
+						FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"},
+					},
+				},
+			},
+		},
+		{
+			name: "duplicate incoming value should replace valueFrom",
+			args: args{
+				new: []corev1.EnvVar{
+					{
+						Name: "ONE",
+						ValueFrom: &corev1.EnvVarSource{
+							FieldRef: &corev1.ObjectFieldSelector{FieldPath: "metadata.name"},
+						},
+					},
+					{Name: "ONE", Value: "second"},
+				},
+				overwriteValues: true,
+			},
+			want: []corev1.EnvVar{
+				{Name: "ONE", Value: "second"},
+			},
+		},
+		{
+			name: "forbidden incoming env var should fail with empty into",
+			args: args{
+				new: []corev1.EnvVar{
+					{Name: "ONE", Value: "oneValue"},
+					{Name: "LD_PRELOAD", Value: "/tmp/malicious.so"},
+				},
+				into:            []corev1.EnvVar{},
+				overwriteValues: true,
+			},
+			want: []corev1.EnvVar{
+				{Name: "ONE", Value: "oneValue"},
+			},
+			wantErr: true,
+		},
 		{
 			name: "duplicate names should fail with overwriteValues false",
 			args: args{