Prevent deletion of account and domain if either of them has deleted protected instance (#12901)

Author: sudo87

engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDao.java

@@ -21,6 +21,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 
 import com.cloud.hypervisor.Hypervisor;
 import com.cloud.utils.Pair;
@@ -192,4 +193,8 @@ List<VMInstanceVO> searchRemovedByRemoveDate(final Date startDate, final Date en
     int getVmCountByOfferingNotInDomain(Long serviceOfferingId, List<Long> domainIds);
 
     List<VMInstanceVO> listByIdsIncludingRemoved(List<Long> ids);
+
+    List<VMInstanceVO> listDeleteProtectedVmsByAccountId(long accountId);
+
+    List<VMInstanceVO> listDeleteProtectedVmsByDomainIds(Set<Long> domainIds);
 }

engine/schema/src/main/java/com/cloud/vm/dao/VMInstanceDaoImpl.java

@@ -25,11 +25,13 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Set;
 import java.util.stream.Collectors;
 
 import javax.annotation.PostConstruct;
 import javax.inject.Inject;
 
+import org.apache.cloudstack.api.ApiConstants;
 import org.apache.commons.collections.CollectionUtils;
 import org.springframework.stereotype.Component;
 
@@ -106,6 +108,8 @@ public class VMInstanceDaoImpl extends GenericDaoBase<VMInstanceVO, Long> implem
     protected SearchBuilder<VMInstanceVO> IdsPowerStateSelectSearch;
     GenericSearchBuilder<VMInstanceVO, Integer> CountByOfferingId;
     GenericSearchBuilder<VMInstanceVO, Integer> CountUserVmNotInDomain;
+    SearchBuilder<VMInstanceVO> DeleteProtectedVmSearchByAccount;
+    SearchBuilder<VMInstanceVO> DeleteProtectedVmSearchByDomainIds;
 
     @Inject
     ResourceTagDao tagsDao;
@@ -368,6 +372,19 @@ protected void init() {
         CountUserVmNotInDomain.and("domainIdsNotIn", CountUserVmNotInDomain.entity().getDomainId(), Op.NIN);
         CountUserVmNotInDomain.done();
 
+        DeleteProtectedVmSearchByAccount = createSearchBuilder();
+        DeleteProtectedVmSearchByAccount.selectFields(DeleteProtectedVmSearchByAccount.entity().getUuid());
+        DeleteProtectedVmSearchByAccount.and(ApiConstants.ACCOUNT_ID, DeleteProtectedVmSearchByAccount.entity().getAccountId(), Op.EQ);
+        DeleteProtectedVmSearchByAccount.and(ApiConstants.DELETE_PROTECTION, DeleteProtectedVmSearchByAccount.entity().isDeleteProtection(), Op.EQ);
+        DeleteProtectedVmSearchByAccount.and(ApiConstants.REMOVED, DeleteProtectedVmSearchByAccount.entity().getRemoved(), Op.NULL);
+        DeleteProtectedVmSearchByAccount.done();
+
+        DeleteProtectedVmSearchByDomainIds = createSearchBuilder();
+        DeleteProtectedVmSearchByDomainIds.selectFields(DeleteProtectedVmSearchByDomainIds.entity().getUuid());
+        DeleteProtectedVmSearchByDomainIds.and(ApiConstants.DOMAIN_IDS, DeleteProtectedVmSearchByDomainIds.entity().getDomainId(), Op.IN);
+        DeleteProtectedVmSearchByDomainIds.and(ApiConstants.DELETE_PROTECTION, DeleteProtectedVmSearchByDomainIds.entity().isDeleteProtection(), Op.EQ);
+        DeleteProtectedVmSearchByDomainIds.and(ApiConstants.REMOVED, DeleteProtectedVmSearchByDomainIds.entity().getRemoved(), Op.NULL);
+        DeleteProtectedVmSearchByDomainIds.done();
     }
 
     @Override
@@ -1296,4 +1313,22 @@ public List<VMInstanceVO> listByIdsIncludingRemoved(List<Long> ids) {
         sc.setParameters("ids", ids.toArray());
         return listIncludingRemovedBy(sc);
     }
+
+    @Override
+    public List<VMInstanceVO> listDeleteProtectedVmsByAccountId(long accountId)  {
+        SearchCriteria<VMInstanceVO> sc = DeleteProtectedVmSearchByAccount.create();
+        sc.setParameters(ApiConstants.ACCOUNT_ID, accountId);
+        sc.setParameters(ApiConstants.DELETE_PROTECTION, true);
+        Filter filter = new Filter(VMInstanceVO.class, null, false, 0L, 10L);
+        return listBy(sc, filter);
+    }
+
+    @Override
+    public List<VMInstanceVO> listDeleteProtectedVmsByDomainIds(Set<Long> domainIds)  {
+        SearchCriteria<VMInstanceVO> sc = DeleteProtectedVmSearchByDomainIds.create();
+        sc.setParameters(ApiConstants.DOMAIN_IDS, domainIds.toArray());
+        sc.setParameters(ApiConstants.DELETE_PROTECTION, true);
+        Filter filter = new Filter(VMInstanceVO.class, null, false, 0L, 10L);
+        return listBy(sc, filter);
+    }
 }

