diff --git a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy index 42aa10d6..ecd34b97 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 @@ -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 awsClient def awsClientService @@ -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 @@ -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. @@ -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 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. */ @@ -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 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 + * 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) { @@ -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 @@ -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 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) { @@ -779,29 +820,12 @@ 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 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}" @@ -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)) } 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 @@ - - + + 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()]) diff --git a/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy b/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy index c95eea82..616acbbe 100644 --- a/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy +++ b/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy @@ -51,6 +51,8 @@ class AwsEc2ServiceUnitSpec extends Specification { CachedMap mockInstanceCache CachedMap mockReservationCache AwsEc2Service awsEc2Service + ConfigService configService + TaskService taskService def setup() { userContext = UserContext.auto() @@ -67,13 +69,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 \ @@ -414,21 +418,22 @@ and groupName is #groupName""") 0 * _ } - 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), @@ -439,6 +444,10 @@ and groupName is #groupName""") ])) 1 * mockAmazonEC2.describeSecurityGroups(_) >> new DescribeSecurityGroupsResult( securityGroups: [new SecurityGroup()]) + 1 * taskService.runTask(_, _, _, _) >> { + Closure work = it[2] + work(new Task()) + } 0 * _ } @@ -469,4 +478,17 @@ and groupName is #groupName""") 1 * mockVpcCache.list() >> [new Vpc(vpcId: 'vpc-123')] 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') + } }