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]>

Author: nrb

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
+}

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 {