split some admission plugins into mutation and validation halves

plugin/pkg/admission/podnodeselector/admission.go

@@ -59,6 +59,8 @@ type podNodeSelector struct {
 	clusterNodeSelectors map[string]string
 }
 
+var _ admission.MutationInterface = &podNodeSelector{}
+var _ admission.ValidationInterface = &podNodeSelector{}
 var _ = kubeapiserveradmission.WantsInternalKubeClientSet(&podNodeSelector{})
 var _ = kubeapiserveradmission.WantsInternalKubeInformerFactory(&podNodeSelector{})
 
@@ -93,77 +95,105 @@ func readConfig(config io.Reader) *pluginConfig {
 
 // Admit enforces that pod and its namespace node label selectors matches at least a node in the cluster.
 func (p *podNodeSelector) Admit(a admission.Attributes) error {
-	resource := a.GetResource().GroupResource()
-	if resource != api.Resource("pods") {
+	if shouldIgnore(a) {
 		return nil
 	}
-	if a.GetSubresource() != "" {
-		// only run the checks below on pods proper and not subresources
+	updateInitialized, err := util.IsUpdatingInitializedObject(a)
+	if err != nil {
+		return err
+	}
+	if updateInitialized {
+		// node selector of an initialized pod is immutable
 		return nil
 	}
+	if !p.WaitForReady() {
+		return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request"))
+	}
 
-	obj := a.GetObject()
-	pod, ok := obj.(*api.Pod)
-	if !ok {
-		glog.Errorf("expected pod but got %s", a.GetKind().Kind)
-		return nil
+	resource := a.GetResource().GroupResource()
+	pod := a.GetObject().(*api.Pod)
+	namespaceNodeSelector, err := p.getNamespaceNodeSelectorMap(a.GetNamespace())
+	if err != nil {
+		return err
+	}
+
+	if labels.Conflicts(namespaceNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
+		return errors.NewForbidden(resource, pod.Name, fmt.Errorf("pod node label selector conflicts with its namespace node label selector"))
 	}
 
+	// Merge pod node selector = namespace node selector + current pod node selector
+	// second selector wins
+	podNodeSelectorLabels := labels.Merge(namespaceNodeSelector, pod.Spec.NodeSelector)
+	pod.Spec.NodeSelector = map[string]string(podNodeSelectorLabels)
+	return p.Validate(a)
+}
+
+// Validate ensures that the pod node selector is allowed
+func (p *podNodeSelector) Validate(a admission.Attributes) error {
+	if shouldIgnore(a) {
+		return nil
+	}
 	if !p.WaitForReady() {
 		return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request"))
 	}
 
-	updateInitialized, err := util.IsUpdatingInitializedObject(a)
+	resource := a.GetResource().GroupResource()
+	pod := a.GetObject().(*api.Pod)
+
+	namespaceNodeSelector, err := p.getNamespaceNodeSelectorMap(a.GetNamespace())
 	if err != nil {
 		return err
 	}
-	if updateInitialized {
-		// node selector of an initialized pod is immutable
-		return nil
+	if labels.Conflicts(namespaceNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
+		return errors.NewForbidden(resource, pod.Name, fmt.Errorf("pod node label selector conflicts with its namespace node label selector"))
+	}
+
+	// whitelist verification
+	whitelist, err := labels.ConvertSelectorToLabelsMap(p.clusterNodeSelectors[a.GetNamespace()])
+	if err != nil {
+		return err
+	}
+	if !labels.AreLabelsInWhiteList(pod.Spec.NodeSelector, whitelist) {
+		return errors.NewForbidden(resource, pod.Name, fmt.Errorf("pod node label selector labels conflict with its namespace whitelist"))
 	}
 
-	name := pod.Name
-	nsName := a.GetNamespace()
-	var namespace *api.Namespace
+	return nil
+}
 
-	namespace, err = p.namespaceLister.Get(nsName)
+func (p *podNodeSelector) getNamespaceNodeSelectorMap(namespaceName string) (labels.Set, error) {
+	namespace, err := p.namespaceLister.Get(namespaceName)
 	if errors.IsNotFound(err) {
-		namespace, err = p.defaultGetNamespace(nsName)
+		namespace, err = p.defaultGetNamespace(namespaceName)
 		if err != nil {
 			if errors.IsNotFound(err) {
-				return err
+				return nil, err
 			}
-			return errors.NewInternalError(err)
+			return nil, errors.NewInternalError(err)
 		}
 	} else if err != nil {
-		return errors.NewInternalError(err)
+		return nil, errors.NewInternalError(err)
 	}
 
-	namespaceNodeSelector, err := p.getNodeSelectorMap(namespace)
-	if err != nil {
-		return err
-	}
+	return p.getNodeSelectorMap(namespace)
+}
 
-	if labels.Conflicts(namespaceNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
-		return errors.NewForbidden(resource, name, fmt.Errorf("pod node label selector conflicts with its namespace node label selector"))
+func shouldIgnore(a admission.Attributes) bool {
+	resource := a.GetResource().GroupResource()
+	if resource != api.Resource("pods") {
+		return true
 	}
-
-	whitelist, err := labels.ConvertSelectorToLabelsMap(p.clusterNodeSelectors[namespace.Name])
-	if err != nil {
-		return err
+	if a.GetSubresource() != "" {
+		// only run the checks below on pods proper and not subresources
+		return true
 	}
 
-	// Merge pod node selector = namespace node selector + current pod node selector
-	podNodeSelectorLabels := labels.Merge(namespaceNodeSelector, pod.Spec.NodeSelector)
-
-	// whitelist verification
-	if !labels.AreLabelsInWhiteList(podNodeSelectorLabels, whitelist) {
-		return errors.NewForbidden(resource, name, fmt.Errorf("pod node label selector labels conflict with its namespace whitelist"))
+	_, ok := a.GetObject().(*api.Pod)
+	if !ok {
+		glog.Errorf("expected pod but got %s", a.GetKind().Kind)
+		return true
 	}
 
-	// Updated pod node selector = namespace node selector + current pod node selector
-	pod.Spec.NodeSelector = map[string]string(podNodeSelectorLabels)
-	return nil
+	return false
 }
 
 func NewPodNodeSelector(clusterNodeSelectors map[string]string) *podNodeSelector {

plugin/pkg/admission/podnodeselector/admission_test.go

@@ -175,6 +175,12 @@ func TestPodAdmission(t *testing.T) {
 		if test.admit && !labels.Equals(test.mergedNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
 			t.Errorf("Test: %s, expected: %s but got: %s", test.testName, test.mergedNodeSelector, pod.Spec.NodeSelector)
 		}
+		err = handler.Validate(admission.NewAttributesRecord(pod, nil, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Create, nil))
+		if test.admit && err != nil {
+			t.Errorf("Test: %s, expected no error but got: %s", test.testName, err)
+		} else if !test.admit && err == nil {
+			t.Errorf("Test: %s, expected an error", test.testName)
+		}
 
 		// handles update of uninitialized pod like it's newly created.
 		err = handler.Admit(admission.NewAttributesRecord(pod, &oldPod, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Update, nil))
@@ -186,6 +192,12 @@ func TestPodAdmission(t *testing.T) {
 		if test.admit && !labels.Equals(test.mergedNodeSelector, labels.Set(pod.Spec.NodeSelector)) {
 			t.Errorf("Test: %s, expected: %s but got: %s", test.testName, test.mergedNodeSelector, pod.Spec.NodeSelector)
 		}
+		err = handler.Validate(admission.NewAttributesRecord(pod, &oldPod, api.Kind("Pod").WithVersion("version"), "testNamespace", namespace.ObjectMeta.Name, api.Resource("pods").WithVersion("version"), "", admission.Update, nil))
+		if test.admit && err != nil {
+			t.Errorf("Test: %s, expected no error but got: %s", test.testName, err)
+		} else if !test.admit && err == nil {
+			t.Errorf("Test: %s, expected an error", test.testName)
+		}
 	}
 }
 

plugin/pkg/admission/priority/admission.go

@@ -64,6 +64,8 @@ type PriorityPlugin struct {
 	globalDefaultPriority *int32
 }
 
+var _ admission.MutationInterface = &PriorityPlugin{}
+var _ admission.ValidationInterface = &PriorityPlugin{}
 var _ = kubeapiserveradmission.WantsInternalKubeInformerFactory(&PriorityPlugin{})
 var _ = kubeapiserveradmission.WantsInternalKubeClientSet(&PriorityPlugin{})
 
@@ -102,11 +104,10 @@ var (
 	priorityClassResource = scheduling.Resource("priorityclasses")
 )
 
-// Admit checks Pods and PriorityClasses and admits or rejects them. It also resolves the priority of pods based on their PriorityClass.
+// Admit checks Pods and admits or rejects them. It also resolves the priority of pods based on their PriorityClass.
 func (p *PriorityPlugin) Admit(a admission.Attributes) error {
 	operation := a.GetOperation()
-	// Ignore all calls to subresources or resources other than pods.
-	// Ignore all operations other than Create and Update.
+	// Ignore all calls to subresources
 	if len(a.GetSubresource()) != 0 {
 		return nil
 	}
@@ -118,9 +119,23 @@ func (p *PriorityPlugin) Admit(a admission.Attributes) error {
 		}
 		return nil
 
+	default:
+		return nil
+	}
+}
+
+// Validate checks PriorityClasses and admits or rejects them.
+func (p *PriorityPlugin) Validate(a admission.Attributes) error {
+	operation := a.GetOperation()
+	// Ignore all calls to subresources
+	if len(a.GetSubresource()) != 0 {
+		return nil
+	}
+
+	switch a.GetResource().GroupResource() {
 	case priorityClassResource:
 		if operation == admission.Create || operation == admission.Update {
-			return p.admitPriorityClass(a)
+			return p.validatePriorityClass(a)
 		}
 		if operation == admission.Delete {
 			p.invalidateCachedDefaultPriority()
@@ -176,8 +191,8 @@ func (p *PriorityPlugin) admitPod(a admission.Attributes) error {
 	return nil
 }
 
-// admitPriorityClass ensures that the value field is not larger than the highest user definable priority. If the GlobalDefault is set, it ensures that there is no other PriorityClass whose GlobalDefault is set.
-func (p *PriorityPlugin) admitPriorityClass(a admission.Attributes) error {
+// validatePriorityClass ensures that the value field is not larger than the highest user definable priority. If the GlobalDefault is set, it ensures that there is no other PriorityClass whose GlobalDefault is set.
+func (p *PriorityPlugin) validatePriorityClass(a admission.Attributes) error {
 	operation := a.GetOperation()
 	pc, ok := a.GetObject().(*scheduling.PriorityClass)
 	if !ok {

plugin/pkg/admission/priority/admission_test.go

@@ -147,7 +147,7 @@ func TestPriorityClassAdmission(t *testing.T) {
 			admission.Create,
 			nil,
 		)
-		err := ctrl.Admit(attrs)
+		err := ctrl.Validate(attrs)
 		glog.Infof("Got %v", err)
 		if err != nil && !test.expectError {
 			t.Errorf("Test %q: unexpected error received: %v", test.name, err)
@@ -219,7 +219,7 @@ func TestDefaultPriority(t *testing.T) {
 			t.Errorf("Test %q: expected default priority %d, but got %d", test.name, test.expectedDefaultBefore, defaultPriority)
 		}
 		if test.attributes != nil {
-			err := ctrl.Admit(test.attributes)
+			err := ctrl.Validate(test.attributes)
 			if err != nil {
 				t.Errorf("Test %q: unexpected error received: %v", test.name, err)
 			}