feat: accept + enforce customer security groups on NLBs (#454)

NLBs hard-rejected customer security groups and always minted a managed SG that
auto-opened listener ports, so aws-load-balancer-controller (which assigns SGs to
type=external NLBs) got a 400 and looped. Accept up to 5 customer SGs on an NLB:
when supplied they replace the managed SG on the ENIs and the caller owns the
listener-port rules (CreateListener already no-ops its auto-open when there is no
managed SG); without them the managed-SG default is unchanged. SetSecurityGroups
follows AWS create-time semantics — replace SGs on an NLB created with SGs, reject
adding them to one created without.

spinifex/handlers/elbv2/service_impl.go

@@ -1117,8 +1117,10 @@ func (s *ELBv2ServiceImpl) createLoadBalancer(input *elbv2.CreateLoadBalancerInp
 		}
 	}
 
-	// NLBs do not support security groups.
-	if lbType == LoadBalancerTypeNetwork && len(input.SecurityGroups) > 0 {
+	// AWS caps a load balancer at 5 security groups (ALBs always; NLBs since Aug
+	// 2023). NLBs with customer SGs skip the managed SG and let the caller own the
+	// listener-port rules; NLBs without SGs keep the managed-SG default below.
+	if len(input.SecurityGroups) > maxLBSecurityGroups {
 		return nil, errors.New(awserrors.ErrorELBv2InvalidConfigurationRequest)
 	}
 
@@ -1170,11 +1172,12 @@ func (s *ELBv2ServiceImpl) createLoadBalancer(input *elbv2.CreateLoadBalancerInp
 	vpcID := ""
 	var nlbManagedSGID string
 	if s.VPCService != nil && len(subnets) > 0 {
-		// NLBs reject customer SGs, so mint a dedicated managed SG for all LB ENIs;
-		// CreateListener opens listener ports on it. Without this, ENIs fall back
-		// to the VPC default SG and inbound listener traffic is dropped.
+		// An NLB without customer SGs gets a dedicated managed SG on all its ENIs
+		// and CreateListener opens listener ports on it; otherwise the ENIs fall
+		// back to the VPC default SG and inbound listener traffic is dropped. When
+		// the caller supplies SGs they replace the managed SG and own the rules.
 		eniGroups := securityGroups
-		if lbType == LoadBalancerTypeNetwork {
+		if lbType == LoadBalancerTypeNetwork && len(securityGroups) == 0 {
 			sgID, sgErr := s.createNLBManagedSG(lbID, lbArn, subnets[0], accountID)
 			if sgErr != nil {
 				slog.Error("CreateLoadBalancer: failed to create managed NLB SG", "lbId", lbID, "err", sgErr)

spinifex/handlers/elbv2/service_impl_lb_network.go

@@ -64,8 +64,11 @@ func (s *ELBv2ServiceImpl) SetSecurityGroups(input *elbv2.SetSecurityGroupsInput
 		return nil, errors.New(awserrors.ErrorELBv2LoadBalancerNotFound)
 	}
 
-	// NLBs do not support security groups (mirrors CreateLoadBalancer).
-	if lb.Type == LoadBalancerTypeNetwork {
+	// SGs are fixed at create time on NLBs: they can be replaced on an NLB created
+	// with SGs, but not added to one created without (it carries the managed SG
+	// instead). ALBs always allow SetSecurityGroups. The empty-input case is
+	// already rejected above, so this never strips an SG-NLB to zero.
+	if lb.Type == LoadBalancerTypeNetwork && len(lb.SecurityGroups) == 0 {
 		return nil, errors.New(awserrors.ErrorELBv2InvalidConfigurationRequest)
 	}
 
@@ -76,6 +79,9 @@ func (s *ELBv2ServiceImpl) SetSecurityGroups(input *elbv2.SetSecurityGroupsInput
 		}
 		sgs = append(sgs, *sg)
 	}
+	if len(sgs) > maxLBSecurityGroups {
+		return nil, errors.New(awserrors.ErrorELBv2InvalidConfigurationRequest)
+	}
 
 	// Re-attach to each ENI; failure aborts before the record is persisted.
 	if s.VPCService != nil {

spinifex/handlers/elbv2/service_impl_lb_network_test.go

@@ -126,6 +126,40 @@ func TestSetSecurityGroups_NLBRejected(t *testing.T) {
 	assert.Contains(t, err.Error(), awserrors.ErrorELBv2InvalidConfigurationRequest)
 }
 
+func TestSetSecurityGroups_NLBWithSGs_Replaces(t *testing.T) {
+	svc := setupTestService(t)
+	out, err := svc.CreateLoadBalancer(&elbv2.CreateLoadBalancerInput{
+		Name:           aws.String("sg-nlb-repl"),
+		Type:           aws.String("network"),
+		SecurityGroups: aws.StringSlice([]string{"sg-orig"}),
+	}, testAccountID)
+	require.NoError(t, err)
+	arn := *out.LoadBalancers[0].LoadBalancerArn
+
+	res, err := svc.SetSecurityGroups(&elbv2.SetSecurityGroupsInput{
+		LoadBalancerArn: aws.String(arn),
+		SecurityGroups:  aws.StringSlice([]string{"sg-new1", "sg-new2"}),
+	}, testAccountID)
+	require.NoError(t, err)
+	assert.Equal(t, []string{"sg-new1", "sg-new2"}, aws.StringValueSlice(res.SecurityGroupIds))
+
+	rec, err := svc.store.GetLoadBalancerByArn(arn)
+	require.NoError(t, err)
+	assert.Equal(t, []string{"sg-new1", "sg-new2"}, rec.SecurityGroups)
+}
+
+func TestSetSecurityGroups_TooManyRejected(t *testing.T) {
+	svc := setupTestService(t)
+	arn := createLBArn(t, svc, "sg-too-many")
+
+	_, err := svc.SetSecurityGroups(&elbv2.SetSecurityGroupsInput{
+		LoadBalancerArn: aws.String(arn),
+		SecurityGroups:  aws.StringSlice([]string{"sg-1", "sg-2", "sg-3", "sg-4", "sg-5", "sg-6"}),
+	}, testAccountID)
+	require.Error(t, err)
+	assert.Contains(t, err.Error(), awserrors.ErrorELBv2InvalidConfigurationRequest)
+}
+
 func TestSetSecurityGroups_CrossAccount(t *testing.T) {
 	svc := setupTestService(t)
 	arn := createLBArn(t, svc, "sg-xacct")

spinifex/handlers/elbv2/service_impl_nlb_sg.go

@@ -17,11 +17,15 @@ func nlbManagedSGName(lbID string) string {
 	return "spinifex-nlb-" + lbID
 }
 
-// lbENIGroups returns the SGs to attach to an LB's data-plane ENIs: the managed SG
-// for NLBs (customer SGs are rejected), or customer SGs for ALBs. Shared by the
-// create-time ENI loop and SetSubnets relaunch so both paths attach the same SG.
+// lbENIGroups returns the SGs to attach to an LB's data-plane ENIs: customer SGs
+// when present (ALBs always, NLBs that were created with SGs), else the managed SG
+// for an NLB created without SGs. Shared by the create-time ENI loop and SetSubnets
+// relaunch so both paths attach the same SG.
 func lbENIGroups(lb *LoadBalancerRecord) []string {
 	if lb.Type == LoadBalancerTypeNetwork {
+		if len(lb.SecurityGroups) > 0 {
+			return lb.SecurityGroups
+		}
 		if lb.NLBManagedSGID != "" {
 			return []string{lb.NLBManagedSGID}
 		}

spinifex/handlers/elbv2/service_impl_nlb_sg_test.go

@@ -119,6 +119,55 @@ func TestCreateNLB_MintsManagedSGAttachedToENI(t *testing.T) {
 	assert.Equal(t, rec.NLBManagedSGID, *eni.Groups[0].GroupId)
 }
 
+// TestLBENIGroups_Precedence checks the ENI-group selector: an NLB prefers its
+// customer SGs over the managed SG, falls back to the managed SG without them, and
+// an ALB always uses its customer SGs.
+func TestLBENIGroups_Precedence(t *testing.T) {
+	assert.Equal(t, []string{"sg-a"}, lbENIGroups(&LoadBalancerRecord{
+		Type: LoadBalancerTypeNetwork, SecurityGroups: []string{"sg-a"}, NLBManagedSGID: "sg-mgd",
+	}))
+	assert.Equal(t, []string{"sg-mgd"}, lbENIGroups(&LoadBalancerRecord{
+		Type: LoadBalancerTypeNetwork, NLBManagedSGID: "sg-mgd",
+	}))
+	assert.Equal(t, []string{"sg-x"}, lbENIGroups(&LoadBalancerRecord{
+		Type: LoadBalancerTypeApplication, SecurityGroups: []string{"sg-x"},
+	}))
+}
+
+// TestCreateNLB_WithCustomerSGs_AttachesThemNoManagedSG verifies an NLB created
+// with customer SGs joins those SGs on its ENI and skips the managed SG entirely;
+// the caller then owns the listener-port ingress rules.
+func TestCreateNLB_WithCustomerSGs_AttachesThemNoManagedSG(t *testing.T) {
+	svc, vpcSvc := setupTestServiceWithVPC(t)
+	subnetID, vpcID := firstSubnet(t, vpcSvc)
+
+	sgOut, err := vpcSvc.CreateSecurityGroup(&ec2.CreateSecurityGroupInput{
+		GroupName:   aws.String("nlb-customer-sg"),
+		Description: aws.String("NLB ingress"),
+		VpcId:       aws.String(vpcID),
+	}, testAccountID)
+	require.NoError(t, err)
+	sgID := *sgOut.GroupId
+
+	out, err := svc.CreateLoadBalancer(&elbv2.CreateLoadBalancerInput{
+		Name:           aws.String("nlb-customer-sg"),
+		Type:           aws.String("network"),
+		Subnets:        []*string{aws.String(subnetID)},
+		SecurityGroups: []*string{aws.String(sgID)},
+	}, testAccountID)
+	require.NoError(t, err)
+
+	rec, err := svc.store.GetLoadBalancerByArn(*out.LoadBalancers[0].LoadBalancerArn)
+	require.NoError(t, err)
+	assert.Equal(t, []string{sgID}, rec.SecurityGroups)
+	assert.Empty(t, rec.NLBManagedSGID, "NLB with customer SGs must not mint a managed SG")
+
+	eni := managedENI(t, vpcSvc)
+	require.NotNil(t, eni)
+	require.Len(t, eni.Groups, 1, "NLB ENI must join exactly the customer SG")
+	assert.Equal(t, sgID, *eni.Groups[0].GroupId)
+}
+
 // TestCreateALB_NoManagedSG verifies the managed-SG behavior is NLB-only: an ALB
 // keeps the existing customer-SG / default-SG semantics.
 func TestCreateALB_NoManagedSG(t *testing.T) {

spinifex/handlers/elbv2/service_impl_test.go

@@ -134,13 +134,32 @@ func TestCreateLoadBalancer_NetworkType(t *testing.T) {
 	assert.Equal(t, "active", *lb.State.Code)
 }
 
-func TestCreateLoadBalancer_NetworkType_RejectsSecurityGroups(t *testing.T) {
+func TestCreateLoadBalancer_NetworkType_AcceptsSecurityGroups(t *testing.T) {
 	svc := setupTestService(t)
 
 	_, err := svc.CreateLoadBalancer(&elbv2.CreateLoadBalancerInput{
 		Name:           aws.String("nlb-with-sg"),
 		Type:           aws.String("network"),
-		SecurityGroups: []*string{aws.String("sg-111")},
+		SecurityGroups: []*string{aws.String("sg-111"), aws.String("sg-222")},
+	}, testAccountID)
+	require.NoError(t, err)
+
+	rec, err := svc.store.GetLoadBalancerByName("nlb-with-sg", testAccountID)
+	require.NoError(t, err)
+	assert.Equal(t, []string{"sg-111", "sg-222"}, rec.SecurityGroups)
+	assert.Empty(t, rec.NLBManagedSGID, "NLB created with customer SGs must not mint a managed SG")
+}
+
+func TestCreateLoadBalancer_NetworkType_RejectsTooManySecurityGroups(t *testing.T) {
+	svc := setupTestService(t)
+
+	_, err := svc.CreateLoadBalancer(&elbv2.CreateLoadBalancerInput{
+		Name: aws.String("nlb-too-many-sg"),
+		Type: aws.String("network"),
+		SecurityGroups: []*string{
+			aws.String("sg-1"), aws.String("sg-2"), aws.String("sg-3"),
+			aws.String("sg-4"), aws.String("sg-5"), aws.String("sg-6"),
+		},
 	}, testAccountID)
 
 	assert.Error(t, err)

spinifex/handlers/elbv2/types.go

@@ -10,6 +10,9 @@ const (
 	LoadBalancerTypeApplication = "application"
 	LoadBalancerTypeNetwork     = "network"
 
+	// maxLBSecurityGroups is the AWS cap on security groups per load balancer.
+	maxLBSecurityGroups = 5
+
 	// LoadBalancer schemes
 	SchemeInternetFacing = "internet-facing"
 	SchemeInternal       = "internal"
@@ -116,9 +119,10 @@ type LoadBalancerRecord struct {
 	State           string   `json:"state"`  // "provisioning", "active", "failed"
 	VpcId           string   `json:"vpc_id"`
 	SecurityGroups  []string `json:"security_groups"`
-	// NLBManagedSGID is the managed SG minted for NLBs (customer SGs are rejected).
-	// Attached to every LB ENI; listener-port ingress is authorized on it.
-	// Not surfaced by DescribeLoadBalancers.
+	// NLBManagedSGID is the managed SG minted for an NLB created without customer
+	// SGs; attached to every LB ENI with listener-port ingress authorized on it.
+	// Empty when the NLB was created with customer SGs (which replace it). Not
+	// surfaced by DescribeLoadBalancers.
 	NLBManagedSGID string `json:"nlb_managed_sg_id,omitempty"`
 	// NLBIngressCIDRs overrides the scheme-based default client CIDRs that
 	// listener ports are opened to on NLBManagedSGID. Empty ⇒ default