Support immutable fields check for Subnet/Subnetset

This patch is to:
1. For Subnet CR, if there is already a nsx subnet created for the CR,
The following fields are immutable:
IPv4SubnetSize/AccessMode/IPAddresses/DHCPConfig
2. For Subnetset CR, if there is already a nsx subnet created from Subnetset
There are two cases:
1). For VM Subnetset
The following fields are immutable:
IPv4SubnetSize/AccessMode/DHCPConfig
2). For POD Subnetset
The following fields are immutable:
IPv4SubnetSize/DHCPConfig

3. Fix the bug that the nsx subnet IPv4SubnetSize is not set by user-defined CR,
either from subnet CR or subnetset CR.The current value of IPv4SubnetSize
is set by vpcnetworkconfiguration.

pkg/controllers/subnet/subnet_controller.go

@@ -104,6 +104,12 @@ func (r *SubnetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
 				updateFail(r, &ctx, obj, err.Error())
 				return ResultNormal, nil
 			}
+			if errors.As(err, &util.ImmutableFieldModifyError{}) {
+				log.Error(err, "not allowed to modify immutable field, would not retry", "subnet", req.NamespacedName)
+				updateFail(r, &ctx, obj, err.Error())
+				return ResultNormal, nil
+			}
+
 			log.Error(err, "operate failed, would retry exponentially", "subnet", req.NamespacedName)
 			updateFail(r, &ctx, obj, "")
 			return ResultRequeue, err

pkg/controllers/subnetset/subnetset_controller.go

