Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

🐛 Fix creation of target groups and listeners in the reconcile loop #5017

Merged
merged 2 commits into from
Jun 13, 2024
Merged

🐛 Fix creation of target groups and listeners in the reconcile loop #5017

Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0700f9b
🐛: elbv2: fix checking for existing listeners
r4f4 Jun 12, 2024
be21aac
🐛: elbv2: fix target group creation
r4f4 Jun 12, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
43 changes: 37 additions & 6 deletions pkg/cloud/services/elb/loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Expand All @@ -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")
Expand Down Expand Up @@ -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,
Expand All @@ -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,
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -1604,8 +1615,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 {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

According to https://docs.aws.amazon.com/sdk-for-go/api/service/elbv2/#ELBV2.CreateListener:

This operation is idempotent, which means that it completes at most one time. If you attempt to create multiple listeners with the same settings, each call succeeds.

So we could probably drop this check for an existing listener and always try createListener.

listener = l
break
}
}

Expand All @@ -1614,9 +1626,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
Expand Down Expand Up @@ -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())
}