This is an automated email from the ASF dual-hosted git repository. pearl11594 pushed a commit to branch dedicate-backup-offering-to-domain in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit 21bc89e07270fbcd7701976e74e1da2df236878c Author: Pearl Dsilva <[email protected]> AuthorDate: Thu Dec 4 10:38:31 2025 -0500 Add tests and UI support and update response params --- .../admin/backup/UpdateBackupOfferingCmd.java | 11 ++- .../api/response/BackupOfferingResponse.java | 17 +++++ .../backup/dao/BackupOfferingDaoImpl.java | 23 ++++++- .../src/main/java/com/cloud/acl/DomainChecker.java | 3 - .../cloudstack/backup/BackupManagerImpl.java | 50 ++++++++++++-- .../cloudstack/backup/BackupManagerTest.java | 78 +++++++++++++++++++++- ui/src/config/section/offering.js | 6 +- ui/src/views/offering/ImportBackupOffering.vue | 69 ++++++++++++++++++- 8 files changed, 241 insertions(+), 16 deletions(-) diff --git a/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java b/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java index 682f2f595ad..2b8643819d4 100644 --- a/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java +++ b/api/src/main/java/org/apache/cloudstack/api/command/admin/backup/UpdateBackupOfferingCmd.java @@ -37,6 +37,7 @@ import com.cloud.user.Account; import com.cloud.utils.exception.CloudRuntimeException; import java.util.List; +import java.util.function.LongFunction; @APICommand(name = "updateBackupOffering", description = "Updates a backup offering.", responseObject = BackupOfferingResponse.class, requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.16.0") @@ -114,7 +115,15 @@ public class UpdateBackupOfferingCmd extends BaseCmd implements DomainAndZoneIdR } public List<Long> getDomainIds() { - return resolveDomainIds(domainIds, id, backupManager::getBackupOfferingDomains, "backup offering"); + // backupManager may be null in unit tests where the command is spied without injection. + // Avoid creating a method reference to a null receiver which causes NPE. When backupManager + // is null, pass null as the defaultDomainsProvider so resolveDomainIds will simply return + // an empty list or parse the explicit domainIds string. + LongFunction<List<Long>> defaultDomainsProvider = null; + if (backupManager != null) { + defaultDomainsProvider = backupManager::getBackupOfferingDomains; + } + return resolveDomainIds(domainIds, id, defaultDomainsProvider, "backup offering"); } @Override diff --git a/api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java b/api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java index 4120f68d9da..0669afa604b 100644 --- a/api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java +++ b/api/src/main/java/org/apache/cloudstack/api/response/BackupOfferingResponse.java @@ -61,6 +61,14 @@ public class BackupOfferingResponse extends BaseResponse { @Param(description = "zone name") private String zoneName; + @SerializedName(ApiConstants.DOMAIN_ID) + @Param(description = "the domain ID(s) this disk offering belongs to. Ignore this information as it is not currently applicable.") + private String domainId; + + @SerializedName(ApiConstants.DOMAIN) + @Param(description = "the domain name(s) this disk offering belongs to. Ignore this information as it is not currently applicable.") + private String domain; + @SerializedName(ApiConstants.CROSS_ZONE_INSTANCE_CREATION) @Param(description = "the backups with this offering can be used to create Instances on all Zones", since = "4.22.0") private Boolean crossZoneInstanceCreation; @@ -108,4 +116,13 @@ public class BackupOfferingResponse extends BaseResponse { public void setCreated(Date created) { this.created = created; } + + public void setDomainId(String domainId) { + this.domainId = domainId; + } + + public void setDomain(String domain) { + this.domain = domain; + } + } diff --git a/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDaoImpl.java b/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDaoImpl.java index a41e4e70d33..708faeef464 100644 --- a/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDaoImpl.java +++ b/engine/schema/src/main/java/org/apache/cloudstack/backup/dao/BackupOfferingDaoImpl.java @@ -20,6 +20,8 @@ package org.apache.cloudstack.backup.dao; import javax.annotation.PostConstruct; import javax.inject.Inject; +import com.cloud.domain.DomainVO; +import com.cloud.domain.dao.DomainDao; import org.apache.cloudstack.api.response.BackupOfferingResponse; import org.apache.cloudstack.backup.BackupOffering; import org.apache.cloudstack.backup.BackupOfferingVO; @@ -30,10 +32,16 @@ import com.cloud.utils.db.GenericDaoBase; import com.cloud.utils.db.SearchBuilder; import com.cloud.utils.db.SearchCriteria; +import java.util.List; + public class BackupOfferingDaoImpl extends GenericDaoBase<BackupOfferingVO, Long> implements BackupOfferingDao { @Inject DataCenterDao dataCenterDao; + @Inject + BackupOfferingDetailsDao backupOfferingDetailsDao; + @Inject + DomainDao domainDao; private SearchBuilder<BackupOfferingVO> backupPoliciesSearch; @@ -51,8 +59,9 @@ public class BackupOfferingDaoImpl extends GenericDaoBase<BackupOfferingVO, Long @Override public BackupOfferingResponse newBackupOfferingResponse(BackupOffering offering, Boolean crossZoneInstanceCreation) { - DataCenterVO zone = dataCenterDao.findById(offering.getZoneId()); + DataCenterVO zone = dataCenterDao.findById(offering.getZoneId()); + List<Long> domainIds = backupOfferingDetailsDao.findDomainIds(offering.getId()); BackupOfferingResponse response = new BackupOfferingResponse(); response.setId(offering.getUuid()); response.setName(offering.getName()); @@ -64,6 +73,18 @@ public class BackupOfferingDaoImpl extends GenericDaoBase<BackupOfferingVO, Long response.setZoneId(zone.getUuid()); response.setZoneName(zone.getName()); } + if (domainIds != null && !domainIds.isEmpty()) { + String domainUUIDs = domainIds.stream().map(Long::valueOf).map(domainId -> { + DomainVO domain = domainDao.findById(domainId); + return domain != null ? domain.getUuid() : ""; + }).filter(name -> !name.isEmpty()).reduce((a, b) -> a + "," + b).orElse(""); + String domainNames = domainIds.stream().map(Long::valueOf).map(domainId -> { + DomainVO domain = domainDao.findById(domainId); + return domain != null ? domain.getName() : ""; + }).filter(name -> !name.isEmpty()).reduce((a, b) -> a + "," + b).orElse(""); + response.setDomain(domainNames); + response.setDomainId(domainUUIDs); + } if (crossZoneInstanceCreation) { response.setCrossZoneInstanceCreation(true); } diff --git a/server/src/main/java/com/cloud/acl/DomainChecker.java b/server/src/main/java/com/cloud/acl/DomainChecker.java index b436d0bdcf3..99a1a4d5a20 100644 --- a/server/src/main/java/com/cloud/acl/DomainChecker.java +++ b/server/src/main/java/com/cloud/acl/DomainChecker.java @@ -481,15 +481,12 @@ public class DomainChecker extends AdapterBase implements SecurityChecker { @Override public boolean checkAccess(Account account, BackupOffering backupOffering) throws PermissionDeniedException { boolean hasAccess = false; - // Check for domains if (account == null || backupOffering == null) { hasAccess = true; } else { - // admin has all permissions if (_accountService.isRootAdmin(account.getId())) { hasAccess = true; } - // if account is normal user or domain admin or project else if (_accountService.isNormalUser(account.getId()) || account.getType() == Account.Type.RESOURCE_DOMAIN_ADMIN || _accountService.isDomainAdmin(account.getId()) diff --git a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java index e1c51813ad6..bc40719c7ec 100644 --- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java @@ -291,7 +291,6 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { } } - // enforce account permissions: domain-admins cannot create public offerings final Account caller = CallContext.current().getCallingAccount(); List<Long> filteredDomainIds = cmd.getDomainIds() == null ? new ArrayList<>() : new ArrayList<>(cmd.getDomainIds()); if (filteredDomainIds.size() > 1) { @@ -310,11 +309,10 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { if (savedOffering == null) { throw new CloudRuntimeException("Unable to create backup offering: " + cmd.getExternalId() + ", name: " + cmd.getName()); } - // Persist domain dedication details (if any) if (org.apache.commons.collections.CollectionUtils.isNotEmpty(filteredDomainIds)) { List<BackupOfferingDetailsVO> detailsVOList = new ArrayList<>(); for (Long domainId : filteredDomainIds) { - detailsVOList.add(new BackupOfferingDetailsVO(savedOffering.getId(), org.apache.cloudstack.api.ApiConstants.DOMAIN_ID, String.valueOf(domainId), false)); + detailsVOList.add(new BackupOfferingDetailsVO(savedOffering.getId(), ApiConstants.DOMAIN_ID, String.valueOf(domainId), false)); } if (!detailsVOList.isEmpty()) { backupOfferingDetailsDao.saveDetails(detailsVOList); @@ -400,7 +398,6 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { throw new CloudRuntimeException("Could not find a backup offering with id: " + offeringId); } - // Ensure caller has permission to delete this offering accountManager.checkAccess(CallContext.current().getCallingAccount(), offering); if (backupDao.listByOfferingId(offering.getId()).size() > 0) { @@ -2179,6 +2176,7 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { String name = updateBackupOfferingCmd.getName(); String description = updateBackupOfferingCmd.getDescription(); Boolean allowUserDrivenBackups = updateBackupOfferingCmd.getAllowUserDrivenBackups(); + List<Long> domainIds = updateBackupOfferingCmd.getDomainIds(); BackupOfferingVO backupOfferingVO = backupOfferingDao.findById(id); if (backupOfferingVO == null) { @@ -2209,16 +2207,58 @@ public class BackupManagerImpl extends ManagerBase implements BackupManager { fields.add("allowUserDrivenBackups: " + allowUserDrivenBackups); } - if (!backupOfferingDao.update(id, offering)) { + if (org.apache.commons.collections.CollectionUtils.isNotEmpty(domainIds)) { + for (final Long domainId: domainIds) { + if (domainDao.findById(domainId) == null) { + throw new InvalidParameterValueException("Please specify a valid domain id"); + } + } + } + List<Long> filteredDomainIds = filterChildSubDomains(domainIds); + Collections.sort(filteredDomainIds); + + boolean success =backupOfferingDao.update(id, offering); + if (!success) { logger.warn(String.format("Couldn't update Backup offering (%s) with [%s].", backupOfferingVO, String.join(", ", fields))); } + if (success) { + List<Long> existingDomainIds = backupOfferingDetailsDao.findDomainIds(id); + Collections.sort(existingDomainIds); + updateBackupOfferingDomainDetails(id, filteredDomainIds, existingDomainIds); + } + BackupOfferingVO response = backupOfferingDao.findById(id); CallContext.current().setEventDetails(String.format("Backup Offering updated [%s].", ReflectionToStringBuilderUtils.reflectOnlySelectedFields(response, "id", "name", "description", "userDrivenBackupAllowed", "externalId"))); return response; } + private void updateBackupOfferingDomainDetails(Long id, List<Long> filteredDomainIds, List<Long> existingDomainIds) { + if (existingDomainIds == null) { + existingDomainIds = new ArrayList<>(); + } + List<BackupOfferingDetailsVO> detailsVO = new ArrayList<>(); + if(!filteredDomainIds.equals(existingDomainIds)) { + SearchBuilder<BackupOfferingDetailsVO> sb = backupOfferingDetailsDao.createSearchBuilder(); + sb.and("offeringId", sb.entity().getResourceId(), SearchCriteria.Op.EQ); + sb.and("detailName", sb.entity().getName(), SearchCriteria.Op.EQ); + sb.done(); + SearchCriteria<BackupOfferingDetailsVO> sc = sb.create(); + sc.setParameters("offeringId", String.valueOf(id)); + sc.setParameters("detailName", ApiConstants.DOMAIN_ID); + backupOfferingDetailsDao.remove(sc); + for (Long domainId : filteredDomainIds) { + detailsVO.add(new BackupOfferingDetailsVO(id, ApiConstants.DOMAIN_ID, String.valueOf(domainId), false)); + } + } + if (!detailsVO.isEmpty()) { + for (BackupOfferingDetailsVO detailVO : detailsVO) { + backupOfferingDetailsDao.persist(detailVO); + } + } + } + Map<String, String> getDetailsFromBackupDetails(Long backupId) { Map<String, String> details = backupDetailsDao.listDetailsKeyPairs(backupId, true); if (details == null) { diff --git a/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java b/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java index 6f6879b6fe3..47c3c0d5d4c 100644 --- a/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java +++ b/server/src/test/java/org/apache/cloudstack/backup/BackupManagerTest.java @@ -85,6 +85,7 @@ import org.apache.cloudstack.api.response.BackupResponse; import org.apache.cloudstack.backup.dao.BackupDao; import org.apache.cloudstack.backup.dao.BackupDetailsDao; import org.apache.cloudstack.backup.dao.BackupOfferingDao; +import org.apache.cloudstack.backup.dao.BackupOfferingDetailsDao; import org.apache.cloudstack.backup.dao.BackupScheduleDao; import org.apache.cloudstack.context.CallContext; import org.apache.cloudstack.framework.config.ConfigKey; @@ -241,6 +242,9 @@ public class BackupManagerTest { @Mock private GuestOSDao _guestOSDao; + @Mock + private BackupOfferingDetailsDao backupOfferingDetailsDao; + private Gson gson; private String[] hostPossibleValues = {"127.0.0.1", "hostname"}; @@ -352,6 +356,7 @@ public class BackupManagerTest { when(cmd.getName()).thenReturn("New name"); when(cmd.getDescription()).thenReturn("New description"); when(cmd.getAllowUserDrivenBackups()).thenReturn(true); + when(backupOfferingDetailsDao.findDomainIds(id)).thenReturn(Collections.emptyList()); BackupOffering updated = backupManager.updateBackupOffering(cmd); assertEquals("New name", updated.getName()); @@ -1081,7 +1086,7 @@ public class BackupManagerTest { assertEquals("root-disk-offering-uuid", VmDiskInfo.getDiskOffering().getUuid()); assertEquals(Long.valueOf(5), VmDiskInfo.getSize()); -// assertNull(com.cloud.vm.VmDiskInfo.getDeviceId()); + assertNull(com.cloud.vm.VmDiskInfo.getDeviceId()); } @Test @@ -2156,4 +2161,75 @@ public class BackupManagerTest { verify(vmInstanceDao, times(1)).findByIdIncludingRemoved(vmId); verify(volumeDao, times(1)).findByInstance(vmId); } + + @Test + public void getBackupOfferingDomainsTestOfferingNotFound() { + Long offeringId = 1L; + when(backupOfferingDao.findById(offeringId)).thenReturn(null); + + InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class, + () -> backupManager.getBackupOfferingDomains(offeringId)); + assertEquals("Unable to find backup offering null", exception.getMessage()); + } + + @Test + public void getBackupOfferingDomainsTestReturnsDomains() { + Long offeringId = 1L; + BackupOfferingVO offering = Mockito.mock(BackupOfferingVO.class); + when(backupOfferingDao.findById(offeringId)).thenReturn(offering); + when(backupOfferingDetailsDao.findDomainIds(offeringId)).thenReturn(List.of(10L, 20L)); + + List<Long> result = backupManager.getBackupOfferingDomains(offeringId); + + assertEquals(2, result.size()); + assertTrue(result.contains(10L)); + assertTrue(result.contains(20L)); + } + + @Test + public void testUpdateBackupOfferingThrowsWhenDomainIdInvalid() { + Long id = 1234L; + UpdateBackupOfferingCmd cmd = Mockito.spy(UpdateBackupOfferingCmd.class); + when(cmd.getId()).thenReturn(id); + when(cmd.getDomainIds()).thenReturn(List.of(99L)); + + when(domainDao.findById(99L)).thenReturn(null); + + InvalidParameterValueException exception = Assert.assertThrows(InvalidParameterValueException.class, + () -> backupManager.updateBackupOffering(cmd)); + assertEquals("Please specify a valid domain id", exception.getMessage()); + } + + @Test + public void testUpdateBackupOfferingPersistsDomainDetailsWhenProvided() { + Long id = 1234L; + Long domainId = 11L; + UpdateBackupOfferingCmd cmd = Mockito.spy(UpdateBackupOfferingCmd.class); + when(cmd.getId()).thenReturn(id); + when(cmd.getDomainIds()).thenReturn(List.of(domainId)); + + DomainVO domain = Mockito.mock(DomainVO.class); + when(domainDao.findById(domainId)).thenReturn(domain); + + when(backupOfferingDetailsDao.findDomainIds(id)).thenReturn(Collections.emptyList()); + + SearchBuilder<BackupOfferingDetailsVO> sb = Mockito.mock(SearchBuilder.class); + SearchCriteria<BackupOfferingDetailsVO> sc = Mockito.mock(SearchCriteria.class); + BackupOfferingDetailsVO entity = Mockito.mock(BackupOfferingDetailsVO.class); + when(backupOfferingDetailsDao.createSearchBuilder()).thenReturn(sb); + when(sb.entity()).thenReturn(entity); + when(sb.and(Mockito.anyString(), (Object) any(), Mockito.any())).thenReturn(sb); + when(sb.create()).thenReturn(sc); + + BackupOfferingVO offering = Mockito.mock(BackupOfferingVO.class); + BackupOfferingVO offeringUpdate = Mockito.mock(BackupOfferingVO.class); + when(backupOfferingDao.findById(id)).thenReturn(offering); + when(backupOfferingDao.createForUpdate(id)).thenReturn(offeringUpdate); + when(backupOfferingDao.update(id, offeringUpdate)).thenReturn(true); + + BackupOffering updated = backupManager.updateBackupOffering(cmd); + + verify(backupOfferingDetailsDao, times(1)).persist(Mockito.any(BackupOfferingDetailsVO.class)); + } + } diff --git a/ui/src/config/section/offering.js b/ui/src/config/section/offering.js index 4a32619b8c2..bc95772d6f7 100644 --- a/ui/src/config/section/offering.js +++ b/ui/src/config/section/offering.js @@ -340,9 +340,9 @@ export default { icon: 'cloud-upload-outlined', docHelp: 'adminguide/virtual_machines.html#backup-offerings', permission: ['listBackupOfferings'], - searchFilters: ['zoneid'], - columns: ['name', 'description', 'zonename'], - details: ['name', 'id', 'description', 'externalid', 'zone', 'allowuserdrivenbackups', 'created'], + searchFilters: ['zoneid', 'domainid'], + columns: ['name', 'description', 'domain', 'zonename'], + details: ['name', 'id', 'description', 'externalid', 'domain', 'zone', 'allowuserdrivenbackups', 'created'], related: [{ name: 'vm', title: 'label.instances', diff --git a/ui/src/views/offering/ImportBackupOffering.vue b/ui/src/views/offering/ImportBackupOffering.vue index b8ac7d8e8e6..f680eacd4a7 100644 --- a/ui/src/views/offering/ImportBackupOffering.vue +++ b/ui/src/views/offering/ImportBackupOffering.vue @@ -85,6 +85,33 @@ </template> <a-switch v-model:checked="form.allowuserdrivenbackups"/> </a-form-item> + <a-form-item name="ispublic" ref="ispublic" :label="$t('label.ispublic')" v-if="isAdmin()"> + <a-switch v-model:checked="form.ispublic" /> + </a-form-item> + <a-form-item name="domainid" ref="domainid" v-if="!form.ispublic"> + <template #label> + <tooltip-label :title="$t('label.domainid')" :tooltip="apiParams.domainid.description"/> + </template> + <a-select + mode="multiple" + :getPopupContainer="(trigger) => trigger.parentNode" + v-model:value="form.domainid" + showSearch + optionFilterProp="label" + :filterOption="(input, option) => { + return option.label.toLowerCase().indexOf(input.toLowerCase()) >= 0 + }" + :loading="domains.loading" + :placeholder="apiParams.domainid.description"> + <a-select-option v-for="(opt, optIndex) in domains.opts" :key="optIndex" :label="opt.path || opt.name || opt.description"> + <span> + <resource-icon v-if="opt && opt.icon" :image="opt.icon.base64image" size="1x" style="margin-right: 5px"/> + <block-outlined v-else style="margin-right: 5px" /> + {{ opt.path || opt.name || opt.description }} + </span> + </a-select-option> + </a-select> + </a-form-item> <div :span="24" class="action-button"> <a-button :loading="loading" @click="closeAction">{{ this.$t('label.cancel') }}</a-button> <a-button :loading="loading" ref="submit" type="primary" @click="handleSubmit">{{ this.$t('label.ok') }}</a-button> @@ -96,6 +123,7 @@ <script> import { ref, reactive, toRaw } from 'vue' import { getAPI, postAPI } from '@/api' +import { isAdmin } from '@/role' import ResourceIcon from '@/components/view/ResourceIcon' import TooltipLabel from '@/components/widgets/TooltipLabel' @@ -108,6 +136,10 @@ export default { data () { return { loading: false, + domains: { + loading: false, + opts: [] + }, zones: { loading: false, opts: [] @@ -129,17 +161,23 @@ export default { initForm () { this.formRef = ref() this.form = reactive({ - allowuserdrivenbackups: true + allowuserdrivenbackups: true, + ispublic: true }) this.rules = reactive({ name: [{ required: true, message: this.$t('message.error.required.input') }], description: [{ required: true, message: this.$t('message.error.required.input') }], zoneid: [{ required: true, message: this.$t('message.error.select') }], - externalid: [{ required: true, message: this.$t('message.error.select') }] + externalid: [{ required: true, message: this.$t('message.error.select') }], + domainid: [{ type: 'array', message: this.$t('message.error.select') }] }) }, + isAdmin () { + return isAdmin() + }, fetchData () { this.fetchZone() + this.fetchDomainData() }, fetchZone () { this.zones.loading = true @@ -151,6 +189,19 @@ export default { this.zones.loading = false }) }, + fetchDomainData () { + const params = {} + params.listAll = true + params.details = 'min' + this.domains.loading = true + getAPI('listDomains', params).then(json => { + this.domains.opts = json.listdomainsresponse.domain || [] + }).catch(error => { + this.$notifyError(error) + }).finally(() => { + this.domains.loading = false + }) + }, fetchExternal (zoneId) { if (!zoneId) { this.externals.opts = [] @@ -179,6 +230,20 @@ export default { params[key] = input } } + if (values.ispublic !== true) { + var domainIndexes = values.domainid + var domainId = null + if (domainIndexes && domainIndexes.length > 0) { + var domainIds = [] + for (var i = 0; i < domainIndexes.length; i++) { + domainIds = domainIds.concat(this.domains.opts[domainIndexes[i]].id) + } + domainId = domainIds.join(',') + } + if (domainId) { + params.domainid = domainId + } + } params.allowuserdrivenbackups = values.allowuserdrivenbackups this.loading = true const title = this.$t('label.import.offering')
