Repository: metallb/metallb
Pull request #1416 (2022-06-07 08:37:03)

Add validation for BGP and L2 advertisement
This commit validates duplicate values in ip address pool, ip address
pool selector, node selector and peers fields and throw an error
when such duplicates are found.

Signed-off-by: Periyasamy Palanisamy <pepalani@redhat.com>

fedepaol: LGTM, thanks!

Patch:
diff --git a/internal/config/config.go b/internal/config/config.go
index e0fcd957..746a2e61 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -611,6 +611,18 @@ func setBGPAdvertisementsToPools(ipPools []metallbv1beta1.IPAddressPool, bgpAdvs
 }
 
 func l2AdvertisementFromCR(crdAd metallbv1beta1.L2Advertisement, nodes []corev1.Node) (*L2Advertisement, error) {
+	err := validateDuplicate(crdAd.Spec.IPAddressPools, "ipAddressPools")
+	if err != nil {
+		return nil, err
+	}
+	err = validateLabelSelectorDuplicate(crdAd.Spec.IPAddressPoolSelectors, "ipAddressPoolSelectors")
+	if err != nil {
+		return nil, err
+	}
+	err = validateLabelSelectorDuplicate(crdAd.Spec.NodeSelectors, "nodeSelectors")
+	if err != nil {
+		return nil, err
+	}
 	selected, err := selectedNodes(nodes, crdAd.Spec.NodeSelectors)
 	if err != nil {
 		return nil, errors.Wrapf(err, "Failed to parse node selector for %s", crdAd.Name)
@@ -621,7 +633,23 @@ func l2AdvertisementFromCR(crdAd metallbv1beta1.L2Advertisement, nodes []corev1.
 }
 
 func bgpAdvertisementFromCR(crdAd metallbv1beta1.BGPAdvertisement, communities map[string]uint32, nodes []corev1.Node) (*BGPAdvertisement, error) {
-	err := validateDuplicateCommunities(crdAd.Spec.Communities)
+	err := validateDuplicate(crdAd.Spec.IPAddressPools, "ipAddressPools")
+	if err != nil {
+		return nil, err
+	}
+	err = validateDuplicate(crdAd.Spec.Communities, "community")
+	if err != nil {
+		return nil, err
+	}
+	err = validateDuplicate(crdAd.Spec.Peers, "peers")
+	if err != nil {
+		return nil, err
+	}
+	err = validateLabelSelectorDuplicate(crdAd.Spec.IPAddressPoolSelectors, "ipAddressPoolSelectors")
+	if err != nil {
+		return nil, err
+	}
+	err = validateLabelSelectorDuplicate(crdAd.Spec.NodeSelectors, "nodeSelectors")
 	if err != nil {
 		return nil, err
 	}
@@ -684,7 +712,7 @@ func bgpAdvertisementsFromLegacyCR(ads []metallbv1beta1.LegacyBgpAdvertisement,
 
 	var ret []*BGPAdvertisement
 	for _, crdAd := range ads {
-		err := validateDuplicateCommunities(crdAd.Communities)
+		err := validateDuplicate(crdAd.Communities, "community")
 		if err != nil {
 			return nil, err
 		}
@@ -905,17 +933,6 @@ func validateDuplicateBGPAdvertisements(ads []metallbv1beta1.BGPAdvertisement) e
 	return nil
 }
 
-func validateDuplicateCommunities(communities []string) error {
-	for i := 0; i < len(communities); i++ {
-		for j := i + 1; j < len(communities); j++ {
-			if communities[i] == communities[j] {
-				return fmt.Errorf("duplicate definition of community %q", communities[i])
-			}
-		}
-	}
-	return nil
-}
-
 func containsAdvertisement(advs []*L2Advertisement, toCheck *L2Advertisement) bool {
 	for _, adv := range advs {
 		if reflect.DeepEqual(adv.Nodes, toCheck.Nodes) {
@@ -976,3 +993,25 @@ OUTER:
 	}
 	return ipPools, nil
 }
+
+func validateDuplicate(strSlice []string, sliceType string) error {
+	for i := 0; i < len(strSlice); i++ {
+		for j := i + 1; j < len(strSlice); j++ {
+			if strSlice[i] == strSlice[j] {
+				return fmt.Errorf("duplicate definition of %s %q", sliceType, strSlice[i])
+			}
+		}
+	}
+	return nil
+}
+
+func validateLabelSelectorDuplicate(labelSelectors []metav1.LabelSelector, labelSelectorType string) error {
+	for i := 0; i < len(labelSelectors); i++ {
+		for j := i + 1; j < len(labelSelectors); j++ {
+			if labelSelectors[i].String() == labelSelectors[j].String() {
+				return fmt.Errorf("duplicate definition of %s %q", labelSelectorType, labelSelectors[i])
+			}
+		}
+	}
+	return nil
+}


Test patch:
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 38b3af62..caa88f4a 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -831,6 +831,61 @@ func TestParse(t *testing.T) {
 				},
 			},
 		},
+		{
+			desc: "duplicate ip address pools - in L2 adv",
+			crs: ClusterResources{
+				Pools: []v1beta1.IPAddressPool{testPool},
+				L2Advs: []v1beta1.L2Advertisement{
+					{
+						ObjectMeta: v1.ObjectMeta{Name: testAdvName},
+						Spec: v1beta1.L2AdvertisementSpec{
+							IPAddressPools: []string{testPoolName, testPoolName},
+						},
+					},
+				},
+			},
+		},
+		{
+			desc: "duplicate ip address pools - in BGP adv",
+			crs: ClusterResources{
+				Pools: []v1beta1.IPAddressPool{testPool},
+				BGPAdvs: []v1beta1.BGPAdvertisement{
+					{
+						ObjectMeta: v1.ObjectMeta{Name: testAdvName},
+						Spec: v1beta1.BGPAdvertisementSpec{
+							IPAddressPools: []string{testPoolName, testPoolName},
+						},
+					},
+				},
+			},
+		},
+		{
+			desc: "duplicate peers - in BGP adv",
+			crs: ClusterResources{
+				Pools: []v1beta1.IPAddressPool{testPool},
+				Peers: []v1beta2.BGPPeer{
+					{
+						ObjectMeta: v1.ObjectMeta{
+							Name: "peer1",
+						},
+						Spec: v1beta2.BGPPeerSpec{
+							MyASN:   42,
+							ASN:     42,
+							Address: "1.2.3.4",
+						},
+					},
+				},
+				BGPAdvs: []v1beta1.BGPAdvertisement{
+					{
+						ObjectMeta: v1.ObjectMeta{Name: testAdvName},
+						Spec: v1beta1.BGPAdvertisementSpec{
+							IPAddressPools: []string{testPoolName},
+							Peers:          []string{"peer1", "peer1"},
+						},
+					},
+				},
+			},
+		},
 		{
 			desc: "bad community literal (wrong format) - in BGP adv",
 			crs: ClusterResources{
@@ -1795,6 +1850,86 @@ func TestParse(t *testing.T) {
 				BFDProfiles: map[string]*BFDProfile{},
 			},
 		},
+		{
+			desc: "use duplicate ip pool selectors - in BGP adv",
+			crs: ClusterResources{
+				Pools: []v1beta1.IPAddressPool{
+					{
+						ObjectMeta: v1.ObjectMeta{
+							Name:   "pool1",
+							Labels: map[string]string{"test": "pool1"},
+						},
+						Spec: v1beta1.IPAddressPoolSpec{
+							Addresses: []string{
+								"10.20.0.0/16",
+							},
+						},
+					},
+				},
+				BGPAdvs: []v1beta1.BGPAdvertisement{
+					{
+						ObjectMeta: v1.ObjectMeta{
+							Name: "adv1",
+						},
+						Spec: v1beta1.BGPAdvertisementSpec{
+							AggregationLength: pointer.Int32Ptr(32),
+							LocalPref:         uint32(100),
+							IPAddressPoolSelectors: []metav1.LabelSelector{
+								{
+									MatchLabels: map[string]string{
+										"test": "pool1",
+									},
+								},
+								{
+									MatchLabels: map[string]string{
+										"test": "pool1",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
+		{
+			desc: "use duplicate ip pool selectors - in L2 adv",
+			crs: ClusterResources{
+				Pools: []v1beta1.IPAddressPool{
+					{
+						ObjectMeta: v1.ObjectMeta{
+							Name:   "pool1",
+							Labels: map[string]string{"test": "pool1"},
+						},
+						Spec: v1beta1.IPAddressPoolSpec{
+							Addresses: []string{
+								"10.20.0.0/16",
+							},
+						},
+					},
+				},
+				L2Advs: []v1beta1.L2Advertisement{
+					{
+						ObjectMeta: v1.ObjectMeta{
+							Name: "adv1",
+						},
+						Spec: v1beta1.L2AdvertisementSpec{
+							IPAddressPoolSelectors: []metav1.LabelSelector{
+								{
+									MatchLabels: map[string]string{
+										"test": "pool1",
+									},
+								},
+								{
+									MatchLabels: map[string]string{
+										"test": "pool1",
+									},
+								},
+							},
+						},
+					},
+				},
+			},
+		},
 		{
 			desc: "use non existent label for ip pool selectors",
 			crs: ClusterResources{
@@ -2072,6 +2207,84 @@ func TestParse(t *testing.T) {
 				BFDProfiles: map[string]*BFDProfile{},
 			},
 		},
+		{
+			desc: "use duplicate node selectors",
+			crs: ClusterResources{
+				Pools: []v1beta1.IPAddressPool{
+					{
+						ObjectMeta: v1.ObjectMeta{
+							Name: "pool1",
+						},
+						Spec: v1beta1.IPAddressPoolSpec{
+							Addresses: []string{
+								"10.20.0.0/16",
+							},
+						},
+					},
+				},
+				BGPAdvs: []v1beta1.BGPAdvertisement{
+					{
+						ObjectMeta: v1.ObjectMeta{
+							Name: "adv1",
+						},
+						Spec: v1beta1.BGPAdvertisementSpec{
+							IPAddressPools: []string{"pool1"},
+							NodeSelectors: []metav1.LabelSelector{
+								{
+									MatchLabels: map[string]string{
+										"second": "true",
+									},
+								},
+								{
+									MatchLabels: map[string]string{
+										"second": "true",
+									},
+								},
+							},
+						},
+					},
+				},
+				L2Advs: []v1beta1.L2Advertisement{
+					{
+						ObjectMeta: v1.ObjectMeta{
+							Name: "l2adv1",
+						},
+						Spec: v1beta1.L2AdvertisementSpec{
+							NodeSelectors: []metav1.LabelSelector{
+								{
+									MatchLabels: map[string]string{
+										"first": "true",
+									},
+								},
+								{
+									MatchLabels: map[string]string{
+										"first": "true",
+									},
+								},
+							},
+							IPAddressPools: []string{"pool1"},
+						},
+					},
+				},
+				Nodes: []corev1.Node{
+					{
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "first",
+							Labels: map[string]string{
+								"first": "true",
+							},
+						},
+					}, {
+						ObjectMeta: metav1.ObjectMeta{
+							Name: "second",
+							Labels: map[string]string{
+								"second": "true",
+							},
+						},
+					},
+				},
+			},
+		},
 		{
 			desc: "no nodes means all nodes",
 			crs: ClusterResources{