From f38b13579e902e923c4cf82ece6ecae62f13f26f Mon Sep 17 00:00:00 2001 From: Daman Arora Date: Fri, 16 Oct 2026 09:15:21 -0400 Subject: [PATCH 1/2] keep other clusters' acl rules when deleting a cks cluster on a vpc --- ...esClusterResourceModifierActionWorker.java | 20 ++++++- ...usterResourceModifierActionWorkerTest.java | 59 +++++++++++++++++++ 2 files changed, 78 insertions(+), 1 deletion(-) 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..ac290fa6f97d 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,27 @@ 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()); + if (clusterAclItems.isEmpty()) { + // Rules created before they were marked with the cluster, remove only one of them + clusterAclItems = aclItems.stream() + .filter(networkACLItem -> StringUtils.isEmpty(networkACLItem.getReason())) + .limit(1) + .collect(Collectors.toList()); + } + + for (NetworkACLItemVO aclItem : clusterAclItems) { networkACLService.revokeNetworkACLItem(aclItem.getId()); } } + protected String getVpcTierAclRuleReason() { + return String.format("Kubernetes cluster %s", kubernetesCluster.getUuid()); + } + 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..119ddbebf66f 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 @@ -23,6 +23,11 @@ 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 +36,8 @@ import org.mockito.Mockito; import org.mockito.junit.MockitoJUnitRunner; +import java.util.List; + @RunWith(MockitoJUnitRunner.class) public class KubernetesClusterResourceModifierActionWorkerTest { @Mock @@ -51,6 +58,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 +77,49 @@ 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(networkACLServiceMock).revokeNetworkACLItem(3L); + Mockito.verifyNoMoreInteractions(networkACLServiceMock); } @Test From 08df402a65e04aed4e8e1171cc0d4898506cebb3 Mon Sep 17 00:00:00 2001 From: Daman Arora Date: Wed, 28 Oct 2026 09:33:15 -0400 Subject: [PATCH 2/2] keep another cluster's old acl rule when a cks delete is retried --- ...esClusterResourceModifierActionWorker.java | 24 ++++++++-- ...usterResourceModifierActionWorkerTest.java | 46 +++++++++++++++++++ 2 files changed, 65 insertions(+), 5 deletions(-) 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 ac290fa6f97d..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 @@ -681,13 +681,23 @@ protected void removeVpcTierAllowPortACLRule(final Network network, int startPor List clusterAclItems = aclItems.stream() .filter(networkACLItem -> getVpcTierAclRuleReason().equals(networkACLItem.getReason())) .collect(Collectors.toList()); - if (clusterAclItems.isEmpty()) { - // Rules created before they were marked with the cluster, remove only one of them - clusterAclItems = aclItems.stream() + // 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())) - .limit(1) - .collect(Collectors.toList()); + .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()); @@ -698,6 +708,10 @@ 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 119ddbebf66f..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,6 +18,7 @@ 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; @@ -118,10 +119,55 @@ public void removeVpcTierAllowPortACLRuleTestRemovesOneUnmarkedRuleForOlderClust 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 public void getKubernetesClusterNodeNamePrefixTestReturnOriginalPrefixWhenNamingAllRequirementsAreMet() { String originalPrefix = "k8s-cluster-01";