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.

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

KEP-127: Support userns in stateless pods with idmap mounts #3811

Merged
merged 16 commits into from
Feb 7, 2023

Conversation

rata
Copy link
Member

@rata rata commented Feb 1, 2023

  • One-line PR description: Change the implementation to rely on idmap mounts. This simplifies the implementation in kubernetes and container runtimes and removes several limitations we hit before
  • Other comments: We did a prototype implementation on container runtimes (containerd, runc, CRIO, crun) with @giuseppe and we didn't find anything unexpected.

The change to make this KEP about only stateless pods was discussed last year in sig-node and when we asked for an exception for the 1.25 release (https://groups.google.com/g/kubernetes-sig-release/c/ASWlhA-tIxE/m/mMWeXx_kAwAJ). The first commits of this PR just update the KEP to be about stateless pods, as was agreed, the rest of the commits re-work the KEP to rely on idmap mounts.

cc @thockin @mrunalp @derekwaynecarr @SergeyKanzhelev

@k8s-ci-robot k8s-ci-robot added the kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory label Feb 1, 2023
@k8s-ci-robot k8s-ci-robot added cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files. labels Feb 1, 2023
@rata rata mentioned this pull request Feb 1, 2023
26 tasks
@remram44
Copy link

remram44 commented Feb 1, 2023

If seems that you have removed the description of the phases but you still reference "phase 2" and "phase 3"?

@rata
Copy link
Member Author

rata commented Feb 1, 2023

@remram44 Yes, phase 2 and 3 were about stateful workloads. As this KEP is now about stateless pods, those sections don't fit in this KEP anymore. This was decided last year and a reason on why an exception was granted for this KEP last year (https://groups.google.com/g/kubernetes-sig-release/c/ASWlhA-tIxE/m/mMWeXx_kAwAJ).

Added this as a clarification on the PR description. Thanks!

@remram44
Copy link

remram44 commented Feb 2, 2023

@rata What I mean is that you removed all definitions of the phases but you kept references to those phases like "See phase 3 for limitations" (line 129) and "Phase 2 (support for pods with volumes) is out of scope" (line 1062). I think if you are going to refer to those phases they should be described, in this document or in another.

@rata
Copy link
Member Author

rata commented Feb 2, 2023

@remram44 oh, thanks. Removed those references to phases too! :)

@thockin @mrunalp fixed my brain fart in the example. PTAL

@rata
Copy link
Member Author

rata commented Feb 3, 2023

@mrunalp @thockin I think all comments are addressed, PTAL. Thanks!

@marosset
Copy link
Contributor

marosset commented Feb 3, 2023

@rata please update the milestones in https://github.com/kubernetes/enhancements/blob/master/keps/sig-node/127-user-namespaces/kep.yaml as part of the PR too.

@rata
Copy link
Member Author

rata commented Feb 3, 2023

@marosset thanks, will do it. How is it, though? Should I update latest-milestone only, and keep the alpha: "v1.25" as that is when it was first merged? That is my understanding from the template, so will do that. But if you can confirm, it will be great :)

@marosset
Copy link
Contributor

marosset commented Feb 3, 2023

@marosset thanks, will do it. How is it, though? Should I update latest-milestone only, and keep the alpha: "v1.25" as that is when it was first merged? That is my understanding from the template, so will do that. But if you can confirm, it will be great :)

Did any code merge for this in v1.25?
If the enhancement is targeting alpha in v1.27 (which it looks like it is but I didn't look too close) then update the alpha milestone to v1.27 too please

keps/sig-node/127-user-namespaces/README.md Outdated Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Outdated Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Outdated Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Outdated Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Outdated Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Show resolved Hide resolved
keps/sig-node/127-user-namespaces/README.md Show resolved Hide resolved
@rata
Copy link
Member Author

rata commented Feb 6, 2023

@marosset thanks, will do it. How is it, though? Should I update latest-milestone only, and keep the alpha: "v1.25" as that is when it was first merged? That is my understanding from the template, so will do that. But if you can confirm, it will be great :)

Did any code merge for this in v1.25?

Yes, we did merge support for userns in 1.25 (without idmap mounts)

If the enhancement is targeting alpha in v1.27 (which it looks like it is but I didn't look too close) then update the alpha milestone to v1.27 too please

It targets alpha this re-work. So, I guess I should leave it as it is now, then :)

We worked in the implementation and we add one function call here,
definitely not a core package we are changing.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
We have implemented most checks as API validations and the pod limit as
a runtime. No need to keep this as unresolved.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Volumes are out of scope now, so most of these concerns are solved.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Now there will be only one phase, and the regular alpha/beta/GA
migration.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
We will add a new way to deal with stateless volumes using idmap mounts.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
While we are there, we move this section to the alternatives section and
link appropriately.

Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
Signed-off-by: Rodrigo Campos <rodrigoca@microsoft.com>
@mrunalp
Copy link
Contributor

mrunalp commented Feb 7, 2023

/lgtm
/approve

@k8s-ci-robot k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Feb 7, 2023
@rata
Copy link
Member Author

rata commented Feb 7, 2023

@dchen1107 This is the userns PR we talked about in the sig-node meeting. PTAL, thanks! :)

@dchen1107
Copy link
Member

/approve
/label leads-opted-in

@k8s-ci-robot
Copy link
Contributor

@dchen1107: The label(s) /label leads-opted-in cannot be applied. These labels are supported: api-review, tide/merge-method-merge, tide/merge-method-rebase, tide/merge-method-squash, team/katacoda, refactor, lead-opted-in, tracked/no, tracked/out-of-tree, tracked/yes. Is this label configured under labels -> additional_labels or labels -> restricted_labels in plugin.yaml?

In response to this:

/approve
/label leads-opted-in

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.

@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

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

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

@dchen1107 dchen1107 added this to the v1.27 milestone Feb 7, 2023
@k8s-ci-robot k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Feb 7, 2023
@k8s-ci-robot k8s-ci-robot merged commit 8c132a3 into kubernetes:master Feb 7, 2023
@wojtek-t
Copy link
Member

wojtek-t commented Feb 8, 2023

FWIW - this LGTM for a second alpha from PRR perspective.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. kind/kep Categorizes KEP tracking issues and PRs modifying the KEP directory lgtm "Looks good to me", indicates that a PR is ready to be merged. sig/node Categorizes an issue or PR as relevant to SIG Node. size/L Denotes a PR that changes 100-499 lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

8 participants