From e57cd534b541a41b164c7b584d8e6fd1721dcb10 Mon Sep 17 00:00:00 2001 From: Ciprian Hacman Date: Tue, 1 Dec 2020 21:33:26 +0200 Subject: [PATCH] Allow attaching same external target group to multiple instance groups --- pkg/model/BUILD.bazel | 2 - pkg/model/awsmodel/BUILD.bazel | 2 +- pkg/model/awsmodel/api_loadbalancer.go | 2 +- pkg/model/awsmodel/autoscalinggroup.go | 13 +-- pkg/model/bastion.go | 2 +- pkg/model/context.go | 36 ------ pkg/model/context_test.go | 65 ----------- pkg/model/firewall_test.go | 44 -------- pkg/model/names.go | 13 ++- .../externallb/cloudformation.json | 1 + .../externallb/in-v1alpha2.yaml | 1 + .../update_cluster/externallb/kubernetes.tf | 2 +- upup/pkg/fi/cloudup/awstasks/BUILD.bazel | 4 - .../fi/cloudup/awstasks/autoscalinggroup.go | 23 ++-- .../cloudup/awstasks/network_load_balancer.go | 14 +-- upup/pkg/fi/cloudup/awstasks/targetgroup.go | 52 +-------- .../fi/cloudup/awstasks/targetgroup_test.go | 106 ------------------ upup/pkg/fi/cloudup/awsup/aws_utils.go | 42 +++++++ upup/pkg/fi/cloudup/awsup/aws_utils_test.go | 35 ++++++ 19 files changed, 113 insertions(+), 346 deletions(-) delete mode 100644 pkg/model/context_test.go delete mode 100644 upup/pkg/fi/cloudup/awstasks/targetgroup_test.go diff --git a/pkg/model/BUILD.bazel b/pkg/model/BUILD.bazel index 4b14ff2243..dc4f4103e5 100644 --- a/pkg/model/BUILD.bazel +++ b/pkg/model/BUILD.bazel @@ -68,7 +68,6 @@ go_test( name = "go_default_test", srcs = [ "bootstrapscript_test.go", - "context_test.go", "firewall_test.go", ], data = glob(["tests/**"]), #keep @@ -76,7 +75,6 @@ go_test( deps = [ "//pkg/apis/kops:go_default_library", "//pkg/apis/nodeup:go_default_library", - "//pkg/model/iam:go_default_library", "//pkg/testutils/golden:go_default_library", "//upup/pkg/fi:go_default_library", "//util/pkg/architectures:go_default_library", diff --git a/pkg/model/awsmodel/BUILD.bazel b/pkg/model/awsmodel/BUILD.bazel index 98543d0dcb..639a221b09 100644 --- a/pkg/model/awsmodel/BUILD.bazel +++ b/pkg/model/awsmodel/BUILD.bazel @@ -20,8 +20,8 @@ go_library( "//pkg/model/spotinstmodel:go_default_library", "//upup/pkg/fi:go_default_library", "//upup/pkg/fi/cloudup/awstasks:go_default_library", + "//upup/pkg/fi/cloudup/awsup:go_default_library", "//upup/pkg/fi/fitasks:go_default_library", - "//vendor/github.com/aws/aws-sdk-go/aws/arn:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/ec2:go_default_library", "//vendor/k8s.io/apimachinery/pkg/util/sets:go_default_library", "//vendor/k8s.io/klog/v2:go_default_library", diff --git a/pkg/model/awsmodel/api_loadbalancer.go b/pkg/model/awsmodel/api_loadbalancer.go index 2820d788f3..415e827e80 100644 --- a/pkg/model/awsmodel/api_loadbalancer.go +++ b/pkg/model/awsmodel/api_loadbalancer.go @@ -98,7 +98,7 @@ func (b *APILoadBalancerBuilder) Build(c *fi.ModelBuilderContext) error { var clb *awstasks.ClassicLoadBalancer var nlb *awstasks.NetworkLoadBalancer { - loadBalancerName := b.GetELBName32("api") + loadBalancerName := b.LBName32("api") idleTimeout := LoadBalancerDefaultIdleTimeout if lbSpec.IdleTimeoutSeconds != nil { diff --git a/pkg/model/awsmodel/autoscalinggroup.go b/pkg/model/awsmodel/autoscalinggroup.go index b060645d80..54c324e04a 100644 --- a/pkg/model/awsmodel/autoscalinggroup.go +++ b/pkg/model/awsmodel/autoscalinggroup.go @@ -21,7 +21,6 @@ import ( "sort" "strings" - "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/aws-sdk-go/service/ec2" "k8s.io/klog/v2" "k8s.io/kops/pkg/apis/kops" @@ -31,6 +30,7 @@ import ( "k8s.io/kops/pkg/model/spotinstmodel" "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup/awstasks" + "k8s.io/kops/upup/pkg/fi/cloudup/awsup" ) const ( @@ -367,6 +367,7 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.ModelBuil t.InstanceProtection = ig.Spec.InstanceProtection + t.LoadBalancers = []*awstasks.ClassicLoadBalancer{} t.TargetGroups = []*awstasks.TargetGroup{} // When Spotinst Elastigroups are used, there is no need to create @@ -401,16 +402,12 @@ func (b *AutoscalingGroupModelBuilder) buildAutoScalingGroupTask(c *fi.ModelBuil } if extLB.TargetGroupARN != nil { - parsed, err := arn.Parse(fi.StringValue(extLB.TargetGroupARN)) + targetGroupName, err := awsup.GetTargetGroupNameFromARN(fi.StringValue(extLB.TargetGroupARN)) if err != nil { - return nil, fmt.Errorf("error parsing target grup ARN: %v", err) - } - resource := strings.Split(parsed.Resource, "/") - if len(resource) != 3 || resource[0] != "targetgroup" { - return nil, fmt.Errorf("error parsing target grup ARN resource: %q", parsed.Resource) + return nil, err } tg := &awstasks.TargetGroup{ - Name: fi.String(resource[1]), + Name: fi.String(name + "-" + targetGroupName), ARN: extLB.TargetGroupARN, Shared: fi.Bool(true), } diff --git a/pkg/model/bastion.go b/pkg/model/bastion.go index b46a467071..c2c345fc3f 100644 --- a/pkg/model/bastion.go +++ b/pkg/model/bastion.go @@ -202,7 +202,7 @@ func (b *BastionModelBuilder) Build(c *fi.ModelBuilderContext) error { // Create ELB itself var elb *awstasks.ClassicLoadBalancer { - loadBalancerName := b.GetELBName32("bastion") + loadBalancerName := b.LBName32("bastion") idleTimeout := BastionELBDefaultIdleTimeout if b.Cluster.Spec.Topology != nil && b.Cluster.Spec.Topology.Bastion != nil && b.Cluster.Spec.Topology.Bastion.IdleTimeoutSeconds != nil { diff --git a/pkg/model/context.go b/pkg/model/context.go index a3e903cb0e..db9372279d 100644 --- a/pkg/model/context.go +++ b/pkg/model/context.go @@ -17,9 +17,7 @@ limitations under the License. package model import ( - "encoding/base32" "fmt" - "hash/fnv" "net" "strings" @@ -52,40 +50,6 @@ type KopsModelContext struct { SSHPublicKeys [][]byte } -// GetELBName32 will attempt to calculate a meaningful name for an ELB given a prefix -// Will never return a string longer than 32 chars -// Note this is _not_ the primary identifier for the ELB - we use the Name tag for that. -func (m *KopsModelContext) GetELBName32(prefix string) string { - c := m.Cluster.ObjectMeta.Name - - // The LoadBalancerName is exposed publicly as the DNS name for the load balancer. - // So this will likely become visible in a CNAME record - this is potentially some - // information leakage. - // But... if a user can see the CNAME record, they can see the actual record also, - // which will be the full cluster name. - s := prefix + "-" + strings.Replace(c, ".", "-", -1) - - // We have a 32 character limit for ELB names - // But we always compute the hash and add it, lest we trick users into assuming that we never do this - h := fnv.New32a() - if _, err := h.Write([]byte(s)); err != nil { - klog.Fatalf("error hashing values: %v", err) - } - hashString := base32.HexEncoding.EncodeToString(h.Sum(nil)) - hashString = strings.ToLower(hashString) - if len(hashString) > 6 { - hashString = hashString[:6] - } - - maxBaseLength := 32 - len(hashString) - 1 - if len(s) > maxBaseLength { - s = s[:maxBaseLength] - } - s = s + "-" + hashString - - return s -} - // GatherSubnets maps the subnet names in an InstanceGroup to the ClusterSubnetSpec objects (which are stored on the Cluster) func (m *KopsModelContext) GatherSubnets(ig *kops.InstanceGroup) ([]*kops.ClusterSubnetSpec, error) { var subnets []*kops.ClusterSubnetSpec diff --git a/pkg/model/context_test.go b/pkg/model/context_test.go deleted file mode 100644 index f5704fc43e..0000000000 --- a/pkg/model/context_test.go +++ /dev/null @@ -1,65 +0,0 @@ -/* -Copyright 2019 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package model - -import ( - "testing" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/kops/pkg/apis/kops" - "k8s.io/kops/pkg/model/iam" -) - -func Test_GetELBName32(t *testing.T) { - grid := []struct { - Prefix string - ClusterName string - Expected string - }{ - { - "bastion", "mycluster", - "bastion-mycluster-vnrjie", - }, - { - "bastion", "mycluster.example.com", - "bastion-mycluster-example-o8elkm", - }, - { - "api", "this.is.a.very.long.cluster.example.com", - "api-this-is-a-very-long-c-q4ukp4", - }, - { - "bastion", "this.is.a.very.long.cluster.example.com", - "bastion-this-is-a-very-lo-4ggpa2", - }, - } - for _, g := range grid { - c := &KopsModelContext{ - IAMModelContext: iam.IAMModelContext{ - Cluster: &kops.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: g.ClusterName, - }, - }, - }, - } - actual := c.GetELBName32(g.Prefix) - if actual != g.Expected { - t.Errorf("unexpected result from %q+%q. expected %q, got %q", g.Prefix, g.ClusterName, g.Expected, actual) - } - } -} diff --git a/pkg/model/firewall_test.go b/pkg/model/firewall_test.go index baf81e5650..72e98dde35 100644 --- a/pkg/model/firewall_test.go +++ b/pkg/model/firewall_test.go @@ -18,52 +18,8 @@ package model import ( "testing" - - metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" - "k8s.io/kops/pkg/apis/kops" - "k8s.io/kops/pkg/model/iam" ) -func Test_SharedGroups(t *testing.T) { - grid := []struct { - Prefix string - ClusterName string - Expected string - }{ - { - "bastion", "mycluster", - "bastion-mycluster-vnrjie", - }, - { - "bastion", "mycluster.example.com", - "bastion-mycluster-example-o8elkm", - }, - { - "api", "this.is.a.very.long.cluster.example.com", - "api-this-is-a-very-long-c-q4ukp4", - }, - { - "bastion", "this.is.a.very.long.cluster.example.com", - "bastion-this-is-a-very-lo-4ggpa2", - }, - } - for _, g := range grid { - c := &KopsModelContext{ - IAMModelContext: iam.IAMModelContext{ - Cluster: &kops.Cluster{ - ObjectMeta: metav1.ObjectMeta{ - Name: g.ClusterName, - }, - }, - }, - } - actual := c.GetELBName32(g.Prefix) - if actual != g.Expected { - t.Errorf("unexpected result from %q+%q. expected %q, got %q", g.Prefix, g.ClusterName, g.Expected, actual) - } - } -} - func TestJoinSuffixes(t *testing.T) { grid := []struct { src SecurityGroupInfo diff --git a/pkg/model/names.go b/pkg/model/names.go index 80aaa67da4..14e5bf60b4 100644 --- a/pkg/model/names.go +++ b/pkg/model/names.go @@ -21,12 +21,12 @@ import ( "regexp" "strings" + "k8s.io/klog/v2" "k8s.io/kops/pkg/apis/kops" "k8s.io/kops/pkg/pki" "k8s.io/kops/upup/pkg/fi" "k8s.io/kops/upup/pkg/fi/cloudup/awstasks" - - "k8s.io/klog/v2" + "k8s.io/kops/upup/pkg/fi/cloudup/awsup" ) // SecurityGroupName returns the security group name for the specific role @@ -81,6 +81,13 @@ func (b *KopsModelContext) LinkToELBSecurityGroup(prefix string) *awstasks.Secur return &awstasks.SecurityGroup{Name: &name} } +// LBName32 will attempt to calculate a meaningful name for an ELB given a prefix +// Will never return a string longer than 32 chars +// Note this is _not_ the primary identifier for the ELB - we use the Name tag for that. +func (m *KopsModelContext) LBName32(prefix string) string { + return awsup.GetResourceName32(m.Cluster.ObjectMeta.Name, prefix) +} + // CLBName returns CLB name plus cluster name func (b *KopsModelContext) CLBName(prefix string) string { return prefix + "." + b.ClusterName() @@ -91,7 +98,7 @@ func (b *KopsModelContext) NLBName(prefix string) string { } func (b *KopsModelContext) NLBTargetGroupName(prefix string) string { - return b.GetELBName32(prefix) + return awsup.GetResourceName32(b.Cluster.ObjectMeta.Name, prefix) } func (b *KopsModelContext) LinkToCLB(prefix string) *awstasks.ClassicLoadBalancer { diff --git a/tests/integration/update_cluster/externallb/cloudformation.json b/tests/integration/update_cluster/externallb/cloudformation.json index 2ed00cfc0d..4b8265a515 100644 --- a/tests/integration/update_cluster/externallb/cloudformation.json +++ b/tests/integration/update_cluster/externallb/cloudformation.json @@ -79,6 +79,7 @@ "my-external-elb-3" ], "TargetGroupARNs": [ + "arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/my-external-tg-1/1", "arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/my-external-tg-2/2", "arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/my-external-tg-3/3" ] diff --git a/tests/integration/update_cluster/externallb/in-v1alpha2.yaml b/tests/integration/update_cluster/externallb/in-v1alpha2.yaml index 492a1e39a9..b0e957d462 100644 --- a/tests/integration/update_cluster/externallb/in-v1alpha2.yaml +++ b/tests/integration/update_cluster/externallb/in-v1alpha2.yaml @@ -80,6 +80,7 @@ spec: subnets: - us-test-1a externalLoadBalancers: + - targetGroupArn: arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/my-external-tg-1/1 - targetGroupArn: arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/my-external-tg-2/2 - targetGroupArn: arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/my-external-tg-3/3 - loadBalancerName: my-external-elb-2 diff --git a/tests/integration/update_cluster/externallb/kubernetes.tf b/tests/integration/update_cluster/externallb/kubernetes.tf index 38f6960225..a7d5317177 100644 --- a/tests/integration/update_cluster/externallb/kubernetes.tf +++ b/tests/integration/update_cluster/externallb/kubernetes.tf @@ -126,7 +126,7 @@ resource "aws_autoscaling_group" "master-us-test-1a-masters-externallb-example-c propagate_at_launch = true value = "owned" } - target_group_arns = ["arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/my-external-tg-2/2", "arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/my-external-tg-3/3"] + target_group_arns = ["arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/my-external-tg-1/1", "arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/my-external-tg-2/2", "arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/my-external-tg-3/3"] vpc_zone_identifier = [aws_subnet.us-test-1a-externallb-example-com.id] } diff --git a/upup/pkg/fi/cloudup/awstasks/BUILD.bazel b/upup/pkg/fi/cloudup/awstasks/BUILD.bazel index ae1172b9f3..1a1c8d671f 100644 --- a/upup/pkg/fi/cloudup/awstasks/BUILD.bazel +++ b/upup/pkg/fi/cloudup/awstasks/BUILD.bazel @@ -90,7 +90,6 @@ go_library( "//util/pkg/maps:go_default_library", "//util/pkg/slice:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws:go_default_library", - "//vendor/github.com/aws/aws-sdk-go/aws/arn:go_default_library", "//vendor/github.com/aws/aws-sdk-go/aws/awserr:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/autoscaling:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/ec2:go_default_library", @@ -117,14 +116,12 @@ go_test( "render_test.go", "securitygroup_test.go", "subnet_test.go", - "targetgroup_test.go", "vpc_test.go", ], embed = [":go_default_library"], deps = [ "//cloudmock/aws/mockautoscaling:go_default_library", "//cloudmock/aws/mockec2:go_default_library", - "//cloudmock/aws/mockelbv2:go_default_library", "//pkg/apis/kops:go_default_library", "//pkg/assets:go_default_library", "//pkg/diff:go_default_library", @@ -135,7 +132,6 @@ go_test( "//vendor/github.com/aws/aws-sdk-go/aws:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/autoscaling:go_default_library", "//vendor/github.com/aws/aws-sdk-go/service/ec2:go_default_library", - "//vendor/github.com/aws/aws-sdk-go/service/elbv2:go_default_library", "//vendor/github.com/ghodss/yaml:go_default_library", ], ) diff --git a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go b/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go index 03a4c885ff..05a22eaaa6 100644 --- a/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go +++ b/upup/pkg/fi/cloudup/awstasks/autoscalinggroup.go @@ -23,7 +23,6 @@ import ( "strings" "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/aws-sdk-go/service/autoscaling" "k8s.io/klog/v2" "k8s.io/kops/upup/pkg/fi" @@ -114,8 +113,8 @@ func (e *AutoscalingGroup) Find(c *fi.Context) (*AutoscalingGroup, error) { MinSize: g.MinSize, } + actual.LoadBalancers = []*ClassicLoadBalancer{} for _, lb := range g.LoadBalancerNames { - actual.LoadBalancers = append(actual.LoadBalancers, &ClassicLoadBalancer{ Name: aws.String(*lb), LoadBalancerName: aws.String(*lb), @@ -162,25 +161,19 @@ 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 { - parsed, err := arn.Parse(fi.StringValue(tg)) + targetGroupName, err := awsup.GetTargetGroupNameFromARN(fi.StringValue(tg)) if err != nil { - return nil, fmt.Errorf("error parsing target grup ARN: %v", err) + return nil, err } - resource := strings.Split(parsed.Resource, "/") - if len(resource) != 3 || resource[0] != "targetgroup" { - return nil, fmt.Errorf("error parsing target grup ARN resource: %q", parsed.Resource) + if targetGroupName != awsup.GetResourceName32(c.Cluster.Name, "tcp") && targetGroupName != awsup.GetResourceName32(c.Cluster.Name, "tls") { + actual.TargetGroups = append(actual.TargetGroups, &TargetGroup{ARN: aws.String(*tg), Name: aws.String(targetGroupName)}) + } else { + actual.TargetGroups = append(actual.TargetGroups, &TargetGroup{ARN: aws.String(*tg), Name: aws.String(fi.StringValue(g.AutoScalingGroupName) + "-" + targetGroupName)}) } - actualTGs = append(actualTGs, &TargetGroup{ARN: aws.String(*tg), Name: aws.String(resource[1])}) } - targetGroups, err := ReconcileTargetGroups(c.Cloud.(awsup.AWSCloud), actualTGs, e.TargetGroups) - if err != nil { - return nil, err - } - actual.TargetGroups = targetGroups - sort.Stable(OrderTargetGroupsByName(actual.TargetGroups)) } + sort.Stable(OrderTargetGroupsByName(actual.TargetGroups)) if g.VPCZoneIdentifier != nil { subnets := strings.Split(*g.VPCZoneIdentifier, ",") diff --git a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go index a6d4cd8839..f5c305d913 100644 --- a/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go +++ b/upup/pkg/fi/cloudup/awstasks/network_load_balancer.go @@ -382,7 +382,11 @@ func (e *NetworkLoadBalancer) Find(c *fi.Context) (*NetworkLoadBalancer, error) if len(l.DefaultActions) > 0 { targetGroupARN := l.DefaultActions[0].TargetGroupArn if targetGroupARN != nil { - actual.TargetGroups = append(actual.TargetGroups, &TargetGroup{ARN: targetGroupARN}) + targetGroupName, err := awsup.GetTargetGroupNameFromARN(fi.StringValue(targetGroupARN)) + if err != nil { + return nil, err + } + actual.TargetGroups = append(actual.TargetGroups, &TargetGroup{ARN: targetGroupARN, Name: fi.String(targetGroupName)}) cloud := c.Cloud.(awsup.AWSCloud) descResp, err := cloud.ELBV2().DescribeTargetGroups(&elbv2.DescribeTargetGroupsInput{ @@ -394,18 +398,12 @@ func (e *NetworkLoadBalancer) Find(c *fi.Context) (*NetworkLoadBalancer, error) 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) - if err != nil { - return nil, err - } - actual.TargetGroups = targetGroups - } sort.Stable(OrderTargetGroupsByName(actual.TargetGroups)) } diff --git a/upup/pkg/fi/cloudup/awstasks/targetgroup.go b/upup/pkg/fi/cloudup/awstasks/targetgroup.go index 9e0dc6494a..6b8b593ea3 100644 --- a/upup/pkg/fi/cloudup/awstasks/targetgroup.go +++ b/upup/pkg/fi/cloudup/awstasks/targetgroup.go @@ -84,6 +84,7 @@ func (e *TargetGroup) Find(c *fi.Context) (*TargetGroup, error) { tg := response.TargetGroups[0] actual := &TargetGroup{ + Name: tg.TargetGroupName, Port: tg.Port, Protocol: tg.Protocol, ARN: tg.TargetGroupArn, @@ -108,7 +109,6 @@ func (e *TargetGroup) Find(c *fi.Context) (*TargetGroup, error) { actual.Tags = tags // Prevent spurious changes - actual.Name = e.Name actual.Lifecycle = e.Lifecycle actual.Shared = e.Shared @@ -203,56 +203,6 @@ func (a OrderTargetGroupsByName) Less(i, j int) bool { 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, -// 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. -// -// This means some TargetGroup tasks have ARN set and others do not. -// When `Find`ing the ASG and recreating the TargetGroup tasks we need them to match how the model creates them, -// but we only know the ARNs, not the task names associated with them. -// This reuslts in spurious changes being reported during subsequent `update cluster` runs because the API TargetGroup task is named differently -// between the kops model and the ASG's `Find`. -// -// To prevent this, we need to update the API TargetGroup tasks in the ASG's TargetGroups list. -// 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) { - 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) { - apiTGTasks = append(apiTGTasks, tg) - } - } - - reconciled := make([]*TargetGroup, 0) - - if len(actual) == 0 { - return reconciled, nil - } - - apiTGs := make(map[string]*TargetGroup) - for _, task := range apiTGTasks { - apiTG, err := FindTargetGroupByName(cloud, fi.StringValue(task.Name)) - if err != nil { - return nil, err - } - if apiTG != nil { - apiTGs[aws.StringValue(apiTG.TargetGroupArn)] = task - } - } - for _, tg := range actual { - if apiTask, ok := apiTGs[aws.StringValue(tg.ARN)]; ok { - reconciled = append(reconciled, apiTask) - } else { - reconciled = append(reconciled, tg) - } - } - - return reconciled, nil -} - type terraformTargetGroup struct { Name string `json:"name" cty:"name"` Port int64 `json:"port" cty:"port"` diff --git a/upup/pkg/fi/cloudup/awstasks/targetgroup_test.go b/upup/pkg/fi/cloudup/awstasks/targetgroup_test.go deleted file mode 100644 index 2092b04894..0000000000 --- a/upup/pkg/fi/cloudup/awstasks/targetgroup_test.go +++ /dev/null @@ -1,106 +0,0 @@ -/* -Copyright 2020 The Kubernetes Authors. - -Licensed under the Apache License, Version 2.0 (the "License"); -you may not use this file except in compliance with the License. -You may obtain a copy of the License at - - http://www.apache.org/licenses/LICENSE-2.0 - -Unless required by applicable law or agreed to in writing, software -distributed under the License is distributed on an "AS IS" BASIS, -WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -See the License for the specific language governing permissions and -limitations under the License. -*/ - -package awstasks - -import ( - "fmt" - "reflect" - "testing" - - "github.com/aws/aws-sdk-go/aws" - "github.com/aws/aws-sdk-go/service/elbv2" - "k8s.io/kops/cloudmock/aws/mockelbv2" - "k8s.io/kops/upup/pkg/fi" - "k8s.io/kops/upup/pkg/fi/cloudup/awsup" -) - -func TestReconcileTargetGroups(t *testing.T) { - cases := []struct { - name string - actualTGs []*TargetGroup - expectedTGs []*TargetGroup - reconciledTGs []*TargetGroup - }{ - { - name: "with external TGs", - actualTGs: []*TargetGroup{ - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg2/2")}, - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg3/3")}, - }, - expectedTGs: []*TargetGroup{ - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg2/2"), Shared: fi.Bool(true)}, - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg3/3"), Shared: fi.Bool(true)}, - }, - reconciledTGs: []*TargetGroup{ - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg2/2")}, - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg3/3")}, - }, - }, - { - name: "with API TGs", - actualTGs: []*TargetGroup{ - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/api-foo1/1")}, - }, - expectedTGs: []*TargetGroup{ - {Name: aws.String("api-foo1"), Shared: fi.Bool(false)}, - }, - reconciledTGs: []*TargetGroup{ - {Name: aws.String("api-foo1"), Shared: fi.Bool(false)}, - }, - }, - { - name: "with API and external TGs", - actualTGs: []*TargetGroup{ - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/api-foo1/1")}, - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg2/2")}, - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg3/3")}, - }, - expectedTGs: []*TargetGroup{ - {Name: aws.String("api-foo1"), Shared: fi.Bool(false)}, - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg2/2"), Shared: fi.Bool(true)}, - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg3/3"), Shared: fi.Bool(true)}, - }, - reconciledTGs: []*TargetGroup{ - {Name: aws.String("api-foo1"), Shared: fi.Bool(false)}, - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg2/2")}, - {ARN: aws.String("arn:aws:elasticloadbalancing:us-test-1:000000000000:targetgroup/tg3/3")}, - }, - }, - } - cloud := awsup.BuildMockAWSCloud("us-east-1", "abc") - c := &mockelbv2.MockELBV2{} - cloud.MockELBV2 = c - c.CreateTargetGroup(&elbv2.CreateTargetGroupInput{Name: aws.String("api-foo1")}) - c.CreateTargetGroup(&elbv2.CreateTargetGroupInput{Name: aws.String("tg2")}) - c.CreateTargetGroup(&elbv2.CreateTargetGroupInput{Name: aws.String("tg3")}) - - resp, _ := c.DescribeTargetGroups(&elbv2.DescribeTargetGroupsInput{}) - fmt.Printf("%+v\n", resp) - for _, tc := range cases { - t.Run(tc.name, func(t *testing.T) { - actualReconciled, err := ReconcileTargetGroups(cloud, tc.actualTGs, tc.expectedTGs) - if err != nil { - t.Error(err) - t.Fail() - } - if !reflect.DeepEqual(actualReconciled, tc.reconciledTGs) { - t.Errorf("Reconciled TGs didn't match: %+v vs %+v\n", actualReconciled, tc.reconciledTGs) - t.Fail() - } - }) - } -} diff --git a/upup/pkg/fi/cloudup/awsup/aws_utils.go b/upup/pkg/fi/cloudup/awsup/aws_utils.go index 26e37c85ac..18edb6fdc8 100644 --- a/upup/pkg/fi/cloudup/awsup/aws_utils.go +++ b/upup/pkg/fi/cloudup/awsup/aws_utils.go @@ -17,11 +17,15 @@ limitations under the License. package awsup import ( + "encoding/base32" "fmt" + "hash/fnv" "os" + "strings" "sync" "github.com/aws/aws-sdk-go/aws" + "github.com/aws/aws-sdk-go/aws/arn" "github.com/aws/aws-sdk-go/aws/awserr" "github.com/aws/aws-sdk-go/aws/endpoints" "github.com/aws/aws-sdk-go/aws/session" @@ -197,3 +201,41 @@ func EC2TagSpecification(resourceType string, tags map[string]string) []*ec2.Tag return []*ec2.TagSpecification{specification} } + +// GetResourceName32 will attempt to calculate a meaningful name for a resource given a prefix +// Will never return a string longer than 32 chars +func GetResourceName32(cluster string, prefix string) string { + s := prefix + "-" + strings.Replace(cluster, ".", "-", -1) + + // We always compute the hash and add it, lest we trick users into assuming that we never do this + h := fnv.New32a() + if _, err := h.Write([]byte(s)); err != nil { + klog.Fatalf("error hashing values: %v", err) + } + hashString := base32.HexEncoding.EncodeToString(h.Sum(nil)) + hashString = strings.ToLower(hashString) + if len(hashString) > 6 { + hashString = hashString[:6] + } + + maxBaseLength := 32 - len(hashString) - 1 + if len(s) > maxBaseLength { + s = s[:maxBaseLength] + } + s = s + "-" + hashString + + return s +} + +// GetTargetGroupNameFromARN will attempt to parse a target group ARN and return its name +func GetTargetGroupNameFromARN(targetGroupARN string) (string, error) { + parsed, err := arn.Parse(targetGroupARN) + if err != nil { + return "", fmt.Errorf("error parsing target group ARN: %v", err) + } + resource := strings.Split(parsed.Resource, "/") + if len(resource) != 3 || resource[0] != "targetgroup" { + return "", fmt.Errorf("error parsing target group ARN resource: %q", parsed.Resource) + } + return resource[1], nil +} diff --git a/upup/pkg/fi/cloudup/awsup/aws_utils_test.go b/upup/pkg/fi/cloudup/awsup/aws_utils_test.go index 8dbc4064f7..ac71b539cd 100644 --- a/upup/pkg/fi/cloudup/awsup/aws_utils_test.go +++ b/upup/pkg/fi/cloudup/awsup/aws_utils_test.go @@ -105,3 +105,38 @@ func TestEC2TagSpecification(t *testing.T) { }) } } + +func Test_GetResourceName32(t *testing.T) { + grid := []struct { + ClusterName string + Prefix string + Expected string + }{ + { + "mycluster", + "bastion", + "bastion-mycluster-vnrjie", + }, + { + "mycluster.example.com", + "bastion", + "bastion-mycluster-example-o8elkm", + }, + { + "this.is.a.very.long.cluster.example.com", + "api", + "api-this-is-a-very-long-c-q4ukp4", + }, + { + "this.is.a.very.long.cluster.example.com", + "bastion", + "bastion-this-is-a-very-lo-4ggpa2", + }, + } + for _, g := range grid { + actual := GetResourceName32(g.ClusterName, g.Prefix) + if actual != g.Expected { + t.Errorf("unexpected result from %q+%q. expected %q, got %q", g.Prefix, g.ClusterName, g.Expected, actual) + } + } +}