You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by ga...@apache.org on 2019/01/23 12:05:24 UTC

[cloudstack] branch master updated: IP address acquired with associate ip address is marked as source nat (#3125)

This is an automated email from the ASF dual-hosted git repository.

gabriel pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/cloudstack.git


The following commit(s) were added to refs/heads/master by this push:
     new 323f791  IP address acquired with associate ip address is marked as source nat (#3125)
323f791 is described below

commit 323f791efca6f1d5b8bb63573d9e385c97c427e1
Author: Dingane Hlaluku <di...@shapeblue.com>
AuthorDate: Wed Jan 23 14:05:16 2019 +0200

    IP address acquired with associate ip address is marked as source nat (#3125)
    
    * CLOUDSTACK-4045 added a check for network state when determining whether a new IP should be source NAT. this prevents associated IP's to be marked as source NAT when the network is in allocated state, causing disassociateIpAddress to fail later
    
    * Remove mock object that cause other tests to fail
    
    * Remove underscores from variable types and add documentation for the created method
    
    * Improve exception message to include network name
    
    * Include network UUID with the Exception message and fix failing marvin test
    
    * Rebase against latest master and format AssociateIPAddrCmd class
---
 .../command/user/address/AssociateIPAddrCmd.java   |  67 +++++++-----
 .../com/cloud/network/IpAddressManagerImpl.java    |  38 +++++--
 .../com/cloud/network/IpAddressManagerTest.java    | 116 ++++++++++++++++++---
 test/integration/component/test_tags.py            |  15 +++
 test/integration/smoke/test_network.py             |  38 +++++++
 test/integration/smoke/test_portforwardingrules.py |  67 ++++++------
 6 files changed, 263 insertions(+), 78 deletions(-)

diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/address/AssociateIPAddrCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/address/AssociateIPAddrCmd.java
index bf4ca88..fea7f20 100644
--- a/api/src/main/java/org/apache/cloudstack/api/command/user/address/AssociateIPAddrCmd.java
+++ b/api/src/main/java/org/apache/cloudstack/api/command/user/address/AssociateIPAddrCmd.java
@@ -57,8 +57,12 @@ import com.cloud.offering.NetworkOffering;
 import com.cloud.projects.Project;
 import com.cloud.user.Account;
 
-@APICommand(name = "associateIpAddress", description = "Acquires and associates a public IP to an account. Either of the parameters are required, i.e. either zoneId, or networkId, or vpcId  ", responseObject = IPAddressResponse.class, responseView = ResponseView.Restricted,
-        requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+@APICommand(name = "associateIpAddress",
+        description = "Acquires and associates a public IP to an account. Either of the parameters are required, i.e. either zoneId, or networkId, or vpcId  ",
+        responseObject = IPAddressResponse.class,
+        responseView = ResponseView.Restricted,
+        requestHasSensitiveInfo = false,
+        responseHasSensitiveInfo = false)
 public class AssociateIPAddrCmd extends BaseAsyncCreateCmd {
     public static final Logger s_logger = Logger.getLogger(AssociateIPAddrCmd.class.getName());
     private static final String s_name = "associateipaddressresponse";
@@ -67,46 +71,57 @@ public class AssociateIPAddrCmd extends BaseAsyncCreateCmd {
     //////////////// API parameters /////////////////////
     /////////////////////////////////////////////////////
 
-    @Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "the account to associate with this IP address")
+    @Parameter(name = ApiConstants.ACCOUNT,
+            type = CommandType.STRING,
+            description = "the account to associate with this IP address")
     private String accountName;
 
     @Parameter(name = ApiConstants.DOMAIN_ID,
-               type = CommandType.UUID,
-               entityType = DomainResponse.class,
-               description = "the ID of the domain to associate with this IP address")
+            type = CommandType.UUID,
+            entityType = DomainResponse.class,
+            description = "the ID of the domain to associate with this IP address")
     private Long domainId;
 
     @Parameter(name = ApiConstants.ZONE_ID,
-               type = CommandType.UUID,
-               entityType = ZoneResponse.class,
-               description = "the ID of the availability zone you want to acquire an public IP address from")
+            type = CommandType.UUID,
+            entityType = ZoneResponse.class,
+            description = "the ID of the availability zone you want to acquire an public IP address from")
     private Long zoneId;
 
     @Parameter(name = ApiConstants.NETWORK_ID,
-               type = CommandType.UUID,
-               entityType = NetworkResponse.class,
-               description = "The network this IP address should be associated to.")
+            type = CommandType.UUID,
+            entityType = NetworkResponse.class,
+            description = "The network this IP address should be associated to.")
     private Long networkId;
 
-    @Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "Deploy VM for the project")
+    @Parameter(name = ApiConstants.PROJECT_ID,
+            type = CommandType.UUID,
+            entityType = ProjectResponse.class,
+            description = "Deploy VM for the project")
     private Long projectId;
 
-    @Parameter(name = ApiConstants.VPC_ID, type = CommandType.UUID, entityType = VpcResponse.class, description = "the VPC you want the IP address to "
-        + "be associated with")
+    @Parameter(name = ApiConstants.VPC_ID,
+            type = CommandType.UUID,
+            entityType = VpcResponse.class,
+            description = "the VPC you want the IP address to be associated with")
     private Long vpcId;
 
-    @Parameter(name = ApiConstants.IS_PORTABLE, type = BaseCmd.CommandType.BOOLEAN, description = "should be set to true "
-        + "if public IP is required to be transferable across zones, if not specified defaults to false")
+    @Parameter(name = ApiConstants.IS_PORTABLE,
+            type = BaseCmd.CommandType.BOOLEAN,
+            description = "should be set to true if public IP is required to be transferable across zones, if not specified defaults to false")
     private Boolean isPortable;
 
     @Parameter(name = ApiConstants.REGION_ID,
-               type = CommandType.INTEGER,
-               entityType = RegionResponse.class,
-               required = false,
-               description = "region ID from where portable IP is to be associated.")
+            type = CommandType.INTEGER,
+            entityType = RegionResponse.class,
+            required = false,
+            description = "region ID from where portable IP is to be associated.")
     private Integer regionId;
 
