Add support for multiple NLB listeners and target groups

pkg/model/awsmodel/api_loadbalancer.go

@@ -109,14 +109,13 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 			"443": {InstancePort: 443},
 		}
 
-		nlbListenerPort := "443"
+		nlbListeners := []*awstasks.NetworkLoadBalancerListener{
-		nlbListeners := map[string]*awstasks.NetworkLoadBalancerListener{
+			{Port: 443},
-			nlbListenerPort: {Port: 443},
 		}
 
 		if lbSpec.SSLCertificate != "" {
 			listeners["443"].SSLCertificateID = lbSpec.SSLCertificate
-			nlbListeners["443"].SSLCertificateID = lbSpec.SSLCertificate
+			nlbListeners[0].SSLCertificateID = lbSpec.SSLCertificate
 		}
 
 		if lbSpec.SecurityGroupOverride != nil {
@@ -138,6 +137,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 			LoadBalancerName: fi.String(loadBalancerName),
 			Subnets:          elbSubnets,
 			Listeners:        nlbListeners,
+			TargetGroups:     make([]*awstasks.TargetGroup, 0),
 
 			Tags: tags,
 			VPC:  b.LinkToVPC(),
@@ -196,19 +196,18 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 			c.AddTask(clb)
 		} else if b.APILoadBalancerClass() == kops.LoadBalancerClassNetwork {
 
-			targetGroupPort := fi.Int64(443)
 			targetGroupName := b.NLBTargetGroupName("api")
-			tags := b.CloudTags(targetGroupName, false)
+			primaryTags := b.CloudTags(targetGroupName, false)
 
 			// Override the returned name to be the expected NLB TG name
-			tags["Name"] = targetGroupName
+			primaryTags["Name"] = targetGroupName
 
 			tg := &awstasks.TargetGroup{
 				Name:               fi.String(targetGroupName),
 				VPC:                b.LinkToVPC(),
-				Tags:               tags,
+				Tags:               primaryTags,
 				Protocol:           fi.String("TCP"),
-				Port:               targetGroupPort,
+				Port:               fi.Int64(443),
 				HealthyThreshold:   fi.Int64(2),
 				UnhealthyThreshold: fi.Int64(2),
 				Shared:             fi.Bool(false),
@@ -216,7 +215,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 
 			c.AddTask(tg)
 
-			nlb.TargetGroup = tg
+			nlb.TargetGroups = append(nlb.TargetGroups, tg)
 
 			c.AddTask(nlb)
 		}
@@ -291,7 +290,6 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 
 			for _, masterGroup := range masterGroups {
 				t := &awstasks.SecurityGroupRule{
-					// TODO: figure out how to only add this to one SG (GetSecurityGroups above returns multiple)
 					Name:          fi.String(fmt.Sprintf("https-api-elb-%s", cidr)),
 					Lifecycle:     b.SecurityLifecycle,
 					CIDR:          fi.String(cidr),
@@ -304,7 +302,6 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error {
 
 				// Allow ICMP traffic required for PMTU discovery
 				c.AddTask(&awstasks.SecurityGroupRule{
-					// TODO: figure out how to only add this to one SG (GetSecurityGroups above returns multiple)
 					Name:          fi.String("icmp-pmtu-api-elb-" + cidr),
 					Lifecycle:     b.SecurityLifecycle,
 					CIDR:          fi.String(cidr),

upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go

@@ -163,12 +163,11 @@ func (e *AutoscalingGroup) Find(c *fi.Context) (*AutoscalingGroup, error) {
 	}
 
 	if len(g.TargetGroupARNs) > 0 {
-		targetGroups := make([]*TargetGroup, 0)
+		actualTGs := make([]*TargetGroup, 0)
 		for _, tg := range g.TargetGroupARNs {
-			targetGroups = append(actual.TargetGroups, &TargetGroup{ARN: aws.String(*tg)})
+			actualTGs = append(actualTGs, &TargetGroup{ARN: aws.String(*tg)})
 		}
-
+		targetGroups, err := ReconcileTargetGroups(c.Cloud.(awsup.AWSCloud), actualTGs, e.TargetGroups)
-		targetGroups, err := ReconcileTargetGroups(c.Cloud.(awsup.AWSCloud), targetGroups, e.TargetGroups)
 		if err != nil {
 			return nil, err
 		}

upup/pkg/fi/cloudup/awstasks/network_load_balancer.go

@@ -55,7 +55,7 @@ type NetworkLoadBalancer struct {
 
 	Subnets []*Subnet
 
-	Listeners map[string]*NetworkLoadBalancerListener
+	Listeners []*NetworkLoadBalancerListener
 
 	Scheme *string
 
@@ -66,8 +66,8 @@ type NetworkLoadBalancer struct {
 
 	Type *string
 
-	VPC         *VPC
+	VPC          *VPC
-	TargetGroup *TargetGroup
+	TargetGroups []*TargetGroup
 }
 
 var _ fi.CompareWithID = &NetworkLoadBalancer{}
@@ -113,6 +113,16 @@ func (e *NetworkLoadBalancerListener) GetDependencies(tasks map[string]fi.Task)
 	return nil
 }
 
+
+// OrderListenersByPort implements sort.Interface for []OrderTargetGroupsByPort, based on port number
+type OrderListenersByPort []*NetworkLoadBalancerListener
+
+func (a OrderListenersByPort) Len() int      { return len(a) }
+func (a OrderListenersByPort) Swap(i, j int) { a[i], a[j] = a[j], a[i] }
+func (a OrderListenersByPort) Less(i, j int) bool {
+	return a[i].Port < a[j].Port
+}
+
 //The load balancer name 'api.renamenlbcluster.k8s.local' can only contain characters that are alphanumeric characters and hyphens(-)\n\tstatus code: 400,
 func findNetworkLoadBalancerByLoadBalancerName(cloud awsup.AWSCloud, loadBalancerName string) (*elbv2.LoadBalancer, error) {
 	request := &elbv2.DescribeLoadBalancersInput{
@@ -317,6 +327,7 @@ func (e *NetworkLoadBalancer) Find(c *fi.Context) (*NetworkLoadBalancer, error)
 	actual.Scheme = lb.Scheme
 	actual.VPC = &VPC{ID: lb.VpcId}
 	actual.Type = lb.Type
+	actual.TargetGroups = make([]*TargetGroup, 0)
 
 	tagMap, err := describeNetworkLoadBalancerTags(cloud, []string{*loadBalancerArn})
 	if err != nil {
@@ -343,35 +354,30 @@ func (e *NetworkLoadBalancer) Find(c *fi.Context) (*NetworkLoadBalancer, error)
 			return nil, fmt.Errorf("error querying for NLB listeners :%v", err)
 		}
 
-		actual.Listeners = make(map[string]*NetworkLoadBalancerListener)
+		actual.Listeners = make([]*NetworkLoadBalancerListener, 0)
 
 		for _, l := range response.Listeners {
-			loadBalancerPort := strconv.FormatInt(aws.Int64Value(l.Port), 10)
-
 			actualListener := &NetworkLoadBalancerListener{}
 			actualListener.Port = int(aws.Int64Value(l.Port))
 			if len(l.Certificates) != 0 {
 				actualListener.SSLCertificateID = aws.StringValue(l.Certificates[0].CertificateArn) // What if there is more then one certificate, can we just grab the default certificate? we don't set it as default, we only set the one.
 			}
-			actual.Listeners[loadBalancerPort] = actualListener
+			actual.Listeners = append(actual.Listeners, actualListener)
 
 			// This will need to be rearranged when we recognized multiple listeners and target groups per NLB
 			if len(l.DefaultActions) > 0 {
 				targetGroupARN := l.DefaultActions[0].TargetGroupArn
 				if targetGroupARN != nil {
-					actual.TargetGroup = &TargetGroup{ARN: targetGroupARN}
+					actual.TargetGroups = append(actual.TargetGroups, &TargetGroup{ARN: targetGroupARN})
 				}
 			}
 		}
-		// This will be cleaned up once the NLB task supports multiple target groups
+		if len(actual.TargetGroups) > 0 {
-		if actual.TargetGroup != nil {
+			targetGroups, err := ReconcileTargetGroups(c.Cloud.(awsup.AWSCloud), actual.TargetGroups, e.TargetGroups)
-			actualTGs := []*TargetGroup{actual.TargetGroup}
-			expectedTGs := []*TargetGroup{e.TargetGroup}
-			targetGroups, err := ReconcileTargetGroups(c.Cloud.(awsup.AWSCloud), actualTGs, expectedTGs)
 			if err != nil {
 				return nil, err
 			}
-			actual.TargetGroup = targetGroups[0]
+			actual.TargetGroups = targetGroups
 		}
 
 	}
@@ -465,6 +471,8 @@ func (e *NetworkLoadBalancer) Run(c *fi.Context) error {
 func (e *NetworkLoadBalancer) Normalize() {
 	// We need to sort our arrays consistently, so we don't get spurious changes
 	sort.Stable(OrderSubnetsById(e.Subnets))
+	sort.Stable(OrderListenersByPort(e.Listeners))
+	sort.Stable(OrderTargetGroupsByPort(e.TargetGroups))
 }
 
 func (s *NetworkLoadBalancer) CheckChanges(a, e, changes *NetworkLoadBalancer) error {
@@ -493,12 +501,18 @@ func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Ne
 	var loadBalancerName string
 	var loadBalancerArn string
 
+	if len(e.Listeners) != len(e.TargetGroups) {
+		return fmt.Errorf("nlb listeners and target groups do not match: %v listeners vs %v target groups", len(e.Listeners), len(e.TargetGroups))
+	}
+
 	if a == nil {
 		if e.LoadBalancerName == nil {
 			return fi.RequiredField("LoadBalancerName")
 		}
-		if e.TargetGroup.ARN == nil {
+		for _, tg := range e.TargetGroups {
-			return fmt.Errorf("missing required target group ARN for NLB creation %v", e.TargetGroup)
+			if tg.ARN == nil {
+				return fmt.Errorf("missing required target group ARN for NLB creation %v", tg)
+			}
 		}
 
 		loadBalancerName = *e.LoadBalancerName
@@ -532,8 +546,8 @@ func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Ne
 		}
 
 		{
-			for _, listener := range e.Listeners {
+			for i, listener := range e.Listeners {
-				createListenerInput := listener.mapToAWS(*e.TargetGroup.ARN, loadBalancerArn)
+				createListenerInput := listener.mapToAWS(*e.TargetGroups[i].ARN, loadBalancerArn)
 
 				klog.V(2).Infof("Creating Listener for NLB")
 				_, err := t.Cloud.ELBV2().CreateListener(createListenerInput)
@@ -605,8 +619,8 @@ func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Ne
 				}
 			}
 
-			for _, listener := range changes.Listeners {
+			for i, listener := range changes.Listeners {
-				awsListener := listener.mapToAWS(*e.TargetGroup.ARN, loadBalancerArn)
+				awsListener := listener.mapToAWS(*e.TargetGroups[i].ARN, loadBalancerArn)
 
 				klog.V(2).Infof("Creating Listener for NLB")
 				_, err := t.Cloud.ELBV2().CreateListener(awsListener)
@@ -674,18 +688,14 @@ func (_ *NetworkLoadBalancer) RenderTerraform(t *terraform.TerraformTarget, a, e
 		return err
 	}
 
-	for portStr, listener := range e.Listeners {
+	for i, listener := range e.Listeners {
-		port, err := strconv.Atoi(portStr)
-		if err != nil {
-			return err
-		}
 		listenerTF := &terraformNetworkLoadBalancerListener{
 			LoadBalancer: e.TerraformLink(),
-			Port:         int64(port),
+			Port:         int64(listener.Port),
 			DefaultAction: []terraformNetworkLoadBalancerListenerAction{
 				{
 					Type:           elbv2.ActionTypeEnumForward,
-					TargetGroupARN: e.TargetGroup.TerraformLink(),
+					TargetGroupARN: e.TargetGroups[i].TerraformLink(),
 				},
 			},
 		}
@@ -696,7 +706,7 @@ func (_ *NetworkLoadBalancer) RenderTerraform(t *terraform.TerraformTarget, a, e
 			listenerTF.Protocol = elbv2.ProtocolEnumTcp
 		}
 
-		err = t.RenderResource("aws_lb_listener", fmt.Sprintf("%v-%v", *e.Name, portStr), listenerTF)
+		err = t.RenderResource("aws_lb_listener", fmt.Sprintf("%v-%v", *e.Name, listener.Port), listenerTF)
 		if err != nil {
 			return err
 		}
@@ -753,18 +763,14 @@ func (_ *NetworkLoadBalancer) RenderCloudformation(t *cloudformation.Cloudformat
 		return err
 	}
 
-	for portStr, listener := range e.Listeners {
+	for i, listener := range e.Listeners {
-		port, err := strconv.Atoi(portStr)
-		if err != nil {
-			return err
-		}
 		listenerCF := &cloudformationNetworkLoadBalancerListener{
 			LoadBalancerARN: e.CloudformationLink(),
-			Port:            int64(port),
+			Port:            int64(listener.Port),
 			DefaultActions: []cloudformationNetworkLoadBalancerListenerAction{
 				{
 					Type:           elbv2.ActionTypeEnumForward,
-					TargetGroupARN: e.TargetGroup.CloudformationLink(),
+					TargetGroupARN: e.TargetGroups[i].CloudformationLink(),
 				},
 			},
 		}
@@ -775,7 +781,7 @@ func (_ *NetworkLoadBalancer) RenderCloudformation(t *cloudformation.Cloudformat
 			listenerCF.Protocol = elbv2.ProtocolEnumTcp
 		}
 
-		err = t.RenderResource("AWS::ElasticLoadBalancingV2::Listener", fmt.Sprintf("%v-%v", *e.Name, portStr), listenerCF)
+		err = t.RenderResource("AWS::ElasticLoadBalancingV2::Listener", fmt.Sprintf("%v-%v", *e.Name, listener.Port), listenerCF)
 		if err != nil {
 			return err
 		}
@@ -794,5 +800,4 @@ func (e *NetworkLoadBalancer) CloudformationAttrCanonicalHostedZoneNameID() *clo
 
 func (e *NetworkLoadBalancer) CloudformationAttrDNSName() *cloudformation.Literal {
 	return cloudformation.GetAtt("AWS::ElasticLoadBalancingV2::LoadBalancer", *e.Name, "DNSName")
-
 }

upup/pkg/fi/cloudup/awstasks/targetgroup.go

@@ -191,6 +191,15 @@ func (_ *TargetGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *TargetGrou
 	return nil
 }
 
+// OrderTargetGroupsByPort implements sort.Interface for []OrderTargetGroupsByPort, based on port number
+type OrderTargetGroupsByPort []*TargetGroup
+
+func (a OrderTargetGroupsByPort) Len() int      { return len(a) }
+func (a OrderTargetGroupsByPort) Swap(i, j int) { a[i], a[j] = a[j], a[i] }
+func (a OrderTargetGroupsByPort) Less(i, j int) bool {
+	return fi.Int64Value(a[i].Port) < fi.Int64Value(a[j].Port)
+}
+
 // pkg/model/awsmodel doesn't know the ARN of the API TargetGroup tasks that it passes to the master ASGs,
 // it only knows the ARN of external target groups passed through the InstanceGroupSpec.
 // We lookup the ARN for TargetGroup tasks that don't have it set in order to attach the LB to the ASG.
@@ -205,12 +214,12 @@ func (_ *TargetGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *TargetGrou
 // Because we don't know whether any given ARN attached to an ASG is an API TargetGroup task or not,
 // we have to find the API TargetGroup task, lookup its ARN, then compare that to the list of attached TargetGroups.
 func ReconcileTargetGroups(cloud awsup.AWSCloud, actual []*TargetGroup, expected []*TargetGroup) ([]*TargetGroup, error) {
-	var apiTGTask *TargetGroup
+	apiTGTasks := make([]*TargetGroup, 0)
 	for _, tg := range expected {
 		// All external TargetGroups have their Shared field set to true. The API TargetGroups do not.
 		// Note that Shared is set by the kops model rather than AWS tags.
 		if !fi.BoolValue(tg.Shared) {
-			apiTGTask = tg
+			apiTGTasks = append(apiTGTasks, tg)
 		}
 	}
 
@@ -220,18 +229,17 @@ func ReconcileTargetGroups(cloud awsup.AWSCloud, actual []*TargetGroup, expected
 		return reconciled, nil
 	}
 
-	var apiTG *elbv2.TargetGroup
+	apiTGs := make(map[string]*TargetGroup)
-	var err error
+	for _, task := range apiTGTasks {
-	if apiTGTask != nil {
+		apiTG, err := FindTargetGroupByName(cloud, fi.StringValue(task.Name))
-		apiTG, err = FindTargetGroupByName(cloud, fi.StringValue(apiTGTask.Name))
 		if err != nil {
 			return nil, err
 		}
+		apiTGs[aws.StringValue(apiTG.TargetGroupArn)] = task
 	}
-	for i := 0; i < len(actual); i++ {
+	for _, tg := range actual {
-		tg := actual[i]
+		if apiTask, ok := apiTGs[aws.StringValue(tg.ARN)]; ok {
-		if apiTG != nil && aws.StringValue(apiTG.TargetGroupArn) == aws.StringValue(tg.ARN) {
+			reconciled = append(reconciled, apiTask)
-			reconciled = append(reconciled, apiTGTask)
 		} else {
 			reconciled = append(reconciled, tg)
 		}