That's the cause of this CI failure:

=== FAIL: arm64.docker.docker.integration.network.bridge TestAllPortMappingsAreReturned (1.84s)
bridge_linux_test.go:557: assertion failed:
--- inspect.NetworkSettings.Ports
+++ →
map[container.PortProto][]container.PortBinding{
"80/tcp": {{HostIP: "0.0.0.0", HostPort: "8000"}, {HostIP: "::", HostPort: "8000"}},
+ "81/tcp": nil,
}

inspect.NetworkSettings.Ports is supposed to contain both currently-mapped ports, and exposed but unmapped ports. Some people / software are depending on this, so we can't change that.

FWIW here's how that test can be reproduced with plain docker commands:

$ DOCKER_USERLANDPROXY=false ./hack/make.sh binary install-binary run
$ docker network create --ipv4=true --ipv6=false testnetv4
$ docker network create --ipv4=false --ipv6=true testnetv6
$ docker run --rm -d -p 8000:80/tcp --expose 81/tcp --network testnetv4 --network testnetv6 busybox httpd -f
$ docker ps
CONTAINER ID   IMAGE                           COMMAND                  CREATED              STATUS              PORTS                                     NAMES
02fbc1e14bb4   busybox                         "httpd -f"               About a minute ago   Up About a minute   0.0.0.0:8000->80/tcp, [::]:8000->80/tcp   pedantic_lalande
$ docker inspect 02fbc1e14bb4

In inspect output, there should be two keys under NetworkSettings.Ports (80/tcp and 81/tcp) but there's only one when running these commands against this PR.

Thanks @akerouanton that saved me lots of debugging time. Trying out a change to restore the behavior for exposed ports but unmapped ports in network settings.

