CLOUDSTACK-4045 IP address acquired with associateIpAddress is marked as source NAT

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

@@ -1378,16 +1378,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);
 
@@ -1425,6 +1416,25 @@ public IPAddressVO associateIPToGuestNetwork(long ipId, long networkId, boolean
         }
     }
 
+    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.
+                        throw new InvalidParameterValueException("Network is in allocated state, implement network first before acquiring an IP address");
+                    }
+                    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;
 
@@ -32,28 +35,62 @@
 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;
 
 public class IpAddressManagerTest {
 
     @Mock
     IPAddressDao _ipAddrDao;
 
+    @Mock
+    NetworkDao _networkDao;
+
+    @Mock
+    NetworkOfferingDao _networkOfferingDao;
+
+    @Mock
+    NetworkModel _networkModel;
+
+    @Spy
     @InjectMocks
     IpAddressManagerImpl _ipManager;
 
     @InjectMocks
     NetworkModelImpl networkModel = Mockito.spy(new NetworkModelImpl());
 
+    IPAddressVO ipAddressVO;
+
+    AccountVO account;
+
     @Before
-    public void setup() {
+    public void setup() throws ResourceUnavailableException {
         MockitoAnnotations.initMocks(this);
+
+        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);
+        when(_networkModel.areServicesSupportedInNetwork(0L, Network.Service.SourceNat)).thenReturn(true);
     }
 
     @Test
@@ -117,6 +154,64 @@ public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestExpectFalseServicesCidrN
         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(_ipManager).getExistingSourceNatInNetwork(1L, 1L);
+
+        boolean isSourceNat = _ipManager.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(_ipManager).getExistingSourceNatInNetwork(1L, 2L);
+
+        _ipManager.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(_ipManager).getExistingSourceNatInNetwork(1L, 3L);
+
+        boolean isSourceNat = _ipManager.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>());

test/integration/component/test_tags.py

@@ -2516,6 +2516,21 @@ def test_24_public_ip_tag(self):
             domainid=self.child_do_admin.domainid,
             zoneid=self.zone.id
         )
+        tag = "tag1"
+        self.so_with_tag = ServiceOffering.create(
+            self.apiclient,
+            self.services["service_offering"],
+            hosttags=tag
+        )
+        self.vm = VirtualMachine.create(
+            self.api_client,
+            self.services["virtual_machine"],
+            accountid=self.child_do_admin.name,
+            domainid=self.child_do_admin.domainid,
+            networkids=self.network.id,
+            serviceofferingid=self.so_with_tag.id
+        )
+
         self.debug("Fetching the network details for account: %s" %
                    self.child_do_admin.name
         )

test/integration/smoke/test_network.py

@@ -110,6 +110,42 @@ def setUpClass(cls):
             cls.user.domainid
         )
 
