Repository: spidernet-io/spiderpool
Pull request #2036 (2023-07-14 03:27:31)

ippool inherits subnet properties 
The IPPool should inherits subnet gateway, routes, vlan if not specified

Signed-off-by: Icarus9913 [icaruswu66@qq.com](mailto:icaruswu66@qq.com)

What this PR does / why we need it:
fix bug

Which issue(s) this PR fixes:
close https://github.com/spidernet-io/spiderpool/issues/1958

Icarus9913: wait for @ty-dc modifying the e2e cases.
ref: https://github.com/spidernet-io/spiderpool/issues/2035
weizhoublue: how about the ci
weizhoublue: @Icarus9913 
ty-dc: > @Icarus9913 

e2e上有个问题，使用 0.0.0.0作为路由导致这个pr失败
ty-dc: 在＃1872 上修复了
weizhoublue: rebase the main
codecov[bot]: ## [Codecov](https://app.codecov.io/gh/spidernet-io/spiderpool/pull/2036?src=pr&el=h1&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spidernet-io) Report
> Merging [#2036](https://app.codecov.io/gh/spidernet-io/spiderpool/pull/2036?src=pr&el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spidernet-io) (e31da38) into [main](https://app.codecov.io/gh/spidernet-io/spiderpool/commit/b4cd92b1215ccce2c99b335eb0eab569f08d6374?el=desc&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spidernet-io) (b4cd92b) will **increase** coverage by `0.05%`.
> The diff coverage is `96.29%`.

[![Impacted file tree graph](https://app.codecov.io/gh/spidernet-io/spiderpool/pull/2036/graphs/tree.svg?width=650&height=150&src=pr&token=YKXY2E4Q8G&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spidernet-io)](https://app.codecov.io/gh/spidernet-io/spiderpool/pull/2036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spidernet-io)

```diff
@@            Coverage Diff             @@
##             main    #2036      +/-   ##
==========================================
+ Coverage   81.11%   81.17%   +0.05%     
==========================================
  Files          49       49              
  Lines        5194     5210      +16     
==========================================
+ Hits         4213     4229      +16     
  Misses        823      823              
  Partials      158      158              
```

| Flag | Coverage Δ | |
|---|---|---|
| unittests | `81.17% <96.29%> (+0.05%)` | :arrow_up: |

Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spidernet-io#carryforward-flags-in-the-pull-request-comment) to find out more.

| [Impacted Files](https://app.codecov.io/gh/spidernet-io/spiderpool/pull/2036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spidernet-io) | Coverage Δ | |
|---|---|---|
| [pkg/ippoolmanager/ippool\_mutate.go](https://app.codecov.io/gh/spidernet-io/spiderpool/pull/2036?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=spidernet-io#diff-cGtnL2lwcG9vbG1hbmFnZXIvaXBwb29sX211dGF0ZS5nbw==) | `97.14% <96.29%> (+0.51%)` | :arrow_up: |

ty-dc: @Icarus9913 rebase main

Patch:
diff --git a/pkg/ippoolmanager/ippool_mutate.go b/pkg/ippoolmanager/ippool_mutate.go
index 1c7c944f0..c8d9f5bb5 100644
--- a/pkg/ippoolmanager/ippool_mutate.go
+++ b/pkg/ippoolmanager/ippool_mutate.go
@@ -9,6 +9,7 @@ import (
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/pointer"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
@@ -63,9 +64,15 @@ func (iw *IPPoolWebhook) mutateIPPool(ctx context.Context, ipPool *spiderpoolv2b
 	}
 
 	if iw.EnableSpiderSubnet {
-		if err := iw.setControllerSubnet(ctx, ipPool); err != nil {
+		subnet, err := iw.setControllerSubnet(ctx, ipPool)
+		if err != nil {
 			return apierrors.NewInternalError(fmt.Errorf("failed to set the reference of the controller Subnet: %v", err))
 		}
+
+		// inherit gateway,vlan,routes from corresponding SpiderSubnet if not set
+		if subnet != nil {
+			inheritSubnetProperties(subnet, ipPool)
+		}
 	}
 
 	if len(ipPool.Spec.IPs) > 1 {
@@ -93,18 +100,18 @@ func (iw *IPPoolWebhook) mutateIPPool(ctx context.Context, ipPool *spiderpoolv2b
 	return nil
 }
 
-func (iw *IPPoolWebhook) setControllerSubnet(ctx context.Context, ipPool *spiderpoolv2beta1.SpiderIPPool) error {
+func (iw *IPPoolWebhook) setControllerSubnet(ctx context.Context, ipPool *spiderpoolv2beta1.SpiderIPPool) (*spiderpoolv2beta1.SpiderSubnet, error) {
 	logger := logutils.FromContext(ctx)
 
 	// TODO(iiiceoo): There was an occasional bug.
 	owner := metav1.GetControllerOf(ipPool)
 	if v, ok := ipPool.Labels[constant.LabelIPPoolOwnerSpiderSubnet]; ok && owner != nil && v == owner.Name {
-		return nil
+		return nil, nil
 	}
 
 	cidr, err := spiderpoolip.CIDRToLabelValue(*ipPool.Spec.IPVersion, ipPool.Spec.Subnet)
 	if err != nil {
-		return fmt.Errorf("failed to parse CIDR %s as a valid label value: %v", ipPool.Spec.Subnet, err)
+		return nil, fmt.Errorf("failed to parse CIDR %s as a valid label value: %v", ipPool.Spec.Subnet, err)
 	}
 
 	var subnetList spiderpoolv2beta1.SpiderSubnetList
@@ -113,17 +120,17 @@ func (iw *IPPoolWebhook) setControllerSubnet(ctx context.Context, ipPool *spider
 		&subnetList,
 		client.MatchingLabels{constant.LabelSubnetCIDR: cidr},
 	); err != nil {
-		return fmt.Errorf("failed to list Subnets: %v", err)
+		return nil, fmt.Errorf("failed to list Subnets: %v", err)
 	}
 
 	if len(subnetList.Items) == 0 {
-		return nil
+		return nil, nil
 	}
 
-	subnet := subnetList.Items[0]
-	if !metav1.IsControlledBy(ipPool, &subnet) {
-		if err := ctrl.SetControllerReference(&subnet, ipPool, iw.Client.Scheme()); err != nil {
-			return fmt.Errorf("failed to set owner reference: %v", err)
+	subnet := subnetList.Items[0].DeepCopy()
+	if !metav1.IsControlledBy(ipPool, subnet) {
+		if err := ctrl.SetControllerReference(subnet, ipPool, iw.Client.Scheme()); err != nil {
+			return nil, fmt.Errorf("failed to set owner reference: %v", err)
 		}
 		logger.Sugar().Infof("Set owner reference as Subnet %s", subnet.Name)
 	}
@@ -133,5 +140,22 @@ func (iw *IPPoolWebhook) setControllerSubnet(ctx context.Context, ipPool *spider
 		logger.Sugar().Infof("Set label %s: %s", constant.LabelIPPoolOwnerSpiderSubnet, subnet.Name)
 	}
 
-	return nil
+	return subnet, nil
+}
+
+func inheritSubnetProperties(subnet *spiderpoolv2beta1.SpiderSubnet, ipPool *spiderpoolv2beta1.SpiderIPPool) {
+	if subnet.Spec.Gateway != nil && ipPool.Spec.Gateway == nil {
+		ipPool.Spec.Gateway = pointer.String(*subnet.Spec.Gateway)
+	}
+
+	if subnet.Spec.Vlan != nil && ipPool.Spec.Vlan == nil {
+		ipPool.Spec.Vlan = pointer.Int64(*subnet.Spec.Vlan)
+	}
+
+	// if customer set empty route for this IPPool, it would not inherit the SpiderSubnet.Spec.Routes
+	if len(subnet.Spec.Routes) != 0 && ipPool.Spec.Routes == nil {
+		routes := make([]spiderpoolv2beta1.Route, len(subnet.Spec.Routes))
+		copy(routes, subnet.Spec.Routes)
+		ipPool.Spec.Routes = routes
+	}
 }


Test patch:
diff --git a/pkg/ippoolmanager/ippool_webhook_test.go b/pkg/ippoolmanager/ippool_webhook_test.go
index 58d0f1d3d..356083f7f 100644
--- a/pkg/ippoolmanager/ippool_webhook_test.go
+++ b/pkg/ippoolmanager/ippool_webhook_test.go
@@ -521,6 +521,53 @@ var _ = Describe("IPPoolWebhook", Label("ippool_webhook_test"), func() {
 					},
 				))
 			})
+
+			It("inherit subnet properties from SpiderSubnet", func() {
+				ipVersion := constant.IPv4
+				subnet := "172.18.50.0/24"
+				cidr, err := spiderpoolip.CIDRToLabelValue(ipVersion, subnet)
+				Expect(err).NotTo(HaveOccurred())
+				Expect(cidr).NotTo(BeEmpty())
+
+				ipPoolWebhook.EnableSpiderSubnet = true
+				subnetT.SetUID(uuid.NewUUID())
+				subnetT.Labels[constant.LabelSubnetCIDR] = cidr
+				subnetT.Spec.IPVersion = pointer.Int64(ipVersion)
+				subnetT.Spec.Subnet = subnet
+				subnetT.Spec.IPs = append(subnetT.Spec.IPs,
+					[]string{
+						"172.18.50.1-172.18.50.2",
+						"172.18.50.10",
+					}...,
+				)
+
+				subnetT.Spec.Gateway = pointer.String("172.18.50.0")
+				subnetT.Spec.Vlan = pointer.Int64(50)
+				subnetT.Spec.Routes = []spiderpoolv2beta1.Route{
+					{
+						Dst: "0.0.0.0/0",
+						Gw:  "172.18.50.0",
+					},
+				}
+
+				err = fakeClient.Create(ctx, subnetT)
+				Expect(err).NotTo(HaveOccurred())
+
+				ipPoolT.Spec.Subnet = subnet
+				err = ipPoolWebhook.Default(ctx, ipPoolT)
+				Expect(err).NotTo(HaveOccurred())
+
+				controlled := metav1.IsControlledBy(ipPoolT, subnetT)
+				Expect(controlled).To(BeTrue())
+
+				v, ok := ipPoolT.Labels[constant.LabelIPPoolOwnerSpiderSubnet]
+				Expect(ok).To(BeTrue())
+				Expect(v).To(Equal(subnetName))
+
+				Expect(ipPoolT.Spec.Gateway).To(Equal(subnetT.Spec.Gateway))
+				Expect(ipPoolT.Spec.Vlan).To(Equal(subnetT.Spec.Vlan))
+				Expect(ipPoolT.Spec.Routes).To(Equal(subnetT.Spec.Routes))
+			})
 		})
 
 		Describe("ValidateCreate", func() {
diff --git a/test/e2e/annotation/annotation_test.go b/test/e2e/annotation/annotation_test.go
index 4b90f2f3f..23fc97601 100644
--- a/test/e2e/annotation/annotation_test.go
+++ b/test/e2e/annotation/annotation_test.go
@@ -548,6 +548,7 @@ var _ = Describe("test annotation", Label("annotation"), func() {
 				GinkgoWriter.Println("Pod IP is successfully released")
 			})
 	})
+
 	It("succeeded to running pod after added valid route field", Label("A00002"), func() {
 		var v4PoolName, v6PoolName, ipv4Dst, ipv6Dst, ipv4Gw, ipv6Gw string
 		var v4Pool, v6Pool *spiderpool.SpiderIPPool