Repository: metallb/metallb
Pull request #1150 (2021-12-24 11:37:34)

FRR mode: fail in unsupported scenarios
Currently FRR does not allow to do two different things:

- Having different myAsns for different bgp peers
- Having different routerids for different peers

The result is misleading, because the rendered configuration will consume only the last added myasn (or routerid) of the list, resulting in a different behaviour from what the user is expecting from the api. Here, we reject the configuration in such cases, and we remove the routerid setting from the tests.


msherif1234: we are doing this via webhooks since CR is where we want to be why we add he additional effort to block those configs
fedepaol: > we are doing this via webhooks since CR is where we want to be why we add he additional effort to block those configs ? I am sure I have myASN now router-id I had originally but been asked to remove from webhoook I could bring it back

Because not all the users of metallb use the operator.
Re: routerID, the thing is, we never noticed it wasn't working as expected, so at this point in time it's safer to block it.
oribon: lgtm
cgoncalves: LGTM

Patch:
diff --git a/internal/config/validate.go b/internal/config/validate.go
index d229de30..4036ee60 100644
--- a/internal/config/validate.go
+++ b/internal/config/validate.go
@@ -52,3 +52,20 @@ func DiscardFRROnly(c *configFile) error {
 func DontValidate(c *configFile) error {
 	return nil
 }
+
+// DiscardNativeOnly returns an error if the current configFile contains
+// any options that are available only in the native implementation.
+func DiscardNativeOnly(c *configFile) error {
+	if len(c.Peers) > 0 {
+		myAsn := c.Peers[0].MyASN
+		for _, p := range c.Peers {
+			if p.RouterID != "" {
+				return fmt.Errorf("peer %s has routerid set on frr mode", p.Addr)
+			}
+			if p.MyASN != myAsn {
+				return fmt.Errorf("peer %s has myAsn different from %s, in FRR mode all myAsn must be equal", p.Addr, c.Peers[0].Addr)
+			}
+		}
+	}
+	return nil
+}
diff --git a/speaker/main.go b/speaker/main.go
index f38c5b38..3c698588 100644
--- a/speaker/main.go
+++ b/speaker/main.go
@@ -135,9 +135,11 @@ func main() {
 		os.Exit(1)
 	}
 
-	validateConfig := metallbcfg.DontValidate
+	var validateConfig metallbcfg.Validate
 	if bgpType == "native" {
 		validateConfig = metallbcfg.DiscardFRROnly
+	} else {
+		validateConfig = metallbcfg.DiscardNativeOnly
 	}
 
 	client, err := k8s.New(&k8s.Config{


Test patch:
diff --git a/e2etest/pkg/config/parse.go b/e2etest/pkg/config/parse.go
index dee8ce2d..a531dddf 100644
--- a/e2etest/pkg/config/parse.go
+++ b/e2etest/pkg/config/parse.go
@@ -27,7 +27,6 @@ type Peer struct {
 	SrcAddr       string         `yaml:"source-address,omitempty"`
 	Port          uint16         `yaml:"peer-port,omitempty"`
 	HoldTime      string         `yaml:"hold-time,omitempty"`
-	RouterID      string         `yaml:"router-id,omitempty"`
 	NodeSelectors []NodeSelector `yaml:"node-selectors,omitempty"`
 	Password      string         `yaml:"password,omitempty"`
 	BFDProfile    string         `yaml:"bfd-profile,omitempty"`
diff --git a/e2etest/pkg/config/update.go b/e2etest/pkg/config/update.go
index a88a8a8e..b91c25d7 100644
--- a/e2etest/pkg/config/update.go
+++ b/e2etest/pkg/config/update.go
@@ -243,7 +243,6 @@ func (o operatorUpdater) peerToOperator(p Peer) (*operatorv1beta1.BGPPeer, error
 			SrcAddress:    p.SrcAddr,
 			Port:          p.Port,
 			HoldTime:      metav1.Duration{Duration: holdtime},
-			RouterID:      p.RouterID,
 			NodeSelectors: nodeselectors,
 			Password:      p.Password,
 			BFDProfile:    p.BFDProfile,
diff --git a/e2etest/pkg/metallb/config.go b/e2etest/pkg/metallb/config.go
index 4e34880b..6e1d10ce 100644
--- a/e2etest/pkg/metallb/config.go
+++ b/e2etest/pkg/metallb/config.go
@@ -13,7 +13,6 @@ import (
 const (
 	defaultNameSpace     = "metallb-system"
 	defaultConfigMapName = "config"
-	baseRouterID         = "10.10.10.%d"
 )
 
 var Namespace = defaultNameSpace
@@ -49,7 +48,6 @@ func PeersForContainers(containers []*frrcontainer.FRR, ipFamily string) []confi
 				ASN:          c.RouterConfig.ASN,
 				MyASN:        c.NeighborConfig.ASN,
 				Port:         c.RouterConfig.BGPPort,
-				RouterID:     fmt.Sprintf(baseRouterID, i),
 				Password:     c.RouterConfig.Password,
 				HoldTime:     holdTime,
 				EBGPMultiHop: ebgpMultihop,
diff --git a/internal/config/validate_test.go b/internal/config/validate_test.go
index f1738fa7..fa270ed8 100644
--- a/internal/config/validate_test.go
+++ b/internal/config/validate_test.go
@@ -105,6 +105,93 @@ func TestValidate(t *testing.T) {
 	}
 }
 
+func TestValidateFRR(t *testing.T) {
+	tests := []struct {
+		desc     string
+		config   *configFile
+		mustFail bool
+	}{
+		{
+			desc: "peer with routerid",
+			config: &configFile{
+				Peers: []peer{
+					{
+						Addr:     "1.2.3.4",
+						RouterID: "1.2.3.4",
+					},
+				},
+			},
+			mustFail: true,
+		},
+		{
+			desc: "bfd profile set",
+			config: &configFile{
+				Peers: []peer{
+					{
+						Addr: "1.2.3.4",
+					},
+				},
+				BFDProfiles: []bfdProfile{
+					{
+						Name: "default",
+					},
+				},
+			},
+		},
+		{
+			desc: "myAsn set, all equals",
+			config: &configFile{
+				Peers: []peer{
+					{
+						Addr:  "1.2.3.4",
+						MyASN: 123,
+					},
+					{
+						Addr:  "1.2.3.5",
+						MyASN: 123,
+					},
+					{
+						Addr:  "1.2.3.6",
+						MyASN: 123,
+					},
+				},
+			},
+		},
+		{
+			desc: "myAsn set, one different",
+			config: &configFile{
+				Peers: []peer{
+					{
+						Addr:  "1.2.3.4",
+						MyASN: 123,
+					},
+					{
+						Addr:  "1.2.3.5",
+						MyASN: 123,
+					},
+					{
+						Addr:  "1.2.3.6",
+						MyASN: 125,
+					},
+				},
+			},
+			mustFail: true,
+		},
+	}
+
+	for _, test := range tests {
+		t.Run(test.desc, func(t *testing.T) {
+			err := DiscardNativeOnly(test.config)
+			if test.mustFail && err == nil {
+				t.Fatalf("Expected error for %s", test.desc)
+			}
+			if !test.mustFail && err != nil {
+				t.Fatalf("Not expected error %s for %s", err, test.desc)
+			}
+		})
+	}
+}
+
 func intPtr(i int) *int {
 	return &i
 }