From bfd966c4c2ff442e4a05b0a29dee21078180677f Mon Sep 17 00:00:00 2001
From: Jordan Liggitt <jliggitt@redhat.com>
Date: Mon, 2 Jul 2018 22:08:14 -0400
Subject: [PATCH] update priority admission for interoperability

---
 plugin/pkg/admission/priority/admission.go    | 29 ++++++--
 .../pkg/admission/priority/admission_test.go  | 68 +++++++++++++++++++
 2 files changed, 92 insertions(+), 5 deletions(-)

diff --git a/plugin/pkg/admission/priority/admission.go b/plugin/pkg/admission/priority/admission.go
index c60a01223bc..e7054a47464 100644
--- a/plugin/pkg/admission/priority/admission.go
+++ b/plugin/pkg/admission/priority/admission.go
@@ -97,6 +97,10 @@ var (
 // Admit checks Pods and admits or rejects them. It also resolves the priority of pods based on their PriorityClass.
 // Note that pod validation mechanism prevents update of a pod priority.
 func (p *priorityPlugin) Admit(a admission.Attributes) error {
+	if !utilfeature.DefaultFeatureGate.Enabled(features.PodPriority) {
+		return nil
+	}
+
 	operation := a.GetOperation()
 	// Ignore all calls to subresources
 	if len(a.GetSubresource()) != 0 {
@@ -105,7 +109,7 @@ func (p *priorityPlugin) Admit(a admission.Attributes) error {
 
 	switch a.GetResource().GroupResource() {
 	case podResource:
-		if operation == admission.Create {
+		if operation == admission.Create || operation == admission.Update {
 			return p.admitPod(a)
 		}
 		return nil
@@ -157,11 +161,22 @@ func (p *priorityPlugin) admitPod(a admission.Attributes) error {
 		return errors.NewBadRequest("resource was marked with kind Pod but was unable to be converted")
 	}
 
-	// Make sure that the client has not set `priority` at the time of pod creation.
-	if operation == admission.Create && pod.Spec.Priority != nil {
-		return admission.NewForbidden(a, fmt.Errorf("the integer value of priority must not be provided in pod spec. Priority admission controller populates the value from the given PriorityClass name"))
+	if operation == admission.Update {
+		oldPod, ok := a.GetOldObject().(*api.Pod)
+		if !ok {
+			return errors.NewBadRequest("resource was marked with kind Pod but was unable to be converted")
+		}
+
+		// This admission plugin set pod.Spec.Priority on create.
+		// Ensure the existing priority is preserved on update.
+		// API validation prevents mutations to Priority and PriorityClassName, so any other changes will fail update validation and not be persisted.
+		if pod.Spec.Priority == nil && oldPod.Spec.Priority != nil {
+			pod.Spec.Priority = oldPod.Spec.Priority
+		}
+		return nil
 	}
-	if utilfeature.DefaultFeatureGate.Enabled(features.PodPriority) {
+
+	if operation == admission.Create {
 		var priority int32
 		// TODO: @ravig - This is for backwards compatibility to ensure that critical pods with annotations just work fine.
 		// Remove when no longer needed.
@@ -194,6 +209,10 @@ func (p *priorityPlugin) admitPod(a admission.Attributes) error {
 
 			priority = pc.Value
 		}
+		// if the pod contained a priority that differs from the one computed from the priority class, error
+		if pod.Spec.Priority != nil && *pod.Spec.Priority != priority {
+			return admission.NewForbidden(a, fmt.Errorf("the integer value of priority (%d) must not be provided in pod spec; priority admission controller computed %d from the given PriorityClass name", *pod.Spec.Priority, priority))
+		}
 		pod.Spec.Priority = &priority
 	}
 	return nil
diff --git a/plugin/pkg/admission/priority/admission_test.go b/plugin/pkg/admission/priority/admission_test.go
index 21e5bf2e728..05f571f3e32 100644
--- a/plugin/pkg/admission/priority/admission_test.go
+++ b/plugin/pkg/admission/priority/admission_test.go
@@ -244,6 +244,7 @@ func TestDefaultPriority(t *testing.T) {
 	}
 }
 
+var zeroPriority = int32(0)
 var intPriority = int32(1000)
 
 func TestPodAdmission(t *testing.T) {
@@ -389,6 +390,52 @@ func TestPodAdmission(t *testing.T) {
 				PriorityClassName: scheduling.SystemClusterCritical,
 			},
 		},
+		// pod[9]: Pod with a priority value that matches the resolved priority
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "pod-w-zero-priority-in-nonsystem-namespace",
+				Namespace: "non-system-namespace",
+			},
+			Spec: api.PodSpec{
+				Containers: []api.Container{
+					{
+						Name: containerName,
+					},
+				},
+				Priority: &zeroPriority,
+			},
+		},
+		// pod[10]: Pod with a priority value that matches the resolved default priority
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "pod-w-priority-matching-default-priority",
+				Namespace: "non-system-namespace",
+			},
+			Spec: api.PodSpec{
+				Containers: []api.Container{
+					{
+						Name: containerName,
+					},
+				},
+				Priority: &defaultClass2.Value,
+			},
+		},
+		// pod[11]: Pod with a priority value that matches the resolved priority
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "pod-w-priority-matching-resolved-default-priority",
+				Namespace: metav1.NamespaceSystem,
+			},
+			Spec: api.PodSpec{
+				Containers: []api.Container{
+					{
+						Name: containerName,
+					},
+				},
+				PriorityClassName: systemClusterCritical.Name,
+				Priority:          &systemClusterCritical.Value,
+			},
+		},
 	}
 	// Enable PodPriority feature gate.
 	utilfeature.DefaultFeatureGate.Set(fmt.Sprintf("%s=true", features.PodPriority))
@@ -481,6 +528,27 @@ func TestPodAdmission(t *testing.T) {
 			scheduling.SystemCriticalPriority,
 			true,
 		},
+		{
+			"pod with priority that matches computed priority",
+			[]*scheduling.PriorityClass{nondefaultClass1},
+			*pods[9],
+			0,
+			false,
+		},
+		{
+			"pod with priority that matches default priority",
+			[]*scheduling.PriorityClass{defaultClass2},
+			*pods[10],
+			defaultClass2.Value,
+			false,
+		},
+		{
+			"pod with priority that matches resolved priority",
+			[]*scheduling.PriorityClass{systemClusterCritical},
+			*pods[11],
+			systemClusterCritical.Value,
+			false,
+		},
 	}
 
 	for _, test := range tests {