Kube Play - allow setting and overriding published host ports #16880

ygalblum · 2022-12-18T14:35:27Z

Does this PR introduce a user-facing change?

Yes

Kube Play - Add --publish flag to set or override port publishing

ygalblum · 2022-12-18T14:35:54Z

This PR is the first step in addressing #16595

vrothberg

LGTM
@Luap99 @umohnani8 PTAL

test/e2e/play_kube_test.go

cmd/podman/kube/play.go

docs/source/markdown/podman-kube-play.1.md.in

pkg/domain/infra/abi/play.go

Luap99 · 2022-12-19T13:09:26Z

test/e2e/play_kube_test.go

+		// The error message is printed only on local call
+		if !IsRemote() {
+			Expect(kube.OutputToString()).Should(ContainSubstring("rootlessport cannot expose privileged port 80"))
+		}


this is a bug and should be fixed? assuming this is already the case before this PR could you fill a issue for it

test/e2e/play_kube_test.go

ygalblum · 2022-12-25T11:28:20Z

I've fixed the code according to the comments. But, I think we need to align the functionality between the two changes this and #16766 before we proceed with either of them

TomSweeneyRedHat · 2022-12-29T20:42:25Z

A number of red tests. The changes LGTM in general, I'll let others decide on the thumb-wrestling between this and #16766

ygalblum · 2023-01-03T10:47:23Z

@Luap99 I've fixed the code according to your comment. Please see my explanation here: #16880 (comment).
@dcermak wrote in #16766 that he thinks this PR should go in first. So, if you agree, let's continue here so he can complete his change as well

pkg/domain/infra/abi/play.go

Luap99

mostly looks good, the test should make sure both ports are present

Luap99 · 2023-01-04T17:21:18Z

test/e2e/play_kube_test.go

+		kube := podmanTest.Podman([]string{"play", "kube", "--publish", "19007:80/udp", kubeYaml})
+		kube.WaitWithDefaultTimeout()
+		Expect(kube).Should(Exit(0))
+


this needs an inspect or something to make sure that both ports are defined, your test only check for the tcp port below

I changed the test to test both UDP and TCP connections. So, I guess there's no need for inspect, right?

sure that works too

Luap99 · 2023-01-04T17:21:31Z

test/e2e/play_kube_test.go

+		kube := podmanTest.Podman([]string{"play", "kube", "--publish", "19010:19008/tcp", kubeYaml})
+		kube.WaitWithDefaultTimeout()
+		Expect(kube).Should(Exit(0))
+


I changed the test to test both UDP and TCP connections. So, I guess there's no need for inspect, right?

Luap99 · 2023-01-04T17:25:25Z

pkg/domain/infra/abi/play.go

