Delete the entries that were added, rather than looking at the service
map (DNS config) and trying to delete entries without accounting for
the container's --hostname.
Signed-off-by: Rob Murray <rob.murray@docker.com>
commit a0807e7cfe configured golangci-lint
to use go1.23 semantics, which alowed linters like `copyloopvar` to lint
using thee correct semantics.
go1.22 now creates a copy of variables when assigned in a loop; make sure we
don't have files that may downgrade semantics to go1.21 in case that also means
disabling that feature; https://go.dev/ref/spec#Go_1.22
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Also, libnetwork: Sandbox.buildHostsFile: rename var that shadowed type
Co-authored-by: Sebastiaan van Stijn <github@gone.nl>
Signed-off-by: Rob Murray <rob.murray@docker.com>
Interface DNSBackend.ResolveName, implemented by Network,
Sandbox (and noopDNSBackend) had a bool return value that
meant 'ipv6Miss'.
But, it was always set to true on a hit, and callers had
to deal with that.
So, changed the meaning of the return value to indicate
whether the name was found - which will also work for
'ipv4Miss' when we have IPv6-only containers/networks.
Signed-off-by: Rob Murray <rob.murray@docker.com>
When a container doesn't support IPv6 and it's joined to an IPv6
network, don't allocate an IPv6 address for it.
Update the DNS resolver to understand that it can have an 'ipv6miss'
(meaning an IPv4 address exists, but no IPv6) when a network is
IPv6 enabled.
Signed-off-by: Rob Murray <rob.murray@docker.com>
The new Network.enableIPv4 flag needs to be set for IPv4
networks created before it was introduced.
Commit 903daa4 attempted to do that in the unmarshalling code
by checking Network.ipamV4Info - but, that field hadn't been
unmarshalled yet, so it was never present.
Instead, check for its presence in the saved map.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Also, return IPv6 records from Network.getSvcRecords()
so that /etc/hosts entries are deleted when an IPv6-only
endpoint is removed.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Similar to EnableIPv6:
- Set it if EnableIPv4 is specified in a create request.
- Otherwise, set it if included in `default-network-opts`.
- Apart from in a config-from network, so that it doesn't look
like the API request set the field.
- Include the new field in Network marshalling/unmarshalling test.
Signed-off-by: Rob Murray <rob.murray@docker.com>
Commit 4b9dc647 (from 2016) added this in response to a
review comment, but I don't think it can ever have worked
because n.enableIPv6 = netMap["enableIPv6"].(bool), added
in 2015, would already have panic'd if the field wasn't
stored.
In any case, it's no longer needed.
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>
The `RequestPool` method has many args and named returns. This
makes the code hard to follow at times. This commit adds one struct,
`PoolRequest`, to replace these args, and one struct, `AllocatedPool`,
to replace these named returns.
Both structs' fields are properly documented to better define their
semantics, and their relationship with address allocation.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Make the internal DNS resolver for Windows containers forward requests
to upsteam DNS servers when it cannot respond itself, rather than
returning SERVFAIL.
Windows containers are normally configured with the internal resolver
first for service discovery (container name lookup), then external
resolvers from '--dns' or the host's networking configuration.
When a tool like ping gets a SERVFAIL from the internal resolver, it
tries the other nameservers. But, nslookup does not, and with this
change it does not need to.
The internal resolver learns external server addresses from the
container's HNSEndpoint configuration, so it will use the same DNS
servers as processes in the container.
The internal resolver for Windows containers listens on the network's
gateway address, and each container may have a different set of external
DNS servers. So, the resolver uses the source address of the DNS request
to select external resolvers.
On Windows, daemon.json feature option 'windows-no-dns-proxy' can be used
to prevent the internal resolver from forwarding requests (restoring the
old behaviour).
Signed-off-by: Rob Murray <rob.murray@docker.com>
Commit 8b7af1d0f added some code to update the DNSNames of all
endpoints attached to a sandbox by loading a new instance of each
affected endpoints from the datastore through a call to
`Network.EndpointByID()`.
This method then calls `Network.getEndpointFromStore()`, that in
turn calls `store.GetObject()`, which then calls `cache.get()`,
which calls `o.CopyTo(kvObject)`. This effectively creates a fresh
new instance of an Endpoint. However, endpoints are already kept in
memory by Sandbox, meaning we now have two in-memory instances of
the same Endpoint.
As it turns out, libnetwork is built around the idea that no two objects
representing the same thing should leave in-memory, otherwise breaking
mutex locking and optimistic locking (as both instances will have a drifting
version tracking ID -- dbIndex in libnetwork parliance).
In this specific case, this bug materializes by container rename failing
when applied a second time for a given container. An integration test is
added to make sure this won't happen again.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Permit container network attachments to set any static IP address within
the network's IPAM master pool, including when a subpool is configured.
Users have come to depend on being able to statically assign container
IP addresses which are guaranteed not to collide with automatically-
assigned container addresses.
Signed-off-by: Cory Snider <csnider@mirantis.com>
No more concept of "anonymous endpoints". The equivalent is now an
endpoint with no DNSNames set.
Some of the code removed by this commit was mutating user-supplied
endpoint's Aliases to add container's short ID to that list. In order to
preserve backward compatibility for the ContainerInspect endpoint, this
commit also takes care of adding that short ID (and the container
hostname) to `EndpointSettings.Aliases` before returning the response.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Instead of special-casing anonymous endpoints, use the list of DNS names
associated to the endpoint.
`(*Endpoint).isAnonymous()` has no more uses, so let's delete it.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This repository is not yet a module (i.e., does not have a `go.mod`). This
is not problematic when building the code in GOPATH or "vendor" mode, but
when using the code as a module-dependency (in module-mode), different semantics
are applied since Go1.21, which switches Go _language versions_ on a per-module,
per-package, or even per-file base.
A condensed summary of that logic [is as follows][1]:
- For modules that have a go.mod containing a go version directive; that
version is considered a minimum _required_ version (starting with the
go1.19.13 and go1.20.8 patch releases: before those, it was only a
recommendation).
- For dependencies that don't have a go.mod (not a module), go language
version go1.16 is assumed.
- Likewise, for modules that have a go.mod, but the file does not have a
go version directive, go language version go1.16 is assumed.
- If a go.work file is present, but does not have a go version directive,
language version go1.17 is assumed.
When switching language versions, Go _downgrades_ the language version,
which means that language features (such as generics, and `any`) are not
available, and compilation fails. For example:
# github.com/docker/cli/cli/context/store
/go/pkg/mod/github.com/docker/cli@v25.0.0-beta.2+incompatible/cli/context/store/storeconfig.go:6:24: predeclared any requires go1.18 or later (-lang was set to go1.16; check go.mod)
/go/pkg/mod/github.com/docker/cli@v25.0.0-beta.2+incompatible/cli/context/store/store.go:74:12: predeclared any requires go1.18 or later (-lang was set to go1.16; check go.mod)
Note that these fallbacks are per-module, per-package, and can even be
per-file, so _(indirect) dependencies_ can still use modern language
features, as long as their respective go.mod has a version specified.
Unfortunately, these failures do not occur when building locally (using
vendor / GOPATH mode), but will affect consumers of the module.
Obviously, this situation is not ideal, and the ultimate solution is to
move to go modules (add a go.mod), but this comes with a non-insignificant
risk in other areas (due to our complex dependency tree).
We can revert to using go1.16 language features only, but this may be
limiting, and may still be problematic when (e.g.) matching signatures
of dependencies.
There is an escape hatch: adding a `//go:build` directive to files that
make use of go language features. From the [go toolchain docs][2]:
> The go line for each module sets the language version the compiler enforces
> when compiling packages in that module. The language version can be changed
> on a per-file basis by using a build constraint.
>
> For example, a module containing code that uses the Go 1.21 language version
> should have a `go.mod` file with a go line such as `go 1.21` or `go 1.21.3`.
> If a specific source file should be compiled only when using a newer Go
> toolchain, adding `//go:build go1.22` to that source file both ensures that
> only Go 1.22 and newer toolchains will compile the file and also changes
> the language version in that file to Go 1.22.
This patch adds `//go:build` directives to those files using recent additions
to the language. It's currently using go1.19 as version to match the version
in our "vendor.mod", but we can consider being more permissive ("any" requires
go1.18 or up), or more "optimistic" (force go1.21, which is the version we
currently use to build).
For completeness sake, note that any file _without_ a `//go:build` directive
will continue to use go1.16 language version when used as a module.
[1]: 58c28ba286/src/cmd/go/internal/gover/version.go (L9-L56)
[2]: https://go.dev/doc/toolchain
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Early return if the iface or its address is nil to make the whole
function slightly easier to read.
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
Inline the tortured logic for deciding when to skip updating the svc
records to give us a fighting chance at deciphering the logic behind the
logic and spotting logic bugs.
Update the service records synchronously. The only potential for issues
is if this change introduces deadlocks, which should be fixed by
restrucuting the mutexes rather than papering over the issue with
sketchy hacks like deferring the operation to a goroutine.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The github.com/containerd/containerd/log package was moved to a separate
module, which will also be used by upcoming (patch) releases of containerd.
This patch moves our own uses of the package to use the new module.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This change creates a few OTEL spans and plumb context through the DNS
resolver and DNS backends (ie. Sandbox and Network). This should help
better understand how much lock contention impacts performance, and
help debug issues related to DNS queries (we basically have no
visibility into what's happening here right now).
Signed-off-by: Albin Kerouanton <albinker@gmail.com>
This method is not part of any interface, and identical to Endpoint.Iface,
but one returns an Interface-type (driverapi.InterfaceInfo) and the other
returns a concrete type (EndpointInterface).
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>
Now that we removed the interface, there's no need to cast the Network
to a NetworkInfo interface, so we can remove uses of the `Info()` method.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Check the preferredPool first, as other checks could be doing more
(such as locking, or validating / parsing). Also adding a note, as
it's unclear why we're ignoring invalid pools here.
The "invalid" conditions was added in [libnetwork#1095][1], which
moved code to reduce os-specific dependencies in the ipam package,
but also introduced a types.IsIPNetValid() function, which considers
"0.0.0.0/0" invalid, and added it to the condition to return early.
Unfortunately review does not mention this change, so there's no
context why. Possibly this was done to prevent errors further down
the line (when checking for overlaps), but returning an error here
instead would likely have avoided that as well, so we can only guess.
To make this code slightly more transparent, this patch also inlines
the "types.IsIPNetValid" function, as it's not used anywhere else,
and inlining it makes it more visible.
[1]: 5ca79d6b87 (diff-bdcd879439d041827d334846f9aba01de6e3683ed8fdd01e63917dae6df23846)
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This code was only run if no preferred pool was specified, however,
since [libnetwork#1162][2], the function would already return early
if a preferred pools was set (and the overlap check to be skipped),
so this was now just dead code.
[2]: 9cc3385f44
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
This function intentionally holds a lock / lease on address-pools to
prevent trying the same pool repeatedly.
Let's try to make this logic slightly more transparent, and prevent
defining defers in a loop. Releasing all the pools in a singe defer
also allows us to get the network-name once, which prevents locking
and unlocking the network for each iteration.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Both functions have multiple output vars with generic types, which made
it hard to grasp what's what.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
