Copilot commented on code in PR #12408:
URL: https://github.com/apache/cloudstack/pull/12408#discussion_r2686372980
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -1105,12 +1023,21 @@ public PublicIp
assignSourceNatIpAddressToGuestNetwork(Account owner, Network gu
if (sourceNatIp != null) {
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));
}
return ipToReturn;
}
+ private boolean isRouted(Network guestNetwork) {
+ VpcOffering vpcOffer = null;
+ NetworkOffering netOffer =
_networkOfferingDao.findById(guestNetwork.getNetworkOfferingId());
+ if (netOffer.isForVpc()) {
+ vpcOffer = vpcOfferingDao.findById(guestNetwork.getVpcId());
Review Comment:
This line is incorrect. The method `vpcOfferingDao.findById()` expects a VPC
offering ID, but `guestNetwork.getVpcId()` returns the VPC ID, not the VPC
offering ID.
To fix this, you need to:
1. First retrieve the VPC object using
`_vpcDao.findById(guestNetwork.getVpcId())`
2. Then get the VPC offering ID from the VPC object using
`vpc.getVpcOfferingId()`
3. Finally call `vpcOfferingDao.findById(vpc.getVpcOfferingId())`
See line 1072-1073 in this file for the correct usage pattern.
```suggestion
if (netOffer.isForVpc() && guestNetwork.getVpcId() != null) {
VpcVO vpc = _vpcDao.findById(guestNetwork.getVpcId());
if (vpc != null) {
vpcOffer = vpcOfferingDao.findById(vpc.getVpcOfferingId());
}
```
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -1733,7 +1654,7 @@ public IPAddressVO
disassociatePortableIPToGuestNetwork(long ipId, long networkI
}
return ip;
} finally {
-
+ // catch (ResourceUnavailableException ignored)
Review Comment:
This comment appears to be a leftover from refactoring. The catch block was
removed but this comment remains. Since there's no actual catch block here and
the finally block is empty, consider either removing the finally block entirely
or removing this comment.
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -1105,12 +1023,21 @@ public PublicIp
assignSourceNatIpAddressToGuestNetwork(Account owner, Network gu
if (sourceNatIp != null) {
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));
}
return ipToReturn;
}
+ private boolean isRouted(Network guestNetwork) {
+ VpcOffering vpcOffer = null;
+ NetworkOffering netOffer =
_networkOfferingDao.findById(guestNetwork.getNetworkOfferingId());
+ if (netOffer.isForVpc()) {
+ vpcOffer = vpcOfferingDao.findById(guestNetwork.getVpcId());
Review Comment:
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.
```suggestion
Long vpcId = guestNetwork.getVpcId();
if (vpcId != null) {
vpcOffer = vpcOfferingDao.findById(vpcId);
}
```
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -1105,12 +1023,21 @@ public PublicIp
assignSourceNatIpAddressToGuestNetwork(Account owner, Network gu
if (sourceNatIp != null) {
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));
Review Comment:
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).
```suggestion
ipToReturn = assignDedicateIpAddress(owner,
guestNetwork.getId(), guestNetwork.getVpcId(), dcId, ! isRouted(guestNetwork));
```
--
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]
