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

Upgrade preparation to verify sysctl values containing forward slashes by regex #102393

Merged
merged 1 commit into from Nov 10, 2021
Merged

Upgrade preparation to verify sysctl values containing forward slashes by regex #102393

merged 1 commit into from Nov 10, 2021

Conversation

mengjiao-liu
Copy link
Member

@mengjiao-liu mengjiao-liu commented May 28, 2021 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

The Regex that validates sysctl values in objects doesn't allow names that contain forward slashes. Sysctl values on network subinterfaces contain these forward slashes.

I looked up Linux's description of the sysctl variable:

       variable
              The name of a key to read from.  An example is
              kernel.ostype.  The '/' separator is also accepted in
              place of a '.'.

Therefore, the dots and slashes serve the same purpose, and the sysctl variable should also support slashes as separator in Kubernetes.

As #102393 (comment), the PR does the following:

  • use strict validation for Create requests, and for Update requests where the existing object's sysctl does not contain a slash
  • use relaxed validation for Update requests where the existing object's sysctl contains a slash

Which issue(s) this PR fixes:

Ref #102373

Special notes for your reviewer:

Does this PR introduce a user-facing change?

Pod SecurityContext sysctls name parameter for update requests where the existing object's sysctl contains slashes and kubelet sysctl whitelist support contains slashes.

Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.:

@k8s-ci-robot k8s-ci-robot added release-note Denotes a PR that will be considered when it comes time to generate release notes. kind/bug Categorizes issue or PR as related to a bug. size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. 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. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels May 28, 2021
@mengjiao-liu
Copy link
Member Author

/triage accepted

@k8s-ci-robot k8s-ci-robot added triage/accepted Indicates an issue or PR is ready to be actively worked on. and removed needs-triage Indicates an issue or PR lacks a `triage/foo` label and requires one. labels May 28, 2021
@mengjiao-liu
Copy link
Member Author

/assign @thockin

@mengjiao-liu
Copy link
Member Author

/cc @liggitt

@k8s-ci-robot k8s-ci-robot requested a review from liggitt June 3, 2021 17:45
@mengjiao-liu mengjiao-liu changed the title Regex for validating Sysctl values allows names to contain forward slashes [WIP]Regex for validating Sysctl values allows names to contain forward slashes Jun 4, 2021
@mengjiao-liu mengjiao-liu changed the title [WIP]Regex for validating Sysctl values allows names to contain forward slashes [WIP] Regex for validating Sysctl values allows names to contain forward slashes Jun 4, 2021
@k8s-ci-robot k8s-ci-robot added the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Jun 4, 2021
@enj enj added this to Needs Triage in SIG Auth Old Jun 7, 2021
@k8s-ci-robot k8s-ci-robot added size/M Denotes a PR that changes 30-99 lines, ignoring generated files. and removed size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. labels Jun 16, 2021
@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. and removed release-note Denotes a PR that will be considered when it comes time to generate release notes. labels Jun 16, 2021
@kolyshkin kolyshkin mentioned this pull request Oct 29, 2021
Merged
@mengjiao-liu
Copy link
Member Author

mengjiao-liu commented Nov 1, 2021 •
edited

We could convert before setting in runc

  • I converted sysctl variable before kubelet called runc
  • I have run the test that failed above on k8s environment and the test has passed.

can you review it again？@mrunalp

but I think runc should probably support what the kernel supports.

I just saw that the runc repository is working on that.
issue: opencontainers/runc#3256
PR:
opencontainers/runc#3254
opencontainers/runc#3257
We can remove the conversion function in the kubernetes code after runc releases a new version that solves this problem.
What do you think？@mrunalp @derekwaynecarr @pacoxu

@AkihiroSuda
Copy link
Member

The runc PR caused regression for values like net.ipv4.conf.eno2/100.rp_filter, so maybe we have to revert the runc PR, and just modify the OCI Runtime Spec to prohibit using / as an alternative to ..

opencontainers/runc#3254 (comment)
(Thanks to @mengjiao-liu for pointing out)

This was referenced Nov 1, 2021
Closed
Merged
@mengjiao-liu
Copy link
Member Author

mengjiao-liu commented Nov 3, 2021 •
edited

The runc PR caused regression for values like net.ipv4.conf.eno2/100.rp_filter, so maybe we have to revert the runc PR, and just modify the OCI Runtime Spec to prohibit using / as an alternative to ..

But sometimes sysctl variable will have dots and slashes at the same time,
E.g:
"net.ipv4.conf.enp3s0/200.forwarding" or
"net/ipv4/conf/enp3s0.200/forwarding"
They can point to files /proc/sys/net/ipv4/conf/enp3s0.200/forwarding

If there is no slash, only dots cannot do this.
So we can't disable the slash.

And I think the disabled slash is inconsistent with the slash supported by the Linux kernel.

ref to https://man7.org/linux/man-pages/man5/sysctl.d.5.html

@mengjiao-liu
Copy link
Member Author

/test pull-kubernetes-e2e-gce-ubuntu-containerd

@mengjiao-liu
Copy link
Member Author

Request review again. @mrunalp

@mengjiao-liu
Copy link
Member Author

Kubernetes 1.23 Code Freeze is Coming, can there be other sig-node approvers who can help to confirm again? @sig-node-approvers

@enj enj moved this from In Review to Pending other SIGs in SIG Auth Old Nov 8, 2021
@mrunalp
Copy link
Contributor

mrunalp commented Nov 10, 2021

@mengjiao-liu Thanks for fixing in runc as well.

/lgtm

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Nov 10, 2021
@mrunalp
Copy link
Contributor

mrunalp commented Nov 10, 2021

@mengjiao-liu We should update the documentation for this.

@mengjiao-liu
Copy link
Member Author

mengjiao-liu commented Nov 10, 2021 •
edited

@mengjiao-liu We should update the documentation for this.

Did you say kubernetes/website documentation?

@mrunalp
Copy link
Contributor

mrunalp commented Nov 10, 2021

/hold cancel

@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 10, 2021
@mrunalp
Copy link
Contributor

mrunalp commented Nov 10, 2021

@mengjiao-liu Yes - https://github.com/kubernetes/website/blob/3ca34b9a3be454055ba234f1d2ff7d55809b5040/content/en/docs/tasks/administer-cluster/sysctl-cluster.md

@mengjiao-liu
Copy link
Member Author

Ok, I will open a PR to update it in the kubernetes/website repository. Thanks for reminding!@mrunalp

@k8s-ci-robot k8s-ci-robot merged commit 5d60c8d into kubernetes:master Nov 10, 2021
SIG Node PR Triage automation moved this from Needs Approver to Done Nov 10, 2021
SIG Auth Old automation moved this from Pending other SIGs to Closed / Done Nov 10, 2021
@k8s-ci-robot k8s-ci-robot added this to the v1.23 milestone Nov 10, 2021
@mengjiao-liu mengjiao-liu mentioned this pull request Nov 10, 2021
Closed
@mengjiao-liu mengjiao-liu mentioned this pull request Nov 23, 2021
Merged
@mengjiao-liu mengjiao-liu mentioned this pull request Dec 6, 2021
Merged
@mengjiao-liu mengjiao-liu mentioned this pull request Aug 5, 2022
Merged
@mengjiao-liu mengjiao-liu mentioned this pull request Dec 14, 2022
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

@SergeyKanzhelev SergeyKanzhelev SergeyKanzhelev approved these changes

@justinsb justinsb Awaiting requested review from justinsb

@smarterclayton smarterclayton Awaiting requested review from smarterclayton

Assignees

@mrunalp mrunalp

@pacoxu pacoxu

@SergeyKanzhelev SergeyKanzhelev

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/bug Categorizes issue or PR as related to a bug. lgtm "Looks good to me", indicates that a PR is ready to be merged. priority/important-longterm Important over the long term, but may not be staffed and/or may need multiple releases to complete. release-note Denotes a PR that will be considered when it comes time to generate release notes. sig/apps Categorizes an issue or PR as relevant to SIG Apps. sig/auth Categorizes an issue or PR as relevant to SIG Auth. sig/node Categorizes an issue or PR as relevant to SIG Node. sig/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. triage/accepted Indicates an issue or PR is ready to be actively worked on.
Projects
API Reviews
Status: API review completed, 1.23
SIG Auth
Archived in project
SIG Node CI/Test Board
Archive-it
SIG Node CI/Test Board
Archived in project
SIG Node PR Triage
Done
SIG Auth Old
Closed / Done
Milestone
v1.23
Development

Successfully merging this pull request may close these issues.

None yet
10 participants
@mengjiao-liu @liggitt @ehashman @SergeyKanzhelev @pacoxu @derekwaynecarr @k8s-ci-robot @mrunalp @AkihiroSuda @thockin