@@ -28,6 +28,7 @@ import (
 	"github.com/vmware-tanzu/nsx-operator/pkg/metrics"
 	servicecommon "github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/subnet"
+	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/util"
 )
 
 var (
@@ -106,6 +107,19 @@ func (r *SubnetSetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (
 			if err := r.SubnetService.UpdateSubnetSetTags(obj.Namespace, nsxSubnets, tags); err != nil {
 				log.Error(err, "failed to update subnetset tags")
 			}
+
+			// If there is already a nsx subnet allocated from the subnetset,
+			// It's needed to check immutable fields to avoid change.
+			isForVM := true
+			if obj.Name == servicecommon.DefaultPodSubnetSet {
+				isForVM = false
+			}
+			err := r.SubnetService.ValidateSubnetSetImmutableFields(obj, nsxSubnets[0], isForVM)
+			if errors.As(err, &util.ImmutableFieldModifyError{}) {
+				log.Error(err, "not allowed to modify immutable field, would not retry", "subnetset", req.NamespacedName)
+				updateFail(r, &ctx, obj, err.Error())
+				return ResultNormal, nil
+			}
 		}
 		updateSuccess(r, &ctx, obj)
 	} else {
@@ -306,7 +320,8 @@ func (r *SubnetSetReconciler) DeleteSubnetForSubnetSet(obj v1alpha1.SubnetSet, u
 
 func StartSubnetSetController(mgr ctrl.Manager, subnetService *subnet.SubnetService,
 	subnetPortService servicecommon.SubnetPortServiceProvider, vpcService servicecommon.VPCServiceProvider,
-	enableWebhook bool) error {
+	enableWebhook bool,
+) error {
 	subnetsetReconciler := &SubnetSetReconciler{
 		Client:            mgr.GetClient(),
 		Scheme:            mgr.GetScheme(),

pkg/nsx/services/common/services.go

@@ -4,6 +4,8 @@ import (
 	"sigs.k8s.io/controller-runtime/pkg/client"
 
 	"github.com/vmware/vsphere-automation-sdk-go/services/nsxt/model"
+
+	"github.com/vmware-tanzu/nsx-operator/pkg/apis/v1alpha1"
 )
 
 // VPCServiceProvider provides to methods other controllers and services.
@@ -27,6 +29,7 @@ type SubnetServiceProvider interface {
 	GetSubnetsByIndex(key, value string) []*model.VpcSubnet
 	CreateOrUpdateSubnet(obj client.Object, vpcInfo VPCResourceInfo, tags []model.Tag) (string, error)
 	GenerateSubnetNSTags(obj client.Object, nsUID string) []model.Tag
+	ValidateSubnetSetImmutableFields(*v1alpha1.SubnetSet, *model.VpcSubnet, bool) error
 }
 
 type SubnetPortServiceProvider interface {

pkg/nsx/services/subnet/builder.go

@@ -50,20 +50,22 @@ func (service *SubnetService) buildSubnet(obj client.Object, tags []model.Tag) (
 	switch o := obj.(type) {
 	case *v1alpha1.Subnet:
 		nsxSubnet = &model.VpcSubnet{
-			Id:          String(service.BuildSubnetID(o)),
-			AccessMode:  String(util.Capitalize(string(o.Spec.AccessMode))),
-			DhcpConfig:  service.buildDHCPConfig(o.Spec.DHCPConfig.EnableDHCP, int64(o.Spec.IPv4SubnetSize-4)),
-			DisplayName: String(service.buildSubnetName(o)),
+			Id:             String(service.BuildSubnetID(o)),
+			AccessMode:     String(util.Capitalize(string(o.Spec.AccessMode))),
+			Ipv4SubnetSize: Int64(int64(o.Spec.IPv4SubnetSize)),
+			DhcpConfig:     service.buildDHCPConfig(o.Spec.DHCPConfig.EnableDHCP, int64(o.Spec.IPv4SubnetSize-4)),
+			DisplayName:    String(service.buildSubnetName(o)),
 		}
 		staticIpAllocation = o.Spec.AdvancedConfig.StaticIPAllocation.Enable
 		nsxSubnet.IpAddresses = o.Spec.IPAddresses
 	case *v1alpha1.SubnetSet:
 		index := uuid.NewString()
 		nsxSubnet = &model.VpcSubnet{
-			Id:          String(service.buildSubnetSetID(o, index)),
-			AccessMode:  String(util.Capitalize(string(o.Spec.AccessMode))),
-			DhcpConfig:  service.buildDHCPConfig(o.Spec.DHCPConfig.EnableDHCP, int64(o.Spec.IPv4SubnetSize-4)),
-			DisplayName: String(service.buildSubnetSetName(o, index)),
+			Id:             String(service.buildSubnetSetID(o, index)),
+			AccessMode:     String(util.Capitalize(string(o.Spec.AccessMode))),
+			Ipv4SubnetSize: Int64(int64(o.Spec.IPv4SubnetSize)),
+			DhcpConfig:     service.buildDHCPConfig(o.Spec.DHCPConfig.EnableDHCP, int64(o.Spec.IPv4SubnetSize-4)),
+			DisplayName:    String(service.buildSubnetSetName(o, index)),
 		}
 		staticIpAllocation = o.Spec.AdvancedConfig.StaticIPAllocation.Enable
 	default:

pkg/nsx/services/subnet/subnet.go

@@ -21,6 +21,7 @@ import (
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/common"
 	"github.com/vmware-tanzu/nsx-operator/pkg/nsx/services/realizestate"
 	nsxutil "github.com/vmware-tanzu/nsx-operator/pkg/nsx/util"
+	"github.com/vmware-tanzu/nsx-operator/pkg/util"
 )
 
 var (
@@ -94,8 +95,16 @@ func (service *SubnetService) CreateOrUpdateSubnet(obj client.Object, vpcInfo co
 		if existingSubnet == nil {
 			changed = true
 		} else {
+			// If there is already a nsx subnet created,
+			// It's needed to check immutable fields to avoid change.
+			err = service.validateSubnetImmutableFields(existingSubnet, nsxSubnet)
+			if err != nil {
+				log.Error(err, "failed to validate Subnet immutable fields")
+				return "", err
+			}
 			changed = common.CompareResource(SubnetToComparable(existingSubnet), SubnetToComparable(nsxSubnet))
 		}
+
 		if !changed {
 			log.Info("subnet not changed, skip updating", "subnet.Id", uid)
 			return uid, nil
@@ -418,3 +427,54 @@ func (service *SubnetService) UpdateSubnetSetTags(ns string, vpcSubnets []*model
 	}
 	return nil
 }
+
+func (service *SubnetService) validateSubnetImmutableFields(existingNsxSubnet *model.VpcSubnet, nsxSubnet *model.VpcSubnet) error {
+	errorMsg := ""
+
+	if *existingNsxSubnet.AccessMode != *nsxSubnet.AccessMode {
+		errorMsg = fmt.Sprintf("Subnet CR AccessMode cannot be modified")
+		return nsxutil.ImmutableFieldModifyError{Desc: errorMsg}
+	}
+
+	if *existingNsxSubnet.DhcpConfig.EnableDhcp != *nsxSubnet.DhcpConfig.EnableDhcp {
+		errorMsg = fmt.Sprintf("Subnet CRDHCPConfig cannot be modified")
+		return nsxutil.ImmutableFieldModifyError{Desc: errorMsg}
+	}
+
+	if existingNsxSubnet.Ipv4SubnetSize != nil && nsxSubnet.Ipv4SubnetSize != nil {
+		if *existingNsxSubnet.Ipv4SubnetSize != *nsxSubnet.Ipv4SubnetSize {
+			errorMsg = fmt.Sprintf("Subnet CR Ipv4SubnetSize cannot be modified")
+			return nsxutil.ImmutableFieldModifyError{Desc: errorMsg}
+		}
+	}
+
+	if !util.CompareStringArrays(existingNsxSubnet.IpAddresses, nsxSubnet.IpAddresses) {
+		errorMsg = fmt.Sprintf("Subnet CR IpAddresses cannot be modified")
+		return nsxutil.ImmutableFieldModifyError{Desc: errorMsg}
+	}
+	return nil
+}
+
+func (service *SubnetService) ValidateSubnetSetImmutableFields(subnetSet *v1alpha1.SubnetSet, nsxSubnet *model.VpcSubnet, isForVM bool) error {
+	errorMsg := ""
+
+	if isForVM {
+		// If allocating the subnet from subnetset for POD, it's allowed to change the AccessMode.
+		// For VM subnet, it's disallowed to change AccessMode
+		if util.Capitalize(string(subnetSet.Spec.AccessMode)) != *(nsxSubnet.AccessMode) {
+			errorMsg = fmt.Sprintf("Subnetset CR AccessMode cannot be modified when VM Subnet exists")
+			return nsxutil.ImmutableFieldModifyError{Desc: errorMsg}
+		}
+	}
+
+	if subnetSet.Spec.DHCPConfig.EnableDHCP != *nsxSubnet.DhcpConfig.EnableDhcp {
+		errorMsg = fmt.Sprintf("Subnetset CR DHCPConfig cannot be modified when Subnet exists")
+		return nsxutil.ImmutableFieldModifyError{Desc: errorMsg}
+	}
+
+	if int64(subnetSet.Spec.IPv4SubnetSize) != *nsxSubnet.Ipv4SubnetSize {
+		errorMsg = fmt.Sprintf("Subnetset CR Ipv4SubnetSize cannot be modified when Subnet exists")
+		return nsxutil.ImmutableFieldModifyError{Desc: errorMsg}
+	}
+	return nil
+}

pkg/nsx/util/errors.go

@@ -585,6 +585,12 @@ type ExceedTagsError struct {
 
 func (err ExceedTagsError) Error() string { return err.Desc }
 
+type ImmutableFieldModifyError struct {
+	Desc string
+}
+
+func (err ImmutableFieldModifyError) Error() string { return err.Desc }
+
 type Status struct {
 	Code    uint32
 	Message string

pkg/util/utils.go

@@ -10,6 +10,8 @@ import (
 	"errors"
 	"fmt"
 	"net"
+	"slices"
+	"sort"
 	"strconv"
 	"strings"
 
@@ -511,3 +513,9 @@ func Capitalize(s string) string {
 	}
 	return strings.ToUpper(s[:1]) + s[1:]
 }
+
+func CompareStringArrays(arr1, arr2 []string) bool {
+	sort.Strings(arr1)
+	sort.Strings(arr2)
+	return slices.Equal(arr1, arr2)
+}

pkg/util/utils_test.go

@@ -98,7 +98,6 @@ func TestUtil_IsNsInSystemNamespace(t *testing.T) {
 	ns = types.NamespacedName{Namespace: "sys-ns", Name: "dummy"}
 
 	isCRInSysNs, err = IsSystemNamespace(client, ns.Namespace, nil)
-
 	if err != nil {
 		t.Fatalf(err.Error())
 	}
@@ -639,3 +638,24 @@ func Test_calculateOffsetIP(t *testing.T) {
 		})
 	}
 }
+
+func Test_CompareStringArrays(t *testing.T) {
+	type args struct {
+		s1 []string
+		s2 []string
+	}
+	tests := []struct {
+		name string
+		args args
+		want bool
+	}{
+		{"1", args{[]string{"172.10.20.0/16", "172.20.30.0/16"}, []string{"172.20.30.0/16", "172.10.20.0/16"}}, true},
+		{"2", args{[]string{"172.10.20.0/16", "172.20.30.0/16"}, []string{"172.10.20.0/16", "172.10.40.0/16"}}, false},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			assert.Equal(t, tt.want, CompareStringArrays(tt.args.s1, tt.args.s2), "CompareStringArrays(%v, %v)", tt.args.s1, tt.args.s2)
+			assert.Equal(t, tt.want, CompareStringArrays(tt.args.s1, tt.args.s2), "CompareStringArrays(%v, %v)", tt.args.s1, tt.args.s2)
+		})
+	}
+}

test/e2e/manifest/testVPC/customize_networkconfig.yaml

@@ -8,7 +8,7 @@ spec:
   defaultGatewayPath: /infra/tier-0s/PLR
   # nsx-operator-ci would replace '{edge-cluster-id}' with real edge-cluster-id of testbed
   edgeClusterPath: /infra/sites/default/enforcement-points/default/edge-clusters/{edge-cluster-id}
-  defaultIPv4SubnetSize: 26
+  defaultIPv4SubnetSize: 32
   nsxtProject: /orgs/default/projects/nsx_operator_e2e_test
   externalIPv4Blocks:
     - /infra/ip-blocks/e2e_test_external_ip_blk

test/e2e/manifest/testVPC/customize_networkconfig_updated.yaml

@@ -8,7 +8,7 @@ spec:
   defaultGatewayPath: /infra/tier-0s/PLR
   # nsx-operator-ci would replace '{edge-cluster-id}' with real edge-cluster-id of testbed
   edgeClusterPath: /infra/sites/default/enforcement-points/default/edge-clusters/{edge-cluster-id}
-  defaultIPv4SubnetSize: 26
+  defaultIPv4SubnetSize: 32
   nsxtProject: /orgs/default/projects/nsx_operator_e2e_test
   externalIPv4Blocks:
     - /infra/ip-blocks/e2e_test_external_ip_blk

test/e2e/manifest/testVPC/default_networkconfig.yaml

@@ -12,7 +12,7 @@ spec:
   defaultGatewayPath: /infra/tier-0s/PLR
   # nsx-operator-ci would replace '{edge-cluster-id}' with real edge-cluster-id of testbed
   edgeClusterPath: /infra/sites/default/enforcement-points/default/edge-clusters/{edge-cluster-id}
-  defaultIPv4SubnetSize: 26
+  defaultIPv4SubnetSize: 32
   nsxtProject: /orgs/default/projects/nsx_operator_e2e_test
   externalIPv4Blocks:
     - /infra/ip-blocks/e2e_test_external_ip_blk

test/e2e/manifest/testVPC/system_networkconfig.yaml

@@ -9,7 +9,7 @@ spec:
   defaultGatewayPath: /infra/tier-0s/PLR
   # nsx-operator-ci would replace '{edge-cluster-id}' with real edge-cluster-id of testbed
   edgeClusterPath: /infra/sites/default/enforcement-points/default/edge-clusters/{edge-cluster-id}
-  defaultIPv4SubnetSize: 26
+  defaultIPv4SubnetSize: 32
   nsxtProject: /orgs/default/projects/nsx_operator_e2e_test
   externalIPv4Blocks:
     - /infra/ip-blocks/e2e_test_external_ip_blk