Repository: metallb/metallb
Pull request #1990 (2023-06-27 10:21:59)

Make the configuration conversion stable
When translating the configuration coming from the k8s api to the internal one, the outcome depends on the order in which the cache returns us the various items. This may cause unnecessary reloads.

In order to avoid this, we pre-order the list of items to feed the conversion with.
The sorting is done in a conversion function in the controller (rather than the config package) in order to make it harder to forget to add the sorting action.



liornoy: /lgtm

Patch:
diff --git a/internal/bgp/frr/frr.go b/internal/bgp/frr/frr.go
index d22e1e46..c8dcff0d 100644
--- a/internal/bgp/frr/frr.go
+++ b/internal/bgp/frr/frr.go
@@ -298,8 +298,8 @@ func (sm *sessionManager) createConfig() (*frrConfig, error) {
 			advConfig := advertisementConfig{
 				IPFamily:         family,
 				Prefix:           prefix,
-				Communities:      communities,
-				LargeCommunities: largeCommunities,
+				Communities:      sort.StringSlice(communities),
+				LargeCommunities: sort.StringSlice(largeCommunities),
 				LocalPref:        adv.LocalPref,
 			}
 
@@ -313,6 +313,7 @@ func (sm *sessionManager) createConfig() (*frrConfig, error) {
 				neighbor.HasV6Advertisements = true
 			}
 		}
+		sortAdvertiesements(neighbor.Advertisements)
 	}
 
 	for _, r := range sortMap(routers) {
@@ -458,3 +459,35 @@ func sortMap[T any](toSort map[string]T) []T {
 	}
 	return res
 }
