Skip to content

Add StaticIPAllocation in Subnet spec #1119

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

heypnus
Copy link
Contributor

@heypnus heypnus commented Jun 24, 2025

The NSX operator will allow the customer to specify the property
subnetAdvancedConfig.staticIpAllocation.enabled for VPC Subnet in the Subnet CR.

If staticIPAllocation is not set explicitly in the Subnet CR, the static
IP pool allocation will be automatically enabled when the DHCP mode is DHCPDeactivated
or not set and disabled when the DHCP mode is DHCPServer or DHCPRelay. The NSX operator
will update the spec.subnetAdvancedConfig.staticIpAllocation.enabled with true of false.

The static IP pool allocation and DHCP mode cannot both be enabled simultaneously
in Subnet.

Testing done:

  1. create Subnet with the following spec, the NSX Subnet can be created with
    subnet_dhcp_config.mode=DHCP_DEACTIVATED and advanced_config.static_ip_allocation.enabled=false.
spec:
  accessMode: Private
  advancedConfig:
    staticIPAllocation:
      enabled: false
  1. create Subnet with the following spec, the NSX Subnet can be created with
    subnet_dhcp_config.mode=DHCP_DEACTIVATED and advanced_config.static_ip_allocation.enabled=true.
spec:
  accessMode: Private
  advancedConfig:
    staticIPAllocation:
      enabled: true
  1. create Subnet with the following spec, the NSX Subnet can be created with
    .subnet_dhcp_config.mode=DHCP_SERVER and static_ip_allocation.enabled=false.
spec:
  accessMode: Private
  subnetDHCPConfig:
    mode: DHCPServer

check the Subnet CR, its spec is updated to

spec:
  accessMode: Private
  advancedConfig:
    connectivityState: Connected
    enableVLANExtension: false
    staticIPAllocation:
      enabled: false
  ipv4SubnetSize: 32
  subnetDHCPConfig:
    mode: DHCPServer
  vpcName: Project Quality:ns-sunq-01_76261cd3-26a7-4e96-97d5-3886c66f0fa1
  1. create Subnet with the following spec, the NSX Subnet can be created with
    subnet_dhcp_config.mode=DHCP_DEACTIVATED and static_ip_allocation.enabled=true.
spec:
  accessMode: Private

check the Subnet CR, its spec is updated to

spec:
  accessMode: Private
  advancedConfig:
    connectivityState: Connected
    enableVLANExtension: false
    staticIPAllocation:
      enabled: true
  ipv4SubnetSize: 32
  subnetDHCPConfig: {}
  vpcName: Project Quality:ns-sunq-01_76261cd3-26a7-4e96-97d5-3886c66f0fa1
  1. create Subnet with the following spec
spec:
  accessMode: Private
  advancedConfig:
    staticIPAllocation:
      enabled: true
  subnetDHCPConfig:
    mode: DHCPServer

the kubectl reports the following error:
The Subnet "subnet-debug-05" is invalid: spec: Invalid value: "object": Static IP pool allocation and Subnet DHCP configuration cannot both be enabled simultaneously in Subnet

@heypnus heypnus force-pushed the feature/multi-nic-0624 branch 2 times, most recently from f44b798 to c08592a Compare June 24, 2025 14:54
@jianjuns
Copy link

Please change "subnet" to "Subnet" in the commit message and description.

Copy link

@jianjuns jianjuns left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What is the use case of exposing StaticIPAllocation in Subnet CRD? We need to support a case that a Subnet has neither DHCP or NSX IPAM enabled? @dantingl @lxiaopei

@dantingl
Copy link
Collaborator

What is the use case of exposing StaticIPAllocation in Subnet CRD? We need to support a case that a Subnet has neither DHCP or NSX IPAM enabled? @dantingl @lxiaopei

VKS has a requirement to not allocate IP for SubentPort, we plan to add StaticIPAllocation in Subnet CRD, so all ports would not get IP if StaticIPAllocation is false for DHCP_Deactivated Subnet.

@heypnus heypnus force-pushed the feature/multi-nic-0624 branch 7 times, most recently from d6a2f6a to 299a782 Compare June 25, 2025 11:58
@heypnus heypnus changed the title Add StaticIPAllocation in subnet CRD Add StaticIPAllocation in Subnet spec and subnetDHCPEnabled in SubnetPort status Jun 25, 2025
@heypnus
Copy link
Contributor Author

