Skip to content

Commit

Permalink
IP address acquired with associate ip address is marked as source nat (…
Browse files Browse the repository at this point in the history 
…#3125)

* CLOUDSTACK-4045 added a check for network state when determining whether a new IP should be source NAT. this prevents associated IP's to be marked as source NAT when the network is in allocated state, causing disassociateIpAddress to fail later

* Remove mock object that cause other tests to fail

* Remove underscores from variable types and add documentation for the created method

* Improve exception message to include network name

* Include network UUID with the Exception message and fix failing marvin test

* Rebase against latest master and format AssociateIPAddrCmd class
  • Loading branch information
@GabrielBrascher
Dingane Hlaluku authored and GabrielBrascher committed Jan 23, 2019
1 parent f967944 commit 323f791
Show file tree
Hide file tree
Showing 6 changed files with 263 additions and 78 deletions.
67 changes: 41 additions & 26 deletions api/src/main/java/org/apache/cloudstack/api/command/user/address/AssociateIPAddrCmd.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -57,8 +57,12 @@
import com.cloud.projects.Project;
import com.cloud.user.Account;

@APICommand(name = "associateIpAddress", description = "Acquires and associates a public IP to an account. Either of the parameters are required, i.e. either zoneId, or networkId, or vpcId ", responseObject = IPAddressResponse.class, responseView = ResponseView.Restricted,
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
@APICommand(name = "associateIpAddress",
description = "Acquires and associates a public IP to an account. Either of the parameters are required, i.e. either zoneId, or networkId, or vpcId ",
responseObject = IPAddressResponse.class,
responseView = ResponseView.Restricted,
requestHasSensitiveInfo = false,
responseHasSensitiveInfo = false)
public class AssociateIPAddrCmd extends BaseAsyncCreateCmd {
public static final Logger s_logger = Logger.getLogger(AssociateIPAddrCmd.class.getName());
private static final String s_name = "associateipaddressresponse";
Expand All @@ -67,46 +71,57 @@ public class AssociateIPAddrCmd extends BaseAsyncCreateCmd {
//////////////// API parameters /////////////////////
/////////////////////////////////////////////////////

@Parameter(name = ApiConstants.ACCOUNT, type = CommandType.STRING, description = "the account to associate with this IP address")
@Parameter(name = ApiConstants.ACCOUNT,
type = CommandType.STRING,
description = "the account to associate with this IP address")
private String accountName;

@Parameter(name = ApiConstants.DOMAIN_ID,
type = CommandType.UUID,
entityType = DomainResponse.class,
description = "the ID of the domain to associate with this IP address")
type = CommandType.UUID,
entityType = DomainResponse.class,
description = "the ID of the domain to associate with this IP address")
private Long domainId;

@Parameter(name = ApiConstants.ZONE_ID,
type = CommandType.UUID,
entityType = ZoneResponse.class,
description = "the ID of the availability zone you want to acquire an public IP address from")
type = CommandType.UUID,
entityType = ZoneResponse.class,
description = "the ID of the availability zone you want to acquire an public IP address from")
private Long zoneId;

@Parameter(name = ApiConstants.NETWORK_ID,
type = CommandType.UUID,
entityType = NetworkResponse.class,
description = "The network this IP address should be associated to.")
type = CommandType.UUID,
entityType = NetworkResponse.class,
description = "The network this IP address should be associated to.")
private Long networkId;

@Parameter(name = ApiConstants.PROJECT_ID, type = CommandType.UUID, entityType = ProjectResponse.class, description = "Deploy VM for the project")
@Parameter(name = ApiConstants.PROJECT_ID,
type = CommandType.UUID,
entityType = ProjectResponse.class,
description = "Deploy VM for the project")
private Long projectId;

@Parameter(name = ApiConstants.VPC_ID, type = CommandType.UUID, entityType = VpcResponse.class, description = "the VPC you want the IP address to "
+ "be associated with")
@Parameter(name = ApiConstants.VPC_ID,
type = CommandType.UUID,
entityType = VpcResponse.class,
description = "the VPC you want the IP address to be associated with")
private Long vpcId;

@Parameter(name = ApiConstants.IS_PORTABLE, type = BaseCmd.CommandType.BOOLEAN, description = "should be set to true "
+ "if public IP is required to be transferable across zones, if not specified defaults to false")
@Parameter(name = ApiConstants.IS_PORTABLE,
type = BaseCmd.CommandType.BOOLEAN,
description = "should be set to true if public IP is required to be transferable across zones, if not specified defaults to false")
private Boolean isPortable;

@Parameter(name = ApiConstants.REGION_ID,
type = CommandType.INTEGER,
entityType = RegionResponse.class,
required = false,
description = "region ID from where portable IP is to be associated.")
type = CommandType.INTEGER,
entityType = RegionResponse.class,
required = false,
description = "region ID from where portable IP is to be associated.")
private Integer regionId;

