Update dhcp validation to prevent switch from DHCPServer/DHCPRelay to…

… DHCPDeactivated (#996) Signed-off-by: Yanjun Zhou <[email protected]>
vmware-tanzu · Jan 10, 2025 · 245ba08 · 245ba08
1 parent 5546a6d
commit 245ba08
Show file tree

Hide file tree

Showing 5 changed files with 59 additions and 50 deletions.
diff --git a/build/yaml/crd/vpc/crd.nsx.vmware.com_subnets.yaml b/build/yaml/crd/vpc/crd.nsx.vmware.com_subnets.yaml
@@ -94,20 +94,26 @@ spec:
                     - DHCPDeactivated
                     type: string
                     x-kubernetes-validations:
-                    - message: subnetDHCPConfig cannot switch from DHCPDeactivated
-                        to other modes
-                      rule: oldSelf!='DHCPDeactivated' || oldSelf==self
+                    - message: subnetDHCPConfig mode can only switch between DHCPServer
+                        and DHCPRelay
+                      rule: oldSelf!='DHCPDeactivated' && self!='DHCPDeactivated'
+                        || oldSelf==self
                 type: object
                 x-kubernetes-validations:
-                - message: subnetDHCPConfig cannot switch from DHCPDeactivated to
-                    other modes
-                  rule: has(oldSelf.mode) || !has(self.mode) || self.mode=='DHCPDeactivated'
+                - message: subnetDHCPConfig mode can only switch between DHCPServer
+                    and DHCPRelay
+                  rule: has(oldSelf.mode)==has(self.mode) || (has(oldSelf.mode) &&
+                    !has(self.mode)  && oldSelf.mode=='DHCPDeactivated') || (!has(oldSelf.mode)
+                    && has(self.mode) && self.mode=='DHCPDeactivated')
             type: object
             x-kubernetes-validations:
-            - message: subnetDHCPConfig cannot switch from DHCPDeactivated to other
-                modes
-              rule: has(oldSelf.subnetDHCPConfig) || !has(self.subnetDHCPConfig) ||
-                !has(self.subnetDHCPConfig.mode) || self.subnetDHCPConfig.mode=='DHCPDeactivated'
+            - message: subnetDHCPConfig mode can only switch between DHCPServer and
+                DHCPRelay
+              rule: has(oldSelf.subnetDHCPConfig)==has(self.subnetDHCPConfig) || (has(oldSelf.subnetDHCPConfig)
+                && !has(self.subnetDHCPConfig) && (!has(oldSelf.subnetDHCPConfig.mode)
+                || oldSelf.subnetDHCPConfig.mode=='DHCPDeactivated')) || (!has(oldSelf.subnetDHCPConfig)
+                && has(self.subnetDHCPConfig) && (!has(self.subnetDHCPConfig.mode)
+                || self.subnetDHCPConfig.mode=='DHCPDeactivated'))
             - message: ipv4SubnetSize is required once set
               rule: '!has(oldSelf.ipv4SubnetSize) || has(self.ipv4SubnetSize)'
             - message: accessMode is required once set
@@ -162,6 +168,9 @@ spec:
                 type: array
             type: object
         type: object
+        x-kubernetes-validations:
+        - message: spec is required once set
+          rule: '!has(oldSelf.spec) || has(self.spec)'
     served: true
     storage: true
     subresources:

diff --git a/build/yaml/crd/vpc/crd.nsx.vmware.com_subnetsets.yaml b/build/yaml/crd/vpc/crd.nsx.vmware.com_subnetsets.yaml
@@ -84,20 +84,26 @@ spec:
                     - DHCPDeactivated
                     type: string
                     x-kubernetes-validations:
-                    - message: subnetDHCPConfig cannot switch from DHCPDeactivated
-                        to other modes
-                      rule: oldSelf!='DHCPDeactivated' || oldSelf==self
+                    - message: subnetDHCPConfig mode can only switch between DHCPServer
+                        and DHCPRelay
+                      rule: oldSelf!='DHCPDeactivated' && self!='DHCPDeactivated'
+                        || oldSelf==self
                 type: object
                 x-kubernetes-validations:
-                - message: subnetDHCPConfig cannot switch from DHCPDeactivated to
-                    other modes
-                  rule: has(oldSelf.mode) || !has(self.mode) || self.mode=='DHCPDeactivated'
+                - message: subnetDHCPConfig mode can only switch between DHCPServer
+                    and DHCPRelay
+                  rule: has(oldSelf.mode)==has(self.mode) || (has(oldSelf.mode) &&
+                    !has(self.mode)  && oldSelf.mode=='DHCPDeactivated') || (!has(oldSelf.mode)
+                    && has(self.mode) && self.mode=='DHCPDeactivated')
             type: object
             x-kubernetes-validations:
-            - message: subnetDHCPConfig cannot switch from DHCPDeactivated to other
-                modes
-              rule: has(oldSelf.subnetDHCPConfig) || !has(self.subnetDHCPConfig) ||
-                !has(self.subnetDHCPConfig.mode) || self.subnetDHCPConfig.mode=='DHCPDeactivated'
+            - message: subnetDHCPConfig mode can only switch between DHCPServer and
+                DHCPRelay
+              rule: has(oldSelf.subnetDHCPConfig)==has(self.subnetDHCPConfig) || (has(oldSelf.subnetDHCPConfig)
+                && !has(self.subnetDHCPConfig) && (!has(oldSelf.subnetDHCPConfig.mode)
+                || oldSelf.subnetDHCPConfig.mode=='DHCPDeactivated')) || (!has(oldSelf.subnetDHCPConfig)
+                && has(self.subnetDHCPConfig) && (!has(self.subnetDHCPConfig.mode)
+                || self.subnetDHCPConfig.mode=='DHCPDeactivated'))
             - message: accessMode is required once set
               rule: '!has(oldSelf.accessMode) || has(self.accessMode)'
             - message: ipv4SubnetSize is required once set
@@ -157,6 +163,9 @@ spec:
                 type: array
             type: object
         type: object
+        x-kubernetes-validations:
+        - message: spec is required once set
+          rule: '!has(oldSelf.spec) || has(self.spec)'
     served: true
     storage: true
     subresources:

diff --git a/pkg/apis/vpc/v1alpha1/subnet_types.go b/pkg/apis/vpc/v1alpha1/subnet_types.go
@@ -20,7 +20,7 @@ const (
 )
 
 // SubnetSpec defines the desired state of Subnet.
-// +kubebuilder:validation:XValidation:rule="has(oldSelf.subnetDHCPConfig) || !has(self.subnetDHCPConfig) || !has(self.subnetDHCPConfig.mode) || self.subnetDHCPConfig.mode=='DHCPDeactivated'", message="subnetDHCPConfig cannot switch from DHCPDeactivated to other modes"
+// +kubebuilder:validation:XValidation:rule="has(oldSelf.subnetDHCPConfig)==has(self.subnetDHCPConfig) || (has(oldSelf.subnetDHCPConfig) && !has(self.subnetDHCPConfig) && (!has(oldSelf.subnetDHCPConfig.mode) || oldSelf.subnetDHCPConfig.mode=='DHCPDeactivated')) || (!has(oldSelf.subnetDHCPConfig) && has(self.subnetDHCPConfig) && (!has(self.subnetDHCPConfig.mode) || self.subnetDHCPConfig.mode=='DHCPDeactivated'))", message="subnetDHCPConfig mode can only switch between DHCPServer and DHCPRelay"
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.ipv4SubnetSize) || has(self.ipv4SubnetSize)", message="ipv4SubnetSize is required once set"
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.accessMode) || has(self.accessMode)", message="accessMode is required once set"
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.ipAddresses) || has(self.ipAddresses)", message="ipAddresses is required once set"
@@ -40,15 +40,17 @@ type SubnetSpec struct {
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable"
 	IPAddresses []string `json:"ipAddresses,omitempty"`
 
-	// DHCP mode of a Subnet cannot switch from DHCPDeactivated to DHCPServer or DHCPRelay.
+	// DHCP mode of a Subnet can only switch between DHCPServer or DHCPRelay.
 	// If subnetDHCPConfig is not set, the DHCP mode is DHCPDeactivated by default.
 	// In order to enforce this rule, three XValidation rules are defined.
 	// The rule on SubnetSpec prevents the condition that subnetDHCPConfig is not set in
-	// old SubnetSpec while the new SubnetSpec specifies a Mode other than DHCPDeactivated.
+	// old or current SubnetSpec while the current or old SubnetSpec specifies a Mode
+	// other than DHCPDeactivated.
 	// The rule on SubnetDHCPConfig prevents the condition that Mode is not set in old
-	// SubnetDHCPConfig while the new one specifies a Mode other than DHCPDeactivated.
-	// The rule on SubnetDHCPConfig.Mode prevents the Mode changing from DHCPDeactivated
-	// to DHCPServer or DHCPRelay.
+	// or current SubnetDHCPConfig while the current or old one specifies a Mode other
+	// than DHCPDeactivated.
+	// The rule on SubnetDHCPConfig.Mode prevents the Mode changing between DHCPDeactivated
+	// and DHCPServer or DHCPRelay.
 
 	// DHCP configuration for Subnet.
 	SubnetDHCPConfig SubnetDHCPConfig `json:"subnetDHCPConfig,omitempty"`
@@ -74,6 +76,7 @@ type SubnetStatus struct {
 // +kubebuilder:printcolumn:name="AccessMode",type=string,JSONPath=`.spec.accessMode`,description="Access mode of Subnet"
 // +kubebuilder:printcolumn:name="IPv4SubnetSize",type=string,JSONPath=`.spec.ipv4SubnetSize`,description="Size of Subnet"
 // +kubebuilder:printcolumn:name="NetworkAddresses",type=string,JSONPath=`.status.networkAddresses[*]`,description="CIDRs for the Subnet"
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec) || has(self.spec)", message="spec is required once set"
 type Subnet struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`
@@ -92,12 +95,12 @@ type SubnetList struct {
 }
 
 // SubnetDHCPConfig is DHCP configuration for Subnet.
-// +kubebuilder:validation:XValidation:rule="has(oldSelf.mode) || !has(self.mode) || self.mode=='DHCPDeactivated'", message="subnetDHCPConfig cannot switch from DHCPDeactivated to other modes"
+// +kubebuilder:validation:XValidation:rule="has(oldSelf.mode)==has(self.mode) || (has(oldSelf.mode) && !has(self.mode)  && oldSelf.mode=='DHCPDeactivated') || (!has(oldSelf.mode) && has(self.mode) && self.mode=='DHCPDeactivated')", message="subnetDHCPConfig mode can only switch between DHCPServer and DHCPRelay"
 type SubnetDHCPConfig struct {
 	// DHCP Mode. DHCPDeactivated will be used if it is not defined.
 	// It cannot switch from DHCPDeactivated to DHCPServer or DHCPRelay.
 	// +kubebuilder:validation:Enum=DHCPServer;DHCPRelay;DHCPDeactivated
-	// +kubebuilder:validation:XValidation:rule="oldSelf!='DHCPDeactivated' || oldSelf==self", message="subnetDHCPConfig cannot switch from DHCPDeactivated to other modes"
+	// +kubebuilder:validation:XValidation:rule="oldSelf!='DHCPDeactivated' && self!='DHCPDeactivated' || oldSelf==self", message="subnetDHCPConfig mode can only switch between DHCPServer and DHCPRelay"
 	Mode DHCPConfigMode `json:"mode,omitempty"`
 }
 

diff --git a/pkg/apis/vpc/v1alpha1/subnetset_types.go b/pkg/apis/vpc/v1alpha1/subnetset_types.go
@@ -8,7 +8,7 @@ import (
 )
 
 // SubnetSetSpec defines the desired state of SubnetSet.
-// +kubebuilder:validation:XValidation:rule="has(oldSelf.subnetDHCPConfig) || !has(self.subnetDHCPConfig) || !has(self.subnetDHCPConfig.mode) || self.subnetDHCPConfig.mode=='DHCPDeactivated'", message="subnetDHCPConfig cannot switch from DHCPDeactivated to other modes"
+// +kubebuilder:validation:XValidation:rule="has(oldSelf.subnetDHCPConfig)==has(self.subnetDHCPConfig) || (has(oldSelf.subnetDHCPConfig) && !has(self.subnetDHCPConfig) && (!has(oldSelf.subnetDHCPConfig.mode) || oldSelf.subnetDHCPConfig.mode=='DHCPDeactivated')) || (!has(oldSelf.subnetDHCPConfig) && has(self.subnetDHCPConfig) && (!has(self.subnetDHCPConfig.mode) || self.subnetDHCPConfig.mode=='DHCPDeactivated'))", message="subnetDHCPConfig mode can only switch between DHCPServer and DHCPRelay"
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.accessMode) || has(self.accessMode)", message="accessMode is required once set"
 // +kubebuilder:validation:XValidation:rule="!has(oldSelf.ipv4SubnetSize) || has(self.ipv4SubnetSize)", message="ipv4SubnetSize is required once set"
 type SubnetSetSpec struct {
@@ -21,15 +21,17 @@ type SubnetSetSpec struct {
 	// +kubebuilder:validation:Enum=Private;Public;PrivateTGW
 	// +kubebuilder:validation:XValidation:rule="self == oldSelf",message="Value is immutable"
 	AccessMode AccessMode `json:"accessMode,omitempty"`
-	// DHCP mode of a SubnetSet cannot switch from DHCPDeactivated to DHCPServer or DHCPRelay.
+	// DHCP mode of a Subnet can only switch between DHCPServer or DHCPRelay.
 	// If subnetDHCPConfig is not set, the DHCP mode is DHCPDeactivated by default.
 	// In order to enforce this rule, three XValidation rules are defined.
-	// The rule on SubnetSetSpec prevents the condition that subnetDHCPConfig is not set in
-	// old SubnetSetSpec while the new SubnetSetSpec specifies a Mode other than DHCPDeactivated.
+	// The rule on SubnetSpec prevents the condition that subnetDHCPConfig is not set in
+	// old or current SubnetSpec while the current or old SubnetSpec specifies a Mode
+	// other than DHCPDeactivated.
 	// The rule on SubnetDHCPConfig prevents the condition that Mode is not set in old
-	// SubnetDHCPConfig while the new one specifies a Mode other than DHCPDeactivated.
-	// The rule on SubnetDHCPConfig.Mode prevents the Mode changing from DHCPDeactivated
-	// to DHCPServer or DHCPRelay.
+	// or current SubnetDHCPConfig while the current or old one specifies a Mode other
+	// than DHCPDeactivated.
+	// The rule on SubnetDHCPConfig.Mode prevents the Mode changing between DHCPDeactivated
+	// and DHCPServer or DHCPRelay.
 
 	// Subnet DHCP configuration.
 	SubnetDHCPConfig SubnetDHCPConfig `json:"subnetDHCPConfig,omitempty"`
@@ -60,6 +62,7 @@ type SubnetSetStatus struct {
 // +kubebuilder:printcolumn:name="AccessMode",type=string,JSONPath=`.spec.accessMode`,description="Access mode of Subnet"
 // +kubebuilder:printcolumn:name="IPv4SubnetSize",type=string,JSONPath=`.spec.ipv4SubnetSize`,description="Size of Subnet"
 // +kubebuilder:printcolumn:name="NetworkAddresses",type=string,JSONPath=`.status.subnets[*].networkAddresses[*]`,description="CIDRs for the SubnetSet"
+// +kubebuilder:validation:XValidation:rule="!has(oldSelf.spec) || has(self.spec)", message="spec is required once set"
 type SubnetSet struct {
 	metav1.TypeMeta   `json:",inline"`
 	metav1.ObjectMeta `json:"metadata,omitempty"`

diff --git a/test/e2e/nsx_subnet_test.go b/test/e2e/nsx_subnet_test.go
@@ -341,21 +341,6 @@ func SubnetCIDR(t *testing.T) {
 	nsxSubnets = testData.fetchSubnetBySubnetUID(t, newSubnetCRUID)
 	require.Equal(t, 1, len(nsxSubnets))
 
-	// Change the DHCP mode from DHCPServer to DHCPDeactived
-	allocatedSubnet.Spec.SubnetDHCPConfig.Mode = v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeDeactivated)
-	_, err = testData.crdClientset.CrdV1alpha1().Subnets(subnetTestNamespace).Update(context.TODO(), allocatedSubnet, v1.UpdateOptions{})
-	require.NoError(t, err)
-	allocatedSubnet = assureSubnet(t, subnetTestNamespace, subnet.Name, v1alpha1.DHCPConfigModeDeactivated)
-	nsxSubnets = testData.fetchSubnetBySubnetUID(t, newSubnetCRUID)
-	require.Equal(t, 1, len(nsxSubnets))
-	require.Equal(t, "DHCP_DEACTIVATED", *nsxSubnets[0].SubnetDhcpConfig.Mode)
-	require.Equal(t, true, *nsxSubnets[0].AdvancedConfig.StaticIpAllocation.Enabled)
-
-	// Change the DHCP mode from DHCPDeactived to DHCPServer
-	allocatedSubnet.Spec.SubnetDHCPConfig.Mode = v1alpha1.DHCPConfigMode(v1alpha1.DHCPConfigModeServer)
-	_, err = testData.crdClientset.CrdV1alpha1().Subnets(subnetTestNamespace).Update(context.TODO(), allocatedSubnet, v1.UpdateOptions{})
-	require.Contains(t, err.Error(), "subnetDHCPConfig cannot switch from DHCPDeactivated to other modes")
-
 	// Delete the Subnet
 	err = testData.crdClientset.CrdV1alpha1().Subnets(subnetTestNamespace).Delete(context.TODO(), subnet.Name, v1.DeleteOptions{})
 	require.NoError(t, err)