[GitHub] rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range.
rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range. URL: https://github.com/apache/cloudstack/pull/2048#discussion_r148809315 ## File path: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ## @@ -1097,8 +1095,243 @@ public void doInTransactionWithoutResult(final TransactionStatus status) { } @Override +@DB +public Pod createPodIpRange(final CreateManagementNetworkIpRangeCmd cmd) { + +//Check if calling account is root admin. +final Account account = CallContext.current().getCallingAccount(); + +if(!_accountMgr.isRootAdmin(account.getId())) { +throw new PermissionDeniedException("Cannot perform this operation, Calling account is not root admin: " + account.getId()); +} + +final long podId = cmd.getPodId(); +final String gateway = cmd.getGateWay(); +final String netmask = cmd.getNetmask(); +final String startIp = cmd.getStartIp(); +String endIp = cmd.getEndIp(); + +final HostPodVO pod = _podDao.findById(podId); + +if(pod == null) { +throw new InvalidParameterValueException("Unable to find pod by ID: " + podId); +} + +final long zoneId = pod.getDataCenterId(); + +//Check if gateway is a valid IP address. Review comment: Remove comments, they tell what the code is trying to do (redundancy) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range.
rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range. URL: https://github.com/apache/cloudstack/pull/2048#discussion_r148808255 ## File path: api/src/org/apache/cloudstack/api/command/admin/network/CreateManagementNetworkIpRangeCmd.java ## @@ -0,0 +1,140 @@ +// 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.network; + +import org.apache.log4j.Logger; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.BaseAsyncCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.PodResponse; + +import com.cloud.dc.Pod; +import com.cloud.event.EventTypes; +import com.cloud.exception.ConcurrentOperationException; +import com.cloud.exception.InsufficientCapacityException; +import com.cloud.exception.ResourceAllocationException; +import com.cloud.exception.ResourceUnavailableException; +import com.cloud.user.Account; + +@APICommand(name = "createManagementNetworkIpRange", +description = "Creates a Management network IP range.", +responseObject = PodResponse.class, +since = "4.10.0.0", Review comment: Fix since to 4.11.0.0 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range.
rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range. URL: https://github.com/apache/cloudstack/pull/2048#discussion_r148808445 ## File path: api/src/org/apache/cloudstack/api/command/admin/network/CreateManagementNetworkIpRangeCmd.java ## @@ -0,0 +1,140 @@ +// 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.network; + +import org.apache.log4j.Logger; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.BaseAsyncCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.PodResponse; + +import com.cloud.dc.Pod; +import com.cloud.event.EventTypes; +import com.cloud.exception.ConcurrentOperationException; +import com.cloud.exception.InsufficientCapacityException; +import com.cloud.exception.ResourceAllocationException; +import com.cloud.exception.ResourceUnavailableException; +import com.cloud.user.Account; + +@APICommand(name = "createManagementNetworkIpRange", +description = "Creates a Management network IP range.", +responseObject = PodResponse.class, +since = "4.10.0.0", +requestHasSensitiveInfo = false, +responseHasSensitiveInfo = false, +authorized = {RoleType.Admin}) +public class CreateManagementNetworkIpRangeCmd extends BaseAsyncCmd { +public static final Logger s_logger = Logger.getLogger(CreateManagementNetworkIpRangeCmd.class); + +private static final String s_name = "createmanagementnetworkiprangeresponse"; Review comment: Please have a look at CreateRoleCmd and other modern API implementation patterns. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range.
rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range. URL: https://github.com/apache/cloudstack/pull/2048#discussion_r148808530 ## File path: api/src/org/apache/cloudstack/api/command/admin/network/DeleteManagementNetworkIpRangeCmd.java ## @@ -0,0 +1,125 @@ +// 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.network; + +import org.apache.log4j.Logger; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.BaseAsyncCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.response.PodResponse; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.SuccessResponse; + +import com.cloud.event.EventTypes; +import com.cloud.exception.ConcurrentOperationException; +import com.cloud.exception.ResourceUnavailableException; +import com.cloud.user.Account; + +@APICommand(name = "deleteManagementNetworkIpRange", +description = "Deletes a management network IP range. This action is only allowed when no IPs in this range are allocated.", +responseObject = SuccessResponse.class, +since = "4.10.0.0", Review comment: Fix the `since` to 4.11.0.0 This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range.
rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range. URL: https://github.com/apache/cloudstack/pull/2048#discussion_r148809158 ## File path: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ## @@ -997,7 +999,9 @@ private void checkPodAttributes(final long podId, final String podName, final lo checkIpRange(startIp, endIp, cidrAddress, cidrSize); // Check if the IP range overlaps with the public ip -checkOverlapPublicIpRange(zoneId, startIp, endIp); +if(startIp != null) { Review comment: Consider changing this to `Strings.isNullOrEmpty()` This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range.
rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range. URL: https://github.com/apache/cloudstack/pull/2048#discussion_r148809330 ## File path: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ## @@ -1097,8 +1095,243 @@ public void doInTransactionWithoutResult(final TransactionStatus status) { } @Override +@DB +public Pod createPodIpRange(final CreateManagementNetworkIpRangeCmd cmd) { + +//Check if calling account is root admin. +final Account account = CallContext.current().getCallingAccount(); + +if(!_accountMgr.isRootAdmin(account.getId())) { +throw new PermissionDeniedException("Cannot perform this operation, Calling account is not root admin: " + account.getId()); +} + +final long podId = cmd.getPodId(); +final String gateway = cmd.getGateWay(); +final String netmask = cmd.getNetmask(); +final String startIp = cmd.getStartIp(); +String endIp = cmd.getEndIp(); + +final HostPodVO pod = _podDao.findById(podId); + +if(pod == null) { +throw new InvalidParameterValueException("Unable to find pod by ID: " + podId); +} + +final long zoneId = pod.getDataCenterId(); + +//Check if gateway is a valid IP address. +if(!NetUtils.isValidIp(gateway)) { +throw new InvalidParameterValueException("The gateway IP address is invalid."); +} + +//Check if netmask is valid. Review comment: Remove comments, they tell what the code is trying to do (redundancy) This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range.
rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range. URL: https://github.com/apache/cloudstack/pull/2048#discussion_r148809473 ## File path: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ## @@ -1097,8 +1095,243 @@ public void doInTransactionWithoutResult(final TransactionStatus status) { } @Override +@DB +public Pod createPodIpRange(final CreateManagementNetworkIpRangeCmd cmd) { + +//Check if calling account is root admin. +final Account account = CallContext.current().getCallingAccount(); + +if(!_accountMgr.isRootAdmin(account.getId())) { +throw new PermissionDeniedException("Cannot perform this operation, Calling account is not root admin: " + account.getId()); +} + +final long podId = cmd.getPodId(); +final String gateway = cmd.getGateWay(); +final String netmask = cmd.getNetmask(); +final String startIp = cmd.getStartIp(); +String endIp = cmd.getEndIp(); + +final HostPodVO pod = _podDao.findById(podId); + +if(pod == null) { +throw new InvalidParameterValueException("Unable to find pod by ID: " + podId); +} + +final long zoneId = pod.getDataCenterId(); + +//Check if gateway is a valid IP address. +if(!NetUtils.isValidIp(gateway)) { +throw new InvalidParameterValueException("The gateway IP address is invalid."); +} + +//Check if netmask is valid. +if(!NetUtils.isValidNetmask(netmask)) { +throw new InvalidParameterValueException("The netmask IP address is invalid."); +} + +if(endIp == null) { +endIp = startIp; +} + +final String cidr = NetUtils.ipAndNetMaskToCidr(gateway, netmask); + +if(!NetUtils.isValidCIDR(cidr)) { +throw new InvalidParameterValueException("The CIDR is invalid " + cidr); +} + +final String cidrAddress = pod.getCidrAddress(); +final long cidrSize = pod.getCidrSize(); + +// Because each pod has only one Gateway and Netmask. +if (!gateway.equals(pod.getGateway())) { +throw new InvalidParameterValueException("Multiple gateways for the POD: " + pod.getId() + " are not allowed. The Gateway should be same as the existing Gateway " + pod.getGateway()); +} + +if (!netmask.equals(NetUtils.getCidrNetmask(cidrSize))) { +throw new InvalidParameterValueException("Multiple subnets for the POD: " + pod.getId() + " are not allowed. The Netmask should be same as the existing Netmask " + NetUtils.getCidrNetmask(cidrSize)); +} + +// Check if the IP range is valid. +checkIpRange(startIp, endIp, cidrAddress, cidrSize); + +// Check if the IP range overlaps with the public ip. +checkOverlapPublicIpRange(zoneId, startIp, endIp); + +// Check if the gateway is in the CIDR subnet +if (!NetUtils.getCidrSubNet(gateway, cidrSize).equalsIgnoreCase(NetUtils.getCidrSubNet(cidrAddress, cidrSize))) { +throw new InvalidParameterValueException("The gateway is not in the CIDR subnet."); +} + +if (NetUtils.ipRangesOverlap(startIp, endIp, gateway, gateway)) { +throw new InvalidParameterValueException("The gateway shouldn't overlap start/end ip addresses"); +} + +final String[] existingPodIpRanges = pod.getDescription().split(","); + +for(String podIpRange: existingPodIpRanges) { +final String[] existingPodIpRange = podIpRange.split("-"); + +if (existingPodIpRange.length > 1) { +if (!NetUtils.isValidIp(existingPodIpRange[0]) || !NetUtils.isValidIp(existingPodIpRange[1])) { +continue; +} +// Check if the range overlaps with any existing range. +if (NetUtils.ipRangesOverlap(startIp, endIp, existingPodIpRange[0], existingPodIpRange[1])) { +throw new InvalidParameterValueException("The new range overlaps with existing range. Please add a mutually exclusive range."); +} +} +} + +try { Review comment: Consider a `try-with-resource` syntax This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range.
rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range. URL: https://github.com/apache/cloudstack/pull/2048#discussion_r148808665 ## File path: api/src/org/apache/cloudstack/api/response/PodResponse.java ## @@ -55,11 +55,11 @@ @SerializedName("startip") @Param(description = "the starting IP for the Pod") -private String startIp; +private List startIp; Review comment: Do you think this breaks API response? This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range.
rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range. URL: https://github.com/apache/cloudstack/pull/2048#discussion_r148808571 ## File path: api/src/org/apache/cloudstack/api/command/admin/network/DeleteManagementNetworkIpRangeCmd.java ## @@ -0,0 +1,125 @@ +// 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.network; + +import org.apache.log4j.Logger; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.BaseAsyncCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.response.PodResponse; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.SuccessResponse; + +import com.cloud.event.EventTypes; +import com.cloud.exception.ConcurrentOperationException; +import com.cloud.exception.ResourceUnavailableException; +import com.cloud.user.Account; + +@APICommand(name = "deleteManagementNetworkIpRange", +description = "Deletes a management network IP range. This action is only allowed when no IPs in this range are allocated.", +responseObject = SuccessResponse.class, +since = "4.10.0.0", +requestHasSensitiveInfo = false, +responseHasSensitiveInfo = false, +authorized = {RoleType.Admin}) +public class DeleteManagementNetworkIpRangeCmd extends BaseAsyncCmd { +public static final Logger s_logger = Logger.getLogger(DeleteManagementNetworkIpRangeCmd.class); + +private static final String s_name = "deletemanagementnetworkiprangeresponse"; Review comment: Please have a look at CreateRoleCmd and other modern API implementation patterns. This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: us...@infra.apache.org With regards, Apache Git Services
[GitHub] rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range.
rhtyd commented on a change in pull request #2048: CLOUDSTACK-9880: Expansion of Management IP Range. URL: https://github.com/apache/cloudstack/pull/2048#discussion_r148809594 ## File path: server/src/com/cloud/configuration/ConfigurationManagerImpl.java ## @@ -1097,8 +1095,243 @@ public void doInTransactionWithoutResult(final TransactionStatus status) { } @Override +@DB +public Pod createPodIpRange(final CreateManagementNetworkIpRangeCmd cmd) { + +//Check if calling account is root admin. +final Account account = CallContext.current().getCallingAccount(); + +if(!_accountMgr.isRootAdmin(account.getId())) { +throw new PermissionDeniedException("Cannot perform this operation, Calling account is not root admin: " + account.getId()); +} + +final long podId = cmd.getPodId(); +final String gateway = cmd.getGateWay(); +final String netmask = cmd.getNetmask(); +final String startIp = cmd.getStartIp(); +String endIp = cmd.getEndIp(); + +final HostPodVO pod = _podDao.findById(podId); + +if(pod == null) { +throw new InvalidParameterValueException("Unable to find pod by ID: " + podId); +} + +final long zoneId = pod.getDataCenterId(); + +//Check if gateway is a valid IP address. +if(!NetUtils.isValidIp(gateway)) { +throw new InvalidParameterValueException("The gateway IP address is invalid."); +} + +//Check if netmask is valid. +if(!NetUtils.isValidNetmask(netmask)) { +throw new InvalidParameterValueException("The netmask IP address is invalid."); +} + +if(endIp == null) { +endIp = startIp; +} + +final String cidr = NetUtils.ipAndNetMaskToCidr(gateway, netmask); + +if(!NetUtils.isValidCIDR(cidr)) { +throw new InvalidParameterValueException("The CIDR is invalid " + cidr); +} + +final String cidrAddress = pod.getCidrAddress(); +final long cidrSize = pod.getCidrSize(); + +// Because each pod has only one Gateway and Netmask. +if (!gateway.equals(pod.getGateway())) { +throw new InvalidParameterValueException("Multiple gateways for the POD: " + pod.getId() + " are not allowed. The Gateway should be same as the existing Gateway " + pod.getGateway()); +} + +if (!netmask.equals(NetUtils.getCidrNetmask(cidrSize))) { +throw new InvalidParameterValueException("Multiple subnets for the POD: " + pod.getId() + " are not allowed. The Netmask should be same as the existing Netmask " + NetUtils.getCidrNetmask(cidrSize)); +} + +// Check if the IP range is valid. +checkIpRange(startIp, endIp, cidrAddress, cidrSize); + +// Check if the IP range overlaps with the public ip. +checkOverlapPublicIpRange(zoneId, startIp, endIp); + +// Check if the gateway is in the CIDR subnet +if (!NetUtils.getCidrSubNet(gateway, cidrSize).equalsIgnoreCase(NetUtils.getCidrSubNet(cidrAddress, cidrSize))) { +throw new InvalidParameterValueException("The gateway is not in the CIDR subnet."); +} + +if (NetUtils.ipRangesOverlap(startIp, endIp, gateway, gateway)) { +throw new InvalidParameterValueException("The gateway shouldn't overlap start/end ip addresses"); +} + +final String[] existingPodIpRanges = pod.getDescription().split(","); + +for(String podIpRange: existingPodIpRanges) { +final String[] existingPodIpRange = podIpRange.split("-"); + +if (existingPodIpRange.length > 1) { +if (!NetUtils.isValidIp(existingPodIpRange[0]) || !NetUtils.isValidIp(existingPodIpRange[1])) { +continue; +} +// Check if the range overlaps with any existing range. +if (NetUtils.ipRangesOverlap(startIp, endIp, existingPodIpRange[0], existingPodIpRange[1])) { +throw new InvalidParameterValueException("The new range overlaps with existing range. Please add a mutually exclusive range."); +} +} +} + +try { +final String endIpFinal = endIp; + +Transaction.execute(new TransactionCallbackNoReturn() { +@Override +public void doInTransactionWithoutResult(final TransactionStatus status) { +String ipRange = pod.getDescription(); + +if(ipRange != null && !ipRange.isEmpty()) +ipRange += ("," + startIp + "-" + endIpFinal); +else +ipRange = (startIp + "-" + endIpFinal); + +pod.setDescription(ipRange); + +HostPodVO lock = null; +try { Review comment: Consider a `try-with-resource` syntax