Skip to content

Fix allocation of more public IPs than the account's limit #7832

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
weizhouapache merged 1 commit into from
Aug 14, 2023
Merged

Fix allocation of more public IPs than the account's limit #7832

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
37 changes: 27 additions & 10 deletions server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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);
}
}
Expand All @@ -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;
Expand Down Expand Up @@ -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) {
Expand Down Expand Up @@ -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);
Expand All @@ -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) {
Expand Down Expand Up @@ -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);
}

Expand All @@ -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
Expand Down
59 changes: 59 additions & 0 deletions server/src/test/java/com/cloud/network/IpAddressManagerTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,9 @@ public class IpAddressManagerTest {
@Mock
NetworkOfferingDao networkOfferingDao;

@Mock
IPAddressVO ipAddressVoMock;

@Spy
@InjectMocks
IpAddressManagerImpl ipAddressManager;
Expand Down Expand Up @@ -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);
}
}