server/src/main/java/com/cloud/user/AccountManagerImpl.java

@@ -2099,6 +2099,7 @@ public boolean deleteUserAccount(long accountId) {
 
         checkIfAccountManagesProjects(accountId);
         verifyCallerPrivilegeForUserOrAccountOperations(account);
+        validateNoDeleteProtectedVmsForAccount(account);
 
         CallContext.current().putContextParameter(Account.class, account.getUuid());
 
@@ -2138,6 +2139,23 @@ protected boolean isDeleteNeeded(AccountVO account, long accountId, Account call
         return true;
     }
 
+    private void validateNoDeleteProtectedVmsForAccount(Account account) {
+        long accountId = account.getId();
+        List<VMInstanceVO> deleteProtectedVms = _vmDao.listDeleteProtectedVmsByAccountId(accountId);
+        if (CollectionUtils.isEmpty(deleteProtectedVms)) {
+            return;
+        }
+
+        if (logger.isDebugEnabled()) {
+            List<String> vmUuids = deleteProtectedVms.stream().map(VMInstanceVO::getUuid).collect(Collectors.toList());
+            logger.debug("Cannot delete Account {}, delete protection enabled for Instances: {}", account, vmUuids);
+        }
+
+        throw new InvalidParameterValueException(
+                String.format("Cannot delete Account '%s'. One or more Instances have delete protection enabled.",
+                        account.getAccountName()));
+    }
+
     @Override
     @ActionEvent(eventType = EventTypes.EVENT_ACCOUNT_ENABLE, eventDescription = "enabling account", async = true)
     public AccountVO enableAccount(String accountName, Long domainId, Long accountId) {

server/src/main/java/com/cloud/user/DomainManagerImpl.java

@@ -25,6 +25,7 @@
 import java.util.UUID;
 import java.util.regex.Matcher;
 import java.util.regex.Pattern;
+import java.util.stream.Collectors;
 
 import javax.inject.Inject;
 
@@ -104,6 +105,7 @@
 import com.cloud.utils.net.NetUtils;
 import com.cloud.vm.ReservationContext;
 import com.cloud.vm.ReservationContextImpl;
+import com.cloud.vm.VMInstanceVO;
 import com.cloud.vm.dao.VMInstanceDao;
 
 import org.apache.commons.lang3.StringUtils;
@@ -364,6 +366,8 @@ public boolean deleteDomain(long domainId, Boolean cleanup) {
         }
 
         _accountMgr.checkAccess(caller, domain);
+        // Check across the domain hierarchy (current + children) for any delete-protected instances
+        validateNoDeleteProtectedVmsForDomain(domain);
 
         return deleteDomain(domain, cleanup);
     }
@@ -724,6 +728,22 @@ protected boolean cleanupDomain(Long domainId, Long ownerId) throws ConcurrentOp
         return success && deleteDomainSuccess;
     }
 
