Repository: metallb/metallb
Pull request #1130 (2021-12-16 15:20:42)

Check aggregation length against the wider cidr generated from range
If we specify addresses in a pool by range (and not by cidr), the check
that the lenght of the cidr is bigger than the aggregation lenght is
performed against all the possible cidrs that range translates to, and
it is misleading. The check makes sense when we declare a cidr, because
we know we won that whole cidr, but in the other case we may be using a
subset of a cidr for other reasons, and the check here leads to false
negatives.

A scenario like the one in the test that is being added should not fail:
3.3.3.2-3.3.3.254 with aggregation length 26



msherif1234: its very odd that we have check for one addresspool fmt but not the other, look like a kludge to me with either remove the check or find a better way to do it 
fedepaol: > its very odd that we have check for one addresspool fmt but not the other, look like a kludge to me, we either remove the check or find a better way to do it

Yep, but I think adding the check when specifying a cidr has some value. Another thing we can do is, when we have multiple cidrs to filter against the largest one. It will be more correct, probably as much as clunky.
oribon: lgtm

Patch:
diff --git a/internal/config/config.go b/internal/config/config.go
index fa83c4e7..f8c885a1 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -424,21 +424,23 @@ func parseAddressPool(p addressPool, bgpCommunities map[string]uint32) (*Pool, e
 	if len(p.Addresses) == 0 {
 		return nil, errors.New("pool has no prefixes defined")
 	}
+
+	cidrsPerAddresses := map[string][]*net.IPNet{}
 	for _, cidr := range p.Addresses {
 		nets, err := ParseCIDR(cidr)
 		if err != nil {
 			return nil, fmt.Errorf("invalid CIDR %q in pool %q: %s", cidr, p.Name, err)
 		}
 		ret.CIDR = append(ret.CIDR, nets...)
+		cidrsPerAddresses[cidr] = nets
 	}
-
 	switch ret.Protocol {
 	case Layer2:
 		if len(p.BGPAdvertisements) > 0 {
 			return nil, errors.New("cannot have bgp-advertisements configuration element in a layer2 address pool")
 		}
 	case BGP:
-		ads, err := parseBGPAdvertisements(p.BGPAdvertisements, ret.CIDR, bgpCommunities)
+		ads, err := parseBGPAdvertisements(p.BGPAdvertisements, cidrsPerAddresses, bgpCommunities)
 		if err != nil {
 			return nil, fmt.Errorf("parsing BGP communities: %s", err)
 		}
@@ -485,7 +487,7 @@ func parseBFDProfile(p bfdProfile) (*BFDProfile, error) {
 	return res, nil
 }
 
-func parseBGPAdvertisements(ads []bgpAdvertisement, cidrs []*net.IPNet, communities map[string]uint32) ([]*BGPAdvertisement, error) {
+func parseBGPAdvertisements(ads []bgpAdvertisement, cidrsPerAddresses map[string][]*net.IPNet, communities map[string]uint32) ([]*BGPAdvertisement, error) {
 	if len(ads) == 0 {
 		return []*BGPAdvertisement{
 			{
@@ -519,15 +521,21 @@ func parseBGPAdvertisements(ads []bgpAdvertisement, cidrs []*net.IPNet, communit
 			}
 		}
 
-		for _, cidr := range cidrs {
-			o, _ := cidr.Mask.Size()
+		for addr, cidrs := range cidrsPerAddresses {
+			if len(cidrs) == 0 {
+				continue
+			}
 			maxLength := ad.AggregationLength
-			if cidr.IP.To4() == nil {
+			if cidrs[0].IP.To4() == nil {
 				maxLength = ad.AggregationLengthV6
 			}
-			if maxLength < o {
+
+			// in case of range format, we may have a set of cidrs associated to a given address.
+			// We reject if none of the cidrs are compatible with the aggregation length.
+			lowest := lowestMask(cidrs)
+			if maxLength < lowest {
 				return nil, fmt.Errorf("invalid aggregation length %d: prefix %q in "+
-					"this pool is more specific than the aggregation length", ad.AggregationLength, cidr)
+					"this pool is more specific than the aggregation length for addresses %s", ad.AggregationLength, lowest, addr)
 			}
 		}
 
@@ -629,6 +637,20 @@ func cidrContainsCIDR(outer, inner *net.IPNet) bool {
 	return false
 }
 
+func lowestMask(cidrs []*net.IPNet) int {
+	if len(cidrs) == 0 {
+		return 0
+	}
+	lowest, _ := cidrs[0].Mask.Size()
+	for _, c := range cidrs {
+		s, _ := c.Mask.Size()
+		if lowest > s {
+			lowest = s
+		}
+	}
+	return lowest
+}
+
 func bfdIntFromConfig(value *uint32, min, max uint32) (*uint32, error) {
 	if value == nil {
 		return nil, nil


Test patch:
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index 757aca25..fcbcba3a 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -523,6 +523,63 @@ address-pools:
 `,
 		},
 
+		{
+			desc: "aggregation length by range",
+			raw: `
+address-pools:
+- name: pool1
+  protocol: bgp
+  addresses:
+  - 3.3.3.2-3.3.3.254
+  bgp-advertisements:
+  - aggregation-length: 26
+`,
+			want: &Config{
+				Pools: map[string]*Pool{
+					"pool1": {
+						Protocol:   BGP,
+						AutoAssign: true,
+						CIDR: []*net.IPNet{
+							ipnet("3.3.3.2/31"),
+							ipnet("3.3.3.4/30"),
+							ipnet("3.3.3.8/29"),
+							ipnet("3.3.3.16/28"),
+							ipnet("3.3.3.32/27"),
+							ipnet("3.3.3.64/26"),
+							ipnet("3.3.3.128/26"),
+							ipnet("3.3.3.192/27"),
+							ipnet("3.3.3.224/28"),
+							ipnet("3.3.3.240/29"),
+							ipnet("3.3.3.248/30"),
+							ipnet("3.3.3.252/31"),
+							ipnet("3.3.3.254/32"),
+						},
+						BGPAdvertisements: []*BGPAdvertisement{
+							{
+								AggregationLength:   26,
+								AggregationLengthV6: 128,
+								Communities:         map[uint32]bool{},
+							},
+						},
+					},
+				},
+				BFDProfiles: map[string]*BFDProfile{},
+			},
+		},
+
+		{
+			desc: "aggregation length by range, too wide",
+			raw: `
+address-pools:
+- name: pool1
+  protocol: bgp
+  addresses:
+  - 3.3.3.2-3.3.3.254
+  bgp-advertisements:
+  - aggregation-length: 24
+`,
+		},
+
 		{
 			desc: "bad community literal (wrong format)",
 			raw: `