[
https://issues.apache.org/jira/browse/CLOUDSTACK-4045?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=16348387#comment-16348387
]
ASF GitHub Bot commented on CLOUDSTACK-4045:
--------------------------------------------
houthuis 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:
us...@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)
