Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Added net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl list #127489

Merged
merged 4 commits into from
Oct 12, 2024
Merged

feat: Added net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl list #127489

merged 4 commits into from
Oct 12, 2024

Conversation

pacoxu
Copy link
Member

@pacoxu pacoxu commented Sep 20, 2024 •
edited
Loading

This is based on @nikzayn's #125270. Feel free to close this PR if you want to continue to work on the original one.

Fixes #125234

This PR adds the net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl list because to handle thousands of concurrent connections used by Cassandra, DataStax recommends these settings to optimize the Linux network stack.

Allow pods to use the `net.ipv4.tcp_rmem` and `net.ipv4.tcp_wmem` sysctl by default
when the kernel version is 4.15 or higher. With the kernel 4.15 the sysctl became namespaced.
Pod Security admission allows these sysctl in v1.32+ versions of the baseline and restricted policies.

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Sep 20, 2024
@pacoxu
Copy link
Member Author

pacoxu commented Sep 20, 2024

This is based on @nikzayn's #125270. Feel free to close this PR if you want to continue to work on the original one.

/sig network
/kind feature
/cc @mauri870
/assign @thockin
/priority important-soon
for v1.32
/triage accepted

@k8s-ci-robot k8s-ci-robot added the sig/network Categorizes an issue or PR as relevant to SIG Network. label Sep 20, 2024
@k8s-ci-robot k8s-ci-robot added kind/feature Categorizes issue or PR as related to a new feature. priority/important-soon Must be staffed and worked on either currently, or very soon, ideally in time for the next release. triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed do-not-merge/needs-kind Indicates a PR lacks a `kind/foo` label and requires one. needs-priority Indicates a PR lacks a `priority/foo` label and requires one. needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels Sep 20, 2024
@pacoxu pacoxu mentioned this pull request Sep 20, 2024
Merged
adrianmoisey
adrianmoisey reviewed Sep 21, 2024
View reviewed changes
staging/src/k8s.io/pod-security-admission/policy/check_sysctls.go
Copy link
Member

Choose a reason for hiding this comment

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

I think the CheckSysctls function needs to be updated with sysctlsV1Dot32 in this file

Copy link
Member Author

Choose a reason for hiding this comment

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

Good Catch.

Added.

@nikzayn @pacoxu
feat: versioning --update kubernetes#125234
97108d5 
Signed-off-by: nikzayn <nikhilvaidyar1997@gmail.com>
@adrianmoisey
Copy link
Member

This LGTM to me, but I'm not confident enough in this part of the code to give a /lgtm. Will leave that for someone else more experienced

@thockin
Copy link
Member

thockin commented Sep 23, 2024

It looks OK to me, but I am also not super familiar here.

@SergeyKanzhelev @bobbypage @mrunalp - can you find a delegate?

@SergeyKanzhelev
Copy link
Member

RelNote clean up suggestion:

Allow pods to use the `net.ipv4.tcp_rmem` and `net.ipv4.tcp_wmem` sysctl by default
when the kernel version is 4.15 or higher. With the kernel 4.15 the sysctl became namespaced.
Pod Security admission allows these sysctl in v1.32+ versions of the baseline and restricted policies.

SergeyKanzhelev
SergeyKanzhelev approved these changes Sep 24, 2024
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm
/approve

kubelet part looks good. Pod security admission changes looks consistent with what was there

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Sep 24, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: a3d9d09a691535f6eb5499472537da803668de60

@pacoxu
Copy link
Member Author

pacoxu commented Sep 25, 2024

RelNote clean up suggestion:

Allow pods to use the `net.ipv4.tcp_rmem` and `net.ipv4.tcp_wmem` sysctl by default
when the kernel version is 4.15 or higher. With the kernel 4.15 the sysctl became namespaced.
Pod Security admission allows these sysctl in v1.32+ versions of the baseline and restricted policies.

Release note was updated.

@pacoxu
Copy link
Member Author

pacoxu commented Sep 29, 2024

/cc @liggitt
for PSA

@dims
Copy link
Member

dims commented Sep 30, 2024

/assign @liggitt

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2024
@liggitt
Copy link
Member

liggitt commented Oct 12, 2024

/approve

for pod-security-admission bits

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: liggitt, mauri870, pacoxu, SergeyKanzhelev

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 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 Oct 12, 2024
@pacoxu
Copy link
Member Author

pacoxu commented Oct 12, 2024

Ping @SergeyKanzhelev. Would you LGTM again after my update to address Jordan's comments?

SergeyKanzhelev
SergeyKanzhelev reviewed Oct 12, 2024
View reviewed changes
Copy link
Member

@SergeyKanzhelev SergeyKanzhelev left a comment

Choose a reason for hiding this comment

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

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Oct 12, 2024
@k8s-ci-robot
Copy link
Contributor

LGTM label has been added.

Git tree hash: 25fc4128c8ab21d95b5fe3296a7fe234dd6bba69

@k8s-ci-robot k8s-ci-robot merged commit 426aa3d into kubernetes:master Oct 12, 2024
14 checks passed
@k8s-ci-robot k8s-ci-robot added this to the v1.32 milestone Oct 12, 2024
@pacoxu pacoxu mentioned this pull request Oct 12, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@adrianmoisey adrianmoisey adrianmoisey left review comments

@hashim21223445 hashim21223445 hashim21223445 approved these changes

@mauri870 mauri870 mauri870 approved these changes

@krmayankk krmayankk Awaiting requested review from krmayankk

@matthyx matthyx Awaiting requested review from matthyx

@liggitt liggitt Awaiting requested review from liggitt

@thockin thockin Awaiting requested review from thockin

Assignees

@liggitt liggitt

@thockin thockin

@SergeyKanzhelev SergeyKanzhelev

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet 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/auth Categorizes an issue or PR as relevant to SIG Auth. sig/network Categorizes an issue or PR as relevant to SIG Network. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
SIG Auth
Archived in project
SIG Node: code and documentation PRs
Status: Done
Milestone
v1.32
Development

Successfully merging this pull request may close these issues.

Add net.ipv4.tcp_rmem and net.ipv4.tcp_wmem into safe sysctl list
10 participants
@pacoxu @adrianmoisey @thockin @SergeyKanzhelev @k8s-ci-robot @dims @liggitt @mauri870 @hashim21223445 @nikzayn