Skip to content

Commit 701146f

Browse files
authored
Merge pull request #1908 from Accelerite/staticnat
CLOUDSTACK-9317: Fixed disable static nat on leaving ips on interface
2 parents 8e087ca + d04a3e8 commit 701146f

11 files changed

Lines changed: 170 additions & 8 deletions

File tree

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -92,4 +92,8 @@ enum Purpose {
9292

9393
public Date getCreated();
9494

95+
State getRuleState();
96+
97+
void setRuleState(State ruleState);
98+
9599
}

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,7 @@ public abstract class NetworkElementCommand extends Command {
3838
public static final String VPC_PRIVATE_GATEWAY = "vpc.gateway.private";
3939
public static final String FIREWALL_EGRESS_DEFAULT = "firewall.egress.default";
4040
public static final String ROUTER_MONITORING_ENABLE = "router.monitor.enable";
41+
public static final String NETWORK_PUB_LAST_IP = "network.public.last.ip";
4142

4243
private String routerAccessIp;
4344

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

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -255,4 +255,13 @@ public Class<?> getEntityType()
255255
return IpAddress.class;
256256
}
257257

258+
@Override
259+
public State getRuleState() {
260+
return _addr.getRuleState();
261+
}
262+
263+
@Override
264+
public void setRuleState(State ruleState) {
265+
_addr.setRuleState(ruleState);
266+
}
258267
}

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

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,11 @@ public class IPAddressVO implements IpAddress {
117117
@Column(name = "display", updatable = true, nullable = false)
118118
protected boolean display = true;
119119

120+
//static nat rule state
121+
@Enumerated(value = EnumType.STRING)
122+
@Column(name = "rule_state")
123+
State ruleState;
124+
120125
@Column(name= GenericDao.REMOVED_COLUMN)
121126
private Date removed;
122127

@@ -367,4 +372,14 @@ public Date getRemoved() {
367372
public Date getCreated() {
368373
return created;
369374
}
375+
376+
@Override
377+
public State getRuleState() {
378+
return ruleState;
379+
}
380+
381+
@Override
382+
public void setRuleState(State ruleState) {
383+
this.ruleState = ruleState;
384+
}
370385
}

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

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -1734,6 +1734,7 @@ protected ExecutionResult cleanupNetworkElementCommand(final IpAssocCommand cmd)
17341734

17351735
final String routerName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME);
17361736
final String routerIp = cmd.getAccessDetail(NetworkElementCommand.ROUTER_IP);
1737+
final String lastIp = cmd.getAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP);
17371738
Connect conn;
17381739

17391740

@@ -1770,9 +1771,12 @@ protected ExecutionResult cleanupNetworkElementCommand(final IpAssocCommand cmd)
17701771
}
17711772
nicNum = broadcastUriAllocatedToVM.get(ip.getBroadcastUri());
17721773

1773-
if (numOfIps == 1 && !ip.isAdd()) {
1774-
vifHotUnPlug(conn, routerName, ip.getVifMacAddress());
1775-
networkUsage(routerIp, "deleteVif", "eth" + nicNum);
1774+
if (org.apache.commons.lang.StringUtils.equalsIgnoreCase(lastIp, "true") && !ip.isAdd()) {
1775+
// in isolated network eth2 is the default public interface. We don't want to delete it.
1776+
if (nicNum != 2) {
1777+
vifHotUnPlug(conn, routerName, ip.getVifMacAddress());
1778+
networkUsage(routerIp, "deleteVif", "eth" + nicNum);
1779+
}
17761780
}
17771781
}
17781782

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

