Merge pull request #47864 from droot/podpreset-conflict-fix

Automatic merge from submit-queue (batch tested with PRs 49444, 47864, 48584, 49395, 49118)

fixed conflict resolution behavior while apply podpresets

**What this PR does / why we need it**:
This fixes the PodPreset application behavior in case of conflicts occur during the merging of Pod's information with PodPreset's. More details are in issue #47861 

**Which issue this PR fixes** *(optional, in `fixes #<issue number>(, fixes #<issue_number>, ...)` format, will close that issue when PR gets merged)*: fixes #
fixes #47861

**Special notes for your reviewer**:
We are splitting the PodPreset application logic in two phases. In first phase, we try to detect the conflicts in information merging without modifying the Pod at all. If conflict occurs, then we reject the PodPresets injection. Incase of no conflicts, we apply the PodPresets and merge the information.

**Release note**:

```release-note
PodPreset is not injected if conflict occurs while applying PodPresets to a Pod.
```

hack/local-up-cluster.sh

@@ -406,7 +406,6 @@ function start_apiserver {
 
     # Admission Controllers to invoke prior to persisting objects in cluster
     ADMISSION_CONTROL=Initializers,NamespaceLifecycle,LimitRanger,ServiceAccount${security_admission},ResourceQuota,DefaultStorageClass,DefaultTolerationSeconds
-
     # This is the default dir and filename where the apiserver will generate a self-signed cert
     # which should be able to be used as the CA to verify itself
 

plugin/pkg/admission/podpreset/BUILD

@@ -43,6 +43,7 @@ go_library(
         "//vendor/k8s.io/apimachinery/pkg/api/errors:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/apis/meta/v1:go_default_library",
         "//vendor/k8s.io/apimachinery/pkg/labels:go_default_library",
+        "//vendor/k8s.io/apimachinery/pkg/util/errors:go_default_library",
         "//vendor/k8s.io/apiserver/pkg/admission:go_default_library",
     ],
 )

plugin/pkg/admission/podpreset/admission.go

