From 7cdef2888f17cbc51bae2ebcd8ed51e32d6b641f Mon Sep 17 00:00:00 2001 From: Tao Zou Date: Wed, 6 May 2026 15:19:17 +0800 Subject: [PATCH] Check more property for pre-created subnet Verify that DHCP mode and static IP allocation are identical across all pre-created subnets in the SubnetSet. --- .../subnetset/subnetset_webhook.go | 38 ++- .../subnetset/subnetset_webhook_test.go | 313 ++++++++++++++++++ 2 files changed, 345 insertions(+), 6 deletions(-) diff --git a/pkg/controllers/subnetset/subnetset_webhook.go b/pkg/controllers/subnetset/subnetset_webhook.go index 6a3b37e4b..f687348ee 100644 --- a/pkg/controllers/subnetset/subnetset_webhook.go +++ b/pkg/controllers/subnetset/subnetset_webhook.go @@ -124,7 +124,7 @@ func (v *SubnetSetValidator) Handle(ctx context.Context, req admission.Request) if isDefaultSubnetSet(subnetSet) && req.UserInfo.Username != NSXOperatorSA { return admission.Denied("default SubnetSet only can be created by nsx-operator") } - deny, err := v.validateSubnetNames(ctx, subnetSet.Namespace, subnetSet.Spec.SubnetNames, subnetSet.Name) + deny, err := v.validateSubnets(ctx, subnetSet.Namespace, subnetSet.Spec.SubnetNames, subnetSet.Name) if err != nil { if deny { return admission.Denied(err.Error()) @@ -145,7 +145,7 @@ func (v *SubnetSetValidator) Handle(ctx context.Context, req admission.Request) log.Debug("Default SubnetSet label change detected", "oldLabels", oldSubnetSet.ObjectMeta.Labels, "newLabels", subnetSet.ObjectMeta.Labels, "username", req.UserInfo.Username) return admission.Denied(fmt.Sprintf("SubnetSet label %s can only be updated by NSX Operator", common.LabelDefaultNetwork)) } - deny, err := v.validateSubnetNames(ctx, subnetSet.Namespace, subnetSet.Spec.SubnetNames, subnetSet.Name) + deny, err := v.validateSubnets(ctx, subnetSet.Namespace, subnetSet.Spec.SubnetNames, subnetSet.Name) if err != nil { if deny { return admission.Denied(err.Error()) @@ -322,10 +322,24 @@ func (v *SubnetSetValidator) getVPCPath(ns string) (string, error) { // If any rule is broken, return true with error message. // If there is error when getting resources, return false with error. -func (v *SubnetSetValidator) validateSubnetNames(ctx context.Context, ns string, subnetNames *[]string, subnetSet string) (bool, error) { +// getEffectiveStaticIPAllocation computes the effective StaticIPAllocation value for a subnet. +// This replicates the logic from subnet controller to avoid race conditions during validation. +func getEffectiveStaticIPAllocation(subnet *v1alpha1.Subnet) bool { + // If explicitly set, use the set value + if subnet.Spec.AdvancedConfig.StaticIPAllocation.Enabled != nil { + return *subnet.Spec.AdvancedConfig.StaticIPAllocation.Enabled + } + // If not set, compute default value based on DHCP config (same logic as subnet controller) + return !util.CRSubnetDHCPEnabled(subnet) +} + +func (v *SubnetSetValidator) validateSubnets(ctx context.Context, ns string, subnetNames *[]string, subnetSet string) (bool, error) { var namespaceVpc string var existingVPC string firstAccessMode := "" + firstDHCPMode := "" + var firstEffectiveStaticIPAllocation bool + isFirstSubnet := true if subnetNames == nil { return true, nil } @@ -338,10 +352,22 @@ func (v *SubnetSetValidator) validateSubnetNames(ctx context.Context, ns string, if crdSubnet.Spec.SubnetDHCPConfig.Mode == v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeRelay) { return true, fmt.Errorf("DHCPRelay Subnet %s/%s is not supported in SubnetSet", crdSubnet.Namespace, crdSubnet.Name) } - if firstAccessMode == "" { + if isFirstSubnet { firstAccessMode = string(crdSubnet.Spec.AccessMode) - } else if firstAccessMode != string(crdSubnet.Spec.AccessMode) { - return true, fmt.Errorf("Subnets in SubnetSet %s/%s must have the same AccessMode, found different AccessModes: [%s, %s]", ns, subnetSet, firstAccessMode, crdSubnet.Spec.AccessMode) + firstDHCPMode = string(crdSubnet.Spec.SubnetDHCPConfig.Mode) + firstEffectiveStaticIPAllocation = getEffectiveStaticIPAllocation(crdSubnet) + isFirstSubnet = false + } else { + if firstAccessMode != string(crdSubnet.Spec.AccessMode) { + return true, fmt.Errorf("Subnets in SubnetSet %s/%s must have the same AccessMode, found different AccessModes: [%s, %s]", ns, subnetSet, firstAccessMode, crdSubnet.Spec.AccessMode) + } + if firstDHCPMode != string(crdSubnet.Spec.SubnetDHCPConfig.Mode) { + return true, fmt.Errorf("Subnets in SubnetSet %s/%s must have the same DHCPConfigMode, found different DHCPConfigModes: [%s, %s]", ns, subnetSet, firstDHCPMode, crdSubnet.Spec.SubnetDHCPConfig.Mode) + } + currentEffectiveStaticIPAllocation := getEffectiveStaticIPAllocation(crdSubnet) + if firstEffectiveStaticIPAllocation != currentEffectiveStaticIPAllocation { + return true, fmt.Errorf("Subnets in SubnetSet %s/%s must have the same StaticIPAllocation, found different StaticIPAllocations: [%t, %t]", ns, subnetSet, firstEffectiveStaticIPAllocation, currentEffectiveStaticIPAllocation) + } } var subnetVPC string associatedResource, exists := crdSubnet.Annotations[common.AnnotationAssociatedResource] diff --git a/pkg/controllers/subnetset/subnetset_webhook_test.go b/pkg/controllers/subnetset/subnetset_webhook_test.go index 941057e1d..91e040e4e 100644 --- a/pkg/controllers/subnetset/subnetset_webhook_test.go +++ b/pkg/controllers/subnetset/subnetset_webhook_test.go @@ -192,6 +192,130 @@ func TestSubnetSetValidator(t *testing.T) { AccessMode: v1alpha1.AccessMode(v1alpha1.AccessModePrivate), }, }) + fakeClient.Create(context.TODO(), &v1alpha1.Subnet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnet-dhcp-1", + Namespace: "ns-dhcp", + Annotations: map[string]string{common.AnnotationAssociatedResource: "default:ns-dhcp:subnet-dhcp-1"}, + }, + Spec: v1alpha1.SubnetSpec{ + SubnetDHCPConfig: v1alpha1.SubnetDHCPConfig{ + Mode: v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeDeactivated), + }, + }, + }) + fakeClient.Create(context.TODO(), &v1alpha1.Subnet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnet-dhcp-2", + Namespace: "ns-dhcp", + Annotations: map[string]string{common.AnnotationAssociatedResource: "default:ns-dhcp:subnet-dhcp-2"}, + }, + Spec: v1alpha1.SubnetSpec{ + SubnetDHCPConfig: v1alpha1.SubnetDHCPConfig{ + Mode: v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeServer), + }, + }, + }) + trueVal := true + falseVal := false + fakeClient.Create(context.TODO(), &v1alpha1.Subnet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnet-staticip-1", + Namespace: "ns-staticip", + Annotations: map[string]string{common.AnnotationAssociatedResource: "default:ns-staticip:subnet-staticip-1"}, + }, + Spec: v1alpha1.SubnetSpec{ + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: &trueVal, + }, + }, + }, + }) + fakeClient.Create(context.TODO(), &v1alpha1.Subnet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnet-staticip-2", + Namespace: "ns-staticip", + Annotations: map[string]string{common.AnnotationAssociatedResource: "default:ns-staticip:subnet-staticip-2"}, + }, + Spec: v1alpha1.SubnetSpec{ + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: &falseVal, + }, + }, + }, + }) + + // Create subnets for testing race condition fix + fakeClient.Create(context.TODO(), &v1alpha1.Subnet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnet-dhcp-deactivated-1", + Namespace: "ns-race", + Annotations: map[string]string{common.AnnotationAssociatedResource: "default:ns-race:subnet-dhcp-deactivated-1"}, + }, + Spec: v1alpha1.SubnetSpec{ + SubnetDHCPConfig: v1alpha1.SubnetDHCPConfig{ + Mode: v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeDeactivated), + }, + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: &trueVal, // Already processed by controller + }, + }, + }, + }) + fakeClient.Create(context.TODO(), &v1alpha1.Subnet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnet-dhcp-deactivated-2", + Namespace: "ns-race", + Annotations: map[string]string{common.AnnotationAssociatedResource: "default:ns-race:subnet-dhcp-deactivated-2"}, + }, + Spec: v1alpha1.SubnetSpec{ + SubnetDHCPConfig: v1alpha1.SubnetDHCPConfig{ + Mode: v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeDeactivated), + }, + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: nil, // Not yet processed by controller (race condition) + }, + }, + }, + }) + fakeClient.Create(context.TODO(), &v1alpha1.Subnet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnet-dhcp-server-1", + Namespace: "ns-race", + Annotations: map[string]string{common.AnnotationAssociatedResource: "default:ns-race:subnet-dhcp-server-1"}, + }, + Spec: v1alpha1.SubnetSpec{ + SubnetDHCPConfig: v1alpha1.SubnetDHCPConfig{ + Mode: v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeServer), + }, + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: &falseVal, // Already processed by controller + }, + }, + }, + }) + fakeClient.Create(context.TODO(), &v1alpha1.Subnet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnet-dhcp-server-2", + Namespace: "ns-race", + Annotations: map[string]string{common.AnnotationAssociatedResource: "default:ns-race:subnet-dhcp-server-2"}, + }, + Spec: v1alpha1.SubnetSpec{ + SubnetDHCPConfig: v1alpha1.SubnetDHCPConfig{ + Mode: v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeServer), + }, + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: nil, // Not yet processed by controller (race condition) + }, + }, + }, + }) patches := gomonkey.ApplyMethod(reflect.TypeOf(validator.vpcService), "ListVPCInfo", func(_ common.VPCServiceProvider, ns string) []common.VPCResourceInfo { return []common.VPCResourceInfo{{OrgID: "default", ProjectID: "default", VPCID: "ns-1"}} @@ -312,6 +436,38 @@ func TestSubnetSetValidator(t *testing.T) { isAllowed: false, msg: "Subnets in SubnetSet ns-accessmode/subnetset-accessmode must have the same AccessMode, found different AccessModes: [Public, Private]", }, + { + name: "Create SubnetSet with different DHCPModes", + op: admissionv1.Create, + subnetSet: &v1alpha1.SubnetSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnetset-dhcp", + Namespace: "ns-dhcp", + }, + Spec: v1alpha1.SubnetSetSpec{ + SubnetNames: &[]string{"subnet-dhcp-1", "subnet-dhcp-2"}, + }, + }, + user: "fake-user", + isAllowed: false, + msg: "Subnets in SubnetSet ns-dhcp/subnetset-dhcp must have the same DHCPConfigMode, found different DHCPConfigModes: [DHCPDeactivated, DHCPServer]", + }, + { + name: "Create SubnetSet with different StaticIPAllocations", + op: admissionv1.Create, + subnetSet: &v1alpha1.SubnetSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnetset-staticip", + Namespace: "ns-staticip", + }, + Spec: v1alpha1.SubnetSetSpec{ + SubnetNames: &[]string{"subnet-staticip-1", "subnet-staticip-2"}, + }, + }, + user: "fake-user", + isAllowed: false, + msg: "Subnets in SubnetSet ns-staticip/subnetset-staticip must have the same StaticIPAllocation, found different StaticIPAllocations: [true, false]", + }, { name: "Create SubnetSet with same AccessMode", op: admissionv1.Create, @@ -507,6 +663,54 @@ func TestSubnetSetValidator(t *testing.T) { isAllowed: false, msg: "used by SubnetPorts cannot be removed", }, + { + name: "Create SubnetSet with subnets in race condition - DHCP Deactivated (should pass)", + op: admissionv1.Create, + subnetSet: &v1alpha1.SubnetSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnetset-race-deactivated", + Namespace: "ns-race", + }, + Spec: v1alpha1.SubnetSetSpec{ + SubnetNames: &[]string{"subnet-dhcp-deactivated-1", "subnet-dhcp-deactivated-2"}, + }, + }, + user: "fake-user", + isAllowed: true, + accessModeCheck: true, + }, + { + name: "Create SubnetSet with subnets in race condition - DHCP Server (should pass)", + op: admissionv1.Create, + subnetSet: &v1alpha1.SubnetSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnetset-race-server", + Namespace: "ns-race", + }, + Spec: v1alpha1.SubnetSetSpec{ + SubnetNames: &[]string{"subnet-dhcp-server-1", "subnet-dhcp-server-2"}, + }, + }, + user: "fake-user", + isAllowed: true, + accessModeCheck: true, + }, + { + name: "Create SubnetSet with mixed DHCP modes in race condition (should fail on DHCP mode first)", + op: admissionv1.Create, + subnetSet: &v1alpha1.SubnetSet{ + ObjectMeta: metav1.ObjectMeta{ + Name: "subnetset-race-mixed", + Namespace: "ns-race", + }, + Spec: v1alpha1.SubnetSetSpec{ + SubnetNames: &[]string{"subnet-dhcp-deactivated-1", "subnet-dhcp-server-1"}, + }, + }, + user: "fake-user", + isAllowed: false, + msg: "must have the same DHCPConfigMode, found different DHCPConfigModes: [DHCPDeactivated, DHCPServer]", + }, } for _, testCase := range testcases { t.Run(testCase.name, func(t *testing.T) { @@ -786,6 +990,115 @@ func TestValidateRemovedSubnets(t *testing.T) { } } +func TestGetEffectiveStaticIPAllocation(t *testing.T) { + trueVal := true + falseVal := false + + tests := []struct { + name string + subnet *v1alpha1.Subnet + expected bool + }{ + { + name: "Explicitly set to true", + subnet: &v1alpha1.Subnet{ + Spec: v1alpha1.SubnetSpec{ + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: &trueVal, + }, + }, + }, + }, + expected: true, + }, + { + name: "Explicitly set to false", + subnet: &v1alpha1.Subnet{ + Spec: v1alpha1.SubnetSpec{ + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: &falseVal, + }, + }, + }, + }, + expected: false, + }, + { + name: "Not set, DHCP Deactivated (should default to true)", + subnet: &v1alpha1.Subnet{ + Spec: v1alpha1.SubnetSpec{ + SubnetDHCPConfig: v1alpha1.SubnetDHCPConfig{ + Mode: v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeDeactivated), + }, + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: nil, + }, + }, + }, + }, + expected: true, + }, + { + name: "Not set, DHCP Server (should default to false)", + subnet: &v1alpha1.Subnet{ + Spec: v1alpha1.SubnetSpec{ + SubnetDHCPConfig: v1alpha1.SubnetDHCPConfig{ + Mode: v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeServer), + }, + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: nil, + }, + }, + }, + }, + expected: false, + }, + { + name: "Not set, DHCP Relay (should default to false)", + subnet: &v1alpha1.Subnet{ + Spec: v1alpha1.SubnetSpec{ + SubnetDHCPConfig: v1alpha1.SubnetDHCPConfig{ + Mode: v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeRelay), + }, + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: nil, + }, + }, + }, + }, + expected: false, + }, + { + name: "Not set, empty DHCP mode (should default to true)", + subnet: &v1alpha1.Subnet{ + Spec: v1alpha1.SubnetSpec{ + SubnetDHCPConfig: v1alpha1.SubnetDHCPConfig{ + Mode: "", + }, + AdvancedConfig: v1alpha1.SubnetAdvancedConfig{ + StaticIPAllocation: v1alpha1.StaticIPAllocation{ + Enabled: nil, + }, + }, + }, + }, + expected: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := getEffectiveStaticIPAllocation(tt.subnet) + assert.Equal(t, tt.expected, result) + }) + } +} + func TestGetSubnetPortsID(t *testing.T) { scheme := runtime.NewScheme() _ = v1alpha1.AddToScheme(scheme)