CLOUDSTACK-9317: Fixed disable static nat on leaving ips on interface

api/src/com/cloud/network/IpAddress.java

@@ -92,4 +92,8 @@ enum Purpose {
 
     public Date getCreated();
 
+    State getRuleState();
+
+    void setRuleState(State ruleState);
+
 }

core/src/com/cloud/agent/api/routing/NetworkElementCommand.java

@@ -38,6 +38,7 @@ public abstract class NetworkElementCommand extends Command {
     public static final String VPC_PRIVATE_GATEWAY = "vpc.gateway.private";
     public static final String FIREWALL_EGRESS_DEFAULT = "firewall.egress.default";
     public static final String ROUTER_MONITORING_ENABLE = "router.monitor.enable";
+    public static final String NETWORK_PUB_LAST_IP = "network.public.last.ip";
 
     private String routerAccessIp;
 

engine/components-api/src/com/cloud/network/addr/PublicIp.java

@@ -255,4 +255,13 @@ public Class<?> getEntityType()
         return IpAddress.class;
     }
 
+    @Override
+    public State getRuleState() {
+        return _addr.getRuleState();
+    }
+
+    @Override
+    public void setRuleState(State ruleState) {
+        _addr.setRuleState(ruleState);
+    }
 }

engine/schema/src/com/cloud/network/dao/IPAddressVO.java

@@ -117,6 +117,11 @@ public class IPAddressVO implements IpAddress {
     @Column(name = "display", updatable = true, nullable = false)
     protected boolean display = true;
 
+    //static nat rule state
+    @Enumerated(value = EnumType.STRING)
+    @Column(name = "rule_state")
+    State ruleState;
+
     @Column(name= GenericDao.REMOVED_COLUMN)
     private Date removed;
 
@@ -367,4 +372,14 @@ public Date getRemoved() {
     public Date getCreated() {
         return created;
     }
+
+    @Override
+    public State getRuleState() {
+        return ruleState;
+    }
+
+    @Override
+    public void setRuleState(State ruleState) {
+        this.ruleState = ruleState;
+    }
 }

plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java

@@ -1734,6 +1734,7 @@ protected ExecutionResult cleanupNetworkElementCommand(final IpAssocCommand cmd)
 
         final String routerName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME);
         final String routerIp = cmd.getAccessDetail(NetworkElementCommand.ROUTER_IP);
+        final String lastIp = cmd.getAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP);
         Connect conn;
 
 
@@ -1770,9 +1771,12 @@ protected ExecutionResult cleanupNetworkElementCommand(final IpAssocCommand cmd)
                 }
                 nicNum = broadcastUriAllocatedToVM.get(ip.getBroadcastUri());
 
-                if (numOfIps == 1 && !ip.isAdd()) {
-                    vifHotUnPlug(conn, routerName, ip.getVifMacAddress());
-                    networkUsage(routerIp, "deleteVif", "eth" + nicNum);
+                if (org.apache.commons.lang.StringUtils.equalsIgnoreCase(lastIp, "true") && !ip.isAdd()) {
+                    // in isolated network eth2 is the default public interface. We don't want to delete it.
+                    if (nicNum != 2) {
+                        vifHotUnPlug(conn, routerName, ip.getVifMacAddress());
+                        networkUsage(routerIp, "deleteVif", "eth" + nicNum);
+                    }
                 }
             }
 

plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java

@@ -595,6 +595,8 @@ protected ExecutionResult cleanupNetworkElementCommand(final IpAssocCommand cmd)
         final Connection conn = getConnection();
         final String routerName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME);
         final String routerIp = cmd.getAccessDetail(NetworkElementCommand.ROUTER_IP);
