Repository: metallb/metallb
Pull request #1515 (2022-07-18 09:50:08)

Add avoidBuggyIPs to the IPAddressPool CIDR
We originally thought that the avoidBuggyIPs flag was replaceable by picking the equivalent range of the cidr minus the extremes.
The issue is, the aggregation length validation won't work in that case because a 10.0.1.1-10.1.1.254 range doesn't correspond to a /24 cidr.
Because of that, we believe that bringing the flag back is safer than trying to understand if a range belongs to a cidr minus the buggy ips (and if the user's intention was to avoid them in first place).


Patch:
diff --git a/api/v1beta1/ipaddresspool_types.go b/api/v1beta1/ipaddresspool_types.go
index 5a029df9..5030400f 100644
--- a/api/v1beta1/ipaddresspool_types.go
+++ b/api/v1beta1/ipaddresspool_types.go
@@ -33,6 +33,12 @@ type IPAddressPoolSpec struct {
 	// +optional
 	// +kubebuilder:default:=true
 	AutoAssign *bool `json:"autoAssign,omitempty"`
+
+	// AvoidBuggyIPs prevents addresses ending with .0 and .255
+	// to be used by a pool.
+	// +optional
+	// +kubebuilder:default:=false
+	AvoidBuggyIPs bool `json:"avoidBuggyIPs,omitempty"`
 }
 
 // IPAddressPoolStatus defines the observed state of IPAddressPool.
diff --git a/charts/metallb/charts/crds/templates/crds.yaml b/charts/metallb/charts/crds/templates/crds.yaml
index 81be6c7a..6eeef634 100644
--- a/charts/metallb/charts/crds/templates/crds.yaml
+++ b/charts/metallb/charts/crds/templates/crds.yaml
@@ -1,6 +1,3 @@
----
-
----
 apiVersion: apiextensions.k8s.io/v1
 kind: CustomResourceDefinition
 metadata:
@@ -845,6 +842,11 @@ spec:
                 description: AutoAssign flag used to prevent MetallB from automatic
                   allocation for a pool.
                 type: boolean
+              avoidBuggyIPs:
+                default: false
+                description: AvoidBuggyIPs prevents addresses ending with .0 and .255
+                  to be used by a pool.
+                type: boolean
             required:
             - addresses
             type: object
diff --git a/config/crd/bases/metallb.io_ipaddresspools.yaml b/config/crd/bases/metallb.io_ipaddresspools.yaml
index 4029039d..2c11b085 100644
--- a/config/crd/bases/metallb.io_ipaddresspools.yaml
+++ b/config/crd/bases/metallb.io_ipaddresspools.yaml
@@ -50,6 +50,11 @@ spec:
                 description: AutoAssign flag used to prevent MetallB from automatic
                   allocation for a pool.
                 type: boolean
+              avoidBuggyIPs:
+                default: false
+                description: AvoidBuggyIPs prevents addresses ending with .0 and .255
+                  to be used by a pool.
+                type: boolean
             required:
             - addresses
             type: object
diff --git a/config/manifests/metallb-frr.yaml b/config/manifests/metallb-frr.yaml
index 3ad00156..bc4327ab 100644
--- a/config/manifests/metallb-frr.yaml
+++ b/config/manifests/metallb-frr.yaml
@@ -912,6 +912,11 @@ spec:
                 description: AutoAssign flag used to prevent MetallB from automatic
                   allocation for a pool.
                 type: boolean
+              avoidBuggyIPs:
+                default: false
+                description: AvoidBuggyIPs prevents addresses ending with .0 and .255
+                  to be used by a pool.
+                type: boolean
             required:
             - addresses
             type: object
diff --git a/config/manifests/metallb-native.yaml b/config/manifests/metallb-native.yaml
index 625ac85a..3128893e 100644
--- a/config/manifests/metallb-native.yaml
+++ b/config/manifests/metallb-native.yaml
@@ -912,6 +912,11 @@ spec:
                 description: AutoAssign flag used to prevent MetallB from automatic
                   allocation for a pool.
                 type: boolean
+              avoidBuggyIPs:
+                default: false
+                description: AvoidBuggyIPs prevents addresses ending with .0 and .255
+                  to be used by a pool.
+                type: boolean
             required:
             - addresses
             type: object
diff --git a/configmaptocrs/main.go b/configmaptocrs/main.go
index e37165fb..97a5aef5 100644
--- a/configmaptocrs/main.go
+++ b/configmaptocrs/main.go
@@ -8,13 +8,10 @@ import (
 	"io"
 	"io/ioutil"
 	"log"
-	"net"
 	"os"
 	"path/filepath"
-	"strings"
 	"time"
 
-	"github.com/mikioh/ipaddr"
 	"go.universe.tf/metallb/api/v1beta1"
 	"go.universe.tf/metallb/api/v1beta2"
 	"go.universe.tf/metallb/internal/config"
@@ -325,13 +322,10 @@ func ipAddressPoolsFor(c *configFile) ([]v1beta1.IPAddressPool, error) {
 		var ap v1beta1.IPAddressPool
 		ap.Name = addresspool.Name
 		ap.Namespace = resourcesNameSpace
-		ap.Spec.Addresses = make([]string, 0)
-		for _, address := range addresspool.Addresses {
-			a, err := configRangeToPoolAddresses(address, addresspool.AvoidBuggyIPs)
-			if err != nil {
-				return nil, err
-			}
-			ap.Spec.Addresses = append(ap.Spec.Addresses, a)
+		ap.Spec.Addresses = make([]string, len(addresspool.Addresses))
+		copy(ap.Spec.Addresses, addresspool.Addresses)
+		if addresspool.AvoidBuggyIPs != nil {
+			ap.Spec.AvoidBuggyIPs = *addresspool.AvoidBuggyIPs
 		}
 		ap.Spec.AutoAssign = addresspool.AutoAssign
 		res[i] = ap
@@ -339,36 +333,6 @@ func ipAddressPoolsFor(c *configFile) ([]v1beta1.IPAddressPool, error) {
 	return res, nil
 }
 
-func configRangeToPoolAddresses(addresses string, avoidBuggyIPs *bool) (string, error) {
-	if avoidBuggyIPs == nil {
-		return addresses, nil
-	}
-	if !*avoidBuggyIPs {
-		return addresses, nil
-	}
-	if strings.Contains(addresses, "-") {
-		return addresses, nil
-	}
-	_, cidr, err := net.ParseCIDR(addresses)
-	if err != nil {
-		return "", err
-	}
-	c := ipaddr.NewCursor([]ipaddr.Prefix{*ipaddr.NewPrefix(cidr)})
-	second := c.Next()
-	if second == nil {
-		return "", fmt.Errorf("not enough addresses")
-	}
-	err = c.Set(c.Last())
-	if err != nil {
-		return "", err
-	}
-	secondToLast := c.Prev()
-	if secondToLast == nil {
-		return "", fmt.Errorf("not enough addresses")
-	}
-	return fmt.Sprintf("%s-%s", second.IP.String(), secondToLast.IP.String()), nil
-}
-
 func bgpAdvertisementsFor(c *configFile) []v1beta1.BGPAdvertisement {
 	res := make([]v1beta1.BGPAdvertisement, 0)
 	index := 1
diff --git a/internal/allocator/allocator.go b/internal/allocator/allocator.go
index cadd6ad6..ff653c21 100644
--- a/internal/allocator/allocator.go
+++ b/internal/allocator/allocator.go
@@ -252,7 +252,7 @@ func (a *Allocator) AllocateFromPool(svc string, serviceIPFamily ipfamily.Family
 			// Not the right ip-family
 			continue
 		}
-		ip := a.getIPFromCIDR(cidr, svc, ports, sharingKey, backendKey)
+		ip := a.getIPFromCIDR(cidr, pool.AvoidBuggyIPs, svc, ports, sharingKey, backendKey)
 		if ip != nil {
 			ips = append(ips, ip)
 			delete(ipfamilySel, cidrIPFamily)
@@ -327,6 +327,28 @@ func poolCount(p *config.Pool) int64 {
 			return math.MaxInt64
 		}
 		sz := int64(math.Pow(2, float64(b-o)))
+
+		cur := ipaddr.NewCursor([]ipaddr.Prefix{*ipaddr.NewPrefix(cidr)})
+		firstIP := cur.First().IP
+		lastIP := cur.Last().IP
+
+		if p.AvoidBuggyIPs {
+			if o <= 24 {
+				// A pair of buggy IPs occur for each /24 present in the range.
+				buggies := int64(math.Pow(2, float64(24-o))) * 2
+				sz -= buggies
+			} else {
+				// Ranges smaller than /24 contain 1 buggy IP if they
+				// start/end on a /24 boundary, otherwise they contain
+				// none.
+				if ipConfusesBuggyFirmwares(firstIP) {
+					sz--
+				}
+				if ipConfusesBuggyFirmwares(lastIP) {
+					sz--
+				}
+			}
+		}
 		total += sz
 	}
 	return total
@@ -337,6 +359,9 @@ func poolFor(pools map[string]*config.Pool, ips []net.IP) string {
 	for pname, p := range pools {
 		cnt := 0
 		for _, ip := range ips {
+			if p.AvoidBuggyIPs && ipConfusesBuggyFirmwares(ip) {
+				continue
+			}
 			for _, cidr := range p.CIDR {
 				if cidr.Contains(ip) {
 					cnt++
@@ -351,13 +376,28 @@ func poolFor(pools map[string]*config.Pool, ips []net.IP) string {
 	return ""
 }
 
-func (a *Allocator) getIPFromCIDR(cidr *net.IPNet, svc string, ports []Port, sharingKey, backendKey string) net.IP {
+// ipConfusesBuggyFirmwares returns true if ip is an IPv4 address ending in 0 or 255.
+//
+// Such addresses can confuse smurf protection on crappy CPE
+// firmwares, leading to packet drops.
+func ipConfusesBuggyFirmwares(ip net.IP) bool {
+	ip = ip.To4()
+	if ip == nil {
+		return false
+	}
+	return ip[3] == 0 || ip[3] == 255
+}
+
+func (a *Allocator) getIPFromCIDR(cidr *net.IPNet, avoidBuggyIPs bool, svc string, ports []Port, sharingKey, backendKey string) net.IP {
 	sk := &key{
 		sharing: sharingKey,
 		backend: backendKey,
 	}
 	c := ipaddr.NewCursor([]ipaddr.Prefix{*ipaddr.NewPrefix(cidr)})
 	for pos := c.First(); pos != nil; pos = c.Next() {
+		if avoidBuggyIPs && ipConfusesBuggyFirmwares(pos.IP) {
+			continue
+		}
 		if a.checkSharing(svc, pos.IP.String(), ports, sk) != nil {
 			continue
 		}
diff --git a/internal/config/config.go b/internal/config/config.go
index 38f20d02..b228a638 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -107,7 +107,12 @@ type Pool struct {
 	// prefixes. config.Parse guarantees that these are
 	// non-overlapping, both within and between pools.
 	CIDR []*net.IPNet
-
+	// Some buggy consumer devices mistakenly drop IPv4 traffic for IP
+	// addresses ending in .0 or .255, due to poor implementations of
+	// smurf protection. This setting marks such addresses as
+	// unusable, for maximum compatibility with ancient parts of the
+	// internet.
+	AvoidBuggyIPs bool
 	// If false, prevents IP addresses to be automatically assigned
 	// from this pool.
 	AutoAssign bool
@@ -439,7 +444,8 @@ func addressPoolFromCR(p metallbv1beta1.IPAddressPool) (*Pool, error) {
 	}
 
 	ret := &Pool{
-		AutoAssign: true,
+		AvoidBuggyIPs: p.Spec.AvoidBuggyIPs,
+		AutoAssign:    true,
 	}
 
 	if p.Spec.AutoAssign != nil {
diff --git a/website/content/apis/_index.md b/website/content/apis/_index.md
index 44395e9a..83911c1d 100644
--- a/website/content/apis/_index.md
+++ b/website/content/apis/_index.md
@@ -509,6 +509,19 @@ bool
 for a pool.</p>
 </td>
 </tr>
+<tr>
+<td>
+<code>avoidBuggyIPs</code><br/>
+<em>
+bool
+</em>
+</td>
+<td>
+<em>(Optional)</em>
+<p>AvoidBuggyIPs prevents addresses ending with .0 and .255
+to be used by a pool.</p>
+</td>
+</tr>
 </table>
 </td>
 </tr>
diff --git a/website/content/release-notes/_index.md b/website/content/release-notes/_index.md
index 6dc88434..09a9443f 100644
--- a/website/content/release-notes/_index.md
+++ b/website/content/release-notes/_index.md
@@ -11,6 +11,9 @@ Bug Fixes:
 - L2 mode error with ipv4 only interfaces with linklocal addresses ([Issue 1507](https://github.com/metallb/metallb/issues/1507) , [PR 1506](https://github.com/metallb/metallb/pull/1506))
 - L2 Mode: get back to announce on interfaces with no IP assigned. Vlans with no IPs should be able to advertise
 the service ([Issue 1511](https://github.com/metallb/metallb/issues/1511) [PR 1516](https://github.com/metallb/metallb/pull/1516))
+- Add the AvoidBuggyIPs flag to the IPAddressPool CRD. Converting a CIDR to a range comes with limitation related
+to setting the aggregation length and validating it. ([Issue 1495](https://github.com/metallb/metallb/issues/1495),
+[PR 1515](https://github.com/metallb/metallb/pull/1515)
 
 ## Version 0.13.3
 


Test patch:
diff --git a/configmaptocrs/conversion_test.go b/configmaptocrs/conversion_test.go
index 28feae4d..7df175b8 100644
--- a/configmaptocrs/conversion_test.go
+++ b/configmaptocrs/conversion_test.go
@@ -40,10 +40,6 @@ func TestGenerateResources(t *testing.T) {
 
 			err := generate(res, tc)
 
-			if strings.Contains(tc, "bad") && !strings.Contains(err.Error(), "not enough addresses") {
-				t.Fatalf("test %s failed, expecting error: %s", tc, err.Error())
-				return
-			}
 			if err != nil && !strings.Contains(tc, "bad") {
 				t.Fatalf("test %s failed to generate resources: %s", tc, err)
 			}
diff --git a/configmaptocrs/testdata/config-avoidbuggy-bad.golden b/configmaptocrs/testdata/config-avoidbuggy-bad.golden
deleted file mode 100644
index e69de29b..00000000
diff --git a/configmaptocrs/testdata/config-avoidbuggy-bad.yaml b/configmaptocrs/testdata/config-avoidbuggy-bad.yaml
deleted file mode 100644
index c8d75913..00000000
--- a/configmaptocrs/testdata/config-avoidbuggy-bad.yaml
+++ /dev/null
@@ -1,13 +0,0 @@
-apiVersion: v1
-kind: ConfigMap
-metadata:
-  namespace: metallb-system
-  name: config
-data:
-  config: |
-    address-pools:
-    - name: my-ip-space
-      protocol: bgp
-      addresses:
-      - 198.51.100.0/32
-      avoid-buggy-ips: true
diff --git a/configmaptocrs/testdata/config-avoidbuggy.golden b/configmaptocrs/testdata/config-avoidbuggy.golden
index ab1f698d..e0d16c43 100644
--- a/configmaptocrs/testdata/config-avoidbuggy.golden
+++ b/configmaptocrs/testdata/config-avoidbuggy.golden
@@ -7,8 +7,9 @@ metadata:
   namespace: metallb-system
 spec:
   addresses:
-  - 198.51.100.1-198.51.100.254
-  - fc00:f853:ccd:e799::1-fc00:f853:ccd:e799::e
+  - 198.51.100.0/24
+  - fc00:f853:0ccd:e799::/124
+  avoidBuggyIPs: true
 status: {}
 ---
 apiVersion: metallb.io/v1beta1
diff --git a/configmaptocrs/testdata/full-config.golden b/configmaptocrs/testdata/full-config.golden
index 5c26a4f9..6d3c14ba 100644
--- a/configmaptocrs/testdata/full-config.golden
+++ b/configmaptocrs/testdata/full-config.golden
@@ -36,9 +36,10 @@ metadata:
   namespace: metallb-system
 spec:
   addresses:
-  - 192.168.10.1-192.168.10.254
+  - 192.168.10.0/24
   - 192.168.0.150-192.168.0.200
   autoAssign: true
+  avoidBuggyIPs: true
 status: {}
 ---
 apiVersion: metallb.io/v1beta1
@@ -49,8 +50,9 @@ metadata:
   namespace: metallb-system
 spec:
   addresses:
-  - 192.168.11.1-192.168.11.254
+  - 192.168.11.0/24
   - 192.168.1.150-192.168.1.200
+  avoidBuggyIPs: true
 status: {}
 ---
 apiVersion: metallb.io/v1beta1
diff --git a/e2etest/pkg/config/validate.go b/e2etest/pkg/config/validate.go
index 3f9d8aee..03504620 100644
--- a/e2etest/pkg/config/validate.go
+++ b/e2etest/pkg/config/validate.go
@@ -68,8 +68,39 @@ func PoolCount(p metallbv1beta1.IPAddressPool) (int64, error) {
 				return math.MaxInt64, nil
 			}
 			sz := int64(math.Pow(2, float64(b-o)))
+
+			cur := ipaddr.NewCursor([]ipaddr.Prefix{*ipaddr.NewPrefix(cidr)})
+			firstIP := cur.First().IP
+			lastIP := cur.Last().IP
+
+			if p.Spec.AvoidBuggyIPs {
+				if o <= 24 {
+					// A pair of buggy IPs occur for each /24 present in the range.
+					buggies := int64(math.Pow(2, float64(24-o))) * 2
+					sz -= buggies
+				} else {
+					// Ranges smaller than /24 contain 1 buggy IP if they
+					// start/end on a /24 boundary, otherwise they contain
+					// none.
+					if ipConfusesBuggyFirmwares(firstIP) {
+						sz--
+					}
+					if ipConfusesBuggyFirmwares(lastIP) {
+						sz--
+					}
+				}
+			}
+
 			total += sz
 		}
 	}
 	return total, nil
 }
+
+func ipConfusesBuggyFirmwares(ip net.IP) bool {
+	ip = ip.To4()
+	if ip == nil {
+		return false
+	}
+	return ip[3] == 0 || ip[3] == 255
+}
diff --git a/internal/allocator/allocator_test.go b/internal/allocator/allocator_test.go
index 8da1b34f..bf2abab7 100644
--- a/internal/allocator/allocator_test.go
+++ b/internal/allocator/allocator_test.go
@@ -27,7 +27,8 @@ func TestAssignment(t *testing.T) {
 			},
 		},
 		"test2": {
-			AutoAssign: true,
+			AvoidBuggyIPs: true,
+			AutoAssign:    true,
 			CIDR: []*net.IPNet{
 				ipnet("1.2.4.0/24"),
 				ipnet("1000::4:0/120"),
@@ -88,6 +89,18 @@ func TestAssignment(t *testing.T) {
 			svc:  "s1",
 			ips:  []string{"1.2.3.5"},
 		},
+		{
+			desc:    "s3 cannot grab a 0 buggy IP",
+			svc:     "s3",
+			ips:     []string{"1.2.4.0"},
+			wantErr: true,
+		},
+		{
+			desc:    "s3 cannot grab a 255 buggy IP",
+			svc:     "s3",
+			ips:     []string{"1.2.4.255"},
+			wantErr: true,
+		},
 		{
 			desc: "s3 can grab another IP in that pool",
 			svc:  "s3",
@@ -1100,12 +1113,14 @@ func TestBuggyIPs(t *testing.T) {
 			CIDR:       []*net.IPNet{ipnet("1.2.3.254/31")},
 		},
 		"test3": {
-			AutoAssign: true,
-			CIDR:       []*net.IPNet{ipnet("1.2.4.0/31")},
+			AvoidBuggyIPs: true,
+			AutoAssign:    true,
+			CIDR:          []*net.IPNet{ipnet("1.2.4.0/31")},
 		},
 		"test4": {
-			AutoAssign: true,
-			CIDR:       []*net.IPNet{ipnet("1.2.4.254/31")},
+			AvoidBuggyIPs: true,
+			AutoAssign:    true,
+			CIDR:          []*net.IPNet{ipnet("1.2.4.254/31")},
 		},
 	}); err != nil {
 		t.Fatalf("SetPools: %s", err)
@@ -1118,8 +1133,6 @@ func TestBuggyIPs(t *testing.T) {
 		"1.2.3.255": true,
 		"1.2.4.1":   true,
 		"1.2.4.254": true,
-		"1.2.4.255": true,
-		"1.2.4.0":   true,
 	}
 
 	tests := []struct {
@@ -1132,7 +1145,10 @@ func TestBuggyIPs(t *testing.T) {
 		{svc: "s4"},
 		{svc: "s5"},
 		{svc: "s6"},
-		{svc: "s7"},
+		{
+			svc:     "s7",
+			wantErr: true,
+		},
 	}
 
 	for i, test := range tests {
@@ -1300,6 +1316,18 @@ func TestConfigReload(t *testing.T) {
 			},
 			pool: "test2",
 		},
+		{
+			desc: "enable buggy IPs not allowed",
+			pools: map[string]*config.Pool{
+				"test2": {
+					AutoAssign:    true,
+					AvoidBuggyIPs: true,
+					CIDR:          []*net.IPNet{ipnet("1.2.3.0/31"), ipnet("1000::/127")},
+				},
+			},
+			pool:    "test2",
+			wantErr: true,
+		},
 	}
 
 	for _, test := range tests {
@@ -1516,10 +1544,19 @@ func TestPoolCount(t *testing.T) {
 			},
 			want: 384,
 		},
+		{
+			desc: "BGP /24 and /25, no buggy IPs",
+			pool: &config.Pool{
+				CIDR:          []*net.IPNet{ipnet("1.2.3.0/24"), ipnet("2.3.4.128/25")},
+				AvoidBuggyIPs: true,
+			},
+			want: 381,
+		},
 		{
 			desc: "BGP a BIG ipv6 range",
 			pool: &config.Pool{
-				CIDR: []*net.IPNet{ipnet("1.2.3.0/24"), ipnet("2.3.4.128/25"), ipnet("1000::/64")},
+				CIDR:          []*net.IPNet{ipnet("1.2.3.0/24"), ipnet("2.3.4.128/25"), ipnet("1000::/64")},
+				AvoidBuggyIPs: true,
 			},
 			want: math.MaxInt64,
 		},
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index d8f35b84..6cecd4ff 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -113,7 +113,8 @@ func TestParse(t *testing.T) {
 								"10.20.0.0/16",
 								"10.50.0.0/24",
 							},
-							AutoAssign: pointer.BoolPtr(false),
+							AvoidBuggyIPs: true,
+							AutoAssign:    pointer.BoolPtr(false),
 						},
 					},
 					{
@@ -241,8 +242,9 @@ func TestParse(t *testing.T) {
 				},
 				Pools: map[string]*Pool{
 					"pool1": {
-						CIDR:       []*net.IPNet{ipnet("10.20.0.0/16"), ipnet("10.50.0.0/24")},
-						AutoAssign: false,
+						CIDR:          []*net.IPNet{ipnet("10.20.0.0/16"), ipnet("10.50.0.0/24")},
+						AvoidBuggyIPs: true,
+						AutoAssign:    false,
 						BGPAdvertisements: []*BGPAdvertisement{
 							{
 								AggregationLength:   32,
@@ -1552,7 +1554,8 @@ func TestParse(t *testing.T) {
 								"10.20.0.0/16",
 								"10.50.0.0/24",
 							},
-							AutoAssign: pointer.BoolPtr(false),
+							AvoidBuggyIPs: true,
+							AutoAssign:    pointer.BoolPtr(false),
 						},
 					},
 				},
@@ -1622,8 +1625,9 @@ func TestParse(t *testing.T) {
 				},
 				Pools: map[string]*Pool{
 					"pool1": {
-						CIDR:       []*net.IPNet{ipnet("10.20.0.0/16"), ipnet("10.50.0.0/24")},
-						AutoAssign: false,
+						CIDR:          []*net.IPNet{ipnet("10.20.0.0/16"), ipnet("10.50.0.0/24")},
+						AvoidBuggyIPs: true,
+						AutoAssign:    false,
 						BGPAdvertisements: []*BGPAdvertisement{
 							{
 								AggregationLength:   32,
@@ -1735,7 +1739,8 @@ func TestParse(t *testing.T) {
 								"10.20.0.0/16",
 								"10.50.0.0/24",
 							},
-							AutoAssign: pointer.BoolPtr(false),
+							AvoidBuggyIPs: true,
+							AutoAssign:    pointer.BoolPtr(false),
 						},
 					},
 				},