Add IPv4SubnetSize webhook validation for subnet and subnetSet CR (#949)

This patch is to: 1. Adding IPv4SubnetSize webhook validation for subnet and subnetSet CR is to prevent NSX Operator from creating subnet or subnetSet CR when given IPv4SubnetSize is invalid. 2. Removing IPv4SubnetSize validation check from subnetSet controller. Also, fix a subnet e2e CR name for subnetset CR used for subnetport.
vmware-tanzu · Dec 13, 2024 · 0222a3d · 0222a3d
1 parent dad2d81
commit 0222a3d
Show file tree

Hide file tree

Showing 14 changed files with 60 additions and 106 deletions.
diff --git a/build/yaml/webhook/manifests.yaml b/build/yaml/webhook/manifests.yaml
@@ -62,6 +62,7 @@ webhooks:
     apiVersions:
     - v1alpha1
     operations:
+    - CREATE
     - DELETE
     resources:
     - subnets

diff --git a/go.mod b/go.mod
@@ -40,6 +40,7 @@ require (
 	k8s.io/apimachinery v0.31.2
 	k8s.io/client-go v0.31.2
 	k8s.io/code-generator v0.31.0
+	k8s.io/utils v0.0.0-20240711033017-18e509b52bc8
 	sigs.k8s.io/controller-runtime v0.19.0
 )
 
@@ -99,7 +100,6 @@ require (
 	k8s.io/gengo/v2 v2.0.0-20240228010128-51d4e06bde70 // indirect
 	k8s.io/klog/v2 v2.130.1 // indirect
 	k8s.io/kube-openapi v0.0.0-20240228011516-70dd3763d340 // indirect
-	k8s.io/utils v0.0.0-20240711033017-18e509b52bc8 // indirect
 	sigs.k8s.io/json v0.0.0-20221116044647-bc3834ca7abd // indirect
 	sigs.k8s.io/structured-merge-diff/v4 v4.4.1 // indirect
 	sigs.k8s.io/yaml v1.4.0 // indirect

diff --git a/go.sum b/go.sum
diff --git a/pkg/controllers/subnet/subnet_webhook.go b/pkg/controllers/subnet/subnet_webhook.go
@@ -10,6 +10,7 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/webhook/admission"
 
 	"github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
+	"github.com/vmware-tanzu/nsx-operator/pkg/util"
 )
 
 var NSXOperatorSA = "system:serviceaccount:vmware-system-nsx:ncp-svc-account"
@@ -40,6 +41,10 @@ func (v *SubnetValidator) Handle(ctx context.Context, req admission.Request) adm
 	}
 	log.V(1).Info("Handling request", "user", req.UserInfo.Username, "operation", req.Operation)
 	switch req.Operation {
+	case admissionv1.Create:
+		if subnet.Spec.IPv4SubnetSize != 0 && !util.IsPowerOfTwo(subnet.Spec.IPv4SubnetSize) {
+			return admission.Denied(fmt.Sprintf("Subnet %s/%s has invalid size %d, which must be power of 2", subnet.Namespace, subnet.Name, subnet.Spec.IPv4SubnetSize))
+		}
 	case admissionv1.Delete:
 		if req.UserInfo.Username != NSXOperatorSA {
 			hasSubnetPort, err := v.checkSubnetPort(ctx, subnet.Namespace, subnet.Name)

diff --git a/pkg/controllers/subnet/subnet_webhook_test.go b/pkg/controllers/subnet/subnet_webhook_test.go
@@ -38,6 +38,18 @@ func TestSubnetValidator_Handle(t *testing.T) {
 			Namespace: "ns-1",
 			Name:      "subnet-1",
 		},
+		Spec: v1alpha1.SubnetSpec{
+			IPv4SubnetSize: 16,
+		},
+	})
+	req2, _ := json.Marshal(&v1alpha1.Subnet{
+		ObjectMeta: metav1.ObjectMeta{
+			Namespace: "ns-2",
+			Name:      "subnet-2",
+		},
+		Spec: v1alpha1.SubnetSpec{
+			IPv4SubnetSize: 24,
+		},
 	})
 	type args struct {
 		req admission.Request
@@ -113,6 +125,14 @@ func TestSubnetValidator_Handle(t *testing.T) {
 			}}},
 			want: admission.Errored(http.StatusBadRequest, errors.New("there is no content to decode")),
 		},
