Make vpc.max.networks dynamic and return a clear error at the limit

Author: MitchDrage

engine/components-api/src/main/java/com/cloud/network/vpc/VpcManager.java

@@ -57,6 +57,13 @@ public interface VpcManager {
             true,
             ConfigKey.Scope.Global,
             null);
+    ConfigKey<Integer> VpcMaxNetworks = new ConfigKey<>("Advanced",
+            Integer.class,
+            "vpc.max.networks",
+            "3",
+            "Maximum number of networks per VPC",
+            true,
+            ConfigKey.Scope.Global);
 
     /**
      * Returns all the Guest networks that are part of VPC

server/src/main/java/com/cloud/configuration/Config.java

@@ -1496,7 +1496,6 @@ public enum Config {
             "3600",
             "The interval (in seconds) between cleanup for Inactive VPCs",
             null),
-    VpcMaxNetworks("Advanced", ManagementServer.class, Integer.class, "vpc.max.networks", "3", "Maximum number of networks per vpc", null),
     DetailBatchQuerySize("Advanced", ManagementServer.class, Integer.class, "detail.batch.query.size", "2000", "Default entity detail batch query size for listing", null),
     NetworkIPv6SearchRetryMax(
             "Network",

server/src/main/java/com/cloud/network/vpc/VpcManagerImpl.java

@@ -341,7 +341,6 @@ public class VpcManagerImpl extends ManagerBase implements VpcManager, VpcProvis
             Provider.JuniperContrailVpcRouter, Provider.Ovs, Provider.BigSwitchBcf, Provider.ConfigDrive, Provider.Nsx, Provider.Netris);
 
     int _cleanupInterval;
-    int _maxNetworks;
     SearchBuilder<IPAddressVO> IpAddressSearch;
 
     protected final List<HypervisorType> hTypes = new ArrayList<HypervisorType>();
@@ -517,9 +516,6 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
         final String value = configs.get(Config.VpcCleanupInterval.key());
         _cleanupInterval = NumbersUtil.parseInt(value, 60 * 60); // 1 hour
 
-        final String maxNtwks = configs.get(Config.VpcMaxNetworks.key());
-        _maxNetworks = NumbersUtil.parseInt(maxNtwks, 3); // max=3 is default
-
         IpAddressSearch = _ipAddressDao.createSearchBuilder();
         IpAddressSearch.and("accountId", IpAddressSearch.entity().getAllocatedToAccountId(), Op.EQ);
         IpAddressSearch.and("dataCenterId", IpAddressSearch.entity().getDataCenterId(), Op.EQ);
@@ -2580,9 +2576,12 @@ public void doInTransactionWithoutResult(final TransactionStatus status) {
 
                 try {
                     // check number of active networks in vpc
-                    if (_ntwkDao.countVpcNetworks(vpc.getId()) >= _maxNetworks) {
-                        logger.warn(String.format("Failed to create a new VPC Guest Network because the number of networks per VPC has reached its maximum capacity of [%s]. Increase it by modifying global config [%s].", _maxNetworks, Config.VpcMaxNetworks));
-                        throw new CloudRuntimeException(String.format("Number of networks per VPC cannot surpass [%s].", _maxNetworks));
+                    final int maxNetworks = VpcMaxNetworks.value();
+                    if (_ntwkDao.countVpcNetworks(vpc.getId()) >= maxNetworks) {
+                        logger.warn("Failed to create a new network in VPC [{}] because it has reached the maximum of [{}] networks for {}. "
+                                + "Increase it by modifying the global setting [{}].", vpc, maxNetworks, _accountMgr.getAccount(vpc.getAccountId()), VpcMaxNetworks.key());
+                        throw new InvalidParameterValueException(String.format("VPC %s has reached the maximum of %d networks. "
+                                + "Delete an unused network or contact your platform administrator to raise the limit.", vpc.getName(), maxNetworks));
                     }
 
                     // 1) CIDR is required
@@ -3721,7 +3720,8 @@ public String getConfigComponentName() {
     public ConfigKey<?>[] getConfigKeys() {
         return new ConfigKey<?>[]{
                 VpcTierNamePrepend,
-                VpcTierNamePrependDelimiter
+                VpcTierNamePrependDelimiter,
+                VpcMaxNetworks
         };
     }
 

server/src/test/java/com/cloud/network/vpc/VpcManagerImplTest.java

@@ -76,6 +76,7 @@
 import org.apache.cloudstack.extension.Extension;
 import org.apache.cloudstack.extension.ExtensionHelper;
 import org.apache.cloudstack.framework.config.ConfigKey;
+import org.apache.cloudstack.framework.config.impl.ConfigDepotImpl;
 import org.apache.cloudstack.network.Ipv4GuestSubnetNetworkMap;
 import org.apache.cloudstack.network.RoutedIpv4Manager;
 import org.junit.After;
@@ -362,7 +363,6 @@ protected Set<Network.Provider> prepareVpcManagerForCheckingCapabilityPerService
     @Test
     public void testCreateVpcNetwork() throws InsufficientCapacityException, ResourceAllocationException {
         final long VPC_ID = 201L;
-        manager._maxNetworks = 3;
         VpcVO vpcMockVO = Mockito.mock(VpcVO.class);
         Vpc vpcMock = Mockito.mock(Vpc.class);
         Account accountMock = Mockito.mock(Account.class);
@@ -400,6 +400,27 @@ public void testCreateVpcNetwork() throws InsufficientCapacityException, Resourc
                 null, null, null, null, null, new Pair<>(1000, 1000), null);
     }
 
+    @Test
+    public void validateNewVpcGuestNetworkTestLimitReadFromDynamicSetting() {
+        final long vpcId = 301L;
+        Vpc vpcMock = Mockito.mock(Vpc.class);
+        Mockito.when(vpcMock.getId()).thenReturn(vpcId);
+        Mockito.when(vpcDao.acquireInLockTable(vpcId)).thenReturn(Mockito.mock(VpcVO.class));
+        Mockito.when(networkDao.countVpcNetworks(vpcId)).thenReturn(2L);
+        ConfigDepotImpl configDepot = Mockito.mock(ConfigDepotImpl.class);
+        Mockito.when(configDepot.getConfigStringValue(VpcManager.VpcMaxNetworks.key(), ConfigKey.Scope.Global, null)).thenReturn("2");
+
+        Object originalDepot = ReflectionTestUtils.getField(VpcManager.VpcMaxNetworks, "s_depot");
+        ReflectionTestUtils.setField(VpcManager.VpcMaxNetworks, "s_depot", configDepot);
+        try {
+            InvalidParameterValueException e = Assert.assertThrows(InvalidParameterValueException.class,
+                    () -> manager.validateNewVpcGuestNetwork("10.10.10.0/24", "10.10.10.1", Mockito.mock(Account.class), vpcMock, "cs1cloud.internal"));
+            assertTrue(e.getMessage().contains("maximum of 2 networks"));
+        } finally {
+            ReflectionTestUtils.setField(VpcManager.VpcMaxNetworks, "s_depot", originalDepot);
+        }
+    }
+
     @Test
     public void testUpdateVpcNetwork() throws ResourceUnavailableException, InsufficientCapacityException {
         long vpcId = 1L;