Merge pull request #55058 from deads2k/admission-11-split-doubles

Automatic merge from submit-queue (batch tested with PRs 53273, 55058, 55237, 50140). If you want to cherry-pick this change to another branch, please follow the instructions <a href="https://github.com/kubernetes/community/blob/master/contributors/devel/cherry-picks.md">here</a>.

split some admission plugins into mutation and validation halves

Splits the podnodeselector, serviceaccount, and priority admission plugins into validating and mutating admission plugins.

@kubernetes/sig-api-machinery-pr-reviews

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,26 +95,9 @@ 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
-		return nil
-	}
-
-	obj := a.GetObject()
-	pod, ok := obj.(*api.Pod)
-	if !ok {
-		glog.Errorf("expected pod but got %s", a.GetKind().Kind)
-		return nil
-	}
-
-	if !p.WaitForReady() {
-		return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request"))
-	}
-
 	updateInitialized, err := util.IsUpdatingInitializedObject(a)
 	if err != nil {
 		return err
@@ -121,51 +106,96 @@ func (p *podNodeSelector) Admit(a admission.Attributes) error {
 		// node selector of an initialized pod is immutable
 		return nil
 	}
-
-	name := pod.Name
-	nsName := a.GetNamespace()
-	var namespace *api.Namespace
-
-	namespace, err = p.namespaceLister.Get(nsName)
-	if errors.IsNotFound(err) {
-		namespace, err = p.defaultGetNamespace(nsName)
-		if err != nil {
-			if errors.IsNotFound(err) {
-				return err
-			}
-			return errors.NewInternalError(err)
-		}
-	} else if err != nil {
-		return errors.NewInternalError(err)
+	if !p.WaitForReady() {
+		return admission.NewForbidden(a, fmt.Errorf("not yet ready to handle request"))
 	}
 
-	namespaceNodeSelector, err := p.getNodeSelectorMap(namespace)
+	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, name, fmt.Errorf("pod node label selector conflicts with its namespace node label selector"))
-	}
-
-	whitelist, err := labels.ConvertSelectorToLabelsMap(p.clusterNodeSelectors[namespace.Name])
-	if err != nil {
-		return err
+		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)
+}
 
-	// whitelist verification
-	if !labels.AreLabelsInWhiteList(podNodeSelectorLabels, whitelist) {
-		return errors.NewForbidden(resource, name, fmt.Errorf("pod node label selector labels conflict with its namespace whitelist"))
+// 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"))
+	}
+
+	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"))
+	}
+
+	// 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"))
 	}
 
-	// Updated pod node selector = namespace node selector + current pod node selector
-	pod.Spec.NodeSelector = map[string]string(podNodeSelectorLabels)
 	return nil
 }
 
+func (p *podNodeSelector) getNamespaceNodeSelectorMap(namespaceName string) (labels.Set, error) {
+	namespace, err := p.namespaceLister.Get(namespaceName)
+	if errors.IsNotFound(err) {
+		namespace, err = p.defaultGetNamespace(namespaceName)
+		if err != nil {
+			if errors.IsNotFound(err) {
+				return nil, err
+			}
+			return nil, errors.NewInternalError(err)
+		}
+	} else if err != nil {
+		return nil, errors.NewInternalError(err)
+	}
+
+	return p.getNodeSelectorMap(namespace)
+}
+
+func shouldIgnore(a admission.Attributes) bool {
+	resource := a.GetResource().GroupResource()
+	if resource != api.Resource("pods") {
+		return true
+	}
+	if a.GetSubresource() != "" {
+		// only run the checks below on pods proper and not subresources
+		return true
+	}
+
+	_, ok := a.GetObject().(*api.Pod)
+	if !ok {
+		glog.Errorf("expected pod but got %s", a.GetKind().Kind)
+		return true
+	}
+
+	return false
+}
+
 func NewPodNodeSelector(clusterNodeSelectors map[string]string) *podNodeSelector {
 	return &podNodeSelector{
 		Handler:              admission.NewHandler(admission.Create, admission.Update),

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)
 			}