diff --git a/spinifex/handlers/ec2/vpc/eni_abandoned_test.go b/spinifex/handlers/ec2/vpc/eni_abandoned_test.go index a05fbf822..f4cfb9315 100644 --- a/spinifex/handlers/ec2/vpc/eni_abandoned_test.go +++ b/spinifex/handlers/ec2/vpc/eni_abandoned_test.go @@ -137,7 +137,8 @@ func TestAbandonedENIBlocksSecurityGroupDelete(t *testing.T) { GroupId: aws.String(sgID), }, testAccountID) require.Error(t, err) - assert.Equal(t, awserrors.ErrorDependencyViolation, err.Error()) + assert.ErrorContains(t, err, awserrors.ErrorDependencyViolation) + assert.ErrorContains(t, err, eniID, "the refusal must name the ENI that blocked it") orphans, err := svc.ListAbandonedInstanceENIs(context.Background(), 15*time.Minute) require.NoError(t, err) diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index ba9c43067..729d57a05 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/spinifex/handlers/ec2/vpc/security_group.go @@ -346,7 +346,12 @@ func (s *VPCServiceImpl) checkSGDependencies(ctx context.Context, accountID, gro return errors.New(awserrors.ErrorServerInternal) } if slices.Contains(eni.SecurityGroupIds, groupId) { - return errors.New(awserrors.ErrorDependencyViolation) + // Name the blocker in the refusal and in the logs: recovering it + // afterwards otherwise means cross-referencing the whole ENI table. + slog.WarnContext(ctx, "checkSGDependencies: SG still attached to ENI", + "groupId", groupId, "eniId", eni.NetworkInterfaceId, "accountID", accountID) + return awserrors.Errorf(awserrors.ErrorDependencyViolation, + "the security group has a dependent network interface %s that must be detached first", eni.NetworkInterfaceId) } } @@ -373,18 +378,30 @@ func (s *VPCServiceImpl) checkSGDependencies(ctx context.Context, accountID, gro } for _, r := range other.IngressRules { if r.SourceSG == groupId { - return errors.New(awserrors.ErrorDependencyViolation) + return sgReferencedByError(ctx, accountID, groupId, other.GroupId, "ingress") } } for _, r := range other.EgressRules { if r.SourceSG == groupId { - return errors.New(awserrors.ErrorDependencyViolation) + return sgReferencedByError(ctx, accountID, groupId, other.GroupId, "egress") } } } return nil } +// sgReferencedByError logs and returns the DependencyViolation naming the SG +// whose rule still references groupId, so the blocking id survives even when +// the message does not reach the client. +func sgReferencedByError(ctx context.Context, accountID, groupId, referencingGroupId, direction string) error { + slog.WarnContext(ctx, "checkSGDependencies: SG referenced by another SG's rule", + "groupId", groupId, "referencingGroupId", referencingGroupId, + "direction", direction, "accountID", accountID) + return awserrors.Errorf(awserrors.ErrorDependencyViolation, + "the security group is referenced by a %s rule on security group %s that must be revoked first", + direction, referencingGroupId) +} + // describeSecurityGroupsValidFilters defines the set of filter names accepted by DescribeSecurityGroups. var describeSecurityGroupsValidFilters = map[string]bool{ "group-id": true, diff --git a/spinifex/handlers/ec2/vpc/security_group_test.go b/spinifex/handlers/ec2/vpc/security_group_test.go index 894525ffd..c89092896 100644 --- a/spinifex/handlers/ec2/vpc/security_group_test.go +++ b/spinifex/handlers/ec2/vpc/security_group_test.go @@ -5,6 +5,7 @@ import ( "encoding/json" "strings" "testing" + "time" "github.com/aws/aws-sdk-go/aws" "github.com/aws/aws-sdk-go/service/ec2" @@ -528,6 +529,46 @@ func ingressRuleIDReferencing(t *testing.T, svc *VPCServiceImpl, sgID, srcSG str return "" } +// TestDeleteSecurityGroup_DependencyViolationNamesENI pins that the refusal +// identifies the attached ENI. A bare code forces the blocking id to be +// recovered by cross-referencing the whole ENI table after the fact. +func TestDeleteSecurityGroup_DependencyViolationNamesENI(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + subnetID := createTestSubnet(t, svc, vpcID, "10.0.1.0/24") + sgID := createTestSG(t, svc, vpcID, "attached-sg") + eniID := createAutoENI(t, svc, subnetID, "i-holder", []string{sgID}, time.Hour) + + _, err := svc.DeleteSecurityGroup(context.Background(), &ec2.DeleteSecurityGroupInput{ + GroupId: aws.String(sgID), + }, testAccountID) + require.Error(t, err) + code, _ := awserrors.ResolveErrorCode(err) + assert.Equal(t, awserrors.ErrorDependencyViolation, code) + assert.ErrorContains(t, err, eniID, "the refusal must name the blocking ENI") +} + +// TestDeleteSecurityGroup_DependencyViolationNamesReferencingSG pins the same +// for the SourceSG case: the group holding the rule is named, not just the code. +func TestDeleteSecurityGroup_DependencyViolationNamesReferencingSG(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + vpcID := createTestVPC(t, svc, "10.0.0.0/16") + targetSG := createTestSG(t, svc, vpcID, "referenced-sg") + holderSG := createTestSG(t, svc, vpcID, "holder-sg") + + authorizeIngressFromSG(t, svc, holderSG, targetSG) + + _, err := svc.DeleteSecurityGroup(context.Background(), &ec2.DeleteSecurityGroupInput{ + GroupId: aws.String(targetSG), + }, testAccountID) + require.Error(t, err) + code, _ := awserrors.ResolveErrorCode(err) + assert.Equal(t, awserrors.ErrorDependencyViolation, code) + assert.ErrorContains(t, err, holderSG, "the refusal must name the referencing security group") +} + // TestRevokeSecurityGroupIngress_ByRuleID_BreaksMutualReferenceTeardown // reproduces a Terraform destroy of two mutually-referencing SGs. Modern TF // (aws_vpc_security_group_ingress_rule) revokes each rule by SecurityGroupRuleId