Repository: spidernet-io/spiderpool
Pull request #1673 (2023-04-17 00:41:43)

title:	fix: The gateway addresses of SpiderSubnet and SpiderIPPool conflict with `spec.ips`
robot cherry pick pr <https://github.com/spidernet-io/spiderpool/pull/1671> to branch release-v0.4,  action <https://github.com/spidernet-io/spiderpool/actions/runs/4716239395>  , commits 88328ae4f619b211c4f3e6101f0b46b41a8ec350  


Patch:
diff --git a/pkg/ip/iprange.go b/pkg/ip/iprange.go
index 2f0d5aed8..cb6a5bffd 100644
--- a/pkg/ip/iprange.go
+++ b/pkg/ip/iprange.go
@@ -141,6 +141,33 @@ func ContainsIPRange(version types.IPVersion, subnet string, ipRange string) (bo
 	return ipNet.Contains(ips[0]) && ipNet.Contains(ips[n-1]), nil
 }
 
+// IPRangeContainsIP reports whether the IP range includes the IP address.
+// Both must belongto the same IP version.
+func IPRangeContainsIP(version types.IPVersion, ipRange string, ip string) (bool, error) {
+	if err := IsIPRange(version, ipRange); err != nil {
+		return false, err
+	}
+	if err := IsIP(version, ip); err != nil {
+		return false, err
+	}
+
+	ips := strings.Split(ipRange, "-")
+	n := len(ips)
+
+	if n == 1 {
+		return ipRange == ip, nil
+	}
+
+	if n == 2 {
+		if Cmp(net.ParseIP(ips[0]), net.ParseIP(ip)) == 1 ||
+			Cmp(net.ParseIP(ips[1]), net.ParseIP(ip)) == -1 {
+			return false, nil
+		}
+	}
+
+	return true, nil
+}
+
 // IsIPRangeOverlap reports whether the IP address slices of specific IP
 // version parsed from two IP ranges overlap.
 func IsIPRangeOverlap(version types.IPVersion, ipRange1, ipRange2 string) (bool, error) {
diff --git a/pkg/ippoolmanager/ippool_validate.go b/pkg/ippoolmanager/ippool_validate.go
index eee920971..c45affb7a 100644
--- a/pkg/ippoolmanager/ippool_validate.go
+++ b/pkg/ippoolmanager/ippool_validate.go
@@ -100,7 +100,7 @@ func (iw *IPPoolWebhook) validateIPPoolSpec(ctx context.Context, ipPool *spiderp
 	if err := iw.validateIPPoolAvailableIPs(ctx, ipPool); err != nil {
 		return err
 	}
-	if err := validateIPPoolGateway(*ipPool.Spec.IPVersion, ipPool.Spec.Subnet, ipPool.Spec.Gateway); err != nil {
+	if err := validateIPPoolGateway(ipPool); err != nil {
 		return err
 	}
 
@@ -318,9 +318,31 @@ func validateIPPoolExcludeIPs(version types.IPVersion, subnet string, excludeIPs
 	return nil
 }
 
-func validateIPPoolGateway(version types.IPVersion, subnet string, gateway *string) *field.Error {
-	if gateway != nil {
-		return ValidateContainsIP(gatewayField, version, subnet, *gateway)
+func validateIPPoolGateway(ipPool *spiderpoolv2beta1.SpiderIPPool) *field.Error {
+	if ipPool.Spec.Gateway == nil {
+		return nil
+	}
+
+	if err := ValidateContainsIP(gatewayField, *ipPool.Spec.IPVersion, ipPool.Spec.Subnet, *ipPool.Spec.Gateway); err != nil {
+		return err
+	}
+
+	for _, r := range ipPool.Spec.ExcludeIPs {
+		contains, _ := spiderpoolip.IPRangeContainsIP(*ipPool.Spec.IPVersion, r, *ipPool.Spec.Gateway)
+		if contains {
+			return nil
+		}
+	}
+
+	for i, r := range ipPool.Spec.IPs {
+		contains, _ := spiderpoolip.IPRangeContainsIP(*ipPool.Spec.IPVersion, r, *ipPool.Spec.Gateway)
+		if contains {
+			return field.Invalid(
+				ipsField.Index(i),
+				r,
+				fmt.Sprintf("conflicts with 'spec.gateway' %s, add the gateway IP address to 'spec.excludeIPs' or remove it from 'spec.ips'", *ipPool.Spec.Gateway),
+			)
+		}
 	}
 
 	return nil
diff --git a/pkg/subnetmanager/subnet_validate.go b/pkg/subnetmanager/subnet_validate.go
index b157cc317..6cf01c969 100644
--- a/pkg/subnetmanager/subnet_validate.go
+++ b/pkg/subnetmanager/subnet_validate.go
@@ -100,7 +100,7 @@ func (sw *SubnetWebhook) validateSubnetSpec(ctx context.Context, subnet *spiderp
 	if err := validateSubnetExcludeIPs(*subnet.Spec.IPVersion, subnet.Spec.Subnet, subnet.Spec.ExcludeIPs); err != nil {
 		return err
 	}
-	if err := validateSubnetGateway(*subnet.Spec.IPVersion, subnet.Spec.Subnet, subnet.Spec.Gateway); err != nil {
+	if err := validateSubnetGateway(subnet); err != nil {
 		return err
 	}
 
@@ -235,9 +235,31 @@ func validateSubnetExcludeIPs(version types.IPVersion, subnet string, excludeIPs
 	return nil
 }
 
-func validateSubnetGateway(version types.IPVersion, subnet string, gateway *string) *field.Error {
-	if gateway != nil {
-		return ippoolmanager.ValidateContainsIP(gatewayField, version, subnet, *gateway)
+func validateSubnetGateway(subnet *spiderpoolv2beta1.SpiderSubnet) *field.Error {
+	if subnet.Spec.Gateway == nil {
+		return nil
+	}
+
+	if err := ippoolmanager.ValidateContainsIP(gatewayField, *subnet.Spec.IPVersion, subnet.Spec.Subnet, *subnet.Spec.Gateway); err != nil {
+		return err
+	}
+
+	for _, r := range subnet.Spec.ExcludeIPs {
+		contains, _ := spiderpoolip.IPRangeContainsIP(*subnet.Spec.IPVersion, r, *subnet.Spec.Gateway)
+		if contains {
+			return nil
+		}
+	}
+
+	for i, r := range subnet.Spec.IPs {
+		contains, _ := spiderpoolip.IPRangeContainsIP(*subnet.Spec.IPVersion, r, *subnet.Spec.Gateway)
+		if contains {
+			return field.Invalid(
+				ipsField.Index(i),
+				r,
+				fmt.Sprintf("conflicts with 'spec.gateway' %s, add the gateway IP address to 'spec.excludeIPs' or remove it from 'spec.ips'", *subnet.Spec.Gateway),
+			)
+		}
 	}
 
 	return nil


Test patch:
diff --git a/pkg/ip/iprange_test.go b/pkg/ip/iprange_test.go
index 46dd59d77..810ef8381 100644
--- a/pkg/ip/iprange_test.go
+++ b/pkg/ip/iprange_test.go
@@ -284,6 +284,64 @@ var _ = Describe("IP range", Label("ip_range_test"), func() {
 		})
 	})
 
+	Describe("Test IPRangeContainsIP", func() {
+		When("Verifying", func() {
+			It("inputs invalid IP version", func() {
+				contains, err := spiderpoolip.IPRangeContainsIP(constant.InvalidIPVersion, "172.18.40.1-172.18.40.2", "172.18.40.1")
+				Expect(err).To(MatchError(spiderpoolip.ErrInvalidIPVersion))
+				Expect(contains).To(BeFalse())
+			})
+
+			It("inputs invalid IP range", func() {
+				contains, err := spiderpoolip.IPRangeContainsIP(constant.IPv4, constant.InvalidIPRange, "172.18.40.1")
+				Expect(err).To(MatchError(spiderpoolip.ErrInvalidIPRangeFormat))
+				Expect(contains).To(BeFalse())
+			})
+
+			It("inputs invalid IP", func() {
+				contains, err := spiderpoolip.IPRangeContainsIP(constant.IPv4, "172.18.40.1-172.18.40.2", constant.InvalidIP)
+				Expect(err).To(MatchError(spiderpoolip.ErrInvalidIPFormat))
+				Expect(contains).To(BeFalse())
+			})
+		})
+
+		When("IPv4", func() {
+			It("tests that a IP range contains the IP", func() {
+				contains, err := spiderpoolip.IPRangeContainsIP(constant.IPv4, "172.18.40.1", "172.18.40.1")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(contains).To(BeTrue())
+
+				contains, err = spiderpoolip.IPRangeContainsIP(constant.IPv4, "172.18.40.1-172.18.40.2", "172.18.40.1")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(contains).To(BeTrue())
+			})
+
+			It("test that a IP range does not contain the IP", func() {
+				contains, err := spiderpoolip.IPRangeContainsIP(constant.IPv4, "172.18.40.1-172.18.40.2", "172.18.40.10")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(contains).To(BeFalse())
+			})
+		})
+
+		When("IPv6", func() {
+			It("tests that a IP range contains the IP", func() {
+				contains, err := spiderpoolip.IPRangeContainsIP(constant.IPv6, "abcd:1234::1", "abcd:1234::1")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(contains).To(BeTrue())
+
+				contains, err = spiderpoolip.IPRangeContainsIP(constant.IPv6, "abcd:1234::1-abcd:1234::2", "abcd:1234::1")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(contains).To(BeTrue())
+			})
+
+			It("test that a IP range does not contain the IP", func() {
+				contains, err := spiderpoolip.IPRangeContainsIP(constant.IPv6, "abcd:1234::1-abcd:1234::2", "abcd:1234::a")
+				Expect(err).NotTo(HaveOccurred())
+				Expect(contains).To(BeFalse())
+			})
+		})
+	})
+
 	Describe("Test IsIPRangeOverlap", func() {
 		When("Verifying", func() {
 			It("inputs invalid IP version", func() {
diff --git a/pkg/ippoolmanager/ippool_webhook_test.go b/pkg/ippoolmanager/ippool_webhook_test.go
index 1f40163b6..03b8fd282 100644
--- a/pkg/ippoolmanager/ippool_webhook_test.go
+++ b/pkg/ippoolmanager/ippool_webhook_test.go
@@ -878,6 +878,37 @@ var _ = Describe("IPPoolWebhook", Label("ippool_webhook_test"), func() {
 					err := ipPoolWebhook.ValidateCreate(ctx, ipPoolT)
 					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
+
+				It("duplicate with 'spec.ips'", func() {
+					ipPoolT.Spec.IPVersion = pointer.Int64(constant.IPv4)
+					ipPoolT.Spec.Subnet = "172.18.40.0/24"
+					ipPoolT.Spec.IPs = append(ipPoolT.Spec.IPs,
+						[]string{
+							"172.18.40.1-172.18.40.2",
+							"172.18.40.10",
+						}...,
+					)
+					ipPoolT.Spec.Gateway = pointer.String("172.18.40.1")
+
+					err := ipPoolWebhook.ValidateCreate(ctx, ipPoolT)
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
+				})
+
+				It("excludes gateway address through 'spec.excludeIPs'", func() {
+					ipPoolT.Spec.IPVersion = pointer.Int64(constant.IPv4)
+					ipPoolT.Spec.Subnet = "172.18.40.0/24"
+					ipPoolT.Spec.IPs = append(ipPoolT.Spec.IPs,
+						[]string{
+							"172.18.40.1-172.18.40.2",
+							"172.18.40.10",
+						}...,
+					)
+					ipPoolT.Spec.ExcludeIPs = append(ipPoolT.Spec.ExcludeIPs, "172.18.40.1")
+					ipPoolT.Spec.Gateway = pointer.String("172.18.40.1")
+
+					err := ipPoolWebhook.ValidateCreate(ctx, ipPoolT)
+					Expect(err).NotTo(HaveOccurred())
+				})
 			})
 
 			When("Validating 'spec.routes'", func() {
@@ -1422,11 +1453,10 @@ var _ = Describe("IPPoolWebhook", Label("ippool_webhook_test"), func() {
 					ipPoolT.Spec.Subnet = "172.18.40.0/24"
 					ipPoolT.Spec.IPs = append(ipPoolT.Spec.IPs,
 						[]string{
-							"172.18.40.2-172.18.40.3",
+							"172.18.40.1-172.18.40.2",
 							"172.18.40.10",
 						}...,
 					)
-					ipPoolT.Spec.Gateway = pointer.String("172.18.40.1")
 
 					newIPPoolT := ipPoolT.DeepCopy()
 					newIPPoolT.Spec.Gateway = pointer.String(constant.InvalidIP)
@@ -1440,11 +1470,10 @@ var _ = Describe("IPPoolWebhook", Label("ippool_webhook_test"), func() {
 					ipPoolT.Spec.Subnet = "172.18.40.0/24"
 					ipPoolT.Spec.IPs = append(ipPoolT.Spec.IPs,
 						[]string{
-							"172.18.40.2-172.18.40.3",
+							"172.18.40.1-172.18.40.2",
 							"172.18.40.10",
 						}...,
 					)
-					ipPoolT.Spec.Gateway = pointer.String("172.18.40.1")
 
 					newIPPoolT := ipPoolT.DeepCopy()
 					newIPPoolT.Spec.Gateway = pointer.String("172.18.41.1")
@@ -1452,6 +1481,41 @@ var _ = Describe("IPPoolWebhook", Label("ippool_webhook_test"), func() {
 					err := ipPoolWebhook.ValidateUpdate(ctx, ipPoolT, newIPPoolT)
 					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
+
+				It("duplicate with 'spec.ips'", func() {
+					ipPoolT.Spec.IPVersion = pointer.Int64(constant.IPv4)
+					ipPoolT.Spec.Subnet = "172.18.40.0/24"
+					ipPoolT.Spec.IPs = append(ipPoolT.Spec.IPs,
+						[]string{
+							"172.18.40.1-172.18.40.2",
+							"172.18.40.10",
+						}...,
+					)
+
+					newIPPoolT := ipPoolT.DeepCopy()
+					newIPPoolT.Spec.Gateway = pointer.String("172.18.40.1")
+
+					err := ipPoolWebhook.ValidateUpdate(ctx, ipPoolT, newIPPoolT)
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
+				})
+
+				It("excludes gateway address through 'spec.excludeIPs'", func() {
+					ipPoolT.Spec.IPVersion = pointer.Int64(constant.IPv4)
+					ipPoolT.Spec.Subnet = "172.18.40.0/24"
+					ipPoolT.Spec.IPs = append(ipPoolT.Spec.IPs,
+						[]string{
+							"172.18.40.1-172.18.40.2",
+							"172.18.40.10",
+						}...,
+					)
+					ipPoolT.Spec.ExcludeIPs = append(ipPoolT.Spec.ExcludeIPs, "172.18.40.1")
+
+					newIPPoolT := ipPoolT.DeepCopy()
+					newIPPoolT.Spec.Gateway = pointer.String("172.18.40.1")
+
+					err := ipPoolWebhook.ValidateUpdate(ctx, ipPoolT, newIPPoolT)
+					Expect(err).NotTo(HaveOccurred())
+				})
 			})
 
 			When("Validating 'spec.routes'", func() {
diff --git a/pkg/subnetmanager/subnet_webhook_test.go b/pkg/subnetmanager/subnet_webhook_test.go
index a614c98e7..d5be93845 100644
--- a/pkg/subnetmanager/subnet_webhook_test.go
+++ b/pkg/subnetmanager/subnet_webhook_test.go
@@ -605,6 +605,37 @@ var _ = Describe("SubnetWebhook", Label("subnet_webhook_test"), func() {
 					err := subnetWebhook.ValidateCreate(ctx, subnetT)
 					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
+
+				It("duplicate with 'spec.ips'", func() {
+					subnetT.Spec.IPVersion = pointer.Int64(constant.IPv4)
+					subnetT.Spec.Subnet = "172.18.40.0/24"
+					subnetT.Spec.IPs = append(subnetT.Spec.IPs,
+						[]string{
+							"172.18.40.1-172.18.40.2",
+							"172.18.40.10",
+						}...,
+					)
+					subnetT.Spec.Gateway = pointer.String("172.18.40.1")
+
+					err := subnetWebhook.ValidateCreate(ctx, subnetT)
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
+				})
+
+				It("excludes gateway address through 'spec.excludeIPs'", func() {
+					subnetT.Spec.IPVersion = pointer.Int64(constant.IPv4)
+					subnetT.Spec.Subnet = "172.18.40.0/24"
+					subnetT.Spec.IPs = append(subnetT.Spec.IPs,
+						[]string{
+							"172.18.40.1-172.18.40.2",
+							"172.18.40.10",
+						}...,
+					)
+					subnetT.Spec.ExcludeIPs = append(subnetT.Spec.ExcludeIPs, "172.18.40.1")
+					subnetT.Spec.Gateway = pointer.String("172.18.40.1")
+
+					err := subnetWebhook.ValidateCreate(ctx, subnetT)
+					Expect(err).NotTo(HaveOccurred())
+				})
 			})
 
 			When("Validating 'spec.routes'", func() {
@@ -915,11 +946,10 @@ var _ = Describe("SubnetWebhook", Label("subnet_webhook_test"), func() {
 					subnetT.Spec.Subnet = "172.18.40.0/24"
 					subnetT.Spec.IPs = append(subnetT.Spec.IPs,
 						[]string{
-							"172.18.40.2-172.18.40.3",
+							"172.18.40.1-172.18.40.2",
 							"172.18.40.10",
 						}...,
 					)
-					subnetT.Spec.Gateway = pointer.String("172.18.40.1")
 
 					newSubnetT := subnetT.DeepCopy()
 					newSubnetT.Spec.Gateway = pointer.String(constant.InvalidIP)
@@ -933,11 +963,10 @@ var _ = Describe("SubnetWebhook", Label("subnet_webhook_test"), func() {
 					subnetT.Spec.Subnet = "172.18.40.0/24"
 					subnetT.Spec.IPs = append(subnetT.Spec.IPs,
 						[]string{
-							"172.18.40.2-172.18.40.3",
+							"172.18.40.1-172.18.40.2",
 							"172.18.40.10",
 						}...,
 					)
-					subnetT.Spec.Gateway = pointer.String("172.18.40.1")
 
 					newSubnetT := subnetT.DeepCopy()
 					newSubnetT.Spec.Gateway = pointer.String("172.18.41.1")
@@ -945,6 +974,41 @@ var _ = Describe("SubnetWebhook", Label("subnet_webhook_test"), func() {
 					err := subnetWebhook.ValidateUpdate(ctx, subnetT, newSubnetT)
 					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
+
+				It("duplicate with 'spec.ips'", func() {
+					subnetT.Spec.IPVersion = pointer.Int64(constant.IPv4)
+					subnetT.Spec.Subnet = "172.18.40.0/24"
+					subnetT.Spec.IPs = append(subnetT.Spec.IPs,
+						[]string{
+							"172.18.40.1-172.18.40.2",
+							"172.18.40.10",
+						}...,
+					)
+
+					newSubnetT := subnetT.DeepCopy()
+					newSubnetT.Spec.Gateway = pointer.String("172.18.40.1")
+
+					err := subnetWebhook.ValidateUpdate(ctx, subnetT, newSubnetT)
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
+				})
+
+				It("excludes gateway address through 'spec.excludeIPs'", func() {
+					subnetT.Spec.IPVersion = pointer.Int64(constant.IPv4)
+					subnetT.Spec.Subnet = "172.18.40.0/24"
+					subnetT.Spec.IPs = append(subnetT.Spec.IPs,
+						[]string{
+							"172.18.40.1-172.18.40.2",
+							"172.18.40.10",
+						}...,
+					)
+					subnetT.Spec.ExcludeIPs = append(subnetT.Spec.ExcludeIPs, "172.18.40.1")
+
+					newSubnetT := subnetT.DeepCopy()
+					newSubnetT.Spec.Gateway = pointer.String("172.18.40.1")
+
+					err := subnetWebhook.ValidateUpdate(ctx, subnetT, newSubnetT)
+					Expect(err).NotTo(HaveOccurred())
+				})
 			})
 
 			When("Validating 'spec.routes'", func() {