Repository: spidernet-io/spiderpool
Pull request #1200 (2022-12-20 10:40:08)

No longer validate when deleting SpiderIPPool, SpiderSubnet and SpiderReservedIP
Signed-off-by: iiiceoo <ziqian.xue@daocloud.io>


Patch:
diff --git a/pkg/ippoolmanager/ippool_subnet_validate.go b/pkg/ippoolmanager/ippool_subnet_validate.go
index 25ea9fb93..f8993230d 100644
--- a/pkg/ippoolmanager/ippool_subnet_validate.go
+++ b/pkg/ippoolmanager/ippool_subnet_validate.go
@@ -8,9 +8,7 @@ import (
 	"fmt"
 
 	"k8s.io/apimachinery/pkg/util/validation/field"
-	"sigs.k8s.io/controller-runtime/pkg/controller/controllerutil"
 
-	"github.com/spidernet-io/spiderpool/pkg/constant"
 	spiderpoolip "github.com/spidernet-io/spiderpool/pkg/ip"
 	spiderpoolv1 "github.com/spidernet-io/spiderpool/pkg/k8s/apis/spiderpool.spidernet.io/v1"
 )
@@ -23,7 +21,7 @@ func (im *ipPoolManager) validateCreateIPPoolAndUpdateSubnetFreeIPs(ctx context.
 		return nil
 	}
 
-	subnet, err := im.validateSubnetControllerExist(ctx, ipPool, false)
+	subnet, err := im.validateSubnetControllerExist(ctx, ipPool)
 	if err != nil {
 		return field.ErrorList{err}
 	}
@@ -42,11 +40,7 @@ func (im *ipPoolManager) validateUpdateIPPoolAndUpdateSubnetFreeIPs(ctx context.
 		return nil
 	}
 
-	if newIPPool.DeletionTimestamp != nil && !controllerutil.ContainsFinalizer(newIPPool, constant.SpiderFinalizer) {
-		return im.validateDeleteIPPoolAndUpdateSubnetFreeIPs(ctx, newIPPool)
-	}
-
-	subnet, err := im.validateSubnetControllerExist(ctx, newIPPool, false)
+	subnet, err := im.validateSubnetControllerExist(ctx, newIPPool)
 	if err != nil {
 		return field.ErrorList{err}
 	}
@@ -57,19 +51,7 @@ func (im *ipPoolManager) validateUpdateIPPoolAndUpdateSubnetFreeIPs(ctx context.
 	return nil
 }
 
