IP address acquired with associate ip address is marked as source nat (apache#3125)

* CLOUDSTACK-4045 added a check for network state when determining whether a new IP should be source NAT. this prevents associated IP's to be marked as source NAT when the network is in allocated state, causing disassociateIpAddress to fail later

* Remove mock object that cause other tests to fail

* Remove underscores from variable types and add documentation for the created method

* Improve exception message to include network name

* Include network UUID with the Exception message and fix failing marvin test

* Rebase against latest master and format AssociateIPAddrCmd class

Author: Dingane Hlaluku

api/src/main/java/org/apache/cloudstack/api/command/user/address/AssociateIPAddrCmd.java

@@ -57,8 +57,12 @@
 import com.cloud.projects.Project;
 import com.cloud.user.Account;
 
-@APICommand(name = "associateIpAddress", description = "Acquires and associates a public IP to an account. Either of the parameters are required, i.e. either zoneId, or networkId, or vpcId  ", responseObject = IPAddressResponse.class, responseView = ResponseView.Restricted,
-        requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+@APICommand(name = "associateIpAddress",
+        description = "Acquires and associates a public IP to an account. Either of the parameters are required, i.e. either zoneId, or networkId, or vpcId  ",
+        responseObject = IPAddressResponse.class,
+        responseView = ResponseView.Restricted,
+        requestHasSensitiveInfo = false,
+        responseHasSensitiveInfo = false)
 public class AssociateIPAddrCmd extends BaseAsyncCreateCmd {
     public static final Logger s_logger = Logger.getLogger(AssociateIPAddrCmd.class.getName());
     private static final String s_name = "associateipaddressresponse";
@@ -67,46 +71,57 @@ public class AssociateIPAddrCmd extends BaseAsyncCreateCmd {
     //////////////// API parameters /////////////////////
     /////////////////////////////////////////////////////
 
-    @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "the account to associate with this IP address")
+    @Parameter(name = ApiConstants.ACCOUNT,
+            type = CommandType.STRING,
+            description = "the account to associate with this IP address")
     private String accountName;
 
     @Parameter(name = ApiConstants.DOMAIN_ID,
-               type = CommandType.UUID,
-               entityType = DomainResponse.class,
-               description = "the ID of the domain to associate with this IP address")
+            type = CommandType.UUID,
+            entityType = DomainResponse.class,
+            description = "the ID of the domain to associate with this IP address")
     private Long domainId;
 
     @Parameter(name = ApiConstants.ZONE_ID,
-               type = CommandType.UUID,
-               entityType = ZoneResponse.class,
-               description = "the ID of the availability zone you want to acquire an public IP address from")
+            type = CommandType.UUID,
+            entityType = ZoneResponse.class,
+            description = "the ID of the availability zone you want to acquire an public IP address from")
     private Long zoneId;
 
     @Parameter(name = ApiConstants.NETWORK_ID,
-               type = CommandType.UUID,
-               entityType = NetworkResponse.class,
-               description = "The network this IP address should be associated to.")
+            type = CommandType.UUID,
+            entityType = NetworkResponse.class,
+            description = "The network this IP address should be associated to.")
     private Long networkId;
 
-    @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "Deploy VM for the project")
+    @Parameter(name = ApiConstants.PROJECT_ID,
+            type = CommandType.UUID,
+            entityType = ProjectResponse.class,
+            description = "Deploy VM for the project")
     private Long projectId;
 
-    @Parameter(name = ApiConstants.VPC_ID, type = CommandType.UUID, entityType = VpcResponse.class, description = "the VPC you want the IP address to "
-        + "be associated with")
+    @Parameter(name = ApiConstants.VPC_ID,
+            type = CommandType.UUID,
+            entityType = VpcResponse.class,
+            description = "the VPC you want the IP address to be associated with")
     private Long vpcId;
 
-    @Parameter(name = ApiConstants.IS_PORTABLE, type = BaseCmd.CommandType.BOOLEAN, description = "should be set to true "
-        + "if public IP is required to be transferable across zones, if not specified defaults to false")
+    @Parameter(name = ApiConstants.IS_PORTABLE,
+            type = BaseCmd.CommandType.BOOLEAN,
+            description = "should be set to true if public IP is required to be transferable across zones, if not specified defaults to false")
     private Boolean isPortable;
 
     @Parameter(name = ApiConstants.REGION_ID,
-               type = CommandType.INTEGER,
-               entityType = RegionResponse.class,
-               required = false,
-               description = "region ID from where portable IP is to be associated.")
+            type = CommandType.INTEGER,
+            entityType = RegionResponse.class,
+            required = false,
+            description = "region ID from where portable IP is to be associated.")
     private Integer regionId;
 
