diff --git a/pkg/env/env.go b/pkg/env/env.go index 6b28f8f0f..5b05e410d 100644 --- a/pkg/env/env.go +++ b/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) } diff --git a/pkg/env/env_test.go b/pkg/env/env_test.go index c7552baa3..a9584ae5c 100644 --- a/pkg/env/env_test.go +++ b/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{