+    private void validateNoDeleteProtectedVmsForDomain(Domain parentDomain) {
+        Set<Long> allDomainIds = getDomainChildrenIds(parentDomain.getPath());
+        List<VMInstanceVO> deleteProtectedVms = vmInstanceDao.listDeleteProtectedVmsByDomainIds(allDomainIds);
+        if (CollectionUtils.isEmpty(deleteProtectedVms)) {
+            return;
+        }
+        if (logger.isDebugEnabled()) {
+            List<String> vmUuids = deleteProtectedVms.stream().map(VMInstanceVO::getUuid).collect(Collectors.toList());
+            logger.debug("Cannot delete Domain {}, it has delete protection enabled for Instances: {}", parentDomain, vmUuids);
+        }
+
+        throw new InvalidParameterValueException(
+                String.format("Cannot delete Domain '%s'. One or more Instances have delete protection enabled.",
+                        parentDomain.getName()));
+    }
+
     @Override
     public Pair<List<? extends Domain>, Integer> searchForDomains(ListDomainsCmd cmd) {
         Account caller = getCaller();

server/src/test/java/com/cloud/user/DomainManagerImplTest.java

@@ -68,6 +68,7 @@
 import com.cloud.utils.db.SearchCriteria;
 import com.cloud.utils.exception.CloudRuntimeException;
 import com.cloud.utils.net.NetUtils;
+import com.cloud.vm.dao.VMInstanceDao;
 
 
 @RunWith(MockitoJUnitRunner.class)
@@ -123,6 +124,8 @@ public class DomainManagerImplTest {
     Account adminAccount;
     @Mock
     GlobalLock lock;
+    @Mock
+    VMInstanceDao vmInstanceDao;
 
     List<AccountVO> domainAccountsForCleanup;
     List<Long> domainNetworkIds;
@@ -213,6 +216,7 @@ public void testDeleteDomainRootDomain() {
     @Test
     public void testDeleteDomainNoCleanup() {
         Mockito.when(_configMgr.releaseDomainSpecificVirtualRanges(Mockito.any())).thenReturn(true);
+        Mockito.doReturn(Collections.emptySet()).when(domainManager).getDomainChildrenIds(Mockito.any());
         domainManager.deleteDomain(DOMAIN_ID, testDomainCleanup);
         Mockito.verify(domainManager).deleteDomain(domain, testDomainCleanup);
         Mockito.verify(domainManager).removeDomainWithNoAccountsForCleanupNetworksOrDedicatedResources(domain);
@@ -278,6 +282,7 @@ public void deleteDomain() {
         Mockito.when(_dedicatedDao.listByDomainId(Mockito.anyLong())).thenReturn(new ArrayList<DedicatedResourceVO>());
         Mockito.when(domainDaoMock.remove(Mockito.anyLong())).thenReturn(true);
         Mockito.when(_configMgr.releaseDomainSpecificVirtualRanges(Mockito.any())).thenReturn(true);
+        Mockito.doReturn(Collections.emptySet()).when(domainManager).getDomainChildrenIds(Mockito.any());
 
         try {
             Assert.assertTrue(domainManager.deleteDomain(20l, false));
@@ -309,7 +314,7 @@ public void deleteDomainCleanup() {
         Mockito.when(_resourceCountDao.removeEntriesByOwner(Mockito.anyLong(), Mockito.eq(ResourceOwnerType.Domain))).thenReturn(1l);
         Mockito.when(_resourceLimitDao.removeEntriesByOwner(Mockito.anyLong(), Mockito.eq(ResourceOwnerType.Domain))).thenReturn(1l);
         Mockito.when(_configMgr.releaseDomainSpecificVirtualRanges(Mockito.any())).thenReturn(true);
-
+        Mockito.when(vmInstanceDao.listDeleteProtectedVmsByDomainIds(Mockito.any())).thenReturn(Collections.emptyList());
         try {
             Assert.assertTrue(domainManager.deleteDomain(20l, true));
         } finally {