From 3fd61f9464fe70dee785a596ffb8a796db540d7f Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Tue, 14 Jul 2026 12:51:23 -0400 Subject: [PATCH 1/2] d/libn/i/nftables: delete and replace map element Support atomically replacing an element of an interval map by deleting the existing key and adding a replacement element with the same key. This is necessary to support updating interval maps as nftables rejects an `add element` whose key overlaps the interval of an existing key with EEXIST, even when the intervals are identical. Signed-off-by: Cory Snider --- .../internal/nftables/nftables_linux.go | 13 +++- .../internal/nftables/nftables_linux_test.go | 66 +++++++++++++++++++ .../TestMapElementAtomicReplace/init.golden | 7 ++ .../updated.golden | 7 ++ .../TestMapElementDeleteCancelsCreate.golden | 6 ++ 5 files changed, 96 insertions(+), 3 deletions(-) create mode 100644 daemon/libnetwork/internal/nftables/testdata/TestMapElementAtomicReplace/init.golden create mode 100644 daemon/libnetwork/internal/nftables/testdata/TestMapElementAtomicReplace/updated.golden create mode 100644 daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteCancelsCreate.golden diff --git a/daemon/libnetwork/internal/nftables/nftables_linux.go b/daemon/libnetwork/internal/nftables/nftables_linux.go index d5b7203ac4..0afbe6c1ad 100644 --- a/daemon/libnetwork/internal/nftables/nftables_linux.go +++ b/daemon/libnetwork/internal/nftables/nftables_linux.go @@ -497,6 +497,8 @@ func (tm *Modifier) Reverse() Modifier { return rtm } +var incrementalUpdateFailedHook func(applied string, err error) + // Apply makes incremental updates to nftables. If there's a validation // error in any of the enqueued objects, or an error applying the updates // to the underlying nftables, the [Table] will be unmodified. @@ -563,6 +565,9 @@ func (t *table) apply(ctx context.Context, tm ...Modifier) (retErr error) { sb.Write(line) } log.G(ctx).Error("nftables: failed to update nftables:\n", sb.String(), "\n", err) + if incrementalUpdateFailedHook != nil { + incrementalUpdateFailedHook(sb.String(), err) + } // It's possible something destructive has happened to nftables. For example, in // integration-cli tests, tests start daemons in the same netns as the integration @@ -936,7 +941,6 @@ func (me MapElement) create(ctx context.Context, t *table) (bool, error) { Comment: me.Comment, } nm.AddedElements[me.Key] = nm.Elements[me.Key] - delete(nm.DeletedElements, me.Key) log.G(ctx).WithFields(log.Fields{ "family": t.Family, "table": t.Name, @@ -962,8 +966,11 @@ func (me MapElement) delete(ctx context.Context, t *table) (bool, error) { me.MapName, me.Key, oldValue.Value, me.Value) } delete(nm.Elements, me.Key) - delete(nm.AddedElements, me.Key) - nm.DeletedElements[me.Key] = me.Value + if _, ok := nm.AddedElements[me.Key]; ok { + delete(nm.AddedElements, me.Key) + } else { + nm.DeletedElements[me.Key] = me.Value + } log.G(ctx).WithFields(log.Fields{ "family": t.Family, "table": t.Name, diff --git a/daemon/libnetwork/internal/nftables/nftables_linux_test.go b/daemon/libnetwork/internal/nftables/nftables_linux_test.go index bde39984db..7299722578 100644 --- a/daemon/libnetwork/internal/nftables/nftables_linux_test.go +++ b/daemon/libnetwork/internal/nftables/nftables_linux_test.go @@ -25,7 +25,12 @@ func testSetup(t *testing.T) func() { t.Fatalf("Failed to enable nftables: %s", err) } cleanupContext := netnsutils.SetupTestOSContext(t) + + incrementalUpdateFailedHook = func(applied string, err error) { + t.Errorf("Incremental update failed\n%s\n---\n%s", applied, err) + } return func() { + incrementalUpdateFailedHook = nil cleanupContext() Disable() } @@ -352,6 +357,53 @@ func TestSet(t *testing.T) { applyAndCheck(t, t.Name()+"/deleted6.golden", tbl6, tm6.Reverse()) } +func TestMapElementAtomicReplace(t *testing.T) { + defer testSetup(t)() + + tbl, err := NewTable(IPv4, "x") + assert.NilError(t, err) + defer tbl.Close() + + const mapName = "a_map" + init := Modifier{} + init.Create(Map{ + Name: mapName, + ElementType: Typeof("numgen random mod 1024").MapTo("ip daddr"), + Flags: []string{"interval"}, + }) + + gen1 := Modifier{} + gen1.Create(MapElement{MapName: mapName, Key: "0-511", Value: "127.0.0.1"}) + gen1.Create(MapElement{MapName: mapName, Key: "512-1023", Value: "192.168.0.1"}) + applyAndCheck(t, t.Name()+"/init.golden", tbl, init, gen1) + + gen2 := Modifier{} + gen2.Create(MapElement{MapName: mapName, Key: "0-511", Value: "169.254.169.254"}) + gen2.Create(MapElement{MapName: mapName, Key: "512-1023", Value: "1.1.1.1"}) + applyAndCheck(t, t.Name()+"/updated.golden", tbl, gen1.Reverse(), gen2) +} + +func TestMapElementDeleteCancelsCreate(t *testing.T) { + // The final state of applying a sequence of creates and deletes should + // be the same, whether applied from one transaction or multiple. + + defer testSetup(t)() + + tbl, err := NewTable(IPv4, "x") + assert.NilError(t, err) + defer tbl.Close() + + const mapName = "a_map" + var init, create Modifier + init.Create(Map{ + Name: mapName, + ElementType: Typeof("numgen random mod 1024").MapTo("ip daddr"), + Flags: []string{"interval"}, + }) + create.Create(MapElement{MapName: mapName, Key: "0-1023", Value: "127.0.0.1"}) + applyAndCheck(t, t.Name()+".golden", tbl, init, create, create.Reverse()) +} + func TestReload(t *testing.T) { defer testSetup(t)() @@ -409,6 +461,7 @@ func TestReload(t *testing.T) { // Check implicit/recovery reload - only deleting something that's gone missing // from a vmap/set will trigger this. + incrementalUpdateFailedHook = nil tm = Modifier{} tm.Delete(SetElement{SetName: setName, Element: "192.0.2.0/24"}) applyAndCheck(t, t.Name()+"/recovered.golden", tbl, tm) @@ -704,6 +757,16 @@ func TestValidation(t *testing.T) { }, expErr: "map 'avmap' does not contain element 'eth0'", }, + { + name: "delete map element twice", + cmds: []command{ + {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, + {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop"}}, + {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop"}, delete: true}, + {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop"}, delete: true}, + }, + expErr: "map 'avmap' does not contain element 'eth0'", + }, { name: "map element with no named map", cmds: []command{ @@ -847,6 +910,9 @@ func TestValidation(t *testing.T) { for _, tc := range testcases { t.Run(tc.name, func(t *testing.T) { defer testSetup(t)() + if tc.expErr != "" { + incrementalUpdateFailedHook = nil + } tbl, err := NewTable(IPv4, "tablename") assert.NilError(t, err) defer tbl.Close() diff --git a/daemon/libnetwork/internal/nftables/testdata/TestMapElementAtomicReplace/init.golden b/daemon/libnetwork/internal/nftables/testdata/TestMapElementAtomicReplace/init.golden new file mode 100644 index 0000000000..9458ed217a --- /dev/null +++ b/daemon/libnetwork/internal/nftables/testdata/TestMapElementAtomicReplace/init.golden @@ -0,0 +1,7 @@ +table ip x { + map a_map { + typeof numgen random mod 1024 : ip daddr + flags interval + elements = { 0-511 : 127.0.0.1, 512-1023 : 192.168.0.1 } + } +} diff --git a/daemon/libnetwork/internal/nftables/testdata/TestMapElementAtomicReplace/updated.golden b/daemon/libnetwork/internal/nftables/testdata/TestMapElementAtomicReplace/updated.golden new file mode 100644 index 0000000000..b6b2b7488b --- /dev/null +++ b/daemon/libnetwork/internal/nftables/testdata/TestMapElementAtomicReplace/updated.golden @@ -0,0 +1,7 @@ +table ip x { + map a_map { + typeof numgen random mod 1024 : ip daddr + flags interval + elements = { 0-511 : 169.254.169.254, 512-1023 : 1.1.1.1 } + } +} diff --git a/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteCancelsCreate.golden b/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteCancelsCreate.golden new file mode 100644 index 0000000000..139ae1ad03 --- /dev/null +++ b/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteCancelsCreate.golden @@ -0,0 +1,6 @@ +table ip x { + map a_map { + typeof numgen random mod 1024 : ip daddr + flags interval + } +} From f7d143ece02696b0d0721474c84e5315593c4a83 Mon Sep 17 00:00:00 2001 From: Cory Snider Date: Fri, 24 Jul 2026 19:20:56 -0400 Subject: [PATCH 2/2] d/libn/i/nftables: add Batch and a map-element delete-func Add MapElementDeleteFunc, which deletes every element of a named map that a predicate selects. The elements to remove are selected from the map's contents when the change is applied. That lets a caller replace whichever of a map's elements are currently its own without keeping a record of what it wrote last time. The predicate is given the whole element, so the comment can carry metadata to select on, such as which caller owns it. "Whatever happens to match" has no inverse to roll back to, so commands are now resolved into individually reversible operations as they're applied, rather than each being one such operation. That makes it possible for a command to have no inverse at all, so split the type that holds them in two: a Modifier still takes only object creations and deletions and can therefore always be reversed, while a Batch takes any Cmd and has no Reverse method. Table.Apply accepts either. Signed-off-by: Cory Snider Co-Authored-By: Claude Opus 4.8 Co-Authored-By: Claude Opus 5 --- .../internal/nftables/fluentapi_linux.go | 2 +- .../internal/nftables/nftables_linux.go | 240 +++++++++++++---- .../internal/nftables/nftables_linux_test.go | 251 ++++++++++++++---- .../TestMapElementDeleteFunc/init.golden | 9 + .../TestMapElementDeleteFunc/replaced.golden | 9 + .../init.golden | 8 + 6 files changed, 422 insertions(+), 97 deletions(-) create mode 100644 daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFunc/init.golden create mode 100644 daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFunc/replaced.golden create mode 100644 daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFuncRollback/init.golden diff --git a/daemon/libnetwork/internal/nftables/fluentapi_linux.go b/daemon/libnetwork/internal/nftables/fluentapi_linux.go index e6ec2dde48..2fa41fcc94 100644 --- a/daemon/libnetwork/internal/nftables/fluentapi_linux.go +++ b/daemon/libnetwork/internal/nftables/fluentapi_linux.go @@ -26,5 +26,5 @@ func (b chainBuilder) Rule(rule ...string) chainBuilder { } func (b chainBuilder) Create(tm *Modifier) { - tm.cmds = append(tm.cmds, b.tm.cmds...) + tm.objs = append(tm.objs, b.tm.objs...) } diff --git a/daemon/libnetwork/internal/nftables/nftables_linux.go b/daemon/libnetwork/internal/nftables/nftables_linux.go index 0afbe6c1ad..104126734f 100644 --- a/daemon/libnetwork/internal/nftables/nftables_linux.go +++ b/daemon/libnetwork/internal/nftables/nftables_linux.go @@ -17,6 +17,11 @@ // The objects are any of: [BaseChain], [Chain], [Rule], [Map], [MapElement], // [Set], [SetElement] // +// A [Modifier] holds only creations and deletions, so it can always be reversed. +// For a change that isn't the creation or deletion of a single object - see +// [MapElementDeleteFunc] - use a [Batch], which takes any [Cmd] and cannot be +// reversed. [Table.Apply] accepts either. +// // The modifier can be reused to apply the same set of commands again or, more // usefully, reversed in order to revert its changes. See [Modifier.Reverse]. // @@ -60,6 +65,7 @@ import ( "time" "github.com/containerd/log" + "github.com/moby/moby/v2/internal/iterutil" ) // Prefix for OTEL span names. @@ -443,36 +449,91 @@ type Obj interface { delete(context.Context, *table) (bool, error) } -// Modifier is used to apply changes to a Table. -type Modifier struct { - cmds []command +// Cmd is a change that can be appended to a [Batch] with [Batch.Append]. Use +// [Create] or [Delete] for a single [Obj]; other Cmds, such as +// [MapElementDeleteFunc], describe their effect in terms of the table's contents +// and resolve to concrete operations only when the Batch is applied. +type Cmd interface { + // ops resolves the command into the individual operations it performs. It's + // called when the command is reached during [Table.Apply], so it sees the + // effects of any preceding commands. Each returned op must fully describe the + // object it acts on, so that Apply is able to roll it back. + ops(t *table) ([]op, error) } -// Create enqueues creation of object o, to be applied by tm.Apply. +// Create returns a [Cmd] that creates o. [Modifier.Create] is shorthand for +// appending it. +func Create(o Obj) Cmd { return objCmd{obj: o} } + +// Delete returns a [Cmd] that deletes o. [Modifier.Delete] is shorthand for +// appending it. +func Delete(o Obj) Cmd { return objCmd{obj: o, delete: true} } + +// Changes is a set of changes to apply to a [Table] - a [Modifier] or a [Batch]. +// Only those two types implement it. +type Changes interface { + // all yields the changes, in the order they were enqueued. + all() iter.Seq[command] +} + +// all has a value receiver on both implementations, so a caller can pass either +// the value or a pointer - the methods that populate them take a pointer, so +// either may be what's to hand. +var ( + _ Changes = Modifier{} + _ Changes = (*Modifier)(nil) + _ Changes = Batch{} + _ Changes = (*Batch)(nil) +) + +// Modifier holds creations and deletions of objects. Because that's all it can +// hold, every Modifier can be reversed - see [Modifier.Reverse]. Use a [Batch] +// for changes that aren't the creation or deletion of a single object. +type Modifier struct { + objs []objCommand +} + +// Create enqueues creation of object o, to be applied by [Table.Apply]. func (tm *Modifier) Create(o Obj) { tm.create(o, 1) } func (tm *Modifier) create(o Obj, skipFrames int) { - _, f, l, _ := runtime.Caller(skipFrames + 1) - tm.cmds = append(tm.cmds, command{ - obj: o, - callerFile: f, - callerLine: l, - }) + tm.add(objCmd{obj: o}, skipFrames+1) } -// Delete enqueues deletion of object o, to be applied by tm.Apply. +// Delete enqueues deletion of object o, to be applied by [Table.Apply]. func (tm *Modifier) Delete(o Obj) { - _, f, l, _ := runtime.Caller(1) - tm.cmds = append(tm.cmds, command{ - obj: o, - delete: true, - callerFile: f, - callerLine: l, - }) + tm.add(objCmd{obj: o, delete: true}, 1) } +func (tm *Modifier) add(c objCmd, skipFrames int) { + _, f, l, _ := runtime.Caller(skipFrames + 1) + tm.objs = append(tm.objs, objCommand{objCmd: c, callerFile: f, callerLine: l}) +} + +func (tm Modifier) all() iter.Seq[command] { + return iterutil.Map(slices.Values(tm.objs), objCommand.command) +} + +// Batch holds any changes, including ones that can't be expressed as the +// creation or deletion of a single object - see [MapElementDeleteFunc]. Use +// [Create] and [Delete] for the ones that can. +// +// Unlike a [Modifier] a Batch cannot be reversed: a command that selects what it +// acts on from the table's contents has no inverse to describe. +type Batch struct { + cmds []command +} + +// Append enqueues c, to be applied by [Table.Apply]. +func (b *Batch) Append(c Cmd) { + _, f, l, _ := runtime.Caller(1) + b.cmds = append(b.cmds, command{cmd: c, callerFile: f, callerLine: l}) +} + +func (b Batch) all() iter.Seq[command] { return slices.Values(b.cmds) } + // Reverse returns a Modifier that will undo the actions of tm. // Its operations are performed in reverse order, creates become // deletes, and deletes become creates. @@ -488,11 +549,11 @@ func (tm *Modifier) Delete(o Obj) { // delete the chain as it is not empty. func (tm *Modifier) Reverse() Modifier { rtm := Modifier{ - cmds: make([]command, len(tm.cmds)), + objs: make([]objCommand, len(tm.objs)), } - for i, cmd := range tm.cmds { - cmd.delete = !cmd.delete - rtm.cmds[len(tm.cmds)-i-1] = cmd + for i, oc := range tm.objs { + oc.delete = !oc.delete + rtm.objs[len(tm.objs)-i-1] = oc } return rtm } @@ -500,9 +561,9 @@ func (tm *Modifier) Reverse() Modifier { var incrementalUpdateFailedHook func(applied string, err error) // Apply makes incremental updates to nftables. If there's a validation -// error in any of the enqueued objects, or an error applying the updates +// error in any of the enqueued changes, or an error applying the updates // to the underlying nftables, the [Table] will be unmodified. -func (t *Table) Apply(ctx context.Context, tm ...Modifier) error { +func (t *Table) Apply(ctx context.Context, changes ...Changes) error { if !Enabled() { return errors.New("nftables is not enabled") } @@ -517,34 +578,44 @@ func (t *Table) Apply(ctx context.Context, tm ...Modifier) error { if err := t.t.checkUsable(); err != nil { return err } - return t.t.apply(ctx, tm...) + return t.t.apply(ctx, changes...) } -// apply makes the incremental updates described by tm. +// apply makes the incremental updates described by changes. // Acquire t.applyLock before calling this function. -func (t *table) apply(ctx context.Context, tm ...Modifier) (retErr error) { - var rollback []command +func (t *table) apply(ctx context.Context, changes ...Changes) (retErr error) { + var rollback []op defer func() { if retErr == nil { return } - for _, c := range slices.Backward(rollback) { - if _, err := c.rollback(ctx, t); err != nil { + for _, o := range slices.Backward(rollback) { + if _, err := o.rollback(ctx, t); err != nil { log.G(ctx).WithError(err).Error("Failed to roll back nftables updates") } } t.updatesApplied() }() - // Apply tm's updates to the Table. - for _, tmm := range tm { - for _, cmd := range tmm.cmds { - applied, err := cmd.apply(ctx, t) + // Apply the updates to the Table. + for _, chg := range changes { + for cmd := range chg.all() { + // Resolve the command here, where it's reached, so that one describing its + // effect in terms of the table's contents sees those of preceding commands. + // The operations it resolves to each name their object in full, so they can + // go on the rollback list in its place. + ops, err := cmd.cmd.ops(t) if err != nil { return fmt.Errorf("rule from %s:%d: %w", cmd.callerFile, cmd.callerLine, err) } - if applied { - rollback = append(rollback, cmd) + for _, o := range ops { + applied, err := o.apply(ctx, t) + if err != nil { + return fmt.Errorf("rule from %s:%d: %w", cmd.callerFile, cmd.callerLine, err) + } + if applied { + rollback = append(rollback, o) + } } } } @@ -982,6 +1053,53 @@ func (me MapElement) delete(ctx context.Context, t *table) (bool, error) { return true, nil } +// MapElementDeleteFunc is a [Cmd] that deletes every element of a named map that +// Fn selects. Being a Cmd rather than an [Obj], it can only be appended to a +// [Batch] with [Batch.Append] - a [Modifier] holds objects, so it cannot carry +// one, which is what keeps every Modifier reversible. There is no create +// counterpart. +// +// The elements are chosen when the [Batch] is applied, not when it's built, so a +// caller can use it to remove whichever of a map's elements are currently its own +// without keeping a record of what it last wrote - which also sweeps up anything +// an earlier failed update left behind. Combine it with [MapElement] creates in +// one Batch to replace a caller's elements wholesale: the deletes are applied, +// and rendered, before the creates. +type MapElementDeleteFunc struct { + MapName string + // Fn reports whether an element should be deleted. It's called with the + // element's Key, Value and Comment populated; the Comment is included so it + // can carry metadata to select on, such as which caller owns the element. + Fn func(MapElement) bool +} + +func (me MapElementDeleteFunc) ops(t *table) ([]op, error) { + if me.MapName == "" { + return nil, errors.New("cannot delete elements from unnamed map") + } + if me.Fn == nil { + return nil, fmt.Errorf("cannot delete elements from map '%s', no function", me.MapName) + } + nm := t.Maps[me.MapName] + if nm == nil { + return nil, fmt.Errorf("cannot delete elements from map '%s', it does not exist", me.MapName) + } + + // The order of the returned ops doesn't matter: element deletions are + // rendered from a map keyed by element, so the generated commands come out in + // key order however these are enqueued. + var ops []op + for k, v := range nm.Elements { + // Describe the element in full, so that the predicate sees exactly what + // would be deleted and rolling the delete back restores it as it was. + elem := MapElement{MapName: me.MapName, Key: k, Value: v.Value, Comment: v.Comment} + if me.Fn(elem) { + ops = append(ops, op{obj: elem, del: true}) + } + } + return ops, nil +} + // //////////////////////////// // Sets @@ -1159,24 +1277,52 @@ func (t *table) deleteChain(ctx context.Context, name string) (bool, error) { } type command struct { - obj Obj + cmd Cmd callerFile string callerLine int - delete bool } -func (c command) apply(ctx context.Context, t *table) (bool, error) { - if c.delete { - return c.obj.delete(ctx, t) - } - return c.obj.create(ctx, t) +// objCmd creates or deletes a single [Obj]. It's what [Create] and [Delete] +// return. +type objCmd struct { + obj Obj + delete bool } -func (c command) rollback(ctx context.Context, t *table) (bool, error) { - if c.delete { - return c.obj.create(ctx, t) +func (c objCmd) ops(*table) ([]op, error) { + return []op{{obj: c.obj, del: c.delete}}, nil +} + +// objCommand is an objCmd with the source location that enqueued it. +type objCommand struct { + objCmd + callerFile string + callerLine int +} + +func (oc objCommand) command() command { + return command{cmd: oc.objCmd, callerFile: oc.callerFile, callerLine: oc.callerLine} +} + +// op is a single reversible operation: the creation or deletion of one +// fully-described object. +type op struct { + obj Obj + del bool +} + +func (o op) apply(ctx context.Context, t *table) (bool, error) { + if o.del { + return o.obj.delete(ctx, t) } - return c.obj.delete(ctx, t) + return o.obj.create(ctx, t) +} + +func (o op) rollback(ctx context.Context, t *table) (bool, error) { + if o.del { + return o.obj.create(ctx, t) + } + return o.obj.delete(ctx, t) } func (t *table) updatesApplied() { diff --git a/daemon/libnetwork/internal/nftables/nftables_linux_test.go b/daemon/libnetwork/internal/nftables/nftables_linux_test.go index 7299722578..1cc3670c2e 100644 --- a/daemon/libnetwork/internal/nftables/nftables_linux_test.go +++ b/daemon/libnetwork/internal/nftables/nftables_linux_test.go @@ -36,9 +36,9 @@ func testSetup(t *testing.T) func() { } } -func applyAndCheck(t *testing.T, goldenFilename string, tbl *Table, tm ...Modifier) { +func applyAndCheck(t *testing.T, goldenFilename string, tbl *Table, changes ...Changes) { t.Helper() - err := tbl.Apply(t.Context(), tm...) + err := tbl.Apply(t.Context(), changes...) assert.Check(t, err) res := icmd.RunCommand("nft", "list", "table", string(tbl.Family()), tbl.Name()) res.Assert(t, icmd.Success) @@ -531,34 +531,34 @@ func TestNetdevChain(t *testing.T) { func TestValidation(t *testing.T) { testcases := []struct { name string - cmds []command + cmds []objCmd expErr string }{ // BaseChain { name: "create with missing base chain name", - cmds: []command{ + cmds: []objCmd{ {obj: BaseChain{ChainType: BaseChainTypeNAT, Hook: BaseChainHookPostrouting, Priority: BaseChainPrioritySrcNAT}}, }, expErr: "base chain must have a name", }, { name: "create with missing base chain type", - cmds: []command{ + cmds: []objCmd{ {obj: BaseChain{Name: "achain", Hook: BaseChainHookPostrouting, Priority: BaseChainPrioritySrcNAT}}, }, expErr: "chain 'achain': fields ChainType and Hook are required", }, { name: "create with missing base chain hook", - cmds: []command{ + cmds: []objCmd{ {obj: BaseChain{Name: "achain", ChainType: BaseChainTypeNAT, Priority: BaseChainPrioritySrcNAT}}, }, expErr: "chain 'achain': fields ChainType and Hook are required", }, { name: "delete non-empty base chain", - cmds: []command{ + cmds: []objCmd{ {obj: BaseChain{ Name: "achain", ChainType: BaseChainTypeNAT, Hook: BaseChainHookPostrouting, Priority: BaseChainPrioritySrcNAT, }}, @@ -575,7 +575,7 @@ func TestValidation(t *testing.T) { // Chain { name: "duplicate chain", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}}, {obj: Chain{Name: "achain"}}, }, @@ -583,21 +583,21 @@ func TestValidation(t *testing.T) { }, { name: "delete missing chain", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}, delete: true}, }, expErr: "does not exist", }, { name: "missing chain name", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{}}, }, expErr: "chain must have a name", }, { name: "delete non-empty chain", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}}, {obj: Rule{Chain: "achain", Rule: []string{"counter"}}}, {obj: Chain{Name: "achain"}, delete: true}, @@ -607,7 +607,7 @@ func TestValidation(t *testing.T) { // Rule { name: "bad rule", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}}, {obj: Rule{Chain: "achain", Rule: []string{"this is nonsense"}}}, }, @@ -615,7 +615,7 @@ func TestValidation(t *testing.T) { }, { name: "duplicate rule", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}}, {obj: Rule{Chain: "achain", Rule: []string{"counter"}}}, {obj: Rule{Chain: "achain", Rule: []string{"counter"}}}, @@ -624,7 +624,7 @@ func TestValidation(t *testing.T) { }, { name: "delete missing rule", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}}, {obj: Rule{Chain: "achain", Rule: []string{"counter"}}, delete: true}, }, @@ -632,7 +632,7 @@ func TestValidation(t *testing.T) { }, { name: "duplicate rule delete", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}}, {obj: Rule{Chain: "achain", Rule: []string{"counter"}}}, {obj: Rule{Chain: "achain", Rule: []string{"counter"}}, delete: true}, @@ -642,7 +642,7 @@ func TestValidation(t *testing.T) { }, { name: "create rule with missing chain name", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}}, {obj: Rule{Rule: []string{"counter"}}}, }, @@ -650,7 +650,7 @@ func TestValidation(t *testing.T) { }, { name: "delete rule with missing chain name", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}}, {obj: Rule{Rule: []string{"counter"}}, delete: true}, }, @@ -658,21 +658,21 @@ func TestValidation(t *testing.T) { }, { name: "create rule with nonexistent chain", - cmds: []command{ + cmds: []objCmd{ {obj: Rule{Chain: "achain", Rule: []string{"counter"}}}, }, expErr: "chain 'achain' does not exist", }, { name: "delete rule with nonexistent chain", - cmds: []command{ + cmds: []objCmd{ {obj: Rule{Chain: "achain", Rule: []string{"counter"}}, delete: true}, }, expErr: "chain 'achain' does not exist", }, { name: "create rule with no rule", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}}, {obj: Rule{Chain: "achain"}}, }, @@ -680,7 +680,7 @@ func TestValidation(t *testing.T) { }, { name: "delete rule with no rule", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}}, {obj: Rule{Chain: "achain"}, delete: true}, }, @@ -688,7 +688,7 @@ func TestValidation(t *testing.T) { }, { name: "bad rule mid-sequence", - cmds: []command{ + cmds: []objCmd{ {obj: Chain{Name: "achain"}}, {obj: Rule{Chain: "achain", Rule: []string{"counter"}}}, {obj: Rule{Chain: "achain", Rule: []string{"counter"}}, delete: true}, @@ -700,7 +700,7 @@ func TestValidation(t *testing.T) { // Map (verdict) { name: "duplicate map", - cmds: []command{ + cmds: []objCmd{ {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, }, @@ -708,24 +708,24 @@ func TestValidation(t *testing.T) { }, { name: "delete nonexistent map", - cmds: []command{ + cmds: []objCmd{ {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}, delete: true}, }, expErr: "cannot delete map 'avmap', it does not exist", }, { name: "missing map name", - cmds: []command{{obj: Map{ElementType: Ifname.VMap()}}}, + cmds: []objCmd{{obj: Map{ElementType: Ifname.VMap()}}}, expErr: "map must have a name", }, { name: "missing map element type", - cmds: []command{{obj: Map{Name: "avmap"}}}, + cmds: []objCmd{{obj: Map{Name: "avmap"}}}, expErr: "map 'avmap' has no element type", }, { name: "delete non-empty map", - cmds: []command{ + cmds: []objCmd{ {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop"}}, {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}, delete: true}, @@ -735,7 +735,7 @@ func TestValidation(t *testing.T) { // MapElement { name: "duplicate map element", - cmds: []command{ + cmds: []objCmd{ {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop"}}, {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop"}}, @@ -744,14 +744,14 @@ func TestValidation(t *testing.T) { }, { name: "add to map that does not exist", - cmds: []command{ + cmds: []objCmd{ {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop"}}, }, expErr: "cannot add to map 'avmap', it does not exist", }, { name: "delete nonexistent map element", - cmds: []command{ + cmds: []objCmd{ {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop"}, delete: true}, }, @@ -759,7 +759,7 @@ func TestValidation(t *testing.T) { }, { name: "delete map element twice", - cmds: []command{ + cmds: []objCmd{ {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop"}}, {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop"}, delete: true}, @@ -769,7 +769,7 @@ func TestValidation(t *testing.T) { }, { name: "map element with no named map", - cmds: []command{ + cmds: []objCmd{ {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, {obj: MapElement{Key: "eth0", Value: "drop"}}, }, @@ -777,7 +777,7 @@ func TestValidation(t *testing.T) { }, { name: "map element with no key", - cmds: []command{ + cmds: []objCmd{ {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, {obj: MapElement{MapName: "avmap", Value: "drop"}}, }, @@ -785,7 +785,7 @@ func TestValidation(t *testing.T) { }, { name: "map element with no value", - cmds: []command{ + cmds: []objCmd{ {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, {obj: MapElement{MapName: "avmap", Key: "eth0"}}, }, @@ -793,7 +793,7 @@ func TestValidation(t *testing.T) { }, { name: "map element with newline in comment", - cmds: []command{ + cmds: []objCmd{ {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop", Comment: "new\nline"}}, }, @@ -801,7 +801,7 @@ func TestValidation(t *testing.T) { }, { name: "map element with quote char in comment", - cmds: []command{ + cmds: []objCmd{ {obj: Map{Name: "avmap", ElementType: Ifname.VMap()}}, {obj: MapElement{MapName: "avmap", Key: "eth0", Value: "drop", Comment: `"quoted"`}}, }, @@ -810,7 +810,7 @@ func TestValidation(t *testing.T) { // Set { name: "duplicate set", - cmds: []command{ + cmds: []objCmd{ {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}}, {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}}, }, @@ -818,28 +818,28 @@ func TestValidation(t *testing.T) { }, { name: "delete nonexistent set", - cmds: []command{ + cmds: []objCmd{ {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}, delete: true}, }, expErr: "cannot delete set 'aset', it does not exist", }, { name: "missing set name", - cmds: []command{ + cmds: []objCmd{ {obj: Set{ElementType: IPv4Addr, Flags: []string{"interval"}}}, }, expErr: "set must have a name", }, { name: "missing set element type", - cmds: []command{ + cmds: []objCmd{ {obj: Set{Name: "aset", Flags: []string{"interval"}}}, }, expErr: "set 'aset' must have a type", }, { name: "delete non-empty set", - cmds: []command{ + cmds: []objCmd{ {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}}, {obj: SetElement{SetName: "aset", Element: "192.0.2.0/24"}}, {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}, delete: true}, @@ -849,7 +849,7 @@ func TestValidation(t *testing.T) { // SetElement { name: "duplicate set element", - cmds: []command{ + cmds: []objCmd{ {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}}, {obj: SetElement{SetName: "aset", Element: "192.0.2.0/24"}}, {obj: SetElement{SetName: "aset", Element: "192.0.2.0/24"}}, @@ -858,7 +858,7 @@ func TestValidation(t *testing.T) { }, { name: "delete nonexistent set element", - cmds: []command{ + cmds: []objCmd{ {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}}, {obj: SetElement{SetName: "aset", Element: "192.0.2.0/24"}, delete: true}, }, @@ -866,7 +866,7 @@ func TestValidation(t *testing.T) { }, { name: "add set element to unnamed set", - cmds: []command{ + cmds: []objCmd{ {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}}, {obj: SetElement{Element: "192.0.2.0/24"}}, }, @@ -874,7 +874,7 @@ func TestValidation(t *testing.T) { }, { name: "add set element with no element", - cmds: []command{ + cmds: []objCmd{ {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}}, {obj: SetElement{SetName: "aset"}}, }, @@ -882,7 +882,7 @@ func TestValidation(t *testing.T) { }, { name: "mismatched set element type", - cmds: []command{ + cmds: []objCmd{ {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}}, {obj: SetElement{SetName: "aset", Element: "2001:db8::/64"}}, }, @@ -890,7 +890,7 @@ func TestValidation(t *testing.T) { }, { name: "set element with newline in comment", - cmds: []command{ + cmds: []objCmd{ {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}}, {obj: SetElement{SetName: "aset", Element: "192.0.2.0/24", Comment: "new\nline"}}, }, @@ -898,7 +898,7 @@ func TestValidation(t *testing.T) { }, { name: "set element with quote char in comment", - cmds: []command{ + cmds: []objCmd{ {obj: Set{Name: "aset", ElementType: IPv4Addr, Flags: []string{"interval"}}}, {obj: SetElement{SetName: "aset", Element: "192.0.2.0/24", Comment: `"quoted"`}}, }, @@ -916,7 +916,10 @@ func TestValidation(t *testing.T) { tbl, err := NewTable(IPv4, "tablename") assert.NilError(t, err) defer tbl.Close() - tm := Modifier{cmds: tc.cmds} + tm := Batch{} + for _, c := range tc.cmds { + tm.Append(c) + } err = tbl.Apply(t.Context(), tm) assert.Check(t, err != nil, "expected error containing '%s'", tc.expErr) assert.Check(t, is.ErrorContains(err, tc.expErr)) @@ -928,3 +931,153 @@ func TestValidation(t *testing.T) { }) } } + +func TestMapElementDeleteFunc(t *testing.T) { + defer testSetup(t)() + + tbl, err := NewTable(IPv4, "x") + assert.NilError(t, err) + defer tbl.Close() + + // Keys are concatenated, as the load balancer's are: the first component + // identifies the owner's destination, so different owners never share - or + // overlap in - the interval space. + const mapName = "a_map" + init := Modifier{} + init.Create(Map{ + Name: mapName, + ElementType: Typeof("ip daddr").Concat("numgen random mod 1024").MapTo("ip daddr"), + Flags: []string{"interval"}, + }) + // Two owners sharing one map, identified by the element comment. + gen1 := Modifier{} + gen1.Create(MapElement{MapName: mapName, Key: "10.0.0.1 . 0-511", Value: "127.0.0.1", Comment: "svc-a"}) + gen1.Create(MapElement{MapName: mapName, Key: "10.0.0.1 . 512-1023", Value: "127.0.0.2", Comment: "svc-a"}) + gen1.Create(MapElement{MapName: mapName, Key: "10.0.0.2 . 0-1023", Value: "127.0.0.3", Comment: "svc-b"}) + applyAndCheck(t, t.Name()+"/init.golden", tbl, init, gen1) + + // Replace svc-a's elements with a different number of intervals, in a single + // Batch: the delete-func sweeps whatever it currently owns (including the + // keys that are about to disappear, which is why they can't simply be + // overwritten), then the new elements are created. svc-b must be left alone. + gen2 := Batch{} + gen2.Append(MapElementDeleteFunc{ + MapName: mapName, + Fn: func(e MapElement) bool { return e.Comment == "svc-a" }, + }) + gen2.Append(Create(MapElement{MapName: mapName, Key: "10.0.0.1 . 0-340", Value: "127.0.0.4", Comment: "svc-a"})) + gen2.Append(Create(MapElement{MapName: mapName, Key: "10.0.0.1 . 341-1023", Value: "127.0.0.5", Comment: "svc-a"})) + applyAndCheck(t, t.Name()+"/replaced.golden", tbl, gen2) + + // Sweeping an owner with nothing in the map is a no-op, not an error. + gen3 := Batch{} + gen3.Append(MapElementDeleteFunc{ + MapName: mapName, + Fn: func(e MapElement) bool { return e.Comment == "svc-nonexistent" }, + }) + applyAndCheck(t, t.Name()+"/replaced.golden", tbl, gen3) +} + +func TestMapElementDeleteFuncRollback(t *testing.T) { + defer testSetup(t)() + // The failing Apply below is expected. + incrementalUpdateFailedHook = nil + + tbl, err := NewTable(IPv4, "x") + assert.NilError(t, err) + defer tbl.Close() + + const mapName = "a_map" + init := Modifier{} + init.Create(Map{ + Name: mapName, + ElementType: Typeof("ip daddr").Concat("numgen random mod 1024").MapTo("ip daddr"), + Flags: []string{"interval"}, + }) + init.Create(MapElement{MapName: mapName, Key: "10.0.0.1 . 0-511", Value: "127.0.0.1", Comment: "svc-a"}) + init.Create(MapElement{MapName: mapName, Key: "10.0.0.1 . 512-1023", Value: "127.0.0.2", Comment: "svc-a"}) + applyAndCheck(t, t.Name()+"/init.golden", tbl, init) + + // Sweep the elements, then fail: deleting an element that doesn't exist is an + // error, so the whole Batch is rolled back. + tm := Batch{} + tm.Append(MapElementDeleteFunc{ + MapName: mapName, + Fn: func(e MapElement) bool { return e.Comment == "svc-a" }, + }) + tm.Append(Delete(MapElement{MapName: mapName, Key: "10.0.0.9 . 0-1023", Value: "127.0.0.9"})) + err = tbl.Apply(t.Context(), tm) + assert.Check(t, err != nil, "expected the Apply to fail") + + // The swept elements must be restored, comments included - the expanded + // deletes name the elements fully, so the normal rollback can recreate them. + applyAndCheck(t, t.Name()+"/init.golden", tbl, Modifier{}) +} + +func TestMapElementDeleteFuncErrors(t *testing.T) { + defer testSetup(t)() + incrementalUpdateFailedHook = nil + + const mapName = "a_map" + newTable := func(t *testing.T) *Table { + t.Helper() + tbl, err := NewTable(IPv4, "x") + assert.NilError(t, err) + t.Cleanup(func() { tbl.Close() }) + init := Modifier{} + init.Create(Map{ + Name: mapName, + ElementType: Typeof("numgen random mod 1024").MapTo("ip daddr"), + Flags: []string{"interval"}, + }) + assert.NilError(t, tbl.Apply(t.Context(), init)) + return tbl + } + + alwaysTrue := func(MapElement) bool { return true } + + tests := []struct { + name string + tm func() Batch + expErr string + }{ + // Neither a "create" nor a "reversed" case is testable any more, and that's + // the point: MapElementDeleteFunc is a Cmd, not an Obj, so it can only reach a + // Batch - which has no Create and no Reverse. Both misuses are compile errors. + { + name: "no function", + tm: func() Batch { + tm := Batch{} + tm.Append(MapElementDeleteFunc{MapName: mapName}) + return tm + }, + expErr: "no function", + }, + { + name: "unnamed map", + tm: func() Batch { + tm := Batch{} + tm.Append(MapElementDeleteFunc{Fn: alwaysTrue}) + return tm + }, + expErr: "unnamed map", + }, + { + name: "no such map", + tm: func() Batch { + tm := Batch{} + tm.Append(MapElementDeleteFunc{MapName: "nope", Fn: alwaysTrue}) + return tm + }, + expErr: "it does not exist", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + tbl := newTable(t) + err := tbl.Apply(t.Context(), tc.tm()) + assert.Check(t, err != nil, "expected error containing '%s'", tc.expErr) + assert.Check(t, is.ErrorContains(err, tc.expErr)) + }) + } +} diff --git a/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFunc/init.golden b/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFunc/init.golden new file mode 100644 index 0000000000..e7bb160310 --- /dev/null +++ b/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFunc/init.golden @@ -0,0 +1,9 @@ +table ip x { + map a_map { + typeof ip daddr . numgen random mod 1024 : ip daddr + flags interval + elements = { 10.0.0.1 . 0-511 comment "svc-a" : 127.0.0.1, + 10.0.0.1 . 512-1023 comment "svc-a" : 127.0.0.2, + 10.0.0.2 . 0-1023 comment "svc-b" : 127.0.0.3 } + } +} diff --git a/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFunc/replaced.golden b/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFunc/replaced.golden new file mode 100644 index 0000000000..06727180f5 --- /dev/null +++ b/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFunc/replaced.golden @@ -0,0 +1,9 @@ +table ip x { + map a_map { + typeof ip daddr . numgen random mod 1024 : ip daddr + flags interval + elements = { 10.0.0.2 . 0-1023 comment "svc-b" : 127.0.0.3, + 10.0.0.1 . 0-340 comment "svc-a" : 127.0.0.4, + 10.0.0.1 . 341-1023 comment "svc-a" : 127.0.0.5 } + } +} diff --git a/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFuncRollback/init.golden b/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFuncRollback/init.golden new file mode 100644 index 0000000000..83af1e1138 --- /dev/null +++ b/daemon/libnetwork/internal/nftables/testdata/TestMapElementDeleteFuncRollback/init.golden @@ -0,0 +1,8 @@ +table ip x { + map a_map { + typeof ip daddr . numgen random mod 1024 : ip daddr + flags interval + elements = { 10.0.0.1 . 0-511 comment "svc-a" : 127.0.0.1, + 10.0.0.1 . 512-1023 comment "svc-a" : 127.0.0.2 } + } +}