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

kubelet: add CRI definitions for user namespaces #110535

Merged
merged 1 commit into from
Jun 24, 2022

Conversation

giuseppe
Copy link
Member

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

What type of PR is this?

/kind feature

What this PR does / why we need it:

Add the definitions for user namespaces to CRI

Which issue(s) this PR fixes:

Special notes for your reviewer:

Does this PR introduce a user-facing change?

None

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

@k8s-ci-robot k8s-ci-robot added release-note-none Denotes a PR that doesn't merit a release note. 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 Jun 13, 2022
@k8s-ci-robot
Copy link
Contributor

@giuseppe: This issue is currently awaiting triage.

If a SIG or subproject determines this is a relevant issue, they will accept it by applying the triage/accepted label and provide further guidance.

The triage/accepted label can be added by org members by writing /triage accepted in a comment.

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 k8s-ci-robot added needs-priority Indicates a PR lacks a `priority/foo` label and requires one. area/kubelet 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 Jun 13, 2022
@rata
Copy link
Member

rata commented Jun 14, 2022

@giuseppe the CI failures seem legit. It seems we are testing the v1 api doesn't change compared to the v1alpha2, which is of course not true now.

@giuseppe
Copy link
Member Author

@giuseppe the CI failures seem legit. It seems we are testing the v1 api doesn't change compared to the v1alpha2, which is of course not true now.

fixed :)

@mrunalp
Copy link
Contributor

mrunalp commented Jun 16, 2022

@saschagrunert @mikebrow ptal.

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto Outdated Show resolved Hide resolved
staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto Outdated Show resolved Hide resolved
Comment on lines 1 to 15
/*
Copyright 2021 The Kubernetes Authors.

Licensed under the Apache License, Version 2.0 (the "License");
you may not use this file except in compliance with the License.
You may obtain a copy of the License at

http://www.apache.org/licenses/LICENSE-2.0

Unless required by applicable law or agreed to in writing, software
distributed under the License is distributed on an "AS IS" BASIS,
WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
See the License for the specific language governing permissions and
limitations under the License.
*/
Copy link
Member

@saschagrunert saschagrunert Jun 16, 2022

Choose a reason for hiding this comment

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

@mrunalp @mikebrow I assume the plan was to remove v1alpha2 support before diverging both CRI versions, right? If so, have we already agreed on a version when to remove v1alpha2? I guess bumping containerd in CI was one requirement.

Created a WIP PR in #110618

If we cannot drop v1alpha2 I suggest we keep both versions in sync for now.

Copy link
Member Author

Choose a reason for hiding this comment

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

I've just pushed a version with the changes to v1alpha2 as well

Copy link
Member

Choose a reason for hiding this comment

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

@saschagrunert taking into account what you said in #110618 (comment), it might be then a good interim solution to update both (v1 and alpha) for now as @giuseppe already did?

The alpha interface will be deprecated before this feature gate is enabled by default, if we remove alpha in October/November, so it is something mostly no one will notice. Does it sound ok? Or too hacky?

Copy link
Member

Choose a reason for hiding this comment

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

it might be then a good interim solution to update both (v1 and alpha) for now as @giuseppe already did?

Yes, I think that's the easiest way to move forward 👍

Copy link
Member

Choose a reason for hiding this comment

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

Awesome, thanks!

Copy link
Member

Choose a reason for hiding this comment

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

Speaking for the container runtime, I don't think we should force old releases of container runtimes to add new features. For example, containerd has a rule to not add new features to service branches. I believe kubernetes has the same type of rule for it's APIs. If the change is "non-breaking" that may be an ok change such as how we did the optional changes last release. I expected to see no more changes to v1alpha2, and after n-2 releases of k8s having added v1, I expected v1alpha2 to drop from kubelet and the container runtimes.

Copy link
Member

Choose a reason for hiding this comment

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

@mikebrow I think we weren't clear, we weren't trying to force old container runtimes releases to implement this, rather this was a way out to not break the current test that assert they v1 and alpha are equal, until we can remove alpha in a few releases. The expectation is that only v1 runtimes will implement the feature (the userns feature gate will still be alpha when the CRI alpha interface is dropped, so probably no one will even notice and no changes in the runtimes that only implement the alpha CRI is needed).

We are fine either way: making the interfaces differ now or adding the change for both, but we need a way forward to merge this changes :)

Copy link
Member

Choose a reason for hiding this comment

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

nod.. container runtimes will just have to clarify support of user ns option in "optional" fields .. my concern, per chat, is more around new apis than optional extensions

@giuseppe giuseppe force-pushed the add-userns-CRI branch 2 times, most recently from 171e570 to b6ebbe7 Compare June 16, 2022 09:17
Copy link
Member

@saschagrunert saschagrunert 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 Jun 16, 2022
@rata
Copy link
Member

rata commented Jun 16, 2022

/assign @sjenning

@rata
Copy link
Member

rata commented Jun 16, 2022

I assigned Seth as per the bot comment, but maybe @mrunalp wants to take a look?

@k8s-ci-robot k8s-ci-robot removed the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jun 16, 2022
@giuseppe
Copy link
Member Author

ping. What is missing to move forward with this PR?

