Allow users to expose ports from the pod to the host #1829
Conversation
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: baude 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 |
openshift-ci-robot
added
the
approved
|
|
||
| if !c.BoolT("infra") && len(c.StringSlice("publish")) > 0 { | ||
| return errors.Errorf("you must have an infra container to publish port bindings to the host") | ||
| } |
There was a problem hiding this comment.
could you also add an error when we are in rootless mode? Exposing port from the host is not currently supported for rootless containers.
baude
force-pushed
the
enableportbindinginpods
branch
from
855d5c6
to
b2ab6d6
Compare
cmd/podman/pod_create.go
Outdated
| @@ -2,12 +2,14 @@ package main | |||
|
|
|||
| import ( | |||
| "fmt" | |||
| "github.com/containers/libpod/pkg/rootless" | |||
libpod/pod.go
Outdated
| @@ -96,7 +97,8 @@ type PodContainerInfo struct { | |||
|
|
|||
| // InfraContainerConfig is the configuration for the pod's infra container | |||
| type InfraContainerConfig struct { | |||
| HasInfraContainer bool `json:"makeInfraContainer"` | |||
| HasInfraContainer bool `json:"makeInfraContainer"` | |||
| PortBindings map[nat.Port][]nat.PortBinding `json:"infraPortBindings"` | |||
There was a problem hiding this comment.
Can we make these match the CNI structs that Libpod uses? So we can move the heavy lifting around ports into cmd/podman and turn CreatePortBindings into a straight assignment when we make the pod container
we need to allow users to expose ports to the host for the purposes of networking, like a webserver. the port exposure must be done at the time the pod is created. strictly speaking, the port exposure occurs on the infra container. Signed-off-by: baude <bbaude@redhat.com>
baude
force-pushed
the
enableportbindinginpods
branch
from
b2ab6d6
to
690c52a
Compare
|
LGTM |
|
LGTM, thanks @baude |
| "strings" | ||
|
|
||
| "github.com/containers/libpod/cmd/podman/libpodruntime" | ||
| "github.com/containers/libpod/cmd/podman/shared" | ||
| "github.com/containers/libpod/libpod" | ||
| "github.com/containers/libpod/pkg/rootless" | ||
| "github.com/cri-o/ocicni/pkg/ocicni" |
There was a problem hiding this comment.
Late to the game. Is there a reason why github.com/cri-o/ocicni/pkg/ocicni is not part of github.com/containers (or ideally a package of libpod)? Would be nice for CI and for consolidating packages.
|
Nice PR! |
|
That is a good question (re: ocicni location). It originally lived in the
CRI-O repo until someone requested it be split out for easy vendoring in
other projects. I wouldn't mind seeing it under the containers umbrella.
…On Wed, Nov 21, 2018, 03:00 Valentin Rothberg ***@***.*** wrote:
Nice PR!
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#1829 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AHYHCLJRaDSYO8zJCmOmsDNbqrBXIqWNks5uxQgLgaJpZM4YpwlU>
.
|
|
SGTM. I think pulling it into libpod and then having CRI-O vendoring matches our long term goals. |
github-actions
bot
added
the
locked - please file new issue/PR
we need to allow users to expose ports to the host for the purposes
of networking, like a webserver. the port exposure must be done at
the time the pod is created.
strictly speaking, the port exposure occurs on the infra container.
Signed-off-by: baude bbaude@redhat.com