+        final String lastIp = cmd.getAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP);
+
         try {
             final IpAddressTO[] ips = cmd.getIpAddresses();
             final int ipsCount = ips.length;
@@ -625,15 +627,20 @@ protected ExecutionResult cleanupNetworkElementCommand(final IpAssocCommand cmd)
 
                 // there is only one ip in this public vlan and removing it, so
                 // remove the nic
-                if (ipsCount == 1 && !ip.isAdd()) {
-                    removeVif = true;
+                if (org.apache.commons.lang.StringUtils.equalsIgnoreCase(lastIp, "true") && !ip.isAdd()) {
+                    final VIF correctVif = getCorrectVif(conn, router, network);
+                    // in isolated network eth2 is the default public interface. We don't want to delete it.
+                    if (correctVif != null && !correctVif.getDevice(conn).equals("2")) {
+                        removeVif = true;
+                    }
                 }
 
                 if (removeVif) {
 
                     // Determine the correct VIF on DomR to
                     // associate/disassociate the
                     // IP address with
+
                     final VIF correctVif = getCorrectVif(conn, router, network);
                     if (correctVif != null) {
                         network = correctVif.getNetwork(conn);
@@ -5222,7 +5229,7 @@ public boolean createVmdataFiles(final String vmName, final List<String[]> vmDat
         //Remove the folder before creating it.
 
         try {
-            deleteLocalFolder("/tmp/"+isoPath);
+            deleteLocalFolder("/tmp/" + isoPath);
         } catch (final IOException e) {
             s_logger.debug("Failed to delete the exiting config drive for vm "+vmName+ " "+ e.getMessage());
         } catch (final Exception e) {

server/src/com/cloud/network/IpAddressManagerImpl.java

@@ -1716,6 +1716,22 @@ public String acquireGuestIpAddress(Network network, String requestedIp) {
 
     Random _rand = new Random(System.currentTimeMillis());
 
+    /**
+     * Get the list of public IPs that need to be applied for a static NAT enable/disable operation.
+     * Manipulating only these ips prevents concurrency issues when disabling static nat at the same time.
+     * @param staticNats
+     * @return The list of IPs that need to be applied for the static NAT to work.
+     */
+    public List<IPAddressVO> getStaticNatSourceIps(List<? extends StaticNat> staticNats) {
+        List<IPAddressVO> userIps = new ArrayList<>();
+
+        for (StaticNat snat : staticNats) {
+            userIps.add(_ipAddressDao.findById(snat.getSourceIpAddressId()));
+        }
+
+        return userIps;
+    }
+
     @Override
     public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError, boolean forRevoke) throws ResourceUnavailableException {
         if (staticNats == null || staticNats.size() == 0) {
@@ -1732,8 +1748,8 @@ public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean con
             return true;
         }
 
-        // get the list of public ip's owned by the network
-        List<IPAddressVO> userIps = _ipAddressDao.listByAssociatedNetwork(network.getId(), null);
+        List<IPAddressVO> userIps = getStaticNatSourceIps(staticNats);
+
         List<PublicIp> publicIps = new ArrayList<PublicIp>();
         if (userIps != null && !userIps.isEmpty()) {
             for (IPAddressVO userIp : userIps) {

server/src/com/cloud/network/router/CommandSetupHelper.java

@@ -84,6 +84,7 @@
 import com.cloud.network.dao.IPAddressDao;
 import com.cloud.network.dao.NetworkDao;
 import com.cloud.network.dao.NetworkVO;
+import com.cloud.network.dao.IPAddressVO;
 import com.cloud.network.dao.Site2SiteCustomerGatewayDao;
 import com.cloud.network.dao.Site2SiteCustomerGatewayVO;
 import com.cloud.network.dao.Site2SiteVpnGatewayDao;
@@ -845,13 +846,38 @@ public int compare(final PublicIpAddress o1, final PublicIpAddress o2) {
                 associatedWithNetworkId = ipAddrList.get(0).getNetworkId();
             }
 
+            // for network if the ips does not have any rules, then only last ip
+            List<IPAddressVO> userIps = _ipAddressDao.listByAssociatedNetwork(associatedWithNetworkId, null);
+
+            int ipsWithrules = 0;
+            int ipsStaticNat = 0;
+            for (IPAddressVO ip : userIps) {
+                if ( _rulesDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active) > 0){
+                    ipsWithrules++;
+                }
+
+                // check onetoonenat and also check if the ip "add":false. If there are 2 PF rules remove and
+                // 1 static nat rule add
+                if (ip.isOneToOneNat() && ip.getRuleState() == null) {
+                    ipsStaticNat++;
+                }
+            }
+
             final IpAssocCommand cmd = new IpAssocCommand(ipsToSend);
             cmd.setAccessDetail(NetworkElementCommand.ROUTER_IP, _routerControlHelper.getRouterControlIp(router.getId()));
             cmd.setAccessDetail(NetworkElementCommand.ROUTER_GUEST_IP, _routerControlHelper.getRouterIpInNetwork(associatedWithNetworkId, router.getId()));
             cmd.setAccessDetail(NetworkElementCommand.ROUTER_NAME, router.getInstanceName());
             final DataCenterVO dcVo = _dcDao.findById(router.getDataCenterId());
             cmd.setAccessDetail(NetworkElementCommand.ZONE_NETWORK_TYPE, dcVo.getNetworkType().toString());
 
+            // if there is 1 static nat then it will be checked for remove at the resource
+            if (ipsWithrules == 0 && ipsStaticNat == 0) {
+                // there is only one ip address for the network.
+                cmd.setAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP, "true");
+            } else {
+                cmd.setAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP, "false");
+            }
+
             cmds.addCommand(ipAssocCommand, cmd);
         }
     }

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

@@ -1259,6 +1259,10 @@ public boolean disableStaticNat(long ipId, Account caller, long callerUserId, bo
             throw ex;
         }
 
+        ipAddress.setRuleState(IpAddress.State.Releasing);
+        _ipAddressDao.update(ipAddress.getId(), ipAddress);
+        ipAddress = _ipAddressDao.findById(ipId);
+
         // Revoke all firewall rules for the ip
         try {
             s_logger.debug("Revoking all " + Purpose.Firewall + "rules as a part of disabling static nat for public IP id=" + ipId);
@@ -1280,6 +1284,7 @@ public boolean disableStaticNat(long ipId, Account caller, long callerUserId, bo
             boolean isIpSystem = ipAddress.getSystem();
             ipAddress.setOneToOneNat(false);
             ipAddress.setAssociatedWithVmId(null);
+            ipAddress.setRuleState(null);
             ipAddress.setVmIp(null);
             if (isIpSystem && !releaseIpIfElastic) {
                 ipAddress.setSystem(false);
@@ -1295,6 +1300,9 @@ public boolean disableStaticNat(long ipId, Account caller, long callerUserId, bo
             return true;
         } else {
             s_logger.warn("Failed to disable one to one nat for the ip address id" + ipId);
+            ipAddress = _ipAddressDao.findById(ipId);
+            ipAddress.setRuleState(null);
+            _ipAddressDao.update(ipAddress.getId(), ipAddress);
             return false;
         }
     }

server/test/com/cloud/network/IpAddressManagerTest.java

@@ -0,0 +1,71 @@
+// 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.
+
+package com.cloud.network;
+
+import org.junit.Assert;
+import org.junit.Before;
+import org.junit.Test;
+import org.mockito.InjectMocks;
+import org.mockito.Mock;
+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 java.util.Collections;
+import java.util.List;
+
+import static org.mockito.Mockito.anyLong;
+import static org.mockito.Mockito.mock;
+
+public class IpAddressManagerTest {
+
+    @Mock
+    IPAddressDao _ipAddrDao;
+
+    @InjectMocks
+    IpAddressManagerImpl _ipManager;
+
+    @Before
+    public void setup() {
+        MockitoAnnotations.initMocks(this);
+    }
+
+    @Test
+    public void testGetStaticNatSourceIps() {
+        String publicIpAddress = "192.168.1.3";
+        IPAddressVO vo = mock(IPAddressVO.class);
+        when(vo.getAddress()).thenReturn(new Ip(publicIpAddress));
+        when(vo.getId()).thenReturn(1l);
+
+        when(_ipAddrDao.findById(anyLong())).thenReturn(vo);
+        StaticNat snat = new StaticNatImpl(1, 1, 1, 1, publicIpAddress, false);
+
+        List<IPAddressVO> ips = _ipManager.getStaticNatSourceIps(Collections.singletonList(snat));
+        Assert.assertNotNull(ips);
+        Assert.assertEquals(1, ips.size());
+
+        IPAddressVO returnedVO = ips.get(0);
+        Assert.assertEquals(vo, returnedVO);
+    }
+}

setup/db/db/schema-4920to41000.sql

@@ -245,3 +245,4 @@ CREATE TABLE `cloud`.`guest_os_details` (
   CONSTRAINT `fk_guest_os_details__guest_os_id` FOREIGN KEY `fk_guest_os_details__guest_os_id`(`guest_os_id`) REFERENCES `guest_os`(`id`) ON DELETE CASCADE
 ) ENGINE=InnoDB DEFAULT CHARSET=utf8;
 
+ALTER TABLE `user_ip_address` ADD COLUMN `rule_state` VARCHAR(32) COMMENT 'static  rule state while removing';