Add ports and hostname correctly in kube yaml #14181

umohnani8 · 2022-05-10T17:26:05Z

If a pod is created with ipc sharing, allow adding
separate ports for each container to the kube yaml
and also set the pod level hostname correctly.

Signed-off-by: Urvashi Mohnani [email protected]

Fixes #13030

Does this PR introduce a user-facing change?

Set ports and hostname correctly in the generated kube yaml.

rhatdan · 2022-05-10T17:28:23Z

LGTM
@containers/podman-maintainers PTAL

giuseppe

LGTM

openshift-ci · 2022-05-10T18:17:03Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: giuseppe, umohnani8

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:

~~OWNERS~~ [giuseppe,umohnani8]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

Luap99 · 2022-05-10T18:47:38Z

libpod/kube.go

-				ctr.Ports = ports
-				first = false
+			// infra container, wipe them here only if we are not sharing the ipc namespace
+			if !p.SharesIPC() {


Why is this ipc namespace? This looks like it should check for the netns. Ports can only be used when there is a netns, this is not related to the ipc namespace.

I think we need to come to a consensus on when we allow containers to expose different ports in a pod.
We currently error out when --share is set to uts, net, and the default value(ipc,net,uts).

➜ podman git:(kube-hostname) ✗ ./bin/podman pod create --name mypod =====s.NetNS.NSMode: slirp4netns 4fda91cfcc0e6cf5cd48e2ec2f81649a9217d5e6cb6b77e3ec2cc5ffe4802176 ➜ podman git:(kube-hostname) ✗ ./bin/podman run --name ss_02 --pod mypod --hostname test -p 5000:6000 -d fedora =====s.NetNS.NSMode: pod Error: invalid config provided: published or exposed ports must be defined when the pod is created: network cannot be configured when it is shared with a pod ➜ podman git:(kube-hostname) ✗ ./bin/podman pod create --name mypod --share uts =====s.NetNS.NSMode: slirp4netns 2293b04ed355ed2338cc1883dd3917bdaec2abbb05cb6f1258a9d9ff150d9603 ➜ podman git:(kube-hostname) ✗ ./bin/podman run --name ss_02 --pod mypod --hostname test -p 5000:6000 -d fedora =====s.NetNS.NSMode: slirp4netns Error: invalid config provided: cannot set hostname when joining the pod UTS namespace: invalid configuration ➜ podman git:(kube-hostname) ✗ ./bin/podman pod create --name mypod --share net =====s.NetNS.NSMode: slirp4netns f2b5169bc3d3599e0110009bee534d73b3e732108ce3161452c73ae7474ac249 ➜ podman git:(kube-hostname) ✗ ./bin/podman run --name ss_02 --pod mypod --hostname test -p 5000:6000 -d fedora =====s.NetNS.NSMode: pod Error: invalid config provided: published or exposed ports must be defined when the pod is created: network cannot be configured when it is shared with a pod

But it works fine with pid and ipc

➜ podman git:(kube-hostname) ✗ ./bin/podman pod create --name mypod --share pid =====s.NetNS.NSMode: slirp4netns f4c4a3ab9f9ad58e6037fd9db6cee170a01e4dc9d3bb3ec272799968742485ec ➜ podman git:(kube-hostname) ✗ ./bin/podman run --name ss_02 --pod mypod --hostname test -p 5000:6000 -d fedora =====s.NetNS.NSMode: slirp4netns c97df4f323dc13ec368798d381ba694685f6b999c5f77113260c18c9ef716eb2 ➜ podman git:(kube-hostname) ✗ ./bin/podman pod create --name mypod --share ipc =====s.NetNS.NSMode: slirp4netns ee955b9ef17d8039e414cb9375897fe9c5baab3d0389b231191e3eca5fe14bdd ➜ podman git:(kube-hostname) ✗ ./bin/podman run --name ss_02 --pod mypod --hostname test -p 5000:6000 -d fedora =====s.NetNS.NSMode: slirp4netns ce3aa72735d21a06aae21eb58bc06725e4d3f379e0e4e73dfd81ea41058432d3

Should we always require ports to be defined at the pod level only regardless of which kernel namespaces the pod will share?

We should match kubernetes, Can I have two different containers in a Pod listen on differnet ports blocked to the other container.

that all depends on whenever the netns is shared or not, if there is no shared netns then you could add ports to the pod(infra container) but since no container uses the namespace they will not have the same ports. Thefore ports must be per container.

If the netns is shared the ports should only be set on the pod(infra container).

Either way this has nothing to do with the ipc namespace.

Containers in a kubernetes pod share the network and ipc namespaces by default. You can set separate ports for each container in a pod, it is actually recommended to do that to separate the incoming traffic to each container.

It seems like podman pods has a requirement of defining all the ports at pod creation to associate them with the infra container and a patch was introduced to ensure that. But from testing, I noticed that we only error out when certain kernel namespaces are shared and not on others.

@Luap99 so basically, we should only enforce port being added at pod creation when only netns is shared. If any other namespace is shared, it should be fine to allow separate ports for each container?

cc @mheon

One more thing to note is, podman pod's default is different. We share uts, net, and ipc by default.

Just make sure this warning does not happen by default, just if users muck around with --share flag.

Doesn't kubernetes share uts, net and ipc by default? They also share User Namespace by defaut, but don't mention that...

Yup, that is true my bad. I didn't see it in their docs. Alright I will log a warning if anyone messes with --share and will add the restriction to --hostname. Thanks!

rhatdan · 2022-06-09T17:57:04Z

@umohnani8 Any movment on this or do you want to hand this off to an intern?

If a pod is created without net sharing, allow adding separate ports for each container to the kube yaml and also set the pod level hostname correctly if the uts namespace is not being shared. Add a warning if the default namespace sharing options have been modified by the user. Signed-off-by: Urvashi Mohnani <[email protected]>

umohnani8 · 2022-07-08T17:05:45Z

Made the requested changes and tests are basically green.
@containers/podman-maintainers PTAL

rhatdan · 2022-07-08T17:11:37Z

LGTM
@Luap99 PTAL

TomSweeneyRedHat · 2022-07-08T19:33:46Z

LGTM
but would like a head nod from @Luap99

Luap99 · 2022-07-11T15:10:01Z

/lgtm

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label May 10, 2022

giuseppe approved these changes May 10, 2022

View reviewed changes

Luap99 reviewed May 10, 2022

View reviewed changes

rhatdan added the kube label May 17, 2022

umohnani8 force-pushed the kube-hostname branch from ee9c785 to 65eb769 Compare June 14, 2022 15:45

umohnani8 force-pushed the kube-hostname branch from 65eb769 to 81a19a5 Compare July 8, 2022 15:23

openshift-ci bot assigned Luap99 Jul 11, 2022

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jul 11, 2022

openshift-ci bot merged commit 810cbf1 into containers:main Jul 11, 2022

umohnani8 deleted the kube-hostname branch February 23, 2023 18:44

github-actions bot added the locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. label Sep 8, 2023

github-actions bot locked as resolved and limited conversation to collaborators Sep 8, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add ports and hostname correctly in kube yaml #14181

Add ports and hostname correctly in kube yaml #14181

umohnani8 commented May 10, 2022

rhatdan commented May 10, 2022

giuseppe left a comment

openshift-ci bot commented May 10, 2022

Luap99 May 10, 2022

umohnani8 May 10, 2022

rhatdan May 10, 2022

Luap99 May 11, 2022

umohnani8 May 11, 2022

umohnani8 May 12, 2022

rhatdan May 12, 2022

rhatdan May 12, 2022

rhatdan May 12, 2022

umohnani8 May 12, 2022

rhatdan commented Jun 9, 2022

umohnani8 commented Jul 8, 2022

rhatdan commented Jul 8, 2022

TomSweeneyRedHat commented Jul 8, 2022

Luap99 commented Jul 11, 2022

Add ports and hostname correctly in kube yaml #14181

Add ports and hostname correctly in kube yaml #14181

Conversation

umohnani8 commented May 10, 2022

Does this PR introduce a user-facing change?

rhatdan commented May 10, 2022

giuseppe left a comment

Choose a reason for hiding this comment

openshift-ci bot commented May 10, 2022

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

rhatdan commented Jun 9, 2022

umohnani8 commented Jul 8, 2022

rhatdan commented Jul 8, 2022

TomSweeneyRedHat commented Jul 8, 2022

Luap99 commented Jul 11, 2022