Fix for Vlan doesn't match issue while adding IP range for the shared network without any IP range

Author: sureshanaparti

api/src/main/java/org/apache/cloudstack/api/command/admin/vlan/CreateVlanIpRangeCmd.java

@@ -155,9 +155,6 @@ public String getStartIp() {
     }
 
     public String getVlan() {
-        if (vlan == null || vlan.isEmpty()) {
-            vlan = "untagged";
-        }
         return vlan;
     }
 

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

@@ -4407,14 +4407,35 @@ public Vlan createVlanAndPublicIpRange(final CreateVlanIpRangeCmd cmd) throws In
         String endIP = cmd.getEndIp();
         final String newVlanGateway = cmd.getGateway();
         final String newVlanNetmask = cmd.getNetmask();
+        Long networkId = cmd.getNetworkID();
+        Long physicalNetworkId = cmd.getPhysicalNetworkId();
+
+        // Verify that network exists
+        Network network = null;
+        if (networkId != null) {
+            network = _networkDao.findById(networkId);
+            if (network == null) {
+                throw new InvalidParameterValueException("Unable to find network by id " + networkId);
+            } else {
+                zoneId = network.getDataCenterId();
+                physicalNetworkId = network.getPhysicalNetworkId();
+            }
+        }
+
         String vlanId = cmd.getVlan();
+        if (StringUtils.isBlank(vlanId)) {
+            vlanId = Vlan.UNTAGGED;
+            if (network != null & network.getTrafficType() == TrafficType.Guest) {
+                boolean connectivityWithoutVlan = isConnectivityWithoutVlan(network);
+                vlanId = getNetworkVlanId(network, connectivityWithoutVlan);
+            }
+        }
+
         // TODO decide if we should be forgiving or demand a valid and complete URI
         if (!(vlanId == null || "".equals(vlanId) || vlanId.startsWith(BroadcastDomainType.Vlan.scheme()))) {
             vlanId = BroadcastDomainType.Vlan.toUri(vlanId).toString();
         }
         final Boolean forVirtualNetwork = cmd.isForVirtualNetwork();
-        Long networkId = cmd.getNetworkID();
-        Long physicalNetworkId = cmd.getPhysicalNetworkId();
         final String accountName = cmd.getAccountName();
         final Long projectId = cmd.getProjectId();
         final Long domainId = cmd.getDomainId();
@@ -4487,18 +4508,6 @@ public Vlan createVlanAndPublicIpRange(final CreateVlanIpRangeCmd cmd) throws In
             }
         }
 
