You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2018/09/21 14:53:35 UTC
[GitHub] houthuis closed pull request #2382: CLOUDSTACK-4045 IP address
acquired with associateIpAddress is marked as source NAT
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 ec7adfd1057..4c4a9ec7369 100644
--- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
+++ b/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.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);
@@ -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.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 2bf989c7c1f..64e8c3419e1 100644
--- a/server/src/test/java/com/cloud/network/IpAddressManagerTest.java
+++ b/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,12 +35,20 @@
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 {
@@ -45,15 +56,41 @@
@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>());
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
With regards,
Apache Git Services