Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
DaanHoogland wants to merge 1 commit into
base: 4.20
Choose a base branch
from
+26 −88
Open

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

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
114 changes: 26 additions & 88 deletions server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,6 @@

import javax.inject.Inject;

import com.cloud.network.dao.PublicIpQuarantineDao;
import com.cloud.network.vo.PublicIpQuarantineVO;
import com.cloud.resourcelimit.CheckedReservation;
import org.apache.cloudstack.acl.ControlledEntity.ACLType;
import org.apache.cloudstack.acl.SecurityChecker.AccessType;
import org.apache.cloudstack.annotation.AnnotationService;
Expand All @@ -57,10 +54,7 @@
import org.apache.cloudstack.reservation.dao.ReservationDao;
import org.apache.commons.collections.CollectionUtils;

import com.cloud.agent.AgentManager;
import com.cloud.alert.AlertManager;
import com.cloud.api.ApiDBUtils;
import com.cloud.configuration.ConfigurationManager;
import com.cloud.configuration.Resource.ResourceType;
import com.cloud.dc.AccountVlanMapVO;
import com.cloud.dc.DataCenter;
Expand All @@ -75,18 +69,15 @@
import com.cloud.dc.dao.AccountVlanMapDao;
import com.cloud.dc.dao.DataCenterDao;
import com.cloud.dc.dao.DataCenterIpAddressDao;
import com.cloud.dc.dao.DataCenterVnetDao;
import com.cloud.dc.dao.DomainVlanMapDao;
import com.cloud.dc.dao.HostPodDao;
import com.cloud.dc.dao.PodVlanMapDao;
import com.cloud.dc.dao.VlanDao;
import com.cloud.deploy.DeployDestination;
import com.cloud.domain.Domain;
import com.cloud.domain.dao.DomainDao;
import com.cloud.event.ActionEventUtils;
import com.cloud.event.EventTypes;
import com.cloud.event.UsageEventUtils;
import com.cloud.event.dao.UsageEventDao;
import com.cloud.exception.AccountLimitException;
import com.cloud.exception.ConcurrentOperationException;
import com.cloud.exception.InsufficientAddressCapacityException;
Expand All @@ -96,7 +87,6 @@
import com.cloud.exception.PermissionDeniedException;
import com.cloud.exception.ResourceAllocationException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.host.dao.HostDao;
import com.cloud.network.IpAddress.State;
import com.cloud.network.Network.Capability;
import com.cloud.network.Network.GuestType;
Expand All @@ -107,21 +97,14 @@
import com.cloud.network.Networks.IsolationType;
import com.cloud.network.Networks.TrafficType;
import com.cloud.network.addr.PublicIp;
import com.cloud.network.dao.AccountGuestVlanMapDao;
import com.cloud.network.dao.FirewallRulesDao;
import com.cloud.network.dao.IPAddressDao;
import com.cloud.network.dao.IPAddressVO;
import com.cloud.network.dao.LoadBalancerDao;
import com.cloud.network.dao.NetworkAccountDao;
import com.cloud.network.dao.NetworkDao;
import com.cloud.network.dao.NetworkDetailsDao;
import com.cloud.network.dao.NetworkDetailVO;
import com.cloud.network.dao.NetworkDomainDao;
import com.cloud.network.dao.NetworkServiceMapDao;
import com.cloud.network.dao.PhysicalNetworkDao;
import com.cloud.network.dao.PhysicalNetworkServiceProviderDao;
import com.cloud.network.dao.PhysicalNetworkTrafficTypeDao;
import com.cloud.network.dao.UserIpv6AddressDao;
import com.cloud.network.dao.PublicIpQuarantineDao;
import com.cloud.network.element.IpDeployer;
import com.cloud.network.element.IpDeployingRequester;
import com.cloud.network.element.NetworkElement;
Expand All @@ -134,21 +117,20 @@
import com.cloud.network.rules.FirewallRuleVO;
import com.cloud.network.rules.RulesManager;
import com.cloud.network.rules.StaticNat;
import com.cloud.network.rules.dao.PortForwardingRulesDao;
import com.cloud.network.vpc.NetworkACLManager;
import com.cloud.network.vo.PublicIpQuarantineVO;
import com.cloud.network.vpc.VpcManager;
import com.cloud.network.vpc.VpcOffering;
import com.cloud.network.vpc.VpcVO;
import com.cloud.network.vpc.dao.PrivateIpDao;
import com.cloud.network.vpc.dao.VpcDao;
import com.cloud.network.vpc.dao.VpcOfferingDao;
import com.cloud.network.vpn.RemoteAccessVpnService;
import com.cloud.offering.NetworkOffering;
import com.cloud.offering.NetworkOffering.Availability;
import com.cloud.offerings.NetworkOfferingVO;
import com.cloud.offerings.dao.NetworkOfferingDao;
import com.cloud.offerings.dao.NetworkOfferingDetailsDao;
import com.cloud.offerings.dao.NetworkOfferingServiceMapDao;
import com.cloud.org.Grouping;
import com.cloud.resourcelimit.CheckedReservation;
import com.cloud.user.Account;
import com.cloud.user.AccountManager;
import com.cloud.user.ResourceLimitService;
Expand Down Expand Up @@ -184,11 +166,7 @@
import com.cloud.vm.ReservationContextImpl;
import com.cloud.vm.VirtualMachine;
import com.cloud.vm.VirtualMachineProfile;
import com.cloud.vm.dao.NicDao;
import com.cloud.vm.dao.NicIpAliasDao;
import com.cloud.vm.dao.NicSecondaryIpDao;
import com.cloud.vm.dao.UserVmDao;
import com.cloud.vm.dao.VMInstanceDao;

