Skip to content

Commit

Permalink
Merge branch 'ASGARD-923-only-one-task-per-security-group-change'
Browse files Browse the repository at this point in the history 
* ASGARD-923-only-one-task-per-security-group-change:
  Remove unused import
  ASGARD-923 - Only one auth-revoke task with many IP permissions for each security group pair change
  Unit tests for portString() method
  Better GroovyDoc comments
  Make ‘tcp’ a static variable for re-use
  Remove unused methods
  Fix small edge case bug on lone()
  Shorten a few lines longer than 120 chars
  Include zebra striping on page load before JavaScript

Conflicts:
	test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy
  • Loading branch information
Joe Sondow committed Jan 3, 2014
2 parents e78efea + 0aa14c9 commit 0d00562
Show file tree
Hide file tree
Showing 6 changed files with 143 additions and 147 deletions.
185 changes: 105 additions & 80 deletions grails-app/services/com/netflix/asgard/AwsEc2Service.groovy
View file
Original file line number Diff line number Diff line change
Expand Up @@ -86,13 +86,11 @@ import com.amazonaws.services.ec2.model.Vpc
import com.google.common.collect.HashMultiset
import com.google.common.collect.Lists
import com.google.common.collect.Multiset
import com.google.common.collect.TreeMultiset
import com.netflix.asgard.cache.CacheInitializer
import com.netflix.asgard.model.AutoScalingGroupData
import com.netflix.asgard.model.SecurityGroupOption
import com.netflix.asgard.model.Subnets
import com.netflix.asgard.model.ZoneAvailability
import com.netflix.frigga.ami.AppVersion
import groovyx.gpars.GParsExecutorsPool
import java.util.regex.Matcher
import java.util.regex.Pattern
Expand All @@ -105,6 +103,7 @@ class AwsEc2Service implements CacheInitializer, InitializingBean {
static transactional = false

private static Pattern SECURITY_GROUP_ID_PATTERN = ~/sg-[a-f0-9]+/
private static final String IP_PROTOCOL = 'tcp'

MultiRegionAwsClient<AmazonEC2> awsClient
def awsClientService
Expand Down Expand Up @@ -458,8 +457,8 @@ class AwsEc2Service implements CacheInitializer, InitializingBean {
if (name ==~ SECURITY_GROUP_ID_PATTERN) {
groupId = name
request.withGroupIds(groupId)
SecurityGroup cachedSecurityGroup = caches.allSecurityGroups.by(region).list().find { it.groupId == groupId }
groupName = cachedSecurityGroup?.groupName
SecurityGroup cachedSecGroup = caches.allSecurityGroups.by(region).list().find { it.groupId == groupId }
groupName = cachedSecGroup?.groupName
} else {
request.withGroupNames(name)
groupName = name
Expand All @@ -470,7 +469,7 @@ class AwsEc2Service implements CacheInitializer, InitializingBean {
SecurityGroup group = null
try {
DescribeSecurityGroupsResult result = awsClient.by(region).describeSecurityGroups(request)
group = Check.lone(result?.getSecurityGroups(), SecurityGroup)
group = Check.loneOrNone(result.getSecurityGroups(), SecurityGroup)
groupName = group?.groupName
} catch (AmazonServiceException e) {
// Can't find a security group with that request.
Expand Down Expand Up @@ -557,32 +556,71 @@ class AwsEc2Service implements CacheInitializer, InitializingBean {
caches.allSecurityGroups.by(userContext.region).remove(name)
}

/** High-level permission update for a group pair: given the desired state, make it so. */
/**
* High-level permission update for a group pair: given the desired state, make it so. Ensure that all the requested
* permissions are authorized for the source to call the target, and ensure that all other permissions from the
* source to the target are revoked.
*
* @param userContext who, where, why
* @param targetGroup the security group of the instances that will be allowed to receive traffic
* @param sourceGroup the security group of the instances that will be allowed to send traffic
* @param desired the IP permissions that should be entirely and solely in effect when this method completes
*/
void updateSecurityGroupPermissions(UserContext userContext, SecurityGroup targetGroup, SecurityGroup sourceGroup,
List<IpPermission> wantPerms) {
List<IpPermission> havePerms = getIngressFrom(targetGroup, sourceGroup)
if (!havePerms && !wantPerms) {
List<IpPermission> desired) {
List<IpPermission> current = getIngressFrom(targetGroup, sourceGroup)
if (!current && !desired) {
return
}
Boolean somethingChanged = false
havePerms.each { havePerm ->
if (!wantPerms.any { wp -> wp.fromPort == havePerm.fromPort && wp.toPort == havePerm.toPort } ) {
revokeSecurityGroupIngress(userContext, targetGroup, sourceGroup, 'tcp',
havePerm.fromPort, havePerm.toPort)
somethingChanged = true
}
String sourceName = sourceGroup.groupName
String targetName = targetGroup.groupName
String targetGroupId = targetGroup.groupId
String userId = configService.awsAccountNumber
UserIdGroupPair srcGroupId = new UserIdGroupPair(userId: userId, groupId: sourceGroup.groupId)
List<IpPermission> toRevoke = determinePermissionsToChange(current, desired, srcGroupId)
List<IpPermission> toAuthorize = determinePermissionsToChange(desired, current, srcGroupId)
if (toRevoke || toAuthorize) {
String msg = "Update Security Group Ingress between source '${sourceName}' and target '${targetName}'"
taskService.runTask(userContext, msg, { Task task ->
authorizePermissions(userContext, sourceGroup, targetGroup, toAuthorize, task)
revokePermissions(userContext, sourceGroup, targetGroup, toRevoke, task)
getSecurityGroup(userContext, targetGroupId)
}, Link.to(EntityType.security, targetGroupId))
}
wantPerms.each { wantPerm ->
if (!havePerms.any { hp -> hp.fromPort == wantPerm.fromPort && hp.toPort == wantPerm.toPort } ) {
authorizeSecurityGroupIngress(userContext, targetGroup, sourceGroup, 'tcp',
wantPerm.fromPort, wantPerm.toPort)
somethingChanged = true
}

/**
* For a set of IP permissions for traffic from instances in a single specified security group, this method
* determines the IP permissions that need to change based on which port ranges are currently in place and which
* port ranges are desired.<p>
*
* If the first set of IP permissions is the current set, and the second set is the desired set, then this method
* returns the IP permissions that need to be revoked. Conversely, if the first set if the desired set and the
* second set is the current, then this method returns the IP permissions that need to be authorized.
*
* @param thesePermissions the current permissions (to determine what to revoke) or the desired permissions (to
* determine what to authorize)
* @param otherPermissions the desired permissions (to determine what to revoke) or the current permissions (to
* determine what to authorize)
* @param userIdGroupPair an AWS account number, and the id of a security group within that account, together
* signifying the coordinates of a globally unique security group, which is the security group of the
* instances that will be sending traffic
* @return the permissions to authorize (if called with current permissions first) or the permissions to revoke
* (if called with desired permissions first)
*/
private List<IpPermission> determinePermissionsToChange(List<IpPermission> thesePermissions,
List<IpPermission> otherPermissions, UserIdGroupPair userIdGroupPair) {

List<IpPermission> permissionsToChange = []
for (IpPermission permission in thesePermissions) {
int fromPort = permission.fromPort
int toPort = permission.toPort
if (!otherPermissions.any { otherPerm -> otherPerm.fromPort == fromPort && otherPerm.toPort == toPort }) {
permissionsToChange << new IpPermission(userIdGroupPairs: [userIdGroupPair], ipProtocol: IP_PROTOCOL,
fromPort: fromPort, toPort: toPort)
}
}
// This method gets called hundreds of times for one user request so don't call Amazon unless necessary.
if (somethingChanged) {
getSecurityGroup(userContext, targetGroup.groupId)
}
permissionsToChange
}

/** Converts a list of IpPermissions into a string representation, or null if none. */
Expand All @@ -597,12 +635,47 @@ class AwsEc2Service implements CacheInitializer, InitializingBean {
}
}

/** Returns the canonical string representation of a from-to port pair. */
private void authorizePermissions(UserContext userContext, SecurityGroup source, SecurityGroup target,
List<IpPermission> permissionsToAuth, Task task) {
if (permissionsToAuth) {
String ports = permissionsToString(permissionsToAuth)
task.log("Authorize Security Group Ingress from '${source.groupName}' to '${target.groupName}' on ${ports}")
AuthorizeSecurityGroupIngressRequest request = new AuthorizeSecurityGroupIngressRequest()
request.withGroupId(target.groupId).withIpPermissions(permissionsToAuth)
awsClient.by(userContext.region).authorizeSecurityGroupIngress(request)
}
}

private void revokePermissions(UserContext userContext, SecurityGroup source, SecurityGroup target,
List<IpPermission> permissionsToRevoke, Task task) {
if (permissionsToRevoke) {
String ports = permissionsToString(permissionsToRevoke)
task.log("Revoke Security Group Ingress from '${source.groupName}' to '${target.groupName}' on ${ports}")
RevokeSecurityGroupIngressRequest request = new RevokeSecurityGroupIngressRequest()
request.withGroupId(target.groupId).withIpPermissions(permissionsToRevoke)
awsClient.by(userContext.region).revokeSecurityGroupIngress(request)
}
}

/**
* Returns the canonical string representation of a from-to port pair. If the two numbers are the same, this method
* returns the number as string. Otherwise, this method returns a string with the fromPort separated from the toPort
* by a hyphen like "7101-7102".
*
* @param fromPort the low end of a port range
* @param toPort the high end of a port range
* @return string representation of the port range
*/
public static String portString(int fromPort, int toPort) {
toPort == fromPort ? "${fromPort}" : "${fromPort}-${toPort}"
}

/** Converts a string ports representation into a list of partially populated IpPermission instances. */
/**
* Converts a string ports representation into a list of partially populated IpPermission instances.
*
* @param portsStr the string representation of a set of comma-separated port ranges, such as "7001,7101-7102"
* @return the IpPermission objects populated only with fromPort and toPort integer values
*/
static List<IpPermission> permissionsFromString(String portsStr) {
List<IpPermission> perms = []
if (portsStr) {
Expand Down Expand Up @@ -630,7 +703,7 @@ class AwsEc2Service implements CacheInitializer, InitializingBean {
private String bestIngressPortsFor(SecurityGroup targetGroup) {
Map guess = ['7001' : 1]
targetGroup.ipPermissions.each {
if (it.ipProtocol == 'tcp' && it.userIdGroupPairs.size() > 0) {
if (it.ipProtocol == IP_PROTOCOL && it.userIdGroupPairs.size() > 0) {
Integer count = it.userIdGroupPairs.size()
String portRange = portString(it.fromPort, it.toPort)
guess[portRange] = guess[portRange] ? guess[portRange] + count : count
Expand All @@ -641,38 +714,6 @@ class AwsEc2Service implements CacheInitializer, InitializingBean {
g
}

// TODO refactor the following two methods to take IpPermissions List from callers now that AWS API takes those.

private void authorizeSecurityGroupIngress(UserContext userContext, SecurityGroup targetgroup, SecurityGroup sourceGroup, String ipProtocol, int fromPort, int toPort) {
String groupName = targetgroup.groupName
String sourceGroupName = sourceGroup.groupName
UserIdGroupPair sourcePair = new UserIdGroupPair().withUserId(accounts[0]).withGroupId(sourceGroup.groupId)
List<IpPermission> perms = [
new IpPermission()
.withUserIdGroupPairs(sourcePair)
.withIpProtocol(ipProtocol).withFromPort(fromPort).withToPort(toPort)
]
taskService.runTask(userContext, "Authorize Security Group Ingress to ${groupName} from ${sourceGroupName} on ${fromPort}-${toPort}", { task ->
awsClient.by(userContext.region).authorizeSecurityGroupIngress(
new AuthorizeSecurityGroupIngressRequest().withGroupId(targetgroup.groupId).withIpPermissions(perms))
}, Link.to(EntityType.security, groupName))
}

private void revokeSecurityGroupIngress(UserContext userContext, SecurityGroup targetgroup, SecurityGroup sourceGroup, String ipProtocol, int fromPort, int toPort) {
String groupName = targetgroup.groupName
String sourceGroupName = sourceGroup.groupName
UserIdGroupPair sourcePair = new UserIdGroupPair().withUserId(accounts[0]).withGroupId(sourceGroup.groupId)
List<IpPermission> perms = [
new IpPermission()
.withUserIdGroupPairs(sourcePair)
.withIpProtocol(ipProtocol).withFromPort(fromPort).withToPort(toPort)
]
taskService.runTask(userContext, "Revoke Security Group Ingress to ${groupName} from ${sourceGroupName} on ${fromPort}-${toPort}", { task ->
awsClient.by(userContext.region).revokeSecurityGroupIngress(
new RevokeSecurityGroupIngressRequest().withGroupId(targetgroup.groupId).withIpPermissions(perms))
}, Link.to(EntityType.security, groupName))
}

// TODO: Delete this method after rewriting AwsResultsRetrieverSpec unit test to use some other use case
DescribeSpotPriceHistoryResult describeSpotPriceHistory(Region region,
DescribeSpotPriceHistoryRequest describeSpotPriceHistoryRequest) {
Expand Down Expand Up @@ -779,29 +820,12 @@ class AwsEc2Service implements CacheInitializer, InitializingBean {
instances ? instances[0] : null
}

Multiset<AppVersion> getCountedAppVersions(UserContext userContext) {
Map<String, Image> imageIdsToImages = caches.allImages.by(userContext.region).unmodifiable()
getCountedAppVersionsForInstancesAndImages(getInstances(userContext), imageIdsToImages)
}

private Multiset<AppVersion> getCountedAppVersionsForInstancesAndImages(Collection<Instance> instances,
Map<String, Image> images) {
Multiset<AppVersion> appVersions = TreeMultiset.create()
instances.each { Instance instance ->
Image image = images.get(instance.imageId)
AppVersion appVersion = image?.parsedAppVersion
if (appVersion) {
appVersions.add(appVersion)
}
}
appVersions
}

Reservation getInstanceReservation(UserContext userContext, String instanceId) {
if (!instanceId) { return null }
def result
try {
result = awsClient.by(userContext.region).describeInstances(new DescribeInstancesRequest().withInstanceIds(instanceId))
DescribeInstancesRequest request = new DescribeInstancesRequest(instanceId: instanceId)
result = awsClient.by(userContext.region).describeInstances(request)
}
catch (AmazonServiceException ase) {
log.info "Request for instance ${instanceId} failed because ${ase}"
Expand Down Expand Up @@ -914,7 +938,8 @@ class AwsEc2Service implements CacheInitializer, InitializingBean {

void associateAddress(UserContext userContext, String publicIp, String instanceId) {
taskService.runTask(userContext, "Associate ${publicIp} with ${instanceId}", { task ->
awsClient.by(userContext.region).associateAddress(new AssociateAddressRequest().withPublicIp(publicIp).withInstanceId(instanceId))
AssociateAddressRequest request = new AssociateAddressRequest(publicIp: publicIp, instanceId: instanceId)
awsClient.by(userContext.region).associateAddress(request)
}, Link.to(EntityType.instance, instanceId))
}

Expand Down
4 changes: 2 additions & 2 deletions grails-app/views/application/security.gsp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -60,8 +60,8 @@
</tr>
</thead>
<tbody>
<g:each var="g" in="${groups}">
<tr>
<g:each var="g" in="${groups}" status="i">
<tr class="${(i % 2) == 0 ? 'odd' : 'even'}">
<td class="checkbox"><g:checkBox name="selectedGroups" value="${g.target}" checked="${g.allowed}"/></td>
<td><label for="${g.target}">${g.target}</label></td>
<td><input type="text" id="${g.target}" name="${g.target}" value="${g.ports}"/></td>
Expand Down
4 changes: 2 additions & 2 deletions grails-app/views/loadBalancer/show.gsp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -142,8 +142,8 @@
</tr>
</thead>
<tbody>
<g:each var="is" in="${instanceStates}">
<tr>
<g:each var="is" in="${instanceStates}" status="i">
<tr class="${(i % 2) == 0 ? 'odd' : 'even'}">
<td><g:linkObject type="instance" name="${is.instanceId}"/></td>
<td><g:availabilityZone value="${is.availabilityZone}"/></td>
<td><g:linkObject type="autoScaling" name="${is.autoScalingGroupName}"/></td>
Expand Down
4 changes: 2 additions & 2 deletions grails-app/views/security/edit.gsp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -61,8 +61,8 @@
</tr>
</thead>
<tbody>
<g:each var="g" in="${groups}">
<tr>
<g:each var="g" in="${groups}" status="i">
<tr class="${(i % 2) == 0 ? 'odd' : 'even'}">
<td class="checkbox"><g:checkBox name="selectedGroups" value="${g.source}" checked="${g.allowed}"/></td>
<td><label for="${g.source}">${g.source}</label></td>
<td><input type="text" id="${g.source}" name="${g.source}" value="${g.ports}"/></td>
Expand Down
Loading

0 comments on commit 0d00562

Please sign in to comment.