Lines changed: 10 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -595,6 +595,8 @@ protected ExecutionResult cleanupNetworkElementCommand(final IpAssocCommand cmd)
595595
final Connection conn = getConnection();
596596
final String routerName = cmd.getAccessDetail(NetworkElementCommand.ROUTER_NAME);
597597
final String routerIp = cmd.getAccessDetail(NetworkElementCommand.ROUTER_IP);
598+
final String lastIp = cmd.getAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP);
599+
598600
try {
599601
final IpAddressTO[] ips = cmd.getIpAddresses();
600602
final int ipsCount = ips.length;
@@ -625,15 +627,20 @@ protected ExecutionResult cleanupNetworkElementCommand(final IpAssocCommand cmd)
625627

626628
// there is only one ip in this public vlan and removing it, so
627629
// remove the nic
628-
if (ipsCount == 1 && !ip.isAdd()) {
629-
removeVif = true;
630+
if (org.apache.commons.lang.StringUtils.equalsIgnoreCase(lastIp, "true") && !ip.isAdd()) {
631+
final VIF correctVif = getCorrectVif(conn, router, network);
632+
// in isolated network eth2 is the default public interface. We don't want to delete it.
633+
if (correctVif != null && !correctVif.getDevice(conn).equals("2")) {
634+
removeVif = true;
635+
}
630636
}
631637

632638
if (removeVif) {
633639

634640
// Determine the correct VIF on DomR to
635641
// associate/disassociate the
636642
// IP address with
643+
637644
final VIF correctVif = getCorrectVif(conn, router, network);
638645
if (correctVif != null) {
639646
network = correctVif.getNetwork(conn);
@@ -5222,7 +5229,7 @@ public boolean createVmdataFiles(final String vmName, final List<String[]> vmDat
52225229
//Remove the folder before creating it.
52235230

52245231
try {
5225-
deleteLocalFolder("/tmp/"+isoPath);
5232+
deleteLocalFolder("/tmp/" + isoPath);
52265233
} catch (final IOException e) {
52275234
s_logger.debug("Failed to delete the exiting config drive for vm "+vmName+ " "+ e.getMessage());
52285235
} catch (final Exception e) {

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

Lines changed: 18 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1716,6 +1716,22 @@ public String acquireGuestIpAddress(Network network, String requestedIp) {
17161716

17171717
Random _rand = new Random(System.currentTimeMillis());
17181718

1719+
/**
1720+
* Get the list of public IPs that need to be applied for a static NAT enable/disable operation.
1721+
* Manipulating only these ips prevents concurrency issues when disabling static nat at the same time.
1722+
* @param staticNats
1723+
* @return The list of IPs that need to be applied for the static NAT to work.
1724+
*/
1725+
public List<IPAddressVO> getStaticNatSourceIps(List<? extends StaticNat> staticNats) {
1726+
List<IPAddressVO> userIps = new ArrayList<>();
1727+
1728+
for (StaticNat snat : staticNats) {
1729+
userIps.add(_ipAddressDao.findById(snat.getSourceIpAddressId()));
1730+
}
1731+
1732+
return userIps;
1733+
}
1734+
17191735
@Override
17201736
public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean continueOnError, boolean forRevoke) throws ResourceUnavailableException {
17211737
if (staticNats == null || staticNats.size() == 0) {
@@ -1732,8 +1748,8 @@ public boolean applyStaticNats(List<? extends StaticNat> staticNats, boolean con
17321748
return true;
17331749
}
17341750

1735-
// get the list of public ip's owned by the network
1736-
List<IPAddressVO> userIps = _ipAddressDao.listByAssociatedNetwork(network.getId(), null);
1751+
List<IPAddressVO> userIps = getStaticNatSourceIps(staticNats);
1752+
17371753
List<PublicIp> publicIps = new ArrayList<PublicIp>();
17381754
if (userIps != null && !userIps.isEmpty()) {
17391755
for (IPAddressVO userIp : userIps) {

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

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -84,6 +84,7 @@
8484
import com.cloud.network.dao.IPAddressDao;
8585
import com.cloud.network.dao.NetworkDao;
8686
import com.cloud.network.dao.NetworkVO;
87+
import com.cloud.network.dao.IPAddressVO;
8788
import com.cloud.network.dao.Site2SiteCustomerGatewayDao;
8889
import com.cloud.network.dao.Site2SiteCustomerGatewayVO;
8990
import com.cloud.network.dao.Site2SiteVpnGatewayDao;
@@ -835,13 +836,38 @@ public int compare(final PublicIpAddress o1, final PublicIpAddress o2) {
835836
associatedWithNetworkId = ipAddrList.get(0).getNetworkId();
836837
}
837838

839+
// for network if the ips does not have any rules, then only last ip
840+
List<IPAddressVO> userIps = _ipAddressDao.listByAssociatedNetwork(associatedWithNetworkId, null);
841+
842+
int ipsWithrules = 0;
843+
int ipsStaticNat = 0;
844+
for (IPAddressVO ip : userIps) {
845+
if ( _rulesDao.countRulesByIpIdAndState(ip.getId(), FirewallRule.State.Active) > 0){
846+
ipsWithrules++;
847+
}
848+
849+
// check onetoonenat and also check if the ip "add":false. If there are 2 PF rules remove and
850+
// 1 static nat rule add
851+
if (ip.isOneToOneNat() && ip.getRuleState() == null) {
852+
ipsStaticNat++;
853+
}
854+
}
855+
838856
final IpAssocCommand cmd = new IpAssocCommand(ipsToSend);
839857
cmd.setAccessDetail(NetworkElementCommand.ROUTER_IP, _routerControlHelper.getRouterControlIp(router.getId()));
840858
cmd.setAccessDetail(NetworkElementCommand.ROUTER_GUEST_IP, _routerControlHelper.getRouterIpInNetwork(associatedWithNetworkId, router.getId()));
841859
cmd.setAccessDetail(NetworkElementCommand.ROUTER_NAME, router.getInstanceName());
842860
final DataCenterVO dcVo = _dcDao.findById(router.getDataCenterId());
843861
cmd.setAccessDetail(NetworkElementCommand.ZONE_NETWORK_TYPE, dcVo.getNetworkType().toString());
844862

863+
// if there is 1 static nat then it will be checked for remove at the resource
864+
if (ipsWithrules == 0 && ipsStaticNat == 0) {
865+
// there is only one ip address for the network.
866+
cmd.setAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP, "true");
867+
} else {
868+
cmd.setAccessDetail(NetworkElementCommand.NETWORK_PUB_LAST_IP, "false");
869+
}
870+
845871
cmds.addCommand(ipAssocCommand, cmd);
846872
}
847873
}

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1259,6 +1259,10 @@ public boolean disableStaticNat(long ipId, Account caller, long callerUserId, bo
12591259
throw ex;
12601260
}
12611261

1262+
ipAddress.setRuleState(IpAddress.State.Releasing);
1263+
_ipAddressDao.update(ipAddress.getId(), ipAddress);
1264+
ipAddress = _ipAddressDao.findById(ipId);
1265+
12621266
// Revoke all firewall rules for the ip
12631267
try {
12641268
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
12801284
boolean isIpSystem = ipAddress.getSystem();
12811285
ipAddress.setOneToOneNat(false);
12821286
ipAddress.setAssociatedWithVmId(null);
1287+
ipAddress.setRuleState(null);
12831288
ipAddress.setVmIp(null);
12841289
if (isIpSystem && !releaseIpIfElastic) {
12851290
ipAddress.setSystem(false);
@@ -1295,6 +1300,9 @@ public boolean disableStaticNat(long ipId, Account caller, long callerUserId, bo
12951300
return true;
12961301
} else {
12971302
s_logger.warn("Failed to disable one to one nat for the ip address id" + ipId);
1303+
ipAddress = _ipAddressDao.findById(ipId);
1304+
ipAddress.setRuleState(null);
1305+
_ipAddressDao.update(ipAddress.getId(), ipAddress);
12981306
return false;
12991307
}
13001308
}
Lines changed: 71 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,71 @@
1+
// Licensed to the Apache Software Foundation (ASF) under one
2+
// or more contributor license agreements. See the NOTICE file
3+
// distributed with this work for additional information
4+
// regarding copyright ownership. The ASF licenses this file
5+
// to you under the Apache License, Version 2.0 (the
6+
// "License"); you may not use this file except in compliance
7+
// with the License. You may obtain a copy of the License at
8+
//
9+
// http://www.apache.org/licenses/LICENSE-2.0
10+
//
11+
// Unless required by applicable law or agreed to in writing,
12+
// software distributed under the License is distributed on an
13+
// "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
14+
// KIND, either express or implied. See the License for the
15+
// specific language governing permissions and limitations
16+
// under the License.
17+
18+
package com.cloud.network;
19+
20+
import org.junit.Assert;
21+
import org.junit.Before;
22+
import org.junit.Test;
23+
import org.mockito.InjectMocks;
24+
import org.mockito.Mock;
25+
import org.mockito.MockitoAnnotations;
26+
27+
import com.cloud.network.dao.IPAddressDao;
28+
import com.cloud.network.dao.IPAddressVO;
29+
import com.cloud.network.rules.StaticNat;
30+
import com.cloud.network.rules.StaticNatImpl;
31+
import com.cloud.utils.net.Ip;
32+
33+
import static org.mockito.Mockito.when;
34+
35+
import java.util.Collections;
36+
import java.util.List;
37+
38+
import static org.mockito.Mockito.anyLong;
39+
import static org.mockito.Mockito.mock;
40+
41+
public class IpAddressManagerTest {
42+
43+
@Mock
44+
IPAddressDao _ipAddrDao;
45+
46+
@InjectMocks
47+
IpAddressManagerImpl _ipManager;
48+
49+
@Before
50+
public void setup() {
51+
MockitoAnnotations.initMocks(this);
52+
}
53+
54+
@Test
55+
public void testGetStaticNatSourceIps() {
56+
String publicIpAddress = "192.168.1.3";
57+
IPAddressVO vo = mock(IPAddressVO.class);
58+
when(vo.getAddress()).thenReturn(new Ip(publicIpAddress));
59+
when(vo.getId()).thenReturn(1l);
60+
61+
when(_ipAddrDao.findById(anyLong())).thenReturn(vo);
62+
StaticNat snat = new StaticNatImpl(1, 1, 1, 1, publicIpAddress, false);
63+
64+
List<IPAddressVO> ips = _ipManager.getStaticNatSourceIps(Collections.singletonList(snat));
65+
Assert.assertNotNull(ips);
66+
Assert.assertEquals(1, ips.size());
67+
68+
IPAddressVO returnedVO = ips.get(0);
69+
Assert.assertEquals(vo, returnedVO);
70+
}
71+
}

0 commit comments

Comments
 (0)