diff --git a/plugin/pkg/admission/podpreset/BUILD b/plugin/pkg/admission/podpreset/BUILD index b3a3b6a4bff..26ba0c86d53 100644 --- a/plugin/pkg/admission/podpreset/BUILD +++ b/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", ], ) diff --git a/plugin/pkg/admission/podpreset/admission.go b/plugin/pkg/admission/podpreset/admission.go index c20073afb87..58b96925624 100644 --- a/plugin/pkg/admission/podpreset/admission.go +++ b/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 } diff --git a/plugin/pkg/admission/podpreset/admission_test.go b/plugin/pkg/admission/podpreset/admission_test.go index babf068ce70..c39f2270897 100644 --- a/plugin/pkg/admission/podpreset/admission_test.go +++ b/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)) + } + + }) + } +}