From 45585ef0724887a78be882465c48a05d7dd73728 Mon Sep 17 00:00:00 2001 From: jeff Date: Mon, 21 Mar 2016 15:20:27 +0000 Subject: [PATCH] CLOUDSTACK-9317: Enable/disable static NAT associates only relevant IPs. The previous behavior would send a list of all IPs with a state of add/revoke. This causes concurrency issues when multiple IPs were being added or removed, and could leave networks with wrong IP addresses assigned to the devices. By assigning and removing only the IP being changed per-request, the concurrency issue is removed and no real performance loss occurs. --- .../cloud/network/IpAddressManagerImpl.java | 20 +++++- .../network/router/CommandSetupHelper.java | 7 +- .../cloud/network/IpAddressManagerTest.java | 71 +++++++++++++++++++ 3 files changed, 93 insertions(+), 5 deletions(-) create mode 100644 server/test/com/cloud/network/IpAddressManagerTest.java diff --git a/server/src/com/cloud/network/IpAddressManagerImpl.java b/server/src/com/cloud/network/IpAddressManagerImpl.java index d4da5fae24c2..bd59c66df3ab 100644 --- a/server/src/com/cloud/network/IpAddressManagerImpl.java +++ b/server/src/com/cloud/network/IpAddressManagerImpl.java @@ -1698,6 +1698,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 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) { @@ -1714,8 +1730,8 @@ public boolean applyStaticNats(List staticNats, boolean con 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 7208b2568139..500fc49a0c0d 100644 --- a/server/src/com/cloud/network/router/CommandSetupHelper.java +++ b/server/src/com/cloud/network/router/CommandSetupHelper.java @@ -780,10 +780,11 @@ public int compare(final PublicIpAddress o1, final PublicIpAddress o2) { final boolean add = ipAddr.getState() == IpAddress.State.Releasing ? false : true; boolean sourceNat = ipAddr.isSourceNat(); - /* enable sourceNAT for the first ip of the public interface */ - if (firstIP) { - sourceNat = true; + /* enable sourceNAT for the first ip of the public interface as long as it's source nat. */ + if (firstIP && !sourceNat) { + firstIP = false; } + final String vlanId = ipAddr.getVlanTag(); final String vlanGateway = ipAddr.getGateway(); final String vlanNetmask = ipAddr.getNetmask(); diff --git a/server/test/com/cloud/network/IpAddressManagerTest.java b/server/test/com/cloud/network/IpAddressManagerTest.java new file mode 100644 index 000000000000..0bf92ee2f692 --- /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); + } +}