@rata
Copy link
Member

rata commented Jun 22, 2022

@mikebrow @mrunalp @saschagrunert friendly ping? (trying to tag you, just in case you missed Giuseppe's ping)

As I said in this comment, we (@giuseppe and I) are fine either way: making the alpha and v1 CRI interfaces differ now or adding the change for both. But we need a way forward.

Is there anything we can do to drive alignment between you all on the way forward?

@giuseppe giuseppe force-pushed the add-userns-CRI branch 2 times, most recently from fc6fa5f to 20d442c Compare June 22, 2022 16:37
Copy link
Member

@rata rata left a comment

Choose a reason for hiding this comment

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

@giuseppe just one small typo, otherwise LGTM :)

staging/src/k8s.io/cri-api/pkg/apis/runtime/v1/api.proto Outdated Show resolved Hide resolved
@giuseppe giuseppe force-pushed the add-userns-CRI branch 2 times, most recently from 86034cb to f3dad38 Compare June 23, 2022 10:08
@rata
Copy link
Member

rata commented Jun 23, 2022

/retest

The failure seems unrelated to this PR

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

some nits.. (ditto for the v1alpha2 proto) other than nits this is looking good to me..

@@ -207,6 +207,17 @@ message Mount {
MountPropagation propagation = 5;
}

// IDMapping describes host to container ID mappings for a pod sandbox.
// pod.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// pod.

@@ -232,6 +243,19 @@ enum NamespaceMode {
TARGET = 3;
}

// UserNamespace describes the intended user namespace configuration for a sandbox.
message UserNamespace {
// User namespace for this sandbox.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// User namespace for this sandbox.
// Mode is the NamespaceMode for this UserNamespace.

// UserNamespace describes the intended user namespace configuration for a sandbox.
message UserNamespace {
// User namespace for this sandbox.
// Note: It currently supports only POD and NODE.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// Note: It currently supports only POD and NODE.
// Note: NamespaceMode for UserNamespace currently supports only POD and NODE, not CONTAINER OR TARGET.

// IDMapping describes host to container ID mappings for a pod sandbox.
// pod.
message IDMapping {
// host_id is the id on the host.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// host_id is the id on the host.
// HostId is the id on the host.

message IDMapping {
// host_id is the id on the host.
uint32 host_id = 1;
// container_id is the id in the container.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// container_id is the id in the container.
// ContainerId is the id in the container.

uint32 host_id = 1;
// container_id is the id in the container.
uint32 container_id = 2;
// length is the size of the range to map.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// length is the size of the range to map.
// Length is the size of the range to map.

@@ -232,6 +243,19 @@ enum NamespaceMode {
TARGET = 3;
}

// UserNamespace describes the intended user namespace configuration for a sandbox.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// UserNamespace describes the intended user namespace configuration for a sandbox.
// UserNamespace describes the intended user namespace configuration for a pod sandbox.

// Note: It currently supports only POD and NODE.
NamespaceMode mode = 1;

// uids specifies the UID mappings for the user namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// uids specifies the UID mappings for the user namespace.
// Uids specifies the UID mappings for the user namespace.

// uids specifies the UID mappings for the user namespace.
repeated IDMapping uids = 2;

// gids specifies the GID mappings for the user namespace.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// gids specifies the GID mappings for the user namespace.
// Gids specifies the GID mappings for the user namespace.

@@ -251,6 +275,12 @@ message NamespaceOption {
// previously created in the same pod. It is not possible to specify different targets
// for each namespace.
string target_id = 4;
// User namespace for this sandbox.
Copy link
Member

@mikebrow mikebrow Jun 23, 2022

Choose a reason for hiding this comment

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

Suggested change
// User namespace for this sandbox.
// UsernsOptions for this pod sandbox.

@@ -251,6 +275,12 @@ message NamespaceOption {
// previously created in the same pod. It is not possible to specify different targets
// for each namespace.
string target_id = 4;
// User namespace for this sandbox.
// The Kubelet picks the user namespace configuration to use for the sandbox. The mappings
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
// The Kubelet picks the user namespace configuration to use for the sandbox. The mappings
// The Kubelet picks the user namespace configuration to use for the pod sandbox. The mappings

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
Copy link
Member Author

some nits.. (ditto for the v1alpha2 proto) other than nits this is looking good to me..

thanks for the review. I've amended the changes you've requested and pushed a new version

Copy link
Member

@mikebrow mikebrow left a comment

Choose a reason for hiding this comment

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

LGTM

@mrunalp
Copy link
Contributor

mrunalp commented Jun 24, 2022

/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 Jun 24, 2022
@k8s-ci-robot
Copy link
Contributor

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, mikebrow, mrunalp, saschagrunert

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 Jun 24, 2022
@k8s-ci-robot k8s-ci-robot merged commit 6219eed into kubernetes:master Jun 24, 2022
@k8s-ci-robot k8s-ci-robot added this to the v1.25 milestone Jun 24, 2022
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. 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. 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. release-note-none Denotes a PR that doesn't merit a release note. sig/node Categorizes an issue or PR as relevant to SIG Node. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.
Projects
Development

Successfully merging this pull request may close these issues.

7 participants