From e7b7b4cbbb51d1dddf1e35f1e04471f6f2edc152 Mon Sep 17 00:00:00 2001 From: Manoj Kumar Date: Fri, 5 Jun 2026 11:28:23 +0530 Subject: [PATCH] UI/UX and improvements and bug fixes: - port validation added - NPE fixes - removed hard coded wait in integration test - DNSRecordsTab: client side pagination, column sorting and search - UX improvement for CreateDnsRecord, DeleteDnsZone and DeleteDnsServer - All DNS forms: wire @finish and htmltype --- .../api/command/user/dns/AddDnsServerCmd.java | 2 +- .../command/user/dns/CreateDnsRecordCmd.java | 2 +- .../command/user/dns/UpdateDnsServerCmd.java | 5 +- .../dns/DnsProviderManagerImpl.java | 40 +++- .../dns/DnsProviderManagerImplTest.java | 8 +- .../smoke/test_dns_framework_powerdns.py | 42 ++++- ui/public/locales/en.json | 5 + ui/src/views/network/dns/AddDnsServer.vue | 55 ++++-- ui/src/views/network/dns/AssociateDnsZone.vue | 5 +- ui/src/views/network/dns/CreateDnsRecord.vue | 30 +-- ui/src/views/network/dns/CreateDnsZone.vue | 20 +- ui/src/views/network/dns/DeleteDnsServer.vue | 59 +++--- ui/src/views/network/dns/DeleteDnsZone.vue | 60 +++--- ui/src/views/network/dns/DnsRecordsTab.vue | 174 +++++++++++------- ui/src/views/network/dns/UpdateDnsServer.vue | 35 ++-- ui/src/views/network/dns/UpdateDnsZone.vue | 26 ++- 16 files changed, 367 insertions(+), 201 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/AddDnsServerCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/AddDnsServerCmd.java index c84aa7cf455..298ddd64a31 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/AddDnsServerCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/AddDnsServerCmd.java @@ -43,7 +43,7 @@ import com.cloud.utils.EnumUtils; description = "Adds a new external DNS server", responseObject = DnsServerResponse.class, entityType = {DnsServer.class}, - requestHasSensitiveInfo = false, + requestHasSensitiveInfo = true, responseHasSensitiveInfo = false, since = "4.23.0", authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/CreateDnsRecordCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/CreateDnsRecordCmd.java index ff350ce065d..d2fe3a5024a 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/CreateDnsRecordCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/CreateDnsRecordCmd.java @@ -85,7 +85,7 @@ public class CreateDnsRecordCmd extends BaseAsyncCmd { response.setResponseName(getCommandName()); setResponseObject(response); } catch (Exception e) { - throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to create DNS Record: " + e.getMessage()); + throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, e.getMessage()); } } diff --git a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsServerCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsServerCmd.java index 53019d980c5..c0ecb283bc5 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsServerCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsServerCmd.java @@ -30,7 +30,6 @@ 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; @@ -40,7 +39,7 @@ import com.cloud.utils.EnumUtils; description = "Update DNS server", responseObject = DnsServerResponse.class, entityType = {DnsServer.class}, - requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, + requestHasSensitiveInfo = true, responseHasSensitiveInfo = false, since = "4.23.0", authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User}) public class UpdateDnsServerCmd extends BaseCmd { @@ -96,7 +95,7 @@ public class UpdateDnsServerCmd extends BaseCmd { return port; } public Boolean isPublic() { - return BooleanUtils.isTrue(isPublic); + return isPublic; } public String getPublicDomainSuffix() { return publicDomainSuffix; diff --git a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java index 27c971e7e44..d113f893366 100644 --- a/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java @@ -28,6 +28,7 @@ import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Objects; import java.util.Set; import java.util.stream.Collectors; @@ -77,6 +78,7 @@ import org.apache.cloudstack.framework.messagebus.MessageBus; import org.apache.cloudstack.framework.messagebus.MessageSubscriber; import org.apache.commons.collections.CollectionUtils; import org.apache.commons.collections.MapUtils; +import org.apache.commons.lang3.BooleanUtils; import org.springframework.stereotype.Component; import com.cloud.domain.Domain; @@ -172,8 +174,8 @@ public class DnsProviderManagerImpl extends ManagerBase implements DnsProviderMa boolean isDnsPublic = cmd.isPublic(); String publicDomainSuffix = cmd.getPublicDomainSuffix(); - if (caller.getType().equals(Account.Type.NORMAL)) { - logger.info("Only admin and domain admin users are allowed to configure a public DNS server"); + if (!accountMgr.isRootAdmin(caller.getId()) && !accountMgr.isDomainAdmin(caller.getId())) { + logger.info("Only root admin and domain admin users are allowed to configure a public DNS server"); isDnsPublic = false; publicDomainSuffix = null; } @@ -264,15 +266,21 @@ public class DnsProviderManagerImpl extends ManagerBase implements DnsProviderMa validationRequired = true; } - if (cmd.getPort() != null) { + if (cmd.getPort() != null && !Objects.equals(cmd.getPort(), dnsServer.getPort())) { dnsServer.setPort(cmd.getPort()); + validationRequired = true; } - if (cmd.isPublic() != null) { - dnsServer.setPublicServer(cmd.isPublic()); - } + if (accountMgr.isRootAdmin(caller.getId()) || accountMgr.isDomainAdmin(caller.getId())) { + if (cmd.isPublic() != null) { + boolean isPublic = BooleanUtils.isTrue(cmd.isPublic()); + dnsServer.setPublicServer(isPublic); - if (cmd.getPublicDomainSuffix() != null) { - dnsServer.setPublicDomainSuffix(DnsProviderUtil.normalizeDomainForDb(cmd.getPublicDomainSuffix())); + String publicDomainSuffix = null; + if (isPublic && StringUtils.isNotBlank(cmd.getPublicDomainSuffix())) { + publicDomainSuffix = DnsProviderUtil.normalizeDomainForDb(cmd.getPublicDomainSuffix()); + } + dnsServer.setPublicDomainSuffix(publicDomainSuffix); + } } if (cmd.getNameServers() != null) { @@ -440,6 +448,9 @@ public class DnsProviderManagerImpl extends ManagerBase implements DnsProviderMa Account caller = CallContext.current().getCallingAccount(); if (cmd.getDnsServerId() != null) { DnsServer dnsServer = dnsServerDao.findById(cmd.getDnsServerId()); + if (dnsServer == null) { + throw new InvalidParameterValueException("DNS server not found for the provided ID"); + } accountMgr.checkAccess(caller, null, true, dnsServer); } List ownDnsServerIds = dnsServerDao.listDnsServerIdsByAccountId(caller.getAccountId()); @@ -501,6 +512,9 @@ public class DnsProviderManagerImpl extends ManagerBase implements DnsProviderMa Account caller = CallContext.current().getCallingAccount(); accountMgr.checkAccess(caller, null, true, dnsZone); DnsServerVO server = dnsServerDao.findById(dnsZone.getDnsServerId()); + if (server == null) { + throw new CloudRuntimeException("The underlying DNS server for this DNS Record is missing."); + } DnsRecord.RecordType recordType = cmd.getType(); try { DnsRecord record = new DnsRecord(); @@ -593,6 +607,10 @@ public class DnsProviderManagerImpl extends ManagerBase implements DnsProviderMa throw new CloudRuntimeException("DNS zone not found during provisioning"); } DnsServerVO server = dnsServerDao.findById(dnsZone.getDnsServerId()); + if (server == null) { + dnsZoneDao.remove(dnsZoneId); + throw new CloudRuntimeException(String.format("DNS server for zone '%s' no longer exists", dnsZone.getName())); + } try { DnsProvider provider = getProviderByType(server.getProviderType()); if (isExistingZone) { @@ -626,6 +644,9 @@ public class DnsProviderManagerImpl extends ManagerBase implements DnsProviderMa public DnsServerResponse createDnsServerResponse(DnsServer dnsServer) { DnsServerJoinVO serverJoin = dnsServerJoinDao.findById(dnsServer.getId()); + if (serverJoin == null) { + throw new CloudRuntimeException(String.format("DNS server not found for ID: %s", dnsServer.getId())); + } return createDnsServerResponse(serverJoin); } @@ -650,6 +671,9 @@ public class DnsProviderManagerImpl extends ManagerBase implements DnsProviderMa @Override public DnsZoneResponse createDnsZoneResponse(DnsZone dnsZone) { DnsZoneJoinVO zoneJoinVO = dnsZoneJoinDao.findById(dnsZone.getId()); + if (zoneJoinVO == null) { + throw new CloudRuntimeException(String.format("DNS zone join view not found for ID: %s", dnsZone.getId())); + } return createDnsZoneResponse(zoneJoinVO); } diff --git a/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java b/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java index e508d55ace4..32cacd916b5 100644 --- a/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java @@ -717,7 +717,7 @@ public class DnsProviderManagerImplTest { public void testAddDnsServerSuccess() throws Exception { org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class); - when(callerMock.getType()).thenReturn(Account.Type.ADMIN); + when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true); when(cmd.getUrl()).thenReturn("http://newpdns:8081"); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null); @@ -764,6 +764,7 @@ public class DnsProviderManagerImplTest { org.apache.cloudstack.api.command.user.dns.ListDnsZonesCmd cmd = mock( org.apache.cloudstack.api.command.user.dns.ListDnsZonesCmd.class); when(cmd.getId()).thenReturn(null); + when(cmd.getDnsServerId()).thenReturn(null); when(dnsServerDao.listDnsServerIdsByAccountId(anyLong())).thenReturn(Collections.emptyList()); List zones = Collections.singletonList(zoneVO); com.cloud.utils.Pair, Integer> searchPair = new com.cloud.utils.Pair<>(zones, 1); @@ -789,7 +790,8 @@ public class DnsProviderManagerImplTest { public void testAddDnsServerNormalUser() throws Exception { org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class); - when(callerMock.getType()).thenReturn(Account.Type.NORMAL); + when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(false); + when(accountMgr.isDomainAdmin(callerMock.getId())).thenReturn(false); when(cmd.getUrl()).thenReturn("http://newpdns:8081"); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(cmd.getNameServers()).thenReturn(Collections.emptyList()); @@ -808,7 +810,7 @@ public class DnsProviderManagerImplTest { public void testAddDnsServerValidationFailure() throws Exception { org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock( org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class); - when(callerMock.getType()).thenReturn(Account.Type.ADMIN); + when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true); when(cmd.getUrl()).thenReturn("http://newpdns:8081"); when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS); when(cmd.getNameServers()).thenReturn(Collections.emptyList()); diff --git a/test/integration/smoke/test_dns_framework_powerdns.py b/test/integration/smoke/test_dns_framework_powerdns.py index 1dfdca35c33..7df8bb984ea 100644 --- a/test/integration/smoke/test_dns_framework_powerdns.py +++ b/test/integration/smoke/test_dns_framework_powerdns.py @@ -34,6 +34,7 @@ class TestCloudStackDNSFramework(cloudstackTestCase): """ super(TestCloudStackDNSFramework, cls).setUpClass() cls.api_client = cls.testClient.getApiClient() + cls.pdns_unavailable = False cls.logger = logging.getLogger("TestCloudStackDNSFramework") cls.stream_handler = logging.StreamHandler() @@ -77,14 +78,41 @@ class TestCloudStackDNSFramework(cloudstackTestCase): cls.tearDownClass() raise Exception(f"Failed to start PDNS:\n{result.stderr}") - # Allow PDNS to initialize - time.sleep(15) - - cls.logger.info("PDNS is up and running") - - # Construct PDNS URL once + # Wait for PDNS to be ready with polling cls.pdns_url = f"http://{cls.marvin_vm_ip}" - cls.logger.info(f"PDNS endpoint: {cls.pdns_url}") + cls.logger.info(f"PDNS endpoint: {cls.pdns_url}:8081") + cls._wait_for_pdns_ready() + + @classmethod + def _wait_for_pdns_ready(cls, timeout=90, interval=1): + """ + Poll PDNS API until it responds or timeout is reached. + Logs a message and sets cls.pdns_unavailable if PDNS is not reachable. + """ + import urllib.request + api_url = f"{cls.pdns_url}:8081/api/v1/servers" + cls.logger.info(f"Waiting for PDNS to be ready at {api_url} (timeout: {timeout}s)") + for _ in range(timeout // interval): + try: + req = urllib.request.Request(api_url) + req.add_header("X-API-Key", "supersecretapikey") + resp = urllib.request.urlopen(req, timeout=2) + if resp.status == 200: + cls.logger.info("PDNS is up and running") + cls.pdns_unavailable = False + return + except Exception: + pass + time.sleep(interval) + + cls.logger.warning("PDNS did not become ready within timeout; skipping tests") + cls.pdns_unavailable = True + + def setUp(self): + if self.__class__.pdns_unavailable: + import unittest + raise unittest.SkipTest("PDNS is unavailable; skipping test") + @attr(tags=["advanced"], required_hardware="true") def test_01_list_dns_providers(self): diff --git a/ui/public/locales/en.json b/ui/public/locales/en.json index a12e347137f..ade717dd1b4 100644 --- a/ui/public/locales/en.json +++ b/ui/public/locales/en.json @@ -958,6 +958,7 @@ "label.dns.create.zone": "Create DNS Zone", "label.dns.delete.server": "Delete DNS Server", "label.dns.delete.zone": "Delete DNS Zone", +"label.dns.delete.record": "Delete DNS Record", "label.dns.dnsapikey": "DNS API key", "label.dns.pdnsserverid": "PowerDNS server ID", "label.dns.name": "DNS Name", @@ -3929,6 +3930,10 @@ "message.success.update.dns.server": "Successfully updated DNS server", "message.success.update.dns.zone": "Successfully updated DNS zone", "message.success.delete.dns.record": "Successfully deleted DNS record", +"message.success.delete.dns.server": "Successfully deleted DNS server", +"message.success.delete.dns.zone": "Successfully deleted DNS zone", +"message.error.delete.dns.server": "Failed to delete DNS server", +"message.error.delete.dns.zone": "Failed to delete DNS zone", "message.success.associate.dns.zone": "Successfully associated DNS zone with network", "message.error.fetch.dns.zones": "Could not load DNS zones.", "message.success.add.guest.network": "Successfully created guest Network", diff --git a/ui/src/views/network/dns/AddDnsServer.vue b/ui/src/views/network/dns/AddDnsServer.vue index c76420b6647..3950c85248d 100644 --- a/ui/src/views/network/dns/AddDnsServer.vue +++ b/ui/src/views/network/dns/AddDnsServer.vue @@ -22,7 +22,8 @@ ref="formRef" :model="form" :rules="rules" - layout="vertical"> + layout="vertical" + @finish="handleSubmit">