Disallow PriorityClass names with 'system-' prefix for user defined priority classes #59382

bsalamat · 2018-02-06T01:06:55Z

What this PR does / why we need it:
This PR changes our Priority admission controller to disallow PriorityClass names with 'system-' prefix for user defined priority classes. Please refer to #59381 for reasons why we need this.

Which issue(s) this PR fixes (optional, in fixes #<issue number>(, fixes #<issue_number>, ...) format, will close the issue(s) when PR gets merged):
Fixes #59381

Release note:

Disallow PriorityClass names with 'system-' prefix for user defined priority classes.

ref #57471
/sig scheduling
/assign @liggitt

dixudx · 2018-02-06T02:38:56Z

/lgtm

liggitt · 2018-02-07T02:05:44Z

@kubernetes/sig-scheduling-api-reviews

liggitt · 2018-02-07T02:08:40Z

plugin/pkg/admission/priority/admission.go

@@ -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) {


If a priority class starting with "system-" is created while this plugin is not enabled, does this prevent update/deleting it?

+1, validate in api server?

If a priority class starting with "system-" is created while this plugin is not enabled, does this prevent update/deleting it?

It does not prevent deletion, but it does prevent updates. I think this should be fine. We don't allow updating names of priority classes anyway.
I move the new logic to the API side.

liggitt · 2018-02-07T02:10:20Z

plugin/pkg/admission/priority/admission.go

@@ -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))
 	}


Also, this is my first time in here. Below, this is attempting to prevent multiple priority class objects that say they are the default, but that's racy/best effort. What happens if multiple defaults end up in the system? Does this prevent those from being updated or deleted?

Also, the invalidateCachedDefaultPriority call wouldn't handle HA apiserver scenarios

What happens if multiple defaults end up in the system? Does this prevent those from being updated or deleted?

It does not prevent deletion, but it does not allow updates, which is fine in my opinion.

Also, the invalidateCachedDefaultPriority call wouldn't handle HA apiserver scenarios

Is this the scenario that is not handled correctly:

Server 1 is the leader. Admission controller populates its cache with default priority.

Server 1 loses its leadership and server 2 becomes leader. Server 2 populates its admission controller cache.

Global defalt priority class is deleted/updated.

Server 2 loses its leadership and server 1 becomes leader again.

Server 1's cache is stale!

This is certainly an issue we need to address. A possible solution is to not cache default priority, but it could cause performance regression. Is there a better solution?

…riority classes

bsalamat

Thanks, @liggitt. PTAL.

bsalamat · 2018-02-07T22:43:59Z

plugin/pkg/admission/priority/admission.go

@@ -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))
 	}


What happens if multiple defaults end up in the system? Does this prevent those from being updated or deleted?

It does not prevent deletion, but it does not allow updates, which is fine in my opinion.

bsalamat · 2018-02-07T22:50:32Z

plugin/pkg/admission/priority/admission.go

@@ -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))
 	}


Also, the invalidateCachedDefaultPriority call wouldn't handle HA apiserver scenarios

Is this the scenario that is not handled correctly:

Server 1 is the leader. Admission controller populates its cache with default priority.

Server 1 loses its leadership and server 2 becomes leader. Server 2 populates its admission controller cache.

Global defalt priority class is deleted/updated.

Server 2 loses its leadership and server 1 becomes leader again.

Server 1's cache is stale!

This is certainly an issue we need to address. A possible solution is to not cache default priority, but it could cause performance regression. Is there a better solution?

bsalamat · 2018-02-08T00:31:50Z

plugin/pkg/admission/priority/admission.go

@@ -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) {


If a priority class starting with "system-" is created while this plugin is not enabled, does this prevent update/deleting it?

It does not prevent deletion, but it does prevent updates. I think this should be fine. We don't allow updating names of priority classes anyway.
I move the new logic to the API side.

k82cn · 2018-02-09T14:14:51Z

/lgtm

liggitt · 2018-02-09T15:35:23Z

validation change looks reasonable (and is acceptable imo because priority feature was alpha in 1.9)... the error message from the admission plugin when an unknown system-... priority is used is misleading ("no PriorityClass with name %v was found") since no PriorityClass with that name would be allowed, but that can be improved as a follow up

/approve

wgliang · 2018-02-09T16:21:44Z

plugin/pkg/admission/priority/admission.go

@@ -44,6 +44,8 @@ const (
 )

 // 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,


Should we manage the magic numbers like 1000? At least we should indicate why it is 1000.
WDYT, Bobby?

@wgliang, the exact value of priority does not have any significance, only the relative value of priority compared to other priorites matters. So, if system-node-critical value was SystemCriticalPriority + 1, it would have had the same effect as SystemCriticalPriority + 1000, but we choose to use a larger number so that we can add more priority classes between the two existing ones if there is a need in the future.

bsalamat · 2018-02-09T18:23:53Z

@deads2k Could you please approve the change under /admission? It is only a change in the comment.

deads2k · 2018-02-09T19:55:13Z

Tightening validation sets of my spidey-sense, but it looks like api-approvers were already here.

/approve

k8s-ci-robot · 2018-02-09T19:55:41Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: bsalamat, deads2k, dixudx, k82cn, liggitt

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these OWNERS Files:

~~pkg/apis/OWNERS~~ [liggitt]
~~plugin/pkg/admission/OWNERS~~ [deads2k]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

k8s-github-robot · 2018-02-09T20:48:56Z

/test all [submit-queue is verifying that this PR is safe to merge]

k8s-github-robot · 2018-02-09T21:29:56Z

Automatic merge from submit-queue. If you want to cherry-pick this change to another branch, please follow the instructions here.

k8s-ci-robot assigned liggitt Feb 6, 2018

k8s-ci-robot requested review from deads2k and derekwaynecarr February 6, 2018 01:06

k8s-ci-robot assigned dixudx Feb 6, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 6, 2018

k8s-ci-robot added the kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API label Feb 7, 2018

liggitt reviewed Feb 7, 2018

View reviewed changes

Disallow PriorityClass names with 'system-' prefix for user defined p…

1016d2d

…riority classes

bsalamat force-pushed the no_system_priority branch from 37762f7 to d1872a6 Compare February 8, 2018 00:31

k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels Feb 8, 2018

bsalamat commented Feb 8, 2018

View reviewed changes

k8s-github-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 8, 2018

Moved validation to the API side

6bad08a

bsalamat force-pushed the no_system_priority branch from d1872a6 to 6bad08a Compare February 8, 2018 01:47

k8s-ci-robot added size/S Denotes a PR that changes 10-29 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Feb 8, 2018

k8s-ci-robot assigned k82cn Feb 9, 2018

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 9, 2018

wgliang reviewed Feb 9, 2018

View reviewed changes

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 9, 2018

k8s-github-robot merged commit 15ad217 into kubernetes:master Feb 9, 2018

bsalamat mentioned this pull request Feb 9, 2018

Caching value of default priority in Priority admission controller is not HA safe #59679

Closed

bsalamat deleted the no_system_priority branch February 12, 2018 17:40

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Disallow PriorityClass names with 'system-' prefix for user defined priority classes #59382

Disallow PriorityClass names with 'system-' prefix for user defined priority classes #59382

bsalamat commented Feb 6, 2018 •

edited

Loading

dixudx commented Feb 6, 2018

liggitt commented Feb 7, 2018

liggitt Feb 7, 2018 •

edited

Loading

k82cn Feb 7, 2018

bsalamat Feb 8, 2018

liggitt Feb 7, 2018

liggitt Feb 7, 2018

bsalamat Feb 7, 2018

bsalamat Feb 7, 2018

bsalamat left a comment

bsalamat Feb 7, 2018

bsalamat Feb 7, 2018

bsalamat Feb 8, 2018

k82cn commented Feb 9, 2018

liggitt commented Feb 9, 2018 •

edited

Loading

wgliang Feb 9, 2018

bsalamat Feb 9, 2018

bsalamat commented Feb 9, 2018

deads2k commented Feb 9, 2018

k8s-ci-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

Disallow PriorityClass names with 'system-' prefix for user defined priority classes #59382

Disallow PriorityClass names with 'system-' prefix for user defined priority classes #59382

Conversation

bsalamat commented Feb 6, 2018 • edited Loading

dixudx commented Feb 6, 2018

liggitt commented Feb 7, 2018

liggitt Feb 7, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsalamat left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

k82cn commented Feb 9, 2018

liggitt commented Feb 9, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bsalamat commented Feb 9, 2018

deads2k commented Feb 9, 2018

k8s-ci-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

k8s-github-robot commented Feb 9, 2018

bsalamat commented Feb 6, 2018 •

edited

Loading

liggitt Feb 7, 2018 •

edited

Loading

liggitt commented Feb 9, 2018 •

edited

Loading