-    @Parameter(name = ApiConstants.FOR_DISPLAY, type = CommandType.BOOLEAN, description = "an optional field, whether to the display the IP to the end user or not", since = "4.4", authorized = {RoleType.Admin})
+    @Parameter(name = ApiConstants.FOR_DISPLAY,
+            type = CommandType.BOOLEAN,
+            description = "an optional field, whether to the display the IP to the end user or not", since = "4.4",
+            authorized = {RoleType.Admin})
     private Boolean display;
 
     /////////////////////////////////////////////////////
@@ -178,7 +193,7 @@ public class AssociateIPAddrCmd extends BaseAsyncCreateCmd {
             if (networks.size() == 0) {
                 String domain = _domainService.getDomain(getDomainId()).getName();
                 throw new InvalidParameterValueException("Account name=" + getAccountName() + " domain=" + domain + " doesn't have virtual networks in zone=" +
-                    zone.getName());
+                        zone.getName());
             }
 
             if (networks.size() < 1) {
@@ -205,7 +220,7 @@ public class AssociateIPAddrCmd extends BaseAsyncCreateCmd {
 
     @Override
     public boolean isDisplay() {
-        if(display == null)
+        if (display == null)
             return true;
         else
             return display;
@@ -224,7 +239,7 @@ public class AssociateIPAddrCmd extends BaseAsyncCreateCmd {
                     return project.getProjectAccountId();
                 } else {
                     throw new PermissionDeniedException("Can't add resources to the project with specified projectId in state=" + project.getState() +
-                        " as it's no longer active");
+                            " as it's no longer active");
                 }
             } else {
                 throw new InvalidParameterValueException("Unable to find project by ID");
@@ -268,7 +283,7 @@ public class AssociateIPAddrCmd extends BaseAsyncCreateCmd {
 
     @Override
     public String getEventDescription() {
-        return  "associating IP to network ID: " + getNetworkId() + " in zone " + getZoneId();
+        return "associating IP to network ID: " + getNetworkId() + " in zone " + getZoneId();
     }
 
     /////////////////////////////////////////////////////
diff --git a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
index f840d0d..c152034 100644
--- a/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
+++ b/server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
@@ -1377,16 +1377,7 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
             }
         }
 
-        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);
 
@@ -1424,6 +1415,33 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage
         }
     }
 
+    /**
+     * Prevents associating an IP address to an allocated (unimplemented network) network, throws an Exception otherwise
+     * @param owner Used to check if the user belongs to the Network
+     * @param ipToAssoc IP address to be associated to a Network, can only be associated to an implemented network for Source NAT
+     * @param network Network to which IP address is to be associated with, must not be in allocated state for Source NAT Network/IP association
+     * @return true if IP address can be successfully associated with Source NAT network
+     */
+    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.
+                        String msg = String.format("Network with UUID:%s is in allocated and needs to be implemented first before acquiring an IP address", network.getUuid());
+                        throw new InvalidParameterValueException(msg);
+                    }
+                    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 2bf989c..09519b9 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;
 