+
+func sortAdvertiesements(toSort []*advertisementConfig) {
+	sort.Slice(toSort, func(i, j int) bool {
+		if toSort[i].IPFamily != toSort[j].IPFamily {
+			return toSort[i].IPFamily < toSort[j].IPFamily
+		}
+		if toSort[i].Prefix != toSort[j].Prefix {
+			return toSort[i].Prefix < toSort[j].Prefix
+		}
+		if toSort[i].LocalPref != toSort[j].LocalPref {
+			return toSort[i].LocalPref < toSort[j].LocalPref
+		}
+		if len(toSort[i].Communities) != len(toSort[j].Communities) {
+			return len(toSort[i].Communities) < len(toSort[j].Communities)
+		}
+		for k := range toSort[i].Communities {
+			if toSort[i].Communities[k] != toSort[j].Communities[k] {
+				return toSort[i].Communities[k] < toSort[j].Communities[k]
+			}
+		}
+		if len(toSort[i].LargeCommunities) != len(toSort[j].LargeCommunities) {
+			return len(toSort[i].LargeCommunities) < len(toSort[j].LargeCommunities)
+		}
+
+		for k := range toSort[i].LargeCommunities {
+			if toSort[i].LargeCommunities[k] != toSort[j].LargeCommunities[k] {
+				return toSort[i].LargeCommunities[k] < toSort[j].LargeCommunities[k]
+			}
+		}
+		return false
+	})
+}
diff --git a/internal/k8s/controllers/config_controller.go b/internal/k8s/controllers/config_controller.go
index 712e167f..a4fc12f1 100644
--- a/internal/k8s/controllers/config_controller.go
+++ b/internal/k8s/controllers/config_controller.go
@@ -142,7 +142,7 @@ var requestHandler = func(r *ConfigReconciler, ctx context.Context, req ctrl.Req
 
 	level.Debug(r.Logger).Log("controller", "ConfigReconciler", "metallb CRs and Secrets", dumpClusterResources(&resources))
 
-	cfg, err := config.For(resources, r.ValidateConfig)
+	cfg, err := toConfig(resources, r.ValidateConfig)
 	if err != nil {
 		configStale.Set(1)
 		level.Error(r.Logger).Log("controller", "ConfigReconciler", "error", "failed to parse the configuration", "error", err)
diff --git a/internal/k8s/controllers/config_conversion.go b/internal/k8s/controllers/config_conversion.go
new file mode 100644
index 00000000..fc36e948
--- /dev/null
+++ b/internal/k8s/controllers/config_conversion.go
@@ -0,0 +1,45 @@
+// SPDX-License-Identifier:Apache-2.0
+
+package controllers
+
+import (
+	"sort"
+
+	"go.universe.tf/metallb/internal/config"
+)
+
+func toConfig(fromK8s config.ClusterResources, validate config.Validate) (*config.Config, error) {
+	resources := config.ClusterResources{
+		Pools:              sortedCopy(fromK8s.Pools),
+		Peers:              sortedCopy(fromK8s.Peers),
+		BFDProfiles:        sortedCopy(fromK8s.BFDProfiles),
+		L2Advs:             sortedCopy(fromK8s.L2Advs),
+		BGPAdvs:            sortedCopy(fromK8s.BGPAdvs),
+		LegacyAddressPools: sortedCopy(fromK8s.LegacyAddressPools),
+		Communities:        sortedCopy(fromK8s.Communities),
+		PasswordSecrets:    fromK8s.PasswordSecrets,
+		Nodes:              sortedCopy(fromK8s.Nodes),
+		Namespaces:         sortedCopy(fromK8s.Namespaces),
+		BGPExtras:          fromK8s.BGPExtras,
+	}
+
+	cfg, err := config.For(resources, validate)
+	return cfg, err
+}
+
+// We need to do this ballet because we need to leverage the GetName() function
+// of the objects, but the interface is implemented by the pointer, not the object,
+// whereas what we are given with .Items is the slice of objects.
+func sortedCopy[T any, PT interface {
+	GetName() string
+	*T
+}](toSort []T) []T {
+	res := make([]T, len(toSort))
+	copy(res, toSort)
+	sort.Slice(res, func(i, j int) bool {
+		first := PT(&toSort[i])
+		second := PT(&toSort[j])
+		return first.GetName() < second.GetName()
+	})
+	return res
+}
diff --git a/internal/k8s/controllers/pool_controller.go b/internal/k8s/controllers/pool_controller.go
index 52ceb111..5de8723f 100644
--- a/internal/k8s/controllers/pool_controller.go
+++ b/internal/k8s/controllers/pool_controller.go
@@ -83,7 +83,7 @@ func (r *PoolReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.
 
 	level.Debug(r.Logger).Log("controller", "PoolReconciler", "metallb CRs", dumpClusterResources(&resources))
 
-	cfg, err := config.For(resources, r.ValidateConfig)
+	cfg, err := toConfig(resources, r.ValidateConfig)
 	if err != nil {
 		configStale.Set(1)
 		level.Error(r.Logger).Log("controller", "PoolReconciler", "error", "failed to parse the configuration", "error", err)


Test patch:
diff --git a/internal/bgp/frr/frr_test.go b/internal/bgp/frr/frr_test.go
index 8a9e4244..16132249 100644
--- a/internal/bgp/frr/frr_test.go
+++ b/internal/bgp/frr/frr_test.go
@@ -5,6 +5,7 @@ package frr
 import (
 	"flag"
 	"fmt"
+	"math/rand"
 	"net"
 	"os"
 	"os/exec"
@@ -81,6 +82,11 @@ func testSetup(t *testing.T) {
 }
 
 func testCheckConfigFile(t *testing.T) {
+	validateGoldenFile(t)
+	validateAgainstFRR(t)
+}
+
+func validateGoldenFile(t *testing.T) {
 	configFile, goldenFile := testGenerateFileNames(t)
 
 	if *update {
@@ -88,7 +94,10 @@ func testCheckConfigFile(t *testing.T) {
 	}
 
 	testCompareFiles(t, configFile, goldenFile)
+}
 
+func validateAgainstFRR(t *testing.T) {
+	configFile, _ := testGenerateFileNames(t)
 	if !strings.Contains(configFile, "Invalid") {
 		err := testFileIsValid(configFile)
 		if err != nil {
@@ -820,75 +829,97 @@ func TestTwoAdvertisementsTwoSessions(t *testing.T) {
 	l := log.NewNopLogger()
 	sessionManager := mockNewSessionManager(l, logging.LevelInfo)
 	defer close(sessionManager.reloadConfig)
-	session, err := sessionManager.NewSession(l,
-		bgp.SessionParameters{
-			PeerAddress:   "10.2.2.254:179",
-			SourceAddress: net.ParseIP("10.1.1.254"),
-			MyASN:         100,
-			RouterID:      net.ParseIP("10.1.1.254"),
-			PeerASN:       200,
-			HoldTime:      time.Second,
-			KeepAliveTime: time.Second,
-			Password:      "password",
-			CurrentNode:   "hostname",
-			EBGPMultiHop:  true,
-			SessionName:   "test-peer"})
-	if err != nil {
-		t.Fatalf("Could not create session: %s", err)
-	}
-	defer session.Close()
-
-	session1, err := sessionManager.NewSession(l,
-		bgp.SessionParameters{
-			PeerAddress:   "10.2.2.255:179",
-			SourceAddress: net.ParseIP("10.1.1.254"),
-			MyASN:         100,
-			RouterID:      net.ParseIP("10.1.1.254"),
-			PeerASN:       200,
-			HoldTime:      time.Second,
-			KeepAliveTime: time.Second,
-			Password:      "password",
-			CurrentNode:   "hostname",
-			EBGPMultiHop:  true,
-			SessionName:   "test-peer1"})
-	if err != nil {
-		t.Fatalf("Could not create session: %s", err)
-	}
-	defer session1.Close()
-
-	prefix1 := &net.IPNet{
-		IP:   net.ParseIP("172.16.1.10"),
-		Mask: classCMask,
-	}
-	communities := []community.BGPCommunity{}
-	community, _ := community.New("1111:2222")
-	communities = append(communities, community)
-	adv1 := &bgp.Advertisement{
-		Prefix:      prefix1,
-		Communities: communities,
-	}
-
-	prefix2 := &net.IPNet{
-		IP:   net.ParseIP("172.16.1.11"),
-		Mask: classCMask,
-	}
-
-	adv2 := &bgp.Advertisement{
-		Prefix:      prefix2,
-		Communities: communities,
-		LocalPref:   2,
-	}
-
-	err = session.Set(adv1, adv2)
-	if err != nil {
-		t.Fatalf("Could not advertise prefix: %s", err)
-	}
-	err = session1.Set(adv1, adv2)
-	if err != nil {
-		t.Fatalf("Could not advertise prefix: %s", err)
-	}
 
-	testCheckConfigFile(t)
+	for i := 0; i < 100; i++ {
+		func() {
+			sessionsParameters := []bgp.SessionParameters{
+				{
+					PeerAddress:   "10.2.2.254:179",
+					SourceAddress: net.ParseIP("10.1.1.254"),
+					MyASN:         100,
+					RouterID:      net.ParseIP("10.1.1.254"),
+					PeerASN:       200,
+					HoldTime:      time.Second,
+					KeepAliveTime: time.Second,
+					Password:      "password",
+					CurrentNode:   "hostname",
+					EBGPMultiHop:  true,
+					SessionName:   "test-peer"},
+				{
+					PeerAddress:   "10.2.2.255:179",
+					SourceAddress: net.ParseIP("10.1.1.254"),
+					MyASN:         100,
+					RouterID:      net.ParseIP("10.1.1.254"),
+					PeerASN:       200,
+					HoldTime:      time.Second,
+					KeepAliveTime: time.Second,
+					Password:      "password",
+					CurrentNode:   "hostname",
+					EBGPMultiHop:  true,
+					SessionName:   "test-peer1"},
+			}
+			seed := time.Now().UnixNano()
+			rand.Seed(seed)
+
+			rand.Shuffle(len(sessionsParameters), func(i, j int) {
+				sessionsParameters[i], sessionsParameters[j] = sessionsParameters[j], sessionsParameters[i]
+			})
+
+			session, err := sessionManager.NewSession(l, sessionsParameters[0])
+			if err != nil {
+				t.Fatalf("Could not create session: %s", err)
+			}
+			defer session.Close()
+
+			session1, err := sessionManager.NewSession(l, sessionsParameters[1])
+			if err != nil {
+				t.Fatalf("Could not create session: %s", err)
+			}
+			defer session1.Close()
+
+			prefix1 := &net.IPNet{
+				IP:   net.ParseIP("172.16.1.10"),
+				Mask: classCMask,
+			}
+			communities := []community.BGPCommunity{}
+			community, _ := community.New("1111:2222")
+			communities = append(communities, community)
+			adv1 := &bgp.Advertisement{
+				Prefix:      prefix1,
+				Communities: communities,
+			}
+
+			prefix2 := &net.IPNet{
+				IP:   net.ParseIP("172.16.1.11"),
+				Mask: classCMask,
+			}
+
+			adv2 := &bgp.Advertisement{
+				Prefix:      prefix2,
+				Communities: communities,
+				LocalPref:   2,
+			}
+			advs := []*bgp.Advertisement{adv1, adv2}
+			rand.Shuffle(len(advs), func(i, j int) {
+				advs[i], advs[j] = advs[j], advs[i]
+			})
+			err = session.Set(advs[0], advs[1])
+			if err != nil {
+				t.Fatalf("Could not advertise prefix: %s", err)
+			}
+
+			rand.Shuffle(len(advs), func(i, j int) {
+				advs[i], advs[j] = advs[j], advs[i]
+			})
+			err = session1.Set(advs[0], advs[1])
+			if err != nil {
+				t.Fatalf("Could not advertise prefix: %s", err)
+			}
+
+			validateGoldenFile(t)
+		}()
+	}
+	validateAgainstFRR(t)
 }
 
 func TestTwoAdvertisementsTwoSessionsOneWithPeerSelector(t *testing.T) {
diff --git a/internal/k8s/controllers/config_conversion_test.go b/internal/k8s/controllers/config_conversion_test.go
new file mode 100644
index 00000000..73092c0e
--- /dev/null
+++ b/internal/k8s/controllers/config_conversion_test.go
@@ -0,0 +1,254 @@
+// SPDX-License-Identifier:Apache-2.0
+
+package controllers
+
+import (
+	"math/rand"
+	"reflect"
+	"testing"
+	"time"
+
+	"github.com/davecgh/go-spew/spew"
+	v1beta1 "go.universe.tf/metallb/api/v1beta1"
+	v1beta2 "go.universe.tf/metallb/api/v1beta2"
+	"go.universe.tf/metallb/internal/config"
+	v1 "k8s.io/api/core/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+)
+
+func TestConversionIsStable(t *testing.T) {
+	peers := []v1beta2.BGPPeer{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "peer1",
+				Namespace: "metallb-system",
+			},
+			Spec: v1beta2.BGPPeerSpec{
+				MyASN:      42,
+				ASN:        142,
+				Address:    "1.2.3.4",
+				BFDProfile: "default",
+			},
+		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "peer2",
+				Namespace: "metallb-system",
+			},
+			Spec: v1beta2.BGPPeerSpec{
+				MyASN:      42,
+				ASN:        142,
+				Address:    "1.2.3.5",
+				BFDProfile: "default",
+			},
+		},
+	}
+	bfdProfiles := []v1beta1.BFDProfile{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "default",
+				Namespace: "metallb-system",
+			},
+		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "bar",
+				Namespace: "metallb-system",
+			},
+		},
+	}
+	pools := []v1beta1.IPAddressPool{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "pool1",
+				Namespace: "metallb-system",
+			},
+			Spec: v1beta1.IPAddressPoolSpec{
+				Addresses: []string{
+					"10.20.0.0/16",
+				},
+			},
+		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "pool2",
+				Namespace: "metallb-system",
+			},
+			Spec: v1beta1.IPAddressPoolSpec{
+				Addresses: []string{
+					"10.10.0.0/16",
+				},
+				AllocateTo: &v1beta1.ServiceAllocation{
+					Namespaces: []string{"foo", "bar"},
+				},
+			},
+		},
+	}
+	legacyAddressPools := []v1beta1.AddressPool{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "legacypool1",
+				Namespace: testNamespace,
+			},
+			Spec: v1beta1.AddressPoolSpec{
+				Addresses: []string{
+					"10.21.0.0/16",
+				},
+				Protocol: "bgp",
+			},
+		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "legacypool2",
+				Namespace: testNamespace,
+			},
+			Spec: v1beta1.AddressPoolSpec{
+				Addresses: []string{
+					"10.22.0.0/16",
+				},
+				Protocol: "layer2",
+			},
+		},
+	}
+	bgpAdvs := []v1beta1.BGPAdvertisement{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "adv1",
+				Namespace: "metallb-system",
+			},
+			Spec: v1beta1.BGPAdvertisementSpec{
+				Communities: []string{"bar"},
+			},
+		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "adv2",
+				Namespace: "metallb-system",
+			},
+			Spec: v1beta1.BGPAdvertisementSpec{
+				Communities: []string{"bar2"},
+			},
+		},
+	}
+	l2Advs := []v1beta1.L2Advertisement{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "l2adv1",
+				Namespace: "metallb-system",
+			},
+		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "l2adv2",
+				Namespace: "metallb-system",
+			},
+			Spec: v1beta1.L2AdvertisementSpec{
+				Interfaces: []string{"foo"},
+			},
+		},
+	}
+
+	communities := []v1beta1.Community{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "community",
+				Namespace: "metallb-system",
+			},
+			Spec: v1beta1.CommunitySpec{
+				Communities: []v1beta1.CommunityAlias{
+					{
+						Name:  "bar",
+						Value: "64512:1234",
+					},
+				},
+			},
+		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name:      "community1",
+				Namespace: "metallb-system",
+			},
+			Spec: v1beta1.CommunitySpec{
+				Communities: []v1beta1.CommunityAlias{
+					{
+						Name:  "bar2",
+						Value: "64512:1235",
+					},
+				},
+			},
+		},
+	}
+	namespaces := []v1.Namespace{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "foo",
+			},
+		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "bar",
+			},
+		},
+	}
+	nodes := []v1.Node{
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "node1",
+			},
+		},
+		{
+			ObjectMeta: metav1.ObjectMeta{
+				Name: "node2",
+			},
+		},
+	}
+
+	resources := config.ClusterResources{
+		Pools:              pools,
+		Peers:              peers,
+		BFDProfiles:        bfdProfiles,
+		L2Advs:             l2Advs,
+		BGPAdvs:            bgpAdvs,
+		LegacyAddressPools: legacyAddressPools,
+		Communities:        communities,
+		PasswordSecrets:    map[string]v1.Secret{},
+		Nodes:              nodes,
+		Namespaces:         namespaces,
+	}
+
+	firstConfig, err := toConfig(resources, config.DontValidate)
+
+	if err != nil {
+		t.Fatalf("conversion failed, err %v", err)
+	}
+	seed := time.Now().UnixNano()
+	rand.Seed(seed)
+
+	// Here we check the stability of the conversion, by shuffling the
+	// order of the resources and checking that the output is the same.
+	for i := 0; i < 100; i++ {
+		shuffleObjects(resources.Peers)
+		shuffleObjects(resources.BFDProfiles)
+		shuffleObjects(resources.Pools)
+		shuffleObjects(resources.BGPAdvs)
+		shuffleObjects(resources.L2Advs)
+		shuffleObjects(resources.Communities)
+		shuffleObjects(resources.LegacyAddressPools)
+		shuffleObjects(resources.Nodes)
+		shuffleObjects(resources.Namespaces)
+
+		config, err := toConfig(resources, config.DontValidate)
+
+		if err != nil {
+			t.Fatalf("conversion failed, seed %d, %v", seed, err)
+		}
+
+		if !reflect.DeepEqual(config, firstConfig) {
+			t.Fatalf("conversion is not stable, seed %d, current %v\n previous %v\n", seed, spew.Sdump(config), spew.Sdump(firstConfig))
+		}
+	}
+}
+
+func shuffleObjects[T any](toShuffle []T) {
+	rand.Shuffle(len(toShuffle), func(i, j int) { toShuffle[i], toShuffle[j] = toShuffle[j], toShuffle[i] })
+}