diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 94ceb0de363..c1840065603 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -2756,13 +2756,18 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir final List userReadOnlySettings = Stream.of(QueryService.UserVMReadOnlyDetails.value().split(",")) .map(item -> (item).trim()) .collect(Collectors.toList()); + List existingDetails = userVmDetailsDao.listDetails(id); if (cleanupDetails){ if (caller != null && caller.getType() == Account.Type.ADMIN) { - userVmDetailsDao.removeDetails(id); + for (final UserVmDetailVO detail : existingDetails) { + if (detail != null && detail.isDisplay()) { + userVmDetailsDao.removeDetail(id, detail.getName()); + } + } } else { - for (final UserVmDetailVO detail : userVmDetailsDao.listDetails(id)) { + for (final UserVmDetailVO detail : existingDetails) { if (detail != null && !userDenyListedSettings.contains(detail.getName()) - && !userReadOnlySettings.contains(detail.getName())) { + && !userReadOnlySettings.contains(detail.getName()) && detail.isDisplay()) { userVmDetailsDao.removeDetail(id, detail.getName()); } } @@ -2782,15 +2787,25 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir if (userReadOnlySettings.contains(detailName)) { throw new InvalidParameterValueException("You're not allowed to add or edit the read-only setting: " + detailName); } + if (existingDetails.stream().anyMatch(d -> Objects.equals(d.getName(), detailName) && !d.isDisplay())){ + throw new InvalidParameterValueException("You're not allowed to add or edit the non-displayable setting: " + detailName); + } } - // Add any hidden/denied or read-only detail - for (final UserVmDetailVO detail : userVmDetailsDao.listDetails(id)) { + // Add any existing user denied or read-only details. We do it here because admins would already provide these (or can delete them). + for (final UserVmDetailVO detail : existingDetails) { if (userDenyListedSettings.contains(detail.getName()) || userReadOnlySettings.contains(detail.getName())) { details.put(detail.getName(), detail.getValue()); } } } + // ensure details marked as non-displayable are maintained, regardless of admin or not + for (final UserVmDetailVO existingDetail : existingDetails) { + if (!existingDetail.isDisplay()) { + details.put(existingDetail.getName(), existingDetail.getValue()); + } + } + verifyVmLimits(vmInstance, details); vmInstance.setDetails(details); _vmDao.saveDetails(vmInstance); diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index 18b4a891917..22a9bed7764 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -149,7 +149,7 @@ public class UserVmManagerImplTest { private EntityManager entityManager; @Mock - private UserVmDetailsDao userVmDetailVO; + private UserVmDetailsDao userVmDetailsDao; @Mock private UserVmVO userVmVoMock; @@ -300,7 +300,7 @@ public class UserVmManagerImplTest { verifyMethodsThatAreAlwaysExecuted(); Mockito.verify(userVmManagerImpl).updateDisplayVmFlag(false, vmId, userVmVoMock); - Mockito.verify(userVmDetailVO, Mockito.times(0)).removeDetails(vmId); + Mockito.verify(userVmDetailsDao, Mockito.times(0)).removeDetail(anyLong(), anyString()); } @Test @@ -310,12 +310,15 @@ public class UserVmManagerImplTest { Mockito.when(_serviceOfferingDao.findById(Mockito.anyLong(), Mockito.anyLong())).thenReturn((ServiceOfferingVO) offering); Mockito.when(updateVmCommand.isCleanupDetails()).thenReturn(true); Mockito.lenient().doNothing().when(userVmManagerImpl).updateDisplayVmFlag(false, vmId, userVmVoMock); - Mockito.doNothing().when(userVmDetailVO).removeDetails(vmId); + Mockito.when(updateVmCommand.getUserdataId()).thenReturn(null); + prepareExistingDetails(vmId, "userdetail"); + userVmManagerImpl.updateVirtualMachine(updateVmCommand); verifyMethodsThatAreAlwaysExecuted(); - Mockito.verify(userVmDetailVO).removeDetails(vmId); + Mockito.verify(userVmDetailsDao).removeDetail(vmId, "userdetail"); + Mockito.verify(userVmDetailsDao, Mockito.times(0)).removeDetail(vmId, "systemdetail"); Mockito.verify(userVmManagerImpl, Mockito.times(0)).updateDisplayVmFlag(false, vmId, userVmVoMock); } @@ -340,6 +343,16 @@ public class UserVmManagerImplTest { prepareAndExecuteMethodDealingWithDetails(false, false); } + private List prepareExistingDetails(Long vmId, String... existingDetailKeys) { + List existingDetails = new ArrayList<>(); + for (String detail : existingDetailKeys) { + existingDetails.add(new UserVmDetailVO(vmId, detail, "foo", true)); + } + existingDetails.add(new UserVmDetailVO(vmId, "systemdetail", "bar", false)); + Mockito.when(userVmDetailsDao.listDetails(vmId)).thenReturn(existingDetails); + return existingDetails; + } + private void prepareAndExecuteMethodDealingWithDetails(boolean cleanUpDetails, boolean isDetailsEmpty) throws ResourceUnavailableException, InsufficientCapacityException { configureDoNothingForMethodsThatWeDoNotWantToTest(); @@ -360,8 +373,9 @@ public class UserVmManagerImplTest { lenient().doNothing().when(_networkMgr).saveExtraDhcpOptions(anyString(), anyLong(), anyMap()); HashMap details = new HashMap<>(); if(!isDetailsEmpty) { - details.put("", ""); + details.put("newdetail", "foo"); } + prepareExistingDetails(vmId, "existingdetail"); Mockito.when(updateVmCommand.getUserdataId()).thenReturn(null); Mockito.when(updateVmCommand.getDetails()).thenReturn(details); Mockito.when(updateVmCommand.isCleanupDetails()).thenReturn(cleanUpDetails); @@ -371,14 +385,15 @@ public class UserVmManagerImplTest { verifyMethodsThatAreAlwaysExecuted(); Mockito.verify(userVmVoMock, Mockito.times(cleanUpDetails || isDetailsEmpty ? 0 : 1)).setDetails(details); - Mockito.verify(userVmDetailVO, Mockito.times(cleanUpDetails ? 1: 0)).removeDetails(vmId); + Mockito.verify(userVmDetailsDao, Mockito.times(cleanUpDetails ? 1 : 0)).removeDetail(vmId, "existingdetail"); + Mockito.verify(userVmDetailsDao, Mockito.times(0)).removeDetail(vmId, "systemdetail"); Mockito.verify(userVmDao, Mockito.times(cleanUpDetails || isDetailsEmpty ? 0 : 1)).saveDetails(userVmVoMock); Mockito.verify(userVmManagerImpl, Mockito.times(0)).updateDisplayVmFlag(false, vmId, userVmVoMock); } private void configureDoNothingForDetailsMethod() { Mockito.lenient().doNothing().when(userVmManagerImpl).updateDisplayVmFlag(false, vmId, userVmVoMock); - Mockito.doNothing().when(userVmDetailVO).removeDetails(vmId); + Mockito.doNothing().when(userVmDetailsDao).removeDetail(anyLong(), anyString()); Mockito.doNothing().when(userVmDao).saveDetails(userVmVoMock); } diff --git a/test/integration/component/test_update_vm.py b/test/integration/component/test_update_vm.py index ca20a5ad3f8..4d167c63330 100644 --- a/test/integration/component/test_update_vm.py +++ b/test/integration/component/test_update_vm.py @@ -15,11 +15,11 @@ # specific language governing permissions and limitations # under the License. - from marvin.cloudstackTestCase import cloudstackTestCase from marvin.lib.base import Account, VirtualMachine, ServiceOffering -from marvin.lib.utils import cleanup_resources +from marvin.lib.utils import (validateList, cleanup_resources) from marvin.lib.common import get_zone, get_domain, get_template +from marvin.codes import PASS from nose.plugins.attrib import attr class TestData(object): @@ -59,6 +59,7 @@ class TestUpdateVirtualMachine(cloudstackTestCase): def setUp(self): self.testdata = TestData().testdata self.apiclient = self.testClient.getApiClient() + self.dbclient = self.testClient.getDbConnection() # Get Zone, Domain and Default Built-in template self.domain = get_domain(self.apiclient) @@ -106,18 +107,7 @@ class TestUpdateVirtualMachine(cloudstackTestCase): templateid=self.template.id ) - list_vms = VirtualMachine.list(self.apiclient, id=self.virtual_machine.id) - self.assertEqual( - isinstance(list_vms, list), - True, - "List VM response was not a valid list" - ) - self.assertNotEqual( - len(list_vms), - 0, - "List VM response was empty" - ) - vm = list_vms[0] + vm = self.listVmById(self.virtual_machine.id) self.debug( "VirtualMachine launched with id, name, displayname: %s %s %s"\ @@ -152,6 +142,112 @@ class TestUpdateVirtualMachine(cloudstackTestCase): self.assertEqual(vmnew.displayname, vmnewstarted.displayname, msg="display name changed on start, displayname is %s" % vmnewstarted.displayname) + @attr(tags=['advanced', 'simulator', 'basic', 'sg', 'details'], required_hardware="false") + def test_update_vm_details_admin(self): + """Test Update VirtualMachine Details + + # Set up a VM + # Set up hidden detail in DB for VM + + # Validate the following: + # 1. Can add two details (detail1, detail2) + # 2. Can fetch new details on VM + # 3. Can delete detail1 + # 4. Hidden detail not removed + # 6. The detail2 remains + # 7. Ensure cleanup parameter doesn't remove hidden details + """ + hidden_detail_name = "configDriveLocation" + detail1 = "detail1" + detail2 = "detail2" + + # set up a VM + self.virtual_machine = VirtualMachine.create( + self.apiclient, + self.testdata["virtual_machine"], + accountid=self.account.name, + zoneid=self.zone.id, + domainid=self.account.domainid, + serviceofferingid=self.service_offering.id, + templateid=self.template.id + ) + self.cleanup.append(self.virtual_machine) + + vm = self.listVmById(self.virtual_machine.id) + + self.debug( + "VirtualMachine launched with id, name, displayname: %s %s %s" \ + % (self.virtual_machine.id, vm.name, vm.displayname) + ) + + # set up a hidden detail + dbresult = self.dbclient.execute("select id from vm_instance where uuid='%s'" % vm.id) + self.assertEqual(validateList(dbresult)[0], PASS, "sql query returned invalid response") + vm_db_id = dbresult[0][0] + self.debug("VM has database id %d" % vm_db_id) + + self.dbclient.execute("insert into user_vm_details (vm_id, name, value, display) values (%d,'%s','HOST', 0)" % (vm_db_id, hidden_detail_name)) + + vm = self.listVmById(self.virtual_machine.id) + self.debug("VirtualMachine fetched with details: %s of type %s" % (vm.details, type(vm.details))) + + self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden") + + # add two details by appending to what was returned via API + updating_vm_details = vm.details.__dict__ + updating_vm_details[detail1] = "foo" + updating_vm_details[detail2] = "bar" + + self.debug("Updating VM to new details: %s" % updating_vm_details) + vm = self.virtual_machine.update(self.apiclient, details=[updating_vm_details]) + + self.assertIsNotNone(vm.details[detail1], "Expect " + detail1) + self.assertIsNotNone(vm.details[detail2], "Expect " + detail2) + self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden") + self.assertTrue(self.detailInDatabase(vm_db_id, hidden_detail_name), "hidden detail should still exist in db") + + # delete one detail + updating_vm_details = vm.details.__dict__ + del updating_vm_details["detail1"] + + self.debug("Deleting one detail by updating details: %s" % updating_vm_details) + vm = self.virtual_machine.update(self.apiclient, details=[updating_vm_details]) + + self.assertIsNone(vm.details[detail1], "Do not expect " + detail1) + self.assertIsNotNone(vm.details[detail2], "Expect " + detail2) + self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden") + self.assertTrue(self.detailInDatabase(vm_db_id, hidden_detail_name), "hidden detail should still exist in db") + + # cleanup, ensure hidden detail is not deleted + vm = self.virtual_machine.update(self.apiclient, cleanupdetails="true") + self.assertIsNone(vm.details[detail1], "Do not expect " + detail1) + self.assertIsNone(vm.details[detail2], "Do not expect " + detail2) + self.assertIsNone(vm.details[hidden_detail_name], "hidden detail should be hidden") + self.assertTrue(self.detailInDatabase(vm_db_id, hidden_detail_name), "hidden detail should still exist in db") + + + def detailInDatabase(self, vm_id, detail_name): + dbresult = self.dbclient.execute("select id from user_vm_details where vm_id=%s and name='%s'" % (vm_id, detail_name)) + self.debug("Detail %s for VM %s: %s" % (detail_name, vm_id, dbresult)) + if validateList(dbresult)[0] == PASS: + return True + return False + + + def listVmById(self, id): + list_vms = VirtualMachine.list(self.apiclient, id=id) + self.assertEqual( + isinstance(list_vms, list), + True, + "List VM response was not a valid list" + ) + self.assertNotEqual( + len(list_vms), + 0, + "List VM response was empty" + ) + return list_vms[0] + def tearDown(self): try: cleanup_resources(self.apiclient, self.cleanup)