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 <[email protected]>
kubernetes-sigs · Feb 20, 2024 · 23828d5 · 23828d5
1 parent 50a7f5b
commit 23828d5
Show file tree

Hide file tree

Showing 2 changed files with 112 additions and 6 deletions.
diff --git 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
@@ -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 {