Skip to content

Commit 16b1cb8

Browse files
erikbockserikbocks
committed
Flexibilized public IP allocation
1 parent 7324ef4 commit 16b1cb8

4 files changed

Lines changed: 93 additions & 38 deletions

File tree

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

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -19,9 +19,21 @@
1919
import com.cloud.network.vo.PublicIpQuarantineVO;
2020
import com.cloud.utils.db.GenericDao;
2121

22+
import java.util.Date;
23+
import java.util.List;
24+
2225
public interface PublicIpQuarantineDao extends GenericDao<PublicIpQuarantineVO, Long> {
2326

2427
PublicIpQuarantineVO findByPublicIpAddressId(long publicIpAddressId);
2528

2629
PublicIpQuarantineVO findByIpAddress(String publicIpAddress);
30+
31+
/**
32+
* Returns a list of public IP addresses that are actively quarantined at the specified date and the previous owner differs from the specified user.
33+
*
34+
* @param userId used to check against the IP's previous owner.
35+
* @param date used to check if the quarantine is active;
36+
* @return a list of PublicIpQuarantineVOs
37+
*/
38+
List<PublicIpQuarantineVO> listQuarantinedIpAddressesToUser(Long userId, Date date);
2739
}

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

Lines changed: 26 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -26,15 +26,19 @@
2626

2727
import javax.annotation.PostConstruct;
2828
import javax.inject.Inject;
29+
import java.util.Date;
30+
import java.util.List;
2931

