From 72e3491cefa1e51d2c884dfe36a637a25adbfba4 Mon Sep 17 00:00:00 2001 From: Fabricio Duarte Date: Mon, 14 Aug 2023 05:33:29 -0300 Subject: [PATCH] server: Fix allocation of more public IPs than the account's limit (#7832) --- .../cloud/network/IpAddressManagerImpl.java | 37 ++++++++---- .../cloud/network/IpAddressManagerTest.java | 59 +++++++++++++++++++ 2 files changed, 86 insertions(+), 10 deletions(-) diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java index b690acd7dd9..5436dd6acb1 100644 --- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java +++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java @@ -991,7 +991,8 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage Account owner = _accountMgr.getAccount(addr.getAllocatedToAccountId()); if (_ipAddressDao.lockRow(addr.getId(), true) != null) { final IPAddressVO userIp = _ipAddressDao.findById(addr.getId()); - if (userIp.getState() == IpAddress.State.Allocating || addr.getState() == IpAddress.State.Free) { + if (userIp.getState() == IpAddress.State.Allocating || addr.getState() == IpAddress.State.Free || addr.getState() == IpAddress.State.Reserved) { + boolean shouldUpdateIpResourceCount = checkIfIpResourceCountShouldBeUpdated(addr); addr.setState(IpAddress.State.Allocated); if (_ipAddressDao.update(addr.getId(), addr)) { // Save usage event @@ -1004,7 +1005,7 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage addr.getAddress().toString(), addr.isSourceNat(), guestType, addr.getSystem(), usageHidden, addr.getClass().getName(), addr.getUuid()); } - if (updateIpResourceCount(addr)) { + if (shouldUpdateIpResourceCount) { _resourceLimitMgr.incrementResourceCount(owner.getId(), ResourceType.public_ip); } } @@ -1020,7 +1021,7 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage } } - private boolean isIpDedicated(IPAddressVO addr) { + protected boolean isIpDedicated(IPAddressVO addr) { List maps = _accountVlanMapDao.listAccountVlanMapsByVlan(addr.getVlanId()); if (maps != null && !maps.isEmpty()) return true; @@ -1113,7 +1114,7 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage // rule is applied. Similarly when last rule on the acquired IP is revoked, IP is not associated with any provider // but still be associated with the account. At this point just mark IP as allocated or released. for (IPAddressVO addr : userIps) { - if (addr.getState() == IpAddress.State.Allocating) { + if (addr.getState() == IpAddress.State.Allocating || addr.getState() == IpAddress.State.Reserved) { addr.setAssociatedWithNetworkId(network.getId()); markPublicIpAsAllocated(addr); } else if (addr.getState() == IpAddress.State.Releasing) { @@ -1510,7 +1511,6 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage IPAddressVO ip = _ipAddressDao.findById(ipId); //update ip address with networkId - ip.setState(State.Allocated); ip.setAssociatedWithNetworkId(networkId); ip.setSourceNat(isSourceNat); _ipAddressDao.update(ipId, ip); @@ -1523,7 +1523,7 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage } else { s_logger.warn("Failed to associate ip address " + ip.getAddress().addr() + " to network " + network); } - return ip; + return _ipAddressDao.findById(ipId); } finally { if (!success && releaseOnFailure) { if (ip != null) { @@ -1919,7 +1919,7 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage return Transaction.execute(new TransactionCallback() { @Override public IPAddressVO doInTransaction(TransactionStatus status) { - if (updateIpResourceCount(ip)) { + if (checkIfIpResourceCountShouldBeUpdated(ip)) { _resourceLimitMgr.decrementResourceCount(_ipAddressDao.findById(addrId).getAllocatedToAccountId(), ResourceType.public_ip); } @@ -1944,9 +1944,26 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage return ip; } - protected boolean updateIpResourceCount(IPAddressVO ip) { - // don't increment resource count for direct and dedicated ip addresses - return (ip.getAssociatedWithNetworkId() != null || ip.getVpcId() != null) && !isIpDedicated(ip); + protected boolean checkIfIpResourceCountShouldBeUpdated(IPAddressVO ip) { + boolean isDirectIp = ip.getAssociatedWithNetworkId() == null && ip.getVpcId() == null; + if (isDirectIp) { + s_logger.debug(String.format("IP address [%s] is direct; therefore, the resource count should not be updated.", ip)); + return false; + } + + if (isIpDedicated(ip)) { + s_logger.debug(String.format("IP address [%s] is dedicated; therefore, the resource count should not be updated.", ip)); + return false; + } + + boolean isReservedIp = ip.getState() == IpAddress.State.Reserved; + if (isReservedIp) { + s_logger.debug(String.format("IP address [%s] is reserved; therefore, the resource count should not be updated.", ip)); + return false; + } + + s_logger.debug(String.format("IP address [%s] is not direct, dedicated or reserved; therefore, the resource count should be updated.", ip)); + return true; } @Override diff --git a/server/src/test/java/com/cloud/network/IpAddressManagerTest.java b/server/src/test/java/com/cloud/network/IpAddressManagerTest.java index 50ad62e0543..94974572267 100644 --- a/server/src/test/java/com/cloud/network/IpAddressManagerTest.java +++ b/server/src/test/java/com/cloud/network/IpAddressManagerTest.java @@ -65,6 +65,9 @@ public class IpAddressManagerTest { @Mock NetworkOfferingDao networkOfferingDao; + @Mock + IPAddressVO ipAddressVoMock; + @Spy @InjectMocks IpAddressManagerImpl ipAddressManager; @@ -230,4 +233,60 @@ public class IpAddressManagerTest { return network; } + private void prepareForCheckIfIpResourceCountShouldBeUpdatedTests() { + Mockito.when(ipAddressVoMock.getAssociatedWithNetworkId()).thenReturn(1L); + Mockito.when(ipAddressVoMock.getVpcId()).thenReturn(1L); + doReturn(false).when(ipAddressManager).isIpDedicated(Mockito.any()); + Mockito.when(ipAddressVoMock.getState()).thenReturn(IpAddress.State.Allocating); + } + + @Test + public void checkIfIpResourceCountShouldBeUpdatedTestIpIsDirectReturnFalse() { + prepareForCheckIfIpResourceCountShouldBeUpdatedTests(); + Mockito.when(ipAddressVoMock.getAssociatedWithNetworkId()).thenReturn(null); + Mockito.when(ipAddressVoMock.getVpcId()).thenReturn(null); + + boolean result = ipAddressManager.checkIfIpResourceCountShouldBeUpdated(ipAddressVoMock); + + Assert.assertFalse(result); + } + + @Test + public void checkIfIpResourceCountShouldBeUpdatedTestIpIsDedicatedReturnFalse() { + prepareForCheckIfIpResourceCountShouldBeUpdatedTests(); + doReturn(true).when(ipAddressManager).isIpDedicated(Mockito.any()); + + boolean result = ipAddressManager.checkIfIpResourceCountShouldBeUpdated(ipAddressVoMock); + + Assert.assertFalse(result); + } + + @Test + public void checkIfIpResourceCountShouldBeUpdatedTestIpIsReservedReturnFalse() { + prepareForCheckIfIpResourceCountShouldBeUpdatedTests(); + Mockito.when(ipAddressVoMock.getState()).thenReturn(IpAddress.State.Reserved); + + boolean result = ipAddressManager.checkIfIpResourceCountShouldBeUpdated(ipAddressVoMock); + + Assert.assertFalse(result); + } + + @Test + public void checkIfIpResourceCountShouldBeUpdatedTestIpIsAssociatedToNetworkAndNotDedicatedAndNotReservedReturnTrue() { + prepareForCheckIfIpResourceCountShouldBeUpdatedTests(); + + boolean result = ipAddressManager.checkIfIpResourceCountShouldBeUpdated(ipAddressVoMock); + + Assert.assertTrue(result); + } + + @Test + public void checkIfIpResourceCountShouldBeUpdatedTestIpIsAssociatedToVpcAndNotDedicatedAndNotReservedReturnTrue() { + prepareForCheckIfIpResourceCountShouldBeUpdatedTests(); + Mockito.when(ipAddressVoMock.getAssociatedWithNetworkId()).thenReturn(null); + + boolean result = ipAddressManager.checkIfIpResourceCountShouldBeUpdated(ipAddressVoMock); + + Assert.assertTrue(result); + } }