fix(agent/agentcontainers): return host port instead of container port #16866
Conversation
There was a problem hiding this comment.
I'm a bit concerned about the potential disconnect between parsing the docker inspect and deciding where the port should map to. Right now we're discarding half of the information. But one question I have is what happens during the actual port forwarding if, for example, this is our JSON:
docker run -it --rm -p :::8181:8181 -p 127.0.0.1:8181:8182 alpine "Ports": {
"8181/tcp": [
{
"HostIp": "::",
"HostPort": "8181"
}
],
"8182/tcp": [
{
"HostIp": "127.0.0.1",
"HostPort": "8181"
}
]
}In this case, host port 8181 is exposed, but it could go to either 8181 or 8182 in the container.
| // Parse the containerPortSpec (e.g., "8080/tcp") | ||
| parts := strings.Split(containerPortSpec, "/") | ||
|
|
||
| // Get the network protocol |
There was a problem hiding this comment.
| // Get the network protocol |
Nit: Comment why not what. What is inferable from the code.
| // Skip if no bindings exist (don't expose the container port) | ||
| if len(bindings) == 0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Nit: This check seems like it's not needed, we're iterating on the map keys so technically this can't happen.
|
|
||
| // Get the network protocol | ||
| network := "tcp" // Default to tcp | ||
| if len(parts) == 2 { |
There was a problem hiding this comment.
Since this is output from docker inspect, we should probably assume len == 2 and anything else is an unexpected state.
Alt: why did we get rid of the convertDockerPort function when it was more complete?
| }) | ||
|
|
||
| // Mark this host port as seen |
There was a problem hiding this comment.
| // Mark this host port as seen |
|
|
||
| // Process all bindings for this port (usually different interfaces) | ||
| for _, binding := range bindings { | ||
| // Skip if we've already seen this host port |
There was a problem hiding this comment.
| // Skip if we've already seen this host port |
| }) | ||
|
|
||
| // Mark this host port as seen |
There was a problem hiding this comment.
| // Mark this host port as seen |
| networkPorts: map[string][]dockerPortBinding{ | ||
| "8080/tcp": { | ||
| {HostIP: "0.0.0.0", HostPort: "9090"}, | ||
| {HostIP: "127.0.0.1", HostPort: "9090"}, |
There was a problem hiding this comment.
| {HostIP: "127.0.0.1", HostPort: "9090"}, | |
| {HostIP: "127.0.0.1", HostPort: "9090"}, | |
| {HostIP: "::", HostPort: "9090"}, |
For completeness, let's include IPv6
| name: "invalid port", | ||
| in: "invalid/tcp", | ||
| expectError: "invalid port", | ||
| name: "no bindings should not create ports", |
There was a problem hiding this comment.
Nit: Can this actually happen? If not, perhaps we could mention it's only here for completeness?
| expectNetwork: "tcp", | ||
| name: "no protocol defaults to tcp", | ||
| networkPorts: map[string][]dockerPortBinding{ | ||
| "8080": { |
There was a problem hiding this comment.
Nit: Mentioned earlier, but is this valid output of docker inspect?
|
Closing in favour of #16887 |
We're currently returning the port mapped inside the container instead of the host port, which is what the agent is able to access.