From 1e8aca8eeba0b010943a4f92e241fd6ce9065341 Mon Sep 17 00:00:00 2001 From: calvix <7136358+calvix@users.noreply.github.com> Date: Thu, 29 Oct 2026 11:34:03 +0100 Subject: [PATCH 1/2] vpc: keep a public IP that other networks still use when its network goes With conserve mode on a VPC offering, one public IP can carry port forwarding and load balancing rules of several networks of the VPC, but it stays bound to the network of its first rule. Shutting down or deleting that network marked the IP Releasing and released it on the router, so the other networks' rules on the IP stopped working at once (load balancing) or at the router's next rebuild (port forwarding), and new rules on the IP failed. Before releasing a VPC network's IPs, an IP that has a rule of another network which is not being revoked is now bound to that network and kept, both on network shutdown and on network delete. --- .../orchestration/NetworkOrchestrator.java | 47 +++++++++++++- .../NetworkOrchestratorTest.java | 63 +++++++++++++++++++ 2 files changed, 109 insertions(+), 1 deletion(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index 84a397349cec..cc019c396489 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java @@ -4214,7 +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 + //release all ip addresses, except the VPC IPs that other networks' rules still use + rebindVpcIpsUsedByOtherNetworks(network); final List ipsToRelease = _ipAddressDao.listByAssociatedNetwork(networkId, null); for (final IPAddressVO ipToRelease : ipsToRelease) { if (ipToRelease.getVpcId() == null) { @@ -4247,6 +4248,47 @@ private boolean cleanupNetworkResources(final long networkId, final Account call return success; } + /** + * 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 or deleted, 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. + * + * @return the IPs that were bound to another network + */ + protected List rebindVpcIpsUsedByOtherNetworks(final Network network) { + final List 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 + */ + protected Long findOtherNetworkWithRulesOnIp(final IPAddressVO ip, final long networkId) { + return _firewallDao.listByIp(ip.getId()).stream() + .filter(rule -> rule.getState() != FirewallRule.State.Revoke) + .map(FirewallRuleVO::getNetworkId) + .filter(ruleNetworkId -> ruleNetworkId != null && ruleNetworkId != networkId) + .findFirst() + .orElse(null); + } + 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; @@ -4392,6 +4434,9 @@ private boolean shutdownNetworkResources(final Network network, final Account ca success = false; } + // Keep the VPC IPs that other networks' rules still use on the router + rebindVpcIpsUsedByOtherNetworks(network); + // Get all ip addresses, mark as releasing and release them on the backend final List userIps = _ipAddressDao.listByAssociatedNetwork(network.getId(), null); final List publicIpsToRelease = new ArrayList<>(); diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java index 66f5b699cc46..c81bfe484aab 100644 --- a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java @@ -66,6 +66,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; @@ -76,6 +77,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; @@ -133,6 +136,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); @@ -1073,4 +1077,63 @@ 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() { + 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 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)); + } + + @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()); + } } From 4bc445e60d017f18d716bc02725cc6ab30bbf257 Mon Sep 17 00:00:00 2001 From: calvix <7136358+calvix@users.noreply.github.com> Date: Thu, 29 Oct 2026 15:30:00 +0100 Subject: [PATCH 2/2] vpc: re-bind a shared VPC IP to a network the router serves When a network goes, a VPC IP that other networks' rules use was bound to the first such network found. A router only plugs the IPs of the networks it serves when it starts, so an IP bound to a network that is not implemented was left off the router after a rebuild, and the rules of the networks that were implemented stopped working. An implemented network is now preferred, then one whose rule is active rather than still being added, and a removed network is never chosen. The IPs to release are listed in one place, after the re-bind, for both the shutdown and the cleanup of a network. A network is shut down not only when it is deleted, but also when it is garbage collected, restarted with cleanup or changes offering; the IP then stays with the network it was bound to. --- .../orchestration/NetworkOrchestrator.java | 52 ++++++++++++------ .../NetworkOrchestratorTest.java | 55 +++++++++++++++++++ 2 files changed, 91 insertions(+), 16 deletions(-) diff --git a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java index cc019c396489..d49a3a9292c6 100644 --- a/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java +++ b/engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java @@ -4215,8 +4215,7 @@ private boolean cleanupNetworkResources(final long networkId, final Account call } //release all ip addresses, except the VPC IPs that other networks' rules still use - rebindVpcIpsUsedByOtherNetworks(network); - final List ipsToRelease = _ipAddressDao.listByAssociatedNetwork(networkId, null); + final List ipsToRelease = rebindSharedVpcIpsAndListIpsToRelease(network); for (final IPAddressVO ipToRelease : ipsToRelease) { if (ipToRelease.getVpcId() == null) { if (!ipToRelease.isPortable()) { @@ -4248,10 +4247,20 @@ 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 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 or deleted, 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. + * 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 */ @@ -4278,15 +4287,28 @@ protected List rebindVpcIpsUsedByOtherNetworks(final Network networ /** * @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 + * 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) { - return _firewallDao.listByIp(ip.getId()).stream() - .filter(rule -> rule.getState() != FirewallRule.State.Revoke) - .map(FirewallRuleVO::getNetworkId) - .filter(ruleNetworkId -> ruleNetworkId != null && ruleNetworkId != networkId) - .findFirst() - .orElse(null); + 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) { @@ -4434,11 +4456,9 @@ private boolean shutdownNetworkResources(final Network network, final Account ca success = false; } - // Keep the VPC IPs that other networks' rules still use on the router - rebindVpcIpsUsedByOtherNetworks(network); - - // Get all ip addresses, mark as releasing and release them on the backend - final List 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 userIps = rebindSharedVpcIpsAndListIpsToRelease(network); final List publicIpsToRelease = new ArrayList<>(); if (userIps != null && !userIps.isEmpty()) { for (final IPAddressVO userIp : userIps) { diff --git a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java index c81bfe484aab..078760e00a3e 100644 --- a/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java +++ b/engine/orchestration/src/test/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestratorTest.java @@ -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; @@ -1102,6 +1103,7 @@ private Network vpcNetwork(long networkId) { @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)); @@ -1128,6 +1130,59 @@ public void revokedRuleOfAnotherNetworkDoesNotKeepTheIp() { 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);