fixed conflict resolution behavior while apply podpresets

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("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 fmt.Errorf("listing pod presets for namespace:%s failed: %v", pod.GetNamespace(), err)
+			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", pp.GetName(), pod.GetName())
+		matchingPPs = append(matchingPPs, pp)
+	}
+	return matchingPPs, nil
+}
 
-		glog.V(4).Infof("PodPreset %s matches pod %s labels", pip.GetName(), pod.GetName())
+// 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
 
-		// 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())
+	// 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)
+	}
+	for _, ctr := range pod.Spec.Containers {
+		if err := safeToApplyPodPresetsOnContainer(&ctr, podPresets); err != nil {
+			errs = append(errs, err)
+		}
+	}
+	return utilerrors.NewAggregate(errs)
+}
 
-					return nil
+// 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)
+	}
+
+	return utilerrors.NewAggregate(errs)
+}
+
+// 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
+	}
+
+	mergedEnv := make([]api.EnvVar, len(envVars))
+	copy(mergedEnv, envVars)
+
+	var errs []error
+
+	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
+				origEnv[v.Name] = v
+				mergedEnv = append(mergedEnv, v)
+				continue
+			}
+
+			// make sure they are identical or throw an error
+			if !reflect.DeepEqual(found, v) {
+				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))
+			}
+		}
+	}
+
+	err := utilerrors.NewAggregate(errs)
+	if err != nil {
+		return nil, err
+	}
+
+	return mergedEnv, err
+}
+
+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...)
+	}
+
+	return mergedEnvFrom, nil
+}
+
+// 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
+	}
+
+	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 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))
 				}
-				pod.Spec.Containers[i].Env = r
 			}
-		}
 
-		// 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())
-
-					return nil
+			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))
 				}
-				pod.Spec.Containers[i].EnvFrom = r
 			}
 		}
+	}
 
-		// 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())
+	err := utilerrors.NewAggregate(errs)
+	if err != nil {
+		return nil, err
+	}
 
-					return nil
-				}
-				pod.Spec.Containers[i].VolumeMounts = r
+	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) {
+				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))
 			}
 		}
-
-		// 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
-		}
-
-		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{}
-		}
-
-		pod.ObjectMeta.Annotations[fmt.Sprintf("%s/podpreset-%s", annotationPrefix, pip.GetName())] = pip.GetResourceVersion()
 	}
 
-	return nil
-}
-
-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
+	err := utilerrors.NewAggregate(errs)
+	if err != nil {
+		return nil, err
 	}
 
-	orig := map[string]interface{}{}
-	for _, v := range original {
-		orig[v.Name] = v
+	if len(mergedVolumes) == 0 {
+		return nil, nil
 	}
 
-	// check for conflicts.
-	for _, v := range pip.Spec.Env {
-		found, ok := orig[v.Name]
-		if !ok {
-			// if we don't already have it append it and continue
-			original = append(original, 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)
-		}
-	}
-
-	return original, nil
-}
-
-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 append(original, pip.Spec.EnvFrom...), nil
-}
-
-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
-	}
-
-	// first key by name
-	orig := map[string]interface{}{}
-	for _, v := range original {
-		orig[v.Name] = v
-	}
-
-	// check for conflicts.
-	for _, v := range pip.Spec.VolumeMounts {
-		found, ok := orig[v.Name]
-		if !ok {
-			// if we don't already have it continue
-			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)
-		}
-	}
-
-	// key by mount path
-	orig = map[string]interface{}{}
-	for _, v := range original {
-		orig[v.MountPath] = v
-	}
-
-	// 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
-		}
-
-		// 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)
 	}
 }