From 31c9cce6c38f7f62a65362e9667a52f414e444cd Mon Sep 17 00:00:00 2001 From: abhishek Date: Thu, 20 Jan 2011 14:21:03 -0800 Subject: [PATCH] bug 7949: changing security group code to use ids instead of name status 7949: resolved fixed --- api/src/com/cloud/api/ApiConstants.java | 1 + .../AuthorizeSecurityGroupIngressCmd.java | 11 +-- .../api/commands/DeleteSecurityGroupCmd.java | 10 +-- .../RevokeSecurityGroupIngressCmd.java | 11 +-- .../security/SecurityGroupManagerImpl.java | 70 +++++++++---------- 5 files changed, 54 insertions(+), 49 deletions(-) diff --git a/api/src/com/cloud/api/ApiConstants.java b/api/src/com/cloud/api/ApiConstants.java index 7b9f96a1bcf..ebded15a1bf 100755 --- a/api/src/com/cloud/api/ApiConstants.java +++ b/api/src/com/cloud/api/ApiConstants.java @@ -129,6 +129,7 @@ public class ApiConstants { public static final String SECRET_KEY = "secretkey"; public static final String SECURITY_GROUP_LIST = "securitygrouplist"; public static final String SECURITY_GROUP_NAME = "securitygroupname"; + public static final String SECURITY_GROUP_ID = "securitygroupid"; public static final String SENT = "sent"; public static final String SENT_BYTES = "sentbytes"; public static final String SERVICE_OFFERING_ID = "serviceofferingid"; diff --git a/api/src/com/cloud/api/commands/AuthorizeSecurityGroupIngressCmd.java b/api/src/com/cloud/api/commands/AuthorizeSecurityGroupIngressCmd.java index 1dc774b402b..17ff761afb9 100644 --- a/api/src/com/cloud/api/commands/AuthorizeSecurityGroupIngressCmd.java +++ b/api/src/com/cloud/api/commands/AuthorizeSecurityGroupIngressCmd.java @@ -32,6 +32,7 @@ import com.cloud.api.BaseCmd; import com.cloud.api.Implementation; import com.cloud.api.Parameter; import com.cloud.api.ServerApiException; +import com.cloud.api.BaseCmd.CommandType; import com.cloud.api.response.IngressRuleResponse; import com.cloud.api.response.SecurityGroupResponse; import com.cloud.event.EventTypes; @@ -65,8 +66,8 @@ 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_NAME, type=CommandType.STRING, required=true, description="the security group name") - private String securityGroupName; + @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; @@ -109,8 +110,8 @@ public class AuthorizeSecurityGroupIngressCmd extends BaseAsyncCmd { return icmpType; } - public String getSecurityGroupName() { - return securityGroupName; + public Long getSecurityGroupId() { + return securityGroupId; } public String getProtocol() { @@ -192,7 +193,7 @@ public class AuthorizeSecurityGroupIngressCmd extends BaseAsyncCmd { sb.append(""); } - return "authorizing ingress to group: " + getSecurityGroupName() + " to " + sb.toString(); + return "authorizing ingress to group: " + getSecurityGroupId() + " to " + sb.toString(); } @Override diff --git a/api/src/com/cloud/api/commands/DeleteSecurityGroupCmd.java b/api/src/com/cloud/api/commands/DeleteSecurityGroupCmd.java index 46ca04a800d..515cabd1b11 100644 --- a/api/src/com/cloud/api/commands/DeleteSecurityGroupCmd.java +++ b/api/src/com/cloud/api/commands/DeleteSecurityGroupCmd.java @@ -7,6 +7,7 @@ import com.cloud.api.BaseCmd; import com.cloud.api.Implementation; import com.cloud.api.Parameter; import com.cloud.api.ServerApiException; +import com.cloud.api.BaseCmd.CommandType; import com.cloud.api.response.SuccessResponse; import com.cloud.exception.ResourceInUseException; @@ -25,8 +26,8 @@ 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.NAME, type=CommandType.STRING, required=true, description="the security group name") - private String securityGroupName; + @Parameter(name=ApiConstants.ID, type=CommandType.LONG, required=true, description="The ID of the security group") + private Long id; ///////////////////////////////////////////////////// @@ -41,11 +42,12 @@ public class DeleteSecurityGroupCmd extends BaseCmd { return domainId; } - public String getSecurityGroupName() { - return securityGroupName; + public Long getId() { + return id; } + ///////////////////////////////////////////////////// /////////////// API Implementation/////////////////// ///////////////////////////////////////////////////// diff --git a/api/src/com/cloud/api/commands/RevokeSecurityGroupIngressCmd.java b/api/src/com/cloud/api/commands/RevokeSecurityGroupIngressCmd.java index 916a72153f1..75032d2ac12 100644 --- a/api/src/com/cloud/api/commands/RevokeSecurityGroupIngressCmd.java +++ b/api/src/com/cloud/api/commands/RevokeSecurityGroupIngressCmd.java @@ -13,6 +13,7 @@ import com.cloud.api.BaseCmd; import com.cloud.api.Implementation; import com.cloud.api.Parameter; import com.cloud.api.ServerApiException; +import com.cloud.api.BaseCmd.CommandType; import com.cloud.api.response.SuccessResponse; import com.cloud.event.EventTypes; import com.cloud.user.Account; @@ -47,8 +48,8 @@ public class RevokeSecurityGroupIngressCmd extends BaseAsyncCmd { @Parameter(name=ApiConstants.ICMP_TYPE, type=CommandType.INTEGER, description="type for this icmp message") private Integer icmpType; - @Parameter(name=ApiConstants.SECURITY_GROUP_NAME, type=CommandType.STRING, required=true, description="name of the security group") - private String securityGroupName; + @Parameter(name=ApiConstants.SECURITY_GROUP_ID, type=CommandType.LONG, required=true, description="The ID of the security group") + private Long securityGroupId; @Parameter(name=ApiConstants.PROTOCOL, type=CommandType.STRING, description="protocol used") private String protocol; @@ -87,8 +88,8 @@ public class RevokeSecurityGroupIngressCmd extends BaseAsyncCmd { return icmpType; } - public String getSecurityGroupName() { - return securityGroupName; + public Long getSecurityGroupId() { + return securityGroupId; } public String getProtocol() { @@ -165,7 +166,7 @@ public class RevokeSecurityGroupIngressCmd extends BaseAsyncCmd { sb.append(""); } - return "revoking ingress from group: " + getSecurityGroupName() + " for " + sb.toString(); + return "revoking ingress from group: " + getSecurityGroupId() + " for " + sb.toString(); } @Override diff --git a/server/src/com/cloud/network/security/SecurityGroupManagerImpl.java b/server/src/com/cloud/network/security/SecurityGroupManagerImpl.java index a3d13946595..601043e8091 100644 --- a/server/src/com/cloud/network/security/SecurityGroupManagerImpl.java +++ b/server/src/com/cloud/network/security/SecurityGroupManagerImpl.java @@ -434,7 +434,7 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG @Override @DB @SuppressWarnings("rawtypes") public List authorizeSecurityGroupIngress(AuthorizeSecurityGroupIngressCmd cmd) throws InvalidParameterValueException, PermissionDeniedException{ - String groupName = cmd.getSecurityGroupName(); + Long groupId = cmd.getSecurityGroupId(); String protocol = cmd.getProtocol(); Integer startPort = cmd.getStartPort(); Integer endPort = cmd.getEndPort(); @@ -510,9 +510,9 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG // if it's an admin account, do a quick permission check if ((account != null) && !_domainDao.isChildDomain(account.getDomainId(), domainId)) { if (s_logger.isDebugEnabled()) { - s_logger.debug("Unable to find rules for security group id = " + groupName + ", permission denied."); + s_logger.debug("Unable to find rules for security group id = " + groupId + ", permission denied."); } - throw new PermissionDeniedException("Unable to find rules for security group id = " + groupName + ", permission denied."); + throw new PermissionDeniedException("Unable to find rules for security group id = " + groupId + ", permission denied."); } Account groupOwner = _accountDao.findActiveAccount(accountName, domainId); @@ -534,7 +534,7 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG } if (accountId == null) { - throw new InvalidParameterValueException("Unable to find account for security group " + groupName + "; failed to authorize ingress."); + throw new InvalidParameterValueException("Unable to find account for security group " + groupId + "; failed to authorize ingress."); } @@ -560,9 +560,9 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG Account authorizedAccount = _accountDao.findActiveAccount(authorizedAccountName, domainId); if (authorizedAccount == null) { if (s_logger.isDebugEnabled()) { - s_logger.debug("Nonexistent account: " + authorizedAccountName + ", domainid: " + domainId + " when trying to authorize ingress for " + groupName + ":" + protocol + ":" + startPortOrType + ":" + endPortOrCode); + s_logger.debug("Nonexistent account: " + authorizedAccountName + ", domainid: " + domainId + " when trying to authorize ingress for " + groupId + ":" + protocol + ":" + startPortOrType + ":" + endPortOrCode); } - throw new InvalidParameterValueException("Nonexistent account: " + authorizedAccountName + " when trying to authorize ingress for " + groupName + ":" + protocol + ":" + startPortOrType + ":" + endPortOrCode); + throw new InvalidParameterValueException("Nonexistent account: " + authorizedAccountName + " when trying to authorize ingress for " + groupId + ":" + protocol + ":" + startPortOrType + ":" + endPortOrCode); } SecurityGroupVO groupVO = _securityGroupDao.findByAccountAndName(authorizedAccount.getId(), group); @@ -581,15 +581,15 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG authorizedGroups2.addAll(authorizedGroups); //Ensure we don't re-lock the same row txn.start(); - SecurityGroupVO securityGroup = _securityGroupDao.findByAccountAndName(accountId, groupName); + SecurityGroupVO securityGroup = _securityGroupDao.findById(groupId); if (securityGroup == null) { - s_logger.warn("Security group not found: name= " + groupName); + s_logger.warn("Security group not found: id= " + groupId); return null; } //Prevents other threads/management servers from creating duplicate ingress rules SecurityGroupVO securityGroupLock = _securityGroupDao.acquireInLockTable(securityGroup.getId()); if (securityGroupLock == null) { - s_logger.warn("Could not acquire lock on network security group: name= " + groupName); + s_logger.warn("Could not acquire lock on network security group: name= " + securityGroup.getName()); return null; } List newRules = new ArrayList(); @@ -597,7 +597,7 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG //Don't delete the group from under us. securityGroup = _securityGroupDao.lockRow(securityGroup.getId(), false); if (securityGroup == null) { - s_logger.warn("Could not acquire lock on network group " + groupName); + s_logger.warn("Could not acquire lock on network group " + securityGroup.getName()); return null; } @@ -632,7 +632,7 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG } } if (s_logger.isDebugEnabled()) { - s_logger.debug("Added " + newRules.size() + " rules to security group " + groupName); + s_logger.debug("Added " + newRules.size() + " rules to security group " + securityGroup.getName()); } txn.commit(); final Set affectedVms = new HashSet(); @@ -663,7 +663,7 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG Integer icmpType = cmd.getIcmpType(); Integer icmpCode = cmd.getIcmpCode(); String protocol = cmd.getProtocol(); - String securityGroup = cmd.getSecurityGroupName(); + Long securityGroupId = cmd.getSecurityGroupId(); String cidrList = cmd.getCidrList(); Map groupList = cmd.getUserSecurityGroupList(); String [] cidrs = null; @@ -724,9 +724,9 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG // if it's an admin account, do a quick permission check if ((account != null) && !_domainDao.isChildDomain(account.getDomainId(), domainId)) { if (s_logger.isDebugEnabled()) { - s_logger.debug("Unable to find rules for network security group id = " + securityGroup + ", permission denied."); + s_logger.debug("Unable to find rules for security group id = " + securityGroupId + ", permission denied."); } - throw new ServerApiException(BaseCmd.ACCOUNT_ERROR, "Unable to find rules for network security group id = " + securityGroup + ", permission denied."); + throw new ServerApiException(BaseCmd.ACCOUNT_ERROR, "Unable to find rules for security group id = " + securityGroupId + ", permission denied."); } Account groupOwner = _accountDao.findActiveAccount(accountName, domainId); if (groupOwner == null) { @@ -747,13 +747,13 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG } if (accountId == null) { - throw new ServerApiException(BaseCmd.PARAM_ERROR, "Unable to find account for network security group " + securityGroup + "; failed to revoke ingress."); + throw new ServerApiException(BaseCmd.PARAM_ERROR, "Unable to find account for security group " + securityGroupId + "; failed to revoke ingress."); } - SecurityGroupVO sg = _securityGroupDao.findByAccountAndName(accountId, securityGroup); + SecurityGroupVO sg = _securityGroupDao.findById(securityGroupId); if (sg == null) { - s_logger.debug("Unable to find network security group with id " + securityGroup); - throw new ServerApiException(BaseCmd.PARAM_ERROR, "Unable to find network security group with id " + securityGroup); + s_logger.debug("Unable to find security group with id " + securityGroupId); + throw new ServerApiException(BaseCmd.PARAM_ERROR, "Unable to find security group with id " + securityGroupId); } if (cidrList == null && groupList == null) { @@ -790,9 +790,9 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG Account authorizedAccount = _accountDao.findActiveAccount(authorizedAccountName, domainId); if (authorizedAccount == null) { if (s_logger.isDebugEnabled()) { - s_logger.debug("Nonexistent account: " + authorizedAccountName + ", domainid: " + domainId + " when trying to revoke ingress for " + securityGroup + ":" + protocol + ":" + startPortOrType + ":" + endPortOrCode); + s_logger.debug("Nonexistent account: " + authorizedAccountName + ", domainid: " + domainId + " when trying to revoke ingress for " + securityGroupId + ":" + protocol + ":" + startPortOrType + ":" + endPortOrCode); } - throw new ServerApiException(BaseCmd.PARAM_ERROR, "Nonexistent account: " + authorizedAccountName + " when trying to revoke ingress for " + securityGroup + ":" + protocol + ":" + startPortOrType + ":" + endPortOrCode); + throw new ServerApiException(BaseCmd.PARAM_ERROR, "Nonexistent account: " + authorizedAccountName + " when trying to revoke ingress for " + securityGroupId + ":" + protocol + ":" + startPortOrType + ":" + endPortOrCode); } SecurityGroupVO groupVO = _securityGroupDao.findByAccountAndName(authorizedAccount.getId(), group); @@ -818,9 +818,9 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG final int numToDelete = cidrList.length() + authorizedGroups.size(); final Transaction txn = Transaction.currentTxn(); - SecurityGroupVO securityGroupHandle = _securityGroupDao.findByAccountAndName(accountId, securityGroup); + SecurityGroupVO securityGroupHandle = _securityGroupDao.findById(securityGroupId); if (securityGroupHandle == null) { - s_logger.warn("Network security group not found: name= " + securityGroup); + s_logger.warn("Security group not found: name= " + securityGroupId); return false; } try { @@ -828,7 +828,7 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG securityGroupHandle = _securityGroupDao.acquireInLockTable(securityGroupHandle.getId()); if (securityGroupHandle == null) { - s_logger.warn("Could not acquire lock on network security group: name= " + securityGroup); + s_logger.warn("Could not acquire lock on network security group: name= " + securityGroupId); return false; } for (final SecurityGroupVO ngVO: authorizedGroups) { @@ -837,7 +837,7 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG for (final String cidr: cidrs) { numDeleted += _ingressRuleDao.deleteByPortProtoAndCidr(securityGroupHandle.getId(), protocol, startPort, endPort, cidr); } - s_logger.debug("revokeSecurityGroupIngress for group: " + securityGroup + ", numToDelete=" + numToDelete + ", numDeleted=" + numDeleted); + s_logger.debug("revokeSecurityGroupIngress for group: " + securityGroupId + ", numToDelete=" + numToDelete + ", numDeleted=" + numDeleted); final Set affectedVms = new HashSet(); affectedVms.addAll(_securityGroupVMMapDao.listVmIdsBySecurityGroup(securityGroupHandle.getId())); @@ -848,7 +848,7 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG s_logger.warn("Exception caught when deleting ingress rules ", e); throw new CloudRuntimeException("Exception caught when deleting ingress rules", e); } finally { - if (securityGroup != null) { + if (securityGroupId != null) { _securityGroupDao.releaseFromLockTable(securityGroupHandle.getId()); } txn.commit(); @@ -1129,7 +1129,7 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG @DB @Override public boolean deleteSecurityGroup(DeleteSecurityGroupCmd cmd) throws ResourceInUseException, PermissionDeniedException, InvalidParameterValueException{ - String name = cmd.getSecurityGroupName(); + Long id = cmd.getId(); String accountName = cmd.getAccountName(); Long domainId = cmd.getDomainId(); Account account = UserContext.current().getCaller(); @@ -1145,9 +1145,9 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG // if it's an admin account, do a quick permission check if ((account != null) && !_domainDao.isChildDomain(account.getDomainId(), domainId)) { if (s_logger.isDebugEnabled()) { - s_logger.debug("Unable to find rules network group " + name + ", permission denied."); + s_logger.debug("Unable to find rules network group: " + id + ", permission denied."); } - throw new PermissionDeniedException("Unable to network group " + name + ", permission denied."); + throw new PermissionDeniedException("Unable to network group: " + id + ", permission denied."); } Account groupOwner = _accountDao.findActiveAccount(accountName, domainId); @@ -1169,12 +1169,12 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG } if (accountId == null) { - throw new InvalidParameterValueException("Unable to find account for network group " + name + "; failed to delete group."); + throw new InvalidParameterValueException("Unable to find account for network group: " + id + "; failed to delete group."); } - SecurityGroupVO sg = _securityGroupDao.findByAccountAndName(accountId, name); + SecurityGroupVO sg = _securityGroupDao.findById(id); if (sg == null) { - throw new ServerApiException(BaseCmd.PARAM_ERROR, "Unable to find network group " + name + "; failed to delete group."); + throw new ServerApiException(BaseCmd.PARAM_ERROR, "Unable to find network group: " + id + "; failed to delete group."); } Long groupId = sg.getId(); @@ -1216,7 +1216,7 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG String accountName = cmd.getAccountName(); Long accountId = null; Long instanceId = cmd.getVirtualMachineId(); - String networkGroup = cmd.getSecurityGroupName(); + String securityGroup = cmd.getSecurityGroupName(); Boolean recursive = Boolean.FALSE; Long id = cmd.getId(); @@ -1244,7 +1244,7 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG } } else if (account != null) { // either an admin is searching for their own group, or admin is listing all groups and the search needs to be restricted to domain admin's domain - if (networkGroup != null) { + if (securityGroup != null) { accountId = account.getId(); } else if (account.getType() != Account.ACCOUNT_TYPE_ADMIN) { domainId = account.getDomainId(); @@ -1297,8 +1297,8 @@ public class SecurityGroupManagerImpl implements SecurityGroupManager, SecurityG SearchCriteria sc = sb.create(); if (accountId != null) { sc.setParameters("accountId", accountId); - if (networkGroup != null) { - sc.setParameters("name", networkGroup); + if (securityGroup != null) { + sc.setParameters("name", securityGroup); } else if (keyword != null) { SearchCriteria ssc = _securityGroupRulesDao.createSearchCriteria(); ssc.addOr("name", SearchCriteria.Op.LIKE, "%" + keyword + "%");