Support configuring BindAddress and Protocol for a PortMapping
#299
Conversation
twelho
added
kind/api-change
| @@ -9,49 +10,95 @@ import ( | |||
|
|
|||
| // PortMapping defines a port mapping between the VM and the host | |||
| type PortMapping struct { | |||
| HostPort uint64 `json:"hostPort"` | |||
| VMPort uint64 `json:"vmPort"` | |||
| BindAddress net.IP `json:"bindAddress,omitempty"` | |||
There was a problem hiding this comment.
this is optional; I'd prefer for this to be a pointer (although, technically maybe omitempty works here)
There was a problem hiding this comment.
net.IP is a []byte, which can be nil, so omitempty works just fine.
pkg/runtime/docker/client.go
Outdated
| protocol := portMapping.Protocol | ||
| if len(protocol) == 0 { | ||
| // Docker uses TCP by default | ||
| protocol = meta.ProtocolTCP |
There was a problem hiding this comment.
create a const DefaultProtocol = ProtocolTCP in meta?
There was a problem hiding this comment.
The default protocol is runtime implementation specific, for Docker it just happens to be TCP, so I wouldn't make it generic.
| BindAddress net.IP `json:"bindAddress,omitempty"` | ||
| HostPort uint64 `json:"hostPort"` | ||
| VMPort uint64 `json:"vmPort"` | ||
| Protocol Protocol `json:"protocol,omitempty"` |
There was a problem hiding this comment.
I'm think that omitempty doesn't work with custom marshallers, but idk. We can test. If so, then we need to use a pointer here.
There was a problem hiding this comment.
Now that we're using the standard mashaler and Docker's parser always adding the proto, this isn't an issue.
| } | ||
|
|
||
| // PortMappings represents a list of port mappings | ||
| type PortMappings []PortMapping | ||
|
|
||
| var _ fmt.Stringer = PortMappings{} | ||
|
|
||
| var errInvalidPortMappingFormat = fmt.Errorf("port mappings must be of form [<bind address>:]<host port>:<VM port>[/<protocol>]") | ||
|
|
||
| func ParsePortMappings(input []string) (PortMappings, error) { |
There was a problem hiding this comment.
I'm strongly of the opinion that we should not duplicate code here that we already have vendored.
Please use https://github.com/docker/go-connections/blob/master/nat/nat.go#L126 for this task; and turn map[Port][]PortBinding from there into meta.PortMappings.
pkg/apis/meta/v1alpha1/net.go
Outdated
| return string(p) | ||
| } | ||
|
|
||
| func (p Protocol) MarshalJSON() ([]byte, error) { |
There was a problem hiding this comment.
this is not needed; that's the default
pkg/runtime/docker/client.go
Outdated
| @@ -105,8 +106,20 @@ func (dc *dockerClient) AttachContainer(container string) (err error) { | |||
| func (dc *dockerClient) RunContainer(image string, config *runtime.ContainerConfig, name string) (string, error) { | |||
| portBindings := make(nat.PortMap) | |||
There was a problem hiding this comment.
factor this out to a portBindingsToPortMap function please?
|
This doesn't include validation for reading the port mappings declaratively, is that a blocker for merging? I'll add a TODO to implement it anyway as we've not had it until now either. |
Sure thing: #301 |
luxas
removed
do-not-merge/wip
luxas
changed the title
BindAddress and Protocol for a PortMapping
luxas
changed the title
BindAddress and Protocol for a PortMappingBindAddress and Protocol for a PortMapping
Fixes #142. This enables configuring the the listening address on the host and the protocol used when specifying a port binding from both declaratively and from the CLI.
This PR is part of the API changes for v0.5.0.
TODO:
cc @luxas