- Notifications
You must be signed in to change notification settings - Fork1k
fix(agent/agentcontainers): return host port instead of container port#16866
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| // 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) | ||
| iflen(bindings)==0 { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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 | ||
| iflen(parts)==2 { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Since this is output fromdocker inspect, we should probably assume len == 2 and anything else is an unexpected state.
Alt: why did we get rid of theconvertDockerPort function when it was more complete?
| Port:uint16(hostPort), | ||
| }) | ||
| // Mark this host port as seen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| // Mark this host port as seen |
| // Process all bindings for this port (usually different interfaces) | ||
| for_,binding:=rangebindings { | ||
| // Skip if we've already seen this host port |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| // Skip if we've already seen this host port |
| Port:uint16(hostPort), | ||
| }) | ||
| // Mark this host port as seen |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
| {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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
Nit: Mentioned earlier, but is this valid output ofdocker inspect?
Closing in favour of#16887 |
Uh oh!
There was an error while loading.Please reload this page.
We're currently returning the port mapped inside the container instead of the host port, which is what the agent is able to access.