cloudstack-issues mailing list archives

Site index · List index
Message view « Date » · « Thread »
Top « Date » · « Thread »
From "ASF GitHub Bot (JIRA)" <j...@apache.org>
Subject [jira] [Commented] (CLOUDSTACK-4045) IP address acquired with associateIpAddress is marked as source NAT, causing disassociateIpAddress to fail later
Date Mon, 05 Feb 2018 12:05:00 GMT

    [ https://issues.apache.org/jira/browse/CLOUDSTACK-4045?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16352298#comment-16352298
] 

ASF GitHub Bot commented on CLOUDSTACK-4045:
--------------------------------------------

DaanHoogland closed pull request #2382: CLOUDSTACK-4045 IP address acquired with associateIpAddress
is marked as source NAT
URL: https://github.com/apache/cloudstack/pull/2382
 
 
   

This is a PR merged from a forked repository.
As GitHub hides the original diff on merge, it is displayed below for
the sake of provenance:

As this is a foreign pull request (from a fork), the diff is supplied
below (as it won't show otherwise due to GitHub magic):

diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
index c00359c92f0..d0b3098c3d3 100644
--- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
+++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
@@ -1370,16 +1370,7 @@ public IPAddressVO associateIPToGuestNetwork(long ipId, long networkId,
boolean
             }
         }
 
-        NetworkOffering offering = _networkOfferingDao.findById(network.getNetworkOfferingId());
-        boolean sharedSourceNat = offering.getSharedSourceNat();
-        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);
 
@@ -1417,6 +1408,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.getSharedSourceNat();
+        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)
diff --git a/server/src/test/java/com/cloud/network/IpAddressManagerTest.java b/server/src/test/java/com/cloud/network/IpAddressManagerTest.java
index 0bf92ee2f69..fe11292e826 100644
--- a/server/src/test/java/com/cloud/network/IpAddressManagerTest.java
+++ b/server/src/test/java/com/cloud/network/IpAddressManagerTest.java
@@ -17,38 +17,74 @@
 
 package com.cloud.network;
 
+import com.cloud.exception.InvalidParameterValueException;
+import com.cloud.exception.ResourceUnavailableException;
+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.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
 import org.mockito.InjectMocks;
 import org.mockito.Mock;
+import org.mockito.Mockito;
 import org.mockito.MockitoAnnotations;
-
-import com.cloud.network.dao.IPAddressDao;
-import com.cloud.network.dao.IPAddressVO;
-import com.cloud.network.rules.StaticNat;
-import com.cloud.network.rules.StaticNatImpl;
-import com.cloud.utils.net.Ip;
-
-import static org.mockito.Mockito.when;
+import org.mockito.Spy;
 
 import java.util.Collections;
 import java.util.List;
 
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
 import static org.mockito.Mockito.anyLong;
+import static org.mockito.Mockito.doReturn;
 import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 public class IpAddressManagerTest {
 
     @Mock
     IPAddressDao _ipAddrDao;
 
+    @Mock
+    NetworkDao _networkDao;
+
+    @Mock
+    NetworkOfferingDao _networkOfferingDao;
+
+    @Mock
+    NetworkModel _networkModel;
+
+    @Spy
     @InjectMocks
     IpAddressManagerImpl _ipManager;
 
+    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
@@ -68,4 +104,62 @@ public void testGetStaticNatSourceIps() {
         IPAddressVO returnedVO = ips.get(0);
         Assert.assertEquals(vo, returnedVO);
     }
+
+    @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);
+    }
 }
diff --git a/test/integration/component/test_tags.py b/test/integration/component/test_tags.py
index ed1aee7a0ee..11a0bbad196 100644
--- a/test/integration/component/test_tags.py
+++ b/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
         )
diff --git a/test/integration/smoke/test_network.py b/test/integration/smoke/test_network.py
index 1a0d1a719b3..e49f5475bee 100644
--- a/test/integration/smoke/test_network.py
+++ b/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,
diff --git a/test/integration/smoke/test_portforwardingrules.py b/test/integration/smoke/test_portforwardingrules.py
index 11901bdf55a..f9ff416bdf7 100644
--- a/test/integration/smoke/test_portforwardingrules.py
+++ b/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
-


 

----------------------------------------------------------------
This is an automated message from the Apache Git Service.
To respond to the message, please log on GitHub and use the
URL above to go to the specific comment.
 
For queries about this service, please contact Infrastructure at:
users@infra.apache.org


> IP address acquired with associateIpAddress is marked as source NAT, causing disassociateIpAddress
to fail later
> ----------------------------------------------------------------------------------------------------------------
>
>                 Key: CLOUDSTACK-4045
>                 URL: https://issues.apache.org/jira/browse/CLOUDSTACK-4045
>             Project: CloudStack
>          Issue Type: Bug
>      Security Level: Public(Anyone can view this level - this is the default.) 
>    Affects Versions: 4.0.0, 4.0.1, 4.0.2, 4.1.0, 4.1.1, 4.2.0
>            Reporter: Murali Reddy
>            Assignee: Henko Holtzhausen
>            Priority: Major
>             Fix For: Future
>
>
> When you can create network, network is in allocated state. when network is implemented
CloudStack implicitly should acquire a public IP for source nat. But there is assumption that
first IP this is associated with network is always for source NAT IP. So when you do
> 1. create network (network is in allocated state)
> 2. acquire a public IP and associate with the network
> 3. disassociate ip address
> #3 will fail because CloudStack marks the IP acquired in #1 to be source NAT. For users
this is counter-intutive because when a IP is acquired, he/she should be able to release it
as well.



--
This message was sent by Atlassian JIRA
(v7.6.3#76005)

Mime
View raw message