-
Notifications
You must be signed in to change notification settings - Fork 40k
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
Conversation
@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 The 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. |
@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 :) |
@saschagrunert @mikebrow ptal. |
/* | ||
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. | ||
*/ |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Awesome, thanks!
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 :)
There was a problem hiding this comment.
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
171e570
to
b6ebbe7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
/assign @sjenning |
I assigned Seth as per the bot comment, but maybe @mrunalp wants to take a look? |
ping. What is missing to move forward with this PR? |
@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? |
fc6fa5f
to
20d442c
Compare
There was a problem hiding this 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 :)
86034cb
to
f3dad38
Compare
/retest The failure seems unrelated to this PR |
There was a problem hiding this 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
// 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>
thanks for the review. I've amended the changes you've requested and pushed a new version |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
/lgtm |
[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 |
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?
Additional documentation e.g., KEPs (Kubernetes Enhancement Proposals), usage docs, etc.: