This is an automated email from the ASF dual-hosted git repository.
DaanHoogland pushed a commit to branch 4.20
in repository https://gitbox.apache.org/repos/asf/cloudstack.git
The following commit(s) were added to refs/heads/4.20 by this push:
new 9f96c9d5eb9 Flexibilize public IP selection (#11076)
9f96c9d5eb9 is described below
commit 9f96c9d5eb97f1836103918cf520d72cc3395c51
Author: Erik Böck <[email protected]>
AuthorDate: Tue Apr 21 07:53:11 2026 -0300
Flexibilize public IP selection (#11076)
---
.../cloud/network/dao/PublicIpQuarantineDao.java | 12 +++
.../network/dao/PublicIpQuarantineDaoImpl.java | 22 ++++++
.../com/cloud/network/IpAddressManagerImpl.java | 91 +++++++++++++---------
.../com/cloud/network/IpAddressManagerTest.java | 1 +
test/integration/smoke/test_quarantined_ips.py | 8 +-
5 files changed, 92 insertions(+), 42 deletions(-)
diff --git
a/engine/schema/src/main/java/com/cloud/network/dao/PublicIpQuarantineDao.java
b/engine/schema/src/main/java/com/cloud/network/dao/PublicIpQuarantineDao.java
index ccba6bb1889..606bdaaaa7a 100644
---
a/engine/schema/src/main/java/com/cloud/network/dao/PublicIpQuarantineDao.java
+++
b/engine/schema/src/main/java/com/cloud/network/dao/PublicIpQuarantineDao.java
@@ -19,9 +19,21 @@ package com.cloud.network.dao;
import com.cloud.network.vo.PublicIpQuarantineVO;
import com.cloud.utils.db.GenericDao;
+import java.util.Date;
+import java.util.List;
+
public interface PublicIpQuarantineDao extends
GenericDao<PublicIpQuarantineVO, Long> {
PublicIpQuarantineVO findByPublicIpAddressId(long publicIpAddressId);
PublicIpQuarantineVO findByIpAddress(String publicIpAddress);
+
+ /**
+ * Returns a list of public IP addresses that are actively quarantined at
the specified date and the previous owner differs from the specified user.
+ *
+ * @param userId used to check against the IP's previous owner;
+ * @param date used to check if the quarantine is active;
+ * @return a list of PublicIpQuarantineVOs.
+ */
+ List<PublicIpQuarantineVO> listQuarantinedIpAddressesToUser(Long userId,
Date date);
}
diff --git
a/engine/schema/src/main/java/com/cloud/network/dao/PublicIpQuarantineDaoImpl.java
b/engine/schema/src/main/java/com/cloud/network/dao/PublicIpQuarantineDaoImpl.java
index a1b789b8a46..0c47a0d36e3 100644
---
a/engine/schema/src/main/java/com/cloud/network/dao/PublicIpQuarantineDaoImpl.java
+++
b/engine/schema/src/main/java/com/cloud/network/dao/PublicIpQuarantineDaoImpl.java
@@ -26,6 +26,8 @@ import org.springframework.stereotype.Component;
import javax.annotation.PostConstruct;
import javax.inject.Inject;
+import java.util.Date;
+import java.util.List;
@Component
public class PublicIpQuarantineDaoImpl extends
GenericDaoBase<PublicIpQuarantineVO, Long> implements PublicIpQuarantineDao {
@@ -33,6 +35,8 @@ public class PublicIpQuarantineDaoImpl extends
GenericDaoBase<PublicIpQuarantine
private SearchBuilder<IPAddressVO> ipAddressSearchBuilder;
+ private SearchBuilder<PublicIpQuarantineVO> quarantinedIpAddressesSearch;
+
@Inject
IPAddressDao ipAddressDao;
@@ -47,8 +51,16 @@ public class PublicIpQuarantineDaoImpl extends
GenericDaoBase<PublicIpQuarantine
publicIpAddressByIdSearch.join("quarantineJoin",
ipAddressSearchBuilder, ipAddressSearchBuilder.entity().getId(),
publicIpAddressByIdSearch.entity().getPublicIpAddressId(),
JoinBuilder.JoinType.INNER);
+ quarantinedIpAddressesSearch = createSearchBuilder();
+ quarantinedIpAddressesSearch.and("previousOwnerId",
quarantinedIpAddressesSearch.entity().getPreviousOwnerId(),
SearchCriteria.Op.NEQ);
+ quarantinedIpAddressesSearch.and();
+ quarantinedIpAddressesSearch.op("removedIsNull",
quarantinedIpAddressesSearch.entity().getRemoved(), SearchCriteria.Op.NULL);
+ quarantinedIpAddressesSearch.and("endDate",
quarantinedIpAddressesSearch.entity().getEndDate(), SearchCriteria.Op.GT);
+ quarantinedIpAddressesSearch.cp();
+
ipAddressSearchBuilder.done();
publicIpAddressByIdSearch.done();
+ quarantinedIpAddressesSearch.done();
}
@Override
@@ -68,4 +80,14 @@ public class PublicIpQuarantineDaoImpl extends
GenericDaoBase<PublicIpQuarantine
return findOneBy(sc, filter);
}
+
+ @Override
+ public List<PublicIpQuarantineVO> listQuarantinedIpAddressesToUser(Long
userId, Date date) {
+ SearchCriteria<PublicIpQuarantineVO> sc =
quarantinedIpAddressesSearch.create();
+
+ sc.setParameters("previousOwnerId", userId);
+ sc.setParameters("endDate", date);
+
+ return searchIncludingRemoved(sc, null, false, false);
+ }
}
diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
index fe555af9d50..c5ca392a437 100644
--- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
+++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
@@ -514,6 +514,9 @@ public class IpAddressManagerImpl extends ManagerBase
implements IpAddressManage
AssignIpAddressSearch.and("allocated",
AssignIpAddressSearch.entity().getAllocatedTime(), Op.NULL);
AssignIpAddressSearch.and("vlanId",
AssignIpAddressSearch.entity().getVlanId(), Op.IN);
AssignIpAddressSearch.and("forSystemVms",
AssignIpAddressSearch.entity().isForSystemVms(), Op.EQ);
+ AssignIpAddressSearch.and("id",
AssignIpAddressSearch.entity().getId(), Op.NIN);
+ AssignIpAddressSearch.and("requestedAddress",
AssignIpAddressSearch.entity().getAddress(), Op.EQ);
+ AssignIpAddressSearch.and("routerAddress",
AssignIpAddressSearch.entity().getAddress(), Op.NEQ);
SearchBuilder<VlanVO> vlanSearch = _vlanDao.createSearchBuilder();
vlanSearch.and("type", vlanSearch.entity().getVlanType(), Op.EQ);
@@ -883,10 +886,23 @@ public class IpAddressManagerImpl extends ManagerBase
implements IpAddressManage
if (podId != null) {
sc = AssignIpAddressFromPodVlanSearch.create();
sc.setJoinParameters("podVlanMapSB", "podId", podId);
- errorMessage.append(" pod id=" + podId);
+ errorMessage.append(" pod id=").append(podId);
} else {
sc = AssignIpAddressSearch.create();
- errorMessage.append(" zone id=" + dcId);
+ errorMessage.append(" zone id=").append(dcId);
+ }
+
+ if (lockOneRow) {
+ logger.debug("Listing quarantined public IPs to ignore on search
for public IP for system VM. The IPs ignored will be the ones that: were not
associated to account [{}]; were not removed yet; and with quarantine end dates
after [{}].", owner.getUuid(), new Date());
+
+ List<PublicIpQuarantineVO> quarantinedAddresses =
publicIpQuarantineDao.listQuarantinedIpAddressesToUser(owner.getId(), new
Date());
+ List<Long> quarantinedAddressesIDs =
quarantinedAddresses.stream().map(PublicIpQuarantineVO::getPublicIpAddressId).collect(Collectors.toList());
+
+ logger.debug("Found addresses with the following IDs: [{}] that
will be ignored when searching for available public IPs.",
quarantinedAddressesIDs);
+
+ if (CollectionUtils.isNotEmpty(quarantinedAddressesIDs)) {
+ sc.setParameters("id", quarantinedAddressesIDs.toArray());
+ }
}
sc.setParameters("dc", dcId);
@@ -894,11 +910,11 @@ public class IpAddressManagerImpl extends ManagerBase
implements IpAddressManage
// for direct network take ip addresses only from the vlans belonging
to the network
if (vlanUse == VlanType.DirectAttached) {
sc.setJoinParameters("vlan", "networkId", guestNetworkId);
- errorMessage.append(", network id=" + guestNetworkId);
+ errorMessage.append(", network id=").append(guestNetworkId);
}
if (requestedGateway != null) {
sc.setJoinParameters("vlan", "vlanGateway", requestedGateway);
- errorMessage.append(", requested gateway=" + requestedGateway);
+ errorMessage.append(", requested
gateway=").append(requestedGateway);
}
sc.setJoinParameters("vlan", "type", vlanUse);
@@ -908,38 +924,39 @@ public class IpAddressManagerImpl extends ManagerBase
implements IpAddressManage
NetworkDetailVO routerIpDetail =
_networkDetailsDao.findDetail(network.getId(), ApiConstants.ROUTER_IP);
routerIpAddress = routerIpDetail != null ?
routerIpDetail.getValue() : null;
}
+
if (requestedIp != null) {
- sc.addAnd("address", SearchCriteria.Op.EQ, requestedIp);
- errorMessage.append(": requested ip " + requestedIp + " is not
available");
+ sc.setParameters("requestedAddress", requestedIp);
+ errorMessage.append(": requested ip
").append(requestedIp).append(" is not available");
} else if (routerIpAddress != null) {
- sc.addAnd("address", Op.NEQ, routerIpAddress);
+ sc.setParameters("routerAddress", routerIpAddress);
}
boolean ascOrder = ! forSystemVms;
- Filter filter = new Filter(IPAddressVO.class, "forSystemVms",
ascOrder, 0l, 1l);
+ Filter filter = new Filter(IPAddressVO.class, "forSystemVms",
ascOrder, 0L, 1L);
filter.addOrderBy(IPAddressVO.class,"vlanId", true);
- List<IPAddressVO> addrs = new ArrayList<>();
+ List<IPAddressVO> addresses = new ArrayList<>();
if (forSystemVms) {
// Get Public IPs for system vms in dedicated ranges
sc.setParameters("forSystemVms", true);
if (lockOneRow) {
- addrs = _ipAddressDao.lockRows(sc, filter, true);
+ addresses = _ipAddressDao.lockRows(sc, filter, true);
} else {
- addrs = new ArrayList<>(_ipAddressDao.search(sc, null));
+ addresses = new ArrayList<>(_ipAddressDao.search(sc, null));
}
}
- if ((!lockOneRow || (lockOneRow && CollectionUtils.isEmpty(addrs))) &&
+ if ((!lockOneRow || (lockOneRow &&
CollectionUtils.isEmpty(addresses))) &&
!(forSystemVms &&
SystemVmPublicIpReservationModeStrictness.value())) {
sc.setParameters("forSystemVms", false);
// If owner has dedicated Public IP ranges, fetch IP from the
dedicated range
// Otherwise fetch IP from the system pool
// Checking if network is null in the case of system VM's. At the
time of allocation of IP address to systemVm, no network is present.
if (network == null || !(network.getGuestType() ==
GuestType.Shared && zone.getNetworkType() == NetworkType.Advanced)) {
- List<AccountVlanMapVO> maps =
_accountVlanMapDao.listAccountVlanMapsByAccount(owner.getId());
- for (AccountVlanMapVO map : maps) {
+ List<AccountVlanMapVO> accountVlanMaps =
_accountVlanMapDao.listAccountVlanMapsByAccount(owner.getId());
+ for (AccountVlanMapVO map : accountVlanMaps) {
if (vlanDbIds == null ||
vlanDbIds.contains(map.getVlanDbId()))
dedicatedVlanDbIds.add(map.getVlanDbId());
}
@@ -958,10 +975,10 @@ public class IpAddressManagerImpl extends ManagerBase
implements IpAddressManage
if (!dedicatedVlanDbIds.isEmpty()) {
fetchFromDedicatedRange = true;
sc.setParameters("vlanId", dedicatedVlanDbIds.toArray());
- errorMessage.append(", vlanId id=" +
Arrays.toString(dedicatedVlanDbIds.toArray()));
+ errorMessage.append(", vlanId
id=").append(Arrays.toString(dedicatedVlanDbIds.toArray()));
} else if (!nonDedicatedVlanDbIds.isEmpty()) {
sc.setParameters("vlanId",
nonDedicatedVlanDbIds.toArray());
- errorMessage.append(", vlanId id=" +
Arrays.toString(nonDedicatedVlanDbIds.toArray()));
+ errorMessage.append(", vlanId
id=").append(Arrays.toString(nonDedicatedVlanDbIds.toArray()));
} else {
if (podId != null) {
InsufficientAddressCapacityException ex = new
InsufficientAddressCapacityException("Insufficient address capacity",
Pod.class, podId);
@@ -975,13 +992,13 @@ public class IpAddressManagerImpl extends ManagerBase
implements IpAddressManage
}
}
if (lockOneRow) {
- addrs = _ipAddressDao.lockRows(sc, filter, true);
+ addresses = _ipAddressDao.lockRows(sc, filter, true);
} else {
- addrs = new ArrayList<>(_ipAddressDao.search(sc, null));
+ addresses = new ArrayList<>(_ipAddressDao.search(sc, null));
}
// If all the dedicated IPs of the owner are in use fetch an IP
from the system pool
- if ((!lockOneRow || (lockOneRow && addrs.size() == 0)) &&
fetchFromDedicatedRange && vlanUse == VlanType.VirtualNetwork) {
+ if ((!lockOneRow || (lockOneRow && addresses.isEmpty())) &&
fetchFromDedicatedRange && vlanUse == VlanType.VirtualNetwork) {
// Verify if account is allowed to acquire IPs from the system
boolean useSystemIps =
UseSystemPublicIps.valueIn(owner.getId());
if (useSystemIps && !nonDedicatedVlanDbIds.isEmpty()) {
@@ -989,15 +1006,15 @@ public class IpAddressManagerImpl extends ManagerBase
implements IpAddressManage
sc.setParameters("vlanId",
nonDedicatedVlanDbIds.toArray());
errorMessage.append(", vlanId id=" +
Arrays.toString(nonDedicatedVlanDbIds.toArray()));
if (lockOneRow) {
- addrs = _ipAddressDao.lockRows(sc, filter, true);
+ addresses = _ipAddressDao.lockRows(sc, filter, true);
} else {
- addrs.addAll(_ipAddressDao.search(sc, null));
+ addresses.addAll(_ipAddressDao.search(sc, null));
}
}
}
}
- if (lockOneRow && addrs.size() == 0) {
+ if (lockOneRow && addresses.isEmpty()) {
if (podId != null) {
InsufficientAddressCapacityException ex = new
InsufficientAddressCapacityException("Insufficient address capacity",
Pod.class, podId);
// for now, we hardcode the table names, but we should ideally
do a lookup for the tablename from the VO object.
@@ -1011,13 +1028,12 @@ public class IpAddressManagerImpl extends ManagerBase
implements IpAddressManage
}
if (lockOneRow) {
- assert (addrs.size() == 1) : "Return size is incorrect: " +
addrs.size();
- IpAddress ipAddress = addrs.get(0);
- boolean ipCanBeAllocated =
canPublicIpAddressBeAllocated(ipAddress, owner);
+ IPAddressVO allocatableIp = addresses.get(0);
+
+ boolean isPublicIpAllocatable =
canPublicIpAddressBeAllocated(allocatableIp, owner);
- if (!ipCanBeAllocated) {
- throw new
InsufficientAddressCapacityException(String.format("Failed to allocate public
IP address [%s] as it is in quarantine.", ipAddress.getAddress()),
- DataCenter.class, dcId);
+ if (!isPublicIpAllocatable) {
+ throw new
InsufficientAddressCapacityException(String.format("Failed to allocate public
IP [%s] as it is in quarantine.", allocatableIp.getAddress()),
DataCenter.class, dcId);
}
}
@@ -1026,12 +1042,12 @@ public class IpAddressManagerImpl extends ManagerBase
implements IpAddressManage
try {
_resourceLimitMgr.checkResourceLimit(owner,
ResourceType.public_ip);
} catch (ResourceAllocationException ex) {
- logger.warn("Failed to allocate resource of type " +
ex.getResourceType() + " for account " + owner);
+ logger.warn("Failed to allocate resource of type {} for
account {}", ex.getResourceType(), owner);
throw new AccountLimitException("Maximum number of public IP
addresses for account: " + owner.getAccountName() + " has been exceeded.");
}
}
- return addrs;
+ return addresses;
}
@DB
@@ -2458,26 +2474,27 @@ public class IpAddressManagerImpl extends ManagerBase
implements IpAddressManage
PublicIpQuarantineVO publicIpQuarantineVO =
publicIpQuarantineDao.findByPublicIpAddressId(ip.getId());
if (publicIpQuarantineVO == null) {
- logger.debug(String.format("Public IP address [%s] is not in
quarantine; therefore, it is allowed to be allocated.", ip));
+ logger.debug("Public IP address [{}] is not in quarantine;
therefore, it is allowed to be allocated.", ip);
return true;
}
if (!isPublicIpAddressStillInQuarantine(publicIpQuarantineVO, new
Date())) {
- logger.debug(String.format("Public IP address [%s] is no longer in
quarantine; therefore, it is allowed to be allocated.", ip));
+ logger.debug("Public IP address [{}] is no longer in quarantine;
therefore, it is allowed to be allocated.", ip);
+ removePublicIpAddressFromQuarantine(publicIpQuarantineVO.getId(),
"IP was removed from quarantine because it was no longer in quarantine.");
return true;
}
Account previousOwner =
_accountMgr.getAccount(publicIpQuarantineVO.getPreviousOwnerId());
if (Objects.equals(previousOwner.getUuid(), newOwner.getUuid())) {
- logger.debug(String.format("Public IP address [%s] is in
quarantine; however, the Public IP previous owner [%s] is the same as the new
owner [%s]; therefore the IP" +
- " can be allocated. The public IP address will be removed
from quarantine.", ip, previousOwner, newOwner));
+ logger.debug("Public IP address [{}] is in quarantine; however,
the Public IP previous owner [{}] is the same as the new owner [{}]; therefore
the IP" +
+ " can be allocated. The public IP address will be removed
from quarantine.", ip, previousOwner, newOwner);
removePublicIpAddressFromQuarantine(publicIpQuarantineVO.getId(),
"IP was removed from quarantine because it has been allocated by the previous
owner");
return true;
}
- logger.error(String.format("Public IP address [%s] is in quarantine
and the previous owner [%s] is different than the new owner [%s]; therefore,
the IP cannot be " +
- "allocated.", ip, previousOwner, newOwner));
+ logger.error("Public IP address [{}] is in quarantine and the previous
owner [{}] is different than the new owner [{}]; therefore, the IP cannot be " +
+ "allocated.", ip, previousOwner, newOwner);
return false;
}
@@ -2528,7 +2545,7 @@ public class IpAddressManagerImpl extends ManagerBase
implements IpAddressManage
publicIpQuarantineVO.setRemovalReason(removalReason);
publicIpQuarantineVO.setRemoverAccountId(removerAccountId);
- logger.debug(String.format("Removing public IP Address [%s] from
quarantine by updating the removed date to [%s].", ipAddress, removedDate));
+ logger.debug("Removing public IP Address [{}] from quarantine by
updating the removed date to [{}].", ipAddress, removedDate);
publicIpQuarantineDao.persist(publicIpQuarantineVO);
}
diff --git a/server/src/test/java/com/cloud/network/IpAddressManagerTest.java
b/server/src/test/java/com/cloud/network/IpAddressManagerTest.java
index 824d4ee4701..cf3a886ce99 100644
--- a/server/src/test/java/com/cloud/network/IpAddressManagerTest.java
+++ b/server/src/test/java/com/cloud/network/IpAddressManagerTest.java
@@ -356,6 +356,7 @@ public class IpAddressManagerTest {
Mockito.when(ipAddressMock.getId()).thenReturn(dummyID);
Mockito.when(publicIpQuarantineDaoMock.findByPublicIpAddressId(Mockito.anyLong())).thenReturn(publicIpQuarantineVOMock);
Mockito.doReturn(false).when(ipAddressManager).isPublicIpAddressStillInQuarantine(Mockito.any(PublicIpQuarantineVO.class),
Mockito.any(Date.class));
+
Mockito.doNothing().when(ipAddressManager).removePublicIpAddressFromQuarantine(Mockito.anyLong(),
Mockito.anyString());
boolean result =
ipAddressManager.canPublicIpAddressBeAllocated(ipAddressMock, newOwnerMock);
diff --git a/test/integration/smoke/test_quarantined_ips.py
b/test/integration/smoke/test_quarantined_ips.py
index 42349fd2a53..2469760da13 100644
--- a/test/integration/smoke/test_quarantined_ips.py
+++ b/test/integration/smoke/test_quarantined_ips.py
@@ -85,7 +85,7 @@ class TestQuarantineIPs(cloudstackTestCase):
self.services["root_admin"]["roletype"])
"""
- Set public.ip.address.quarantine.duration to 60 minutes
+ Set public.ip.address.quarantine.duration to 1 minute
"""
update_configuration_cmd = updateConfiguration.updateConfigurationCmd()
update_configuration_cmd.name = "public.ip.address.quarantine.duration"
@@ -168,8 +168,7 @@ class TestQuarantineIPs(cloudstackTestCase):
zoneid=self.zone.id,
vpcid=root_vpc.id,
ipaddress=ip_address)
- self.assertIn(f"Failed to allocate public IP address [{ip_address}] as
it is in quarantine.",
- exception.exception.errorMsg)
+ self.assertIn("errorCode: 533", exception.exception.errorMsg)
# Owner should be able to allocate its IP in quarantine
public_ip = PublicIPAddress.create(self.domain_admin_apiclient,
@@ -267,8 +266,7 @@ class TestQuarantineIPs(cloudstackTestCase):
zoneid=self.zone.id,
networkid=root_network.id,
ipaddress=ip_address)
- self.assertIn(f"Failed to allocate public IP address [{ip_address}] as
it is in quarantine.",
- exception.exception.errorMsg)
+ self.assertIn("errorCode: 533", exception.exception.errorMsg)
# Owner should be able to allocate its IP in quarantine
public_ip = PublicIPAddress.create(self.domain_admin_apiclient,
