Skip to content

libnet/d/bridge: Don't set container's gateway when network is internal#46603

Merged
thaJeztah merged 2 commits intomoby:masterfrom
akerouanton:libnet-bridge-internal
Oct 11, 2023
Merged

libnet/d/bridge: Don't set container's gateway when network is internal#46603
thaJeztah merged 2 commits intomoby:masterfrom
akerouanton:libnet-bridge-internal

Conversation

@akerouanton
Copy link
Member

Related to:

- What I did

So far, internal networks were only isolated from the host by iptables DROP rules. As a consequence, outbound connections from containers would timeout instead of being "rejected" through an immediate ICMP dest/port unreachable, a TCP RST or a failing connect syscall.

This was visible when internal containers were trying to resolve a domain that don't match any container on the same network (be it a truly "external" domain, or a container that don't exist/is dead). In that case, the embedded resolver would try to forward DNS queries for the different values of resolv.conf search option, making DNS resolution slow to return an error, and the slowness being exacerbated by some libc implementations.

This change makes connect syscall to return ENETUNREACH, and thus solves the broader issue of failing fast when external connections are attempted.

- How to verify it

CI is green.

- Description for the changelog

  • Make connect syscall fail fast when a container is only attached to an internal network.

- A picture of a cute animal (not mandatory but encouraged)

So far, internal networks were only isolated from the host by iptables
DROP rules. As a consequence, outbound connections from containers would
timeout instead of being "rejected" through an immediate ICMP dest/port
unreachable, a TCP RST or a failing `connect` syscall.

This was visible when internal containers were trying to resolve a
domain that don't match any container on the same network (be it a truly
"external" domain, or a container that don't exist/is dead). In that
case, the embedded resolver would try to forward DNS queries for the
different values of resolv.conf `search` option, making DNS resolution
slow to return an error, and the slowness being exacerbated by some libc
implementations.

This change makes `connect` syscall to return ENETUNREACH, and thus
solves the broader issue of failing fast when external connections are
attempted.

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
@akerouanton akerouanton force-pushed the libnet-bridge-internal branch from 690c308 to cbc2a71 Compare October 9, 2023 11:58
func setupBridgeIPv4(config *networkConfiguration, i *bridgeInterface) error {
// TODO(aker): the bridge driver panics if its bridgeIPv4 field isn't set. Once bridge subnet and bridge IP address
// are decoupled, we should assign it only when it's really needed.
i.bridgeIPv4 = config.AddressIPv4
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the bridge driver should rely less on this field and only have a mandatory subnet. Bridge and gateway addresses should be optional. @corhere WDYT?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config.AddressIPv4 and i.bridgeIPv4 are of type *net.IPNet. That is the data type for an IP subnet. When not in canonical form (i.e. the host-part bits are not all zero) it can also pull double duty to also encode a particular address within the subnet. (The same is also true for netip.Prefix.) Since the bridge's external-facing IP address has to be within the bridge subnet anyway, I don't see the need to encode just the subnet in a separate field. It would just be bridgeIPv4 in canonical form.

@akerouanton
Copy link
Member Author

akerouanton commented Oct 9, 2023

Looking more closely at the resolver response, I think we should return a proper NXDOMAIN instead of a SERVFAIL like what happens on non-internal networks. However, this is a more involved change as the resolver has currently no way to toggle upstream forwarding.

EDIT: see #46609

Signed-off-by: Albin Kerouanton <albinker@gmail.com>
func setupBridgeIPv4(config *networkConfiguration, i *bridgeInterface) error {
// TODO(aker): the bridge driver panics if its bridgeIPv4 field isn't set. Once bridge subnet and bridge IP address
// are decoupled, we should assign it only when it's really needed.
i.bridgeIPv4 = config.AddressIPv4
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

config.AddressIPv4 and i.bridgeIPv4 are of type *net.IPNet. That is the data type for an IP subnet. When not in canonical form (i.e. the host-part bits are not all zero) it can also pull double duty to also encode a particular address within the subnet. (The same is also true for netip.Prefix.) Since the bridge's external-facing IP address has to be within the bridge subnet anyway, I don't see the need to encode just the subnet in a separate field. It would just be bridgeIPv4 in canonical form.

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah thaJeztah merged commit 2835d1f into moby:master Oct 11, 2023
@thaJeztah thaJeztah added this to the 25.0.0 milestone Oct 11, 2023
@akerouanton akerouanton deleted the libnet-bridge-internal branch October 11, 2023 17:07
dvdksn pushed a commit to docker/docs that referenced this pull request Jan 22, 2024
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
dvdksn added a commit to docker/docs that referenced this pull request Jan 22, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants