diff --git a/spinifex/awserrors/awserrors.go b/spinifex/awserrors/awserrors.go index b691d0a8f..e26a667b2 100644 --- a/spinifex/awserrors/awserrors.go +++ b/spinifex/awserrors/awserrors.go @@ -640,6 +640,18 @@ func Errorf(code, format string, args ...any) error { return outer } +// IDNotFound returns code with EC2's not-found message for one resource ID, +// "The ID '' does not exist". +func IDNotFound(code, kind, id string) error { + return Errorf(code, "The %s ID '%s' does not exist", kind, id) +} + +// HasDependencies returns DependencyViolation with EC2's message for a +// resource that cannot be deleted while others depend on it. +func HasDependencies(kind, id string) error { + return Errorf(ErrorDependencyViolation, "The %s '%s' has dependencies and cannot be deleted.", kind, id) +} + // retryableError wraps a registered code with a suggested Retry-After // duration, for a 503 that a client can expect to clear on its own within a // bounded window (e.g. a warm-up race) rather than an open-ended outage. diff --git a/spinifex/handlers/ec2/eigw/service_impl.go b/spinifex/handlers/ec2/eigw/service_impl.go index 8f7b1057c..84edbe699 100644 --- a/spinifex/handlers/ec2/eigw/service_impl.go +++ b/spinifex/handlers/ec2/eigw/service_impl.go @@ -90,7 +90,7 @@ func (s *EgressOnlyIGWServiceImpl) CreateEgressOnlyInternetGateway(ctx context.C } if _, err := s.vpcKV.Get(ctx, utils.AccountKey(accountID, *input.VpcId)); err != nil { slog.WarnContext(ctx, "CreateEgressOnlyInternetGateway: VPC not found for account", "vpcId", *input.VpcId, "accountID", accountID) - return nil, errors.New(awserrors.ErrorInvalidVpcIDNotFound) + return nil, awserrors.IDNotFound(awserrors.ErrorInvalidVpcIDNotFound, "vpc", *input.VpcId) } eigwID := utils.GenerateResourceID("eigw") diff --git a/spinifex/handlers/ec2/igw/service_impl.go b/spinifex/handlers/ec2/igw/service_impl.go index d71cf5133..f9464757f 100644 --- a/spinifex/handlers/ec2/igw/service_impl.go +++ b/spinifex/handlers/ec2/igw/service_impl.go @@ -156,7 +156,7 @@ func (s *IGWServiceImpl) DeleteInternetGateway(ctx context.Context, input *ec2.D // tolerates it on destroy); destroy orchestration tolerates it too. // A transient read error stays a server error. if errors.Is(err, jetstream.ErrKeyNotFound) { - return nil, errors.New(awserrors.ErrorInvalidInternetGatewayIDNotFound) + return nil, igwNotFoundError(igwID) } return nil, errors.New(awserrors.ErrorServerInternal) } @@ -166,12 +166,11 @@ func (s *IGWServiceImpl) DeleteInternetGateway(ctx context.Context, input *ec2.D return nil, errors.New(awserrors.ErrorServerInternal) } - // Cannot delete an attached IGW. The VPC is named because a pending - // attachment is hidden from describes, so the caller has no other way to - // learn what it must detach from. + // Cannot delete an attached IGW. AWS's message does not name the VPC, and + // a pending attachment is hidden from describes, so the log names it. if record.VpcId != "" { - return nil, awserrors.Errorf(awserrors.ErrorDependencyViolation, - "the internet gateway is attached to %s and must be detached first", record.VpcId) + slog.WarnContext(ctx, "DeleteInternetGateway: gateway is attached", "internetGatewayId", igwID, "vpcId", record.VpcId) + return nil, awserrors.HasDependencies("internetGateway", igwID) } if err := s.igwKV.Delete(ctx, key); err != nil { @@ -262,9 +261,9 @@ func (s *IGWServiceImpl) DescribeInternetGateways(ctx context.Context, input *ec } // Return error if specific IDs were requested but not found - for id := range igwIDs { - if !foundIDs[id] { - return nil, errors.New(awserrors.ErrorInvalidInternetGatewayIDNotFound) + for _, id := range input.InternetGatewayIds { + if id != nil && !foundIDs[*id] { + return nil, igwNotFoundError(*id) } } @@ -324,7 +323,7 @@ func (s *IGWServiceImpl) AttachInternetGateway(ctx context.Context, input *ec2.A entry, err := s.igwKV.Get(ctx, key) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidInternetGatewayIDNotFound) + return nil, igwNotFoundError(igwID) } var record IGWRecord @@ -333,7 +332,7 @@ func (s *IGWServiceImpl) AttachInternetGateway(ctx context.Context, input *ec2.A } if record.VpcId != "" { - return nil, errors.New(awserrors.ErrorResourceAlreadyAssociated) + return nil, awserrors.Errorf(awserrors.ErrorResourceAlreadyAssociated, "resource %s is already attached to network %s", igwID, record.VpcId) } // Verify the caller owns the target VPC (fail-closed if KV unavailable) @@ -343,7 +342,7 @@ func (s *IGWServiceImpl) AttachInternetGateway(ctx context.Context, input *ec2.A } if _, err := s.vpcKV.Get(ctx, utils.AccountKey(accountID, vpcID)); err != nil { slog.WarnContext(ctx, "AttachInternetGateway: VPC not found for account", "vpcId", vpcID, "accountID", accountID) - return nil, errors.New(awserrors.ErrorInvalidVpcIDNotFound) + return nil, awserrors.IDNotFound(awserrors.ErrorInvalidVpcIDNotFound, "vpc", vpcID) } record.VpcId = vpcID @@ -386,7 +385,7 @@ func (s *IGWServiceImpl) DetachInternetGateway(ctx context.Context, input *ec2.D entry, err := s.igwKV.Get(ctx, key) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidInternetGatewayIDNotFound) + return nil, igwNotFoundError(igwID) } var record IGWRecord @@ -458,7 +457,7 @@ func (s *IGWServiceImpl) CreateAttachedInternetGateway(ctx context.Context, acco return false, errors.New(awserrors.ErrorServerInternal) } if _, err := s.vpcKV.Get(ctx, utils.AccountKey(accountID, vpcID)); err != nil { - return false, errors.New(awserrors.ErrorInvalidVpcIDNotFound) + return false, awserrors.IDNotFound(awserrors.ErrorInvalidVpcIDNotFound, "vpc", vpcID) } key := utils.AccountKey(accountID, igwID) @@ -628,3 +627,7 @@ func (s *IGWServiceImpl) RemoveRecordTags(input *ec2.DeleteTagsInput, accountID func(r *IGWRecord) *map[string]string { return &r.Tags }, utils.RemoveTagsMut(input)) } + +func igwNotFoundError(id string) error { + return awserrors.IDNotFound(awserrors.ErrorInvalidInternetGatewayIDNotFound, "internetGateway", id) +} diff --git a/spinifex/handlers/ec2/igw/service_impl_test.go b/spinifex/handlers/ec2/igw/service_impl_test.go index 5bcd89cf4..af08d56cc 100644 --- a/spinifex/handlers/ec2/igw/service_impl_test.go +++ b/spinifex/handlers/ec2/igw/service_impl_test.go @@ -173,7 +173,37 @@ func TestDeleteInternetGateway_WhileAttached(t *testing.T) { _, err = svc.DeleteInternetGateway(context.Background(), &ec2.DeleteInternetGatewayInput{ InternetGatewayId: aws.String(igwID), }, testAccountID) - assert.ErrorContains(t, err, "DependencyViolation") + requireAWSError(t, err, awserrors.ErrorDependencyViolation, "The internetGateway '"+igwID+"' has dependencies and cannot be deleted.") + + // Attaching again names the gateway and the network it is on + _, err = svc.AttachInternetGateway(context.Background(), &ec2.AttachInternetGatewayInput{ + InternetGatewayId: aws.String(igwID), + VpcId: aws.String("vpc-test123"), + }, testAccountID) + requireAWSError(t, err, awserrors.ErrorResourceAlreadyAssociated, "resource "+igwID+" is already attached to network vpc-test123") +} + +func TestInternetGateway_NotFoundNamesTheID(t *testing.T) { + svc, _ := setupTestIGWService(t) + createTestIGW(t, svc) + + _, err := svc.DescribeInternetGateways(context.Background(), &ec2.DescribeInternetGatewaysInput{ + InternetGatewayIds: []*string{aws.String("igw-0000000000000dead")}, + }, testAccountID) + requireAWSError(t, err, awserrors.ErrorInvalidInternetGatewayIDNotFound, "The internetGateway ID 'igw-0000000000000dead' does not exist") + + _, err = svc.DeleteInternetGateway(context.Background(), &ec2.DeleteInternetGatewayInput{ + InternetGatewayId: aws.String("igw-0000000000000dead"), + }, testAccountID) + requireAWSError(t, err, awserrors.ErrorInvalidInternetGatewayIDNotFound, "The internetGateway ID 'igw-0000000000000dead' does not exist") +} + +func requireAWSError(t *testing.T, err error, code, message string) { + t.Helper() + got, msg, ok := awserrors.ResolveErrorDetail(err) + require.True(t, ok, "error %v carries no AWS code", err) + assert.Equal(t, code, got) + assert.Equal(t, message, msg) } func TestDescribeInternetGateways_All(t *testing.T) { diff --git a/spinifex/handlers/ec2/routetable/service_impl.go b/spinifex/handlers/ec2/routetable/service_impl.go index e52f37367..f3b54d20a 100644 --- a/spinifex/handlers/ec2/routetable/service_impl.go +++ b/spinifex/handlers/ec2/routetable/service_impl.go @@ -92,7 +92,7 @@ func (s *RouteTableServiceImpl) getRouteTable(ctx context.Context, accountID, rt entry, err := s.rtbKV.Get(ctx, utils.AccountKey(accountID, rtbID)) if err != nil { if errors.Is(err, jetstream.ErrKeyNotFound) { - return nil, errors.New(awserrors.ErrorInvalidRouteTableIDNotFound) + return nil, rtbNotFoundError(rtbID) } slog.ErrorContext(ctx, "Failed to read route table from KV", "routeTableId", rtbID, "err", err) return nil, errors.New(awserrors.ErrorServerInternal) @@ -146,7 +146,7 @@ func (s *RouteTableServiceImpl) mutateRouteTableCAS(ctx context.Context, account case err == nil: return nil case errors.Is(err, errRTBAbsent): - return errors.New(awserrors.ErrorInvalidRouteTableIDNotFound) + return rtbNotFoundError(rtbID) case errors.Is(err, errRTBContended): // Contended rather than broken. slog.ErrorContext(ctx, "Route table CAS retries exhausted under contention", @@ -169,7 +169,7 @@ func (s *RouteTableServiceImpl) mutateRouteTableCAS(ctx context.Context, account func (s *RouteTableServiceImpl) getVPCCidr(ctx context.Context, accountID, vpcID string) (string, error) { entry, err := s.vpcKV.Get(ctx, utils.AccountKey(accountID, vpcID)) if err != nil { - return "", errors.New(awserrors.ErrorInvalidVpcIDNotFound) + return "", awserrors.IDNotFound(awserrors.ErrorInvalidVpcIDNotFound, "vpc", vpcID) } var vpcRecord handlers_ec2_vpc.VPCRecord if err := json.Unmarshal(entry.Value(), &vpcRecord); err != nil { @@ -571,13 +571,13 @@ func (s *RouteTableServiceImpl) DeleteRouteTable(ctx context.Context, input *ec2 } if record.IsMain { - return nil, errors.New(awserrors.ErrorDependencyViolation) + return nil, awserrors.HasDependencies("routeTable", rtbID) } // Check for non-main associations (subnets still using this table) for _, assoc := range record.Associations { if !assoc.Main && assoc.SubnetId != "" { - return nil, errors.New(awserrors.ErrorDependencyViolation) + return nil, awserrors.HasDependencies("routeTable", rtbID) } } @@ -674,9 +674,9 @@ func (s *RouteTableServiceImpl) DescribeRouteTables(ctx context.Context, input * } // Return error if specific IDs were requested but not found - for id := range rtbIDs { - if !foundIDs[id] { - return nil, errors.New(awserrors.ErrorInvalidRouteTableIDNotFound) + for _, id := range input.RouteTableIds { + if id != nil && !foundIDs[*id] { + return nil, rtbNotFoundError(*id) } } @@ -688,6 +688,14 @@ func (s *RouteTableServiceImpl) DescribeRouteTables(ctx context.Context, input * // CreateRoute adds a route to a route table. // gatewayNotFoundError is AWS's answer to a route naming a gateway that does // not exist, whatever the gateway's type. +func rtbNotFoundError(id string) error { + return awserrors.IDNotFound(awserrors.ErrorInvalidRouteTableIDNotFound, "routeTable", id) +} + +func associationNotFoundError(id string) error { + return awserrors.IDNotFound(awserrors.ErrorInvalidAssociationIDNotFound, "association", id) +} + func gatewayNotFoundError(id string) error { return awserrors.Errorf(awserrors.ErrorInvalidGatewayIDNotFound, "The gateway ID '%s' does not exist", id) } @@ -827,7 +835,7 @@ func (s *RouteTableServiceImpl) DeleteRoute(ctx context.Context, input *ec2.Dele } } if idx < 0 { - return nil, errors.New(awserrors.ErrorInvalidRouteNotFound) + return nil, awserrors.Errorf(awserrors.ErrorInvalidRouteNotFound, "no route with destination-cidr-block %s in route table %s", destCidr, rtbID) } departing := record.Routes[idx] @@ -1034,7 +1042,7 @@ func (s *RouteTableServiceImpl) DisassociateRouteTable(ctx context.Context, inpu keys, err := s.rtbKV.Keys(ctx) if err != nil { if errors.Is(err, jetstream.ErrNoKeysFound) { - return nil, errors.New(awserrors.ErrorInvalidAssociationIDNotFound) + return nil, associationNotFoundError(assocID) } slog.ErrorContext(ctx, "Failed to list route table keys", "err", err) return nil, errors.New(awserrors.ErrorServerInternal) @@ -1099,7 +1107,7 @@ func (s *RouteTableServiceImpl) DisassociateRouteTable(ctx context.Context, inpu } } - return nil, errors.New(awserrors.ErrorInvalidAssociationIDNotFound) + return nil, associationNotFoundError(assocID) } // ReplaceRouteTableAssociation atomically moves a subnet from one route table to another. @@ -1125,7 +1133,7 @@ func (s *RouteTableServiceImpl) ReplaceRouteTableAssociation(ctx context.Context keys, err := s.rtbKV.Keys(ctx) if err != nil { if errors.Is(err, jetstream.ErrNoKeysFound) { - return nil, errors.New(awserrors.ErrorInvalidAssociationIDNotFound) + return nil, associationNotFoundError(assocID) } slog.ErrorContext(ctx, "Failed to list route table keys", "err", err) return nil, errors.New(awserrors.ErrorServerInternal) @@ -1217,7 +1225,7 @@ func (s *RouteTableServiceImpl) ReplaceRouteTableAssociation(ctx context.Context } } - return nil, errors.New(awserrors.ErrorInvalidAssociationIDNotFound) + return nil, associationNotFoundError(assocID) } // rtbMatchesFilters checks if a route table record matches all parsed filters. diff --git a/spinifex/handlers/ec2/routetable/service_impl_test.go b/spinifex/handlers/ec2/routetable/service_impl_test.go index b6af1c132..155c7d94d 100644 --- a/spinifex/handlers/ec2/routetable/service_impl_test.go +++ b/spinifex/handlers/ec2/routetable/service_impl_test.go @@ -192,7 +192,7 @@ func TestCreateRouteTable_VpcNotFound(t *testing.T) { _, err := svc.CreateRouteTable(t.Context(), &ec2.CreateRouteTableInput{ VpcId: aws.String("vpc-nonexistent"), }, testAccountID) - assert.EqualError(t, err, awserrors.ErrorInvalidVpcIDNotFound) + requireAWSError(t, err, awserrors.ErrorInvalidVpcIDNotFound, "The vpc ID 'vpc-nonexistent' does not exist") } func TestDeleteRouteTable(t *testing.T) { @@ -207,7 +207,7 @@ func TestDeleteRouteTable(t *testing.T) { // Should be gone _, err = svc.getRouteTable(t.Context(), testAccountID, rtbID) - assert.EqualError(t, err, awserrors.ErrorInvalidRouteTableIDNotFound) + requireAWSError(t, err, awserrors.ErrorInvalidRouteTableIDNotFound, "The routeTable ID '"+rtbID+"' does not exist") } func TestDeleteRouteTable_Main(t *testing.T) { @@ -219,7 +219,7 @@ func TestDeleteRouteTable_Main(t *testing.T) { _, err = svc.DeleteRouteTable(t.Context(), &ec2.DeleteRouteTableInput{ RouteTableId: aws.String(record.RouteTableId), }, testAccountID) - assert.EqualError(t, err, awserrors.ErrorDependencyViolation) + requireAWSError(t, err, awserrors.ErrorDependencyViolation, "The routeTable '"+record.RouteTableId+"' has dependencies and cannot be deleted.") } func TestDeleteRouteTable_WithAssociations(t *testing.T) { @@ -238,7 +238,7 @@ func TestDeleteRouteTable_WithAssociations(t *testing.T) { _, err = svc.DeleteRouteTable(t.Context(), &ec2.DeleteRouteTableInput{ RouteTableId: aws.String(rtbID), }, testAccountID) - assert.EqualError(t, err, awserrors.ErrorDependencyViolation) + requireAWSError(t, err, awserrors.ErrorDependencyViolation, "The routeTable '"+rtbID+"' has dependencies and cannot be deleted.") } func TestDescribeRouteTables(t *testing.T) { @@ -490,7 +490,7 @@ func TestDeleteRoute_NotFound(t *testing.T) { RouteTableId: aws.String(rtbID), DestinationCidrBlock: aws.String("192.168.0.0/16"), }, testAccountID) - assert.EqualError(t, err, awserrors.ErrorInvalidRouteNotFound) + requireAWSError(t, err, awserrors.ErrorInvalidRouteNotFound, "no route with destination-cidr-block 192.168.0.0/16 in route table "+rtbID) } func TestReplaceRoute(t *testing.T) { diff --git a/spinifex/handlers/ec2/vpc/GetSecurityGroupsForVpc_test.go b/spinifex/handlers/ec2/vpc/GetSecurityGroupsForVpc_test.go index 3ceb95832..5c968c22d 100644 --- a/spinifex/handlers/ec2/vpc/GetSecurityGroupsForVpc_test.go +++ b/spinifex/handlers/ec2/vpc/GetSecurityGroupsForVpc_test.go @@ -114,7 +114,7 @@ func TestGetSecurityGroupsForVpc_ExcludesOtherAccount(t *testing.T) { VpcId: otherVpc.Vpc.VpcId, }, testAccountID) require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidVpcIDNotFound, err.Error()) + assert.True(t, awserrors.IsErrorCode(err, awserrors.ErrorInvalidVpcIDNotFound), err.Error()) } func TestGetSecurityGroupsForVpc_MissingVpcId(t *testing.T) { @@ -136,7 +136,7 @@ func TestGetSecurityGroupsForVpc_UnknownVpcIsNotFound(t *testing.T) { }, testAccountID) require.Error(t, err, "an unknown VPC must not be reported as a VPC with no groups") assert.Nil(t, out) - assert.Equal(t, awserrors.ErrorInvalidVpcIDNotFound, err.Error()) + assert.True(t, awserrors.IsErrorCode(err, awserrors.ErrorInvalidVpcIDNotFound), err.Error()) } // A VPC whose only group is the default one still answers, so the not-found diff --git a/spinifex/handlers/ec2/vpc/eni.go b/spinifex/handlers/ec2/vpc/eni.go index 768908c31..8584356f7 100644 --- a/spinifex/handlers/ec2/vpc/eni.go +++ b/spinifex/handlers/ec2/vpc/eni.go @@ -149,7 +149,7 @@ func (s *VPCServiceImpl) CreateNetworkInterface(ctx context.Context, input *ec2. if err != nil { switch { case errors.Is(err, ErrIPInUse): - return nil, errors.New(awserrors.ErrorInvalidIPAddressInUse) + return nil, awserrors.Errorf(awserrors.ErrorInvalidIPAddressInUse, "The specified address is already in use.") case errors.Is(err, ErrIPOutOfRange): return nil, eniAddressError(*input.PrivateIpAddress, subnet.CidrBlock) default: @@ -257,7 +257,7 @@ func (s *VPCServiceImpl) deleteNetworkInterface(ctx context.Context, eniId, acco if force { return &ec2.DeleteNetworkInterfaceOutput{}, nil } - return nil, errors.New(awserrors.ErrorInvalidNetworkInterfaceIDNotFound) + return nil, eniNotFoundError(eniId) } return nil, errors.New(awserrors.ErrorServerInternal) } @@ -352,7 +352,7 @@ func (s *VPCServiceImpl) DetachAndDeleteENI(ctx context.Context, accountID, eniI if force { return false, nil } - return false, errors.New(awserrors.ErrorInvalidNetworkInterfaceIDNotFound) + return false, eniNotFoundError(eniID) } return false, errors.New(awserrors.ErrorServerInternal) } @@ -456,7 +456,7 @@ func (s *VPCServiceImpl) ModifyNetworkInterfaceAttribute(ctx context.Context, in entry, err := s.eniKV.Get(ctx, key) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidNetworkInterfaceIDNotFound) + return nil, eniNotFoundError(eniId) } var record ENIRecord @@ -603,7 +603,7 @@ func (s *VPCServiceImpl) DescribeNetworkInterfaces(ctx context.Context, input *e } for _, id := range input.NetworkInterfaceIds { if id != nil && !found[*id] { - return nil, awserrors.Errorf(awserrors.ErrorInvalidNetworkInterfaceIDNotFound, "The networkInterface ID '%s' does not exist", *id) + return nil, eniNotFoundError(*id) } } } @@ -701,7 +701,7 @@ func (s *VPCServiceImpl) attachENI(ctx context.Context, accountID, eniId, instan key := utils.AccountKey(accountID, eniId) entry, err := s.eniKV.Get(ctx, key) if err != nil { - return "", errors.New(awserrors.ErrorInvalidNetworkInterfaceIDNotFound) + return "", eniNotFoundError(eniId) } var record ENIRecord @@ -743,7 +743,7 @@ func (s *VPCServiceImpl) DetachENI(ctx context.Context, accountID, eniId string) key := utils.AccountKey(accountID, eniId) entry, err := s.eniKV.Get(ctx, key) if err != nil { - return errors.New(awserrors.ErrorInvalidNetworkInterfaceIDNotFound) + return eniNotFoundError(eniId) } var record ENIRecord @@ -779,7 +779,7 @@ func (s *VPCServiceImpl) getENIRecord(ctx context.Context, accountID, eniId stri key := utils.AccountKey(accountID, eniId) entry, err := s.eniKV.Get(ctx, key) if err != nil { - return ENIRecord{}, errors.New(awserrors.ErrorInvalidNetworkInterfaceIDNotFound) + return ENIRecord{}, eniNotFoundError(eniId) } var record ENIRecord if err := json.Unmarshal(entry.Value(), &record); err != nil { @@ -799,7 +799,7 @@ func (s *VPCServiceImpl) updateENI(ctx context.Context, accountID, eniId string, key := utils.AccountKey(accountID, eniId) entry, err := s.eniKV.Get(ctx, key) if err != nil { - return errors.New(awserrors.ErrorInvalidNetworkInterfaceIDNotFound) + return eniNotFoundError(eniId) } var record ENIRecord if err := json.Unmarshal(entry.Value(), &record); err != nil { @@ -1198,7 +1198,7 @@ func (s *VPCServiceImpl) validateSGAttachment(ctx context.Context, accountID str for _, sgId := range sgIds { sgEntry, err := s.sgKV.Get(ctx, utils.AccountKey(accountID, sgId)) if err != nil { - return errors.New(awserrors.ErrorInvalidGroupNotFound) + return sgNotFoundError(sgId) } var sg SecurityGroupRecord if err := json.Unmarshal(sgEntry.Value(), &sg); err != nil { diff --git a/spinifex/handlers/ec2/vpc/eni_abandoned_test.go b/spinifex/handlers/ec2/vpc/eni_abandoned_test.go index f4cfb9315..b5c320459 100644 --- a/spinifex/handlers/ec2/vpc/eni_abandoned_test.go +++ b/spinifex/handlers/ec2/vpc/eni_abandoned_test.go @@ -137,8 +137,7 @@ func TestAbandonedENIBlocksSecurityGroupDelete(t *testing.T) { GroupId: aws.String(sgID), }, testAccountID) require.Error(t, err) - assert.ErrorContains(t, err, awserrors.ErrorDependencyViolation) - assert.ErrorContains(t, err, eniID, "the refusal must name the ENI that blocked it") + requireAWSError(t, err, awserrors.ErrorDependencyViolation, "resource "+sgID+" has a dependent object") orphans, err := svc.ListAbandonedInstanceENIs(context.Background(), 15*time.Minute) require.NoError(t, err) diff --git a/spinifex/handlers/ec2/vpc/eni_test.go b/spinifex/handlers/ec2/vpc/eni_test.go index 0d5d7da6d..ec192c4ca 100644 --- a/spinifex/handlers/ec2/vpc/eni_test.go +++ b/spinifex/handlers/ec2/vpc/eni_test.go @@ -218,7 +218,7 @@ func TestCreateNetworkInterface_RequestedPrivateIPRaceHasOneWinner(t *testing.T) switch { case err == nil: successes++ - case err.Error() == awserrors.ErrorInvalidIPAddressInUse: + case awserrors.IsErrorCode(err, awserrors.ErrorInvalidIPAddressInUse): conflicts++ default: t.Fatalf("unexpected error: %v", err) diff --git a/spinifex/handlers/ec2/vpc/ids.go b/spinifex/handlers/ec2/vpc/ids.go index 0af2db260..46a9820f4 100644 --- a/spinifex/handlers/ec2/vpc/ids.go +++ b/spinifex/handlers/ec2/vpc/ids.go @@ -46,3 +46,23 @@ func sgIDMalformedError(id string) error { func eniIDMalformedError(id string) error { return malformedIDError(awserrors.ErrorInvalidNetworkInterfaceIdMalformed, "eni", id, isLowerHex) } + +func vpcNotFoundError(id string) error { + return awserrors.IDNotFound(awserrors.ErrorInvalidVpcIDNotFound, "vpc", id) +} + +func eniNotFoundError(id string) error { + return awserrors.IDNotFound(awserrors.ErrorInvalidNetworkInterfaceIDNotFound, "networkInterface", id) +} + +// sgNotFoundError is AWS's security group not-found answer, which unlike the +// other resource types says no "ID". +func sgNotFoundError(id string) error { + return awserrors.Errorf(awserrors.ErrorInvalidGroupNotFound, "The security group '%s' does not exist", id) +} + +// subnetRangeError answers a subnet CIDR too large, too small or outside +// its VPC, which AWS does not tell apart. +func subnetRangeError(cidr string) error { + return awserrors.Errorf(awserrors.ErrorInvalidSubnetRange, "The CIDR '%s' is invalid.", cidr) +} diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index 389f4d4e8..4ccb00d0c 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/spinifex/handlers/ec2/vpc/security_group.go @@ -85,7 +85,7 @@ func validateSGRule(r SGRule) error { if r.CidrIp != "" { _, ipnet, err := net.ParseCIDR(r.CidrIp) if err != nil { - return fmt.Errorf("invalid CidrIp %q: %w", r.CidrIp, err) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "CIDR block %s is malformed", r.CidrIp) } if ipnet.IP.To4() == nil { return fmt.Errorf("invalid CidrIp %q: IPv6 belongs in CidrIpv6", r.CidrIp) @@ -97,7 +97,7 @@ func validateSGRule(r SGRule) error { if r.CidrIpv6 != "" { _, ipnet, err := net.ParseCIDR(r.CidrIpv6) if err != nil { - return fmt.Errorf("invalid CidrIpv6 %q: %w", r.CidrIpv6, err) + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "CIDR block %s is malformed", r.CidrIpv6) } if ipnet.IP.To4() != nil { return fmt.Errorf("invalid CidrIpv6 %q: IPv4 belongs in CidrIp", r.CidrIpv6) @@ -232,7 +232,7 @@ func (s *VPCServiceImpl) CreateSecurityGroup(ctx context.Context, input *ec2.Cre } sgsInVPC++ if existing.GroupName == groupName { - return nil, errors.New(awserrors.ErrorInvalidGroupDuplicate) + return nil, awserrors.Errorf(awserrors.ErrorInvalidGroupDuplicate, "The security group '%s' already exists for VPC '%s'", groupName, vpcId) } } if sgsInVPC >= maxSGsPerVPC { @@ -305,7 +305,7 @@ func (s *VPCServiceImpl) DeleteSecurityGroup(ctx context.Context, input *ec2.Del // success. Destroy orchestration tolerates it via awserrors.IsNotFound; // a transient read error stays a server error. if errors.Is(err, jetstream.ErrKeyNotFound) { - return nil, errors.New(awserrors.ErrorInvalidGroupNotFound) + return nil, sgNotFoundError(groupId) } return nil, errors.New(awserrors.ErrorServerInternal) } @@ -316,7 +316,7 @@ func (s *VPCServiceImpl) DeleteSecurityGroup(ctx context.Context, input *ec2.Del } if record.IsDefault { - return nil, errors.New(awserrors.ErrorCannotDelete) + return nil, awserrors.Errorf(awserrors.ErrorCannotDelete, "the specified group: %q name: %q cannot be deleted by a user", groupId, record.GroupName) } if err := s.checkSGDependencies(ctx, accountID, groupId); err != nil { @@ -351,14 +351,14 @@ func (s *VPCServiceImpl) validateSGRuleReferences(ctx context.Context, accountID } entry, err := s.sgKV.Get(ctx, utils.AccountKey(accountID, r.SourceSG)) if err != nil { - return errors.New(awserrors.ErrorInvalidGroupNotFound) + return sgNotFoundError(r.SourceSG) } var rec SecurityGroupRecord if err := json.Unmarshal(entry.Value(), &rec); err != nil { return errors.New(awserrors.ErrorServerInternal) } if rec.VpcId != ownerVpcId { - return errors.New(awserrors.ErrorInvalidGroupNotFound) + return sgNotFoundError(r.SourceSG) } } return nil @@ -395,8 +395,7 @@ func (s *VPCServiceImpl) checkSGDependencies(ctx context.Context, accountID, gro // 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) + return sgDependentObjectError(groupId) } } @@ -435,16 +434,36 @@ func (s *VPCServiceImpl) checkSGDependencies(ctx context.Context, accountID, gro 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. +// sgReferencedByError logs the SG whose rule still references groupId, since +// AWS's message does not name it, and returns the DependencyViolation. 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) + return sgDependentObjectError(groupId) +} + +// sgDependentObjectError is AWS's answer to deleting a group that an +// interface or another group's rule still uses; it does not say which. +func sgDependentObjectError(groupId string) error { + return awserrors.Errorf(awserrors.ErrorDependencyViolation, "resource %s has a dependent object", groupId) +} + +// sgRuleDuplicateError is AWS's answer to adding a rule the group already +// has, which describes the rule by its peer, protocol and ports. +func sgRuleDuplicateError(rule SGRule) error { + peer := rule.CidrIp + if peer == "" { + peer = rule.CidrIpv6 + } + if peer == "" { + peer = rule.SourceSG + } + desc := fmt.Sprintf("peer: %s, ALL, ALLOW", peer) + if rule.IpProtocol != allProtocols { + desc = fmt.Sprintf("peer: %s, %s, from port: %d, to port: %d, ALLOW", peer, strings.ToUpper(rule.IpProtocol), rule.FromPort, rule.ToPort) + } + return awserrors.Errorf(awserrors.ErrorInvalidPermissionDuplicate, "the specified rule %q already exists", desc) } // describeSecurityGroupsValidFilters defines the set of filter names accepted by DescribeSecurityGroups. @@ -543,7 +562,7 @@ func (s *VPCServiceImpl) DescribeSecurityGroups(ctx context.Context, input *ec2. } for _, id := range input.GroupIds { if id != nil && !found[*id] { - return nil, awserrors.Errorf(awserrors.ErrorInvalidGroupNotFound, "The security group '%s' does not exist", *id) + return nil, sgNotFoundError(*id) } } } @@ -856,11 +875,9 @@ func (s *VPCServiceImpl) DescribeSecurityGroupRules(ctx context.Context, input * } } - if len(requested) > 0 { - for id := range requested { - if !emitted[id] { - return nil, errors.New(awserrors.ErrorInvalidSecurityGroupRuleIdNotFound) - } + for _, id := range input.SecurityGroupRuleIds { + if !emitted[*id] { + return nil, sgRuleNotFoundError(*id) } } @@ -1038,7 +1055,7 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupIngress(ctx context.Context, inpu entry, err := s.sgKV.Get(ctx, key) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidGroupNotFound) + return nil, sgNotFoundError(groupId) } var record SecurityGroupRecord @@ -1068,7 +1085,7 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupIngress(ctx context.Context, inpu } for _, nr := range newRules { if _, ok := existing[sgRuleKey(nr)]; ok { - return nil, errors.New(awserrors.ErrorInvalidPermissionDuplicate) + return nil, sgRuleDuplicateError(nr) } } if len(record.IngressRules)+len(newRules) > maxRulesPerSGSide { @@ -1121,7 +1138,7 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupEgress(ctx context.Context, input entry, err := s.sgKV.Get(ctx, key) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidGroupNotFound) + return nil, sgNotFoundError(groupId) } var record SecurityGroupRecord @@ -1151,7 +1168,7 @@ func (s *VPCServiceImpl) AuthorizeSecurityGroupEgress(ctx context.Context, input } for _, nr := range newRules { if _, ok := existing[sgRuleKey(nr)]; ok { - return nil, errors.New(awserrors.ErrorInvalidPermissionDuplicate) + return nil, sgRuleDuplicateError(nr) } } if len(record.EgressRules)+len(newRules) > maxRulesPerSGSide { @@ -1204,7 +1221,7 @@ func (s *VPCServiceImpl) RevokeSecurityGroupIngress(ctx context.Context, input * entry, err := s.sgKV.Get(ctx, key) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidGroupNotFound) + return nil, sgNotFoundError(groupId) } var record SecurityGroupRecord @@ -1271,7 +1288,7 @@ func (s *VPCServiceImpl) RevokeSecurityGroupEgress(ctx context.Context, input *e entry, err := s.sgKV.Get(ctx, key) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidGroupNotFound) + return nil, sgNotFoundError(groupId) } var record SecurityGroupRecord @@ -1392,7 +1409,7 @@ func (s *VPCServiceImpl) updateSGRuleDescriptions(ctx context.Context, accountID key := utils.AccountKey(accountID, req.groupId) entry, err := s.sgKV.Get(ctx, key) if err != nil { - return errors.New(awserrors.ErrorInvalidGroupNotFound) + return sgNotFoundError(req.groupId) } var record SecurityGroupRecord @@ -1468,7 +1485,7 @@ func applySGRuleDescriptions(rules []SGRule, descriptions []*ec2.SecurityGroupRu // absent from this record and so is not-found, never a silent no-op. i, ok := byID[id] if !ok { - return nil, errors.New(awserrors.ErrorInvalidSecurityGroupRuleIdNotFound) + return nil, sgRuleNotFoundError(id) } if d.Description != nil { out[i].Description = *d.Description @@ -1585,7 +1602,7 @@ func (s *VPCServiceImpl) ModifySecurityGroupRules(ctx context.Context, input *ec entry, err := s.sgKV.Get(ctx, key) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidGroupNotFound) + return nil, sgNotFoundError(groupId) } var record SecurityGroupRecord @@ -1728,7 +1745,7 @@ func checkSGRuleModificationDuplicates(record *SecurityGroupRecord, mods []sgRul for i, r := range rules { if !replaced[i] && newKeys[sgRuleKey(r)] { - return errors.New(awserrors.ErrorInvalidPermissionDuplicate) + return sgRuleDuplicateError(r) } } return nil @@ -1836,8 +1853,11 @@ func sgRuleIDLookupError(id string) error { if SGRuleIDIsMalformed(id) { return awserrors.Errorf(awserrors.ErrorInvalidSecurityGroupRuleIdMalformed, "Invalid id: %q", id) } - return awserrors.Errorf(awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, - "The security group rule ID '%s' does not exist", id) + return sgRuleNotFoundError(id) +} + +func sgRuleNotFoundError(id string) error { + return awserrors.IDNotFound(awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, "security group rule", id) } // sgRecordToEC2 converts a SecurityGroupRecord to an EC2 SecurityGroup. @@ -2102,7 +2122,7 @@ func resolveRuleIDsToRemove(existing []SGRule, ruleIDs []*string) ([]SGRule, err } r, ok := byID[*idp] if !ok { - return nil, errors.New(awserrors.ErrorInvalidSecurityGroupRuleIdNotFound) + return nil, sgRuleNotFoundError(*idp) } out = append(out, r) } diff --git a/spinifex/handlers/ec2/vpc/security_group_test.go b/spinifex/handlers/ec2/vpc/security_group_test.go index 7f6b81bd6..0ec21685e 100644 --- a/spinifex/handlers/ec2/vpc/security_group_test.go +++ b/spinifex/handlers/ec2/vpc/security_group_test.go @@ -94,8 +94,7 @@ func TestCreateSecurityGroup_DuplicateName(t *testing.T) { GroupName: aws.String("dup-sg"), VpcId: aws.String(vpcID), }, testAccountID) - assert.Error(t, err) - assert.Contains(t, err.Error(), "InvalidGroup.Duplicate") + requireAWSError(t, err, awserrors.ErrorInvalidGroupDuplicate, "The security group 'dup-sg' already exists for VPC '"+vpcID+"'") } // --- DeleteSecurityGroup --- @@ -635,29 +634,25 @@ 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) { +// TestDeleteSecurityGroup_DependencyViolationForENI pins AWS's refusal for a +// group still attached to an interface, which names the group, not the ENI. +func TestDeleteSecurityGroup_DependencyViolationForENI(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) + 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") + requireAWSError(t, err, awserrors.ErrorDependencyViolation, "resource "+sgID+" has a dependent object") } -// 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) { +// TestDeleteSecurityGroup_DependencyViolationForReferencingSG pins the same +// answer for a group another group's rule references. +func TestDeleteSecurityGroup_DependencyViolationForReferencingSG(t *testing.T) { t.Parallel() svc := setupTestVPCService(t) vpcID := createTestVPC(t, svc, "10.0.0.0/16") @@ -669,10 +664,7 @@ func TestDeleteSecurityGroup_DependencyViolationNamesReferencingSG(t *testing.T) _, 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") + requireAWSError(t, err, awserrors.ErrorDependencyViolation, "resource "+targetSG+" has a dependent object") } // TestRevokeSecurityGroupIngress_ByRuleID_BreaksMutualReferenceTeardown @@ -1615,8 +1607,12 @@ func TestDeleteSecurityGroup_RejectsDefault(t *testing.T) { _, err := svc.DeleteSecurityGroup(context.Background(), &ec2.DeleteSecurityGroupInput{ GroupId: aws.String(sgID), }, testAccountID) - require.Error(t, err) - assert.Contains(t, err.Error(), "CannotDelete") + requireAWSError(t, err, awserrors.ErrorCannotDelete, `the specified group: "`+sgID+`" name: "default" cannot be deleted by a user`) + + _, err = svc.DeleteSecurityGroup(context.Background(), &ec2.DeleteSecurityGroupInput{ + GroupId: aws.String("sg-0000000000000dead"), + }, testAccountID) + requireAWSError(t, err, awserrors.ErrorInvalidGroupNotFound, "The security group 'sg-0000000000000dead' does not exist") } func TestDeleteSecurityGroup_RejectsAttachedToENI(t *testing.T) { @@ -1667,14 +1663,10 @@ func TestDeleteVpc_BlocksOnNonDefaultSG(t *testing.T) { t.Parallel() svc := setupTestVPCService(t) vpcID := createTestVPC(t, svc, "10.0.0.0/16") - sgID := createTestSG(t, svc, vpcID, "user-sg") + createTestSG(t, svc, vpcID, "user-sg") _, err := svc.DeleteVpc(context.Background(), &ec2.DeleteVpcInput{VpcId: aws.String(vpcID)}, testAccountID) - require.Error(t, err) - assert.Contains(t, err.Error(), "DependencyViolation") - // The message must name the blocking security group so the caller knows - // what to delete first, not just that something is blocking. - assert.Contains(t, err.Error(), sgID) + requireAWSError(t, err, awserrors.ErrorDependencyViolation, "The vpc '"+vpcID+"' has dependencies and cannot be deleted.") } func TestDeleteVpc_CascadesDefaultSG(t *testing.T) { @@ -2402,8 +2394,32 @@ func TestAuthorizeSecurityGroupIngress_RejectsContentDuplicate(t *testing.T) { GroupId: aws.String(sgID), IpPermissions: []*ec2.IpPermission{perm}, }, testAccountID) - require.Error(t, err) - assert.Contains(t, err.Error(), "InvalidPermission.Duplicate") + requireAWSError(t, err, awserrors.ErrorInvalidPermissionDuplicate, + `the specified rule "peer: 10.0.0.0/24, TCP, from port: 22, to port: 22, ALLOW" already exists`) + + // The default egress rule, re-added, is described without ports + _, err = svc.AuthorizeSecurityGroupEgress(context.Background(), &ec2.AuthorizeSecurityGroupEgressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: aws.String("-1"), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("0.0.0.0/0")}}, + }}, + }, testAccountID) + requireAWSError(t, err, awserrors.ErrorInvalidPermissionDuplicate, `the specified rule "peer: 0.0.0.0/0, ALL, ALLOW" already exists`) + + _, err = svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{ + GroupId: aws.String(sgID), + IpPermissions: []*ec2.IpPermission{{ + IpProtocol: &proto, FromPort: aws.Int64(22), ToPort: aws.Int64(22), + IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/33")}}, + }}, + }, testAccountID) + requireAWSError(t, err, awserrors.ErrorInvalidParameterValue, "CIDR block 10.0.0.0/33 is malformed") + + _, err = svc.DescribeSecurityGroupRules(context.Background(), &ec2.DescribeSecurityGroupRulesInput{ + SecurityGroupRuleIds: []*string{aws.String("sgr-0000000000000dead")}, + }, testAccountID) + requireAWSError(t, err, awserrors.ErrorInvalidSecurityGroupRuleIdNotFound, "The security group rule ID 'sgr-0000000000000dead' does not exist") } func TestDescribeSecurityGroupRules_InvalidFilter(t *testing.T) { diff --git a/spinifex/handlers/ec2/vpc/service_impl.go b/spinifex/handlers/ec2/vpc/service_impl.go index 0225c6000..337e653da 100644 --- a/spinifex/handlers/ec2/vpc/service_impl.go +++ b/spinifex/handlers/ec2/vpc/service_impl.go @@ -350,7 +350,7 @@ func (s *VPCServiceImpl) CreateVpc(ctx context.Context, input *ec2.CreateVpcInpu // this account. func (s *VPCServiceImpl) requireVPCExists(ctx context.Context, accountID, vpcId string) error { if _, err := s.vpcKV.Get(ctx, utils.AccountKey(accountID, vpcId)); err != nil { - return errors.New(awserrors.ErrorInvalidVpcIDNotFound) + return vpcNotFoundError(vpcId) } return nil } @@ -369,7 +369,7 @@ func (s *VPCServiceImpl) DeleteVpc(ctx context.Context, input *ec2.DeleteVpcInpu // tolerates it on destroy); destroy orchestration tolerates it too. // A transient read error stays a server error. if errors.Is(err, jetstream.ErrKeyNotFound) { - return nil, errors.New(awserrors.ErrorInvalidVpcIDNotFound) + return nil, vpcNotFoundError(vpcID) } return nil, errors.New(awserrors.ErrorServerInternal) } @@ -405,8 +405,8 @@ func (s *VPCServiceImpl) DeleteVpc(ctx context.Context, input *ec2.DeleteVpcInpu return nil, errors.New(awserrors.ErrorServerInternal) } if subnet.VpcId == vpcID { - return nil, awserrors.Errorf(awserrors.ErrorDependencyViolation, - "the VPC has a dependent subnet %s that must be deleted first", subnet.SubnetId) + slog.WarnContext(ctx, "DeleteVpc: VPC has a dependent subnet", "vpcId", vpcID, "subnetId", subnet.SubnetId) + return nil, awserrors.HasDependencies("vpc", vpcID) } } @@ -442,8 +442,8 @@ func (s *VPCServiceImpl) DeleteVpc(ctx context.Context, input *ec2.DeleteVpcInpu continue } if !sg.IsDefault { - return nil, awserrors.Errorf(awserrors.ErrorDependencyViolation, - "the VPC has a dependent security group %s that must be deleted first", sg.GroupId) + slog.WarnContext(ctx, "DeleteVpc: VPC has a dependent security group", "vpcId", vpcID, "groupId", sg.GroupId) + return nil, awserrors.HasDependencies("vpc", vpcID) } defaultSGId = sg.GroupId } @@ -483,8 +483,8 @@ func (s *VPCServiceImpl) DeleteVpc(ctx context.Context, input *ec2.DeleteVpcInpu continue } if !rtb.IsMain { - return nil, awserrors.Errorf(awserrors.ErrorDependencyViolation, - "the VPC has a dependent route table %s that must be deleted first", rtb.RouteTableId) + slog.WarnContext(ctx, "DeleteVpc: VPC has a dependent route table", "vpcId", vpcID, "routeTableId", rtb.RouteTableId) + return nil, awserrors.HasDependencies("vpc", vpcID) } } @@ -577,8 +577,8 @@ func (s *VPCServiceImpl) rejectAttachedIGW(ctx context.Context, accountID, vpcID return errors.New(awserrors.ErrorServerInternal) } if igw.VpcId == vpcID { - return awserrors.Errorf(awserrors.ErrorDependencyViolation, - "the VPC has a dependent internet gateway %s that must be detached first", igw.InternetGatewayId) + slog.WarnContext(ctx, "DeleteVpc: VPC has an attached internet gateway", "vpcId", vpcID, "internetGatewayId", igw.InternetGatewayId) + return awserrors.HasDependencies("vpc", vpcID) } } @@ -677,9 +677,9 @@ func (s *VPCServiceImpl) DescribeVpcs(ctx context.Context, input *ec2.DescribeVp found[*vpc.VpcId] = true } } - for id := range vpcIDs { - if !found[id] { - return nil, errors.New(awserrors.ErrorInvalidVpcIDNotFound) + for _, id := range input.VpcIds { + if id != nil && !found[*id] { + return nil, vpcNotFoundError(*id) } } } @@ -711,7 +711,7 @@ func (s *VPCServiceImpl) CreateSubnet(ctx context.Context, input *ec2.CreateSubn // Verify VPC exists and belongs to this account vpcEntry, err := s.vpcKV.Get(ctx, utils.AccountKey(accountID, vpcID)) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidVpcIDNotFound) + return nil, vpcNotFoundError(vpcID) } var vpcRecord VPCRecord @@ -722,7 +722,7 @@ func (s *VPCServiceImpl) CreateSubnet(ctx context.Context, input *ec2.CreateSubn // AWS allows /16 to /28 for subnet CIDR blocks ones, _ := subnetNet.Mask.Size() if ones < 16 || ones > 28 { - return nil, errors.New(awserrors.ErrorInvalidSubnetRange) + return nil, subnetRangeError(*input.CidrBlock) } // Verify subnet CIDR is within VPC CIDR @@ -732,7 +732,7 @@ func (s *VPCServiceImpl) CreateSubnet(ctx context.Context, input *ec2.CreateSubn } if !vpcNet.Contains(subnetNet.IP) { - return nil, errors.New(awserrors.ErrorInvalidSubnetRange) + return nil, subnetRangeError(*input.CidrBlock) } // Check for CIDR conflicts with existing subnets in this VPC (same account) @@ -765,7 +765,7 @@ func (s *VPCServiceImpl) CreateSubnet(ctx context.Context, input *ec2.CreateSubn continue } if existingNet.Contains(subnetNet.IP) || subnetNet.Contains(existingNet.IP) { - return nil, errors.New(awserrors.ErrorInvalidSubnetConflict) + return nil, awserrors.Errorf(awserrors.ErrorInvalidSubnetConflict, "The CIDR '%s' conflicts with another subnet", *input.CidrBlock) } } @@ -895,7 +895,7 @@ func (s *VPCServiceImpl) checkSubnetResidents(ctx context.Context, accountID, su continue } if record.SubnetId == subnetID && eniIsLiveAttachment(&record) { - return errors.New(awserrors.ErrorDependencyViolation) + return awserrors.HasDependencies("subnet", subnetID) } } return nil @@ -1404,7 +1404,7 @@ func (s *VPCServiceImpl) ModifyVpcAttribute(ctx context.Context, input *ec2.Modi entry, err := s.vpcKV.Get(ctx, key) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidVpcIDNotFound) + return nil, vpcNotFoundError(vpcID) } var record VPCRecord @@ -1452,7 +1452,7 @@ func (s *VPCServiceImpl) DescribeVpcAttribute(ctx context.Context, input *ec2.De entry, err := s.vpcKV.Get(ctx, key) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidVpcIDNotFound) + return nil, vpcNotFoundError(vpcID) } var record VPCRecord diff --git a/spinifex/handlers/ec2/vpc/service_impl_test.go b/spinifex/handlers/ec2/vpc/service_impl_test.go index 7ef67a41a..81696471a 100644 --- a/spinifex/handlers/ec2/vpc/service_impl_test.go +++ b/spinifex/handlers/ec2/vpc/service_impl_test.go @@ -213,16 +213,13 @@ func TestDeleteVpc_WithSubnets(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") + createTestSubnet(t, svc, vpcID, "10.0.1.0/24") // Should fail because VPC has subnets _, err := svc.DeleteVpc(context.Background(), &ec2.DeleteVpcInput{ VpcId: aws.String(vpcID), }, testAccountID) - assert.ErrorContains(t, err, "DependencyViolation") - // The message must name the blocking subnet so the caller knows what to - // delete first, not just that something is blocking. - assert.ErrorContains(t, err, subnetID) + requireAWSError(t, err, awserrors.ErrorDependencyViolation, "The vpc '"+vpcID+"' has dependencies and cannot be deleted.") } func TestDescribeVpcs_All(t *testing.T) { @@ -264,7 +261,10 @@ func TestDescribeVpcs_NotFound(t *testing.T) { _, err := svc.DescribeVpcs(context.Background(), &ec2.DescribeVpcsInput{ VpcIds: []*string{aws.String("vpc-nonexistent")}, }, testAccountID) - assert.ErrorContains(t, err, "InvalidVpcID.NotFound") + requireAWSError(t, err, awserrors.ErrorInvalidVpcIDNotFound, "The vpc ID 'vpc-nonexistent' does not exist") + + _, err = svc.DeleteVpc(context.Background(), &ec2.DeleteVpcInput{VpcId: aws.String("vpc-nonexistent")}, testAccountID) + requireAWSError(t, err, awserrors.ErrorInvalidVpcIDNotFound, "The vpc ID 'vpc-nonexistent' does not exist") } // --- Subnet Tests --- @@ -382,7 +382,7 @@ func TestCreateSubnet_OutsideVpcCidr(t *testing.T) { VpcId: aws.String(vpcID), CidrBlock: aws.String("192.168.1.0/24"), }, testAccountID) - assert.ErrorContains(t, err, "InvalidSubnet.Range") + requireAWSError(t, err, awserrors.ErrorInvalidSubnetRange, "The CIDR '192.168.1.0/24' is invalid.") } func TestCreateSubnet_ConflictingCidr(t *testing.T) { @@ -396,7 +396,7 @@ func TestCreateSubnet_ConflictingCidr(t *testing.T) { VpcId: aws.String(vpcID), CidrBlock: aws.String("10.0.1.0/25"), }, testAccountID) - assert.ErrorContains(t, err, "InvalidSubnet.Conflict") + requireAWSError(t, err, awserrors.ErrorInvalidSubnetConflict, "The CIDR '10.0.1.0/25' conflicts with another subnet") } func TestCreateSubnet_WithTags(t *testing.T) { @@ -857,8 +857,7 @@ func TestDeleteVpc_RejectsAttachedInternetGateway(t *testing.T) { _, err = svc.DeleteVpc(context.Background(), &ec2.DeleteVpcInput{VpcId: aws.String(vpcID)}, testAccountID) require.Error(t, err) - assert.ErrorContains(t, err, "DependencyViolation") - assert.ErrorContains(t, err, "igw-pending", "the caller must be told which gateway to detach") + requireAWSError(t, err, awserrors.ErrorDependencyViolation, "The vpc '"+vpcID+"' has dependencies and cannot be deleted.") desc, err := svc.DescribeVpcs(context.Background(), &ec2.DescribeVpcsInput{VpcIds: []*string{aws.String(vpcID)}}, testAccountID) require.NoError(t, err) @@ -1097,7 +1096,7 @@ func TestCreateSubnet_CidrTooSmall(t *testing.T) { VpcId: aws.String(vpcID), CidrBlock: aws.String("10.0.0.0/29"), }, testAccountID) - assert.ErrorContains(t, err, "InvalidSubnet.Range") + requireAWSError(t, err, awserrors.ErrorInvalidSubnetRange, "The CIDR '10.0.0.0/29' is invalid.") } func TestVpcCidrBlockAssociation(t *testing.T) {