From 3297a11066ff4061b970fd87f154050162627ecc Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Mon, 9 Jul 2012 10:34:47 -0700 Subject: [PATCH 1/9] Include zebra striping on page load before JavaScript This removes the table's occasional white flicker when the page loads. --- grails-app/views/application/security.gsp | 4 ++-- grails-app/views/loadBalancer/show.gsp | 4 ++-- grails-app/views/security/edit.gsp | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/grails-app/views/application/security.gsp b/grails-app/views/application/security.gsp index e5f5f635..21069e01 100644 --- a/grails-app/views/application/security.gsp +++ b/grails-app/views/application/security.gsp @@ -60,8 +60,8 @@ - - + + diff --git a/grails-app/views/loadBalancer/show.gsp b/grails-app/views/loadBalancer/show.gsp index 20eb085a..bbe3a5e4 100644 --- a/grails-app/views/loadBalancer/show.gsp +++ b/grails-app/views/loadBalancer/show.gsp @@ -142,8 +142,8 @@ - - + + diff --git a/grails-app/views/security/edit.gsp b/grails-app/views/security/edit.gsp index 01ba5a25..ce9d7afc 100644 --- a/grails-app/views/security/edit.gsp +++ b/grails-app/views/security/edit.gsp @@ -61,8 +61,8 @@ - - + + From 4a89bbb568498d697769ccf991939979ba234850 Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Wed, 18 Dec 2013 13:58:58 -0800 Subject: [PATCH 2/9] Shorten a few lines longer than 120 chars --- .../services/com/netflix/asgard/AwsEc2Service.groovy | 10 ++++++---- 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy index 0f3d82ed..bb234ffd 100644 --- a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy +++ b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy @@ -435,8 +435,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 @@ -778,7 +778,8 @@ class AwsEc2Service implements CacheInitializer, InitializingBean { 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}" @@ -891,7 +892,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)) } From d7e22044256e81de533d9b6266aa4b0c82a2cc22 Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Tue, 10 Jul 2012 16:53:33 -0700 Subject: [PATCH 3/9] Fix small edge case bug on lone() If security group stops existing, use loneOrNone() to get null instead of lone(). Otherwise there's no reason to use null safe operator on the next line. Also, remove unnecessary null safety on "result". --- grails-app/services/com/netflix/asgard/AwsEc2Service.groovy | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy index bb234ffd..3528752d 100644 --- a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy +++ b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy @@ -447,7 +447,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. From 7641a8e077b295f86f4424240ee7e97427e9fc3c Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Wed, 18 Dec 2013 14:35:54 -0800 Subject: [PATCH 4/9] Remove unused methods --- .../com/netflix/asgard/AwsEc2Service.groovy | 20 -------- .../netflix/asgard/AwsEc2ServiceTests.groovy | 51 ------------------- 2 files changed, 71 deletions(-) diff --git a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy index 3528752d..84da5a29 100644 --- a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy +++ b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy @@ -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 @@ -756,24 +754,6 @@ class AwsEc2Service implements CacheInitializer, InitializingBean { instances ? instances[0] : null } - Multiset getCountedAppVersions(UserContext userContext) { - Map imageIdsToImages = caches.allImages.by(userContext.region).unmodifiable() - getCountedAppVersionsForInstancesAndImages(getInstances(userContext), imageIdsToImages) - } - - private Multiset getCountedAppVersionsForInstancesAndImages(Collection instances, - Map images) { - Multiset 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 diff --git a/test/unit/com/netflix/asgard/AwsEc2ServiceTests.groovy b/test/unit/com/netflix/asgard/AwsEc2ServiceTests.groovy index cb5100b9..0ba94c0f 100644 --- a/test/unit/com/netflix/asgard/AwsEc2ServiceTests.groovy +++ b/test/unit/com/netflix/asgard/AwsEc2ServiceTests.groovy @@ -21,13 +21,10 @@ import com.amazonaws.services.ec2.model.DescribeImagesRequest import com.amazonaws.services.ec2.model.DescribeImagesResult import com.amazonaws.services.ec2.model.Filter import com.amazonaws.services.ec2.model.Image -import com.amazonaws.services.ec2.model.Instance import com.amazonaws.services.ec2.model.SecurityGroup import com.amazonaws.services.ec2.model.SpotInstanceRequest import com.amazonaws.services.ec2.model.Tag -import com.google.common.collect.Multiset import com.netflix.asgard.mock.Mocks -import com.netflix.frigga.ami.AppVersion import grails.test.GrailsUnitTestCase import grails.test.MockUtils @@ -81,54 +78,6 @@ class AwsEc2ServiceTests extends GrailsUnitTestCase { assertNull awsEc2Service.getImage(Mocks.userContext(), "doesn't exist") } - @SuppressWarnings("GroovyAccessibility") - void testgetCountedAppVersionsForInstancesAndImages() { - - AwsEc2Service service = new AwsEc2Service() - - Multiset noAppVersions = service.getCountedAppVersionsForInstancesAndImages([], [:]) - assert noAppVersions.isEmpty() - - String appVersionString1 = "app-1.2.3-456789.h321/WE-WAPP-app/321" - String appVersionString2 = "app-1.2.4-567890.h432/WE-WAPP-app/432" - - AppVersion appVersion1 = Relationships.dissectAppVersion(appVersionString1) - AppVersion appVersion2 = Relationships.dissectAppVersion(appVersionString2) - - Image image1 = (new Image()).withImageId("image-1").withTags(new Tag("appversion", appVersionString1)) - Image image2 = (new Image()).withImageId("image-2").withTags(new Tag("appversion", appVersionString2)) - Image imageNoApp = (new Image()).withImageId("image-noapp") - - Instance img1_inst1 = (new Instance()).withImageId(image1.imageId) - Instance img1_inst2 = (new Instance()).withImageId(image1.imageId) - Instance img2_inst1 = (new Instance()).withImageId(image2.imageId) - Instance img2_inst2 = (new Instance()).withImageId(image2.imageId) - Instance imgNoApp_inst1 = (new Instance()).withImageId(imageNoApp.imageId) - - Multiset oneAppOneInstance = service.getCountedAppVersionsForInstancesAndImages([img1_inst1], [(image1.imageId): image1]) - assert 1 == oneAppOneInstance.size() - assert 1 == oneAppOneInstance.count(appVersion1) - assert 0 == oneAppOneInstance.count(appVersion2) - - Multiset oneAppTwoInstance = service.getCountedAppVersionsForInstancesAndImages([img1_inst1, img1_inst2], - [(image1.imageId): image1]) - assert 2 == oneAppTwoInstance.size() - assert 2 == oneAppTwoInstance.count(appVersion1) - assert 0 == oneAppTwoInstance.count(appVersion2) - - Multiset twoAppThreeInstance = service.getCountedAppVersionsForInstancesAndImages([img1_inst1, img2_inst1, img2_inst2], - [(image1.imageId): image1, (image2.imageId): image2]) - assert 3 == twoAppThreeInstance.size() - assert 1 == twoAppThreeInstance.count(appVersion1) - assert 2 == twoAppThreeInstance.count(appVersion2) - - Multiset threeAppThreeInstance = service.getCountedAppVersionsForInstancesAndImages([img1_inst1, img2_inst1, imgNoApp_inst1], - [(image1.imageId): image1, (image2.imageId): image2, (imageNoApp.imageId): imageNoApp]) - assert 2 == threeAppThreeInstance.size() - assert 1 == threeAppThreeInstance.count(appVersion1) - assert 1 == threeAppThreeInstance.count(appVersion2) - } - void testRetrieveImagesWithTags() { Mocks.monkeyPatcherService() Image image1 = new Image(imageId: 'imageId1', tags: [new Tag()]) From a2e0d285521c97fe4f342047adba9c4ec67fe0d4 Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Wed, 18 Dec 2013 14:37:20 -0800 Subject: [PATCH 5/9] =?UTF-8?q?Make=20=E2=80=98tcp=E2=80=99=20a=20static?= =?UTF-8?q?=20variable=20for=20re-use?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- grails-app/services/com/netflix/asgard/AwsEc2Service.groovy | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy index 84da5a29..626d3f23 100644 --- a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy +++ b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy @@ -103,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 awsClient def awsClientService @@ -605,7 +606,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 From 6cb1a6a438f50da29292b443247b2acf8610730a Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Wed, 18 Dec 2013 14:39:18 -0800 Subject: [PATCH 6/9] Better GroovyDoc comments --- .../com/netflix/asgard/AwsEc2Service.groovy | 28 +++++++++++++++++-- 1 file changed, 25 insertions(+), 3 deletions(-) diff --git a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy index 626d3f23..a21d1fef 100644 --- a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy +++ b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy @@ -533,7 +533,16 @@ 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 wantPerms) { List havePerms = getIngressFrom(targetGroup, sourceGroup) @@ -573,12 +582,25 @@ class AwsEc2Service implements CacheInitializer, InitializingBean { } } - /** Returns the canonical string representation of a from-to port pair. */ + /** + * 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 permissionsFromString(String portsStr) { List perms = [] if (portsStr) { From c5903a4f4f69dbe6f576bbb299152556b3c2d3ad Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Wed, 18 Dec 2013 14:40:28 -0800 Subject: [PATCH 7/9] Unit tests for portString() method --- .../com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy | 13 +++++++++++++ 1 file changed, 13 insertions(+) diff --git a/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy b/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy index c50f8ba1..b2c477df 100644 --- a/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy +++ b/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy @@ -431,4 +431,17 @@ and groupName is #groupName""") securityGroups: [new SecurityGroup()]) 0 * _ } + + def 'should convert two port numbers to a port range string'(){ + expect: + '7001-7002' == AwsEc2Service.portString(7001, 7002) + '7001' == AwsEc2Service.portString(7001, 7001) + } + + def 'should convert a comma-delimited string of port ranges to port-populated IpPermission objects'() { + expect: + [ + new IpPermission(fromPort: 7001, toPort: 7001), new IpPermission(fromPort: 7101, toPort: 7102) + ] == AwsEc2Service.permissionsFromString('7001,7101-7102') + } } From 0a2f5e647a28c5d5c5013c2fa70ceec621881183 Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Wed, 18 Dec 2013 16:32:48 -0800 Subject: [PATCH 8/9] ASGARD-923 - Only one auth-revoke task with many IP permissions for each security group pair change Refactored and enhanced the Security Group permissions authorization and revocation code so that, for each relationship between a target-source pair of Security Groups, Asgard will: Combine all IP permission authorizations into one Amazon call Combine all IP permission revocations into one Amazon call Combine all authorization and revocations into one Asgard task --- .../com/netflix/asgard/AwsEc2Service.groovy | 122 ++++++++++-------- .../asgard/AwsEc2ServiceUnitSpec.groovy | 30 +++-- 2 files changed, 91 insertions(+), 61 deletions(-) diff --git a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy index a21d1fef..77e635df 100644 --- a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy +++ b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy @@ -544,30 +544,60 @@ class AwsEc2Service implements CacheInitializer, InitializingBean { * @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 wantPerms) { - List havePerms = getIngressFrom(targetGroup, sourceGroup) - if (!havePerms && !wantPerms) { + List desired) { + List 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 toRevoke = determinePermissionsToChange(current, desired, srcGroupId) + List 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.

+ * + * 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 determinePermissionsToChange(List thesePermissions, + List otherPermissions, UserIdGroupPair userIdGroupPair) { + + List 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. */ @@ -582,6 +612,28 @@ class AwsEc2Service implements CacheInitializer, InitializingBean { } } + private void authorizePermissions(UserContext userContext, SecurityGroup source, SecurityGroup target, + List 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 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 @@ -639,38 +691,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 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 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) { diff --git a/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy b/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy index b2c477df..7ef70174 100644 --- a/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy +++ b/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy @@ -16,6 +16,7 @@ package com.netflix.asgard import com.amazonaws.AmazonServiceException +import com.amazonaws.services.autoscaling.model.AutoScalingGroup import com.amazonaws.services.ec2.AmazonEC2 import com.amazonaws.services.ec2.model.AuthorizeSecurityGroupIngressRequest import com.amazonaws.services.ec2.model.DescribeSecurityGroupsRequest @@ -48,6 +49,8 @@ class AwsEc2ServiceUnitSpec extends Specification { CachedMap mockInstanceCache CachedMap mockReservationCache AwsEc2Service awsEc2Service + ConfigService configService + TaskService taskService def setup() { userContext = UserContext.auto() @@ -60,13 +63,15 @@ class AwsEc2ServiceUnitSpec extends Specification { (EntityType.instance): mockInstanceCache, (EntityType.reservation): mockReservationCache, ])) - TaskService taskService = new TaskService() { - def runTask(UserContext userContext, String name, Closure work, Link link = null) { + taskService = Spy(TaskService) { + runTask(_, _, _, _) >> { + Closure work = it[2] work(new Task()) } } + configService = Mock(ConfigService) awsEc2Service = new AwsEc2Service(awsClient: new MultiRegionAwsClient({ mockAmazonEC2 }), caches: caches, - taskService: taskService) + configService: configService, taskService: taskService) } @Unroll("""getInstancesWithSecurityGroup should return #instanceIds when groupId is #groupId \ @@ -404,21 +409,22 @@ and groupName is #groupName""") 1 * mockSecurityGroupCache.list() >> { simulateWarGames() } } - def 'should update security groups'() { + def 'should update security group ingress permissions with auth and revoke but only one task run'() { List userIdGroupPairs = [new UserIdGroupPair(groupId: 'sg-s')] - SecurityGroup source = new SecurityGroup(groupName: 'source', groupId: 'sg-s') - SecurityGroup target = new SecurityGroup(groupName: 'target', groupId: 'sg-t', ipPermissions: [ - new IpPermission(fromPort: 1, toPort: 1, userIdGroupPairs: userIdGroupPairs), - new IpPermission(fromPort: 2, toPort: 2, userIdGroupPairs: userIdGroupPairs), + SecurityGroup source = new SecurityGroup(groupId: 'sg-s') + SecurityGroup target = new SecurityGroup(groupId: 'sg-t', ipPermissions: [ + new IpPermission(ipProtocol: 'tcp', fromPort: 1, toPort: 1, userIdGroupPairs: userIdGroupPairs), + new IpPermission(ipProtocol: 'tcp', fromPort: 2, toPort: 2, userIdGroupPairs: userIdGroupPairs), ]) when: awsEc2Service.updateSecurityGroupPermissions(userContext, target, source, [ - new IpPermission(fromPort: 2, toPort: 2), - new IpPermission(fromPort: 3, toPort: 3), + new IpPermission(ipProtocol: 'tcp', fromPort: 2, toPort: 2), + new IpPermission(ipProtocol: 'tcp', fromPort: 3, toPort: 3) ]) then: + 1 * configService.getAwsAccountNumber() 1 * mockAmazonEC2.authorizeSecurityGroupIngress(new AuthorizeSecurityGroupIngressRequest(groupId: 'sg-t', ipPermissions: [ new IpPermission(fromPort: 3, toPort: 3, ipProtocol: 'tcp', userIdGroupPairs: userIdGroupPairs), @@ -429,6 +435,10 @@ and groupName is #groupName""") ])) 1 * mockAmazonEC2.describeSecurityGroups(_) >> new DescribeSecurityGroupsResult( securityGroups: [new SecurityGroup()]) + 1 * taskService.runTask(_, _, _, _) >> { + Closure work = it[2] + work(new Task()) + } 0 * _ } From 0aa14c97762692781a094fb2a7c02f63586d60da Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Fri, 3 Jan 2014 13:23:23 -0800 Subject: [PATCH 9/9] Remove unused import --- test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy | 1 - 1 file changed, 1 deletion(-) diff --git a/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy b/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy index 7ef70174..691d92a1 100644 --- a/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy +++ b/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy @@ -16,7 +16,6 @@ package com.netflix.asgard import com.amazonaws.AmazonServiceException -import com.amazonaws.services.autoscaling.model.AutoScalingGroup import com.amazonaws.services.ec2.AmazonEC2 import com.amazonaws.services.ec2.model.AuthorizeSecurityGroupIngressRequest import com.amazonaws.services.ec2.model.DescribeSecurityGroupsRequest