From c20e0ef88f133065cabe7189d787c4d56848abb6 Mon Sep 17 00:00:00 2001 From: Jayapal Date: Tue, 17 Jan 2017 15:20:16 +0530 Subject: [PATCH] CLOUDSTACK-9317: Fixed disable static nat on leaving ips on interface --- .../api/routing/NetworkElementCommand.java | 1 + .../resource/LibvirtComputingResource.java | 10 ++- .../resource/CitrixResourceBase.java | 13 +++- .../cloud/network/IpAddressManagerImpl.java | 20 +++++- .../network/router/CommandSetupHelper.java | 29 ++++++++ .../cloud/network/IpAddressManagerTest.java | 71 +++++++++++++++++++ 6 files changed, 136 insertions(+), 8 deletions(-) create mode 100644 server/test/com/cloud/network/IpAddressManagerTest.java diff --git a/core/src/com/cloud/agent/api/routing/NetworkElementCommand.java b/core/src/com/cloud/agent/api/routing/NetworkElementCommand.java index 6e62e816ece..a23b1a3d3e7 100644 --- a/core/src/com/cloud/agent/api/routing/NetworkElementCommand.java +++ b/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 = "newtork.public.last.ip"; private String routerAccessIp; diff --git a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java index d7059f116ce..b43f57a04d6 100644 --- a/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java +++ b/plugins/hypervisors/kvm/src/com/cloud/hypervisor/kvm/resource/LibvirtComputingResource.java @@ -1734,6 +1734,7 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv 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 @@ public class LibvirtComputingResource extends ServerResourceBase implements Serv } nicNum = broadcastUriAllocatedToVM.get(ip.getBroadcastUri()); - if (numOfIps == 1 && !ip.isAdd()) { - vifHotUnPlug(conn, routerName, ip.getVifMacAddress()); - networkUsage(routerIp, "deleteVif", "eth" + nicNum); + if (lastIp != null && !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); + } } } diff --git a/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java b/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java index 9699361ff84..27e1180c484 100644 --- a/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java +++ b/plugins/hypervisors/xenserver/src/com/cloud/hypervisor/xenserver/resource/CitrixResourceBase.java @@ -595,6 +595,8 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe 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,8 +627,12 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe // there is only one ip in this public vlan and removing it, so // remove the nic - if (ipsCount == 1 && !ip.isAdd()) { - removeVif = true; + if (lastIp != null && !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) { @@ -634,6 +640,7 @@ public abstract class CitrixResourceBase implements ServerResource, HypervisorRe // 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 abstract class CitrixResourceBase implements ServerResource, HypervisorRe //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) { diff --git a/server/src/com/cloud/network/IpAddressManagerImpl.java b/server/src/com/cloud/network/IpAddressManagerImpl.java index 9fe88bbab5e..80684217ca5 100644 --- a/server/src/com/cloud/network/IpAddressManagerImpl.java +++ b/server/src/com/cloud/network/IpAddressManagerImpl.java @@ -1716,6 +1716,22 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage 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 getStaticNatSourceIps(List staticNats) { + List userIps = new ArrayList<>(); + + for (StaticNat snat : staticNats) { + userIps.add(_ipAddressDao.findById(snat.getSourceIpAddressId())); + } + + return userIps; + } + @Override public boolean applyStaticNats(List staticNats, boolean continueOnError, boolean forRevoke) throws ResourceUnavailableException { if (staticNats == null || staticNats.size() == 0) { @@ -1732,8 +1748,8 @@ public class IpAddressManagerImpl extends ManagerBase implements IpAddressManage return true; } - // get the list of public ip's owned by the network - List userIps = _ipAddressDao.listByAssociatedNetwork(network.getId(), null); + List userIps = getStaticNatSourceIps(staticNats); + List publicIps = new ArrayList(); if (userIps != null && !userIps.isEmpty()) { for (IPAddressVO userIp : userIps) { diff --git a/server/src/com/cloud/network/router/CommandSetupHelper.java b/server/src/com/cloud/network/router/CommandSetupHelper.java index e5d634c2ae4..e1d5ef7b7fe 100644 --- a/server/src/com/cloud/network/router/CommandSetupHelper.java +++ b/server/src/com/cloud/network/router/CommandSetupHelper.java @@ -84,6 +84,7 @@ import com.cloud.network.dao.FirewallRulesDao; 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; @@ -175,6 +176,8 @@ public class CommandSetupHelper { @Inject private IPAddressDao _ipAddressDao; @Inject + private FirewallRulesDao _firewallsDao; + @Inject private GuestOSDao _guestOSDao; @Inject @@ -845,6 +848,21 @@ public class CommandSetupHelper { associatedWithNetworkId = ipAddrList.get(0).getNetworkId(); } + // for network if the ips does not have any rules, then only last ip + List userIps = _ipAddressDao.listByAssociatedNetwork(associatedWithNetworkId, null); + + int ipsWithrules = 0; + int ipsStaticNat = 0; + for (IPAddressVO ip : userIps) { + if ( _firewallsDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active) > 0 ) { + ipsWithrules++; + } + + if (ip.isOneToOneNat()) { + 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())); @@ -852,6 +870,17 @@ public class CommandSetupHelper { final DataCenterVO dcVo = _dcDao.findById(router.getDataCenterId()); cmd.setAccessDetail(NetworkElementCommand.ZONE_NETWORK_TYPE, dcVo.getNetworkType().toString()); + boolean remove = false; + // if there is only one static nat then it will be checked for remove at the resource + if (ipsWithrules == 0 && (ipsStaticNat == 0 || ipsStaticNat == 1)) { + remove = true; + } + + if (remove) { + // there is only one ip address for the network. + cmd.setAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP, "true"); + } + cmds.addCommand(ipAssocCommand, cmd); } } diff --git a/server/test/com/cloud/network/IpAddressManagerTest.java b/server/test/com/cloud/network/IpAddressManagerTest.java new file mode 100644 index 00000000000..0bf92ee2f69 --- /dev/null +++ b/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 ips = _ipManager.getStaticNatSourceIps(Collections.singletonList(snat)); + Assert.assertNotNull(ips); + Assert.assertEquals(1, ips.size()); + + IPAddressVO returnedVO = ips.get(0); + Assert.assertEquals(vo, returnedVO); + } +}