Fix podpreset merging of envFrom to be idempontent

plugin/pkg/admission/podpreset/BUILD

@@ -17,11 +17,13 @@ go_test(
         "//staging/src/k8s.io/api/settings/v1alpha1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//staging/src/k8s.io/apimachinery/pkg/runtime:go_default_library",
+        "//staging/src/k8s.io/apimachinery/pkg/util/diff:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/admission:go_default_library",
         "//staging/src/k8s.io/apiserver/pkg/authentication/user:go_default_library",
         "//staging/src/k8s.io/client-go/informers:go_default_library",
         "//staging/src/k8s.io/client-go/kubernetes/fake:go_default_library",
         "//staging/src/k8s.io/client-go/listers/settings/v1alpha1:go_default_library",
+        "//vendor/github.com/google/gofuzz:go_default_library",
     ],
 )
 

plugin/pkg/admission/podpreset/admission.go

@@ -252,21 +252,56 @@ func mergeEnv(envVars []api.EnvVar, podPresets []*settingsv1alpha1.PodPreset) ([
 	return mergedEnv, err
 }
 
+type envFromMergeKey struct {
+	prefix           string
+	configMapRefName string
+	secretRefName    string
+}
+
+func newEnvFromMergeKey(e api.EnvFromSource) envFromMergeKey {
+	k := envFromMergeKey{prefix: e.Prefix}
+	if e.ConfigMapRef != nil {
+		k.configMapRefName = e.ConfigMapRef.Name
+	}
+	if e.SecretRef != nil {
+		k.secretRefName = e.SecretRef.Name
+	}
+	return k
+}
+
 func mergeEnvFrom(envSources []api.EnvFromSource, podPresets []*settingsv1alpha1.PodPreset) ([]api.EnvFromSource, error) {
 	var mergedEnvFrom []api.EnvFromSource
 
+	// merge envFrom using a identify key to ensure Admit reinvocations are idempotent
+	origEnvSources := map[envFromMergeKey]api.EnvFromSource{}
+	for _, envSource := range envSources {
+		origEnvSources[newEnvFromMergeKey(envSource)] = envSource
+	}
 	mergedEnvFrom = append(mergedEnvFrom, envSources...)
+	var errs []error
 	for _, pp := range podPresets {
 		for _, envFromSource := range pp.Spec.EnvFrom {
 			internalEnvFrom := api.EnvFromSource{}
 			if err := apiscorev1.Convert_v1_EnvFromSource_To_core_EnvFromSource(&envFromSource, &internalEnvFrom, nil); err != nil {
 				return nil, err
 			}
-			mergedEnvFrom = append(mergedEnvFrom, internalEnvFrom)
+			found, ok := origEnvSources[newEnvFromMergeKey(internalEnvFrom)]
+			if !ok {
+				mergedEnvFrom = append(mergedEnvFrom, internalEnvFrom)
+				continue
+			}
+			if !reflect.DeepEqual(found, internalEnvFrom) {
+				errs = append(errs, fmt.Errorf("merging envFrom for %s has a conflict: \n%#v\ndoes not match\n%#v\n in container", pp.GetName(), internalEnvFrom, found))
+			}
 		}
 
 	}
 
+	err := utilerrors.NewAggregate(errs)
+	if err != nil {
+		return nil, err
+	}
+
 	return mergedEnvFrom, nil
 }
 

plugin/pkg/admission/podpreset/admission_test.go

@@ -17,13 +17,16 @@ limitations under the License.
 package podpreset
 
 import (
+	"fmt"
 	"reflect"
 	"testing"
 
+	fuzz "github.com/google/gofuzz"
 	corev1 "k8s.io/api/core/v1"
 	settingsv1alpha1 "k8s.io/api/settings/v1alpha1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/runtime"
+	"k8s.io/apimachinery/pkg/util/diff"
 	kadmission "k8s.io/apiserver/pkg/admission"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/client-go/informers"
@@ -831,3 +834,55 @@ func admitPod(pod *api.Pod, pip *settingsv1alpha1.PodPreset) error {
 
 	return nil
 }
+
+func TestEnvFromMergeKey(t *testing.T) {
+	f := fuzz.New()
+	for i := 0; i < 100; i++ {
+		t.Run(fmt.Sprintf("Run %d/100", i), func(t *testing.T) {
+			orig := api.EnvFromSource{}
+			f.Fuzz(&orig)
+			clone := api.EnvFromSource{}
+			f.Fuzz(&clone)
+
+			key := newEnvFromMergeKey(orig)
+
+			// copy all key fields into the clone so it only differs by fields not from the key
+			clone.Prefix = key.prefix
+			if orig.ConfigMapRef == nil {
+				clone.ConfigMapRef = nil
+			} else {
+				if clone.ConfigMapRef == nil {
+					clone.ConfigMapRef = &api.ConfigMapEnvSource{
+						LocalObjectReference: api.LocalObjectReference{},
+					}
+				}
+				clone.ConfigMapRef.Name = key.configMapRefName
+			}
+			if orig.SecretRef == nil {
+				clone.SecretRef = nil
+			} else {
+				if clone.SecretRef == nil {
+					clone.SecretRef = &api.SecretEnvSource{
+						LocalObjectReference: api.LocalObjectReference{},
+					}
+				}
+				clone.SecretRef.Name = key.secretRefName
+			}
+
+			// zero out known non-identifying fields
+			for _, e := range []api.EnvFromSource{orig, clone} {
+				if e.ConfigMapRef != nil {
+					e.ConfigMapRef.Optional = nil
+				}
+				if e.SecretRef != nil {
+					e.SecretRef.Optional = nil
+				}
+			}
+
+			if !reflect.DeepEqual(orig, clone) {
+				t.Errorf("expected all but known non-identifying fields for envFrom to be in envFromMergeKey but found unaccounted for differences, diff:\n%s", diff.ObjectReflectDiff(orig, clone))
+			}
+
+		})
+	}
+}