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>
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>
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>
An nftables vmap is just a map whose element values are of type
`verdict`. Generalize VMap, VMapElement and Set to support any element
type.
Add a fluent API to build arbitrary tuple and mapping types from the
composition of other types or the 'typeof' an nftables expression.
Block the invalid composition of named types and `typeof` expressions at
compile time.
There are only a handful of contexts where the data type needs to be
specified: set and map definitions. Modelling set and map types as a
singular "nft type" does not align well with the semantics of nftables.
Map types are always composite types with a key and a value part. Set
types do not have a value part; it is an error to create a map with a
set type or vice versa. Encode this distinction into the Go type system
so it is a compile-time error to try to use a set type in a map context
or a map type in a set context.
Drop the 'NftType' prefix from the primitive set-type constants. The
prefix stutters with the package name and, as discussed above, it is not
accurate to call them "nft types." Verdicts cannot be used as set
elements or map keys. Provide dedicated methods to construct verdict-map
types from set types instead of modelling verdicts as types themselves.
Signed-off-by: Cory Snider <csnider@mirantis.com>
Add nftables.Modifier, to hold a queue of commands that can be applied
using Modifier.Apply. No updates are made to the underlying Table
until Apply is called, errors in the queue if commands are deferred
until Apply.
This has the advantages that:
- less error handling is needed in code that generates update commands
- it's transactional, without needing explicit transactions
Minor disadvantages are that it's slightly more difficult to debug updates,
as it's no longer possible to step through the call making an update to
the Table manipulation in a debugger - and errors in the command, and
errors like trying to update a nonexistent chain/set/vmap, deleting an
object that doesn't exist or creating a duplicate are not reported
until the updates are applied (but, the file/line where the rule was
added is reported).
Signed-off-by: Rob Murray <rob.murray@docker.com>
