daemon: return port-mappings from all endpoints #49657
Conversation
The function `getEndpointPortMapInfo` is called by `updateJoinInfo` to update the field `NetworkSettings.Ports` of a container. However, `updateJoinInfo` is only called by `connectToNetwork` which is also calling `getPortMapInfo` (which in turn calls `getEndpointPortMapInfo`). So, remove the call to `getEndpointPortMapInfo` from `updateJoinInfo`. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
26e8aad to
fa124ab
Compare
daemon/network.go
Outdated
| natPort, err := nat.NewPort(tp.Proto.String(), strconv.Itoa(int(tp.Port))) | ||
| if err != nil { | ||
| return pm, fmt.Errorf("Error parsing Port value(%v):%v", tp.Port, err) | ||
| if containertypes.NetworkMode(ep.Network()).IsBridge() { |
There was a problem hiding this comment.
To be honest, I feel like this is a bit crappy as this is going to cause issues when we'll add support for DNS-based links to custom networks. But this is the shortest path to get this bug fixed in v28.0.2.
There was a problem hiding this comment.
Should we add some TODO comment? 🙈
There was a problem hiding this comment.
Maybe… I think one part of the problem here is that we're using a single variable to report two distinct concepts (ie. 'exposed ports' and 'published ports').
Currently you can't connect a container to the default bridge and to a custom network, so a container can't have port-mappings provided by two networks and legacy links at the same time.
But once we have DNS-based links available, this will become possible and we'll realize that 'exposed ports' should be an endpoint property. So, maybe it's not worth adding a TODO for a block of code which will be deleted without getting fixed?
There was a problem hiding this comment.
I think only creating the entry if there's not already a mapping would preserve the existing behaviour for this patch release? ...
if _, ok := pm[natPort]; !ok {
pm[natPort] = nil
}
fa124ab to
4d62124
Compare
0e22e8a to
adba758
Compare
| defer network.RemoveNoError(ctx, t, apiClient, nwV6) | ||
|
|
||
| ctrID := ctr.Run(ctx, t, apiClient, | ||
| ctr.WithExposedPorts("80/tcp"), |
There was a problem hiding this comment.
Could add another exposed port with no host port mapping, to check it shows up?
adba758 to
cef6b95
Compare
Instead, log the error returned by `nat.NewPort` and move on to the next port mapping / exposed port. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
With improved IPv6 support, a dual-stack container can map a port using two different networks -- one IPv4-only, the other IPv6-only. The daemon was updating containers' `EndpointSettings.Ports` by looking for the first network providing port-mappings. This was incorrect. Instead, iterate over the whole list of endpoints, and merge everything together. The function doing that, ie. `getEndpointPortMapInfo`, is also considered exposed ports, and nil the PortMap entry if an exposed port is found. However, exposed ports are always set on a bridge network, so this was erasing port-mappings found for other networks. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
cef6b95 to
f2a183a
Compare
- What I did
First two commits are small refactorings.
The third commit addresses the issue described in #49654.
daemon: remove redundant call to getEndpointPortMapInfo
daemon: getEndpointPortMapInfo: err is never used
daemon: return port-mappings from all endpoints
With improved IPv6 support, a dual-stack container can map a port using two different networks -- one IPv4-only, the other IPv6-only.
The daemon was updating containers'
EndpointSettings.Portsby looking for the first network providing port-mappings. This was incorrect.Instead, iterate over the whole list of endpoints, and merge everything together.
The function doing that, ie.
getEndpointPortMapInfo, is also considered exposed ports, and nil the PortMap entry if an exposed port is found. However, exposed ports are always set on a bridge network, so this was erasing port-mappings found for other networks. Instead, do not consider exposed ports unless the current network is the default bridge.- How to verify it
A new integration test is added.
- Human readable description for the release notes