extensions: ignore removed offerings when removing a network service

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 @@
     @Inject
     VpcOfferingServiceMapDao vpcOfferingServiceMapDao;
 
+    @Inject
+    NetworkOfferingDao networkOfferingDao;
+
+    @Inject
+    VpcOfferingDao vpcOfferingDao;
+
     @Inject
     NetworkModel networkModel;
 
@@ -1306,10 +1314,13 @@
             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 @@
     @Mock
     private VpcOfferingServiceMapDao vpcOfferingServiceMapDao;
 
+    @Mock
+    private NetworkOfferingDao networkOfferingDao;
+
+    @Mock
+    private VpcOfferingDao vpcOfferingDao;
+
     @Before
     public void setUp() {
         MockitoAnnotations.openMocks(this);
@@ -961,6 +972,7 @@
 
         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,43 @@
                 .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
+    @SuppressWarnings("unchecked")
+    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);

test/integration/smoke/test_network_extension_namespace.py

@@ -408,6 +408,8 @@ class TestNetworkExtensionNamespace(cloudstackTestCase):
       test_09 — VPC source NAT IP update without VPC restart
       test_10 — custom-action smoke for Policy-Based Routing (PBR) actions on VPC tier network
       test_11 — IPv6 dualstack isolated network: namespace IPv6 address + radvd
