Skip to content

🐛 Reconcile target groups and listeners as their own entities #5004

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

Merged
merged 6 commits into from
Jun 11, 2024
Merged

🐛 Reconcile target groups and listeners as their own entities #5004

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
16a6004
Reconcile target groups and listeners separately
nrb Jun 5, 2024
0991311
Look up existing target groups and listeners
nrb Jun 6, 2024
158fc75
Update unit tests for target groups and listeners
nrb Jun 6, 2024
a49371d
Fix linting issues
nrb Jun 6, 2024
34edeed
Correct arguments and clarify var names
nrb Jun 7, 2024
cc2cfe4
Remove incorrect optimization and fixup tests
nrb Jun 10, 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
2 changes: 2 additions & 0 deletions controllers/awscluster_controller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -303,6 +303,8 @@ func TestAWSClusterReconcilerIntegrationTests(t *testing.T) {
mockedCreateLBV2Calls(t, e)
mockedDescribeInstanceCall(m)
mockedDescribeAvailabilityZones(m, []string{"us-east-1c", "us-east-1a"})
mockedDescribeTargetGroupsCall(t, e)
mockedDescribeListenersCall(t, e)
}

expect(ec2Mock.EXPECT(), elbv2Mock.EXPECT())
Expand Down
47 changes: 47 additions & 0 deletions controllers/helpers_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -291,6 +291,53 @@ func mockedCreateLBV2Calls(t *testing.T, m *mocks.MockELBV2APIMockRecorder) {
})).MaxTimes(1)
}

func mockedDescribeTargetGroupsCall(t *testing.T, m *mocks.MockELBV2APIMockRecorder) {
t.Helper()
m.DescribeTargetGroups(gomock.Eq(&elbv2.DescribeTargetGroupsInput{
LoadBalancerArn: lbArn,
})).
Return(&elbv2.DescribeTargetGroupsOutput{
NextMarker: new(string),
TargetGroups: []*elbv2.TargetGroup{
{
HealthCheckEnabled: aws.Bool(true),
HealthCheckIntervalSeconds: new(int64),
HealthCheckPath: new(string),
HealthCheckPort: new(string),
HealthCheckProtocol: new(string),
HealthCheckTimeoutSeconds: new(int64),
HealthyThresholdCount: new(int64),
IpAddressType: new(string),
LoadBalancerArns: []*string{lbArn},
Matcher: &elbv2.Matcher{},
Port: new(int64),
Protocol: new(string),
ProtocolVersion: new(string),
TargetGroupArn: aws.String("arn::targetgroup"),
TargetGroupName: new(string),
TargetType: new(string),
UnhealthyThresholdCount: new(int64),
VpcId: new(string),
}},
}, nil)
}

func mockedDescribeListenersCall(t *testing.T, m *mocks.MockELBV2APIMockRecorder) {
t.Helper()
m.DescribeListeners(gomock.Eq(&elbv2.DescribeListenersInput{
LoadBalancerArn: lbArn,
})).
Return(&elbv2.DescribeListenersOutput{
Listeners: []*elbv2.Listener{{
DefaultActions: []*elbv2.Action{{
TargetGroupArn: aws.String("arn::targetgroup"),
}},
ListenerArn: aws.String("arn::listener"),
LoadBalancerArn: lbArn,
}},
}, nil)
}

