From 4d5b96f94ea836b4296e281787496f7b69afff85 Mon Sep 17 00:00:00 2001 From: Sunil Arora Date: Tue, 20 Jun 2017 13:34:19 -0700 Subject: [PATCH] fixed conflict resolution behavior while apply podpresets --- hack/local-up-cluster.sh | 1 - plugin/pkg/admission/podpreset/BUILD | 1 + plugin/pkg/admission/podpreset/admission.go | 414 ++++++++++-------- .../pkg/admission/podpreset/admission_test.go | 60 ++- 4 files changed, 299 insertions(+), 177 deletions(-) diff --git a/hack/local-up-cluster.sh b/hack/local-up-cluster.sh index 16713a9d774..decbb2e07b7 100755 --- a/hack/local-up-cluster.sh +++ b/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 diff --git a/plugin/pkg/admission/podpreset/BUILD b/plugin/pkg/admission/podpreset/BUILD index aae12af2bc7..b8a8997ab99 100644 --- a/plugin/pkg/admission/podpreset/BUILD +++ b/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", ], ) diff --git a/plugin/pkg/admission/podpreset/admission.go b/plugin/pkg/admission/podpreset/admission.go index 3f9808ecc28..86bf707c8a4 100644 --- a/plugin/pkg/admission/podpreset/admission.go +++ b/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 +} diff --git a/plugin/pkg/admission/podpreset/admission_test.go b/plugin/pkg/admission/podpreset/admission_test.go index 922ac0d520d..4caa3d6bf82 100644 --- a/plugin/pkg/admission/podpreset/admission_test.go +++ b/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) } }