From 1016d2d16adaa44b8131abe9b7f280155e722e05 Mon Sep 17 00:00:00 2001 From: "Bobby (Babak) Salamat" Date: Mon, 5 Feb 2018 16:57:32 -0800 Subject: [PATCH 1/2] Disallow PriorityClass names with 'system-' prefix for user defined priority classes --- plugin/pkg/admission/priority/admission.go | 7 +++++++ plugin/pkg/admission/priority/admission_test.go | 15 +++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/plugin/pkg/admission/priority/admission.go b/plugin/pkg/admission/priority/admission.go index 686f98bb5a6..ab39703b74d 100644 --- a/plugin/pkg/admission/priority/admission.go +++ b/plugin/pkg/admission/priority/admission.go @@ -19,6 +19,7 @@ package admission import ( "fmt" "io" + "strings" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/labels" @@ -41,6 +42,9 @@ const ( HighestUserDefinablePriority = 1000000000 // SystemCriticalPriority is the beginning of the range of priority values for critical system components. SystemCriticalPriority = 2 * HighestUserDefinablePriority + // SystemPriorityClassPrefix is the prefix reserved for system priority class names. Other priority + // classes are not allowed to start with this prefix. + SystemPriorityClassPrefix = "system-" ) // SystemPriorityClasses defines special priority classes which are used by system critical pods that should not be preempted by workload pods. @@ -203,6 +207,9 @@ func (p *PriorityPlugin) validatePriorityClass(a admission.Attributes) error { if pc.Value > HighestUserDefinablePriority { return admission.NewForbidden(a, fmt.Errorf("maximum allowed value of a user defined priority is %v", HighestUserDefinablePriority)) } + if strings.HasPrefix(pc.Name, SystemPriorityClassPrefix) { + return admission.NewForbidden(a, fmt.Errorf("priority class names with '%v' prefix are reserved for system use only: %v", SystemPriorityClassPrefix, pc.Name)) + } if _, ok := SystemPriorityClasses[pc.Name]; ok { return admission.NewForbidden(a, fmt.Errorf("the name of the priority class is a reserved name for system use only: %v", pc.Name)) } diff --git a/plugin/pkg/admission/priority/admission_test.go b/plugin/pkg/admission/priority/admission_test.go index c9122ac63f0..0066c2a731f 100644 --- a/plugin/pkg/admission/priority/admission_test.go +++ b/plugin/pkg/admission/priority/admission_test.go @@ -127,6 +127,21 @@ func TestPriorityClassAdmission(t *testing.T) { systemClass, true, }, + { + "forbidden system name prefix", + []*scheduling.PriorityClass{}, + &scheduling.PriorityClass{ + TypeMeta: metav1.TypeMeta{ + Kind: "PriorityClass", + }, + ObjectMeta: metav1.ObjectMeta{ + Name: "system-something", + }, + Value: 5, + Description: "Name with 'system-' prefix is reserved for system use", + }, + true, + }, } for _, test := range tests { From 6bad08ab0c3ea52eef071bab351ce2b968efc87f Mon Sep 17 00:00:00 2001 From: "Bobby (Babak) Salamat" Date: Wed, 7 Feb 2018 15:30:26 -0800 Subject: [PATCH 2/2] Moved validation to the API side --- pkg/apis/scheduling/types.go | 3 +++ pkg/apis/scheduling/validation/validation.go | 14 +++++++++++--- pkg/apis/scheduling/validation/validation_test.go | 4 ++++ plugin/pkg/admission/priority/admission.go | 9 ++------- plugin/pkg/admission/priority/admission_test.go | 15 --------------- 5 files changed, 20 insertions(+), 25 deletions(-) diff --git a/pkg/apis/scheduling/types.go b/pkg/apis/scheduling/types.go index 06cceadb74d..5130d624a03 100644 --- a/pkg/apis/scheduling/types.go +++ b/pkg/apis/scheduling/types.go @@ -23,6 +23,9 @@ const ( // that do not specify any priority class and there is no priority class // marked as default. DefaultPriorityWhenNoDefaultClassExists = 0 + // SystemPriorityClassPrefix is the prefix reserved for system priority class names. Other priority + // classes are not allowed to start with this prefix. + SystemPriorityClassPrefix = "system-" ) // +genclient diff --git a/pkg/apis/scheduling/validation/validation.go b/pkg/apis/scheduling/validation/validation.go index f4fac9d9cea..c8b85ef85d4 100644 --- a/pkg/apis/scheduling/validation/validation.go +++ b/pkg/apis/scheduling/validation/validation.go @@ -17,14 +17,22 @@ limitations under the License. package validation import ( + "strings" + "k8s.io/apimachinery/pkg/util/validation/field" apivalidation "k8s.io/kubernetes/pkg/apis/core/validation" "k8s.io/kubernetes/pkg/apis/scheduling" ) -// ValidatePriorityClassName can be used to check whether the given priority -// class name is valid. -var ValidatePriorityClassName = apivalidation.NameIsDNSSubdomain +// ValidatePriorityClassName checks whether the given priority class name is valid. +func ValidatePriorityClassName(name string, prefix bool) []string { + var allErrs []string + if strings.HasPrefix(name, scheduling.SystemPriorityClassPrefix) { + allErrs = append(allErrs, "priority class names with '"+scheduling.SystemPriorityClassPrefix+"' prefix are reserved for system use only") + } + allErrs = append(allErrs, apivalidation.NameIsDNSSubdomain(name, prefix)...) + return allErrs +} // ValidatePriorityClass tests whether required fields in the PriorityClass are // set correctly. diff --git a/pkg/apis/scheduling/validation/validation_test.go b/pkg/apis/scheduling/validation/validation_test.go index 8b0cec40b6c..e35ee426936 100644 --- a/pkg/apis/scheduling/validation/validation_test.go +++ b/pkg/apis/scheduling/validation/validation_test.go @@ -53,6 +53,10 @@ func TestValidatePriorityClass(t *testing.T) { ObjectMeta: metav1.ObjectMeta{Name: "tier&1", Namespace: ""}, Value: 100, }, + "invalid system name": { + ObjectMeta: metav1.ObjectMeta{Name: scheduling.SystemPriorityClassPrefix + "test"}, + Value: 100, + }, } for k, v := range errorCases { diff --git a/plugin/pkg/admission/priority/admission.go b/plugin/pkg/admission/priority/admission.go index ab39703b74d..5a0c98d278c 100644 --- a/plugin/pkg/admission/priority/admission.go +++ b/plugin/pkg/admission/priority/admission.go @@ -19,7 +19,6 @@ package admission import ( "fmt" "io" - "strings" "k8s.io/apimachinery/pkg/api/errors" "k8s.io/apimachinery/pkg/labels" @@ -42,12 +41,11 @@ const ( HighestUserDefinablePriority = 1000000000 // SystemCriticalPriority is the beginning of the range of priority values for critical system components. SystemCriticalPriority = 2 * HighestUserDefinablePriority - // SystemPriorityClassPrefix is the prefix reserved for system priority class names. Other priority - // classes are not allowed to start with this prefix. - SystemPriorityClassPrefix = "system-" ) // SystemPriorityClasses defines special priority classes which are used by system critical pods that should not be preempted by workload pods. +// NOTE: In order to avoid conflict of names with user-defined priority classes, all the names must +// start with scheduling.SystemPriorityClassPrefix which is by default "system-". var SystemPriorityClasses = map[string]int32{ "system-cluster-critical": SystemCriticalPriority, "system-node-critical": SystemCriticalPriority + 1000, @@ -207,9 +205,6 @@ func (p *PriorityPlugin) validatePriorityClass(a admission.Attributes) error { if pc.Value > HighestUserDefinablePriority { return admission.NewForbidden(a, fmt.Errorf("maximum allowed value of a user defined priority is %v", HighestUserDefinablePriority)) } - if strings.HasPrefix(pc.Name, SystemPriorityClassPrefix) { - return admission.NewForbidden(a, fmt.Errorf("priority class names with '%v' prefix are reserved for system use only: %v", SystemPriorityClassPrefix, pc.Name)) - } if _, ok := SystemPriorityClasses[pc.Name]; ok { return admission.NewForbidden(a, fmt.Errorf("the name of the priority class is a reserved name for system use only: %v", pc.Name)) } diff --git a/plugin/pkg/admission/priority/admission_test.go b/plugin/pkg/admission/priority/admission_test.go index 0066c2a731f..c9122ac63f0 100644 --- a/plugin/pkg/admission/priority/admission_test.go +++ b/plugin/pkg/admission/priority/admission_test.go @@ -127,21 +127,6 @@ func TestPriorityClassAdmission(t *testing.T) { systemClass, true, }, - { - "forbidden system name prefix", - []*scheduling.PriorityClass{}, - &scheduling.PriorityClass{ - TypeMeta: metav1.TypeMeta{ - Kind: "PriorityClass", - }, - ObjectMeta: metav1.ObjectMeta{ - Name: "system-something", - }, - Value: 5, - Description: "Name with 'system-' prefix is reserved for system use", - }, - true, - }, } for _, test := range tests {