Skip to content

vpc: keep a public IP that other networks still use when its network goes #20

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

Open
calvix wants to merge 2 commits into
base: integration/all-fixes-4.23.0.0
Choose a base branch
from
Open

vpc: keep a public IP that other networks still use when its network goes #20

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
1e8aca8
vpc: keep a public IP that other networks still use when its network …
calvix Oct 29, 2026
4bc445e
vpc: re-bind a shared VPC IP to a network the router serves
calvix Oct 29, 2026
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
73 changes: 69 additions & 4 deletions ...tration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4214,8 +4214,8 @@ private boolean cleanupNetworkResources(final long networkId, final Account call
logger.warn("Failed to cleanup Network ACLs as a part of network {} cleanup due to resourceUnavailable ", network, ex);
}

//release all ip addresses
final List<IPAddressVO> ipsToRelease = _ipAddressDao.listByAssociatedNetwork(networkId, null);
//release all ip addresses, except the VPC IPs that other networks' rules still use
final List<IPAddressVO> ipsToRelease = rebindSharedVpcIpsAndListIpsToRelease(network);
for (final IPAddressVO ipToRelease : ipsToRelease) {
if (ipToRelease.getVpcId() == null) {
if (!ipToRelease.isPortable()) {
Expand Down Expand Up @@ -4247,6 +4247,70 @@ private boolean cleanupNetworkResources(final long networkId, final Account call
return success;
}

/**
* @return the IPs associated with the network that are to be released with it, once the VPC IPs that other
* networks' rules still use are bound to one of those networks
*/
protected List<IPAddressVO> rebindSharedVpcIpsAndListIpsToRelease(final Network network) {
rebindVpcIpsUsedByOtherNetworks(network);
return _ipAddressDao.listByAssociatedNetwork(network.getId(), null);
}

/**
* In a VPC with conserve mode, one public IP can carry rules of several networks while it is bound to one of
* them. When that network is shut down (deleted, garbage collected, restarted with cleanup or changing offering),
* such an IP is bound to another network whose rules it carries, so it stays on the router; releasing it would
* take the other networks' rules down with it. The IP stays with that network when this one comes back.
*
* @return the IPs that were bound to another network
*/
protected List<IPAddressVO> rebindVpcIpsUsedByOtherNetworks(final Network network) {
final List<IPAddressVO> reboundIps = new ArrayList<>();
if (network.getVpcId() == null) {
return reboundIps;
}
for (final IPAddressVO ip : _ipAddressDao.listByAssociatedNetwork(network.getId(), null)) {
if (ip.getVpcId() == null) {
continue;
}
final Long otherNetworkId = findOtherNetworkWithRulesOnIp(ip, network.getId());
if (otherNetworkId == null) {
continue;
}
ip.setAssociatedWithNetworkId(otherNetworkId);
_ipAddressDao.update(ip.getId(), ip);
reboundIps.add(ip);
logger.info("Bound VPC public IP {} to network with ID {}, whose rules it carries, as network {} goes away", ip, otherNetworkId, network);
}
return reboundIps;
}

/**
* @return the ID of a network, other than the given one, that has a rule on the IP that is not being revoked;
* null when there is none. An implemented network comes first, as the router only applies the IPs of the networks
* it is plugged into when it starts, then one whose rule is active rather than still being added.
*/
protected Long findOtherNetworkWithRulesOnIp(final IPAddressVO ip, final long networkId) {
Long otherNetworkId = null;
int otherNetworkRank = -1;
for (final FirewallRuleVO rule : _firewallDao.listByIp(ip.getId())) {
final Long ruleNetworkId = rule.getNetworkId();
if (rule.getState() == FirewallRule.State.Revoke || ruleNetworkId == null || ruleNetworkId == networkId) {
continue;
}
final NetworkVO ruleNetwork = _networksDao.findById(ruleNetworkId);
if (ruleNetwork == null) {
continue;
}
final int rank = (ruleNetwork.getState() == Network.State.Implemented ? 2 : 0) + (rule.getState() == FirewallRule.State.Active ? 1 : 0);
if (rank > otherNetworkRank) {
otherNetworkId = ruleNetworkId;
otherNetworkRank = rank;
}
}
return otherNetworkId;
}

private boolean shutdownNetworkResources(final Network network, final Account caller, final long callerUserId) {
// This method cleans up network rules on the backend w/o touching them in the DB
boolean success = true;
Expand Down Expand Up @@ -4392,8 +4456,9 @@ private boolean shutdownNetworkResources(final Network network, final Account ca
success = false;
}

// Get all ip addresses, mark as releasing and release them on the backend
final List<IPAddressVO> userIps = _ipAddressDao.listByAssociatedNetwork(network.getId(), null);
// Get all ip addresses, except the VPC IPs that other networks' rules still use, mark as releasing and release
// them on the backend
final List<IPAddressVO> userIps = rebindSharedVpcIpsAndListIpsToRelease(network);
final List<PublicIp> publicIpsToRelease = new ArrayList<>();
if (userIps != null && !userIps.isEmpty()) {
for (final IPAddressVO userIp : userIps) {
Expand Down
118 changes: 118 additions & 0 deletions ...ion/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,7 @@
import org.springframework.test.util.ReflectionTestUtils;
import org.mockito.ArgumentMatchers;
import org.mockito.MockedStatic;
import org.mockito.InOrder;
import org.mockito.Mockito;

import com.cloud.api.query.dao.DomainRouterJoinDao;
Expand All @@ -66,6 +67,7 @@
import com.cloud.network.NetworkModel;
import com.cloud.network.Networks.TrafficType;
import com.cloud.network.dao.IPAddressDao;
import com.cloud.network.dao.FirewallRulesDao;
import com.cloud.network.dao.IPAddressVO;
import com.cloud.network.dao.NetworkDao;
import com.cloud.network.dao.NetworkServiceMapDao;
Expand All @@ -76,6 +78,8 @@
import com.cloud.network.element.NetworkElement;
import com.cloud.network.guru.GuestNetworkGuru;
import com.cloud.network.guru.NetworkGuru;
import com.cloud.network.rules.FirewallRule;
import com.cloud.network.rules.FirewallRuleVO;
import com.cloud.network.vpc.VpcManager;
import com.cloud.network.vpc.VpcVO;
import com.cloud.offerings.NetworkOfferingVO;
Expand Down Expand Up @@ -133,6 +137,7 @@ public void setUp() {
testOrchestrator._ntwkSrvcDao = mock(NetworkServiceMapDao.class);
testOrchestrator._nicIpAliasDao = mock(NicIpAliasDao.class);
testOrchestrator._ipAddressDao = mock(IPAddressDao.class);
testOrchestrator._firewallDao = mock(FirewallRulesDao.class);
testOrchestrator._vlanDao = mock(VlanDao.class);
testOrchestrator._networkModel = mock(NetworkModel.class);
testOrchestrator._nicExtraDhcpOptionDao = mock(NicExtraDhcpOptionDao.class);
Expand Down Expand Up @@ -1073,4 +1078,117 @@ public void getNetworkElementsIncludingExtensionsReturnsBaseListWhenExtensionHel
assertNotNull(result);
assertEquals(1, result.size());
}

private IPAddressVO vpcIpOnNetwork(long ipId, long networkId, FirewallRuleVO... rules) {
IPAddressVO ip = mock(IPAddressVO.class);
when(ip.getId()).thenReturn(ipId);
when(ip.getVpcId()).thenReturn(9L);
when(testOrchestrator._firewallDao.listByIp(ipId)).thenReturn(List.of(rules));
return ip;
}

private FirewallRuleVO rule(Long networkId, FirewallRule.State state) {
FirewallRuleVO rule = mock(FirewallRuleVO.class);
when(rule.getNetworkId()).thenReturn(networkId);
when(rule.getState()).thenReturn(state);
return rule;
}

private Network vpcNetwork(long networkId) {
Network network = mock(Network.class);
when(network.getId()).thenReturn(networkId);
when(network.getVpcId()).thenReturn(9L);
return network;
}

@Test
public void vpcIpWithRulesOfAnotherNetworkIsBoundToThatNetwork() {
otherNetwork(2L, Network.State.Implemented);
IPAddressVO ip = vpcIpOnNetwork(10L, 1L, rule(1L, FirewallRule.State.Revoke), rule(2L, FirewallRule.State.Active));
when(testOrchestrator._ipAddressDao.listByAssociatedNetwork(1L, null)).thenReturn(List.of(ip));

List<IPAddressVO> rebound = testOrchestrator.rebindVpcIpsUsedByOtherNetworks(vpcNetwork(1L));

assertEquals(List.of(ip), rebound);
Mockito.verify(ip).setAssociatedWithNetworkId(2L);
Mockito.verify(testOrchestrator._ipAddressDao).update(10L, ip);
}

@Test
public void vpcIpWithOnlyRulesOfTheNetworkGoingAwayIsNotRebound() {
IPAddressVO ip = vpcIpOnNetwork(10L, 1L, rule(1L, FirewallRule.State.Active), rule(null, FirewallRule.State.Active));
when(testOrchestrator._ipAddressDao.listByAssociatedNetwork(1L, null)).thenReturn(List.of(ip));

Assert.assertTrue(testOrchestrator.rebindVpcIpsUsedByOtherNetworks(vpcNetwork(1L)).isEmpty());
Mockito.verify(ip, Mockito.never()).setAssociatedWithNetworkId(Mockito.any());
}

@Test
public void revokedRuleOfAnotherNetworkDoesNotKeepTheIp() {
IPAddressVO ip = vpcIpOnNetwork(10L, 1L, rule(2L, FirewallRule.State.Revoke));

Assert.assertNull(testOrchestrator.findOtherNetworkWithRulesOnIp(ip, 1L));
}

private void otherNetwork(long networkId, Network.State state) {
NetworkVO network = mock(NetworkVO.class);
when(network.getState()).thenReturn(state);
when(testOrchestrator._networksDao.findById(networkId)).thenReturn(network);
}

@Test
public void ipIsBoundToAnImplementedNetworkBeforeAnIdleOne() {
otherNetwork(2L, Network.State.Allocated);
otherNetwork(3L, Network.State.Implemented);
IPAddressVO ip = vpcIpOnNetwork(10L, 1L, rule(2L, FirewallRule.State.Active), rule(3L, FirewallRule.State.Active));

assertEquals(Long.valueOf(3L), testOrchestrator.findOtherNetworkWithRulesOnIp(ip, 1L));
}

@Test
public void ipIsBoundToANetworkWithAnActiveRuleBeforeOneWithARuleBeingAdded() {
otherNetwork(2L, Network.State.Implemented);
otherNetwork(3L, Network.State.Implemented);
IPAddressVO ip = vpcIpOnNetwork(10L, 1L, rule(2L, FirewallRule.State.Add), rule(3L, FirewallRule.State.Active));

assertEquals(Long.valueOf(3L), testOrchestrator.findOtherNetworkWithRulesOnIp(ip, 1L));
}

@Test
public void ipIsBoundToAnIdleNetworkWhenNoOtherUsesIt() {
otherNetwork(2L, Network.State.Allocated);
IPAddressVO ip = vpcIpOnNetwork(10L, 1L, rule(2L, FirewallRule.State.Active));

assertEquals(Long.valueOf(2L), testOrchestrator.findOtherNetworkWithRulesOnIp(ip, 1L));
}

@Test
public void ruleOfARemovedNetworkDoesNotKeepTheIp() {
IPAddressVO ip = vpcIpOnNetwork(10L, 1L, rule(2L, FirewallRule.State.Active));

Assert.assertNull(testOrchestrator.findOtherNetworkWithRulesOnIp(ip, 1L));
}

@Test
public void ipsToReleaseAreListedOnlyAfterTheSharedOnesAreRebound() {
otherNetwork(2L, Network.State.Implemented);
IPAddressVO shared = vpcIpOnNetwork(10L, 1L, rule(2L, FirewallRule.State.Active));
IPAddressVO own = vpcIpOnNetwork(11L, 1L, rule(1L, FirewallRule.State.Active));
when(testOrchestrator._ipAddressDao.listByAssociatedNetwork(1L, null)).thenReturn(List.of(shared, own), List.of(own));

assertEquals(List.of(own), testOrchestrator.rebindSharedVpcIpsAndListIpsToRelease(vpcNetwork(1L)));

InOrder inOrder = Mockito.inOrder(testOrchestrator._ipAddressDao);
inOrder.verify(testOrchestrator._ipAddressDao).update(10L, shared);
inOrder.verify(testOrchestrator._ipAddressDao).listByAssociatedNetwork(1L, null);
}

@Test
public void networkOutsideAVpcRebindsNothing() {
Network network = mock(Network.class);
when(network.getVpcId()).thenReturn(null);

Assert.assertTrue(testOrchestrator.rebindVpcIpsUsedByOtherNetworks(network).isEmpty());
Mockito.verify(testOrchestrator._ipAddressDao, Mockito.never()).listByAssociatedNetwork(Mockito.anyLong(), Mockito.any());
}
}