Update Docker Shared PID proposal for per-pod configuration #1048

verb · 2017-09-12T16:29:00Z

We will need to support configuring shared PID namespace in docker per-pod.

k8s-ci-robot · 2017-09-12T16:29:02Z

@verb: Your pull request title starts with "WIP", so the do-not-merge/work-in-progress label will be added.

This label will ensure that your pull request will not be merged. Remove the prefix from your pull request title to trigger the removal of the label and allow for your pull request to be merged.

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.

verb · 2017-09-12T16:29:19Z

/sig node

thockin · 2017-09-15T20:58:08Z

@kubernetes/sig-node-proposals

smarterclayton · 2017-09-16T01:17:31Z

contributors/design-proposals/pod-pid-namespace.md

@@ -60,18 +95,11 @@ The ability to disable shared PID namespaces is intended as a way to roll back
 to prior behavior in the event of unforeseen problems. It won't be possible to
 configure the behavior per-pod. We believe this is acceptable because:


Should probably change this section.

yujuhong · 2017-09-22T01:38:23Z

contributors/design-proposals/pod-pid-namespace.md

+but choosing not to support isolated PID namespaces indefinitely would influence
+design choices. In this scenario we would avoid modifying the CRI or
+implementing isolated namespaces in other runtimes. Enabling the shared
+namespace in v1.Pod could happen in a new field or in an annotation.


I am having problems parsing the two options. What's the difference? Is it just the default?

verb · 2017-09-22T04:22:54Z

Please stand by for a full rewrite based on our discussion in sig node.

…

verb · 2017-09-22T14:34:08Z

@smarterclayton @yujuhong @derekwaynecarr This is now updated, internally consistent (I hope) and ready for review. It includes what we discussed previously in the SIG node meetup, which was adding a configuration option to both the Pod & CRI APIs.

thockin

About time! :)

thockin · 2017-09-15T20:56:59Z

contributors/design-proposals/pod-pid-namespace.md

-* We have not identified a concrete use case requiring isolated PID namespaces.
-* Making PID namespace configurable requires changing the CRI, which we would
-  like to avoid since there are no use cases.
+*   We have not identified a concrete use case requiring isolated PID


What about "I use systemd in my container and systemd pukes when it is not pid 1" ?

@thockin I'm not sure how this happened, but some of your comments are for an outdated revision and some are current. I suspect you started a review and I updated the doc in the middle of it. Sorry about that.

The latest version of the proposal dropped this section and lists supporting pid == 1 in the goals.

thockin · 2017-09-15T20:57:40Z

contributors/design-proposals/pod-pid-namespace.md

+per-pod configurable PID namespaces, and the choice hinges mainly on long-term
+plans for PID namespace sharing.
+
+### Option 1: Configurable Namespaces


@yujuhong What do you think here?

thockin · 2017-09-22T23:33:36Z

contributors/design-proposals/node/pod-pid-namespace.md

+
+### Kubernetes API Changes
+
+`v1.PodSecurityContext` gains a new field resembling the existing `hostPID`


Do we need this? is shared PID somewhow a security escalation that we might want to deny users?

From the discussion in kubernetes/kubernetes#48937 and kubernetes/kubernetes#1615, people want to be able to use PID namespace separation as a security feature.

derekwaynecarr · 2017-09-25T16:28:17Z

@mrunalp - please review as well.

mrunalp · 2017-09-25T17:00:19Z

contributors/design-proposals/node/pod-pid-namespace.md

+The Container Runtime Interface (CRI) will be updated to explicitly support
+shared PID namespaces. When `sharedPID` is true, runtimes MUST share a PID
+namespace for the pod or return an error. When `sharedPID` is false, runtimes
+MAY implement isolated PID namespaces. That is, omitting this option defaults to


Wondering why this is left as a MAY. Can't it be MUST?

+1 for MUST.
I'd rather see the behavior clearly defined in CRI.

MAY is the status quo of no explicit default. I left it MAY for two reasons:

Do isolated PID namespaces exist in all runtimes? What do hypervisor runtimes do? What about rkt? I'm not sure since I only use Docker.

Do any current runtimes default to shared PID? Do we care about being backwards compatible with them? If yes, then we can't retroactively define a default behavior for v1.Pod, I think.

@yujuhong What do you think about an enum for the CRI? Something like: verb/kubernetes@db76a1b

I've rewritten this section to clearly define the behavior.

