From 3403d0d8a8644e2229862acfe6236d7f773d9723 Mon Sep 17 00:00:00 2001 From: Christian Brunner Date: Tue, 28 Jul 2026 12:33:54 +0200 Subject: [PATCH 1/2] Apply FRR config before port config on SONiC Deleting a firewall leaves bgpd with a dangling peer pointer, and the switch crashes with SIGSEGV moments later when the port is bounced. Sonic.Apply() wrote the port and interface configuration first. When a firewall is deleted its port moves from Ports.Firewalls to Ports.Unprovisioned, so configureUnprovisionedPort() calls ensureNotRouted(), which deletes the INTERFACE entry and then waits until the router interface has disappeared from the ASIC. That removes the interface's link-local address, and FRR responds by clearing peer->su. Only afterwards did the FRR configuration withdraw the neighbor. With peer->su already unset, FRR can no longer find the peer's nexthop cache entry while deleting the peer, so bnc->nht_info is left pointing at the freed peer. Moving the port into the PXE vlan bounces the interface immediately afterwards. FRR tracks link-local nexthops through interface events rather than through zebra nexthop registration, so bgp_nht_ifp_up() walks every nexthop cache entry whose ifindex matches the interface and calls evaluate_paths() on each, which dereferences the freed peer. Arming and firing therefore happen seconds apart within a single deprovisioning run. Applying the FRR configuration first withdraws the neighbor while its interface still carries a link-local address, so FRR's cleanup path succeeds and no dangling pointer is left behind. This works around an FRR defect fixed upstream, but not yet in the currently used Sonic Linux release. Co-Authored-By: Claude Opus 5 --- cmd/internal/switcher/sonic/sonic.go | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/cmd/internal/switcher/sonic/sonic.go b/cmd/internal/switcher/sonic/sonic.go index 9598620e..fdd50ca4 100644 --- a/cmd/internal/switcher/sonic/sonic.go +++ b/cmd/internal/switcher/sonic/sonic.go @@ -67,13 +67,15 @@ func loadRedisConfig(path string) (*db.Config, error) { return cfg, nil } +// Apply writes the FRR configuration before the port and interface configuration. +// The order matters, because this a workaround for an FRR defect. func (s *Sonic) Apply(ctx context.Context, cfg *types.Conf) error { - err := s.redisApplier.Apply(ctx, cfg) + err := s.frrApplier.Apply(ctx, cfg) if err != nil { return err } - return s.frrApplier.Apply(ctx, cfg) + return s.redisApplier.Apply(ctx, cfg) } func (s *Sonic) IsInitialized(ctx context.Context) (initialized bool, err error) { From 484de7232bbd366699f55eb3a075dd06defb18db Mon Sep 17 00:00:00 2001 From: Christian Brunner Date: Wed, 29 Jul 2026 08:40:42 +0200 Subject: [PATCH 2/2] Apply the FRR config first only when it is required The last commit moved the FRR configuration in front of the port configuration to keep bgpd from crashing when a firewall is deleted. The dependency between the two appliers runs both ways, though: a tenant vrf only becomes usable for FRR once the redis applier has written it to the CONFIG_DB and vrfmgrd has created the vrf device. Referencing a vrf that does not exist yet makes frr-reload reject the configuration with "% VRF Vrf not active", and every interface line below the rejected "interface ... vrf ..." is then parsed at the wrong node and reported as an unknown command. Because Apply() returned on the first FRR error, the redis applier never ran, so the vrf was never created, so the next reload failed for the same reason. reloadFrr() restores the previous frr.conf, so every reconcile run re-rendered the same configuration and failed identically - once a machine was provisioned into a new vrf, the switch never converged again. Ask the redis applier which order a run needs instead of fixing it. A port that is about to be deprovisioned and still carries a routing configuration without a vrf_name is a firewall port whose neighbor has to be withdrawn from FRR before ensureNotRouted() takes its link-local address away - only those runs need the FRR configuration first, everything else keeps the historic order and never references a vrf too early. The decision is made from the CONFIG_DB rather than from a remembered configuration, so it also holds for the first run after a restart. If it cannot be made, FRR goes first: a superfluous frr-reload is cheaper than a crashing bgpd. A run that deprovisions a firewall and adds a vrf at the same time cannot satisfy both requirements. FRR is applied first there as well, but a failure no longer keeps the port configuration from being applied and the FRR configuration is retried once the vrfs exist. The protection against the dangling peer pointer is kept either way, because frr-reload applies deletions before additions, so the neighbor is withdrawn in the first pass even when that pass later fails on the missing vrf. Co-Authored-By: Claude Opus 5 --- cmd/internal/switcher/sonic/db/configdb.go | 13 ++- .../switcher/sonic/db/configdb_test.go | 62 ++++++++++++ cmd/internal/switcher/sonic/db/db.go | 2 +- cmd/internal/switcher/sonic/redis/applier.go | 32 +++++++ .../switcher/sonic/redis/applier_test.go | 94 +++++++++++++++++++ cmd/internal/switcher/sonic/sonic.go | 37 +++++++- 6 files changed, 233 insertions(+), 7 deletions(-) create mode 100644 cmd/internal/switcher/sonic/redis/applier_test.go diff --git a/cmd/internal/switcher/sonic/db/configdb.go b/cmd/internal/switcher/sonic/db/configdb.go index c6f9452d..ae77a8af 100644 --- a/cmd/internal/switcher/sonic/db/configdb.go +++ b/cmd/internal/switcher/sonic/db/configdb.go @@ -43,7 +43,9 @@ type VxlanMap struct { Vlan string } -func newConfigDB(rdb valkey.Client, sep string) *ConfigDB { +// NewConfigDB returns a ConfigDB that talks to the given client. Use New to obtain the +// databases of a switch, this constructor is for callers that bring their own client. +func NewConfigDB(rdb valkey.Client, sep string) *ConfigDB { return &ConfigDB{ c: NewClient(rdb, sep), } @@ -265,6 +267,15 @@ func (d *ConfigDB) getVTEPName(ctx context.Context) (string, error) { return key[len(key)-1], nil } +// GetInterfaces returns a view of the interfaces that carry a routing configuration. +// The view also holds the keys of the ip addresses of an interface, they are of the +// form and never collide with a plain interface name. +func (d *ConfigDB) GetInterfaces(ctx context.Context) (View, error) { + t := d.c.GetTable(Key{interfaceTable}) + + return t.GetView(ctx) +} + func (d *ConfigDB) DeleteInterfaceConfiguration(ctx context.Context, interfaceName string) error { key := Key{interfaceTable, interfaceName} diff --git a/cmd/internal/switcher/sonic/db/configdb_test.go b/cmd/internal/switcher/sonic/db/configdb_test.go index be38aa0a..427c750a 100644 --- a/cmd/internal/switcher/sonic/db/configdb_test.go +++ b/cmd/internal/switcher/sonic/db/configdb_test.go @@ -1,11 +1,13 @@ package db import ( + "maps" "slices" "strings" "testing" "github.com/google/go-cmp/cmp" + "github.com/google/go-cmp/cmp/cmpopts" "github.com/metal-stack/metal-core/cmd/internal/switcher/sonic/db/test" "github.com/metal-stack/metal-core/cmd/internal/switcher/types" "github.com/stretchr/testify/require" @@ -1526,6 +1528,66 @@ func TestConfigDB_DeleteInterfaceConfiguration(t *testing.T) { } } +func TestConfigDB_GetInterfaces(t *testing.T) { + tests := []struct { + name string + data test.StringMap + want []string + }{ + { + name: "get all interfaces with a routing configuration", + data: configDBTestData, + want: []string{"Ethernet0", "Ethernet1", "Ethernet3"}, + }, + { + name: "ip addresses do not collide with interface names", + data: test.StringMap{ + "INTERFACE": test.StringMap{ + "Ethernet0": test.StringMap{ + "ipv6_use_link_local_only": "enable", + }, + "Ethernet0|10.0.0.1/24": test.StringMap{}, + }, + }, + want: []string{"Ethernet0", "Ethernet0|10.0.0.1/24"}, + }, + { + name: "no interface is routed", + data: test.StringMap{ + "PORT": test.StringMap{ + "Ethernet0": test.StringMap{ + "admin_status": "up", + }, + }, + }, + want: []string{}, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var ( + ctx = t.Context() + sep = "|" + vc = test.StartValkey(t) + ) + defer vc.Close() + + err := test.LoadData(ctx, vc, tt.data, sep) + require.NoError(t, err) + + d := NewConfigDB(vc, sep) + view, err := d.GetInterfaces(ctx) + require.NoError(t, err) + + got := slices.Collect(maps.Keys(view)) + slices.Sort(got) + if diff := cmp.Diff(tt.want, got, cmpopts.EquateEmpty()); diff != "" { + t.Errorf("ConfigDB.GetInterfaces() diff = %s", diff) + } + }) + } +} + func TestConfigDB_IsLinkLocalOnly(t *testing.T) { tests := []struct { name string diff --git a/cmd/internal/switcher/sonic/db/db.go b/cmd/internal/switcher/sonic/db/db.go index d1ef7a41..5dbf74f6 100644 --- a/cmd/internal/switcher/sonic/db/db.go +++ b/cmd/internal/switcher/sonic/db/db.go @@ -61,7 +61,7 @@ func New(cfg *Config) (*DB, error) { db := &DB{ Appl: newApplDB(applClient, applDB.Separator), Asic: newAsicDB(asicClient, asicDB.Separator), - Config: newConfigDB(configClient, configDB.Separator), + Config: NewConfigDB(configClient, configDB.Separator), Counters: newCountersDB(countersClient, countersDB.Separator), } return db, nil diff --git a/cmd/internal/switcher/sonic/redis/applier.go b/cmd/internal/switcher/sonic/redis/applier.go index a391e53a..aeb3c162 100644 --- a/cmd/internal/switcher/sonic/redis/applier.go +++ b/cmd/internal/switcher/sonic/redis/applier.go @@ -112,6 +112,38 @@ func (a *Applier) Apply(ctx context.Context, cfg *types.Conf) error { return errors.Join(errs...) } +// NeedsFrrFirst reports whether the FRR configuration has to be applied before cfg is +// written to the CONFIG_DB. That is the case when a port that is currently routed in +// the default vrf - a firewall port - is about to be deprovisioned, because +// configureUnprovisionedPort() tears its router interface down and FRR has to have +// withdrawn the neighbor by then. See Sonic.Apply for the full story. +func (a *Applier) NeedsFrrFirst(ctx context.Context, cfg *types.Conf) (bool, error) { + if len(cfg.Ports.Unprovisioned) == 0 { + return false, nil + } + + interfaces, err := a.db.Config.GetInterfaces(ctx) + if err != nil { + return false, fmt.Errorf("could not retrieve the routed interfaces: %w", err) + } + + for _, interfaceName := range cfg.Ports.Unprovisioned { + if !interfaces.Has(interfaceName) { + continue + } + + vrf, err := a.db.Config.GetVrfMembership(ctx, interfaceName) + if err != nil { + return false, fmt.Errorf("could not retrieve vrf membership for %s: %w", interfaceName, err) + } + if vrf == "" { + return true, nil + } + } + + return false, nil +} + func (a *Applier) GetPorts(ctx context.Context) ([]*db.Port, error) { return a.db.Config.GetPorts(ctx) } diff --git a/cmd/internal/switcher/sonic/redis/applier_test.go b/cmd/internal/switcher/sonic/redis/applier_test.go new file mode 100644 index 00000000..781a9147 --- /dev/null +++ b/cmd/internal/switcher/sonic/redis/applier_test.go @@ -0,0 +1,94 @@ +package redis + +import ( + "log/slog" + "testing" + + "github.com/stretchr/testify/require" + + "github.com/metal-stack/metal-core/cmd/internal/switcher/sonic/db" + "github.com/metal-stack/metal-core/cmd/internal/switcher/sonic/db/test" + "github.com/metal-stack/metal-core/cmd/internal/switcher/types" +) + +func TestApplier_NeedsFrrFirst(t *testing.T) { + // Ethernet0 is a machine port, Ethernet1 a firewall port, Ethernet2 is not routed + // and Ethernet3 carries an ip address next to its interface configuration. + data := test.StringMap{ + "INTERFACE": test.StringMap{ + "Ethernet0": test.StringMap{ + "ipv6_use_link_local_only": "enable", + "vrf_name": "Vrf102", + }, + "Ethernet1": test.StringMap{ + "ipv6_use_link_local_only": "enable", + }, + "Ethernet3": test.StringMap{ + "ipv6_use_link_local_only": "enable", + }, + "Ethernet3|10.0.0.1/24": test.StringMap{}, + }, + } + + tests := []struct { + name string + unprovisioned []string + want bool + }{ + { + name: "nothing is deprovisioned", + unprovisioned: nil, + want: false, + }, + { + name: "a firewall port is deprovisioned", + unprovisioned: []string{"Ethernet1"}, + want: true, + }, + { + name: "a machine port is deprovisioned", + unprovisioned: []string{"Ethernet0"}, + want: false, + }, + { + name: "a port that is not routed stays unprovisioned", + unprovisioned: []string{"Ethernet2"}, + want: false, + }, + { + name: "a firewall port among machine ports is deprovisioned", + unprovisioned: []string{"Ethernet0", "Ethernet2", "Ethernet1"}, + want: true, + }, + { + name: "a firewall port with an ip address is deprovisioned", + unprovisioned: []string{"Ethernet3"}, + want: true, + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + var ( + ctx = t.Context() + sep = "|" + vc = test.StartValkey(t) + ) + defer vc.Close() + + err := test.LoadData(ctx, vc, data, sep) + require.NoError(t, err) + + a := NewApplier(slog.New(slog.DiscardHandler), &db.DB{ + Config: db.NewConfigDB(vc, sep), + }) + + got, err := a.NeedsFrrFirst(ctx, &types.Conf{ + Ports: types.Ports{ + Unprovisioned: tt.unprovisioned, + }, + }) + require.NoError(t, err) + require.Equal(t, tt.want, got) + }) + } +} diff --git a/cmd/internal/switcher/sonic/sonic.go b/cmd/internal/switcher/sonic/sonic.go index fdd50ca4..fa2dd4b9 100644 --- a/cmd/internal/switcher/sonic/sonic.go +++ b/cmd/internal/switcher/sonic/sonic.go @@ -3,6 +3,7 @@ package sonic import ( "context" "encoding/json" + "errors" "fmt" "log/slog" "net" @@ -67,15 +68,41 @@ func loadRedisConfig(path string) (*db.Config, error) { return cfg, nil } -// Apply writes the FRR configuration before the port and interface configuration. -// The order matters, because this a workaround for an FRR defect. +// Apply writes the port and interface configuration and the FRR configuration, +// in the order the pending changes require. +// +// This is a workaround for an FRR defect. Only default-VRF peers - the +// firewalls - are affected. func (s *Sonic) Apply(ctx context.Context, cfg *types.Conf) error { - err := s.frrApplier.Apply(ctx, cfg) + frrFirst, err := s.redisApplier.NeedsFrrFirst(ctx, cfg) if err != nil { - return err + // a superfluous frr-reload is cheaper than a crashing bgpd + s.log.Error("could not determine in which order the configuration has to be applied", "error", err) + frrFirst = true } - return s.redisApplier.Apply(ctx, cfg) + if !frrFirst { + if err := s.redisApplier.Apply(ctx, cfg); err != nil { + return err + } + + return s.frrApplier.Apply(ctx, cfg) + } + + frrErr := s.frrApplier.Apply(ctx, cfg) + if frrErr != nil { + s.log.Info("could not apply the frr configuration before the port configuration, retrying afterwards", "error", frrErr) + } + + if err := s.redisApplier.Apply(ctx, cfg); err != nil { + return errors.Join(frrErr, err) + } + + if frrErr == nil { + return nil + } + + return s.frrApplier.Apply(ctx, cfg) } func (s *Sonic) IsInitialized(ctx context.Context) (initialized bool, err error) {