You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@cloudstack.apache.org by GitBox <gi...@apache.org> on 2021/12/06 16:52:45 UTC
[GitHub] [cloudstack] GutoVeronezi commented on a change in pull request #5730: Enhancement: create Shared networks and VPC private gateways by users
GutoVeronezi commented on a change in pull request #5730:
URL: https://github.com/apache/cloudstack/pull/5730#discussion_r763067267
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/admin/vpc/CreatePrivateGatewayByAdminCmd.java
##########
@@ -0,0 +1,76 @@
+// 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.ApiConstants;
+import org.apache.cloudstack.api.Parameter;
+import org.apache.cloudstack.api.ResponseObject.ResponseView;
+import org.apache.cloudstack.api.command.admin.AdminCmd;
+import org.apache.cloudstack.api.command.user.vpc.CreatePrivateGatewayCmd;
+import org.apache.cloudstack.api.response.PhysicalNetworkResponse;
+import org.apache.cloudstack.api.response.PrivateGatewayResponse;
+
+import com.cloud.network.vpc.VpcGateway;
+
+@APICommand(name = "createPrivateGateway", description = "Creates a private gateway",
+ responseObject = PrivateGatewayResponse.class,
+ responseView = ResponseView.Full,
+ entityType = {VpcGateway.class},
+ since = "4.17.0",
+ requestHasSensitiveInfo = false, responseHasSensitiveInfo = false)
+public class CreatePrivateGatewayByAdminCmd extends CreatePrivateGatewayCmd implements AdminCmd {
+ public static final Logger s_logger = Logger.getLogger(CreatePrivateGatewayByAdminCmd.class.getName());
+
+ /////////////////////////////////////////////////////
+ //////////////// API parameters /////////////////////
+ /////////////////////////////////////////////////////
+
+ @Parameter(name = ApiConstants.PHYSICAL_NETWORK_ID,
+ type = CommandType.UUID,
+ entityType = PhysicalNetworkResponse.class,
+ description = "the Physical Network ID the network belongs to")
+ private Long physicalNetworkId;
+
+ @Parameter(name = ApiConstants.VLAN, type = CommandType.STRING, description = "the network implementation uri for the private gateway")
+ private String broadcastUri;
+
+ @Parameter(name = ApiConstants.BYPASS_VLAN_OVERLAP_CHECK, type = CommandType.BOOLEAN, description = "when true bypasses VLAN id/range overlap check during private gateway creation")
+ private Boolean bypassVlanOverlapCheck;
+
+ /////////////////////////////////////////////////////
+ /////////////////// Accessors ///////////////////////
+ /////////////////////////////////////////////////////
+
+
+ public String getBroadcastUri() {
+ return broadcastUri;
+ }
+
+ public Long getPhysicalNetworkId() {
+ return physicalNetworkId;
+ }
+
+ public Boolean getBypassVlanOverlapCheck() {
+ if (bypassVlanOverlapCheck != null) {
+ return bypassVlanOverlapCheck;
+ }
+ return false;
+ }
Review comment:
We could use `org.apache.commons.lang3.BooleanUtils#toBoolean` here.
##########
File path: api/src/main/java/org/apache/cloudstack/api/command/user/network/ListNetworksCmd.java
##########
@@ -96,6 +96,13 @@
@Parameter(name = ApiConstants.NETWORK_OFFERING_ID, type = CommandType.UUID, entityType = NetworkOfferingResponse.class, description = "list networks by network offering ID")
private Long networkOfferingId;
+ @Parameter(name = ApiConstants.ASSOCIATED_NETWORK_ID,
+ type = CommandType.UUID,
+ entityType = NetworkResponse.class,
+ since = "4.17.0",
+ description = "List networks by associated networks. only available if create a Shared network")
Review comment:
```suggestion
description = "List networks by associated networks. Only available if create a Shared network.")
```
##########
File path: engine/orchestration/src/main/java/org/apache/cloudstack/engine/orchestration/NetworkOrchestrator.java
##########
@@ -2530,7 +2530,9 @@ private Network createGuestNetwork(final long networkOfferingId, final String na
final boolean vlanSpecified = vlanId != null;
if (vlanSpecified != ntwkOff.isSpecifyVlan()) {
if (vlanSpecified) {
- throw new InvalidParameterValueException("Can't specify vlan; corresponding offering says specifyVlan=false");
+ if (! isSharedNetworkWithoutSpecifyVlan(ntwkOff) && ! isPrivateGatewayWithoutSpecifyVlan(ntwkOff)) {
Review comment:
```suggestion
if (!isSharedNetworkWithoutSpecifyVlan(ntwkOff) && !isPrivateGatewayWithoutSpecifyVlan(ntwkOff)) {
```
##########
File path: api/src/main/java/org/apache/cloudstack/api/response/GuestVlanResponse.java
##########
@@ -0,0 +1,156 @@
+// 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.response;
+
+import com.google.gson.annotations.SerializedName;
+
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.api.BaseResponse;
+
+import com.cloud.serializer.Param;
+
+import java.util.Date;
+import java.util.List;
+
+@SuppressWarnings("unused")
+public class GuestVlanResponse extends BaseResponse implements ControlledEntityResponse {
+
+ @SerializedName(ApiConstants.ID)
+ @Param(description = "the guest VLAN id")
+ private long id;
+
+ @SerializedName(ApiConstants.VLAN)
+ @Param(description = "the guest VLAN")
+ private String guestVlan;
+
+ @SerializedName(ApiConstants.ACCOUNT)
+ @Param(description = "the account of the guest VLAN range")
+ private String accountName;
+
+ @SerializedName(ApiConstants.DOMAIN_ID)
+ @Param(description = "the domain ID of the guest VLAN range")
+ private String domainId;
+
+ @SerializedName(ApiConstants.DOMAIN)
+ @Param(description = "the domain name of the guest VLAN range")
+ private String domainName;
+
+ @SerializedName(ApiConstants.PROJECT_ID)
+ @Param(description = "the project id of the guest vlan range")
+ private String projectId;
+
+ @SerializedName(ApiConstants.PROJECT)
+ @Param(description = "the project name of the guest vlan range")
+ private String projectName;
+
+ @SerializedName(ApiConstants.ZONE_ID)
+ @Param(description = "the zone ID of the guest vlan range")
+ private String zoneId;
+
+ @SerializedName(ApiConstants.ZONE_NAME)
+ @Param(description = "the zone name of the guest vlan range")
+ private String zoneName;
+
+ @SerializedName(ApiConstants.PHYSICAL_NETWORK_ID)
+ @Param(description = "the physical network ID of the guest vlan range")
+ private String physicalNetworkId;
+
+ @SerializedName(ApiConstants.PHYSICAL_NETWORK_NAME)
+ @Param(description = "the physical network name of the guest vlan range")
+ private String physicalNetworkName;
+
+ @SerializedName(ApiConstants.IS_DEDICATED)
+ @Param(description = "true if the guest vlan is dedicated to the account")
+ private Boolean isDedicated;
+
+ @SerializedName(ApiConstants.ALLOCATION_STATE)
+ @Param(description = "the allocation state of the guest vlan")
+ private String allocationState;
+
+ @SerializedName(ApiConstants.TAKEN)
+ @Param(description = "date the guest vlan was taken")
+ private Date taken;
+
+ @SerializedName(ApiConstants.NETWORK)
+ @Param(description = "the list of networks who use this guest vlan", responseObject = NetworkResponse.class)
Review comment:
`vlan` -> `VLAN`
##########
File path: server/src/main/java/com/cloud/api/ApiResponseHelper.java
##########
@@ -2424,6 +2429,17 @@ public NetworkResponse createNetworkResponse(ResponseView view, Network network)
response.setVpcName(vpc.getName());
}
}
+
+ final NetworkDetailVO detail = networkDetailsDao.findDetail(network.getId(), Network.AssociatedNetworkId);
+ if (detail != null) {
+ Long associatedNetworkId = Long.valueOf(detail.getValue());
+ NetworkVO associatedNetwork = ApiDBUtils.findNetworkById(associatedNetworkId);
+ if (associatedNetwork != null) {
+ response.setAssociatedNetworkId(associatedNetwork.getUuid());
+ response.setAssociatedNetworkName(associatedNetwork.getName());
+ }
+ }
+
Review comment:
We could invert this `if` statement and add a return, to reduce code indentation.
##########
File path: engine/schema/src/main/java/com/cloud/vm/dao/DomainRouterDaoImpl.java
##########
@@ -376,7 +376,8 @@ public DomainRouterVO persist(final DomainRouterVO router, final List<Network> g
public void addRouterToGuestNetwork(final VirtualRouter router, final Network guestNetwork) {
if (_routerNetworkDao.findByRouterAndNetwork(router.getId(), guestNetwork.getId()) == null) {
final NetworkOffering off = _offDao.findById(guestNetwork.getNetworkOfferingId());
- if (!off.getName().equalsIgnoreCase(NetworkOffering.SystemPrivateGatewayNetworkOffering)) {
+ if (!off.getName().equalsIgnoreCase(NetworkOffering.SystemPrivateGatewayNetworkOffering)
+ && !off.getName().equalsIgnoreCase(NetworkOffering.SystemPrivateGatewayNetworkOfferingWithoutVlan)) {
Review comment:
We could use `org.apache.commons.lang3.StringUtils#equalsAnyIgnoreCase` here.
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -4377,6 +4320,73 @@ public Vlan createVlanAndPublicIpRange(final long zoneId, final long networkId,
return vlan;
}
+ private void checkZoneVlanIpOverlap(DataCenterVO zone, Network network, String newCidr, String vlanId, String vlanGateway, String vlanNetmask, String startIP, String endIP) {
+ // Throw an exception if this subnet overlaps with subnet on other VLAN,
+ // if this is ip range extension, gateway, network mask should be same and ip range should not overlap
+
+ final List<VlanVO> vlans = _vlanDao.listByZone(zone.getId());
+ for (final VlanVO vlan : vlans) {
+ final String otherVlanGateway = vlan.getVlanGateway();
+ final String otherVlanNetmask = vlan.getVlanNetmask();
+ // Continue if it's not IPv4
+ if ( otherVlanGateway == null || otherVlanNetmask == null ) {
+ continue;
+ }
+ if ( vlan.getNetworkId() == null ) {
+ continue;
+ }
Review comment:
We could use `org.apache.commons.lang3.ObjectUtils#anyNull` here.
##########
File path: server/src/main/java/com/cloud/configuration/ConfigurationManagerImpl.java
##########
@@ -4377,6 +4320,73 @@ public Vlan createVlanAndPublicIpRange(final long zoneId, final long networkId,
return vlan;
}
+ private void checkZoneVlanIpOverlap(DataCenterVO zone, Network network, String newCidr, String vlanId, String vlanGateway, String vlanNetmask, String startIP, String endIP) {
+ // Throw an exception if this subnet overlaps with subnet on other VLAN,
+ // if this is ip range extension, gateway, network mask should be same and ip range should not overlap
+
+ final List<VlanVO> vlans = _vlanDao.listByZone(zone.getId());
+ for (final VlanVO vlan : vlans) {
+ final String otherVlanGateway = vlan.getVlanGateway();
+ final String otherVlanNetmask = vlan.getVlanNetmask();
+ // Continue if it's not IPv4
+ if ( otherVlanGateway == null || otherVlanNetmask == null ) {
+ continue;
+ }
+ if ( vlan.getNetworkId() == null ) {
+ continue;
+ }
+ final String otherCidr = NetUtils.getCidrFromGatewayAndNetmask(otherVlanGateway, otherVlanNetmask);
+ if( !NetUtils.isNetworksOverlap(newCidr, otherCidr)) {
+ continue;
+ }
+ // from here, subnet overlaps
+ if (vlanId.toLowerCase().contains(Vlan.UNTAGGED) || UriUtils.checkVlanUriOverlap(
+ BroadcastDomainType.getValue(BroadcastDomainType.fromString(vlanId)),
+ BroadcastDomainType.getValue(BroadcastDomainType.fromString(vlan.getVlanTag())))) {
+ // For untagged VLAN Id and overlapping URIs we need to expand and verify IP ranges
+ final String[] otherVlanIpRange = vlan.getIpRange().split("\\-");
+ final String otherVlanStartIP = otherVlanIpRange[0];
+ String otherVlanEndIP = null;
+ if (otherVlanIpRange.length > 1) {
+ otherVlanEndIP = otherVlanIpRange[1];
+ }
+
+ // extend IP range
+ if (!vlanGateway.equals(otherVlanGateway) || !vlanNetmask.equals(vlan.getVlanNetmask())) {
+ throw new InvalidParameterValueException("The IP range has already been added with gateway "
+ + otherVlanGateway + " ,and netmask " + otherVlanNetmask
+ + ", Please specify the gateway/netmask if you want to extend ip range" );
+ }
+ if (!NetUtils.is31PrefixCidr(newCidr)) {
+ if (NetUtils.ipRangesOverlap(startIP, endIP, otherVlanStartIP, otherVlanEndIP)) {
Review comment:
We could join these 2 `if` statement.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@cloudstack.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org