bug 9873: always add default security group to the SG list when deploy vm in 1) Basic zone 2) Advance zone using SG enabled network

status 9873: resolved fixed

Following fixes were made as a part of the checkin:

* When deploy user vm and default SG doesn't exist in the DB, create it automatically.
* SecurityGroup enabled use vm start: if map to default group is not present in security_group_vm_map table, create one.
* Added "name" (securityGroupName) parameter back to deleteSecurityGroup/authorizeSecurityGroupIngress/deployVm. Mutually exclusive with security group id parameter.
This commit is contained in:
alena 2011-05-17 14:16:23 -07:00
parent ca84d4e496
commit 550a4cda66
15 changed files with 221 additions and 49 deletions

View File

@ -136,6 +136,7 @@ public class ApiConstants {
public static final String SCOPE = "scope";
public static final String SECRET_KEY = "secretkey";
public static final String SECURITY_GROUP_IDS = "securitygroupids";
public static final String SECURITY_GROUP_NAMES = "securitygroupnames";
public static final String SECURITY_GROUP_NAME = "securitygroupname";
public static final String SECURITY_GROUP_ID = "securitygroupid";
public static final String SECURITY_GROUP_EANBLED = "securitygroupenabled";

View File

@ -204,5 +204,7 @@ public interface ResponseGenerator {
UserResponse createUserResponse(User user);
AccountResponse createUserAccountResponse(UserAccount user);
Long getSecurityGroupId(String groupName, long accountId);
}

View File

@ -36,6 +36,7 @@ import com.cloud.api.response.IngressRuleResponse;
import com.cloud.api.response.SecurityGroupResponse;
import com.cloud.async.AsyncJob;
import com.cloud.event.EventTypes;
import com.cloud.exception.InvalidParameterValueException;
import com.cloud.network.security.IngressRule;
import com.cloud.user.Account;
import com.cloud.user.UserContext;
@ -66,9 +67,6 @@ public class AuthorizeSecurityGroupIngressCmd extends BaseAsyncCmd {
@Parameter(name=ApiConstants.ICMP_CODE, type=CommandType.INTEGER, description="error code for this icmp message")
private Integer icmpCode;
@Parameter(name=ApiConstants.SECURITY_GROUP_ID, type=CommandType.LONG, required=true, description="The ID of the security group")
private Long securityGroupId;
@Parameter(name=ApiConstants.CIDR_LIST, type=CommandType.LIST, collectionType=CommandType.STRING, description="the cidr list associated")
private List cidrList;
@ -80,6 +78,12 @@ public class AuthorizeSecurityGroupIngressCmd extends BaseAsyncCmd {
@Parameter(name=ApiConstants.DOMAIN_ID, type=CommandType.LONG, description="an optional domainId for the security group. If the account parameter is used, domainId must also be used.")
private Long domainId;
@Parameter(name=ApiConstants.SECURITY_GROUP_ID, type=CommandType.LONG, description="The ID of the security group. Mutually exclusive with securityGroupName parameter")
private Long securityGroupId;
@Parameter(name=ApiConstants.SECURITY_GROUP_NAME, type=CommandType.STRING, description="The name of the security group. Mutually exclusive with securityGroupName parameter")
private String securityGroupName;
/////////////////////////////////////////////////////
@ -111,6 +115,17 @@ public class AuthorizeSecurityGroupIngressCmd extends BaseAsyncCmd {
}
public Long getSecurityGroupId() {
if (securityGroupId != null && securityGroupName != null) {
throw new InvalidParameterValueException("securityGroupId and securityGroupName parameters are mutually exclusive");
}
if (securityGroupName != null) {
securityGroupId = _responseGenerator.getSecurityGroupId(securityGroupName, getEntityOwnerId());
if (securityGroupId == null) {
throw new InvalidParameterValueException("Unable to find security group " + securityGroupName + " for account id=" + getEntityOwnerId());
}
}
return securityGroupId;
}
@ -151,15 +166,13 @@ public class AuthorizeSecurityGroupIngressCmd extends BaseAsyncCmd {
Account userAccount = _responseGenerator.findAccountByNameDomain(accountName, domainId);
if (userAccount != null) {
return userAccount.getId();
} else {
throw new InvalidParameterValueException("Unable to find account by name " + accountName + " in domain " + domainId);
}
}
}
if (account != null) {
return account.getId();
}
return Account.ACCOUNT_ID_SYSTEM; // no account info given, parent this command to SYSTEM so ERROR events are tracked
return account.getId();
}
@Override

View File

@ -41,13 +41,13 @@ public class CreateSecurityGroupCmd extends BaseCmd {
@Parameter(name=ApiConstants.ACCOUNT, type=CommandType.STRING, description="an optional account for the security group. Must be used with domainId.")
private String accountName;
@Parameter(name=ApiConstants.DOMAIN_ID, type=CommandType.LONG, description="an optional domainId for the security group. If the account parameter is used, domainId must also be used.")
private Long domainId;
@Parameter(name=ApiConstants.DESCRIPTION, type=CommandType.STRING, description="the description of the security group")
private String description;
@Parameter(name=ApiConstants.DOMAIN_ID, type=CommandType.LONG, description="an optional domainId for the security group. If the account parameter is used, domainId must also be used.")
private Long domainId;
@Parameter(name=ApiConstants.NAME, type=CommandType.STRING, required=true, description="name of the security group")
private String securityGroupName;

View File

@ -8,9 +8,10 @@ import com.cloud.api.Implementation;
import com.cloud.api.Parameter;
import com.cloud.api.ServerApiException;
import com.cloud.api.response.SuccessResponse;
import com.cloud.exception.InvalidParameterValueException;
import com.cloud.exception.ResourceInUseException;
import com.cloud.network.security.SecurityGroup;
import com.cloud.user.Account;
import com.cloud.user.UserContext;
@Implementation(description="Deletes security group", responseObject=SuccessResponse.class)
public class DeleteSecurityGroupCmd extends BaseCmd {
@ -27,8 +28,11 @@ public class DeleteSecurityGroupCmd extends BaseCmd {
@Parameter(name=ApiConstants.DOMAIN_ID, type=CommandType.LONG, description="the domain ID of account owning the security group")
private Long domainId;
@Parameter(name=ApiConstants.ID, type=CommandType.LONG, required=true, description="The ID of the security group")
@Parameter(name=ApiConstants.ID, type=CommandType.LONG, description="The ID of the security group. Mutually exclusive with name parameter")
private Long id;
@Parameter(name=ApiConstants.NAME, type=CommandType.STRING, description="The ID of the security group. Mutually exclusive with id parameter")
private String name;
/////////////////////////////////////////////////////
@ -44,6 +48,17 @@ public class DeleteSecurityGroupCmd extends BaseCmd {
}
public Long getId() {
if (id != null && name != null) {
throw new InvalidParameterValueException("name and id parameters are mutually exclusive");
}
if (name != null) {
id = _responseGenerator.getSecurityGroupId(name, getEntityOwnerId());
if (id == null) {
throw new InvalidParameterValueException("Unable to find security group by name " + name + " for the account id=" + getEntityOwnerId());
}
}
return id;
}
@ -60,12 +75,19 @@ public class DeleteSecurityGroupCmd extends BaseCmd {
@Override
public long getEntityOwnerId() {
SecurityGroup group = _entityMgr.findById(SecurityGroup.class, getId());
if (group != null) {
return group.getAccountId();
Account account = UserContext.current().getCaller();
if ((account == null) || isAdmin(account.getType())) {
if ((domainId != null) && (accountName != null)) {
Account userAccount = _responseGenerator.findAccountByNameDomain(accountName, domainId);
if (userAccount != null) {
return userAccount.getId();
} else {
throw new InvalidParameterValueException("Unable to find account by name " + accountName + " in domain " + domainId);
}
}
}
return Account.ACCOUNT_ID_SYSTEM; // no account info given, parent this command to SYSTEM so ERROR events are tracked
return account.getId();
}
@Override

View File

@ -18,6 +18,7 @@
package com.cloud.api.commands;
import java.util.ArrayList;
import java.util.List;
import org.apache.log4j.Logger;
@ -82,9 +83,6 @@ public class DeployVMCmd extends BaseAsyncCreateCmd {
//Network information
@Parameter(name=ApiConstants.NETWORK_IDS, type=CommandType.LIST, collectionType=CommandType.LONG, description="list of network ids used by virtual machine")
private List<Long> networkIds;
@Parameter(name=ApiConstants.SECURITY_GROUP_IDS, type=CommandType.LIST, collectionType=CommandType.LONG, description="comma separated list of security groups id that going to be applied to the virtual machine. Should be passed only when vm is created from a zone with Basic Network support")
private List<Long> securityGroupIdList;
//DataDisk information
@Parameter(name=ApiConstants.DISK_OFFERING_ID, type=CommandType.LONG, description="the ID of the disk offering for the virtual machine. If the template is of ISO format, the diskOfferingId is for the root disk volume. Otherwise this parameter is used to indicate the offering for the data disk volume. If the templateId parameter passed is from a Template object, the diskOfferingId refers to a DATA Disk Volume created. If the templateId parameter passed is from an ISO object, the diskOfferingId refers to a ROOT Disk Volume created.")
@ -108,6 +106,12 @@ public class DeployVMCmd extends BaseAsyncCreateCmd {
@Parameter(name=ApiConstants.HOST_ID, type=CommandType.LONG, description="destination Host ID to deploy the VM to - parameter available for root admin only")
private Long hostId;
@Parameter(name=ApiConstants.SECURITY_GROUP_IDS, type=CommandType.LIST, collectionType=CommandType.LONG, description="comma separated list of security groups id that going to be applied to the virtual machine. Should be passed only when vm is created from a zone with Basic Network support. Mutually exclusive with securitygroupnames parameter")
private List<Long> securityGroupIdList;
@Parameter(name=ApiConstants.SECURITY_GROUP_NAMES, type=CommandType.LIST, collectionType=CommandType.STRING, description="comma separated list of security groups names that going to be applied to the virtual machine. Should be passed only when vm is created from a zone with Basic Network support. Mutually exclusive with securitygroupids parameter")
private List<String> securityGroupNameList;
/////////////////////////////////////////////////////
/////////////////// Accessors ///////////////////////
/////////////////////////////////////////////////////
@ -143,7 +147,24 @@ public class DeployVMCmd extends BaseAsyncCreateCmd {
}
public List<Long> getSecurityGroupIdList() {
return securityGroupIdList;
if (securityGroupIdList != null && securityGroupIdList != null) {
throw new InvalidParameterValueException("securitygroupids parameter is mutually exclusive with securitygroupnames parameter");
}
//transform group names to ids here
if (securityGroupNameList != null) {
securityGroupIdList = new ArrayList<Long>();
for (String groupName : securityGroupNameList) {
Long groupId = _responseGenerator.getSecurityGroupId(groupName, getEntityOwnerId());
if (groupId == null) {
throw new InvalidParameterValueException("Unable to find group by name " + groupName + " for account " + getEntityOwnerId());
} else {
securityGroupIdList.add(groupId);
}
}
}
return securityGroupIdList;
}
public Long getServiceOfferingId() {
@ -203,15 +224,13 @@ public class DeployVMCmd extends BaseAsyncCreateCmd {
Account userAccount = _responseGenerator.findAccountByNameDomain(accountName, domainId);
if (userAccount != null) {
return userAccount.getId();
} else {
throw new InvalidParameterValueException("Unable to find account by name " + getAccountName() + " in domain " + getDomainId());
}
}
}
if (account != null) {
return account.getId();
}
return Account.ACCOUNT_ID_SYSTEM; // no account info given, parent this command to SYSTEM so ERROR events are tracked
return account.getId();
}
@Override

View File

@ -654,7 +654,7 @@ def default_network_rules(session, args):
return 'false'
for v in vifs:
default_ebtables_rules(vm_chain, v, vm_ip, vm_mac)
default_ebtables_rules(vmchain, v, vm_ip, vm_mac)
if write_rule_log_for_vm(vm_name, vm_id, vm_ip, domid, '_initial_', '-1') == False:
util.SMlog("Failed to log default network rules, ignoring")

View File

@ -105,7 +105,7 @@ public class ApiDBUtils {
private static AccountManager _accountMgr;
private static AgentManager _agentMgr;
public static AsyncJobManager _asyncMgr;
private static SecurityGroupManager _networkGroupMgr;
private static SecurityGroupManager _securityGroupMgr;
private static SnapshotManager _snapMgr;
private static StorageManager _storageMgr;
private static UserVmManager _userVmMgr;
@ -150,7 +150,7 @@ public class ApiDBUtils {
_accountMgr = locator.getManager(AccountManager.class);
_agentMgr = locator.getManager(AgentManager.class);
_asyncMgr = locator.getManager(AsyncJobManager.class);
_networkGroupMgr = locator.getManager(SecurityGroupManager.class);
_securityGroupMgr = locator.getManager(SecurityGroupManager.class);
_snapMgr = locator.getManager(SnapshotManager.class);
_storageMgr = locator.getManager(StorageManager.class);
_userVmMgr = locator.getManager(UserVmManager.class);
@ -252,11 +252,11 @@ public class ApiDBUtils {
}
public static String getNetworkGroupsNamesForVm(long vmId) {
return _networkGroupMgr.getSecurityGroupsNamesForVm(vmId);
return _securityGroupMgr.getSecurityGroupsNamesForVm(vmId);
}
public static List<SecurityGroupVO> getSecurityGroupsForVm(long vmId) {
return _networkGroupMgr.getSecurityGroupsForVm(vmId);
return _securityGroupMgr.getSecurityGroupsForVm(vmId);
}
public static String getSnapshotIntervalTypes(long snapshotId) {
@ -554,5 +554,9 @@ public class ApiDBUtils {
float cpuOverprovisioningFactor = NumbersUtil.parseFloat(opFactor, 1);
return cpuOverprovisioningFactor;
}
public static SecurityGroup getSecurityGroup(String groupName, long ownerId) {
return _securityGroupMgr.getSecurityGroup(groupName, ownerId);
}
}

View File

@ -2430,4 +2430,14 @@ public class ApiResponseHelper implements ResponseGenerator {
response.setObjectName("network");
return response;
}
@Override
public Long getSecurityGroupId (String groupName, long accountId) {
SecurityGroup sg = ApiDBUtils.getSecurityGroup(groupName, accountId);
if (sg == null) {
return null;
} else {
return sg.getId();
}
}
}

View File

@ -1433,7 +1433,7 @@ public class NetworkManagerImpl implements NetworkManager, NetworkService, Manag
@Override
public void expungeNics(VirtualMachineProfile<? extends VMInstanceVO> vm) {
List<NicVO> nics = _nicDao.listIncludingRemovedBy(vm.getId());
List<NicVO> nics = _nicDao.listByVmIdIncludingRemoved(vm.getId());
for (NicVO nic : nics) {
_nicDao.expunge(nic.getId());
}

View File

@ -34,7 +34,7 @@ public interface SecurityGroupManager {
public SecurityGroupVO createSecurityGroup(String name, String description, Long domainId, Long accountId, String accountName);
public SecurityGroupVO createDefaultSecurityGroup( Long accountId);
public SecurityGroupVO createDefaultSecurityGroup(Long accountId);
public boolean addInstanceToGroups(Long userVmId, List<Long> groups);
@ -47,4 +47,10 @@ public interface SecurityGroupManager {
public List<SecurityGroupVO> getSecurityGroupsForVm(long vmId);
public boolean isVmSecurityGroupEnabled(Long vmId);
SecurityGroup getDefaultSecurityGroup(long accountId);
SecurityGroup getSecurityGroup(String name, long accountId);
boolean isVmMappedToDefaultSecurityGroup(long vmId);
}

View File

@ -1327,4 +1327,30 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG
}
return false;
}
@Override
public SecurityGroupVO getDefaultSecurityGroup(long accountId) {
return _securityGroupDao.findByAccountAndName(accountId, DEFAULT_GROUP_NAME);
}
@Override
public SecurityGroup getSecurityGroup(String name, long accountId) {
return _securityGroupDao.findByAccountAndName(accountId, name);
}
@Override
public boolean isVmMappedToDefaultSecurityGroup(long vmId) {
UserVmVO vm = _userVmMgr.getVirtualMachine(vmId);
SecurityGroup defaultGroup = getDefaultSecurityGroup(vm.getAccountId());
if (defaultGroup == null) {
s_logger.warn("Unable to find default security group for account id=" + vm.getAccountId());
return false;
}
SecurityGroupVMMapVO map = _securityGroupVMMapDao.findByVmIdGroupId(vmId, defaultGroup.getId());
if (map == null) {
return false;
} else {
return true;
}
}
}

View File

@ -121,6 +121,7 @@ import com.cloud.network.dao.NetworkDao;
import com.cloud.network.lb.LoadBalancingRulesManager;
import com.cloud.network.router.VirtualNetworkApplianceManager;
import com.cloud.network.rules.RulesManager;
import com.cloud.network.security.SecurityGroup;
import com.cloud.network.security.SecurityGroupManager;
import com.cloud.network.vpn.PasswordResetElement;
import com.cloud.offering.NetworkOffering;
@ -246,7 +247,7 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager
@Inject protected AccountVlanMapDao _accountVlanMapDao;
@Inject protected StoragePoolDao _storagePoolDao;
@Inject protected VMTemplateHostDao _vmTemplateHostDao;
@Inject protected SecurityGroupManager _networkGroupMgr;
@Inject protected SecurityGroupManager _securityGroupMgr;
@Inject protected ServiceOfferingDao _serviceOfferingDao;
@Inject protected NetworkOfferingDao _networkOfferingDao;
@Inject protected EventDao _eventDao = null;
@ -1158,7 +1159,7 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager
boolean success = true;
//Remove vm from security groups
_networkGroupMgr.removeInstanceFromGroups(vmId);
_securityGroupMgr.removeInstanceFromGroups(vmId);
//Remove vm from instance group
removeInstanceFromInstanceGroup(vmId);
@ -1933,6 +1934,34 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager
networkList.add(defaultNetwork);
}
if (securityGroupIdList == null) {
securityGroupIdList = new ArrayList<Long>();
}
SecurityGroup defaultGroup = _securityGroupMgr.getDefaultSecurityGroup(owner.getId());
if (defaultGroup != null) {
//check if security group id list already contains Default security group, and if not - add it
boolean defaultGroupPresent = false;
for (Long securityGroupId : securityGroupIdList) {
if (securityGroupId.longValue() == defaultGroup.getId()) {
defaultGroupPresent = true;
break;
}
}
if (!defaultGroupPresent) {
securityGroupIdList.add(defaultGroup.getId());
}
} else {
//create default security group for the account
if (s_logger.isDebugEnabled()) {
s_logger.debug("Couldn't find default security group for the account " + owner + " so creating a new one");
}
defaultGroup = _securityGroupMgr.createSecurityGroup(SecurityGroupManager.DEFAULT_GROUP_NAME, SecurityGroupManager.DEFAULT_GROUP_DESCRIPTION, owner.getDomainId(), owner.getId(), owner.getAccountName());
securityGroupIdList.add(defaultGroup.getId());
}
return createVirtualMachine(zone, serviceOffering, template, hostName, displayName, owner, diskOfferingId,
diskSize, networkList, securityGroupIdList, group, userData, sshKeyPair, hypervisor, caller, destinationHost);
}
@ -1946,6 +1975,7 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager
Account caller = UserContext.current().getCaller();
List<NetworkVO> networkList = new ArrayList<NetworkVO>();
boolean isSecurityGroupEnabledNetworkUsed = false;
//Verify that caller can perform actions in behalf of vm owner
_accountMgr.checkAccess(caller, owner);
@ -1976,6 +2006,7 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager
}
networkList.add(network);
isSecurityGroupEnabledNetworkUsed = true;
} else {
//Verify that all the networks are Direct/Guest/AccountSpecific; can't create combination of SG enabled network and regular networks
@ -2007,6 +2038,37 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager
}
}
// if network is security group enabled, and default security group is not present in the list of groups specified, add it automatically
if (isSecurityGroupEnabledNetworkUsed) {
if (securityGroupIdList == null) {
securityGroupIdList = new ArrayList<Long>();
}
SecurityGroup defaultGroup = _securityGroupMgr.getDefaultSecurityGroup(owner.getId());
if (defaultGroup != null) {
//check if security group id list already contains Default security group, and if not - add it
boolean defaultGroupPresent = false;
for (Long securityGroupId : securityGroupIdList) {
if (securityGroupId.longValue() == defaultGroup.getId()) {
defaultGroupPresent = true;
break;
}
}
if (!defaultGroupPresent) {
securityGroupIdList.add(defaultGroup.getId());
}
} else {
//create default security group for the account
if (s_logger.isDebugEnabled()) {
s_logger.debug("Couldn't find default security group for the account " + owner + " so creating a new one");
}
defaultGroup = _securityGroupMgr.createSecurityGroup(SecurityGroupManager.DEFAULT_GROUP_NAME, SecurityGroupManager.DEFAULT_GROUP_DESCRIPTION, owner.getDomainId(), owner.getId(), owner.getAccountName());
securityGroupIdList.add(defaultGroup.getId());
}
}
return createVirtualMachine(zone, serviceOffering, template, hostName, displayName, owner, diskOfferingId,
diskSize, networkList, securityGroupIdList, group, userData, sshKeyPair, hypervisor, caller, destinationHost);
}
@ -2347,7 +2409,7 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager
throw new CloudRuntimeException("Unable to assign Vm to the group " + group);
}
_networkGroupMgr.addInstanceToGroups(vm.getId(), securityGroupIdList);
_securityGroupMgr.addInstanceToGroups(vm.getId(), securityGroupIdList);
return vm;
}
@ -2588,6 +2650,22 @@ public class UserVmManagerImpl implements UserVmManager, UserVmService, Manager
userId = accountAndUserValidation(vmId, account, userId, vm);
UserVO user = _userDao.findById(userId);
//check if vm is security group enabled
if (_securityGroupMgr.isVmSecurityGroupEnabled(vmId) && !_securityGroupMgr.isVmMappedToDefaultSecurityGroup(vmId)) {
//if vm is not mapped to security group, create a mapping
if (s_logger.isDebugEnabled()) {
s_logger.debug("Vm " + vm + " is security group enabled, but not mapped to default security group; creating the mapping automatically");
}
SecurityGroup defaultSecurityGroup = _securityGroupMgr.getDefaultSecurityGroup(vm.getAccountId());
if (defaultSecurityGroup != null) {
List<Long> groupList = new ArrayList<Long>();
groupList.add(defaultSecurityGroup.getId());
_securityGroupMgr.addInstanceToGroups(vmId, groupList);
}
}
return _itMgr.start(vm, null, user, account);
}

View File

@ -12,10 +12,8 @@ import com.cloud.vm.VirtualMachine;
public interface NicDao extends GenericDao<NicVO, Long> {
List<NicVO> listByVmId(long instanceId);
List<NicVO> listByVmIdIncludingRemoved(long instanceId);
List<String> listIpAddressInNetwork(long networkConfigId);
List<NicVO> listIncludingRemovedBy(long instanceId);
List<NicVO> listByVmIdIncludingRemoved(long instanceId);
List<NicVO> listByNetworkId(long networkId);

View File

@ -59,13 +59,6 @@ public class NicDaoImpl extends GenericDaoBase<NicVO, Long> implements NicDao {
}
@Override
public List<NicVO> listIncludingRemovedBy(long instanceId) {
SearchCriteria<NicVO> sc = AllFieldsSearch.create();
sc.setParameters("instance", instanceId);
return listIncludingRemovedBy(sc);
}
@Override
public List<String> listIpAddressInNetwork(long networkId) {
SearchCriteria<String> sc = IpSearch.create();