diff --git a/pkg/apis/flowcontrol/validation/validation.go b/pkg/apis/flowcontrol/validation/validation.go index fe214285b1a..0e70c85002a 100644 --- a/pkg/apis/flowcontrol/validation/validation.go +++ b/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 diff --git a/pkg/apis/flowcontrol/validation/validation_test.go b/pkg/apis/flowcontrol/validation/validation_test.go index 2caef0dafb9..14300a45913 100644 --- a/pkg/apis/flowcontrol/validation/validation_test.go +++ b/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{ diff --git a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go b/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go index a155aafebb8..d76040c8b73 100644 --- a/pkg/registry/flowcontrol/ensurer/prioritylevelconfiguration_test.go +++ b/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 { diff --git a/pkg/registry/flowcontrol/ensurer/strategy.go b/pkg/registry/flowcontrol/ensurer/strategy.go index ff0b9c2717f..84008b559d2 100644 --- a/pkg/registry/flowcontrol/ensurer/strategy.go +++ b/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",