[ 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: 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)