Copilot commented on code in PR #12408:
URL: https://github.com/apache/cloudstack/pull/12408#discussion_r2686452383
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -435,15 +362,15 @@ public boolean configure(String name, Map<String, Object>
params) {
defaultIsolatedNetworkOfferingProviders.put(Service.PortForwarding,
defaultProviders);
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<>();
Review Comment:
The contents of this container are never accessed.
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -1105,12 +1023,24 @@ 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() && 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 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.
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -1105,12 +1023,24 @@ 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() && 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 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:
##########
@@ -457,7 +384,7 @@ public boolean configure(String name, Map<String, Object>
params) {
defaultIsolatedSourceNatEnabledNetworkOfferingProviders.put(Service.PortForwarding,
defaultProviders);
defaultIsolatedSourceNatEnabledNetworkOfferingProviders.put(Service.Vpn,
defaultProviders);
- Map<Network.Service, Set<Network.Provider>> defaultVPCOffProviders =
new HashMap<Network.Service, Set<Network.Provider>>();
+ Map<Network.Service, Set<Network.Provider>> defaultVPCOffProviders =
new HashMap<>();
Review Comment:
The contents of this container are never accessed.
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -570,12 +497,8 @@ boolean checkIfIpAssocRequired(Network network, boolean
postApplyRules, List<Pub
}
for (PublicIp ip : publicIps) {
- if (ip.isSourceNat()) {
- continue;
- } else if (ip.isOneToOneNat()) {
- continue;
- } else {
- Long totalCount = null;
+ if ( ! (ip.isSourceNat() || ip.isOneToOneNat())) {
+ Long totalCount;
Review Comment:
The variable 'totalCount' is only assigned values of primitive type and is
never 'null', but it is declared with the boxed type 'Long'.
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -487,12 +414,12 @@ public boolean configure(String name, Map<String, Object>
params) {
internalLbOffProviders.put(Service.Lb, defaultInternalLbProvider);
internalLbOffProviders.put(Service.SourceNat, defaultVpcProvider);
- Map<Network.Service, Set<Network.Provider>> netscalerServiceProviders
= new HashMap<Network.Service, Set<Network.Provider>>();
- Set<Network.Provider> vrProvider = new HashSet<Network.Provider>();
+ Map<Network.Service, Set<Network.Provider>> netscalerServiceProviders
= new HashMap<>();
Review Comment:
The contents of this container are never accessed.
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -1733,7 +1657,7 @@ public IPAddressVO
disassociatePortableIPToGuestNetwork(long ipId, long networkI
}
return ip;
} finally {
-
+ // catch (ResourceUnavailableException ignored)
Review Comment:
This comment appears to be a leftover from code refactoring and should be
removed. The try-finally block doesn't have a catch clause, so this comment is
confusing and serves no purpose.
```suggestion
```
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -435,15 +362,15 @@ public boolean configure(String name, Map<String, Object>
params) {
defaultIsolatedNetworkOfferingProviders.put(Service.PortForwarding,
defaultProviders);
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<>();
defaultSharedSGEnabledNetworkOfferingProviders.put(Service.Dhcp,
defaultProviders);
defaultSharedSGEnabledNetworkOfferingProviders.put(Service.Dns,
defaultProviders);
defaultSharedSGEnabledNetworkOfferingProviders.put(Service.UserData,
defaultProviders);
- Set<Provider> sgProviders = new HashSet<Provider>();
+ Set<Provider> sgProviders = new HashSet<>();
sgProviders.add(Provider.SecurityGroupProvider);
defaultSharedSGEnabledNetworkOfferingProviders.put(Service.SecurityGroup,
sgProviders);
- Map<Network.Service, Set<Network.Provider>>
defaultIsolatedSourceNatEnabledNetworkOfferingProviders = new
HashMap<Network.Service, Set<Network.Provider>>();
+ Map<Network.Service, Set<Network.Provider>>
defaultIsolatedSourceNatEnabledNetworkOfferingProviders = new HashMap<>();
Review Comment:
The contents of this container are never accessed.
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -472,11 +399,11 @@ public boolean configure(String name, Map<String, Object>
params) {
defaultVPCOffProviders.put(Service.Vpn, defaultProviders);
//#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<>();
Review Comment:
The contents of this container are never accessed.
##########
server/src/main/java/com/cloud/network/IpAddressManagerImpl.java:
##########
@@ -501,10 +428,10 @@ public boolean configure(String name, Map<String, Object>
params) {
netscalerServiceProviders.put(Service.StaticNat, nsProvider);
netscalerServiceProviders.put(Service.Lb, nsProvider);
- Map<Service, Map<Capability, String>> serviceCapabilityMap = new
HashMap<Service, Map<Capability, String>>();
- Map<Capability, String> elb = new HashMap<Capability, String>();
+ Map<Service, Map<Capability, String>> serviceCapabilityMap = new
HashMap<>();
Review Comment:
The contents of this container are never accessed.
--
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]
