Apply FRR config before port config on SONiC (work around FRR defect)

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 <interface><separator><prefix> 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}
 

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

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

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)
 }

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)
+		})
+	}
+}

cmd/internal/switcher/sonic/sonic.go

@@ -3,6 +3,7 @@ package sonic
 import (
 	"context"
 	"encoding/json"
+	"errors"
 	"fmt"
 	"log/slog"
 	"net"
@@ -67,10 +68,38 @@ func loadRedisConfig(path string) (*db.Config, error) {
 	return cfg, nil
 }
 
+// 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.redisApplier.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
+	}
+
+	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)