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..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 @@ -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 ipsToRelease = _ipAddressDao.listByAssociatedNetwork(networkId, null); + //release all ip addresses, except the VPC IPs that other networks' rules still use + final List ipsToRelease = rebindSharedVpcIpsAndListIpsToRelease(network); for (final IPAddressVO ipToRelease : ipsToRelease) { if (ipToRelease.getVpcId() == null) { if (!ipToRelease.isPortable()) { @@ -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 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 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. 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; @@ -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 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 66f5b699cc46..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; @@ -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; @@ -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; @@ -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); @@ -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 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()); + } }