Close() frees the libnftables context and nils out the handle, so passing
a closed nftCtx to Apply() would hand a nil pointer to libnftables and
crash the daemon.
No caller can do that today: table.nftApply() nil-checks its *nftCtx and
creates a new context when it has been closed, and RunCmd() owns its
context for the duration of a single call. Return an error anyway, rather
than depending on every future caller to get the lifecycle right.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Cory Snider <csnider@mirantis.com>
The libnftables context is C-allocated memory which is only freed when
nftCtx.Close() is called. Attach a runtime cleanup so that a context
which is dropped without being closed is freed once it becomes
unreachable, instead of being leaked for the lifetime of the process.
runtime.AddCleanup needs a pointer to a Go-heap object to attach to, so
wrap the libnftables handle in a Go struct rather than type-defining the
C struct. Keep the nftCtx reachable across the C calls in Apply(), and
across Cleanup.Stop() in Close(): a receiver may otherwise become
unreachable at its last mention, and Stop() has no effect once the
cleanup has already been queued for execution.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Cory Snider <csnider@mirantis.com>
Namespace.RemoveInterface closes the Interface's stopCh to stop the
unsolicited ARP/NA sender, but only unregisters the Interface from the
Namespace once the removal has completed. Several failure exits sit in
between, so a removal that fails part-way leaves the Interface
registered with a closed stopCh.
Callers tear interfaces down by iterating over Namespace.Interfaces, so
the same *Interface is handed back to RemoveInterface during a later
teardown, and the second close panics, taking down the API request
being served:
http: panic serving @: close of closed channel
Seen in CI while disconnecting an endpoint during Sandbox.Refresh:
LinkSetNsFd failed with EBADF after the link had already been renamed
back to its source name, so the following re-join couldn't find the
link, and the rollback in Endpoint.sbJoin removed the same Interface
again.
Close stopCh through a sync.Once, so the ARP/NA sender is still
guaranteed to stop whether or not the netlink teardown succeeds, and
however many times removal is attempted. Keep leaving a failed
Interface registered in the Namespace: the link may still be present
there, and generateIfaceName relies on n.iFaces to avoid handing out
its name again.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Signed-off-by: Cory Snider <csnider@mirantis.com>
SetBaseChainPolicy read the table's Chains map and modified the chain it
found without holding applyLock, only taking the lock when it called
Apply to make the change. Racing with an Apply that adds a chain, the map
read is a fatal "concurrent map read and map write".
Nothing calls it yet - the nftabler doesn't implement filterForwardDrop -
so this isn't a live bug, but the API shouldn't come with the race
attached.
Hold applyLock for the whole read-modify-apply. That needs an unexported
apply which assumes the lock is held, so split the body out of
Table.Apply, leaving the exported method as the wrapper that checks the
table and takes the lock, much like Reload and table.reload.
While here, make Reload report a closed table the same way as the other
two, rather than relying on the check in table.nftApply and reporting a
generic "invalid table" for a table that raced with Close.
The happy path of SetBaseChainPolicy had no test coverage at all, which
now matters more because it applies the table with applyLock held - a
deadlock would be silent. Add one.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Table.Close() had a value receiver, so setting t.t = nil only modified
the callee's copy of the handle. The caller's Table was left looking
valid:
t, _ := nftables.NewTable(...)
t.Close()
t.IsValid() // true
Worse, Close() only dropped the nftables handle, and nftApply() opens a
new one whenever it finds none. So Apply() and Reload() on a closed table
silently reopened a handle and carried on updating the ruleset.
At the root of it, a Table looked like a plain value but behaved like a
reference, and nothing stopped it from being copied. So embed table in
Table by value and hand out *Table instead. The Table/table split is
still needed - table's fields have to be exported for text/template -
but reference semantics are now visible at every call site, and they're
enforced: because table contains a sync.Mutex, "go vet" reports both a
copy of a Table and a method or function that takes one by value, so the
shape of this bug is no longer expressible.
Close() therefore can't invalidate the table by clearing a pointer, it
has to record the state. Add a closed flag, and refuse to open a new
nftables handle for a table that's been closed.
Apply() checked neither for a closed table nor for a nil *Table, which
would have panicked. It now reports an error, checking the closed state
with applyLock held so that it can't race with Close(), and before the
in-memory table is touched so that a rejected update isn't recorded as
applied.
The invalid table is now a nil *Table rather than a zero-value Table,
which also removes the need for consumers to return an empty Table
alongside an error. That made it obvious that the nftabler was leaking
the table it had just created when it gave up on setting up IPv6, so
close it.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Correct the version referenced in the comment describing the migration
performed by Network.dropLegacyFilterDirectAccess.
The migration was introduced in v28.2.0, not v28.0.2. Also update the
TODO to clarify that the migration can be removed once we no longer
expect upgrades from v28.0.x or v28.1.x directly to a release that
includes the migration.
Refs: a0ff0a361e ("libnetwork: drop
legacy direct access filter rules")
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
While at it, also check for zero value to prevent a (theoretical)
panic (panic: runtime error: integer divide by zero).
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Replace go-fuzz-headers with native fuzz parameters and construct Record
values directly from the fuzz input. This also fuzzes netip.Addr values
instead of the zero value.
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
From the [DB.FreelistType] GoDoc:
// FreelistType sets the backend freelist type. There are two options. Array which is simple but endures
// dramatic performance degradation if database is large and fragmentation in freelist is common.
// The alternative one is using hashmap, it is faster in almost all circumstances
// but it doesn't guarantee that it offers the smallest page id available. In normal case it is safe.
// The default type is array
FreelistType FreelistType
While the default is still `FreelistArrayType`, the package shows that
the intent is to make `FreelistMapType` the default in future;
https://pkg.go.dev/go.etcd.io/bbolt@v1.5.0#pkg-constants
// TODO(ahrtr): eventually we should (step by step)
// 1. default to `FreelistMapType`;
// 2. remove the `FreelistArrayType`, do not export `FreelistMapType`
// and remove field `FreelistType' from both `DB` and `Options`;
[DB.FreelistType]: https://pkg.go.dev/go.etcd.io/bbolt@v1.5.0#DB.FreelistType
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Replace the custom seeded RNG and mutex with the top-level rand.Shuffle
from math/rand/v2, which is properly seeded and safe for concurrent use,
and inline the shuffle at its only call site in handleIPQuery.
Signed-off-by: Sabith Saheb <shabi7204192361@gmail.com>
When plumbIngressPortsProxy fails to bind a port, it stored a typed-nil
listener in ingressProxyTbl. closeIngressPortsProxy then panicked on
Close() during ingress port removal.
Add a regression test and continue on bind error instead of recording
the failed listener.
Signed-off-by: Jason Green <jason@jasg.org>
Add support for defining nftables maps and sets with size and timeout
specified, which are required values for maps and sets that are updated
from the packet path.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Errors when parsing and applying templates reference the offending line
number and column within the template text. It's not so easy to
correlate the position mentioned in the error with the template source
when the template is inlined as string literals in a Go source file.
Extract the nftables reload and incremental-update templates to
dedicated files and embed them into the program with //go:embed
directives so the offset mentioned in error messages is the offset
within the template file.
Having the templates in dedicated files also enables more rich IDE
integration. gopls for instance will highlight template directives (when
the `semanticTokens` setting is enabled) and report template syntax
errors inline when a template is in a dedicated file.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Looks like the linter didn't detect this in de71462b97
daemon/libnetwork/internal/nftables/fluentapi_linux.go:8:20: ST1016: methods on the same type should have the same receiver name (seen 1x "b", 2x "cd") (staticcheck)
func (b BaseChain) Builder() chainBuilder {
^
daemon/libnetwork/internal/nftables/fluentapi_linux.go:14:16: ST1016: methods on the same type should have the same receiver name (seen 1x "c", 2x "cd") (staticcheck)
func (c Chain) Builder() chainBuilder {
^
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Change (*nftables.Table).Apply() to take an arbitrary number of
Modifiers to be applied as a single atomic unit. Having the ability to
atomically apply multiple modifiers to a table enables some powerful
patterns such as atomically replacing one collection of rules or
elements with another:
var update nftables.Modifier
update.Create(/* ... */)
t.Apply(reversePrevious, update)
reversePrevious = update.Reverse()
Signed-off-by: Cory Snider <csnider@mirantis.com>
With most of the dynamism of nftables rulesets being powered by named
maps and sets, the rules of a chain are often initialized when the chain
is added, and never touched again. Add a fluent API for adding the
creation of a chain and all its rules to a modifier without having to
repeat the chain name for each rule.
Signed-off-by: Cory Snider <csnider@mirantis.com>
The early return duplicated the length check, making the custom error
path unreachable. As a result, a nil error was wrapped and returned
instead of the intended error message.
Signed-off-by: Jintao Zhang <zhangjintao9020@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
Clearing the daemon umask means creation sites can no longer rely on
umask 0022 to remove group/other-write bits from permissive modes.
Adjust modes for daemon-owned files that previously depended on the old
umask:
- fuse-overlayfs lower file
- goroutine stack dump file
- layer migration tar-data file
- network namespace mount file
Signed-off-by: Paweł Gronowski <pawel.gronowski@docker.com>
- update the Dockerfile to switch to the cli-variant (as it doesn't
require a docker daemon), and update to the latest v29 image
- update the script to not use the deprecated libnetwork repository
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
As libnftables uses `select(2)` on the netlink socket the process is
aborted if the socket's file descriptor is >= 1024. A dockerd process
could easily exceed 1024 open file descriptors at a time under normal
circumstances, so there is a risk of libnftables killing dockerd at a
random time through no fault of dockerd. Default to programming nftables
rulesets by exec'ing `nft -f` until libnftables is updated to be
compatible with processes that open a large number of file descriptors
by using `poll(2)` or `epoll(2)` instead of `select(2)`.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Concurrent bulkSyncNode calls targeting the same node overwrite each
other's entry in bulkSyncAckTbl. Only the last channel gets closed by
handleBulkSync; the rest block for 30s on a channel nobody will ever
close. This causes unnecessary delays for DNS resolution on newly
joined swarm nodes.
Only have unsolicited bulk syncs subscribe to be notified when the peer
replies with its own bulk sync as only unsolicited bulk syncs solicit a
reply. Correlate the reply to its soliciting bulk-sync using Lamport
timestamps.
Co-authored-by: Dustin Kaiser <8209087+mrnicegyu11@users.noreply.github.com>
Signed-off-by: Cory Snider <csnider@mirantis.com>
Follow-up to PR 52804, applying thaJeztah's review suggestion: check
IsLoopback first for both address families (preserving any requested
loopback address), and only fall back to the canonical loopback for
the family otherwise. No behavior change; ::1 now returns through the
loopback-preserving branch instead of the IPv6 fallback, with the same
result.
Signed-off-by: Andrew Liu <andrewjliu22@gmail.com>
Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
The allocator reads its bounds from net.ipv4.ip_local_port_range but
ignored net.ipv4.ip_local_reserved_ports, so dynamically allocated host
ports could land on ports the kernel itself would never hand out
automatically, typically ports set aside for other applications.
Skip those ports when allocating from the default ephemeral range,
mirroring the kernel behaviour for automatic port assignment. Requests
for a specific port or an explicit port range are unchanged, like
explicit binds are unchanged by ip_local_reserved_ports.
Signed-off-by: Mathieu Champlon <mathieu.champlon@docker.com>
Stale rules in one firewall backend could persist if the daemon's
firewall backend is switched without rebooting the host, which could
interfere with the rules being programmed for the current firewall
backend. Have the overlay network driver delete any stale nftables table
when starting in iptables mode, and delete any stale iptables per-VNI
encryption rules when programming encryption for the VNI in nftables
mode.
Signed-off-by: Cory Snider <csnider@mirantis.com>
When the daemon is linked against libnftables it programs the kernel
without invoking the `nft` command. Allow the nftables firewall backend
to be enabled when libnftables is used, irrespective of whether `nft` is
installed on the host.
Update the bridge network driver to clean up stale nftables tables in
iptables mode without depending on the `nft` command.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Afford applying nft commands via libnftables without needing to go
through our table abstraction. Make the table abstraction responsible
for lazily allocating an nft context.
Signed-off-by: Cory Snider <csnider@mirantis.com>
