From 0fd0e8255f3bc0c660abe6520c1e2fdf9fc187c2 Mon Sep 17 00:00:00 2001 From: Albin Kerouanton Date: Tue, 21 Nov 2023 16:51:51 +0100 Subject: [PATCH] daemon: build ports-related ep options in a dedicated func The `buildCreateEndpointOptions` does a lot of things to build the list of `libnetwork.EndpointOption` from the `EndpointSettings` spec. To skip ports-related options, an early return was put in the middle of that function body. Early returns are generally great, but put in the middle of a 150-loc long function that does a lot, they're just a potential footgun. And I'm the one who pulled the trigger in 052562f. Since this commit, generic options won't be applied to endpoints if there's already one with exposed/published ports. As a consequence, only the first endpoint can have a user-defined MAC address right now. Instead of moving up the code line that adds generic options, a better change IMO is to move ports-related options, and the early-return gating those options, to a dedicated func to make `buildCreateEndpointOptions` slightly easier to read and reason about. There was actually one oddity in the original `buildCreateEndpointOptions`: the early-return also gates the addition of `CreateOptionDNS`. These options are Windows-specific; a comment is added to explain that. But the oddity is really: why are we checking if an endpoint with exposed / published ports joined this sandbox to decide whether we want to configure DNS server on the endpoint's adapter? Well, this early-return was most probably overlooked by the original author and by reviewers at the time these options were added (in commit d1e0a78) Let's fix that in a follow-up commit. Signed-off-by: Albin Kerouanton --- daemon/network.go | 46 +++++++++++++++++++++++++++++++++++----------- 1 file changed, 35 insertions(+), 11 deletions(-) diff --git a/daemon/network.go b/daemon/network.go index b93043083b..897b898ad7 100644 --- a/daemon/network.go +++ b/daemon/network.go @@ -862,11 +862,43 @@ func buildCreateEndpointOptions(c *container.Container, n *libnetwork.Network, e createOptions = append(createOptions, libnetwork.CreateOptionDisableResolution()) } + opts, err := buildPortsRelatedCreateEndpointOptions(c, n, sb) + if err != nil { + return nil, err + } + createOptions = append(createOptions, opts...) + + // On Windows, DNS config is a per-adapter config option whereas on Linux, it's a sandbox-wide parameter; hence why + // we're dealing with DNS config both here and in buildSandboxOptions. Following DNS options are only honored by + // Windows netdrivers, whereas DNS options in buildSandboxOptions are only honored by Linux netdrivers. + // + // Now that being said, you might ask: why is this cond checking whether there's already an endpoint with exposed / + // published ports tied to the container sandbox? Isn't that logic flawed? Well, probably it is! These DNS options + // were added by d1e0a78 at the end of buildCreateEndpointOptions. The fact that it was added *after* an + // early-return checking exposed / published ports was most probably overlooked by the original author and + // reviewers. + // TODO(aker): fix this ^ + if !n.Internal() && len(getPortMapInfo(sb)) == 0 { + if len(c.HostConfig.DNS) > 0 { + createOptions = append(createOptions, libnetwork.CreateOptionDNS(c.HostConfig.DNS)) + } else if len(daemonDNS) > 0 { + createOptions = append(createOptions, libnetwork.CreateOptionDNS(daemonDNS)) + } + } + + createOptions = append(createOptions, libnetwork.EndpointOptionGeneric(genericOptions)) + + return createOptions, nil +} + +// buildPortsRelatedCreateEndpointOptions returns the appropriate endpoint options to apply config related to port +// mapping and exposed ports. +func buildPortsRelatedCreateEndpointOptions(c *container.Container, n *libnetwork.Network, sb *libnetwork.Sandbox) ([]libnetwork.EndpointOption, error) { // Port-mapping rules belong to the container & applicable only to non-internal networks. // // TODO(thaJeztah): Look if we can provide a more minimal function for getPortMapInfo, as it does a lot, and we only need the "length". if n.Internal() || len(getPortMapInfo(sb)) > 0 { - return createOptions, nil + return nil, nil } bindings := make(nat.PortMap) @@ -927,18 +959,10 @@ func buildCreateEndpointOptions(c *container.Container, n *libnetwork.Network, e } } - if len(c.HostConfig.DNS) > 0 { - createOptions = append(createOptions, libnetwork.CreateOptionDNS(c.HostConfig.DNS)) - } else if len(daemonDNS) > 0 { - createOptions = append(createOptions, libnetwork.CreateOptionDNS(daemonDNS)) - } - - createOptions = append(createOptions, + return []libnetwork.EndpointOption{ libnetwork.CreateOptionPortMapping(publishedPorts), libnetwork.CreateOptionExposedPorts(exposedPorts), - libnetwork.EndpointOptionGeneric(genericOptions)) - - return createOptions, nil + }, nil } // getPortMapInfo retrieves the current port-mapping programmed for the given sandbox