+      test_12 — service of a deleted network offering can be removed from the extension
+      test_13 — service of a deleted VPC offering can be removed from the extension
     """
 
     @staticmethod
@@ -1252,6 +1254,152 @@ def test_03_extension_enable_disable_and_delete_restriction(self):
         self.physical_network = None
         self.logger.info("test_03 PASSED")
 
+    def _register_extension_without_hosts(self, ext_name_prefix):
+        """Create a NetworkOrchestrator extension, register it to the guest
+        physical network without any hosts and enable its NSP.
+
+        Returns the extension (= provider) name.
+        """
+        self.physical_network = self._get_physical_network()
+        ext_name = "%s-%s" % (ext_name_prefix, random_gen())
+        self.extension = Extension.create(
+            self.apiclient,
+            name=ext_name,
+            type='NetworkOrchestrator',
+            details=[
+                {"network.services": NETWORK_SERVICES},
+                {"network.service.capabilities": NETWORK_SERVICE_CAPABILITIES_JSON},
+            ]
+        )
+        self.extension.register(self.apiclient, self.physical_network.id,
+                                'PhysicalNetwork')
+        provider = self._find_provider(self.physical_network.id, ext_name)
+        self.assertIsNotNone(provider,
+                             "NSP '%s' not found after registration" % ext_name)
+        self.provider_id = provider.id
+        if provider.state != 'Enabled':
+            self._update_provider_state(provider.id, 'Enabled')
+        return ext_name
+
+    def _count_service_map_rows(self, map_table, offering_column,
+                                offering_table, offering_uuid, provider):
+        """Count CustomAction rows in an offering service map for *provider*."""
+        rows = self.dbclient.execute(
+            "SELECT COUNT(*) FROM %s m JOIN %s o ON o.id = m.%s "
+            "WHERE o.uuid = '%s' AND m.service = 'CustomAction' "
+            "AND m.provider = '%s'"
+            % (map_table, offering_table, offering_column, offering_uuid,
+               provider))
+        return int(rows[0][0])
+
+    def _remove_custom_action_from_extension(self):
+        """Drop CustomAction from the extension's network.services detail."""
+        reduced_services = ",".join(
+            s for s in NETWORK_SERVICES.split(",") if s != "CustomAction")
+        self.extension.update(
+            self.apiclient,
+            details=[
+                {"network.services": reduced_services},
+                {"network.service.capabilities": NETWORK_SERVICE_CAPABILITIES_JSON},
+            ]
+        )
+
+    def _get_extension_services(self):
+        ext = Extension.list(self.apiclient, id=self.extension.id)[0]
+        d = (ext.details.__dict__
+             if not isinstance(ext.details, dict) else ext.details)
+        return d["network.services"].split(",")
+
+    def _get_provider_services(self, ext_name):
+        return self._find_provider(self.physical_network.id,
+                                   ext_name).servicelist
+
+    def _assert_custom_action_removable_after_offering_deleted(
+            self, ext_name, offering, map_table, offering_column,
+            offering_table):
+        """Removing CustomAction from the extension must be refused while
+        *offering* exists and allowed once it has been deleted, even though
+        deleting the offering leaves its service map rows behind."""
+        # While the offering exists the service is in use: refuse removal and
+        # leave both the extension detail and the NSP untouched.
+        with self.assertRaises(Exception) as ctx:
+            self._remove_custom_action_from_extension()
+        self.assertIn("Cannot remove service CustomAction", str(ctx.exception))
+        self.assertIn("CustomAction", self._get_extension_services())
+        self.assertIn("CustomAction", self._get_provider_services(ext_name))
+
+        offering.delete(self.apiclient)
+
+        # Deleting an offering only marks it removed; the service map rows that
+        # tie CustomAction to this provider are kept.
+        self.assertEqual(
+            1,
+            self._count_service_map_rows(map_table, offering_column,
+                                         offering_table, offering.id,
+                                         ext_name),
+            "Expected the %s row of the deleted offering to remain"
+            % map_table)
+
+        # The only offering using CustomAction is gone, so removal must succeed.
+        self._remove_custom_action_from_extension()
+        self.assertNotIn("CustomAction", self._get_extension_services())
+        self.assertNotIn("CustomAction", self._get_provider_services(ext_name))
+
+    @attr(tags=["advanced", "smoke"], required_hardware="false")
+    def test_12_remove_service_used_by_deleted_network_offering(self):
+        """A service whose only network offering was deleted can be removed
+        from the extension."""
+        self.dbclient = self.testClient.getDbConnection()
+        ext_name = self._register_extension_without_hosts("extnet-rmsvc-nw")
+        offering = None
+        try:
+            svc = ISOLATED_NETWORK_SERVICES
+            offering = NetworkOffering.create(self.apiclient, {
+                "name":              "ExtNet-RmSvc-%s" % random_gen(),
+                "displaytext":       "ExtNet remove service offering",
+                "guestiptype":       "Isolated",
+                "traffictype":       "GUEST",
+                "supportedservices": svc,
+                "serviceProviderList": {s: ext_name for s in svc.split(',')},
+                "serviceCapabilityList": {
+                    "SourceNat": {"SupportedSourceNatTypes": "peraccount"},
+                },
+            })
+            self._assert_custom_action_removable_after_offering_deleted(
+                ext_name, offering, "ntwk_offering_service_map",
+                "network_offering_id", "network_offerings")
+            offering = None
+        finally:
+            if offering:
+                offering.delete(self.apiclient)
+            self._teardown_extension()
+        self.logger.info("test_12 PASSED")
+
+    @attr(tags=["advanced", "smoke"], required_hardware="false")
+    def test_13_remove_service_used_by_deleted_vpc_offering(self):
+        """A service whose only VPC offering was deleted can be removed from
+        the extension."""
+        self.dbclient = self.testClient.getDbConnection()
+        ext_name = self._register_extension_without_hosts("extnet-rmsvc-vpc")
+        offering = None
+        try:
+            svc = VPC_NETWORK_SERVICES
+            offering = VpcOffering.create(self.apiclient, {
+                "name":              "ExtNet-RmSvc-VPC-%s" % random_gen(),
+                "displaytext":       "ExtNet remove service VPC offering",
+                "supportedservices": svc,
+                "serviceProviderList": {s: ext_name for s in svc.split(',')},
+            })
+            self._assert_custom_action_removable_after_offering_deleted(
+                ext_name, offering, "vpc_offering_service_map",
+                "vpc_offering_id", "vpc_offerings")
+            offering = None
+        finally:
+            if offering:
+                offering.delete(self.apiclient)
+            self._teardown_extension()
+        self.logger.info("test_13 PASSED")
+
     # ------------------------------------------------------------------
     # Tests — hardware required
     # ------------------------------------------------------------------