DaanHoogland commented on code in PR #7378: URL: https://github.com/apache/cloudstack/pull/7378#discussion_r1180080244
########## api/src/main/java/org/apache/cloudstack/api/command/user/address/RemoveQuarantinedIpCmd.java: ########## @@ -0,0 +1,64 @@ +// 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.user.address; + +import com.cloud.network.PublicIpQuarantine; +import com.cloud.user.Account; +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.response.IpQuarantineResponse; +import org.apache.cloudstack.api.response.SuccessResponse; + +@APICommand(name = "removeQuarantinedIp", responseObject = IpQuarantineResponse.class, description = "Removes a public IP address from quarantine. Only IPs in active " + + "quarantine can be removed.", + since = "4.18.0", entityType = {PublicIpQuarantine.class}, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, Review Comment: ```suggestion since = "4.19.0", entityType = {PublicIpQuarantine.class}, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, ``` ########## api/src/main/java/org/apache/cloudstack/api/command/user/address/ListQuarantinedIpsCmd.java: ########## @@ -0,0 +1,51 @@ +// 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.user.address; + +import com.cloud.network.PublicIpQuarantine; +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.BaseListCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.response.IpQuarantineResponse; +import org.apache.cloudstack.api.response.ListResponse; + +@APICommand(name = "listQuarantinedIps", responseObject = IpQuarantineResponse.class, description = "List public IP addresses in quarantine.", since = "4.18.0", Review Comment: ```suggestion @APICommand(name = "listQuarantinedIps", responseObject = IpQuarantineResponse.class, description = "List public IP addresses in quarantine.", since = "4.19.0", ``` ########## api/src/main/java/org/apache/cloudstack/api/command/user/address/UpdateQuarantinedIpCmd.java: ########## @@ -0,0 +1,67 @@ +// 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.user.address; + +import com.cloud.network.PublicIpQuarantine; +import com.cloud.user.Account; +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.BaseCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.IpQuarantineResponse; + +import java.util.Date; + +@APICommand(name = "updateQuarantinedIp", responseObject = IpQuarantineResponse.class, description = "Updates the quarantine end date for the given public IP address.", + since = "4.18.0", entityType = {PublicIpQuarantine.class}, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, Review Comment: ```suggestion since = "4.19.0", entityType = {PublicIpQuarantine.class}, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, ``` ########## engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java: ########## @@ -233,4 +235,52 @@ List<IPAddressVO> listAvailablePublicIps(final long dcId, public static final String MESSAGE_ASSIGN_IPADDR_EVENT = "Message.AssignIpAddr.Event"; public static final String MESSAGE_RELEASE_IPADDR_EVENT = "Message.ReleaseIpAddr.Event"; + + + /** + * Checks if the given public IP address is not in active quarantine. + * It returns `true` if: + * <ul> + * <li>The IP was never in quarantine;</li> + * <li>The IP was in quarantine, but the quarantine expired;</li> + * <li>The IP is still in quarantine; however, the new owner is the same as the previous owner, therefore, the IP can be allocated.</li> + * </ul> + * + * It returns `false` if: + * <ul> + * <li>The IP is in active quarantine and the new owner is different from the previous owner.</li> + * </ul> + * + * @param ip used to check if it is in active quarantine. + * @param account used to identify the new owner of the public IP. + * @return true if the IP can be allocated, and false otherwise. + */ + boolean checkIfPublicIpAddressIsNotInQuarantineAndCanBeAllocated(IpAddress ip, Account account); + + /** + * Adds the given public IP address to quarantine for the duration of the global configuration `public.ip.address.quarantine.duration` value. + * + * @param publicIpAddress to be quarantined. + * @return the {@link PublicIpQuarantine} persisted in the database. + */ + PublicIpQuarantine addPublicIpAddressToQuarantine(IpAddress publicIpAddress); + + /** + * Prematurely removes a public IP address from quarantine. It is required to provide a reason for removing it. + * + * @param quarantineProcessId the ID of the active quarantine process. + * @param removalReason for prematurely removing the public IP address from quarantine. + */ + void removePublicIpAddressFromQuarantine(Long quarantineProcessId, String removalReason); + + /** + * Updates the end date of a public IP address in active quarantine. It can increase and decrease the duration of the quarantine. + * + * @param quarantineProcessId the ID of the quarantine process. + * @param endDate the new end date for the quarantine. + * @return the updated quarantine object. + */ + PublicIpQuarantine updatePublicIpAddressInQuarantine(Long quarantineProcessId, Date endDate); + + void buildQuarantineSearchCriteria(SearchCriteria<IPAddressVO> sc); Review Comment: why is this one on the interface? doesn't feel right. ########## server/src/main/java/com/cloud/network/IpAddressManagerImpl.java: ########## @@ -2358,4 +2383,109 @@ public boolean isUsageHidden(IPAddressVO ip) { public static ConfigKey<Boolean> getSystemvmpublicipreservationmodestrictness() { return SystemVmPublicIpReservationModeStrictness; } + + @Override + public boolean checkIfPublicIpAddressIsNotInQuarantineAndCanBeAllocated(IpAddress ip, Account newOwner) { Review Comment: the name suggest a validation/check. I think ```suggestion public boolean canPublicIpAddressBeAllocated(IpAddress ip, Account newOwner) { ``` ########## engine/components-api/src/main/java/com/cloud/network/IpAddressManager.java: ########## @@ -233,4 +235,52 @@ List<IPAddressVO> listAvailablePublicIps(final long dcId, public static final String MESSAGE_ASSIGN_IPADDR_EVENT = "Message.AssignIpAddr.Event"; public static final String MESSAGE_RELEASE_IPADDR_EVENT = "Message.ReleaseIpAddr.Event"; + + + /** + * Checks if the given public IP address is not in active quarantine. + * It returns `true` if: + * <ul> + * <li>The IP was never in quarantine;</li> + * <li>The IP was in quarantine, but the quarantine expired;</li> + * <li>The IP is still in quarantine; however, the new owner is the same as the previous owner, therefore, the IP can be allocated.</li> + * </ul> + * + * It returns `false` if: + * <ul> + * <li>The IP is in active quarantine and the new owner is different from the previous owner.</li> + * </ul> + * + * @param ip used to check if it is in active quarantine. + * @param account used to identify the new owner of the public IP. + * @return true if the IP can be allocated, and false otherwise. + */ + boolean checkIfPublicIpAddressIsNotInQuarantineAndCanBeAllocated(IpAddress ip, Account account); Review Comment: as commented in the implementation: ```suggestion boolean canPublicIpAddressBeAllocated(IpAddress ip, Account account); ``` ########## engine/schema/src/main/resources/META-INF/db/schema-41800to41810.sql: ########## Review Comment: I comment on the `@since` in the command classes but see here that you want this in 4.18.1. We'll have to decide on this. I think 4.19 is best but if 4.18.1 the `Cmd` classes, and the target branch need to be adjusted accordingly. ########## server/src/main/java/com/cloud/network/IpAddressManagerImpl.java: ########## @@ -971,6 +988,13 @@ public List<IPAddressVO> doInTransaction(TransactionStatus status) throws Insuff if (lockOneRow) { assert (addrs.size() == 1) : "Return size is incorrect: " + addrs.size(); + IpAddress ipAddress = addrs.get(0); + boolean ipCanBeAllocated = checkIfPublicIpAddressIsNotInQuarantineAndCanBeAllocated(ipAddress, owner); + + if (!ipCanBeAllocated) { + throw new InsufficientAddressCapacityException(String.format("Failed to allocate public IP address [%s] as it is in quarantine.", ipAddress.getAddress()), + DataCenter.class, dcId); + } Review Comment: I think it would be best to remove/replace the assert and extract this to a private method ########## server/src/main/java/com/cloud/network/IpAddressManagerImpl.java: ########## @@ -2358,4 +2383,109 @@ public boolean isUsageHidden(IPAddressVO ip) { public static ConfigKey<Boolean> getSystemvmpublicipreservationmodestrictness() { return SystemVmPublicIpReservationModeStrictness; } + + @Override + public boolean checkIfPublicIpAddressIsNotInQuarantineAndCanBeAllocated(IpAddress ip, Account newOwner) { + PublicIpQuarantineVO publicIpQuarantineVO = publicIpQuarantineDao.findByPublicIpAddressId(ip.getId()); + + if (publicIpQuarantineVO == null) { + s_logger.debug(String.format("Public IP address [%s] is not in quarantine; therefore, it is allowed to be allocated.", ip)); + return true; + } + + if (!isPublicIpAddressStillInQuarantine(publicIpQuarantineVO, new Date())) { + s_logger.debug(String.format("Public IP address [%s] is no longer in quarantine; therefore, it is allowed to be allocated.", ip)); + return true; + } + + Account previousOwner = _accountMgr.getAccount(publicIpQuarantineVO.getPreviousOwnerId()); + + if (Objects.equals(previousOwner.getUuid(), newOwner.getUuid())) { + s_logger.debug(String.format("Public IP address [%s] is in quarantine; however, the Public IP previous owner [%s] is the same as the new owner [%s]; therefore the IP" + + " can be allocated. The public IP address will be removed from quarantine.", ip, previousOwner, newOwner)); + removePublicIpAddressFromQuarantine(publicIpQuarantineVO.getId(), "IP was removed from quarantine because it has been allocated by the previous owner"); + return true; + } + + s_logger.error(String.format("Public IP address [%s] is in quarantine and the previous owner [%s] is different than the new owner [%s]; therefore, the IP cannot be " + + "allocated.", ip, previousOwner, newOwner)); + return false; + } + + public boolean isPublicIpAddressStillInQuarantine(PublicIpQuarantineVO publicIpQuarantineVO, Date currentDate) { + Date quarantineEndDate = publicIpQuarantineVO.getEndDate(); + Date removedDate = publicIpQuarantineVO.getRemoved(); + boolean hasQuarantineEndedEarly = removedDate != null; + + return hasQuarantineEndedEarly && currentDate.before(removedDate) || + !hasQuarantineEndedEarly && currentDate.before(quarantineEndDate); + } + + @Override + public PublicIpQuarantine addPublicIpAddressToQuarantine(IpAddress publicIpAddress) { + Integer quarantineDuration = PUBLIC_IP_ADDRESS_QUARANTINE_DURATION.value(); + if (quarantineDuration <= 0) { + s_logger.debug(String.format("Not adding IP [%s] to quarantine because configuration [%s] has value equal or less to 0.", publicIpAddress.getAddress(), + PUBLIC_IP_ADDRESS_QUARANTINE_DURATION.key())); + return null; + } + + long ipId = publicIpAddress.getId(); + long accountId = publicIpAddress.getAccountId(); + + if (accountId == Account.ACCOUNT_ID_SYSTEM) { + s_logger.debug(String.format("Not adding IP [%s] to quarantine because it belongs to the system account.", publicIpAddress.getAddress())); + return null; + } + + Date currentDate = new Date(); + Calendar quarantineEndDate = Calendar.getInstance(); + quarantineEndDate.setTime(currentDate); + quarantineEndDate.add(Calendar.HOUR, quarantineDuration); + + PublicIpQuarantineVO publicIpQuarantine = new PublicIpQuarantineVO(ipId, accountId, currentDate, quarantineEndDate.getTime()); + s_logger.debug(String.format("Adding public IP Address [%s] to quarantine for the duration of [%s] hour(s).", publicIpAddress.getAddress(), quarantineDuration)); + return publicIpQuarantineDao.persist(publicIpQuarantine); + } + + @Override + public void removePublicIpAddressFromQuarantine(Long quarantineProcessId, String removalReason) { + PublicIpQuarantineVO publicIpQuarantineVO = publicIpQuarantineDao.findById(quarantineProcessId); + Ip ipAddress = _ipAddressDao.findById(publicIpQuarantineVO.getPublicIpAddressId()).getAddress(); + Date removedDate = new Date(); + + publicIpQuarantineVO.setRemoved(removedDate); + publicIpQuarantineVO.setRemovalReason(removalReason); + + s_logger.debug(String.format("Removing public IP Address [%s] from quarantine by updating the removed date to [%s].", ipAddress, removedDate)); + publicIpQuarantineDao.persist(publicIpQuarantineVO); + } + + @Override + public PublicIpQuarantine updatePublicIpAddressInQuarantine(Long quarantineProcessId, Date newEndDate) { + PublicIpQuarantineVO publicIpQuarantineVO = publicIpQuarantineDao.findById(quarantineProcessId); + Ip ipAddress = _ipAddressDao.findById(publicIpQuarantineVO.getPublicIpAddressId()).getAddress(); + Date currentEndDate = publicIpQuarantineVO.getEndDate(); + + publicIpQuarantineVO.setEndDate(newEndDate); + + s_logger.debug(String.format("Updating the end date for the quarantine of the public IP Address [%s] from [%s] to [%s].", ipAddress, currentEndDate, newEndDate)); + publicIpQuarantineDao.persist(publicIpQuarantineVO); + return publicIpQuarantineVO; + } + + @Override + public void buildQuarantineSearchCriteria(SearchCriteria<IPAddressVO> sc) { Review Comment: I think this should be in the IpAddressDaoImpl -- 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: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
