diff --git a/pkg/controllers/subnet/subnet_controller.go b/pkg/controllers/subnet/subnet_controller.go index d2de63968..8cc79cf33 100644 --- a/pkg/controllers/subnet/subnet_controller.go +++ b/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 diff --git a/pkg/controllers/subnetset/subnetset_controller.go b/pkg/controllers/subnetset/subnetset_controller.go index b50fa42b6..b23b2cf3d 100644 --- a/pkg/controllers/subnetset/subnetset_controller.go +++ b/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(), diff --git a/pkg/nsx/services/common/services.go b/pkg/nsx/services/common/services.go index 5829be053..75f647799 100644 --- a/pkg/nsx/services/common/services.go +++ b/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 { diff --git a/pkg/nsx/services/subnet/builder.go b/pkg/nsx/services/subnet/builder.go index 270d5d577..a934d0b84 100644 --- a/pkg/nsx/services/subnet/builder.go +++ b/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: diff --git a/pkg/nsx/services/subnet/subnet.go b/pkg/nsx/services/subnet/subnet.go index 0ece82fde..8d8a552d1 100644 --- a/pkg/nsx/services/subnet/subnet.go +++ b/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 CR DHCPConfig 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 +} diff --git a/pkg/nsx/util/errors.go b/pkg/nsx/util/errors.go index b606dfa26..694be3e40 100644 --- a/pkg/nsx/util/errors.go +++ b/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 diff --git a/pkg/util/utils.go b/pkg/util/utils.go index 0b537b313..82d668403 100644 --- a/pkg/util/utils.go +++ b/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) +} diff --git a/pkg/util/utils_test.go b/pkg/util/utils_test.go index f29b74ac4..48a68040e 100644 --- a/pkg/util/utils_test.go +++ b/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) + }) + } +} diff --git a/test/e2e/manifest/testVPC/customize_networkconfig.yaml b/test/e2e/manifest/testVPC/customize_networkconfig.yaml index 913c8e46d..379d650bc 100644 --- a/test/e2e/manifest/testVPC/customize_networkconfig.yaml +++ b/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 diff --git a/test/e2e/manifest/testVPC/customize_networkconfig_updated.yaml b/test/e2e/manifest/testVPC/customize_networkconfig_updated.yaml index a718cd518..5826b987c 100644 --- a/test/e2e/manifest/testVPC/customize_networkconfig_updated.yaml +++ b/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 diff --git a/test/e2e/manifest/testVPC/default_networkconfig.yaml b/test/e2e/manifest/testVPC/default_networkconfig.yaml index dbfb2edb3..e00ea68f4 100644 --- a/test/e2e/manifest/testVPC/default_networkconfig.yaml +++ b/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 diff --git a/test/e2e/manifest/testVPC/system_networkconfig.yaml b/test/e2e/manifest/testVPC/system_networkconfig.yaml index 843f7b5d0..56cc1c32f 100644 --- a/test/e2e/manifest/testVPC/system_networkconfig.yaml +++ b/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