From 6c5b2fc58f1783fde659aca2834f1183f33605ba Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Mon, 2 Nov 2020 12:30:29 -0600 Subject: [PATCH 01/12] Add support for multiple NLB listeners and target groups --- pkg/model/awsmodel/api_loadbalancer.go | 21 +++-- .../fi/cloudup/awstasks/autoscalinggroup.go | 7 +- .../cloudup/awstasks/network_load_balancer.go | 79 ++++++++++--------- upup/pkg/fi/cloudup/awstasks/targetgroup.go | 28 ++++--- 4 files changed, 72 insertions(+), 63 deletions(-) diff --git a/pkg/model/awsmodel/api_loadbalancer.go b/pkg/model/awsmodel/api_loadbalancer.go index cca9fddd89..2e4439665c 100644 --- a/pkg/model/awsmodel/api_loadbalancer.go +++ b/pkg/model/awsmodel/api_loadbalancer.go @@ -109,14 +109,13 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { "443": {InstancePort: 443}, } - nlbListenerPort := "443" - nlbListeners := map[string]*awstasks.NetworkLoadBalancerListener{ - nlbListenerPort: {Port: 443}, + nlbListeners := []*awstasks.NetworkLoadBalancerListener{ + {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), diff --git a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go b/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go index e1575639d7..130d27c989 100644 --- a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go +++ b/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), targetGroups, e.TargetGroups) + targetGroups, err := ReconcileTargetGroups(c.Cloud.(awsup.AWSCloud), actualTGs, e.TargetGroups) if err != nil { return nil, err } diff --git a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go index 33affe89fc..2f6bd5192c 100644 --- a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go +++ b/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 - TargetGroup *TargetGroup + VPC *VPC + 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 actual.TargetGroup != nil { - actualTGs := []*TargetGroup{actual.TargetGroup} - expectedTGs := []*TargetGroup{e.TargetGroup} - targetGroups, err := ReconcileTargetGroups(c.Cloud.(awsup.AWSCloud), actualTGs, expectedTGs) + if len(actual.TargetGroups) > 0 { + targetGroups, err := ReconcileTargetGroups(c.Cloud.(awsup.AWSCloud), actual.TargetGroups, e.TargetGroups) 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 { - return fmt.Errorf("missing required target group ARN for NLB creation %v", e.TargetGroup) + for _, tg := range e.TargetGroups { + 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 { - createListenerInput := listener.mapToAWS(*e.TargetGroup.ARN, loadBalancerArn) + for i, listener := range e.Listeners { + 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 { - awsListener := listener.mapToAWS(*e.TargetGroup.ARN, loadBalancerArn) + for i, listener := range changes.Listeners { + 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 { - port, err := strconv.Atoi(portStr) - if err != nil { - return err - } + for i, listener := range e.Listeners { 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 { - port, err := strconv.Atoi(portStr) - if err != nil { - return err - } + for i, listener := range e.Listeners { 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") - } diff --git a/upup/pkg/fi/cloudup/awstasks/targetgroup.go b/upup/pkg/fi/cloudup/awstasks/targetgroup.go index cd26112479..07d54ebcbf 100644 --- a/upup/pkg/fi/cloudup/awstasks/targetgroup.go +++ b/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 - var err error - if apiTGTask != nil { - apiTG, err = FindTargetGroupByName(cloud, fi.StringValue(apiTGTask.Name)) + apiTGs := make(map[string]*TargetGroup) + for _, task := range apiTGTasks { + apiTG, err := FindTargetGroupByName(cloud, fi.StringValue(task.Name)) if err != nil { return nil, err } + apiTGs[aws.StringValue(apiTG.TargetGroupArn)] = task } - for i := 0; i < len(actual); i++ { - tg := actual[i] - if apiTG != nil && aws.StringValue(apiTG.TargetGroupArn) == aws.StringValue(tg.ARN) { - reconciled = append(reconciled, apiTGTask) + for _, tg := range actual { + if apiTask, ok := apiTGs[aws.StringValue(tg.ARN)]; ok { + reconciled = append(reconciled, apiTask) } else { reconciled = append(reconciled, tg) } From 6357cc45c8d4745a4796ff4303dedeeff6c3f9e0 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Mon, 2 Nov 2020 12:51:22 -0600 Subject: [PATCH 02/12] Fix cloudformation NLB listener certificate rendering --- .../cloudup/awstasks/network_load_balancer.go | 19 ++++++++++++------- 1 file changed, 12 insertions(+), 7 deletions(-) diff --git a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go index 2f6bd5192c..b658cd69c3 100644 --- a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go +++ b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go @@ -113,7 +113,6 @@ func (e *NetworkLoadBalancerListener) GetDependencies(tasks map[string]fi.Task) return nil } - // OrderListenersByPort implements sort.Interface for []OrderTargetGroupsByPort, based on port number type OrderListenersByPort []*NetworkLoadBalancerListener @@ -731,11 +730,15 @@ type cloudformationNetworkLoadBalancer struct { } type cloudformationNetworkLoadBalancerListener struct { - Certificates []string `json:"Certificates,omitempty"` - DefaultActions []cloudformationNetworkLoadBalancerListenerAction `json:"DefaultActions"` - LoadBalancerARN *cloudformation.Literal `json:"LoadBalancerArn"` - Port int64 `json:"Port"` - Protocol string `json:"Protocol"` + Certificates []cloudformationNetworkLoadBalancerListenerCertificate `json:"Certificates,omitempty"` + DefaultActions []cloudformationNetworkLoadBalancerListenerAction `json:"DefaultActions"` + LoadBalancerARN *cloudformation.Literal `json:"LoadBalancerArn"` + Port int64 `json:"Port"` + Protocol string `json:"Protocol"` +} + +type cloudformationNetworkLoadBalancerListenerCertificate struct { + CertificateArn string `json:"CertificateArn"` } type cloudformationNetworkLoadBalancerListenerAction struct { @@ -775,7 +778,9 @@ func (_ *NetworkLoadBalancer) RenderCloudformation(t *cloudformation.Cloudformat }, } if listener.SSLCertificateID != "" { - listenerCF.Certificates = []string{listener.SSLCertificateID} + listenerCF.Certificates = []cloudformationNetworkLoadBalancerListenerCertificate{ + {CertificateArn: listener.SSLCertificateID}, + } listenerCF.Protocol = elbv2.ProtocolEnumTls } else { listenerCF.Protocol = elbv2.ProtocolEnumTcp From 9242c34a3829d528e0a0a206b167bb21e76eec72 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Mon, 2 Nov 2020 12:31:05 -0600 Subject: [PATCH 03/12] Setup a second NLB listener on 8443 when sslCertificate is set --- pkg/apis/kops/validation/validation.go | 6 +++ pkg/model/awsmodel/api_loadbalancer.go | 51 ++++++++++++++++++- pkg/model/awsmodel/autoscalinggroup.go | 3 ++ pkg/model/firewall.go | 1 + pkg/model/names.go | 2 +- .../cloudup/awstasks/network_load_balancer.go | 39 ++++++++++---- 6 files changed, 88 insertions(+), 14 deletions(-) diff --git a/pkg/apis/kops/validation/validation.go b/pkg/apis/kops/validation/validation.go index 4ed2471ae2..8bb3538f3f 100644 --- a/pkg/apis/kops/validation/validation.go +++ b/pkg/apis/kops/validation/validation.go @@ -217,6 +217,12 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie if featureflag.Spotinst.Enabled() && spec.API.LoadBalancer.Class == kops.LoadBalancerClassNetwork { allErrs = append(allErrs, field.Forbidden(fieldPath, "cannot use NLB together with spotinst")) } + if spec.API.LoadBalancer.SSLCertificate != "" && spec.API.LoadBalancer.Class != kops.LoadBalancerClassNetwork && c.IsKubernetesGTE("1.19") { + allErrs = append(allErrs, field.Forbidden(fieldPath, "sslCertificate requires network loadbalancer for K8s 1.19+ see ")) + } + if spec.API.LoadBalancer.Class == kops.LoadBalancerClassNetwork && spec.API.LoadBalancer.UseForInternalApi && spec.API.LoadBalancer.Type == kops.LoadBalancerTypeInternal { + allErrs = append(allErrs, field.Forbidden(fieldPath, "useForInternalApi cannot be used with internal NLB due lack of hairpinning support")) + } } return allErrs diff --git a/pkg/model/awsmodel/api_loadbalancer.go b/pkg/model/awsmodel/api_loadbalancer.go index 2e4439665c..47d507b98c 100644 --- a/pkg/model/awsmodel/api_loadbalancer.go +++ b/pkg/model/awsmodel/api_loadbalancer.go @@ -110,12 +110,22 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { } nlbListeners := []*awstasks.NetworkLoadBalancerListener{ - {Port: 443}, + { + Port: 443, + TargetGroupName: b.NLBTargetGroupName("api"), + }, } if lbSpec.SSLCertificate != "" { listeners["443"].SSLCertificateID = lbSpec.SSLCertificate nlbListeners[0].SSLCertificateID = lbSpec.SSLCertificate + nlbListeners = append(nlbListeners, &awstasks.NetworkLoadBalancerListener{ + Port: 8443, + TargetGroupName: b.NLBTargetGroupName("tcp"), + }) + klog.Info("Using ACM certificate for API ELB") + } else { + klog.Info("NOT using ACM certificate for API ELB") } if lbSpec.SecurityGroupOverride != nil { @@ -202,11 +212,15 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { // Override the returned name to be the expected NLB TG name primaryTags["Name"] = targetGroupName + protocol := fi.String("TCP") + if lbSpec.SSLCertificate != "" { + protocol = fi.String("TLS") + } tg := &awstasks.TargetGroup{ Name: fi.String(targetGroupName), VPC: b.LinkToVPC(), Tags: primaryTags, - Protocol: fi.String("TCP"), + Protocol: protocol, Port: fi.Int64(443), HealthyThreshold: fi.Int64(2), UnhealthyThreshold: fi.Int64(2), @@ -217,6 +231,26 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { nlb.TargetGroups = append(nlb.TargetGroups, tg) + if lbSpec.SSLCertificate != "" { + secondaryTags := b.CloudTags(targetGroupName, false) + secondaryName := b.NLBTargetGroupName("tcp") + + // Override the returned name to be the expected NLB TG name + secondaryTags["Name"] = secondaryName + secondaryTG := &awstasks.TargetGroup{ + Name: fi.String(secondaryName), + VPC: b.LinkToVPC(), + Tags: secondaryTags, + Protocol: fi.String("TCP"), + Port: fi.Int64(443), + HealthyThreshold: fi.Int64(2), + UnhealthyThreshold: fi.Int64(2), + Shared: fi.Bool(false), + } + c.AddTask(secondaryTG) + nlb.TargetGroups = append(nlb.TargetGroups, secondaryTG) + } + c.AddTask(nlb) } @@ -310,6 +344,19 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { SecurityGroup: masterGroup.Task, ToPort: fi.Int64(4), }) + + if b.Cluster.Spec.API != nil && b.Cluster.Spec.API.LoadBalancer != nil && b.Cluster.Spec.API.LoadBalancer.SSLCertificate != "" { + // Allow access to masters on secondary port through NLB + c.AddTask(&awstasks.SecurityGroupRule{ + Name: fi.String(fmt.Sprintf("tcp-api-%s", cidr)), + Lifecycle: b.SecurityLifecycle, + CIDR: fi.String(cidr), + FromPort: fi.Int64(8443), + Protocol: fi.String("tcp"), + SecurityGroup: masterGroup.Task, + ToPort: fi.Int64(8443), + }) + } } } } diff --git a/pkg/model/awsmodel/autoscalinggroup.go b/pkg/model/awsmodel/autoscalinggroup.go index 95b7608cc3..356ba8cc5e 100644 --- a/pkg/model/awsmodel/autoscalinggroup.go +++ b/pkg/model/awsmodel/autoscalinggroup.go @@ -371,6 +371,9 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.ModelBuil if b.UseLoadBalancerForAPI() && ig.Spec.Role == kops.InstanceGroupRoleMaster { if b.UseNetworkLoadBalancer() { t.TargetGroups = append(t.TargetGroups, b.LinkToTargetGroup("api")) + if b.Cluster.Spec.API.LoadBalancer.SSLCertificate != "" { + t.TargetGroups = append(t.TargetGroups, b.LinkToTargetGroup("tcp")) + } } else { t.LoadBalancers = append(t.LoadBalancers, b.LinkToCLB("api")) } diff --git a/pkg/model/firewall.go b/pkg/model/firewall.go index 2e834d7b0f..d331611cfa 100644 --- a/pkg/model/firewall.go +++ b/pkg/model/firewall.go @@ -418,6 +418,7 @@ func (b *KopsModelContext) GetSecurityGroups(role kops.InstanceGroupRole) ([]Sec "port=4002", // etcd events "port=4789", // VXLAN "port=179", // Calico + "port=8443", // k8s api secondary listener // TODO: UDP vs TCP // TODO: Protocol 4 for calico diff --git a/pkg/model/names.go b/pkg/model/names.go index fc46d232a3..80aaa67da4 100644 --- a/pkg/model/names.go +++ b/pkg/model/names.go @@ -105,7 +105,7 @@ func (b *KopsModelContext) LinkToNLB(prefix string) *awstasks.NetworkLoadBalance } func (b *KopsModelContext) LinkToTargetGroup(prefix string) *awstasks.TargetGroup { - name := b.NLBTargetGroupName(prefix) // TODO: this will need to change for the ACM cert bugfix since we'll have multiple TGs + name := b.NLBTargetGroupName(prefix) return &awstasks.TargetGroup{Name: &name} } diff --git a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go index b658cd69c3..6535f42f12 100644 --- a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go +++ b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go @@ -78,15 +78,25 @@ func (e *NetworkLoadBalancer) CompareWithID() *string { type NetworkLoadBalancerListener struct { Port int + TargetGroupName string SSLCertificateID string } -func (e *NetworkLoadBalancerListener) mapToAWS(targetGroupArn string, loadBalancerArn string) *elbv2.CreateListenerInput { +func (e *NetworkLoadBalancerListener) mapToAWS(targetGroups []*TargetGroup, loadBalancerArn string) (*elbv2.CreateListenerInput, error) { + var tgARN string + for _, tg := range targetGroups { + if fi.StringValue(tg.Name) == e.TargetGroupName { + tgARN = fi.StringValue(tg.ARN) + } + } + if tgARN == "" { + return nil, fmt.Errorf("target group not found for NLB listener %+v", e) + } l := &elbv2.CreateListenerInput{ DefaultActions: []*elbv2.Action{ { - TargetGroupArn: aws.String(targetGroupArn), + TargetGroupArn: aws.String(tgARN), Type: aws.String(elbv2.ActionTypeEnumForward), }, }, @@ -104,7 +114,7 @@ func (e *NetworkLoadBalancerListener) mapToAWS(targetGroupArn string, loadBalanc l.Protocol = aws.String(elbv2.ProtocolEnumTcp) } - return l + return l, nil } var _ fi.HasDependencies = &NetworkLoadBalancerListener{} @@ -545,11 +555,14 @@ func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Ne } { - for i, listener := range e.Listeners { - createListenerInput := listener.mapToAWS(*e.TargetGroups[i].ARN, loadBalancerArn) + for _, listener := range e.Listeners { + createListenerInput, err := listener.mapToAWS(e.TargetGroups, loadBalancerArn) + if err != nil { + return err + } - klog.V(2).Infof("Creating Listener for NLB") - _, err := t.Cloud.ELBV2().CreateListener(createListenerInput) + klog.V(2).Infof("Creating Listener for NLB with port %v", listener.Port) + _, err = t.Cloud.ELBV2().CreateListener(createListenerInput) if err != nil { return fmt.Errorf("error creating listener for NLB: %v", err) } @@ -618,11 +631,15 @@ func (_ *NetworkLoadBalancer) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Ne } } - for i, listener := range changes.Listeners { - awsListener := listener.mapToAWS(*e.TargetGroups[i].ARN, loadBalancerArn) + for _, listener := range changes.Listeners { - klog.V(2).Infof("Creating Listener for NLB") - _, err := t.Cloud.ELBV2().CreateListener(awsListener) + awsListener, err := listener.mapToAWS(e.TargetGroups, loadBalancerArn) + if err != nil { + return err + } + + klog.V(2).Infof("Creating Listener for NLB with port %v", listener.Port) + _, err = t.Cloud.ELBV2().CreateListener(awsListener) if err != nil { return fmt.Errorf("error creating NLB listener: %v", err) } From 316c1eec8a38f0dc9b73a59841abbf8cbea1d321 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Mon, 2 Nov 2020 12:31:29 -0600 Subject: [PATCH 04/12] Update complex integration test for ACM cert and second listener --- .../complex/cloudformation.json | 88 ++++++++++++++++++- .../complex/in-legacy-v1alpha2.yaml | 1 + .../update_cluster/complex/in-v1alpha2.yaml | 1 + .../update_cluster/complex/kubernetes.tf | 54 +++++++++++- 4 files changed, 140 insertions(+), 4 deletions(-) diff --git a/tests/integration/update_cluster/complex/cloudformation.json b/tests/integration/update_cluster/complex/cloudformation.json index 0157aa0d22..6dca568f0f 100644 --- a/tests/integration/update_cluster/complex/cloudformation.json +++ b/tests/integration/update_cluster/complex/cloudformation.json @@ -87,6 +87,9 @@ "TargetGroupARNs": [ { "Ref": "AWSElasticLoadBalancingV2TargetGroupapicomplexexamplecomvd3t5n" + }, + { + "Ref": "AWSElasticLoadBalancingV2TargetGrouptcpcomplexexamplecomvpjolq" } ] } @@ -850,6 +853,30 @@ "CidrIpv6": "2001:0:85a3::/48" } }, + "AWSEC2SecurityGroupIngresstcpapi111024": { + "Type": "AWS::EC2::SecurityGroupIngress", + "Properties": { + "GroupId": { + "Ref": "AWSEC2SecurityGroupmasterscomplexexamplecom" + }, + "FromPort": 8443, + "ToPort": 8443, + "IpProtocol": "tcp", + "CidrIp": "1.1.1.0/24" + } + }, + "AWSEC2SecurityGroupIngresstcpapi20010850040": { + "Type": "AWS::EC2::SecurityGroupIngress", + "Properties": { + "GroupId": { + "Ref": "AWSEC2SecurityGroupmasterscomplexexamplecom" + }, + "FromPort": 8443, + "ToPort": 8443, + "IpProtocol": "tcp", + "CidrIpv6": "2001:0:8500::/40" + } + }, "AWSEC2SecurityGroupapielbcomplexexamplecom": { "Type": "AWS::EC2::SecurityGroup", "Properties": { @@ -1148,6 +1175,11 @@ "AWSElasticLoadBalancingV2Listenerapicomplexexamplecom443": { "Type": "AWS::ElasticLoadBalancingV2::Listener", "Properties": { + "Certificates": [ + { + "CertificateArn": "arn:aws:acm:us-test-1:000000000000:certificate/123456789012-1234-1234-1234-12345678" + } + ], "DefaultActions": [ { "Type": "forward", @@ -1160,6 +1192,24 @@ "Ref": "AWSElasticLoadBalancingV2LoadBalancerapicomplexexamplecom" }, "Port": 443, + "Protocol": "TLS" + } + }, + "AWSElasticLoadBalancingV2Listenerapicomplexexamplecom8443": { + "Type": "AWS::ElasticLoadBalancingV2::Listener", + "Properties": { + "DefaultActions": [ + { + "Type": "forward", + "TargetGroupArn": { + "Ref": "AWSElasticLoadBalancingV2TargetGrouptcpcomplexexamplecomvpjolq" + } + } + ], + "LoadBalancerArn": { + "Ref": "AWSElasticLoadBalancingV2LoadBalancerapicomplexexamplecom" + }, + "Port": 8443, "Protocol": "TCP" } }, @@ -1203,7 +1253,7 @@ "Properties": { "Name": "api-complex-example-com-vd3t5n", "Port": 443, - "Protocol": "TCP", + "Protocol": "TLS", "VpcId": { "Ref": "AWSEC2VPCcomplexexamplecom" }, @@ -1229,6 +1279,42 @@ "Value": "owned" } ], + "HealthCheckProtocol": "TLS", + "HealthyThresholdCount": 2, + "UnhealthyThresholdCount": 2 + } + }, + "AWSElasticLoadBalancingV2TargetGrouptcpcomplexexamplecomvpjolq": { + "Type": "AWS::ElasticLoadBalancingV2::TargetGroup", + "Properties": { + "Name": "tcp-complex-example-com-vpjolq", + "Port": 443, + "Protocol": "TCP", + "VpcId": { + "Ref": "AWSEC2VPCcomplexexamplecom" + }, + "Tags": [ + { + "Key": "KubernetesCluster", + "Value": "complex.example.com" + }, + { + "Key": "Name", + "Value": "tcp-complex-example-com-vpjolq" + }, + { + "Key": "Owner", + "Value": "John Doe" + }, + { + "Key": "foo/bar", + "Value": "fib+baz" + }, + { + "Key": "kubernetes.io/cluster/complex.example.com", + "Value": "owned" + } + ], "HealthCheckProtocol": "TCP", "HealthyThresholdCount": 2, "UnhealthyThresholdCount": 2 diff --git a/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml b/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml index e4c2eb2a97..1d5d49c890 100644 --- a/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml +++ b/tests/integration/update_cluster/complex/in-legacy-v1alpha2.yaml @@ -12,6 +12,7 @@ spec: - sg-exampleid4 crossZoneLoadBalancing: true class: Network + sslCertificate: arn:aws:acm:us-test-1:000000000000:certificate/123456789012-1234-1234-1234-12345678 kubernetesApiAccess: - 1.1.1.0/24 - 2001:0:8500::/40 diff --git a/tests/integration/update_cluster/complex/in-v1alpha2.yaml b/tests/integration/update_cluster/complex/in-v1alpha2.yaml index f841312b85..12ac5e9233 100644 --- a/tests/integration/update_cluster/complex/in-v1alpha2.yaml +++ b/tests/integration/update_cluster/complex/in-v1alpha2.yaml @@ -12,6 +12,7 @@ spec: - sg-exampleid4 crossZoneLoadBalancing: true class: Network + sslCertificate: arn:aws:acm:us-test-1:000000000000:certificate/123456789012-1234-1234-1234-12345678 kubernetesApiAccess: - 1.1.1.0/24 - 2001:0:8500::/40 diff --git a/tests/integration/update_cluster/complex/kubernetes.tf b/tests/integration/update_cluster/complex/kubernetes.tf index 973cd44baa..0cb76bc1e4 100644 --- a/tests/integration/update_cluster/complex/kubernetes.tf +++ b/tests/integration/update_cluster/complex/kubernetes.tf @@ -135,7 +135,7 @@ resource "aws_autoscaling_group" "master-us-test-1a-masters-complex-example-com" propagate_at_launch = true value = "owned" } - target_group_arns = [aws_lb_target_group.api-complex-example-com-vd3t5n.id] + target_group_arns = [aws_lb_target_group.api-complex-example-com-vd3t5n.id, aws_lb_target_group.tcp-complex-example-com-vpjolq.id] vpc_zone_identifier = [aws_subnet.us-test-1a-complex-example-com.id] } @@ -423,24 +423,35 @@ resource "aws_launch_template" "nodes-complex-example-com" { } resource "aws_lb_listener" "api-complex-example-com-443" { + certificate_arn = "arn:aws:acm:us-test-1:000000000000:certificate/123456789012-1234-1234-1234-12345678" default_action { target_group_arn = aws_lb_target_group.api-complex-example-com-vd3t5n.id type = "forward" } load_balancer_arn = aws_lb.api-complex-example-com.id port = 443 + protocol = "TLS" +} + +resource "aws_lb_listener" "api-complex-example-com-8443" { + default_action { + target_group_arn = aws_lb_target_group.tcp-complex-example-com-vpjolq.id + type = "forward" + } + load_balancer_arn = aws_lb.api-complex-example-com.id + port = 8443 protocol = "TCP" } resource "aws_lb_target_group" "api-complex-example-com-vd3t5n" { health_check { healthy_threshold = 2 - protocol = "TCP" + protocol = "TLS" unhealthy_threshold = 2 } name = "api-complex-example-com-vd3t5n" port = 443 - protocol = "TCP" + protocol = "TLS" tags = { "KubernetesCluster" = "complex.example.com" "Name" = "api-complex-example-com-vd3t5n" @@ -451,6 +462,25 @@ resource "aws_lb_target_group" "api-complex-example-com-vd3t5n" { vpc_id = aws_vpc.complex-example-com.id } +resource "aws_lb_target_group" "tcp-complex-example-com-vpjolq" { + health_check { + healthy_threshold = 2 + protocol = "TCP" + unhealthy_threshold = 2 + } + name = "tcp-complex-example-com-vpjolq" + port = 443 + protocol = "TCP" + tags = { + "KubernetesCluster" = "complex.example.com" + "Name" = "tcp-complex-example-com-vpjolq" + "Owner" = "John Doe" + "foo/bar" = "fib+baz" + "kubernetes.io/cluster/complex.example.com" = "owned" + } + vpc_id = aws_vpc.complex-example-com.id +} + resource "aws_lb" "api-complex-example-com" { enable_cross_zone_load_balancing = true internal = false @@ -716,6 +746,24 @@ resource "aws_security_group_rule" "ssh-external-to-node-2001_0_85a3__--48" { type = "ingress" } +resource "aws_security_group_rule" "tcp-api-1-1-1-0--24" { + cidr_blocks = ["1.1.1.0/24"] + from_port = 8443 + protocol = "tcp" + security_group_id = aws_security_group.masters-complex-example-com.id + to_port = 8443 + type = "ingress" +} + +resource "aws_security_group_rule" "tcp-api-2001_0_8500__--40" { + cidr_blocks = ["2001:0:8500::/40"] + from_port = 8443 + protocol = "tcp" + security_group_id = aws_security_group.masters-complex-example-com.id + to_port = 8443 + type = "ingress" +} + resource "aws_security_group" "api-elb-complex-example-com" { description = "Security group for api ELB" name = "api-elb.complex.example.com" From 30f3d149796b7596c649ceeeddc2149816a2e772 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Sat, 15 Aug 2020 14:34:39 -0500 Subject: [PATCH 05/12] Use the secondary ELB port when exporting kubecfg w/ --admin and sslCertificate --- pkg/kubeconfig/create_kubecfg.go | 10 ++- pkg/kubeconfig/create_kubecfg_test.go | 97 ++++++++++++++++++++++++--- 2 files changed, 97 insertions(+), 10 deletions(-) diff --git a/pkg/kubeconfig/create_kubecfg.go b/pkg/kubeconfig/create_kubecfg.go index 4e129a075f..8c5a5ff00a 100644 --- a/pkg/kubeconfig/create_kubecfg.go +++ b/pkg/kubeconfig/create_kubecfg.go @@ -99,11 +99,17 @@ func BuildKubecfg(cluster *kops.Cluster, keyStore fi.Keystore, secretStore fi.Se b := NewKubeconfigBuilder() + // Use the secondary load balancer port if a certificate is on the primary listener + if admin != 0 && cluster.Spec.API != nil && cluster.Spec.API.LoadBalancer != nil && cluster.Spec.API.LoadBalancer.SSLCertificate != "" && cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassNetwork { + server = server + ":8443" + } + b.Context = clusterName b.Server = server - // add the CA Cert to the kubeconfig only if we didn't specify a SSL cert for the LB or are targeting the internal DNS name - if cluster.Spec.API == nil || cluster.Spec.API.LoadBalancer == nil || cluster.Spec.API.LoadBalancer.SSLCertificate == "" || internal { + // add the CA Cert to the kubeconfig only if we didn't specify a certificate for the LB + // or if we're using admin credentials and the secondary port + if cluster.Spec.API == nil || cluster.Spec.API.LoadBalancer == nil || cluster.Spec.API.LoadBalancer.SSLCertificate == "" || cluster.Spec.API.LoadBalancer.Class == kops.LoadBalancerClassNetwork || internal { cert, _, _, err := keyStore.FindKeypair(fi.CertificateIDCA) if err != nil { return nil, fmt.Errorf("error fetching CA keypair: %v", err) diff --git a/pkg/kubeconfig/create_kubecfg_test.go b/pkg/kubeconfig/create_kubecfg_test.go index 762f01a7ec..a6f5394f24 100644 --- a/pkg/kubeconfig/create_kubecfg_test.go +++ b/pkg/kubeconfig/create_kubecfg_test.go @@ -71,10 +71,20 @@ func (f fakeKeyStore) MirrorTo(basedir vfs.Path) error { } // build a generic minimal cluster -func buildMinimalCluster(clusterName string, masterPublicName string) *kops.Cluster { +func buildMinimalCluster(clusterName string, masterPublicName string, lbCert bool, nlb bool) *kops.Cluster { cluster := testutils.BuildMinimalCluster(clusterName) cluster.Spec.MasterPublicName = masterPublicName cluster.Spec.MasterInternalName = fmt.Sprintf("internal.%v", masterPublicName) + cluster.Spec.KubernetesVersion = "1.19.3" + cluster.Spec.API = &kops.AccessSpec{ + LoadBalancer: &kops.LoadBalancerAccessSpec{}, + } + if lbCert { + cluster.Spec.API.LoadBalancer.SSLCertificate = "cert-arn" + } + if nlb { + cluster.Spec.API.LoadBalancer.Class = kops.LoadBalancerClassNetwork + } return cluster } @@ -107,9 +117,12 @@ func TestBuildKubecfg(t *testing.T) { useKopsAuthenticationPlugin bool } - publiccluster := buildMinimalCluster("testcluster", "testcluster.test.com") - emptyMasterPublicNameCluster := buildMinimalCluster("emptyMasterPublicNameCluster", "") - gossipCluster := buildMinimalCluster("testgossipcluster.k8s.local", "") + publicCluster := buildMinimalCluster("testcluster", "testcluster.test.com", false, false) + emptyMasterPublicNameCluster := buildMinimalCluster("emptyMasterPublicNameCluster", "", false, false) + gossipCluster := buildMinimalCluster("testgossipcluster.k8s.local", "", false, false) + certCluster := buildMinimalCluster("testcluster", "testcluster.test.com", true, false) + certNLBCluster := buildMinimalCluster("testcluster", "testcluster.test.com", true, true) + certGossipNLBCluster := buildMinimalCluster("testgossipcluster.k8s.local", "", true, true) tests := []struct { name string @@ -121,7 +134,7 @@ func TestBuildKubecfg(t *testing.T) { { name: "Test Kube Config Data For Public DNS with admin", args: args{ - cluster: publiccluster, + cluster: publicCluster, status: fakeStatusStore{}, admin: DefaultKubecfgAdminLifetime, user: "", @@ -134,10 +147,55 @@ func TestBuildKubecfg(t *testing.T) { }, wantClientCert: true, }, + { + name: "Test Kube Config Data For Public DNS with admin and secondary NLB port", + args: args{ + cluster: certNLBCluster, + status: fakeStatusStore{}, + admin: DefaultKubecfgAdminLifetime, + }, + want: &KubeconfigBuilder{ + Context: "testcluster", + Server: "https://testcluster.test.com:8443", + CACert: []byte(certData), + User: "testcluster", + }, + wantClientCert: true, + }, + { + name: "Test Kube Config Data For Public DNS with admin and CLB ACM Certificate", + args: args{ + cluster: certCluster, + status: fakeStatusStore{}, + admin: DefaultKubecfgAdminLifetime, + }, + want: &KubeconfigBuilder{ + Context: "testcluster", + Server: "https://testcluster.test.com", + CACert: nil, + User: "testcluster", + }, + wantClientCert: true, + }, + { + name: "Test Kube Config Data For Public DNS without admin and with ACM certificate", + args: args{ + cluster: certNLBCluster, + status: fakeStatusStore{}, + admin: 0, + }, + want: &KubeconfigBuilder{ + Context: "testcluster", + Server: "https://testcluster.test.com", + CACert: []byte(certData), + User: "testcluster", + }, + wantClientCert: false, + }, { name: "Test Kube Config Data For Public DNS without admin", args: args{ - cluster: publiccluster, + cluster: publicCluster, status: fakeStatusStore{}, admin: 0, user: "myuser", @@ -191,7 +249,7 @@ func TestBuildKubecfg(t *testing.T) { { name: "Public DNS with kops auth plugin", args: args{ - cluster: publiccluster, + cluster: publicCluster, status: fakeStatusStore{}, admin: 0, useKopsAuthenticationPlugin: true, @@ -214,7 +272,7 @@ func TestBuildKubecfg(t *testing.T) { { name: "Test Kube Config Data For internal DNS name with admin", args: args{ - cluster: publiccluster, + cluster: publicCluster, status: fakeStatusStore{}, admin: DefaultKubecfgAdminLifetime, internal: true, @@ -227,6 +285,29 @@ func TestBuildKubecfg(t *testing.T) { }, wantClientCert: true, }, + { + name: "Test Kube Config Data For Gossip cluster with admin and secondary NLB port", + args: args{ + cluster: certGossipNLBCluster, + status: fakeStatusStore{ + GetApiIngressStatusFn: func(cluster *kops.Cluster) ([]kops.ApiIngressStatus, error) { + return []kops.ApiIngressStatus{ + { + Hostname: "nlbHostName", + }, + }, nil + }, + }, + admin: DefaultKubecfgAdminLifetime, + }, + want: &KubeconfigBuilder{ + Context: "testgossipcluster.k8s.local", + Server: "https://nlbHostName:8443", + CACert: []byte(certData), + User: "testgossipcluster.k8s.local", + }, + wantClientCert: true, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { From 15ba84df1664b2aba95d50e5a117455c0258f397 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Thu, 5 Nov 2020 10:31:32 -0600 Subject: [PATCH 06/12] Find target group names for existing NLB listeners --- .../fi/cloudup/awstasks/network_load_balancer.go | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go index 6535f42f12..6beaa7ca64 100644 --- a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go +++ b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go @@ -371,15 +371,27 @@ func (e *NetworkLoadBalancer) Find(c *fi.Context) (*NetworkLoadBalancer, error) 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 = 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.TargetGroups = append(actual.TargetGroups, &TargetGroup{ARN: targetGroupARN}) + + cloud := c.Cloud.(awsup.AWSCloud) + descResp, err := cloud.ELBV2().DescribeTargetGroups(&elbv2.DescribeTargetGroupsInput{ + TargetGroupArns: []*string{targetGroupARN}, + }) + if err != nil { + return nil, fmt.Errorf("error querying for NLB listener target groups: %v", err) + } + if len(descResp.TargetGroups) != 1 { + return nil, fmt.Errorf("unexpected DescribeTargetGroups response: %v", descResp) + } + actualListener.TargetGroupName = aws.StringValue(descResp.TargetGroups[0].TargetGroupName) } } + actual.Listeners = append(actual.Listeners, actualListener) } if len(actual.TargetGroups) > 0 { targetGroups, err := ReconcileTargetGroups(c.Cloud.(awsup.AWSCloud), actual.TargetGroups, e.TargetGroups) From 3417ef366c2c5c67df138bd48f57a37d94554b7f Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Thu, 5 Nov 2020 11:31:07 -0600 Subject: [PATCH 07/12] Handle target groups that dont yet exist when reconciling --- upup/pkg/fi/cloudup/awstasks/targetgroup.go | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/upup/pkg/fi/cloudup/awstasks/targetgroup.go b/upup/pkg/fi/cloudup/awstasks/targetgroup.go index 07d54ebcbf..99db628d8c 100644 --- a/upup/pkg/fi/cloudup/awstasks/targetgroup.go +++ b/upup/pkg/fi/cloudup/awstasks/targetgroup.go @@ -127,6 +127,9 @@ func FindTargetGroupByName(cloud awsup.AWSCloud, findName string) (*elbv2.Target resp, err := cloud.ELBV2().DescribeTargetGroups(request) if err != nil { + if aerr, ok := err.(awserr.Error); ok && aerr.Code() == elbv2.ErrCodeTargetGroupNotFoundException { + return nil, nil + } return nil, fmt.Errorf("error describing TargetGroups: %v", err) } if len(resp.TargetGroups) == 0 { @@ -235,7 +238,9 @@ func ReconcileTargetGroups(cloud awsup.AWSCloud, actual []*TargetGroup, expected if err != nil { return nil, err } - apiTGs[aws.StringValue(apiTG.TargetGroupArn)] = task + if apiTG != nil { + apiTGs[aws.StringValue(apiTG.TargetGroupArn)] = task + } } for _, tg := range actual { if apiTask, ok := apiTGs[aws.StringValue(tg.ARN)]; ok { From 370092cb5aaab3b9e8407a6f486ffad9bd299a82 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Thu, 5 Nov 2020 12:58:20 -0600 Subject: [PATCH 08/12] Update TG ports rather than protocols when adding/removing ACM certs from listeners This also renames the TGs to be more descriptive, with tcp and tls prefixes. --- pkg/model/awsmodel/api_loadbalancer.go | 26 +++--- pkg/model/awsmodel/autoscalinggroup.go | 4 +- .../complex/cloudformation.json | 80 +++++++++---------- .../update_cluster/complex/kubernetes.tf | 44 +++++----- .../cloudup/awstasks/network_load_balancer.go | 1 + upup/pkg/fi/cloudup/awstasks/targetgroup.go | 5 +- 6 files changed, 79 insertions(+), 81 deletions(-) diff --git a/pkg/model/awsmodel/api_loadbalancer.go b/pkg/model/awsmodel/api_loadbalancer.go index 47d507b98c..b298997788 100644 --- a/pkg/model/awsmodel/api_loadbalancer.go +++ b/pkg/model/awsmodel/api_loadbalancer.go @@ -112,20 +112,18 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { nlbListeners := []*awstasks.NetworkLoadBalancerListener{ { Port: 443, - TargetGroupName: b.NLBTargetGroupName("api"), + TargetGroupName: b.NLBTargetGroupName("tcp"), }, } if lbSpec.SSLCertificate != "" { listeners["443"].SSLCertificateID = lbSpec.SSLCertificate - nlbListeners[0].SSLCertificateID = lbSpec.SSLCertificate + nlbListeners[0].Port = 8443 nlbListeners = append(nlbListeners, &awstasks.NetworkLoadBalancerListener{ - Port: 8443, - TargetGroupName: b.NLBTargetGroupName("tcp"), + Port: 443, + TargetGroupName: b.NLBTargetGroupName("tls"), + SSLCertificateID: lbSpec.SSLCertificate, }) - klog.Info("Using ACM certificate for API ELB") - } else { - klog.Info("NOT using ACM certificate for API ELB") } if lbSpec.SecurityGroupOverride != nil { @@ -206,21 +204,17 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { c.AddTask(clb) } else if b.APILoadBalancerClass() == kops.LoadBalancerClassNetwork { - targetGroupName := b.NLBTargetGroupName("api") + targetGroupName := b.NLBTargetGroupName("tcp") primaryTags := b.CloudTags(targetGroupName, false) // Override the returned name to be the expected NLB TG name primaryTags["Name"] = targetGroupName - protocol := fi.String("TCP") - if lbSpec.SSLCertificate != "" { - protocol = fi.String("TLS") - } tg := &awstasks.TargetGroup{ Name: fi.String(targetGroupName), VPC: b.LinkToVPC(), Tags: primaryTags, - Protocol: protocol, + Protocol: fi.String("TCP"), Port: fi.Int64(443), HealthyThreshold: fi.Int64(2), UnhealthyThreshold: fi.Int64(2), @@ -233,7 +227,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { if lbSpec.SSLCertificate != "" { secondaryTags := b.CloudTags(targetGroupName, false) - secondaryName := b.NLBTargetGroupName("tcp") + secondaryName := b.NLBTargetGroupName("tls") // Override the returned name to be the expected NLB TG name secondaryTags["Name"] = secondaryName @@ -241,7 +235,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { Name: fi.String(secondaryName), VPC: b.LinkToVPC(), Tags: secondaryTags, - Protocol: fi.String("TCP"), + Protocol: fi.String("TLS"), Port: fi.Int64(443), HealthyThreshold: fi.Int64(2), UnhealthyThreshold: fi.Int64(2), @@ -250,7 +244,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { c.AddTask(secondaryTG) nlb.TargetGroups = append(nlb.TargetGroups, secondaryTG) } - + sort.Stable(awstasks.OrderTargetGroupsByPort(nlb.TargetGroups)) c.AddTask(nlb) } diff --git a/pkg/model/awsmodel/autoscalinggroup.go b/pkg/model/awsmodel/autoscalinggroup.go index 356ba8cc5e..de922bb962 100644 --- a/pkg/model/awsmodel/autoscalinggroup.go +++ b/pkg/model/awsmodel/autoscalinggroup.go @@ -370,9 +370,9 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.ModelBuil if !featureflag.Spotinst.Enabled() { if b.UseLoadBalancerForAPI() && ig.Spec.Role == kops.InstanceGroupRoleMaster { if b.UseNetworkLoadBalancer() { - t.TargetGroups = append(t.TargetGroups, b.LinkToTargetGroup("api")) + t.TargetGroups = append(t.TargetGroups, b.LinkToTargetGroup("tcp")) if b.Cluster.Spec.API.LoadBalancer.SSLCertificate != "" { - t.TargetGroups = append(t.TargetGroups, b.LinkToTargetGroup("tcp")) + t.TargetGroups = append(t.TargetGroups, b.LinkToTargetGroup("tls")) } } else { t.LoadBalancers = append(t.LoadBalancers, b.LinkToCLB("api")) diff --git a/tests/integration/update_cluster/complex/cloudformation.json b/tests/integration/update_cluster/complex/cloudformation.json index 6dca568f0f..fc2bacb308 100644 --- a/tests/integration/update_cluster/complex/cloudformation.json +++ b/tests/integration/update_cluster/complex/cloudformation.json @@ -86,10 +86,10 @@ ], "TargetGroupARNs": [ { - "Ref": "AWSElasticLoadBalancingV2TargetGroupapicomplexexamplecomvd3t5n" + "Ref": "AWSElasticLoadBalancingV2TargetGrouptcpcomplexexamplecomvpjolq" }, { - "Ref": "AWSElasticLoadBalancingV2TargetGrouptcpcomplexexamplecomvpjolq" + "Ref": "AWSElasticLoadBalancingV2TargetGrouptlscomplexexamplecom5nursn" } ] } @@ -1184,7 +1184,7 @@ { "Type": "forward", "TargetGroupArn": { - "Ref": "AWSElasticLoadBalancingV2TargetGroupapicomplexexamplecomvd3t5n" + "Ref": "AWSElasticLoadBalancingV2TargetGrouptcpcomplexexamplecomvpjolq" } } ], @@ -1202,7 +1202,7 @@ { "Type": "forward", "TargetGroupArn": { - "Ref": "AWSElasticLoadBalancingV2TargetGrouptcpcomplexexamplecomvpjolq" + "Ref": "AWSElasticLoadBalancingV2TargetGrouptlscomplexexamplecom5nursn" } } ], @@ -1248,42 +1248,6 @@ ] } }, - "AWSElasticLoadBalancingV2TargetGroupapicomplexexamplecomvd3t5n": { - "Type": "AWS::ElasticLoadBalancingV2::TargetGroup", - "Properties": { - "Name": "api-complex-example-com-vd3t5n", - "Port": 443, - "Protocol": "TLS", - "VpcId": { - "Ref": "AWSEC2VPCcomplexexamplecom" - }, - "Tags": [ - { - "Key": "KubernetesCluster", - "Value": "complex.example.com" - }, - { - "Key": "Name", - "Value": "api-complex-example-com-vd3t5n" - }, - { - "Key": "Owner", - "Value": "John Doe" - }, - { - "Key": "foo/bar", - "Value": "fib+baz" - }, - { - "Key": "kubernetes.io/cluster/complex.example.com", - "Value": "owned" - } - ], - "HealthCheckProtocol": "TLS", - "HealthyThresholdCount": 2, - "UnhealthyThresholdCount": 2 - } - }, "AWSElasticLoadBalancingV2TargetGrouptcpcomplexexamplecomvpjolq": { "Type": "AWS::ElasticLoadBalancingV2::TargetGroup", "Properties": { @@ -1320,6 +1284,42 @@ "UnhealthyThresholdCount": 2 } }, + "AWSElasticLoadBalancingV2TargetGrouptlscomplexexamplecom5nursn": { + "Type": "AWS::ElasticLoadBalancingV2::TargetGroup", + "Properties": { + "Name": "tls-complex-example-com-5nursn", + "Port": 443, + "Protocol": "TLS", + "VpcId": { + "Ref": "AWSEC2VPCcomplexexamplecom" + }, + "Tags": [ + { + "Key": "KubernetesCluster", + "Value": "complex.example.com" + }, + { + "Key": "Name", + "Value": "tls-complex-example-com-5nursn" + }, + { + "Key": "Owner", + "Value": "John Doe" + }, + { + "Key": "foo/bar", + "Value": "fib+baz" + }, + { + "Key": "kubernetes.io/cluster/complex.example.com", + "Value": "owned" + } + ], + "HealthCheckProtocol": "TLS", + "HealthyThresholdCount": 2, + "UnhealthyThresholdCount": 2 + } + }, "AWSIAMInstanceProfilemasterscomplexexamplecom": { "Type": "AWS::IAM::InstanceProfile", "Properties": { diff --git a/tests/integration/update_cluster/complex/kubernetes.tf b/tests/integration/update_cluster/complex/kubernetes.tf index 0cb76bc1e4..71d3d31021 100644 --- a/tests/integration/update_cluster/complex/kubernetes.tf +++ b/tests/integration/update_cluster/complex/kubernetes.tf @@ -135,7 +135,7 @@ resource "aws_autoscaling_group" "master-us-test-1a-masters-complex-example-com" propagate_at_launch = true value = "owned" } - target_group_arns = [aws_lb_target_group.api-complex-example-com-vd3t5n.id, aws_lb_target_group.tcp-complex-example-com-vpjolq.id] + target_group_arns = [aws_lb_target_group.tcp-complex-example-com-vpjolq.id, aws_lb_target_group.tls-complex-example-com-5nursn.id] vpc_zone_identifier = [aws_subnet.us-test-1a-complex-example-com.id] } @@ -425,7 +425,7 @@ resource "aws_launch_template" "nodes-complex-example-com" { resource "aws_lb_listener" "api-complex-example-com-443" { certificate_arn = "arn:aws:acm:us-test-1:000000000000:certificate/123456789012-1234-1234-1234-12345678" default_action { - target_group_arn = aws_lb_target_group.api-complex-example-com-vd3t5n.id + target_group_arn = aws_lb_target_group.tcp-complex-example-com-vpjolq.id type = "forward" } load_balancer_arn = aws_lb.api-complex-example-com.id @@ -435,7 +435,7 @@ resource "aws_lb_listener" "api-complex-example-com-443" { resource "aws_lb_listener" "api-complex-example-com-8443" { default_action { - target_group_arn = aws_lb_target_group.tcp-complex-example-com-vpjolq.id + target_group_arn = aws_lb_target_group.tls-complex-example-com-5nursn.id type = "forward" } load_balancer_arn = aws_lb.api-complex-example-com.id @@ -443,25 +443,6 @@ resource "aws_lb_listener" "api-complex-example-com-8443" { protocol = "TCP" } -resource "aws_lb_target_group" "api-complex-example-com-vd3t5n" { - health_check { - healthy_threshold = 2 - protocol = "TLS" - unhealthy_threshold = 2 - } - name = "api-complex-example-com-vd3t5n" - port = 443 - protocol = "TLS" - tags = { - "KubernetesCluster" = "complex.example.com" - "Name" = "api-complex-example-com-vd3t5n" - "Owner" = "John Doe" - "foo/bar" = "fib+baz" - "kubernetes.io/cluster/complex.example.com" = "owned" - } - vpc_id = aws_vpc.complex-example-com.id -} - resource "aws_lb_target_group" "tcp-complex-example-com-vpjolq" { health_check { healthy_threshold = 2 @@ -481,6 +462,25 @@ resource "aws_lb_target_group" "tcp-complex-example-com-vpjolq" { vpc_id = aws_vpc.complex-example-com.id } +resource "aws_lb_target_group" "tls-complex-example-com-5nursn" { + health_check { + healthy_threshold = 2 + protocol = "TLS" + unhealthy_threshold = 2 + } + name = "tls-complex-example-com-5nursn" + port = 443 + protocol = "TLS" + tags = { + "KubernetesCluster" = "complex.example.com" + "Name" = "tls-complex-example-com-5nursn" + "Owner" = "John Doe" + "foo/bar" = "fib+baz" + "kubernetes.io/cluster/complex.example.com" = "owned" + } + vpc_id = aws_vpc.complex-example-com.id +} + resource "aws_lb" "api-complex-example-com" { enable_cross_zone_load_balancing = true internal = false diff --git a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go index 6beaa7ca64..d7d78806ee 100644 --- a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go +++ b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go @@ -400,6 +400,7 @@ func (e *NetworkLoadBalancer) Find(c *fi.Context) (*NetworkLoadBalancer, error) } actual.TargetGroups = targetGroups } + sort.Stable(OrderTargetGroupsByPort(actual.TargetGroups)) } diff --git a/upup/pkg/fi/cloudup/awstasks/targetgroup.go b/upup/pkg/fi/cloudup/awstasks/targetgroup.go index 99db628d8c..434c328de7 100644 --- a/upup/pkg/fi/cloudup/awstasks/targetgroup.go +++ b/upup/pkg/fi/cloudup/awstasks/targetgroup.go @@ -200,7 +200,10 @@ 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) + if a[i].ARN != nil || a[j].ARN != nil { + return fi.StringValue(a[i].ARN) < fi.StringValue(a[j].ARN) + } + return fi.StringValue(a[i].Name) < fi.StringValue(a[j].Name) } // pkg/model/awsmodel doesn't know the ARN of the API TargetGroup tasks that it passes to the master ASGs, From 54decbc47920f501c1f9a0615b16642eb64c4d0c Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Fri, 6 Nov 2020 11:08:43 -0600 Subject: [PATCH 09/12] Always use TCP health check protocol for target groups --- tests/integration/update_cluster/complex/kubernetes.tf | 2 +- upup/pkg/fi/cloudup/awstasks/targetgroup.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/tests/integration/update_cluster/complex/kubernetes.tf b/tests/integration/update_cluster/complex/kubernetes.tf index 71d3d31021..2571e6979c 100644 --- a/tests/integration/update_cluster/complex/kubernetes.tf +++ b/tests/integration/update_cluster/complex/kubernetes.tf @@ -465,7 +465,7 @@ resource "aws_lb_target_group" "tcp-complex-example-com-vpjolq" { resource "aws_lb_target_group" "tls-complex-example-com-5nursn" { health_check { healthy_threshold = 2 - protocol = "TLS" + protocol = "TCP" unhealthy_threshold = 2 } name = "tls-complex-example-com-5nursn" diff --git a/upup/pkg/fi/cloudup/awstasks/targetgroup.go b/upup/pkg/fi/cloudup/awstasks/targetgroup.go index 434c328de7..03ec08e0f6 100644 --- a/upup/pkg/fi/cloudup/awstasks/targetgroup.go +++ b/upup/pkg/fi/cloudup/awstasks/targetgroup.go @@ -290,7 +290,7 @@ func (_ *TargetGroup) RenderTerraform(t *terraform.TerraformTarget, a, e, change HealthCheck: terraformTargetGroupHealthCheck{ HealthyThreshold: *e.HealthyThreshold, UnhealthyThreshold: *e.UnhealthyThreshold, - Protocol: *e.Protocol, + Protocol: elbv2.ProtocolEnumTcp, }, } From 4758ea9f2fa1c40c0fa25c97d720350d9eb08ad0 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Mon, 9 Nov 2020 17:24:32 -0600 Subject: [PATCH 10/12] Address feedback --- pkg/model/awsmodel/api_loadbalancer.go | 22 +++++++++---------- .../cloudup/awstasks/network_load_balancer.go | 9 +++----- upup/pkg/fi/cloudup/awstasks/targetgroup.go | 10 ++++----- 3 files changed, 19 insertions(+), 22 deletions(-) diff --git a/pkg/model/awsmodel/api_loadbalancer.go b/pkg/model/awsmodel/api_loadbalancer.go index b298997788..5b27c213b8 100644 --- a/pkg/model/awsmodel/api_loadbalancer.go +++ b/pkg/model/awsmodel/api_loadbalancer.go @@ -204,16 +204,16 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { c.AddTask(clb) } else if b.APILoadBalancerClass() == kops.LoadBalancerClassNetwork { - targetGroupName := b.NLBTargetGroupName("tcp") - primaryTags := b.CloudTags(targetGroupName, false) + tcpGroupName := b.NLBTargetGroupName("tcp") + tcpGroupTags := b.CloudTags(tcpGroupName, false) // Override the returned name to be the expected NLB TG name - primaryTags["Name"] = targetGroupName + tcpGroupTags["Name"] = tcpGroupName tg := &awstasks.TargetGroup{ - Name: fi.String(targetGroupName), + Name: fi.String(tcpGroupName), VPC: b.LinkToVPC(), - Tags: primaryTags, + Tags: tcpGroupTags, Protocol: fi.String("TCP"), Port: fi.Int64(443), HealthyThreshold: fi.Int64(2), @@ -226,15 +226,15 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { nlb.TargetGroups = append(nlb.TargetGroups, tg) if lbSpec.SSLCertificate != "" { - secondaryTags := b.CloudTags(targetGroupName, false) - secondaryName := b.NLBTargetGroupName("tls") + tlsGroupName := b.NLBTargetGroupName("tls") + tlsGroupTags := b.CloudTags(tlsGroupName, false) // Override the returned name to be the expected NLB TG name - secondaryTags["Name"] = secondaryName + tlsGroupTags["Name"] = tlsGroupName secondaryTG := &awstasks.TargetGroup{ - Name: fi.String(secondaryName), + Name: fi.String(tlsGroupName), VPC: b.LinkToVPC(), - Tags: secondaryTags, + Tags: tlsGroupTags, Protocol: fi.String("TLS"), Port: fi.Int64(443), HealthyThreshold: fi.Int64(2), @@ -244,7 +244,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { c.AddTask(secondaryTG) nlb.TargetGroups = append(nlb.TargetGroups, secondaryTG) } - sort.Stable(awstasks.OrderTargetGroupsByPort(nlb.TargetGroups)) + sort.Stable(awstasks.OrderTargetGroupsByName(nlb.TargetGroups)) c.AddTask(nlb) } diff --git a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go index d7d78806ee..5b13386add 100644 --- a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go +++ b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go @@ -123,7 +123,7 @@ func (e *NetworkLoadBalancerListener) GetDependencies(tasks map[string]fi.Task) return nil } -// OrderListenersByPort implements sort.Interface for []OrderTargetGroupsByPort, based on port number +// OrderListenersByPort implements sort.Interface for []OrderListenersByPort, based on port number type OrderListenersByPort []*NetworkLoadBalancerListener func (a OrderListenersByPort) Len() int { return len(a) } @@ -336,7 +336,6 @@ 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 { @@ -363,8 +362,6 @@ func (e *NetworkLoadBalancer) Find(c *fi.Context) (*NetworkLoadBalancer, error) return nil, fmt.Errorf("error querying for NLB listeners :%v", err) } - actual.Listeners = make([]*NetworkLoadBalancerListener, 0) - for _, l := range response.Listeners { actualListener := &NetworkLoadBalancerListener{} actualListener.Port = int(aws.Int64Value(l.Port)) @@ -400,7 +397,7 @@ func (e *NetworkLoadBalancer) Find(c *fi.Context) (*NetworkLoadBalancer, error) } actual.TargetGroups = targetGroups } - sort.Stable(OrderTargetGroupsByPort(actual.TargetGroups)) + sort.Stable(OrderTargetGroupsByName(actual.TargetGroups)) } @@ -494,7 +491,7 @@ 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)) + sort.Stable(OrderTargetGroupsByName(e.TargetGroups)) } func (s *NetworkLoadBalancer) CheckChanges(a, e, changes *NetworkLoadBalancer) error { diff --git a/upup/pkg/fi/cloudup/awstasks/targetgroup.go b/upup/pkg/fi/cloudup/awstasks/targetgroup.go index 03ec08e0f6..2a514779a9 100644 --- a/upup/pkg/fi/cloudup/awstasks/targetgroup.go +++ b/upup/pkg/fi/cloudup/awstasks/targetgroup.go @@ -194,12 +194,12 @@ 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 +// OrderTargetGroupsByName implements sort.Interface for []OrderTargetGroupsByName, based on port number +type OrderTargetGroupsByName []*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 { +func (a OrderTargetGroupsByName) Len() int { return len(a) } +func (a OrderTargetGroupsByName) Swap(i, j int) { a[i], a[j] = a[j], a[i] } +func (a OrderTargetGroupsByName) Less(i, j int) bool { if a[i].ARN != nil || a[j].ARN != nil { return fi.StringValue(a[i].ARN) < fi.StringValue(a[j].ARN) } From 0072abd1a28f8dd0c77c1737b6272db751076178 Mon Sep 17 00:00:00 2001 From: Peter Rifel Date: Tue, 10 Nov 2020 08:52:47 -0600 Subject: [PATCH 11/12] Update validation error permalink --- pkg/apis/kops/validation/validation.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pkg/apis/kops/validation/validation.go b/pkg/apis/kops/validation/validation.go index 8bb3538f3f..e543543569 100644 --- a/pkg/apis/kops/validation/validation.go +++ b/pkg/apis/kops/validation/validation.go @@ -218,7 +218,7 @@ func validateClusterSpec(spec *kops.ClusterSpec, c *kops.Cluster, fieldPath *fie allErrs = append(allErrs, field.Forbidden(fieldPath, "cannot use NLB together with spotinst")) } if spec.API.LoadBalancer.SSLCertificate != "" && spec.API.LoadBalancer.Class != kops.LoadBalancerClassNetwork && c.IsKubernetesGTE("1.19") { - allErrs = append(allErrs, field.Forbidden(fieldPath, "sslCertificate requires network loadbalancer for K8s 1.19+ see ")) + allErrs = append(allErrs, field.Forbidden(fieldPath, "sslCertificate requires network loadbalancer for K8s 1.19+ see https://github.com/kubernetes/kops/blob/master/permalinks/acm_nlb.md")) } if spec.API.LoadBalancer.Class == kops.LoadBalancerClassNetwork && spec.API.LoadBalancer.UseForInternalApi && spec.API.LoadBalancer.Type == kops.LoadBalancerTypeInternal { allErrs = append(allErrs, field.Forbidden(fieldPath, "useForInternalApi cannot be used with internal NLB due lack of hairpinning support")) From 0934374fe29dcbaf69f46dc44fafd13aeeeba969 Mon Sep 17 00:00:00 2001 From: Ciprian Hacman Date: Tue, 10 Nov 2020 17:14:28 +0200 Subject: [PATCH 12/12] Fix various NLB nits --- pkg/model/awsmodel/autoscalinggroup.go | 6 ++++-- upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go | 3 ++- upup/pkg/fi/cloudup/awstasks/network_load_balancer.go | 2 ++ upup/pkg/fi/cloudup/awstasks/targetgroup.go | 6 +----- 4 files changed, 9 insertions(+), 8 deletions(-) diff --git a/pkg/model/awsmodel/autoscalinggroup.go b/pkg/model/awsmodel/autoscalinggroup.go index de922bb962..1149cacbc3 100644 --- a/pkg/model/awsmodel/autoscalinggroup.go +++ b/pkg/model/awsmodel/autoscalinggroup.go @@ -364,6 +364,8 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.ModelBuil t.InstanceProtection = ig.Spec.InstanceProtection + t.TargetGroups = []*awstasks.TargetGroup{} + // When Spotinst Elastigroups are used, there is no need to create // a separate task for the attachment of the load balancer since this // is already done as part of the Elastigroup's creation, if needed. @@ -384,7 +386,7 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.ModelBuil } } - for _, extLB := range ig.Spec.ExternalLoadBalancers { + for i, extLB := range ig.Spec.ExternalLoadBalancers { if extLB.LoadBalancerName != nil { lb := &awstasks.ClassicLoadBalancer{ Name: extLB.LoadBalancerName, @@ -397,7 +399,7 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.ModelBuil if extLB.TargetGroupARN != nil { tg := &awstasks.TargetGroup{ - Name: extLB.TargetGroupARN, + Name: fi.String(fmt.Sprintf("external-tg-%d", i)), ARN: extLB.TargetGroupARN, Shared: fi.Bool(true), } diff --git a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go b/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go index 130d27c989..4e02911bc9 100644 --- a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go +++ b/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go @@ -162,6 +162,7 @@ func (e *AutoscalingGroup) Find(c *fi.Context) (*AutoscalingGroup, error) { } } + actual.TargetGroups = []*TargetGroup{} if len(g.TargetGroupARNs) > 0 { actualTGs := make([]*TargetGroup, 0) for _, tg := range g.TargetGroupARNs { @@ -643,7 +644,7 @@ func (v *AutoscalingGroup) RenderAWS(t *awsup.AWSAPITarget, a, e, changes *Autos } if detachTGRequest != nil { if _, err := t.Cloud.Autoscaling().DetachLoadBalancerTargetGroups(detachTGRequest); err != nil { - return fmt.Errorf("error attaching TargetGroups: %v", err) + return fmt.Errorf("error detaching TargetGroups: %v", err) } } if attachTGRequest != nil { diff --git a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go index 5b13386add..1f26612e59 100644 --- a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go +++ b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go @@ -362,6 +362,8 @@ func (e *NetworkLoadBalancer) Find(c *fi.Context) (*NetworkLoadBalancer, error) return nil, fmt.Errorf("error querying for NLB listeners :%v", err) } + actual.Listeners = []*NetworkLoadBalancerListener{} + actual.TargetGroups = []*TargetGroup{} for _, l := range response.Listeners { actualListener := &NetworkLoadBalancerListener{} actualListener.Port = int(aws.Int64Value(l.Port)) diff --git a/upup/pkg/fi/cloudup/awstasks/targetgroup.go b/upup/pkg/fi/cloudup/awstasks/targetgroup.go index 2a514779a9..f4cde9b2c4 100644 --- a/upup/pkg/fi/cloudup/awstasks/targetgroup.go +++ b/upup/pkg/fi/cloudup/awstasks/targetgroup.go @@ -60,8 +60,7 @@ func (e *TargetGroup) Find(c *fi.Context) (*TargetGroup, error) { request := &elbv2.DescribeTargetGroupsInput{} if e.ARN != nil { request.TargetGroupArns = []*string{e.ARN} - } - if e.Name != nil { + } else if e.Name != nil { request.Names = []*string{e.Name} } @@ -200,9 +199,6 @@ type OrderTargetGroupsByName []*TargetGroup func (a OrderTargetGroupsByName) Len() int { return len(a) } func (a OrderTargetGroupsByName) Swap(i, j int) { a[i], a[j] = a[j], a[i] } func (a OrderTargetGroupsByName) Less(i, j int) bool { - if a[i].ARN != nil || a[j].ARN != nil { - return fi.StringValue(a[i].ARN) < fi.StringValue(a[j].ARN) - } return fi.StringValue(a[i].Name) < fi.StringValue(a[j].Name) }