fix: NsxResource.executeRequest DeleteNsxNatRuleCommand comparison bug

[ZhyliaievD]

Description

PR Fixes an issue in NsxResource.executeRequest where Network.Service comparison failed when DeleteNsxNatRuleCommand was executed in a different process. Due to serialization/deserialization, the deserialized Network.Service instance was not equal to the static instances Network.Service.StaticNat and Network.Service.PortForwarding, causing the comparison to always return false.

Types of changes

• Breaking change (fix or feature that would cause existing functionality to change)
• New feature (non-breaking change which adds functionality)
• Bug fix (non-breaking change which fixes an issue)
• Enhancement (improves an existing feature and functionality)
• Cleanup (Code refactoring and cleanup, that may add test cases)
• Build/CI
• Test (unit or integration test code)

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

• Major
• Minor

Bug Severity

• BLOCKER
• Critical
• Major
• Minor
• Trivial

[boring-cyborg]

Congratulations on your first Pull Request and welcome to the Apache CloudStack community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/cloudstack/blob/main/CONTRIBUTING.md)
Here are some useful points:

• In case of a new feature add useful documentation (raise doc PR at https://github.com/apache/cloudstack-documentation)
• Be patient and persistent. It might take some time to get a review or get the final approval from the committers.
• Pay attention to the quality of your code, ensure tests are passing and your PR doesn't have conflicts.
• Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Issues, Mailing list and Slack.
• Be sure to read the CloudStack Coding Conventions.
  Apache CloudStack is a community-driven project and together we are making it better 🚀.
  In case of doubts contact the developers at:
  Mailing List: dev@cloudstack.apache.org (https://cloudstack.apache.org/mailing-lists.html)
  Slack: https://apachecloudstack.slack.com/

[DaanHoogland]

clgtm

[DaanHoogland]

@blueorangutan package

[blueorangutan]

@DaanHoogland a [SL] Jenkins job has been kicked to build packages. It will be bundled with no SystemVM templates. I'll keep you posted as I make progress.

[codecov]

Codecov Report

❌ Patch coverage is 37.50000% with 5 lines in your changes missing coverage. Please review.
✅ Project coverage is 18.02%. Comparing base (3bd5410) to head (f300f5a).
⚠️ Report is 39 commits behind head on main.

Files with missing lines	Patch %	Lines
...va/org/apache/cloudstack/resource/NsxResource.java	0.00%	1 Missing and 2 partials ⚠️
.../cloudstack/agent/api/DeleteNsxNatRuleCommand.java	60.00%	1 Missing and 1 partial ⚠️

Additional details and impacted files

@@ Coverage Diff @@## main #12833 +/- ##
=========================================
Coverage 18.02% 18.02% - Complexity 16450 16452 +2 
=========================================
Files 5968 5968 Lines 537086 537091 +5 Branches 65961 65961 =========================================
+ Hits 96820 96825 +5 + Misses 429346 429344 -2 - Partials 10920 10922 +2 

Flag	Coverage Δ
uitests	3.53% <ø> (ø)
unittests	19.19% <37.50%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[Copilot]

Pull request overview

Fixes NSX NAT rule deletion behavior when DeleteNsxNatRuleCommand is executed in a different process (where Network.Service may be deserialized into a non-singleton instance), by switching comparisons to use the service name rather than object identity.

Changes:

• Update NsxResource.executeRequest(DeleteNsxNatRuleCommand) to compare Network.Service via getName() (through a new getNetworkServiceName() accessor).
• Add/update a unit test to exercise deletion using a non-static Network.Service instance and verify the deleteNatRule(...) call parameters.
• Fix an incorrect log message in load balancer rule deletion error handling (“add” → “delete”).

Reviewed changes

Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.

File	Description
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/resource/NsxResource.java	Uses service-name equality for NAT ruleName selection; fixes LB delete log message.
plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/agent/api/DeleteNsxNatRuleCommand.java	Adds getNetworkServiceName() helper to safely fetch the service name.
plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/resource/NsxResourceTest.java	Extends NAT delete test to set a non-static service and verify NSX client invocation.

Comments suppressed due to low confidence (2)

plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/resource/NsxResource.java:429

• executeRequest(DeleteNsxNatRuleCommand) now correctly uses the service name for ruleName selection, but it still passes cmd.getService() into nsxApiClient.deleteNatRule(...). Downstream, NsxApiClient.deleteNatRule uses identity comparison (service == Network.Service.PortForwarding) to decide whether to delete the port-forwarding service object, so a deserialized Network.Service instance will still skip that cleanup. Consider canonicalizing the service before calling the API (e.g., resolve via Network.Service.getService(cmd.getNetworkServiceName()) and pass that), or update the API to use service.getName()/serviceName instead of identity checks.

 if (Network.Service.StaticNat.getName().equals(cmd.getNetworkServiceName())) {
ruleName = NsxControllerUtils.getStaticNatRuleName(cmd.getDomainId(), cmd.getAccountId(), cmd.getZoneId(),
cmd.getNetworkResourceId(), cmd.isResourceVpc());
} else if (Network.Service.PortForwarding.getName().equals(cmd.getNetworkServiceName())) {
ruleName = NsxControllerUtils.getPortForwardRuleName(cmd.getDomainId(), cmd.getAccountId(), cmd.getZoneId(),
cmd.getNetworkResourceId(), cmd.getRuleId(), cmd.isResourceVpc());
}
String tier1GatewayName = NsxControllerUtils.getTier1GatewayName(cmd.getDomainId(), cmd.getAccountId(), cmd.getZoneId(),
cmd.getNetworkResourceId(), cmd.isResourceVpc());
try {
nsxApiClient.deleteNatRule(cmd.getService(), cmd.getPrivatePort(), cmd.getProtocol(),
cmd.getNetworkResourceName(), tier1GatewayName, ruleName);

plugins/network-elements/nsx/src/main/java/org/apache/cloudstack/resource/NsxResource.java:430

• If cmd.getNetworkServiceName() is null or not one of the handled values, ruleName stays null but the code still calls nsxApiClient.deleteNatRule(...) with a null ruleName, which can lead to confusing errors or NPEs inside the NSX client. Please add explicit validation/handling (e.g., return a failed NsxAnswer with a clear message) when the service is unsupported or ruleName cannot be derived.

 private NsxAnswer executeRequest(DeleteNsxNatRuleCommand cmd) {
String ruleName = null;
if (Network.Service.StaticNat.getName().equals(cmd.getNetworkServiceName())) {
ruleName = NsxControllerUtils.getStaticNatRuleName(cmd.getDomainId(), cmd.getAccountId(), cmd.getZoneId(),
cmd.getNetworkResourceId(), cmd.isResourceVpc());
} else if (Network.Service.PortForwarding.getName().equals(cmd.getNetworkServiceName())) {
ruleName = NsxControllerUtils.getPortForwardRuleName(cmd.getDomainId(), cmd.getAccountId(), cmd.getZoneId(),
cmd.getNetworkResourceId(), cmd.getRuleId(), cmd.isResourceVpc());
}
String tier1GatewayName = NsxControllerUtils.getTier1GatewayName(cmd.getDomainId(), cmd.getAccountId(), cmd.getZoneId(),
cmd.getNetworkResourceId(), cmd.isResourceVpc());
try {
nsxApiClient.deleteNatRule(cmd.getService(), cmd.getPrivatePort(), cmd.getProtocol(),
cmd.getNetworkResourceName(), tier1GatewayName, ruleName);
} catch (Exception e) {

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17171

[blueorangutan]

Packaging result [SF]: ✔️ el8 ✔️ el9 ✔️ el10 ✔️ debian ✔️ suse15. SL-JID 17172

[sureshanaparti]

clgtm

[DaanHoogland]

@blueorangutan test

[blueorangutan]

@DaanHoogland a [SL] Trillian-Jenkins test job (ol8 mgmt + kvm-ol8) has been kicked to run smoke tests

[blueorangutan]

[SF] Trillian test result (tid-15696)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 62750 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12833-t15696-kvm-ol8.zip
Smoke tests completed. 142 look OK, 9 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestListIdsParams>:teardown	Error	0.17	test_list_ids_parameter.py
ContextSuite context=TestSharedFSLifecycle>:setup	Error	0.00	test_sharedfs_lifecycle.py
test_01_snapshot_root_disk	Error	5.23	test_snapshots.py
test_02_list_snapshots_with_removed_data_store	Error	49.27	test_snapshots.py
test_02_list_snapshots_with_removed_data_store	Error	49.28	test_snapshots.py
ContextSuite context=TestSnapshotStandaloneBackup>:teardown	Error	35.32	test_snapshots.py
test_01_snapshot_usage	Error	28.10	test_usage.py
test_01_vpn_usage	Error	2.21	test_usage.py
test_02_migrate_vm_strict_tags_failure	Error	5.83	test_vm_strict_host_tags.py
ContextSuite context=TestRestoreVMStrictTags>:setup	Error	1531.28	test_vm_strict_host_tags.py
test_01_deploy_vm_on_specific_host_without_strict_tags	Error	5.92	test_vm_strict_host_tags.py
test_02_deploy_vm_on_any_host_without_strict_tags	Error	5.96	test_vm_strict_host_tags.py
test_03_deploy_vm_on_specific_host_with_strict_tags_success	Error	6.95	test_vm_strict_host_tags.py
test_04_deploy_vm_on_any_host_with_strict_tags_success	Error	2.89	test_vm_strict_host_tags.py
ContextSuite context=TestCreateVolume>:setup	Error	0.00	test_volumes.py
test_01_root_volume_encryption	Error	1.50	test_volumes.py
test_02_data_volume_encryption	Error	1.46	test_volumes.py
test_03_root_and_data_volume_encryption	Error	1.45	test_volumes.py
ContextSuite context=TestVolumes>:setup	Error	32.89	test_volumes.py
ContextSuite context=TestVPCConserveModeRules>:setup	Error	0.00	test_vpc_conserve_mode.py
ContextSuite context=TestIpv6Vpc>:setup	Error	0.00	test_vpc_ipv6.py
ContextSuite context=TestVPCRedundancy>:setup	Error	0.00	test_vpc_redundant.py

[ZhyliaievD]

Hi
@winterhazel@Pearl1594@nvazquez@DaanHoogland@sureshanaparti Test failures do not appear related to PR changes as far as I can see
Also as I can see all failed PR checks also fail in main branch

[blueorangutan]

[SF] Trillian test result (tid-15756)
Environment: kvm-ol8 (x2), zone: Advanced Networking with Mgmt server ol8
Total time taken: 51494 seconds
Marvin logs: https://github.com/blueorangutan/acs-prs/releases/download/trillian/pr12833-t15756-kvm-ol8.zip
Smoke tests completed. 148 look OK, 3 have errors, 0 did not run
Only failed and skipped tests results shown below:

Test	Result	Time (s)	Test File
ContextSuite context=TestListIdsParams>:teardown	Error	1.19	test_list_ids_parameter.py
test_01_snapshot_root_disk	Error	4.06	test_snapshots.py
test_02_list_snapshots_with_removed_data_store	Error	50.31	test_snapshots.py
test_02_list_snapshots_with_removed_data_store	Error	50.31	test_snapshots.py
ContextSuite context=TestSnapshotStandaloneBackup>:teardown	Error	33.93	test_snapshots.py
test_01_snapshot_usage	Error	19.94	test_usage.py
test_01_vpn_usage	Error	1.14	test_usage.py

[DaanHoogland]

looking at the code it makes sense to conclude the failures have nothing to do with this PR. only nsx plugin is touched.

@weizhouapache@winterhazel , up to you guys

[ZhyliaievD]

@weizhouapache@winterhazel a kind reminder about this PR 😅

[winterhazel]

@ZhyliaievD sorry for the delay. This one seems a safe change and the test failures unrelated, so I'm merging it already.

[boring-cyborg]

Awesome work, congrats on your first merged pull request!

[weizhouapache]

do we need to cherry-pick the commit from main to 4.22 branch ?

cc @DaanHoogland@sureshanaparti@rajujith@nvazquez@Pearl1594 @@ZhyliaievD

[ZhyliaievD]

yeah, I think so

[winterhazel]

do we need to cherry-pick the commit from main to 4.22 branch ?

@weizhouapache yes, it would be nice to cherry-pick it to 4.22, as the other NSX patches are being re-targeted too.