From 351b0120f4efacade370b55b3189af34a37712ea Mon Sep 17 00:00:00 2001 From: Vishesh Date: Tue, 4 Mar 2025 00:34:10 +0530 Subject: [PATCH 1/3] Fix NPE on updating security groups for an instance --- .../java/com/cloud/vm/UserVmManagerImpl.java | 77 ++++++++++--------- ui/src/views/compute/EditVM.vue | 2 +- ui/src/views/compute/InstanceTab.vue | 1 + 3 files changed, 42 insertions(+), 38 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 021c6ff62267..59590c2d68f2 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -28,7 +28,6 @@ import java.net.URLDecoder; import java.util.ArrayList; import java.util.Arrays; -import java.util.Collections; import java.util.Date; import java.util.HashMap; import java.util.HashSet; @@ -3105,42 +3104,6 @@ public UserVm updateVirtualMachine(long id, String displayName, String group, Bo } } - boolean isVMware = (vm.getHypervisorType() == HypervisorType.VMware); - - if (securityGroupIdList != null && isVMware) { - throw new InvalidParameterValueException("Security group feature is not supported for vmWare hypervisor"); - } else { - // Get default guest network in Basic zone - Network defaultNetwork = null; - try { - DataCenterVO zone = _dcDao.findById(vm.getDataCenterId()); - if (zone.getNetworkType() == NetworkType.Basic) { - // Get default guest network in Basic zone - defaultNetwork = _networkModel.getExclusiveGuestNetwork(zone.getId()); - } else if (_networkModel.checkSecurityGroupSupportForNetwork(_accountMgr.getActiveAccountById(vm.getAccountId()), zone, Collections.emptyList(), securityGroupIdList)) { - NicVO defaultNic = _nicDao.findDefaultNicForVM(vm.getId()); - if (defaultNic != null) { - defaultNetwork = _networkDao.findById(defaultNic.getNetworkId()); - } - } - } catch (InvalidParameterValueException e) { - if(logger.isDebugEnabled()) { - logger.debug(e.getMessage(),e); - } - defaultNetwork = _networkModel.getDefaultNetworkForVm(id); - } - - if (securityGroupIdList != null && _networkModel.isSecurityGroupSupportedInNetwork(defaultNetwork) && _networkModel.canAddDefaultSecurityGroup()) { - if (vm.getState() == State.Stopped) { - // Remove instance from security groups - _securityGroupMgr.removeInstanceFromGroups(vm); - // Add instance in provided groups - _securityGroupMgr.addInstanceToGroups(vm, securityGroupIdList); - } else { - throw new InvalidParameterValueException("Virtual machine must be stopped prior to update security groups "); - } - } - } List nics = _nicDao.listByVmId(vm.getId()); if (hostName != null) { // Check is hostName is RFC compliant @@ -3173,6 +3136,35 @@ public UserVm updateVirtualMachine(long id, String displayName, String group, Bo .getUuid(), nic.getId(), extraDhcpOptionsMap); } + boolean isVMware = (vm.getHypervisorType() == HypervisorType.VMware); + + if (securityGroupIdList != null && isVMware) { + throw new InvalidParameterValueException("Security group feature is not supported for vmWare hypervisor"); + } else if (securityGroupIdList != null){ + DataCenterVO zone = _dcDao.findById(vm.getDataCenterId()); + List networkIds = new ArrayList<>(); + try { + if (zone.getNetworkType() == NetworkType.Basic) { + // Get default guest network in Basic zone + Network defaultNetwork = _networkModel.getExclusiveGuestNetwork(zone.getId()); + networkIds.add(defaultNetwork.getId()); + } else { + networkIds = networks.stream().map(Network::getId).collect(Collectors.toList()); + } + } catch (InvalidParameterValueException e) { + if(logger.isDebugEnabled()) { + logger.debug(e.getMessage(),e); + } + } + + if (_networkModel.checkSecurityGroupSupportForNetwork( + _accountMgr.getActiveAccountById(vm.getAccountId()), + zone, networkIds, securityGroupIdList) + ) { + updateSecurityGroup(vm, securityGroupIdList); + } + } + _vmDao.updateVM(id, displayName, ha, osTypeId, userData, userDataId, userDataDetails, isDisplayVmEnabled, isDynamicallyScalable, deleteProtection, customId, hostName, instanceName); @@ -3188,6 +3180,17 @@ public UserVm updateVirtualMachine(long id, String displayName, String group, Bo return _vmDao.findById(id); } + private void updateSecurityGroup(UserVmVO vm, List securityGroupIdList) { + if (vm.getState() == State.Stopped) { + // Remove instance from security groups + _securityGroupMgr.removeInstanceFromGroups(vm); + // Add instance in provided groups + _securityGroupMgr.addInstanceToGroups(vm, securityGroupIdList); + } else { + throw new InvalidParameterValueException("Virtual machine must be stopped prior to update security groups "); + } + } + protected void updateUserData(UserVm vm) throws ResourceUnavailableException, InsufficientCapacityException { boolean result = updateUserDataInternal(vm); if (result) { diff --git a/ui/src/views/compute/EditVM.vue b/ui/src/views/compute/EditVM.vue index f2d679ee4445..75a297cee3e0 100644 --- a/ui/src/views/compute/EditVM.vue +++ b/ui/src/views/compute/EditVM.vue @@ -206,7 +206,7 @@ export default { zoneid: this.resource.zoneid }).then(response => { const zone = response?.listzonesresponse?.zone || [] - this.securityGroupsEnabled = zone?.[0]?.securitygroupsenabled + this.securityGroupsEnabled = zone?.[0]?.securitygroupsenabled || this.$store.getters.showSecurityGroups }) }, fetchSecurityGroups () { diff --git a/ui/src/views/compute/InstanceTab.vue b/ui/src/views/compute/InstanceTab.vue index b22f576e70ad..925f707591ad 100644 --- a/ui/src/views/compute/InstanceTab.vue +++ b/ui/src/views/compute/InstanceTab.vue @@ -179,6 +179,7 @@ export default { vm: {}, totalStorage: 0, currentTab: 'details', + showUpdateSecurityGroupsModal: false, showAddVolumeModal: false, diskOfferings: [], annotations: [], From 6c1505463952ac4afe32b765d02850e3bc18ea25 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Wed, 16 Apr 2025 11:13:57 +0530 Subject: [PATCH 2/3] addressed review comments --- .../src/main/java/com/cloud/vm/UserVmManagerImpl.java | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 59590c2d68f2..254800f69bbc 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -3139,8 +3139,8 @@ public UserVm updateVirtualMachine(long id, String displayName, String group, Bo boolean isVMware = (vm.getHypervisorType() == HypervisorType.VMware); if (securityGroupIdList != null && isVMware) { - throw new InvalidParameterValueException("Security group feature is not supported for vmWare hypervisor"); - } else if (securityGroupIdList != null){ + throw new InvalidParameterValueException("Security group feature is not supported for VMware hypervisor"); + } else if (securityGroupIdList != null) { DataCenterVO zone = _dcDao.findById(vm.getDataCenterId()); List networkIds = new ArrayList<>(); try { @@ -3187,7 +3187,7 @@ private void updateSecurityGroup(UserVmVO vm, List securityGroupIdList) { // Add instance in provided groups _securityGroupMgr.addInstanceToGroups(vm, securityGroupIdList); } else { - throw new InvalidParameterValueException("Virtual machine must be stopped prior to update security groups "); + throw new InvalidParameterValueException(String.format("VM %s must be stopped prior to update security groups", vm.getUuid())); } } @@ -3698,7 +3698,7 @@ public UserVm createBasicSecurityGroupVirtualMachine(DataCenter zone, ServiceOff boolean isVmWare = (template.getHypervisorType() == HypervisorType.VMware || (hypervisor != null && hypervisor == HypervisorType.VMware)); if (securityGroupIdList != null && isVmWare) { - throw new InvalidParameterValueException("Security group feature is not supported for vmWare hypervisor"); + throw new InvalidParameterValueException("Security group feature is not supported for VMware hypervisor"); } else if (!isVmWare && _networkModel.isSecurityGroupSupportedInNetwork(defaultNetwork) && _networkModel.canAddDefaultSecurityGroup()) { //add the default securityGroup only if no security group is specified if (securityGroupIdList == null || securityGroupIdList.isEmpty()) { @@ -3758,7 +3758,7 @@ public UserVm createAdvancedSecurityGroupVirtualMachine(DataCenter zone, Service } else if (securityGroupIdList != null && !securityGroupIdList.isEmpty()) { if (isVmWare) { - throw new InvalidParameterValueException("Security group feature is not supported for vmWare hypervisor"); + throw new InvalidParameterValueException("Security group feature is not supported for VMware hypervisor"); } // Only one network can be specified, and it should be security group enabled if (networkIdList.size() > 1 && template.getHypervisorType() != HypervisorType.KVM && hypervisor != HypervisorType.KVM) { From 3ca9b61c8bf84813b08ca5916e237c18bc466598 Mon Sep 17 00:00:00 2001 From: Harikrishna Patnala Date: Wed, 16 Apr 2025 16:07:45 +0530 Subject: [PATCH 3/3] Method refactoring --- .../java/com/cloud/vm/UserVmManagerImpl.java | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java index 254800f69bbc..167995a6b5f3 100644 --- a/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java +++ b/server/src/main/java/com/cloud/vm/UserVmManagerImpl.java @@ -3136,6 +3136,24 @@ public UserVm updateVirtualMachine(long id, String displayName, String group, Bo .getUuid(), nic.getId(), extraDhcpOptionsMap); } + checkAndUpdateSecurityGroupForVM(securityGroupIdList, vm, networks); + + _vmDao.updateVM(id, displayName, ha, osTypeId, userData, userDataId, + userDataDetails, isDisplayVmEnabled, isDynamicallyScalable, + deleteProtection, customId, hostName, instanceName); + + if (updateUserdata) { + updateUserData(vm); + } + + if (State.Running == vm.getState()) { + updateDns(vm, hostName); + } + + return _vmDao.findById(id); + } + + private void checkAndUpdateSecurityGroupForVM(List securityGroupIdList, UserVmVO vm, List networks) { boolean isVMware = (vm.getHypervisorType() == HypervisorType.VMware); if (securityGroupIdList != null && isVMware) { @@ -3164,20 +3182,6 @@ public UserVm updateVirtualMachine(long id, String displayName, String group, Bo updateSecurityGroup(vm, securityGroupIdList); } } - - _vmDao.updateVM(id, displayName, ha, osTypeId, userData, userDataId, - userDataDetails, isDisplayVmEnabled, isDynamicallyScalable, - deleteProtection, customId, hostName, instanceName); - - if (updateUserdata) { - updateUserData(vm); - } - - if (State.Running == vm.getState()) { - updateDns(vm, hostName); - } - - return _vmDao.findById(id); } private void updateSecurityGroup(UserVmVO vm, List securityGroupIdList) {