From 8949efe8d1bf67fda0b8deaccedc2c2d4de0f246 Mon Sep 17 00:00:00 2001 From: Sigert Goeminne Date: Sat, 10 Feb 2018 18:24:01 +0100 Subject: [PATCH 1/5] CLOUDSTACK-10218: Fix for forced network update in a nuage network (#2445) Fix for forced network update to a nuage network offering with vr fails with IllegalArgumentException. Addressed review comments DaanHoogland. --- .../AssociateNuageVspDomainTemplateCmd.java | 4 +- .../network/element/NuageVspElement.java | 11 +- .../guru/NuageVspGuestNetworkGuru.java | 27 ++- .../network/manager/NuageVspManager.java | 34 +++- .../network/manager/NuageVspManagerImpl.java | 146 ++++++++++++++++- .../com/cloud/util/NuageVspEntityBuilder.java | 154 +----------------- .../src/com/cloud/util/NuageVspUtil.java | 11 ++ .../nuage-vsp/test/com/cloud/NuageTest.java | 2 - .../network/element/NuageVspElementTest.java | 9 +- .../nuagevsp/test_nuage_internal_dns.py | 60 +++++++ 10 files changed, 269 insertions(+), 189 deletions(-) diff --git a/plugins/network-elements/nuage-vsp/src/com/cloud/api/commands/AssociateNuageVspDomainTemplateCmd.java b/plugins/network-elements/nuage-vsp/src/com/cloud/api/commands/AssociateNuageVspDomainTemplateCmd.java index 760916a5b10..9b36aaff735 100644 --- a/plugins/network-elements/nuage-vsp/src/com/cloud/api/commands/AssociateNuageVspDomainTemplateCmd.java +++ b/plugins/network-elements/nuage-vsp/src/com/cloud/api/commands/AssociateNuageVspDomainTemplateCmd.java @@ -89,10 +89,10 @@ public class AssociateNuageVspDomainTemplateCmd extends BaseCmd { @Override public void execute() throws ResourceUnavailableException, InsufficientCapacityException, ServerApiException, ConcurrentOperationException, ResourceAllocationException { try { - boolean result =_nuageVspManager.associateNuageVspDomainTemplate(this); + _nuageVspManager.associateNuageVspDomainTemplate(this); SuccessResponse response = new SuccessResponse(getCommandName()); response.setResponseName(getCommandName()); - response.setSuccess(result); + response.setSuccess(true); this.setResponseObject(response); } catch (InvalidParameterValueException invalidParamExcp) { throw new ServerApiException(ApiErrorCode.PARAM_ERROR, invalidParamExcp.getMessage()); diff --git a/plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java b/plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java index abe7a71098b..d5496b0157a 100644 --- a/plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java +++ b/plugins/network-elements/nuage-vsp/src/com/cloud/network/element/NuageVspElement.java @@ -90,7 +90,6 @@ import com.cloud.network.dao.IPAddressDao; import com.cloud.network.dao.IPAddressVO; import com.cloud.network.dao.NetworkDao; import com.cloud.network.dao.NetworkServiceMapDao; -import com.cloud.network.dao.NetworkVO; import com.cloud.network.dao.PhysicalNetworkDao; import com.cloud.network.dao.PhysicalNetworkVO; import com.cloud.network.manager.NuageVspManager; @@ -276,14 +275,8 @@ public class NuageVspElement extends AdapterBase implements ConnectivityProvider return false; } - if (!_nuageVspEntityBuilder.usesVirtualRouter(offering.getId())) { - // Update broadcast uri if VR is no longer used - NetworkVO networkToUpdate = _networkDao.findById(network.getId()); - String broadcastUriStr = networkToUpdate.getUuid() + "/null"; - networkToUpdate.setBroadcastUri(Networks.BroadcastDomainType.Vsp.toUri(broadcastUriStr)); - _networkDao.update(network.getId(), networkToUpdate); - } - + _nuageVspManager.updateBroadcastUri(network); + network = _networkDao.findById(network.getId()); VspNetwork vspNetwork = _nuageVspEntityBuilder.buildVspNetwork(network); List ingressFirewallRules = getFirewallRulesToApply(network, FirewallRule.TrafficType.Ingress); List egressFirewallRules = getFirewallRulesToApply(network, FirewallRule.TrafficType.Egress); diff --git a/plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java b/plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java index 23a0efce739..b17840c194d 100644 --- a/plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java +++ b/plugins/network-elements/nuage-vsp/src/com/cloud/network/guru/NuageVspGuestNetworkGuru.java @@ -255,7 +255,7 @@ public class NuageVspGuestNetworkGuru extends GuestNetworkGuru implements Networ VpcDetailVO detail = _vpcDetailsDao.findDetail(network.getVpcId(), NuageVspManager.nuageDomainTemplateDetailName); if (detail != null && network.getNetworkACLId() != null) { s_logger.error("Pre-configured DT are used in combination with ACL lists. Which is not supported."); - throw new IllegalArgumentException("CloudStack ACLs are not supported with Nuage Preconfigured Domain Template"); + throw new IllegalArgumentException("CloudStack ACLs are not supported with Nuage Pre-configured Domain Template"); } if(detail != null && !_nuageVspManager.checkIfDomainTemplateExist(network.getDomainId(),detail.getValue(),network.getDataCenterId(),null)){ @@ -302,7 +302,9 @@ public class NuageVspGuestNetworkGuru extends GuestNetworkGuru implements Networ implemented.setCidr(network.getCidr()); } - VspNetwork vspNetwork = _nuageVspEntityBuilder.buildVspNetwork(implemented, true); + implemented.setBroadcastUri(_nuageVspManager.calculateBroadcastUri(implemented)); + implemented.setBroadcastDomainType(Networks.BroadcastDomainType.Vsp); + VspNetwork vspNetwork = _nuageVspEntityBuilder.buildVspNetwork(implemented); if (vspNetwork.isShared()) { Boolean previousUnderlay= null; @@ -321,11 +323,6 @@ public class NuageVspGuestNetworkGuru extends GuestNetworkGuru implements Networ } } - String tenantId = context.getDomain().getName() + "-" + context.getAccount().getAccountId(); - String broadcastUriStr = implemented.getUuid() + "/" + vspNetwork.getVirtualRouterIp(); - implemented.setBroadcastUri(Networks.BroadcastDomainType.Vsp.toUri(broadcastUriStr)); - implemented.setBroadcastDomainType(Networks.BroadcastDomainType.Vsp); - boolean implementSucceeded = implement(network.getVpcId(), physicalNetworkId, vspNetwork, implemented, _nuageVspEntityBuilder.buildNetworkDhcpOption(network, offering)); if (!implementSucceeded) { @@ -340,6 +337,7 @@ public class NuageVspGuestNetworkGuru extends GuestNetworkGuru implements Networ } } + String tenantId = context.getDomain().getName() + "-" + context.getAccount().getAccountId(); s_logger.info("Implemented OK, network " + implemented.getUuid() + " in tenant " + tenantId + " linked to " + implemented.getBroadcastUri()); } finally { _networkDao.releaseFromLockTable(network.getId()); @@ -430,7 +428,7 @@ public class NuageVspGuestNetworkGuru extends GuestNetworkGuru implements Networ public NicProfile allocate(Network network, NicProfile nic, VirtualMachineProfile vm) throws InsufficientVirtualNetworkCapacityException, InsufficientAddressCapacityException { if (vm.getType() != VirtualMachine.Type.DomainRouter && _nuageVspEntityBuilder.usesVirtualRouter(network.getNetworkOfferingId())) { VspNetwork vspNetwork = _nuageVspEntityBuilder.buildVspNetwork(network); - if (nic != null && nic.getRequestedIPv4() != null && vspNetwork.getVirtualRouterIp().equals(nic.getRequestedIPv4())) { + if (nic != null && nic.getRequestedIPv4() != null && nic.getRequestedIPv4().equals(vspNetwork.getVirtualRouterIp())) { DataCenter dc = _dcDao.findById(network.getDataCenterId()); s_logger.error("Unable to acquire requested Guest IP address " + nic.getRequestedIPv4() + " because it is reserved for the VR in network " + network); throw new InsufficientVirtualNetworkCapacityException("Unable to acquire requested Guest IP address " + nic.getRequestedIPv4() + " because it is reserved " + @@ -470,14 +468,13 @@ public class NuageVspGuestNetworkGuru extends GuestNetworkGuru implements Networ HostVO nuageVspHost = _nuageVspManager.getNuageVspHost(network.getPhysicalNetworkId()); VspNetwork vspNetwork = _nuageVspEntityBuilder.buildVspNetwork(vm.getVirtualMachine().getDomainId(), network); - if (vm.getType() == VirtualMachine.Type.DomainRouter && vspNetwork.getVirtualRouterIp().equals("null")) { - //In case of upgrade network offering - vspNetwork = _nuageVspEntityBuilder.buildVspNetwork(vm.getVirtualMachine().getDomainId(), network, null, true); - String broadcastUriStr = network.getUuid() + "/" + vspNetwork.getVirtualRouterIp(); - NetworkVO updatedNetwork = _networkDao.createForUpdate(network.getId()); - updatedNetwork.setBroadcastUri(Networks.BroadcastDomainType.Vsp.toUri(broadcastUriStr)); - _networkDao.update(updatedNetwork.getId(), updatedNetwork); + boolean vrAddedToNuage = vm.getType() == VirtualMachine.Type.DomainRouter && vspNetwork.getVirtualRouterIp() + .equals("null"); + if (vrAddedToNuage) { + //In case a VR is added due to upgrade network offering - recalculate the broadcast uri before using it. + _nuageVspManager.updateBroadcastUri(network); network = _networkDao.findById(network.getId()); + vspNetwork = _nuageVspEntityBuilder.buildVspNetwork(vm.getVirtualMachine().getDomainId(), network, null); } if (vspNetwork.isShared()) { diff --git a/plugins/network-elements/nuage-vsp/src/com/cloud/network/manager/NuageVspManager.java b/plugins/network-elements/nuage-vsp/src/com/cloud/network/manager/NuageVspManager.java index 6f91dc34276..803063060f7 100644 --- a/plugins/network-elements/nuage-vsp/src/com/cloud/network/manager/NuageVspManager.java +++ b/plugins/network-elements/nuage-vsp/src/com/cloud/network/manager/NuageVspManager.java @@ -19,6 +19,7 @@ package com.cloud.network.manager; +import java.net.URI; import java.util.List; import org.apache.cloudstack.framework.config.ConfigKey; @@ -35,6 +36,7 @@ import com.cloud.api.response.NuageVlanIpRangeResponse; import com.cloud.api.response.NuageVspDeviceResponse; import com.cloud.dc.Vlan; import com.cloud.api.response.NuageVspDomainTemplateResponse; +import com.cloud.exception.InsufficientVirtualNetworkCapacityException; import com.cloud.host.HostVO; import com.cloud.network.Network; import com.cloud.network.NuageVspDeviceVO; @@ -119,10 +121,9 @@ public interface NuageVspManager extends PluggableService { /** * Associates a Nuage Vsp domain template with a - * @param cmd - * @return + * @param cmd Associate cmd which contains all the data */ - boolean associateNuageVspDomainTemplate(AssociateNuageVspDomainTemplateCmd cmd); + void associateNuageVspDomainTemplate(AssociateNuageVspDomainTemplateCmd cmd); /** * Queries the VSD to check if the entity provided in the entityCmd exists on the VSD @@ -134,26 +135,41 @@ public interface NuageVspManager extends PluggableService { /** * Sets the preconfigured domain template for a given network - * @param network - * @param domainTemplateName + * @param network the network for which we want to set the domain template + * @param domainTemplateName the domain template name we want to use */ void setPreConfiguredDomainTemplateName(Network network, String domainTemplateName); /** * Returns the current pre configured domain template for a given network - * @param network - * @return + * @param network the network for which we want the domain template name + * @return the domain template name */ String getPreConfiguredDomainTemplateName(Network network); /** * Checks if a given domain template exists or not on the VSD. - * @param domainId + * @param domainId Id of the domain to search in. * @param domainTemplate The name of the domain template for which we need to query the VSD. * @param zoneId zoneId OR PhysicalNetworkId needs to be provided. * @param physicalNetworkId zoneId OR PhysicalNetworkId needs to be provided. * @return true if the domain template exists on the VSD else false if it does not exist on the VSD */ - public boolean checkIfDomainTemplateExist(Long domainId, String domainTemplate, Long zoneId, Long physicalNetworkId); + boolean checkIfDomainTemplateExist(Long domainId, String domainTemplate, Long zoneId, Long physicalNetworkId); + + /** + * calculates the new broadcast uri of a network and persists it in the database + * @param network the network for which you want to calculate the broadcast uri + * @throws InsufficientVirtualNetworkCapacityException in case there is no free ip that can be used as the VR ip. + */ + void updateBroadcastUri(Network network) throws InsufficientVirtualNetworkCapacityException; + + /** + * Calculates the broadcast uri based on the network and the offering of the given network + * @param network the network for which you want to calculate the broadcast uri + * @return the calculated broadcast uri + * @throws InsufficientVirtualNetworkCapacityException in case there is no free ip that can be used as the VR ip. + */ + URI calculateBroadcastUri(Network network) throws InsufficientVirtualNetworkCapacityException; } diff --git a/plugins/network-elements/nuage-vsp/src/com/cloud/network/manager/NuageVspManagerImpl.java b/plugins/network-elements/nuage-vsp/src/com/cloud/network/manager/NuageVspManagerImpl.java index 748de100c0c..0a3a966cb78 100644 --- a/plugins/network-elements/nuage-vsp/src/com/cloud/network/manager/NuageVspManagerImpl.java +++ b/plugins/network-elements/nuage-vsp/src/com/cloud/network/manager/NuageVspManagerImpl.java @@ -35,6 +35,7 @@ import net.nuage.vsp.acs.client.api.model.VspDomainTemplate; import net.nuage.vsp.acs.client.api.model.VspHost; import net.nuage.vsp.acs.client.common.NuageVspApiVersion; import net.nuage.vsp.acs.client.common.NuageVspConstants; +import net.nuage.vsp.acs.client.common.model.Pair; import net.nuage.vsp.acs.client.exception.NuageVspException; import org.apache.cloudstack.api.ResponseGenerator; import org.apache.cloudstack.context.CallContext; @@ -53,15 +54,19 @@ import org.apache.log4j.Logger; import javax.annotation.Nonnull; import javax.inject.Inject; import javax.naming.ConfigurationException; +import java.net.URI; import java.util.ArrayList; import java.util.Arrays; import java.util.Comparator; import java.util.HashMap; +import java.util.HashSet; +import java.util.Iterator; import java.util.LinkedList; import java.util.List; import java.util.Map; import java.util.Optional; import java.util.Set; +import java.util.TreeSet; import java.util.UUID; import java.util.stream.Collectors; @@ -105,6 +110,7 @@ import com.cloud.dc.dao.VlanDetailsDao; import com.cloud.domain.Domain; import com.cloud.domain.DomainVO; import com.cloud.domain.dao.DomainDao; +import com.cloud.exception.InsufficientVirtualNetworkCapacityException; import com.cloud.exception.InvalidParameterValueException; import com.cloud.host.DetailVO; import com.cloud.host.Host; @@ -113,6 +119,7 @@ import com.cloud.host.Status; import com.cloud.host.dao.HostDao; import com.cloud.host.dao.HostDetailsDao; import com.cloud.network.Network; +import com.cloud.network.NetworkModel; import com.cloud.network.Networks; import com.cloud.network.NuageVspDeviceVO; import com.cloud.network.PhysicalNetwork; @@ -154,6 +161,11 @@ import com.cloud.utils.db.TransactionStatus; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.fsm.StateListener; import com.cloud.utils.fsm.StateMachine2; +import com.cloud.utils.net.NetUtils; +import com.cloud.vm.VMInstanceVO; +import com.cloud.vm.VirtualMachine; +import com.cloud.vm.dao.NicDao; +import com.cloud.vm.dao.VMInstanceDao; import static com.cloud.agent.api.sync.SyncNuageVspCmsIdCommand.SyncType; @@ -213,6 +225,12 @@ public class NuageVspManagerImpl extends ManagerBase implements NuageVspManager, ResponseGenerator _responseGenerator; @Inject MessageBus _messageBus; + @Inject + VMInstanceDao _vmInstanceDao; + @Inject + NicDao _nicDao; + @Inject + NetworkModel _networkModel; static { Set nuageVspProviders = ImmutableSet.of(Network.Provider.NuageVsp); @@ -902,7 +920,7 @@ public class NuageVspManagerImpl extends ManagerBase implements NuageVspManager, } @Override - public boolean associateNuageVspDomainTemplate(AssociateNuageVspDomainTemplateCmd cmd){ + public void associateNuageVspDomainTemplate(AssociateNuageVspDomainTemplateCmd cmd){ VpcVO vpc = _vpcDao.findById(cmd.getVpcId()); Long physicalNetworkId; if (cmd.getPhysicalNetworkId() != null) { @@ -923,7 +941,6 @@ public class NuageVspManagerImpl extends ManagerBase implements NuageVspManager, throw new InvalidParameterValueException("Could not find a Domain Template with name: " + cmd.getDomainTemplate()); } setPreConfiguredDomainTemplateName(cmd.getVpcId(), cmd.getDomainTemplate()); - return true; } @Override @@ -939,6 +956,131 @@ public class NuageVspManagerImpl extends ManagerBase implements NuageVspManager, return false; } + @Override + public void updateBroadcastUri(Network network) throws InsufficientVirtualNetworkCapacityException { + NetworkVO updatedNetwork = _networkDao.createForUpdate(network.getId()); + URI broadcastUri = calculateBroadcastUri(network); + if (!broadcastUri.equals(network.getBroadcastUri())) { + updatedNetwork.setBroadcastUri(broadcastUri); + _networkDao.update(network.getId(), updatedNetwork); + } + } + + @Override + public URI calculateBroadcastUri(Network network) throws InsufficientVirtualNetworkCapacityException { + String vrIp = calculateVirtualRouterIp(network); + return Networks.BroadcastDomainType.Vsp.toUri(network.getUuid() + "/" + vrIp); + } + + private boolean usesVirtualRouter(long networkOfferingId) { + return _networkOfferingServiceMapDao.isProviderForNetworkOffering(networkOfferingId, Network.Provider.VirtualRouter) || + _networkOfferingServiceMapDao.isProviderForNetworkOffering(networkOfferingId, Network.Provider.VPCVirtualRouter); + } + + private String calculateVirtualRouterIp(Network network) + throws InsufficientVirtualNetworkCapacityException { + if (!usesVirtualRouter(network.getNetworkOfferingId())) { + return null; + } + + List> ipAddressRanges = + network.getGuestType() == Network.GuestType.Shared ? getSharedIpAddressRanges(network.getId()) : getIpAddressRanges(network); + + //check if a vr might be present already or not? CLOUD-1216 - before we always picked .2 + List vrs =_vmInstanceDao.listNonRemovedVmsByTypeAndNetwork(network.getId(), VirtualMachine.Type.DomainRouter); + + for (VMInstanceVO vr : vrs) { + return _nicDao.listByVmIdAndNicIdAndNtwkId(vr.getId(), null, network.getId()).get(0).getIPv4Address(); + } + + ensureIpCapacity(network, ipAddressRanges); + + if(network.getGuestType() == Network.GuestType.Shared) { + return ipAddressRanges.stream() + .sorted(Comparator.comparingLong(p -> NetUtils.ip2Long(p.getLeft()))) + .findFirst() + .map(Pair::getLeft) + .orElseThrow(() -> new IllegalArgumentException("Shared network without ip ranges? How can this happen?")); + } + + Network networkToCheck; + if (isMigratingNetwork(network)) { + networkToCheck = _networkDao.findById(network.getRelated()); + } else { + networkToCheck = network; + } + + Long freeIp = _networkModel.getAvailableIps(networkToCheck, null) + .stream() + .findFirst() + .orElseThrow(() -> new InsufficientVirtualNetworkCapacityException("There is no free ip available for the VirtualRouter.", + Network.class, + network.getId())); + + return NetUtils.long2Ip(freeIp); + } + + private List> getSharedIpAddressRanges(long networkId) { + List vlans = _vlanDao.listVlansByNetworkId(networkId); + List> ipAddressRanges = Lists.newArrayList(); + for (VlanVO vlan : vlans) { + Pair ipAddressRange = NuageVspUtil.getIpAddressRange(vlan); + if (ipAddressRange != null) { + ipAddressRanges.add(ipAddressRange); + } + } + return ipAddressRanges; + } + + private List> getIpAddressRanges(Network network) { + List> ipAddressRanges = Lists.newArrayList(); + String subnet = NetUtils.getCidrSubNet(network.getCidr()); + String netmask = NetUtils.getCidrNetmask(network.getCidr()); + long cidrSize = NetUtils.getCidrSize(netmask); + Set allIPsInCidr = NetUtils.getAllIpsFromCidr(subnet, cidrSize, new HashSet()); + if (allIPsInCidr == null || !(allIPsInCidr instanceof TreeSet)) { + throw new IllegalStateException("The IPs in CIDR for subnet " + subnet + " where null or returned in a non-ordered set."); + } + + Iterator ipIterator = allIPsInCidr.iterator(); + long ip = ipIterator.next(); + long gatewayIp = NetUtils.ip2Long(network.getGateway()); + String lastIp = NetUtils.getIpRangeEndIpFromCidr(subnet, cidrSize); + if (gatewayIp == ip) { + ip = ipIterator.next(); + ipAddressRanges.add(Pair.of(NetUtils.long2Ip(ip), lastIp)); + } else if (!network.getGateway().equals(lastIp)) { + ipAddressRanges.add(Pair.of(NetUtils.long2Ip(ip), NetUtils.long2Ip(gatewayIp - 1))); + ipAddressRanges.add(Pair.of(NetUtils.long2Ip(gatewayIp + 1), lastIp)); + } else { + ipAddressRanges.add(Pair.of(NetUtils.long2Ip(ip), NetUtils.long2Ip(gatewayIp - 1))); + } + + return ipAddressRanges; + } + + private void ensureIpCapacity(Network network, List> ipAddressRanges) throws InsufficientVirtualNetworkCapacityException { + long ipCount = ipAddressRanges.stream() + .mapToLong(this::getIpCount) + .sum(); + + if (ipCount == 0) { + throw new InsufficientVirtualNetworkCapacityException("VSP allocates an IP for VirtualRouter." + " But no ip address ranges are specified", Network.class, + network.getId()); + } else if (ipCount < 3) { + throw new InsufficientVirtualNetworkCapacityException("VSP allocates an IP for VirtualRouter." + " So, subnet should have atleast minimum 3 hosts", Network.class, + network.getId()); + } + } + + private boolean isMigratingNetwork(Network network) { + return network.getRelated() != network.getId(); + } + + private long getIpCount(Pair ipAddressRange) { + return NetUtils.ip2Long(ipAddressRange.getRight()) - NetUtils.ip2Long(ipAddressRange.getLeft()) + 1; + } + @Override public boolean entityExist(EntityExistsCommand cmd, Long physicalNetworkId){ Long hostId = getNuageVspHostId(physicalNetworkId); diff --git a/plugins/network-elements/nuage-vsp/src/com/cloud/util/NuageVspEntityBuilder.java b/plugins/network-elements/nuage-vsp/src/com/cloud/util/NuageVspEntityBuilder.java index cd986e5cbcb..5b1419f015b 100644 --- a/plugins/network-elements/nuage-vsp/src/com/cloud/util/NuageVspEntityBuilder.java +++ b/plugins/network-elements/nuage-vsp/src/com/cloud/util/NuageVspEntityBuilder.java @@ -19,13 +19,8 @@ package com.cloud.util; -import java.util.Comparator; -import java.util.HashSet; -import java.util.Iterator; import java.util.List; import java.util.Map; -import java.util.Set; -import java.util.TreeSet; import java.util.stream.Collectors; import javax.inject.Inject; @@ -44,10 +39,8 @@ import net.nuage.vsp.acs.client.api.model.VspVm; import net.nuage.vsp.acs.client.common.model.Pair; import org.apache.commons.collections.MapUtils; -import org.apache.commons.lang.StringUtils; import org.apache.log4j.Logger; -import com.google.common.collect.Iterables; import com.google.common.collect.Lists; import com.google.common.collect.Maps; @@ -60,7 +53,6 @@ import com.cloud.dc.dao.VlanDetailsDao; import com.cloud.domain.Domain; import com.cloud.domain.DomainVO; import com.cloud.domain.dao.DomainDao; -import com.cloud.exception.InsufficientVirtualNetworkCapacityException; import com.cloud.network.Network; import com.cloud.network.NetworkModel; import com.cloud.network.dao.IPAddressDao; @@ -79,7 +71,6 @@ import com.cloud.offerings.dao.NetworkOfferingDao; import com.cloud.offerings.dao.NetworkOfferingServiceMapDao; import com.cloud.user.AccountVO; import com.cloud.user.dao.AccountDao; -import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.net.NetUtils; import com.cloud.vm.NicProfile; import com.cloud.vm.NicVO; @@ -157,30 +148,18 @@ public class NuageVspEntityBuilder { } public VspNetwork buildVspNetwork(Network network) { - return buildVspNetwork(network.getDomainId(), network, null, false); - } - - public VspNetwork buildVspNetwork(Network network, boolean recalculateBroadcastUri) { - return buildVspNetwork(network.getDomainId(), network, null, recalculateBroadcastUri); + return buildVspNetwork(network.getDomainId(), network, null); } public VspNetwork buildVspNetwork(Network network, String vsdSubnetId) { - return buildVspNetwork(network.getDomainId(), network, vsdSubnetId, false); + return buildVspNetwork(network.getDomainId(), network, vsdSubnetId); } public VspNetwork buildVspNetwork(long domainId, Network network) { - return buildVspNetwork(domainId, network, null, false); - } - - public VspNetwork buildVspNetwork(long domainId, Network network, boolean recalculateBroadcastUri) { - return buildVspNetwork(domainId, network, null, recalculateBroadcastUri); + return buildVspNetwork(domainId, network, null); } public VspNetwork buildVspNetwork(long domainId, Network network, String vsdSubnetId) { - return buildVspNetwork(domainId, network, vsdSubnetId, false); - } - - public VspNetwork buildVspNetwork(long domainId, Network network, String vsdSubnetId, boolean recalculateBroadcastUri) { VspNetwork.Builder vspNetworkBuilder = new VspNetwork.Builder() .id(network.getId()) .uuid(network.getUuid()) @@ -254,15 +233,8 @@ public class NuageVspEntityBuilder { vspNetworkBuilder.domainTemplateName(preConfiguredDomainTemplateName); if (usesVirtualRouter(networkOffering.getId())) { - try { - List> ipAddressRanges = - networkOffering.getGuestType() == Network.GuestType.Shared ? getSharedIpAddressRanges(network.getId()) : getIpAddressRanges(network); - String virtualRouterIp = getVirtualRouterIP(network, ipAddressRanges, recalculateBroadcastUri); + String virtualRouterIp = getVirtualRouterIP(network); vspNetworkBuilder.virtualRouterIp(virtualRouterIp); - } catch (InsufficientVirtualNetworkCapacityException ex) { - s_logger.error("There is an insufficient network capacity in network " + network.getId(), ex); - throw new CloudRuntimeException("There is an insufficient network capacity in network " + network.getId(), ex); - } } return vspNetworkBuilder.build(); @@ -298,128 +270,16 @@ public class NuageVspEntityBuilder { } private boolean isVlanContainingIp(Vlan vlan, long ip) { - Pair ipAddressRange = getIpAddressRange(vlan); + Pair ipAddressRange = NuageVspUtil.getIpAddressRange(vlan); long startIp = NetUtils.ip2Long(ipAddressRange.getLeft()); long endIp = NetUtils.ip2Long(ipAddressRange.getRight()); return startIp <= ip && ip <= endIp; } - private List> getSharedIpAddressRanges(long networkId) { - List vlans = _vlanDao.listVlansByNetworkId(networkId); - List> ipAddressRanges = Lists.newArrayList(); - for (VlanVO vlan : vlans) { - Pair ipAddressRange = getIpAddressRange(vlan); - if (ipAddressRange != null) { - ipAddressRanges.add(ipAddressRange); - } - } - return ipAddressRanges; + private String getVirtualRouterIP(Network network) { + return network.getBroadcastUri() != null ? network.getBroadcastUri().getPath().substring(1) : null; } - private List> getIpAddressRanges(Network network) { - List> ipAddressRanges = Lists.newArrayList(); - String subnet = NetUtils.getCidrSubNet(network.getCidr()); - String netmask = NetUtils.getCidrNetmask(network.getCidr()); - long cidrSize = NetUtils.getCidrSize(netmask); - Set allIPsInCidr = NetUtils.getAllIpsFromCidr(subnet, cidrSize, new HashSet()); - if (allIPsInCidr == null || !(allIPsInCidr instanceof TreeSet)) { - throw new IllegalStateException("The IPs in CIDR for subnet " + subnet + " where null or returned in a non-ordered set."); - } - - Iterator ipIterator = allIPsInCidr.iterator(); - long ip = ipIterator.next(); - long gatewayIp = NetUtils.ip2Long(network.getGateway()); - String lastIp = NetUtils.getIpRangeEndIpFromCidr(subnet, cidrSize); - if (gatewayIp == ip) { - ip = ipIterator.next(); - ipAddressRanges.add(Pair.of(NetUtils.long2Ip(ip), lastIp)); - } else if (!network.getGateway().equals(lastIp)) { - ipAddressRanges.add(Pair.of(NetUtils.long2Ip(ip), NetUtils.long2Ip(gatewayIp - 1))); - ipAddressRanges.add(Pair.of(NetUtils.long2Ip(gatewayIp + 1), lastIp)); - } else { - ipAddressRanges.add(Pair.of(NetUtils.long2Ip(ip), NetUtils.long2Ip(gatewayIp - 1))); - } - - return ipAddressRanges; - } - - public Pair getIpAddressRange(Vlan vlan) { - boolean isIpv4 = StringUtils.isNotBlank(vlan.getIpRange()); - String[] range = isIpv4 ? vlan.getIpRange().split("-") : vlan.getIp6Range().split("-"); - if (range.length == 2) { - return Pair.of(range[0], range[1]); - } - return null; - } - - private String getVirtualRouterIP(Network network, List> ipAddressRanges, boolean recalculateBroadcastUri) throws InsufficientVirtualNetworkCapacityException { - //Add a check to see if a VR should be present in the offering or not? - if (!recalculateBroadcastUri && network.getBroadcastUri() != null) { - return network.getBroadcastUri().getPath().substring(1); - } - ensureIpCapacity(network, ipAddressRanges); - - if(network.getGuestType() == Network.GuestType.Shared) { - return ipAddressRanges.stream() - .sorted(Comparator.comparingLong(p -> NetUtils.ip2Long(p.getLeft()))) - .findFirst() - .map(Pair::getLeft) - .orElseThrow(() -> new IllegalArgumentException("Shared network without ip ranges? How can this happen?")); - } - - Pair lowestIpAddressRange = null; - long ipCount = 0; - if (ipAddressRanges.size() == 1) { - lowestIpAddressRange = Iterables.getOnlyElement(ipAddressRanges); - ipCount = NetUtils.ip2Long(lowestIpAddressRange.getRight()) - NetUtils.ip2Long(lowestIpAddressRange.getLeft()) + 1; - } else { - for (Pair ipAddressRange : ipAddressRanges) { - if (lowestIpAddressRange == null || NetUtils.ip2Long(ipAddressRange.getLeft()) < NetUtils.ip2Long(lowestIpAddressRange.getLeft())) { - lowestIpAddressRange = ipAddressRange; - } - ipCount += NetUtils.ip2Long(ipAddressRange.getRight()) - NetUtils.ip2Long(ipAddressRange.getLeft()) + 1; - } - } - - - Network networkToCheck; - if (isMigratingNetwork(network)) { - networkToCheck = _networkDao.findById(network.getRelated()); - } else { - networkToCheck = network; - } - - Long freeIp = _networkModel.getAvailableIps(networkToCheck, null) - .stream() - .findFirst() - .orElseThrow(() -> new InsufficientVirtualNetworkCapacityException("There is no free ip available for the VirtualRouter.", - Network.class, - network.getId())); - - return NetUtils.long2Ip(freeIp); - } - - private boolean isMigratingNetwork(Network network) { - return network.getRelated() != network.getId(); - } - - private void ensureIpCapacity(Network network, List> ipAddressRanges) throws InsufficientVirtualNetworkCapacityException { - long ipCount = ipAddressRanges.stream() - .mapToLong(this::getIpCount) - .sum(); - - if (ipCount == 0) { - throw new InsufficientVirtualNetworkCapacityException("VSP allocates an IP for VirtualRouter." + " But no ip address ranges are specified", Network.class, - network.getId()); - } else if (ipCount < 3) { - throw new InsufficientVirtualNetworkCapacityException("VSP allocates an IP for VirtualRouter." + " So, subnet should have atleast minimum 3 hosts", Network.class, - network.getId()); - } - } - - private long getIpCount(Pair ipAddressRange) { - return NetUtils.ip2Long(ipAddressRange.getRight()) - NetUtils.ip2Long(ipAddressRange.getLeft()) + 1; - } public VspVm buildVspVm(VirtualMachine vm, Network network) { VspVm.Builder vspVmBuilder = new VspVm.Builder() diff --git a/plugins/network-elements/nuage-vsp/src/com/cloud/util/NuageVspUtil.java b/plugins/network-elements/nuage-vsp/src/com/cloud/util/NuageVspUtil.java index 16d28b64aec..e5266d0ff15 100644 --- a/plugins/network-elements/nuage-vsp/src/com/cloud/util/NuageVspUtil.java +++ b/plugins/network-elements/nuage-vsp/src/com/cloud/util/NuageVspUtil.java @@ -24,6 +24,8 @@ import com.cloud.dc.VlanDetailsVO; import com.cloud.dc.dao.VlanDetailsDao; import com.cloud.network.manager.NuageVspManager; import com.cloud.utils.StringUtils; + +import net.nuage.vsp.acs.client.common.model.Pair; import org.apache.commons.codec.binary.Base64; public class NuageVspUtil { @@ -44,4 +46,13 @@ public class NuageVspUtil { VlanDetailsVO nuageUnderlayDetail = vlanDetailsDao.findDetail(vlan.getId(), NuageVspManager.nuageUnderlayVlanIpRangeDetailKey); return nuageUnderlayDetail != null && nuageUnderlayDetail.getValue().equalsIgnoreCase(String.valueOf(true)); } + + public static Pair getIpAddressRange(Vlan vlan) { + boolean isIpv4 = StringUtils.isNotBlank(vlan.getIpRange()); + String[] range = isIpv4 ? vlan.getIpRange().split("-") : vlan.getIp6Range().split("-"); + if (range.length == 2) { + return Pair.of(range[0], range[1]); + } + return null; + } } diff --git a/plugins/network-elements/nuage-vsp/test/com/cloud/NuageTest.java b/plugins/network-elements/nuage-vsp/test/com/cloud/NuageTest.java index ae383bfea0b..02497994570 100644 --- a/plugins/network-elements/nuage-vsp/test/com/cloud/NuageTest.java +++ b/plugins/network-elements/nuage-vsp/test/com/cloud/NuageTest.java @@ -92,9 +92,7 @@ public class NuageTest { VspNetwork vspNetwork = buildVspNetwork(); when(_nuageVspEntityBuilder.buildVspNetwork(any(Network.class))).thenReturn(vspNetwork); - when(_nuageVspEntityBuilder.buildVspNetwork(any(Network.class), anyBoolean())).thenReturn(vspNetwork); when(_nuageVspEntityBuilder.buildVspNetwork(anyLong(), any(Network.class))).thenReturn(vspNetwork); - when(_nuageVspEntityBuilder.buildVspNetwork(anyLong(), any(Network.class), anyBoolean())).thenReturn(vspNetwork); when(_nuageVspEntityBuilder.buildVspVm(any(VirtualMachine.class), any(Network.class))).thenReturn(buildVspVm()); diff --git a/plugins/network-elements/nuage-vsp/test/com/cloud/network/element/NuageVspElementTest.java b/plugins/network-elements/nuage-vsp/test/com/cloud/network/element/NuageVspElementTest.java index 46046fd9643..e48d9a46ced 100644 --- a/plugins/network-elements/nuage-vsp/test/com/cloud/network/element/NuageVspElementTest.java +++ b/plugins/network-elements/nuage-vsp/test/com/cloud/network/element/NuageVspElementTest.java @@ -26,6 +26,8 @@ import java.util.Arrays; import java.util.Collections; import java.util.Set; +import com.cloud.network.dao.NetworkDao; +import com.cloud.network.dao.NetworkVO; import com.cloud.tags.dao.ResourceTagDao; import org.junit.Before; import org.junit.Test; @@ -60,9 +62,7 @@ import com.cloud.network.NuageVspDeviceVO; import com.cloud.network.dao.FirewallRulesDao; import com.cloud.network.dao.IPAddressDao; import com.cloud.network.dao.IPAddressVO; -import com.cloud.network.dao.NetworkDao; import com.cloud.network.dao.NetworkServiceMapDao; -import com.cloud.network.dao.NetworkVO; import com.cloud.network.dao.NuageVspDao; import com.cloud.network.dao.PhysicalNetworkDao; import com.cloud.network.dao.PhysicalNetworkVO; @@ -89,6 +89,7 @@ import static org.mockito.Matchers.any; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.mockito.Mockito.withSettings; public class NuageVspElementTest extends NuageTest { @@ -119,6 +120,7 @@ public class NuageVspElementTest extends NuageTest { _nuageVspElement._nuageVspEntityBuilder = _nuageVspEntityBuilder; _nuageVspElement._vpcDetailsDao = _vpcDetailsDao; _nuageVspElement._routerDao = _domainRouterDao; + _nuageVspElement._networkDao = _networkDao; when(_networkServiceMapDao.canProviderSupportServiceInNetwork(NETWORK_ID, Service.Connectivity, Provider.NuageVsp)).thenReturn(true); when(_networkServiceMapDao.canProviderSupportServiceInNetwork(NETWORK_ID, Service.SourceNat, Provider.NuageVsp)).thenReturn(true); @@ -166,7 +168,7 @@ public class NuageVspElementTest extends NuageTest { @Test public void testImplement() throws ConcurrentOperationException, ResourceUnavailableException, InsufficientCapacityException, URISyntaxException { - final Network network = mock(Network.class); + final Network network = mock(NetworkVO.class, withSettings().extraInterfaces(Network.class)); when(network.getBroadcastDomainType()).thenReturn(BroadcastDomainType.Vsp); when(network.getId()).thenReturn(NETWORK_ID); when(network.getVpcId()).thenReturn(null); @@ -209,6 +211,7 @@ public class NuageVspElementTest extends NuageTest { when(_firewallRulesDao.listByNetworkPurposeTrafficType(NETWORK_ID, FirewallRule.Purpose.Firewall, FirewallRule.TrafficType.Egress)).thenReturn(new ArrayList()); when(_ipAddressDao.listStaticNatPublicIps(NETWORK_ID)).thenReturn(new ArrayList()); when(_nuageVspManager.getDnsDetails(network.getDataCenterId())).thenReturn(new ArrayList()); + when(_networkDao.findById(network.getId())).thenReturn((NetworkVO)network); assertTrue(_nuageVspElement.implement(network, offering, deployDest, context)); } diff --git a/test/integration/plugins/nuagevsp/test_nuage_internal_dns.py b/test/integration/plugins/nuagevsp/test_nuage_internal_dns.py index b0026d7c77c..0c2a6b611ed 100644 --- a/test/integration/plugins/nuagevsp/test_nuage_internal_dns.py +++ b/test/integration/plugins/nuagevsp/test_nuage_internal_dns.py @@ -947,3 +947,63 @@ class TestNuageInternalDns(nuageTestCase): self.debug("excepted value found in vm: " + item) else: self.fail("excepted value not found in vm: " + item) + + @attr(tags=["advanced", "nuagevsp"], required_hardware="true") + def test_09_update_network_offering_isolated_network(self): + """Test Update network offering for isolated Networks + with Nuage VSP SDN plugin + """ + # Create an Isolated Network with Nuage VSP Isolated Network + # offering specifying Services which don't need a VR. + # Update the network offering of this network to one that + # needs a VR, check that a VR is spawn + # After that update network to previous offering + # Check that VR is destroyed and removed. + + self.debug("+++Create an Isolated network with a network " + "offering which has only services without VR") + cmd = updateZone.updateZoneCmd() + cmd.id = self.zone.id + cmd.domain = "isolated.com" + self.apiclient.updateZone(cmd) + self.debug("Creating and enabling Nuage Vsp Isolated Network " + "offering which has only service without VR...") + network_offering = self.create_NetworkOffering( + self.test_data["nuagevsp"] + ["isolated_network_offering_without_vr"]) + self.validate_NetworkOffering(network_offering, state="Enabled") + + network_1 = self.create_Network(network_offering) + self.validate_Network(network_1, state="Allocated") + + self.debug("+++Deploy VM in the created Isolated network " + "with only services without VR") + vm_1 = self.create_VM(network_1) + + # VSD verification + self.verify_vsd_network(self.domain.id, network_1) + self.verify_vsd_vm(vm_1) + + with self.assertRaises(Exception): + self.get_Router(network_1) + self.debug("+++Verified no VR is spawned for this network ") + + self.debug("+++ Upgrade offering of created Isolated network with " + "a dns offering which spins a VR") + self.upgrade_Network(self.test_data["nuagevsp"][ + "isolated_network_offering"], + network_1) + vr = self.get_Router(network_1) + self.check_Router_state(vr, state="Running") + # VSD verification + self.verify_vsd_network(self.domain.id, network_1) + self.verify_vsd_router(vr) + + self.debug("+++ Upgrade offering of created Isolated network with " + "an offering which removes the VR...") + self.upgrade_Network(self.test_data["nuagevsp"][ + "isolated_network_offering_without_vr"], + network_1) + with self.assertRaises(Exception): + self.get_Router(network_1) + self.debug("+++Verified no VR is spawned for this network ") From 38ccbfb79c0654495ab03d54f94d3cdc481ba7c6 Mon Sep 17 00:00:00 2001 From: Dingane Hlaluku Date: Sat, 10 Feb 2018 12:25:25 -0500 Subject: [PATCH 2/5] CLOUDSTACK-9663: updateRole cmd to return updated role as JSON (#2406) This fixes updateRole to return a role response, like other update APIs. Signed-off-by: Rohit Yadav --- .../api/command/admin/acl/RoleCmd.java | 36 ++++++++++ .../apache/cloudstack/acl/RoleService.java | 2 +- .../api/command/admin/acl/CreateRoleCmd.java | 12 +--- .../api/command/admin/acl/UpdateRoleCmd.java | 13 ++-- .../api/command/test/UpdateRoleCmdTest.java | 70 +++++++++++++++++++ .../cloudstack/acl/RoleManagerImpl.java | 10 +-- test/integration/smoke/test_dynamicroles.py | 9 ++- 7 files changed, 125 insertions(+), 27 deletions(-) create mode 100644 api/src/main/java/org/apache/cloudstack/api/command/admin/acl/RoleCmd.java create mode 100644 api/test/org/apache/cloudstack/api/command/test/UpdateRoleCmdTest.java diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/RoleCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/RoleCmd.java new file mode 100644 index 00000000000..9054ff5fada --- /dev/null +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/acl/RoleCmd.java @@ -0,0 +1,36 @@ +// 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 org.apache.cloudstack.api.command.admin.acl; + +import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.response.RoleResponse; + +public abstract class RoleCmd extends BaseCmd { + + protected void setupResponse(final Role role) { + final RoleResponse response = new RoleResponse(); + response.setId(role.getUuid()); + response.setRoleName(role.getName()); + response.setRoleType(role.getRoleType()); + response.setDescription(role.getDescription()); + response.setResponseName(getCommandName()); + response.setObjectName("role"); + setResponseObject(response); + } +} diff --git a/api/src/org/apache/cloudstack/acl/RoleService.java b/api/src/org/apache/cloudstack/acl/RoleService.java index 98d170b5853..721bef08c20 100644 --- a/api/src/org/apache/cloudstack/acl/RoleService.java +++ b/api/src/org/apache/cloudstack/acl/RoleService.java @@ -31,7 +31,7 @@ public interface RoleService { boolean isEnabled(); Role findRole(final Long id); Role createRole(final String name, final RoleType roleType, final String description); - boolean updateRole(final Role role, final String name, final RoleType roleType, final String description); + Role updateRole(final Role role, final String name, final RoleType roleType, final String description); boolean deleteRole(final Role role); RolePermission findRolePermission(final Long id); diff --git a/api/src/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java b/api/src/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java index 994573d1796..87f0288dd01 100644 --- a/api/src/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java +++ b/api/src/org/apache/cloudstack/api/command/admin/acl/CreateRoleCmd.java @@ -34,7 +34,7 @@ import org.apache.cloudstack.context.CallContext; requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.9.0", authorized = {RoleType.Admin}) -public class CreateRoleCmd extends BaseCmd { +public class CreateRoleCmd extends RoleCmd { public static final String APINAME = "createRole"; ///////////////////////////////////////////////////// @@ -83,16 +83,6 @@ public class CreateRoleCmd extends BaseCmd { return Account.ACCOUNT_ID_SYSTEM; } - private void setupResponse(final Role role) { - final RoleResponse response = new RoleResponse(); - response.setId(role.getUuid()); - response.setRoleName(role.getName()); - response.setRoleType(role.getRoleType()); - response.setResponseName(getCommandName()); - response.setObjectName("role"); - setResponseObject(response); - } - @Override public void execute() { CallContext.current().setEventDetails("Role: " + getRoleName() + ", type:" + getRoleType() + ", description: " + getRoleDescription()); diff --git a/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java b/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java index e17fc6f5714..f9519aeec5a 100644 --- a/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java +++ b/api/src/org/apache/cloudstack/api/command/admin/acl/UpdateRoleCmd.java @@ -22,21 +22,20 @@ import com.google.common.base.Strings; import org.apache.cloudstack.acl.Role; import org.apache.cloudstack.acl.RoleType; import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiArgValidator; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.ApiErrorCode; import org.apache.cloudstack.api.BaseCmd; import org.apache.cloudstack.api.Parameter; import org.apache.cloudstack.api.ServerApiException; -import org.apache.cloudstack.api.ApiArgValidator; import org.apache.cloudstack.api.response.RoleResponse; -import org.apache.cloudstack.api.response.SuccessResponse; import org.apache.cloudstack.context.CallContext; -@APICommand(name = UpdateRoleCmd.APINAME, description = "Updates a role", responseObject = SuccessResponse.class, +@APICommand(name = UpdateRoleCmd.APINAME, description = "Updates a role", responseObject = RoleResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.9.0", authorized = {RoleType.Admin}) -public class UpdateRoleCmd extends BaseCmd { +public class UpdateRoleCmd extends RoleCmd { public static final String APINAME = "updateRole"; ///////////////////////////////////////////////////// @@ -100,9 +99,7 @@ public class UpdateRoleCmd extends BaseCmd { throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Invalid role id provided"); } CallContext.current().setEventDetails("Role: " + getRoleName() + ", type:" + getRoleType() + ", description: " + getRoleDescription()); - boolean result = roleService.updateRole(role, getRoleName(), getRoleType(), getRoleDescription()); - SuccessResponse response = new SuccessResponse(getCommandName()); - response.setSuccess(result); - setResponseObject(response); + role = roleService.updateRole(role, getRoleName(), getRoleType(), getRoleDescription()); + setupResponse(role); } } diff --git a/api/test/org/apache/cloudstack/api/command/test/UpdateRoleCmdTest.java b/api/test/org/apache/cloudstack/api/command/test/UpdateRoleCmdTest.java new file mode 100644 index 00000000000..c0bd390c196 --- /dev/null +++ b/api/test/org/apache/cloudstack/api/command/test/UpdateRoleCmdTest.java @@ -0,0 +1,70 @@ +// 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 org.apache.cloudstack.api.command.test; + +import junit.framework.TestCase; +import org.apache.cloudstack.acl.Role; +import org.apache.cloudstack.acl.RoleService; +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.command.admin.acl.UpdateRoleCmd; +import org.apache.cloudstack.api.response.RoleResponse; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; +import org.springframework.test.util.ReflectionTestUtils; + +import static org.mockito.Mockito.when; + + +public class UpdateRoleCmdTest extends TestCase{ + + private UpdateRoleCmd updateRoleCmd; + private RoleService roleService; + private Role role; + + @Override + @Before + public void setUp() { + roleService = Mockito.spy(RoleService.class); + updateRoleCmd = new UpdateRoleCmd(); + ReflectionTestUtils.setField(updateRoleCmd,"roleService",roleService); + ReflectionTestUtils.setField(updateRoleCmd,"roleId",1L); + ReflectionTestUtils.setField(updateRoleCmd,"roleName","user"); + ReflectionTestUtils.setField(updateRoleCmd,"roleType", "User"); + ReflectionTestUtils.setField(updateRoleCmd,"roleDescription","Description Initial"); + role = Mockito.mock(Role.class); + } + + @Test + public void testUpdateSuccess() { + when(roleService.findRole(updateRoleCmd.getRoleId())).thenReturn(role); + when(role.getId()).thenReturn(1L); + when(role.getUuid()).thenReturn("12345-abcgdkajd"); + when(role.getDescription()).thenReturn("Defualt user"); + when(role.getName()).thenReturn("User"); + when(role.getRoleType()).thenReturn(RoleType.User); + when(roleService.updateRole(role,updateRoleCmd.getRoleName(),updateRoleCmd.getRoleType(),updateRoleCmd.getRoleDescription())).thenReturn(role); + when(role.getId()).thenReturn(1L); + when(role.getDescription()).thenReturn("Description Initial"); + when(role.getName()).thenReturn("User"); + updateRoleCmd.execute(); + RoleResponse response = (RoleResponse) updateRoleCmd.getResponseObject(); + assertEquals((String)ReflectionTestUtils.getField(response, "roleName"),role.getName()); + assertEquals((String)ReflectionTestUtils.getField(response, "roleDescription"),role.getDescription()); + } +} diff --git a/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java b/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java index 5557aff71e0..b1e13313117 100644 --- a/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java +++ b/server/src/org/apache/cloudstack/acl/RoleManagerImpl.java @@ -119,11 +119,9 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu @Override @ActionEvent(eventType = EventTypes.EVENT_ROLE_UPDATE, eventDescription = "updating Role") - public boolean updateRole(final Role role, final String name, final RoleType roleType, final String description) { + public Role updateRole(final Role role, final String name, final RoleType roleType, final String description) { checkCallerAccess(); - if (role == null) { - return false; - } + if (roleType != null && roleType == RoleType.Unknown) { throw new ServerApiException(ApiErrorCode.PARAM_ERROR, "Unknown is not a valid role type"); } @@ -145,7 +143,9 @@ public class RoleManagerImpl extends ManagerBase implements RoleService, Configu if (!Strings.isNullOrEmpty(description)) { roleVO.setDescription(description); } - return roleDao.update(role.getId(), roleVO); + + roleDao.update(role.getId(), roleVO); + return role; } @Override diff --git a/test/integration/smoke/test_dynamicroles.py b/test/integration/smoke/test_dynamicroles.py index 5d105f6a427..61e1d199741 100644 --- a/test/integration/smoke/test_dynamicroles.py +++ b/test/integration/smoke/test_dynamicroles.py @@ -209,7 +209,8 @@ class TestDynamicRoles(cloudstackTestCase): """ self.account.delete(self.apiclient) new_role_name = self.getRandomRoleName() - self.role.update(self.apiclient, name=new_role_name, type='Admin') + new_role_description = "Fake role description created after update" + self.role.update(self.apiclient, name=new_role_name, type='Admin', description=new_role_description) update_role = Role.list(self.apiclient, id=self.role.id)[0] self.assertEqual( update_role.name, @@ -221,7 +222,11 @@ class TestDynamicRoles(cloudstackTestCase): 'Admin', msg="Role type does not match updated role type" ) - + self.assertEqual( + update_role.description, + new_role_description, + msg="Role description does not match updated role description" + ) @attr(tags=['advanced', 'simulator', 'basic', 'sg'], required_hardware=False) def test_role_lifecycle_update_role_inuse(self): From ce67726c6d3db6e7db537e76da6217c5d5f4b10e Mon Sep 17 00:00:00 2001 From: Wido den Hollander Date: Sat, 10 Feb 2018 18:27:00 +0100 Subject: [PATCH 3/5] CLOUDSTACK-10243: Do not use wait() on Python subprocess (#2421) This might (and does block) in certain situations on the VR as also explained in the Python documentation: https://docs.python.org/2/library/subprocess.html#subprocess.Popen.wait Warning This will deadlock when using stdout=PIPE and/or stderr=PIPE and the child process generates enough output to a pipe such that it blocks waiting for the OS pipe buffer to accept more data. Use communicate() to avoid that. Using the check_output function handles most of this for us and also provides better error handling. Signed-off-by: Wido den Hollander --- systemvm/debian/opt/cloud/bin/cs/CsHelper.py | 18 ++++++++++++------ 1 file changed, 12 insertions(+), 6 deletions(-) diff --git a/systemvm/debian/opt/cloud/bin/cs/CsHelper.py b/systemvm/debian/opt/cloud/bin/cs/CsHelper.py index 5397038fd1e..241643d9956 100755 --- a/systemvm/debian/opt/cloud/bin/cs/CsHelper.py +++ b/systemvm/debian/opt/cloud/bin/cs/CsHelper.py @@ -183,13 +183,19 @@ def get_hostname(): def execute(command): """ Execute command """ - p = subprocess.Popen(command, stdout=subprocess.PIPE, stderr=subprocess.PIPE, shell=True) - p.wait() - rc = p.returncode + returncode = -1 + try: + logging.info("Executing: %s" % command) + result = subprocess.check_output(command, shell=True) + returncode = 0 + return result.splitlines() + except subprocess.CalledProcessError as e: + logging.error(e) + returncode = e.returncode + finally: + logging.debug("Executed: %s - exitstatus=%s " % (command, returncode)) - logging.debug("Executed: %s - exitstatus=%s " % (command, rc)) - result = p.communicate()[0] - return result.splitlines() + return list() def save_iptables(command, iptables_file): From 6e09529bdee0be84cb435beef0735444b3f6f692 Mon Sep 17 00:00:00 2001 From: Nicolas Vazquez Date: Sat, 10 Feb 2018 14:28:59 -0300 Subject: [PATCH 4/5] CLOUDSTACK-10251: HTTPS downloader for Direct Download templates failure (#2424) Failure on HTTPS downloader for Direct Download templates on KVM. Reason: Incorrect request caused NullPointerException getting the response InputStream --- .../HttpsDirectTemplateDownloader.java | 31 +++++++++++++++++-- 1 file changed, 29 insertions(+), 2 deletions(-) diff --git a/agent/src/com/cloud/agent/direct/download/HttpsDirectTemplateDownloader.java b/agent/src/com/cloud/agent/direct/download/HttpsDirectTemplateDownloader.java index 436303f71cf..664181fbda3 100644 --- a/agent/src/com/cloud/agent/direct/download/HttpsDirectTemplateDownloader.java +++ b/agent/src/com/cloud/agent/direct/download/HttpsDirectTemplateDownloader.java @@ -21,6 +21,9 @@ package com.cloud.agent.direct.download; import com.cloud.utils.exception.CloudRuntimeException; import com.cloud.utils.script.Script; +import org.apache.commons.io.IOUtils; +import org.apache.http.HttpEntity; +import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpGet; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.conn.ssl.SSLConnectionSocketFactory; @@ -33,6 +36,9 @@ import javax.net.ssl.SSLContext; import java.io.File; import java.io.FileInputStream; import java.io.IOException; +import java.io.InputStream; +import java.io.OutputStream; +import java.io.FileOutputStream; import java.security.KeyManagementException; import java.security.KeyStore; import java.security.KeyStoreException; @@ -79,11 +85,32 @@ public class HttpsDirectTemplateDownloader extends HttpDirectTemplateDownloader @Override public boolean downloadTemplate() { + CloseableHttpResponse response; try { - httpsClient.execute(req); + response = httpsClient.execute(req); } catch (IOException e) { throw new CloudRuntimeException("Error on HTTPS request: " + e.getMessage()); } - return performDownload(); + return consumeResponse(response); } + + /** + * Consume response and persist it on getDownloadedFilePath() file + */ + protected boolean consumeResponse(CloseableHttpResponse response) { + if (response.getStatusLine().getStatusCode() != 200) { + throw new CloudRuntimeException("Error on HTTPS response"); + } + try { + HttpEntity entity = response.getEntity(); + InputStream in = entity.getContent(); + OutputStream out = new FileOutputStream(getDownloadedFilePath()); + IOUtils.copy(in, out); + } catch (Exception e) { + s_logger.error("Error parsing response for template " + getTemplateId() + " due to: " + e.getMessage()); + return false; + } + return true; + } + } \ No newline at end of file From b2a19f7587fdd86fddc6c32fa8476eeac6269116 Mon Sep 17 00:00:00 2001 From: Nicolas Vazquez Date: Sat, 10 Feb 2018 14:29:41 -0300 Subject: [PATCH 5/5] CLOUDSTACK-10247: L2 network not shared on projects (#2420) When trying to deploy a vm providing a project id and a L2 network id, this error is logged. --- .../src/com/cloud/network/NetworkModelImpl.java | 17 +++++++++-------- .../com/cloud/network/NetworkServiceImpl.java | 4 ++-- 2 files changed, 11 insertions(+), 10 deletions(-) diff --git a/server/src/com/cloud/network/NetworkModelImpl.java b/server/src/com/cloud/network/NetworkModelImpl.java index e583b717a16..b8e7b53b1fa 100644 --- a/server/src/com/cloud/network/NetworkModelImpl.java +++ b/server/src/com/cloud/network/NetworkModelImpl.java @@ -930,7 +930,7 @@ public class NetworkModelImpl extends ManagerBase implements NetworkModel, Confi @Override public String getIpOfNetworkElementInVirtualNetwork(long accountId, long dataCenterId) { - List virtualNetworks = _networksDao.listByZoneAndGuestType(accountId, dataCenterId, Network.GuestType.Isolated, false); + List virtualNetworks = _networksDao.listByZoneAndGuestType(accountId, dataCenterId, GuestType.Isolated, false); if (virtualNetworks.isEmpty()) { s_logger.trace("Unable to find default Virtual network account id=" + accountId); @@ -950,13 +950,13 @@ public class NetworkModelImpl extends ManagerBase implements NetworkModel, Confi } @Override - public List listNetworksForAccount(long accountId, long zoneId, Network.GuestType type) { + public List listNetworksForAccount(long accountId, long zoneId, GuestType type) { List accountNetworks = new ArrayList(); List zoneNetworks = _networksDao.listByZone(zoneId); for (NetworkVO network : zoneNetworks) { if (!isNetworkSystem(network)) { - if (network.getGuestType() == Network.GuestType.Shared || !_networksDao.listBy(accountId, network.getId()).isEmpty()) { + if (network.getGuestType() == GuestType.Shared || !_networksDao.listBy(accountId, network.getId()).isEmpty()) { if (type == null || type == network.getGuestType()) { accountNetworks.add(network); } @@ -967,7 +967,7 @@ public class NetworkModelImpl extends ManagerBase implements NetworkModel, Confi } @Override - public List listAllNetworksInAllZonesByType(Network.GuestType type) { + public List listAllNetworksInAllZonesByType(GuestType type) { List networks = new ArrayList(); for (NetworkVO network : _networksDao.listAll()) { if (!isNetworkSystem(network)) { @@ -1637,7 +1637,8 @@ public class NetworkModelImpl extends ManagerBase implements NetworkModel, Confi throw new CloudRuntimeException("cannot check permissions on (Network) "); } // Perform account permission check - if (network.getGuestType() != Network.GuestType.Shared || (network.getGuestType() == Network.GuestType.Shared && network.getAclType() == ACLType.Account)) { + if ((network.getGuestType() != GuestType.Shared && network.getGuestType() != GuestType.L2) || + (network.getGuestType() == GuestType.Shared && network.getAclType() == ACLType.Account)) { AccountVO networkOwner = _accountDao.findById(network.getAccountId()); if (networkOwner == null) throw new PermissionDeniedException("Unable to use network with id= " + ((NetworkVO)network).getUuid() + @@ -1802,14 +1803,14 @@ public class NetworkModelImpl extends ManagerBase implements NetworkModel, Confi public boolean isNetworkAvailableInDomain(long networkId, long domainId) { Long networkDomainId = null; Network network = getNetwork(networkId); - if (network.getGuestType() != Network.GuestType.Shared) { - s_logger.trace("Network id=" + networkId + " is not shared"); + if (network.getGuestType() != GuestType.Shared && network.getGuestType() != GuestType.L2) { + s_logger.trace("Network id=" + networkId + " is not shared or L2"); return false; } NetworkDomainVO networkDomainMap = _networkDomainDao.getDomainNetworkMapByNetworkId(networkId); if (networkDomainMap == null) { - s_logger.trace("Network id=" + networkId + " is shared, but not domain specific"); + s_logger.trace("Network id=" + networkId + " is shared or L2, but not domain specific"); return true; } else { networkDomainId = networkDomainMap.getDomainId(); diff --git a/server/src/com/cloud/network/NetworkServiceImpl.java b/server/src/com/cloud/network/NetworkServiceImpl.java index d7ae6276f85..93f73d20a5f 100644 --- a/server/src/com/cloud/network/NetworkServiceImpl.java +++ b/server/src/com/cloud/network/NetworkServiceImpl.java @@ -1101,8 +1101,8 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService { } // Only Admin can create Shared networks - if (ntwkOff.getGuestType() == GuestType.Shared && !_accountMgr.isAdmin(caller.getId())) { - throw new InvalidParameterValueException("Only Admins can create network with guest type " + GuestType.Shared); + if ((ntwkOff.getGuestType() == GuestType.Shared || ntwkOff.getGuestType() == GuestType.L2) && !_accountMgr.isAdmin(caller.getId())) { + throw new InvalidParameterValueException("Only Admins can create network with guest type " + GuestType.Shared + " or " + GuestType.L2); } // Check if the network is domain specific