Default missing hostPort to containerPort is defined in kube.yaml #15946

rhatdan · 2022-09-26T18:50:46Z

If user does not specify hostPort in a kube.yml file but does specify a containerPort, then the hostPort should default to the containerPort.

Fixes: #15942

Signed-off-by: Daniel J Walsh dwalsh@redhat.com

Does this PR introduce a user-facing change?

podman kube play will default hostPort of Pod to containerPort if hostPort is not specified by containerPort is specified.

openshift-ci · 2022-09-26T18:50:54Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: rhatdan

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~~ [rhatdan]

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

rhatdan · 2022-09-26T18:51:03Z

@umohnani8 @saschagrunert @haircommander PTAL

edsantiago

I ran your new test against main @ aaeabb0. It passes (local & remote, root & rootless). That suggests that your test is not testing what you think it's testing. (Or, I'm missing something crucial, which is always a high probability).

edsantiago · 2022-09-26T18:55:54Z

test/system/700-play.bats

+
+@test "podman kube play - hostport" {
+    HOST_PORT=$(random_free_port)
+    TESTDIR=$PODMAN_TMPDIR/testdir


$PODMAN_TMPDIR is fresh and new for each @test; so I don't think you need $TESTDIR.

haircommander · 2022-09-26T19:01:18Z

LGTM; nice and easy

edsantiago · 2022-09-26T19:30:46Z

test/system/700-play.bats

+
+    run_podman kube play $PODMAN_TMPDIR/testpod.yaml
+    run_podman pod inspect test_pod --format "{{.InfraConfig.PortBindings}}"
+    is "$output" "map[$HOST_PORT/tcp:[{ $HOST_PORT}]]"


Whew! Figured out why test passes on main. It's because the is function tries an equal-match (==) first, but then tries expr if that fails. Which means all those brackets end up meaning "map, plus maybe other stuff". Fix:

- is "$output" "map[$HOST_PORT/tcp:[{ $HOST_PORT}]]" + assert "$output" = "map[$HOST_PORT/tcp:[{ $HOST_PORT}]]"

asserts, with explicit =, will not try a regex match. With this fix, the test fails on main and passes with your PR.

mheon · 2022-09-26T19:50:35Z

LGTM

rhatdan · 2022-09-26T19:59:53Z

/hold

vrothberg · 2022-09-27T08:49:09Z

int tests are barking

edsantiago · 2022-09-27T11:28:21Z

rootless is failing, and I'm kind of almost sure that it's this:

podman/test/e2e/play_kube_test.go

Line 137 in 5fc6d95

- containerPort: 80

(also line 148). Haven't looked closely enough to know how to fix: maybe skip if rootless, maybe change the port, maybe remove that port line entirely.

edsantiago · 2022-09-27T12:28:03Z

Friendly suggestion:

diff --git a/test/e2e/play_kube_test.go b/test/e2e/play_kube_test.go
index 67c88953a..651cb1074 100644
--- a/test/e2e/play_kube_test.go
+++ b/test/e2e/play_kube_test.go
@@ -133,8 +133,6 @@ spec:
   containers:
   - name: podnameEqualsContainerNameYaml
     image: quay.io/libpod/alpine:latest
-    ports:
-    - containerPort: 80
 `
 
 var podWithoutAName = `
@@ -239,8 +237,6 @@ spec:
         - "1.5"
         name: alpine
         image: quay.io/libpod/alpine:latest
-        ports:
-        - containerPort: 80
         livenessProbe:
           exec:
             command:
@@ -274,8 +270,6 @@ spec:
         - "1.5"
         name: alpine
         image: quay.io/libpod/alpine:latest
-        ports:
-        - containerPort: 80
         livenessProbe:
           exec:
             command:

Important note: I choose to remove the port lines completely, vs change to 8080 or somesuch, because of the ginkgo parallel-test thing in which two tests could want to bind to the same port at the same time.

If user does not specify hostPort in a kube.yml file but does specify a containerPort, then the hostPort should default to the containerPort. Fixes: containers#15942 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

umohnani8 · 2022-09-27T18:30:28Z

/lgtm

edsantiago · 2022-09-27T20:17:10Z

/hold cancel

podman play kube started to expose the port of containers in k8s deployments on the same port on the host with containers#15946. However, that breaks once replicas are involved, as they would then bind to the same port on the host. With this commit, we change the default behavior so that podman will pick a random port for the container instead of using `containerPort`, but only if no `hostPort` is set. This fixes: containers#16765 Signed-off-by: Dan Čermák <dcermak@suse.com>

openshift-ci bot added the do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None label Sep 26, 2022

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

openshift-ci bot added release-note and removed do-not-merge/release-note-label-needed Enforce release-note requirement, even if just None labels Sep 26, 2022

edsantiago requested changes Sep 26, 2022

View reviewed changes

rhatdan force-pushed the kube branch from 60afdd1 to c2f8682 Compare September 26, 2022 19:24

edsantiago reviewed Sep 26, 2022

View reviewed changes

openshift-ci bot added the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 26, 2022

rhatdan added the lgtm Indicates that a PR is ready to be merged. label Sep 26, 2022

rhatdan force-pushed the kube branch from c2f8682 to 7fa6309 Compare September 27, 2022 10:18

openshift-ci bot removed the lgtm Indicates that a PR is ready to be merged. label Sep 27, 2022

rhatdan force-pushed the kube branch from 7fa6309 to 7a39d64 Compare September 27, 2022 10:18

Default missing hostPort to containerPort is defined in kube.yaml

f5fc096

If user does not specify hostPort in a kube.yml file but does specify a containerPort, then the hostPort should default to the containerPort. Fixes: containers#15942 Signed-off-by: Daniel J Walsh <dwalsh@redhat.com>

rhatdan force-pushed the kube branch from 7a39d64 to f5fc096 Compare September 27, 2022 18:04

openshift-ci bot assigned umohnani8 Sep 27, 2022

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

openshift-ci bot removed the do-not-merge/hold Indicates that a PR should not merge because someone has issued a /hold command. label Sep 27, 2022

openshift-merge-robot merged commit dca5ead into containers:main Sep 27, 2022

rhatdan deleted the kube branch December 1, 2022 22:00

dcermak mentioned this pull request Dec 7, 2022

podman play kube fails when a container has a containerPort set and replicas > 1 #16765

Closed

dcermak mentioned this pull request Dec 7, 2022

Pick a random port on the host for play kube deployments #16766

Closed

Luap99 mentioned this pull request May 11, 2023

[Bug]: Disable automatic publishing of all ports for kube play #17028

Closed

trchen1033 mentioned this pull request May 15, 2023

podman play kube should NOT bind ports when hostPort is not specified. #18576

Closed

joelpurra mentioned this pull request May 18, 2023

Add --publish-all flag for kube play #18557

Closed

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 20, 2023

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Default missing hostPort to containerPort is defined in kube.yaml #15946

Default missing hostPort to containerPort is defined in kube.yaml #15946

rhatdan commented Sep 26, 2022 •

edited

openshift-ci bot commented Sep 26, 2022

rhatdan commented Sep 26, 2022

edsantiago left a comment

edsantiago Sep 26, 2022

haircommander commented Sep 26, 2022 •

edited

edsantiago Sep 26, 2022

mheon commented Sep 26, 2022

rhatdan commented Sep 26, 2022

vrothberg commented Sep 27, 2022

edsantiago commented Sep 27, 2022

edsantiago commented Sep 27, 2022

umohnani8 commented Sep 27, 2022

edsantiago commented Sep 27, 2022

Default missing hostPort to containerPort is defined in kube.yaml #15946

Default missing hostPort to containerPort is defined in kube.yaml #15946

Conversation

rhatdan commented Sep 26, 2022 • edited

Does this PR introduce a user-facing change?

openshift-ci bot commented Sep 26, 2022

rhatdan commented Sep 26, 2022

edsantiago left a comment

Choose a reason for hiding this comment

edsantiago Sep 26, 2022

Choose a reason for hiding this comment

haircommander commented Sep 26, 2022 • edited

edsantiago Sep 26, 2022

Choose a reason for hiding this comment

mheon commented Sep 26, 2022

rhatdan commented Sep 26, 2022

vrothberg commented Sep 27, 2022

edsantiago commented Sep 27, 2022

edsantiago commented Sep 27, 2022

umohnani8 commented Sep 27, 2022

edsantiago commented Sep 27, 2022

rhatdan commented Sep 26, 2022 •

edited

haircommander commented Sep 26, 2022 •

edited