+		{
+			name: "CreateSubnet with invalid IPv4SubnetSize",
+			args: args{req: admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{
+				Operation: admissionv1.Create,
+				Object:    runtime.RawExtension{Raw: req2},
+			}}},
+			want: admission.Denied("Subnet ns-2/subnet-2 has invalid size 24, which must be power of 2"),
+		},
 		{
 			name: "CreateSubnet",
 			args: args{req: admission.Request{AdmissionRequest: admissionv1.AdmissionRequest{

diff --git a/pkg/controllers/subnetset/subnetset_controller.go b/pkg/controllers/subnetset/subnetset_controller.go
@@ -30,7 +30,6 @@ import (
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnet"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnetbinding"
 	nsxutil "github.com/vmware-tanzu/nsx-operator/pkg/nsx/util"
-	"github.com/vmware-tanzu/nsx-operator/pkg/util"
 )
 
 var (
@@ -137,11 +136,6 @@ func (r *SubnetSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 		subnetsetCR.Spec.IPv4SubnetSize = vpcNetworkConfig.DefaultSubnetSize
 		specChanged = true
 	}
-	if !util.IsPowerOfTwo(subnetsetCR.Spec.IPv4SubnetSize) {
-		err := fmt.Errorf("ipv4SubnetSize has invalid size %d, which needs to be >= 16 and power of 2", subnetsetCR.Spec.IPv4SubnetSize)
-		r.StatusUpdater.UpdateFail(ctx, subnetsetCR, err, "", setSubnetSetReadyStatusFalse)
-		return ResultNormal, nil
-	}
 
 	if specChanged {
 		err := r.Client.Update(ctx, subnetsetCR)

diff --git a/pkg/controllers/subnetset/subnetset_controller_test.go b/pkg/controllers/subnetset/subnetset_controller_test.go
@@ -53,8 +53,7 @@ func (recorder fakeRecorder) Eventf(object runtime.Object, eventtype, reason, me
 func (recorder fakeRecorder) AnnotatedEventf(object runtime.Object, annotations map[string]string, eventtype, reason, messageFmt string, args ...interface{}) {
 }
 
-type fakeOrgRootClient struct {
-}
+type fakeOrgRootClient struct{}
 
 func (f fakeOrgRootClient) Get(basePathParam *string, filterParam *string, typeFilterParam *string) (model.OrgRoot, error) {
 	return model.OrgRoot{}, nil
@@ -64,8 +63,7 @@ func (f fakeOrgRootClient) Patch(orgRootParam model.OrgRoot, enforceRevisionChec
 	return errors.New("patch error")
 }
 
-type fakeSubnetStatusClient struct {
-}
+type fakeSubnetStatusClient struct{}
 
 func (f fakeSubnetStatusClient) List(orgIdParam string, projectIdParam string, vpcIdParam string, subnetIdParam string) (model.VpcSubnetStatusListResult, error) {
 	dhcpServerAddress := "1.1.1.1"
@@ -163,22 +161,6 @@ func TestReconcile(t *testing.T) {
 				return patches
 			},
 		},
-		{
-			// TODO: should check the SubnetSet status has error message, which contains 'ipv4SubnetSize has invalid size'
-			name:         "Create a SubnetSet with invalid IPv4SubnetSize",
-			expectRes:    ResultNormal,
-			expectErrStr: "",
-			patches: func(r *SubnetSetReconciler) *gomonkey.Patches {
-				vpcnetworkInfo := &common.VPCNetworkConfigInfo{DefaultSubnetSize: 15}
-				patches := gomonkey.ApplyMethod(reflect.TypeOf(r.VPCService), "GetVPCNetworkConfigByNamespace", func(_ *vpc.VPCService, ns string) *common.VPCNetworkConfigInfo {
-					return vpcnetworkInfo
-				})
-				patches.ApplyPrivateMethod(reflect.TypeOf(r), "getSubnetBindingCRsBySubnetSet", func(_ *SubnetSetReconciler, _ context.Context, _ *v1alpha1.SubnetSet) []v1alpha1.SubnetConnectionBindingMap {
-					return []v1alpha1.SubnetConnectionBindingMap{}
-				})
-				return patches
-			},
-		},
 		{
 			name:      "Create a SubnetSet",
 			expectRes: ResultNormal,

diff --git a/pkg/controllers/subnetset/subnetset_webhook.go b/pkg/controllers/subnetset/subnetset_webhook.go
@@ -14,6 +14,7 @@ import (
 
 	"github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
+	"github.com/vmware-tanzu/nsx-operator/pkg/util"
 )
 
 var NSXOperatorSA = "system:serviceaccount:vmware-system-nsx:ncp-svc-account"
@@ -63,6 +64,9 @@ func (v *SubnetSetValidator) Handle(ctx context.Context, req admission.Request)
 	log.V(1).Info("Handling request", "user", req.UserInfo.Username, "operation", req.Operation)
 	switch req.Operation {
 	case admissionv1.Create:
+		if subnetSet.Spec.IPv4SubnetSize != 0 && !util.IsPowerOfTwo(subnetSet.Spec.IPv4SubnetSize) {
+			return admission.Denied(fmt.Sprintf("SubnetSet %s/%s has invalid size %d, which must be power of 2", subnetSet.Namespace, subnetSet.Name, subnetSet.Spec.IPv4SubnetSize))
+		}
 		if isDefaultSubnetSet(subnetSet) && req.UserInfo.Username != NSXOperatorSA {
 			return admission.Denied("default SubnetSet only can be created by nsx-operator")
 		}

diff --git a/pkg/controllers/subnetset/subnetset_webhook_test.go b/pkg/controllers/subnetset/subnetset_webhook_test.go
@@ -43,10 +43,23 @@ func TestSubnetSetValidator(t *testing.T) {
 		},
 	}
 
+	invalidSubnetSet := &v1alpha1.SubnetSet{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "fake-subnetset",
+			Namespace: "ns-1",
+		},
+		Spec: v1alpha1.SubnetSetSpec{
+			IPv4SubnetSize: 24,
+		},
+	}
+
 	subnetSet := &v1alpha1.SubnetSet{
 		ObjectMeta: metav1.ObjectMeta{
 			Name: "fake-subnetset",
 		},
+		Spec: v1alpha1.SubnetSetSpec{
+			IPv4SubnetSize: 32,
+		},
 	}
 
 	subnetSetWithStalePorts := &v1alpha1.SubnetSet{
@@ -94,6 +107,14 @@ func TestSubnetSetValidator(t *testing.T) {
 			isAllowed: false,
 			msg:       "default SubnetSet only can be created by nsx-operator",
 		},
+		{
+			name:      "Create default SubnetSet with invalid IPv4SubnetSize",
+			op:        admissionv1.Create,
+			subnetSet: invalidSubnetSet,
+			user:      NSXOperatorSA,
+			isAllowed: false,
+			msg:       "SubnetSet ns-1/fake-subnetset has invalid size 24, which must be power of 2",
+		},
 		{
 			name:      "Create normal SubnetSet",
 			op:        admissionv1.Create,

diff --git a/test/e2e/manifest/testSubnet/subnetport-in-dhcp-subnetset.yaml b/test/e2e/manifest/testSubnet/subnetport-in-dhcp-subnetset.yaml
@@ -4,4 +4,4 @@ metadata:
   name: port-in-dhcp-subnetset
   namespace: subnet-e2e
 spec:
-  subnetSet: user-pod-subnetset-dhcp
+  subnetSet: user-subnetset-dhcp
diff --git a/test/e2e/manifest/testSubnet/subnetport-in-static-subnetset.yaml b/test/e2e/manifest/testSubnet/subnetport-in-static-subnetset.yaml
@@ -4,4 +4,4 @@ metadata:
   name: port-in-static-subnetset
   namespace: subnet-e2e
 spec:
-  subnetSet: user-pod-subnetset-static
+  subnetSet: user-subnetset-static
diff --git a/test/e2e/manifest/testSubnet/subnetset-dhcp.yaml b/test/e2e/manifest/testSubnet/subnetset-dhcp.yaml
@@ -1,7 +1,7 @@
 apiVersion: crd.nsx.vmware.com/v1alpha1
 kind: SubnetSet
 metadata:
-  name: user-pod-subnetset-dhcp
+  name: user-subnetset-dhcp
   namespace: subnet-e2e
 spec:
   accessMode: PrivateTGW

diff --git a/test/e2e/manifest/testSubnet/subnetset-static.yaml b/test/e2e/manifest/testSubnet/subnetset-static.yaml
@@ -1,7 +1,7 @@
 apiVersion: crd.nsx.vmware.com/v1alpha1
 kind: SubnetSet
 metadata:
-  name: user-pod-subnetset-static
+  name: user-subnetset-static
   namespace: subnet-e2e
 spec:
   accessMode: Private
diff --git a/test/e2e/nsx_subnet_test.go b/test/e2e/nsx_subnet_test.go
@@ -180,8 +180,8 @@ func UserSubnetSet(t *testing.T) {
 		"./manifest/testSubnet/subnetset-dhcp.yaml",
 	}
 	subnetSetNames := []string{
-		"user-pod-subnetset-static",
-		"user-pod-subnetset-dhcp",
+		"user-subnetset-static",
+		"user-subnetset-dhcp",
 	}
 	portYAMLs := []string{
 		"./manifest/testSubnet/subnetport-in-static-subnetset.yaml",