@Parameter(name = ApiConstants.FOR_DISPLAY, type = CommandType.BOOLEAN, description = "an optional field, whether to the display the IP to the end user or not", since = "4.4", authorized = {RoleType.Admin})
@Parameter(name = ApiConstants.FOR_DISPLAY,
type = CommandType.BOOLEAN,
description = "an optional field, whether to the display the IP to the end user or not", since = "4.4",
authorized = {RoleType.Admin})
private Boolean display;

/////////////////////////////////////////////////////
Expand Down Expand Up @@ -178,7 +193,7 @@ public Long getNetworkId() {
if (networks.size() == 0) {
String domain = _domainService.getDomain(getDomainId()).getName();
throw new InvalidParameterValueException("Account name=" + getAccountName() + " domain=" + domain + " doesn't have virtual networks in zone=" +
zone.getName());
zone.getName());
}

if (networks.size() < 1) {
Expand All @@ -205,7 +220,7 @@ public Boolean getDisplayIp() {

@Override
public boolean isDisplay() {
if(display == null)
if (display == null)
return true;
else
return display;
Expand All @@ -224,7 +239,7 @@ public long getEntityOwnerId() {
return project.getProjectAccountId();
} else {
throw new PermissionDeniedException("Can't add resources to the project with specified projectId in state=" + project.getState() +
" as it's no longer active");
" as it's no longer active");
}
} else {
throw new InvalidParameterValueException("Unable to find project by ID");
Expand Down Expand Up @@ -268,7 +283,7 @@ public String getEventType() {

@Override
public String getEventDescription() {
return "associating IP to network ID: " + getNetworkId() + " in zone " + getZoneId();
return "associating IP to network ID: " + getNetworkId() + " in zone " + getZoneId();
}

/////////////////////////////////////////////////////
Expand Down
38 changes: 28 additions & 10 deletions server/src/main/java/com/cloud/network/IpAddressManagerImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1377,16 +1377,7 @@ public IPAddressVO associateIPToGuestNetwork(long ipId, long networkId, boolean
}
}

NetworkOffering offering = _networkOfferingDao.findById(network.getNetworkOfferingId());
boolean sharedSourceNat = offering.isSharedSourceNat();
boolean isSourceNat = false;
if (!sharedSourceNat) {
if (getExistingSourceNatInNetwork(owner.getId(), networkId) == null) {
if (network.getGuestType() == GuestType.Isolated && network.getVpcId() == null && !ipToAssoc.isPortable()) {
isSourceNat = true;
}
}
}
boolean isSourceNat = isSourceNatAvailableForNetwork(owner, ipToAssoc, network);

s_logger.debug("Associating ip " + ipToAssoc + " to network " + network);

Expand Down Expand Up @@ -1424,6 +1415,33 @@ public IPAddressVO associateIPToGuestNetwork(long ipId, long networkId, boolean
}
}

/**
* Prevents associating an IP address to an allocated (unimplemented network) network, throws an Exception otherwise
* @param owner Used to check if the user belongs to the Network
* @param ipToAssoc IP address to be associated to a Network, can only be associated to an implemented network for Source NAT
* @param network Network to which IP address is to be associated with, must not be in allocated state for Source NAT Network/IP association
* @return true if IP address can be successfully associated with Source NAT network
*/
protected boolean isSourceNatAvailableForNetwork(Account owner, IPAddressVO ipToAssoc, Network network) {
NetworkOffering offering = _networkOfferingDao.findById(network.getNetworkOfferingId());
boolean sharedSourceNat = offering.isSharedSourceNat();
boolean isSourceNat = false;
if (!sharedSourceNat) {
if (getExistingSourceNatInNetwork(owner.getId(), network.getId()) == null) {
if (network.getGuestType() == GuestType.Isolated && network.getVpcId() == null && !ipToAssoc.isPortable()) {
if (network.getState() == Network.State.Allocated) {
//prevent associating an ip address to an allocated (unimplemented network).
//it will cause the ip to become source nat, and it can't be disassociated later on.
String msg = String.format("Network with UUID:%s is in allocated and needs to be implemented first before acquiring an IP address", network.getUuid());
throw new InvalidParameterValueException(msg);
}
isSourceNat = true;
}
}
}
return isSourceNat;
}

protected boolean isSharedNetworkOfferingWithServices(long networkOfferingId) {
NetworkOfferingVO networkOffering = _networkOfferingDao.findById(networkOfferingId);
if ((networkOffering.getGuestType() == Network.GuestType.Shared)
Expand Down
116 changes: 104 additions & 12 deletions server/src/test/java/com/cloud/network/IpAddressManagerTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,10 @@

package com.cloud.network;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertTrue;
import static org.mockito.Matchers.anyLong;
import static org.mockito.Mockito.doReturn;
import static org.mockito.Mockito.mock;
import static org.mockito.Mockito.when;

Expand All @@ -28,32 +31,63 @@
import org.junit.Assert;
import org.junit.Before;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Mockito;
import org.mockito.MockitoAnnotations;
import org.mockito.Spy;

import com.cloud.exception.InvalidParameterValueException;
import com.cloud.exception.ResourceUnavailableException;
import com.cloud.network.Network.Service;
import com.cloud.network.dao.IPAddressDao;
import com.cloud.network.dao.IPAddressVO;
import com.cloud.network.dao.NetworkDao;
import com.cloud.network.dao.NetworkVO;
import com.cloud.network.rules.StaticNat;
import com.cloud.network.rules.StaticNatImpl;
import com.cloud.offerings.NetworkOfferingVO;
import com.cloud.offerings.dao.NetworkOfferingDao;
import com.cloud.user.AccountVO;
import com.cloud.utils.net.Ip;
import org.mockito.runners.MockitoJUnitRunner;

@RunWith(MockitoJUnitRunner.class)
public class IpAddressManagerTest {

@Mock
IPAddressDao _ipAddrDao;
IPAddressDao ipAddressDao;

@Mock
NetworkDao networkDao;

@Mock
NetworkOfferingDao networkOfferingDao;

@Spy
@InjectMocks
IpAddressManagerImpl _ipManager;
IpAddressManagerImpl ipAddressManager;

@InjectMocks
NetworkModelImpl networkModel = Mockito.spy(new NetworkModelImpl());

IPAddressVO ipAddressVO;

AccountVO account;

@Before
public void setup() {
MockitoAnnotations.initMocks(this);
public void setup() throws ResourceUnavailableException {

ipAddressVO = new IPAddressVO(new Ip("192.0.0.1"), 1L, 1L, 1L,false);
ipAddressVO.setAllocatedToAccountId(1L);

account = new AccountVO("admin", 1L, null, (short) 1, 1L, "c65a73d5-ebbd-11e7-8f45-107b44277808");
account.setId(1L);

NetworkOfferingVO networkOfferingVO = Mockito.mock(NetworkOfferingVO.class);
networkOfferingVO.setSharedSourceNat(false);

Mockito.when(networkOfferingDao.findById(Mockito.anyLong())).thenReturn(networkOfferingVO);
}

@Test
Expand All @@ -63,10 +97,10 @@ public void testGetStaticNatSourceIps() {
when(vo.getAddress()).thenReturn(new Ip(publicIpAddress));
when(vo.getId()).thenReturn(1l);

when(_ipAddrDao.findById(anyLong())).thenReturn(vo);
when(ipAddressDao.findById(anyLong())).thenReturn(vo);
StaticNat snat = new StaticNatImpl(1, 1, 1, 1, publicIpAddress, false);

List<IPAddressVO> ips = _ipManager.getStaticNatSourceIps(Collections.singletonList(snat));
List<IPAddressVO> ips = ipAddressManager.getStaticNatSourceIps(Collections.singletonList(snat));
Assert.assertNotNull(ips);
Assert.assertEquals(1, ips.size());

Expand All @@ -78,7 +112,7 @@ public void testGetStaticNatSourceIps() {
public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestRequestedIpEqualsIp6Gateway() {
Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", null, new ArrayList<Service>());

boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "ip6Gateway");
boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "ip6Gateway");

Mockito.verify(networkModel, Mockito.times(0)).listNetworkOfferingServices(Mockito.anyLong());
Assert.assertTrue(result);
Expand All @@ -88,7 +122,7 @@ public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestRequestedIpEqualsIp6Gate
public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestRequestedIpEqualsGateway() {
Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", null, new ArrayList<Service>());

boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "gateway");
boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "gateway");

Mockito.verify(networkModel, Mockito.times(0)).listNetworkOfferingServices(Mockito.anyLong());
Assert.assertTrue(result);
Expand All @@ -101,7 +135,7 @@ public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestExpectFalseServicesNotEm
services.add(serviceGateway);
Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", null, services);

boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");

Mockito.verify(networkModel).listNetworkOfferingServices(Mockito.anyLong());
Assert.assertFalse(result);
Expand All @@ -111,17 +145,75 @@ public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestExpectFalseServicesNotEm
public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestExpectFalseServicesCidrNotNull() {
Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", "cidr", new ArrayList<Service>());

boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");

Mockito.verify(networkModel).listNetworkOfferingServices(Mockito.anyLong());
Assert.assertFalse(result);
}

@Test
public void assertSourceNatImplementedNetwork() {

NetworkVO networkImplemented = Mockito.mock(NetworkVO.class);
when(networkImplemented.getTrafficType()).thenReturn(Networks.TrafficType.Guest);
when(networkImplemented.getNetworkOfferingId()).thenReturn(8L);
when(networkImplemented.getState()).thenReturn(Network.State.Implemented);
when(networkImplemented.getGuestType()).thenReturn(Network.GuestType.Isolated);
when(networkImplemented.getVpcId()).thenReturn(null);
when(networkImplemented.getId()).thenReturn(1L);

Mockito.when(networkDao.findById(1L)).thenReturn(networkImplemented);
doReturn(null).when(ipAddressManager).getExistingSourceNatInNetwork(1L, 1L);

boolean isSourceNat = ipAddressManager.isSourceNatAvailableForNetwork(account, ipAddressVO, networkImplemented);

assertTrue("Source NAT should be true", isSourceNat);
}

@Test(expected = InvalidParameterValueException.class)
public void assertSourceNatAllocatedNetwork() {

NetworkVO networkAllocated = Mockito.mock(NetworkVO.class);
when(networkAllocated.getTrafficType()).thenReturn(Networks.TrafficType.Guest);
when(networkAllocated.getNetworkOfferingId()).thenReturn(8L);
when(networkAllocated.getState()).thenReturn(Network.State.Allocated);
when(networkAllocated.getGuestType()).thenReturn(Network.GuestType.Isolated);
when(networkAllocated.getVpcId()).thenReturn(null);
when(networkAllocated.getId()).thenReturn(2L);

Mockito.when(networkDao.findById(2L)).thenReturn(networkAllocated);
doReturn(null).when(ipAddressManager).getExistingSourceNatInNetwork(1L, 2L);

ipAddressManager.isSourceNatAvailableForNetwork(account, ipAddressVO, networkAllocated);
}

@Test
public void assertExistingSourceNatAllocatedNetwork() {

NetworkVO networkNat = Mockito.mock(NetworkVO.class);
when(networkNat.getTrafficType()).thenReturn(Networks.TrafficType.Guest);
when(networkNat.getNetworkOfferingId()).thenReturn(8L);
when(networkNat.getState()).thenReturn(Network.State.Implemented);
when(networkNat.getGuestType()).thenReturn(Network.GuestType.Isolated);
when(networkNat.getId()).thenReturn(3L);
when(networkNat.getVpcId()).thenReturn(null);
when(networkNat.getId()).thenReturn(3L);

IPAddressVO sourceNat = new IPAddressVO(new Ip("192.0.0.2"), 1L, 1L, 1L,true);

Mockito.when(networkDao.findById(3L)).thenReturn(networkNat);
doReturn(sourceNat).when(ipAddressManager).getExistingSourceNatInNetwork(1L, 3L);

boolean isSourceNat = ipAddressManager.isSourceNatAvailableForNetwork(account, ipAddressVO, networkNat);

assertFalse("Source NAT should be false", isSourceNat);
}

@Test
public void isIpEqualsGatewayOrNetworkOfferingsEmptyTestNetworkOfferingsEmptyAndCidrNull() {
Network network = setTestIsIpEqualsGatewayOrNetworkOfferingsEmpty(0l, "gateway", "ip6Gateway", null, new ArrayList<Service>());

boolean result = _ipManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");
boolean result = ipAddressManager.isIpEqualsGatewayOrNetworkOfferingsEmpty(network, "requestedIp");

Mockito.verify(networkModel).listNetworkOfferingServices(Mockito.anyLong());
Assert.assertTrue(result);
Expand Down
Loading

0 comments on commit 323f791

Please sign in to comment.