Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix all ports exposed on host by kube play #19823

Merged
merged 1 commit into from
Sep 28, 2023
Merged

Fix all ports exposed on host by kube play #19823

merged 1 commit into from
Sep 28, 2023

Conversation

Backfighter
Copy link
Contributor

Container ports defined with containerPort were exposed by default even though kubernetes interprets them as mostly informative.
Closes #17028

User-facing changes

Fixed podman kube play exposing all container ports on host even if only specified as containerPort insteadof hostPort (#17028)

Added new flag --publish-all which allows to restore the old behaviour of "podman kube play" exposing all ports including those only defined as containerPort

@Backfighter Backfighter force-pushed the fix-17028 branch 2 times, most recently from 8e7eb12 to 6980823 Compare September 1, 2023 09:16
@Backfighter
Copy link
Contributor Author

Seems like sys remote debian-13 root host boltdb had a problem stopping one of the test containers causing subsequent tests to fail. Is there a way to retry just a single task without rerunning the whole suite?

@vrothberg
Copy link
Member

I restarted the job 👍

mheon
mheon reviewed Sep 1, 2023
View reviewed changes
pkg/specgen/generate/kube/kube.go
var infraPorts []types.PortMapping
for _, container := range containers {
for _, p := range container.Ports {
if p.HostPort != 0 && p.ContainerPort == 0 {
p.ContainerPort = p.HostPort
}
if p.HostPort == 0 && p.ContainerPort != 0 {
if p.HostPort == 0 && p.ContainerPort != 0 && publishAll {
p.HostPort = p.ContainerPort
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this correct for K8S? From podman run/docker run with --public-all, I'd expect a random HostPort to be assigned in this case, not setting HostPort to ContainerPort

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@umohnani8 @ygalblum PTAL

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree that it should have been a random port. But, the question here is backward compatibility.
The new flag breaks the current behavior, and we agreed that it is OK to do so. However, with the proposed change, users can overcome this break by adding this flag. Assigning random ports will break the behavior even further.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will need a decisive decision by the maintainers

@vrothberg @rhatdan @mheon @umohnani8 what do we decide here?

I think we should leave the old behavior to allow a simple fix to the backward compatibility break.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree with @ygalblum. The goal of this PR is to stop publishing all ports by default with an easy way to get back to the previous behavior if needed. Picking random ports would break the capability of restoring the previous behavior. 👍

@rhatdan
Copy link
Member

rhatdan commented Sep 12, 2023

@Backfighter still working on this?

@Backfighter
Copy link
Contributor Author

@Backfighter still working on this?

Yes but I am currently short on time and there are some open topics that I will need a decisive decision by the maintainers on. (See #19823 (comment))
Maybe on the weekend I will have the time to apply at least the suggestion regarding documentation.

@Backfighter Backfighter force-pushed the fix-17028 branch 2 times, most recently from e10613d to 726970a Compare September 19, 2023 12:26
@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 22, 2023
@Backfighter
Fix all ports exposed by kube play
f52b02f 
Container ports defined with containerPort were exposed by default
even though kubernetes interprets them as mostly informative.
Closes containers#17028

Signed-off-by: Peter Werner <[email protected]>
@openshift-merge-robot openshift-merge-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 23, 2023
vrothberg
vrothberg approved these changes Sep 28, 2023
View reviewed changes
Copy link
Member

@vrothberg vrothberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@vrothberg
Copy link
Member

Thanks for contributing, @Backfighter !

@openshift-ci
Copy link
Contributor

openshift-ci bot commented Sep 28, 2023

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Backfighter, vrothberg

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 /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@openshift-ci openshift-ci bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 28, 2023
@ygalblum
Copy link
Contributor

/lgtm

@openshift-ci openshift-ci bot added the lgtm Indicates that a PR is ready to be merged. label Sep 28, 2023
@openshift-merge-robot openshift-merge-robot merged commit 4212b49 into containers:main Sep 28, 2023
97 checks passed
@fsdrw08
Copy link

fsdrw08 commented Oct 7, 2023

may I know which podman version will contain this bug fix?

@Backfighter Backfighter deleted the fix-17028 branch October 7, 2023 09:56
@rhatdan
Copy link
Member

rhatdan commented Oct 7, 2023

Definitely podman 4.8 You can request a back port to 4.7 if it is just a bugfix.

@github-actions 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 Jan 6, 2024
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 6, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@TomSweeneyRedHat TomSweeneyRedHat TomSweeneyRedHat left review comments

@mheon mheon mheon left review comments

@ygalblum ygalblum ygalblum left review comments

@rhatdan rhatdan rhatdan left review comments

@vrothberg vrothberg vrothberg approved these changes

Assignees

@ygalblum ygalblum

Labels
approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. release-note
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[Bug]: Disable automatic publishing of all ports for kube play
8 participants
@Backfighter @vrothberg @rhatdan @ygalblum @fsdrw08 @mheon @TomSweeneyRedHat @openshift-merge-robot