[vpc-bridge] Support host port mapping for Windows #93
Conversation
Presently, in the `vpc-bridge` plugin, we use HNS V1 APIs to create the HNS network and the HNS endpoint. Once the endpoint is created, we use either V1 or V2 APIs for attaching the endpoint to the container. Given that V2 is the way forward where Microsoft supports newer features, we are converting our plugins to be V2 first. In this commit, we make the following changes- - We use HNS V2 to create the network - We use HNS V2 to create the HNS endpoint After that, we will use either V1 or V2 to attach the endpoint to our container/namespace depending upon the runtime.
|
Very interested in this - was somewhat surprised to discover that it wasn't already available on Windows on EKS, though it is understandable. Making this functionality work should enable using Agones on Windows nodes in EKS, as that makes fairly heavy use of HostPorts to expose game servers. |
| HostPort int `json:"hostPort"` | ||
| ContainerPort int `json:"containerPort"` | ||
| Protocol string `json:"protocol"` | ||
| HostIP string `json:"hostIP,omitempty"` |
There was a problem hiding this comment.
Where is "HostIP" used? I don't see any references to it.
There was a problem hiding this comment.
There was a problem hiding this comment.
We can add it later if we need to.
Also please make Protocol the first field.
There was a problem hiding this comment.
Yes sure. I will then omit this field from the config for the time being.
| log.Infof("Creating HNS network: %+v", hnsRequest) | ||
| hnsResponse, err := hcsshim.HNSNetworkRequest("POST", "", hnsRequest) | ||
| log.Infof("Creating HNS network: %+v", hnsNetwork) | ||
| hnsResponse, err := hnsNetwork.Create() |
There was a problem hiding this comment.
Switching to HCN (v2) is a major change. There may be various behavioral differences between v1 and v2 that are hard to notice just by reading this CR. This is just a reminder to ensure proper test coverage for all ECS and EKS supported OS versions and scenarios before merging.
network/vpc/port.go
Outdated
| // protocolTcp indicates tcp protocol number for port mapping. | ||
| protocolTcp = 6 | ||
| // protocolUdp indicates udp protocol number for port mapping. | ||
| protocolUdp = 17 |
There was a problem hiding this comment.
These are not port definitions and therefore shouldn't be in port.go.
If you want to keep them and the ProtocolToNumber function in the vpc package, maybe create a new protocol.go and put them there? Also go convention is to keep acronyms uppercase as in "protocolTCP".
There was a problem hiding this comment.
Yes sure. I will create a new file named protocol.go and refactor the methods there.
Presently, the vpc-bridge plugin silently ignores any port mapping between container to host for the configured endpoint. As per the current CNI convention, plugins can request that the runtime insert this dynamic configuration by explicitly listing their capabilities in the network configuration. Dynamic information (i.e. data that a runtime fills out) should be placed in a runtimeConfig section. Reference: https://www.cni.dev/docs/conventions/#dynamic-plugin-specific-fields-capabilities--runtime-configuration `portMappings` is one such capability. This change adds the support for creating NAT port mappings between container and host ports as configured by the runtime.
Summary
vpc-bridgeCNI plugin (previously known asvpc-shared-eni) is used for networking on Windows nodes running in EKS. Presently, we do not support container to host port mapping using this CNI plugin. Therefore,vpc-bridgeplugin silently ignores any port mapping between container to host for the configured endpoint.As per the CNI convention, plugins can request that the runtime insert this dynamic configuration by explicitly listing their capabilities in the network configuration. Dynamic information (i.e. data that a runtime fills out) should be placed in a runtimeConfig section.
Reference: https://www.cni.dev/docs/conventions/#dynamic-plugin-specific-fields-capabilities--runtime-configuration
portMappingsis one such capability.In order to support host port usage, we need to add the appropriate policy during the HNS Endpoint creation.
However, as communicated in microsoft/Windows-Containers#360, we need to use HNS V2 for creating this endpoint policy. Therefore in this PR, we will enhance the
vpc-bridgeCNI plugin to predominantly useHNS V2and also add support for host port mappings.Description of changes:
we are making the following changes-
vpc-bridgeCNI plugin to be HNS V2 first. This means that we will only useHNS V1APIs for attaching the endpoint to the container when usingdockerruntime.RuntimeConfigusing which CNI plugin can obtain the configured port mappingBy submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.