heypnus commented Jun 25, 2025

Please change "subnet" to "Subnet" in the commit message and description.

done

@heypnus heypnus requested a review from jianjuns June 25, 2025 12:04
@heypnus heypnus force-pushed the feature/multi-nic-0624 branch from 299a782 to 210249c Compare June 25, 2025 17:12
@jianjuns
Copy link

VKS has a requirement to not allocate IP for SubentPort, we plan to add StaticIPAllocation in Subnet CRD, so all ports would not get IP if StaticIPAllocation is false for DHCP_Deactivated Subnet.

I see. In NSX API, if StaticIPAllocation is false on a Subnet, a SubnetPort cannot request IP allocation from NSX IPAM? @dantingl

@heypnus
Copy link
Contributor Author

heypnus commented Jun 26, 2025

VKS has a requirement to not allocate IP for SubentPort, we plan to add StaticIPAllocation in Subnet CRD, so all ports would not get IP if StaticIPAllocation is false for DHCP_Deactivated Subnet.

I see. In NSX API, if StaticIPAllocation is false on a Subnet, a SubnetPort cannot request IP allocation from NSX IPAM? @dantingl

Yes. For the NSX API behavior for the IP allocation request, I ever posted the verification results in the design doc, if the StaticIPAllocation is disabled, all port creation request with allocate_addresses=BOTH/IP_POOL failed (e.g. Apr 22, 2025, 3:20:00 PM : LogicalSwitch 8ab0b3d5-c7b1-4d19-8712-5e82b8d87b73 does not have IpPoolId needed by LogicalPort address allocation type Both.). For the VKS use case, you can refer to the testing done case 8 in commit message. The 3 port kinds(DHCP, IPAM, no IP) maps to case 6, 7, 8, I posted the port status for the reviewer to compare.

@heypnus heypnus requested a review from jianjuns June 26, 2025 02:23
@heypnus heypnus force-pushed the feature/multi-nic-0624 branch 2 times, most recently from 30b937e to 9e57c2d Compare June 26, 2025 02:48
dantingl
dantingl previously approved these changes Jun 26, 2025
Copy link
Collaborator

@dantingl dantingl left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 73.05%. Comparing base (1265b52) to head (ea62c01).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #1119      +/-   ##
==========================================
+ Coverage   73.03%   73.05%   +0.01%     
==========================================
  Files         137      137              
  Lines       21668    21680      +12     
==========================================
+ Hits        15826    15838      +12     
  Misses       4847     4847              
  Partials      995      995              
Flag Coverage Δ
unit-tests 73.05% <100.00%> (+0.01%) ⬆️
Files with missing lines Coverage Δ
pkg/controllers/subnet/subnet_controller.go 68.63% <100.00%> (+0.29%) ⬆️
pkg/nsx/services/subnet/builder.go 88.88% <100.00%> (+0.13%) ⬆️
pkg/nsx/services/subnetport/builder.go 85.35% <100.00%> (+0.18%) ⬆️
pkg/nsx/services/subnetport/subnetport.go 86.12% <100.00%> (+0.61%) ⬆️
pkg/util/utils.go 83.12% <100.00%> (+0.43%) ⬆️

... and 1 file with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@heypnus heypnus force-pushed the feature/multi-nic-0624 branch from 951d094 to 2dba326 Compare June 27, 2025 02:32
@heypnus heypnus force-pushed the feature/multi-nic-0624 branch 2 times, most recently from 235a30e to 40249da Compare July 7, 2025 09:23
@heypnus heypnus force-pushed the feature/multi-nic-0624 branch 5 times, most recently from fe75170 to 7c7c8a0 Compare July 8, 2025 15:23
@heypnus heypnus requested review from jianjuns and dantingl July 8, 2025 16:16
@heypnus
Copy link
Contributor Author

heypnus commented Jul 15, 2025

/e2e

@dantingl dantingl requested a review from yanjunz97 July 15, 2025 09:01
@heypnus heypnus force-pushed the feature/multi-nic-0624 branch 2 times, most recently from 3b42e36 to f963a6c Compare July 16, 2025 05:03
@heypnus heypnus changed the title Add StaticIPAllocation in Subnet spec and subnetDHCPEnabled in SubnetPort status Add StaticIPAllocation in Subnet spec Jul 16, 2025
@heypnus heypnus force-pushed the feature/multi-nic-0624 branch from f963a6c to eec6a32 Compare July 16, 2025 07:08
@heypnus heypnus requested a review from dantingl July 16, 2025 07:08
@heypnus heypnus force-pushed the feature/multi-nic-0624 branch 2 times, most recently from 250a41b to 142a4e1 Compare July 16, 2025 07:29
@@ -168,6 +169,12 @@ func (r *SubnetReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctr
subnetCR.Spec.IPv4SubnetSize = vpcNetworkConfig.Spec.DefaultSubnetSize
specChanged = true
}

