Plumb context from the API down to libnet driver method `CreateNetwork`,
and add an OTel span to the bridge driver's `createNetwork` method.
Include a few attributes describing the network configuration (e.g.
IPv4/IPv6, ICC, internal and MTU).
A new util function, `RecordStatus`, is added to the `otelutil` package
to easily record any error, and update the span status accordingly.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Commit 0546d90 introduced the use of ipset to reduce the number
of rules that need to be processed per-packet, and make the code
a bit simpler.
But, docker's used on embedded kernels compiled without support
for ipset, so the change is too disruptive.
Replace the two ipset rules with a new chain that writes out the
rule's actions long-hand. So ..
This rule:
-A FORWARD -m set --match-set docker-ext-bridges-v4 dst \
-m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
Is transformed into a per-bridge rule in new chain DOCKER-CT:
-A DOCKER-FORWARD -j DOCKER-CT
-A DOCKER-CT -o docker0 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
-A DOCKER-CT -o bridge1 -m conntrack --ctstate RELATED,ESTABLISHED -j ACCEPT
And:
-A FORWARD -m set --match-set docker-ext-bridges-v4 dst -j DOCKER
Is transformed into a per-bridge rule in new chain DOCKER-BRIDGE:
-A DOCKER-FORWARD -j DOCKER-BRIDGE
-A DOCKER-BRIDGE -o docker0 -j DOCKER
-A DOCKER-BRIDGE -o bridge1 -j DOCKER
Signed-off-by: Rob Murray <rob.murray@docker.com>
To support this, a new netlabel is added: `com.docker.network.endpoint.ifname`.
It gives the ability to specify the interface name to be set by
netdrivers when the interface is added / moved into the container's
network namespace.
All builtin netdrivers support it.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Since commit 933fcc9 (Re-remove the SetKey OCI prestart hook),
the network namespace will be set up before endpoints are
created in most cases, apart from build containers.
So, when possible, create the veth with one end in that netns
to save moving it in later. On my host, that saves about 20ms
for each bridge network a container is connected to.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Now a gratuitous/unsolicted ARP is sent, there's no need to
use an IPv4-based MAC address to preserve arp-cache mappings
between an endpoint's IP addresses and its MAC addresses.
Because a random MAC address is used for the default bridge,
it no longer makes sense to derive container IPv6 addresses
from the MAC address. This "postIPv6" behaviour was needed
before IPv6 addresses could be configured, but not now. So,
IPv6 addresses will now be IPAM-allocated on the default
bridge network, just as they are for user-defined bridges.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Before that change, we were passing the datastore to network drivers
through a `map[string]interface{}`. Then, each driver that needed the
store would cast the datastore to the correct type.
This was not a good design, as it was not clear which drivers were using
the store and which were not. Not all unit tests were passing the store,
leading to logs about uninitialized store being written.
This change makes the store a parameter of the `RegisterX` functions.
All unit tests are now passing a valid datastore to the drivers. A new
testutil func is added for that purpose.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Rules no longer need to be rearranged when creating a network.
Per-network rules are always appended to the FORWARD chain so,
after adding them, there's no need to delete the per-driver
rules to re-insert them at the top of the chain.
Signed-off-by: Rob Murray <rob.murray@docker.com>
libnetwork/drivers/bridge/bridge_linux_test.go:1079:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar)
tc := tc
^
libnetwork/drivers/bridge/port_mapping_linux_test.go:763:3: The copy of the 'for' variable "tc" can be deleted (Go 1.22+) (copyloopvar)
tc := tc
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
A recent change to the vishvananda/netlink package exposes
NLM_F_DUMP_INTR in some netlink responses as an EINTR (with
no data).
Retry the requests when that happens, up to five times, before
returning the error. The limit of five is arbitrary, on most
systems a single retry will be rare but, there's no guarantee
that a retry will succeed. So, on a very busy or misbehaving
system the error may still be returned. In most cases, this
will lead to failure of the operation being attempted (which
may lead to daemon startup failure, network initialisation
failure etc).
Signed-off-by: Rob Murray <rob.murray@docker.com>
It was added so that tests could replace it before it was picked
up and used by a new network's PortMapper, so that tests were isolated
from each other. Now the PortMapper is not used by the bridge driver,
neither is driver's portAllocator.
Instead of replacing the driver.portAllocator in tests, reset the
singleton instance using its ReleaseAll().
Un-export portallocator.NewInstance, now the tests aren't using it.
Signed-off-by: Rob Murray <rob.murray@docker.com>
The bridge driver now does its own port-mapping, rather than using the
portmapper module (which ran as two completely separate instances, for
IPv4 and IPv6).
When asked for a mapping from any host address (0.0.0.0/0) with a range
of host ports, the same port will be allocated for IPv4 and IPv6, or the
mapping will fail with an error if that's not possible.
The bridge driver now manages its own port mappings. So, remove
linux-specific PortMapper code and make what's left Windows-only.
Also, replace the portmapper.userlandProxy interface with StartProxy().
Signed-off-by: Rob Murray <rob.murray@docker.com>
For a port mapping like '-p 8080-8083:80', when some non-docker process
is using a port in the range, try other ports in the range. And, don't
do that on live-restore.
Because the port mapping may fail on live-restore, leaving no ports
mapped for the endpoint - update the view of mapped ports shown in
'inspect' output. (The wrong mappings will still be shown in 'docker ps',
the container will be left running and connected to the network, it just
won't work. There's plenty of scope for better error handling here.)
Signed-off-by: Rob Murray <rob.murray@docker.com>
The previous allocator was subnetting address pools eagerly
when the daemon started, and would then just iterate over that
list whenever RequestPool was called. This was leading to high
memory usage whenever IPv6 pools were configured with a target
subnet size too different from the pools prefix size.
For instance: pool = fd00::/8, target size = /64 -- 2 ^ (64-8)
subnets would be generated upfront. This would take approx.
9 * 10^18 bits -- way too much for any human computer in 2024.
Another noteworthy issue, the previous implementation was allocating
a subnet, and then in another layer was checking whether the
allocation was conflicting with some 'reserved networks'. If so,
the allocation would be retried, etc... To make it worse, 'reserved
networks' would be recomputed on every iteration. This is totally
ineffective as there could be 'reserved networks' that fully overlap
a given address pool (or many!).
To fix this issue, a new field `Exclude` is added to `RequestPool`.
It's up to each driver to take it into account. Since we don't know
whether this retry loop is useful for some remote IPAM driver, it's
reimplemented bug-for-bug directly in the remote driver.
The new allocator uses a linear-search algorithm. It takes advantage
of all lists (predefined pools, allocated subnets and reserved
networks) being sorted and logically combines 'allocated' and
'reserved' through a 'double cursor' to iterate on both lists at the
same time while preserving the total order. At the same time, it
iterates over 'predefined' pools and looks for the first empty space
that would be a good fit.
Currently, the size of the allocated subnet is still dictated by
each 'predefined' pools. We should consider hardcoding that size
instead, and let users specify what subnet size they want. This
wasn't possible before as the subnets were generated upfront. This
new allocator should be able to deal with this easily.
The method used for static allocation has been updated to make sure
the ascending order of 'allocated' is preserved. It's bug-for-bug
compatible with the previous implementation.
One consequence of this new algorithm is that we don't keep track
of where the last allocation happened, we just allocate the first
free subnet we find.
Before:
- Allocate: 10.0.1.0/24, 10.0.2.0/24 ; Deallocate: 10.0.1.0/24 ;
Allocate 10.0.3.0/24.
Now, the 3rd allocation would yield 10.0.1.0/24 once again.
As it doesn't change the semantics of the allocator, there's no
reason to worry about that.
Finally, about 'reserved networks'. The heuristics we use are
now properly documented. It was discovered that we don't check
routes for IPv6 allocations -- this can't be changed because
there's no such thing as on-link routes for IPv6.
(Kudos to Rob Murray for coming up with the linear-search idea.)
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Libnet's method `(*Network).createEndpoint()` is already parsing this
netlabel to set the field `ep.iface.mac`. Later on, this same method
invoke the driver's method `CreateEndpoint` with an `InterfaceInfo` arg
and an `options` arg (an opaque map of driver otps).
The `InterfaceInfo` interface contains a `MacAddress()` method that
returns `ep.iface.mac`. And the opaque map may contain the key
`netlabel.MacAddress`.
Prior to this change, the bridge driver was calling `MacAddress()`. If
no value was returned, it'd fall back to the option set in the `options`
map, or generate a MAC address based on the IP address.
However, the expected type of the `options` value is a `net.HardwareAddr`.
This is what's set by the daemon when handing over the endpoint config
to libnet controller. If the value is a string, as is the case if the
MAC address is provided through `EndpointsSettings.DriverOpts`, it
produces an error.
As such, the opaque option and the `MacAddress()` are necessarily the
same -- either nothing or a `net.HardwareAddr`. No need to keep both.
Moreover, the struct `endpointConfiguration` was only used to store that
netlabel value. Drop it too.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
The bridge driver's setupIPChains() had an initial sanity check that
"--iptables=true".
But, it's called with "version=IPv6" when "--iptables=false" and
"--ip6tables=true" - the sanity test needed to allow for that.
Signed-off-by: Rob Murray <rob.murray@docker.com>
- Remove package variable bridge.bridgeIPv6
- Use netip in more places
- Improve error messages from fixed-cidr-v6 checks
Signed-off-by: Rob Murray <rob.murray@docker.com>
When mapping a port with the userland-proxy enabled, the daemon would
perform an "exec.LookPath" for every mapped port (which, in case of
a range of ports, would be for every port in the range).
This was both inefficient (looking up the binary for each port), inconsistent
(when running in rootless-mode, the binary was looked-up once), as well as
inconvenient, because a missing binary, or a mis-configureed userland-proxy-path
would not be detected daeemon startup, and not produce an error until starting
the container;
docker run -d -P nginx:alpine
4f7b6589a1680f883d98d03db12203973387f9061e7a963331776170e4414194
docker: Error response from daemon: driver failed programming external connectivity on endpoint romantic_wiles (7cfdc361821f75cbc665564cf49856cf216a5b09046d3c22d5b9988836ee088d): fork/exec docker-proxy: no such file or directory.
However, the container would still be created (but invalid);
docker ps -a
CONTAINER ID IMAGE COMMAND CREATED STATUS PORTS NAMES
869f41d7e94f nginx:alpine "/docker-entrypoint.…" 10 seconds ago Created romantic_wiles
This patch changes how the userland-proxy is configured;
- The path of the userland-proxy is now looked up / configured at daemon
startup; this is similar to how the proxy is configured in rootless-mode.
- A warning is logged when failing to lookup the binary.
- If the daemon is configured with "userland-proxy" enabled, an error is
produced, and the daemon will refuse to start.
- The "proxyPath" argument for newProxyCommand() (in libnetwork/portmapper)
is now required to be set. It no longer looks up the executable, and
produces an error if no path was provided. While this change was not
required, it makes the daemon config the canonical source of truth, instead
of logic spread accross multiplee locations.
Some of this logic is a change of behavior, but these changes were made with
the assumption that we don't want to support;
- installing the userland proxy _after_ the daemon was started
- moving the userland proxy (or installing a proxy with a higher
preference in PATH)
With this patch:
Validating the config produces an error if the binary is not found:
dockerd --validate
WARN[2023-12-29T11:36:39.748699591Z] failed to lookup default userland-proxy binary error="exec: \"docker-proxy\": executable file not found in $PATH"
userland-proxy is enabled, but userland-proxy-path is not set
Disabling userland-proxy prints a warning, but validates as "OK":
dockerd --userland-proxy=false --validate
WARN[2023-12-29T11:38:30.752523879Z] ffailed to lookup default userland-proxy binary error="exec: \"docker-proxy\": executable file not found in $PATH"
configuration OK
Speficying a non-absolute path produces an error:
dockerd --userland-proxy-path=docker-proxy --validate
invalid userland-proxy-path: must be an absolute path: docker-proxy
Befor this patch, we would not validate this path, which would allow the daemon
to start, but fail to map a port;
docker run -d -P nginx:alpine
4f7b6589a1680f883d98d03db12203973387f9061e7a963331776170e4414194
docker: Error response from daemon: driver failed programming external connectivity on endpoint romantic_wiles (7cfdc361821f75cbc665564cf49856cf216a5b09046d3c22d5b9988836ee088d): fork/exec docker-proxy: no such file or directory.
Specifying an invalid userland-proxy-path produces an error as well:
dockerd --userland-proxy-path=/usr/local/bin/no-such-binary --validate
userland-proxy-path is invalid: stat /usr/local/bin/no-such-binary: no such file or directory
mkdir -p /usr/local/bin/not-a-file
dockerd --userland-proxy-path=/usr/local/bin/not-a-file --validate
userland-proxy-path is invalid: exec: "/usr/local/bin/not-a-file": is a directory
touch /usr/local/bin/not-an-executable
dockerd --userland-proxy-path=/usr/local/bin/not-an-executable --validate
userland-proxy-path is invalid: exec: "/usr/local/bin/not-an-executable": permission denied
Same when using the daemon.json config-file;
echo '{"userland-proxy-path":"no-such-binary"}' > /etc/docker/daemon.json
dockerd --validate
unable to configure the Docker daemon with file /etc/docker/daemon.json: merged configuration validation from file and command line flags failed: invalid userland-proxy-path: must be an absolute path: no-such-binary
dockerd --userland-proxy-path=hello --validate
unable to configure the Docker daemon with file /etc/docker/daemon.json: the following directives are specified both as a flag and in the configuration file: userland-proxy-path: (from flag: hello, from file: /usr/local/bin/docker-proxy)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
If IPv6 is enabled for a bridge network, by the time configuration
is applied, the bridge will always have an address. Assert that, by
raising an error when the configuration is validated.
Use that to simplify the logic used to calculate which addresses
should be assigned to a bridge. Also remove a redundant check in
setupGatewayIPv6() and the error associated with it.
Fix unit tests that enabled IPv6, but didn't supply an IPv6 IPAM
address/pool. Before this change, these tests passed but silently
left the bridge without an IPv6 address.
(The daemon already ensured there was an IPv6 address, this change
does not add a new restriction on config at that level.)
Signed-off-by: Rob Murray <rob.murray@docker.com>
Calculate the IPv6 addreesses needed on a bridge, then reconcile them
with the addresses on an existing bridge by deleting then adding as
required.
(Previously, required addresses were added one-by-one, then unwanted
addresses were removed. This meant the daemon failed to start if, for
example, an existing bridge had address '2000:db8::/64' and the config
was changed to '2000:db8::/80'.)
IPv6 addresses are now calculated and applied in one go, so there's no
need for setupVerifyAndReconcile() to check the set of IPv6 addresses on
the bridge. And, it was guarded by !config.InhibitIPv4, which can't have
been right. So, removed its IPv6 parts, and added IPv4 to its name.
Link local addresses, the example given in the original ticket, are now
released when containers are stopped. Not releasing them meant that
when using an LL subnet on the default bridge, no container could be
started after a container was stopped (because the calculated address
could not be re-allocated). In non-default bridge networks using an
LL subnet, addresses leaked.
Linux always uses the standard 'fe80::/64' LL network. So, if a bridge
is configured with an LL subnet prefix that overlaps with it, a config
error is reported. Non-overlapping LL subnet prefixes are allowed.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Rename all variables/fields/map keys associated with the
`com.docker.network.host_ipv4` option from `HostIP` to `HostIPv4`.
Rationale:
* This makes the variable/field name consistent with the option
name.
* This makes the code more readable because it is clear that the
variable/field does not hold an IPv6 address. This will hopefully
avoid bugs like <https://github.com/moby/moby/issues/46445> in the
future.
* If IPv6 SNAT support is ever added, the names will be symmetric.
Signed-off-by: Richard Hansen <rhansen@rhansen.org>
Interface-matching should generally happen on the receiver side, and this
function was only used in a single location, and passed as argument to
Driver.CreateEndpoint, which already matches the interface by accepting
a driverapi.InterfaceInfo.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
InvalidParameter is now compatible with errdefs.InvalidParameter. Thus,
these errors will now return a 400 status code instead of a 500.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This makes it easier to spot if code is only used on Linux. Note that "all of"
the bridge driver is Linux-only.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