+	for _, publishedPort := range publishedPorts {
+		if port.ContainerPort >= publishedPort.ContainerPort && port.ContainerPort < publishedPort.ContainerPort+publishedPort.Range && isSamePortProtocol(port.Protocol, publishedPort.Protocol) {
+			return true
+


necessary newline

Right, fixed

rhatdan · 2023-01-05T18:52:55Z

/approve
@ygalblum Looks like your test is failing?

ygalblum · 2023-01-06T11:05:57Z

@containers/podman-maintainers The tests pass locally. Looking at the failure logs, it seems that the containers are started successfully, but then the test code fails to connect to the port. In addition, other flavors of the suite pass. So, it might be a difference in the setup that causes the failures. Can you think of a reason why they fail in the CI?

Luap99 · 2023-01-06T11:23:32Z

CI systems are always much slower compared to local. You need to add some retry mechanism for the connect in the test. If we do a podman run -d or in this case podman play kube it will return as soon as the container process is started but that does not mean that this container process is ready to accept incoming connections.
something like this

podman/test/e2e/network_test.go

Lines 514 to 526 in 98d95f0

    
           // Wait for the nginx service to be running 
        
           for i := 0; i < 6; i++ { 
        
           	// Test curl against the container's name 
        
           	c1 := podmanTest.Podman([]string{"run", "--dns-search", "dns.podman", "--network=" + netName, NGINX_IMAGE, "curl", "web"}) 
        
           	c1.WaitWithDefaultTimeout() 
        
           	worked = c1.ExitCode() == 0 
        
           	if worked { 
        
           		break 
        
           	} 
        
           	time.Sleep(interval) 
        
           	interval *= 2 
        
           } 
        
           Expect(worked).To(BeTrue())

ygalblum · 2023-01-08T06:51:40Z

@Luap99 Thanks for the tip. I've updated the test code

rhatdan · 2023-01-08T11:40:12Z

Lint is not happy with this change.

Add a new flag --publish Remote - Pass PublishPorts as a string array ABI - translate the string array to Ports and merge with the ports in the spec Add e2e tests Add option to man doc Signed-off-by: Ygal Blum <[email protected]>

ygalblum · 2023-01-08T11:50:29Z

Lint is not happy with this change.

Thanks, fixed

ygalblum · 2023-01-09T06:43:13Z

@containers/podman-maintainers can someone please restart the failed test. This time it is unrelated to my changes

mheon · 2023-01-09T14:34:28Z

Done

Luap99

LGTM

rhatdan · 2023-01-09T18:25:45Z

/lgtm
/hold

ygalblum · 2023-01-10T16:40:33Z

@Luap99 I've made all the change and all tests are done and passing, but the PR still has the "Changes requested" state. Can you please re-review?

Luap99

LGTM

openshift-ci · 2023-01-10T16:44:12Z

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Luap99, rhatdan, ygalblum

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

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

Luap99 · 2023-01-10T16:44:15Z

/hold cancel

baude · 2023-02-03T17:16:34Z

/cherry-pick v4.4

openshift-cherrypick-robot · 2023-02-03T17:17:12Z

@baude: #16880 failed to apply on top of branch "v4.4":

Applying: Kube Play - allow setting and overriding published host ports
Using index info to reconstruct a base tree...
M	cmd/podman/kube/play.go
M	docs/source/markdown/podman-kube-play.1.md.in
M	pkg/api/handlers/libpod/kube.go
M	pkg/bindings/kube/types.go
M	pkg/bindings/kube/types_play_options.go
M	pkg/domain/entities/play.go
M	pkg/domain/infra/abi/play.go
M	pkg/domain/infra/tunnel/kube.go
M	test/e2e/play_kube_test.go
Falling back to patching base and 3-way merge...
Auto-merging test/e2e/play_kube_test.go
CONFLICT (content): Merge conflict in test/e2e/play_kube_test.go
Auto-merging pkg/domain/infra/abi/play.go
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Kube Play - allow setting and overriding published host ports
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".

In response to this:

/cherry-pick v4.4

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.

vrothberg · 2023-02-06T09:03:24Z

This is already in the v4.4 branch

openshift-ci bot added the release-note label Dec 18, 2022

github-actions bot added the kind/api-change Change to remote API; merits scrutiny label Dec 18, 2022

ygalblum force-pushed the kube-publish-ports branch 3 times, most recently from 3e86976 to 42a00be Compare December 19, 2022 08:48

vrothberg reviewed Dec 19, 2022

View reviewed changes

test/e2e/play_kube_test.go Outdated Show resolved Hide resolved

ygalblum force-pushed the kube-publish-ports branch from 42a00be to 4bd3e46 Compare December 19, 2022 09:08

Luap99 requested changes Dec 19, 2022

View reviewed changes

ygalblum mentioned this pull request Dec 25, 2022

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

Closed

ygalblum force-pushed the kube-publish-ports branch 2 times, most recently from bc11c51 to 5021458 Compare December 25, 2022 11:23

ygalblum force-pushed the kube-publish-ports branch from 5021458 to a8976b4 Compare December 28, 2022 06:52

ygalblum mentioned this pull request Dec 28, 2022

Quadlet kube support for HostPort overrides #16595

Closed

ygalblum force-pushed the kube-publish-ports branch from a8976b4 to 0b44907 Compare December 28, 2022 07:17

ygalblum force-pushed the kube-publish-ports branch from 0b44907 to 48d9973 Compare January 1, 2023 07:49

Luap99 reviewed Jan 3, 2023

View reviewed changes

pkg/domain/infra/abi/play.go Outdated Show resolved Hide resolved

ygalblum force-pushed the kube-publish-ports branch from 48d9973 to a50b51c Compare January 4, 2023 16:35

Luap99 reviewed Jan 4, 2023

View reviewed changes

ygalblum force-pushed the kube-publish-ports branch 2 times, most recently from 46676b2 to f8b0959 Compare January 5, 2023 07:57

openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Jan 5, 2023

rhatdan added the 4.4 label Jan 7, 2023

ygalblum force-pushed the kube-publish-ports branch from f8b0959 to 16ff0bc Compare January 8, 2023 06:50

ygalblum force-pushed the kube-publish-ports branch from 16ff0bc to 6596dcb Compare January 8, 2023 09:46

Syquel mentioned this pull request Jan 8, 2023

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

Closed

ygalblum force-pushed the kube-publish-ports branch from 6596dcb to 07cc49e Compare January 8, 2023 11:49

Luap99 reviewed Jan 9, 2023

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 Jan 9, 2023

openshift-ci bot assigned rhatdan Jan 9, 2023

openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Jan 9, 2023

Luap99 approved these changes Jan 10, 2023

View reviewed changes

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

openshift-merge-robot merged commit 6ba308f into containers:main Jan 10, 2023

ygalblum deleted the kube-publish-ports branch January 11, 2023 06:30

edsantiago mentioned this pull request Jan 11, 2023

CI: timeout in new kube tests #17076

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

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Kube Play - allow setting and overriding published host ports #16880

Kube Play - allow setting and overriding published host ports #16880

ygalblum commented Dec 18, 2022

ygalblum commented Dec 18, 2022

vrothberg left a comment

Luap99 Dec 19, 2022

ygalblum commented Dec 25, 2022

TomSweeneyRedHat commented Dec 29, 2022

ygalblum commented Jan 3, 2023 •

edited

Loading

Luap99 left a comment

Luap99 Jan 4, 2023

ygalblum Jan 5, 2023

Luap99 Jan 9, 2023

Luap99 Jan 4, 2023

ygalblum Jan 5, 2023

Luap99 Jan 4, 2023

ygalblum Jan 5, 2023

rhatdan commented Jan 5, 2023

ygalblum commented Jan 6, 2023 •

edited

Loading

Luap99 commented Jan 6, 2023 •

edited

Loading

ygalblum commented Jan 8, 2023

rhatdan commented Jan 8, 2023

ygalblum commented Jan 8, 2023

ygalblum commented Jan 9, 2023

mheon commented Jan 9, 2023

Luap99 left a comment

rhatdan commented Jan 9, 2023

ygalblum commented Jan 10, 2023

Luap99 left a comment

openshift-ci bot commented Jan 10, 2023

Luap99 commented Jan 10, 2023

baude commented Feb 3, 2023

openshift-cherrypick-robot commented Feb 3, 2023

vrothberg commented Feb 6, 2023

Kube Play - allow setting and overriding published host ports #16880

Kube Play - allow setting and overriding published host ports #16880

Conversation

ygalblum commented Dec 18, 2022

Does this PR introduce a user-facing change?

ygalblum commented Dec 18, 2022

vrothberg left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ygalblum commented Dec 25, 2022

TomSweeneyRedHat commented Dec 29, 2022

ygalblum commented Jan 3, 2023 • edited Loading

Luap99 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rhatdan commented Jan 5, 2023

ygalblum commented Jan 6, 2023 • edited Loading

Luap99 commented Jan 6, 2023 • edited Loading

ygalblum commented Jan 8, 2023

rhatdan commented Jan 8, 2023

ygalblum commented Jan 8, 2023

ygalblum commented Jan 9, 2023

mheon commented Jan 9, 2023

Luap99 left a comment

Choose a reason for hiding this comment

rhatdan commented Jan 9, 2023

ygalblum commented Jan 10, 2023

Luap99 left a comment

Choose a reason for hiding this comment

openshift-ci bot commented Jan 10, 2023

Luap99 commented Jan 10, 2023

baude commented Feb 3, 2023

openshift-cherrypick-robot commented Feb 3, 2023

vrothberg commented Feb 6, 2023

ygalblum commented Jan 3, 2023 •

edited

Loading

ygalblum commented Jan 6, 2023 •

edited

Loading

Luap99 commented Jan 6, 2023 •

edited

Loading