@yujuhong What do you think about an enum for the CRI? Something like: verb/kubernetes@db76a1b

Personally I like that better than a set of booleans for toggling the behavior :-) We could name the enum NamespaceType to be more generic.
That is, however, an API breaking change for CRI.

@mrunalp @Random-Liu @feiskyer thoughts?

Ok, great, a default of isolated it is. Good point on the generic name.

Also I realized that we'll need to be able to target a container's namespace for debug containers, which might influence the API design. This could be done as a fourth NamespaceType: verb/kubernetes@48fe4c2

@feiskyer for frakti? @Random-Liu for containerd @mrunalp for cri-o?

At sig-node, we talked about this, and the decision is introducing enum. Thanks!

@yujuhong Enum does look cleaner to me. And we could easily change to use the new enum in cri-containerd.

derekwaynecarr · 2017-09-25T17:02:00Z

contributors/design-proposals/node/pod-pid-namespace.md

+shared PID namespaces. When `sharedPID` is true, runtimes MUST share a PID
+namespace for the pod or return an error. When `sharedPID` is false, runtimes
+MAY implement isolated PID namespaces. That is, omitting this option defaults to
+pre-existing runtime behavior. For Docker the default remains isolated PID


nit: the default is not really runtime specific. we need the api default to remain isolated across any runtime at this moment.

Oh, I thought the rkt & hypervisor runtimes defaulted to shared PID so making isolated the default could break existing config. I'm happy to define the default as isolated, I was just trying to be careful about backwards compatibility.

I've rewritten this section to clearly define the behavior.

mrunalp · 2017-09-25T17:07:37Z

contributors/design-proposals/node/pod-pid-namespace.md

+to true in `LinuxContainerSecurityContext` to a call to Docker's create
+container request with `PidMode` set to the container ID of the sandbox
+container. All containers in the pod will then start in the same PID namespace
+as the sandbox container, with the `/pause` binary as PID 1.


Does it make sense to allow users to use their own custom sandbox image with pid 1 capabilities?

There's nothing preventing users to configure their runtime/shim to use a different image, if the runtime/shim supports it. I don't see we support that in kube/CRI in the near future though.

It'd be great to support such a feature, but I think it would be complicated. That's more towards the "general purpose init" to which I alluded in Non-goals.

yujuhong · 2017-09-25T18:49:10Z

contributors/design-proposals/node/pod-pid-namespace.md

+The Container Runtime Interface (CRI) will be updated to explicitly support
+shared PID namespaces. When `sharedPID` is true, runtimes MUST share a PID
+namespace for the pod or return an error. When `sharedPID` is false, runtimes
+MAY implement isolated PID namespaces. That is, omitting this option defaults to


+1 for MUST.
I'd rather see the behavior clearly defined in CRI.

yujuhong · 2017-09-25T18:50:59Z

contributors/design-proposals/node/pod-pid-namespace.md

+We will add first class support for configuring isolated and shared PID
+namespaces for pods by adding a new boolean field `sharedPID` to the pod spec.
+When set to true, all containers in the pod will share a single PID namespace
+when supported by runtime. When not supported by the runtime, for example with


The runtime behavior should be move to the next paragraph, together with CRI.

yujuhong · 2017-09-25T18:52:33Z

contributors/design-proposals/node/pod-pid-namespace.md

-* Making PID namespace configurable requires changing the CRI, which we would
-  like to avoid since there are no use cases.
+We will add first class support for configuring isolated and shared PID
+namespaces for pods by adding a new boolean field `sharedPID` to the pod spec.


Clarify that default will be False.

yujuhong · 2017-09-25T19:04:41Z

contributors/design-proposals/node/pod-pid-namespace.md

