Skip to content

Relax namespace restriction for critical pods#76310

Merged
k8s-ci-robot merged 2 commits intokubernetes:masterfrom
ravisantoshgudimetla:fix-priority-quota
Nov 13, 2019
Merged

Relax namespace restriction for critical pods#76310
k8s-ci-robot merged 2 commits intokubernetes:masterfrom
ravisantoshgudimetla:fix-priority-quota

Conversation

@ravisantoshgudimetla
Copy link
Copy Markdown
Contributor

@ravisantoshgudimetla ravisantoshgudimetla commented Apr 9, 2019 •
edited by liggitt
Loading

What type of PR is this?

/kind bug

What this PR does / why we need it:

I think, we should relax the restriction on critical pods to be created within kube-system, I have outlined an approach which ensures that, initial quota will be restricted to kube-system namespace which can later be expanded to other namespaces(https://github.com/kubernetes/kubernetes/pull/76310/files#diff-4e1245d9faac6e47c01c1c068f4de517R199). Let me know, if I have missed something.

Which issue(s) this PR fixes:

Fixes #76308

Special notes for your reviewer:
/cc @bsalamat @vikaschoudhary16

Does this PR introduce a user-facing change?:

Critical pods can now be created in namespaces other than kube-system. To limit critical pods to the kube-system namespace, cluster admins should create an admission configuration file limiting critical pods by default, and a matching quota object in the `kube-system` namespace permitting critical pods in that namespace. See https://kubernetes.io/docs/concepts/policy/resource-quotas/#limit-priority-class-consumption-by-default for details.

Ref: document

@k8s-ci-robot k8s-ci-robot added do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. size/S Denotes a PR that changes 10-29 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed do-not-merge/release-note-label-needed Indicates that a PR should not merge because it's missing one of the release note labels. labels Apr 9, 2019
@ravisantoshgudimetla
Copy link
Copy Markdown
Contributor Author

ravisantoshgudimetla commented Apr 9, 2019

/sig scheduling
/kind bug

@k8s-ci-robot k8s-ci-robot added sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. kind/bug Categorizes issue or PR as related to a bug. and removed needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-kind Indicates a PR lacks a `kind/foo` label and requires one. labels Apr 9, 2019
vikaschoudhary16
vikaschoudhary16 reviewed Apr 9, 2019
View reviewed changes
plugin/pkg/admission/priority/admission.go Outdated
Copy link
Copy Markdown
Contributor

@vikaschoudhary16 vikaschoudhary16 Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we would desire that quota is required mandatorily to be present in a namespace for allowing SystemPriorityClasses to be created in that namespace. To enforce this, AdmissionConfiguration object with matching scopes for SystemPriorityClasses must be configured.

Then to allow creation, quota can be created in desired namespaces.

Copy link
Copy Markdown
Contributor Author

@ravisantoshgudimetla ravisantoshgudimetla Apr 9, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thnx, @vikaschoudhary16, yes that's what I am planning to do. Will create an admission config obj with SystemPriorityClasses in kube-system namespace at the bootstrap phase so that we are backwards compatible.

/cc @bsalamat

Copy link
Copy Markdown
Member

@bsalamat bsalamat Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The problem with this approach is that users will need to add extra configuration to their clusters when they enable ResourceQuotaScopeSelectors. If they don't, this change may create a security gap in their clusters.
Whatever solution we propose here should not need extra configuration by users.

bsalamat
bsalamat suggested changes Apr 9, 2019
View reviewed changes
Copy link
Copy Markdown
Member

@bsalamat bsalamat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This restriction was an informed decision that we made to prevent a potential vulnerability where a user may create a lot of high priority pods that would preempt system critical components. I still believe that other pods in the system shouldn't be run with those predefined priority classes reserved for system critical components. Why pods in other namespaces need to run with these special priority classes instead of running with a user defined priority that is higher than any other priority, but lower than the system critical priorities?

bsalamat
bsalamat reviewed Apr 9, 2019
View reviewed changes
plugin/pkg/admission/priority/admission.go Outdated
Copy link
Copy Markdown
Member

@bsalamat bsalamat Apr 9, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Our recent benchmarks show that checking feature gates is relatively slow. We should avoid checking feature gates here. Instead we should check the feature gates at the time of plugin creation and store it in a member variable and use that variable instead.

This was referenced Apr 17, 2019
Closed
Closed
@trdavi2
Copy link
Copy Markdown

trdavi2 commented Apr 24, 2019

Is there any estimate as to when this will be merged and released? My team and I could really use it

@k8s-ci-robot k8s-ci-robot added size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. and removed size/S Denotes a PR that changes 10-29 lines, ignoring generated files. labels May 13, 2019
@k8s-ci-robot k8s-ci-robot added size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. and removed size/XL Denotes a PR that changes 500-999 lines, ignoring generated files. labels May 13, 2019
@k8s-ci-robot k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. labels May 13, 2019
@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. and removed release-note-action-required Denotes a PR that introduces potentially breaking changes that require user action. labels Nov 12, 2019
@liggitt liggitt added the priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. label Nov 12, 2019
@k8s-ci-robot k8s-ci-robot removed the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Nov 12, 2019
@liggitt
Copy link
Copy Markdown
Member

liggitt commented Nov 12, 2019

/hold cancel
/lgtm
/approve

@k8s-ci-robot k8s-ci-robot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Nov 12, 2019
@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 12, 2019
@liggitt
Copy link
Copy Markdown
Member

liggitt commented Nov 12, 2019

matches changes discussed with @deads2k, @ravisantoshgudimetla, and @ahg-g

@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Nov 12, 2019

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, ravisantoshgudimetla

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

The pull request process is described here

Details Needs approval from an approver in each of these files:

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

@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Nov 12, 2019
@k8s-ci-robot
Copy link
Copy Markdown
Contributor

k8s-ci-robot commented Nov 12, 2019 •
edited
Loading

@ravisantoshgudimetla: The following test failed, say /retest to rerun them all:

Test name Commit Details Rerun command
pull-kubernetes-local-e2e 9dd610719afb182d32456ab5b9526ace3dceebde link /test pull-kubernetes-local-e2e

Full PR test history. Your PR dashboard. Please help us cut down on flakes by linking to an open issue when you hit one in your PR.

Details

Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes/test-infra repository. I understand the commands that are listed here.

@ravisantoshgudimetla
Copy link
Copy Markdown
Contributor Author

ravisantoshgudimetla commented Nov 13, 2019

/retest

@k8s-ci-robot k8s-ci-robot merged commit bb55aa7 into kubernetes:master Nov 13, 2019
@liggitt liggitt mentioned this pull request Nov 28, 2019
Closed
@AkihiroSuda AkihiroSuda mentioned this pull request Dec 11, 2019
Open
@pires
Copy link
Copy Markdown
Contributor

pires commented Feb 5, 2020

Can someone please clarify why this is labeled as a bug, has been fixed but not backported? Right now, every version where the feature-gate is enabled actually didn't get this relaxation. So what is the point of validating the feature-gate is enabled when it is already GA in 1.17?

@liggitt
Copy link
Copy Markdown
Member

liggitt commented Feb 5, 2020

Can someone please clarify why this is labeled as a bug, has been fixed but not backported?

This was incorrectly labeled as a bug. It is the completion of the feature which includes support for default quota limiting critical pods.

Right now, every version where the feature-gate is enabled actually didn't get this relaxation. So what is the point of validating the feature-gate is enabled when it is already GA in 1.17?

In 1.17, the feature is enabled unconditionally, and the supporting quota configuration graduated to v1 status.

@pires
Copy link
Copy Markdown
Contributor

pires commented Feb 6, 2020

Thanks.

@iverberk iverberk mentioned this pull request Mar 2, 2020
Merged
@aanm aanm mentioned this pull request Mar 6, 2020
Closed
@justinsb justinsb mentioned this pull request Mar 11, 2020
Merged
@michi-covalent michi-covalent mentioned this pull request Mar 12, 2020
Merged
@kishorejonnala kishorejonnala mentioned this pull request May 8, 2020
Merged
@runningman84 runningman84 mentioned this pull request Jul 3, 2020
Closed
@liggitt liggitt mentioned this pull request Jul 15, 2020
Merged
@sozercan sozercan mentioned this pull request Jul 7, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@liggitt liggitt liggitt left review comments

@deads2k deads2k deads2k left review comments

+4 more reviewers

@derekwaynecarr derekwaynecarr derekwaynecarr left review comments

@vikaschoudhary16 vikaschoudhary16 vikaschoudhary16 left review comments

@bsalamat bsalamat bsalamat requested changes

@ahg-g ahg-g ahg-g left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@liggitt liggitt

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/feature Categorizes issue or PR as related to a new feature. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/scheduling Categorizes an issue or PR as relevant to SIG Scheduling. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/M Denotes a PR that changes 30-99 lines, ignoring generated files.

Projects

None yet

Milestone

v1.17

Development

Successfully merging this pull request may close these issues.

Quota for priorityClasses not allowing critical pods to be created in namespaces other than "kube-*"