@@ -28,32 +31,63 @@ import java.util.List;
 import org.junit.Assert;
 import org.junit.Before;
 import org.junit.Test;
+import org.junit.runner.RunWith;
 import org.mockito.InjectMocks;
 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;
+import org.mockito.runners.MockitoJUnitRunner;
 
+@RunWith(MockitoJUnitRunner.class)
 public class IpAddressManagerTest {
 
     @Mock
-    IPAddressDao _ipAddrDao;
+    IPAddressDao ipAddressDao;
 
+    @Mock
+    NetworkDao networkDao;
+
+    @Mock
+    NetworkOfferingDao networkOfferingDao;
+
+    @Spy
     @InjectMocks
-    IpAddressManagerImpl _ipManager;
+    IpAddressManagerImpl ipAddressManager;
 
     @InjectMocks
     NetworkModelImpl networkModel = Mockito.spy(new NetworkModelImpl());
 
+    IPAddressVO ipAddressVO;
+
+    AccountVO account;
+
     @Before
-    public void setup() {
-        MockitoAnnotations.initMocks(this);
+    public void setup() throws ResourceUnavailableException {
+
+        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);
     }
 
     @Test
@@ -63,10 +97,10 @@ public class IpAddressManagerTest {
         when(vo.getAddress()).thenReturn(new Ip(publicIpAddress));
         when(vo.getId()).thenReturn(1l);
 
-        when(_ipAddrDao.findById(anyLong())).thenReturn(vo);
+        when(ipAddressDao.findById(anyLong())).thenReturn(vo);
         StaticNat snat = new StaticNatImpl(1, 1, 1, 1, publicIpAddress, false);
 
-        List<IPAddressVO> ips = _ipManager.getStaticNatSourceIps(Collections.singletonList(snat));
+        List<IPAddressVO> ips = ipAddressManager.getStaticNatSourceIps(Collections.singletonList(snat));
         Assert.assertNotNull(ips);
         Assert.assertEquals(1, ips.size());
 
@@ -78,7 +112,7 @@ public class IpAddressManagerTest {
     public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestRequestedIpEqualsIp6Gateway() {
         Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", null, new ArrayList<Service>());
 
-        boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "ip6Gateway");
+        boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "ip6Gateway");
 
         Mockito.verify(networkModel, Mockito.times(0)).listNetworkOfferingServices(Mockito.anyLong());
         Assert.assertTrue(result);
@@ -88,7 +122,7 @@ public class IpAddressManagerTest {
     public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestRequestedIpEqualsGateway() {
         Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", null, new ArrayList<Service>());
 
-        boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "gateway");
+        boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "gateway");
 
         Mockito.verify(networkModel, Mockito.times(0)).listNetworkOfferingServices(Mockito.anyLong());
         Assert.assertTrue(result);
@@ -101,7 +135,7 @@ public class IpAddressManagerTest {
         services.add(serviceGateway);
         Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", null, services);
 
-        boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
+        boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
 
         Mockito.verify(networkModel).listNetworkOfferingServices(Mockito.anyLong());
         Assert.assertFalse(result);
@@ -111,17 +145,75 @@ public class IpAddressManagerTest {
     public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestExpectFalseServicesCidrNotNull() {
         Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", "cidr", new ArrayList<Service>());
 
-        boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
+        boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
 
         Mockito.verify(networkModel).listNetworkOfferingServices(Mockito.anyLong());
         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(ipAddressManager).getExistingSourceNatInNetwork(1L, 1L);
+
+        boolean isSourceNat = ipAddressManager.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(ipAddressManager).getExistingSourceNatInNetwork(1L, 2L);
+
+        ipAddressManager.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(ipAddressManager).getExistingSourceNatInNetwork(1L, 3L);
+
+        boolean isSourceNat = ipAddressManager.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>());
 
-        boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
+        boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
 
         Mockito.verify(networkModel).listNetworkOfferingServices(Mockito.anyLong());
         Assert.assertTrue(result);
diff --git a/test/integration/component/test_tags.py b/test/integration/component/test_tags.py
index ed1aee7..11a0bba 100644
--- a/test/integration/component/test_tags.py
+++ b/test/integration/component/test_tags.py
@@ -2516,6 +2516,21 @@ class TestResourceTags(cloudstackTestCase):
             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 1a0d1a7..e49f547 100644
--- a/test/integration/smoke/test_network.py
+++ b/test/integration/smoke/test_network.py
@@ -110,6 +110,42 @@ class TestPublicIP(cloudstackTestCase):
             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 @@ class TestPublicIP(cloudstackTestCase):
             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 11901bd..8aaa088 100644
--- a/test/integration/smoke/test_portforwardingrules.py
+++ b/test/integration/smoke/test_portforwardingrules.py
@@ -16,33 +16,23 @@
 # under the License.
 
 # Import Local Modules
-from marvin.cloudstackTestCase import cloudstackTestCase, unittest
+from marvin.cloudstackTestCase import cloudstackTestCase
+from marvin.codes import PASS
 from marvin.lib.base import (PublicIPAddress,
                              NetworkOffering,
-                             Autoscale,
                              Network,
-                             NetworkServiceProvider,
-                             Template,
                              VirtualMachine,
-                             VPC,
                              VpcOffering,
-                             StaticNATRule,
-                             FireWallRule,
                              NATRule,
-                             Vpn,
-                             VpnUser,
-                             LoadBalancerRule,
                              Account,
-                             ServiceOffering,
-                             PhysicalNetwork,
-                             User)
+                             ServiceOffering)
 from marvin.lib.common import (get_domain,
                                get_zone,
                                get_test_template)
 from marvin.lib.utils import validateList, cleanup_resources
-from marvin.codes import PASS
 from nose.plugins.attrib import attr
 
+
 class TestPortForwardingRules(cloudstackTestCase):
 
     @classmethod
@@ -121,6 +111,7 @@ class TestPortForwardingRules(cloudstackTestCase):
             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 +144,6 @@ class TestPortForwardingRules(cloudstackTestCase):
                     (exp_val, act_val))
         return return_flag
 
-
-
     @attr(tags=["advanced"], required_hardware="true")
     def test_01_create_delete_portforwarding_fornonvpc(self):
         """
@@ -213,6 +202,7 @@ class TestPortForwardingRules(cloudstackTestCase):
             networkofferingid=network_offerings_list[0].id,
             zoneid=self.zone.id
         )
+
         self.assertIsNotNone(
             network,
             "Network creation failed"
@@ -227,6 +217,25 @@ class TestPortForwardingRules(cloudstackTestCase):
             list_ipaddresses_before,
             "IP Addresses listed for newly created User"
         )
+
+        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=service_offering.id
+        )
+
+        VirtualMachine.delete(vm, self.apiClient, expunge=True)
+
         # Associating an IP Addresses to Network created
         associated_ipaddress = PublicIPAddress.create(
             self.userapiclient,
@@ -248,9 +257,9 @@ class TestPortForwardingRules(cloudstackTestCase):
             status[0],
             "IP Addresses Association Failed"
         )
-        # Verifying the length of the list is 1
+        # Verifying the length of the list is 2
         self.assertEqual(
-            1,
+            2,
             len(list_ipaddresses_after),
             "Number of IP Addresses associated are not matching expected"
         )
@@ -281,9 +290,9 @@ class TestPortForwardingRules(cloudstackTestCase):
             status[0],
             "VM Created is not in Running state"
         )
-        # Verifying the length of the list is 1
+        # Verifying the length of the list is 2
         self.assertEqual(
-            1,
+            2,
             len(list_ipaddresses_after),
             "VM Created is not in Running state"
         )
@@ -370,35 +379,34 @@ class TestPortForwardingRules(cloudstackTestCase):
         # Deleting Port Forwarding Rule
         portfwd_rule.delete(self.userapiclient)
 
-
         # Creating a Port Forwarding rule with port range
         portfwd_rule = NATRule.create(
             self.userapiclient,
             virtual_machine=vm_created,
             services=self.services["natrulerange"],
             ipaddressid=associated_ipaddress.ipaddress.id,
-            )
+        )
         self.assertIsNotNone(
             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,
-                            services=self.services["updatenatrulerange"],
-                            )
+                                             portfwd_rule.id,
+                                             virtual_machine=vm_created,
+                                             services=self.services["updatenatrulerange"],
+                                             )
 
         # Verifying details of Sticky Policy created
         # Creating expected and actual values dictionaries
         expected_dict = {
             "privateport": str(self.services["updatenatrulerange"]["privateport"]),
             "privateendport": str(self.services["updatenatrulerange"]["privateendport"]),
-            }
+        }
         actual_dict = {
             "privateport": str(updatefwd_rule.privateport),
             "privateendport": str(updatefwd_rule.privateendport),
-            }
+        }
         portfwd_status = self.__verify_values(
             expected_dict,
             actual_dict
@@ -425,4 +433,3 @@ class TestPortForwardingRules(cloudstackTestCase):
         vm_created.delete(self.apiClient)
         self.cleanup.append(self.account)
         return
-