From 3ec5fba92018ebbccc63f700885b3e38e6d7ab1a Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Sat, 21 Dec 2013 14:09:29 -0800 Subject: [PATCH 1/7] Warnings and whitespace --- test/unit/com/netflix/asgard/model/SubnetsSpec.groovy | 2 ++ 1 file changed, 2 insertions(+) diff --git a/test/unit/com/netflix/asgard/model/SubnetsSpec.groovy b/test/unit/com/netflix/asgard/model/SubnetsSpec.groovy index e9a9c1d8..48bcae13 100644 --- a/test/unit/com/netflix/asgard/model/SubnetsSpec.groovy +++ b/test/unit/com/netflix/asgard/model/SubnetsSpec.groovy @@ -21,6 +21,7 @@ import com.amazonaws.services.ec2.model.Tag import com.google.common.collect.ImmutableSet import spock.lang.Specification +@SuppressWarnings(["GroovyAccessibility", "GroovyAssignabilityCheck"]) class SubnetsSpec extends Specification { static SubnetData subnet(String id, String zone, String purpose, SubnetTarget target, String vpcId = 'vpc-1') { @@ -332,6 +333,7 @@ class SubnetsSpec extends Specification { (null): ['us-east-1a', 'us-east-1b'], ] } + def 'should return zones grouped by purpose including extra specified zones'() { subnets = new Subnets([ subnet('subnet-e9b0a3a1', 'us-east-1a', 'internal', SubnetTarget.EC2), From 8580455727a9f10522bf606a8748a7d1cdd6d15c Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Sat, 4 Jan 2014 19:08:02 -0800 Subject: [PATCH 2/7] Consistent comment style. --- .../com/netflix/asgard/model/Subnets.groovy | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/src/groovy/com/netflix/asgard/model/Subnets.groovy b/src/groovy/com/netflix/asgard/model/Subnets.groovy index 6c3bbe0e..a058de51 100644 --- a/src/groovy/com/netflix/asgard/model/Subnets.groovy +++ b/src/groovy/com/netflix/asgard/model/Subnets.groovy @@ -56,7 +56,7 @@ import groovy.transform.Canonical } /** - * Simply find a subnet based on its ID. + * Simply finds a subnet based on its ID. * * @param id of the subnet * @return the unique subnet with that ID or null @@ -78,7 +78,7 @@ import groovy.transform.Canonical } /** - * Find the subnet associated with the first Subnet ID. This is useful in cases where the attribute you care about + * Finds the subnet associated with the first Subnet ID. This is useful in cases where the attribute you care about * is guaranteed to be the same for all subnets. * * @param subnetIds Subnet IDs @@ -89,7 +89,7 @@ import groovy.transform.Canonical } /** - * Find the subnet IDs that map to specific zones + * Finds the subnet IDs that map to specific zones * * @param zones the zones in AWS that you want Subnet IDs for * @param purpose only subnets with the specified purpose will be returned @@ -114,7 +114,7 @@ import groovy.transform.Canonical } /** - * Group zones by subnet purposes they contain. + * Groups zones by subnet purposes they contain. * * @param allAvailabilityZones complete list of zones to group * @param target is the type of AWS object the subnet applies to (null means any object type) @@ -136,7 +136,7 @@ import groovy.transform.Canonical } /** - * Find all purposes across all specified zones for the specified target. + * Finds all purposes across all specified zones for the specified target. * * @param zones the zones in AWS that you want purposes for * @param target is the type of AWS object the subnet applies to (null means any object type) @@ -185,7 +185,7 @@ import groovy.transform.Canonical } /** - * Construct a new VPC Zone Identifier based on an existing VPC Zone Identifier and a list of zones. + * Constructs a new VPC Zone Identifier based on an existing VPC Zone Identifier and a list of zones. * A VPC Zone Identifier is really just a comma delimited list of subnet IDs. * I'm not happy that this method has to exist. It's just a wrapper around other methods that operate on a cleaner * abstraction without knowledge of the unfortunate structure of VPC Zone Identifier. @@ -204,7 +204,7 @@ import groovy.transform.Canonical } /** - * Figure out the subnet purpose given a VPC zone identifier. + * Figures out the subnet purpose given a VPC zone identifier. * * @param vpcZoneIdentifier is used to derive a subnet purpose from * @return the subnet purpose indicated by the vpcZoneIdentifier @@ -216,7 +216,7 @@ import groovy.transform.Canonical } /** - * Construct a new VPC Zone Identifier based on a subnet purpose and a list of zones. + * Constructs a new VPC Zone Identifier based on a subnet purpose and a list of zones. * * @param purpose is used to derive a subnet purpose from * @param zones which the new VPC Zone Identifier will contain From 90ae2e7764c3368f0b5977f758ffdc623c96827d Mon Sep 17 00:00:00 2001 From: Clay McCoy Date: Thu, 2 Jan 2014 15:55:48 -0800 Subject: [PATCH 3/7] use default VPC as a default --- .../asgard/AutoScalingController.groovy | 2 +- .../netflix/asgard/ClusterController.groovy | 8 +++---- .../asgard/LoadBalancerController.groovy | 5 ++--- .../com/netflix/asgard/AwsEc2Service.groovy | 2 +- .../com/netflix/asgard/PushService.groovy | 3 +-- .../com/netflix/asgard/model/Subnets.groovy | 21 ++++++++++++++++--- 6 files changed, 27 insertions(+), 14 deletions(-) diff --git a/grails-app/controllers/com/netflix/asgard/AutoScalingController.groovy b/grails-app/controllers/com/netflix/asgard/AutoScalingController.groovy index a7e5b348..1f376090 100644 --- a/grails-app/controllers/com/netflix/asgard/AutoScalingController.groovy +++ b/grails-app/controllers/com/netflix/asgard/AutoScalingController.groovy @@ -294,7 +294,7 @@ class AutoScalingController { String groupName = Relationships.buildGroupName(params) Subnets subnets = awsEc2Service.getSubnets(userContext) String subnetPurpose = params.subnetPurpose ?: null - String vpcId = subnets.mapPurposeToVpcId()[subnetPurpose] ?: '' + String vpcId = subnets.getVpcIdForSubnetPurpose(subnetPurpose) ?: '' // Auto Scaling Group Integer minSize = (params.min ?: 0) as Integer diff --git a/grails-app/controllers/com/netflix/asgard/ClusterController.groovy b/grails-app/controllers/com/netflix/asgard/ClusterController.groovy index fe9655dc..90c619ea 100644 --- a/grails-app/controllers/com/netflix/asgard/ClusterController.groovy +++ b/grails-app/controllers/com/netflix/asgard/ClusterController.groovy @@ -136,7 +136,7 @@ class ClusterController { Subnets subnets = awsEc2Service.getSubnets(userContext) List subnetIds = Relationships.subnetIdsFromVpcZoneIdentifier(lastGroup.vpcZoneIdentifier) String subnetPurpose = subnets.coerceLoneOrNoneFromIds(subnetIds)?.purpose - String vpcId = subnets.mapPurposeToVpcId()[subnetPurpose] ?: '' + String vpcId = subnets.getVpcIdForSubnetPurpose(subnetPurpose) ?: '' List selectedLoadBalancers = Requests.ensureList( params["selectedLoadBalancersForVpcId${vpcId}"]) ?: lastGroup.loadBalancerNames log.debug """ClusterController.show for Cluster '${cluster.name}' Load Balancers from last Group: \ @@ -265,7 +265,7 @@ ${lastGroup.loadBalancerNames}""" Subnets subnets = awsEc2Service.getSubnets(userContext) List subnetIds = Relationships.subnetIdsFromVpcZoneIdentifier(lastGroup.vpcZoneIdentifier) String subnetPurpose = subnets.coerceLoneOrNoneFromIds(subnetIds)?.purpose - String vpcId = subnets.mapPurposeToVpcId()[subnetPurpose] ?: '' + String vpcId = subnets.getVpcIdForSubnetPurpose(subnetPurpose) ?: '' List selectedLoadBalancers = Requests.ensureList( params["selectedLoadBalancersForVpcId${vpcId}"]) ?: lastGroup.loadBalancerNames List subnetPurposes = subnets.getPurposesForZones(availabilityZones*.zoneName, @@ -310,7 +310,7 @@ ${lastGroup.loadBalancerNames}""" } String subnetPurpose = params.subnetPurpose Subnets subnets = awsEc2Service.getSubnets(userContext) - String vpcId = subnets.mapPurposeToVpcId()[subnetPurpose] ?: '' + String vpcId = subnets.getVpcIdForSubnetPurpose(subnetPurpose) ?: '' List loadBalancerNames = Requests.ensureList(params["selectedLoadBalancersForVpcId${vpcId}"] ?: params["selectedLoadBalancers"]) @@ -375,7 +375,7 @@ ${lastGroup.loadBalancerNames}""" List termPolicies = Requests.ensureList(params.terminationPolicy) Subnets subnets = awsEc2Service.getSubnets(userContext) String subnetPurpose = params.subnetPurpose - String vpcId = subnets.mapPurposeToVpcId()[subnetPurpose] ?: '' + String vpcId = subnets.getVpcIdForSubnetPurpose(subnetPurpose) ?: '' List loadBalancerNames = Requests.ensureList(params["selectedLoadBalancersForVpcId${vpcId}"] ?: params["selectedLoadBalancers"]) // Availability zones default to the last group's value since this field is required. diff --git a/grails-app/controllers/com/netflix/asgard/LoadBalancerController.groovy b/grails-app/controllers/com/netflix/asgard/LoadBalancerController.groovy index 56496560..942e2cff 100644 --- a/grails-app/controllers/com/netflix/asgard/LoadBalancerController.groovy +++ b/grails-app/controllers/com/netflix/asgard/LoadBalancerController.groovy @@ -106,7 +106,6 @@ class LoadBalancerController { Collection selectedZones = awsEc2Service.preselectedZoneNames(availabilityZones, Requests.ensureList(params.selectedZones)) Subnets subnets = awsEc2Service.getSubnets(userContext) - Map purposeToVpcId = subnets.mapPurposeToVpcId() [ applications: applicationService.getRegisteredApplicationsForLoadBalancer(userContext), stacks: stackService.getStacks(userContext), @@ -114,8 +113,8 @@ class LoadBalancerController { subnetPurposes: subnets.getPurposesForZones(availabilityZones*.zoneName, SubnetTarget.ELB).sort(), zonesGroupedByPurpose: subnets.groupZonesByPurpose(availabilityZones*.zoneName, SubnetTarget.ELB), selectedZones: selectedZones, - purposeToVpcId: purposeToVpcId, - vpcId: purposeToVpcId[params.subnetPurpose], + purposeToVpcId: subnets.mapPurposeToVpcId(), + vpcId: subnets.getVpcIdForSubnetPurpose(params.subnetPurpose), securityGroupsGroupedByVpcId: securityGroupsGroupedByVpcId, selectedSecurityGroups: Requests.ensureList(params.selectedSecurityGroups), ] diff --git a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy index ecd34b97..3c9c23f1 100644 --- a/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy +++ b/grails-app/services/com/netflix/asgard/AwsEc2Service.groovy @@ -211,7 +211,7 @@ class AwsEc2Service implements CacheInitializer, InitializingBean { * @return a wrapper for querying subnets */ Subnets getSubnets(UserContext userContext) { - Subnets.from(caches.allSubnets.by(userContext.region).list()) + Subnets.from(caches.allSubnets.by(userContext.region).list(), getDefaultVpcId(userContext)) } /** diff --git a/grails-app/services/com/netflix/asgard/PushService.groovy b/grails-app/services/com/netflix/asgard/PushService.groovy index ad693a71..1d024952 100644 --- a/grails-app/services/com/netflix/asgard/PushService.groovy +++ b/grails-app/services/com/netflix/asgard/PushService.groovy @@ -147,8 +147,7 @@ class PushService { Boolean imageListIsShort = images.size() < fullCount Subnets subnets = awsEc2Service.getSubnets(userContext) List effectiveSecurityGroups = awsEc2Service.getEffectiveSecurityGroups(userContext) - List subnetIds = Relationships.subnetIdsFromVpcZoneIdentifier(group.VPCZoneIdentifier) - String vpcId = subnets.coerceLoneOrNoneFromIds(subnetIds)?.vpcId + String vpcId = subnets.getVpcIdForVpcZoneIdentifier(group.VPCZoneIdentifier) Map purposeToVpcId = subnets.mapPurposeToVpcId() String pricing = lc.spotPrice ? InstancePriceType.SPOT.name() : InstancePriceType.ON_DEMAND.name() Map result = [ diff --git a/src/groovy/com/netflix/asgard/model/Subnets.groovy b/src/groovy/com/netflix/asgard/model/Subnets.groovy index a058de51..7cde6363 100644 --- a/src/groovy/com/netflix/asgard/model/Subnets.groovy +++ b/src/groovy/com/netflix/asgard/model/Subnets.groovy @@ -32,7 +32,10 @@ import groovy.transform.Canonical /** All of the subnets contained in this object. */ final private Collection allSubnets - private Subnets(Collection allSubnets) { + final private String defaultVpcId + + private Subnets(Collection allSubnets, String defaultVpcId) { + this.defaultVpcId = defaultVpcId this.allSubnets = ImmutableSet.copyOf(allSubnets) } @@ -42,8 +45,8 @@ import groovy.transform.Canonical * @param subnets the actual AWS Subnets * @return a new immutable Subnets based off the subnets */ - public static Subnets from(Collection subnets) { - new Subnets(subnets.collect() { SubnetData.from(it) }) + public static Subnets from(Collection subnets, String defaultVpcId = null) { + new Subnets(subnets.collect() { SubnetData.from(it) }, defaultVpcId) } /** @@ -88,6 +91,11 @@ import groovy.transform.Canonical subnetIds ? findSubnetById(subnetIds.iterator().next()?.trim()) : null } + String getVpcIdForVpcZoneIdentifier(String vpcZoneIdentifier) { + List subnetIds = Relationships.subnetIdsFromVpcZoneIdentifier(vpcZoneIdentifier) + coerceLoneOrNoneFromIds(subnetIds)?.vpcId ?: defaultVpcId + } + /** * Finds the subnet IDs that map to specific zones * @@ -160,6 +168,10 @@ import groovy.transform.Canonical Multimaps.index(targetSubnetsWithPurpose, { it.availabilityZone } as Function) } + String getVpcIdForSubnetPurpose(String subnetPurpose) { + mapPurposeToVpcId()[subnetPurpose ?: null] + } + /** * Provides a one to one mapping from a Subnet purpose to its VPC ID. Purposes that span VPCs in the same region * are invalid and will be left out of the map. @@ -171,6 +183,9 @@ import groovy.transform.Canonical subnetsGroupedByPurpose.inject([:]) { Map purposeToVpcId, Map.Entry entry -> String purpose = entry.key if (!purpose) { + if (defaultVpcId) { + purposeToVpcId[null] = defaultVpcId + } return purposeToVpcId } List subnets = entry.value as List From eae57e49d52e530351315361000d947c26de3f53 Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Sat, 4 Jan 2014 02:36:01 -0800 Subject: [PATCH 4/7] Subnets docs --- .../com/netflix/asgard/model/Subnets.groovy | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/src/groovy/com/netflix/asgard/model/Subnets.groovy b/src/groovy/com/netflix/asgard/model/Subnets.groovy index 7cde6363..395bae0f 100644 --- a/src/groovy/com/netflix/asgard/model/Subnets.groovy +++ b/src/groovy/com/netflix/asgard/model/Subnets.groovy @@ -32,6 +32,7 @@ import groovy.transform.Canonical /** All of the subnets contained in this object. */ final private Collection allSubnets + /** The identifier of the default VPC of the account-region, if available. */ final private String defaultVpcId private Subnets(Collection allSubnets, String defaultVpcId) { @@ -40,9 +41,10 @@ import groovy.transform.Canonical } /** - * Construct Subnets from AWS Subnets + * Constructs Subnets from AWS Subnets. * - * @param subnets the actual AWS Subnets + * @param subnets the actual AWS Subnets + * @param defaultVpcId the identifier of the default VPC, if available * @return a new immutable Subnets based off the subnets */ public static Subnets from(Collection subnets, String defaultVpcId = null) { @@ -91,6 +93,13 @@ import groovy.transform.Canonical subnetIds ? findSubnetById(subnetIds.iterator().next()?.trim()) : null } + /** + * Finds the identifier of the VPC indicated by the specified VPC Zone Identifier string. + * + * @param vpcZoneIdentifier the comma-delimited list of subnet IDs used in an ASG field as a roundabout way of + * indicating which VPC where the ASG launches instances + * @return the identifier of the VPC where the subnets exist if available, or the default VPC if available, or null + */ String getVpcIdForVpcZoneIdentifier(String vpcZoneIdentifier) { List subnetIds = Relationships.subnetIdsFromVpcZoneIdentifier(vpcZoneIdentifier) coerceLoneOrNoneFromIds(subnetIds)?.vpcId ?: defaultVpcId @@ -168,6 +177,14 @@ import groovy.transform.Canonical Multimaps.index(targetSubnetsWithPurpose, { it.availabilityZone } as Function) } + /** + * Finds a matching VPC identifier for the specified purpose, or null if there is no VPC ID match for that purpose. + * If the purpose is null or an empty string, this method looks for default VPC ID if available. + * + * @param subnetPurpose the name of the purpose of the VPC + * @return the identifier of the VPC that has the specified purpose, or the ID of the default VPC if the purpose + * specified is null or an empty string, or null if no matching VPC exists + */ String getVpcIdForSubnetPurpose(String subnetPurpose) { mapPurposeToVpcId()[subnetPurpose ?: null] } From a045505086b4775fff6d5bb27d3d063041e3594b Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Fri, 3 Jan 2014 15:29:09 -0800 Subject: [PATCH 5/7] Fixed load balancer update bug for region-account with default VPC --- .../asgard/LoadBalancerController.groovy | 3 +- .../asgard/LoadBalancerControllerSpec.groovy | 59 ++++++++++++++++--- 2 files changed, 52 insertions(+), 10 deletions(-) diff --git a/grails-app/controllers/com/netflix/asgard/LoadBalancerController.groovy b/grails-app/controllers/com/netflix/asgard/LoadBalancerController.groovy index 942e2cff..30931c95 100644 --- a/grails-app/controllers/com/netflix/asgard/LoadBalancerController.groovy +++ b/grails-app/controllers/com/netflix/asgard/LoadBalancerController.groovy @@ -215,7 +215,8 @@ class LoadBalancerController { LoadBalancerDescription lb = awsLoadBalancerService.getLoadBalancer(userContext, name) List zoneList = Requests.ensureList(params.selectedZones) - if (lb.subnets) { + List defaultVpcSubnetIds = awsEc2Service.getDefaultVpcSubnetIds(userContext) + if (lb.subnets.any { !(it in defaultVpcSubnetIds) }) { updateLbSubnets(userContext, name, zoneList, lb.subnets) } else { updateLbZones(userContext, name, zoneList, lb.availabilityZones) diff --git a/test/unit/com/netflix/asgard/LoadBalancerControllerSpec.groovy b/test/unit/com/netflix/asgard/LoadBalancerControllerSpec.groovy index 625ebf7b..df2902c9 100644 --- a/test/unit/com/netflix/asgard/LoadBalancerControllerSpec.groovy +++ b/test/unit/com/netflix/asgard/LoadBalancerControllerSpec.groovy @@ -15,19 +15,22 @@ */ package com.netflix.asgard +import com.amazonaws.services.elasticloadbalancing.model.HealthCheck +import com.amazonaws.services.elasticloadbalancing.model.LoadBalancerDescription import com.amazonaws.services.elasticloadbalancing.AmazonElasticLoadBalancing -import com.amazonaws.services.elasticloadbalancing.model.CreateLoadBalancerListenersRequest -import com.amazonaws.services.elasticloadbalancing.model.DeleteLoadBalancerListenersRequest import com.amazonaws.services.elasticloadbalancing.model.Listener -import com.netflix.asgard.mock.Mocks import grails.test.MockUtils import grails.test.mixin.TestFor import spock.lang.Specification +import spock.lang.Unroll +@SuppressWarnings("GroovyAssignabilityCheck") @TestFor(LoadBalancerController) class LoadBalancerControllerSpec extends Specification { AmazonElasticLoadBalancing mockElb = Mock(AmazonElasticLoadBalancing) + AwsLoadBalancerService awsLoadBalancerService = Mock(AwsLoadBalancerService) + AwsEc2Service awsEc2Service = Mock(AwsEc2Service) void setup() { TestUtils.setUpMockRequest() @@ -35,10 +38,49 @@ class LoadBalancerControllerSpec extends Specification { [AddListenerCommand, RemoveListenerCommand].each { MockUtils.prepareForConstraintsTests(it) } - controller.awsLoadBalancerService = Mocks.newAwsLoadBalancerService(mockElb) + controller.awsLoadBalancerService = awsLoadBalancerService + awsLoadBalancerService.awsClient = new MultiRegionAwsClient({ mockElb }) + controller.awsEc2Service = awsEc2Service mockElb.describeLoadBalancers(_) >> { [] } } + private void setUpHealthCheckParams() { + params.target = 'HTTP:8080/' + params.interval = '5' + params.timeout = '10' + params.unhealthy = '3' + params.healthy = '4' + } + + @Unroll('update should change zones but not subnets when default VPC subnets #defaultVpcSubnetsCondition') + def 'update without custom VPC should change zones but not subnets'() { + params.name = 'hello' + params.selectedZones = ['us-east-1b', 'us-east-1c'] + setUpHealthCheckParams() + + when: + controller.update() + + then: + flash.message == "Added zone [us-east-1c] to load balancer. Removed zone [us-east-1a] from load balancer. " + + "Load Balancer 'hello' health check has been updated. " + 1 * controller.awsLoadBalancerService.getLoadBalancer(_, 'hello') >> + new LoadBalancerDescription(availabilityZones: ['us-east-1a', 'us-east-1b'], subnets: lbSubnetIds) + 1 * controller.awsEc2Service.getDefaultVpcSubnetIds(_) >> defaultVpcSubnetIds + 1 * controller.awsLoadBalancerService.addZones(_, 'hello', ['us-east-1c']) + 1 * controller.awsLoadBalancerService.removeZones(_, 'hello', ['us-east-1a']) + 1 * controller.awsLoadBalancerService.configureHealthCheck(_, 'hello', new HealthCheck(target: 'HTTP:8080/', + interval: 5, timeout: 10, unhealthyThreshold: 3, healthyThreshold: 4)) + 0 * _ + + where: + defaultVpcSubnetIds | lbSubnetIds | defaultVpcSubnetsCondition + [] | [] | 'do not exist and load balancer has no subnets' + ['subnet-123', 'subnet-456'] | [] | 'exist but load balancer has no subnets' + ['subnet-123', 'subnet-456'] | ['subnet-123'] | 'are partially used by load balancer' + ['subnet-123', 'subnet-456'] | ['subnet-123', 'subnet-456'] | 'are all used by load balancer' + } + def 'addListener should fail without instance port'() { final cmd = new AddListenerCommand(name: 'app--test') cmd.validate() @@ -54,7 +96,7 @@ class LoadBalancerControllerSpec extends Specification { def 'addListener should fail with error'() { final cmd = new AddListenerCommand(name: 'app--test', protocol: 'http', lbPort: 80, instancePort: 7001) cmd.validate() - mockElb.createLoadBalancerListeners(_) >> { + controller.awsLoadBalancerService.addListeners(* _) >> { throw new IllegalArgumentException("ELB service problems!") } @@ -76,8 +118,8 @@ class LoadBalancerControllerSpec extends Specification { then: response.redirectUrl == '/loadBalancer/show/app--test' controller.flash.message == "Listener has been added to port 80." - 1 * mockElb.createLoadBalancerListeners(new CreateLoadBalancerListenersRequest(loadBalancerName: 'app--test', - listeners: [new Listener(protocol: 'http', loadBalancerPort: 80, instancePort: 7001)])) + 1 * controller.awsLoadBalancerService.addListeners(_, 'app--test', + [new Listener(protocol: 'http', loadBalancerPort: 80, instancePort: 7001)]) 0 * _._ } @@ -91,8 +133,7 @@ class LoadBalancerControllerSpec extends Specification { then: response.redirectUrl == '/loadBalancer/show/app--test' controller.flash.message == "Listener on port 80 has been removed." - 1 * mockElb.deleteLoadBalancerListeners(new DeleteLoadBalancerListenersRequest(loadBalancerName: 'app--test', - loadBalancerPorts: [80])) + 1 * controller.awsLoadBalancerService.removeListeners(_, 'app--test', [80]) 0 * _._ } } From f14fcd98fa3af8697a97517160e29b558033840f Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Fri, 3 Jan 2014 23:42:05 -0800 Subject: [PATCH 6/7] Unit tests for Subnets and Load Balancer operations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Made fields of Subnets class readable so they’ll be included in the equals comparisons for unit testing. Still fully immutable. --- .../com/netflix/asgard/model/Subnets.groovy | 6 +- .../asgard/AwsEc2ServiceUnitSpec.groovy | 3 +- .../AwsLoadBalancerServiceUnitSpec.groovy | 31 ++++- .../asgard/LoadBalancerControllerSpec.groovy | 56 +++++++-- .../netflix/asgard/model/SubnetsSpec.groovy | 106 +++++++++++++++--- 5 files changed, 172 insertions(+), 30 deletions(-) diff --git a/src/groovy/com/netflix/asgard/model/Subnets.groovy b/src/groovy/com/netflix/asgard/model/Subnets.groovy index 395bae0f..27f4abd1 100644 --- a/src/groovy/com/netflix/asgard/model/Subnets.groovy +++ b/src/groovy/com/netflix/asgard/model/Subnets.groovy @@ -30,12 +30,12 @@ import groovy.transform.Canonical */ @Canonical class Subnets { /** All of the subnets contained in this object. */ - final private Collection allSubnets + final Collection allSubnets /** The identifier of the default VPC of the account-region, if available. */ - final private String defaultVpcId + final String defaultVpcId - private Subnets(Collection allSubnets, String defaultVpcId) { + private Subnets(Collection allSubnets, String defaultVpcId = null) { this.defaultVpcId = defaultVpcId this.allSubnets = ImmutableSet.copyOf(allSubnets) } diff --git a/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy b/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy index 616acbbe..c8ed0fa5 100644 --- a/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy +++ b/test/unit/com/netflix/asgard/AwsEc2ServiceUnitSpec.groovy @@ -41,6 +41,7 @@ import com.netflix.asgard.model.ZoneAvailability import spock.lang.Specification import spock.lang.Unroll +@SuppressWarnings("GroovyAssignabilityCheck") class AwsEc2ServiceUnitSpec extends Specification { UserContext userContext @@ -458,7 +459,7 @@ and groupName is #groupName""") then: subnetIds == ['subnet-luke', 'subnet-han'] - 1 * mockVpcCache.list() >> [new Vpc(vpcId: 'vpc-123'), new Vpc(vpcId: 'vpc-789', isDefault: true)] + 2 * mockVpcCache.list() >> [new Vpc(vpcId: 'vpc-123'), new Vpc(vpcId: 'vpc-789', isDefault: true)] 1 * mockSubnetCache.list() >> [ new Subnet(subnetId: 'subnet-luke', vpcId: 'vpc-789'), new Subnet(subnetId: 'subnet-han', vpcId: 'vpc-789'), diff --git a/test/unit/com/netflix/asgard/AwsLoadBalancerServiceUnitSpec.groovy b/test/unit/com/netflix/asgard/AwsLoadBalancerServiceUnitSpec.groovy index 2f4c6061..55028d50 100644 --- a/test/unit/com/netflix/asgard/AwsLoadBalancerServiceUnitSpec.groovy +++ b/test/unit/com/netflix/asgard/AwsLoadBalancerServiceUnitSpec.groovy @@ -20,16 +20,20 @@ import com.amazonaws.services.autoscaling.model.Instance import com.amazonaws.services.ec2.model.SecurityGroup import com.amazonaws.services.elasticloadbalancing.AmazonElasticLoadBalancing import com.amazonaws.services.elasticloadbalancing.model.AttachLoadBalancerToSubnetsRequest +import com.amazonaws.services.elasticloadbalancing.model.CreateLoadBalancerListenersRequest +import com.amazonaws.services.elasticloadbalancing.model.DeleteLoadBalancerListenersRequest import com.amazonaws.services.elasticloadbalancing.model.DescribeInstanceHealthResult import com.amazonaws.services.elasticloadbalancing.model.DescribeLoadBalancersRequest import com.amazonaws.services.elasticloadbalancing.model.DescribeLoadBalancersResult import com.amazonaws.services.elasticloadbalancing.model.DetachLoadBalancerFromSubnetsRequest import com.amazonaws.services.elasticloadbalancing.model.InstanceState +import com.amazonaws.services.elasticloadbalancing.model.Listener import com.amazonaws.services.elasticloadbalancing.model.LoadBalancerDescription import com.netflix.asgard.model.InstanceStateData import spock.lang.Specification import spock.lang.Unroll +@SuppressWarnings("GroovyAssignabilityCheck") class AwsLoadBalancerServiceUnitSpec extends Specification { UserContext userContext @@ -42,7 +46,8 @@ class AwsLoadBalancerServiceUnitSpec extends Specification { mockAmazonElasticLoadBalancing = Mock(AmazonElasticLoadBalancing) MultiRegionAwsClient awsClient = new MultiRegionAwsClient({ mockAmazonElasticLoadBalancing }) TaskService taskService = new TaskService() { - def runTask(UserContext userContext, String name, Closure work, Link link = null) { + def runTask(UserContext userContext, String name, Closure work, Link link = null, + Task existingTask = null) { work(new Task()) } } @@ -122,6 +127,30 @@ class AwsLoadBalancerServiceUnitSpec extends Specification { ].collect { new InstanceStateData(it) } } + def 'should add listener'() { + + List listeners = [new Listener(protocol: 'http', loadBalancerPort: 80, instancePort: 7001)] + + when: + awsLoadBalancerService.addListeners(UserContext.auto(), 'app--frontend', listeners) + + then: + 1 * mockAmazonElasticLoadBalancing.createLoadBalancerListeners( + new CreateLoadBalancerListenersRequest('app--frontend', listeners)) + 0 * _ + } + + def 'should remove listener'() { + + when: + awsLoadBalancerService.removeListeners(UserContext.auto(), 'app--frontend', [80]) + + then: + 1 * mockAmazonElasticLoadBalancing.deleteLoadBalancerListeners( + new DeleteLoadBalancerListenersRequest('app--frontend', [80])) + 0 * _ + } + def 'should update subnets'() { Collection oldSubnets = ['subnet-101', 'subnet-102', 'subnet-103'] Collection newSubnets = ['subnet-103', 'subnet-104'] diff --git a/test/unit/com/netflix/asgard/LoadBalancerControllerSpec.groovy b/test/unit/com/netflix/asgard/LoadBalancerControllerSpec.groovy index df2902c9..febc37f5 100644 --- a/test/unit/com/netflix/asgard/LoadBalancerControllerSpec.groovy +++ b/test/unit/com/netflix/asgard/LoadBalancerControllerSpec.groovy @@ -15,10 +15,13 @@ */ package com.netflix.asgard +import com.amazonaws.services.ec2.model.Subnet +import com.amazonaws.services.ec2.model.Tag import com.amazonaws.services.elasticloadbalancing.model.HealthCheck import com.amazonaws.services.elasticloadbalancing.model.LoadBalancerDescription import com.amazonaws.services.elasticloadbalancing.AmazonElasticLoadBalancing import com.amazonaws.services.elasticloadbalancing.model.Listener +import com.netflix.asgard.model.Subnets import grails.test.MockUtils import grails.test.mixin.TestFor import spock.lang.Specification @@ -53,7 +56,7 @@ class LoadBalancerControllerSpec extends Specification { } @Unroll('update should change zones but not subnets when default VPC subnets #defaultVpcSubnetsCondition') - def 'update without custom VPC should change zones but not subnets'() { + def 'update without custom VPC subnets should change zones but not subnets'() { params.name = 'hello' params.selectedZones = ['us-east-1b', 'us-east-1c'] setUpHealthCheckParams() @@ -64,13 +67,13 @@ class LoadBalancerControllerSpec extends Specification { then: flash.message == "Added zone [us-east-1c] to load balancer. Removed zone [us-east-1a] from load balancer. " + "Load Balancer 'hello' health check has been updated. " - 1 * controller.awsLoadBalancerService.getLoadBalancer(_, 'hello') >> + 1 * awsLoadBalancerService.getLoadBalancer(_, 'hello') >> new LoadBalancerDescription(availabilityZones: ['us-east-1a', 'us-east-1b'], subnets: lbSubnetIds) - 1 * controller.awsEc2Service.getDefaultVpcSubnetIds(_) >> defaultVpcSubnetIds - 1 * controller.awsLoadBalancerService.addZones(_, 'hello', ['us-east-1c']) - 1 * controller.awsLoadBalancerService.removeZones(_, 'hello', ['us-east-1a']) - 1 * controller.awsLoadBalancerService.configureHealthCheck(_, 'hello', new HealthCheck(target: 'HTTP:8080/', - interval: 5, timeout: 10, unhealthyThreshold: 3, healthyThreshold: 4)) + 1 * awsEc2Service.getDefaultVpcSubnetIds(_) >> defaultVpcSubnetIds + 1 * awsLoadBalancerService.addZones(_, 'hello', ['us-east-1c']) + 1 * awsLoadBalancerService.removeZones(_, 'hello', ['us-east-1a']) + 1 * awsLoadBalancerService.configureHealthCheck(_, 'hello', new HealthCheck(target: 'HTTP:8080/', interval: 5, + timeout: 10, unhealthyThreshold: 3, healthyThreshold: 4)) 0 * _ where: @@ -81,6 +84,45 @@ class LoadBalancerControllerSpec extends Specification { ['subnet-123', 'subnet-456'] | ['subnet-123', 'subnet-456'] | 'are all used by load balancer' } + @Unroll('update should change subnets but not zones when default VPC subnets #defaultVpcSubnetsCondition') + def 'update with custom VPC subnets should change subnets but not zones'() { + params.name = 'hello' + params.selectedZones = ['us-east-1b', 'us-east-1c'] + setUpHealthCheckParams() + + Subnets allSubnets = Subnets.from([ + new Subnet(subnetId: 'sn-123', vpcId: 'vpc-def', availabilityZone: 'us-east-1a'), + new Subnet(subnetId: 'sn-456', vpcId: 'vpc-def', availabilityZone: 'us-east-1b'), + new Subnet(subnetId: 'sn-789', vpcId: 'vpc-def', availabilityZone: 'us-east-1c'), + new Subnet(subnetId: 'sn-ant', vpcId: 'vpc-custom', availabilityZone: 'us-east-1a', + tags: [new Tag(key: 'immutable_metadata', value: '{"purpose":"external","target":"elb"}')]), + new Subnet(subnetId: 'sn-bat', vpcId: 'vpc-custom', availabilityZone: 'us-east-1b', + tags: [new Tag(key: 'immutable_metadata', value: '{"purpose":"external","target":"elb"}')]), + new Subnet(subnetId: 'sn-cat', vpcId: 'vpc-custom', availabilityZone: 'us-east-1c', + tags: [new Tag(key: 'immutable_metadata', value: '{"purpose":"external","target":"elb"}')]), + ]) + + when: + controller.update() + + then: + flash.message == "Load Balancer 'hello' health check has been updated. " + 1 * awsLoadBalancerService.getLoadBalancer(_, 'hello') >> new LoadBalancerDescription( + availabilityZones: ['us-east-1a', 'us-east-1b'], subnets: ['sn-ant', 'sn-bat']) + 1 * awsEc2Service.getDefaultVpcSubnetIds(_) >> defaultVpcSubnetIds + 1 * awsEc2Service.getSubnets(_) >> allSubnets + + 1 * awsLoadBalancerService.updateSubnets(_, 'hello', ['sn-ant', 'sn-bat'], ['sn-bat', 'sn-cat']) + 1 * awsLoadBalancerService.configureHealthCheck(_, 'hello', new HealthCheck(target: 'HTTP:8080/', + interval: 5, timeout: 10, unhealthyThreshold: 3, healthyThreshold: 4)) + 0 * _ + + where: + defaultVpcSubnetIds | defaultVpcSubnetsCondition + [] | 'do not exist and load balancer has subnets' + ['sn-123', 'sn-456', 'sn-789'] | 'exist but load balancer has other subnets' + } + def 'addListener should fail without instance port'() { final cmd = new AddListenerCommand(name: 'app--test') cmd.validate() diff --git a/test/unit/com/netflix/asgard/model/SubnetsSpec.groovy b/test/unit/com/netflix/asgard/model/SubnetsSpec.groovy index 48bcae13..afaf7b2b 100644 --- a/test/unit/com/netflix/asgard/model/SubnetsSpec.groovy +++ b/test/unit/com/netflix/asgard/model/SubnetsSpec.groovy @@ -33,19 +33,26 @@ class SubnetsSpec extends Specification { } Subnets subnets + Subnets subnetsForEc2Classic void setup() { - subnets = new Subnets([ - subnet('subnet-e9b0a3a1', 'us-east-1a', 'internal', SubnetTarget.EC2), - subnet('subnet-e9b0a3a2', 'us-east-1a', 'external', SubnetTarget.EC2), - subnet('subnet-e9b0a3a3', 'us-east-1a', 'internal', SubnetTarget.ELB), - subnet('subnet-e9b0a3a4', 'us-east-1a', 'external', SubnetTarget.ELB), - subnet('subnet-c1e8b2b1', 'us-east-1b', 'internal', SubnetTarget.EC2), - subnet('subnet-c1e8b2b2', 'us-east-1b', 'external', SubnetTarget.EC2), - subnet('subnet-a3770585', 'us-east-1a', null, null, 'vpc-456'), - subnet('subnet-a3770586', 'us-east-1b', null, null, 'vpc-456'), - subnet('subnet-a3770587', 'us-east-1c', null, null, 'vpc-456'), - ]) + + List subnetDatasForEc2Classic = [ + subnet('subnet-e9b0a3a1', 'us-east-1a', 'internal', SubnetTarget.EC2, 'vpc-abcd'), + subnet('subnet-e9b0a3a2', 'us-east-1a', 'external', SubnetTarget.EC2, 'vpc-feed'), + subnet('subnet-e9b0a3a3', 'us-east-1a', 'internal', SubnetTarget.ELB, 'vpc-abcd'), + subnet('subnet-e9b0a3a4', 'us-east-1a', 'external', SubnetTarget.ELB, 'vpc-feed'), + subnet('subnet-c1e8b2b1', 'us-east-1b', 'internal', SubnetTarget.EC2, 'vpc-abcd'), + subnet('subnet-c1e8b2b2', 'us-east-1b', 'external', SubnetTarget.EC2, 'vpc-feed'), + ] + subnetsForEc2Classic = new Subnets(subnetDatasForEc2Classic) + + List subnetDatas = subnetDatasForEc2Classic + [ + subnet('subnet-a3770585', 'us-east-1a', null, null, 'vpc-def'), + subnet('subnet-a3770586', 'us-east-1b', null, null, 'vpc-def'), + subnet('subnet-a3770587', 'us-east-1c', null, null, 'vpc-def'), + ] + subnets = new Subnets(subnetDatas, 'vpc-def') } def 'should create Subnets from AWS objects'() { @@ -83,7 +90,7 @@ class SubnetsSpec extends Specification { def 'should find subnet by ID'() { SubnetData expectedSubnet = new SubnetData(subnetId: 'subnet-e9b0a3a1', availabilityZone: 'us-east-1a', - purpose: 'internal', target: SubnetTarget.EC2, vpcId: 'vpc-1') + purpose: 'internal', target: SubnetTarget.EC2, vpcId: 'vpc-abcd') expect: expectedSubnet == subnets.findSubnetById('subnet-e9b0a3a1') } @@ -98,11 +105,12 @@ class SubnetsSpec extends Specification { def 'should find subnets by VPC ID'() { Subnets expectedSubnets = Subnets.from([ - new Subnet(subnetId: 'subnet-a3770585', availabilityZone: 'us-east-1a', vpcId: 'vpc-456'), - new Subnet(subnetId: 'subnet-a3770586', availabilityZone: 'us-east-1a', vpcId: 'vpc-456'), - new Subnet(subnetId: 'subnet-a3770587', availabilityZone: 'us-east-1a', vpcId: 'vpc-456'), + new Subnet(subnetId: 'subnet-a3770585', availabilityZone: 'us-east-1a', vpcId: 'vpc-def'), + new Subnet(subnetId: 'subnet-a3770586', availabilityZone: 'us-east-1b', vpcId: 'vpc-def'), + new Subnet(subnetId: 'subnet-a3770587', availabilityZone: 'us-east-1c', vpcId: 'vpc-def'), ]) - expect: expectedSubnets == subnets.findSubnetsByVpc('vpc-456') + + expect: expectedSubnets == subnets.findSubnetsByVpc('vpc-def') } def 'should fail when finding subnets by null VPC ID'() { @@ -110,6 +118,46 @@ class SubnetsSpec extends Specification { then: thrown(NullPointerException) } + def 'should get purpose from VPC zone identifier string when default VPC exists'() { + expect: purpose == subnets.getPurposeFromVpcZoneIdentifier(vpcZoneIdentifier) + + where: + vpcZoneIdentifier | purpose + 'subnet-e9b0a3a1,subnet-e9b0a3b1' | 'internal' + 'subnet-e9b0a3a4' | 'external' + 'subnet-a3770585,subnet-a3770586' | null + } + + def 'should get purpose from VPC zone identifier string when default VPC does not exist'() { + expect: purpose == subnetsForEc2Classic.getPurposeFromVpcZoneIdentifier(vpcZoneIdentifier) + + where: + vpcZoneIdentifier | purpose + 'subnet-e9b0a3a1,subnet-e9b0a3b1' | 'internal' + 'subnet-e9b0a3a4' | 'external' + 'subnet-a3770585,subnet-a3770586' | null + } + + def 'should get VPC ID for VPC zone identifier when default VPC exists'() { + expect: vpcId == subnets.getVpcIdForVpcZoneIdentifier(vpcZoneIdentifier) + + where: + vpcZoneIdentifier | vpcId + 'subnet-e9b0a3a1,subnet-e9b0a3b1' | 'vpc-abcd' + 'subnet-e9b0a3a4' | 'vpc-feed' + 'subnet-a3770585,subnet-a3770586' | 'vpc-def' + } + + def 'should get VPC ID for VPC zone identifier when default VPC does not exist'() { + expect: vpcId == subnetsForEc2Classic.getVpcIdForVpcZoneIdentifier(vpcZoneIdentifier) + + where: + vpcZoneIdentifier | vpcId + 'subnet-e9b0a3a1,subnet-e9b0a3b1' | 'vpc-abcd' + 'subnet-e9b0a3a4' | 'vpc-feed' + 'subnet-a3770585,subnet-a3770586' | null + } + def 'should return subnets for zones'() { List zones = ['us-east-1a', 'us-east-1b'] List expectedSubnets = ['subnet-e9b0a3a1', 'subnet-c1e8b2b1'] @@ -381,14 +429,14 @@ class SubnetsSpec extends Specification { } def 'should return subnet for subnet ID'() { - SubnetData expectedSubnet = subnet('subnet-e9b0a3a2', 'us-east-1a', 'external', SubnetTarget.EC2) + SubnetData expectedSubnet = subnet('subnet-e9b0a3a2', 'us-east-1a', 'external', SubnetTarget.EC2, 'vpc-feed') expect: subnets.coerceLoneOrNoneFromIds(['subnet-e9b0a3a2']) == expectedSubnet } def 'should return subnet for first subnet ID if there are multiple'() { - SubnetData expectedSubnet = subnet('subnet-e9b0a3a2', 'us-east-1a', 'external', SubnetTarget.EC2) + SubnetData expectedSubnet = subnet('subnet-e9b0a3a2', 'us-east-1a', 'external', SubnetTarget.EC2, 'vpc-feed') expect: subnets.coerceLoneOrNoneFromIds(['subnet-e9b0a3a2', 'subnet-e9b0a3a1']) == expectedSubnet } @@ -401,6 +449,28 @@ class SubnetsSpec extends Specification { expect: null == subnets.coerceLoneOrNoneFromIds(['subnet-deadbeef']) } + def 'with default VPC, should get the VPC ID for a purpose or get the default VPC ID for empty or null purpose'() { + expect: vpcId == subnets.getVpcIdForSubnetPurpose(purpose) + + where: + vpcId | purpose + 'vpc-feed' | 'external' + 'vpc-abcd' | 'internal' + 'vpc-def' | '' + 'vpc-def' | null + } + + def 'in EC2 classic, should either get the VPC ID for a subnet purpose or null'() { + expect: vpcId == subnetsForEc2Classic.getVpcIdForSubnetPurpose(purpose) + + where: + vpcId | purpose + 'vpc-feed' | 'external' + 'vpc-abcd' | 'internal' + null | '' + null | null + } + def 'should map purpose to VPC ID'() { subnets = new Subnets([ subnet('subnet-e9b0a3a1', 'us-east-1a', 'internal', SubnetTarget.EC2), From 9fc0012638a04d2e6baa3d7395b20621351d70ee Mon Sep 17 00:00:00 2001 From: Joe Sondow Date: Sat, 4 Jan 2014 23:51:21 -0800 Subject: [PATCH 7/7] Release 1.4.1 --- CHANGES.txt | 13 +++++++++++++ application.properties | 2 +- 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/CHANGES.txt b/CHANGES.txt index ff5b7f20..f241a15c 100644 --- a/CHANGES.txt +++ b/CHANGES.txt @@ -1,3 +1,16 @@ +1.4.1 + +Bug Fixes +- Support for updating load balancer in recent AWS region-accounts that have a default VPC +- Support for creating auto scaling groups with load balancers in recent AWS region-accounts that have a default VPC +- Only one auth-revoke task with many IP permissions for each security group pair change +- Zebra-stripe more lists before JavaScript loads + +Infrastructure +- Some code clean up +- More unit tests + + 1.4 Features diff --git a/application.properties b/application.properties index 9fd659e0..67a2c6a6 100644 --- a/application.properties +++ b/application.properties @@ -3,4 +3,4 @@ app.grails.version=2.2.4 app.name=asgard app.servlet.version=2.4 -app.version=1.4 +app.version=1.4.1