server: Fix allocation of more public IPs than the account's limit (#7832)

Author: winterhazel

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

@@ -991,7 +991,8 @@ public void doInTransactionWithoutResult(TransactionStatus status) {
                     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 void doInTransactionWithoutResult(TransactionStatus status) {
                                                 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 void doInTransactionWithoutResult(TransactionStatus status) {
         }
     }
 
-    private boolean isIpDedicated(IPAddressVO addr) {
+    protected boolean isIpDedicated(IPAddressVO addr) {
         List<AccountVlanMapVO> maps = _accountVlanMapDao.listAccountVlanMapsByVlan(addr.getVlanId());
         if (maps != null && !maps.isEmpty())
             return true;
@@ -1113,7 +1114,7 @@ public boolean applyIpAssociations(Network network, boolean continueOnError) thr
         // 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 IPAddressVO associateIPToGuestNetwork(long ipId, long networkId, boolean
 
         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 IPAddressVO associateIPToGuestNetwork(long ipId, long networkId, boolean
             } 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 IPAddressVO markIpAsUnavailable(final long addrId) {
             return Transaction.execute(new TransactionCallback<IPAddressVO>() {
                 @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 IPAddressVO doInTransaction(TransactionStatus status) {
         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

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 @@ private Network setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(long networkOffe
         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);
+    }
 }