children = new ArrayList<>();
+ for (Backup b : backupDao.listByVmId(null, parent.getVmId())) {
+ if (b.getId() == parent.getId()) {
+ continue;
+ }
+ if (!chainId.equals(readDetail(b, NASBackupChainKeys.CHAIN_ID))) {
+ continue;
+ }
+ if (!parentUuid.equals(readDetail(b, NASBackupChainKeys.PARENT_BACKUP_ID))) {
+ continue;
+ }
+ if (isDeletePending(b)) {
+ // Tombstoned children don't keep us alive — they're already on the way out.
+ continue;
+ }
+ children.add(b);
+ }
+ return children;
+ }
+
+ /**
+ * Look up this backup's immediate parent in the chain (by {@code PARENT_BACKUP_ID}).
+ * Returns {@code null} if this is the full (no parent) or the parent row is gone.
+ *
+ * Prefer {@link #getChainOrderedLeafToRoot(Backup)} when walking the whole chain —
+ * this method hits the DB on each call and is O(N²) when used in a loop.
+ */
+ private Backup findChainParent(Backup backup) {
+ String parentUuid = readDetail(backup, NASBackupChainKeys.PARENT_BACKUP_ID);
+ if (parentUuid == null || parentUuid.isEmpty()) {
+ return null;
+ }
+ for (Backup b : backupDao.listByVmId(null, backup.getVmId())) {
+ if (parentUuid.equals(b.getUuid())) {
+ return b;
+ }
+ }
+ return null;
+ }
+
+ /**
+ * Return the chain containing {@code member}, ordered leaf-first
+ * (highest {@code CHAIN_POSITION} → root).
+ *
+ *
Materialises the chain via a single {@link BackupDao#listByVmId} call. Callers that
+ * previously walked the chain by repeatedly calling {@link #findChainParent} were doing
+ * O(N) {@code listByVmId} calls (one per ancestor); this collapses that to one.
+ *
+ *
If {@code member} has no {@code CHAIN_ID} metadata it is returned as a one-element
+ * list (it is its own degenerate chain).
+ */
+ private List getChainOrderedLeafToRoot(Backup member) {
+ String chainId = readDetail(member, NASBackupChainKeys.CHAIN_ID);
+ if (chainId == null) {
+ return Collections.singletonList(member);
+ }
+ List chain = new ArrayList<>();
+ for (Backup b : backupDao.listByVmId(null, member.getVmId())) {
+ if (chainId.equals(readDetail(b, NASBackupChainKeys.CHAIN_ID))) {
+ chain.add(b);
+ }
+ }
+ // Descending CHAIN_POSITION = leaf-first (highest position = furthest from root).
+ chain.sort((a, b) -> Integer.compare(chainPosition(b), chainPosition(a)));
+ return chain;
+ }
+
+ /**
+ * Physically delete the leaf {@code backup}, then walk up the chain while each ancestor
+ * is in delete-pending state. Mirrors the snapshot subsystem pattern: once a leaf is
+ * gone, garbage-collect any tombstoned parents.
+ *
+ * Caller must guarantee {@code backup} is a leaf (no live children). Each tombstoned
+ * ancestor is by definition childless once its sole child is deleted here, so no extra
+ * live-children check is needed inside the loop.
+ */
+ private boolean deleteLeafBackupAndSweepPendingAncestors(Backup backup, BackupRepository repo, Host host) {
+ // Snapshot the chain BEFORE the leaf delete — deleteBackupFileAndRow removes the row,
+ // after which the in-memory list still resolves but the DB no longer would.
+ List chain = getChainOrderedLeafToRoot(backup);
+ if (!deleteBackupFileAndRow(backup, repo, host)) {
+ return false;
+ }
+ // Walk the snapshot from leaf+1 upward, deleting tombstoned ancestors until a live
+ // one is reached or the root is past.
+ int leafIdx = indexOfBackupById(chain, backup.getId());
+ if (leafIdx < 0) {
+ // Leaf wasn't in its own CHAIN_ID list — degenerate case, nothing more to sweep.
+ return true;
+ }
+ for (int i = leafIdx + 1; i < chain.size(); i++) {
+ Backup ancestor = chain.get(i);
+ if (!isDeletePending(ancestor)) {
+ break;
+ }
+ if (!deleteBackupFileAndRow(ancestor, repo, host)) {
+ // Stop the sweep; the rest of the tombstoned chain will be collected on a
+ // future delete that re-runs the sweep.
+ return true;
+ }
+ }
+ return true;
+ }
+
+ /**
+ * Forced delete of {@code root}'s entire chain, leaf-first. NAS backups form a linear
+ * chain (full → inc → inc → …), not a tree, so we just walk the ordered chain and
+ * delete each member without re-querying parents.
+ */
+ private boolean cascadeDeleteSubtree(Backup root, BackupRepository repo, Host host) {
+ for (Backup b : getChainOrderedLeafToRoot(root)) {
+ if (!deleteBackupFileAndRow(b, repo, host)) {
+ return false;
+ }
+ }
+ return true;
+ }
+
+ private static int indexOfBackupById(List chain, long id) {
+ for (int i = 0; i < chain.size(); i++) {
+ if (chain.get(i).getId() == id) {
+ return i;
+ }
+ }
+ return -1;
+ }
+
+ /**
+ * Return the backup with the highest {@code CHAIN_POSITION} sharing {@code root}'s
+ * {@code CHAIN_ID}. Returns {@code root} if it has no chain metadata or is itself the tail.
+ */
+ private Backup findChainTail(Backup root) {
+ String chainId = readDetail(root, NASBackupChainKeys.CHAIN_ID);
+ if (chainId == null) {
+ return root;
+ }
+ Backup tail = root;
+ int tailPos = chainPosition(root);
+ for (Backup b : backupDao.listByVmId(null, root.getVmId())) {
+ if (b.getId() == root.getId()) {
+ continue;
+ }
+ if (!chainId.equals(readDetail(b, NASBackupChainKeys.CHAIN_ID))) {
+ continue;
+ }
+ int pos = chainPosition(b);
+ if (pos > tailPos) {
+ tail = b;
+ tailPos = pos;
+ }
+ }
+ return tail;
+ }
+
+ private int chainPosition(Backup b) {
+ String s = readDetail(b, NASBackupChainKeys.CHAIN_POSITION);
+ if (s == null) {
+ return Integer.MAX_VALUE; // no metadata => sort to end
+ }
+ try {
+ return Integer.parseInt(s);
+ } catch (NumberFormatException e) {
+ return Integer.MAX_VALUE;
+ }
}
public void syncBackupMetrics(Long zoneId) {
@@ -543,6 +1203,11 @@ public boolean assignVMToBackupOffering(VirtualMachine vm, BackupOffering backup
@Override
public boolean removeVMFromBackupOffering(VirtualMachine vm) {
+ // Clear the VM's active checkpoint so any future re-assignment to a backup offering
+ // starts a fresh chain. Without this, a detach-volume + attach-different-volume cycle
+ // while the offering is unassigned would lead to the next backup trying to rebase
+ // onto a stale parent (different volume identity, same VM id).
+ clearVmActiveCheckpoint(vm.getId());
return true;
}
@@ -629,7 +1294,9 @@ public Boolean crossZoneInstanceCreationEnabled(BackupOffering backupOffering) {
@Override
public ConfigKey>[] getConfigKeys() {
return new ConfigKey[]{
- NASBackupRestoreMountTimeout
+ NASBackupRestoreMountTimeout,
+ NASBackupFullEvery,
+ NASBackupIncrementalEnabled
};
}
diff --git a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
index a512292cd28f..3ba7dbad0416 100644
--- a/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
+++ b/plugins/backup/nas/src/test/java/org/apache/cloudstack/backup/NASBackupProviderTest.java
@@ -28,6 +28,7 @@
import org.junit.Assert;
import org.junit.Test;
import org.junit.runner.RunWith;
+import org.mockito.ArgumentCaptor;
import org.mockito.InjectMocks;
import org.mockito.Mock;
import org.mockito.Mockito;
@@ -38,22 +39,35 @@
import com.cloud.agent.AgentManager;
import com.cloud.exception.AgentUnavailableException;
import com.cloud.exception.OperationTimedoutException;
+import com.cloud.configuration.Resource;
import com.cloud.host.Host;
import com.cloud.host.HostVO;
import com.cloud.host.Status;
import com.cloud.host.dao.HostDao;
import com.cloud.hypervisor.Hypervisor;
import com.cloud.resource.ResourceManager;
+import com.cloud.storage.DiskOfferingVO;
+import com.cloud.storage.ScopeType;
+import com.cloud.storage.Storage;
import com.cloud.storage.Volume;
import com.cloud.storage.VolumeVO;
+import com.cloud.storage.dao.DiskOfferingDao;
import com.cloud.storage.dao.VolumeDao;
+import com.cloud.user.ResourceLimitService;
import com.cloud.utils.Pair;
+import com.cloud.vm.VMInstanceDetailVO;
import com.cloud.vm.VMInstanceVO;
+import com.cloud.vm.VirtualMachine;
import com.cloud.vm.dao.VMInstanceDao;
+import com.cloud.vm.dao.VMInstanceDetailsDao;
+
+import com.google.gson.Gson;
import org.apache.cloudstack.backup.dao.BackupDao;
+import org.apache.cloudstack.backup.dao.BackupDetailsDao;
import org.apache.cloudstack.backup.dao.BackupRepositoryDao;
import org.apache.cloudstack.backup.dao.BackupOfferingDao;
+import org.apache.cloudstack.engine.subsystem.api.storage.DataStoreManager;
import org.apache.cloudstack.storage.datastore.db.PrimaryDataStoreDao;
import org.apache.cloudstack.storage.datastore.db.StoragePoolVO;
@@ -96,6 +110,21 @@ public class NASBackupProviderTest {
@Mock
private VMSnapshotDao vmSnapshotDaoMock;
+ @Mock
+ private BackupDetailsDao backupDetailsDao;
+
+ @Mock
+ private VMInstanceDetailsDao vmInstanceDetailsDao;
+
+ @Mock
+ private DiskOfferingDao diskOfferingDao;
+
+ @Mock
+ private DataStoreManager dataStoreMgr;
+
+ @Mock
+ private ResourceLimitService resourceLimitMgr;
+
@Test
public void testDeleteBackup() throws OperationTimedoutException, AgentUnavailableException {
Long hostId = 1L;
@@ -353,4 +382,427 @@ public void testGetVMHypervisorHostFallbackToZoneWideKVMHost() {
Mockito.verify(hostDao).findHypervisorHostInCluster(clusterId);
Mockito.verify(resourceManager).findOneRandomRunningHostByHypervisor(Hypervisor.HypervisorType.KVM, zoneId);
}
+
+ // -- nas.backup.incremental.enabled master switch ------------------------------------
+
+ /**
+ * When the operator sets nas.backup.incremental.enabled=false at the zone level, every
+ * backup must be a fresh full anchor, regardless of VM state or nas.backup.full.every.
+ * This is a single toggle the
+ * operator can flip without having to count remaining backups in a chain.
+ */
+ @Test
+ public void decideChainReturnsLegacyFullWhenIncrementalDisabled() {
+ Long zoneId = 1L;
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ Mockito.lenient().when(vm.getDataCenterId()).thenReturn(zoneId);
+
+ // Stub the master switch to false. ConfigKey.valueIn delegates to the framework's
+ // ConfigDepot at runtime; for the unit test we override the in-memory value via the
+ // ConfigKey's local override (set by ReflectionTestUtils on the spy provider).
+ ReflectionTestUtils.setField(nasBackupProvider, "NASBackupIncrementalEnabled",
+ new org.apache.cloudstack.framework.config.ConfigKey<>("Advanced", Boolean.class,
+ "nas.backup.incremental.enabled", "false",
+ "test override — disabled", true,
+ org.apache.cloudstack.framework.config.ConfigKey.Scope.Zone));
+
+ NASBackupProvider.ChainDecision decision = nasBackupProvider.decideChain(vm);
+ Assert.assertNotNull(decision);
+ Assert.assertEquals(NASBackupChainKeys.TYPE_LEGACY_FULL, decision.mode);
+ Assert.assertNull("legacy-full must not carry a bitmap", decision.bitmapNew);
+ Assert.assertNull(decision.bitmapParent);
+ Assert.assertNull("legacy-full must not start a chain", decision.chainId);
+ Assert.assertEquals(0, decision.chainPosition);
+ }
+
+ // -- decideChain anchored on VM's active_checkpoint_id -------------------------------
+
+ /**
+ * No active_checkpoint_id on the VM (post-restore, first-ever backup, or detail purged) =>
+ * decideChain must return a fresh full. Relying on the last backup taken as the parent
+ * breaks after a restore, so the decision is anchored on the active checkpoint instead.
+ */
+ @Test
+ public void decideChainReturnsFullWhenVmHasNoActiveCheckpoint() {
+ Long zoneId = 1L;
+ Long vmId = 42L;
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ Mockito.when(vm.getId()).thenReturn(vmId);
+ Mockito.when(vm.getDataCenterId()).thenReturn(zoneId);
+ Mockito.when(vm.getState()).thenReturn(VMInstanceVO.State.Running);
+
+ // Master switch defaults to false (opt-in by zone) — explicitly enable it for this
+ // test so we exercise the "no active_checkpoint_id" branch rather than short-circuit
+ // at the master-switch gate.
+ ReflectionTestUtils.setField(nasBackupProvider, "NASBackupIncrementalEnabled",
+ new org.apache.cloudstack.framework.config.ConfigKey<>("Advanced", Boolean.class,
+ "nas.backup.incremental.enabled", "true",
+ "test override — enabled", true,
+ org.apache.cloudstack.framework.config.ConfigKey.Scope.Zone));
+
+ Mockito.when(vmInstanceDetailsDao.findDetail(vmId, NASBackupChainKeys.VM_ACTIVE_CHECKPOINT_ID)).thenReturn(null);
+
+ NASBackupProvider.ChainDecision decision = nasBackupProvider.decideChain(vm);
+ Assert.assertNotNull(decision);
+ Assert.assertEquals(NASBackupChainKeys.TYPE_FULL, decision.mode);
+ Assert.assertNull(decision.bitmapParent);
+ Assert.assertEquals(0, decision.chainPosition);
+ }
+
+ // -- incremental storage-capability guard (Ceph-RBD / Linstor stay on legacy full) ----
+
+ /**
+ * Incremental checkpoints are only possible on file-based qcow2 storage. A VM whose every
+ * volume sits on NFS / HOST-scope local / SharedMountPoint is checkpoint-capable.
+ */
+ @Test
+ public void allVolumesOnCheckpointCapableStorageTrueForNfsHostAndSharedMount() {
+ Long vmId = 55L;
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ Mockito.when(vm.getId()).thenReturn(vmId);
+
+ VolumeVO nfsVol = mock(VolumeVO.class);
+ Mockito.when(nfsVol.getPoolId()).thenReturn(1L);
+ VolumeVO hostVol = mock(VolumeVO.class);
+ Mockito.when(hostVol.getPoolId()).thenReturn(2L);
+ VolumeVO smpVol = mock(VolumeVO.class);
+ Mockito.when(smpVol.getPoolId()).thenReturn(3L);
+ Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(nfsVol, hostVol, smpVol));
+
+ StoragePoolVO nfs = mock(StoragePoolVO.class);
+ Mockito.when(nfs.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem);
+ StoragePoolVO host = mock(StoragePoolVO.class);
+ Mockito.when(host.getScope()).thenReturn(ScopeType.HOST);
+ StoragePoolVO smp = mock(StoragePoolVO.class);
+ Mockito.when(smp.getPoolType()).thenReturn(Storage.StoragePoolType.SharedMountPoint);
+ Mockito.when(storagePoolDao.findById(1L)).thenReturn(nfs);
+ Mockito.when(storagePoolDao.findById(2L)).thenReturn(host);
+ Mockito.when(storagePoolDao.findById(3L)).thenReturn(smp);
+
+ Assert.assertTrue(nasBackupProvider.allVolumesOnCheckpointCapableStorage(vm));
+ }
+
+ /**
+ * A single volume on Ceph-RBD (or any pool that cannot carry a per-disk checkpoint) forces
+ * the whole VM onto the legacy full-only path — avoids regressing RBD/Linstor storages.
+ */
+ @Test
+ public void allVolumesOnCheckpointCapableStorageFalseWhenAnyVolumeOnRbd() {
+ Long vmId = 56L;
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ Mockito.when(vm.getId()).thenReturn(vmId);
+
+ VolumeVO nfsVol = mock(VolumeVO.class);
+ Mockito.when(nfsVol.getPoolId()).thenReturn(1L);
+ VolumeVO rbdVol = mock(VolumeVO.class);
+ Mockito.when(rbdVol.getPoolId()).thenReturn(9L);
+ Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(nfsVol, rbdVol));
+
+ StoragePoolVO nfs = mock(StoragePoolVO.class);
+ Mockito.when(nfs.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem);
+ StoragePoolVO rbd = mock(StoragePoolVO.class);
+ Mockito.when(rbd.getPoolType()).thenReturn(Storage.StoragePoolType.RBD);
+ Mockito.when(storagePoolDao.findById(1L)).thenReturn(nfs);
+ Mockito.when(storagePoolDao.findById(9L)).thenReturn(rbd);
+
+ Assert.assertFalse(nasBackupProvider.allVolumesOnCheckpointCapableStorage(vm));
+ }
+
+ /** A volume whose storage pool can no longer be resolved is treated as incapable (safe). */
+ @Test
+ public void allVolumesOnCheckpointCapableStorageFalseWhenPoolUnresolvable() {
+ Long vmId = 57L;
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ Mockito.when(vm.getId()).thenReturn(vmId);
+ VolumeVO vol = mock(VolumeVO.class);
+ Mockito.when(vol.getPoolId()).thenReturn(1L);
+ Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(List.of(vol));
+ Mockito.when(storagePoolDao.findById(1L)).thenReturn(null);
+
+ Assert.assertFalse(nasBackupProvider.allVolumesOnCheckpointCapableStorage(vm));
+ }
+
+ // -- restore clears active_checkpoint_id ---------------------------------------------
+
+ /**
+ * After a successful restoreVMFromBackup, decideChain on the next backup must produce
+ * a full. We verify this end-to-end by checking that vmInstanceDetailsDao.removeDetail
+ * is called with the active_checkpoint_id key.
+ */
+ @Test
+ public void restoreClearsActiveCheckpointDetail() throws AgentUnavailableException, OperationTimedoutException {
+ Long vmId = 7L;
+ Long hostId = 8L;
+ Long backupOfferingId = 9L;
+
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ Mockito.when(vm.getId()).thenReturn(vmId);
+ Mockito.when(vm.getLastHostId()).thenReturn(hostId);
+ Mockito.when(vm.getRemoved()).thenReturn(null);
+ Mockito.when(vm.getName()).thenReturn("vm7");
+
+ HostVO host = mock(HostVO.class);
+ Mockito.when(host.getStatus()).thenReturn(Status.Up);
+ Mockito.when(host.getId()).thenReturn(hostId);
+ Mockito.when(hostDao.findById(hostId)).thenReturn(host);
+
+ BackupVO backup = new BackupVO();
+ backup.setVmId(vmId);
+ backup.setBackupOfferingId(backupOfferingId);
+ backup.setExternalId("i-2-7-VM/2026.05.16.10.00.00");
+ ReflectionTestUtils.setField(backup, "id", 100L);
+ // backedUpVolumes defaults to null => BackupVO.getBackedUpVolumes returns emptyList().
+
+ BackupRepositoryVO repo = new BackupRepositoryVO(1L, "nas", "test-repo",
+ "nfs", "address", "sync", 1024L, null);
+ Mockito.when(backupRepositoryDao.findByBackupOfferingId(backupOfferingId)).thenReturn(repo);
+
+ Mockito.when(volumeDao.findByInstance(vmId)).thenReturn(Collections.emptyList());
+
+ BackupAnswer answer = mock(BackupAnswer.class);
+ Mockito.when(answer.getResult()).thenReturn(true);
+ Mockito.when(agentManager.send(Mockito.anyLong(), Mockito.any(RestoreBackupCommand.class))).thenReturn(answer);
+
+ // Pre-existing checkpoint detail so removeDetail has something to "clear".
+ VMInstanceDetailVO existing = mock(VMInstanceDetailVO.class);
+ Mockito.when(existing.getValue()).thenReturn("backup-1715000000");
+ Mockito.when(vmInstanceDetailsDao.findDetail(vmId, NASBackupChainKeys.VM_ACTIVE_CHECKPOINT_ID)).thenReturn(existing);
+
+ boolean ok = nasBackupProvider.restoreVMFromBackup(vm, backup);
+ Assert.assertTrue(ok);
+ Mockito.verify(vmInstanceDetailsDao).removeDetail(vmId, NASBackupChainKeys.VM_ACTIVE_CHECKPOINT_ID);
+ }
+
+ /**
+ * Single-volume restore (restoreBackedUpVolume) must also clear the target VM's
+ * active_checkpoint_id, so the next backup of that VM is a fresh full — the restored
+ * volume's image carries no QEMU bitmap.
+ */
+ @Test
+ public void restoreBackedUpVolumeClearsTargetVmActiveCheckpoint()
+ throws AgentUnavailableException, OperationTimedoutException {
+ Long targetVmId = 42L;
+ Long backupOfferingId = 9L;
+ String targetVmName = "i-2-42-VM";
+ String volUuid = "vol-uuid-1";
+ String hostIp = "10.0.0.5";
+ String dsUuid = "ds-uuid-1";
+
+ VolumeVO srcVolume = mock(VolumeVO.class);
+ Mockito.when(srcVolume.getUuid()).thenReturn(volUuid);
+ Mockito.when(srcVolume.getName()).thenReturn("data1");
+ Mockito.when(volumeDao.findByUuid(volUuid)).thenReturn(srcVolume);
+
+ DiskOfferingVO diskOffering = mock(DiskOfferingVO.class);
+ Mockito.when(diskOffering.getId()).thenReturn(5L);
+ Mockito.when(diskOffering.getProvisioningType()).thenReturn(Storage.ProvisioningType.THIN);
+ Mockito.when(diskOfferingDao.findByUuid(Mockito.anyString())).thenReturn(diskOffering);
+
+ StoragePoolVO pool = mock(StoragePoolVO.class);
+ Mockito.when(pool.getId()).thenReturn(11L);
+ Mockito.when(pool.getPoolType()).thenReturn(Storage.StoragePoolType.NetworkFilesystem);
+ Mockito.when(storagePoolDao.findByUuid(dsUuid)).thenReturn(pool);
+
+ HostVO host = mock(HostVO.class);
+ Mockito.when(host.getId()).thenReturn(8L);
+ Mockito.when(hostDao.findByIp(hostIp)).thenReturn(host);
+
+ Backup.VolumeInfo backedUp = new Backup.VolumeInfo(volUuid, "i-2-99-VM/2026/data1.qcow2",
+ Volume.Type.DATADISK, 1024L, 1L, "disk-offering-uuid", null, null);
+
+ BackupVO backup = new BackupVO();
+ backup.setVmId(99L);
+ backup.setBackupOfferingId(backupOfferingId);
+ backup.setExternalId("i-2-99-VM/2026.06.22.10.00.00");
+ backup.setSize(1024L);
+ backup.setBackedUpVolumes(new Gson().toJson(Collections.singletonList(backedUp)));
+ ReflectionTestUtils.setField(backup, "id", 200L);
+
+ BackupRepositoryVO repo = new BackupRepositoryVO(1L, "nas", "test-repo",
+ "nfs", "address", "sync", 1024L, null);
+ Mockito.when(backupRepositoryDao.findByBackupOfferingId(backupOfferingId)).thenReturn(repo);
+
+ BackupAnswer answer = mock(BackupAnswer.class);
+ Mockito.when(answer.getResult()).thenReturn(true);
+ Mockito.when(agentManager.send(Mockito.anyLong(), Mockito.any(RestoreBackupCommand.class))).thenReturn(answer);
+
+ VMInstanceVO targetVm = mock(VMInstanceVO.class);
+ Mockito.when(targetVm.getId()).thenReturn(targetVmId);
+ Mockito.when(vmInstanceDao.findVMByInstanceName(targetVmName)).thenReturn(targetVm);
+
+ VMInstanceDetailVO existing = mock(VMInstanceDetailVO.class);
+ Mockito.when(existing.getValue()).thenReturn("backup-1718000000");
+ Mockito.when(vmInstanceDetailsDao.findDetail(targetVmId, NASBackupChainKeys.VM_ACTIVE_CHECKPOINT_ID)).thenReturn(existing);
+
+ Pair result = nasBackupProvider.restoreBackedUpVolume(
+ backup, backedUp, hostIp, dsUuid, new Pair<>(targetVmName, VirtualMachine.State.Stopped));
+
+ Assert.assertTrue(result.first());
+ Mockito.verify(vmInstanceDetailsDao).removeDetail(targetVmId, NASBackupChainKeys.VM_ACTIVE_CHECKPOINT_ID);
+ }
+
+ // -- delete-pending cascade ----------------------------------------------------------
+
+ /**
+ * Deleting an incremental that has a live child must mark the incremental as
+ * delete-pending in backup_details and NOT touch the on-NAS file or the backups row.
+ * A parent with live children is soft-deleted (delete-pending) rather than removed.
+ */
+ @Test
+ public void deleteWithLiveChildMarksDeletePendingAndPreservesFile()
+ throws AgentUnavailableException, OperationTimedoutException {
+ Long zoneId = 1L;
+ Long vmId = 2L;
+ Long hostId = 3L;
+ Long offeringId = 4L;
+
+ BackupVO parent = new BackupVO();
+ parent.setVmId(vmId);
+ parent.setBackupOfferingId(offeringId);
+ parent.setExternalId("i-2-2-VM/2026.05.10.10.00.00");
+ parent.setZoneId(zoneId);
+ ReflectionTestUtils.setField(parent, "id", 50L);
+ ReflectionTestUtils.setField(parent, "uuid", "parent-uuid");
+
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ Mockito.when(vm.getLastHostId()).thenReturn(hostId);
+ HostVO host = mock(HostVO.class);
+ Mockito.when(host.getStatus()).thenReturn(Status.Up);
+ // Note: host.getId() is intentionally not stubbed — the live-child path never
+ // contacts the agent (verified below), so the stub would be unnecessary.
+ Mockito.when(hostDao.findById(hostId)).thenReturn(host);
+
+ BackupRepositoryVO repo = new BackupRepositoryVO(1L, "nas", "test-repo",
+ "nfs", "address", "sync", 1024L, null);
+ Mockito.when(backupRepositoryDao.findByBackupOfferingId(offeringId)).thenReturn(repo);
+ Mockito.when(vmInstanceDao.findByIdIncludingRemoved(vmId)).thenReturn(vm);
+
+ // CHAIN_ID on the parent => not the no-chain fast path.
+ BackupDetailVO chainIdDetail = new BackupDetailVO(50L, NASBackupChainKeys.CHAIN_ID, "chain-1", true);
+ Mockito.when(backupDetailsDao.findDetail(50L, NASBackupChainKeys.CHAIN_ID)).thenReturn(chainIdDetail);
+
+ // A live child references parent-uuid via PARENT_BACKUP_ID.
+ BackupVO child = new BackupVO();
+ child.setVmId(vmId);
+ child.setBackupOfferingId(offeringId);
+ child.setExternalId("i-2-2-VM/2026.05.10.10.30.00");
+ child.setZoneId(zoneId);
+ child.setStatus(Backup.Status.BackedUp);
+ ReflectionTestUtils.setField(child, "id", 51L);
+ ReflectionTestUtils.setField(child, "uuid", "child-uuid");
+
+ BackupDetailVO childChainId = new BackupDetailVO(51L, NASBackupChainKeys.CHAIN_ID, "chain-1", true);
+ BackupDetailVO childParent = new BackupDetailVO(51L, NASBackupChainKeys.PARENT_BACKUP_ID, "parent-uuid", true);
+ Mockito.when(backupDetailsDao.findDetail(51L, NASBackupChainKeys.CHAIN_ID)).thenReturn(childChainId);
+ Mockito.when(backupDetailsDao.findDetail(51L, NASBackupChainKeys.PARENT_BACKUP_ID)).thenReturn(childParent);
+
+ Mockito.when(backupDao.listByVmId(null, vmId)).thenReturn(List.of(parent, child));
+ // markDeletePending loads the row to flip its status to Hidden.
+ Mockito.when(backupDao.findById(50L)).thenReturn(parent);
+
+ boolean result = nasBackupProvider.deleteBackup(parent, false);
+ Assert.assertTrue(result);
+
+ // No agent traffic — the on-NAS file must be preserved while children are alive.
+ Mockito.verify(agentManager, Mockito.never()).send(Mockito.anyLong(), Mockito.any(DeleteBackupCommand.class));
+ // No DB row removal — the row is the tombstone marker.
+ Mockito.verify(backupDao, Mockito.never()).remove(50L);
+ // A tombstoned backup is NOT decremented — its space is still occupied until swept.
+ Mockito.verify(resourceLimitMgr, Mockito.never()).decrementResourceCount(Mockito.anyLong(), Mockito.eq(Resource.ResourceType.backup));
+ // The tombstoned backup is moved to Status.Hidden (replaces the old DELETE_PENDING detail).
+ ArgumentCaptor captor = ArgumentCaptor.forClass(BackupVO.class);
+ Mockito.verify(backupDao).update(Mockito.eq(50L), captor.capture());
+ Assert.assertEquals(Backup.Status.Hidden, captor.getValue().getStatus());
+ Mockito.verify(backupDetailsDao, Mockito.never()).persist(Mockito.any(BackupDetailVO.class));
+ }
+
+ /**
+ * Deleting a leaf incremental whose parent is delete-pending must (a) delete the leaf and
+ * then (b) sweep up the tombstoned parent. Mirrors DefaultSnapshotStrategy's
+ * "delete leaf, then walk up while parent is destroying-and-childless".
+ */
+ @Test
+ public void deletingLeafSweepsUpDeletePendingParent()
+ throws AgentUnavailableException, OperationTimedoutException {
+ Long zoneId = 1L;
+ Long vmId = 2L;
+ Long hostId = 3L;
+ Long offeringId = 4L;
+
+ BackupVO leaf = new BackupVO();
+ leaf.setVmId(vmId);
+ leaf.setBackupOfferingId(offeringId);
+ leaf.setExternalId("i-2-2-VM/2026.05.10.11.00.00");
+ leaf.setZoneId(zoneId);
+ ReflectionTestUtils.setField(leaf, "id", 51L);
+ ReflectionTestUtils.setField(leaf, "uuid", "leaf-uuid");
+
+ BackupVO parent = new BackupVO();
+ parent.setVmId(vmId);
+ parent.setBackupOfferingId(offeringId);
+ parent.setExternalId("i-2-2-VM/2026.05.10.10.30.00");
+ parent.setZoneId(zoneId);
+ ReflectionTestUtils.setField(parent, "id", 50L);
+ ReflectionTestUtils.setField(parent, "uuid", "parent-uuid");
+
+ VMInstanceVO vm = mock(VMInstanceVO.class);
+ Mockito.when(vm.getLastHostId()).thenReturn(hostId);
+ HostVO host = mock(HostVO.class);
+ Mockito.when(host.getStatus()).thenReturn(Status.Up);
+ Mockito.when(host.getId()).thenReturn(hostId);
+ Mockito.when(hostDao.findById(hostId)).thenReturn(host);
+
+ BackupRepositoryVO repo = new BackupRepositoryVO(1L, "nas", "test-repo",
+ "nfs", "address", "sync", 1024L, null);
+ Mockito.when(backupRepositoryDao.findByBackupOfferingId(offeringId)).thenReturn(repo);
+ Mockito.when(vmInstanceDao.findByIdIncludingRemoved(vmId)).thenReturn(vm);
+
+ // Leaf details. CHAIN_POSITION=1 puts the leaf after the full anchor in the
+ // ordered chain — getChainOrderedLeafToRoot sorts by CHAIN_POSITION descending.
+ BackupDetailVO leafChainId = new BackupDetailVO(51L, NASBackupChainKeys.CHAIN_ID, "chain-1", true);
+ BackupDetailVO leafChainPos = new BackupDetailVO(51L, NASBackupChainKeys.CHAIN_POSITION, "1", true);
+ Mockito.when(backupDetailsDao.findDetail(51L, NASBackupChainKeys.CHAIN_ID)).thenReturn(leafChainId);
+ Mockito.when(backupDetailsDao.findDetail(51L, NASBackupChainKeys.CHAIN_POSITION)).thenReturn(leafChainPos);
+
+ // Parent is the tombstoned full anchor (CHAIN_POSITION=0).
+ BackupDetailVO parentChainId = new BackupDetailVO(50L, NASBackupChainKeys.CHAIN_ID, "chain-1", true);
+ BackupDetailVO parentChainPos = new BackupDetailVO(50L, NASBackupChainKeys.CHAIN_POSITION, "0", true);
+ // The parent is the tombstone — now represented by Status.Hidden (was the DELETE_PENDING detail).
+ parent.setStatus(Backup.Status.Hidden);
+ Mockito.when(backupDetailsDao.findDetail(50L, NASBackupChainKeys.CHAIN_ID)).thenReturn(parentChainId);
+ Mockito.when(backupDetailsDao.findDetail(50L, NASBackupChainKeys.CHAIN_POSITION)).thenReturn(parentChainPos);
+ // Parent has no parent of its own (it's the full anchor).
+ Mockito.when(backupDetailsDao.findDetail(50L, NASBackupChainKeys.PARENT_BACKUP_ID)).thenReturn(null);
+
+ // listByVmId is called once now (chain snapshot taken before the leaf delete).
+ // We still use a mutable list + remove() answer so the DAO contract is realistic.
+ java.util.List liveBackups = new java.util.ArrayList<>(List.of(parent, leaf));
+ Mockito.when(backupDao.listByVmId(null, vmId)).thenAnswer(inv -> new java.util.ArrayList<>(liveBackups));
+
+ // Agent acknowledges every delete.
+ Mockito.when(agentManager.send(Mockito.anyLong(), Mockito.any(DeleteBackupCommand.class)))
+ .thenReturn(new BackupAnswer(new DeleteBackupCommand(null, null, null, null), true, "ok"));
+ // backupDao.remove(id) drops the corresponding row from the live list so the next
+ // listByVmId call reflects post-delete state — mirrors the real DAO contract.
+ Mockito.when(backupDao.remove(Mockito.anyLong())).thenAnswer(inv -> {
+ Long id = inv.getArgument(0);
+ liveBackups.removeIf(b -> b.getId() == id);
+ return true;
+ });
+
+ boolean result = nasBackupProvider.deleteBackup(leaf, false);
+ Assert.assertTrue(result);
+
+ // Both backups must be physically deleted (leaf first, then tombstoned parent).
+ Mockito.verify(agentManager, Mockito.times(2))
+ .send(Mockito.anyLong(), Mockito.any(DeleteBackupCommand.class));
+ Mockito.verify(backupDao).remove(51L);
+ Mockito.verify(backupDao).remove(50L);
+ // Exactly-once resource accounting: decremented for BOTH physically-removed backups
+ // (leaf + swept ancestor), not just one.
+ Mockito.verify(resourceLimitMgr, Mockito.times(2))
+ .decrementResourceCount(Mockito.anyLong(), Mockito.eq(Resource.ResourceType.backup));
+ Mockito.verify(resourceLimitMgr, Mockito.times(2))
+ .decrementResourceCount(Mockito.anyLong(), Mockito.eq(Resource.ResourceType.backup_storage), Mockito.any());
+ }
}
diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java
index 22dbfbdd67a2..cc2a0868fe17 100644
--- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java
+++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapper.java
@@ -60,6 +60,15 @@ public class LibvirtRestoreBackupCommandWrapper extends CommandWrapper/dev/null | grep -q '\"backing-filename\"'";
private String getVolumeUuidFromPath(String volumePath, PrimaryDataStoreTO volumePool) {
if (Storage.StoragePoolType.Linstor.equals(volumePool.getPoolType())) {
@@ -270,10 +279,27 @@ private boolean replaceVolumeWithBackup(KVMStoragePoolManager storagePoolMgr, Pr
return replaceBlockDeviceWithBackup(storagePoolMgr, volumePool, volumePath, backupPath, timeout, createTargetVolume, size);
}
+ // For NAS-backed incremental backups, the source qcow2 has a backing-file
+ // reference to its parent (set by nasbackup.sh's qemu-img rebase). A plain
+ // rsync would copy only the differential blocks, leaving a volume that
+ // depends on a backing file the primary storage doesn't have. Flatten the
+ // chain via qemu-img convert, which follows the backing-file links and
+ // produces a single self-contained qcow2.
+ if (hasBackingChain(backupPath)) {
+ int flattenExit = Script.runSimpleBashScriptForExitValue(
+ String.format(QEMU_IMG_FLATTEN_COMMAND, backupPath, volumePath), timeout, false);
+ return flattenExit == 0;
+ }
+
int exitValue = Script.runSimpleBashScriptForExitValue(String.format(RSYNC_COMMAND, backupPath, volumePath), timeout, false);
return exitValue == 0;
}
+ private boolean hasBackingChain(String qcow2Path) {
+ return Script.runSimpleBashScriptForExitValue(
+ String.format(QEMU_IMG_HAS_BACKING_COMMAND, qcow2Path)) == 0;
+ }
+
private boolean replaceBlockDeviceWithBackup(KVMStoragePoolManager storagePoolMgr, PrimaryDataStoreTO volumePool, String volumePath, String backupPath, int timeout, boolean createTargetVolume, Long size) {
KVMStoragePool volumeStoragePool = storagePoolMgr.getStoragePool(volumePool.getPoolType(), volumePool.getUuid());
QemuImg qemu;
diff --git a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java
index 42953aa9f835..106fe31a0f18 100644
--- a/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java
+++ b/plugins/hypervisors/kvm/src/main/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtTakeBackupCommandWrapper.java
@@ -42,6 +42,16 @@
@ResourceWrapper(handles = TakeBackupCommand.class)
public class LibvirtTakeBackupCommandWrapper extends CommandWrapper {
private static final Integer EXIT_CLEANUP_FAILED = 20;
+ // nasbackup.sh prints this on stdout when it could not proceed as an incremental and
+ // completed a full backup instead; the orchestrator then records the backup as a full.
+ private static final String INCREMENTAL_FALLBACK_MARKER = "INCREMENTAL_FALLBACK=true";
+
+ private static final String MODE_FULL = "full";
+ private static final String MODE_INCREMENTAL = "incremental";
+ // Incremental feature disabled: plain full backup with no QEMU bitmap/checkpoint and no
+ // chain metadata. Matches nasbackup.sh's "legacy-full" mode (make_checkpoint=0).
+ private static final String MODE_LEGACY_FULL = "legacy-full";
+
@Override
public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvirtComputingResource) {
final String vmName = command.getVmName();
@@ -54,6 +64,13 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
KVMStoragePoolManager storagePoolMgr = libvirtComputingResource.getStoragePoolMgr();
int timeout = command.getWait() > 0 ? command.getWait() * 1000 : libvirtComputingResource.getCmdsTimeout();
+ // Pre-validate incremental args here rather than relying on the script to error out.
+ // Keeps the script agnostic to caller policy (it just does what it's told).
+ String validationError = validateBackupArgs(command);
+ if (validationError != null) {
+ return new BackupAnswer(command, false, validationError);
+ }
+
List diskPaths = new ArrayList<>();
if (Objects.nonNull(volumePaths)) {
for (int idx = 0; idx < volumePaths.size(); idx++) {
@@ -69,8 +86,63 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
}
}
- List commands = new ArrayList<>();
- commands.add(new String[]{
+ Pair result = runBackupScript(libvirtComputingResource, command, vmName, backupRepoType, backupRepoAddress,
+ mountOptions, backupPath, diskPaths, command.getMode(),
+ command.getBitmapNew(), command.getBitmapParent(), command.getParentPaths(), timeout);
+
+ if (result.first() != 0) {
+ logger.debug("Failed to take VM backup: " + result.second());
+ BackupAnswer answer = new BackupAnswer(command, false, result.second().trim());
+ if (EXIT_CLEANUP_FAILED.equals(result.first())) {
+ logger.debug("Backup cleanup failed");
+ answer.setNeedsCleanup(true);
+ }
+ return answer;
+ }
+
+ // The script self-heals to a full backup when an incremental can't proceed (e.g. the
+ // parent checkpoint can't be re-registered) and signals it with INCREMENTAL_FALLBACK
+ // on stdout. Detect it, then strip the marker line before parsing the backup size.
+ String rawStdout = result.second();
+ boolean incrementalFallback = rawStdout.contains(INCREMENTAL_FALLBACK_MARKER);
+ String stdout = stripMarkerLines(rawStdout).trim();
+ long backupSize = parseBackupSize(stdout, diskPaths);
+
+ BackupAnswer answer = new BackupAnswer(command, true, stdout);
+ answer.setSize(backupSize);
+ // A successful run always created command.getBitmapNew() (full and incremental both do;
+ // it is null for legacy-full, which the orchestrator treats as "no bitmap").
+ answer.setBitmapCreated(command.getBitmapNew());
+ answer.setIncrementalFallback(incrementalFallback);
+ return answer;
+ }
+
+ /** Remove nasbackup.sh's stdout signalling marker lines so they don't pollute size parsing. */
+ private String stripMarkerLines(String stdout) {
+ if (stdout == null || stdout.isEmpty()) {
+ return "";
+ }
+ StringBuilder sb = new StringBuilder();
+ for (String line : stdout.split("\n", -1)) {
+ if (line.contains(INCREMENTAL_FALLBACK_MARKER)) {
+ continue;
+ }
+ if (sb.length() > 0) {
+ sb.append('\n');
+ }
+ sb.append(line);
+ }
+ return sb.toString();
+ }
+
+ /**
+ * Run nasbackup.sh once with the given args. Returns the exit code + captured stdout.
+ */
+ private Pair runBackupScript(LibvirtComputingResource libvirtComputingResource,
+ TakeBackupCommand command, String vmName, String backupRepoType, String backupRepoAddress,
+ String mountOptions, String backupPath, List diskPaths, String mode,
+ String bitmapNew, String bitmapParent, List parentPaths, int timeout) {
+ List argv = new ArrayList<>(Arrays.asList(
libvirtComputingResource.getNasBackupPath(),
"-o", "backup",
"-v", vmName,
@@ -80,35 +152,79 @@ public Answer execute(TakeBackupCommand command, LibvirtComputingResource libvir
"-p", backupPath,
"-q", command.getQuiesce() != null && command.getQuiesce() ? "true" : "false",
"-d", diskPaths.isEmpty() ? "" : String.join(",", diskPaths)
- });
+ ));
+ if (mode != null && !mode.isEmpty()) {
+ argv.add("-M");
+ argv.add(mode);
+ }
+ if (bitmapNew != null && !bitmapNew.isEmpty()) {
+ argv.add("--bitmap-new");
+ argv.add(bitmapNew);
+ }
+ if (bitmapParent != null && !bitmapParent.isEmpty()) {
+ argv.add("--bitmap-parent");
+ argv.add(bitmapParent);
+ }
+ if (parentPaths != null && !parentPaths.isEmpty()) {
+ argv.add("--parent-paths");
+ argv.add(String.join(",", parentPaths));
+ }
- Pair result = Script.executePipedCommands(commands, timeout);
+ List commands = new ArrayList<>();
+ commands.add(argv.toArray(new String[0]));
+ return Script.executePipedCommands(commands, timeout);
+ }
- if (result.first() != 0) {
- logger.debug("Failed to take VM backup: " + result.second());
- BackupAnswer answer = new BackupAnswer(command, false, result.second().trim());
- if (result.first() == EXIT_CLEANUP_FAILED) {
- logger.debug("Backup cleanup failed");
- answer.setNeedsCleanup(true);
+ /**
+ * Return a human-readable validation error string, or {@code null} if the command's
+ * incremental-backup args are internally consistent.
+ */
+ private String validateBackupArgs(TakeBackupCommand command) {
+ String mode = command.getMode();
+ if (mode == null || mode.isEmpty()) {
+ return null; // legacy full-only — no extra args expected
+ }
+ if (MODE_INCREMENTAL.equals(mode)) {
+ if (command.getBitmapNew() == null || command.getBitmapNew().isEmpty()) {
+ return "incremental mode requires bitmapNew";
}
- return answer;
+ if (command.getBitmapParent() == null || command.getBitmapParent().isEmpty()) {
+ return "incremental mode requires bitmapParent";
+ }
+ if (command.getParentPaths() == null || command.getParentPaths().isEmpty()) {
+ return "incremental mode requires parentPaths";
+ }
+ return null;
+ }
+ if (MODE_FULL.equals(mode)) {
+ if (command.getBitmapNew() == null || command.getBitmapNew().isEmpty()) {
+ return "full mode requires bitmapNew (the bitmap to create for the next incremental)";
+ }
+ return null;
+ }
+ if (MODE_LEGACY_FULL.equals(mode)) {
+ return null; // feature-off full backup — no bitmap or chain args expected
}
+ return "Unknown backup mode: " + mode;
+ }
+ /**
+ * Sum the per-disk size lines emitted by nasbackup.sh. Single-volume mode emits one
+ * line containing just the byte count; multi-volume mode emits one line per disk
+ * whose first whitespace-separated token is the byte count.
+ */
+ private long parseBackupSize(String stdout, List diskPaths) {
long backupSize = 0L;
if (CollectionUtils.isNullOrEmpty(diskPaths)) {
- List outputLines = Arrays.asList(result.second().trim().split("\n"));
+ List outputLines = Arrays.asList(stdout.split("\n"));
if (!outputLines.isEmpty()) {
backupSize = Long.parseLong(outputLines.get(outputLines.size() - 1).trim());
}
} else {
- String[] outputLines = result.second().trim().split("\n");
- for(String line : outputLines) {
+ for (String line : stdout.split("\n")) {
backupSize = backupSize + Long.parseLong(line.split(" ")[0].trim());
}
}
-
- BackupAnswer answer = new BackupAnswer(command, true, result.second().trim());
- answer.setSize(backupSize);
- return answer;
+ return backupSize;
}
}
diff --git a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java
index ef6b5c08189d..fd8a3b02e0a0 100644
--- a/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java
+++ b/plugins/hypervisors/kvm/src/test/java/com/cloud/hypervisor/kvm/resource/wrapper/LibvirtRestoreBackupCommandWrapperTest.java
@@ -407,6 +407,8 @@ public void testExecuteWithRsyncFailure() throws Exception {
return 0; // File exists
} else if (command.contains("qemu-img check")) {
return 0; // File is valid
+ } else if (command.contains("qemu-img info") && command.contains("backing-filename")) {
+ return 1; // No backing chain — exercise the rsync path (full backups)
}
return 0; // Other commands success
});
diff --git a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java
index 1a8d9f7b59e2..287601d47d6b 100644
--- a/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java
+++ b/plugins/hypervisors/vmware/src/main/java/com/cloud/hypervisor/guru/VMwareGuru.java
@@ -1164,7 +1164,7 @@ private ManagedObjectReference getDestStoreMor(VirtualMachineMO vmMo) throws Exc
@Override
public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId, long accountId, long userId, String vmInternalName, Backup backup) throws Exception {
logger.debug(String.format("Trying to import VM [vmInternalName: %s] from Backup [%s].", vmInternalName,
- ReflectionToStringBuilderUtils.reflectOnlySelectedFields(backup, "id", "uuid", "vmId", "externalId", "backupType")));
+ ReflectionToStringBuilderUtils.reflectOnlySelectedFields(backup, "id", "uuid", "vmId", "externalId", "type")));
DatacenterMO dcMo = getDatacenterMO(zoneId);
VirtualMachineMO vmToImport = dcMo.findVm(vmInternalName);
if (vmToImport == null) {
diff --git a/scripts/vm/hypervisor/kvm/nasbackup.sh b/scripts/vm/hypervisor/kvm/nasbackup.sh
index b35908a433ed..d252b1f4e978 100755
--- a/scripts/vm/hypervisor/kvm/nasbackup.sh
+++ b/scripts/vm/hypervisor/kvm/nasbackup.sh
@@ -33,6 +33,15 @@ MOUNT_OPTS=""
BACKUP_DIR=""
DISK_PATHS=""
QUIESCE=""
+# Incremental backup parameters (all optional; legacy callers omit them)
+MODE="" # "full" or "incremental"; empty => legacy full-only behavior (no checkpoint created)
+BITMAP_NEW="" # Bitmap/checkpoint name to create with this backup (e.g. "backup-1711586400")
+BITMAP_PARENT="" # For incremental: parent bitmap name to read changes since
+PARENT_PATHS="" # For incremental: comma-separated list of parent backup file paths,
+ # one per VM volume in the same order as DISK_PATHS. Each new qcow2
+ # is rebased onto its corresponding parent file. Required because
+ # data-disk backup files don't share the root volume's UUID, so
+ # each disk must be rebased onto its own parent.
logFile="/var/log/cloudstack/agent/agent.log"
UNMOUNT_TIMEOUT=60
EXIT_CLEANUP_FAILED=20
@@ -113,20 +122,117 @@ backup_running_vm() {
mount_operation
mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit 1; }
+ # Determine effective mode for this run.
+ # Legacy callers (no -M argument) get the original full-only behavior with no checkpoint.
+ # The Java wrapper (LibvirtTakeBackupCommandWrapper) pre-validates required args before
+ # invoking the script; the case below is a defensive fallback for direct invocations.
+ local effective_mode="${MODE:-legacy-full}"
+ local make_checkpoint=0
+ case "$effective_mode" in
+ incremental|full)
+ make_checkpoint=1
+ ;;
+ legacy-full)
+ make_checkpoint=0
+ ;;
+ *)
+ echo "Unknown mode: $effective_mode"
+ cleanup
+ exit 1
+ ;;
+ esac
+
+ # Incremental needs the parent checkpoint registered with libvirt. CloudStack rebuilds the
+ # domain XML on every VM start, wiping libvirt's checkpoint registry while the dirty bitmap
+ # persists on the qcow2, so a fresh checkpoint-create fails with "Bitmap already exists".
+ # Re-register the parent with --redefine (needs only a name + creationTime) via a minimal
+ # synthesized XML. If the parent bitmap is missing from the qcow2 (e.g. after a migration),
+ # fall back to a full backup instead of letting backup-begin fail below.
+ if [[ "$effective_mode" == "incremental" ]]; then
+ # The parent bitmap must be present on EVERY disk, not just one. A snapshot restore or partial
+ # migration can wipe it on some disks; require it on all by comparing the disk count to the
+ # number of disks that carry it.
+ disk_count=$(virsh -c qemu:///system domblklist "$VM" --details 2>/dev/null | awk '$2=="disk"{c++} END{print c+0}')
+ # Count per-device (one per inserted.file whose dirty-bitmaps holds the parent), mirroring
+ # getVmDiskPathHasFromCheckpointMap(): query-block lists a bitmap under multiple nodes, so raw
+ # name matches double-count. "|| echo 0" keeps a no-match from aborting under "set -eo pipefail"
+ # before the fallback runs.
+ bitmap_count=$(virsh -c qemu:///system qemu-monitor-command "$VM" '{"execute":"query-block"}' 2>/dev/null | python3 -c '
+import sys, json
+target = sys.argv[1]
+try:
+ data = json.load(sys.stdin)
+except Exception:
+ print(0); sys.exit(0)
+files = set()
+for dev in data.get("return", []) or []:
+ inserted = dev.get("inserted") or {}
+ f = inserted.get("file")
+ if not f:
+ continue
+ if any((b or {}).get("name") == target for b in (inserted.get("dirty-bitmaps") or [])):
+ files.add(f)
+print(len(files))
+' "$BITMAP_PARENT" 2>/dev/null || echo 0)
+ if [[ "$disk_count" -eq 0 || "$bitmap_count" -lt "$disk_count" ]]; then
+ log -e "incremental: parent bitmap $BITMAP_PARENT present on $bitmap_count/$disk_count disk(s) — falling back to full"
+ echo "INCREMENTAL_FALLBACK=true"
+ effective_mode="full"
+ fi
+ fi
+
+ if [[ "$effective_mode" == "incremental" ]]; then
+ if ! virsh -c qemu:///system checkpoint-list "$VM" --name 2>/dev/null | grep -qx "$BITMAP_PARENT"; then
+ redefine_xml=$(mktemp)
+ printf '%s%s' \
+ "$BITMAP_PARENT" "$(date +%s)" > "$redefine_xml"
+ if virsh -c qemu:///system checkpoint-create "$VM" --xmlfile "$redefine_xml" --redefine > /dev/null 2>&1; then
+ rm -f "$redefine_xml" # parent checkpoint re-registered; the incremental can proceed against it
+ else
+ rm -f "$redefine_xml"
+ # Parent checkpoint could not be re-registered — fall back to a full backup in place so
+ # the chain restarts cleanly instead of failing. Emit a stdout marker so the wrapper
+ # records this backup as a full (incrementalFallback=true).
+ log -e "incremental: parent checkpoint $BITMAP_PARENT could not be re-registered — falling back to full"
+ echo "INCREMENTAL_FALLBACK=true"
+ effective_mode="full"
+ fi
+ fi
+ fi
+
+ # Build backup XML (and matching checkpoint XML when applicable).
name="root"
- echo "" > $dest/backup.xml
+ echo "" > $dest/backup.xml
+ if [[ "$effective_mode" == "incremental" ]]; then
+ echo "$BITMAP_PARENT" >> $dest/backup.xml
+ fi
+ echo "" >> $dest/backup.xml
+ if [[ $make_checkpoint -eq 1 ]]; then
+ echo "$BITMAP_NEW" > $dest/checkpoint.xml
+ fi
while read -r disk fullpath; do
if [[ "$fullpath" == /dev/drbd/by-res/* ]]; then
volUuid=$(get_linstor_uuid_from_path "$fullpath")
else
volUuid="${fullpath##*/}"
fi
- echo "" >> $dest/backup.xml
+ if [[ "$effective_mode" == "incremental" ]]; then
+ # Incremental disk entry — no backupmode attr, libvirt picks it up from .
+ echo "" >> $dest/backup.xml
+ else
+ echo "" >> $dest/backup.xml
+ fi
+ if [[ $make_checkpoint -eq 1 ]]; then
+ echo "" >> $dest/checkpoint.xml
+ fi
name="datadisk"
done < <(
virsh -c qemu:///system domblklist "$VM" --details 2>/dev/null | awk '$2=="disk"{print $3, $4}'
)
echo "" >> $dest/backup.xml
+ if [[ $make_checkpoint -eq 1 ]]; then
+ echo "" >> $dest/checkpoint.xml
+ fi
local thaw=0
if [[ ${QUIESCE} == "true" ]]; then
@@ -135,14 +241,22 @@ backup_running_vm() {
fi
fi
- # Start push backup
+ # Start push backup, atomically registering the new checkpoint when applicable.
local backup_begin=0
- if virsh -c qemu:///system backup-begin --domain $VM --backupxml $dest/backup.xml 2>&1 > /dev/null; then
- backup_begin=1;
+ if [[ $make_checkpoint -eq 1 ]]; then
+ # Order matters: redirect stdout to /dev/null first, then merge stderr into stdout.
+ # The reversed `2>&1 > /dev/null` form leaves stderr pointing at the original tty.
+ if virsh -c qemu:///system backup-begin --domain $VM --backupxml $dest/backup.xml --checkpointxml $dest/checkpoint.xml > /dev/null 2>&1; then
+ backup_begin=1;
+ fi
+ else
+ if virsh -c qemu:///system backup-begin --domain $VM --backupxml $dest/backup.xml > /dev/null 2>&1; then
+ backup_begin=1;
+ fi
fi
if [[ $thaw -eq 1 ]]; then
- if ! response=$(virsh -c qemu:///system qemu-agent-command "$VM" '{"execute":"guest-fsfreeze-thaw"}' 2>&1 > /dev/null); then
+ if ! response=$(virsh -c qemu:///system qemu-agent-command "$VM" '{"execute":"guest-fsfreeze-thaw"}' 2>&1); then
echo "Failed to thaw the filesystem for vm $VM: $response"
cleanup
exit 1
@@ -173,9 +287,47 @@ backup_running_vm() {
sleep 5
done
- # Use qemu-img convert to sparsify linstor backups which get bloated due to virsh backup-begin.
+ # Sparsify behavior:
+ # - For LINSTOR backups (existing): qemu-img convert sparsifies the bloated output.
+ # - For INCREMENTAL: rebase the resulting thin qcow2 onto its parent so the chain is self-describing
+ # (so a future restore can flatten without external chain metadata).
name="root"
+ # PARENT_PATHS arrives as a comma-separated list, one entry per VM volume in the same
+ # order as DISK_PATHS. Split into a bash array so we can index by disk position.
+ local -a parent_paths_arr=()
+ if [[ "$effective_mode" == "incremental" && -n "$PARENT_PATHS" ]]; then
+ IFS=',' read -ra parent_paths_arr <<< "$PARENT_PATHS"
+ fi
+ local disk_idx=0
while read -r disk fullpath; do
+ if [[ "$effective_mode" == "incremental" ]]; then
+ volUuid="${fullpath##*/}"
+ # Pick this disk's specific parent file. Each volume's backup is named after its
+ # own UUID, so a single PARENT_PATH would wrongly rebase data disks onto the root
+ # parent.
+ if [[ $disk_idx -ge ${#parent_paths_arr[@]} ]]; then
+ echo "PARENT_PATHS list shorter than DISK_PATHS — missing parent for disk index $disk_idx"
+ cleanup
+ exit 1
+ fi
+ local this_parent_rel="${parent_paths_arr[$disk_idx]}"
+ local parent_abs="$mount_point/$this_parent_rel"
+ if [[ ! -f "$parent_abs" ]]; then
+ echo "Parent backup file does not exist on NAS: $parent_abs"
+ cleanup
+ exit 1
+ fi
+ local parent_rel
+ parent_rel=$(realpath --relative-to="$dest" "$parent_abs")
+ if ! qemu-img rebase -u -b "$parent_rel" -F qcow2 "$dest/$name.$volUuid.qcow2" >> "$logFile" 2> >(cat >&2); then
+ echo "qemu-img rebase failed for $dest/$name.$volUuid.qcow2 onto $parent_rel"
+ cleanup
+ exit 1
+ fi
+ name="datadisk"
+ disk_idx=$((disk_idx + 1))
+ continue
+ fi
if [[ "$fullpath" != /dev/drbd/by-res/* ]]; then
continue
fi
@@ -192,9 +344,43 @@ backup_running_vm() {
virsh -c qemu:///system domblklist "$VM" --details 2>/dev/null | awk '$2=="disk"{print $3, $4}'
)
- rm -f $dest/backup.xml
+ rm -f $dest/backup.xml $dest/checkpoint.xml
sync
+ # Free the parent bitmap now that the incremental is written and rebased: its delta is captured
+ # here and BITMAP_NEW tracks changes going forward, so it only accrues metadata/IO cost over a
+ # long chain. Remove it per-disk with block-dirty-bitmap-remove (a clean free) rather than
+ # checkpoint-delete, which would merge its bits into BITMAP_NEW and re-copy backed-up regions.
+ # Best-effort: a failure here does not fail the backup, the bitmap is reclaimed on a later run.
+ if [[ "$effective_mode" == "incremental" && -n "$BITMAP_PARENT" ]]; then
+ while read -r node; do
+ [[ -z "$node" ]] && continue
+ if ! virsh -c qemu:///system qemu-monitor-command "$VM" \
+ "{\"execute\":\"block-dirty-bitmap-remove\",\"arguments\":{\"node\":\"$node\",\"name\":\"$BITMAP_PARENT\"}}" \
+ > /dev/null 2>>"$logFile"; then
+ log -e "cleanup: failed to remove parent bitmap $BITMAP_PARENT on node $node (non-fatal)"
+ fi
+ done < <(
+ virsh -c qemu:///system qemu-monitor-command "$VM" '{"execute":"query-block"}' 2>/dev/null | python3 -c '
+import sys, json
+target = sys.argv[1]
+try:
+ data = json.load(sys.stdin)
+except Exception:
+ sys.exit(0)
+seen = set()
+for dev in data.get("return", []) or []:
+ inserted = dev.get("inserted") or {}
+ node = inserted.get("node-name")
+ if not node or node in seen:
+ continue
+ if any((b or {}).get("name") == target for b in (inserted.get("dirty-bitmaps") or [])):
+ seen.add(node)
+ print(node)
+' "$BITMAP_PARENT" 2>/dev/null || true
+ )
+ fi
+
# Print statistics
virsh -c qemu:///system domjobinfo $VM --completed
backup_size=$(du -sb "$dest" 2>>"$logFile" | cut -f1) || { log -ne "WARNING: du failed for $dest, reporting size as 0"; backup_size=0; }
@@ -204,6 +390,8 @@ backup_running_vm() {
}
backup_stopped_vm() {
+ # Stopped VMs cannot use libvirt's backup-begin (no QEMU process); take a full backup via
+ # qemu-img convert. The orchestrator never sends incremental mode for a stopped VM.
mount_operation
mkdir -p "$dest" || { echo "Failed to create backup directory $dest"; exit 1; }
@@ -224,6 +412,23 @@ backup_stopped_vm() {
cleanup
exit 1
fi
+
+ # Pre-seed a persistent bitmap on the source disk so the NEXT backup (taken
+ # after this VM is started again) can be incremental against the qcow2 we
+ # just wrote. Without this, every backup after a stopped-VM backup would
+ # fall back to full because no parent bitmap exists on the host yet.
+ # Only applies to file-backed qcow2 sources — RBD/LINSTOR have their own
+ # snapshot mechanisms and qemu-img bitmap is not the right primitive there.
+ # bitmap --add should not fail on a file-backed qcow2; if it does, fail the backup so the
+ # underlying problem is surfaced rather than silently degrading future backups to full.
+ if [[ -n "$BITMAP_NEW" && "$disk" != rbd:* && "$disk" != /dev/drbd/by-res/* ]]; then
+ if ! qemu-img bitmap --add "$disk" "$BITMAP_NEW" 2>>"$logFile"; then
+ echo "Failed to pre-seed bitmap $BITMAP_NEW on $disk"
+ cleanup
+ exit 1
+ fi
+ fi
+
name="datadisk"
done
sync
@@ -293,6 +498,15 @@ cleanup() {
function usage {
echo ""
echo "Usage: $0 -o -v|--vm -t -s -m -p -d -q|--quiesce "
+ echo " [-M|--mode ] [--bitmap-new ] [--bitmap-parent ] [--parent-paths ]"
+ echo ""
+ echo "Incremental backup options (running VMs only; requires QEMU >= 4.2 and libvirt >= 7.2):"
+ echo " -M|--mode full Take a full backup AND create a checkpoint (--bitmap-new required) for future incrementals."
+ echo " -M|--mode incremental Take an incremental backup since --bitmap-parent and create new checkpoint --bitmap-new."
+ echo " Requires --bitmap-parent, --bitmap-new, and --parent-paths (comma-separated list, one"
+ echo " parent qcow2 path per disk: root..qcow2, datadisk..qcow2, … same order"
+ echo " as -d|--disks)."
+ echo " Without -M, behaves as legacy full-only backup with no checkpoint creation."
echo ""
exit 1
}
@@ -339,6 +553,26 @@ while [[ $# -gt 0 ]]; do
shift
shift
;;
+ -M|--mode)
+ MODE="$2"
+ shift
+ shift
+ ;;
+ --bitmap-new)
+ BITMAP_NEW="$2"
+ shift
+ shift
+ ;;
+ --bitmap-parent)
+ BITMAP_PARENT="$2"
+ shift
+ shift
+ ;;
+ --parent-paths)
+ PARENT_PATHS="$2"
+ shift
+ shift
+ ;;
-h|--help)
usage
shift
@@ -350,7 +584,7 @@ while [[ $# -gt 0 ]]; do
esac
done
-# Perform Initial sanity checks
+# Perform initial environment sanity checks (QEMU/libvirt version).
sanity_checks
if [ "$OP" = "backup" ]; then
diff --git a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java
index 6c1c3424b1b9..6154522af4ae 100644
--- a/server/src/main/java/com/cloud/hypervisor/KVMGuru.java
+++ b/server/src/main/java/com/cloud/hypervisor/KVMGuru.java
@@ -351,7 +351,7 @@ public Map getClusterSettings(long vmId) {
@Override
public VirtualMachine importVirtualMachineFromBackup(long zoneId, long domainId, long accountId, long userId, String vmInternalName, Backup backup) {
logger.debug(String.format("Trying to import VM [vmInternalName: %s] from Backup [%s].", vmInternalName,
- ReflectionToStringBuilderUtils.reflectOnlySelectedFields(backup, "id", "uuid", "vmId", "externalId", "backupType")));
+ ReflectionToStringBuilderUtils.reflectOnlySelectedFields(backup, "id", "uuid", "vmId", "externalId", "type")));
VMInstanceVO vm = _instanceDao.findVMByInstanceNameIncludingRemoved(vmInternalName);
if (vm == null) {
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 f9c98cb141b6..0636ffdc3c1a 100644
--- a/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java
+++ b/server/src/main/java/org/apache/cloudstack/backup/BackupManagerImpl.java
@@ -1137,6 +1137,9 @@ public Pair, Integer> listBackups(final ListBackupsCmd cmd) {
sb.and("name", sb.entity().getName(), SearchCriteria.Op.EQ);
sb.and("zoneId", sb.entity().getZoneId(), SearchCriteria.Op.EQ);
sb.and("backupOfferingId", sb.entity().getBackupOfferingId(), SearchCriteria.Op.EQ);
+ // Tombstoned chain backups (Status.Hidden) are never shown to users; they exist only so the
+ // incremental chain GC can sweep them once their last descendant is deleted.
+ sb.and("statusNeq", sb.entity().getStatus(), SearchCriteria.Op.NEQ);
sb.and("backupStatus", sb.entity().getStatus(), SearchCriteria.Op.EQ);
if (keyword != null) {
@@ -1149,6 +1152,7 @@ public Pair, Integer> listBackups(final ListBackupsCmd cmd) {
SearchCriteria sc = sb.create();
accountManager.buildACLSearchCriteria(sc, domainId, isRecursive, permittedAccounts, listProjectResourcesCriteria);
+ sc.setParameters("statusNeq", Backup.Status.Hidden);
if (id != null) {
sc.setParameters("id", id);
@@ -1190,7 +1194,7 @@ public boolean importRestoredVM(long zoneId, long domainId, long accountId, long
vm = guru.importVirtualMachineFromBackup(zoneId, domainId, accountId, userId, vmInternalName, backup);
} catch (final Exception e) {
logger.error(String.format("Failed to import VM [vmInternalName: %s] from backup restoration [%s] with hypervisor [type: %s] due to: [%s].", vmInternalName,
- ReflectionToStringBuilderUtils.reflectOnlySelectedFields(backup, "id", "uuid", "vmId", "externalId", "backupType"), hypervisorType, e.getMessage()), e);
+ ReflectionToStringBuilderUtils.reflectOnlySelectedFields(backup, "id", "uuid", "vmId", "externalId", "type"), hypervisorType, e.getMessage()), e);
ActionEventUtils.onCompletedActionEvent(User.UID_SYSTEM, vm.getAccountId(), EventVO.LEVEL_ERROR, EventTypes.EVENT_VM_BACKUP_RESTORE,
String.format("Failed to import Instance %s from Backup %s with hypervisor [type: %s]", vmInternalName, backup.getUuid(), hypervisorType),
vm.getId(), ApiCommandResourceType.VirtualMachine.toString(),0);
@@ -1722,6 +1726,15 @@ private boolean deleteCheckedBackup(Boolean forced, BackupProvider backupProvide
reservationDao, resourceLimitMgr)) {
boolean result = backupProvider.deleteBackup(backup, forced);
if (result) {
+ // Chain-aware providers (e.g. NAS) physically remove several backups per call
+ // (leaf + swept delete-pending ancestors) and decrement resource count/usage and
+ // remove each DB row themselves, exactly once per removed backup. Decrementing or
+ // removing again here would double-handle and destroy delete-pending tombstones,
+ // so defer entirely to the provider for those.
+ if (backupProvider.handlesChainDeleteResourceAccounting()) {
+ checkAndGenerateUsageForLastBackupDeletedAfterOfferingRemove(vm, backup);
+ return true;
+ }
resourceLimitMgr.decrementResourceCount(backup.getAccountId(), Resource.ResourceType.backup);
resourceLimitMgr.decrementResourceCount(backup.getAccountId(), Resource.ResourceType.backup_storage, backupSize);
if (backupDao.remove(backup.getId())) {
diff --git a/test/integration/smoke/test_backup_recovery_nas.py b/test/integration/smoke/test_backup_recovery_nas.py
index 409a08acc9f0..e55c1b6f0f93 100644
--- a/test/integration/smoke/test_backup_recovery_nas.py
+++ b/test/integration/smoke/test_backup_recovery_nas.py
@@ -265,3 +265,323 @@ def test_vm_backup_create_vm_from_backup_in_another_zone(self):
self.assertEqual(backup_repository.crosszoneinstancecreation, True, "Cross-Zone Instance Creation could not be enabled on the backup repository")
self.vm_backup_create_vm_from_backup_int(template.id, [network.id])
+
+ # ------------------------------------------------------------------
+ # Incremental backup tests (RFC #12899 / PR #13074)
+ # ------------------------------------------------------------------
+ # These tests exercise the incremental NAS backup chain semantics:
+ # full -> incN cadence, restore-from-incremental, delete-middle chain
+ # repair, refuse-delete-full-with-children, and stopped-VM fallback.
+ #
+ # All tests set nas.backup.full.every to a small value (3) so a chain
+ # forms quickly without needing many backup iterations. The original
+ # zone value (whatever the test environment has configured) is captured
+ # before the test runs and restored verbatim in finally, so we don't
+ # leak config changes across tests on shared environments.
+
+ def _set_full_every(self, value):
+ Configurations.update(self.apiclient, name='nas.backup.full.every',
+ value=str(value), zoneid=self.zone.id)
+
+ def _get_full_every(self):
+ """Read the current zone-scoped (or global fallback) value of nas.backup.full.every."""
+ configs = Configurations.list(self.apiclient, name='nas.backup.full.every',
+ zoneid=self.zone.id)
+ if configs and len(configs) > 0 and configs[0].value is not None:
+ return configs[0].value
+ # Fall back to global default — Configurations.list returns the global value
+ # when no zone override exists. Defensive fallback to '10' (the framework default).
+ return '10'
+
+ def _backup_type(self, backup):
+ # Backup objects expose `type`; for chained backups it's "INCREMENTAL", else "FULL".
+ return getattr(backup, 'type', 'FULL') or 'FULL'
+
+ @attr(tags=["advanced", "backup"], required_hardware="true")
+ def test_incremental_chain_cadence(self):
+ """
+ With nas.backup.full.every=3, the sequence of backups should be
+ FULL, INCREMENTAL, INCREMENTAL, FULL, INCREMENTAL, ...
+ """
+ self.backup_offering.assignOffering(self.apiclient, self.vm.id)
+ original_full_every = self._get_full_every()
+ self._set_full_every(3)
+ try:
+ ssh_client_vm = self.vm.get_ssh_client(reconnect=True)
+ ssh_client_vm.execute("touch /root/incremental_marker_1.txt")
+
+ created = []
+ for i in range(5):
+ Backup.create(self.apiclient, self.vm.id, "inc_chain_%d" % i)
+ # write a small change so each incremental has something to capture
+ ssh_client_vm.execute("dd if=/dev/urandom of=/root/delta_%d bs=64k count=4 2>/dev/null" % i)
+ time.sleep(2)
+ created = Backup.list(self.apiclient, self.vm.id)
+
+ self.assertEqual(len(created), 5, "Expected 5 backups after 5 Backup.create calls")
+ # Sort oldest-first by date
+ created.sort(key=lambda b: b.created)
+
+ expected = ['FULL', 'INCREMENTAL', 'INCREMENTAL', 'FULL', 'INCREMENTAL']
+ actual = [self._backup_type(b).upper() for b in created]
+ self.assertEqual(actual, expected,
+ "With nas.backup.full.every=3, chain pattern should be %s but was %s" % (expected, actual))
+
+ # Cleanup all backups (newest first to satisfy chain rules without forced=true)
+ for b in reversed(created):
+ Backup.delete(self.apiclient, b.id)
+ finally:
+ self._set_full_every(original_full_every)
+ self.backup_offering.removeOffering(self.apiclient, self.vm.id)
+
+ @attr(tags=["advanced", "backup"], required_hardware="true")
+ def test_incremental_after_vm_restart(self):
+ """
+ Regression for the parent-checkpoint recreation bug (PR #13074): an incremental
+ backup taken AFTER the VM has been restarted must still succeed and restore
+ correctly.
+
+ A VM (re)start rebuilds the libvirt domain XML and wipes libvirt's checkpoint
+ registry, while the dirty bitmap persists on the qcow2. The agent must then
+ re-register the parent checkpoint with `checkpoint-create --redefine` (from the
+ saved checkpoint XML) rather than a fresh create — a fresh create fails with
+ "Bitmap already exists", and qemu-img cannot drop the bitmap on a running disk.
+
+ How this was reproduced manually on a libvirt 10.0.0 host, and what this test
+ automates:
+ FULL + marker1 -> stop/start the VM (wipes the checkpoint registry)
+ -> INCREMENTAL + marker2 -> restore the tip -> both markers present.
+ """
+ self.backup_offering.assignOffering(self.apiclient, self.vm.id)
+ original_full_every = self._get_full_every()
+ # High cadence so the post-restart backup is INCREMENTAL, not a periodic FULL.
+ self._set_full_every(100)
+ backups = []
+ try:
+ ssh_client_vm = self.vm.get_ssh_client(reconnect=True)
+ ssh_client_vm.execute("echo restart-test-1 > /root/restart_marker_1.txt; sync")
+
+ # 1) FULL anchor
+ Backup.create(self.apiclient, self.vm.id, "restart_full")
+ time.sleep(2)
+
+ # 2) Restart the VM — wipes libvirt's checkpoint registry (the bug trigger).
+ self.vm.stop(self.apiclient)
+ self.vm.start(self.apiclient)
+ ssh_client_vm = self.vm.get_ssh_client(reconnect=True)
+ ssh_client_vm.execute("echo restart-test-2 > /root/restart_marker_2.txt; sync")
+
+ # 3) INCREMENTAL after the restart — the previously-broken path.
+ Backup.create(self.apiclient, self.vm.id, "restart_incr")
+ time.sleep(2)
+
+ backups = Backup.list(self.apiclient, self.vm.id)
+ self.assertEqual(len(backups), 2,
+ "Expected FULL + INCREMENTAL after restart, got %d" % len(backups))
+ backups.sort(key=lambda b: b.created)
+ self.assertEqual(self._backup_type(backups[0]).upper(), 'FULL',
+ "First backup should be FULL")
+ self.assertEqual(self._backup_type(backups[1]).upper(), 'INCREMENTAL',
+ "Backup taken after the VM restart must be INCREMENTAL, not silently a FULL")
+
+ # 4) Restore the tip (incremental) and verify BOTH markers survived the chain
+ # across the restart — i.e. the post-restart incremental really captured data.
+ new_vm_name = "vm-restart-restore-" + str(int(time.time()))
+ new_vm = Backup.createVMFromBackup(
+ self.apiclient,
+ self.services["small"],
+ mode=self.services["mode"],
+ backupid=backups[1].id,
+ vmname=new_vm_name,
+ accountname=self.account.name,
+ domainid=self.account.domainid,
+ zoneid=self.zone.id
+ )
+ self.cleanup.append(new_vm)
+ self.assertIsNotNone(new_vm, "Failed to create VM from the post-restart incremental backup")
+ self.assertEqual(new_vm.state, "Running", "Restored VM should be Running")
+
+ ssh_new = new_vm.get_ssh_client(reconnect=True)
+ r1 = "".join(ssh_new.execute("cat /root/restart_marker_1.txt"))
+ r2 = "".join(ssh_new.execute("cat /root/restart_marker_2.txt"))
+ self.assertIn("restart-test-1", r1,
+ "Marker written before the restart is missing from the restore")
+ self.assertIn("restart-test-2", r2,
+ "Marker written after the restart (captured by the post-restart incremental) "
+ "is missing from the restore")
+ finally:
+ for b in reversed(backups):
+ try:
+ Backup.delete(self.apiclient, b.id)
+ except Exception:
+ pass
+ self._set_full_every(original_full_every)
+ self.backup_offering.removeOffering(self.apiclient, self.vm.id)
+
+ @attr(tags=["advanced", "backup"], required_hardware="true")
+ def test_restore_from_incremental(self):
+ """
+ Take FULL + 2 INCREMENTAL backups, each with a marker file. Restore from the
+ latest incremental and verify all three markers are present (chain flatten).
+ """
+ self.backup_offering.assignOffering(self.apiclient, self.vm.id)
+ original_full_every = self._get_full_every()
+ self._set_full_every(5)
+ try:
+ ssh_client_vm = self.vm.get_ssh_client(reconnect=True)
+ ssh_client_vm.execute("touch /root/marker_full.txt")
+ Backup.create(self.apiclient, self.vm.id, "rfi_full")
+ time.sleep(3)
+
+ ssh_client_vm.execute("touch /root/marker_inc1.txt")
+ Backup.create(self.apiclient, self.vm.id, "rfi_inc1")
+ time.sleep(3)
+
+ ssh_client_vm.execute("touch /root/marker_inc2.txt")
+ Backup.create(self.apiclient, self.vm.id, "rfi_inc2")
+ time.sleep(3)
+
+ backups = Backup.list(self.apiclient, self.vm.id)
+ backups.sort(key=lambda b: b.created)
+ self.assertEqual(len(backups), 3)
+ self.assertEqual(self._backup_type(backups[0]).upper(), 'FULL')
+ self.assertEqual(self._backup_type(backups[2]).upper(), 'INCREMENTAL')
+
+ new_vm_name = "vm-from-inc-" + str(int(time.time()))
+ new_vm = Backup.createVMFromBackup(self.apiclient, self.services["small"],
+ mode=self.services["mode"], backupid=backups[2].id, vmname=new_vm_name,
+ accountname=self.account.name, domainid=self.account.domainid,
+ zoneid=self.zone.id)
+ self.cleanup.append(new_vm)
+
+ ssh_new = new_vm.get_ssh_client(reconnect=True)
+ for marker in ("marker_full.txt", "marker_inc1.txt", "marker_inc2.txt"):
+ result = ssh_new.execute("ls /root/%s" % marker)
+ self.assertIn(marker, result[0],
+ "Restored VM should have %s (chain flattened correctly)" % marker)
+
+ for b in reversed(backups):
+ Backup.delete(self.apiclient, b.id)
+ finally:
+ self._set_full_every(original_full_every)
+ self.backup_offering.removeOffering(self.apiclient, self.vm.id)
+
+ @attr(tags=["advanced", "backup"], required_hardware="true")
+ def test_delete_middle_incremental_repairs_chain(self):
+ """
+ Delete a MIDDLE incremental from a FULL -> INC1 -> INC2 chain.
+ The chain repair should rebase INC2 onto FULL, and the final restore
+ should still produce a working VM with all expected blocks.
+ """
+ self.backup_offering.assignOffering(self.apiclient, self.vm.id)
+ original_full_every = self._get_full_every()
+ self._set_full_every(5)
+ try:
+ ssh_client_vm = self.vm.get_ssh_client(reconnect=True)
+ ssh_client_vm.execute("touch /root/dmi_full.txt")
+ Backup.create(self.apiclient, self.vm.id, "dmi_full")
+ time.sleep(3)
+ ssh_client_vm.execute("touch /root/dmi_inc1.txt")
+ Backup.create(self.apiclient, self.vm.id, "dmi_inc1")
+ time.sleep(3)
+ ssh_client_vm.execute("touch /root/dmi_inc2.txt")
+ Backup.create(self.apiclient, self.vm.id, "dmi_inc2")
+ time.sleep(3)
+
+ backups = Backup.list(self.apiclient, self.vm.id)
+ backups.sort(key=lambda b: b.created)
+ full, inc1, inc2 = backups[0], backups[1], backups[2]
+
+ # Delete the middle incremental — should succeed via chain repair (no force needed)
+ Backup.delete(self.apiclient, inc1.id)
+ remaining = Backup.list(self.apiclient, self.vm.id)
+ self.assertEqual(len(remaining), 2, "After deleting middle inc, two backups should remain")
+
+ # Restore from the remaining tail (formerly inc2) — must still produce a usable VM
+ new_vm_name = "vm-after-mid-del-" + str(int(time.time()))
+ new_vm = Backup.createVMFromBackup(self.apiclient, self.services["small"],
+ mode=self.services["mode"], backupid=inc2.id, vmname=new_vm_name,
+ accountname=self.account.name, domainid=self.account.domainid,
+ zoneid=self.zone.id)
+ self.cleanup.append(new_vm)
+ ssh_new = new_vm.get_ssh_client(reconnect=True)
+ # Both the FULL marker and (importantly) the deleted-INC1 marker should still
+ # be present, because the rebase merged INC1's blocks into INC2.
+ for marker in ("dmi_full.txt", "dmi_inc1.txt", "dmi_inc2.txt"):
+ result = ssh_new.execute("ls /root/%s" % marker)
+ self.assertIn(marker, result[0],
+ "After mid-incremental delete and rebase, %s should still be restorable" % marker)
+
+ Backup.delete(self.apiclient, inc2.id)
+ Backup.delete(self.apiclient, full.id)
+ finally:
+ self._set_full_every(original_full_every)
+ self.backup_offering.removeOffering(self.apiclient, self.vm.id)
+
+ @attr(tags=["advanced", "backup"], required_hardware="true")
+ def test_refuse_delete_full_with_children(self):
+ """
+ Deleting a FULL that has surviving incrementals must fail without forced=true.
+ With forced=true it must succeed and remove the entire chain.
+ """
+ self.backup_offering.assignOffering(self.apiclient, self.vm.id)
+ original_full_every = self._get_full_every()
+ self._set_full_every(5)
+ try:
+ Backup.create(self.apiclient, self.vm.id, "rdc_full")
+ time.sleep(3)
+ Backup.create(self.apiclient, self.vm.id, "rdc_inc")
+ time.sleep(3)
+
+ backups = Backup.list(self.apiclient, self.vm.id)
+ backups.sort(key=lambda b: b.created)
+ full = backups[0]
+
+ failed = False
+ try:
+ Backup.delete(self.apiclient, full.id)
+ except Exception:
+ failed = True
+ self.assertTrue(failed, "Deleting a FULL with children should be refused without forced=true")
+
+ # Forced delete should succeed and clear the whole chain
+ Backup.delete(self.apiclient, full.id, forced=True)
+ remaining = Backup.list(self.apiclient, self.vm.id)
+ self.assertIsNone(remaining, "Forced delete of FULL should remove the entire chain")
+ finally:
+ self._set_full_every(original_full_every)
+ self.backup_offering.removeOffering(self.apiclient, self.vm.id)
+
+ @attr(tags=["advanced", "backup"], required_hardware="true")
+ def test_stopped_vm_falls_back_to_full(self):
+ """
+ When a backup is requested while the VM is stopped, even if the chain cadence
+ would call for an incremental, the agent must fall back to a full and start a
+ new chain. The incrementalFallback flag should be reflected in backup.type=FULL.
+ """
+ self.backup_offering.assignOffering(self.apiclient, self.vm.id)
+ original_full_every = self._get_full_every()
+ self._set_full_every(2) # next backup after the first should be incremental
+ try:
+ Backup.create(self.apiclient, self.vm.id, "svf_first")
+ time.sleep(3)
+
+ # Stop the VM and trigger another backup — should fall back to FULL
+ self.vm.stop(self.apiclient)
+ time.sleep(5)
+ Backup.create(self.apiclient, self.vm.id, "svf_second")
+ time.sleep(3)
+
+ backups = Backup.list(self.apiclient, self.vm.id)
+ backups.sort(key=lambda b: b.created)
+ self.assertEqual(len(backups), 2)
+ self.assertEqual(self._backup_type(backups[0]).upper(), 'FULL')
+ self.assertEqual(self._backup_type(backups[1]).upper(), 'FULL',
+ "Stopped-VM backup must be a FULL even when cadence would have asked for an INCREMENTAL")
+
+ self.vm.start(self.apiclient)
+ for b in reversed(backups):
+ Backup.delete(self.apiclient, b.id)
+ finally:
+ self._set_full_every(original_full_every)
+ self.backup_offering.removeOffering(self.apiclient, self.vm.id)
From 333973ab3cd1a692dc34d536776c8cc21ebc75e1 Mon Sep 17 00:00:00 2001
From: N/A <16502919+erma07@users.noreply.github.com>
Date: Wed, 8 Jul 2026 17:17:34 +0300
Subject: [PATCH 17/58] Add xenserver.create.full.clone global setting (#13114)
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit
* Add xenserver.create.full.clone global setting
Adds a StoragePool-scoped boolean ConfigKey mirroring vmware.create.full.clone
so XenServer-backed VMs can be deployed as full VDI copies (VDI.copy) instead
of always using linked clones (VDI.clone). Default false preserves today's
behavior.
The per-pool flag flows into the existing PrimaryDataStoreTO.fullCloneFlag
through a new dispatch method addFullCloneAndDiskprovisiongStrictnessFlagOnDest
that switches on hypervisor type.
* Fix HypervisorType dispatch in addFullClone flag helper
Replace invalid switch on Hypervisor.HypervisorType (not a Java enum) with
equality checks so cloud-engine-storage-datamotion compiles.
---------
Co-authored-by: Erki Märks
---
.../com/cloud/storage/StorageManager.java | 8 +++
.../motion/AncientDataMotionStrategy.java | 51 ++++++++++++++++---
.../motion/AncientDataMotionStrategyTest.java | 8 +++
.../resource/XenServerStorageProcessor.java | 9 +++-
.../com/cloud/storage/StorageManagerImpl.java | 1 +
5 files changed, 69 insertions(+), 8 deletions(-)
diff --git a/engine/components-api/src/main/java/com/cloud/storage/StorageManager.java b/engine/components-api/src/main/java/com/cloud/storage/StorageManager.java
index 3c62738f9ed5..d6604cffc40a 100644
--- a/engine/components-api/src/main/java/com/cloud/storage/StorageManager.java
+++ b/engine/components-api/src/main/java/com/cloud/storage/StorageManager.java
@@ -195,6 +195,14 @@ public interface StorageManager extends StorageService {
true,
ConfigKey.Scope.StoragePool,
null);
+ ConfigKey XenserverCreateCloneFull = new ConfigKey<>(Boolean.class,
+ "xenserver.create.full.clone",
+ "Storage",
+ "false",
+ "If set to true, creates VMs as full clones on XenServer hypervisor (uses VDI.copy instead of VDI.clone, removing the linked-clone parent relationship).",
+ true,
+ ConfigKey.Scope.StoragePool,
+ null);
ConfigKey VmwareAllowParallelExecution = new ConfigKey<>(Boolean.class,
"vmware.allow.parallel.command.execution",
"Advanced",
diff --git a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
index 1144a29986a6..dd54dd580052 100644
--- a/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
+++ b/engine/storage/datamotion/src/main/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategy.java
@@ -191,7 +191,7 @@ protected Answer copyObject(DataObject srcData, DataObject destData, Host destHo
srcForCopy = cacheData = cacheMgr.createCacheObject(srcData, destScope);
}
- CopyCommand cmd = new CopyCommand(srcForCopy.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnVMwareDest(destData.getTO()), primaryStorageDownloadWait,
+ CopyCommand cmd = new CopyCommand(srcForCopy.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnDest(destData.getTO()), primaryStorageDownloadWait,
VirtualMachineManager.ExecuteInSequence.value());
EndPoint ep = destHost != null ? RemoteHostEndPoint.getHypervisorHostEndPoint(destHost) : selector.select(srcForCopy, destData);
if (ep == null) {
@@ -257,6 +257,43 @@ protected DataTO addFullCloneAndDiskprovisiongStrictnessFlagOnVMwareDest(DataTO
return dataTO;
}
+ /**
+ * Adds {@code 'xenserver.create.full.clone'} value for a given primary storage, whose HV is XenServer, on datastore's {@code fullCloneFlag} field
+ * @param dataTO Dest data store TO
+ * @return dataTO including fullCloneFlag, if provided
+ */
+ protected DataTO addFullCloneAndDiskprovisiongStrictnessFlagOnXenServerDest(DataTO dataTO) {
+ if (dataTO != null && dataTO.getHypervisorType().equals(Hypervisor.HypervisorType.XenServer)){
+ DataStoreTO dataStoreTO = dataTO.getDataStore();
+ if (dataStoreTO != null && dataStoreTO instanceof PrimaryDataStoreTO){
+ PrimaryDataStoreTO primaryDataStoreTO = (PrimaryDataStoreTO) dataStoreTO;
+ primaryDataStoreTO.setFullCloneFlag(StorageManager.XenserverCreateCloneFull.valueIn(primaryDataStoreTO.getId()));
+ }
+ }
+ return dataTO;
+ }
+
+ /**
+ * Dispatches to the per-hypervisor {@code addFullCloneAndDiskprovisiongStrictnessFlagOn*Dest} helper
+ * based on {@code dataTO.getHypervisorType()}. Returns {@code dataTO} unchanged for hypervisors
+ * that do not have a full-clone toggle.
+ * @param dataTO Dest data store TO
+ * @return dataTO including fullCloneFlag, if provided
+ */
+ protected DataTO addFullCloneAndDiskprovisiongStrictnessFlagOnDest(DataTO dataTO) {
+ if (dataTO == null) {
+ return dataTO;
+ }
+ Hypervisor.HypervisorType hypervisorType = dataTO.getHypervisorType();
+ if (Hypervisor.HypervisorType.VMware.equals(hypervisorType)) {
+ return addFullCloneAndDiskprovisiongStrictnessFlagOnVMwareDest(dataTO);
+ }
+ if (Hypervisor.HypervisorType.XenServer.equals(hypervisorType)) {
+ return addFullCloneAndDiskprovisiongStrictnessFlagOnXenServerDest(dataTO);
+ }
+ return dataTO;
+ }
+
protected Answer copyObject(DataObject srcData, DataObject destData) {
return copyObject(srcData, destData, null);
}
@@ -315,7 +352,7 @@ protected Answer copyVolumeFromSnapshot(DataObject snapObj, DataObject volObj) {
updateLockHostForVolume(ep, volObj);
- CopyCommand cmd = new CopyCommand(srcData.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnVMwareDest(volObj.getTO()), _createVolumeFromSnapshotWait, VirtualMachineManager.ExecuteInSequence.value());
+ CopyCommand cmd = new CopyCommand(srcData.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnDest(volObj.getTO()), _createVolumeFromSnapshotWait, VirtualMachineManager.ExecuteInSequence.value());
Answer answer = null;
if (ep == null) {
@@ -361,7 +398,7 @@ private void updateLockHostForVolume(EndPoint ep, DataObject volObj) {
}
protected Answer cloneVolume(DataObject template, DataObject volume) {
- CopyCommand cmd = new CopyCommand(template.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnVMwareDest(volume.getTO()), 0, VirtualMachineManager.ExecuteInSequence.value());
+ CopyCommand cmd = new CopyCommand(template.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnDest(volume.getTO()), 0, VirtualMachineManager.ExecuteInSequence.value());
try {
EndPoint ep = selector.select(volume, anyVolumeRequiresEncryption(volume));
Answer answer = null;
@@ -445,7 +482,7 @@ protected Answer copyVolumeBetweenPools(DataObject srcData, DataObject destData)
objOnImageStore.processEvent(Event.CopyingRequested);
- CopyCommand cmd = new CopyCommand(objOnImageStore.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnVMwareDest(destData.getTO()), _copyvolumewait, VirtualMachineManager.ExecuteInSequence.value());
+ CopyCommand cmd = new CopyCommand(objOnImageStore.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnDest(destData.getTO()), _copyvolumewait, VirtualMachineManager.ExecuteInSequence.value());
EndPoint ep = selector.select(objOnImageStore, destData, encryptionRequired);
if (ep == null) {
String errMsg = String.format(NO_REMOTE_ENDPOINT_WITH_ENCRYPTION, encryptionRequired);
@@ -692,7 +729,7 @@ protected Answer createTemplateFromSnapshot(DataObject srcData, DataObject destD
ep = selector.select(srcData, destData);
}
- CopyCommand cmd = new CopyCommand(srcData.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnVMwareDest(destData.getTO()), _createprivatetemplatefromsnapshotwait, VirtualMachineManager.ExecuteInSequence.value());
+ CopyCommand cmd = new CopyCommand(srcData.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnDest(destData.getTO()), _createprivatetemplatefromsnapshotwait, VirtualMachineManager.ExecuteInSequence.value());
Answer answer = null;
if (ep == null) {
logger.error(NO_REMOTE_ENDPOINT_SSVM);
@@ -730,7 +767,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) {
Scope selectedScope = pickCacheScopeForCopy(srcData, destData);
cacheData = cacheMgr.getCacheObject(srcData, selectedScope);
- CopyCommand cmd = new CopyCommand(srcData.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnVMwareDest(destData.getTO()), _backupsnapshotwait, VirtualMachineManager.ExecuteInSequence.value());
+ CopyCommand cmd = new CopyCommand(srcData.getTO(), addFullCloneAndDiskprovisiongStrictnessFlagOnDest(destData.getTO()), _backupsnapshotwait, VirtualMachineManager.ExecuteInSequence.value());
cmd.setCacheTO(cacheData.getTO());
cmd.setOptions(options);
EndPoint ep = selector.select(srcData, destData, encryptionRequired);
@@ -741,7 +778,7 @@ protected Answer copySnapshot(DataObject srcData, DataObject destData) {
answer = ep.sendMessage(cmd);
}
} else {
- addFullCloneAndDiskprovisiongStrictnessFlagOnVMwareDest(destData.getTO());
+ addFullCloneAndDiskprovisiongStrictnessFlagOnDest(destData.getTO());
CopyCommand cmd = new CopyCommand(srcData.getTO(), destData.getTO(), _backupsnapshotwait, VirtualMachineManager.ExecuteInSequence.value());
cmd.setOptions(options);
EndPoint ep = selector.select(srcData, destData, StorageAction.BACKUPSNAPSHOT, encryptionRequired);
diff --git a/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategyTest.java b/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategyTest.java
index e84163656b10..86af81899e8f 100755
--- a/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategyTest.java
+++ b/engine/storage/datamotion/src/test/java/org/apache/cloudstack/storage/motion/AncientDataMotionStrategyTest.java
@@ -113,6 +113,14 @@ public void testAddFullCloneFlagOnVMwareDest(){
verify(dataStoreTO).setFullCloneFlag(FULL_CLONE_FLAG);
}
+ @Test
+ public void testAddFullCloneFlagOnXenServerDest() throws IllegalAccessException, NoSuchFieldException {
+ overrideDefaultConfigValue(StorageManager.XenserverCreateCloneFull, String.valueOf(FULL_CLONE_FLAG));
+ when(dataTO.getHypervisorType()).thenReturn(HypervisorType.XenServer);
+ strategy.addFullCloneAndDiskprovisiongStrictnessFlagOnXenServerDest(dataTO);
+ verify(dataStoreTO).setFullCloneFlag(FULL_CLONE_FLAG);
+ }
+
@Test
public void testAddFullCloneFlagOnNotVmwareDest(){
verify(dataStoreTO, never()).setFullCloneFlag(any(Boolean.class));
diff --git a/plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/XenServerStorageProcessor.java b/plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/XenServerStorageProcessor.java
index c9e6118340cc..a1d27b65abac 100644
--- a/plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/XenServerStorageProcessor.java
+++ b/plugins/hypervisors/xenserver/src/main/java/com/cloud/hypervisor/xenserver/resource/XenServerStorageProcessor.java
@@ -859,12 +859,19 @@ public Answer cloneVolumeFromBaseTemplate(final CopyCommand cmd) {
final DataTO srcData = cmd.getSrcTO();
final DataTO destData = cmd.getDestTO();
final VolumeObjectTO volume = (VolumeObjectTO) destData;
+ final DataStoreTO destStore = volume.getDataStore();
+ final boolean fullClone = destStore instanceof PrimaryDataStoreTO
+ && Boolean.TRUE.equals(((PrimaryDataStoreTO) destStore).isFullCloneFlag());
VDI vdi = null;
try {
VDI tmpltvdi = null;
tmpltvdi = getVDIbyUuid(conn, srcData.getPath());
- vdi = tmpltvdi.createClone(conn, new HashMap());
+ if (fullClone) {
+ vdi = tmpltvdi.copy(conn, tmpltvdi.getSR(conn));
+ } else {
+ vdi = tmpltvdi.createClone(conn, new HashMap());
+ }
Long virtualSize = vdi.getVirtualSize(conn);
if (volume.getSize() > virtualSize) {
logger.debug("Overriding provided Template's size with new size " + toHumanReadableSize(volume.getSize()) + " for volume: " + volume.getName());
diff --git a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java
index ae2facf38619..9cb5155753c6 100644
--- a/server/src/main/java/com/cloud/storage/StorageManagerImpl.java
+++ b/server/src/main/java/com/cloud/storage/StorageManagerImpl.java
@@ -4621,6 +4621,7 @@ public ConfigKey>[] getConfigKeys() {
SecStorageVMAutoScaleDown,
MountDisabledStoragePool,
VmwareCreateCloneFull,
+ XenserverCreateCloneFull,
VmwareAllowParallelExecution,
DataStoreDownloadFollowRedirects,
AllowVolumeReSizeBeyondAllocation,
From 1400616e7129e854173a1e1481e2f1525e8599f3 Mon Sep 17 00:00:00 2001
From: Vishesh <8760112+vishesh92@users.noreply.github.com>
Date: Thu, 9 Jul 2026 16:02:08 +0530
Subject: [PATCH 18/58] UI: Fix icon for KMS & ordering in the left side menu
(#13568)
* Fix icon for KMS
* Move KMS further down in the left side menu
---
ui/src/config/router.js | 2 +-
ui/src/config/section/kms.js | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/ui/src/config/router.js b/ui/src/config/router.js
index a48c3fef81e3..b9c60bcd0c21 100644
--- a/ui/src/config/router.js
+++ b/ui/src/config/router.js
@@ -218,9 +218,9 @@ export function asyncRouterMap () {
generateRouterMap(compute),
generateRouterMap(storage),
- generateRouterMap(kms),
generateRouterMap(network),
generateRouterMap(image),
+ generateRouterMap(kms),
generateRouterMap(event),
generateRouterMap(project),
generateRouterMap(user),
diff --git a/ui/src/config/section/kms.js b/ui/src/config/section/kms.js
index 648a8064b5c6..3120fee54cc8 100644
--- a/ui/src/config/section/kms.js
+++ b/ui/src/config/section/kms.js
@@ -21,7 +21,7 @@ import store from '@/store'
export default {
name: 'kms',
title: 'label.kms',
- icon: 'hdd-outlined',
+ icon: 'lock-outlined',
show: () => {
return ['Admin'].includes(store.getters.userInfo.roletype) || store.getters.features.hashsmprofiles
},
From 33c3967b6b636ff50a644be8f445c558b5076051 Mon Sep 17 00:00:00 2001
From: Daman Arora <61474540+Damans227@users.noreply.github.com>
Date: Thu, 9 Jul 2026 07:00:31 -0400
Subject: [PATCH 19/58] Add per-domain OAuth (Google, GitHub) provider support
(#12702)
* Add domain_id to oauth_provider table and VO
* Add domain-aware methods to OauthProviderDao
* Add domainId parameter to OAuth provider API commands and response
* Add domain support to OAuth2AuthManager
* Add domain-aware OAuth verification
* Add domain support to ListOAuthProvidersCmd and update related tests
* fix domain path issue
* Add domainId support to OAuth provider
* Return domain name and UUID in OAuth provider API responses using ApiDBUtils
* Refactor domain ID resolution in VerifyOAuthCodeAndGetUserCmd to improve code clarity
* Enhance OAuth2 plugin support for domain-level configuration and authentication checks
* Update OAuth2 tests and VerifyOAuthCodeAndGetUserCmdTest
* Add method to find OAuth provider by domain with global fallback
* Update OAuth provider configuration to use 'domain' instead of 'domainid' in columns and details
* Refactor OAuth provider methods to support domain-level queries and enhance user verification
* Add caching for access token retrieval in GithubOAuth2Provider
* Refactor access token checks in GithubOAuth2Provider to use StringUtils for improved readability and consistency
* Refactor null checks to use utility for improved readability and consistency
* Update OAuth2UserAuthenticatorTest to include domainId in user verification method
* Remove unnecessary blank line and unused imports in OAuth provider command classes
* Refactor and cleanup
* Remove unnecessary blank lines
* Enhance RegisterOAuthProviderCmdTest with additional provider mock data
* Remove startup gate from OAuth plugin initialization to support dynamic config toggling
* Add strictScope to ConfigKey to disable global fallback for domain-scoped oauth2.enabled
* Add domain-scoped provider filtering to listOauthProvider and centralize domain resolution in OAuth2AuthManager
* Add External OAuth tab with domain-scoped provider selection to login page
* code cleanup
* test fix
* Handle login page provider visibility
* UI cleanup
* UI Cleanup
* Keep text color consistent
* add unit tests
* Add Multiple-domain OAuth tests
* Refactor as per PR comments
* Use idempotent DDL helpers for oauth_provider schema migration
* Use global config check for global providers and extract oauthEnabled variable
* Make strictScope return null when id is null
* Rename verification methods to use 'verifySecretCodeAndFetchEmail' for consistency
* Refactor domain handling in OAuth2AuthManagerImpl to use DomainService instead of DomainDao
* Enhance domain ID descriptions in OAuth command classes for clarity
* Add domain path handling to OAuth provider commands and improve descriptions
* Update domain path description in VerifyOAuthCodeAndGetUserCmd to clarify behavior with Domain ID
* Replace remove method with expunge in deleteOauthProvider and add corresponding unit test
* Add external login label to Login.vue and update i18n locale handling
* Fix stale value issue in updateConfiguration response handling in ConfigurationValue.vue
* Enhance OAuth login error handling and add unit test for missing parameters
* Add validation to reject enabling OAuth provider when plugin is disabled at domain scope
* Add domain reassignment support to UpdateOAuthProviderCmd and enhance validation in OAuth2AuthManagerImpl
* Add domain ID to OAuth provider arguments in config
* Fix condition for OAuth verification URL handling in router
* Add domain path to OauthProviderResponse and update UI config to display it
* Update config to remove 'secretkey' from columns and details
* Add secretkey to details in config and display in DetailsTab
* Implement normalization of ROOT domain to null for global OAuth provider handling and add corresponding unit tests
* Refactor OAuth plugin domain scope handling to use a centralized method for enabling checks
* Add strict scope handling to ConfigKey and update OAuth2AuthManager usage
* Implement domain removal listener to clean up OAuth providers on domain deletion
* Enhance OAuth tab icons with disabled state styling for better UX
* Add domain-specific provider prompt and update OAuth provider handling
---------
Co-authored-by: Daman Arora
---
.../auth/UserOAuth2Authenticator.java | 13 +-
.../META-INF/db/schema-42210to42300.sql | 6 +
.../framework/config/ConfigKey.java | 20 +-
.../cloudstack/oauth2/OAuth2AuthManager.java | 29 +-
.../oauth2/OAuth2AuthManagerImpl.java | 161 +++++-
.../oauth2/OAuth2UserAuthenticator.java | 11 +-
.../api/command/ListOAuthProvidersCmd.java | 58 +-
.../OauthLoginAPIAuthenticatorCmd.java | 47 +-
.../api/command/RegisterOAuthProviderCmd.java | 22 +-
.../api/command/UpdateOAuthProviderCmd.java | 26 +-
.../command/VerifyOAuthCodeAndGetUserCmd.java | 24 +-
.../api/response/OauthProviderResponse.java | 54 +-
.../oauth2/dao/OauthProviderDao.java | 10 +-
.../oauth2/dao/OauthProviderDaoImpl.java | 45 +-
.../oauth2/github/GithubOAuth2Provider.java | 31 +-
.../oauth2/google/GoogleOAuth2Provider.java | 43 +-
.../keycloak/KeycloakOAuth2Provider.java | 18 +-
.../cloudstack/oauth2/vo/OauthProviderVO.java | 11 +
.../oauth2/OAuth2AuthManagerImplTest.java | 510 +++++++++++++++++-
.../oauth2/OAuth2UserAuthenticatorTest.java | 54 +-
.../OauthLoginAPIAuthenticatorCmdTest.java | 30 ++
.../command/RegisterOAuthProviderCmdTest.java | 31 +-
.../VerifyOAuthCodeAndGetUserCmdTest.java | 14 +-
.../google/GoogleOAuth2ProviderTest.java | 8 +-
.../keycloak/KeycloakOAuth2ProviderTest.java | 16 +-
.../cloud/server/ManagementServerImpl.java | 9 +-
ui/public/locales/en.json | 1 +
ui/src/components/view/DetailsTab.vue | 21 +-
ui/src/config/section/config.js | 8 +-
ui/src/locales/index.js | 52 +-
ui/src/permission.js | 2 +-
ui/src/views/AutogenView.vue | 2 +-
ui/src/views/auth/Login.vue | 290 +++++++---
ui/src/views/dashboard/VerifyOauth.vue | 2 +-
ui/src/views/setting/ConfigurationValue.vue | 3 +
35 files changed, 1432 insertions(+), 250 deletions(-)
diff --git a/api/src/main/java/org/apache/cloudstack/auth/UserOAuth2Authenticator.java b/api/src/main/java/org/apache/cloudstack/auth/UserOAuth2Authenticator.java
index ee3b98b8a4b6..cccc1fff9823 100644
--- a/api/src/main/java/org/apache/cloudstack/auth/UserOAuth2Authenticator.java
+++ b/api/src/main/java/org/apache/cloudstack/auth/UserOAuth2Authenticator.java
@@ -42,8 +42,19 @@ public interface UserOAuth2Authenticator extends Adapter {
* Verifies the code provided by provider and fetches email
* @return returns email
*/
- String verifyCodeAndFetchEmail(String secretCode);
+ String verifySecretCodeAndFetchEmail(String secretCode);
+ /**
+ * Verifies if the logged in user is valid for a specific domain
+ * @return true if it's a valid user, otherwise false
+ */
+ boolean verifyUser(String email, String secretCode, Long domainId);
+
+ /**
+ * Verifies the secret code provided by provider and fetches email for a specific domain
+ * @return email for the specified domain
+ */
+ String verifySecretCodeAndFetchEmail(String secretCode, Long domainId);
/**
* Fetches email using the accessToken
diff --git a/engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql b/engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql
index 9f4353490956..16d46fddf7b2 100644
--- a/engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql
+++ b/engine/schema/src/main/resources/META-INF/db/schema-42210to42300.sql
@@ -19,6 +19,12 @@
-- Schema upgrade from 4.22.1.0 to 4.23.0.0
--;
+CALL `cloud`.`IDEMPOTENT_ADD_COLUMN`('cloud.oauth_provider', 'domain_id', 'bigint unsigned DEFAULT NULL COMMENT "NULL for global provider, domain ID for domain-specific" AFTER `redirect_uri`');
+CALL `cloud`.`IDEMPOTENT_ADD_FOREIGN_KEY`('cloud.oauth_provider', 'fk_oauth_provider__domain_id', '(`domain_id`)', '`domain`(`id`)');
+CALL `cloud`.`IDEMPOTENT_ADD_KEY`('i_oauth_provider__domain_id', 'cloud.oauth_provider', '(`domain_id`)');
+
+CALL `cloud`.`IDEMPOTENT_ADD_UNIQUE_KEY`('cloud.oauth_provider', 'uk_oauth_provider__provider_domain', '(`provider`, `domain_id`)');
+
CREATE TABLE `cloud`.`backup_offering_details` (
`id` bigint unsigned NOT NULL auto_increment,
`backup_offering_id` bigint unsigned NOT NULL COMMENT 'Backup offering id',
diff --git a/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java b/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java
index ef50064050f8..fd007f12957b 100644
--- a/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java
+++ b/framework/config/src/main/java/org/apache/cloudstack/framework/config/ConfigKey.java
@@ -269,6 +269,17 @@ public String toString() {
private String _defaultValueIfEmpty = null;
+ private boolean _strictScope = false;
+
+ public boolean isStrictScope() {
+ return _strictScope;
+ }
+
+ public ConfigKey withStrictScope() {
+ this._strictScope = true;
+ return this;
+ }
+
public static void init(ConfigDepotImpl depot) {
s_depot = depot;
}
@@ -429,11 +440,18 @@ public T valueInDomain(Long domainId) {
}
public T valueInScope(Scope scope, Long id) {
+ return valueInScope(scope, id, false);
+ }
+
+ public T valueInScope(Scope scope, Long id, boolean strictScope) {
if (id == null) {
- return value();
+ return strictScope ? null : value();
}
String value = s_depot != null ? s_depot.getConfigStringValue(_name, scope, id) : null;
if (value == null) {
+ if (strictScope) {
+ return null;
+ }
return valueInGlobalOrAvailableParentScope(scope, id);
}
logger.trace("Scope({}) value for config ({}): {}", scope, _name, _value);
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManager.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManager.java
index ece012db3a40..133131d3928a 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManager.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManager.java
@@ -18,6 +18,7 @@
//
package org.apache.cloudstack.oauth2;
+import com.cloud.domain.Domain;
import com.cloud.utils.component.PluggableService;
import org.apache.cloudstack.api.auth.PluggableAPIAuthenticator;
import org.apache.cloudstack.auth.UserOAuth2Authenticator;
@@ -27,10 +28,15 @@
import org.apache.cloudstack.oauth2.vo.OauthProviderVO;
import java.util.List;
+import java.util.Map;
public interface OAuth2AuthManager extends PluggableAPIAuthenticator, PluggableService {
+ String GLOBAL_DOMAIN_FILTER = "-1";
+ Long GLOBAL_DOMAIN_ID = -1L;
+
public static ConfigKey OAuth2IsPluginEnabled = new ConfigKey("Advanced", Boolean.class, "oauth2.enabled", "false",
- "Indicates whether OAuth plugin is enabled or not", false);
+ "Indicates whether OAuth plugin is enabled or not. This can be configured at domain level.", true, ConfigKey.Scope.Domain)
+ .withStrictScope();
public static final ConfigKey OAuth2Plugins = new ConfigKey("Advanced", String.class, "oauth2.plugins", "google,github",
"List of OAuth plugins", true);
public static final ConfigKey OAuth2PluginsExclude = new ConfigKey("Advanced", String.class, "oauth2.plugins.exclude", "",
@@ -49,13 +55,30 @@ public interface OAuth2AuthManager extends PluggableAPIAuthenticator, PluggableS
*/
UserOAuth2Authenticator getUserOAuth2AuthenticationProvider(final String providerName);
- String verifyCodeAndFetchEmail(String code, String provider);
+ String verifySecretCodeAndFetchEmail(String code, String provider, Long domainId);
OauthProviderVO registerOauthProvider(RegisterOAuthProviderCmd cmd);
- List listOauthProviders(String provider, String uuid);
+ List listOauthProviders(String provider, String uuid, Long domainId);
boolean deleteOauthProvider(Long id);
OauthProviderVO updateOauthProvider(UpdateOAuthProviderCmd cmd);
+
+ Long resolveDomainId(Map params);
+
+ /**
+ * Resolves whether the OAuth plugin is enabled for the given domain scope.
+ * A null domain or the ROOT domain is treated as the global scope, since the
+ * ROOT domain has no domain-level override and inherits the global value;
+ * any other domain is checked strictly at its own domain scope (no inheritance).
+ * @param domainId domain id, or null for global
+ * @return true if OAuth is enabled for that scope
+ */
+ static boolean isPluginEnabledForDomain(Long domainId) {
+ if (domainId == null || domainId == Domain.ROOT_DOMAIN) {
+ return Boolean.TRUE.equals(OAuth2IsPluginEnabled.value());
+ }
+ return Boolean.TRUE.equals(OAuth2IsPluginEnabled.valueInScope(ConfigKey.Scope.Domain, domainId, true));
+ }
}
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java
index b1bb8292f24a..c3bad43be40e 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImpl.java
@@ -23,9 +23,11 @@
import java.util.HashMap;
import java.util.List;
import java.util.Map;
+import java.util.Objects;
import javax.inject.Inject;
+import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.auth.UserOAuth2Authenticator;
import org.apache.cloudstack.framework.config.ConfigKey;
import org.apache.cloudstack.framework.config.Configurable;
@@ -39,15 +41,28 @@
import org.apache.cloudstack.oauth2.vo.OauthProviderVO;
import org.apache.commons.lang3.StringUtils;
+import org.apache.commons.lang3.ArrayUtils;
+
+import com.cloud.domain.Domain;
+import com.cloud.domain.DomainVO;
+import com.cloud.user.DomainManager;
+import com.cloud.user.DomainService;
import com.cloud.utils.component.Manager;
import com.cloud.utils.component.ManagerBase;
import com.cloud.utils.exception.CloudRuntimeException;
+import org.apache.cloudstack.framework.messagebus.MessageBus;
public class OAuth2AuthManagerImpl extends ManagerBase implements OAuth2AuthManager, Manager, Configurable {
@Inject
protected OauthProviderDao _oauthProviderDao;
+ @Inject
+ private DomainService _domainService;
+
+ @Inject
+ private MessageBus _messageBus;
+
protected static Map userOAuth2AuthenticationProvidersMap = new HashMap<>();
private List userOAuth2AuthenticationProviders;
@@ -63,17 +78,29 @@ public List> getAuthCommands() {
@Override
public boolean start() {
- if (isOAuthPluginEnabled()) {
- logger.info("OAUTH plugin loaded");
- initializeUserOAuth2AuthenticationProvidersMap();
- } else {
- logger.info("OAUTH plugin not enabled so not loading");
- }
+ initializeUserOAuth2AuthenticationProvidersMap();
+ addDomainRemovalListener();
+ logger.info("OAUTH plugin loaded");
return true;
}
- protected boolean isOAuthPluginEnabled() {
- return OAuth2IsPluginEnabled.value();
+ private void addDomainRemovalListener() {
+ _messageBus.subscribe(DomainManager.MESSAGE_PRE_REMOVE_DOMAIN_EVENT, (senderAddress, subject, args) -> {
+ try {
+ long domainId = ((DomainVO) args).getId();
+ List providers = _oauthProviderDao.listByDomain(domainId);
+ for (OauthProviderVO provider : providers) {
+ _oauthProviderDao.expunge(provider.getId());
+ logger.debug("Removed OAuth provider {} for deleted domain {}", provider.getProvider(), domainId);
+ }
+ } catch (Exception e) {
+ logger.error("Failed to remove OAuth providers for deleted domain", e);
+ }
+ });
+ }
+
+ protected boolean isOAuthPluginEnabled(Long domainId) {
+ return OAuth2AuthManager.isPluginEnabledForDomain(domainId);
}
@Override
@@ -124,9 +151,11 @@ protected void initializeUserOAuth2AuthenticationProvidersMap() {
}
@Override
- public String verifyCodeAndFetchEmail(String code, String provider) {
+ public String verifySecretCodeAndFetchEmail(String code, String provider, Long domainId) {
UserOAuth2Authenticator authenticator = getUserOAuth2AuthenticationProvider(provider);
- return authenticator.verifyCodeAndFetchEmail(code);
+ String email = authenticator.verifySecretCodeAndFetchEmail(code, domainId);
+
+ return email;
}
@Override
@@ -136,27 +165,38 @@ public OauthProviderVO registerOauthProvider(RegisterOAuthProviderCmd cmd) {
String clientId = StringUtils.trim(cmd.getClientId());
String redirectUri = StringUtils.trim(cmd.getRedirectUri());
String secretKey = StringUtils.trim(cmd.getSecretKey());
+ Long domainId = normalizeGlobalScope(resolveDomainIdFromIdOrPath(cmd.getDomainId(), cmd.getDomainPath()));
String authorizeUrl = StringUtils.trim(cmd.getAuthorizeUrl());
String tokenUrl = StringUtils.trim(cmd.getTokenUrl());
- if (!isOAuthPluginEnabled()) {
+ if (!isOAuthPluginEnabled(domainId)) {
throw new CloudRuntimeException("OAuth is not enabled, please enable to register");
}
- OauthProviderVO providerVO = _oauthProviderDao.findByProvider(provider);
+
+ // Check for existing provider with same name and domain
+ OauthProviderVO providerVO = _oauthProviderDao.findByProviderAndDomain(provider, domainId);
if (providerVO != null) {
- throw new CloudRuntimeException(String.format("Provider with the name %s is already registered", provider));
+ if (domainId == null) {
+ throw new CloudRuntimeException(String.format("Global provider with the name %s is already registered", provider));
+ } else {
+ throw new CloudRuntimeException(String.format("Provider with the name %s is already registered for domain %d", provider, domainId));
+ }
}
- return saveOauthProvider(provider, description, clientId, secretKey, redirectUri, authorizeUrl, tokenUrl);
+ return saveOauthProvider(provider, description, clientId, secretKey, redirectUri, authorizeUrl, tokenUrl, domainId);
}
@Override
- public List listOauthProviders(String provider, String uuid) {
+ public List listOauthProviders(String provider, String uuid, Long domainId) {
List providers;
if (uuid != null) {
providers = Collections.singletonList(_oauthProviderDao.findByUuid(uuid));
+ } else if (StringUtils.isNotBlank(provider) && domainId != null) {
+ providers = Collections.singletonList(_oauthProviderDao.findByProviderAndDomain(provider, domainId));
} else if (StringUtils.isNotBlank(provider)) {
- providers = Collections.singletonList(_oauthProviderDao.findByProvider(provider));
+ providers = Collections.singletonList(_oauthProviderDao.findByProviderAndDomain(provider, null));
+ } else if (domainId != null) {
+ providers = _oauthProviderDao.listByDomainIncludingGlobal(domainId);
} else {
providers = _oauthProviderDao.listAll();
}
@@ -179,6 +219,30 @@ public OauthProviderVO updateOauthProvider(UpdateOAuthProviderCmd cmd) {
throw new CloudRuntimeException("Provider with the given id is not there");
}
+ Long targetDomainId = providerVO.getDomainId();
+ if (cmd.getDomainId() != null || StringUtils.isNotEmpty(cmd.getDomainPath())) {
+ Long resolved = resolveDomainIdFromIdOrPath(cmd.getDomainId(), cmd.getDomainPath());
+ if (resolved == null) {
+ throw new CloudRuntimeException("Unable to resolve the supplied domain. Provide a valid domain id or path.");
+ }
+ resolved = normalizeGlobalScope(resolved);
+ if (!Objects.equals(resolved, providerVO.getDomainId())) {
+ OauthProviderVO existing = _oauthProviderDao.findByProviderAndDomain(providerVO.getProvider(), resolved);
+ if (existing != null) {
+ throw new CloudRuntimeException(String.format(
+ "Provider with the name %s is already registered for domain %s", providerVO.getProvider(),
+ resolved == null ? "ROOT (global)" : resolved));
+ }
+ }
+ targetDomainId = resolved;
+ }
+
+ if (Boolean.TRUE.equals(enabled) && !isOAuthPluginEnabled(targetDomainId)) {
+ throw new CloudRuntimeException(String.format(
+ "OAuth plugin is not enabled %s. Enable oauth2.enabled at that scope before enabling this provider.",
+ targetDomainId == null ? "globally" : "for this domain"));
+ }
+
if (StringUtils.isNotEmpty(description)) {
providerVO.setDescription(description);
}
@@ -200,13 +264,14 @@ public OauthProviderVO updateOauthProvider(UpdateOAuthProviderCmd cmd) {
if (enabled != null) {
providerVO.setEnabled(enabled);
}
+ providerVO.setDomainId(targetDomainId);
_oauthProviderDao.update(id, providerVO);
return _oauthProviderDao.findById(id);
}
- private OauthProviderVO saveOauthProvider(String provider, String description, String clientId, String secretKey, String redirectUri, String authorizeUrl, String tokenUrl) {
+ private OauthProviderVO saveOauthProvider(String provider, String description, String clientId, String secretKey, String redirectUri, String authorizeUrl, String tokenUrl, Long domainId) {
final OauthProviderVO oauthProviderVO = new OauthProviderVO();
oauthProviderVO.setProvider(provider);
@@ -214,6 +279,7 @@ private OauthProviderVO saveOauthProvider(String provider, String description, S
oauthProviderVO.setClientId(clientId);
oauthProviderVO.setSecretKey(secretKey);
oauthProviderVO.setRedirectUri(redirectUri);
+ oauthProviderVO.setDomainId(domainId);
oauthProviderVO.setAuthorizeUrl(authorizeUrl);
oauthProviderVO.setTokenUrl(tokenUrl);
oauthProviderVO.setEnabled(true);
@@ -225,7 +291,66 @@ private OauthProviderVO saveOauthProvider(String provider, String description, S
@Override
public boolean deleteOauthProvider(Long id) {
- return _oauthProviderDao.remove(id);
+ return _oauthProviderDao.expunge(id);
+ }
+
+ @Override
+ public Long resolveDomainId(Map params) {
+ final String[] domainIdArray = (String[])params.get(ApiConstants.DOMAIN_ID);
+ if (ArrayUtils.isNotEmpty(domainIdArray)) {
+ String domainUuid = domainIdArray[0];
+ if (GLOBAL_DOMAIN_FILTER.equals(domainUuid)) {
+ return GLOBAL_DOMAIN_ID;
+ }
+ Domain domain = _domainService.getDomain(domainUuid);
+ if (Objects.nonNull(domain)) {
+ return domain.getId();
+ }
+ }
+ final String[] domainArray = (String[])params.get(ApiConstants.DOMAIN);
+ if (ArrayUtils.isNotEmpty(domainArray)) {
+ String path = normalizeDomainPath(domainArray[0]);
+ if (StringUtils.isNotEmpty(path)) {
+ Domain domain = _domainService.findDomainByIdOrPath(null, path);
+ if (Objects.nonNull(domain)) {
+ return domain.getId();
+ }
+ }
+ }
+ return null;
+ }
+
+ protected Long resolveDomainIdFromIdOrPath(Long domainId, String domainPath) {
+ if (domainId != null) {
+ return domainId;
+ }
+ String path = normalizeDomainPath(domainPath);
+ if (StringUtils.isNotEmpty(path)) {
+ Domain domain = _domainService.findDomainByIdOrPath(null, path);
+ if (Objects.nonNull(domain)) {
+ return domain.getId();
+ }
+ }
+ return null;
+ }
+
+ // The ROOT domain is the top of the tree, so a provider scoped to it is equivalent
+ // to a global provider; treat it as global so the global oauth2.enabled config applies.
+ protected Long normalizeGlobalScope(Long domainId) {
+ return (domainId != null && Domain.ROOT_DOMAIN == domainId) ? null : domainId;
+ }
+
+ protected String normalizeDomainPath(String path) {
+ if (StringUtils.isEmpty(path)) {
+ return null;
+ }
+ if (!path.startsWith("/")) {
+ path = "/" + path;
+ }
+ if (!path.endsWith("/")) {
+ path += "/";
+ }
+ return path;
}
@Override
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticator.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticator.java
index dde50c8bb34d..49df94709836 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticator.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/OAuth2UserAuthenticator.java
@@ -30,8 +30,7 @@
import javax.inject.Inject;
import java.util.Map;
-
-import static org.apache.cloudstack.oauth2.OAuth2AuthManager.OAuth2IsPluginEnabled;
+import java.util.Objects;
public class OAuth2UserAuthenticator extends AdapterBase implements UserAuthenticator {
@@ -49,7 +48,7 @@ public Pair authenticate(String username,
logger.debug("Trying OAuth2 auth for user: " + username);
}
- if (!isOAuthPluginEnabled()) {
+ if (!isOAuthPluginEnabled(domainId)) {
logger.debug("OAuth2 plugin is disabled");
return new Pair(false, null);
} else if (requestParameters == null) {
@@ -76,7 +75,7 @@ public Pair authenticate(String username,
String secretCode = ((secretCodeArray == null) ? null : secretCodeArray[0]);
UserOAuth2Authenticator authenticator = userOAuth2mgr.getUserOAuth2AuthenticationProvider(oauthProvider);
- if (user != null && authenticator.verifyUser(email, secretCode)) {
+ if (Objects.nonNull(user) && authenticator.verifyUser(email, secretCode, domainId)) {
return new Pair(true, null);
}
}
@@ -89,7 +88,7 @@ public String encode(String password) {
return null;
}
- protected boolean isOAuthPluginEnabled() {
- return OAuth2IsPluginEnabled.value();
+ protected boolean isOAuthPluginEnabled(Long domainId) {
+ return OAuth2AuthManager.isPluginEnabledForDomain(domainId);
}
}
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/ListOAuthProvidersCmd.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/ListOAuthProvidersCmd.java
index 9b91a1d879c2..2d0a2e2a7417 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/ListOAuthProvidersCmd.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/ListOAuthProvidersCmd.java
@@ -35,14 +35,17 @@
import org.apache.cloudstack.api.auth.APIAuthenticationType;
import org.apache.cloudstack.api.auth.APIAuthenticator;
import org.apache.cloudstack.api.auth.PluggableAPIAuthenticator;
+import org.apache.cloudstack.api.response.DomainResponse;
import org.apache.cloudstack.api.response.ListResponse;
import org.apache.cloudstack.auth.UserOAuth2Authenticator;
import org.apache.cloudstack.oauth2.OAuth2AuthManager;
import org.apache.cloudstack.oauth2.api.response.OauthProviderResponse;
import org.apache.cloudstack.oauth2.vo.OauthProviderVO;
-import org.apache.commons.lang.ArrayUtils;
+import org.apache.commons.lang3.ArrayUtils;
+import com.cloud.api.ApiDBUtils;
import com.cloud.api.response.ApiResponseSerializer;
+import com.cloud.domain.Domain;
import com.cloud.user.Account;
@APICommand(name = "listOauthProvider", description = "List OAuth providers registered", responseObject = OauthProviderResponse.class, entityType = {},
@@ -60,6 +63,14 @@ public class ListOAuthProvidersCmd extends BaseListCmd implements APIAuthenticat
@Parameter(name = ApiConstants.PROVIDER, type = CommandType.STRING, description = "Name of the provider")
private String provider;
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class,
+ description = "Domain ID to list OAuth providers for a specific domain. Use -1 for global providers only.", since = "4.23.0")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.DOMAIN, type = CommandType.STRING,
+ description = "Domain path for domain-specific OAuth provider lookup. Ignored when Domain ID is passed.", since = "4.23.0")
+ private String domainPath;
+
/////////////////////////////////////////////////////
/////////////////// Accessors ///////////////////////
/////////////////////////////////////////////////////
@@ -71,6 +82,10 @@ public String getProvider() {
return provider;
}
+ public Long getDomainId() {
+ return domainId;
+ }
+
/////////////////////////////////////////////////////
/////////////// API Implementation///////////////////
/////////////////////////////////////////////////////
@@ -99,7 +114,26 @@ public String authenticate(String command, Map params, HttpSes
provider = providerArray[0];
}
- List resultList = _oauth2mgr.listOauthProviders(provider, id);
+ boolean domainRequested = ArrayUtils.isNotEmpty((String[])params.get(ApiConstants.DOMAIN_ID))
+ || ArrayUtils.isNotEmpty((String[])params.get(ApiConstants.DOMAIN));
+ domainId = _oauth2mgr.resolveDomainId(params);
+
+ if (domainRequested && domainId == null) {
+ ListResponse response = new ListResponse<>();
+ response.setResponses(new ArrayList<>(), 0);
+ response.setResponseName(getCommandName());
+ setResponseObject(response);
+ return ApiResponseSerializer.toSerializedString(response, responseType);
+ }
+
+ List resultList = _oauth2mgr.listOauthProviders(provider, id, domainId);
+ boolean isAuthenticated = session != null && session.getAttribute(ApiConstants.USER_ID) != null;
+ if (domainRequested && domainId != null && domainId > 0) {
+ resultList.removeIf(p -> p.getDomainId() == null);
+ } else if (!domainRequested && !isAuthenticated) {
+ resultList.removeIf(p -> p.getDomainId() != null);
+ }
+
List userOAuth2AuthenticatorPlugins = _oauth2mgr.listUserOAuth2AuthenticationProviders();
List authenticatorPluginNames = new ArrayList<>();
for (UserOAuth2Authenticator authenticator : userOAuth2AuthenticatorPlugins) {
@@ -108,9 +142,11 @@ public String authenticate(String command, Map params, HttpSes
}
List responses = new ArrayList<>();
for (OauthProviderVO result : resultList) {
+ Domain domain = result.getDomainId() != null ? ApiDBUtils.findDomainById(result.getDomainId()) : null;
OauthProviderResponse r = new OauthProviderResponse(result.getUuid(), result.getProvider(),
- result.getDescription(), result.getClientId(), result.getSecretKey(), result.getRedirectUri(), result.getAuthorizeUrl(), result.getTokenUrl());
- if (OAuth2AuthManager.OAuth2IsPluginEnabled.value() && authenticatorPluginNames.contains(result.getProvider()) && result.isEnabled()) {
+ result.getDescription(), result.getClientId(), result.getSecretKey(), result.getRedirectUri(), result.getAuthorizeUrl(), result.getTokenUrl(), domain);
+ boolean oauthEnabled = OAuth2AuthManager.isPluginEnabledForDomain(result.getDomainId());
+ if (oauthEnabled && authenticatorPluginNames.contains(result.getProvider()) && result.isEnabled()) {
r.setEnabled(true);
} else {
r.setEnabled(false);
@@ -119,8 +155,20 @@ public String authenticate(String command, Map params, HttpSes
responses.add(r);
}
+ int totalEnabledCount = responses.size();
+ if (!domainRequested && !isAuthenticated) {
+ List allProviders = _oauth2mgr.listOauthProviders(null, null, null);
+ for (OauthProviderVO domainProvider : allProviders) {
+ if (domainProvider.getDomainId() != null && domainProvider.isEnabled()
+ && OAuth2AuthManager.isPluginEnabledForDomain(domainProvider.getDomainId())
+ && authenticatorPluginNames.contains(domainProvider.getProvider())) {
+ totalEnabledCount++;
+ }
+ }
+ }
+
ListResponse response = new ListResponse<>();
- response.setResponses(responses, resultList.size());
+ response.setResponses(responses, totalEnabledCount);
response.setResponseName(getCommandName());
setResponseObject(response);
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/OauthLoginAPIAuthenticatorCmd.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/OauthLoginAPIAuthenticatorCmd.java
index d2af4c24ce43..e9ef030aaab5 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/OauthLoginAPIAuthenticatorCmd.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/OauthLoginAPIAuthenticatorCmd.java
@@ -16,6 +16,8 @@
// under the License.
package org.apache.cloudstack.oauth2.api.command;
+import java.util.Objects;
+
import com.cloud.api.ApiServlet;
import com.cloud.domain.Domain;
import com.cloud.user.User;
@@ -48,7 +50,7 @@
import java.util.Map;
import java.net.InetAddress;
-import static org.apache.cloudstack.oauth2.OAuth2AuthManager.OAuth2IsPluginEnabled;
+import org.apache.cloudstack.oauth2.OAuth2AuthManager;
@APICommand(name = "oauthlogin", description = "Logs a user into the CloudStack after successful verification of OAuth secret code from the particular provider." +
"A successful login attempt will generate a JSESSIONID cookie value that can be passed in subsequent Query command calls until the \"logout\" command has been issued or the session has expired.",
@@ -120,9 +122,6 @@ public void execute() throws ServerApiException {
@Override
public String authenticate(String command, Map params, HttpSession session, InetAddress remoteAddress, String responseType, StringBuilder auditTrailSb, final HttpServletRequest req, final HttpServletResponse resp) throws ServerApiException {
- if (!OAuth2IsPluginEnabled.value()) {
- throw new CloudAuthenticationException("OAuth is not enabled in CloudStack, users cannot login using OAuth");
- }
final String[] provider = (String[])params.get(ApiConstants.PROVIDER);
final String[] emailArray = (String[])params.get(ApiConstants.EMAIL);
final String[] secretCodeArray = (String[])params.get(ApiConstants.SECRET_CODE);
@@ -130,15 +129,41 @@ public String authenticate(String command, Map params, HttpSes
String oauthProvider = ((provider == null) ? null : provider[0]);
String email = ((emailArray == null) ? null : emailArray[0]);
String secretCode = ((secretCodeArray == null) ? null : secretCodeArray[0]);
- if (StringUtils.isAnyEmpty(oauthProvider, email, secretCode)) {
- throw new CloudAuthenticationException("OAuth provider, email, secretCode any of these cannot be null");
- }
- Long domainId = getDomainIdFromParams(params, auditTrailSb, responseType);
- final String[] domainName = (String[])params.get(ApiConstants.DOMAIN);
- String domain = getDomainName(auditTrailSb, domainName);
+ try {
+ if (StringUtils.isAnyEmpty(oauthProvider, email, secretCode)) {
+ throw new CloudAuthenticationException("OAuth provider, email, secretCode any of these cannot be null");
+ }
+
+ Long domainId = getDomainIdFromParams(params, auditTrailSb, responseType);
+ final String[] domainName = (String[])params.get(ApiConstants.DOMAIN);
+ String domain = getDomainName(auditTrailSb, domainName);
+
+ final Domain userDomain = _domainService.findDomainByIdOrPath(domainId, domain);
+ if (Objects.nonNull(userDomain)) {
+ domainId = userDomain.getId();
+ }
+
+ boolean oauthEnabled = OAuth2AuthManager.isPluginEnabledForDomain(domainId);
+ if (!oauthEnabled) {
+ logger.debug(String.format("OAuth is not enabled %s, users cannot login using OAuth", domainId == null ? "globally" : "in domain " + domainId));
+ throw new CloudAuthenticationException(String.format(
+ "OAuth login is not enabled %s. Please contact your administrator.",
+ domainId == null ? "globally" : "for this domain"));
+ }
+
+ return doOauthAuthentication(session, domainId, domain, email, params, remoteAddress, responseType, auditTrailSb);
+ } catch (final CloudAuthenticationException ex) {
+ throw toServerApiException(session, params, responseType, auditTrailSb, ex);
+ }
+ }
- return doOauthAuthentication(session, domainId, domain, email, params, remoteAddress, responseType, auditTrailSb);
+ private ServerApiException toServerApiException(HttpSession session, Map params, String responseType, StringBuilder auditTrailSb, CloudAuthenticationException ex) {
+ ApiServlet.invalidateHttpSession(session, "fall through to API key,");
+ String msg = ex.getMessage() != null ? ex.getMessage() : "failed to authenticate user via OAuth";
+ auditTrailSb.append(" " + ApiErrorCode.ACCOUNT_ERROR + " " + msg);
+ String serializedResponse = _apiServer.getSerializedApiError(ApiErrorCode.ACCOUNT_ERROR.getHttpCode(), msg, params, responseType);
+ return new ServerApiException(ApiErrorCode.ACCOUNT_ERROR, serializedResponse);
}
private String doOauthAuthentication(HttpSession session, Long domainId, String domain, String email, Map params, InetAddress remoteAddress, String responseType, StringBuilder auditTrailSb) {
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java
index 8eb4493d76d8..79274ba904b1 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/RegisterOAuthProviderCmd.java
@@ -26,6 +26,7 @@
import org.apache.cloudstack.api.BaseCmd;
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
import org.apache.cloudstack.api.response.SuccessResponse;
import org.apache.cloudstack.context.CallContext;
import org.apache.cloudstack.oauth2.OAuth2AuthManager;
@@ -35,6 +36,8 @@
import org.apache.commons.collections.MapUtils;
import org.apache.commons.lang3.StringUtils;
+import com.cloud.api.ApiDBUtils;
+import com.cloud.domain.Domain;
import com.cloud.exception.ConcurrentOperationException;
@APICommand(name = "registerOauthProvider", responseObject = SuccessResponse.class, description = "Register the OAuth2 provider in CloudStack", since = "4.19.0")
@@ -59,6 +62,14 @@ public class RegisterOAuthProviderCmd extends BaseCmd {
@Parameter(name = ApiConstants.REDIRECT_URI, type = CommandType.STRING, description = "Redirect URI pre-registered in the specific OAuth provider", required = true)
private String redirectUri;
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class,
+ description = "Domain ID for domain-specific OAuth provider. If not provided, registers as global provider", since = "4.23.0")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.DOMAIN, type = CommandType.STRING,
+ description = "Domain path for domain-specific OAuth provider. Ignored when Domain ID is passed.", since = "4.23.0")
+ private String domainPath;
+
@Parameter(name = ApiConstants.AUTHORIZE_URL, type = CommandType.STRING, description = "Authorize URL for OAuth initialization (only required for keycloak provider)")
private String authorizeUrl;
@@ -94,6 +105,14 @@ public String getRedirectUri() {
return redirectUri;
}
+ public Long getDomainId() {
+ return domainId;
+ }
+
+ public String getDomainPath() {
+ return domainPath;
+ }
+
public String getAuthorizeUrl() {
return authorizeUrl;
}
@@ -126,9 +145,10 @@ public void execute() throws ServerApiException, ConcurrentOperationException, E
OauthProviderVO provider = _oauth2mgr.registerOauthProvider(this);
+ Domain domain = provider.getDomainId() != null ? ApiDBUtils.findDomainById(provider.getDomainId()) : null;
OauthProviderResponse response = new OauthProviderResponse(provider.getUuid(), provider.getProvider(),
provider.getDescription(), provider.getClientId(), provider.getSecretKey(), provider.getRedirectUri(),
- provider.getAuthorizeUrl(), provider.getTokenUrl());
+ provider.getAuthorizeUrl(), provider.getTokenUrl(), domain);
response.setResponseName(getCommandName());
response.setObjectName(ApiConstants.OAUTH_PROVIDER);
setResponseObject(response);
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/UpdateOAuthProviderCmd.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/UpdateOAuthProviderCmd.java
index a8b0604a9bba..f32c08e048eb 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/UpdateOAuthProviderCmd.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/UpdateOAuthProviderCmd.java
@@ -28,12 +28,16 @@
import org.apache.cloudstack.api.BaseCmd;
import org.apache.cloudstack.api.Parameter;
import org.apache.cloudstack.api.ServerApiException;
+import org.apache.cloudstack.api.response.DomainResponse;
import org.apache.cloudstack.auth.UserOAuth2Authenticator;
import org.apache.cloudstack.context.CallContext;
import org.apache.cloudstack.oauth2.OAuth2AuthManager;
import org.apache.cloudstack.oauth2.api.response.OauthProviderResponse;
import org.apache.cloudstack.oauth2.vo.OauthProviderVO;
+import com.cloud.api.ApiDBUtils;
+import com.cloud.domain.Domain;
+
@APICommand(name = "updateOauthProvider", description = "Updates the registered OAuth provider details", responseObject = OauthProviderResponse.class,
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false, since = "4.19.0")
public final class UpdateOAuthProviderCmd extends BaseCmd {
@@ -66,6 +70,14 @@ public final class UpdateOAuthProviderCmd extends BaseCmd {
@Parameter(name = ApiConstants.ENABLED, type = CommandType.BOOLEAN, description = "OAuth provider will be enabled or disabled based on this value")
private Boolean enabled;
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class,
+ description = "Domain ID to reassign this OAuth provider to. If not provided, the current domain assignment is kept.", since = "4.23.0")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.DOMAIN, type = CommandType.STRING,
+ description = "Domain path to reassign this OAuth provider to. Ignored when Domain ID is passed. If neither is provided, the current domain assignment is kept.", since = "4.23.0")
+ private String domainPath;
+
@Inject
OAuth2AuthManager _oauthMgr;
@@ -105,6 +117,14 @@ public Boolean getEnabled() {
return enabled;
}
+ public Long getDomainId() {
+ return domainId;
+ }
+
+ public String getDomainPath() {
+ return domainPath;
+ }
+
/////////////////////////////////////////////////////
/////////////// API Implementation///////////////////
/////////////////////////////////////////////////////
@@ -128,9 +148,10 @@ public ApiCommandResourceType getApiResourceType() {
public void execute() {
OauthProviderVO result = _oauthMgr.updateOauthProvider(this);
if (result != null) {
+ Domain domain = result.getDomainId() != null ? ApiDBUtils.findDomainById(result.getDomainId()) : null;
OauthProviderResponse r = new OauthProviderResponse(result.getUuid(), result.getProvider(),
result.getDescription(), result.getClientId(), result.getSecretKey(), result.getRedirectUri(),
- result.getAuthorizeUrl(), result.getTokenUrl());
+ result.getAuthorizeUrl(), result.getTokenUrl(), domain);
List userOAuth2AuthenticatorPlugins = _oauthMgr.listUserOAuth2AuthenticationProviders();
List authenticatorPluginNames = new ArrayList<>();
@@ -138,7 +159,8 @@ public void execute() {
String name = authenticator.getName();
authenticatorPluginNames.add(name);
}
- if (OAuth2AuthManager.OAuth2IsPluginEnabled.value() && authenticatorPluginNames.contains(result.getProvider()) && result.isEnabled()) {
+ boolean oauthEnabled = OAuth2AuthManager.isPluginEnabledForDomain(result.getDomainId());
+ if (oauthEnabled && authenticatorPluginNames.contains(result.getProvider()) && result.isEnabled()) {
r.setEnabled(true);
} else {
r.setEnabled(false);
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/VerifyOAuthCodeAndGetUserCmd.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/VerifyOAuthCodeAndGetUserCmd.java
index b3d2d335ba25..d5c7455d0f33 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/VerifyOAuthCodeAndGetUserCmd.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/command/VerifyOAuthCodeAndGetUserCmd.java
@@ -20,6 +20,9 @@
import java.util.List;
import java.util.Map;
+import com.cloud.api.response.ApiResponseSerializer;
+import com.cloud.user.Account;
+
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import javax.servlet.http.HttpSession;
@@ -34,13 +37,11 @@
import org.apache.cloudstack.api.auth.APIAuthenticationType;
import org.apache.cloudstack.api.auth.APIAuthenticator;
import org.apache.cloudstack.api.auth.PluggableAPIAuthenticator;
+import org.apache.cloudstack.api.response.DomainResponse;
import org.apache.cloudstack.api.response.UserResponse;
import org.apache.cloudstack.oauth2.OAuth2AuthManager;
import org.apache.cloudstack.oauth2.api.response.OauthProviderResponse;
-import org.apache.commons.lang.ArrayUtils;
-
-import com.cloud.api.response.ApiResponseSerializer;
-import com.cloud.user.Account;
+import org.apache.commons.lang3.ArrayUtils;
@APICommand(name = "verifyOAuthCodeAndGetUser", description = "Verify the OAuth Code and fetch the corresponding user from provider", responseObject = OauthProviderResponse.class, entityType = {},
requestHasSensitiveInfo = false, responseHasSensitiveInfo = false,
@@ -58,6 +59,14 @@ public class VerifyOAuthCodeAndGetUserCmd extends BaseListCmd implements APIAuth
@Parameter(name = ApiConstants.SECRET_CODE, type = CommandType.STRING, description = "Code that is provided by OAuth provider (Eg. google, github) after successful login")
private String secretCode;
+ @Parameter(name = ApiConstants.DOMAIN_ID, type = CommandType.UUID, entityType = DomainResponse.class,
+ description = "Domain ID for domain-specific OAuth provider lookup. If not provided, uses global provider", since = "4.23.0")
+ private Long domainId;
+
+ @Parameter(name = ApiConstants.DOMAIN, type = CommandType.STRING,
+ description = "Domain path for domain-specific OAuth provider lookup. Ignored when Domain ID is passed.", since = "4.23.0")
+ private String domainPath;
+
/////////////////////////////////////////////////////
/////////////////// Accessors ///////////////////////
/////////////////////////////////////////////////////
@@ -70,6 +79,10 @@ public String getSecretCode() {
return secretCode;
}
+ public Long getDomainId() {
+ return domainId;
+ }
+
/////////////////////////////////////////////////////
/////////////// API Implementation///////////////////
/////////////////////////////////////////////////////
@@ -97,8 +110,9 @@ public String authenticate(String command, Map params, HttpSes
if (ArrayUtils.isNotEmpty(providerArray)) {
provider = providerArray[0];
}
+ domainId = _oauth2mgr.resolveDomainId(params);
- String email = _oauth2mgr.verifyCodeAndFetchEmail(secretCode, provider);
+ String email = _oauth2mgr.verifySecretCodeAndFetchEmail(secretCode, provider, domainId);
if (email != null) {
UserResponse response = new UserResponse();
response.setEmail(email);
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/response/OauthProviderResponse.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/response/OauthProviderResponse.java
index 289dc6650137..b363e13516bc 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/response/OauthProviderResponse.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/api/response/OauthProviderResponse.java
@@ -16,11 +16,14 @@
// under the License.
package org.apache.cloudstack.oauth2.api.response;
+import java.util.Objects;
+
import org.apache.cloudstack.api.ApiConstants;
import org.apache.cloudstack.api.BaseResponse;
import org.apache.cloudstack.api.EntityReference;
import org.apache.cloudstack.oauth2.vo.OauthProviderVO;
+import com.cloud.domain.Domain;
import com.cloud.serializer.Param;
import com.google.gson.annotations.SerializedName;
@@ -55,6 +58,18 @@ public class OauthProviderResponse extends BaseResponse {
@Param(description = "Redirect URI registered in the OAuth provider")
private String redirectUri;
+ @SerializedName(ApiConstants.DOMAIN_ID)
+ @Param(description = "UUID of the domain the provider belongs to (empty for global)", since = "4.23.0")
+ private String domainUuid;
+
+ @SerializedName(ApiConstants.DOMAIN)
+ @Param(description = "name of the domain the provider belongs to (empty for global)", since = "4.23.0")
+ private String domainName;
+
+ @SerializedName(ApiConstants.DOMAIN_PATH)
+ @Param(description = "path of the domain the provider belongs to (empty for global)", since = "4.23.0")
+ private String domainPath;
+
@SerializedName(ApiConstants.AUTHORIZE_URL)
@Param(description = "Authorize URL registered in the OAuth provider")
private String authorizeUrl;
@@ -67,7 +82,7 @@ public class OauthProviderResponse extends BaseResponse {
@Param(description = "Whether the OAuth provider is enabled or not")
private boolean enabled;
- public OauthProviderResponse(String id, String provider, String description, String clientId, String secretKey, String redirectUri, String authorizeUrl, String tokenUrl) {
+ public OauthProviderResponse(String id, String provider, String description, String clientId, String secretKey, String redirectUri, String authorizeUrl, String tokenUrl, Domain domain) {
this.id = id;
this.provider = provider;
this.name = provider;
@@ -77,6 +92,19 @@ public OauthProviderResponse(String id, String provider, String description, Str
this.redirectUri = redirectUri;
this.authorizeUrl = authorizeUrl;
this.tokenUrl = tokenUrl;
+ if (Objects.nonNull(domain)) {
+ this.domainUuid = domain.getUuid();
+ this.domainName = domain.getName();
+ this.domainPath = prettifyDomainPath(domain.getPath());
+ }
+ }
+
+ private static String prettifyDomainPath(String path) {
+ if (path == null) {
+ return null;
+ }
+ String trimmed = path.endsWith("/") ? path.substring(0, path.length() - 1) : path;
+ return "ROOT" + trimmed;
}
public String getId() {
@@ -128,6 +156,30 @@ public void setRedirectUri(String redirectUri) {
this.redirectUri = redirectUri;
}
+ public String getDomainUuid() {
+ return domainUuid;
+ }
+
+ public void setDomainUuid(String domainUuid) {
+ this.domainUuid = domainUuid;
+ }
+
+ public String getDomainName() {
+ return domainName;
+ }
+
+ public void setDomainName(String domainName) {
+ this.domainName = domainName;
+ }
+
+ public String getDomainPath() {
+ return domainPath;
+ }
+
+ public void setDomainPath(String domainPath) {
+ this.domainPath = domainPath;
+ }
+
public String getAuthorizeUrl() {
return authorizeUrl;
}
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/dao/OauthProviderDao.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/dao/OauthProviderDao.java
index 31738ac75a0f..629abb0bb2ec 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/dao/OauthProviderDao.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/dao/OauthProviderDao.java
@@ -19,8 +19,16 @@
import com.cloud.utils.db.GenericDao;
import org.apache.cloudstack.oauth2.vo.OauthProviderVO;
+import java.util.List;
+
public interface OauthProviderDao extends GenericDao {
- public OauthProviderVO findByProvider(String provider);
+ public OauthProviderVO findByProviderAndDomain(String provider, Long domainId);
+
+ public List listByDomainIncludingGlobal(Long domainId);
+
+ public List listByDomain(Long domainId);
+
+ public OauthProviderVO findByProviderAndDomainWithGlobalFallback(String provider, Long domainId);
}
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/dao/OauthProviderDaoImpl.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/dao/OauthProviderDaoImpl.java
index 27eea4d22a6b..eecff0f4f506 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/dao/OauthProviderDaoImpl.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/dao/OauthProviderDaoImpl.java
@@ -22,23 +22,54 @@
import com.cloud.utils.db.SearchCriteria;
import org.apache.cloudstack.oauth2.vo.OauthProviderVO;
+import java.util.List;
+import java.util.Objects;
+
public class OauthProviderDaoImpl extends GenericDaoBase implements OauthProviderDao {
- private final SearchBuilder oauthProviderSearchByName;
+ private final SearchBuilder oauthProviderSearchByProviderAndDomain;
public OauthProviderDaoImpl() {
super();
- oauthProviderSearchByName = createSearchBuilder();
- oauthProviderSearchByName.and("provider", oauthProviderSearchByName.entity().getProvider(), SearchCriteria.Op.EQ);
- oauthProviderSearchByName.done();
+ oauthProviderSearchByProviderAndDomain = createSearchBuilder();
+ oauthProviderSearchByProviderAndDomain.and("provider", oauthProviderSearchByProviderAndDomain.entity().getProvider(), SearchCriteria.Op.EQ);
+ oauthProviderSearchByProviderAndDomain.and("domainId", oauthProviderSearchByProviderAndDomain.entity().getDomainId(), SearchCriteria.Op.EQ);
+ oauthProviderSearchByProviderAndDomain.done();
}
@Override
- public OauthProviderVO findByProvider(String provider) {
- SearchCriteria sc = oauthProviderSearchByName.create();
+ public OauthProviderVO findByProviderAndDomain(String provider, Long domainId) {
+ SearchCriteria sc = oauthProviderSearchByProviderAndDomain.create();
sc.setParameters("provider", provider);
-
+ sc.setParameters("domainId", domainId);
return findOneBy(sc);
}
+
+ @Override
+ public List listByDomainIncludingGlobal(Long domainId) {
+ SearchCriteria sc = createSearchCriteria();
+ sc.addOr("domainId", SearchCriteria.Op.EQ, domainId);
+ sc.addOr("domainId", SearchCriteria.Op.NULL);
+ return listBy(sc);
+ }
+
+ @Override
+ public List listByDomain(Long domainId) {
+ SearchCriteria sc = createSearchCriteria();
+ sc.addAnd("domainId", SearchCriteria.Op.EQ, domainId);
+ return listBy(sc);
+ }
+
+ @Override
+ public OauthProviderVO findByProviderAndDomainWithGlobalFallback(String provider, Long domainId) {
+ OauthProviderVO providerVO = null;
+ if (Objects.nonNull(domainId)) {
+ providerVO = findByProviderAndDomain(provider, domainId);
+ }
+ if (Objects.isNull(providerVO)) {
+ providerVO = findByProviderAndDomain(provider, null);
+ }
+ return providerVO;
+ }
}
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/github/GithubOAuth2Provider.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/github/GithubOAuth2Provider.java
index 4d426181a94b..7413d22b2fa6 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/github/GithubOAuth2Provider.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/github/GithubOAuth2Provider.java
@@ -56,17 +56,27 @@ public String getDescription() {
@Override
public boolean verifyUser(String email, String secretCode) {
+ return verifyUser(email, secretCode, null);
+ }
+
+ @Override
+ public String verifySecretCodeAndFetchEmail(String secretCode) {
+ return verifySecretCodeAndFetchEmail(secretCode, null);
+ }
+
+ @Override
+ public boolean verifyUser(String email, String secretCode, Long domainId) {
if (StringUtils.isAnyEmpty(email, secretCode)) {
throw new CloudRuntimeException(String.format("Either email or secretcode should not be null/empty"));
}
- OauthProviderVO providerVO = _oauthProviderDao.findByProvider(getName());
+ OauthProviderVO providerVO = _oauthProviderDao.findByProviderAndDomainWithGlobalFallback(getName(), domainId);
if (providerVO == null) {
throw new CloudRuntimeException("Github provider is not registered, so user cannot be verified");
}
- String verifiedEmail = getUserEmailAddress();
- if (verifiedEmail == null || !email.equals(verifiedEmail)) {
+ String verifiedEmail = verifySecretCodeAndFetchEmail(secretCode, domainId);
+ if (StringUtils.isEmpty(verifiedEmail) || !email.equals(verifiedEmail)) {
throw new CloudRuntimeException("Unable to verify the email address with the provided secret");
}
@@ -76,16 +86,19 @@ public boolean verifyUser(String email, String secretCode) {
}
@Override
- public String verifyCodeAndFetchEmail(String secretCode) {
- String accessToken = getAccessToken(secretCode);
- if (accessToken == null) {
+ public String verifySecretCodeAndFetchEmail(String secretCode, Long domainId) {
+ String accessToken = getAccessToken(secretCode, domainId);
+ if (StringUtils.isEmpty(accessToken)) {
return null;
}
return getUserEmailAddress();
}
- protected String getAccessToken(String secretCode) throws CloudRuntimeException {
- OauthProviderVO githubProvider = _oauthProviderDao.findByProvider(getName());
+ protected String getAccessToken(String secretCode, Long domainId) throws CloudRuntimeException {
+ if (StringUtils.isNotEmpty(accessToken)) {
+ return accessToken;
+ }
+ OauthProviderVO githubProvider = _oauthProviderDao.findByProviderAndDomainWithGlobalFallback(getName(), domainId);
String tokenUrl = "https://github.com/login/oauth/access_token";
String generatedAccessToken = null;
try {
@@ -131,7 +144,7 @@ protected String getAccessToken(String secretCode) throws CloudRuntimeException
}
public String getUserEmailAddress() throws CloudRuntimeException {
- if (accessToken == null) {
+ if (StringUtils.isEmpty(accessToken)) {
throw new CloudRuntimeException("Access Token not found to fetch the email address");
}
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2Provider.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2Provider.java
index 885930181c91..3b37f0f6e0b8 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2Provider.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/google/GoogleOAuth2Provider.java
@@ -60,16 +60,36 @@ public String getDescription() {
@Override
public boolean verifyUser(String email, String secretCode) {
+ return verifyUser(email, secretCode, null);
+ }
+
+ @Override
+ public String verifySecretCodeAndFetchEmail(String secretCode) {
+ return verifySecretCodeAndFetchEmail(secretCode, null);
+ }
+
+ protected void clearAccessAndRefreshTokens() {
+ accessToken = null;
+ refreshToken = null;
+ }
+
+ @Override
+ public String getUserEmailAddress() throws CloudRuntimeException {
+ return null;
+ }
+
+ @Override
+ public boolean verifyUser(String email, String secretCode, Long domainId) {
if (StringUtils.isAnyEmpty(email, secretCode)) {
throw new CloudAuthenticationException("Either email or secret code should not be null/empty");
}
- OauthProviderVO providerVO = _oauthProviderDao.findByProvider(getName());
+ OauthProviderVO providerVO = _oauthProviderDao.findByProviderAndDomainWithGlobalFallback(getName(), domainId);
if (providerVO == null) {
throw new CloudAuthenticationException("Google provider is not registered, so user cannot be verified");
}
- String verifiedEmail = verifyCodeAndFetchEmail(secretCode);
+ String verifiedEmail = verifySecretCodeAndFetchEmail(secretCode, domainId);
if (verifiedEmail == null || !email.equals(verifiedEmail)) {
throw new CloudRuntimeException("Unable to verify the email address with the provided secret");
}
@@ -79,11 +99,11 @@ public boolean verifyUser(String email, String secretCode) {
}
@Override
- public String verifyCodeAndFetchEmail(String secretCode) {
- OauthProviderVO googleProvider = _oauthProviderDao.findByProvider(getName());
- String clientId = googleProvider.getClientId();
- String secret = googleProvider.getSecretKey();
- String redirectURI = googleProvider.getRedirectUri();
+ public String verifySecretCodeAndFetchEmail(String secretCode, Long domainId) {
+ OauthProviderVO provider = _oauthProviderDao.findByProviderAndDomainWithGlobalFallback(getName(), domainId);
+ String clientId = provider.getClientId();
+ String secret = provider.getSecretKey();
+ String redirectURI = provider.getRedirectUri();
GoogleClientSecrets clientSecrets = new GoogleClientSecrets()
.setWeb(new GoogleClientSecrets.Details()
.setClientId(clientId)
@@ -129,13 +149,4 @@ public String verifyCodeAndFetchEmail(String secretCode) {
return userinfo.getEmail();
}
- protected void clearAccessAndRefreshTokens() {
- accessToken = null;
- refreshToken = null;
- }
-
- @Override
- public String getUserEmailAddress() throws CloudRuntimeException {
- return null;
- }
}
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java
index 3f537b1984d0..2a625c6f7570 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/keycloak/KeycloakOAuth2Provider.java
@@ -81,16 +81,21 @@ public String getDescription() {
@Override
public boolean verifyUser(String email, String secretCode) {
+ return verifyUser(email, secretCode, null);
+ }
+
+ @Override
+ public boolean verifyUser(String email, String secretCode, Long domainId) {
if (StringUtils.isAnyEmpty(email, secretCode)) {
throw new CloudAuthenticationException("Either email or secret code should not be null/empty");
}
- OauthProviderVO providerVO = oauthProviderDao.findByProvider(getName());
+ OauthProviderVO providerVO = oauthProviderDao.findByProviderAndDomainWithGlobalFallback(getName(), domainId);
if (providerVO == null) {
throw new CloudAuthenticationException("Keycloak provider is not registered, so user cannot be verified");
}
- String verifiedEmail = verifyCodeAndFetchEmail(secretCode);
+ String verifiedEmail = verifySecretCodeAndFetchEmail(secretCode, domainId);
if (StringUtils.isBlank(verifiedEmail) || !email.equals(verifiedEmail)) {
throw new CloudRuntimeException("Unable to verify the email address with the provided secret");
}
@@ -100,8 +105,13 @@ public boolean verifyUser(String email, String secretCode) {
}
@Override
- public String verifyCodeAndFetchEmail(String secretCode) {
- OauthProviderVO provider = oauthProviderDao.findByProvider(getName());
+ public String verifySecretCodeAndFetchEmail(String secretCode) {
+ return verifySecretCodeAndFetchEmail(secretCode, null);
+ }
+
+ @Override
+ public String verifySecretCodeAndFetchEmail(String secretCode, Long domainId) {
+ OauthProviderVO provider = oauthProviderDao.findByProviderAndDomainWithGlobalFallback(getName(), domainId);
if (provider == null) {
throw new CloudAuthenticationException("Keycloak provider is not registered, so user cannot be verified");
}
diff --git a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/vo/OauthProviderVO.java b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/vo/OauthProviderVO.java
index 54d667bc9143..8aa9006e763e 100644
--- a/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/vo/OauthProviderVO.java
+++ b/plugins/user-authenticators/oauth2/src/main/java/org/apache/cloudstack/oauth2/vo/OauthProviderVO.java
@@ -57,6 +57,9 @@ public class OauthProviderVO implements Identity, InternalIdentity {
@Column(name = "redirect_uri")
private String redirectUri;
+ @Column(name = "domain_id")
+ private Long domainId;
+
@Column(name = "authorize_url")
private String authorizeUrl;
@@ -142,6 +145,14 @@ public void setSecretKey(String secretKey) {
this.secretKey = secretKey;
}
+ public Long getDomainId() {
+ return domainId;
+ }
+
+ public void setDomainId(Long domainId) {
+ this.domainId = domainId;
+ }
+
public boolean isEnabled() {
return enabled;
}
diff --git a/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImplTest.java b/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImplTest.java
index 3fd5636102ce..e3e8f7594b37 100644
--- a/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImplTest.java
+++ b/plugins/user-authenticators/oauth2/src/test/java/org/apache/cloudstack/oauth2/OAuth2AuthManagerImplTest.java
@@ -19,7 +19,13 @@
package org.apache.cloudstack.oauth2;
+import com.cloud.domain.Domain;
+import com.cloud.domain.DomainVO;
+import com.cloud.user.DomainService;
import com.cloud.utils.exception.CloudRuntimeException;
+import org.apache.cloudstack.api.ApiConstants;
+import org.apache.cloudstack.framework.messagebus.MessageBus;
+import org.apache.cloudstack.framework.messagebus.MessageSubscriber;
import org.apache.cloudstack.oauth2.api.command.DeleteOAuthProviderCmd;
import org.apache.cloudstack.oauth2.api.command.RegisterOAuthProviderCmd;
import org.apache.cloudstack.oauth2.api.command.UpdateOAuthProviderCmd;
@@ -36,12 +42,17 @@
import org.mockito.Spy;
import java.util.ArrayList;
+import java.util.Arrays;
import java.util.Collections;
+import java.util.HashMap;
import java.util.List;
+import java.util.Map;
import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertNull;
import static org.junit.Assert.assertTrue;
import static org.mockito.Mockito.doNothing;
+import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;
public class OAuth2AuthManagerImplTest {
@@ -53,6 +64,12 @@ public class OAuth2AuthManagerImplTest {
@Mock
OauthProviderDao _oauthProviderDao;
+ @Mock
+ DomainService _domainService;
+
+ @Mock
+ MessageBus _messageBus;
+
AutoCloseable closeable;
@Before
public void setUp() {
@@ -66,7 +83,7 @@ public void tearDown() throws Exception {
@Test
public void testRegisterOauthProvider() {
- when(_authManager.isOAuthPluginEnabled()).thenReturn(false);
+ when(_authManager.isOAuthPluginEnabled(Mockito.nullable(Long.class))).thenReturn(false);
RegisterOAuthProviderCmd cmd = Mockito.mock(RegisterOAuthProviderCmd.class);
try {
_authManager.registerOauthProvider(cmd);
@@ -76,25 +93,27 @@ public void testRegisterOauthProvider() {
}
// Test when provider is already registered
- when(_authManager.isOAuthPluginEnabled()).thenReturn(true);
+ when(_authManager.isOAuthPluginEnabled(Mockito.nullable(Long.class))).thenReturn(true);
OauthProviderVO providerVO = new OauthProviderVO();
providerVO.setProvider("testProvider");
- when(_authManager._oauthProviderDao.findByProvider(Mockito.anyString())).thenReturn(providerVO);
+ when(_authManager._oauthProviderDao.findByProviderAndDomain(Mockito.anyString(), Mockito.isNull())).thenReturn(providerVO);
when(cmd.getProvider()).thenReturn("testProvider");
+ when(cmd.getDomainId()).thenReturn(null);
try {
_authManager.registerOauthProvider(cmd);
Assert.fail("Expected CloudRuntimeException was not thrown");
} catch (CloudRuntimeException e) {
- assertEquals("Provider with the name testProvider is already registered", e.getMessage());
+ assertEquals("Global provider with the name testProvider is already registered", e.getMessage());
}
// Test when provider is github and secret key is not null
when(cmd.getSecretKey()).thenReturn("testSecretKey");
providerVO = null;
- when(_authManager._oauthProviderDao.findByProvider(Mockito.anyString())).thenReturn(providerVO);
+ when(_authManager._oauthProviderDao.findByProviderAndDomain(Mockito.anyString(), Mockito.isNull())).thenReturn(providerVO);
OauthProviderVO savedProviderVO = new OauthProviderVO();
when(cmd.getProvider()).thenReturn("github");
+ when(cmd.getDomainId()).thenReturn(null);
when(_authManager._oauthProviderDao.persist(Mockito.any(OauthProviderVO.class))).thenReturn(savedProviderVO);
OauthProviderVO result = _authManager.registerOauthProvider(cmd);
assertEquals("github", result.getProvider());
@@ -140,6 +159,115 @@ public void testUpdateOauthProvider() {
assertEquals(secretKey, result.getSecretKey());
}
+ @Test
+ public void testUpdateOauthProviderReassignsDomain() {
+ Long id = 5L;
+ Long oldDomainId = 10L;
+ Long newDomainId = 20L;
+
+ UpdateOAuthProviderCmd cmd = Mockito.mock(UpdateOAuthProviderCmd.class);
+ when(cmd.getId()).thenReturn(id);
+ when(cmd.getDomainId()).thenReturn(newDomainId);
+
+ OauthProviderVO providerVO = new OauthProviderVO();
+ providerVO.setProvider("github");
+ providerVO.setDomainId(oldDomainId);
+ when(_oauthProviderDao.findById(id)).thenReturn(providerVO);
+
+ Domain newDomain = Mockito.mock(Domain.class);
+ when(newDomain.getId()).thenReturn(newDomainId);
+ when(_domainService.getDomain(Mockito.anyString())).thenReturn(newDomain);
+ Mockito.doReturn(newDomainId).when(_authManager).resolveDomainIdFromIdOrPath(newDomainId, null);
+ when(_oauthProviderDao.findByProviderAndDomain("github", newDomainId)).thenReturn(null);
+ when(_oauthProviderDao.update(Mockito.eq(id), Mockito.any(OauthProviderVO.class))).thenReturn(true);
+ when(_oauthProviderDao.findById(id)).thenReturn(providerVO);
+
+ OauthProviderVO result = _authManager.updateOauthProvider(cmd);
+ assertEquals(newDomainId, result.getDomainId());
+ }
+
+ @Test
+ public void testUpdateOauthProviderRejectsDuplicateAtTargetDomain() {
+ Long id = 5L;
+ Long oldDomainId = 10L;
+ Long newDomainId = 20L;
+
+ UpdateOAuthProviderCmd cmd = Mockito.mock(UpdateOAuthProviderCmd.class);
+ when(cmd.getId()).thenReturn(id);
+ when(cmd.getDomainId()).thenReturn(newDomainId);
+
+ OauthProviderVO providerVO = new OauthProviderVO();
+ providerVO.setProvider("github");
+ providerVO.setDomainId(oldDomainId);
+ when(_oauthProviderDao.findById(id)).thenReturn(providerVO);
+
+ Mockito.doReturn(newDomainId).when(_authManager).resolveDomainIdFromIdOrPath(newDomainId, null);
+ OauthProviderVO collision = new OauthProviderVO();
+ collision.setProvider("github");
+ collision.setDomainId(newDomainId);
+ when(_oauthProviderDao.findByProviderAndDomain("github", newDomainId)).thenReturn(collision);
+
+ try {
+ _authManager.updateOauthProvider(cmd);
+ Assert.fail("Expected CloudRuntimeException for duplicate at target domain");
+ } catch (CloudRuntimeException e) {
+ assertTrue(e.getMessage().contains("already registered"));
+ }
+ }
+
+ @Test
+ public void testRegisterOauthProviderForRootDomainTreatedAsGlobal() {
+ RegisterOAuthProviderCmd cmd = Mockito.mock(RegisterOAuthProviderCmd.class);
+ when(cmd.getProvider()).thenReturn("github");
+ when(cmd.getDomainId()).thenReturn(com.cloud.domain.Domain.ROOT_DOMAIN);
+ when(cmd.getSecretKey()).thenReturn("secret");
+ when(cmd.getClientId()).thenReturn("clientId");
+ when(cmd.getRedirectUri()).thenReturn("https://redirect");
+
+ // global check must be consulted (domainId resolves to null), not the ROOT domain scope
+ when(_authManager.isOAuthPluginEnabled(Mockito.isNull())).thenReturn(true);
+ when(_oauthProviderDao.findByProviderAndDomain("github", null)).thenReturn(null);
+ when(_oauthProviderDao.persist(Mockito.any(OauthProviderVO.class))).thenAnswer(i -> i.getArgument(0));
+
+ OauthProviderVO result = _authManager.registerOauthProvider(cmd);
+ assertNull(result.getDomainId());
+ Mockito.verify(_oauthProviderDao).findByProviderAndDomain("github", null);
+ }
+
+ @Test
+ public void testNormalizeGlobalScopeMapsRootToNull() {
+ assertNull(_authManager.normalizeGlobalScope(com.cloud.domain.Domain.ROOT_DOMAIN));
+ assertNull(_authManager.normalizeGlobalScope(null));
+ assertEquals(Long.valueOf(42L), _authManager.normalizeGlobalScope(42L));
+ }
+
+ @Test
+ public void testUpdateOauthProviderRejectsEnableWhenPluginDisabledAtScope() {
+ Long id = 7L;
+ Long domainId = 42L;
+
+ UpdateOAuthProviderCmd cmd = Mockito.mock(UpdateOAuthProviderCmd.class);
+ when(cmd.getId()).thenReturn(id);
+ when(cmd.getEnabled()).thenReturn(true);
+
+ OauthProviderVO providerVO = new OauthProviderVO();
+ providerVO.setProvider("github");
+ providerVO.setDomainId(domainId);
+ providerVO.setEnabled(false);
+
+ when(_oauthProviderDao.findById(id)).thenReturn(providerVO);
+ Mockito.doReturn(false).when(_authManager).isOAuthPluginEnabled(domainId);
+
+ try {
+ _authManager.updateOauthProvider(cmd);
+ Assert.fail("Expected CloudRuntimeException when enabling provider while oauth2.enabled is false at scope");
+ } catch (CloudRuntimeException e) {
+ assertTrue(e.getMessage().contains("OAuth plugin is not enabled"));
+ }
+
+ Mockito.verify(_oauthProviderDao, Mockito.never()).update(Mockito.eq(id), Mockito.any(OauthProviderVO.class));
+ }
+
@Test
public void testListOauthProviders() {
String uuid = "1234-5678-9101";
@@ -150,20 +278,32 @@ public void testListOauthProviders() {
// Test when uuid is not null
when(_oauthProviderDao.findByUuid(uuid)).thenReturn(providerVO);
- List result = _authManager.listOauthProviders(null, uuid);
+ List