Merge pull request #455 from joesondow/fix-for-default-vpc-account

Fix for default vpc account

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

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

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

grails-app/controllers/com/netflix/asgard/ClusterController.groovy

@@ -136,7 +136,7 @@ class ClusterController {
                     Subnets subnets = awsEc2Service.getSubnets(userContext)
                     List<String> subnetIds = Relationships.subnetIdsFromVpcZoneIdentifier(lastGroup.vpcZoneIdentifier)
                     String subnetPurpose = subnets.coerceLoneOrNoneFromIds(subnetIds)?.purpose
-                    String vpcId = subnets.mapPurposeToVpcId()[subnetPurpose] ?: ''
+                    String vpcId = subnets.getVpcIdForSubnetPurpose(subnetPurpose) ?: ''
                     List<String> 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<String> subnetIds = Relationships.subnetIdsFromVpcZoneIdentifier(lastGroup.vpcZoneIdentifier)
         String subnetPurpose = subnets.coerceLoneOrNoneFromIds(subnetIds)?.purpose
-        String vpcId = subnets.mapPurposeToVpcId()[subnetPurpose] ?: ''
+        String vpcId = subnets.getVpcIdForSubnetPurpose(subnetPurpose) ?: ''
         List<String> selectedLoadBalancers = Requests.ensureList(
                 params["selectedLoadBalancersForVpcId${vpcId}"]) ?: lastGroup.loadBalancerNames
         List<String> 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<String> loadBalancerNames = Requests.ensureList(params["selectedLoadBalancersForVpcId${vpcId}"] ?:
             params["selectedLoadBalancers"])
 
@@ -375,7 +375,7 @@ ${lastGroup.loadBalancerNames}"""
             List<String> 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<String> loadBalancerNames = Requests.ensureList(params["selectedLoadBalancersForVpcId${vpcId}"] ?:
                     params["selectedLoadBalancers"])
             // Availability zones default to the last group's value since this field is required.

grails-app/controllers/com/netflix/asgard/LoadBalancerController.groovy

@@ -106,16 +106,15 @@ class LoadBalancerController {
         Collection<String> selectedZones = awsEc2Service.preselectedZoneNames(availabilityZones,
                 Requests.ensureList(params.selectedZones))
         Subnets subnets = awsEc2Service.getSubnets(userContext)
-        Map<String, String> purposeToVpcId = subnets.mapPurposeToVpcId()
         [
             applications: applicationService.getRegisteredApplicationsForLoadBalancer(userContext),
             stacks: stackService.getStacks(userContext),
             subnetPurpose: params.subnetPurpose ?: null,
             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),
         ]
@@ -216,7 +215,8 @@ class LoadBalancerController {
         LoadBalancerDescription lb = awsLoadBalancerService.getLoadBalancer(userContext, name)
 
         List<String> zoneList = Requests.ensureList(params.selectedZones)
-        if (lb.subnets) {
+        List<String> defaultVpcSubnetIds = awsEc2Service.getDefaultVpcSubnetIds(userContext)
+        if (lb.subnets.any { !(it in defaultVpcSubnetIds) }) {
             updateLbSubnets(userContext, name, zoneList, lb.subnets)
         } else {
             updateLbZones(userContext, name, zoneList, lb.availabilityZones)

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))
     }
 
     /**

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<SecurityGroup> effectiveSecurityGroups = awsEc2Service.getEffectiveSecurityGroups(userContext)
-        List<String> subnetIds = Relationships.subnetIdsFromVpcZoneIdentifier(group.VPCZoneIdentifier)
-        String vpcId = subnets.coerceLoneOrNoneFromIds(subnetIds)?.vpcId
+        String vpcId = subnets.getVpcIdForVpcZoneIdentifier(group.VPCZoneIdentifier)
         Map<String, String> purposeToVpcId = subnets.mapPurposeToVpcId()
         String pricing = lc.spotPrice ? InstancePriceType.SPOT.name() : InstancePriceType.ON_DEMAND.name()
         Map<String, Object> result = [

src/groovy/com/netflix/asgard/model/Subnets.groovy

@@ -30,20 +30,25 @@ import groovy.transform.Canonical
  */
 @Canonical class Subnets {
     /** All of the subnets contained in this object. */
-    final private Collection<SubnetData> allSubnets
+    final Collection<SubnetData> allSubnets
 
-    private Subnets(Collection<SubnetData> allSubnets) {
+    /** The identifier of the default VPC of the account-region, if available. */
+    final String defaultVpcId
+
+    private Subnets(Collection<SubnetData> allSubnets, String defaultVpcId = null) {
+        this.defaultVpcId = defaultVpcId
         this.allSubnets = ImmutableSet.copyOf(allSubnets)
     }
 
     /**
-     * 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<Subnet> subnets) {
-        new Subnets(subnets.collect() { SubnetData.from(it) })
+    public static Subnets from(Collection<Subnet> subnets, String defaultVpcId = null) {
+        new Subnets(subnets.collect() { SubnetData.from(it) }, defaultVpcId)
     }
 
     /**
@@ -56,7 +61,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 +83,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 +94,19 @@ import groovy.transform.Canonical
     }
 
     /**
-     * Find the subnet IDs that map to specific zones
+     * 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<String> subnetIds = Relationships.subnetIdsFromVpcZoneIdentifier(vpcZoneIdentifier)
+        coerceLoneOrNoneFromIds(subnetIds)?.vpcId ?: defaultVpcId
+    }
+
+    /**
+     * 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 +131,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 +153,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)
@@ -160,6 +177,18 @@ 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]
+    }
+
     /**
      * 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 +200,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<SubnetData> subnets = entry.value as List
@@ -185,7 +217,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 +236,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 +248,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

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'),

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<Listener> 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<String> oldSubnets = ['subnet-101', 'subnet-102', 'subnet-103']
         Collection<String> newSubnets = ['subnet-103', 'subnet-104']