mirror of
https://github.com/mulgadc/spinifex.git
synced 2026-10-15 09:01:31 +00:00
fix: name the blocking resource in DeleteSecurityGroup DependencyViolation
checkSGDependencies knew which ENI or referencing security group blocked the delete and discarded it, returning a bare DependencyViolation with no log line, so the blocking id had to be recovered by cross-referencing the ENI table. Return it via awserrors.Errorf and log it at the point of refusal.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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,
|
||||
|
||||
@@ -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
|
||||
|
||||
Reference in New Issue
Block a user