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

Add support for user namespaces phase 1 (KEP 127) #111090

Merged
merged 12 commits into from Aug 3, 2022
Merged

Add support for user namespaces phase 1 (KEP 127) #111090

merged 12 commits into from Aug 3, 2022

Conversation

rata
Copy link
Member

@rata rata commented Jul 12, 2022 •
edited

What type of PR is this?

/kind feature

What this PR does / why we need it:

Implements KEP 127 phase I: Support for user namespaces.

Which issue(s) this PR fixes:

Fixes #

Special notes for your reviewer:

Please note we added several unit tests but integration and e2e tests need support from the container runtime to make meaningful tests. One option is to make this feature alpha longer, until a container runtime with support is released and we can include those versions on the CI. We will investigate if other options are desirable. Any feedback on that is super welcome :)

Does this PR introduce a user-facing change?

Added alpha support for user namespaces in pods phase 1 (KEP 127, feature gate: UserNamespacesStatelessPodsSupport)

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

-[KEP]: https://github.com/kubernetes/enhancements/tree/master/keps/sig-node/127-user-namespaces#design-details

@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/feature Categorizes issue or PR as related to a new feature. size/XXL Denotes a PR that changes 1000+ 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. labels Jul 12, 2022
@rata
Copy link
Member Author

rata commented Jul 12, 2022

cc @giuseppe

@k8s-ci-robot k8s-ci-robot added the needs-priority Indicates a PR lacks a `priority/foo` label and requires one. label Jul 12, 2022
@giuseppe
Copy link
Member

/sig node

@k8s-ci-robot k8s-ci-robot added sig/node Categorizes an issue or PR as relevant to SIG Node. area/code-generation area/kubelet area/test kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API and removed do-not-merge/needs-sig Indicates an issue or PR lacks a `sig/foo` label and requires one. labels Jul 12, 2022
@k8s-ci-robot k8s-ci-robot added sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. 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/storage Categorizes an issue or PR as relevant to SIG Storage. sig/testing Categorizes an issue or PR as relevant to SIG Testing. labels Jul 12, 2022
@rata
Copy link
Member Author

rata commented Jul 12, 2022

Hmm, it seems the CI is failing to chown: atomic_writer_test.go:431: dotfiles fsUser: unexpected error writing payload: chown /tmp/atomic-write2144783139/..2022_07_12_15_50_37.3835937769/bar: operation not permitted. Locally I see the same error if I run go test . in that folder, but all tests pass if I run sudo go test .

I'll remove the patch adding tests to atomic_writer, to see if that helps to make the CI happy, and then open a different PR with them and see how to fix it there

@rata rata force-pushed the rata/userns-support-2022 branch from 6d1d848 to 9130dc1 Compare July 12, 2022 16:25
@rata
Copy link
Member Author

rata commented Jul 12, 2022 •
edited

Hmm, no, that didn't make the CI happier. I still see:

...
E0712 15:50:37.842614   70419 atomic_writer.go:407] -test-: unable to change file /tmp/atomic-write2760274370/..2022_07_12_15_50_37.3043849901/foo with owner 100: chown /tmp/atomic-write2760274370/..2022_07_12_15_50_37.3043849901/foo: operation not permitted
...

which shouldn't happen, the chown should work. Is it possible that the CI is broken?

I'll push again the atomir_writer tests that I added, then.

(there are other things we need to fix in the unit tests, we will work on those. But that specific error seems like a problem in the CI)

@k8s-triage-robot
Copy link

This PR may require API review.

If so, when the changes are ready, complete the pre-review checklist and request an API review.

Status of requested reviews is tracked in the API Review project.

@leilajal
Copy link
Contributor

/remove-sig api-machinery

@k8s-ci-robot k8s-ci-robot removed the sig/api-machinery Categorizes an issue or PR as relevant to SIG API Machinery. label Jul 12, 2022
@sftim
Copy link
Contributor

sftim commented Jul 12, 2022

For this PR title, I suggest making it clear in the PR title and changelog that these are Linux namespaces or node-level namespaces. Kubernetes' API also has namespaces and some readers might not know about the other kind.

To help with localization, write “phase 1” not “phase I”.

@liggitt
Copy link
Member

liggitt commented Aug 3, 2022

@liggitt @mrunalp @derekwaynecarr @gnufied @thockin @msau42 Can you PTAL? I've updated the PR with all the requested changes. All concerns should be addressed now.

Thanks, the changes resolved my comments. I'll defer API approval to @thockin who had more context on the validation resolution.

