apf: allow admin to change the Exempt field only of the exempt pl

Signed-off-by: Mike Spreitzer <mspreitz@us.ibm.com>

pkg/apis/flowcontrol/validation/validation.go

@@ -345,19 +345,41 @@ func ValidatePriorityLevelConfiguration(pl *flowcontrol.PriorityLevelConfigurati
 	allErrs := apivalidation.ValidateObjectMeta(&pl.ObjectMeta, false, ValidatePriorityLevelConfigurationName, field.NewPath("metadata"))
 	specPath := field.NewPath("spec")
 	allErrs = append(allErrs, ValidatePriorityLevelConfigurationSpec(&pl.Spec, requestGV, pl.Name, specPath)...)
-	if mand, ok := internalbootstrap.MandatoryPriorityLevelConfigurations[pl.Name]; ok {
-		// Check for almost exact equality.  This is a pretty
-		// strict test, and it is OK in this context because both
-		// sides of this comparison are intended to ultimately
-		// come from the same code.
-		if !apiequality.Semantic.DeepEqual(pl.Spec, mand.Spec) {
-			allErrs = append(allErrs, field.Invalid(specPath, pl.Spec, fmt.Sprintf("spec of '%s' must equal the fixed value", pl.Name)))
-		}
-	}
+	allErrs = append(allErrs, ValidateIfMandatoryPriorityLevelConfigurationObject(pl, specPath)...)
 	allErrs = append(allErrs, ValidatePriorityLevelConfigurationStatus(&pl.Status, field.NewPath("status"))...)
 	return allErrs
 }
 
+func ValidateIfMandatoryPriorityLevelConfigurationObject(pl *flowcontrol.PriorityLevelConfiguration, fldPath *field.Path) field.ErrorList {
+	var allErrs field.ErrorList
+	mand, ok := internalbootstrap.MandatoryPriorityLevelConfigurations[pl.Name]
+	if !ok {
+		return allErrs
+	}
+
+	if pl.Name == flowcontrol.PriorityLevelConfigurationNameExempt {
+		// we allow the admin to change the contents of the 'Exempt' field of
+		// the singleton 'exempt' priority level object, every other fields of
+		// the Spec should not be allowed to change.
+		want := &mand.Spec
+		have := pl.Spec.DeepCopy()
+		have.Exempt = want.Exempt
+		if !apiequality.Semantic.DeepEqual(want, have) {
+			allErrs = append(allErrs, field.Invalid(fldPath, pl.Spec, fmt.Sprintf("spec of '%s' except the 'spec.exempt' field must equal the fixed value", pl.Name)))
+		}
+		return allErrs
+	}
+
+	// Check for almost exact equality.  This is a pretty
+	// strict test, and it is OK in this context because both
+	// sides of this comparison are intended to ultimately
+	// come from the same code.
+	if !apiequality.Semantic.DeepEqual(pl.Spec, mand.Spec) {
+		allErrs = append(allErrs, field.Invalid(fldPath, pl.Spec, fmt.Sprintf("spec of '%s' must equal the fixed value", pl.Name)))
+	}
+	return allErrs
+}
+
 // ValidatePriorityLevelConfigurationSpec validates priority-level-configuration's spec.
 func ValidatePriorityLevelConfigurationSpec(spec *flowcontrol.PriorityLevelConfigurationSpec, requestGV schema.GroupVersion, name string, fldPath *field.Path) field.ErrorList {
 	var allErrs field.ErrorList

pkg/apis/flowcontrol/validation/validation_test.go

@@ -32,6 +32,7 @@ import (
 	"k8s.io/apimachinery/pkg/util/validation/field"
 	"k8s.io/apiserver/pkg/authentication/user"
 	"k8s.io/kubernetes/pkg/apis/flowcontrol"
+	"k8s.io/kubernetes/pkg/apis/flowcontrol/internalbootstrap"
 	"k8s.io/utils/pointer"
 )
 
@@ -759,6 +760,43 @@ func TestPriorityLevelConfigurationValidation(t *testing.T) {
 		},
 	}
 
