From 23828d5f53672466015c46857f96858b1b9083b6 Mon Sep 17 00:00:00 2001 From: Nolan Brubaker Date: Thu, 15 Feb 2024 14:27:12 -0500 Subject: [PATCH] Don't reconcile security groups on older NLBs Network load balancers created under older versions of CAPI did not have security groups added at creation, and therefore could not have any security groups set at all. Allow NLBs created in this configuration to continue reconciling. Signed-off-by: Nolan Brubaker --- pkg/cloud/services/elb/loadbalancer.go | 26 ++++-- pkg/cloud/services/elb/loadbalancer_test.go | 92 +++++++++++++++++++++ 2 files changed, 112 insertions(+), 6 deletions(-) diff --git a/pkg/cloud/services/elb/loadbalancer.go b/pkg/cloud/services/elb/loadbalancer.go index d962b7aefe..7094ac849d 100644 --- a/pkg/cloud/services/elb/loadbalancer.go +++ b/pkg/cloud/services/elb/loadbalancer.go @@ -137,7 +137,7 @@ func (s *Service) reconcileV2LB(lbSpec *infrav1.AWSLoadBalancerSpec) error { } // Reconcile the security groups from the spec and the ones currently attached to the load balancer - if !sets.NewString(lb.SecurityGroupIDs...).Equal(sets.NewString(spec.SecurityGroupIDs...)) { + if shouldReconcileSGs(s.scope, lb, spec.SecurityGroupIDs) { _, err := s.ELBV2Client.SetSecurityGroups(&elbv2.SetSecurityGroupsInput{ LoadBalancerArn: &lb.ARN, SecurityGroups: aws.StringSlice(spec.SecurityGroupIDs), @@ -1536,11 +1536,11 @@ func fromSDKTypeToLB(v *elbv2.LoadBalancer, attrs []*elbv2.LoadBalancerAttribute availabilityZones[i] = az.ZoneName } res := &infrav1.LoadBalancer{ - ARN: aws.StringValue(v.LoadBalancerArn), - Name: aws.StringValue(v.LoadBalancerName), - Scheme: infrav1.ELBScheme(aws.StringValue(v.Scheme)), - SubnetIDs: aws.StringValueSlice(subnetIds), - // SecurityGroupIDs: aws.StringValueSlice(v.SecurityGroups), + ARN: aws.StringValue(v.LoadBalancerArn), + Name: aws.StringValue(v.LoadBalancerName), + Scheme: infrav1.ELBScheme(aws.StringValue(v.Scheme)), + SubnetIDs: aws.StringValueSlice(subnetIds), + SecurityGroupIDs: aws.StringValueSlice(v.SecurityGroups), AvailabilityZones: aws.StringValueSlice(availabilityZones), DNSName: aws.StringValue(v.DNSName), Tags: converters.V2TagsToMap(tags), @@ -1567,3 +1567,17 @@ func chunkELBs(names []string) [][]string { } return chunked } + +func shouldReconcileSGs(scope scope.ELBScope, lb *infrav1.LoadBalancer, specSGs []string) bool { + // Backwards compat: NetworkLoadBalancers were not always capable of having security groups attached. + // Once created without a security group, the NLB can never have any added. + // (https://docs.aws.amazon.com/elasticloadbalancing/latest/network/load-balancer-security-groups.html) + if lb.LoadBalancerType == infrav1.LoadBalancerTypeNLB && len(lb.SecurityGroupIDs) == 0 { + scope.Info("Pre-existing NLB %s without security groups, cannot reconcile security groups.", lb.Name) + return false + } + if !sets.NewString(lb.SecurityGroupIDs...).Equal(sets.NewString(specSGs...)) { + return true + } + return true +} diff --git a/pkg/cloud/services/elb/loadbalancer_test.go b/pkg/cloud/services/elb/loadbalancer_test.go index c1f91613c9..4762edc251 100644 --- a/pkg/cloud/services/elb/loadbalancer_test.go +++ b/pkg/cloud/services/elb/loadbalancer_test.go @@ -1847,6 +1847,96 @@ func TestReconcileV2LB(t *testing.T) { } }, }, + { + name: "ensure NLB without SGs doesn't attempt to add new SGs", + spec: func(spec infrav1.LoadBalancer) infrav1.LoadBalancer { + return spec + }, + awsCluster: func(acl infrav1.AWSCluster) infrav1.AWSCluster { + acl.Spec.ControlPlaneLoadBalancer.Name = aws.String(elbName) + acl.Spec.ControlPlaneLoadBalancer.LoadBalancerType = infrav1.LoadBalancerTypeNLB + acl.Spec.ControlPlaneLoadBalancer.AdditionalSecurityGroups = []string{"sg-001"} + return acl + }, + elbV2APIMocks: func(m *mocks.MockELBV2APIMockRecorder) { + m.DescribeLoadBalancers(gomock.Eq(&elbv2.DescribeLoadBalancersInput{ + Names: aws.StringSlice([]string{elbName}), + })). + Return(&elbv2.DescribeLoadBalancersOutput{ + LoadBalancers: []*elbv2.LoadBalancer{ + { + LoadBalancerArn: aws.String(elbArn), + LoadBalancerName: aws.String(elbName), + Scheme: aws.String(string(infrav1.ELBSchemeInternetFacing)), + AvailabilityZones: []*elbv2.AvailabilityZone{ + { + SubnetId: aws.String(clusterSubnetID), + ZoneName: aws.String(az), + }, + }, + VpcId: aws.String(vpcID), + }, + }, + }, nil) + m.ModifyLoadBalancerAttributes(&elbv2.ModifyLoadBalancerAttributesInput{ + LoadBalancerArn: aws.String(elbArn), + Attributes: []*elbv2.LoadBalancerAttribute{ + { + Key: aws.String("load_balancing.cross_zone.enabled"), + Value: aws.String("false"), + }, + }}). + Return(&elbv2.ModifyLoadBalancerAttributesOutput{}, nil) + m.DescribeLoadBalancerAttributes(&elbv2.DescribeLoadBalancerAttributesInput{LoadBalancerArn: aws.String(elbArn)}).Return( + &elbv2.DescribeLoadBalancerAttributesOutput{ + Attributes: []*elbv2.LoadBalancerAttribute{ + { + Key: aws.String("load_balancing.cross_zone.enabled"), + Value: aws.String("false"), + }, + { + Key: aws.String(infrav1.ClusterTagKey(clusterName)), + Value: aws.String(string(infrav1.ResourceLifecycleOwned)), + }, + }, + }, + nil, + ) + m.DescribeTags(&elbv2.DescribeTagsInput{ResourceArns: []*string{aws.String(elbArn)}}).Return( + &elbv2.DescribeTagsOutput{ + TagDescriptions: []*elbv2.TagDescription{ + { + ResourceArn: aws.String(elbArn), + Tags: []*elbv2.Tag{ + { + Key: aws.String(infrav1.ClusterTagKey(clusterName)), + Value: aws.String(string(infrav1.ResourceLifecycleOwned)), + }, + }, + }, + }, + }, + nil, + ) + + // Avoid the need to sort the AddTagsInput.Tags slice + m.AddTags(gomock.AssignableToTypeOf(&elbv2.AddTagsInput{})).Return(&elbv2.AddTagsOutput{}, nil) + + m.SetSubnets(&elbv2.SetSubnetsInput{ + LoadBalancerArn: aws.String(elbArn), + Subnets: []*string{}, + }).Return(&elbv2.SetSubnetsOutput{}, nil) + }, + check: func(t *testing.T, lb *infrav1.LoadBalancer, err error) { + t.Helper() + if err != nil { + t.Fatalf("did not expect error: %v", err) + } + if len(lb.SecurityGroupIDs) != 0 { + t.Errorf("Expected LB to contain 0 security groups, got %v", len(lb.SecurityGroupIDs)) + } + }, + }, } for _, tc := range tests { @@ -1897,6 +1987,7 @@ func TestReconcileV2LB(t *testing.T) { } err = s.reconcileV2LB(clusterScope.ControlPlaneLoadBalancer()) lb := s.scope.Network().APIServerELB + tc.check(t, &lb, err) }) } @@ -2886,6 +2977,7 @@ func TestGetHealthCheckProtocol(t *testing.T) { }) } } + func setupScheme() (*runtime.Scheme, error) { scheme := runtime.NewScheme() if err := clusterv1.AddToScheme(scheme); err != nil {