@@ -20,12 +20,14 @@ import (
 	"fmt"
 	"io"
 	"reflect"
+	"strings"
 
 	"github.com/golang/glog"
 
 	"k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/labels"
+	utilerrors "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/apiserver/pkg/admission"
 	"k8s.io/kubernetes/pkg/api"
 	"k8s.io/kubernetes/pkg/api/ref"
@@ -103,8 +105,6 @@ func (c *podPresetPlugin) Admit(a admission.Attributes) error {
 		return nil
 	}
 
-	list, err := c.lister.PodPresets(a.GetNamespace()).List(labels.Everything())
-
 	// Ignore if exclusion annotation is present
 	if podAnnotations := pod.GetAnnotations(); podAnnotations != nil {
 		glog.V(5).Infof("Looking at pod annotations, found: %v", podAnnotations)
@@ -112,205 +112,237 @@ func (c *podPresetPlugin) Admit(a admission.Attributes) error {
 			return nil
 		}
 	}
+
+	list, err := c.lister.PodPresets(a.GetNamespace()).List(labels.Everything())
 	if err != nil {
 		return fmt.Errorf("listing pod presets failed: %v", err)
 	}
 
-	// get the pod presets and iterate over them
-	for _, pip := range list {
-		selector, err := metav1.LabelSelectorAsSelector(&pip.Spec.Selector)
+	matchingPPs, err := filterPodPresets(list, pod)
 	if err != nil {
-			return fmt.Errorf("listing pod presets for namespace:%s failed: %v", pod.GetNamespace(), err)
+		return fmt.Errorf("filtering pod presets failed: %v", err)
+	}
+
+	if len(matchingPPs) == 0 {
+		return nil
+	}
+
+	presetNames := make([]string, len(matchingPPs))
+	for i, pp := range matchingPPs {
+		presetNames[i] = pp.GetName()
+	}
+
+	// detect merge conflict
+	err = safeToApplyPodPresetsOnPod(pod, matchingPPs)
+	if err != nil {
+		// conflict, ignore the error, but raise an event
+		glog.Warningf("conflict occurred while applying podpresets: %s on pod: %v err: %v",
+			strings.Join(presetNames, ","), pod.GetGenerateName(), err)
+		return nil
+	}
+
+	applyPodPresetsOnPod(pod, matchingPPs)
+
+	glog.Infof("applied podpresets: %s successfully on Pod: %+v ", strings.Join(presetNames, ","), pod.GetGenerateName())
+
+	return nil
+}
+
+// filterPodPresets returns list of PodPresets which match given Pod.
+func filterPodPresets(list []*settings.PodPreset, pod *api.Pod) ([]*settings.PodPreset, error) {
+	var matchingPPs []*settings.PodPreset
+
+	for _, pp := range list {
+		selector, err := metav1.LabelSelectorAsSelector(&pp.Spec.Selector)
+		if err != nil {
+			return nil, fmt.Errorf("label selector conversion failed: %v for selector: %v", pp.Spec.Selector, err)
 		}
 
 		// check if the pod labels match the selector
 		if !selector.Matches(labels.Set(pod.Labels)) {
 			continue
 		}
-
-		glog.V(4).Infof("PodPreset %s matches pod %s labels", pip.GetName(), pod.GetName())
-
-		// merge in policy for Env
-		if pip.Spec.Env != nil {
-			for i, ctr := range pod.Spec.Containers {
-				r, err := mergeEnv(pip, ctr.Env)
-				if err != nil {
-					// add event to pod
-					c.addEvent(pod, pip, err.Error())
-
-					return nil
-				}
-				pod.Spec.Containers[i].Env = r
+		glog.V(4).Infof("PodPreset %s matches pod %s labels", pp.GetName(), pod.GetName())
+		matchingPPs = append(matchingPPs, pp)
 	}
+	return matchingPPs, nil
 }
 
-		// merge in policy for EnvFrom
-		if pip.Spec.EnvFrom != nil {
-			for i, ctr := range pod.Spec.Containers {
-				r, err := mergeEnvFrom(pip, ctr.EnvFrom)
-				if err != nil {
-					// add event to pod
-					c.addEvent(pod, pip, err.Error())
+// safeToApplyPodPresetsOnPod determines if there is any conflict in information
+// injected by given PodPresets in the Pod.
+func safeToApplyPodPresetsOnPod(pod *api.Pod, podPresets []*settings.PodPreset) error {
+	var errs []error
 
-					return nil
+	// volumes attribute is defined at the Pod level, so determine if volumes
+	// injection is causing any conflict.
+	if _, err := mergeVolumes(pod.Spec.Volumes, podPresets); err != nil {
+		errs = append(errs, err)
 	}
-				pod.Spec.Containers[i].EnvFrom = r
+	for _, ctr := range pod.Spec.Containers {
+		if err := safeToApplyPodPresetsOnContainer(&ctr, podPresets); err != nil {
+			errs = append(errs, err)
 		}
 	}
+	return utilerrors.NewAggregate(errs)
+}
 
-		// merge in policy for VolumeMounts
-		if pip.Spec.VolumeMounts != nil {
-			for i, ctr := range pod.Spec.Containers {
-				r, err := mergeVolumeMounts(pip, ctr.VolumeMounts)
-				if err != nil {
-					// add event to pod
-					c.addEvent(pod, pip, err.Error())
-
-					return nil
-				}
-				pod.Spec.Containers[i].VolumeMounts = r
+// safeToApplyPodPresetsOnContainer determines if there is any conflict in
+// information injected by given PodPresets in the given container.
+func safeToApplyPodPresetsOnContainer(ctr *api.Container, podPresets []*settings.PodPreset) error {
+	var errs []error
+	// check if it is safe to merge env vars and volume mounts from given podpresets and
+	// container's existing env vars.
+	if _, err := mergeEnv(ctr.Env, podPresets); err != nil {
+		errs = append(errs, err)
 	}
+	if _, err := mergeVolumeMounts(ctr.VolumeMounts, podPresets); err != nil {
+		errs = append(errs, err)
 	}
 
-		// merge in policy for Volumes
-		if pip.Spec.Volumes != nil {
-			r, err := mergeVolumes(pip, pod.Spec.Volumes)
-			if err != nil {
-				// add event to pod
-				c.addEvent(pod, pip, err.Error())
-
-				return nil
-			}
-			pod.Spec.Volumes = r
+	return utilerrors.NewAggregate(errs)
 }
 
-		glog.V(4).Infof("PodPreset %s merged with pod %s successfully", pip.GetName(), pod.GetName())
-
-		// add annotation
-		if pod.ObjectMeta.Annotations == nil {
-			pod.ObjectMeta.Annotations = map[string]string{}
+// mergeEnv merges a list of env vars with the env vars injected by given list podPresets.
+// It returns an error if it detects any conflict during the merge.
+func mergeEnv(envVars []api.EnvVar, podPresets []*settings.PodPreset) ([]api.EnvVar, error) {
+	origEnv := map[string]api.EnvVar{}
+	for _, v := range envVars {
+		origEnv[v.Name] = v
 	}
 
-		pod.ObjectMeta.Annotations[fmt.Sprintf("%s/podpreset-%s", annotationPrefix, pip.GetName())] = pip.GetResourceVersion()
-	}
+	mergedEnv := make([]api.EnvVar, len(envVars))
+	copy(mergedEnv, envVars)
 
-	return nil
-}
+	var errs []error
 
-func mergeEnv(pip *settings.PodPreset, original []api.EnvVar) ([]api.EnvVar, error) {
-	// if there were no original envvar just return the pip envvar
-	if original == nil {
-		return pip.Spec.Env, nil
-	}
-
-	orig := map[string]interface{}{}
-	for _, v := range original {
-		orig[v.Name] = v
-	}
-
-	// check for conflicts.
-	for _, v := range pip.Spec.Env {
-		found, ok := orig[v.Name]
+	for _, pp := range podPresets {
+		for _, v := range pp.Spec.Env {
+			found, ok := origEnv[v.Name]
 			if !ok {
 				// if we don't already have it append it and continue
-			original = append(original, v)
+				origEnv[v.Name] = v
+				mergedEnv = append(mergedEnv, v)
 				continue
 			}
 
 			// make sure they are identical or throw an error
 			if !reflect.DeepEqual(found, v) {
-			return nil, fmt.Errorf("merging env for %s has a conflict on %s: \n%#v\ndoes not match\n%#v\n in container", pip.GetName(), v.Name, v, found)
+				errs = append(errs, fmt.Errorf("merging env for %s has a conflict on %s: \n%#v\ndoes not match\n%#v\n in container", pp.GetName(), v.Name, v, found))
+			}
 		}
 	}
 
-	return original, nil
+	err := utilerrors.NewAggregate(errs)
+	if err != nil {
+		return nil, err
 	}
 
-func mergeEnvFrom(pip *settings.PodPreset, original []api.EnvFromSource) ([]api.EnvFromSource, error) {
-	// if there were no original envfrom just return the pip envfrom
-	if original == nil {
-		return pip.Spec.EnvFrom, nil
+	return mergedEnv, err
 }
 
-	return append(original, pip.Spec.EnvFrom...), nil
+func mergeEnvFrom(envSources []api.EnvFromSource, podPresets []*settings.PodPreset) ([]api.EnvFromSource, error) {
+	var mergedEnvFrom []api.EnvFromSource
+
+	mergedEnvFrom = append(mergedEnvFrom, envSources...)
+	for _, pp := range podPresets {
+		mergedEnvFrom = append(mergedEnvFrom, pp.Spec.EnvFrom...)
 	}
 
-func mergeVolumeMounts(pip *settings.PodPreset, original []api.VolumeMount) ([]api.VolumeMount, error) {
-	// if there were no original volume mount just return the pip volume mount
-	if original == nil {
-		return pip.Spec.VolumeMounts, nil
+	return mergedEnvFrom, nil
 }
 
-	// first key by name
-	orig := map[string]interface{}{}
-	for _, v := range original {
-		orig[v.Name] = v
+// mergeVolumeMounts merges given list of VolumeMounts with the volumeMounts
+// injected by given podPresets. It returns an error if it detects any conflict during the merge.
+func mergeVolumeMounts(volumeMounts []api.VolumeMount, podPresets []*settings.PodPreset) ([]api.VolumeMount, error) {
+
+	origVolumeMounts := map[string]api.VolumeMount{}
+	volumeMountsByPath := map[string]api.VolumeMount{}
+	for _, v := range volumeMounts {
+		origVolumeMounts[v.Name] = v
+		volumeMountsByPath[v.MountPath] = v
 	}
 
-	// check for conflicts.
-	for _, v := range pip.Spec.VolumeMounts {
-		found, ok := orig[v.Name]
+	mergedVolumeMounts := make([]api.VolumeMount, len(volumeMounts))
+	copy(mergedVolumeMounts, volumeMounts)
+
+	var errs []error
+
+	for _, pp := range podPresets {
+		for _, v := range pp.Spec.VolumeMounts {
+			found, ok := origVolumeMounts[v.Name]
 			if !ok {
-			// if we don't already have it continue
+				// if we don't already have it append it and continue
+				origVolumeMounts[v.Name] = v
+				mergedVolumeMounts = append(mergedVolumeMounts, v)
+			} else {
+				// make sure they are identical or throw an error
+				// shall we throw an error for identical volumeMounts ?
+				if !reflect.DeepEqual(found, v) {
+					errs = append(errs, fmt.Errorf("merging volume mounts for %s has a conflict on %s: \n%#v\ndoes not match\n%#v\n in container", pp.GetName(), v.Name, v, found))
+				}
+			}
+
+			found, ok = volumeMountsByPath[v.MountPath]
+			if !ok {
+				// if we don't already have it append it and continue
+				volumeMountsByPath[v.MountPath] = v
+			} else {
+				// make sure they are identical or throw an error
+				if !reflect.DeepEqual(found, v) {
+					errs = append(errs, fmt.Errorf("merging volume mounts for %s has a conflict on mount path %s: \n%#v\ndoes not match\n%#v\n in container", pp.GetName(), v.MountPath, v, found))
+				}
+			}
+		}
+	}
+
+	err := utilerrors.NewAggregate(errs)
+	if err != nil {
+		return nil, err
+	}
+
+	return mergedVolumeMounts, err
+}
+
+// mergeVolumes merges given list of Volumes with the volumes injected by given
+// podPresets. It returns an error if it detects any conflict during the merge.
+func mergeVolumes(volumes []api.Volume, podPresets []*settings.PodPreset) ([]api.Volume, error) {
+	origVolumes := map[string]api.Volume{}
+	for _, v := range volumes {
+		origVolumes[v.Name] = v
+	}
+
+	mergedVolumes := make([]api.Volume, len(volumes))
+	copy(mergedVolumes, volumes)
+
+	var errs []error
+
+	for _, pp := range podPresets {
+		for _, v := range pp.Spec.Volumes {
+			found, ok := origVolumes[v.Name]
+			if !ok {
+				// if we don't already have it append it and continue
+				origVolumes[v.Name] = v
+				mergedVolumes = append(mergedVolumes, v)
 				continue
 			}
 
 			// make sure they are identical or throw an error
 			if !reflect.DeepEqual(found, v) {
-			return nil, fmt.Errorf("merging volume mounts for %s has a conflict on %s: \n%#v\ndoes not match\n%#v\n in container", pip.GetName(), v.Name, v, found)
+				errs = append(errs, fmt.Errorf("merging volumes for %s has a conflict on %s: \n%#v\ndoes not match\n%#v\n in container", pp.GetName(), v.Name, v, found))
+			}
 		}
 	}
 
-	// key by mount path
-	orig = map[string]interface{}{}
-	for _, v := range original {
-		orig[v.MountPath] = v
+	err := utilerrors.NewAggregate(errs)
+	if err != nil {
+		return nil, err
 	}
 
-	// check for conflicts.
-	for _, v := range pip.Spec.VolumeMounts {
-		found, ok := orig[v.MountPath]
-		if !ok {
-			// if we don't already have it append it and continue
-			original = append(original, v)
-			continue
+	if len(mergedVolumes) == 0 {
+		return nil, nil
 	}
 
-		// make sure they are identical or throw an error
-		if !reflect.DeepEqual(found, v) {
-			return nil, fmt.Errorf("merging volume mounts for %s has a conflict on mount path %s: \n%#v\ndoes not match\n%#v\n in container", pip.GetName(), v.MountPath, v, found)
-		}
-	}
-
-	return original, nil
-}
-
-func mergeVolumes(pip *settings.PodPreset, original []api.Volume) ([]api.Volume, error) {
-	// if there were no original volumes just return the pip volumes
-	if original == nil {
-		return pip.Spec.Volumes, nil
-	}
-
-	orig := map[string]api.Volume{}
-	for _, v := range original {
-		orig[v.Name] = v
-	}
-
-	// check for conflicts.
-	for _, v := range pip.Spec.Volumes {
-		found, ok := orig[v.Name]
-		if !ok {
-			// if we don't already have it append it and continue
-			original = append(original, v)
-			continue
-		}
-
-		if !reflect.DeepEqual(found, v) {
-			return nil, fmt.Errorf("merging volumes for %s has a conflict on %s: \n%#v\ndoes not match\n%#v\nin pod spec", pip.GetName(), v.Name, v, found)
-		}
-	}
-
-	return original, nil
+	return mergedVolumes, err
 }
 
 func (c *podPresetPlugin) addEvent(pod *api.Pod, pip *settings.PodPreset, message string) {
@@ -334,3 +366,43 @@ func (c *podPresetPlugin) addEvent(pod *api.Pod, pip *settings.PodPreset, messag
 		return
 	}
 }
+
+// applyPodPresetsOnPod updates the PodSpec with merged information from all the
+// applicable PodPresets. It ignores the errors of merge functions because merge
+// errors have already been checked in safeToApplyPodPresetsOnPod function.
+func applyPodPresetsOnPod(pod *api.Pod, podPresets []*settings.PodPreset) {
+	if len(podPresets) == 0 {
+		return
+	}
+
+	volumes, _ := mergeVolumes(pod.Spec.Volumes, podPresets)
+	pod.Spec.Volumes = volumes
+
+	for i, ctr := range pod.Spec.Containers {
+		applyPodPresetsOnContainer(&ctr, podPresets)
+		pod.Spec.Containers[i] = ctr
+	}
+
+	// add annotation
+	if pod.ObjectMeta.Annotations == nil {
+		pod.ObjectMeta.Annotations = map[string]string{}
+	}
+
+	for _, pp := range podPresets {
+		pod.ObjectMeta.Annotations[fmt.Sprintf("%s/podpreset-%s", annotationPrefix, pp.GetName())] = pp.GetResourceVersion()
+	}
+}
+
+// applyPodPresetsOnContainer injects envVars, VolumeMounts and envFrom from
+// given podPresets in to the given container. It ignores conflict errors
+// because it assumes those have been checked already by the caller.
+func applyPodPresetsOnContainer(ctr *api.Container, podPresets []*settings.PodPreset) {
+	envVars, _ := mergeEnv(ctr.Env, podPresets)
+	ctr.Env = envVars
+
+	volumeMounts, _ := mergeVolumeMounts(ctr.VolumeMounts, podPresets)
+	ctr.VolumeMounts = volumeMounts
+
+	envFrom, _ := mergeEnvFrom(ctr.EnvFrom, podPresets)
+	ctr.EnvFrom = envFrom
+}

plugin/pkg/admission/podpreset/admission_test.go

@@ -66,8 +66,8 @@ func TestMergeEnv(t *testing.T) {
 
 	for name, test := range tests {
 		result, err := mergeEnv(
-			&settings.PodPreset{Spec: settings.PodPresetSpec{Env: test.mod}},
 			test.orig,
+			[]*settings.PodPreset{{Spec: settings.PodPresetSpec{Env: test.mod}}},
 		)
 		if test.shouldFail && err == nil {
 			t.Fatalf("expected test %q to fail but got nil", name)
@@ -162,8 +162,8 @@ func TestMergeEnvFrom(t *testing.T) {
 
 	for name, test := range tests {
 		result, err := mergeEnvFrom(
-			&settings.PodPreset{Spec: settings.PodPresetSpec{EnvFrom: test.mod}},
 			test.orig,
+			[]*settings.PodPreset{{Spec: settings.PodPresetSpec{EnvFrom: test.mod}}},
 		)
 		if test.shouldFail && err == nil {
 			t.Fatalf("expected test %q to fail but got nil", name)
@@ -295,8 +295,8 @@ func TestMergeVolumeMounts(t *testing.T) {
 
 	for name, test := range tests {
 		result, err := mergeVolumeMounts(
-			&settings.PodPreset{Spec: settings.PodPresetSpec{VolumeMounts: test.mod}},
 			test.orig,
+			[]*settings.PodPreset{{Spec: settings.PodPresetSpec{VolumeMounts: test.mod}}},
 		)
 		if test.shouldFail && err == nil {
 			t.Fatalf("expected test %q to fail but got nil", name)
@@ -376,8 +376,8 @@ func TestMergeVolumes(t *testing.T) {
 
 	for name, test := range tests {
 		result, err := mergeVolumes(
-			&settings.PodPreset{Spec: settings.PodPresetSpec{Volumes: test.mod}},
 			test.orig,
+			[]*settings.PodPreset{{Spec: settings.PodPresetSpec{Volumes: test.mod}}},
 		)
 		if test.shouldFail && err == nil {
 			t.Fatalf("expected test %q to fail but got nil", name)
@@ -496,6 +496,56 @@ func TestAdmitConflictWithNonMatchingLabelsShouldNotError(t *testing.T) {
 	}
 }
 
+func TestAdmitConflictShouldNotModifyPod(t *testing.T) {
+	containerName := "container"
+
+	pod := &api.Pod{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "mypod",
+			Namespace: "namespace",
+			Labels: map[string]string{
+				"security": "S2",
+			},
+		},
+		Spec: api.PodSpec{
+			Containers: []api.Container{
+				{
+					Name: containerName,
+					Env:  []api.EnvVar{{Name: "abc", Value: "value2"}, {Name: "ABC", Value: "value3"}},
+				},
+			},
+		},
+	}
+	origPod := *pod
+
+	pip := &settings.PodPreset{
+		ObjectMeta: v1.ObjectMeta{
+			Name:      "hello",
+			Namespace: "namespace",
+		},
+		Spec: settings.PodPresetSpec{
+			Selector: v1.LabelSelector{
+				MatchExpressions: []v1.LabelSelectorRequirement{
+					{
+						Key:      "security",
+						Operator: v1.LabelSelectorOpIn,
+						Values:   []string{"S2"},
+					},
+				},
+			},
+			Env: []api.EnvVar{{Name: "abc", Value: "value"}, {Name: "ABC", Value: "value"}},
+		},
+	}
+
+	err := admitPod(pod, pip)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if !reflect.DeepEqual(&origPod, pod) {
+		t.Fatalf("pod should not get modified in case of conflict origPod: %+v got: %+v", &origPod, pod)
+	}
+}
+
 func TestAdmit(t *testing.T) {
 	containerName := "container"
 
@@ -759,7 +809,7 @@ func TestAdmitEmptyPodNamespace(t *testing.T) {
 
 	// verify PodSpec has not been mutated
 	if !reflect.DeepEqual(pod, originalPod) {
-		t.Fatalf("Expected pod spec of '%v' to be unchanged", pod.Name)
+		t.Fatalf("pod should not get modified in case of emptyNamespace origPod: %+v got: %+v", originalPod, pod)
 	}
 }