+	badExemptSpec3 := flowcontrol.PriorityLevelConfigurationSpec{
+		Type:   flowcontrol.PriorityLevelEnablementExempt,
+		Exempt: &flowcontrol.ExemptPriorityLevelConfiguration{},
+		Limited: &flowcontrol.LimitedPriorityLevelConfiguration{
+			NominalConcurrencyShares: 42,
+			LimitResponse: flowcontrol.LimitResponse{
+				Type: flowcontrol.LimitResponseTypeReject},
+		},
+	}
+
+	validChangesInExemptFieldOfExemptPLFn := func() flowcontrol.PriorityLevelConfigurationSpec {
+		have, _ := internalbootstrap.MandatoryPriorityLevelConfigurations[flowcontrol.PriorityLevelConfigurationNameExempt]
+		return flowcontrol.PriorityLevelConfigurationSpec{
+			Type: flowcontrol.PriorityLevelEnablementExempt,
+			Exempt: &flowcontrol.ExemptPriorityLevelConfiguration{
+				NominalConcurrencyShares: pointer.Int32(*have.Spec.Exempt.NominalConcurrencyShares + 10),
+				LendablePercent:          pointer.Int32(*have.Spec.Exempt.LendablePercent + 10),
+			},
+		}
+	}
+
+	exemptTypeRepurposed := &flowcontrol.PriorityLevelConfiguration{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: flowcontrol.PriorityLevelConfigurationNameExempt,
+		},
+		Spec: flowcontrol.PriorityLevelConfigurationSpec{
+			// changing the type from exempt to limited
+			Type:   flowcontrol.PriorityLevelEnablementLimited,
+			Exempt: &flowcontrol.ExemptPriorityLevelConfiguration{},
+			Limited: &flowcontrol.LimitedPriorityLevelConfiguration{
+				NominalConcurrencyShares: 42,
+				LimitResponse: flowcontrol.LimitResponse{
+					Type: flowcontrol.LimitResponseTypeReject},
+			},
+		},
+	}
+
 	testCases := []struct {
 		name                       string
 		priorityLevelConfiguration *flowcontrol.PriorityLevelConfiguration
@@ -788,7 +826,7 @@ func TestPriorityLevelConfigurationValidation(t *testing.T) {
 		},
 		expectedErrors: field.ErrorList{
 			field.Invalid(field.NewPath("spec").Child("type"), flowcontrol.PriorityLevelEnablementLimited, "type must be 'Exempt' if and only if name is 'exempt'"),
-			field.Invalid(field.NewPath("spec"), badSpec, "spec of 'exempt' must equal the fixed value"),
+			field.Invalid(field.NewPath("spec"), badSpec, "spec of 'exempt' except the 'spec.exempt' field must equal the fixed value"),
 		},
 	}, {
 		name: "limited must not set exempt priority level configuration for borrowing",
@@ -819,7 +857,6 @@ func TestPriorityLevelConfigurationValidation(t *testing.T) {
 		expectedErrors: field.ErrorList{
 			field.Invalid(field.NewPath("spec").Child("exempt").Child("nominalConcurrencyShares"), int32(-1), "must be a non-negative integer"),
 			field.Invalid(field.NewPath("spec").Child("exempt").Child("lendablePercent"), int32(101), "must be between 0 and 100, inclusive"),
-			field.Invalid(field.NewPath("spec"), badExemptSpec1, "spec of 'exempt' must equal the fixed value"),
 		},
 	}, {
 		name: "exempt priority level should have appropriate values for Exempt field",
@@ -832,8 +869,36 @@ func TestPriorityLevelConfigurationValidation(t *testing.T) {
 		expectedErrors: field.ErrorList{
 			field.Invalid(field.NewPath("spec").Child("exempt").Child("nominalConcurrencyShares"), int32(-1), "must be a non-negative integer"),
 			field.Invalid(field.NewPath("spec").Child("exempt").Child("lendablePercent"), int32(-1), "must be between 0 and 100, inclusive"),
-			field.Invalid(field.NewPath("spec"), badExemptSpec2, "spec of 'exempt' must equal the fixed value"),
 		},
+	}, {
+		name:                       "admins are not allowed to repurpose the 'exempt' pl to a limited type",
+		priorityLevelConfiguration: exemptTypeRepurposed,
+		expectedErrors: field.ErrorList{
+			field.Invalid(field.NewPath("spec").Child("type"), flowcontrol.PriorityLevelEnablementLimited, "type must be 'Exempt' if and only if name is 'exempt'"),
+			field.Required(field.NewPath("spec").Child("exempt"), "must be nil if the type is Limited"),
+			field.Invalid(field.NewPath("spec"), exemptTypeRepurposed.Spec, "spec of 'exempt' except the 'spec.exempt' field must equal the fixed value"),
+		},
+	}, {
+		name: "admins are not allowed to change any field of the 'exempt' pl except 'Exempt'",
+		priorityLevelConfiguration: &flowcontrol.PriorityLevelConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: flowcontrol.PriorityLevelConfigurationNameExempt,
+			},
+			Spec: badExemptSpec3,
+		},
+		expectedErrors: field.ErrorList{
+			field.Invalid(field.NewPath("spec"), badExemptSpec3, "spec of 'exempt' except the 'spec.exempt' field must equal the fixed value"),
+			field.Forbidden(field.NewPath("spec").Child("limited"), "must be nil if the type is not Limited"),
+		},
+	}, {
+		name: "admins are allowed to change the Exempt field of the 'exempt' pl",
+		priorityLevelConfiguration: &flowcontrol.PriorityLevelConfiguration{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: flowcontrol.PriorityLevelConfigurationNameExempt,
+			},
+			Spec: validChangesInExemptFieldOfExemptPLFn(),
+		},
+		expectedErrors: field.ErrorList{},
 	}, {
 		name: "limited must not set exempt priority level configuration for borrowing",
 		priorityLevelConfiguration: &flowcontrol.PriorityLevelConfiguration{

pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go

@@ -35,6 +35,14 @@ import (
 )
 
 func TestEnsurePriorityLevel(t *testing.T) {
+	validExemptPL := func() *flowcontrolv1beta3.PriorityLevelConfiguration {
+		copy := bootstrap.MandatoryPriorityLevelConfigurationExempt.DeepCopy()
+		copy.Annotations[flowcontrolv1beta3.AutoUpdateAnnotationKey] = "false"
+		copy.Spec.Exempt.NominalConcurrencyShares = pointer.Int32(10)
+		copy.Spec.Exempt.LendablePercent = pointer.Int32(50)
+		return copy
+	}()
+
 	tests := []struct {
 		name      string
 		strategy  func() EnsureStrategy[*flowcontrolv1beta3.PriorityLevelConfiguration]
@@ -87,6 +95,15 @@ func TestEnsurePriorityLevel(t *testing.T) {
 			current:   newPLConfiguration("pl1").WithAutoUpdateAnnotation("false").WithLimited(10).Object(),
 			expected:  newPLConfiguration("pl1").WithAutoUpdateAnnotation("true").WithLimited(20).Object(),
 		},
+		{
+			name:     "admin changes the Exempt field of the exempt priority level configuration",
+			strategy: NewMandatoryEnsureStrategy[*flowcontrolv1beta3.PriorityLevelConfiguration],
+			bootstrap: func() *flowcontrolv1beta3.PriorityLevelConfiguration {
+				return bootstrap.MandatoryPriorityLevelConfigurationExempt
+			}(),
+			current:  validExemptPL,
+			expected: validExemptPL,
+		},
 	}
 
 	for _, test := range tests {

pkg/registry/flowcontrol/ensurer/strategy.go

@@ -150,6 +150,11 @@ func NewSuggestedEnsureStrategy[ObjectType configurationObjectType]() EnsureStra
 func NewMandatoryEnsureStrategy[ObjectType configurationObjectType]() EnsureStrategy[ObjectType] {
 	return &strategy[ObjectType]{
 		alwaysAutoUpdateSpecFn: func(want, have ObjectType) bool {
+			if want.GetName() == flowcontrolv1beta3.PriorityLevelConfigurationNameExempt {
+				// we allow the cluster operator to update the spec of the
+				// singleton 'exempt' prioritylevelconfiguration object.
+				return false
+			}
 			return true
 		},
 		name: "mandatory",