if subnetCR.Spec.AdvancedConfig.StaticIPAllocation.Enabled == nil {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we need to check whether subnetCR.Spec.AdvancedConfig is nil or not?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We don't need because its data structure is not a pointer. In the other word, if I change this line to if subnetCR.Spec.AdvancedConfig != nil && subnetCR.Spec.AdvancedConfig.StaticIPAllocation.Enabled == nil {, the golang will report error invalid operation: subnetCR.Spec.AdvancedConfig != nil (mismatched types "github.com/vmware-tanzu/nsx-operator/pkg/apis/vpc/v1alpha1".SubnetAdvancedConfig and untyped nil)
And the test cases 3 and 4 already covered the case that AdvancedConfig is not set.

@heypnus heypnus requested a review from dantingl July 16, 2025 08:48
The NSX operator will allow the customer to specify the property
subnetAdvancedConfig.staticIpAllocation.enabled for VPC Subnet in the Subnet CR.

If staticIPAllocation is not set explicitly in the Subnet CR, the static
IP pool allocation will be automatically enabled when the DHCP mode is DHCPDeactivated
or not set and disabled when the DHCP mode is DHCPServer or DHCPRelay. The NSX operator
will update the spec.subnetAdvancedConfig.staticIpAllocation.enabled with true of false.

The static IP pool allocation and DHCP mode cannot both be enabled simultaneously
in Subnet.

Testing done:
1. create Subnet with the following spec, the NSX Subnet can be created with
subnet_dhcp_config.mode=DHCP_DEACTIVATED and advanced_config.static_ip_allocation.enabled=false.
```
spec:
  accessMode: Private
  advancedConfig:
    staticIPAllocation:
      enabled: false
```
2. create Subnet with the following spec, the NSX Subnet can be created with
subnet_dhcp_config.mode=DHCP_DEACTIVATED and advanced_config.static_ip_allocation.enabled=true.
```
spec:
  accessMode: Private
  advancedConfig:
    staticIPAllocation:
      enabled: true
```
3. create Subnet with the following spec, the NSX Subnet can be created with
.subnet_dhcp_config.mode=DHCP_SERVER and static_ip_allocation.enabled=false.
```
spec:
  accessMode: Private
  subnetDHCPConfig:
    mode: DHCPServer
```
check the Subnet CR, its spec is updated to
```
spec:
  accessMode: Private
  advancedConfig:
    connectivityState: Connected
    enableVLANExtension: false
    staticIPAllocation:
      enabled: false
  ipv4SubnetSize: 32
  subnetDHCPConfig:
    mode: DHCPServer
  vpcName: Project Quality:ns-sunq-01_76261cd3-26a7-4e96-97d5-3886c66f0fa1
```
4. create Subnet with the following spec, the NSX Subnet can be created with
subnet_dhcp_config.mode=DHCP_DEACTIVATED and static_ip_allocation.enabled=true.
```
spec:
  accessMode: Private
```
check the Subnet CR, its spec is updated to
```
spec:
  accessMode: Private
  advancedConfig:
    connectivityState: Connected
    enableVLANExtension: false
    staticIPAllocation:
      enabled: true
  ipv4SubnetSize: 32
  subnetDHCPConfig: {}
  vpcName: Project Quality:ns-sunq-01_76261cd3-26a7-4e96-97d5-3886c66f0fa1
```
5. create Subnet with the following spec
```
spec:
  accessMode: Private
  advancedConfig:
    staticIPAllocation:
      enabled: true
  subnetDHCPConfig:
    mode: DHCPServer
```
the kubectl reports the following error:
The Subnet "subnet-debug-05" is invalid: spec: Invalid value: "object": Static IP pool allocation and Subnet DHCP configuration cannot both be enabled simultaneously in Subnet
@heypnus heypnus force-pushed the feature/multi-nic-0624 branch from 142a4e1 to ea62c01 Compare July 16, 2025 08:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants