Skip to content

Commit e7b7b4c

Browse files
sudo87sudo87
committed
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
1 parent 2aae36d commit e7b7b4c

16 files changed

Lines changed: 367 additions & 201 deletions

File tree

‎api/src/main/java/org/apache/cloudstack/api/command/user/dns/AddDnsServerCmd.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -43,7 +43,7 @@
4343
description = "Adds a new external DNS server",
4444
responseObject = DnsServerResponse.class,
4545
entityType = {DnsServer.class},
46-
requestHasSensitiveInfo = false,
46+
requestHasSensitiveInfo = true,
4747
responseHasSensitiveInfo = false,
4848
since = "4.23.0",
4949
authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User})

‎api/src/main/java/org/apache/cloudstack/api/command/user/dns/CreateDnsRecordCmd.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -85,7 +85,7 @@ public void execute() {
8585
response.setResponseName(getCommandName());
8686
setResponseObject(response);
8787
} catch (Exception e) {
88-
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, "Failed to create DNS Record: " + e.getMessage());
88+
throw new ServerApiException(ApiErrorCode.INTERNAL_ERROR, e.getMessage());
8989
}
9090
}
9191

‎api/src/main/java/org/apache/cloudstack/api/command/user/dns/UpdateDnsServerCmd.java‎

Lines changed: 2 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -30,7 +30,6 @@
3030
import org.apache.cloudstack.api.ServerApiException;
3131
import org.apache.cloudstack.api.response.DnsServerResponse;
3232
import org.apache.cloudstack.dns.DnsServer;
33-
import org.apache.commons.lang3.BooleanUtils;
3433
import org.apache.commons.lang3.StringUtils;
3534