3032
@Component
3133
public class PublicIpQuarantineDaoImpl extends GenericDaoBase<PublicIpQuarantineVO, Long> implements PublicIpQuarantineDao {
3234
private SearchBuilder<PublicIpQuarantineVO> publicIpAddressByIdSearch;
3335

3436
private SearchBuilder<IPAddressVO> ipAddressSearchBuilder;
3537

38+
private SearchBuilder<PublicIpQuarantineVO> quarantinedIpAddressesSearch;
39+
3640
@Inject
37-
IPAddressDao ipAddressDao;
41+
private IPAddressDao ipAddressDao;
3842

3943
@PostConstruct
4044
public void init() {
@@ -47,8 +51,18 @@ public void init() {
4751
publicIpAddressByIdSearch.join("quarantineJoin", ipAddressSearchBuilder, ipAddressSearchBuilder.entity().getId(),
4852
publicIpAddressByIdSearch.entity().getPublicIpAddressId(), JoinBuilder.JoinType.INNER);
4953

54+
quarantinedIpAddressesSearch = createSearchBuilder();
55+
quarantinedIpAddressesSearch.and("previousOwnerId", quarantinedIpAddressesSearch.entity().getPreviousOwnerId(), SearchCriteria.Op.NEQ);
56+
quarantinedIpAddressesSearch.and();
57+
quarantinedIpAddressesSearch.op("removedIsNull", quarantinedIpAddressesSearch.entity().getRemoved(), SearchCriteria.Op.NULL);
58+
quarantinedIpAddressesSearch.and("endDate", quarantinedIpAddressesSearch.entity().getEndDate(), SearchCriteria.Op.GT);
59+
quarantinedIpAddressesSearch.or("removedIsNotNull", quarantinedIpAddressesSearch.entity().getRemoved(), SearchCriteria.Op.NNULL);
60+
quarantinedIpAddressesSearch.and("removedDateGt", quarantinedIpAddressesSearch.entity().getRemoved(), SearchCriteria.Op.GT);
61+
quarantinedIpAddressesSearch.cp();
62+
5063
ipAddressSearchBuilder.done();
5164
publicIpAddressByIdSearch.done();
65+
quarantinedIpAddressesSearch.done();
5266
}
5367

5468
@Override
@@ -68,4 +82,15 @@ public PublicIpQuarantineVO findByIpAddress(String publicIpAddress) {
6882

6983
return findOneBy(sc, filter);
7084
}
85+
86+
@Override
87+
public List<PublicIpQuarantineVO> listQuarantinedIpAddressesToUser(Long userId, Date date) {
88+
SearchCriteria<PublicIpQuarantineVO> sc = quarantinedIpAddressesSearch.create();
89+
90+
sc.setParameters("previousOwnerId", userId);
91+
sc.setParameters("endDate", date);
92+
sc.setParameters("removedDateGt", date);
93+
94+
return searchIncludingRemoved(sc, null, false, false);
95+
}
7196
}

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

Lines changed: 54 additions & 37 deletions
Original file line numberDiff line numberDiff line change
@@ -539,6 +539,9 @@ public boolean configure(String name, Map<String, Object> params) {
539539
AssignIpAddressSearch.and("allocated", AssignIpAddressSearch.entity().getAllocatedTime(), Op.NULL);
540540
AssignIpAddressSearch.and("vlanId", AssignIpAddressSearch.entity().getVlanId(), Op.IN);
541541
AssignIpAddressSearch.and("forSystemVms", AssignIpAddressSearch.entity().isForSystemVms(), Op.EQ);
542+
AssignIpAddressSearch.and("id", AssignIpAddressSearch.entity().getId(), Op.NIN);
543+
AssignIpAddressSearch.and("requestedAddress", AssignIpAddressSearch.entity().getAddress(), Op.EQ);
544+
AssignIpAddressSearch.and("routerAddress", AssignIpAddressSearch.entity().getAddress(), Op.NEQ);
542545

543546
SearchBuilder<VlanVO> vlanSearch = _vlanDao.createSearchBuilder();
544547
vlanSearch.and("type", vlanSearch.entity().getVlanType(), Op.EQ);
@@ -945,22 +948,35 @@ public List<IPAddressVO> listAvailablePublicIps(final long dcId, final Long podI
945948
if (podId != null) {
946949
sc = AssignIpAddressFromPodVlanSearch.create();
947950
sc.setJoinParameters("podVlanMapSB", "podId", podId);
948-
errorMessage.append(" pod id=" + podId);
951+
errorMessage.append(" pod id=").append(podId);
949952
} else {
950953
sc = AssignIpAddressSearch.create();
951-
errorMessage.append(" zone id=" + dcId);
954+
errorMessage.append(" zone id=").append(dcId);
955+
}
956+
957+
if (lockOneRow) {
958+
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());
959+
960+
List<PublicIpQuarantineVO> quarantinedAddresses = publicIpQuarantineDao.listQuarantinedIpAddressesToUser(owner.getId(), new Date());
961+
List<Long> quarantinedAddressesIDs = quarantinedAddresses.stream().map(PublicIpQuarantineVO::getPublicIpAddressId).collect(Collectors.toList());
962+
963+
logger.debug("Found addresses with the following IDs: {}.", quarantinedAddressesIDs);
964+
965+
if (CollectionUtils.isNotEmpty(quarantinedAddressesIDs)) {
966+
sc.setParameters("id", quarantinedAddressesIDs.toArray());
967+
}
952968
}
953969

954970
sc.setParameters("dc", dcId);
955971

956972
// for direct network take ip addresses only from the vlans belonging to the network
957973
if (vlanUse == VlanType.DirectAttached) {
958974
sc.setJoinParameters("vlan", "networkId", guestNetworkId);
959-
errorMessage.append(", network id=" + guestNetworkId);
975+
errorMessage.append(", network id=").append(guestNetworkId);
960976
}
961977
if (requestedGateway != null) {
962978
sc.setJoinParameters("vlan", "vlanGateway", requestedGateway);
963-
errorMessage.append(", requested gateway=" + requestedGateway);
979+
errorMessage.append(", requested gateway=").append(requestedGateway);
964980
}
965981
sc.setJoinParameters("vlan", "type", vlanUse);
966982

@@ -970,38 +986,39 @@ public List<IPAddressVO> listAvailablePublicIps(final long dcId, final Long podI
970986
NetworkDetailVO routerIpDetail = _networkDetailsDao.findDetail(network.getId(), ApiConstants.ROUTER_IP);
971987
routerIpAddress = routerIpDetail != null ? routerIpDetail.getValue() : null;
972988
}
989+
973990
if (requestedIp != null) {
974-
sc.addAnd("address", SearchCriteria.Op.EQ, requestedIp);
975-
errorMessage.append(": requested ip " + requestedIp + " is not available");
991+
sc.setParameters("requestedAddress", requestedIp);
992+
errorMessage.append(": requested ip ").append(requestedIp).append(" is not available");
976993
} else if (routerIpAddress != null) {
977-
sc.addAnd("address", Op.NEQ, routerIpAddress);
994+
sc.setParameters("routerAddress", routerIpAddress);
978995
}
979996

980997
boolean ascOrder = ! forSystemVms;
981-
Filter filter = new Filter(IPAddressVO.class, "forSystemVms", ascOrder, 0l, 1l);
998+
Filter filter = new Filter(IPAddressVO.class, "forSystemVms", ascOrder, 0L, 1L);
982999

9831000
filter.addOrderBy(IPAddressVO.class,"vlanId", true);
9841001

985-
List<IPAddressVO> addrs = new ArrayList<>();
1002+
List<IPAddressVO> addresses = new ArrayList<>();
9861003

9871004
if (forSystemVms) {
9881005
// Get Public IPs for system vms in dedicated ranges
9891006
sc.setParameters("forSystemVms", true);
9901007
if (lockOneRow) {
991-
addrs = _ipAddressDao.lockRows(sc, filter, true);
1008+
addresses = _ipAddressDao.lockRows(sc, filter, true);
9921009
} else {
993-
addrs = new ArrayList<>(_ipAddressDao.search(sc, null));
1010+
addresses = new ArrayList<>(_ipAddressDao.search(sc, null));
9941011
}
9951012
}
996-
if ((!lockOneRow || (lockOneRow && CollectionUtils.isEmpty(addrs))) &&
1013+
if ((!lockOneRow || (lockOneRow && CollectionUtils.isEmpty(addresses))) &&
9971014
!(forSystemVms && SystemVmPublicIpReservationModeStrictness.value())) {
9981015
sc.setParameters("forSystemVms", false);
9991016
// If owner has dedicated Public IP ranges, fetch IP from the dedicated range
10001017
// Otherwise fetch IP from the system pool
10011018
// 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.
10021019
if (network == null || !(network.getGuestType() == GuestType.Shared && zone.getNetworkType() == NetworkType.Advanced)) {
1003-
List<AccountVlanMapVO> maps = _accountVlanMapDao.listAccountVlanMapsByAccount(owner.getId());
1004-
for (AccountVlanMapVO map : maps) {
1020+
List<AccountVlanMapVO> accountVlanMaps = _accountVlanMapDao.listAccountVlanMapsByAccount(owner.getId());
1021+
for (AccountVlanMapVO map : accountVlanMaps) {
10051022
if (vlanDbIds == null || vlanDbIds.contains(map.getVlanDbId()))
10061023
dedicatedVlanDbIds.add(map.getVlanDbId());
10071024
}
@@ -1020,10 +1037,10 @@ public List<IPAddressVO> listAvailablePublicIps(final long dcId, final Long podI
10201037
if (!dedicatedVlanDbIds.isEmpty()) {
10211038
fetchFromDedicatedRange = true;
10221039
sc.setParameters("vlanId", dedicatedVlanDbIds.toArray());
1023-
errorMessage.append(", vlanId id=" + Arrays.toString(dedicatedVlanDbIds.toArray()));
1040+
errorMessage.append(", vlanId id=").append(Arrays.toString(dedicatedVlanDbIds.toArray()));
10241041
} else if (!nonDedicatedVlanDbIds.isEmpty()) {
10251042
sc.setParameters("vlanId", nonDedicatedVlanDbIds.toArray());
1026-
errorMessage.append(", vlanId id=" + Arrays.toString(nonDedicatedVlanDbIds.toArray()));
1043+
errorMessage.append(", vlanId id=").append(Arrays.toString(nonDedicatedVlanDbIds.toArray()));
10271044
} else {
10281045
if (podId != null) {
10291046
InsufficientAddressCapacityException ex = new InsufficientAddressCapacityException("Insufficient address capacity", Pod.class, podId);
@@ -1037,29 +1054,29 @@ public List<IPAddressVO> listAvailablePublicIps(final long dcId, final Long podI
10371054
}
10381055
}
10391056
if (lockOneRow) {
1040-
addrs = _ipAddressDao.lockRows(sc, filter, true);
1057+
addresses = _ipAddressDao.lockRows(sc, filter, true);
10411058
} else {
1042-
addrs = new ArrayList<>(_ipAddressDao.search(sc, null));
1059+
addresses = new ArrayList<>(_ipAddressDao.search(sc, null));
10431060
}
10441061

10451062
// If all the dedicated IPs of the owner are in use fetch an IP from the system pool
1046-
if ((!lockOneRow || (lockOneRow && addrs.size() == 0)) && fetchFromDedicatedRange && vlanUse == VlanType.VirtualNetwork) {
1063+
if ((!lockOneRow || (lockOneRow && addresses.isEmpty())) && fetchFromDedicatedRange && vlanUse == VlanType.VirtualNetwork) {
10471064
// Verify if account is allowed to acquire IPs from the system
10481065
boolean useSystemIps = UseSystemPublicIps.valueIn(owner.getId());
10491066
if (useSystemIps && !nonDedicatedVlanDbIds.isEmpty()) {
10501067
fetchFromDedicatedRange = false;
10511068
sc.setParameters("vlanId", nonDedicatedVlanDbIds.toArray());
10521069
errorMessage.append(", vlanId id=" + Arrays.toString(nonDedicatedVlanDbIds.toArray()));
10531070
if (lockOneRow) {
1054-
addrs = _ipAddressDao.lockRows(sc, filter, true);
1071+
addresses = _ipAddressDao.lockRows(sc, filter, true);
10551072
} else {
1056-
addrs.addAll(_ipAddressDao.search(sc, null));
1073+
addresses.addAll(_ipAddressDao.search(sc, null));
10571074
}
10581075
}
10591076
}
10601077
}
10611078

1062-
if (lockOneRow && addrs.size() == 0) {
1079+
if (lockOneRow && addresses.isEmpty()) {
10631080
if (podId != null) {
10641081
InsufficientAddressCapacityException ex = new InsufficientAddressCapacityException("Insufficient address capacity", Pod.class, podId);
10651082
// for now, we hardcode the table names, but we should ideally do a lookup for the tablename from the VO object.
@@ -1073,13 +1090,12 @@ public List<IPAddressVO> listAvailablePublicIps(final long dcId, final Long podI
10731090
}
10741091

10751092
if (lockOneRow) {
1076-
assert (addrs.size() == 1) : "Return size is incorrect: " + addrs.size();
1077-
IpAddress ipAddress = addrs.get(0);
1078-
boolean ipCanBeAllocated = canPublicIpAddressBeAllocated(ipAddress, owner);
1093+
IPAddressVO allocatableIp = addresses.get(0);
1094+
1095+
boolean isPublicIpAllocatable = canPublicIpAddressBeAllocated(allocatableIp, owner);
10791096

1080-
if (!ipCanBeAllocated) {
1081-
throw new InsufficientAddressCapacityException(String.format("Failed to allocate public IP address [%s] as it is in quarantine.", ipAddress.getAddress()),
1082-
DataCenter.class, dcId);
1097+
if (!isPublicIpAllocatable) {
1098+
throw new InsufficientAddressCapacityException(String.format("Failed to allocate public IP [%s] as it is in quarantine.", allocatableIp.getAddress()), DataCenter.class, dcId);
10831099
}
10841100
}
10851101

@@ -1088,12 +1104,12 @@ public List<IPAddressVO> listAvailablePublicIps(final long dcId, final Long podI
10881104
try {
10891105
_resourceLimitMgr.checkResourceLimit(owner, ResourceType.public_ip);
10901106
} catch (ResourceAllocationException ex) {
1091-
logger.warn("Failed to allocate resource of type " + ex.getResourceType() + " for account " + owner);
1107+
logger.warn("Failed to allocate resource of type {} for account {}", ex.getResourceType(), owner);
10921108
throw new AccountLimitException("Maximum number of public IP addresses for account: " + owner.getAccountName() + " has been exceeded.");
10931109
}
10941110
}
10951111

1096-
return addrs;
1112+
return addresses;
10971113
}
10981114

10991115
@DB
@@ -2546,26 +2562,27 @@ public boolean canPublicIpAddressBeAllocated(IpAddress ip, Account newOwner) {
25462562
PublicIpQuarantineVO publicIpQuarantineVO = publicIpQuarantineDao.findByPublicIpAddressId(ip.getId());
25472563

25482564
if (publicIpQuarantineVO == null) {
2549-
logger.debug(String.format("Public IP address [%s] is not in quarantine; therefore, it is allowed to be allocated.", ip));
2565+
logger.debug("Public IP address [{}] is not in quarantine; therefore, it is allowed to be allocated.", ip);
25502566
return true;
25512567
}
25522568

25532569
if (!isPublicIpAddressStillInQuarantine(publicIpQuarantineVO, new Date())) {
2554-
logger.debug(String.format("Public IP address [%s] is no longer in quarantine; therefore, it is allowed to be allocated.", ip));
2570+
logger.debug("Public IP address [{}] is no longer in quarantine; therefore, it is allowed to be allocated.", ip);
2571+
removePublicIpAddressFromQuarantine(publicIpQuarantineVO.getId(), "IP was removed from quarantine because it was no longer in quarantine.");
25552572
return true;
25562573
}
25572574

25582575
Account previousOwner = _accountMgr.getAccount(publicIpQuarantineVO.getPreviousOwnerId());
25592576

25602577
if (Objects.equals(previousOwner.getUuid(), newOwner.getUuid())) {
2561-
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" +
2562-
" can be allocated. The public IP address will be removed from quarantine.", ip, previousOwner, newOwner));
2578+
logger.debug("Public IP address [{}] is in quarantine; however, the Public IP previous owner [{}] is the same as the new owner [{}]; therefore the IP" +
2579+
" can be allocated. The public IP address will be removed from quarantine.", ip, previousOwner, newOwner);
25632580
removePublicIpAddressFromQuarantine(publicIpQuarantineVO.getId(), "IP was removed from quarantine because it has been allocated by the previous owner");
25642581
return true;
25652582
}
25662583

2567-
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 " +
2568-
"allocated.", ip, previousOwner, newOwner));
2584+
logger.error("Public IP address [{}] is in quarantine and the previous owner [{}] is different than the new owner [{}]; therefore, the IP cannot be " +
2585+
"allocated.", ip, previousOwner, newOwner);
25692586
return false;
25702587
}
25712588

@@ -2616,7 +2633,7 @@ public void removePublicIpAddressFromQuarantine(Long quarantineProcessId, String
26162633
publicIpQuarantineVO.setRemovalReason(removalReason);
26172634
publicIpQuarantineVO.setRemoverAccountId(removerAccountId);
26182635

2619-
logger.debug(String.format("Removing public IP Address [%s] from quarantine by updating the removed date to [%s].", ipAddress, removedDate));
2636+
logger.debug("Removing public IP Address [{}] from quarantine by updating the removed date to [{}].", ipAddress, removedDate);
26202637
publicIpQuarantineDao.persist(publicIpQuarantineVO);
26212638
}
26222639

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -356,6 +356,7 @@ public void checkIfPublicIpAddressIsNotInQuarantineAndCanBeAllocatedTestIpIsNoLo
356356
Mockito.when(ipAddressMock.getId()).thenReturn(dummyID);
357357
Mockito.when(publicIpQuarantineDaoMock.findByPublicIpAddressId(Mockito.anyLong())).thenReturn(publicIpQuarantineVOMock);
358358
Mockito.doReturn(false).when(ipAddressManager).isPublicIpAddressStillInQuarantine(Mockito.any(PublicIpQuarantineVO.class), Mockito.any(Date.class));
359+
Mockito.doNothing().when(ipAddressManager).removePublicIpAddressFromQuarantine(Mockito.anyLong(), Mockito.anyString());
359360

360361
boolean result = ipAddressManager.canPublicIpAddressBeAllocated(ipAddressMock, newOwnerMock);
361362

0 commit comments

Comments
 (0)