From 6ea054cb01b1be93cc5a7d54cd559a600e7d7224 Mon Sep 17 00:00:00 2001 From: Pearl Dsilva Date: Mon, 2 Feb 2026 13:16:02 -0500 Subject: [PATCH] add some checks to prevent networkmode change when provider is nsx/netris from the source networkmode --- .../ConfigurationManagerImpl.java | 86 ++++++++++-------- .../com/cloud/network/vpc/VpcManagerImpl.java | 28 ++++-- .../ConfigurationManagerImplTest.java | 89 +++++++++++++++++++ 3 files changed, 163 insertions(+), 40 deletions(-) diff --git a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java index b47b12aa1a5..d9244378b71 100644 --- a/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java +++ b/server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java @@ -8298,44 +8298,61 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati logger.info("Cloning network offering {} (id: {}) to new offering with name: {}", sourceOffering.getName(), sourceOfferingId, name); - String detectedProvider = cmd.getProvider(); - - if (detectedProvider == null || detectedProvider.isEmpty()) { - Map> sourceServiceProviderMap = + Map> sourceServiceProviderMap = _networkModel.getNetworkOfferingServiceProvidersMap(sourceOfferingId); - if (sourceServiceProviderMap.containsKey(Network.Service.NetworkACL)) { - Set networkAclProviders = sourceServiceProviderMap.get(Network.Service.NetworkACL); - if (networkAclProviders != null && !networkAclProviders.isEmpty()) { - Network.Provider provider = networkAclProviders.iterator().next(); - if (provider == Network.Provider.Nsx) { - detectedProvider = "NSX"; - } else if (provider == Network.Provider.Netris) { - detectedProvider = "Netris"; - } - } - } - } + validateProvider(sourceOffering, sourceServiceProviderMap, cmd.getProvider(), cmd.getNetworkMode()); - // If this is an NSX/Netris offering, prevent network mode changes - if (detectedProvider != null && (detectedProvider.equals("NSX") || detectedProvider.equals("Netris"))) { - String cmdNetworkMode = cmd.getNetworkMode(); - if (cmdNetworkMode != null && sourceOffering.getNetworkMode() != null) { - if (!cmdNetworkMode.equalsIgnoreCase(sourceOffering.getNetworkMode().toString())) { - throw new InvalidParameterValueException( - String.format("Cannot change network mode when cloning %s provider network offerings. " + - "Source offering has network mode '%s', but '%s' was specified. " + - "The network mode is determined by the provider configuration and cannot be modified.", - detectedProvider, sourceOffering.getNetworkMode(), cmdNetworkMode)); - } - } - } - - applySourceOfferingValuesToCloneCmd(cmd, sourceOffering); + applySourceOfferingValuesToCloneCmd(cmd, sourceServiceProviderMap, sourceOffering); return createNetworkOffering(cmd); } + private void validateProvider(NetworkOfferingVO sourceOffering, + Map> sourceServiceProviderMap, + String detectedProvider, String networkMode) { + + detectedProvider = getExternalNetworkProvider(detectedProvider, sourceServiceProviderMap); + // If this is an NSX/Netris offering, prevent network mode changes + if (detectedProvider != null && (detectedProvider.equals("NSX") || detectedProvider.equals("Netris"))) { + if (networkMode != null && sourceOffering.getNetworkMode() != null) { + if (!networkMode.equalsIgnoreCase(sourceOffering.getNetworkMode().toString())) { + throw new InvalidParameterValueException( + String.format("Cannot change network mode when cloning %s provider network offerings. " + + "Source offering has network mode '%s', but '%s' was specified. ", + detectedProvider, sourceOffering.getNetworkMode(), networkMode)); + } + } + } + } + + public static String getExternalNetworkProvider(String detectedProvider, + Map> sourceServiceProviderMap) { + if (StringUtils.isNotEmpty(detectedProvider)) { + return detectedProvider; + } + + if (sourceServiceProviderMap == null || sourceServiceProviderMap.isEmpty()) { + return null; + } + + for (Set providers : sourceServiceProviderMap.values()) { + if (CollectionUtils.isEmpty(providers)) { + continue; + } + for (Provider provider : providers) { + if (provider == Provider.Nsx) { + return "NSX"; + } + if (provider == Provider.Netris) { + return "Netris"; + } + } + } + + return null; + } + /** * Converts service provider map from internal format to API parameter format. * @@ -8364,12 +8381,11 @@ public class ConfigurationManagerImpl extends ManagerBase implements Configurati return apiFormatMap; } - private void applySourceOfferingValuesToCloneCmd(CloneNetworkOfferingCmd cmd, NetworkOfferingVO sourceOffering) { + private void applySourceOfferingValuesToCloneCmd(CloneNetworkOfferingCmd cmd, + Map> sourceServiceProviderMap, + NetworkOfferingVO sourceOffering) { Long sourceOfferingId = sourceOffering.getId(); - Map> sourceServiceProviderMap = - _networkModel.getNetworkOfferingServiceProvidersMap(sourceOfferingId); - // Build final services list with add/drop support List finalServices = resolveFinalServicesList(cmd, sourceServiceProviderMap); diff --git a/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java b/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java index 2ec1a0983e4..fabf1b9e4a5 100644 --- a/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java +++ b/server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java @@ -835,21 +835,39 @@ public class VpcManagerImpl extends ManagerBase implements VpcManager, VpcProvis VpcOfferingVO vpcOfferingVO = _vpcOffDao.findByUniqueName(name); if (vpcOfferingVO != null) { throw new InvalidParameterValueException(String.format("A VPC offering with name %s already exists", name)); - } logger.info("Cloning VPC offering {} (id: {}) to new offering with name: {}", sourceVpcOffering.getName(), sourceVpcOfferingId, name); - applySourceOfferingValuesToCloneCmd(cmd, sourceVpcOffering); + Map> sourceServiceProviderMap = getVpcOffSvcProvidersMap(sourceVpcOfferingId); + validateProvider(sourceVpcOffering, sourceServiceProviderMap, cmd.getProvider(), cmd.getNetworkMode()); + + applySourceOfferingValuesToCloneCmd(cmd, sourceServiceProviderMap, sourceVpcOffering); return createVpcOffering(cmd); } - private void applySourceOfferingValuesToCloneCmd(CloneVPCOfferingCmd cmd, VpcOffering sourceVpcOffering) { - Long sourceOfferingId = sourceVpcOffering.getId(); + private void validateProvider(VpcOffering sourceVpcOffering, + Map> sourceServiceProviderMap, + String provider, String networkMode) { + provider = ConfigurationManagerImpl.getExternalNetworkProvider(provider, sourceServiceProviderMap); + if (provider != null && (provider.equals("NSX") || provider.equals("Netris"))) { + if (networkMode != null && sourceVpcOffering.getNetworkMode() != null) { + if (!networkMode.equalsIgnoreCase(sourceVpcOffering.getNetworkMode().toString())) { + throw new InvalidParameterValueException( + String.format("Cannot change network mode when cloning %s provider VPC offerings. " + + "Source offering has network mode '%s', but '%s' was specified. ", + provider, sourceVpcOffering.getNetworkMode(), networkMode)); + } + } + } + } - Map> sourceServiceProviderMap = getVpcOffSvcProvidersMap(sourceOfferingId); + private void applySourceOfferingValuesToCloneCmd(CloneVPCOfferingCmd cmd, + Map> sourceServiceProviderMap, + VpcOffering sourceVpcOffering) { + Long sourceOfferingId = sourceVpcOffering.getId(); List finalServices = resolveFinalServicesList(cmd, sourceServiceProviderMap); diff --git a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java b/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java index 524d112ca37..286d4d04fa6 100644 --- a/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java +++ b/server/src/test/java/com/cloud/configuration/ConfigurationManagerImplTest.java @@ -90,11 +90,15 @@ import org.mockito.junit.MockitoJUnitRunner; import org.springframework.test.util.ReflectionTestUtils; import java.lang.reflect.Field; +import java.lang.reflect.InvocationTargetException; +import java.lang.reflect.Method; import java.util.ArrayList; import java.util.Collections; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import static org.mockito.ArgumentMatchers.any; import static org.mockito.ArgumentMatchers.anyLong; @@ -1292,4 +1296,89 @@ public class ConfigurationManagerImplTest { Assert.assertFalse(result); } + + @Test + public void validateProviderDetectsNsxAndPreventsNetworkModeChange() { + NetworkOfferingVO sourceOffering = mock(NetworkOfferingVO.class); + when(sourceOffering.getNetworkMode()).thenReturn(NetworkOffering.NetworkMode.NATTED); + + Map> serviceProviderMap = new HashMap<>(); + Set providers = new HashSet<>(); + providers.add(Network.Provider.Nsx); + serviceProviderMap.put(Network.Service.Firewall, providers); + try { + Method method = null; + try { + method = configurationManagerImplSpy.getClass().getDeclaredMethod("validateProvider", NetworkOfferingVO.class, Map.class, String.class, String.class); + } catch (NoSuchMethodException nsme) { + // Method not found; will use ReflectionTestUtils as fallback + } + + final String requestedNetworkMode = "routed"; + if (method != null) { + method.setAccessible(true); + try { + method.invoke(configurationManagerImplSpy, sourceOffering, serviceProviderMap, null, requestedNetworkMode); + Assert.fail("Expected InvalidParameterValueException to be thrown"); + } catch (InvocationTargetException ite) { + Throwable cause = ite.getCause(); + if (cause instanceof InvalidParameterValueException) { + return; + } + cause.printStackTrace(System.out); + Assert.fail("Unexpected exception type: " + cause); + } + } + } catch (Exception e) { + e.printStackTrace(System.out); + Assert.fail("Test encountered unexpected exception: " + e); + } + } + + @Test + public void testGetExternalNetworkProviderReturnsDetectedProviderWhenNonEmpty() { + String detected = "CustomProvider"; + Map> serviceProviderMap = new HashMap<>(); + + String result = ConfigurationManagerImpl.getExternalNetworkProvider(detected, serviceProviderMap); + + Assert.assertEquals(detected, result); + } + + @Test + public void testGetExternalNetworkProviderDetectsNsxFromAnyService() { + Map> serviceProviderMap = new HashMap<>(); + Set providers = new HashSet<>(); + providers.add(Network.Provider.Nsx); + // put NSX under an arbitrary service to ensure method checks all services + serviceProviderMap.put(Network.Service.Dhcp, providers); + + String result = ConfigurationManagerImpl.getExternalNetworkProvider(null, serviceProviderMap); + + Assert.assertEquals("NSX", result); + } + + @Test + public void testGetExternalNetworkProviderDetectsNetrisFromAnyService() { + Map> serviceProviderMap = new HashMap<>(); + Set providers = new HashSet<>(); + providers.add(Network.Provider.Netris); + serviceProviderMap.put(Network.Service.StaticNat, providers); + + String result = ConfigurationManagerImpl.getExternalNetworkProvider(null, serviceProviderMap); + + Assert.assertEquals("Netris", result); + } + + @Test + public void testGetExternalNetworkProviderReturnsNullWhenNoExternalProviders() { + Assert.assertNull(ConfigurationManagerImpl.getExternalNetworkProvider(null, null)); + + Map> emptyMap = new HashMap<>(); + Assert.assertNull(ConfigurationManagerImpl.getExternalNetworkProvider(null, emptyMap)); + + Map> mapWithEmptySet = new HashMap<>(); + mapWithEmptySet.put(Network.Service.Firewall, Collections.emptySet()); + Assert.assertNull(ConfigurationManagerImpl.getExternalNetworkProvider(null, mapWithEmptySet)); + } }