check if a source NAT IP address is needed before assigning one #12408

DaanHoogland · 2026-01-12T15:30:39Z

Description

This PR...

Fixes: #10122

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

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

codecov · 2026-01-12T15:38:41Z

Codecov Report

❌ Patch coverage is 2.63158% with 74 lines in your changes missing coverage. Please review.
✅ Project coverage is 16.23%. Comparing base (ef1aaa0) to head (e8cebc6).
⚠️ Report is 8 commits behind head on 4.20.

Files with missing lines	Patch %	Lines
...n/java/com/cloud/network/IpAddressManagerImpl.java	2.63%	73 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##               4.20   #12408   +/-   ##
=========================================
  Coverage     16.23%   16.23%           
+ Complexity    13381    13380    -1     
=========================================
  Files          5657     5657           
  Lines        498947   498983   +36     
  Branches      60555    60570   +15     
=========================================
+ Hits          81025    81033    +8     
- Misses       408889   408914   +25     
- Partials       9033     9036    +3

Flag	Coverage Δ
uitests	`4.03% <ø> (+0.02%)`	⬆️
unittests	`17.09% <2.63%> (-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.

DaanHoogland · 2026-01-13T07:47:50Z

@blueorangutan package

blueorangutan · 2026-01-13T07:50:04Z

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

blueorangutan · 2026-01-13T09:17:35Z

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

Copilot

Pull request overview

This PR addresses issue #10122 regarding incorrect IP address labeling as "Source NAT" in Routed VPC networks. The main change introduces logic to check if a network is routed before assigning a source NAT IP address, which prevents unnecessary source NAT IP allocation for routed networks where Source NAT service is not applicable.

Changes:

Adds new isRouted() method to determine if a network uses routing mode
Updates assignSourceNatIpAddressToGuestNetwork() to conditionally assign source NAT based on routing mode
Updates isSourceNatAvailableForNetwork() to treat networks with routing mode as having shared source NAT
Includes extensive code cleanup: removes unused imports and fields, applies diamond operators, and simplifies conditional logic

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

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

Copilot · 2026-01-13T13:12:32Z

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

+        VpcOffering vpcOffer = null;
+        NetworkOffering netOffer = _networkOfferingDao.findById(guestNetwork.getNetworkOfferingId());
+        if (netOffer.isForVpc()) {
+            vpcOffer = vpcOfferingDao.findById(guestNetwork.getVpcId());


Potential null pointer exception: If guestNetwork.getVpcId() returns null (which can happen for non-VPC networks even if the network offering is for VPC), the subsequent call to vpcOfferingDao.findById(null) will likely fail or return null, and then vpcOffer.getRoutingMode() would throw a NullPointerException.

Consider adding a null check for the VPC ID before attempting to retrieve the VPC offering, or handle the null case appropriately.

Suggested change

vpcOffer = vpcOfferingDao.findById(guestNetwork.getVpcId());

Long vpcId = guestNetwork.getVpcId();

if (vpcId != null) {

vpcOffer = vpcOfferingDao.findById(vpcId);

}

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

Copilot · 2026-01-13T13:12:33Z

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

            ipToReturn = PublicIp.createFromAddrAndVlan(sourceNatIp, _vlanDao.findById(sourceNatIp.getVlanId()));
        } else {
-            ipToReturn = assignDedicateIpAddress(owner, guestNetwork.getId(), null, dcId, true);
+            ipToReturn = assignDedicateIpAddress(owner, guestNetwork.getId(), null, dcId, ! isRouted(guestNetwork));


Potential logic issue: When the guest network is part of a VPC (i.e., a VPC tier network), passing null for the vpcId parameter may be incorrect. Consider passing guestNetwork.getVpcId() instead of null for the third parameter to properly handle VPC tier networks. This would align with how VPC IP assignment is done elsewhere in the codebase (see VpcManagerImpl line 3357).

Suggested change

ipToReturn = assignDedicateIpAddress(owner, guestNetwork.getId(), null, dcId, ! isRouted(guestNetwork));

ipToReturn = assignDedicateIpAddress(owner, guestNetwork.getId(), guestNetwork.getVpcId(), dcId, ! isRouted(guestNetwork));

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

weizhouapache · 2026-01-13T13:24:05Z

@DaanHoogland
as I understand, #10122 is a UI issue.
Does this PR fix other issues ?

DaanHoogland · 2026-01-13T13:27:45Z

@DaanHoogland as I understand, #10122 is a UI issue. Does this PR fix other issues ?

it is not a pure UI issue. The IP address is marked as source NAT in the DB. I am not sure if this addresses all scenarios yet, but it will prevent marking the primary IP for a ROUTED-mode network as source NAT.

Copilot

Pull request overview

Copilot reviewed 1 out of 1 changed files in this pull request and generated 10 comments.

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

Copilot · 2026-01-13T13:32:38Z

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

+        NetworkOffering netOffer = _networkOfferingDao.findById(guestNetwork.getNetworkOfferingId());
+        if (netOffer.isForVpc() && guestNetwork.getVpcId() != null) {
+            VpcVO vpc = _vpcDao.findById(guestNetwork.getVpcId());
+            if (vpc != null) {
+                vpcOffer = vpcOfferingDao.findById(vpc.getVpcOfferingId());
+            }
+        }
+        return netOffer.getRoutingMode() != null || (vpcOffer != null && vpcOffer.getRoutingMode() != null);


The method doesn't handle the case where netOffer could be null. If _networkOfferingDao.findById returns null, a NullPointerException will be thrown when calling netOffer.isForVpc() at line 1035 or netOffer.getRoutingMode() at line 1041. Add a null check for netOffer and return an appropriate default value (likely false) if it's null.

Copilot · 2026-01-13T13:32:39Z

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

+    private boolean isRouted(Network guestNetwork) {
+        VpcOffering vpcOffer = null;
+        NetworkOffering netOffer = _networkOfferingDao.findById(guestNetwork.getNetworkOfferingId());
+        if (netOffer.isForVpc() && guestNetwork.getVpcId() != null) {
+            VpcVO vpc = _vpcDao.findById(guestNetwork.getVpcId());
+            if (vpc != null) {
+                vpcOffer = vpcOfferingDao.findById(vpc.getVpcOfferingId());
+            }
+        }
+        return netOffer.getRoutingMode() != null || (vpcOffer != null && vpcOffer.getRoutingMode() != null);
+    }


The new isRouted method and the logic change in assignSourceNatIpAddressToGuestNetwork lack test coverage. Consider adding unit tests to verify the behavior when networks have routing modes set, including VPC networks with routing modes and regular networks with routing modes.

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

Copilot · 2026-01-13T13:32:39Z

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

        defaultIsolatedNetworkOfferingProviders.put(Service.Vpn, defaultProviders);

-        Map<Network.Service, Set<Network.Provider>> defaultSharedSGEnabledNetworkOfferingProviders = new HashMap<Network.Service, Set<Network.Provider>>();
+        Map<Network.Service, Set<Network.Provider>> defaultSharedSGEnabledNetworkOfferingProviders = new HashMap<>();


The contents of this container are never accessed.

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

Copilot · 2026-01-13T13:32:40Z

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

        //#8 - network offering with internal lb service
-        Map<Network.Service, Set<Network.Provider>> internalLbOffProviders = new HashMap<Network.Service, Set<Network.Provider>>();
-        Set<Network.Provider> defaultVpcProvider = new HashSet<Network.Provider>();
+        Map<Network.Service, Set<Network.Provider>> internalLbOffProviders = new HashMap<>();


The contents of this container are never accessed.

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

Copilot · 2026-01-13T13:32:41Z

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

-            } else {
-                Long totalCount = null;
+            if ( ! (ip.isSourceNat() || ip.isOneToOneNat())) {
+                Long totalCount;


The variable 'totalCount' is only assigned values of primitive type and is never 'null', but it is declared with the boxed type 'Long'.

check if a source NAT IP address is needed before assigning one

4dc321d

boring-cyborg bot added the component:networking label Jan 12, 2026

cleanup

7d116a1

DaanHoogland requested a review from Copilot January 13, 2026 13:02

Copilot started reviewing on behalf of DaanHoogland January 13, 2026 13:02 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

get offer for vpc

e8cebc6

Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>

DaanHoogland requested a review from Copilot January 13, 2026 13:21

Copilot started reviewing on behalf of DaanHoogland January 13, 2026 13:22 View session

Copilot AI reviewed Jan 13, 2026

View reviewed changes

-            vpcOffer = vpcOfferingDao.findById(guestNetwork.getVpcId());
+            Long vpcId = guestNetwork.getVpcId();
+            if (vpcId != null) {
+                vpcOffer = vpcOfferingDao.findById(vpcId);
+            }

	ipToReturn = assignDedicateIpAddress(owner, guestNetwork.getId(), null, dcId, ! isRouted(guestNetwork));
	ipToReturn = assignDedicateIpAddress(owner, guestNetwork.getId(), guestNetwork.getVpcId(), dcId, ! isRouted(guestNetwork));

check if a source NAT IP address is needed before assigning one #12408

Are you sure you want to change the base?

check if a source NAT IP address is needed before assigning one #12408

Conversation

DaanHoogland commented Jan 12, 2026

Description

Types of changes

Feature/Enhancement Scale or Bug Severity

Feature/Enhancement Scale

Bug Severity

Screenshots (if appropriate):

How Has This Been Tested?

How did you try to break this feature and the system with this change?

Uh oh!

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

Uh oh!

Codecov Report

Uh oh!

DaanHoogland commented Jan 13, 2026

Uh oh!

blueorangutan commented Jan 13, 2026

Uh oh!

blueorangutan commented Jan 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

weizhouapache commented Jan 13, 2026

Uh oh!

DaanHoogland commented Jan 13, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI Jan 13, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

codecov bot commented Jan 12, 2026 •

edited

Loading