Ah, yes I think this was one of my quick hacks to make it work; possibly we no longer need it if we have a version of go-connections that aliases things (and/or has docker/go-connections#146)

For some of these we need to look if we REALLY need the utilities from go-connections or if we should have utilities more specific to the use at hand; I think there's various places where the daemon code has to do multiple conversions that are not always needed, but just because it was the only way to do it with the tools in go-connections.

The only interesting bit from nat.SortPortMap() is that it creates 'dummy' entries (i.e. nil slices) for un-mapped but exposed ports. People / software are depending on that it seems, so we can't break it.

Otherwise, the sorting itself is not needed. I guess at some point it was influencing the sorting of NetworkSettings.Ports but: 1. if that was the original intent, it should have been done in (*Daemon).ContainerInspect(), not here; 2. since commit 043db3b (released in v27.0), the bridge driver has its own sorting logic to fit its needs.

@akerouanton, @thaJeztah, wanted to get your thoughts here. Through test/debugging I found the issue with migrating away from nat.SortPortMap is it would create a default PortBinding for any ports with no specified port binding.

it would create a default PortBinding for any ports with no specified port binding.

You mean for any Exposed ports with no specified port binding? I believe this is the current behavior 🤔

If I'm misunderstanding your comment, could you please include a diff of before / after removing nat.SortPortMap?

@akerouanton, below is some before/afters from debug code and running network/bridge.TestPortMappingRestore.

This is the bindings map before these changes before/after sorting.

bindings before sorting: map[80/tcp:[]]
bindings after sorting: map[80/tcp:[{ }]]

This is the portBindings slice with my changes before/after the new exposed port handling logic.

portBindings before exposed ports: map[80/tcp:[]]
portBindings after exposed ports: map[80/tcp:[{HostIP: HostPort:}]]

The piece to note is the empty struct after sorting.

Let me summurize my findings here so that we can easily link to it in the fix.

nat.SortPortMap is backfilling an empty PortBinding into the PortBindings slice if it's empty. That sort function is called during ContainerStart. So, containers created with empty PortBindings would have something like that stored on-disk:

containertypes.PortMap{"80/tcp": {}}

But it'd become something like that after a ContainerStart:

containertypes.PortMap{"80/tcp": {{HostIP: "", HostPort: 0}}}

This doesn't make much sense, and it's not clear whether this was intentional or just a bug that slipped through.

From an API standpoint, it'd be less surprising if an empty PortBindings slice was the equivalent of no containertypes.PortMap{} entry.

We can't drop this behavior unceremoniously for older API clients that (unknowingly) rely on that behavior, but we can move that backfilling to the API layer, return a warning for API 1.52, and drop it in 1.53. We also need to take care of correctly backfilling containers stored on-disk as they might have been created with older Engine versions.

Once done, we'll be able to remove the backfilling done here.

it would create a default PortBinding for any ports with no specified port binding.

You mean for any Exposed ports with no specified port binding? I believe this is the current behavior 🤔

If I'm misunderstanding your comment, could you please include a diff of before / after removing nat.SortPortMap?

What's the reason for this? An empty PortBinding is treated as a port binding requesting an ephemeral port to be bound on unspecified addrs (i.e. 0.0.0.0 and ::). So, I think we should not add an empty PortBinding here, but maybe I'm missing something.

@austinvazquez This still needs to be fixed 😉

Sorry about that. I slightly misunderstood #50874 to mean we needed to keep this logic for one more release. Now I understand we can remove it and the issue will be handled (and removed) higher in the call stack.

@corhere , I know we discussed keeping the protocol validation on the server side to minimize the interop issues with older clients and newer servers, but I assume alphanumeric check here is somewhat future-proof. Mainly I was trying to handle the test case ParsePort("0/tcp/").

I would not mind if ParsePort("0/tcp/") == Port{proto: unique.Make("tcp/")}. And I don't think it would be wise to restrict ourselves to alphanumerics anyway. We might have a good reason to encode more than just an L4 protocol name into the proto part of the Port. Some hypothetical examples: "443/tcp+tls", "8000/udp+ipsec", "8080/tcp:ipv6only".

Also, consider the PortFrom function. If only alphanumeric protocol names are permitted, the constructor will need to validate the proto parameter and report an error if invalid. It's just simpler all around to allow any nonempty string as the protocol and let the daemon reject protocols it does not recognize.

I would not mind if ParsePort("0/tcp/") == Port{proto: unique.Make("tcp/")}. And I don't think it would be wise to restrict ourselves to alphanumerics anyway. We might have a good reason to encode more than just an L4 protocol name into the proto part of the Port. Some hypothetical examples: "443/tcp+tls", "8000/udp+ipsec", "8080/tcp:ipv6only".

Also, consider the PortFrom function. If only alphanumeric protocol names are permitted, the constructor will need to validate the proto parameter and report an error if invalid. It's just simpler all around to allow any nonempty string as the protocol and let the daemon reject protocols it does not recognize.

I am now second-guessing how these cases are handled. I can't think of a good justification to explain why some edge case inputs result in a zero-value PortRange while others result in an error. It also violates the Go convention where err == nil (or ok == true) signals that the other result parameters are meaningful.

One possibility would be to implicitly normalize the port numbers: if end < start { start, end = end, start }. That would allow us to make PortRangeFrom consistent with PortFrom, returning the zero value iff the protocol is empty, and eliminating the need for an error result parameter. But that would be inconsistent with ParsePortRange, and I am having a hard time justifying why PortRangeFrom(2, 1, TCP) is acceptable but ParsePortRange("2-1/tcp") is not.

Do we simply drop the error result, returning PortRange{} when end < start || proto == ""? That doesn't seem right either, as that would marshal to the empty string.

Perhaps we're just overthinking things. What if we split the difference and return ok bool, like netip.AddrFromSlice?

// PortFrom returns a Port with the given number and protocol.
//
// If no protocol is specified (i.e. proto == ""), then PortFrom returns Port{}, false.
func PortFrom(num uint16, proto PortProto) (p port, ok bool)

// PortRangeFrom returns a [PortRange] with the given start and end port numbers and protocol.
//
// If end < start or no protocol is specified (i.e. proto == ""), then PortRangeFrom returns PortRange{}, false.
func PortRangeFrom(start, end uint16, proto PortProto) (r PortRange, ok bool)

It's not like the error string is particularly informative apart from its nilness. Returning multiple values forces the caller to acknowledge that the constructor is not guaranteed to return a valid value for all inputs, and to handle (or explicitly ignore) the unhappy path.

It is notable that netip.PrefixFrom has a single result parameter despite not being guaranteed to return a valid value for all inputs either. It returns an invalid but nonzero value when the bits parameter is out of range. I do not think that such behaviour would make sense for either of our PortFrom or PortRangeFrom constructors. A netip.Prefix wraps a netip.Addr; even if the prefix length is bogus the IP address still could be meaningful. There is value in encoding and passing around such invalid Prefixes. In contrast, when end < start in a PortRangeFrom call the only possibly well-formed piece of information is the protocol, which unlike the Addr of an invalid Prefix, is not meaningful out of context. Since the caller of PortFrom or PortRangeFrom is expected to get a well-formed PortRange as the result, the zero value is not a valid result for the function. Therefore returning a single result that may be zero-valued would be considered in-band signalling, which is not best practice.

In summary, my recommendation would be to have both PortFrom and PortRangeFrom return an ok bool result parameter.

Okay yeah I think I can get behind that. Even when authoring the error solution, it didn't feel as nice as the PortFrom implementation. +1 to the argument and reference to netip.AddrFromSlice. At least we can give a consistent experience across Port(Range)From functions while forcing the user to acknowledge the potential invalid value. Still brings most of the value of a compile time check.

It would be really slick if we provided a convenience method to iterate over the ports of a range.

func (pr PortRange) All() iter.Seq[Port] {
	return func(yield func(Port) bool) {
		for i := pr.Start(); i <= pr.End(); i++ {
			if !yield(Port{num: i, proto: pr.proto}) {
				return
			}
		}
	}
}

Chatted a bit offline but doc'ing here as well. While unit testing I found it is easy to make the mistake where the iter variable is typed to uint16 and the increment can cause an infinite loop when start or end port is max value. More over proving the need for All() function.

PortProto sounds like a weird name to me. Every time I see it I instinctively think that it's a union of a port number and a proto, whereas it's just a proto. Maybe we can rename that type into Proto 🤔

@akerouanton , It's a little challenging because the type is actually part of the container package so container.Protocol is a little strange as well. How about NetworkProtocol?

Every time normalizePortProto is called, its output is passed to unique.Make. Maybe we can internalize this into normalizePortProto?

nit: could add a doc string

Just curious - as the API module already requires "gotest.tools/v3", could these tests use it? For example ...

		assert.Check(t, p.IsZero())
		assert.Check(t, !p.IsValid())
		assert.Check(t, is.Equal(p.String(), "invalid port"))

nit: the protos are different, so at-most one can be TCP - so I don't think c is needed? ...

	if ip.Proto() == container.TCP {
		return -1
	}
	if jp.Proto() == container.TCP {
		return 1
	}

Micro-nit; the / is optional, but required when specifying a protocol; probably <portnum>[/<proto>] (or even <portnum>[</proto>] is more accurate. That last one is probably confusing though as it looks like a closing "html" tag 😂

Silly question; do we want <port>/ (trailing slash, but no proto) to be considered valid, or (also see above) make it only accepted if used together with a proto?

Possibly fine for a follow-up, but I noticed we define a type in types/swarm as well, so in som places we need to convert one to the other; wondering if we need to alias one to the other (or have an internal type that's used as alias for both);

I guess this answers my question whether we consider a trailing / to be OK; was this needed for backward compatibility, or something we want to endorse as a whole? (is this something we could get out of somehow?)

(thinking along the lines of fixing up before we parse for old API versions, to allow us to be more strict in future 🤔 - but not sure if we have that opportunity)

Oh, wait; was this one new? (the :ipv6only suffix)? Or was this already there?

Mostly looking at the use of : for separator here, because ... we have a lot of colons already in our formats (consider [<hostip>[<port or range>]]:[<port or range>][/<proto>][:ipv6only], where <hostip> may be IPv6.

If itt's new, would tcp6, udp6 (e.g.) be an option?

It's an invented hypothetical to test that the parser accepts protocols with non-alphabetic characters.

The only thing I'm eyeing is the use of uint16 in some places; while "correct" (as in it's he range accepted for ports), they also tend to be slightly awkward to use at times (e.g. a strconv.Itoa won't work).

But of course, it depends on how it's used; for this one, as it's constructing the Port, it means the caller already must've done' half the work (make sure the port-number is of the right type and range); I haven't looked yet how / where it's used and if that currently requires additional handling in callers 🤔

Correct. ParsePort is available for users who don't have a good reason to do half the work themselves.

Slightly related to the above (also depend on how it's used).

(Also still hate that net.JoinHostPort requires a string for the port 😠 😂 )