extensions: ignore removed offerings when removing a network service

Author: DaanHoogland

framework/extensions/src/main/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImpl.java

@@ -140,8 +140,10 @@
 import com.cloud.network.dao.PhysicalNetworkDao;
 import com.cloud.network.dao.PhysicalNetworkVO;
 import com.cloud.network.vpc.Vpc;
+import com.cloud.network.vpc.dao.VpcOfferingDao;
 import com.cloud.network.vpc.dao.VpcOfferingServiceMapDao;
 import com.cloud.network.vpc.dao.VpcServiceMapDao;
+import com.cloud.offerings.dao.NetworkOfferingDao;
 import com.cloud.offerings.dao.NetworkOfferingServiceMapDao;
 import com.cloud.org.Cluster;
 import com.cloud.serializer.GsonHelper;
@@ -248,6 +250,12 @@ public class ExtensionsManagerImpl extends ManagerBase implements ExtensionsMana
     @Inject
     VpcOfferingServiceMapDao vpcOfferingServiceMapDao;
 
+    @Inject
+    NetworkOfferingDao networkOfferingDao;
+
+    @Inject
+    VpcOfferingDao vpcOfferingDao;
+
     @Inject
     NetworkModel networkModel;
 
@@ -1306,10 +1314,13 @@ protected void updateNetworkExtensionServicesOnPhysicalNetworks(ExtensionVO exte
             return;
         }
         for (Service removedService : removedServices) {
-            boolean usedByNetworkOffering = CollectionUtils.isNotEmpty(
-                    networkOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(removedService, extension.getName()));
-            boolean usedByVpcOffering = CollectionUtils.isNotEmpty(
-                    vpcOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(removedService, extension.getName()));
+            // offering service map rows are kept when an offering is deleted, so ignore removed offerings
+            boolean usedByNetworkOffering = networkOfferingServiceMapDao
+                    .listOfferingIdsByServiceAndProvider(removedService, extension.getName()).stream()
+                    .anyMatch(offeringId -> networkOfferingDao.findById(offeringId) != null);
+            boolean usedByVpcOffering = vpcOfferingServiceMapDao
+                    .listOfferingIdsByServiceAndProvider(removedService, extension.getName()).stream()
+                    .anyMatch(offeringId -> vpcOfferingDao.findById(offeringId) != null);
             if (usedByNetworkOffering || usedByVpcOffering) {
                 throw new CloudRuntimeException(String.format(
                         "Cannot remove service %s from extension '%s' as it is used by a network/VPC offering",

framework/extensions/src/test/java/org/apache/cloudstack/framework/extensions/manager/ExtensionsManagerImplTest.java

@@ -52,6 +52,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.UUID;
 
 import org.apache.cloudstack.acl.Role;
@@ -139,8 +140,12 @@
 import com.cloud.network.dao.PhysicalNetworkVO;
 import com.cloud.network.element.NetworkElement;
 import com.cloud.network.vpc.Vpc;
+import com.cloud.network.vpc.VpcOfferingVO;
+import com.cloud.network.vpc.dao.VpcOfferingDao;
 import com.cloud.network.vpc.dao.VpcOfferingServiceMapDao;
 import com.cloud.network.vpc.dao.VpcServiceMapDao;
+import com.cloud.offerings.NetworkOfferingVO;
+import com.cloud.offerings.dao.NetworkOfferingDao;
 import com.cloud.offerings.dao.NetworkOfferingServiceMapDao;
 import org.apache.cloudstack.extension.NetworkCustomActionProvider;
 import com.cloud.org.Cluster;
@@ -223,6 +228,12 @@ public class ExtensionsManagerImplTest {
     @Mock
     private VpcOfferingServiceMapDao vpcOfferingServiceMapDao;
 
+    @Mock
+    private NetworkOfferingDao networkOfferingDao;
+
+    @Mock
+    private VpcOfferingDao vpcOfferingDao;
+
     @Before
     public void setUp() {
         MockitoAnnotations.openMocks(this);
@@ -961,6 +972,7 @@ public void testUpdateExtension_RemovingUsedNetworkServiceThrows() {
 
         when(networkOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(Network.Service.StaticNat, "MyExt"))
                 .thenReturn(Collections.singletonList(1L));
+        when(networkOfferingDao.findById(1L)).thenReturn(mock(NetworkOfferingVO.class));
 
         extensionsManager.updateExtension(cmd);
     }
@@ -992,10 +1004,42 @@ public void testUpdateExtension_RemovingServiceUsedByVpcOfferingThrows() {
                 .thenReturn(Collections.emptyList());
         when(vpcOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(Network.Service.StaticNat, "MyExt"))
                 .thenReturn(Collections.singletonList(1L));
+        when(vpcOfferingDao.findById(1L)).thenReturn(mock(VpcOfferingVO.class));
 
         extensionsManager.updateExtension(cmd);
     }
 
+    @Test
+    public void updateNetworkExtensionServicesOnPhysicalNetworks_IgnoresRemovedOfferings() {
+        ExtensionVO ext = mock(ExtensionVO.class);
+        when(ext.getId()).thenReturn(9L);
+        when(ext.getName()).thenReturn("MyExt");
+
+        // service map rows still reference offerings 1 (network) and 2 (VPC), but both offerings are removed
+        when(networkOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(Network.Service.CustomAction, "MyExt"))
+                .thenReturn(Collections.singletonList(1L));
+        when(vpcOfferingServiceMapDao.listOfferingIdsByServiceAndProvider(Network.Service.CustomAction, "MyExt"))
+                .thenReturn(Collections.singletonList(2L));
+        when(networkOfferingDao.findById(1L)).thenReturn(null);
+        when(vpcOfferingDao.findById(2L)).thenReturn(null);
+
+        when(extensionResourceMapDao.listResourceIdsByExtensionIdAndType(9L, ExtensionResourceMap.ResourceType.PhysicalNetwork))
+                .thenReturn(Collections.singletonList(100L));
+        PhysicalNetworkServiceProviderVO nsp = mock(PhysicalNetworkServiceProviderVO.class);
+        when(nsp.getId()).thenReturn(500L);
+        when(nsp.getEnabledServices()).thenReturn(new ArrayList<>(Collections.singletonList(Network.Service.CustomAction)));
+        when(physicalNetworkServiceProviderDao.findByServiceProvider(100L, "MyExt")).thenReturn(nsp);
+
+        extensionsManager.updateNetworkExtensionServicesOnPhysicalNetworks(ext,
+                Set.of(Network.Service.CustomAction), Set.of(Network.Service.UserData));
+
+        ArgumentCaptor<List<Network.Service>> captor = ArgumentCaptor.forClass(List.class);
+        verify(nsp).setEnabledServices(captor.capture());
+        assertFalse(captor.getValue().contains(Network.Service.CustomAction));
+        assertTrue(captor.getValue().contains(Network.Service.UserData));
+        verify(physicalNetworkServiceProviderDao).update(500L, nsp);
+    }
+
     @Test
     public void testUpdateExtension_UpdatesPhysicalNetworkServicesWhenNotInUse() {
         UpdateExtensionCmd cmd = mock(UpdateExtensionCmd.class);