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

Fix all ports exposed on host by kube play #19823

Merged
merged 1 commit into from
Sep 28, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions cmd/podman/kube/play.go
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,9 @@ func playFlags(cmd *cobra.Command) {
flags.StringSliceVar(&playOptions.PublishPorts, publishPortsFlagName, []string{}, "Publish a container's port, or a range of ports, to the host")
_ = cmd.RegisterFlagCompletionFunc(publishPortsFlagName, completion.AutocompleteNone)

publishAllPortsFlagName := "publish-all"
flags.BoolVar(&playOptions.PublishAllPorts, publishAllPortsFlagName, false, "Whether to publish all ports defined in the K8S YAML file (containerPort, hostPort), if false only hostPort will be published")

waitFlagName := "wait"
flags.BoolVarP(&playOptions.Wait, waitFlagName, "w", false, "Clean up all objects created when a SIGTERM is received or pods exit")

Expand Down
10 changes: 10 additions & 0 deletions docs/source/markdown/podman-kube-play.1.md.in
Original file line number Diff line number Diff line change
Expand Up @@ -221,6 +221,16 @@ Define or override a port definition in the YAML file.
The lists of ports in the YAML file and the command line are merged. Matching is done by using the **containerPort** field.
If **containerPort** exists in both the YAML file and the option, the latter takes precedence.

#### **--publish-all**

Setting this option to `true` will expose all ports to the host,
even if only specified via **containerPort** in the K8 YAML.
In terms of which port will be exposed, **--publish** has higher priority than **hostPort**, has higher priority than
**containerPort**.

If set to `false` (which is the default), only ports defined via **hostPort**
or **--publish** are published on the host.

#### **--quiet**, **-q**

Suppress output information when pulling images
Expand Down
3 changes: 3 additions & 0 deletions pkg/bindings/kube/types.go
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,9 @@ type PlayOptions struct {
Force *bool
// PublishPorts - configure how to expose ports configured inside the K8S YAML file
PublishPorts []string
// PublishAllPorts - whether to publish all ports defined in the K8S YAML file
// (containerPort, hostPort) otherwise only hostPort will be published
PublishAllPorts *bool
// Wait - indicates whether to return after having created the pods
Wait *bool
ServiceContainer *bool
Expand Down
15 changes: 15 additions & 0 deletions pkg/bindings/kube/types_play_options.go

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions pkg/domain/entities/play.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,6 +70,9 @@ type PlayKubeOptions struct {
Force bool
// PublishPorts - configure how to expose ports configured inside the K8S YAML file
PublishPorts []string
// PublishAllPorts - whether to publish all ports defined in the K8S YAML file
// (containerPort, hostPort) otherwise only hostPort will be published
PublishAllPorts bool
// Wait - indicates whether to return after having created the pods
Wait bool
}
Expand Down
2 changes: 1 addition & 1 deletion pkg/domain/infra/abi/play.go
Original file line number Diff line number Diff line change
Expand Up @@ -473,7 +473,7 @@ func (ic *ContainerEngine) playKubePod(ctx context.Context, podName string, podY
Net: &entities.NetOptions{NoHosts: options.NoHosts},
ExitPolicy: string(config.PodExitPolicyStop),
}
podOpt, err = kube.ToPodOpt(ctx, podName, podOpt, podYAML)
podOpt, err = kube.ToPodOpt(ctx, podName, podOpt, options.PublishAllPorts, podYAML)
if err != nil {
return nil, nil, err
}
Expand Down
1 change: 1 addition & 0 deletions pkg/domain/infra/tunnel/kube.go
Original file line number Diff line number Diff line change
Expand Up @@ -73,6 +73,7 @@ func (ic *ContainerEngine) PlayKube(ctx context.Context, body io.Reader, opts en
options.WithStart(start == types.OptionalBoolTrue)
}
options.WithPublishPorts(opts.PublishPorts)
options.WithPublishAllPorts(opts.PublishAllPorts)
options.WithNoTrunc(opts.UseLongAnnotations)
return play.KubeWithBody(ic.ClientCtx, body, options)
}
Expand Down
8 changes: 4 additions & 4 deletions pkg/specgen/generate/kube/kube.go
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ import (
"sigs.k8s.io/yaml"
)

func ToPodOpt(ctx context.Context, podName string, p entities.PodCreateOptions, podYAML *v1.PodTemplateSpec) (entities.PodCreateOptions, error) {
func ToPodOpt(ctx context.Context, podName string, p entities.PodCreateOptions, publishAllPorts bool, podYAML *v1.PodTemplateSpec) (entities.PodCreateOptions, error) {
p.Net = &entities.NetOptions{NoHosts: p.Net.NoHosts}

p.Name = podName
Expand Down Expand Up @@ -82,7 +82,7 @@ func ToPodOpt(ctx context.Context, podName string, p entities.PodCreateOptions,
}
p.Net.AddHosts = hosts
}
podPorts := getPodPorts(podYAML.Spec.Containers)
podPorts := getPodPorts(podYAML.Spec.Containers, publishAllPorts)
p.Net.PublishPorts = podPorts

if dnsConfig := podYAML.Spec.DNSConfig; dnsConfig != nil {
Expand Down Expand Up @@ -1143,14 +1143,14 @@ func getContainerResources(container v1.Container) (v1.ResourceRequirements, err

// getPodPorts converts a slice of kube container descriptions to an
// array of portmapping
func getPodPorts(containers []v1.Container) []types.PortMapping {
func getPodPorts(containers []v1.Container, publishAll bool) []types.PortMapping {
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.

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. 👍

}
if p.Protocol == "" {
Expand Down
33 changes: 33 additions & 0 deletions pkg/specgen/generate/kube/kube_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -44,6 +44,39 @@ func TestParseMountPathRO(t *testing.T) {
assert.NotContains(t, options, "ro")
}

func TestGetPodPorts(t *testing.T) {
c1 := v1.Container{
Name: "container1",
Ports: []v1.ContainerPort{{
ContainerPort: 5000,
}, {
ContainerPort: 5001,
HostPort: 5002,
}},
}
c2 := v1.Container{
Name: "container2",
Ports: []v1.ContainerPort{{
HostPort: 5004,
}},
}
r := getPodPorts([]v1.Container{c1, c2}, false)
assert.Equal(t, 2, len(r))
assert.Equal(t, uint16(5001), r[0].ContainerPort)
assert.Equal(t, uint16(5002), r[0].HostPort)
assert.Equal(t, uint16(5004), r[1].ContainerPort)
assert.Equal(t, uint16(5004), r[1].HostPort)

r = getPodPorts([]v1.Container{c1, c2}, true)
assert.Equal(t, 3, len(r))
assert.Equal(t, uint16(5000), r[0].ContainerPort)
assert.Equal(t, uint16(5000), r[0].HostPort)
assert.Equal(t, uint16(5001), r[1].ContainerPort)
assert.Equal(t, uint16(5002), r[1].HostPort)
assert.Equal(t, uint16(5004), r[2].ContainerPort)
assert.Equal(t, uint16(5004), r[2].HostPort)
}

func TestGetPortNumber(t *testing.T) {
portSpec := intstr.IntOrString{Type: intstr.Int, IntVal: 3000, StrVal: "myport"}
cp1 := v1.ContainerPort{Name: "myport", ContainerPort: 4000}
Expand Down
13 changes: 12 additions & 1 deletion test/e2e/play_kube_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -5425,7 +5425,7 @@ spec:
err := writeYaml(publishPortsPodWithContainerPort, kubeYaml)
Expect(err).ToNot(HaveOccurred())

kube := podmanTest.Podman([]string{"kube", "play", kubeYaml})
kube := podmanTest.Podman([]string{"kube", "play", "--publish-all=true", kubeYaml})
kube.WaitWithDefaultTimeout()
Expect(kube).Should(Exit(125))
// The error message is printed only on local call
Expand All @@ -5434,6 +5434,17 @@ spec:
}
})

It("podman play kube should not publish containerPort by default", func() {
err := writeYaml(publishPortsPodWithContainerPort, kubeYaml)
Expect(err).ToNot(HaveOccurred())

kube := podmanTest.Podman([]string{"kube", "play", kubeYaml})
kube.WaitWithDefaultTimeout()
Expect(kube).Should(Exit(0))
vrothberg marked this conversation as resolved.
Show resolved Hide resolved

testHTTPServer("80", true, "connection refused")
})

It("with privileged containers ports and publish in command line - curl should succeed", func() {
err := writeYaml(publishPortsPodWithContainerPort, kubeYaml)
Expect(err).ToNot(HaveOccurred())
Expand Down
2 changes: 1 addition & 1 deletion test/system/700-play.bats
Original file line number Diff line number Diff line change
Expand Up @@ -532,7 +532,7 @@ EOF
image: $IMAGE
ports:
- name: hostp
containerPort: $HOST_PORT
hostPort: $HOST_PORT
EOF

run_podman kube play $PODMAN_TMPDIR/test.yaml
Expand Down