+       ...
+```
+
+`PodSecurityContext` was chosen because it's where namespace configuration


I believe this is the internal API object..... The v1.PodSecurityContext does not contain the PID namespace fields...Could you update the text/code here to reflect that?

Whoops, you're right. Updated.

yujuhong · 2017-09-25T19:08:28Z

contributors/design-proposals/node/pod-pid-namespace.md

+to true in `LinuxContainerSecurityContext` to a call to Docker's create
+container request with `PidMode` set to the container ID of the sandbox
+container. All containers in the pod will then start in the same PID namespace
+as the sandbox container, with the `/pause` binary as PID 1.


There's nothing preventing users to configure their runtime/shim to use a different image, if the runtime/shim supports it. I don't see we support that in kube/CRI in the near future though.

dchen1107 · 2017-12-20T17:38:44Z

contributors/design-proposals/node/pod-pid-namespace.md

+
+Setting both `SharedPID` and `HostPID` will cause a validation error.
+
+### Container Runtime Interface Changes


cc/ @Random-Liu for containerd and @mrunalp for cri-o on CRI changes.

smarterclayton · 2017-12-31T20:54:52Z

contributors/design-proposals/node/pod-pid-namespace.md

-2. Modify the Infra container used by the Docker runtime to reap orphaned
-   zombies ([#36853](https://pr.k8s.io/36853)).
+We will add first class support for configuring isolated and shared PID
+namespaces for pods by adding a new boolean field `sharedPID` to the pod spec,


The name leaves something to be desired, even if it is a mirror of hostPID (which itself is not a great name).

I'd probably prefer a name that covers the two important aspects of the flag:

that none of the containers will be PID 1

that all processes in each container will be able to see each other (and potentially interact)

shareProcessNamespace or something that captures "all containers will be part of the shared namespace" is better than sharedPID.

SGTM. I like the more descriptive name. We'll need to be sure to document well the relationship between shareProcessNamespace and hostPID.

@smarterclayton could you suggest an API reviewer for this change? It'd be nice to loop in that person to the proposal rather than waiting for one to be auto-assigned by the code PR.

smarterclayton · 2017-12-31T20:56:35Z

contributors/design-proposals/node/pod-pid-namespace.md

+1.  easier troubleshooting of pods.
+1.  addressing [Docker's zombie
+    problem](https://blog.phusion.nl/2015/01/20/docker-and-the-pid-1-zombie-reaping-problem/)
+    by reaping orphaned zombies in the infra container.


Please note the downsides of enabling this shared mode - sidecar containers that were previously isolated are no longer so, environment variables are now visible to all other processes, any "kill all" semantics used within the process are now broken, exec processes from other containers will now show up, etc. This doc should clarify tradeoffs.

Added a section here. Let me know if I missed anything.

smarterclayton · 2017-12-31T20:57:15Z

@mrunalp @runcom can you please comment on this with respect to CRI-O? I'd like to see forward progress on this in 1.10.

yujuhong · 2018-01-02T22:07:14Z

contributors/design-proposals/node/pod-pid-namespace.md

+    // PID namespace for this container/sandbox.
+    // Note: The CRI default is POD, but the v1.PodSpec default is CONTAINER.
+    // The kubelet's runtime manager will set this to CONTAINER by default for v1 pods.
+    // Runtimes must support: POD, CONTAINER, NODE


Shouldn't this support TARGET too?

Follow-up question: do we want other namespaces to also support TARGET?

It's a bit strange for the other namespaces since they don't support isolation. We don't need to support TARGET for things like network & ipc because POD & NODE are sufficient for the new container to be placed in the correct namespace.

yujuhong · 2018-01-02T22:13:56Z

contributors/design-proposals/node/pod-pid-namespace.md

+    // IPC namespace for this container/sandbox.
+    // Runtimes must support: POD, NODE
+    NamespaceMode ipc = 3;
+    // Target Container or Sandbox ID for NamespaceMode of TARGET


Do we need to allow Sandbox IDs?

Oh, no, and I don't think we want to. Containers should only be able to target their own pod. I'll remove that.

yujuhong · 2018-01-02T22:17:21Z

contributors/design-proposals/node/pod-pid-namespace.md

+Since we will support isolated PID namespaces indefinitely, we will need to be
+able to target the namespace of specific isolated containers to enable
+[Troubleshooting Running Pods](troubleshooting-running-pods.md). This is done
+with an additional field in `NamespaceOption`:


I think the previous subsection is more straightforward, but the introduction of TARGET makes the use case more complex. Could you elaborate a bit more on how the debug container would be configured in CRI?

In CRI today, you'd need to create a container in a PodSandbox, i.e., your debug container needs to join an existing PodSandbox, or you'd need to create a new PodSandbox for it. Are you going to allow pod A to join some of pod B's namespaces, or do you always require the debug container to join the PodSandbox in which the target container resides? What combination of NamespaceOptions and PodSandbox configs are valid?

Containers should not be able to join a namespace from a different pod. We require the debug container to join the sandbox in which the target container resides. I've updated the propose to be explicit about this.

yujuhong · 2018-01-02T22:19:21Z

contributors/design-proposals/node/pod-pid-namespace.md

+
+### Container Runtime Interface Changes
+
+Namespace options in the CRI are currently specified for both `PodSandbox` and


The CRI changes in this subsection looks good to me. It makes the API more clear.

/cc a few CRI runtime maintainers since the change won't be backward compatible.
@Random-Liu @mrunalp @runcom

mrunalp · 2018-01-09T18:45:45Z

We should be able to support this in CRI-O.

wking · 2018-01-23T19:53:04Z

To get the infra-reaping benefits of pod mode with the isolation benefits of container-mode, I'd like to also specify a mode where the pod's worker containers have PID namespaces which are children of the infra container's PID namespace. So:

host PID namespace
- infra PID namespace
  - worker PID namespace
  - worker PID namespace
  - …

That's the pod-container choice in the implementation I've floated in cri-o/cri-o#1280.

verb · 2018-01-23T20:58:32Z

@wking Interesting, what are the benefits of "pod-container" over "container"? Is it that with pod-container the infra container will inherit orphaned zombies somehow?

wking · 2018-01-23T21:14:06Z

Interesting, what are the benefits of "pod-container" over "container"? Is it that with pod-container the infra container will inherit orphaned zombies somehow?

Assuming there are no subreaper modifications, here's an example process tree with pod-container:

PID (host)	PID (infra)	PID (container)	Reaper (host namespace)
1	-	-	-
2	1	-	1
3	2	1	2
4	3	2	3

And here's the same setup with container:

PID (host)	PID (infra)	PID (container)	Reaper (host namespace)
1	-	-	-
2	1	-	1
3	-	1	1
4	-	2	3

And here's the same setup with pod:

PID (host)	PID (infra)	Reaper (host namespace)
1	-	-
2	1	1
3	2	2
4	3	2

But I think the real benefit is that pod-container makes it easy for the infra process(es) to monitor processes in the worker containers (like the pod approach) while making it hard for those worker processes to mess with the infra processes (like the container approach).

yujuhong · 2018-01-23T22:30:48Z

That's the pod-container choice in the implementation I've floated in cri-o/cri-o#1280.

I think this falls under the purview of the runtime given that we don't restrict how podsandbox must be implemented in CRI.

yujuhong · 2018-01-23T22:30:56Z

/lgtm

wking · 2018-01-23T22:39:23Z

On Tue, Jan 23, 2018 at 10:30:50PM +0000, Yu-Ju Hong wrote: > That's the pod-container choice in the implementation I've floated > in cri-o/cri-o#1280. I think this falls under the purview of the runtime given that we don't restrict how podsandbox must be implemented in CRI.

Are you saying that both my ‘container’ and ‘pod-container’ approaches would be valid implementations of this proposal's ‘container’ choice, and that there would be no standard way for a user to select between them?

yujuhong · 2018-01-23T22:42:52Z

@wking yes. As of now, CRI does not dictate how runtime implements the podsandbox.

dchen1107 · 2018-01-24T17:34:32Z

/lgtm

k8s-ci-robot · 2018-01-24T17:34:37Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: dchen1107, verb, yujuhong

The full list of commands accepted by this bot can be found here.

Needs approval from an approver in each of these OWNERS Files:

~~contributors/design-proposals/node/OWNERS~~ [dchen1107]

You can indicate your approval by writing /approve in a comment
You can cancel your approval by writing /approve cancel in a comment

Update Docker Shared PID proposal for per-pod configuration

* Simplfy org configuration * Give members back triage

k8s-ci-robot added do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. size/M Denotes a PR that changes 30-99 lines, ignoring generated files. cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. labels Sep 12, 2017

k8s-ci-robot added the sig/node Categorizes an issue or PR as relevant to SIG Node. label Sep 12, 2017

k8s-github-robot assigned thockin and smarterclayton Sep 12, 2017

k8s-github-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 15, 2017

smarterclayton reviewed Sep 16, 2017

View reviewed changes

yujuhong reviewed Sep 22, 2017

View reviewed changes

verb force-pushed the per-pod branch from efe3cf2 to a88e620 Compare September 22, 2017 14:16

k8s-ci-robot added size/L Denotes a PR that changes 100-499 lines, ignoring generated files. and removed size/M Denotes a PR that changes 30-99 lines, ignoring generated files. labels Sep 22, 2017

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2017

verb force-pushed the per-pod branch 2 times, most recently from 07dbdc9 to d765a80 Compare September 22, 2017 14:29

verb changed the title ~~WIP: Update Docker Shared PID proposal for per-pod configuration~~ Update Docker Shared PID proposal for per-pod configuration Sep 22, 2017

k8s-ci-robot removed the do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. label Sep 22, 2017

thockin reviewed Sep 22, 2017

View reviewed changes

verb force-pushed the per-pod branch from d765a80 to 5e0fd8b Compare September 25, 2017 16:24

derekwaynecarr self-assigned this Sep 25, 2017

mrunalp reviewed Sep 25, 2017

View reviewed changes

derekwaynecarr reviewed Sep 25, 2017

View reviewed changes

mrunalp reviewed Sep 25, 2017

View reviewed changes

yujuhong reviewed Sep 25, 2017

View reviewed changes

dchen1107 reviewed Dec 20, 2017

View reviewed changes

verb force-pushed the per-pod branch from 61eafa3 to 8e847fa Compare December 22, 2017 12:04

k8s-github-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Dec 22, 2017

smarterclayton reviewed Dec 31, 2017

View reviewed changes

yujuhong reviewed Jan 2, 2018

View reviewed changes

verb force-pushed the per-pod branch from 8e847fa to 0803971 Compare January 10, 2018 15:29

verb mentioned this pull request Jan 10, 2018

Remove --docker-disable-shared-pid from kubelet kubernetes/kubernetes#41938

Closed

verb force-pushed the per-pod branch from 0803971 to 7952451 Compare January 23, 2018 16:52

mrunalp mentioned this pull request Jan 23, 2018

*: Add --pid-namespace=[container|pod|pod-container] cri-o/cri-o#1280

Closed

Update Docker Shared PID NS for per-pod config

333729b

verb force-pushed the per-pod branch from 7952451 to 333729b Compare January 23, 2018 21:23

verb mentioned this pull request Jan 23, 2018

Add ShareProcessNamespace to PodSpec kubernetes/kubernetes#58716

Merged

3 tasks

k8s-ci-robot added the lgtm "Looks good to me", indicates that a PR is ready to be merged. label Jan 23, 2018

k8s-ci-robot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 24, 2018

k8s-ci-robot merged commit 483f393 into kubernetes:master Jan 24, 2018

yujuhong mentioned this pull request Jan 29, 2018

Update Container Runtime Interface to use enumerated namespace modes kubernetes/kubernetes#58973

Merged

MadhavJivrajani pushed a commit to MadhavJivrajani/community that referenced this pull request Nov 30, 2021

Merge pull request kubernetes#1048 from verb/per-pod

008b847

Update Docker Shared PID proposal for per-pod configuration

danehans pushed a commit to danehans/community that referenced this pull request Jul 18, 2023

Simplfy org configuration (kubernetes#1048)

6221d3f

* Simplfy org configuration * Give members back triage

		@@ -60,18 +95,11 @@ The ability to disable shared PID namespaces is intended as a way to roll back
		to prior behavior in the event of unforeseen problems. It won't be possible to
		configure the behavior per-pod. We believe this is acceptable because:


		### Kubernetes API Changes

		`v1.PodSecurityContext` gains a new field resembling the existing `hostPID`


		Setting both `SharedPID` and `HostPID` will cause a validation error.

		### Container Runtime Interface Changes


		### Container Runtime Interface Changes

		Namespace options in the CRI are currently specified for both `PodSandbox` and

Update Docker Shared PID proposal for per-pod configuration #1048

Update Docker Shared PID proposal for per-pod configuration #1048

Conversation

verb commented Sep 12, 2017 • edited Loading

k8s-ci-robot commented Sep 12, 2017

verb commented Sep 12, 2017

thockin commented Sep 15, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

verb commented Sep 22, 2017 via email

verb commented Sep 22, 2017

thockin left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

derekwaynecarr commented Sep 25, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

smarterclayton commented Dec 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mrunalp commented Jan 9, 2018

wking commented Jan 23, 2018

verb commented Jan 23, 2018

wking commented Jan 23, 2018

yujuhong commented Jan 23, 2018

yujuhong commented Jan 23, 2018

wking commented Jan 23, 2018 via email

yujuhong commented Jan 23, 2018

dchen1107 commented Jan 24, 2018

k8s-ci-robot commented Jan 24, 2018

verb commented Sep 12, 2017 •

edited

Loading