public class IpAddressManagerImpl extends ManagerBase implements IpAddressManager, Configurable {

Expand All @@ -205,20 +183,12 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
@Inject
AccountDao _accountDao;
@Inject
DomainDao _domainDao;
@Inject
UserDao _userDao;
@Inject
ConfigurationDao _configDao;
@Inject
UserVmDao _userVmDao;
@Inject
AlertManager _alertMgr;
@Inject
AccountManager _accountMgr;
@Inject
ConfigurationManager _configMgr;
@Inject
AccountVlanMapDao _accountVlanMapDao;
@Inject
DomainVlanMapDao _domainVlanMapDao;
Expand All @@ -229,8 +199,6 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
@Inject
NetworkDetailsDao _networkDetailsDao;
@Inject
NicDao _nicDao;
@Inject
RulesManager _rulesMgr;
@Inject
LoadBalancingRulesManager _lbMgr;
Expand All @@ -239,22 +207,10 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
@Inject
PodVlanMapDao _podVlanMapDao;
@Inject
NetworkOfferingDetailsDao _ntwkOffDetailsDao;
@Inject
AccountGuestVlanMapDao _accountGuestVlanMapDao;
@Inject
DataCenterVnetDao _datacenterVnetDao;
@Inject
NetworkAccountDao _networkAccountDao;
@Inject
protected NicIpAliasDao _nicIpAliasDao;
@Inject
protected IPAddressDao _publicIpAddressDao;
@Inject
NetworkDomainDao _networkDomainDao;
@Inject
VMInstanceDao _vmDao;
@Inject
FirewallManager _firewallMgr;
@Inject
FirewallRulesDao _firewallDao;
Expand All @@ -268,36 +224,10 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
@Inject
PhysicalNetworkDao _physicalNetworkDao;
@Inject
PhysicalNetworkServiceProviderDao _pNSPDao;
@Inject
PortForwardingRulesDao _portForwardingRulesDao;
@Inject
LoadBalancerDao _lbDao;
@Inject
PhysicalNetworkTrafficTypeDao _pNTrafficTypeDao;
@Inject
AgentManager _agentMgr;
@Inject
HostDao _hostDao;
@Inject
NetworkServiceMapDao _ntwkSrvcDao;
@Inject
StorageNetworkManager _stnwMgr;
@Inject
VpcManager _vpcMgr;
@Inject
PrivateIpDao _privateIpDao;
@Inject
NetworkACLManager _networkACLMgr;
@Inject
UsageEventDao _usageEventDao;
@Inject
NetworkModel _networkModel;
@Inject
NicSecondaryIpDao _nicSecondaryIpDao;
@Inject
UserIpv6AddressDao _ipv6Dao;
@Inject
Ipv6AddressManager _ipv6Mgr;
@Inject
PortableIpDao _portableIpDao;
Expand Down Expand Up @@ -570,12 +500,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;
Long revokeCount = null;
Long activeCount = null;
Long addCount = null;
Expand All @@ -588,13 +514,13 @@ boolean checkIfIpAssocRequired(Network network, boolean postApplyRules, List<Pub
addCount = _firewallDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Add);
}

if (totalCount == null || totalCount.longValue() == 0L) {
if (totalCount == 0L) {
continue;
}

if (postApplyRules) {

if (revokeCount != null && revokeCount.longValue() == totalCount.longValue()) {
if (revokeCount != null && revokeCount.longValue() == totalCount) {
logger.trace("All rules are in Revoke state, have to dis-assiciate IP from the backend");
return true;
}
Expand All @@ -607,12 +533,9 @@ boolean checkIfIpAssocRequired(Network network, boolean postApplyRules, List<Pub
// reboot the VR. So ipassoc is needed.
return true;
}
continue;
} else if (addCount != null && addCount.longValue() == totalCount.longValue()) {
} else if (addCount != null && addCount.longValue() == totalCount) {
logger.trace("All rules are in Add state, have to assiciate IP with the backend");
return true;
} else {
continue;
}
}
}
Expand Down Expand Up @@ -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()));
Comment on lines +1028 to 1029
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.
} else {
ipToReturn = assignDedicateIpAddress(owner, guestNetwork.getId(), null, dcId, true);
Expand All @@ -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);
}
Comment on lines +1037 to +1050
Copy link

Copilot AI Jan 14, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copilot uses AI. Check for mistakes.

@DB
@Override
public PublicIp assignDedicateIpAddress(Account owner, final Long guestNtwkId, final Long vpcId, final long dcId, final boolean isSourceNat)
Expand Down Expand Up @@ -1633,7 +1571,7 @@ private static void validateNetworkAndIpOwnership(Account owner, IPAddressVO ipT
*/
protected boolean isSourceNatAvailableForNetwork(Account owner, IPAddressVO ipToAssoc, Network network) {
NetworkOffering offering = _networkOfferingDao.findById(network.getNetworkOfferingId());
boolean sharedSourceNat = offering.isSharedSourceNat();
boolean sharedSourceNat = offering.isSharedSourceNat() || offering.getRoutingMode() != null;
boolean isSourceNat = false;
if (!sharedSourceNat) {
if (getExistingSourceNatInNetwork(owner.getId(), network.getId()) == null) {
Expand Down
Loading