Copilot commented on code in PR #12408:
URL: https://github.com/apache/cloudstack/pull/12408#discussion_r2690785466
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -1111,6 +1034,21 @@ public PublicIp
assignSourceNatIpAddressToGuestNetwork(Account owner, Network gu
return ipToReturn;
}
+ private boolean isRouted(Network guestNetwork) {
+ VpcOffering vpcOffer = null;
+ NetworkOffering netOffer =
_networkOfferingDao.findById(guestNetwork.getNetworkOfferingId());
+ if (netOffer == null) {
+ throw new CloudRuntimeException("network without offering
found???");
+ }
+ 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);
+ }
Review Comment:
The new `isRouted` method lacks test coverage. Since the repository has
comprehensive test coverage for IpAddressManagerImpl (see
IpAddressManagerTest.java), consider adding unit tests to verify:
1. The method correctly identifies routed networks based on NetworkOffering
routing mode
2. The method correctly identifies routed VPC networks based on VPC offering
routing mode
3. Edge cases like null VPC or null VPC offering are handled properly
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -1102,7 +1025,7 @@ public PublicIp
assignSourceNatIpAddressToGuestNetwork(Account owner, Network gu
IPAddressVO sourceNatIp = getExistingSourceNatInNetwork(owner.getId(),
guestNetwork.getId());
PublicIp ipToReturn = null;
- if (sourceNatIp != null) {
+ if (sourceNatIp != null || isRouted(guestNetwork)) {
ipToReturn = PublicIp.createFromAddrAndVlan(sourceNatIp,
_vlanDao.findById(sourceNatIp.getVlanId()));
Review Comment:
This condition introduces a critical bug. When `isRouted(guestNetwork)`
returns true but `sourceNatIp` is null, line 1029 will throw a
NullPointerException when attempting to call `sourceNatIp.getVlanId()`.
The logic should either:
1. Return null when `isRouted()` is true and `sourceNatIp` is null
(indicating no source NAT IP is needed), or
2. Only create the PublicIp object if `sourceNatIp` is not null
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
