From f8e4e8abac8637f6510838d7d476a838ce612659 Mon Sep 17 00:00:00 2001 From: Abu Kashem Date: Mon, 15 May 2023 12:08:18 -0400 Subject: [PATCH] apf: add validation to exempt for borrowing Signed-off-by: Mike Spreitzer --- pkg/apis/flowcontrol/validation/validation.go | 18 +++++ .../flowcontrol/validation/validation_test.go | 79 +++++++++++++++++++ .../pkg/apis/flowcontrol/bootstrap/default.go | 4 + .../flowcontrol/bootstrap/default_test.go | 7 ++ 4 files changed, 108 insertions(+) diff --git a/pkg/apis/flowcontrol/validation/validation.go b/pkg/apis/flowcontrol/validation/validation.go index f83b2cbff67..fe214285b1a 100644 --- a/pkg/apis/flowcontrol/validation/validation.go +++ b/pkg/apis/flowcontrol/validation/validation.go @@ -369,7 +369,14 @@ func ValidatePriorityLevelConfigurationSpec(spec *flowcontrol.PriorityLevelConfi if spec.Limited != nil { allErrs = append(allErrs, field.Forbidden(fldPath.Child("limited"), "must be nil if the type is not Limited")) } + if spec.Exempt != nil { + allErrs = append(allErrs, ValidateExemptPriorityLevelConfiguration(spec.Exempt, fldPath.Child("exempt"))...) + } case flowcontrol.PriorityLevelEnablementLimited: + if spec.Exempt != nil { + allErrs = append(allErrs, field.Required(fldPath.Child("exempt"), "must be nil if the type is Limited")) + } + if spec.Limited == nil { allErrs = append(allErrs, field.Required(fldPath.Child("limited"), "must not be empty when type is Limited")) } else { @@ -399,6 +406,17 @@ func ValidateLimitedPriorityLevelConfiguration(lplc *flowcontrol.LimitedPriority return allErrs } +func ValidateExemptPriorityLevelConfiguration(eplc *flowcontrol.ExemptPriorityLevelConfiguration, fldPath *field.Path) field.ErrorList { + var allErrs field.ErrorList + if eplc.NominalConcurrencyShares != nil && *eplc.NominalConcurrencyShares < 0 { + allErrs = append(allErrs, field.Invalid(fldPath.Child("nominalConcurrencyShares"), *eplc.NominalConcurrencyShares, "must be a non-negative integer")) + } + if eplc.LendablePercent != nil && !(*eplc.LendablePercent >= 0 && *eplc.LendablePercent <= 100) { + allErrs = append(allErrs, field.Invalid(fldPath.Child("lendablePercent"), *eplc.LendablePercent, "must be between 0 and 100, inclusive")) + } + return allErrs +} + func getVersionedFieldNameForConcurrencyShares(requestGV schema.GroupVersion) string { switch { case requestGV == flowcontrolv1alpha1.SchemeGroupVersion || diff --git a/pkg/apis/flowcontrol/validation/validation_test.go b/pkg/apis/flowcontrol/validation/validation_test.go index 7b5f11b69c8..2caef0dafb9 100644 --- a/pkg/apis/flowcontrol/validation/validation_test.go +++ b/pkg/apis/flowcontrol/validation/validation_test.go @@ -743,6 +743,22 @@ func TestPriorityLevelConfigurationValidation(t *testing.T) { Type: flowcontrol.LimitResponseTypeReject}, }, } + + badExemptSpec1 := flowcontrol.PriorityLevelConfigurationSpec{ + Type: flowcontrol.PriorityLevelEnablementExempt, + Exempt: &flowcontrol.ExemptPriorityLevelConfiguration{ + NominalConcurrencyShares: pointer.Int32(-1), + LendablePercent: pointer.Int32(101), + }, + } + badExemptSpec2 := flowcontrol.PriorityLevelConfigurationSpec{ + Type: flowcontrol.PriorityLevelEnablementExempt, + Exempt: &flowcontrol.ExemptPriorityLevelConfiguration{ + NominalConcurrencyShares: pointer.Int32(-1), + LendablePercent: pointer.Int32(-1), + }, + } + testCases := []struct { name string priorityLevelConfiguration *flowcontrol.PriorityLevelConfiguration @@ -755,6 +771,10 @@ func TestPriorityLevelConfigurationValidation(t *testing.T) { }, Spec: flowcontrol.PriorityLevelConfigurationSpec{ Type: flowcontrol.PriorityLevelEnablementExempt, + Exempt: &flowcontrol.ExemptPriorityLevelConfiguration{ + NominalConcurrencyShares: pointer.Int32(0), + LendablePercent: pointer.Int32(0), + }, }, }, expectedErrors: field.ErrorList{}, @@ -770,6 +790,65 @@ func TestPriorityLevelConfigurationValidation(t *testing.T) { 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"), }, + }, { + name: "limited must not set exempt priority level configuration for borrowing", + priorityLevelConfiguration: &flowcontrol.PriorityLevelConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "broken-limited", + }, + Spec: flowcontrol.PriorityLevelConfigurationSpec{ + Type: flowcontrol.PriorityLevelEnablementLimited, + Exempt: &flowcontrol.ExemptPriorityLevelConfiguration{ + NominalConcurrencyShares: pointer.Int32(10), + LendablePercent: pointer.Int32(20), + }, + }, + }, + expectedErrors: field.ErrorList{ + field.Required(field.NewPath("spec").Child("exempt"), "must be nil if the type is Limited"), + field.Required(field.NewPath("spec").Child("limited"), "must not be empty when type is Limited"), + }, + }, { + name: "exempt priority level should have appropriate values for Exempt field", + priorityLevelConfiguration: &flowcontrol.PriorityLevelConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: flowcontrol.PriorityLevelConfigurationNameExempt, + }, + Spec: badExemptSpec1, + }, + 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", + priorityLevelConfiguration: &flowcontrol.PriorityLevelConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: flowcontrol.PriorityLevelConfigurationNameExempt, + }, + Spec: badExemptSpec2, + }, + 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: "limited must not set exempt priority level configuration for borrowing", + priorityLevelConfiguration: &flowcontrol.PriorityLevelConfiguration{ + ObjectMeta: metav1.ObjectMeta{ + Name: "broken-limited", + }, + Spec: flowcontrol.PriorityLevelConfigurationSpec{ + Type: flowcontrol.PriorityLevelEnablementLimited, + Exempt: &flowcontrol.ExemptPriorityLevelConfiguration{}, + }, + }, + expectedErrors: field.ErrorList{ + field.Required(field.NewPath("spec").Child("exempt"), "must be nil if the type is Limited"), + field.Required(field.NewPath("spec").Child("limited"), "must not be empty when type is Limited"), + }, }, { name: "limited requires more details", priorityLevelConfiguration: &flowcontrol.PriorityLevelConfiguration{ diff --git a/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default.go b/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default.go index 3859a54d1fe..b037371e3a8 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default.go @@ -89,6 +89,10 @@ var ( flowcontrol.PriorityLevelConfigurationNameExempt, flowcontrol.PriorityLevelConfigurationSpec{ Type: flowcontrol.PriorityLevelEnablementExempt, + Exempt: &flowcontrol.ExemptPriorityLevelConfiguration{ + NominalConcurrencyShares: pointer.Int32(0), + LendablePercent: pointer.Int32(0), + }, }, ) MandatoryPriorityLevelConfigurationCatchAll = newPriorityLevelConfiguration( diff --git a/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default_test.go b/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default_test.go index d65bb4be51f..241dd29d3f2 100644 --- a/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default_test.go +++ b/staging/src/k8s.io/apiserver/pkg/apis/flowcontrol/bootstrap/default_test.go @@ -114,4 +114,11 @@ func TestBootstrapPriorityLevelConfigurationWithBorrowing(t *testing.T) { t.Errorf("bootstrap PriorityLevelConfiguration objects not accounted by this test: %v", names) } } + exemptPL := MandatoryPriorityLevelConfigurationExempt + if exemptPL.Spec.Exempt.NominalConcurrencyShares != nil && *exemptPL.Spec.Exempt.NominalConcurrencyShares != 0 { + t.Errorf("Expected exempt priority level to have NominalConcurrencyShares==0 but got %d instead", *exemptPL.Spec.Exempt.NominalConcurrencyShares) + } + if exemptPL.Spec.Exempt.LendablePercent != nil && *exemptPL.Spec.Exempt.LendablePercent != 0 { + t.Errorf("Expected exempt priority level to have LendablePercent==0 but got %d instead", *exemptPL.Spec.Exempt.LendablePercent) + } }