Skip to content

Fix NPE on updating security groups for an instance #10493

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 22, 2025
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
77 changes: 40 additions & 37 deletions server/src/main/java/com/cloud/vm/UserVmManagerImpl.java
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -3105,42 +3104,6 @@
}
}

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<? extends Nic> nics = _nicDao.listByVmId(vm.getId());
if (hostName != null) {
// Check is hostName is RFC compliant
Expand Down Expand Up @@ -3173,6 +3136,35 @@
.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");

Check warning on line 3142 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java#L3142

Added line #L3142 was not covered by tests
} else if (securityGroupIdList != null){
DataCenterVO zone = _dcDao.findById(vm.getDataCenterId());
List<Long> 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 {

Check warning on line 3151 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java#L3149-L3151

Added lines #L3149 - L3151 were not covered by tests
networkIds = networks.stream().map(Network::getId).collect(Collectors.toList());
}
} catch (InvalidParameterValueException e) {

Check warning on line 3154 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java#L3154

Added line #L3154 was not covered by tests
if(logger.isDebugEnabled()) {
logger.debug(e.getMessage(),e);

Check warning on line 3156 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java#L3156

Added line #L3156 was not covered by tests
}
}

if (_networkModel.checkSecurityGroupSupportForNetwork(
_accountMgr.getActiveAccountById(vm.getAccountId()),
zone, networkIds, securityGroupIdList)
) {
updateSecurityGroup(vm, securityGroupIdList);

Check warning on line 3164 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java#L3164

Added line #L3164 was not covered by tests
}
}

_vmDao.updateVM(id, displayName, ha, osTypeId, userData, userDataId,
userDataDetails, isDisplayVmEnabled, isDynamicallyScalable,
deleteProtection, customId, hostName, instanceName);
Expand All @@ -3188,6 +3180,17 @@
return _vmDao.findById(id);
}

private void updateSecurityGroup(UserVmVO vm, List<Long> securityGroupIdList) {

Check warning on line 3183 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java#L3183

Added line #L3183 was not covered by tests
if (vm.getState() == State.Stopped) {
// Remove instance from security groups
_securityGroupMgr.removeInstanceFromGroups(vm);

Check warning on line 3186 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java#L3186

Added line #L3186 was not covered by tests
// Add instance in provided groups
_securityGroupMgr.addInstanceToGroups(vm, securityGroupIdList);

Check warning on line 3188 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java#L3188

Added line #L3188 was not covered by tests
} else {
throw new InvalidParameterValueException("Virtual machine must be stopped prior to update security groups ");

Check warning on line 3190 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java#L3190

Added line #L3190 was not covered by tests
}
}

Check warning on line 3192 in server/src/main/java/com/cloud/vm/UserVmManagerImpl.java

View check run for this annotation

Codecov / codecov/patch

server/src/main/java/com/cloud/vm/UserVmManagerImpl.java#L3192

Added line #L3192 was not covered by tests

protected void updateUserData(UserVm vm) throws ResourceUnavailableException, InsufficientCapacityException {
boolean result = updateUserDataInternal(vm);
if (result) {
Expand Down
2 changes: 1 addition & 1 deletion ui/src/views/compute/EditVM.vue
Original file line number Diff line number Diff line change
Expand Up @@ -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 () {
Expand Down
1 change: 1 addition & 0 deletions ui/src/views/compute/InstanceTab.vue
Original file line number Diff line number Diff line change
Expand Up @@ -179,6 +179,7 @@ export default {
vm: {},
totalStorage: 0,
currentTab: 'details',
showUpdateSecurityGroupsModal: false,
showAddVolumeModal: false,
diskOfferings: [],
annotations: [],
Expand Down
Loading