From bb0c1f93afea55a7e86240e5e5233205fb74abe9 Mon Sep 17 00:00:00 2001 From: Harikrishna Date: Mon, 17 Jun 2024 14:06:47 +0530 Subject: [PATCH] Add volume encryption checks during the disk offering change (#9209) --- .../subsystem/api/storage/VolumeService.java | 2 + .../storage/volume/VolumeServiceImpl.java | 15 +++++++ .../storage/volume/VolumeServiceTest.java | 43 +++++++++++++++++++ .../cloud/storage/VolumeApiServiceImpl.java | 4 +- .../java/com/cloud/vm/UserVmManagerImpl.java | 7 +-- .../com/cloud/vm/UserVmManagerImplTest.java | 42 ------------------ 6 files changed, 64 insertions(+), 49 deletions(-) diff --git a/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java b/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java index 7c4d56e12b9..682473ec94f 100644 --- a/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java +++ b/engine/api/src/main/java/org/apache/cloudstack/engine/subsystem/api/storage/VolumeService.java @@ -121,4 +121,6 @@ public interface VolumeService { Pair checkAndRepairVolume(VolumeInfo volume); void checkAndRepairVolumeBasedOnConfig(DataObject dataObject, Host host); + + void validateChangeDiskOfferingEncryptionType(long existingDiskOfferingId, long newDiskOfferingId); } diff --git a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java index 59d027e3c42..a47cb41a323 100644 --- a/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java +++ b/engine/storage/volume/src/main/java/org/apache/cloudstack/storage/volume/VolumeServiceImpl.java @@ -32,7 +32,10 @@ import java.util.concurrent.ExecutionException; import javax.inject.Inject; +import com.cloud.exception.InvalidParameterValueException; +import com.cloud.storage.DiskOfferingVO; import com.cloud.storage.VolumeApiServiceImpl; +import com.cloud.storage.dao.DiskOfferingDao; import org.apache.cloudstack.annotation.AnnotationService; import org.apache.cloudstack.annotation.dao.AnnotationDao; import org.apache.cloudstack.api.command.user.volume.CheckAndRepairVolumeCmd; @@ -212,6 +215,8 @@ public class VolumeServiceImpl implements VolumeService { private SnapshotApiService snapshotApiService; @Inject private PassphraseDao passphraseDao; + @Inject + protected DiskOfferingDao diskOfferingDao; public VolumeServiceImpl() { } @@ -2794,6 +2799,16 @@ public class VolumeServiceImpl implements VolumeService { } } + @Override + public void validateChangeDiskOfferingEncryptionType(long existingDiskOfferingId, long newDiskOfferingId) { + DiskOfferingVO existingDiskOffering = diskOfferingDao.findByIdIncludingRemoved(existingDiskOfferingId); + DiskOfferingVO newDiskOffering = diskOfferingDao.findById(newDiskOfferingId); + + if (existingDiskOffering.getEncrypt() != newDiskOffering.getEncrypt()) { + throw new InvalidParameterValueException("Cannot change the encryption type of a volume, please check the selected offering"); + } + } + @Override public Pair checkAndRepairVolume(VolumeInfo volume) { Long poolId = volume.getPoolId(); diff --git a/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java index 55ff2f659af..97ccc37be13 100644 --- a/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java +++ b/engine/storage/volume/src/test/java/org/apache/cloudstack/storage/volume/VolumeServiceTest.java @@ -22,13 +22,16 @@ package org.apache.cloudstack.storage.volume; import com.cloud.agent.api.storage.CheckAndRepairVolumeAnswer; import com.cloud.agent.api.storage.CheckAndRepairVolumeCommand; import com.cloud.agent.api.to.StorageFilerTO; +import com.cloud.exception.InvalidParameterValueException; import com.cloud.exception.StorageUnavailableException; import com.cloud.host.HostVO; import com.cloud.host.dao.HostDao; import com.cloud.storage.CheckAndRepairVolumePayload; +import com.cloud.storage.DiskOfferingVO; import com.cloud.storage.StorageManager; import com.cloud.storage.StoragePool; import com.cloud.storage.VolumeVO; +import com.cloud.storage.dao.DiskOfferingDao; import com.cloud.storage.dao.VolumeDao; import com.cloud.storage.snapshot.SnapshotManager; import java.util.ArrayList; @@ -88,6 +91,9 @@ public class VolumeServiceTest extends TestCase{ @Mock HostDao hostDaoMock; + @Mock + DiskOfferingDao diskOfferingDaoMock; + @Before public void setup(){ volumeServiceImplSpy = Mockito.spy(new VolumeServiceImpl()); @@ -96,6 +102,7 @@ public class VolumeServiceTest extends TestCase{ volumeServiceImplSpy.snapshotMgr = snapshotManagerMock; volumeServiceImplSpy._storageMgr = storageManagerMock; volumeServiceImplSpy._hostDao = hostDaoMock; + volumeServiceImplSpy.diskOfferingDao = diskOfferingDaoMock; } @Test(expected = InterruptedException.class) @@ -303,4 +310,40 @@ public class VolumeServiceTest extends TestCase{ Assert.assertEquals(null, result); } + + @Test + public void validateDiskOfferingCheckForEncryption1Test() { + prepareOfferingsForEncryptionValidation(1L, true); + prepareOfferingsForEncryptionValidation(2L, true); + volumeServiceImplSpy.validateChangeDiskOfferingEncryptionType(1L, 2L); + } + + @Test + public void validateDiskOfferingCheckForEncryption2Test() { + prepareOfferingsForEncryptionValidation(1L, false); + prepareOfferingsForEncryptionValidation(2L, false); + volumeServiceImplSpy.validateChangeDiskOfferingEncryptionType(1L, 2L); + } + + @Test (expected = InvalidParameterValueException.class) + public void validateDiskOfferingCheckForEncryptionFail1Test() { + prepareOfferingsForEncryptionValidation(1L, false); + prepareOfferingsForEncryptionValidation(2L, true); + volumeServiceImplSpy.validateChangeDiskOfferingEncryptionType(1L, 2L); + } + + @Test (expected = InvalidParameterValueException.class) + public void validateDiskOfferingCheckForEncryptionFail2Test() { + prepareOfferingsForEncryptionValidation(1L, true); + prepareOfferingsForEncryptionValidation(2L, false); + volumeServiceImplSpy.validateChangeDiskOfferingEncryptionType(1L, 2L); + } + + private void prepareOfferingsForEncryptionValidation(long diskOfferingId, boolean encryption) { + DiskOfferingVO diskOffering = Mockito.mock(DiskOfferingVO.class); + + Mockito.when(diskOffering.getEncrypt()).thenReturn(encryption); + Mockito.when(diskOfferingDaoMock.findByIdIncludingRemoved(diskOfferingId)).thenReturn(diskOffering); + Mockito.when(diskOfferingDaoMock.findById(diskOfferingId)).thenReturn(diskOffering); + } } diff --git a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java index 1cf069feae8..31778b33b24 100644 --- a/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java +++ b/server/src/main/java/com/cloud/storage/VolumeApiServiceImpl.java @@ -2027,7 +2027,8 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic } private Volume changeDiskOfferingForVolumeInternal(VolumeVO volume, Long newDiskOfferingId, Long newSize, Long newMinIops, Long newMaxIops, boolean autoMigrateVolume, boolean shrinkOk) throws ResourceAllocationException { - DiskOfferingVO existingDiskOffering = _diskOfferingDao.findById(volume.getDiskOfferingId()); + long existingDiskOfferingId = volume.getDiskOfferingId(); + DiskOfferingVO existingDiskOffering = _diskOfferingDao.findByIdIncludingRemoved(existingDiskOfferingId); DiskOfferingVO newDiskOffering = _diskOfferingDao.findById(newDiskOfferingId); Integer newHypervisorSnapshotReserve = null; @@ -2039,6 +2040,7 @@ public class VolumeApiServiceImpl extends ManagerBase implements VolumeApiServic Long[] updateNewMinIops = {newMinIops}; Long[] updateNewMaxIops = {newMaxIops}; Integer[] updateNewHypervisorSnapshotReserve = {newHypervisorSnapshotReserve}; + volService.validateChangeDiskOfferingEncryptionType(existingDiskOfferingId, newDiskOfferingId); validateVolumeResizeWithNewDiskOfferingAndLoad(volume, existingDiskOffering, newDiskOffering, updateNewSize, updateNewMinIops, updateNewMaxIops, updateNewHypervisorSnapshotReserve); newSize = updateNewSize[0]; newMinIops = updateNewMinIops[0]; diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 0c8ab1cd793..7fd074e13e8 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -2116,12 +2116,7 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir throw new InvalidParameterValueException("Unable to Scale VM, since disk offering id associated with the old service offering is not same for new service offering"); } - DiskOfferingVO currentRootDiskOffering = _diskOfferingDao.findByIdIncludingRemoved(currentServiceOffering.getDiskOfferingId()); - DiskOfferingVO newRootDiskOffering = _diskOfferingDao.findById(newServiceOffering.getDiskOfferingId()); - - if (currentRootDiskOffering.getEncrypt() != newRootDiskOffering.getEncrypt()) { - throw new InvalidParameterValueException("Cannot change volume encryption type via service offering change"); - } + _volService.validateChangeDiskOfferingEncryptionType(currentServiceOffering.getDiskOfferingId(), newServiceOffering.getDiskOfferingId()); } private void changeDiskOfferingForRootVolume(Long vmId, DiskOfferingVO newDiskOffering, Map customParameters, Long zoneId) throws ResourceAllocationException { diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index 303a9b08b1c..ba9e5ba6922 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -677,34 +677,6 @@ public class UserVmManagerImplTest { prepareAndRunResizeVolumeTest(2L, 10L, 20L, largerDisdkOffering, smallerDisdkOffering); } - @Test - public void validateDiskOfferingCheckForEncryption1Test() { - ServiceOfferingVO currentOffering = prepareOfferingsForEncryptionValidation(1L, true); - ServiceOfferingVO newOffering = prepareOfferingsForEncryptionValidation(2L, true); - userVmManagerImpl.validateDiskOfferingChecks(currentOffering, newOffering); - } - - @Test - public void validateDiskOfferingCheckForEncryption2Test() { - ServiceOfferingVO currentOffering = prepareOfferingsForEncryptionValidation(1L, false); - ServiceOfferingVO newOffering = prepareOfferingsForEncryptionValidation(2L, false); - userVmManagerImpl.validateDiskOfferingChecks(currentOffering, newOffering); - } - - @Test (expected = InvalidParameterValueException.class) - public void validateDiskOfferingCheckForEncryptionFail1Test() { - ServiceOfferingVO currentOffering = prepareOfferingsForEncryptionValidation(1L, false); - ServiceOfferingVO newOffering = prepareOfferingsForEncryptionValidation(2L, true); - userVmManagerImpl.validateDiskOfferingChecks(currentOffering, newOffering); - } - - @Test (expected = InvalidParameterValueException.class) - public void validateDiskOfferingCheckForEncryptionFail2Test() { - ServiceOfferingVO currentOffering = prepareOfferingsForEncryptionValidation(1L, true); - ServiceOfferingVO newOffering = prepareOfferingsForEncryptionValidation(2L, false); - userVmManagerImpl.validateDiskOfferingChecks(currentOffering, newOffering); - } - private void prepareAndRunResizeVolumeTest(Long expectedOfferingId, long expectedMinIops, long expectedMaxIops, DiskOfferingVO currentRootDiskOffering, DiskOfferingVO newRootDiskOffering) { long rootVolumeId = 1l; VolumeVO rootVolumeOfVm = Mockito.mock(VolumeVO.class); @@ -728,20 +700,6 @@ public class UserVmManagerImplTest { return newRootDiskOffering; } - private ServiceOfferingVO prepareOfferingsForEncryptionValidation(long diskOfferingId, boolean encryption) { - ServiceOfferingVO svcOffering = Mockito.mock(ServiceOfferingVO.class); - DiskOfferingVO diskOffering = Mockito.mock(DiskOfferingVO.class); - - Mockito.when(svcOffering.getDiskOfferingId()).thenReturn(diskOfferingId); - Mockito.when(diskOffering.getEncrypt()).thenReturn(encryption); - - // Be aware - Multiple calls with the same disk offering ID could conflict - Mockito.when(diskOfferingDao.findByIdIncludingRemoved(diskOfferingId)).thenReturn(diskOffering); - Mockito.when(diskOfferingDao.findById(diskOfferingId)).thenReturn(diskOffering); - - return svcOffering; - } - @Test (expected = CloudRuntimeException.class) public void testUserDataDenyOverride() { Long userDataId = 1L;