Copilot commented on code in PR #12737: URL: https://github.com/apache/cloudstack/pull/12737#discussion_r3181373194
########## api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsServerCmd.java: ########## @@ -0,0 +1,143 @@ +// 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.dns; + +import java.util.List; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.acl.SecurityChecker; +import org.apache.cloudstack.api.ACL; +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.DnsServerResponse; +import org.apache.cloudstack.dns.DnsServer; +import org.apache.commons.lang3.BooleanUtils; +import org.apache.commons.lang3.StringUtils; + +import com.cloud.user.Account; +import com.cloud.utils.EnumUtils; + +@APICommand(name = "updateDnsServer", + description = "Update DNS server", + responseObject = DnsServerResponse.class, + entityType = {DnsServer.class}, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, + since = "4.23.0", + authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) +public class UpdateDnsServerCmd extends BaseCmd { + + ///////////////////////////////////////////////////// + //////////////// API parameters ///////////////////// + ///////////////////////////////////////////////////// + + @ACL(accessType = SecurityChecker.AccessType.OperateEntry) + @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = DnsServerResponse.class, + required = true, description = "The ID of the DNS server to update") + private Long id; + + @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, description = "Name of the DNS server") + private String name; + + @Parameter(name = ApiConstants.URL, type = CommandType.STRING, description = "API URL of the provider") + private String url; + + @Parameter(name = ApiConstants.DNS_API_KEY, type = CommandType.STRING, description = "API Key or Credentials for the external provider") + private String dnsApiKey; + + @Parameter(name = ApiConstants.PORT, type = CommandType.INTEGER, description = "Port number of the external DNS server") + private Integer port; + + @Parameter(name = ApiConstants.IS_PUBLIC, type = CommandType.BOOLEAN, + description = "Whether this DNS server can be used by accounts other than the owner to create and manage DNS zones") + private Boolean isPublic; + + @Parameter(name = ApiConstants.PUBLIC_DOMAIN_SUFFIX, type = CommandType.STRING, + description = "Domain suffix that restricts DNS zones created by non-owner accounts to subdomains of this " + + "suffix (for example, sub.example.com under example.com)") + private String publicDomainSuffix; + + @Parameter(name = ApiConstants.NAME_SERVERS, type = CommandType.LIST, collectionType = CommandType.STRING, + description = "Comma separated list of name servers; used to create NS records for the DNS Zone (for example, ns1.example.com, ns2.example.com)") + private List<String> nameServers; + + @Parameter(name = ApiConstants.STATE, type = CommandType.STRING, description = "Update state for the DNS server (Enabled, Disabled)") + private String state; + + ///////////////////////////////////////////////////// + /////////////////// Accessors /////////////////////// + ///////////////////////////////////////////////////// + + public Long getId() { return id; } + public String getName() { return name; } + public String getUrl() { return url; } + public String getDnsApiKey() { + return dnsApiKey; + } + public Integer getPort() { + return port; + } + public Boolean isPublic() { + return BooleanUtils.isTrue(isPublic); + } + public String getPublicDomainSuffix() { + return publicDomainSuffix; + } + public String getNameServers() { return String.join(",", nameServers); } Review Comment: `nameServers` is an optional API parameter and can be null; `String.join` will throw a NullPointerException in that case. Return null/empty string when `nameServers` is null (and consider trimming/normalizing values) to keep updates safe for partial updates. ########## api/src/main/java/org/apache/cloudstack/api/command/user/dns/CreateDnsZoneCmd.java: ########## @@ -0,0 +1,154 @@ +// 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.dns; + +import java.util.Arrays; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.api.ACL; +import org.apache.cloudstack.api.APICommand; +import org.apache.cloudstack.api.ApiConstants; +import org.apache.cloudstack.api.ApiErrorCode; +import org.apache.cloudstack.api.BaseAsyncCreateCmd; +import org.apache.cloudstack.api.Parameter; +import org.apache.cloudstack.api.ServerApiException; +import org.apache.cloudstack.api.response.DnsServerResponse; +import org.apache.cloudstack.api.response.DnsZoneResponse; +import org.apache.cloudstack.context.CallContext; +import org.apache.cloudstack.dns.DnsZone; +import org.apache.commons.lang3.StringUtils; + +import com.cloud.event.EventTypes; +import com.cloud.exception.ResourceAllocationException; +import com.cloud.utils.EnumUtils; + +@APICommand(name = "createDnsZone", + description = "Creates a new DNS Zone on a specific server", + responseObject = DnsZoneResponse.class, + entityType = {DnsZone.class}, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, + since = "4.23.0", + authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) +public class CreateDnsZoneCmd extends BaseAsyncCreateCmd { + + ///////////////////////////////////////////////////// + //////////////// API Parameters ///////////////////// + ///////////////////////////////////////////////////// + + @Parameter(name = ApiConstants.NAME, type = CommandType.STRING, required = true, + description = "The name of the DNS zone (e.g. example.com)") + private String name; + + @ACL + @Parameter(name = ApiConstants.DNS_SERVER_ID, type = CommandType.UUID, entityType = DnsServerResponse.class, + required = true, description = "The ID of the DNS server to host this zone") + private Long dnsServerId; + + @Parameter(name = ApiConstants.TYPE, type = CommandType.STRING, + description = "The type of zone (Public, Private). Defaults to Public.") + private String type; + + @Parameter(name = ApiConstants.DESCRIPTION, type = CommandType.STRING, description = "The description of the DNS zone") + private String description; + + @Parameter(name = ApiConstants.EXISTING, type = CommandType.BOOLEAN, entityType = DnsZoneResponse.class, + description = "If true, imports an existing DNS zone from the DNS provider into CloudStack. " + + "If false, creates the zone in the DNS provider and registers it in CloudStack. Default is false") + private Boolean existing = false; + + ///////////////////////////////////////////////////// + /////////////////// Accessors /////////////////////// + ///////////////////////////////////////////////////// + + public String getName() { + return name; + } + + public Long getDnsServerId() { + return dnsServerId; + } + + public DnsZone.ZoneType getType() { + if (StringUtils.isBlank(type)) { + return DnsZone.ZoneType.Public; + } + DnsZone.ZoneType zoneType = EnumUtils.getEnumIgnoreCase(DnsZone.ZoneType.class, type); + if (type == null) { + throw new IllegalArgumentException("Invalid type value, supported values are: " + Arrays.toString(DnsZone.ZoneType.values())); Review Comment: The invalid-type check is incorrect: it checks `type == null` instead of `zoneType == null`. With a non-blank invalid value, `zoneType` becomes null and is returned, which can cause downstream NPEs. Change the condition to validate `zoneType` and throw a parameter exception for unsupported values. ########## api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsZoneCmd.java: ########## @@ -0,0 +1,87 @@ +// 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.dns; + +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.DnsZoneResponse; +import org.apache.cloudstack.context.CallContext; +import org.apache.cloudstack.dns.DnsZone; + +@APICommand(name = "updateDnsZone", + description = "Updates a DNS Zone's metadata", + responseObject = DnsZoneResponse.class, + entityType = {DnsZone.class}, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, + since = "4.23.0", + authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) +public class UpdateDnsZoneCmd extends BaseCmd { + + ///////////////////////////////////////////////////// + //////////////// API Parameters ///////////////////// + ///////////////////////////////////////////////////// + + @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = DnsZoneResponse.class, + required = true, description = "The ID of the DNS zone") + private Long id; Review Comment: Unlike other mutating DNS commands in this PR, this update command does not mark the `id` parameter with an ACL access type (e.g. `@ACL(accessType = SecurityChecker.AccessType.OperateEntry)`). This can bypass standard access enforcement for update operations. Add the appropriate `@ACL` annotation on the zone ID parameter (and required imports) to ensure authorization is applied consistently. ########## api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsZoneCmd.java: ########## @@ -0,0 +1,87 @@ +// 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.dns; + +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.DnsZoneResponse; +import org.apache.cloudstack.context.CallContext; +import org.apache.cloudstack.dns.DnsZone; + +@APICommand(name = "updateDnsZone", + description = "Updates a DNS Zone's metadata", + responseObject = DnsZoneResponse.class, + entityType = {DnsZone.class}, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, + since = "4.23.0", + authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) +public class UpdateDnsZoneCmd extends BaseCmd { + + ///////////////////////////////////////////////////// + //////////////// API Parameters ///////////////////// + ///////////////////////////////////////////////////// + + @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = DnsZoneResponse.class, + required = true, description = "The ID of the DNS zone") + private Long id; + + @Parameter(name = ApiConstants.DESCRIPTION, type = CommandType.STRING, description = "The description of the DNS zone to be updated") + private String description; + + ///////////////////////////////////////////////////// + /////////////////// Accessors /////////////////////// + ///////////////////////////////////////////////////// + + public String getDescription() { + return description; + } + + public Long getId() { + return id; + } + + ///////////////////////////////////////////////////// + /////////////// Implementation ////////////////////// + ///////////////////////////////////////////////////// + + @Override + public void execute() { + try { + DnsZone result = dnsProviderManager.updateDnsZone(this); + if (result != null) { + DnsZoneResponse response = dnsProviderManager.createDnsZoneResponse(result); + response.setResponseName(getCommandName()); + setResponseObject(response); + } else { + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to update DNS Zone on external provider"); + } + } catch (Exception e) { + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to update DNS Zone: " + e.getMessage()); + } + } + + @Override + public long getEntityOwnerId() { + return CallContext.current().getCallingAccount().getId(); + } Review Comment: For entity-bound update commands, `getEntityOwnerId()` should typically resolve the owner from the target entity (via `_entityMgr.findById`) rather than always using the calling account. As written, event ownership/auditing can be incorrect and may weaken permission patterns that rely on entity ownership. Align this with the other DNS commands by looking up the zone and returning its account ID (or system if not found). ########## api/src/main/java/com/cloud/user/ResourceLimitService.java: ########## @@ -57,6 +57,8 @@ public interface ResourceLimitService { "The default maximum number of GPU devices that can be used for a domain", false); static final ConfigKey<Long> DefaultMaxProjectGpus = new ConfigKey<>("Project Defaults",Long.class,"max.project.gpus","20", "The default maximum number of GPU devices that can be used for a project", false); + ConfigKey<Long> DefaultMaxDnsAccounts = new ConfigKey<>("Account Defaults",Long.class, "max.account.dns_zones","10", Review Comment: This config key name is misleading (`DefaultMaxDnsAccounts`) while the description/key indicate a DNS zones-per-account limit. Rename it to something like `DefaultMaxAccountDnsZones` (or similar to existing naming patterns), and keep style consistent with adjacent keys (they’re declared as `static final`). ########## api/src/main/java/org/apache/cloudstack/api/command/user/dns/DeleteDnsZoneCmd.java: ########## @@ -0,0 +1,109 @@ +// 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.dns; + +import org.apache.cloudstack.acl.RoleType; +import org.apache.cloudstack.acl.SecurityChecker; +import org.apache.cloudstack.api.ACL; +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.DnsZoneResponse; +import org.apache.cloudstack.api.response.SuccessResponse; +import org.apache.cloudstack.dns.DnsZone; + +import com.cloud.event.EventTypes; +import com.cloud.user.Account; + +@APICommand(name = "deleteDnsZone", + description = "Removes a DNS Zone from CloudStack and the external provider", + responseObject = SuccessResponse.class, + entityType = {DnsZone.class}, + requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, + since = "4.23.0", + authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) +public class DeleteDnsZoneCmd extends BaseAsyncCmd { + + ///////////////////////////////////////////////////// + //////////////// API Parameters ///////////////////// + ///////////////////////////////////////////////////// + + @ACL(accessType = SecurityChecker.AccessType.OperateEntry) + @Parameter(name = ApiConstants.ID, type = CommandType.UUID, entityType = DnsZoneResponse.class, required = true, + description = "The ID of the DNS zone") + private Long id; + + @Parameter(name = ApiConstants.UNMANAGE, type = CommandType.BOOLEAN, entityType = DnsZoneResponse.class, + description = "If true, imports an existing DNS zone from the DNS provider into CloudStack; " + + "if false, creates the DNS zone in the provider and registers it with CloudStack. Default: false") Review Comment: The `unmanage` parameter description does not match the command behavior (delete/unmanage). It currently describes import/create semantics (copied from create). Update the description to reflect delete behavior (e.g., true = remove only from CloudStack, false = delete from CloudStack and external provider). ########## api/src/main/java/com/cloud/event/EventTypes.java: ########## @@ -1406,6 +1420,15 @@ public class EventTypes { // Backup Repository entityEventDetails.put(EVENT_BACKUP_REPOSITORY_ADD, BackupRepositoryService.class); entityEventDetails.put(EVENT_BACKUP_REPOSITORY_UPDATE, BackupRepositoryService.class); + + // DNS Framework Events + entityEventDetails.put(EVENT_DNS_SERVER_ADD, DnsServer.class); + entityEventDetails.put(EVENT_DNS_SERVER_DELETE, DnsServer.class); + entityEventDetails.put(EVENT_DNS_ZONE_CREATE, DnsZone.class); Review Comment: DNS update events are defined (`EVENT_DNS_SERVER_UPDATE`, `EVENT_DNS_ZONE_UPDATE`) but not added to `entityEventDetails`. This can lead to missing entity metadata for update events in event/audit handling. Add mappings for the update event types to the appropriate entity classes. -- 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]
