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.

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

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

Closed

Conversation

Shikachuu
Copy link

Does this PR introduce a user-facing change?

Added `--publish-all` flag for CLI, which prevents container port publication without specifying a `hostPort` pair.

Added `publishAll` query parameter for API, which prevents container port publication without specifying a `hostPort` pair.

Closes #17028

@openshift-ci openshift-ci bot added release-note do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. labels May 13, 2023
@openshift-ci
Copy link
Contributor

openshift-ci bot commented May 13, 2023

[APPROVALNOTIFIER] This PR is NOT APPROVED

This pull-request has been approved by: Shikachuu
Once this PR has been reviewed and has the lgtm label, please assign edsantiago for approval. For more information see the Kubernetes Code Review Process.

The full list of commands accepted by this bot can be found 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

@github-actions github-actions bot added the kind/api-change Change to remote API; merits scrutiny label May 13, 2023
@rhatdan
Copy link
Member

rhatdan commented May 14, 2023

Please update man pages.

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.

Thanks!

We also need to add a test or two.

@@ -100,6 +101,7 @@ func KubePlay(w http.ResponseWriter, r *http.Request) {
PublishPorts: query.PublishPorts,
Wait: query.Wait,
ServiceContainer: query.ServiceContainer,
PublishAll: query.PublishAll,
Copy link
Member

Choose a reason for hiding this comment

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

For this to work remotely, we also need to touch pkg/bindings. Those are the go-bindings podman-remote uses. In case the client does not set the publish-all parameter, we should query contianers.conf here and use the configured defaults on the server side.

Copy link
Author

Choose a reason for hiding this comment

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

Since it is mostly generated code I might need some guidance.

The PlayOptions in pkg/binding/kube/types.go needs to be modified right?

After that we need to check if the flag is checked in the PlayWithBody func in the pkg/binding/kube/kube.go if I'm not mistaken.

Where and how should I run the generate command?

Currently I'm not sure how and where should I query the containers.conf may I require some aditional information about where should we do that exactly?

@Shikachuu Shikachuu force-pushed the disable_kube_auto_public branch from 6f6ddb0 to 567e456 Compare May 18, 2023 18:16
@Shikachuu
Copy link
Author

Thanks!

We also need to add a test or two.

Sure!
First I think we should get it to work using both the go bindings, the API and locally too.

After that I'll add test cases for each of the above mentioned use cases.

@joelpurra
Copy link
Contributor

joelpurra commented May 18, 2023

@Shikachuu: both the commit message and pull request description are outdated; could they be changed? In brief, they are not reflecting recent decisions in #17028 where the --publish-all flag usage was inverted to resolve a security issue.

My own summary:

The start (v4.3.0) of the version range is based on when #15946 was first included. The end (current release v4.5.0) of the version range depends on which release this pull request is first included.

Edit: referencing decisions in #17028 rather than #18576.

@github-actions
Copy link

A friendly reminder that this PR had no activity for 30 days.

@openshift-merge-robot openshift-merge-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 13, 2023
@openshift-merge-robot
Copy link
Collaborator

PR needs rebase.

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.

@rhatdan
Copy link
Member

rhatdan commented Jul 15, 2023

@Shikachuu Still working on this?

@github-actions github-actions bot removed the stale-pr label Jul 31, 2023
@vrothberg vrothberg closed this Sep 1, 2023
@Shikachuu Shikachuu deleted the disable_kube_auto_public branch September 3, 2023 17:54
@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 Dec 3, 2023
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Dec 3, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
do-not-merge/work-in-progress Indicates that a PR should not merge because it is a work in progress. kind/api-change Change to remote API; merits scrutiny locked - please file new issue/PR Assist humans wanting to comment on an old issue or PR with locked comments. needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. release-note
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Disable automatic publishing of all ports for kube play
5 participants