This is an automated email from the ASF dual-hosted git repository. vishesh pushed a commit to branch enforce-strict-host-tag-check in repository https://gitbox.apache.org/repos/asf/cloudstack.git
commit e7ff0325fe982c9c1c9972c54ac37dda3a70a1be Author: Vishesh <[email protected]> AuthorDate: Wed Apr 24 15:36:20 2024 +0530 Enforce strict host tag checking --- .../src/main/java/com/cloud/host/HostVO.java | 15 ++++---- .../src/test/java/com/cloud/host/HostVOTest.java | 35 ++++++++++-------- .../deploy/DeploymentPlanningManagerImpl.java | 3 +- .../src/main/java/com/cloud/vm/UserVmManager.java | 18 ++++++++++ .../main/java/com/cloud/vm/UserVmManagerImpl.java | 38 +++++++++++++++----- .../cloudstack/vm/UnmanagedVMsManagerImpl.java | 2 +- .../java/com/cloud/vm/UserVmManagerImplTest.java | 41 ++++++++++++++++++++++ .../cloudstack/vm/UnmanagedVMsManagerImplTest.java | 1 + 8 files changed, 121 insertions(+), 32 deletions(-) diff --git a/engine/schema/src/main/java/com/cloud/host/HostVO.java b/engine/schema/src/main/java/com/cloud/host/HostVO.java index 3e64d20d0e2..d5f99a07b42 100644 --- a/engine/schema/src/main/java/com/cloud/host/HostVO.java +++ b/engine/schema/src/main/java/com/cloud/host/HostVO.java @@ -16,13 +16,13 @@ // under the License. package com.cloud.host; -import java.util.ArrayList; import java.util.Arrays; import java.util.Date; import java.util.HashMap; import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.UUID; import javax.persistence.Column; @@ -768,24 +768,25 @@ public class HostVO implements Host { this.uuid = uuid; } - public boolean checkHostServiceOfferingAndTemplateTags(ServiceOffering serviceOffering, VirtualMachineTemplate template) { + public boolean checkHostServiceOfferingAndTemplateTags(ServiceOffering serviceOffering, VirtualMachineTemplate template, Set<String> strictHostTags) { if (serviceOffering == null || template == null) { return false; } if (StringUtils.isEmpty(serviceOffering.getHostTag()) && StringUtils.isEmpty(template.getTemplateTag())) { return true; } - if (getHostTags() == null) { - return false; - } HashSet<String> hostTagsSet = new HashSet<>(getHostTags()); - List<String> tags = new ArrayList<>(); + HashSet<String> tags = new HashSet<>(); if (StringUtils.isNotEmpty(serviceOffering.getHostTag())) { tags.addAll(Arrays.asList(serviceOffering.getHostTag().split(","))); } - if (StringUtils.isNotEmpty(template.getTemplateTag()) && !tags.contains(template.getTemplateTag())) { + if (StringUtils.isNotEmpty(template.getTemplateTag())) { tags.add(template.getTemplateTag()); } + tags.removeIf(tag -> !strictHostTags.contains(tag)); + if (tags.isEmpty()) { + return true; + } return hostTagsSet.containsAll(tags); } diff --git a/engine/schema/src/test/java/com/cloud/host/HostVOTest.java b/engine/schema/src/test/java/com/cloud/host/HostVOTest.java index cd9ac3cc172..a49c70511c8 100755 --- a/engine/schema/src/test/java/com/cloud/host/HostVOTest.java +++ b/engine/schema/src/test/java/com/cloud/host/HostVOTest.java @@ -20,14 +20,17 @@ import com.cloud.offering.ServiceOffering; import com.cloud.service.ServiceOfferingVO; import com.cloud.template.VirtualMachineTemplate; import com.cloud.vm.VirtualMachine; +import org.junit.Before; +import org.junit.Test; +import org.mockito.Mockito; + import java.util.Arrays; +import java.util.Collections; import java.util.List; +import java.util.Set; import static org.junit.Assert.assertFalse; import static org.junit.Assert.assertTrue; -import org.junit.Test; -import org.junit.Before; -import org.mockito.Mockito; public class HostVOTest { HostVO host; @@ -37,7 +40,7 @@ public class HostVOTest { public void setUp() throws Exception { host = new HostVO(); offering = new ServiceOfferingVO("TestSO", 0, 0, 0, 0, 0, - false, "TestSO", false,VirtualMachine.Type.User,false); + false, "TestSO", false, VirtualMachine.Type.User, false); } @Test @@ -52,14 +55,14 @@ public class HostVOTest { @Test public void testRightTag() { - host.setHostTags(Arrays.asList("tag1","tag2"), false); + host.setHostTags(Arrays.asList("tag1", "tag2"), false); offering.setHostTag("tag2,tag1"); assertTrue(host.checkHostServiceOfferingTags(offering)); } @Test public void testWrongTag() { - host.setHostTags(Arrays.asList("tag1","tag2"), false); + host.setHostTags(Arrays.asList("tag1", "tag2"), false); offering.setHostTag("tag2,tag4"); assertFalse(host.checkHostServiceOfferingTags(offering)); } @@ -87,40 +90,42 @@ public class HostVOTest { @Test public void testEitherNoSOOrTemplate() { - assertFalse(host.checkHostServiceOfferingAndTemplateTags(null, Mockito.mock(VirtualMachineTemplate.class))); - assertFalse(host.checkHostServiceOfferingAndTemplateTags(Mockito.mock(ServiceOffering.class), null)); + assertFalse(host.checkHostServiceOfferingAndTemplateTags(null, Mockito.mock(VirtualMachineTemplate.class), null)); + assertFalse(host.checkHostServiceOfferingAndTemplateTags(Mockito.mock(ServiceOffering.class), null, null)); } @Test public void testNoTagOfferingTemplate() { - assertTrue(host.checkHostServiceOfferingAndTemplateTags(offering, Mockito.mock(VirtualMachineTemplate.class))); + assertTrue(host.checkHostServiceOfferingAndTemplateTags(offering, Mockito.mock(VirtualMachineTemplate.class), Collections.emptySet())); + assertTrue(host.checkHostServiceOfferingAndTemplateTags(offering, Mockito.mock(VirtualMachineTemplate.class), Set.of("tag1", "tag2"))); } @Test public void testRightTagOfferingTemplate() { host.setHostTags(Arrays.asList("tag1", "tag2"), false); offering.setHostTag("tag2,tag1"); - assertTrue(host.checkHostServiceOfferingAndTemplateTags(offering, Mockito.mock(VirtualMachineTemplate.class))); + assertTrue(host.checkHostServiceOfferingAndTemplateTags(offering, Mockito.mock(VirtualMachineTemplate.class), Set.of("tag1"))); host.setHostTags(Arrays.asList("tag1", "tag2", "tag3"), false); offering.setHostTag("tag2,tag1"); VirtualMachineTemplate template = Mockito.mock(VirtualMachineTemplate.class); Mockito.when(template.getTemplateTag()).thenReturn("tag3"); - assertTrue(host.checkHostServiceOfferingAndTemplateTags(offering, template)); + assertTrue(host.checkHostServiceOfferingAndTemplateTags(offering, template, Set.of("tag2", "tag3"))); host.setHostTags(List.of("tag3"), false); offering.setHostTag(null); - assertTrue(host.checkHostServiceOfferingAndTemplateTags(offering, template)); + assertTrue(host.checkHostServiceOfferingAndTemplateTags(offering, template, Set.of("tag3"))); + assertTrue(host.checkHostServiceOfferingAndTemplateTags(offering, template, Set.of("tag2", "tag1"))); } @Test public void testWrongOfferingTag() { - host.setHostTags(Arrays.asList("tag1","tag2"), false); + host.setHostTags(Arrays.asList("tag1", "tag2"), false); offering.setHostTag("tag2,tag4"); VirtualMachineTemplate template = Mockito.mock(VirtualMachineTemplate.class); Mockito.when(template.getTemplateTag()).thenReturn("tag1"); - assertFalse(host.checkHostServiceOfferingAndTemplateTags(offering, template)); + assertFalse(host.checkHostServiceOfferingAndTemplateTags(offering, template, Set.of("tag1", "tag2", "tag3", "tag4"))); offering.setHostTag("tag1,tag2"); template = Mockito.mock(VirtualMachineTemplate.class); Mockito.when(template.getTemplateTag()).thenReturn("tag3"); - assertFalse(host.checkHostServiceOfferingAndTemplateTags(offering, template)); + assertFalse(host.checkHostServiceOfferingAndTemplateTags(offering, template, Set.of("tag1", "tag2", "tag3", "tag4"))); } } diff --git a/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java b/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java index d97fcef7453..b5a916143db 100644 --- a/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java +++ b/server/src/main/java/com/cloud/deploy/DeploymentPlanningManagerImpl.java @@ -35,6 +35,7 @@ import java.util.stream.Collectors; import javax.inject.Inject; import javax.naming.ConfigurationException; +import com.cloud.vm.UserVmManager; import org.apache.cloudstack.affinity.AffinityGroupDomainMapVO; import com.cloud.storage.VMTemplateVO; import com.cloud.storage.dao.VMTemplateDao; @@ -778,7 +779,7 @@ StateListener<State, VirtualMachine.Event, VirtualMachine>, Configurable { VirtualMachineTemplate template = vmProfile.getTemplate(); if (offering.getHostTag() != null || template.getTemplateTag() != null) { _hostDao.loadHostTags(host); - if (!host.checkHostServiceOfferingAndTemplateTags(offering, template)) { + if (!host.checkHostServiceOfferingAndTemplateTags(offering, template, UserVmManager.getStrictHostTags())) { logger.debug("Service Offering host tag or template tag does not match the last host of this VM"); return false; } diff --git a/server/src/main/java/com/cloud/vm/UserVmManager.java b/server/src/main/java/com/cloud/vm/UserVmManager.java index b107a520205..f0035531aaa 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManager.java +++ b/server/src/main/java/com/cloud/vm/UserVmManager.java @@ -17,8 +17,10 @@ package com.cloud.vm; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import org.apache.cloudstack.api.BaseCmd.HTTPMethod; import org.apache.cloudstack.framework.config.ConfigKey; @@ -38,6 +40,8 @@ import com.cloud.template.VirtualMachineTemplate; import com.cloud.uservm.UserVm; import com.cloud.utils.Pair; +import static com.cloud.user.ResourceLimitService.ResourceLimitHostTags; + /** * * @@ -59,6 +63,12 @@ public interface UserVmManager extends UserVmService { "Destroys the VM's root volume when the VM is destroyed.", true, ConfigKey.Scope.Domain); + ConfigKey<Boolean> EnforceStrictResourceLimitHostTagCheck = new ConfigKey<Boolean>( + "Advanced", Boolean.class, "vm.strict.resource.limit.host.tag.check", "true", + "Determines whether the resource limits tags are considered strict or not", true); + ConfigKey<String> StrictHostTags = new ConfigKey<>("Advanced", String.class, "vm.strict.host.tags", "", + "A comma-separated list of tags for strict host check", true); + static final int MAX_USER_DATA_LENGTH_BYTES = 2048; public static final String CKS_NODE = "cksnode"; @@ -127,6 +137,14 @@ public interface UserVmManager extends UserVmService { void generateUsageEvent(VirtualMachine vm, boolean isDisplay, String eventType); + static Set<String> getStrictHostTags() { + Set<String> strictHostTags = new HashSet<>(List.of(StrictHostTags.value().split(","))); + if (EnforceStrictResourceLimitHostTagCheck.value()) { + strictHostTags.addAll(List.of(ResourceLimitHostTags.value().split(","))); + } + return strictHostTags; + } + void persistDeviceBusInfo(UserVmVO paramUserVmVO, String paramString); boolean checkIfDynamicScalingCanBeEnabled(VirtualMachine vm, ServiceOffering offering, VirtualMachineTemplate template, Long zoneId); diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index c924a124fa1..346d383eae8 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -1964,9 +1964,9 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir HostVO instanceHost = _hostDao.findById(vmInstance.getHostId()); _hostDao.loadHostTags(instanceHost); - if (!instanceHost.checkHostServiceOfferingAndTemplateTags(newServiceOfferingVO, template)) { - logger.error(String.format("Cannot upgrade VM [%s] as the new service offering [%s] does not have the required host tags %s.", vmInstance, newServiceOfferingVO, - instanceHost.getHostTags())); + if (!instanceHost.checkHostServiceOfferingAndTemplateTags(newServiceOfferingVO, template, UserVmManager.getStrictHostTags())) { + logger.error(String.format("Cannot upgrade VM [%s] as the new service offering [%s] does not have the required host tags %s.", + vmInstance, newServiceOfferingVO, instanceHost.getHostTags())); return false; } } @@ -5484,11 +5484,14 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir boolean isRootAdmin = _accountService.isRootAdmin(callerAccount.getId()); Pod destinationPod = getDestinationPod(podId, isRootAdmin); Cluster destinationCluster = getDestinationCluster(clusterId, isRootAdmin); - Host destinationHost = getDestinationHost(hostId, isRootAdmin, isExplicitHost); + HostVO destinationHost = getDestinationHost(hostId, isRootAdmin, isExplicitHost); DataCenterDeployment plan = null; boolean deployOnGivenHost = false; if (destinationHost != null) { logger.debug("Destination Host to deploy the VM is specified, specifying a deployment plan to deploy the VM"); + _hostDao.loadHostTags(destinationHost); + checkEnforceStrictHostTagCheck(vm, destinationHost); + final ServiceOfferingVO offering = serviceOfferingDao.findById(vm.getId(), vm.getServiceOfferingId()); Pair<Boolean, Boolean> cpuCapabilityAndCapacity = _capacityMgr.checkIfHostHasCpuCapabilityAndCapacity(destinationHost, offering, false); if (!cpuCapabilityAndCapacity.first() || !cpuCapabilityAndCapacity.second()) { @@ -5659,8 +5662,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir return destinationCluster; } - private Host getDestinationHost(Long hostId, boolean isRootAdmin, boolean isExplicitHost) { - Host destinationHost = null; + private HostVO getDestinationHost(Long hostId, boolean isRootAdmin, boolean isExplicitHost) { + HostVO destinationHost = null; if (hostId != null) { if (isExplicitHost && !isRootAdmin) { throw new PermissionDeniedException( @@ -6702,6 +6705,21 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir } } + protected void checkEnforceStrictHostTagCheck(VMInstanceVO vm, HostVO host) { + ServiceOffering serviceOffering = serviceOfferingDao.findByIdIncludingRemoved(vm.getServiceOfferingId()); + VirtualMachineTemplate template = _templateDao.findByIdIncludingRemoved(vm.getTemplateId()); + + Set<String> strictHostTags = UserVmManager.getStrictHostTags(); + if (!host.checkHostServiceOfferingAndTemplateTags(serviceOffering, template, strictHostTags)) { + s_logger.error(String.format( + "Cannot deploy VM: %s to host : %s due to tag mismatch." + + " strictHosts: %s serviceOffering tags: %s, template tags: %s", + vm, host, strictHostTags, serviceOffering.getHostTag(), template.getTemplateTag())); + throw new InvalidParameterValueException(String.format("Cannot deploy VM, destination host: %s " + + "is not compatible for the VM", host.getName())); + } + } + private DeployDestination checkVmMigrationDestination(VMInstanceVO vm, Host srcHost, Host destinationHost) throws VirtualMachineMigrationException { if (destinationHost == null) { return null; @@ -6727,6 +6745,10 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir throw new CloudRuntimeException("Cannot migrate VM, VM is DPDK enabled VM but destination host is not DPDK enabled"); } + HostVO destinationHostVO = _hostDao.findById(destinationHost.getId()); + _hostDao.loadHostTags(destinationHostVO); + checkEnforceStrictHostTagCheck(vm, destinationHostVO); + checkHostsDedication(vm, srcHost.getId(), destinationHost.getId()); // call to core process @@ -6736,7 +6758,6 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir DeployDestination dest = new DeployDestination(dcVO, pod, cluster, destinationHost); // check max guest vm limit for the destinationHost - HostVO destinationHostVO = _hostDao.findById(destinationHost.getId()); if (_capacityMgr.checkIfHostReachMaxGuestLimit(destinationHostVO)) { if (logger.isDebugEnabled()) { logger.debug("Host name: " + destinationHost.getName() + ", hostId: " + destinationHost.getId() @@ -8395,7 +8416,8 @@ public class UserVmManagerImpl extends ManagerBase implements UserVmManager, Vir public ConfigKey<?>[] getConfigKeys() { return new ConfigKey<?>[] {EnableDynamicallyScaleVm, AllowDiskOfferingChangeDuringScaleVm, AllowUserExpungeRecoverVm, VmIpFetchWaitInterval, VmIpFetchTrialMax, VmIpFetchThreadPoolMax, VmIpFetchTaskWorkers, AllowDeployVmIfGivenHostFails, EnableAdditionalVmConfig, DisplayVMOVFProperties, - KvmAdditionalConfigAllowList, XenServerAdditionalConfigAllowList, VmwareAdditionalConfigAllowList, DestroyRootVolumeOnVmDestruction}; + KvmAdditionalConfigAllowList, XenServerAdditionalConfigAllowList, VmwareAdditionalConfigAllowList, DestroyRootVolumeOnVmDestruction, + EnforceStrictResourceLimitHostTagCheck, StrictHostTags}; } @Override diff --git a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java index f2b552c7c07..3789d6952bc 100644 --- a/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java +++ b/server/src/main/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImpl.java @@ -447,7 +447,7 @@ public class UnmanagedVMsManagerImpl implements UnmanagedVMsManager { return true; } hostDao.loadHostTags(host); - return host.checkHostServiceOfferingAndTemplateTags(serviceOffering, template); + return host.checkHostServiceOfferingAndTemplateTags(serviceOffering, template, UserVmManager.getStrictHostTags()); } private boolean storagePoolSupportsDiskOffering(StoragePool pool, DiskOffering diskOffering) { diff --git a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java index c464af6385b..0821e641ac5 100644 --- a/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java +++ b/server/src/test/java/com/cloud/vm/UserVmManagerImplTest.java @@ -1563,4 +1563,45 @@ public class UserVmManagerImplTest { Assert.fail(e.getMessage()); } } + + @Test + public void testCheckEnforceStrictHostTagCheckPass() { + ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + VMTemplateVO template = Mockito.mock(VMTemplateVO.class); + + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + HostVO destinationHostVO = Mockito.mock(HostVO.class); + + Mockito.when(_serviceOfferingDao.findByIdIncludingRemoved(1L)).thenReturn(serviceOffering); + Mockito.when(templateDao.findByIdIncludingRemoved(2L)).thenReturn(template); + + Mockito.when(vm.getServiceOfferingId()).thenReturn(1L); + Mockito.when(vm.getTemplateId()).thenReturn(2L); + + Mockito.when(destinationHostVO.checkHostServiceOfferingAndTemplateTags(Mockito.any(ServiceOffering.class), Mockito.any(VirtualMachineTemplate.class), Mockito.anySet())).thenReturn(true); + + userVmManagerImpl.checkEnforceStrictHostTagCheck(vm, destinationHostVO); + + Mockito.verify( + destinationHostVO, Mockito.times(1) + ).checkHostServiceOfferingAndTemplateTags(Mockito.any(ServiceOffering.class), Mockito.any(VirtualMachineTemplate.class), Mockito.anySet()); + } + + @Test(expected = InvalidParameterValueException.class) + public void testCheckEnforceStrictHostTagCheckFail() { + ServiceOfferingVO serviceOffering = Mockito.mock(ServiceOfferingVO.class); + VMTemplateVO template = Mockito.mock(VMTemplateVO.class); + + VMInstanceVO vm = Mockito.mock(VMInstanceVO.class); + HostVO destinationHostVO = Mockito.mock(HostVO.class); + + Mockito.when(_serviceOfferingDao.findByIdIncludingRemoved(1L)).thenReturn(serviceOffering); + Mockito.when(templateDao.findByIdIncludingRemoved(2L)).thenReturn(template); + + Mockito.when(vm.getServiceOfferingId()).thenReturn(1L); + Mockito.when(vm.getTemplateId()).thenReturn(2L); + + Mockito.when(destinationHostVO.checkHostServiceOfferingAndTemplateTags(Mockito.any(ServiceOffering.class), Mockito.any(VirtualMachineTemplate.class), Mockito.anySet())).thenReturn(false); + userVmManagerImpl.checkEnforceStrictHostTagCheck(vm, destinationHostVO); + } } diff --git a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java index 81358d99ae7..761949bf65d 100644 --- a/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java +++ b/server/src/test/java/org/apache/cloudstack/vm/UnmanagedVMsManagerImplTest.java @@ -302,6 +302,7 @@ public class UnmanagedVMsManagerImplTest { HostVO hostVO = Mockito.mock(HostVO.class); when(hostVO.isInMaintenanceStates()).thenReturn(false); hosts.add(hostVO); + when(hostVO.checkHostServiceOfferingAndTemplateTags(Mockito.any(), Mockito.any(), Mockito.any())).thenReturn(true); when(resourceManager.listHostsInClusterByStatus(anyLong(), any(Status.class))).thenReturn(hosts); when(resourceManager.listAllUpAndEnabledHostsInOneZoneByHypervisor(any(Hypervisor.HypervisorType.class), anyLong())).thenReturn(hosts); List<VMTemplateStoragePoolVO> templates = new ArrayList<>();
