Fix several subnet/subnetset issues

1. Read the real StaticIPAllocation from CR during building NSX subnet. 2. Set StaticIPAllocation to true in default subnetset. 3. Correct the finalizer names of subnet/subnetset.
vmware-tanzu · Aug 14, 2023 · 1d2e592 · 1d2e592
1 parent 9864be7
commit 1d2e592
Show file tree

Hide file tree

Showing 7 changed files with 30 additions and 19 deletions.
diff --git a/pkg/controllers/securitypolicy/securitypolicy_controller.go b/pkg/controllers/securitypolicy/securitypolicy_controller.go
@@ -100,8 +100,8 @@ func (r *SecurityPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 
 	if obj.ObjectMeta.DeletionTimestamp.IsZero() {
 		metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerUpdateTotal, MetricResType)
-		if !controllerutil.ContainsFinalizer(obj, servicecommon.FinalizerName) {
-			controllerutil.AddFinalizer(obj, servicecommon.FinalizerName)
+		if !controllerutil.ContainsFinalizer(obj, servicecommon.SecurityPolicyFinalizerName) {
+			controllerutil.AddFinalizer(obj, servicecommon.SecurityPolicyFinalizerName)
 			if err := r.Client.Update(ctx, obj); err != nil {
 				log.Error(err, "add finalizer", "securitypolicy", req.NamespacedName)
 				updateFail(r, &ctx, obj, &err)
@@ -134,14 +134,14 @@ func (r *SecurityPolicyReconciler) Reconcile(ctx context.Context, req ctrl.Reque
 		}
 		updateSuccess(r, &ctx, obj)
 	} else {
-		if controllerutil.ContainsFinalizer(obj, servicecommon.FinalizerName) {
+		if controllerutil.ContainsFinalizer(obj, servicecommon.SecurityPolicyFinalizerName) {
 			metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResType)
 			if err := r.Service.DeleteSecurityPolicy(obj.UID); err != nil {
 				log.Error(err, "deletion failed, would retry exponentially", "securitypolicy", req.NamespacedName)
 				deleteFail(r, &ctx, obj, &err)
 				return ResultRequeue, err
 			}
-			controllerutil.RemoveFinalizer(obj, servicecommon.FinalizerName)
+			controllerutil.RemoveFinalizer(obj, servicecommon.SecurityPolicyFinalizerName)
 			if err := r.Client.Update(ctx, obj); err != nil {
 				log.Error(err, "deletion failed, would retry exponentially", "securitypolicy", req.NamespacedName)
 				deleteFail(r, &ctx, obj, &err)

diff --git a/pkg/controllers/securitypolicy/securitypolicy_controller_test.go b/pkg/controllers/securitypolicy/securitypolicy_controller_test.go
@@ -175,7 +175,7 @@ func TestSecurityPolicyReconciler_Reconcile(t *testing.T) {
 	_, ret = r.Reconcile(ctx, req)
 	assert.Equal(t, err, ret)
 
-	//  DeletionTimestamp.IsZero = false, Finalizers doesn't include util.FinalizerName
+	//  DeletionTimestamp.IsZero = false, Finalizers doesn't include util.SecurityPolicyFinalizerName
 	k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error {
 		v1sp := obj.(*v1alpha1.SecurityPolicy)
 		time := metav1.Now()
@@ -191,12 +191,12 @@ func TestSecurityPolicyReconciler_Reconcile(t *testing.T) {
 	assert.Equal(t, ret, nil)
 	patch.Reset()
 
-	//  DeletionTimestamp.IsZero = false, Finalizers include util.FinalizerName
+	//  DeletionTimestamp.IsZero = false, Finalizers include util.SecurityPolicyFinalizerName
 	k8sClient.EXPECT().Get(ctx, gomock.Any(), sp).Return(nil).Do(func(_ context.Context, _ client.ObjectKey, obj client.Object, option ...client.GetOption) error {
 		v1sp := obj.(*v1alpha1.SecurityPolicy)
 		time := metav1.Now()
 		v1sp.ObjectMeta.DeletionTimestamp = &time
-		v1sp.Finalizers = []string{common.FinalizerName}
+		v1sp.Finalizers = []string{common.SecurityPolicyFinalizerName}
 		return nil
 	})
 	patch = gomonkey.ApplyMethod(reflect.TypeOf(service), "DeleteSecurityPolicy", func(_ *securitypolicy.SecurityPolicyService, UID interface{}) error {

diff --git a/pkg/controllers/subnet/subnet_controller.go b/pkg/controllers/subnet/subnet_controller.go
@@ -50,8 +50,8 @@ func (r *SubnetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 
 	if obj.ObjectMeta.DeletionTimestamp.IsZero() {
 		metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerUpdateTotal, MetricResTypeSubnet)
-		if !controllerutil.ContainsFinalizer(obj, servicecommon.FinalizerName) {
-			controllerutil.AddFinalizer(obj, servicecommon.FinalizerName)
+		if !controllerutil.ContainsFinalizer(obj, servicecommon.SubnetFinalizerName) {
+			controllerutil.AddFinalizer(obj, servicecommon.SubnetFinalizerName)
 			if err := r.Client.Update(ctx, obj); err != nil {
 				log.Error(err, "add finalizer", "subnet", req.NamespacedName)
 				updateFail(r, &ctx, obj)
@@ -71,14 +71,14 @@ func (r *SubnetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 		}
 		updateSuccess(r, &ctx, obj)
 	} else {
-		if controllerutil.ContainsFinalizer(obj, servicecommon.FinalizerName) {
+		if controllerutil.ContainsFinalizer(obj, servicecommon.SubnetFinalizerName) {
 			metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResTypeSubnet)
 			if err := r.DeleteSubnet(*obj); err != nil {
 				log.Error(err, "deletion failed, would retry exponentially", "subnet", req.NamespacedName)
 				deleteFail(r, &ctx, obj)
 				return ResultRequeue, err
 			}
-			controllerutil.RemoveFinalizer(obj, servicecommon.FinalizerName)
+			controllerutil.RemoveFinalizer(obj, servicecommon.SubnetFinalizerName)
 			if err := r.Client.Update(ctx, obj); err != nil {
 				log.Error(err, "deletion failed, would retry exponentially", "subnet", req.NamespacedName)
 				deleteFail(r, &ctx, obj)

diff --git a/pkg/controllers/subnetset/subnetset_controller.go b/pkg/controllers/subnetset/subnetset_controller.go
@@ -53,8 +53,8 @@ func (r *SubnetSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 
 	if obj.ObjectMeta.DeletionTimestamp.IsZero() {
 		metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerUpdateTotal, MetricResTypeSubnetSet)
-		if !controllerutil.ContainsFinalizer(obj, servicecommon.FinalizerName) {
-			controllerutil.AddFinalizer(obj, servicecommon.FinalizerName)
+		if !controllerutil.ContainsFinalizer(obj, servicecommon.SubnetSetFinalizerName) {
+			controllerutil.AddFinalizer(obj, servicecommon.SubnetSetFinalizerName)
 			if err := r.Client.Update(ctx, obj); err != nil {
 				log.Error(err, "add finalizer", "subnetset", req.NamespacedName)
 				updateFail(r, &ctx, obj)
@@ -64,14 +64,14 @@ func (r *SubnetSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		}
 		updateSuccess(r, &ctx, obj)
 	} else {
-		if controllerutil.ContainsFinalizer(obj, servicecommon.FinalizerName) {
+		if controllerutil.ContainsFinalizer(obj, servicecommon.SubnetSetFinalizerName) {
 			metrics.CounterInc(r.Service.NSXConfig, metrics.ControllerDeleteTotal, MetricResTypeSubnetSet)
 			if err := r.DeleteSubnetForSubnetSet(*obj, false); err != nil {
 				log.Error(err, "deletion failed, would retry exponentially", "subnetset", req.NamespacedName)
 				deleteFail(r, &ctx, obj)
 				return ResultRequeue, err
 			}
-			controllerutil.RemoveFinalizer(obj, servicecommon.FinalizerName)
+			controllerutil.RemoveFinalizer(obj, servicecommon.SubnetSetFinalizerName)
 			if err := r.Client.Update(ctx, obj); err != nil {
 				log.Error(err, "deletion failed, would retry exponentially", "subnetset", req.NamespacedName)
 				deleteFail(r, &ctx, obj)

diff --git a/pkg/controllers/subnetset/vpc_handler.go b/pkg/controllers/subnetset/vpc_handler.go
@@ -2,6 +2,7 @@ package subnetset
 
 import (
 	"context"
+
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
 
 	"sigs.k8s.io/controller-runtime/pkg/predicate"
@@ -56,7 +57,13 @@ func (h *VPCHandler) Create(e event.CreateEvent, _ workqueue.RateLimitingInterfa
 						common.LabelDefaultSubnetSet: subnetSetType,
 					},
 				},
-				Spec: v1alpha1.SubnetSetSpec{},
+				Spec: v1alpha1.SubnetSetSpec{
+					AdvancedConfig: v1alpha1.AdvancedConfig{
+						StaticIPAllocation: v1alpha1.StaticIPAllocation{
+							Enable: true,
+						},
+					},
+				},
 			}
 			if err := h.Client.Create(context.Background(), obj); err != nil {
 				return err

diff --git a/pkg/nsx/services/common/types.go b/pkg/nsx/services/common/types.go
@@ -61,9 +61,11 @@ const (
 	IPPoolTypePublic    = "public"
 	IPPoolTypePrivate   = "private"
 
-	FinalizerName                  = "securitypolicy.nsx.vmware.com/finalizer"
+	SecurityPolicyFinalizerName    = "securitypolicy.nsx.vmware.com/finalizer"
 	StaticRouteFinalizerName       = "staticroute.nsx.vmware.com/finalizer"
 	NSXServiceAccountFinalizerName = "nsxserviceaccount.nsx.vmware.com/finalizer"
+	SubnetFinalizerName            = "subnet.nsx.vmware.com/finalizer"
+	SubnetSetFinalizerName         = "subnetset.nsx.vmware.com/finalizer"
 	SubnetPortFinalizerName        = "subnetport.nsx.vmware.com/finalizer"
 	VPCFinalizerName               = "vpc.nsx.vmware.com/finalizer"
 

diff --git a/pkg/nsx/services/subnet/builder.go b/pkg/nsx/services/subnet/builder.go
@@ -24,7 +24,7 @@ func getCluster(service *SubnetService) string {
 func (service *SubnetService) buildSubnet(obj client.Object, tags []model.Tag) (*model.VpcSubnet, error) {
 	tags = append(tags, service.buildBasicTags(obj)...)
 	var nsxSubnet *model.VpcSubnet
-	boolTrue := true
+	var staticIpAllocation bool
 	switch o := obj.(type) {
 	case *v1alpha1.Subnet:
 		nsxSubnet = &model.VpcSubnet{
@@ -33,6 +33,7 @@ func (service *SubnetService) buildSubnet(obj client.Object, tags []model.Tag) (
 			DhcpConfig:  service.buildDHCPConfig(int64(o.Spec.IPv4SubnetSize - 4)),
 			DisplayName: String(fmt.Sprintf("%s-%s", obj.GetNamespace(), obj.GetName())),
 		}
+		staticIpAllocation = o.Spec.AdvancedConfig.StaticIPAllocation.Enable
 	case *v1alpha1.SubnetSet:
 		index := uuid.NewString()
 		nsxSubnet = &model.VpcSubnet{
@@ -41,13 +42,14 @@ func (service *SubnetService) buildSubnet(obj client.Object, tags []model.Tag) (
 			DhcpConfig:  service.buildDHCPConfig(int64(o.Spec.IPv4SubnetSize - 4)),
 			DisplayName: String(fmt.Sprintf("%s-%s-%s", obj.GetNamespace(), obj.GetName(), index)),
 		}
+		staticIpAllocation = o.Spec.AdvancedConfig.StaticIPAllocation.Enable
 	default:
 		return nil, SubnetTypeError
 	}
 	nsxSubnet.Tags = tags
 	nsxSubnet.AdvancedConfig = &model.SubnetAdvancedConfig{
 		StaticIpAllocation: &model.StaticIpAllocation{
-			Enabled: &boolTrue,
+			Enabled: &staticIpAllocation,
 		},
 	}
 	return nsxSubnet, nil