CLOUDSTACK-9812: Update 'updatePortForwardingRule' api to include additional parameter end port (#1985)

Configure a PF rule Private port : Start port ; 20 ENd POrt 25 || Public Port : Start port 20 ; ENd Port : 25.
Trigger UpdatePortForwardingRule api
ApI fails with following error : " Unable to update the private port of port forwarding rule as the rule has port range "

Solution-
Port range gets modified

api/src/com/cloud/network/rules/RulesService.java

@@ -81,6 +81,6 @@ public interface RulesService {
 
     boolean disableStaticNat(long ipId) throws ResourceUnavailableException, NetworkRuleConflictException, InsufficientAddressCapacityException;
 
-    PortForwardingRule updatePortForwardingRule(long id, Integer privatePort, Long virtualMachineId, Ip vmGuestIp, String customId, Boolean forDisplay);
+    PortForwardingRule updatePortForwardingRule(long id, Integer privatePort, Integer privateEndPort, Long virtualMachineId, Ip vmGuestIp, String customId, Boolean forDisplay);
 
 }

api/src/org/apache/cloudstack/api/command/user/firewall/UpdatePortForwardingRuleCmd.java

@@ -47,9 +47,13 @@ public class UpdatePortForwardingRuleCmd extends BaseAsyncCustomIdCmd {
     @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = FirewallRuleResponse.class, required = true, description = "the ID of the port forwarding rule", since = "4.4")
     private Long id;
 
-    @Parameter(name=ApiConstants.PRIVATE_PORT, type=CommandType.INTEGER, description="the private port of the port forwarding rule")
+    @Parameter(name=ApiConstants.PRIVATE_START_PORT, type=CommandType.INTEGER, description="the private start port of the port forwarding rule")
     private Integer privatePort;
 
+
+    @Parameter(name=ApiConstants.PRIVATE_END_PORT, type=CommandType.INTEGER, description="the private end port of the port forwarding rule")
+    private Integer privateEndPort;
+
     @Parameter(name = ApiConstants.VIRTUAL_MACHINE_ID,
                type = CommandType.UUID,
                entityType = UserVmResponse.class,
@@ -74,6 +78,10 @@ public class UpdatePortForwardingRuleCmd extends BaseAsyncCustomIdCmd {
         return privatePort;
     }
 
+    public Integer getPrivateEndPort() {
+        return privateEndPort;
+    }
+
     public Long getVirtualMachineId() {
         return virtualMachineId;
     }
@@ -130,7 +138,7 @@ public class UpdatePortForwardingRuleCmd extends BaseAsyncCustomIdCmd {
 
     @Override
     public void execute() {
-        PortForwardingRule rule = _rulesService.updatePortForwardingRule(id, getPrivatePort(), getVirtualMachineId(), getVmGuestIp(), getCustomId(), getDisplay());
+        PortForwardingRule rule = _rulesService.updatePortForwardingRule(getId(), getPrivatePort(), getPrivateEndPort(), getVirtualMachineId(), getVmGuestIp(), getCustomId(), getDisplay());
         FirewallRuleResponse fwResponse = new FirewallRuleResponse();
         if (rule != null) {
             fwResponse = _responseGenerator.createPortForwardingRuleResponse(rule);

server/src/com/cloud/network/rules/RulesManagerImpl.java

@@ -1525,7 +1525,7 @@ public class RulesManagerImpl extends ManagerBase implements RulesManager, Rules
 
     @Override
     @ActionEvent(eventType = EventTypes.EVENT_NET_RULE_MODIFY, eventDescription = "updating forwarding rule", async = true)
-    public PortForwardingRule updatePortForwardingRule(long id, Integer privatePort, Long virtualMachineId, Ip vmGuestIp, String customId, Boolean forDisplay) {
+    public PortForwardingRule updatePortForwardingRule(long id, Integer privatePort, Integer privateEndPort, Long virtualMachineId, Ip vmGuestIp, String customId, Boolean forDisplay) {
         Account caller = CallContext.current().getCallingAccount();
         PortForwardingRuleVO rule = _portForwardingDao.findById(id);
         if (rule == null) {
@@ -1541,9 +1541,29 @@ public class RulesManagerImpl extends ManagerBase implements RulesManager, Rules
             rule.setDisplay(forDisplay);
         }
 
-        if (!rule.getSourcePortStart().equals(rule.getSourcePortEnd()) && privatePort != null) {
-            throw new InvalidParameterValueException("Unable to update the private port of port forwarding rule as  the rule has port range : " + rule.getSourcePortStart() + " to " + rule.getSourcePortEnd());
+        if (privatePort != null && !NetUtils.isValidPort(privatePort)) {
+            throw new InvalidParameterValueException("privatePort is an invalid value: " + privatePort);
         }
+        if (privateEndPort != null && !NetUtils.isValidPort(privateEndPort)) {
+            throw new InvalidParameterValueException("PrivateEndPort has an invalid value: " + privateEndPort);
+        }
+
+        if (privatePort != null && privateEndPort != null && ((privateEndPort - privatePort) != (rule.getSourcePortEnd() - rule.getSourcePortStart())))
+        {
+            throw new InvalidParameterValueException("Unable to update the private port range of port forwarding rule as  " +
+                    "the provided port range is not consistent with the port range : " + rule.getSourcePortStart() + " to " + rule.getSourcePortEnd());
+        }
+
+        //in case of port range
+        if (!rule.getSourcePortStart().equals(rule.getSourcePortEnd())) {
+             if ((privatePort == null || privateEndPort == null) && !(privatePort == null && privateEndPort == null)) {
+                throw new InvalidParameterValueException("Unable to update the private port range of port forwarding rule as  " +
+                        "the provided port range is not consistent with the port range : " + rule.getSourcePortStart() + " to " + rule.getSourcePortEnd());
+            }
+        }
+
+
+
         if (virtualMachineId == null && vmGuestIp != null) {
             throw new InvalidParameterValueException("vmguestip should be set along with virtualmachineid");
         }
@@ -1588,8 +1608,12 @@ public class RulesManagerImpl extends ManagerBase implements RulesManager, Rules
         rule.setState(State.Add);
         if (privatePort != null) {
             rule.setDestinationPortStart(privatePort.intValue());
-            rule.setDestinationPortEnd(privatePort.intValue());
+            rule.setDestinationPortEnd((privateEndPort == null) ? privatePort.intValue() : privateEndPort.intValue());
+        } else if (privateEndPort != null) {
+            rule.setDestinationPortStart(privateEndPort.intValue());
+            rule.setDestinationPortEnd(privateEndPort);
         }
+
         if (virtualMachineId != null) {
             rule.setVirtualMachineId(virtualMachineId);
             rule.setDestinationIpAddress(dstIp);

test/integration/smoke/test_portforwardingrules.py

@@ -0,0 +1,428 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements.  See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership.  The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License.  You may obtain a copy of the License at
+#
+#   http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied.  See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+# Import Local Modules
+from marvin.cloudstackTestCase import cloudstackTestCase, unittest
+from marvin.lib.base import (PublicIPAddress,
+                             NetworkOffering,
+                             Autoscale,
+                             Network,
+                             NetworkServiceProvider,
+                             Template,
+                             VirtualMachine,
+                             VPC,
+                             VpcOffering,
+                             StaticNATRule,
+                             FireWallRule,
+                             NATRule,
+                             Vpn,
+                             VpnUser,
+                             LoadBalancerRule,
+                             Account,
+                             ServiceOffering,
+                             PhysicalNetwork,
+                             User)
+from marvin.lib.common import (get_domain,
+                               get_zone,
+                               get_template)
+from marvin.lib.utils import validateList, cleanup_resources
+from marvin.codes import PASS
+from nose.plugins.attrib import attr
+
+class TestPortForwardingRules(cloudstackTestCase):
+
+    @classmethod
+    def setUpClass(cls):
+        try:
+            cls._cleanup = []
+            cls.testClient = super(TestPortForwardingRules, cls).getClsTestClient()
+            cls.api_client = cls.testClient.getApiClient()
+            cls.services = cls.testClient.getParsedTestDataConfig()
+            cls.hypervisor = cls.testClient.getHypervisorInfo()
+            # Get Domain, Zone, Template
+            cls.domain = get_domain(cls.api_client)
+            cls.zone = get_zone(
+                cls.api_client,
+                cls.testClient.getZoneForTests())
+            cls.template = get_template(
+                cls.api_client,
+                cls.zone.id,
+                cls.services["ostype"]
+            )
+            if cls.zone.localstorageenabled:
+                cls.storagetype = 'local'
+                cls.services["service_offerings"][
+                    "tiny"]["storagetype"] = 'local'
+            else:
+                cls.storagetype = 'shared'
+                cls.services["service_offerings"][
+                    "tiny"]["storagetype"] = 'shared'
+
+            cls.services['mode'] = cls.zone.networktype
+            cls.services["virtual_machine"][
+                "hypervisor"] = cls.hypervisor
+            cls.services["virtual_machine"]["zoneid"] = cls.zone.id
+            cls.services["virtual_machine"]["template"] = cls.template.id
+            cls.service_offering = ServiceOffering.create(
+                cls.api_client,
+                cls.services["service_offerings"]["tiny"]
+            )
+            cls._cleanup.append(cls.service_offering)
+            cls.services['mode'] = cls.zone.networktype
+
+            cls.vpc_offering = VpcOffering.create(cls.api_client,
+                                                  cls.services["vpc_offering"]
+                                                  )
+            cls.vpc_offering.update(cls.api_client, state='Enabled')
+            cls._cleanup.append(cls.vpc_offering)
+
+        except Exception as e:
+            cls.tearDownClass()
+            raise Exception("Warning: Exception in setup : %s" % e)
+        return
+
+    def setUp(self):
+
+        self.apiClient = self.testClient.getApiClient()
+        self.cleanup = []
+        self.account = Account.create(
+            self.apiClient,
+            self.services["account"],
+            domainid=self.domain.id
+        )
+        # Getting authentication for user in newly created Account
+        self.user = self.account.user[0]
+        self.userapiclient = self.testClient.getUserApiClient(
+            self.user.username,
+            self.domain.name)
+
+    def tearDown(self):
+        # Clean up, terminate the created volumes
+        cleanup_resources(self.apiClient, self.cleanup)
+        return
+
+    @classmethod
+    def tearDownClass(cls):
+        try:
+            cleanup_resources(cls.api_client, cls._cleanup)
+        except Exception as e:
+            raise Exception("Warning: Exception during cleanup : %s" % e)
+
+    def __verify_values(self, expected_vals, actual_vals):
+        """
+        @summary: Function to verify expected and actual values
+        Step1: Initializing return flag to True
+        Step1: Verifying length of expected and actual dictionaries is matching
+               If not matching returning false
+        Step2: Listing all the keys from expected dictionary
+        Step3: Looping through each key from step2 and verifying expected
+               and actual dictionaries have same value
+               If not making return flag to False
+        Step4: returning the return flag after all the values are verified
+        """
+        return_flag = True
+
+        if len(expected_vals) != len(actual_vals):
+            return False
+
+        keys = expected_vals.keys()
+        for i in range(0, len(expected_vals)):
+            exp_val = expected_vals[keys[i]]
+            act_val = actual_vals[keys[i]]
+            if exp_val == act_val:
+                return_flag = return_flag and True
+            else:
+                return_flag = return_flag and False
+                self.debug(
+                    "expected Value: %s, is not matching with actual value: %s"
+                    %
+                    (exp_val, act_val))
+        return return_flag
+
+
+
+    @attr(tags=["advanced"], required_hardware="true")
+    def test_01_create_delete_portforwarding_fornonvpc(self):
+        """
+        @summary: Test to list, create and delete Port Forwarding for
+        IP Address associated to Non VPC network
+        @Steps:
+        Step1: Creating a Network for the user
+        Step2: Associating an IP Addresses for Network
+        Step3: Launching Virtual Machine in network created in step 2
+        Step4: Listing Port Forwarding Rules for the IP Address associated
+               in Step2
+        Step5: Verifying that no Port Forwarding Rules are listed
+        Step6: Creating a Port Forwarding Rule for IP Address associated in
+               Step2
+        Step7: Listing Port Forwarding Rules for the IP Address associated in
+               Step2
+        Step8: Verifying 1 Port Forwarding Rule is listed
+        Step9: Deleting the Port Forwarding Rule created in Step6
+        Step10: Listing Port Forwarding Rules for the IP Address associated in
+               Step2
+        Step11: Verifying that no Port Forwarding Rules are listed
+        """
+        # Listing all the Networks's for a user
+        list_networks_before = Network.list(
+            self.userapiclient,
+            listall=self.services["listall"],
+            type="Isolated"
+        )
+        # Verifying No Networks are listed
+        self.assertIsNone(
+            list_networks_before,
+            "Networks listed for newly created User"
+        )
+        # Listing Network Offerings
+        network_offerings_list = NetworkOffering.list(
+            self.apiClient,
+            forvpc="false",
+            guestiptype="Isolated",
+            state="Enabled",
+            supportedservices="SourceNat,PortForwarding",
+            zoneid=self.zone.id
+        )
+        status = validateList(network_offerings_list)
+        self.assertEquals(
+            PASS,
+            status[0],
+            "Isolated Network Offerings with sourceNat,\
+                    PortForwarding enabled are not found"
+        )
+        # Creating a network
+        network = Network.create(
+            self.userapiclient,
+            self.services["network"],
+            accountid=self.account.name,
+            domainid=self.domain.id,
+            networkofferingid=network_offerings_list[0].id,
+            zoneid=self.zone.id
+        )
+        self.assertIsNotNone(
+            network,
+            "Network creation failed"
+        )
+        # Listing all the IP Addresses for a user
+        list_ipaddresses_before = PublicIPAddress.list(
+            self.userapiclient,
+            listall=self.services["listall"]
+        )
+        # Verifying no IP Addresses are listed
+        self.assertIsNone(
+            list_ipaddresses_before,
+            "IP Addresses listed for newly created User"
+        )
+        # Associating an IP Addresses to Network created
+        associated_ipaddress = PublicIPAddress.create(
+            self.userapiclient,
+            services=self.services["network"],
+            networkid=network.id
+        )
+        self.assertIsNotNone(
+            associated_ipaddress,
+            "Failed to Associate IP Address"
+        )
+        # Listing all the IP Addresses for a user
+        list_ipaddresses_after = PublicIPAddress.list(
+            self.userapiclient,
+            listall=self.services["listall"]
+        )
+        status = validateList(list_ipaddresses_after)
+        self.assertEquals(
+            PASS,
+            status[0],
+            "IP Addresses Association Failed"
+        )
+        # Verifying the length of the list is 1
+        self.assertEqual(
+            1,
+            len(list_ipaddresses_after),
+            "Number of IP Addresses associated are not matching expected"
+        )
+        # Launching a Virtual Machine with above created Network
+        vm_created = VirtualMachine.create(
+            self.userapiclient,
+            self.services["virtual_machine"],
+            accountid=self.account.name,
+            domainid=self.account.domainid,
+            networkids=network.id,
+            serviceofferingid=self.service_offering.id,
+        )
+        self.assertIsNotNone(
+            vm_created,
+            "Failed to launch a VM under network created"
+        )
+        self.cleanup.append(network)
+        # Listing Virtual Machines in running state in above created network
+        list_vms_running = VirtualMachine.list(
+            self.userapiclient,
+            listall=self.services["listall"],
+            state="Running",
+            networkid=network.id
+        )
+        status = validateList(list_vms_running)
+        self.assertEquals(
+            PASS,
+            status[0],
+            "VM Created is not in Running state"
+        )
+        # Verifying the length of the list is 1
+        self.assertEqual(
+            1,
+            len(list_ipaddresses_after),
+            "VM Created is not in Running state"
+        )
+        self.assertEquals(
+            vm_created.id,
+            list_vms_running[0].id,
+            "VM Created is not in Running state"
+        )
+        # Listing Virtual Machines in stopped state in above created network
+        list_vms_stopped = VirtualMachine.list(
+            self.userapiclient,
+            listall=self.services["listall"],
+            state="Stopped",
+            networkid=network.id
+        )
+        # Verifying no VMs are in stopped state
+        self.assertIsNone(
+            list_vms_stopped,
+            "VM Created is in stopped state"
+        )
+        # Listing Port Forwarding Rules for the IP Address associated
+        list_prtfwdrule_before = NATRule.list(
+            self.userapiclient,
+            listall=self.services["listall"],
+            ipaddressid=associated_ipaddress.ipaddress.id
+        )
+        # Verifying no port forwarding rules are listed
+        self.assertIsNone(
+            list_prtfwdrule_before,
+            "Port Forwarding Rules listed for newly associated IP Address"
+        )
+        # Creating a Port Forwarding rule
+        portfwd_rule = NATRule.create(
+            self.userapiclient,
+            virtual_machine=vm_created,
+            services=self.services["natrule"],
+            ipaddressid=associated_ipaddress.ipaddress.id,
+        )
+        self.assertIsNotNone(
+            portfwd_rule,
+            "Failed to create Port Forwarding Rule"
+        )
+        # Verifying details of Sticky Policy created
+        # Creating expected and actual values dictionaries
+        expected_dict = {
+            "ipaddressid": associated_ipaddress.ipaddress.id,
+            "privateport": str(self.services["natrule"]["privateport"]),
+            "publicport": str(self.services["natrule"]["publicport"]),
+            "protocol": str(self.services["natrule"]["protocol"]).lower(),
+        }
+        actual_dict = {
+            "ipaddressid": portfwd_rule.ipaddressid,
+            "privateport": str(portfwd_rule.privateport),
+            "publicport": str(portfwd_rule.publicport),
+            "protocol": portfwd_rule.protocol,
+        }
+        portfwd_status = self.__verify_values(
+            expected_dict,
+            actual_dict
+        )
+        self.assertEqual(
+            True,
+            portfwd_status,
+            "Created Port Forward Rule details are not as expected"
+        )
+        # Listing Port Forwarding Rules for the IP Address associated
+        list_prtfwdrule_after = NATRule.list(
+            self.userapiclient,
+            listall=self.services["listall"],
+            ipaddressid=associated_ipaddress.ipaddress.id
+        )
+        status = validateList(list_prtfwdrule_after)
+        self.assertEquals(
+            PASS,
+            status[0],
+            "Failed to create Port Forwarding Rule"
+        )
+        # Verifying the length of the list is 1
+        self.assertEqual(
+            1,
+            len(list_prtfwdrule_after),
+            "Failed to create Port Forwarding Rule"
+        )
+        # 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
+        updatefwd_rule = portfwd_rule.update(self.userapiclient,
+                            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
+        )
+        self.assertEqual(
+            True,
+            portfwd_status,
+            "Updated Port Forward Rule details are not as expected"
+        )
+        # Deleting Port Forwarding Rule
+        portfwd_rule.delete(self.userapiclient)
+        # Listing Port Forwarding Rules for the IP Address associated
+        list_prtfwdrule_after = NATRule.list(
+            self.userapiclient,
+            listall=self.services["listall"],
+            ipaddressid=associated_ipaddress.ipaddress.id
+        )
+        # Verifying no port forwarding rules are listed
+        self.assertIsNone(
+            list_prtfwdrule_after,
+            "Port Forwarding Rules listed after deletion"
+        )
+        # Destroying the VM Launched
+        vm_created.delete(self.apiClient)
+        self.cleanup.append(self.account)
+        return
+

tools/marvin/marvin/config/test_data.py

@@ -1,4 +1,4 @@
-# Licensed to the Apache Software Foundation (ASF) under one
+#t	 Licensed to the Apache Software Foundation (ASF) under one
 # or more contributor license agreements.  See the NOTICE file
 # distributed with this work for additional information
 # regarding copyright ownership.  The ASF licenses this file
@@ -779,6 +779,17 @@ test_data = {
         "publicport": 22,
         "protocol": "TCP"
     },
+    "natrulerange": {
+        "privateport": 70,
+        "privateendport": 75,
+        "publicport": 70,
+        "publicendport": 75,
+        "protocol": "TCP"
+    },
+    "updatenatrulerange": {
+        "privateport": 50,
+        "privateendport": 55,
+    },
    "egress_80": {
         "startport": 80,
         "endport": 80,

tools/marvin/marvin/lib/base.py

@@ -1649,6 +1649,30 @@ class NATRule:
 
         return NATRule(apiclient.createPortForwardingRule(cmd).__dict__)
 
+    @classmethod
+    def update(self, apiclient, id, virtual_machine, services, fordisplay=False,
+           vmguestip=None):
+        """Create Port forwarding rule"""
+        cmd = updatePortForwardingRule.updatePortForwardingRuleCmd()
+        cmd.id = id
+
+        if "privateport" in services:
+            cmd.privateport = services["privateport"]
+
+        if "privateendport" in services:
+            cmd.privateendport = services["privateendport"]
+
+        if vmguestip:
+            cmd.vmguestip = vmguestip
+
+        if fordisplay:
+            cmd.fordisplay = fordisplay
+
+        if virtual_machine.id:
+            cmd.virtualmachineid = virtual_machine.id
+
+        return NATRule(apiclient.updatePortForwardingRule(cmd).__dict__)
+
     def delete(self, apiclient):
         """Delete port forwarding"""
         cmd = deletePortForwardingRule.deletePortForwardingRuleCmd()