+        cls.service_offering = ServiceOffering.create(
+            cls.apiclient,
+            cls.services["service_offerings"]["tiny"],
+        )
+
+        cls.hypervisor = testClient.getHypervisorInfo()
+        cls.template = get_test_template(
+            cls.apiclient,
+            cls.zone.id,
+            cls.hypervisor
+        )
+        if cls.template == FAILED:
+            assert False, "get_test_template() failed to return template"
+
+        cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+
+        cls.account_vm = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["virtual_machine"],
+            templateid=cls.template.id,
+            accountid=cls.account.name,
+            domainid=cls.account.domainid,
+            networkids=cls.account_network.id,
+            serviceofferingid=cls.service_offering.id
+        )
+
+        cls.user_vm = VirtualMachine.create(
+            cls.apiclient,
+            cls.services["virtual_machine"],
+            templateid=cls.template.id,
+            accountid=cls.user.name,
+            domainid=cls.user.domainid,
+            networkids=cls.user_network.id,
+            serviceofferingid=cls.service_offering.id
+        )
+
         # Create Source NAT IP addresses
         PublicIPAddress.create(
             cls.apiclient,
@@ -124,6 +160,8 @@ def setUpClass(cls):
             cls.user.domainid
         )
         cls._cleanup = [
+            cls.account_vm,
+            cls.user_vm,
             cls.account_network,
             cls.user_network,
             cls.account,

test/integration/smoke/test_portforwardingrules.py

@@ -16,6 +16,7 @@
 # under the License.
 
 # Import Local Modules
+from marvin.codes import (FAILED)
 from marvin.cloudstackTestCase import cloudstackTestCase, unittest
 from marvin.lib.base import (PublicIPAddress,
                              NetworkOffering,
@@ -43,6 +44,7 @@
 from marvin.codes import PASS
 from nose.plugins.attrib import attr
 
+
 class TestPortForwardingRules(cloudstackTestCase):
 
     @classmethod
@@ -121,6 +123,7 @@ def tearDownClass(cls):
             cleanup_resources(cls.api_client, cls._cleanup)
         except Exception as e:
             raise Exception("Warning: Exception during cleanup : %s" % e)
+        return
 
     def __verify_values(self, expected_vals, actual_vals):
         """
@@ -153,8 +156,6 @@ def __verify_values(self, expected_vals, actual_vals):
                     (exp_val, act_val))
         return return_flag
 
-
-
     @attr(tags=["advanced"], required_hardware="true")
     def test_01_create_delete_portforwarding_fornonvpc(self):
         """
@@ -227,6 +228,27 @@ def test_01_create_delete_portforwarding_fornonvpc(self):
             list_ipaddresses_before,
             "IP Addresses listed for newly created User"
         )
+
+        self.service_offering = ServiceOffering.create(
+            self.apiClient,
+            self.services["service_offerings"]["tiny"],
+        )
+
+        self.services["virtual_machine"]["zoneid"] = self.zone.id
+
+        vm = VirtualMachine.create(
+            self.userapiclient,
+            self.services["virtual_machine"],
+            accountid=self.account.name,
+            domainid=self.account.domainid,
+            networkids=network.id,
+            serviceofferingid=self.service_offering.id
+        )
+
+        VirtualMachine.delete(vm, self.userapiclient, expunge=False)
+
+        self.cleanup.append(vm)
+
         # Associating an IP Addresses to Network created
         associated_ipaddress = PublicIPAddress.create(
             self.userapiclient,
@@ -250,7 +272,7 @@ def test_01_create_delete_portforwarding_fornonvpc(self):
         )
         # Verifying the length of the list is 1
         self.assertEqual(
-            1,
+            2,
             len(list_ipaddresses_after),
             "Number of IP Addresses associated are not matching expected"
         )
@@ -283,7 +305,7 @@ def test_01_create_delete_portforwarding_fornonvpc(self):
         )
         # Verifying the length of the list is 1
         self.assertEqual(
-            1,
+            2,
             len(list_ipaddresses_after),
             "VM Created is not in Running state"
         )
@@ -370,7 +392,6 @@ def test_01_create_delete_portforwarding_fornonvpc(self):
         # Deleting Port Forwarding Rule
         portfwd_rule.delete(self.userapiclient)
 
-
         # Creating a Port Forwarding rule with port range
         portfwd_rule = NATRule.create(
             self.userapiclient,
@@ -382,7 +403,7 @@ def test_01_create_delete_portforwarding_fornonvpc(self):
             portfwd_rule,
             "Failed to create Port Forwarding Rule"
         )
-        #update the private port for port forwarding rule
+        # update the private port for port forwarding rule
         updatefwd_rule = portfwd_rule.update(self.userapiclient,
                             portfwd_rule.id,
                             virtual_machine=vm_created,
@@ -425,4 +446,3 @@ def test_01_create_delete_portforwarding_fornonvpc(self):
         vm_created.delete(self.apiClient)
         self.cleanup.append(self.account)
         return
-