fix: NsxResource.executeRequest DeleteNsxNatRuleCommand comparison bug by ZhyliaievD · Pull Request #12833 · apache/cloudstack

ZhyliaievD · 2026-03-17T10:01:53Z

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

boring-cyborg · 2026-03-17T10:01:56Z

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 · 2026-03-17T10:29:45Z

@blueorangutan package

blueorangutan · 2026-03-17T10:32:05Z

@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 · 2026-03-17T10:38:38Z

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 1 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.

plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/resource/NsxResourceTest.java

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.

blueorangutan · 2026-03-17T11:16:23Z

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

blueorangutan · 2026-03-17T12:58:19Z

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

sureshanaparti

clgtm

boring-cyborg bot added the component:networking label Mar 17, 2026

DaanHoogland approved these changes Mar 17, 2026

View reviewed changes

DaanHoogland requested review from Pearl1594, Copilot and nvazquez March 17, 2026 10:49

Copilot started reviewing on behalf of DaanHoogland March 17, 2026 10:50 View session

Copilot AI reviewed Mar 17, 2026

View reviewed changes

plugins/network-elements/nsx/src/test/java/org/apache/cloudstack/resource/NsxResourceTest.java Outdated Show resolved Hide resolved

ZhyliaievD force-pushed the fix/delete-nsx-rule-cmd-execution branch from 6bff177 to f300f5a Compare March 17, 2026 11:02

winterhazel added this to the 4.23.0 milestone Mar 17, 2026

winterhazel added the status:needs-testing label Mar 17, 2026

sureshanaparti approved these changes Mar 17, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: NsxResource.executeRequest DeleteNsxNatRuleCommand comparison bug#12833

fix: NsxResource.executeRequest DeleteNsxNatRuleCommand comparison bug#12833
ZhyliaievD wants to merge 1 commit intoapache:mainfrom
PlaytikaOSS:fix/delete-nsx-rule-cmd-execution

ZhyliaievD commented Mar 17, 2026

Uh oh!

boring-cyborg bot commented Mar 17, 2026

Uh oh!

DaanHoogland left a comment

Uh oh!

DaanHoogland commented Mar 17, 2026

Uh oh!

blueorangutan commented Mar 17, 2026

Uh oh!

codecov bot commented Mar 17, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

blueorangutan commented Mar 17, 2026

Uh oh!

blueorangutan commented Mar 17, 2026

Uh oh!

sureshanaparti left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Conversation

ZhyliaievD commented Mar 17, 2026

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Uh oh!

boring-cyborg bot commented Mar 17, 2026

Uh oh!

DaanHoogland left a comment

Choose a reason for hiding this comment

Uh oh!

DaanHoogland commented Mar 17, 2026

Uh oh!

blueorangutan commented Mar 17, 2026

Uh oh!

codecov bot commented Mar 17, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Uh oh!

blueorangutan commented Mar 17, 2026

Uh oh!

blueorangutan commented Mar 17, 2026

Uh oh!

sureshanaparti left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

codecov bot commented Mar 17, 2026 •

edited

Loading