@liggitt liggitt moved this from Changes requested to Assigned in API Reviews Aug 3, 2022
@liggitt liggitt removed their assignment Aug 3, 2022
thockin
thockin reviewed Aug 3, 2022
View reviewed changes
Copy link
Member

@thockin thockin left a comment

Choose a reason for hiding this comment

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

API and validation LGTM

@thockin
Copy link
Member

thockin commented Aug 3, 2022

/approve, but I think you need a final LGTM from storage?

@rata
Copy link
Member Author

rata commented Aug 3, 2022

/test pull-kubernetes-unit

It seems completely unrelated to this PR

@rata
Copy link
Member Author

rata commented Aug 3, 2022

@thockin thanks! I will ping @gnufied

@mrunalp
Copy link
Contributor

mrunalp commented Aug 3, 2022

/approve
for node. Thanks!

@liggitt liggitt moved this from Assigned to API review completed, 1.25 in API Reviews Aug 3, 2022
@thockin
Copy link
Member

thockin commented Aug 3, 2022

/approve

end-of-line remarks are not supported, sigh

@gnufied
Copy link
Member

gnufied commented Aug 3, 2022

/lgtm

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

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: mrunalp, rata, thockin

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 Aug 3, 2022
@rata rata moved this from Needs Reviewer to Done in SIG Node PR Triage Aug 3, 2022
@cici37
Copy link
Contributor

cici37 commented Aug 3, 2022

Hello 👋, 1.25 Release Lead here.

The exception request is approved and your updated deadline to make any changes to your PR is 10:00 AM PST Monday 8th August 2022. Thank you!

/milestone v1.25

@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Aug 3, 2022
@k8s-ci-robot k8s-ci-robot merged commit 4b6134b into kubernetes:master Aug 3, 2022
SIG Node CI/Test Board automation moved this from PRs Waiting on Author to Done Aug 3, 2022
@rata rata deleted the rata/userns-support-2022 branch August 3, 2022 20:06
@rata rata mentioned this pull request Aug 4, 2022
Merged
rata added a commit to kinvolk/kubernetes that referenced this pull request Aug 4, 2022
@rata
volume: FeatureGate access to GetHostIDsForPod()
4267f2e 
After the userns PR got merged:
	kubernetes#111090

gnufied decided it might be safer if we feature gate this part of the
code, due to the kubelet volume host type assertion.

That is a great catch and this patch just moves the code inside the
feature gate if.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
@imperialguy imperialguy mentioned this pull request Aug 11, 2022
Open
@jpmcb jpmcb mentioned this pull request Oct 3, 2022
Closed
@nalind nalind mentioned this pull request Oct 11, 2022
Closed
@rata rata mentioned this pull request Jun 29, 2023
Merged
@uablrek uablrek mentioned this pull request Oct 25, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gnufied gnufied gnufied left review comments

@mrunalp mrunalp mrunalp left review comments

@giuseppe giuseppe giuseppe left review comments

@jpbetz jpbetz jpbetz requested changes

@liggitt liggitt liggitt left review comments

@endocrimes endocrimes endocrimes left review comments

@ruiwen-zhao ruiwen-zhao ruiwen-zhao left review comments

@thockin thockin thockin left review comments

@deads2k deads2k deads2k left review comments

@mikebrow mikebrow mikebrow left review comments

@sftim sftim sftim left review comments

@msau42 msau42 msau42 left review comments

@tallclair tallclair tallclair left review comments

@akankshakumari393 akankshakumari393 akankshakumari393 left review comments

@caesarxuchao caesarxuchao Awaiting requested review from caesarxuchao

@chrishenzie chrishenzie Awaiting requested review from chrishenzie

@derekwaynecarr derekwaynecarr Awaiting requested review from derekwaynecarr

Assignees

@gnufied gnufied

@ruiwen-zhao ruiwen-zhao

@thockin thockin

@msau42 msau42

Labels
api-review Categorizes an issue or PR as actively needing an API review. approved Indicates a PR has been approved by an approver from all required OWNERS files. area/code-generation area/e2e-test-framework Issues or PRs related to refactoring the kubernetes e2e test framework area/kubelet area/test cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/api-change Categorizes issue or PR as related to adding, removing, or otherwise changing an API 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. 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. 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/network Categorizes an issue or PR as relevant to SIG Network. 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/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
API Reviews
Status: API review completed, 1.25
SIG Node CI/Test Board
Done
SIG Node CI/Test Board
Archived in project
SIG Node PR Triage
Done
Milestone
v1.25
Development

Successfully merging this pull request may close these issues.

None yet