diff --git a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java index a1b2294bcd4d..6ad43dda6bff 100644 --- a/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java +++ b/plugins/integrations/kubernetes-service/src/main/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorker.java @@ -658,6 +658,8 @@ protected void provisionVpcTierAllowPortACLRule(final Network network, int start networkACLRule.setAction(NetworkACLItem.Action.Allow.toString()); + networkACLRule.setReason(getVpcTierAclRuleReason()); + NetworkACLItem aclRule = networkACLService.createNetworkACLItem(networkACLRule); networkACLService.moveRuleToTheTopInACLList(aclRule); networkACLService.applyNetworkACL(aclRule.getAclId()); @@ -675,11 +677,41 @@ protected void removeVpcTierAllowPortACLRule(final Network network, int startPor networkACLItem.getAction().equals(NetworkACLItem.Action.Allow))) .collect(Collectors.toList()); - for (NetworkACLItemVO aclItem : aclItems) { + // Only remove the rules this cluster created, other clusters may use the same ACL list + List clusterAclItems = aclItems.stream() + .filter(networkACLItem -> getVpcTierAclRuleReason().equals(networkACLItem.getReason())) + .collect(Collectors.toList()); + // Record the cleanup before revoking, so a retried deletion knows this cluster's rule is already gone + String cleanupDetail = getVpcTierAclRuleCleanupDetail(startPort, endPort); + if (clusterAclItems.isEmpty() && kubernetesClusterDetailsDao.findDetail(kubernetesCluster.getId(), cleanupDetail) == null) { + // Rule created before rules were marked with the cluster, claim only one of them + NetworkACLItemVO legacyAclItem = aclItems.stream() + .filter(networkACLItem -> StringUtils.isEmpty(networkACLItem.getReason())) + .findFirst().orElse(null); + if (legacyAclItem != null) { + legacyAclItem.setReason(getVpcTierAclRuleReason()); + networkACLItemDao.update(legacyAclItem.getId(), legacyAclItem); + clusterAclItems = List.of(legacyAclItem); + } + } + if (clusterAclItems.isEmpty()) { + return; + } + kubernetesClusterDetailsDao.addDetail(kubernetesCluster.getId(), cleanupDetail, String.valueOf(true), false); + + for (NetworkACLItemVO aclItem : clusterAclItems) { networkACLService.revokeNetworkACLItem(aclItem.getId()); } } + protected String getVpcTierAclRuleReason() { + return String.format("Kubernetes cluster %s", kubernetesCluster.getUuid()); + } + + protected String getVpcTierAclRuleCleanupDetail(int startPort, int endPort) { + return String.format("vpcTierAclRuleRemoved-%d-%d", startPort, endPort); + } + protected void provisionLoadBalancerRule(final IpAddress publicIp, final Network network, final Account account, final List clusterVMIds, final int port) throws NetworkRuleConflictException, InsufficientAddressCapacityException { diff --git a/plugins/integrations/kubernetes-service/src/test/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorkerTest.java b/plugins/integrations/kubernetes-service/src/test/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorkerTest.java index c220a3468afb..b31948cd4d8d 100644 --- a/plugins/integrations/kubernetes-service/src/test/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorkerTest.java +++ b/plugins/integrations/kubernetes-service/src/test/java/com/cloud/kubernetes/cluster/actionworkers/KubernetesClusterResourceModifierActionWorkerTest.java @@ -18,11 +18,17 @@ package com.cloud.kubernetes.cluster.actionworkers; import com.cloud.kubernetes.cluster.KubernetesCluster; +import com.cloud.kubernetes.cluster.KubernetesClusterDetailsVO; import com.cloud.kubernetes.cluster.KubernetesClusterManagerImpl; import com.cloud.kubernetes.cluster.dao.KubernetesClusterDao; import com.cloud.kubernetes.cluster.dao.KubernetesClusterDetailsDao; import com.cloud.kubernetes.cluster.dao.KubernetesClusterVmMapDao; import com.cloud.kubernetes.version.dao.KubernetesSupportedVersionDao; +import com.cloud.network.Network; +import com.cloud.network.vpc.NetworkACLItem; +import com.cloud.network.vpc.NetworkACLItemDao; +import com.cloud.network.vpc.NetworkACLItemVO; +import com.cloud.network.vpc.NetworkACLService; import org.junit.Assert; import org.junit.Before; import org.junit.Test; @@ -31,6 +37,8 @@ import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import java.util.List; + @RunWith(MockitoJUnitRunner.class) public class KubernetesClusterResourceModifierActionWorkerTest { @Mock @@ -51,6 +59,15 @@ public class KubernetesClusterResourceModifierActionWorkerTest { @Mock private KubernetesCluster kubernetesClusterMock; + @Mock + private NetworkACLItemDao networkACLItemDaoMock; + + @Mock + private NetworkACLService networkACLServiceMock; + + @Mock + private Network networkMock; + private KubernetesClusterResourceModifierActionWorker kubernetesClusterResourceModifierActionWorker; @Before @@ -61,6 +78,94 @@ public void setUp() { kubernetesClusterManagerMock.kubernetesClusterVmMapDao = kubernetesClusterVmMapDaoMock; kubernetesClusterResourceModifierActionWorker = new KubernetesClusterResourceModifierActionWorker(kubernetesClusterMock, kubernetesClusterManagerMock); + kubernetesClusterResourceModifierActionWorker.networkACLItemDao = networkACLItemDaoMock; + kubernetesClusterResourceModifierActionWorker.networkACLService = networkACLServiceMock; + } + + private NetworkACLItemVO mockAclItem(long id, int port, String reason) { + NetworkACLItemVO aclItem = Mockito.mock(NetworkACLItemVO.class); + Mockito.lenient().when(aclItem.getId()).thenReturn(id); + Mockito.lenient().when(aclItem.getProtocol()).thenReturn("TCP"); + Mockito.lenient().when(aclItem.getSourcePortStart()).thenReturn(port); + Mockito.lenient().when(aclItem.getSourcePortEnd()).thenReturn(port); + Mockito.lenient().when(aclItem.getAction()).thenReturn(NetworkACLItem.Action.Allow); + Mockito.lenient().when(aclItem.getReason()).thenReturn(reason); + return aclItem; + } + + @Test + public void removeVpcTierAllowPortACLRuleTestRemovesOnlyThisClusterRules() throws Exception { + Mockito.when(kubernetesClusterMock.getUuid()).thenReturn("cluster-a"); + Mockito.when(networkMock.getNetworkACLId()).thenReturn(10L); + NetworkACLItemVO clusterARule = mockAclItem(1L, 6443, "Kubernetes cluster cluster-a"); + NetworkACLItemVO clusterBRule = mockAclItem(2L, 6443, "Kubernetes cluster cluster-b"); + NetworkACLItemVO oldRule = mockAclItem(3L, 6443, null); + Mockito.when(networkACLItemDaoMock.listByACL(10L)).thenReturn(List.of(clusterARule, clusterBRule, oldRule)); + + kubernetesClusterResourceModifierActionWorker.removeVpcTierAllowPortACLRule(networkMock, 6443, 6443); + + Mockito.verify(networkACLServiceMock).revokeNetworkACLItem(1L); + Mockito.verifyNoMoreInteractions(networkACLServiceMock); + } + + @Test + public void removeVpcTierAllowPortACLRuleTestRemovesOneUnmarkedRuleForOlderClusters() throws Exception { + Mockito.when(kubernetesClusterMock.getUuid()).thenReturn("cluster-a"); + Mockito.when(networkMock.getNetworkACLId()).thenReturn(10L); + NetworkACLItemVO clusterBRule = mockAclItem(2L, 22, "Kubernetes cluster cluster-b"); + NetworkACLItemVO oldRule1 = mockAclItem(3L, 22, null); + NetworkACLItemVO oldRule2 = mockAclItem(4L, 22, null); + Mockito.when(networkACLItemDaoMock.listByACL(10L)).thenReturn(List.of(clusterBRule, oldRule1, oldRule2)); + + kubernetesClusterResourceModifierActionWorker.removeVpcTierAllowPortACLRule(networkMock, 22, 22); + + Mockito.verify(oldRule1).setReason("Kubernetes cluster cluster-a"); + Mockito.verify(networkACLItemDaoMock).update(3L, oldRule1); + Mockito.verify(networkACLServiceMock).revokeNetworkACLItem(3L); + Mockito.verifyNoMoreInteractions(networkACLServiceMock); + } + + private void rememberCleanupDetails() { + Mockito.doAnswer(invocation -> { + Mockito.when(kubernetesClusterDetailsDaoMock.findDetail(1L, invocation.getArgument(1))) + .thenReturn(Mockito.mock(KubernetesClusterDetailsVO.class)); + return null; + }).when(kubernetesClusterDetailsDaoMock).addDetail(Mockito.eq(1L), Mockito.anyString(), Mockito.anyString(), Mockito.anyBoolean()); + } + + @Test + public void removeVpcTierAllowPortACLRuleTestRetryKeepsOtherClusterUnmarkedRule() throws Exception { + Mockito.when(kubernetesClusterMock.getId()).thenReturn(1L); + Mockito.when(kubernetesClusterMock.getUuid()).thenReturn("cluster-a"); + Mockito.when(networkMock.getNetworkACLId()).thenReturn(10L); + rememberCleanupDetails(); + NetworkACLItemVO clusterARule = mockAclItem(1L, 6443, "Kubernetes cluster cluster-a"); + NetworkACLItemVO clusterBOldRule = mockAclItem(2L, 6443, null); + Mockito.when(networkACLItemDaoMock.listByACL(10L)).thenReturn(List.of(clusterARule, clusterBOldRule), List.of(clusterBOldRule)); + + kubernetesClusterResourceModifierActionWorker.removeVpcTierAllowPortACLRule(networkMock, 6443, 6443); + kubernetesClusterResourceModifierActionWorker.removeVpcTierAllowPortACLRule(networkMock, 6443, 6443); + + Mockito.verify(networkACLServiceMock).revokeNetworkACLItem(1L); + Mockito.verify(networkACLServiceMock, Mockito.never()).revokeNetworkACLItem(2L); + Mockito.verify(networkACLItemDaoMock, Mockito.never()).update(Mockito.anyLong(), Mockito.any()); + } + + @Test + public void removeVpcTierAllowPortACLRuleTestRetryOfOlderClusterRemovesOnlyOneUnmarkedRule() throws Exception { + Mockito.when(kubernetesClusterMock.getId()).thenReturn(1L); + Mockito.when(kubernetesClusterMock.getUuid()).thenReturn("cluster-a"); + Mockito.when(networkMock.getNetworkACLId()).thenReturn(10L); + rememberCleanupDetails(); + NetworkACLItemVO oldRule1 = mockAclItem(3L, 22, null); + NetworkACLItemVO oldRule2 = mockAclItem(4L, 22, null); + Mockito.when(networkACLItemDaoMock.listByACL(10L)).thenReturn(List.of(oldRule1, oldRule2), List.of(oldRule2)); + + kubernetesClusterResourceModifierActionWorker.removeVpcTierAllowPortACLRule(networkMock, 22, 22); + kubernetesClusterResourceModifierActionWorker.removeVpcTierAllowPortACLRule(networkMock, 22, 22); + + Mockito.verify(networkACLServiceMock).revokeNetworkACLItem(3L); + Mockito.verify(networkACLServiceMock, Mockito.never()).revokeNetworkACLItem(4L); } @Test