From 0700f9b17190300a0789b6b007154c08604e83db Mon Sep 17 00:00:00 2001 From: Rafael Fonseca Date: Wed, 12 Jun 2024 22:11:49 +0200 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B:=20elbv2:=20fix=20checking=20f?= =?UTF-8?q?or=20existing=20listeners?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We were comparing pointers that were never going to be equal. Let's check their pointed-to values instead. Also added a break when the listener is found. Changed the `createdListeners` list to only include a listener if it was created. This list is currently not used for anything, so this change should have no impact. --- pkg/cloud/services/elb/loadbalancer.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go index bb44a8629e..47faff7a54 100644 --- a/pkg/cloud/services/elb/loadbalancer.go +++ b/pkg/cloud/services/elb/loadbalancer.go @@ -1604,8 +1604,9 @@ func (s *Service) reconcileTargetGroupsAndListeners(lbARN string, spec *infrav1. var listener *elbv2.Listener for _, l := range existingListeners.Listeners { - if l.DefaultActions != nil && len(l.DefaultActions) > 0 && l.DefaultActions[0].TargetGroupArn == group.TargetGroupArn { + if l.DefaultActions != nil && len(l.DefaultActions) > 0 && *l.DefaultActions[0].TargetGroupArn == *group.TargetGroupArn { listener = l + break } } @@ -1614,9 +1615,8 @@ func (s *Service) reconcileTargetGroupsAndListeners(lbARN string, spec *infrav1. if err != nil { return nil, nil, err } + createdListeners = append(createdListeners, listener) } - - createdListeners = append(createdListeners, listener) } return createdTargetGroups, createdListeners, nil From be21aace4adece81d0e2b91307330ed7b171cf86 Mon Sep 17 00:00:00 2001 From: Rafael Fonseca Date: Wed, 12 Jun 2024 22:20:11 +0200 Subject: [PATCH 2/2] =?UTF-8?q?=F0=9F=90=9B:=20elbv2:=20fix=20target=20gro?= =?UTF-8?q?up=20creation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We cannot compare target groups by name because every time we get the desired spec, it contains newly-generated random names. Instead, let's check that the prefixes match and that the port and protocol properties are the same. --- pkg/cloud/services/elb/loadbalancer.go | 37 +++++++++++++++++++++++--- 1 file changed, 34 insertions(+), 3 deletions(-) diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go index 47faff7a54..c7beb578d4 100644 --- a/pkg/cloud/services/elb/loadbalancer.go +++ b/pkg/cloud/services/elb/loadbalancer.go @@ -35,6 +35,7 @@ import ( kerrors "k8s.io/apimachinery/pkg/util/errors" "k8s.io/apimachinery/pkg/util/sets" "k8s.io/apiserver/pkg/storage/names" + "k8s.io/utils/ptr" infrav1 "sigs.k8s.io/cluster-api-provider-aws/v2/api/v1beta2" "sigs.k8s.io/cluster-api-provider-aws/v2/pkg/cloud/awserrors" @@ -55,6 +56,14 @@ const elbResourceType = "elasticloadbalancing:loadbalancer" // see: https://docs.aws.amazon.com/elasticloadbalancing/2012-06-01/APIReference/API_DescribeTags.html const maxELBsDescribeTagsRequest = 20 +// apiServerTargetGroupPrefix is the target group name prefix used when creating a target group for the API server +// listener. +const apiServerTargetGroupPrefix = "apiserver-target-" + +// additionalTargetGroupPrefix is the target group name prefix used when creating target groups for additional +// listeners. +const additionalTargetGroupPrefix = "additional-listener-" + // ReconcileLoadbalancers reconciles the load balancers for the given cluster. func (s *Service) ReconcileLoadbalancers() error { s.scope.Debug("Reconciling load balancers") @@ -271,7 +280,7 @@ func (s *Service) getAPIServerLBSpec(elbName string, lbSpec *infrav1.AWSLoadBala Protocol: infrav1.ELBProtocolTCP, Port: infrav1.DefaultAPIServerPort, TargetGroup: infrav1.TargetGroupSpec{ - Name: names.SimpleNameGenerator.GenerateName("apiserver-target-"), + Name: names.SimpleNameGenerator.GenerateName(apiServerTargetGroupPrefix), Port: infrav1.DefaultAPIServerPort, Protocol: infrav1.ELBProtocolTCP, VpcID: s.scope.VPC().ID, @@ -296,7 +305,7 @@ func (s *Service) getAPIServerLBSpec(elbName string, lbSpec *infrav1.AWSLoadBala Protocol: listener.Protocol, Port: listener.Port, TargetGroup: infrav1.TargetGroupSpec{ - Name: names.SimpleNameGenerator.GenerateName("additional-listener-"), + Name: names.SimpleNameGenerator.GenerateName(additionalTargetGroupPrefix), Port: listener.Port, Protocol: listener.Protocol, VpcID: s.scope.VPC().ID, @@ -1573,9 +1582,11 @@ func (s *Service) reconcileTargetGroupsAndListeners(lbARN string, spec *infrav1. // https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues/3899 for _, ln := range spec.ELBListeners { var group *elbv2.TargetGroup + tgSpec := ln.TargetGroup for _, g := range existingTargetGroups.TargetGroups { - if *g.TargetGroupName == ln.TargetGroup.Name { + if isSDKTargetGroupEqualToTargetGroup(g, &tgSpec) { group = g + break } } // create the target group first @@ -1785,3 +1796,23 @@ func shouldReconcileSGs(scope scope.ELBScope, lb *infrav1.LoadBalancer, specSGs } return true } + +// isSDKTargetGroupEqualToTargetGroup checks if a given AWS SDK target group matches a target group spec. +func isSDKTargetGroupEqualToTargetGroup(elbTG *elbv2.TargetGroup, spec *infrav1.TargetGroupSpec) bool { + // We can't check only the target group's name because it's randomly generated every time we get a spec + // But CAPA-created target groups are guaranteed to have the "apiserver-target-" or "additional-listener-" prefix. + switch { + case strings.HasPrefix(*elbTG.TargetGroupName, apiServerTargetGroupPrefix): + if !strings.HasPrefix(spec.Name, apiServerTargetGroupPrefix) { + return false + } + case strings.HasPrefix(*elbTG.TargetGroupName, additionalTargetGroupPrefix): + if !strings.HasPrefix(spec.Name, additionalTargetGroupPrefix) { + return false + } + default: + // Not created by CAPA + return false + } + return ptr.Deref(elbTG.Port, 0) == spec.Port && strings.EqualFold(*elbTG.Protocol, spec.Protocol.String()) +}