-func (im *ipPoolManager) validateDeleteIPPoolAndUpdateSubnetFreeIPs(ctx context.Context, ipPool *spiderpoolv1.SpiderIPPool) field.ErrorList {
-	subnet, err := im.validateSubnetControllerExist(ctx, ipPool, true)
-	if err != nil {
-		return field.ErrorList{err}
-	}
-	if subnet == nil {
-		return nil
-	}
-
-	return nil
-}
-
-func (im *ipPoolManager) validateSubnetControllerExist(ctx context.Context, ipPool *spiderpoolv1.SpiderIPPool, terminaing bool) (*spiderpoolv1.SpiderSubnet, *field.Error) {
+func (im *ipPoolManager) validateSubnetControllerExist(ctx context.Context, ipPool *spiderpoolv1.SpiderIPPool) (*spiderpoolv1.SpiderSubnet, *field.Error) {
 	subnetList, err := im.subnetManager.ListSubnets(ctx)
 	if err != nil {
 		return nil, field.InternalError(subnetField, err)
@@ -77,7 +59,7 @@ func (im *ipPoolManager) validateSubnetControllerExist(ctx context.Context, ipPo
 
 	for _, subnet := range subnetList.Items {
 		if subnet.Spec.Subnet == ipPool.Spec.Subnet {
-			if !terminaing && subnet.DeletionTimestamp != nil {
+			if subnet.DeletionTimestamp != nil {
 				return nil, field.Forbidden(
 					subnetField,
 					fmt.Sprintf("cannot update IPPool that controlled by terminating Subnet %s", subnet.Name),
@@ -87,14 +69,10 @@ func (im *ipPoolManager) validateSubnetControllerExist(ctx context.Context, ipPo
 		}
 	}
 
-	if !terminaing {
-		return nil, field.Forbidden(
-			subnetField,
-			fmt.Sprintf("orphan IPPool, must be controlled by Subnet with the same 'spec.subnet' %s", ipPool.Spec.Subnet),
-		)
-	}
-
-	return nil, nil
+	return nil, field.Forbidden(
+		subnetField,
+		fmt.Sprintf("orphan IPPool, must be controlled by Subnet with the same 'spec.subnet' %s", ipPool.Spec.Subnet),
+	)
 }
 
 func validateSubnetTotalIPsContainsIPPoolTotalIPs(subnet *spiderpoolv1.SpiderSubnet, ipPool *spiderpoolv1.SpiderIPPool) *field.Error {
diff --git a/pkg/ippoolmanager/ippool_webhook.go b/pkg/ippoolmanager/ippool_webhook.go
index 004e981f7..0bfc1a8c9 100644
--- a/pkg/ippoolmanager/ippool_webhook.go
+++ b/pkg/ippoolmanager/ippool_webhook.go
@@ -187,7 +187,11 @@ func (im *ipPoolManager) ValidateUpdate(ctx context.Context, oldObj, newObj runt
 	logger.Sugar().Debugf("Request old IPPool: %v", oldIPPool)
 	logger.Sugar().Debugf("Request new IPPool: %v", newIPPool)
 
-	if newIPPool.DeletionTimestamp != nil && controllerutil.ContainsFinalizer(newIPPool, constant.SpiderFinalizer) {
+	if newIPPool.DeletionTimestamp != nil {
+		if !controllerutil.ContainsFinalizer(newIPPool, constant.SpiderFinalizer) {
+			return nil
+		}
+
 		return apierrors.NewForbidden(
 			schema.GroupResource{},
 			"",
diff --git a/pkg/reservedipmanager/reservedip_webhook.go b/pkg/reservedipmanager/reservedip_webhook.go
index fd77978e3..7532e496a 100644
--- a/pkg/reservedipmanager/reservedip_webhook.go
+++ b/pkg/reservedipmanager/reservedip_webhook.go
@@ -5,6 +5,7 @@ package reservedipmanager
 
 import (
 	"context"
+	"errors"
 
 	"go.uber.org/zap"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
@@ -96,6 +97,18 @@ func (rw *ReservedIPWebhook) ValidateUpdate(ctx context.Context, oldObj, newObj
 	logger.Sugar().Debugf("Request old ReservedIP: %+v", *oldRIP)
 	logger.Sugar().Debugf("Request new ReservedIP: %+v", *newRIP)
 
+	if newRIP.DeletionTimestamp != nil {
+		if oldRIP.DeletionTimestamp == nil {
+			return nil
+		}
+
+		return apierrors.NewForbidden(
+			schema.GroupResource{},
+			"",
+			errors.New("cannot update a terminating ReservedIP"),
+		)
+	}
+
 	if errs := rw.validateUpdateReservedIP(logutils.IntoContext(ctx, logger), oldRIP, newRIP); len(errs) != 0 {
 		logger.Sugar().Errorf("Failed to update ReservedIP: %v", errs.ToAggregate().Error())
 		return apierrors.NewInvalid(
diff --git a/pkg/reservedipmanager/utils.go b/pkg/reservedipmanager/utils.go
index 00243d09b..1dfaaeeba 100644
--- a/pkg/reservedipmanager/utils.go
+++ b/pkg/reservedipmanager/utils.go
@@ -24,15 +24,13 @@ func AssembleReservedIPs(version types.IPVersion, rIPList *spiderpoolv1.SpiderRe
 
 	var ips []net.IP
 	for _, r := range rIPList.Items {
-		if *r.Spec.IPVersion != version {
-			continue
+		if r.DeletionTimestamp == nil && *r.Spec.IPVersion == version {
+			rIPs, err := spiderpoolip.ParseIPRanges(version, r.Spec.IPs)
+			if err != nil {
+				return nil, err
+			}
+			ips = append(ips, rIPs...)
 		}
-
-		rIPs, err := spiderpoolip.ParseIPRanges(version, r.Spec.IPs)
-		if err != nil {
-			return nil, err
-		}
-		ips = append(ips, rIPs...)
 	}
 
 	return ips, nil
diff --git a/pkg/subnetmanager/subnet_webhook.go b/pkg/subnetmanager/subnet_webhook.go
index e6d9ced20..87df0d6bf 100644
--- a/pkg/subnetmanager/subnet_webhook.go
+++ b/pkg/subnetmanager/subnet_webhook.go
@@ -142,7 +142,11 @@ func (sm *subnetManager) ValidateUpdate(ctx context.Context, oldObj, newObj runt
 	logger.Sugar().Debugf("Request old Subnet: %+v", *oldSubnet)
 	logger.Sugar().Debugf("Request new Subnet: %+v", *newSubnet)
 
-	if newSubnet.DeletionTimestamp != nil && controllerutil.ContainsFinalizer(newSubnet, constant.SpiderFinalizer) {
+	if newSubnet.DeletionTimestamp != nil {
+		if !controllerutil.ContainsFinalizer(newSubnet, constant.SpiderFinalizer) {
+			return nil
+		}
+
 		return apierrors.NewForbidden(
 			schema.GroupResource{},
 			"",


Test patch:
diff --git a/pkg/podmanager/utils_test.go b/pkg/podmanager/utils_test.go
index 59d1f03f4..7c9fa17d7 100644
--- a/pkg/podmanager/utils_test.go
+++ b/pkg/podmanager/utils_test.go
@@ -41,8 +41,8 @@ var _ = Describe("PodManager utils", Label("pod_manager_utils_test"), func() {
 
 		It("checks terminated Pod", func() {
 			now := metav1.Now()
-			podT.DeletionTimestamp = &now
-			podT.DeletionGracePeriodSeconds = pointer.Int64(0)
+			podT.SetDeletionTimestamp(&now)
+			podT.SetDeletionGracePeriodSeconds(pointer.Int64(0))
 
 			status, allocatable := podmanager.CheckPodStatus(podT)
 			Expect(status).To(Equal(constant.PodGraceTimeout))
@@ -51,8 +51,8 @@ var _ = Describe("PodManager utils", Label("pod_manager_utils_test"), func() {
 
 		It("checks terminating Pod", func() {
 			now := metav1.Now()
-			podT.DeletionTimestamp = &now
-			podT.DeletionGracePeriodSeconds = pointer.Int64(30)
+			podT.SetDeletionTimestamp(&now)
+			podT.SetDeletionGracePeriodSeconds(pointer.Int64(30))
 
 			status, allocatable := podmanager.CheckPodStatus(podT)
 			Expect(status).To(Equal(constant.PodTerminating))
diff --git a/pkg/reservedipmanager/reservedip_webhook_test.go b/pkg/reservedipmanager/reservedip_webhook_test.go
index b0dd4a4e1..a6c3a3fa1 100644
--- a/pkg/reservedipmanager/reservedip_webhook_test.go
+++ b/pkg/reservedipmanager/reservedip_webhook_test.go
@@ -11,7 +11,9 @@ import (
 	"github.com/golang/mock/gomock"
 	. "github.com/onsi/ginkgo/v2"
 	. "github.com/onsi/gomega"
+	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/config"
 	"sigs.k8s.io/controller-runtime/pkg/manager"
@@ -209,7 +211,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 				It("inputs nil 'spec.ipVersion'", func() {
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateCreate(ctx, rIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("inputs invalid 'spec.ipVersion'", func() {
@@ -218,7 +220,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateCreate(ctx, rIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("creates IPv4 ReservedIP but IPv4 is disbale'", func() {
@@ -228,7 +230,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateCreate(ctx, rIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("creates IPv6 ReservedIP but IPv6 is disbale'", func() {
@@ -238,7 +240,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateCreate(ctx, rIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 			})
 
@@ -259,7 +261,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateCreate(ctx, rIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("failed to list ReservedIPs due to some unknown errors", func() {
@@ -279,7 +281,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateCreate(ctx, rIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("exists invalid ReservedIPs in the cluster", func() {
@@ -305,7 +307,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateCreate(ctx, rIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("overlaps with the existing ReservedIP", func() {
@@ -331,7 +333,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateCreate(ctx, rIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 			})
 
@@ -393,7 +395,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateUpdate(ctx, rIPT, newRIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("updates 'spec.ipVersion' to invalid IP version", func() {
@@ -407,7 +409,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateUpdate(ctx, rIPT, newRIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("changes 'spec.ipVersion'", func() {
@@ -421,7 +423,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateUpdate(ctx, rIPT, newRIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("updates IPv4 ReservedIP but IPv4 is disbale'", func() {
@@ -434,7 +436,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateUpdate(ctx, rIPT, newRIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("updates IPv6 ReservedIP but IPv6 is disbale'", func() {
@@ -447,7 +449,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateUpdate(ctx, rIPT, newRIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 			})
 
@@ -461,7 +463,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateUpdate(ctx, rIPT, newRIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("failed to list ReservedIPs due to some unknown errors", func() {
@@ -479,7 +481,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateUpdate(ctx, rIPT, newRIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("exists invalid ReservedIPs in the cluster", func() {
@@ -503,7 +505,7 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateUpdate(ctx, rIPT, newRIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 
 				It("overlaps with the existing ReservedIP", func() {
@@ -527,10 +529,32 @@ var _ = Describe("ReservedIPWebhook", Label("reservedip_webhook_test"), func() {
 
 					ctx := context.TODO()
 					err := rIPWebhook.ValidateUpdate(ctx, rIPT, newRIPT)
-					Expect(err).To(HaveOccurred())
+					Expect(apierrors.IsInvalid(err)).To(BeTrue())
 				})
 			})
 
+			It("deletes ReservedIP", func() {
+				newRIPT := rIPT.DeepCopy()
+				now := metav1.Now()
+				newRIPT.SetDeletionTimestamp(&now)
+				newRIPT.SetDeletionGracePeriodSeconds(pointer.Int64(0))
+
+				ctx := context.TODO()
+				err := rIPWebhook.ValidateUpdate(ctx, rIPT, newRIPT)
+				Expect(err).NotTo(HaveOccurred())
+			})
+
+			It("updates terminating ReservedIP", func() {
+				now := metav1.Now()
+				rIPT.SetDeletionTimestamp(&now)
+				rIPT.SetDeletionGracePeriodSeconds(pointer.Int64(30))
+				newRIPT := rIPT.DeepCopy()
+
+				ctx := context.TODO()
+				err := rIPWebhook.ValidateUpdate(ctx, rIPT, newRIPT)
+				Expect(apierrors.IsForbidden(err)).To(BeTrue())
+			})
+
 			It("updates IPv4 ReservedIP with all fields valid", func() {
 				mockRIPManager.EXPECT().
 					ListReservedIPs(gomock.All()).
diff --git a/pkg/reservedipmanager/utils_test.go b/pkg/reservedipmanager/utils_test.go
index 28809c098..4a1c67275 100644
--- a/pkg/reservedipmanager/utils_test.go
+++ b/pkg/reservedipmanager/utils_test.go
@@ -15,12 +15,14 @@ import (
 	spiderpoolv1 "github.com/spidernet-io/spiderpool/pkg/k8s/apis/spiderpool.spidernet.io/v1"
 	"github.com/spidernet-io/spiderpool/pkg/reservedipmanager"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/utils/pointer"
 )
 
 var _ = Describe("ReservedIPManager utils", Label("reservedip_manager_utils_test"), func() {
 	Describe("Test AssembleReservedIPs", func() {
 		var v4RIPT spiderpoolv1.SpiderReservedIP
 		var v6RIPT spiderpoolv1.SpiderReservedIP
+		var terminatingV4RIPT spiderpoolv1.SpiderReservedIP
 		var rIPListT *spiderpoolv1.SpiderReservedIPList
 
 		BeforeEach(func() {
@@ -61,6 +63,25 @@ var _ = Describe("ReservedIPManager utils", Label("reservedip_manager_utils_test
 				},
 			}
 
+			now := metav1.Now()
+			terminatingV4RIPT = spiderpoolv1.SpiderReservedIP{
+				TypeMeta: metav1.TypeMeta{
+					Kind:       constant.SpiderReservedIPKind,
+					APIVersion: fmt.Sprintf("%s/%s", constant.SpiderpoolAPIGroup, constant.SpiderpoolAPIVersionV1),
+				},
+				ObjectMeta: metav1.ObjectMeta{
+					Name:                       "terminating-ipv4-reservedip",
+					DeletionTimestamp:          &now,
+					DeletionGracePeriodSeconds: pointer.Int64(30),
+				},
+				Spec: spiderpoolv1.ReservedIPSpec{
+					IPVersion: &ipv4,
+					IPs: []string{
+						"172.18.40.40",
+					},
+				},
+			}
+
 			rIPListT = &spiderpoolv1.SpiderReservedIPList{
 				TypeMeta: metav1.TypeMeta{
 					Kind:       constant.SpiderReservedIPListKind,
@@ -92,7 +113,21 @@ var _ = Describe("ReservedIPManager utils", Label("reservedip_manager_utils_test
 			Expect(ips).To(BeEmpty())
 		})
 
-		It("assembles IPv4 reserved IP addresses", func() {
+		It("does not assemble terminating IPv4 reserved-IP addresses", func() {
+			rIPListT.Items = append(rIPListT.Items, v4RIPT, terminatingV4RIPT)
+
+			ips, err := reservedipmanager.AssembleReservedIPs(constant.IPv4, rIPListT)
+			Expect(err).NotTo(HaveOccurred())
+			Expect(ips).To(Equal(
+				[]net.IP{
+					net.IPv4(172, 18, 40, 1),
+					net.IPv4(172, 18, 40, 2),
+					net.IPv4(172, 18, 40, 10),
+				},
+			))
+		})
+
+		It("assembles IPv4 reserved-IP addresses", func() {
 			rIPListT.Items = append(rIPListT.Items, v4RIPT, v6RIPT)
 
 			ips, err := reservedipmanager.AssembleReservedIPs(constant.IPv4, rIPListT)
@@ -106,7 +141,7 @@ var _ = Describe("ReservedIPManager utils", Label("reservedip_manager_utils_test
 			))
 		})
 
-		It("assembles IPv6 reserved IP addresses", func() {
+		It("assembles IPv6 reserved-IP addresses", func() {
 			rIPListT.Items = append(rIPListT.Items, v4RIPT, v6RIPT)
 
 			ips, err := reservedipmanager.AssembleReservedIPs(constant.IPv6, rIPListT)