-    @Parameter(name = ApiConstants.FOR_DISPLAY, type = CommandType.BOOLEAN, description = "an optional field, whether to the display the IP to the end user or not", since = "4.4", authorized = {RoleType.Admin})
+    @Parameter(name = ApiConstants.FOR_DISPLAY,
+            type = CommandType.BOOLEAN,
+            description = "an optional field, whether to the display the IP to the end user or not", since = "4.4",
+            authorized = {RoleType.Admin})
     private Boolean display;
 
     /////////////////////////////////////////////////////
@@ -178,7 +193,7 @@ public Long getNetworkId() {
             if (networks.size() == 0) {
                 String domain = _domainService.getDomain(getDomainId()).getName();
                 throw new InvalidParameterValueException("Account name=" + getAccountName() + " domain=" + domain + " doesn't have virtual networks in zone=" +
-                    zone.getName());
+                        zone.getName());
             }
 
             if (networks.size() < 1) {
@@ -205,7 +220,7 @@ public Boolean getDisplayIp() {
 
     @Override
     public boolean isDisplay() {
-        if(display == null)
+        if (display == null)
             return true;
         else
             return display;
@@ -224,7 +239,7 @@ public long getEntityOwnerId() {
                     return project.getProjectAccountId();
                 } else {
                     throw new PermissionDeniedException("Can't add resources to the project with specified projectId in state=" + project.getState() +
-                        " as it's no longer active");
+                            " as it's no longer active");
                 }
             } else {
                 throw new InvalidParameterValueException("Unable to find project by ID");
@@ -268,7 +283,7 @@ public String getEventType() {
 
     @Override
     public String getEventDescription() {
-        return  "associating IP to network ID: " + getNetworkId() + " in zone " + getZoneId();
+        return "associating IP to network ID: " + getNetworkId() + " in zone " + getZoneId();
     }
 
     /////////////////////////////////////////////////////

server/src/main/java/com/cloud/network/IpAddressManagerImpl.java

@@ -1377,16 +1377,7 @@ public IPAddressVO associateIPToGuestNetwork(long ipId, long networkId, boolean
             }
         }
 
-        NetworkOffering offering = _networkOfferingDao.findById(network.getNetworkOfferingId());
-        boolean sharedSourceNat = offering.isSharedSourceNat();
-        boolean isSourceNat = false;
-        if (!sharedSourceNat) {
-            if (getExistingSourceNatInNetwork(owner.getId(), networkId) == null) {
-                if (network.getGuestType() == GuestType.Isolated && network.getVpcId() == null && !ipToAssoc.isPortable()) {
-                    isSourceNat = true;
-                }
-            }
-        }
+        boolean isSourceNat = isSourceNatAvailableForNetwork(owner, ipToAssoc, network);
 
         s_logger.debug("Associating ip " + ipToAssoc + " to network " + network);
 
@@ -1424,6 +1415,33 @@ public IPAddressVO associateIPToGuestNetwork(long ipId, long networkId, boolean
         }
     }
 
+    /**
+     * Prevents associating an IP address to an allocated (unimplemented network) network, throws an Exception otherwise
+     * @param owner Used to check if the user belongs to the Network
+     * @param ipToAssoc IP address to be associated to a Network, can only be associated to an implemented network for Source NAT
+     * @param network Network to which IP address is to be associated with, must not be in allocated state for Source NAT Network/IP association
+     * @return true if IP address can be successfully associated with Source NAT network
+     */
+    protected boolean isSourceNatAvailableForNetwork(Account owner, IPAddressVO ipToAssoc, Network network) {
+        NetworkOffering offering = _networkOfferingDao.findById(network.getNetworkOfferingId());
+        boolean sharedSourceNat = offering.isSharedSourceNat();
+        boolean isSourceNat = false;
+        if (!sharedSourceNat) {
+            if (getExistingSourceNatInNetwork(owner.getId(), network.getId()) == null) {
+                if (network.getGuestType() == GuestType.Isolated && network.getVpcId() == null && !ipToAssoc.isPortable()) {
+                    if (network.getState() == Network.State.Allocated) {
+                        //prevent associating an ip address to an allocated (unimplemented network).
+                        //it will cause the ip to become source nat, and it can't be disassociated later on.
+                        String msg = String.format("Network with UUID:%s is in allocated and needs to be implemented first before acquiring an IP address", network.getUuid());
+                        throw new InvalidParameterValueException(msg);
+                    }
+                    isSourceNat = true;
+                }
+            }
+        }
+        return isSourceNat;
+    }
+
     protected boolean isSharedNetworkOfferingWithServices(long networkOfferingId) {
         NetworkOfferingVO networkOffering = _networkOfferingDao.findById(networkOfferingId);
         if ((networkOffering.getGuestType() == Network.GuestType.Shared)

server/src/test/java/com/cloud/network/IpAddressManagerTest.java

@@ -17,7 +17,10 @@
 
 package com.cloud.network;
 
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Matchers.anyLong;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
 import static org.mockito.Mockito.when;
 
@@ -28,32 +31,63 @@
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
 import org.mockito.Mockito;
-import org.mockito.MockitoAnnotations;
+import org.mockito.Spy;
 
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.ResourceUnavailableException;
 import com.cloud.network.Network.Service;
 import com.cloud.network.dao.IPAddressDao;
 import com.cloud.network.dao.IPAddressVO;
+import com.cloud.network.dao.NetworkDao;
+import com.cloud.network.dao.NetworkVO;
 import com.cloud.network.rules.StaticNat;
 import com.cloud.network.rules.StaticNatImpl;
+import com.cloud.offerings.NetworkOfferingVO;
+import com.cloud.offerings.dao.NetworkOfferingDao;
+import com.cloud.user.AccountVO;
 import com.cloud.utils.net.Ip;
+import org.mockito.runners.MockitoJUnitRunner;
 
+@RunWith(MockitoJUnitRunner.class)
 public class IpAddressManagerTest {
 
     @Mock
-    IPAddressDao _ipAddrDao;
+    IPAddressDao ipAddressDao;
 
+    @Mock
+    NetworkDao networkDao;
+
+    @Mock
+    NetworkOfferingDao networkOfferingDao;
+
+    @Spy
     @InjectMocks
-    IpAddressManagerImpl _ipManager;
+    IpAddressManagerImpl ipAddressManager;
 
     @InjectMocks
     NetworkModelImpl networkModel = Mockito.spy(new NetworkModelImpl());
 
+    IPAddressVO ipAddressVO;
+
+    AccountVO account;
+
     @Before
-    public void setup() {
-        MockitoAnnotations.initMocks(this);
+    public void setup() throws ResourceUnavailableException {
+
+        ipAddressVO = new IPAddressVO(new Ip("192.0.0.1"), 1L, 1L, 1L,false);
+        ipAddressVO.setAllocatedToAccountId(1L);
+
+        account = new AccountVO("admin", 1L, null, (short) 1, 1L, "c65a73d5-ebbd-11e7-8f45-107b44277808");
+        account.setId(1L);
+
+        NetworkOfferingVO networkOfferingVO = Mockito.mock(NetworkOfferingVO.class);
+        networkOfferingVO.setSharedSourceNat(false);
+
+        Mockito.when(networkOfferingDao.findById(Mockito.anyLong())).thenReturn(networkOfferingVO);
     }
 
     @Test
@@ -63,10 +97,10 @@ public void testGetStaticNatSourceIps() {
         when(vo.getAddress()).thenReturn(new Ip(publicIpAddress));
         when(vo.getId()).thenReturn(1l);
 
-        when(_ipAddrDao.findById(anyLong())).thenReturn(vo);
+        when(ipAddressDao.findById(anyLong())).thenReturn(vo);
         StaticNat snat = new StaticNatImpl(1, 1, 1, 1, publicIpAddress, false);
 
-        List<IPAddressVO> ips = _ipManager.getStaticNatSourceIps(Collections.singletonList(snat));
+        List<IPAddressVO> ips = ipAddressManager.getStaticNatSourceIps(Collections.singletonList(snat));
         Assert.assertNotNull(ips);
         Assert.assertEquals(1, ips.size());
 
@@ -78,7 +112,7 @@ public void testGetStaticNatSourceIps() {
     public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestRequestedIpEqualsIp6Gateway() {
         Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", null, new ArrayList<Service>());
 
-        boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "ip6Gateway");
+        boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "ip6Gateway");
 
         Mockito.verify(networkModel, Mockito.times(0)).listNetworkOfferingServices(Mockito.anyLong());
         Assert.assertTrue(result);
@@ -88,7 +122,7 @@ public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestRequestedIpEqualsIp6Gate
     public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestRequestedIpEqualsGateway() {
         Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", null, new ArrayList<Service>());
 
-        boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "gateway");
+        boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "gateway");
 
         Mockito.verify(networkModel, Mockito.times(0)).listNetworkOfferingServices(Mockito.anyLong());
         Assert.assertTrue(result);
@@ -101,7 +135,7 @@ public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestExpectFalseServicesNotEm
         services.add(serviceGateway);
         Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", null, services);
 
-        boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
+        boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
 
         Mockito.verify(networkModel).listNetworkOfferingServices(Mockito.anyLong());
         Assert.assertFalse(result);
@@ -111,17 +145,75 @@ public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestExpectFalseServicesNotEm
     public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestExpectFalseServicesCidrNotNull() {
         Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", "cidr", new ArrayList<Service>());
 
-        boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
+        boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
 
         Mockito.verify(networkModel).listNetworkOfferingServices(Mockito.anyLong());
         Assert.assertFalse(result);
     }
 
+    @Test
+    public void assertSourceNatImplementedNetwork() {
+
+        NetworkVO networkImplemented = Mockito.mock(NetworkVO.class);
+        when(networkImplemented.getTrafficType()).thenReturn(Networks.TrafficType.Guest);
+        when(networkImplemented.getNetworkOfferingId()).thenReturn(8L);
+        when(networkImplemented.getState()).thenReturn(Network.State.Implemented);
+        when(networkImplemented.getGuestType()).thenReturn(Network.GuestType.Isolated);
+        when(networkImplemented.getVpcId()).thenReturn(null);
+        when(networkImplemented.getId()).thenReturn(1L);
+
+        Mockito.when(networkDao.findById(1L)).thenReturn(networkImplemented);
+        doReturn(null).when(ipAddressManager).getExistingSourceNatInNetwork(1L, 1L);
+
+        boolean isSourceNat = ipAddressManager.isSourceNatAvailableForNetwork(account, ipAddressVO, networkImplemented);
+
+        assertTrue("Source NAT should be true", isSourceNat);
+    }
+
+    @Test(expected = InvalidParameterValueException.class)
+    public void assertSourceNatAllocatedNetwork() {
+
+        NetworkVO networkAllocated = Mockito.mock(NetworkVO.class);
+        when(networkAllocated.getTrafficType()).thenReturn(Networks.TrafficType.Guest);
+        when(networkAllocated.getNetworkOfferingId()).thenReturn(8L);
+        when(networkAllocated.getState()).thenReturn(Network.State.Allocated);
+        when(networkAllocated.getGuestType()).thenReturn(Network.GuestType.Isolated);
+        when(networkAllocated.getVpcId()).thenReturn(null);
+        when(networkAllocated.getId()).thenReturn(2L);
+
+        Mockito.when(networkDao.findById(2L)).thenReturn(networkAllocated);
+        doReturn(null).when(ipAddressManager).getExistingSourceNatInNetwork(1L, 2L);
+
+        ipAddressManager.isSourceNatAvailableForNetwork(account, ipAddressVO, networkAllocated);
+    }
+
+    @Test
+    public void assertExistingSourceNatAllocatedNetwork() {
+
+        NetworkVO networkNat = Mockito.mock(NetworkVO.class);
+        when(networkNat.getTrafficType()).thenReturn(Networks.TrafficType.Guest);
+        when(networkNat.getNetworkOfferingId()).thenReturn(8L);
+        when(networkNat.getState()).thenReturn(Network.State.Implemented);
+        when(networkNat.getGuestType()).thenReturn(Network.GuestType.Isolated);
+        when(networkNat.getId()).thenReturn(3L);
+        when(networkNat.getVpcId()).thenReturn(null);
+        when(networkNat.getId()).thenReturn(3L);
+
+        IPAddressVO sourceNat = new IPAddressVO(new Ip("192.0.0.2"), 1L, 1L, 1L,true);
+
+        Mockito.when(networkDao.findById(3L)).thenReturn(networkNat);
+        doReturn(sourceNat).when(ipAddressManager).getExistingSourceNatInNetwork(1L, 3L);
+
+        boolean isSourceNat = ipAddressManager.isSourceNatAvailableForNetwork(account, ipAddressVO, networkNat);
+
+        assertFalse("Source NAT should be false", isSourceNat);
+    }
+
     @Test
     public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestNetworkOfferingsEmptyAndCidrNull() {
         Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", null, new ArrayList<Service>());
 
-        boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
+        boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
 
         Mockito.verify(networkModel).listNetworkOfferingServices(Mockito.anyLong());
         Assert.assertTrue(result);