3635
import com.cloud.user.Account;
@@ -40,7 +39,7 @@
4039
description = "Update DNS server",
4140
responseObject = DnsServerResponse.class,
4241
entityType = {DnsServer.class},
43-
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
42+
requestHasSensitiveInfo = true, responseHasSensitiveInfo = false,
4443
since = "4.23.0",
4544
authorized = {RoleType.Admin, RoleType.ResourceAdmin, RoleType.DomainAdmin, RoleType.User})
4645
public class UpdateDnsServerCmd extends BaseCmd {
@@ -96,7 +95,7 @@ public Integer getPort() {
9695
return port;
9796
}
9897
public Boolean isPublic() {
99-
return BooleanUtils.isTrue(isPublic);
98+
return isPublic;
10099
}
101100
public String getPublicDomainSuffix() {
102101
return publicDomainSuffix;

‎server/src/main/java/org/apache/cloudstack/dns/DnsProviderManagerImpl.java‎

Lines changed: 32 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -28,6 +28,7 @@
2828
import java.util.HashSet;
2929
import java.util.List;
3030
import java.util.Map;
31+
import java.util.Objects;
3132
import java.util.Set;
3233
import java.util.stream.Collectors;
3334

@@ -77,6 +78,7 @@
7778
import org.apache.cloudstack.framework.messagebus.MessageSubscriber;
7879
import org.apache.commons.collections.CollectionUtils;
7980
import org.apache.commons.collections.MapUtils;
81+
import org.apache.commons.lang3.BooleanUtils;
8082
import org.springframework.stereotype.Component;
8183

8284
import com.cloud.domain.Domain;
@@ -172,8 +174,8 @@ public DnsServer addDnsServer(AddDnsServerCmd cmd) {
172174

173175
boolean isDnsPublic = cmd.isPublic();
174176
String publicDomainSuffix = cmd.getPublicDomainSuffix();
175-
if (caller.getType().equals(Account.Type.NORMAL)) {
176-
logger.info("Only admin and domain admin users are allowed to configure a public DNS server");
177+
if (!accountMgr.isRootAdmin(caller.getId()) && !accountMgr.isDomainAdmin(caller.getId())) {
178+
logger.info("Only root admin and domain admin users are allowed to configure a public DNS server");
177179
isDnsPublic = false;
178180
publicDomainSuffix = null;
179181
}
@@ -264,15 +266,21 @@ public DnsServer updateDnsServer(UpdateDnsServerCmd cmd) {
264266
validationRequired = true;
265267
}
266268

267-
if (cmd.getPort() != null) {
269+
if (cmd.getPort() != null && !Objects.equals(cmd.getPort(), dnsServer.getPort())) {
268270
dnsServer.setPort(cmd.getPort());
271+
validationRequired = true;
269272
}
270-
if (cmd.isPublic() != null) {
271-
dnsServer.setPublicServer(cmd.isPublic());
272-
}
273+
if (accountMgr.isRootAdmin(caller.getId()) || accountMgr.isDomainAdmin(caller.getId())) {
274+
if (cmd.isPublic() != null) {
275+
boolean isPublic = BooleanUtils.isTrue(cmd.isPublic());
276+
dnsServer.setPublicServer(isPublic);
273277

274-
if (cmd.getPublicDomainSuffix() != null) {
275-
dnsServer.setPublicDomainSuffix(DnsProviderUtil.normalizeDomainForDb(cmd.getPublicDomainSuffix()));
278+
String publicDomainSuffix = null;
279+
if (isPublic && StringUtils.isNotBlank(cmd.getPublicDomainSuffix())) {
280+
publicDomainSuffix = DnsProviderUtil.normalizeDomainForDb(cmd.getPublicDomainSuffix());
281+
}
282+
dnsServer.setPublicDomainSuffix(publicDomainSuffix);
283+
}
276284
}
277285

278286
if (cmd.getNameServers() != null) {
@@ -440,6 +448,9 @@ private Pair<List<DnsZoneVO>, Integer> searchForDnsZonesInternal(ListDnsZonesCmd
440448
Account caller = CallContext.current().getCallingAccount();
441449
if (cmd.getDnsServerId() != null) {
442450
DnsServer dnsServer = dnsServerDao.findById(cmd.getDnsServerId());
451+
if (dnsServer == null) {
452+
throw new InvalidParameterValueException("DNS server not found for the provided ID");
453+
}
443454
accountMgr.checkAccess(caller, null, true, dnsServer);
444455
}
445456
List<Long> ownDnsServerIds = dnsServerDao.listDnsServerIdsByAccountId(caller.getAccountId());
@@ -501,6 +512,9 @@ public boolean deleteDnsRecord(DeleteDnsRecordCmd cmd) {
501512
Account caller = CallContext.current().getCallingAccount();
502513
accountMgr.checkAccess(caller, null, true, dnsZone);
503514
DnsServerVO server = dnsServerDao.findById(dnsZone.getDnsServerId());
515+
if (server == null) {
516+
throw new CloudRuntimeException("The underlying DNS server for this DNS Record is missing.");
517+
}
504518
DnsRecord.RecordType recordType = cmd.getType();
505519
try {
506520
DnsRecord record = new DnsRecord();
@@ -593,6 +607,10 @@ public DnsZone provisionDnsZone(long dnsZoneId, boolean isExistingZone) {
593607
throw new CloudRuntimeException("DNS zone not found during provisioning");
594608
}
595609
DnsServerVO server = dnsServerDao.findById(dnsZone.getDnsServerId());
610+
if (server == null) {
611+
dnsZoneDao.remove(dnsZoneId);
612+
throw new CloudRuntimeException(String.format("DNS server for zone '%s' no longer exists", dnsZone.getName()));
613+
}
596614
try {
597615
DnsProvider provider = getProviderByType(server.getProviderType());
598616
if (isExistingZone) {
@@ -626,6 +644,9 @@ public DnsZone provisionDnsZone(long dnsZoneId, boolean isExistingZone) {
626644

627645
public DnsServerResponse createDnsServerResponse(DnsServer dnsServer) {
628646
DnsServerJoinVO serverJoin = dnsServerJoinDao.findById(dnsServer.getId());
647+
if (serverJoin == null) {
648+
throw new CloudRuntimeException(String.format("DNS server not found for ID: %s", dnsServer.getId()));
649+
}
629650
return createDnsServerResponse(serverJoin);
630651
}
631652

@@ -650,6 +671,9 @@ DnsServerResponse createDnsServerResponse(DnsServerJoinVO server) {
650671
@Override
651672
public DnsZoneResponse createDnsZoneResponse(DnsZone dnsZone) {
652673
DnsZoneJoinVO zoneJoinVO = dnsZoneJoinDao.findById(dnsZone.getId());
674+
if (zoneJoinVO == null) {
675+
throw new CloudRuntimeException(String.format("DNS zone join view not found for ID: %s", dnsZone.getId()));
676+
}
653677
return createDnsZoneResponse(zoneJoinVO);
654678
}
655679

‎server/src/test/java/org/apache/cloudstack/dns/DnsProviderManagerImplTest.java‎

Lines changed: 5 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -717,7 +717,7 @@ public void testConfigure() throws Exception {
717717
public void testAddDnsServerSuccess() throws Exception {
718718
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
719719
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
720-
when(callerMock.getType()).thenReturn(Account.Type.ADMIN);
720+
when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true);
721721
when(cmd.getUrl()).thenReturn("http://newpdns:8081");
722722
when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS);
723723
when(dnsServerDao.findByUrlAndAccount(anyString(), anyLong())).thenReturn(null);
@@ -764,6 +764,7 @@ public void testListDnsZones() {
764764
org.apache.cloudstack.api.command.user.dns.ListDnsZonesCmd cmd = mock(
765765
org.apache.cloudstack.api.command.user.dns.ListDnsZonesCmd.class);
766766
when(cmd.getId()).thenReturn(null);
767+
when(cmd.getDnsServerId()).thenReturn(null);
767768
when(dnsServerDao.listDnsServerIdsByAccountId(anyLong())).thenReturn(Collections.emptyList());
768769
List<DnsZoneVO> zones = Collections.singletonList(zoneVO);
769770
com.cloud.utils.Pair<List<DnsZoneVO>, Integer> searchPair = new com.cloud.utils.Pair<>(zones, 1);
@@ -789,7 +790,8 @@ public void testAddDnsServerAlreadyExists() {
789790
public void testAddDnsServerNormalUser() throws Exception {
790791
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
791792
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
792-
when(callerMock.getType()).thenReturn(Account.Type.NORMAL);
793+
when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(false);
794+
when(accountMgr.isDomainAdmin(callerMock.getId())).thenReturn(false);
793795
when(cmd.getUrl()).thenReturn("http://newpdns:8081");
794796
when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS);
795797
when(cmd.getNameServers()).thenReturn(Collections.emptyList());
@@ -808,7 +810,7 @@ public void testAddDnsServerNormalUser() throws Exception {
808810
public void testAddDnsServerValidationFailure() throws Exception {
809811
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd cmd = mock(
810812
org.apache.cloudstack.api.command.user.dns.AddDnsServerCmd.class);
811-
when(callerMock.getType()).thenReturn(Account.Type.ADMIN);
813+
when(accountMgr.isRootAdmin(callerMock.getId())).thenReturn(true);
812814
when(cmd.getUrl()).thenReturn("http://newpdns:8081");
813815
when(cmd.getProvider()).thenReturn(DnsProviderType.PowerDNS);
814816
when(cmd.getNameServers()).thenReturn(Collections.emptyList());

‎test/integration/smoke/test_dns_framework_powerdns.py‎

Lines changed: 34 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,7 @@ def setUpClass(cls):
3434
"""
3535
super(TestCloudStackDNSFramework, cls).setUpClass()
3636
cls.api_client = cls.testClient.getApiClient()
37+
cls.pdns_unavailable = False
3738

3839
cls.logger = logging.getLogger("TestCloudStackDNSFramework")
3940
cls.stream_handler = logging.StreamHandler()
@@ -77,14 +78,41 @@ def setUpClass(cls):
7778
cls.tearDownClass()
7879
raise Exception(f"Failed to start PDNS:\n{result.stderr}")
7980

80-
# Allow PDNS to initialize
81-
time.sleep(15)
81+
# Wait for PDNS to be ready with polling
82+
cls.pdns_url = f"http://{cls.marvin_vm_ip}"
83+
cls.logger.info(f"PDNS endpoint: {cls.pdns_url}:8081")
84+
cls._wait_for_pdns_ready()
8285

83-
cls.logger.info("PDNS is up and running")
86+
@classmethod
87+
def _wait_for_pdns_ready(cls, timeout=90, interval=1):
88+
"""
89+
Poll PDNS API until it responds or timeout is reached.
90+
Logs a message and sets cls.pdns_unavailable if PDNS is not reachable.
91+
"""
92+
import urllib.request
93+
api_url = f"{cls.pdns_url}:8081/api/v1/servers"
94+
cls.logger.info(f"Waiting for PDNS to be ready at {api_url} (timeout: {timeout}s)")
95+
for _ in range(timeout // interval):
96+
try:
97+
req = urllib.request.Request(api_url)
98+
req.add_header("X-API-Key", "supersecretapikey")
99+
resp = urllib.request.urlopen(req, timeout=2)
100+
if resp.status == 200:
101+
cls.logger.info("PDNS is up and running")
102+
cls.pdns_unavailable = False
103+
return
104+
except Exception:
105+
pass
106+
time.sleep(interval)
107+
108+
cls.logger.warning("PDNS did not become ready within timeout; skipping tests")
109+
cls.pdns_unavailable = True
110+
111+
def setUp(self):
112+
if self.__class__.pdns_unavailable:
113+
import unittest
114+
raise unittest.SkipTest("PDNS is unavailable; skipping test")
84115

85-
# Construct PDNS URL once
86-
cls.pdns_url = f"http://{cls.marvin_vm_ip}"
87-
cls.logger.info(f"PDNS endpoint: {cls.pdns_url}")
88116

89117
@attr(tags=["advanced"], required_hardware="true")
90118
def test_01_list_dns_providers(self):

‎ui/public/locales/en.json‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -958,6 +958,7 @@
958958
"label.dns.create.zone": "Create DNS Zone",
959959
"label.dns.delete.server": "Delete DNS Server",
960960
"label.dns.delete.zone": "Delete DNS Zone",
961+
"label.dns.delete.record": "Delete DNS Record",
961962
"label.dns.dnsapikey": "DNS API key",
962963
"label.dns.pdnsserverid": "PowerDNS server ID",
963964
"label.dns.name": "DNS Name",
@@ -3929,6 +3930,10 @@
39293930
"message.success.update.dns.server": "Successfully updated DNS server",
39303931
"message.success.update.dns.zone": "Successfully updated DNS zone",
39313932
"message.success.delete.dns.record": "Successfully deleted DNS record",
3933+
"message.success.delete.dns.server": "Successfully deleted DNS server",
3934+
"message.success.delete.dns.zone": "Successfully deleted DNS zone",
3935+
"message.error.delete.dns.server": "Failed to delete DNS server",
3936+
"message.error.delete.dns.zone": "Failed to delete DNS zone",
39323937
"message.success.associate.dns.zone": "Successfully associated DNS zone with network",
39333938
"message.error.fetch.dns.zones": "Could not load DNS zones.",
39343939
"message.success.add.guest.network": "Successfully created guest Network",

‎ui/src/views/network/dns/AddDnsServer.vue‎

Lines changed: 40 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@
2222
ref="formRef"
2323
:model="form"
2424
:rules="rules"
25-
layout="vertical">
25+
layout="vertical"
26+
@finish="handleSubmit">
2627

2728
<a-form-item name="name" ref="name">
2829
<template #label>
@@ -102,16 +103,18 @@
102103
:placeholder="'Enter PowerDNS Server ID (optional if localhost)'" />
103104
</a-form-item>
104105

105-
<a-form-item v-if="isAdminOrDomainAdmin()" name="publicdomainsuffix" ref="publicdomainsuffix">
106-
<template #label>
107-
<tooltip-label
108-
:title="$t('label.dns.publicdomainsuffix')"
109-
:tooltip="apiParams.publicdomainsuffix?.description" />
110-
</template>
111-
<a-input
112-
v-model:value="form.publicdomainsuffix"
113-
:placeholder="apiParams.publicdomainsuffix?.description || 'Enter Public Domain Suffix e.g. example.com'" />
114-
</a-form-item>
106+
<Transition name="slide-down">
107+
<a-form-item v-if="isAdminOrDomainAdmin() && form.ispublic" name="publicdomainsuffix" ref="publicdomainsuffix">
108+
<template #label>
109+
<tooltip-label
110+
:title="$t('label.dns.publicdomainsuffix')"
111+
:tooltip="apiParams.publicdomainsuffix?.description" />
112+
</template>
113+
<a-input
114+
v-model:value="form.publicdomainsuffix"
115+
:placeholder="apiParams.publicdomainsuffix?.description || 'Enter Public Domain Suffix e.g. example.com'" />
116+
</a-form-item>
117+
</Transition>
115118

116119
<a-form-item name="nameservers" ref="nameservers">
117120
<template #label>
@@ -143,7 +146,7 @@
143146
<a-button
144147
type="primary"
145148
:loading="loading"
146-
@click="handleSubmit">
149+
htmlType="submit">
147150
{{ $t('label.ok') }}
148151
</a-button>
149152
</div>
@@ -234,11 +237,22 @@ export default {
234237
if (this.form.pdnsserverid) {
235238
params['details[0].pdnsServerId'] = this.form.pdnsserverid?.trim()
236239
}
237-
await postAPI('addDnsServer', params)
240+
const response = await postAPI('addDnsServer', params)
241+
const serverData = response?.adddnsserverresponse?.dnsserver || response?.adddnsserverresponse
242+
if (!serverData?.id) {
243+
this.$notification.error({
244+
message: this.$t('message.request.failed'),
245+
description: 'Failed to get server from response',
246+
duration: 0
247+
})
248+
this.loading = false
249+
return
250+
}
238251
this.$notification.success({
239252
message: this.$t('label.dns.add.server'),
240-
description: this.$t('message.success.add.dns.server')
253+
description: `${this.$t('message.success.add.dns.server')} ${params.name}`
241254
})
255+
this.loading = false
242256
this.$emit('refresh-data')
243257
this.closeAction()
244258
} catch (error) {
@@ -247,7 +261,6 @@ export default {
247261
description: error?.response?.headers['x-description'] || error.message,
248262
duration: 0
249263
})
250-
} finally {
251264
this.loading = false
252265
}
253266
},
@@ -353,4 +366,16 @@ export default {
353366
.action-button button {
354367
margin-left: 8px;
355368
}
369+
370+
.slide-down-enter-active,
371+
.slide-down-leave-active {
372+
transition: max-height 0.3s ease, opacity 0.3s ease;
373+
overflow: hidden;
374+
max-height: 120px;
375+
}
376+
.slide-down-enter-from,
377+
.slide-down-leave-to {
378+
max-height: 0;
379+
opacity: 0;
380+
}
356381
</style>

‎ui/src/views/network/dns/AssociateDnsZone.vue‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,7 +22,8 @@
2222
ref="formRef"
2323
:model="form"
2424
:rules="rules"
25-
layout="vertical">
25+
layout="vertical"
26+
@finish="handleSubmit">
2627

2728
<a-form-item name="dnszoneid" ref="dnszoneid">
2829
<template #label>
@@ -65,7 +66,7 @@
6566
<a-button
6667
type="primary"
6768
:loading="loading"
68-
@click="handleSubmit">
69+
htmlType="submit">
6970
{{ $t('label.ok') }}
7071
</a-button>
7172
</div>

0 commit comments

Comments
 (0)