From e333f2705aa39007683fca83edac704410e72ca1 Mon Sep 17 00:00:00 2001 From: Wei Zhou Date: Mon, 9 Oct 2023 09:41:44 +0200 Subject: [PATCH] user-shared networks: fix few issues (#6887) This PR fixes few issues: - check ip range of new network instead of network cidr, so that the two networks can use same cidr but no IP conflicts. - Private gateways: return vlan number only for root admins - when update isolated network, check new guest vm cidr and IPs of neworks/vpc gateways associated to it --- .../cloudstack/api/ResponseGenerator.java | 2 +- .../vpc/ListPrivateGatewaysCmdByAdminCmd.java | 35 +++++++++++ .../user/vpc/CreatePrivateGatewayCmd.java | 2 +- .../user/vpc/ListPrivateGatewaysCmd.java | 7 ++- .../java/com/cloud/api/ApiResponseHelper.java | 6 +- .../com/cloud/network/NetworkServiceImpl.java | 61 +++++++++++++------ .../cloud/server/ManagementServerImpl.java | 2 + .../component/test_ip_reservation.py | 6 +- 8 files changed, 93 insertions(+), 28 deletions(-) create mode 100644 api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/ListPrivateGatewaysCmdByAdminCmd.java diff --git a/api/src/main/java/org/apache/cloudstack/api/ResponseGenerator.java b/api/src/main/java/org/apache/cloudstack/api/ResponseGenerator.java index 88b3571385a..1a0df486298 100644 --- a/api/src/main/java/org/apache/cloudstack/api/ResponseGenerator.java +++ b/api/src/main/java/org/apache/cloudstack/api/ResponseGenerator.java @@ -438,7 +438,7 @@ public interface ResponseGenerator { * @param result * @return */ - PrivateGatewayResponse createPrivateGatewayResponse(PrivateGateway result); + PrivateGatewayResponse createPrivateGatewayResponse(ResponseView view, PrivateGateway result); /** * @param result diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/ListPrivateGatewaysCmdByAdminCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/ListPrivateGatewaysCmdByAdminCmd.java new file mode 100644 index 00000000000..13a63e9cdd8 --- /dev/null +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/ListPrivateGatewaysCmdByAdminCmd.java @@ -0,0 +1,35 @@ +// 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.vpc; + +import org.apache.log4j.Logger; + +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ResponseObject; +import org.apache.cloudstack.api.command.admin.AdminCmd; +import org.apache.cloudstack.api.command.user.vpc.ListPrivateGatewaysCmd; +import org.apache.cloudstack.api.response.PrivateGatewayResponse; + +import com.cloud.network.vpc.VpcGateway; + +@APICommand(name = "listPrivateGateways", description = "List private gateways", responseObject = PrivateGatewayResponse.class, entityType = {VpcGateway.class}, + responseView = ResponseObject.ResponseView.Full, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) +public class ListPrivateGatewaysCmdByAdminCmd extends ListPrivateGatewaysCmd implements AdminCmd { + public static final Logger s_logger = Logger.getLogger(ListPrivateGatewaysCmdByAdminCmd.class.getName()); + +} diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vpc/CreatePrivateGatewayCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vpc/CreatePrivateGatewayCmd.java index 8527bd0e7fb..cf1315c9d55 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/vpc/CreatePrivateGatewayCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/vpc/CreatePrivateGatewayCmd.java @@ -169,7 +169,7 @@ public class CreatePrivateGatewayCmd extends BaseAsyncCreateCmd implements UserC public void execute() throws InsufficientCapacityException, ConcurrentOperationException, ResourceAllocationException, ResourceUnavailableException { PrivateGateway result = _vpcService.applyVpcPrivateGateway(getEntityId(), true); if (result != null) { - PrivateGatewayResponse response = _responseGenerator.createPrivateGatewayResponse(result); + PrivateGatewayResponse response = _responseGenerator.createPrivateGatewayResponse(getResponseView(), result); response.setResponseName(getCommandName()); setResponseObject(response); } else { diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/vpc/ListPrivateGatewaysCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/vpc/ListPrivateGatewaysCmd.java index 47861c8362e..8813cccc779 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/vpc/ListPrivateGatewaysCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/vpc/ListPrivateGatewaysCmd.java @@ -25,6 +25,8 @@ import org.apache.cloudstack.api.APICommand; import org.apache.cloudstack.api.ApiConstants; import org.apache.cloudstack.api.BaseListProjectAndAccountResourcesCmd; import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ResponseObject; +import org.apache.cloudstack.api.command.user.UserCmd; import org.apache.cloudstack.api.response.ListResponse; import org.apache.cloudstack.api.response.PrivateGatewayResponse; import org.apache.cloudstack.api.response.VpcResponse; @@ -34,8 +36,9 @@ import com.cloud.network.vpc.VpcGateway; import com.cloud.utils.Pair; @APICommand(name = "listPrivateGateways", description = "List private gateways", responseObject = PrivateGatewayResponse.class, entityType = {VpcGateway.class}, + responseView = ResponseObject.ResponseView.Restricted, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false) -public class ListPrivateGatewaysCmd extends BaseListProjectAndAccountResourcesCmd { +public class ListPrivateGatewaysCmd extends BaseListProjectAndAccountResourcesCmd implements UserCmd { public static final Logger s_logger = Logger.getLogger(ListPrivateGatewaysCmd.class.getName()); @@ -90,7 +93,7 @@ public class ListPrivateGatewaysCmd extends BaseListProjectAndAccountResourcesCm ListResponse response = new ListResponse(); List projectResponses = new ArrayList(); for (PrivateGateway gateway : gateways.first()) { - PrivateGatewayResponse gatewayResponse = _responseGenerator.createPrivateGatewayResponse(gateway); + PrivateGatewayResponse gatewayResponse = _responseGenerator.createPrivateGatewayResponse(getResponseView(), gateway); projectResponses.add(gatewayResponse); } response.setResponses(projectResponses, gateways.second()); diff --git a/server/src/main/java/com/cloud/api/ApiResponseHelper.java b/server/src/main/java/com/cloud/api/ApiResponseHelper.java index 1eb9dd84259..7ed31e21bc3 100644 --- a/server/src/main/java/com/cloud/api/ApiResponseHelper.java +++ b/server/src/main/java/com/cloud/api/ApiResponseHelper.java @@ -3357,10 +3357,12 @@ public class ApiResponseHelper implements ResponseGenerator { } @Override - public PrivateGatewayResponse createPrivateGatewayResponse(PrivateGateway result) { + public PrivateGatewayResponse createPrivateGatewayResponse(ResponseView view, PrivateGateway result) { PrivateGatewayResponse response = new PrivateGatewayResponse(); response.setId(result.getUuid()); - response.setBroadcastUri(result.getBroadcastUri()); + if (view == ResponseView.Full) { + response.setBroadcastUri(result.getBroadcastUri()); + } response.setGateway(result.getGateway()); response.setNetmask(result.getNetmask()); if (result.getVpcId() != null) { diff --git a/server/src/main/java/com/cloud/network/NetworkServiceImpl.java b/server/src/main/java/com/cloud/network/NetworkServiceImpl.java index 7b160416547..4d439ceda43 100644 --- a/server/src/main/java/com/cloud/network/NetworkServiceImpl.java +++ b/server/src/main/java/com/cloud/network/NetworkServiceImpl.java @@ -178,11 +178,13 @@ import com.cloud.network.security.SecurityGroupService; import com.cloud.network.vpc.NetworkACL; import com.cloud.network.vpc.PrivateIpVO; import com.cloud.network.vpc.Vpc; +import com.cloud.network.vpc.VpcGatewayVO; import com.cloud.network.vpc.VpcManager; import com.cloud.network.vpc.VpcVO; import com.cloud.network.vpc.dao.NetworkACLDao; import com.cloud.network.vpc.dao.PrivateIpDao; import com.cloud.network.vpc.dao.VpcDao; +import com.cloud.network.vpc.dao.VpcGatewayDao; import com.cloud.network.vpc.dao.VpcOfferingDao; import com.cloud.offering.NetworkOffering; import com.cloud.offerings.NetworkOfferingVO; @@ -388,6 +390,8 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService, C @Inject Ipv6GuestPrefixSubnetNetworkMapDao ipv6GuestPrefixSubnetNetworkMapDao; @Inject + VpcGatewayDao vpcGatewayDao; + @Inject AlertManager alertManager; @Inject DomainRouterDao routerDao; @@ -1939,24 +1943,17 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService, C if (domainId != null && associatedNetwork.getDomainId() != domainId) { throw new InvalidParameterValueException("The new network and associated network MUST be in same domain"); } - if (cidr != null && associatedNetwork.getCidr() != null && NetUtils.isNetworksOverlap(cidr, associatedNetwork.getCidr())) { - throw new InvalidParameterValueException("The cidr overlaps with associated network: " + associatedNetwork.getName()); - } - List associatedNetworks = _networkDetailsDao.findDetails(Network.AssociatedNetworkId, String.valueOf(associatedNetworkId), null); - for (NetworkDetailVO networkDetailVO : associatedNetworks) { - NetworkVO associatedNetwork2 = _networksDao.findById(networkDetailVO.getResourceId()); - if (associatedNetwork2 != null) { - List vlans = _vlanDao.listVlansByNetworkId(associatedNetwork2.getId()); - if (vlans.isEmpty()) { - continue; - } - String startIP2 = vlans.get(0).getIpRange().split("-")[0]; - String endIP2 = vlans.get(0).getIpRange().split("-")[1]; - if (StringUtils.isNoneBlank(startIp, startIP2) && NetUtils.ipRangesOverlap(startIp, endIp, startIP2, endIP2)) { - throw new InvalidParameterValueException("The startIp/endIp overlaps with network: " + associatedNetwork2.getName()); - } + if (cidr != null && associatedNetwork.getCidr() != null) { + String[] guestVmCidrPair = associatedNetwork.getCidr().split("\\/"); + String[] cidrIpRange = NetUtils.getIpRangeFromCidr(guestVmCidrPair[0], Long.valueOf(guestVmCidrPair[1])); + if (StringUtils.isNoneBlank(startIp, endIp) && NetUtils.ipRangesOverlap(startIp, endIp, cidrIpRange[0], cidrIpRange[1])) { + throw new InvalidParameterValueException(String.format("The IP range (%s-%s) overlaps with cidr of associated network: %s (%s)", + startIp, endIp, associatedNetwork.getName(), associatedNetwork.getCidr())); } } + // Check IP range overlap on shared networks and vpc private gateways associated to the same network + checkIpRangeOverlapWithAssociatedNetworks(associatedNetworkId, startIp, endIp); + associatedNetwork = implementedNetworkInCreation(caller, zone, associatedNetwork); if (associatedNetwork == null || (associatedNetwork.getState() != Network.State.Implemented && associatedNetwork.getState() != Network.State.Setup)) { throw new InvalidParameterValueException("Unable to implement associated network " + associatedNetwork); @@ -3065,8 +3062,9 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService, C if (networkOfferingChanged) { throw new InvalidParameterValueException("Cannot specify this network offering change and guestVmCidr at same time. Specify only one."); } - if (!(network.getState() == Network.State.Implemented)) { - throw new InvalidParameterValueException("The network must be in " + Network.State.Implemented + " state. IP Reservation cannot be applied in " + network.getState() + " state"); + if (network.getState() != Network.State.Implemented && network.getState() != Network.State.Allocated) { + throw new InvalidParameterValueException(String.format("The network must be in %s or %s state. IP Reservation cannot be applied in %s state", + Network.State.Implemented, Network.State.Allocated, network.getState())); } if (!NetUtils.isValidIp4Cidr(guestVmCidr)) { throw new InvalidParameterValueException("Invalid format of Guest VM CIDR."); @@ -3125,6 +3123,9 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService, C } } + // Check IP range overlap on shared networks and vpc private gateways associated to this network + checkIpRangeOverlapWithAssociatedNetworks(networkId, cidrIpRange[0], cidrIpRange[1]); + // When reservation is applied for the first time, network_cidr will be null // Populate it with the actual network cidr if (network.getNetworkCidr() == null) { @@ -5911,6 +5912,30 @@ public class NetworkServiceImpl extends ManagerBase implements NetworkService, C return accountIds; } + private void checkIpRangeOverlapWithAssociatedNetworks(Long associatedNetworkId, String startIp, String endIp) { + List associatedNetworks = _networkDetailsDao.findDetails(Network.AssociatedNetworkId, String.valueOf(associatedNetworkId), null); + for (NetworkDetailVO networkDetailVO : associatedNetworks) { + NetworkVO associatedNetwork2 = _networksDao.findById(networkDetailVO.getResourceId()); + if (associatedNetwork2 != null) { + List vlans = _vlanDao.listVlansByNetworkId(associatedNetwork2.getId()); + if (vlans.isEmpty()) { + VpcGatewayVO vpcGateway = vpcGatewayDao.getVpcGatewayByNetworkId(associatedNetwork2.getId()); + if (vpcGateway != null && NetUtils.ipRangesOverlap(startIp, endIp, vpcGateway.getIp4Address(), vpcGateway.getIp4Address())) { + throw new InvalidParameterValueException(String.format("The startIp/endIp (%s - %s) overlaps with vpc private gateway %s (%s): ", + startIp, endIp, associatedNetwork2.getName(), vpcGateway.getIp4Address())); + } + continue; + } + String startIP2 = vlans.get(0).getIpRange().split("-")[0]; + String endIP2 = vlans.get(0).getIpRange().split("-")[1]; + if (StringUtils.isNoneBlank(startIp, startIP2) && NetUtils.ipRangesOverlap(startIp, endIp, startIP2, endIP2)) { + throw new InvalidParameterValueException(String.format("The startIp/endIp (%s - %s) overlaps with network %s (%s - %s)", + startIp, endIp, associatedNetwork2.getName(), startIP2, endIP2)); + } + } + } + } + @Override public String getConfigComponentName() { return NetworkService.class.getSimpleName(); diff --git a/server/src/main/java/com/cloud/server/ManagementServerImpl.java b/server/src/main/java/com/cloud/server/ManagementServerImpl.java index 77a652d7690..3620e52d547 100644 --- a/server/src/main/java/com/cloud/server/ManagementServerImpl.java +++ b/server/src/main/java/com/cloud/server/ManagementServerImpl.java @@ -309,6 +309,7 @@ import org.apache.cloudstack.api.command.admin.vpc.CreateVPCCmdByAdmin; import org.apache.cloudstack.api.command.admin.vpc.CreateVPCOfferingCmd; import org.apache.cloudstack.api.command.admin.vpc.DeletePrivateGatewayCmd; import org.apache.cloudstack.api.command.admin.vpc.DeleteVPCOfferingCmd; +import org.apache.cloudstack.api.command.admin.vpc.ListPrivateGatewaysCmdByAdminCmd; import org.apache.cloudstack.api.command.admin.vpc.ListVPCsCmdByAdmin; import org.apache.cloudstack.api.command.admin.vpc.UpdateVPCCmdByAdmin; import org.apache.cloudstack.api.command.admin.vpc.UpdateVPCOfferingCmd; @@ -3834,6 +3835,7 @@ public class ManagementServerImpl extends ManagerBase implements ManagementServe cmdList.add(ListVPCsCmdByAdmin.class); cmdList.add(UpdateVPCCmdByAdmin.class); cmdList.add(CreatePrivateGatewayByAdminCmd.class); + cmdList.add(ListPrivateGatewaysCmdByAdminCmd.class); cmdList.add(UpdateLBStickinessPolicyCmd.class); cmdList.add(UpdateLBHealthCheckPolicyCmd.class); cmdList.add(GetUploadParamsForTemplateCmd.class); diff --git a/test/integration/component/test_ip_reservation.py b/test/integration/component/test_ip_reservation.py index 0ea01be4799..838aa30f7d0 100644 --- a/test/integration/component/test_ip_reservation.py +++ b/test/integration/component/test_ip_reservation.py @@ -1069,7 +1069,7 @@ class TestFailureScnarios(cloudstackTestCase): # 1. update guestvmcidr of isolated network (non persistent) # # validation - # should throw exception as network is not in implemented state as no vm is created + # should succeed although network is not in implemented state networkOffering = self.isolated_network_offering resultSet = createIsolatedNetwork(self, networkOffering.id) @@ -1078,9 +1078,7 @@ class TestFailureScnarios(cloudstackTestCase): else: isolated_network = resultSet[1] - with self.assertRaises(Exception): - isolated_network.update(self.apiclient, guestvmcidr="10.1.1.0/26") - return + isolated_network.update(self.apiclient, guestvmcidr="10.1.1.0/26") @attr(tags=["advanced"], required_hardware="false") def test_vm_create_after_reservation(self):