addNicToVirtualMachine: Fixes #2540 handle invalid MAC address arg (#2653)

Look for the next available MAC address if the given MAC address in command addNicToVirtualMachine is invalid (null, empty, blank). Fixes #2540

server/src/com/cloud/vm/UserVmManagerImpl.java

@@ -1169,6 +1169,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
             }
         }
 
+        macAddress = validateOrReplaceMacAddress(macAddress, network.getId());
+
         if(_nicDao.findByNetworkIdAndMacAddress(networkId, macAddress) != null) {
             throw new CloudRuntimeException("A NIC with this MAC address exists for network: " + network.getUuid());
         }
@@ -1239,6 +1241,19 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir
         return _vmDao.findById(vmInstance.getId());
     }
 
+    /**
+     * If the given MAC address is invalid it replaces the given MAC with the next available MAC address
+     */
+    protected String validateOrReplaceMacAddress(String macAddress, long networkId) {
+        if (!NetUtils.isValidMac(macAddress)) {
+            try {
+                macAddress = _networkModel.getNextAvailableMacAddressInNetwork(networkId);
+            } catch (InsufficientAddressCapacityException e) {
+                throw new CloudRuntimeException(String.format("A MAC address cannot be generated for this NIC in the network [id=%s] ", networkId));
+            }
+        }
+        return macAddress;
+    }
 
     private void saveExtraDhcpOptions(long nicId, Map<Integer, String> dhcpOptions) {
         List<NicExtraDhcpOptionVO> nicExtraDhcpOptionVOList = dhcpOptions

server/test/com/cloud/vm/UserVmManagerImplTest.java

@@ -22,6 +22,7 @@ import java.util.HashMap;
 import org.apache.cloudstack.api.BaseCmd.HTTPMethod;
 import org.apache.cloudstack.api.command.user.vm.UpdateVMCmd;
 import org.apache.cloudstack.context.CallContext;
+import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
@@ -34,9 +35,11 @@ import org.powermock.api.mockito.PowerMockito;
 import org.powermock.core.classloader.annotations.PrepareForTest;
 import org.powermock.modules.junit4.PowerMockRunner;
 
+import com.cloud.exception.InsufficientAddressCapacityException;
 import com.cloud.exception.InsufficientCapacityException;
 import com.cloud.exception.InvalidParameterValueException;
 import com.cloud.exception.ResourceUnavailableException;
+import com.cloud.network.NetworkModel;
 import com.cloud.storage.GuestOSVO;
 import com.cloud.storage.dao.GuestOSDao;
 import com.cloud.user.Account;
@@ -70,6 +73,9 @@ public class UserVmManagerImplTest {
     @Mock
     private UserVmVO userVmVoMock;
 
+    @Mock
+    private NetworkModel networkModel;
+
     private long vmId = 1l;
 
     @Before
@@ -221,4 +227,53 @@ public class UserVmManagerImplTest {
                 Mockito.anyString(), Mockito.anyBoolean(), Mockito.any(HTTPMethod.class), Mockito.anyString(), Mockito.anyString(), Mockito.anyString(), Mockito.anyListOf(Long.class),
                 Mockito.anyMap());
     }
+
+    @Test
+    public void validateOrReplaceMacAddressTestMacAddressValid() throws InsufficientAddressCapacityException {
+        configureValidateOrReplaceMacAddressTest(0, "01:23:45:67:89:ab", "01:23:45:67:89:ab");
+    }
+
+    @Test
+    public void validateOrReplaceMacAddressTestMacAddressNull() throws InsufficientAddressCapacityException {
+        configureValidateOrReplaceMacAddressTest(1, null, "01:23:45:67:89:ab");
+    }
+
+    @Test
+    public void validateOrReplaceMacAddressTestMacAddressBlank() throws InsufficientAddressCapacityException {
+        configureValidateOrReplaceMacAddressTest(1, " ", "01:23:45:67:89:ab");
+    }
+
+    @Test
+    public void validateOrReplaceMacAddressTestMacAddressEmpty() throws InsufficientAddressCapacityException {
+        configureValidateOrReplaceMacAddressTest(1, "", "01:23:45:67:89:ab");
+    }
+
+    @Test
+    public void validateOrReplaceMacAddressTestMacAddressNotValidOption1() throws InsufficientAddressCapacityException {
+        configureValidateOrReplaceMacAddressTest(1, "abcdef:gh:ij:kl", "01:23:45:67:89:ab");
+    }
+
+    @Test
+    public void validateOrReplaceMacAddressTestMacAddressNotValidOption2() throws InsufficientAddressCapacityException {
+        configureValidateOrReplaceMacAddressTest(1, "01:23:45:67:89:", "01:23:45:67:89:ab");
+    }
+
+    @Test
+    public void validateOrReplaceMacAddressTestMacAddressNotValidOption3() throws InsufficientAddressCapacityException {
+        configureValidateOrReplaceMacAddressTest(1, "01:23:45:67:89:az", "01:23:45:67:89:ab");
+    }
+
+    @Test
+    public void validateOrReplaceMacAddressTestMacAddressNotValidOption4() throws InsufficientAddressCapacityException {
+        configureValidateOrReplaceMacAddressTest(1, "@1:23:45:67:89:ab", "01:23:45:67:89:ab");
+    }
+
+    private void configureValidateOrReplaceMacAddressTest(int times, String macAddress, String expectedMacAddress) throws InsufficientAddressCapacityException {
+        Mockito.when(networkModel.getNextAvailableMacAddressInNetwork(Mockito.anyLong())).thenReturn(expectedMacAddress);
+
+        String returnedMacAddress = userVmManagerImpl.validateOrReplaceMacAddress(macAddress, 1l);
+
+        Mockito.verify(networkModel, Mockito.times(times)).getNextAvailableMacAddressInNetwork(Mockito.anyLong());
+        Assert.assertEquals(expectedMacAddress, returnedMacAddress);
+    }
 }