diff --git a/spinifex/handlers/elbv2/service_impl.go b/spinifex/handlers/elbv2/service_impl.go index 62fc8e0c9..d561c550e 100644 --- a/spinifex/handlers/elbv2/service_impl.go +++ b/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) diff --git a/spinifex/handlers/elbv2/service_impl_lb_network.go b/spinifex/handlers/elbv2/service_impl_lb_network.go index 64fe4727e..82baaea44 100644 --- a/spinifex/handlers/elbv2/service_impl_lb_network.go +++ b/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 { diff --git a/spinifex/handlers/elbv2/service_impl_lb_network_test.go b/spinifex/handlers/elbv2/service_impl_lb_network_test.go index 588fd0c62..f30d00ed3 100644 --- a/spinifex/handlers/elbv2/service_impl_lb_network_test.go +++ b/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") diff --git a/spinifex/handlers/elbv2/service_impl_nlb_sg.go b/spinifex/handlers/elbv2/service_impl_nlb_sg.go index c589d7e7e..a178872de 100644 --- a/spinifex/handlers/elbv2/service_impl_nlb_sg.go +++ b/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} } diff --git a/spinifex/handlers/elbv2/service_impl_nlb_sg_test.go b/spinifex/handlers/elbv2/service_impl_nlb_sg_test.go index b63e6618d..fd984d114 100644 --- a/spinifex/handlers/elbv2/service_impl_nlb_sg_test.go +++ b/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) { diff --git a/spinifex/handlers/elbv2/service_impl_test.go b/spinifex/handlers/elbv2/service_impl_test.go index fb183c28a..a55917172 100644 --- a/spinifex/handlers/elbv2/service_impl_test.go +++ b/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) diff --git a/spinifex/handlers/elbv2/types.go b/spinifex/handlers/elbv2/types.go index 237fb94f6..3b6481759 100644 --- a/spinifex/handlers/elbv2/types.go +++ b/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