func mockedDeleteLBCalls(expectV2Call bool, mv2 *mocks.MockELBV2APIMockRecorder, m *mocks.MockELBAPIMockRecorder) {
if expectV2Call {
mv2.DescribeLoadBalancers(gomock.Any()).Return(describeLBOutputV2, nil)
Expand Down
248 changes: 165 additions & 83 deletions pkg/cloud/services/elb/loadbalancer.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,6 +112,12 @@ func (s *Service) reconcileV2LB(lbSpec *infrav1.AWSLoadBalancerSpec) error {
// set up the type for later processing
lb.LoadBalancerType = lbSpec.LoadBalancerType
if lb.IsManaged(s.scope.Name()) {
// Reconcile the target groups and listeners from the spec and the ones currently attached to the load balancer.
_, _, err := s.reconcileTargetGroupsAndListeners(lb, lbSpec)
if err != nil {
return errors.Wrapf(err, "failed to create target groups/listeners for load balancer %q", lb.Name)
}

if !cmp.Equal(spec.ELBAttributes, lb.ELBAttributes) {
if err := s.configureLBAttributes(lb.ARN, spec.ELBAttributes); err != nil {
return err
Expand Down Expand Up @@ -388,89 +394,7 @@ func (s *Service) createLB(spec *infrav1.LoadBalancer, lbSpec *infrav1.AWSLoadBa
return nil, errors.New("no new network load balancer was created; the returned list is empty")
}

// TODO(Skarlso): Add options to set up SSL.
// https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues/3899
for _, ln := range spec.ELBListeners {
// create the target group first
targetGroupInput := &elbv2.CreateTargetGroupInput{
Name: aws.String(ln.TargetGroup.Name),
Port: aws.Int64(ln.TargetGroup.Port),
Protocol: aws.String(ln.TargetGroup.Protocol.String()),
VpcId: aws.String(ln.TargetGroup.VpcID),
Tags: input.Tags,
HealthCheckIntervalSeconds: aws.Int64(infrav1.DefaultAPIServerHealthCheckIntervalSec),
HealthCheckTimeoutSeconds: aws.Int64(infrav1.DefaultAPIServerHealthCheckTimeoutSec),
HealthyThresholdCount: aws.Int64(infrav1.DefaultAPIServerHealthThresholdCount),
UnhealthyThresholdCount: aws.Int64(infrav1.DefaultAPIServerUnhealthThresholdCount),
}
if s.scope.VPC().IsIPv6Enabled() {
targetGroupInput.IpAddressType = aws.String("ipv6")
}
if ln.TargetGroup.HealthCheck != nil {
targetGroupInput.HealthCheckEnabled = aws.Bool(true)
targetGroupInput.HealthCheckProtocol = ln.TargetGroup.HealthCheck.Protocol
targetGroupInput.HealthCheckPort = ln.TargetGroup.HealthCheck.Port
if ln.TargetGroup.HealthCheck.Path != nil {
targetGroupInput.HealthCheckPath = ln.TargetGroup.HealthCheck.Path
}
if ln.TargetGroup.HealthCheck.IntervalSeconds != nil {
targetGroupInput.HealthCheckIntervalSeconds = ln.TargetGroup.HealthCheck.IntervalSeconds
}
if ln.TargetGroup.HealthCheck.TimeoutSeconds != nil {
targetGroupInput.HealthCheckTimeoutSeconds = ln.TargetGroup.HealthCheck.TimeoutSeconds
}
if ln.TargetGroup.HealthCheck.ThresholdCount != nil {
targetGroupInput.HealthyThresholdCount = ln.TargetGroup.HealthCheck.ThresholdCount
}
if ln.TargetGroup.HealthCheck.UnhealthyThresholdCount != nil {
targetGroupInput.UnhealthyThresholdCount = ln.TargetGroup.HealthCheck.UnhealthyThresholdCount
}
}
s.scope.Debug("creating target group", "group", targetGroupInput, "listener", ln)
group, err := s.ELBV2Client.CreateTargetGroup(targetGroupInput)
if err != nil {
return nil, errors.Wrapf(err, "failed to create target group for load balancer")
}
if len(group.TargetGroups) == 0 {
return nil, errors.New("no target group was created; the returned list is empty")
}

if !lbSpec.PreserveClientIP {
targetGroupAttributeInput := &elbv2.ModifyTargetGroupAttributesInput{
TargetGroupArn: group.TargetGroups[0].TargetGroupArn,
Attributes: []*elbv2.TargetGroupAttribute{
{
Key: aws.String(infrav1.TargetGroupAttributeEnablePreserveClientIP),
Value: aws.String("false"),
},
},
}
if _, err := s.ELBV2Client.ModifyTargetGroupAttributes(targetGroupAttributeInput); err != nil {
return nil, errors.Wrapf(err, "failed to modify target group attribute")
}
}

listenerInput := &elbv2.CreateListenerInput{
DefaultActions: []*elbv2.Action{
{
TargetGroupArn: group.TargetGroups[0].TargetGroupArn,
Type: aws.String(elbv2.ActionTypeEnumForward),
},
},
LoadBalancerArn: out.LoadBalancers[0].LoadBalancerArn,
Port: aws.Int64(ln.Port),
Protocol: aws.String(string(ln.Protocol)),
Tags: converters.MapToV2Tags(spec.Tags),
}
// Create ClassicELBListeners
listener, err := s.ELBV2Client.CreateListener(listenerInput)
if err != nil {
return nil, errors.Wrap(err, "failed to create listener")
}
if len(listener.Listeners) == 0 {
return nil, errors.New("no listener was created; the returned list is empty")
}
}
// Target Groups and listeners will be reconciled separately

s.scope.Info("Created network load balancer", "dns-name", *out.LoadBalancers[0].DNSName)

Expand Down Expand Up @@ -1604,6 +1528,164 @@ func (s *Service) reconcileV2LBTags(lb *infrav1.LoadBalancer, desiredTags map[st
return nil
}

// reconcileTargetGroupsAndListeners reconciles a Load Balancer's defined listeners with corresponding AWS Target Groups and Listeners.
// These are combined into a single function since they are tightly integrated.
func (s *Service) reconcileTargetGroupsAndListeners(spec *infrav1.LoadBalancer, lbSpec *infrav1.AWSLoadBalancerSpec) ([]*elbv2.TargetGroup, []*elbv2.Listener, error) {
existingTargetGroups, err := s.ELBV2Client.DescribeTargetGroups(
&elbv2.DescribeTargetGroupsInput{
LoadBalancerArn: aws.String(spec.ARN),
})
if err != nil {
s.scope.Error(err, "could not describe target groups for load balancer", "arn", spec.ARN)
return nil, nil, err
}

existingListeners, err := s.ELBV2Client.DescribeListeners(
&elbv2.DescribeListenersInput{
LoadBalancerArn: aws.String(spec.ARN),
})
if err != nil {
s.scope.Error(err, "could not describe listeners for load balancer", "arn", spec.ARN)
}

if len(existingTargetGroups.TargetGroups) == len(existingListeners.Listeners) && len(existingListeners.Listeners) == len(spec.ELBListeners) {
return existingTargetGroups.TargetGroups, existingListeners.Listeners, nil
}
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a comment on why this is necessary? Is this an optimization?

Also is this condition always a guarantee there is nothing to do?
Especially looking at, could there be a case where the count is off and doing g.TargetGroupName == ln.TargetGroup.Name naming matching would be better?

len(existingTargetGroups.TargetGroups) == len(existingListeners.Listeners)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it was an attempt to optimize so we don't go through the full loop every time.

Matching on the name might be better. I suppose it's possible that a user could create more target groups in AWS than CAPA knows about, though I'm not sure why they'd do that.

In any case, if we check that the target group names line up between AWS and our spec, we're still repeating the loop every time. The main benefit there would be that we could potentially skip doing more API calls within a single iteration.

Copy link
Contributor

Choose a reason for hiding this comment

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

One reason we saw the LB creation fail was because of API call rate limiting in very busy envs, so avoiding extra calls when possible would be nice.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thinking this through some more, I think the only numeric comparison we can do that's valid all the time is if len(existingTargetGroups) < len(desiredTargetGroups) { reconcile }. Even if the lists are equal in length, it doesn't guarantee that they're the exact same names.

I'll look through this and see if I can do any further optimizations, but I'll at least remove this length check.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the productive back-and-forth here.

Please consider a future PR for optimizations, because this PR is already complex (although the new tests are very helpful, they themselves are complex). 🙏


createdTargetGroups := make([]*elbv2.TargetGroup, 0, len(spec.ELBListeners))
createdListeners := make([]*elbv2.Listener, 0, len(spec.ELBListeners))

// TODO(Skarlso): Add options to set up SSL.
// https://github.com/kubernetes-sigs/cluster-api-provider-aws/issues/3899
for _, ln := range spec.ELBListeners {
var group *elbv2.TargetGroup
for _, g := range existingTargetGroups.TargetGroups {
if *g.TargetGroupName == ln.TargetGroup.Name {
group = g
}
}
// create the target group first
if group == nil {
group, err = s.createTargetGroup(ln, spec.Tags)
if err != nil {
return nil, nil, err
}
createdTargetGroups = append(createdTargetGroups, group)

if !lbSpec.PreserveClientIP {
targetGroupAttributeInput := &elbv2.ModifyTargetGroupAttributesInput{
TargetGroupArn: group.TargetGroupArn,
Attributes: []*elbv2.TargetGroupAttribute{
{
Key: aws.String(infrav1.TargetGroupAttributeEnablePreserveClientIP),
Value: aws.String("false"),
},
},
}
if _, err := s.ELBV2Client.ModifyTargetGroupAttributes(targetGroupAttributeInput); err != nil {
return nil, nil, errors.Wrapf(err, "failed to modify target group attribute")
}
}
}

var listener *elbv2.Listener
for _, l := range existingListeners.Listeners {
if l.DefaultActions != nil && len(l.DefaultActions) > 0 && l.DefaultActions[0].TargetGroupArn == group.TargetGroupArn {
listener = l
}
}

if listener == nil {
listener, err = s.createListener(ln, group, spec.ARN, spec.Tags)
if err != nil {
return nil, nil, err
}
}

createdListeners = append(createdListeners, listener)
}

return createdTargetGroups, createdListeners, nil
}

// createListener creates a single Listener.
func (s *Service) createListener(ln infrav1.Listener, group *elbv2.TargetGroup, lbARN string, tags map[string]string) (*elbv2.Listener, error) {
listenerInput := &elbv2.CreateListenerInput{
DefaultActions: []*elbv2.Action{
{
TargetGroupArn: group.TargetGroupArn,
Type: aws.String(elbv2.ActionTypeEnumForward),
},
},
LoadBalancerArn: aws.String(lbARN),
Port: aws.Int64(ln.Port),
Protocol: aws.String(string(ln.Protocol)),
Tags: converters.MapToV2Tags(tags),
}
// Create ClassicELBListeners
listener, err := s.ELBV2Client.CreateListener(listenerInput)
if err != nil {
return nil, errors.Wrap(err, "failed to create listener")
}
if len(listener.Listeners) == 0 {
return nil, errors.New("no listener was created; the returned list is empty")
}
if len(listener.Listeners) > 1 {
return nil, errors.New("more than one listener created; expected only one")
}
return listener.Listeners[0], nil
}

// createTargetGroup creates a single Target Group.
func (s *Service) createTargetGroup(ln infrav1.Listener, tags map[string]string) (*elbv2.TargetGroup, error) {
targetGroupInput := &elbv2.CreateTargetGroupInput{
Name: aws.String(ln.TargetGroup.Name),
Port: aws.Int64(ln.TargetGroup.Port),
Protocol: aws.String(ln.TargetGroup.Protocol.String()),
VpcId: aws.String(ln.TargetGroup.VpcID),
Tags: converters.MapToV2Tags(tags),
HealthCheckIntervalSeconds: aws.Int64(infrav1.DefaultAPIServerHealthCheckIntervalSec),
HealthCheckTimeoutSeconds: aws.Int64(infrav1.DefaultAPIServerHealthCheckTimeoutSec),
HealthyThresholdCount: aws.Int64(infrav1.DefaultAPIServerHealthThresholdCount),
UnhealthyThresholdCount: aws.Int64(infrav1.DefaultAPIServerUnhealthThresholdCount),
}
if s.scope.VPC().IsIPv6Enabled() {
targetGroupInput.IpAddressType = aws.String("ipv6")
}
if ln.TargetGroup.HealthCheck != nil {
targetGroupInput.HealthCheckEnabled = aws.Bool(true)
targetGroupInput.HealthCheckProtocol = ln.TargetGroup.HealthCheck.Protocol
targetGroupInput.HealthCheckPort = ln.TargetGroup.HealthCheck.Port
if ln.TargetGroup.HealthCheck.Path != nil {
targetGroupInput.HealthCheckPath = ln.TargetGroup.HealthCheck.Path
}
if ln.TargetGroup.HealthCheck.IntervalSeconds != nil {
targetGroupInput.HealthCheckIntervalSeconds = ln.TargetGroup.HealthCheck.IntervalSeconds
}
if ln.TargetGroup.HealthCheck.TimeoutSeconds != nil {
targetGroupInput.HealthCheckTimeoutSeconds = ln.TargetGroup.HealthCheck.TimeoutSeconds
}
if ln.TargetGroup.HealthCheck.ThresholdCount != nil {
targetGroupInput.HealthyThresholdCount = ln.TargetGroup.HealthCheck.ThresholdCount
}
if ln.TargetGroup.HealthCheck.UnhealthyThresholdCount != nil {
targetGroupInput.UnhealthyThresholdCount = ln.TargetGroup.HealthCheck.UnhealthyThresholdCount
}
}
s.scope.Debug("creating target group", "group", targetGroupInput, "listener", ln)
group, err := s.ELBV2Client.CreateTargetGroup(targetGroupInput)
if err != nil {
return nil, errors.Wrapf(err, "failed to create target group for load balancer")
}
if len(group.TargetGroups) == 0 {
return nil, errors.New("no target group was created; the returned list is empty")
}
if len(group.TargetGroups) > 1 {
return nil, errors.New("more than one target group created; expected only one")
}
return group.TargetGroups[0], nil
}

func (s *Service) getHealthCheckTarget() string {
controlPlaneELB := s.scope.ControlPlaneLoadBalancer()
protocol := &infrav1.ELBProtocolSSL
Expand Down
Loading