-        // Verify that network exists
-        Network network = null;
-        if (networkId != null) {
-            network = _networkDao.findById(networkId);
-            if (network == null) {
-                throw new InvalidParameterValueException("Unable to find network by id " + networkId);
-            } else {
-                zoneId = network.getDataCenterId();
-                physicalNetworkId = network.getPhysicalNetworkId();
-            }
-        }
-
         // Verify that zone exists
         final DataCenterVO zone = _zoneDao.findById(zoneId);
         if (zone == null) {
@@ -4847,28 +4856,8 @@ public Vlan createVlanAndPublicIpRange(final long zoneId, final long networkId,
         // same as network's vlan
         // 2) if vlan is missing, default it to the guest network's vlan
         if (network.getTrafficType() == TrafficType.Guest) {
-            String networkVlanId = null;
-            boolean connectivityWithoutVlan = false;
-            if (_networkModel.areServicesSupportedInNetwork(network.getId(), Service.Connectivity)) {
-                Map<Capability, String> connectivityCapabilities = _networkModel.getNetworkServiceCapabilities(network.getId(), Service.Connectivity);
-                connectivityWithoutVlan = MapUtils.isNotEmpty(connectivityCapabilities) && connectivityCapabilities.containsKey(Capability.NoVlan);
-            }
-
-            final URI uri = network.getBroadcastUri();
-            if (connectivityWithoutVlan) {
-                networkVlanId = network.getBroadcastDomainType().toUri(network.getUuid()).toString();
-            } else if (uri != null) {
-                // Do not search for the VLAN tag when the network doesn't support VLAN
-               if (uri.toString().startsWith("vlan")) {
-                    final String[] vlan = uri.toString().split("vlan:\\/\\/");
-                    networkVlanId = vlan[1];
-                    // For pvlan
-                    if (network.getBroadcastDomainType() != BroadcastDomainType.Vlan) {
-                        networkVlanId = networkVlanId.split("-")[0];
-                    }
-               }
-            }
-
+            boolean connectivityWithoutVlan = isConnectivityWithoutVlan(network);
+            String networkVlanId = getNetworkVlanId(network, connectivityWithoutVlan);
             if (vlanId != null && !connectivityWithoutVlan) {
                 // if vlan is specified, throw an error if it's not equal to
                 // network's vlanId
@@ -5000,6 +4989,36 @@ public Vlan createVlanAndPublicIpRange(final long zoneId, final long networkId,
         return vlan;
     }
 
+    private boolean isConnectivityWithoutVlan(Network network) {
+        boolean connectivityWithoutVlan = false;
+        if (_networkModel.areServicesSupportedInNetwork(network.getId(), Service.Connectivity)) {
+            Map<Capability, String> connectivityCapabilities = _networkModel.getNetworkServiceCapabilities(network.getId(), Service.Connectivity);
+            connectivityWithoutVlan = MapUtils.isNotEmpty(connectivityCapabilities) && connectivityCapabilities.containsKey(Capability.NoVlan);
+        }
+        return connectivityWithoutVlan;
+    }
+
+    private String getNetworkVlanId(Network network, boolean connectivityWithoutVlan) {
+        String networkVlanId = null;
+        if (connectivityWithoutVlan) {
+            return network.getBroadcastDomainType().toUri(network.getUuid()).toString();
+        }
+
+        final URI uri = network.getBroadcastUri();
+        if (uri != null) {
+            // Do not search for the VLAN tag when the network doesn't support VLAN
+            if (uri.toString().startsWith("vlan")) {
+                final String[] vlan = uri.toString().split("vlan:\\/\\/");
+                networkVlanId = vlan[1];
+                // For pvlan
+                if (network.getBroadcastDomainType() != BroadcastDomainType.Vlan) {
+                    networkVlanId = networkVlanId.split("-")[0];
+                }
+            }
+        }
+        return networkVlanId;
+    }
+
     private void checkZoneVlanIpOverlap(DataCenterVO zone, Network network, String newCidr, String vlanId, String vlanGateway, String vlanNetmask, String startIP, String endIP) {
         // Throw an exception if this subnet overlaps with subnet on other VLAN,
         // if this is ip range extension, gateway, network mask should be same and ip range should not overlap

server/src/test/java/com/cloud/configuration/ConfigurationManagerTest.java

@@ -54,6 +54,8 @@
 import com.cloud.network.dao.IPAddressDao;
 import com.cloud.network.dao.IPAddressVO;
 import com.cloud.network.dao.Ipv6GuestPrefixSubnetNetworkMapDao;
+import com.cloud.network.dao.NetworkDao;
+import com.cloud.network.dao.NetworkVO;
 import com.cloud.network.dao.PhysicalNetworkDao;
 import com.cloud.network.dao.PhysicalNetworkVO;
 import com.cloud.offering.DiskOffering;
@@ -189,6 +191,8 @@ public class ConfigurationManagerTest {
     @Mock
     HostPodDao _podDao;
     @Mock
+    NetworkDao _networkDao;
+    @Mock
     PhysicalNetworkDao _physicalNetworkDao;
     @Mock
     ImageStoreDao _imageStoreDao;
@@ -1325,6 +1329,8 @@ public void testDisabledConfigCreateIpv6NetworkOffering() {
     public void testWrongIpv6CreateVlanAndPublicIpRange() {
         CreateVlanIpRangeCmd cmd = Mockito.mock(CreateVlanIpRangeCmd.class);
         Mockito.when(cmd.getIp6Cidr()).thenReturn("fd17:5:8a43:e2a4:c000::/66");
+        NetworkVO network = Mockito.mock(NetworkVO.class);
+        Mockito.when(_networkDao.findById(Mockito.anyLong())).thenReturn(network);
         try {
             configurationMgr.createVlanAndPublicIpRange(cmd);
         } catch (InsufficientCapacityException | ResourceUnavailableException | ResourceAllocationException e) {