Merge pull request #10335 from hakman/same-tg-multiple-igs

Allow attaching same external target group to multiple instance groups

pkg/model/BUILD.bazel

@@ -68,15 +68,13 @@ go_test(
     name = "go_default_test",
     srcs = [
         "bootstrapscript_test.go",
-        "context_test.go",
         "firewall_test.go",
     ],
     data = glob(["tests/**"]),  #keep
     embed = [":go_default_library"],
     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",

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",

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 {

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

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 {

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

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

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 {

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"
         ]

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

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]
 }
 

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",
     ],
 )