daemon: backfill empty PBs slices for backward compat#50874
daemon: backfill empty PBs slices for backward compat#50874thaJeztah merged 1 commit intomoby:masterfrom
Conversation
| } | ||
|
|
||
| if versions.Equal(version, "1.52") { | ||
| warning = "Starting with API 1.53, empty list of port bindings will be discarded" |
There was a problem hiding this comment.
Name the offending binding portProto(s) in the warning?
|
Just a note for future reference: the go-connections bug this PR is emulating has been (unintentionally?) fixed in docker/go-connections#147 |
So far, on ContainerStart, the daemon was silently backfilling empty PortBindings slices with a PortBinding with unspecified HostIP and HostPort. This was done by github.com/docker/go-connections/nat.SortPortMap. This backfilling doesn't make much sense, and we're trying to remove that package. So, move the backfilling to the API server, keep it for older API versions, deprecate it for API 1.52, and drop it for API 1.53 and above. Signed-off-by: Albin Kerouanton <albinker@gmail.com>
5208655 to
0ca7ac3
Compare
| } | ||
|
|
||
| if versions.Equal(version, "1.52") { | ||
| warning = fmt.Sprintf("Container port %s has an empty list of port-bindings. Starting with API 1.53, this will be discarded.", portProto) |
There was a problem hiding this comment.
If multiple port bindings trigger the warning, why only list the last one iterated upon?
for ... {
if versions.Equal(version, "1.52") {
warningPorts = append(warningPorts, portProto)
}
}
if len(warningPorts) > 0 {
return fmt.Sprintf("One or more container ports has an empty list of port-bindings. Starting with API 1.53, the bindings for %s will be discarded.", strings.Join(warningPorts, ", "))
}
return ""
Ah, yes, I think @akerouanton or @austinvazquez mentioned that I "brixed" (fixed, but because of that broke) something. Some of the code in that repo caused me some head-scratching, and no tests failed, so 🤷 |
|
Oh! I see Cory's last comment was posted after his review; I guess that one can be addressed a follow-up though (sorry!) |
@thaJeztah, IDK if you ever had a chance to look too deeply into it, but it's kinda funny because the optimization changed a field from a struct to a pointer and therefore changing behavior due to the difference in zero values. Although I don't really blame the change because it was an untested, unintentional functionality and sorting function changing the state was weird anyways. 🤭 |
- What I did
So far, on ContainerStart, the daemon was silently backfilling empty PortBindings slices with a PortBinding with unspecified HostIP and HostPort. This was done by github.com/docker/go-connections/nat.SortPortMap.
This backfilling doesn't make much sense, and we're trying to remove that package. So, move the backfilling to the API server, keep it for older API versions, deprecate it for API 1.52, and drop it for API 1.53 and above.
- Human readable description for the release notes