Flexibilize public IP selection

engine/schema/src/main/java/com/cloud/network/dao/PublicIpQuarantineDao.java

@@ -19,9 +19,21 @@
 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);
 }

engine/schema/src/main/java/com/cloud/network/dao/PublicIpQuarantineDaoImpl.java

@@ -26,15 +26,19 @@
 
 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 {
     private SearchBuilder<PublicIpQuarantineVO> publicIpAddressByIdSearch;
 
     private SearchBuilder<IPAddressVO> ipAddressSearchBuilder;
 
+    private SearchBuilder<PublicIpQuarantineVO> quarantinedIpAddressesSearch;
+
     @Inject
-    IPAddressDao ipAddressDao;
+    private IPAddressDao ipAddressDao;
 
     @PostConstruct
     public void init() {
@@ -47,8 +51,18 @@
         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.or("removedIsNotNull", quarantinedIpAddressesSearch.entity().getRemoved(), SearchCriteria.Op.NNULL);
+        quarantinedIpAddressesSearch.and("removedDateGt", quarantinedIpAddressesSearch.entity().getRemoved(), SearchCriteria.Op.GT);
+        quarantinedIpAddressesSearch.cp();
+
         ipAddressSearchBuilder.done();
         publicIpAddressByIdSearch.done();
+        quarantinedIpAddressesSearch.done();
     }
 
     @Override
@@ -68,4 +82,15 @@
 
         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);
+        sc.setParameters("removedDateGt", date);
+
+        return searchIncludingRemoved(sc, null, false, false);
+    }
 }

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

@@ -514,6 +514,9 @@
         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,22 +886,35 @@
         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: {}.", quarantinedAddressesIDs);
+
+            if (CollectionUtils.isNotEmpty(quarantinedAddressesIDs)) {
+                sc.setParameters("id", quarantinedAddressesIDs.toArray());
+            }
         }
 
         sc.setParameters("dc", dcId);
 
         // 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 @@
             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 @@
                 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,29 +992,29 @@
                 }
             }
             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()) {
                     fetchFromDedicatedRange = false;
                     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 @@
         }
 
         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 @@
             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 @@
         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 @@
         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);
     }
 

server/src/test/java/com/cloud/network/IpAddressManagerTest.java

@@ -356,6 +356,7 @@ public void checkIfPublicIpAddressIsNotInQuarantineAndCanBeAllocatedTestIpIsNoLo
         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);