From 363aa1c4abe1706196e3d032aa827bb406130241 Mon Sep 17 00:00:00 2001 From: Julian Sommer <115516918+juliansommer@users.noreply.github.com> Date: Wed, 30 Sep 2026 10:42:33 +1000 Subject: [PATCH] fix: match AWS's error codes and messages for bad CIDRs, missing gateways and routes, the default group name and malformed IDs --- spinifex/awserrors/awserrors.go | 4 +- spinifex/handlers/ec2/eigw/service_impl.go | 18 +++++++ .../handlers/ec2/eigw/service_impl_test.go | 21 ++++++++ spinifex/handlers/ec2/eip/associate_test.go | 5 +- .../handlers/ec2/routetable/service_impl.go | 45 +++++++++-------- .../ec2/routetable/service_impl_test.go | 43 +++++++++++++++++ spinifex/handlers/ec2/vpc/eni.go | 9 ++-- spinifex/handlers/ec2/vpc/eni_test.go | 25 +++++++++- spinifex/handlers/ec2/vpc/ids.go | 48 +++++++++++++++++++ spinifex/handlers/ec2/vpc/security_group.go | 25 ++++++---- .../ec2/vpc/security_group_modify_test.go | 8 ++++ .../handlers/ec2/vpc/security_group_test.go | 33 +++++++++++-- spinifex/handlers/ec2/vpc/service_impl.go | 22 +++++---- .../handlers/ec2/vpc/service_impl_test.go | 17 +++++-- tests/integration/routetable_test.go | 4 +- 15 files changed, 271 insertions(+), 56 deletions(-) create mode 100644 spinifex/handlers/ec2/vpc/ids.go diff --git a/spinifex/awserrors/awserrors.go b/spinifex/awserrors/awserrors.go index bf6885f51..b691d0a8f 100644 --- a/spinifex/awserrors/awserrors.go +++ b/spinifex/awserrors/awserrors.go @@ -314,7 +314,7 @@ var ( ErrorInvalidVpcPeeringConnectionIDNotFound = "InvalidVpcPeeringConnectionID.NotFound" ErrorInvalidVpcPeeringConnectionIdMalformed = "InvalidVpcPeeringConnectionId.Malformed" ErrorInvalidVpcPeeringConnectionStateDnsHostnamesDisabled = "InvalidVpcPeeringConnectionState.DnsHostnamesDisabled" - ErrorInvalidVpcRange = "InvalidVpcRange" + ErrorInvalidVpcRange = "InvalidVpc.Range" ErrorInvalidVpcState = "InvalidVpcState" ErrorInvalidVpnConnectionInvalidState = "InvalidVpnConnection.InvalidState" ErrorInvalidVpnConnectionInvalidType = "InvalidVpnConnection.InvalidType" @@ -1097,7 +1097,7 @@ var ErrorLookup = map[string]ErrorMessage{ ErrorInvalidVpcPeeringConnectionIDNotFound: {HTTPCode: 404, Message: "The specified VPC peering connection ID does not exist."}, ErrorInvalidVpcPeeringConnectionIdMalformed: {HTTPCode: 400, Message: "The specified VPC peering connection ID is malformed. Ensure that you provide the ID in the form pcx-xxxxxxxxxxxxxxxxx."}, ErrorInvalidVpcPeeringConnectionStateDnsHostnamesDisabled: {HTTPCode: 400, Message: "To enable DNS hostname resolution for the VPC peering connection, DNS hostname support must be enabled for the VPCs."}, - ErrorInvalidVpcRange: {HTTPCode: 400, Message: "The specified CIDR block range is not valid. The block range must be between a /28 netmask and /16 netmask. For more information, see VPC CIDR blocks."}, + ErrorInvalidVpcRange: {HTTPCode: 400, Message: "The CIDR block is invalid."}, ErrorInvalidVpcState: {HTTPCode: 400, Message: "The specified VPC already has a virtual private gateway attached to it."}, ErrorInvalidVpnConnectionInvalidState: {HTTPCode: 400, Message: "The VPN connection must be in the available state to complete the request."}, ErrorInvalidVpnConnectionInvalidType: {HTTPCode: 400, Message: "The specified VPN connection does not support static routes."}, diff --git a/spinifex/handlers/ec2/eigw/service_impl.go b/spinifex/handlers/ec2/eigw/service_impl.go index 7bb9fa72c..8f7b1057c 100644 --- a/spinifex/handlers/ec2/eigw/service_impl.go +++ b/spinifex/handlers/ec2/eigw/service_impl.go @@ -2,6 +2,7 @@ package handlers_ec2_eigw import ( "context" + "encoding/hex" "encoding/json" "errors" "fmt" @@ -167,6 +168,11 @@ func (s *EgressOnlyIGWServiceImpl) DescribeEgressOnlyInternetGateways(ctx contex slog.WarnContext(ctx, "DescribeEgressOnlyInternetGateways: invalid filter", "err", err) return nil, err } + for _, id := range input.EgressOnlyInternetGatewayIds { + if id != nil && !eigwIDWellFormed(*id) { + return nil, awserrors.Errorf(awserrors.ErrorInvalidEgressOnlyInternetGatewayIdMalformed, "The eigw ID %s is malformed", *id) + } + } prefix := accountID + "." keys, err := s.eigwKV.Keys(ctx) @@ -212,6 +218,18 @@ func (s *EgressOnlyIGWServiceImpl) DescribeEgressOnlyInternetGateways(ctx contex }, nil } +// eigwIDWellFormed reports whether AWS accepts id as an egress-only gateway ID: +// "eigw-" and exactly 17 hex digits of either case. A well-formed unknown ID is +// not an error; the describe returns no gateway for it. +func eigwIDWellFormed(id string) bool { + suffix, ok := strings.CutPrefix(id, "eigw-") + if !ok || len(suffix) != 17 { + return false + } + _, err := hex.DecodeString("0" + suffix) + return err == nil +} + // eigwMatchesFilters checks whether an EgressOnlyIGWRecord satisfies all parsed filters. func eigwMatchesFilters(record *EgressOnlyIGWRecord, filters map[string][]string) bool { for name, values := range filters { diff --git a/spinifex/handlers/ec2/eigw/service_impl_test.go b/spinifex/handlers/ec2/eigw/service_impl_test.go index 731ea1d22..ac771a31c 100644 --- a/spinifex/handlers/ec2/eigw/service_impl_test.go +++ b/spinifex/handlers/ec2/eigw/service_impl_test.go @@ -337,3 +337,24 @@ func TestCreateEgressOnlyInternetGateway_CrossAccountVPCRejected(t *testing.T) { require.NoError(t, err) assert.NotNil(t, out.EgressOnlyInternetGateway) } + +func TestDescribeEgressOnlyInternetGateways_MalformedID(t *testing.T) { + svc := setupTestEIGWService(t) + describe := func(ids ...string) error { + _, err := svc.DescribeEgressOnlyInternetGateways(context.Background(), &ec2.DescribeEgressOnlyInternetGatewaysInput{ + EgressOnlyInternetGatewayIds: aws.StringSlice(ids), + }, testAccountID) + return err + } + + for _, id := range []string{"eigw-xyz", "eigw-12345", "eigw-0000000000000001", "eigw-000000000000000001", "foo"} { + err := describe(id) + code, msg, ok := awserrors.ResolveErrorDetail(err) + require.True(t, ok, "error %v carries no AWS code", err) + assert.Equal(t, awserrors.ErrorInvalidEgressOnlyInternetGatewayIdMalformed, code) + assert.Equal(t, "The eigw ID "+id+" is malformed", msg) + } + // Exactly 17 hex digits of either case is well formed, and unknown is not an error. + assert.NoError(t, describe("eigw-00000000000000001")) + assert.NoError(t, describe("eigw-0AAAAAAAAAAAAAAA1")) +} diff --git a/spinifex/handlers/ec2/eip/associate_test.go b/spinifex/handlers/ec2/eip/associate_test.go index 404d2bc44..ee1a979f9 100644 --- a/spinifex/handlers/ec2/eip/associate_test.go +++ b/spinifex/handlers/ec2/eip/associate_test.go @@ -117,10 +117,11 @@ func TestEIP_AssociateByNetworkInterfaceID_NotFound(t *testing.T) { _, err := svc.AssociateAddress(context.Background(), &ec2.AssociateAddressInput{ AllocationId: aws.String(allocID), - NetworkInterfaceId: aws.String("eni-doesnotexist"), + NetworkInterfaceId: aws.String("eni-0000000000000dead"), }, testAccountID) require.Error(t, err) - assert.Equal(t, awserrors.ErrorInvalidNetworkInterfaceIDNotFound, err.Error()) + code, _ := awserrors.ResolveErrorCode(err) + assert.Equal(t, awserrors.ErrorInvalidNetworkInterfaceIDNotFound, code) // The allocation must survive a failed association. addr := describeEIP(t, svc, allocID) diff --git a/spinifex/handlers/ec2/routetable/service_impl.go b/spinifex/handlers/ec2/routetable/service_impl.go index 8394186f2..d815cb3e4 100644 --- a/spinifex/handlers/ec2/routetable/service_impl.go +++ b/spinifex/handlers/ec2/routetable/service_impl.go @@ -685,6 +685,12 @@ 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 gatewayNotFoundError(id string) error { + return awserrors.Errorf(awserrors.ErrorInvalidGatewayIDNotFound, "The gateway ID '%s' does not exist", id) +} + func (s *RouteTableServiceImpl) CreateRoute(ctx context.Context, input *ec2.CreateRouteInput, accountID string) (*ec2.CreateRouteOutput, error) { if input.RouteTableId == nil || *input.RouteTableId == "" { return nil, errors.New(awserrors.ErrorMissingParameter) @@ -716,7 +722,7 @@ func (s *RouteTableServiceImpl) CreateRoute(ctx context.Context, input *ec2.Crea // Verify IGW exists and is attached to the same VPC igwEntry, err := s.igwKV.Get(ctx, utils.AccountKey(accountID, igwID)) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidInternetGatewayIDNotFound) + return nil, gatewayNotFoundError(igwID) } var igwRecord handlers_ec2_igw.IGWRecord if err := json.Unmarshal(igwEntry.Value(), &igwRecord); err != nil { @@ -868,6 +874,23 @@ func (s *RouteTableServiceImpl) ReplaceRoute(ctx context.Context, input *ec2.Rep return nil, err } + // V1: only GatewayId target supported + if input.GatewayId == nil || *input.GatewayId == "" { + return nil, errors.New(awserrors.ErrorInvalidParameterValue) + } + + igwID := *input.GatewayId + + // AWS reports a missing gateway ahead of a missing route. + igwEntry, err := s.igwKV.Get(ctx, utils.AccountKey(accountID, igwID)) + if err != nil { + return nil, gatewayNotFoundError(igwID) + } + var igwRecord handlers_ec2_igw.IGWRecord + if err := json.Unmarshal(igwEntry.Value(), &igwRecord); err != nil { + return nil, errors.New(awserrors.ErrorServerInternal) + } + idx := -1 for i, r := range record.Routes { if r.DestinationCidrBlock == destCidr { @@ -876,29 +899,13 @@ func (s *RouteTableServiceImpl) ReplaceRoute(ctx context.Context, input *ec2.Rep } } if idx < 0 { - return nil, errors.New(awserrors.ErrorInvalidRouteNotFound) + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "There is no route defined for '%s' in the route table. Use CreateRoute instead.", destCidr) } if record.Routes[idx].GatewayId == "local" { return nil, errors.New(awserrors.ErrorInvalidParameterValue) } - - // V1: only GatewayId target supported - if input.GatewayId == nil || *input.GatewayId == "" { - return nil, errors.New(awserrors.ErrorInvalidParameterValue) - } - - igwID := *input.GatewayId - - // Verify IGW exists and is attached to same VPC - igwEntry, err := s.igwKV.Get(ctx, utils.AccountKey(accountID, igwID)) - if err != nil { - return nil, errors.New(awserrors.ErrorInvalidInternetGatewayIDNotFound) - } - var igwRecord handlers_ec2_igw.IGWRecord - if err := json.Unmarshal(igwEntry.Value(), &igwRecord); err != nil { - return nil, errors.New(awserrors.ErrorServerInternal) - } if igwRecord.VpcId != record.VpcId { return nil, errors.New(awserrors.ErrorInvalidParameterValue) } diff --git a/spinifex/handlers/ec2/routetable/service_impl_test.go b/spinifex/handlers/ec2/routetable/service_impl_test.go index 202a6246a..2df529526 100644 --- a/spinifex/handlers/ec2/routetable/service_impl_test.go +++ b/spinifex/handlers/ec2/routetable/service_impl_test.go @@ -490,6 +490,49 @@ func TestReplaceRoute(t *testing.T) { require.NoError(t, err) } +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 TestCreateRoute_MissingGateway(t *testing.T) { + t.Parallel() + svc := setupTestService(t) + rtbID := createTestRtb(t, svc) + + _, err := svc.CreateRoute(t.Context(), &ec2.CreateRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("0.0.0.0/0"), + GatewayId: aws.String("igw-0000000000000dead"), + }, testAccountID) + requireAWSError(t, err, awserrors.ErrorInvalidGatewayIDNotFound, "The gateway ID 'igw-0000000000000dead' does not exist") +} + +func TestReplaceRoute_MissingRouteOrGateway(t *testing.T) { + t.Parallel() + svc := setupTestService(t) + rtbID := createTestRtb(t, svc) + + _, err := svc.ReplaceRoute(t.Context(), &ec2.ReplaceRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("198.51.100.0/24"), + GatewayId: aws.String("igw-test1"), + }, testAccountID) + requireAWSError(t, err, awserrors.ErrorInvalidParameterValue, + "There is no route defined for '198.51.100.0/24' in the route table. Use CreateRoute instead.") + + // AWS reports the missing gateway ahead of the missing route. + _, err = svc.ReplaceRoute(t.Context(), &ec2.ReplaceRouteInput{ + RouteTableId: aws.String(rtbID), + DestinationCidrBlock: aws.String("198.51.100.0/24"), + GatewayId: aws.String("igw-0000000000000dead"), + }, testAccountID) + requireAWSError(t, err, awserrors.ErrorInvalidGatewayIDNotFound, "The gateway ID 'igw-0000000000000dead' does not exist") +} + func TestAssociateRouteTable(t *testing.T) { t.Parallel() svc := setupTestService(t) diff --git a/spinifex/handlers/ec2/vpc/eni.go b/spinifex/handlers/ec2/vpc/eni.go index 683645718..e45a5a164 100644 --- a/spinifex/handlers/ec2/vpc/eni.go +++ b/spinifex/handlers/ec2/vpc/eni.go @@ -546,6 +546,9 @@ func (s *VPCServiceImpl) DescribeNetworkInterfaces(ctx context.Context, input *e eniIDs := make(map[string]bool) for _, id := range input.NetworkInterfaceIds { if id != nil { + if err := eniIDMalformedError(*id); err != nil { + return nil, err + } eniIDs[*id] = true } } @@ -594,9 +597,9 @@ func (s *VPCServiceImpl) DescribeNetworkInterfaces(ctx context.Context, input *e found[*eni.NetworkInterfaceId] = true } } - for id := range eniIDs { - if !found[id] { - return nil, errors.New(awserrors.ErrorInvalidNetworkInterfaceIDNotFound) + for _, id := range input.NetworkInterfaceIds { + if id != nil && !found[*id] { + return nil, awserrors.Errorf(awserrors.ErrorInvalidNetworkInterfaceIDNotFound, "The networkInterface ID '%s' does not exist", *id) } } } diff --git a/spinifex/handlers/ec2/vpc/eni_test.go b/spinifex/handlers/ec2/vpc/eni_test.go index e6508ea9f..752874caf 100644 --- a/spinifex/handlers/ec2/vpc/eni_test.go +++ b/spinifex/handlers/ec2/vpc/eni_test.go @@ -413,9 +413,30 @@ func TestDescribeNetworkInterfaces_NotFound(t *testing.T) { t.Parallel() svc := setupTestVPCService(t) _, err := svc.DescribeNetworkInterfaces(context.Background(), &ec2.DescribeNetworkInterfacesInput{ - NetworkInterfaceIds: []*string{aws.String("eni-nonexistent")}, + NetworkInterfaceIds: []*string{aws.String("eni-0000000000000dead")}, }, testAccountID) - assert.ErrorContains(t, err, "InvalidNetworkInterfaceID.NotFound") + requireAWSError(t, err, awserrors.ErrorInvalidNetworkInterfaceIDNotFound, "The networkInterface ID 'eni-0000000000000dead' does not exist") +} + +func TestDescribeNetworkInterfaces_MalformedID(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + describe := func(ids ...string) error { + _, err := svc.DescribeNetworkInterfaces(context.Background(), &ec2.DescribeNetworkInterfacesInput{ + NetworkInterfaceIds: aws.StringSlice(ids), + }, testAccountID) + return err + } + + for _, id := range []string{"eni-xyz", "eni-", "eni-ABC"} { + requireAWSError(t, describe(id), awserrors.ErrorInvalidNetworkInterfaceIdMalformed, fmt.Sprintf("Invalid id: %q", id)) + } + requireAWSError(t, describe("foo"), awserrors.ErrorInvalidNetworkInterfaceIdMalformed, `Invalid id: "foo" (expecting "eni-...")`) + // Unlike a group ID, any length of lowercase hex is merely unknown. + for _, id := range []string{"eni-12345", "eni-000000000000000000001"} { + requireAWSCode(t, describe(id), awserrors.ErrorInvalidNetworkInterfaceIDNotFound) + } + requireAWSCode(t, describe("eni-0aaaaaaa", "eni-xyz"), awserrors.ErrorInvalidNetworkInterfaceIdMalformed) } func TestAttachENI(t *testing.T) { diff --git a/spinifex/handlers/ec2/vpc/ids.go b/spinifex/handlers/ec2/vpc/ids.go new file mode 100644 index 000000000..0af2db260 --- /dev/null +++ b/spinifex/handlers/ec2/vpc/ids.go @@ -0,0 +1,48 @@ +package handlers_ec2_vpc + +import ( + "strings" + + "github.com/mulgadc/spinifex/spinifex/awserrors" +) + +// isLowerHex reports whether s is non-empty and holds only lowercase hex digits. +func isLowerHex(s string) bool { + if s == "" { + return false + } + for _, c := range s { + if (c < '0' || c > '9') && (c < 'a' || c > 'f') { + return false + } + } + return true +} + +// malformedIDError is AWS's Invalid id answer for an ID that is not of the +// form -, or whose suffix fails suffixOK. +func malformedIDError(code, prefix, id string, suffixOK func(string) bool) error { + suffix, ok := strings.CutPrefix(id, prefix+"-") + if !ok { + return awserrors.Errorf(code, "Invalid id: %q (expecting %q)", id, prefix+"-...") + } + if !suffixOK(suffix) { + return awserrors.Errorf(code, "Invalid id: %q", id) + } + return nil +} + +// sgIDMalformedError returns AWS's error for a group ID it calls malformed, or +// nil. AWS answers NotFound for up to 8 hex digits or exactly 17, the length +// of every Spinifex ID; other lengths are Malformed. +func sgIDMalformedError(id string) error { + return malformedIDError(awserrors.ErrorInvalidGroupIdMalformed, "sg", id, func(s string) bool { + return isLowerHex(s) && (len(s) <= 8 || len(s) == 17) + }) +} + +// eniIDMalformedError returns AWS's error for an interface ID it calls +// malformed, or nil. AWS answers NotFound for lowercase hex of any length. +func eniIDMalformedError(id string) error { + return malformedIDError(awserrors.ErrorInvalidNetworkInterfaceIdMalformed, "eni", id, isLowerHex) +} diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index 61e42e876..5926516f8 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/spinifex/handlers/ec2/vpc/security_group.go @@ -184,15 +184,14 @@ func (s *VPCServiceImpl) CreateSecurityGroup(ctx context.Context, input *ec2.Cre vpcId := *input.VpcId groupName := *input.GroupName - // "default" is reserved for the per-VPC default SG that CreateVpc - // provisions internally. Matches AWS behavior. - if groupName == defaultSecurityGroupName { - return nil, errors.New(awserrors.ErrorInvalidGroupReserved) - } - if err := s.requireVPCExists(ctx, accountID, vpcId); err != nil { return nil, err } + // "default" is reserved for the per-VPC default SG that CreateVpc + // provisions internally. AWS checks the VPC first. + if groupName == defaultSecurityGroupName { + return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "Cannot use reserved security group name: %s", groupName) + } if strings.HasPrefix(groupName, "sg-") { return nil, awserrors.Errorf(awserrors.ErrorInvalidParameterValue, "Value (%s) for parameter GroupName is invalid. Group names may not be in the format sg-*.", groupName) @@ -487,6 +486,14 @@ func (s *VPCServiceImpl) DescribeSecurityGroups(ctx context.Context, input *ec2. slog.WarnContext(ctx, "DescribeSecurityGroups: invalid filter", "err", err) return nil, err } + for _, id := range input.GroupIds { + if id == nil { + continue + } + if err := sgIDMalformedError(*id); err != nil { + return nil, err + } + } prefix := accountID + "." keys, err := s.sgKV.Keys(ctx) @@ -533,9 +540,9 @@ func (s *VPCServiceImpl) DescribeSecurityGroups(ctx context.Context, input *ec2. found[*sg.GroupId] = true } } - for id := range groupIDs { - if !found[id] { - return nil, errors.New(awserrors.ErrorInvalidGroupNotFound) + for _, id := range input.GroupIds { + if id != nil && !found[*id] { + return nil, awserrors.Errorf(awserrors.ErrorInvalidGroupNotFound, "The security group '%s' does not exist", *id) } } } diff --git a/spinifex/handlers/ec2/vpc/security_group_modify_test.go b/spinifex/handlers/ec2/vpc/security_group_modify_test.go index 47648f9da..22fcf6cc3 100644 --- a/spinifex/handlers/ec2/vpc/security_group_modify_test.go +++ b/spinifex/handlers/ec2/vpc/security_group_modify_test.go @@ -28,6 +28,14 @@ func requireAWSCode(t *testing.T, err error, code string) { assert.Equal(t, code, got, "error: %v", err) } +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 modifyRules(svc *VPCServiceImpl, sgID string, updates ...*ec2.SecurityGroupRuleUpdate) error { _, err := svc.ModifySecurityGroupRules(context.Background(), &ec2.ModifySecurityGroupRulesInput{ GroupId: aws.String(sgID), diff --git a/spinifex/handlers/ec2/vpc/security_group_test.go b/spinifex/handlers/ec2/vpc/security_group_test.go index 793c240dc..8e05ad60b 100644 --- a/spinifex/handlers/ec2/vpc/security_group_test.go +++ b/spinifex/handlers/ec2/vpc/security_group_test.go @@ -3,6 +3,7 @@ package handlers_ec2_vpc import ( "context" "encoding/json" + "fmt" "strings" "testing" "time" @@ -177,10 +178,33 @@ func TestDescribeSecurityGroups_NotFound(t *testing.T) { svc := setupTestVPCService(t) _, err := svc.DescribeSecurityGroups(context.Background(), &ec2.DescribeSecurityGroupsInput{ - GroupIds: []*string{aws.String("sg-nonexistent")}, + GroupIds: []*string{aws.String("sg-0000000000000dead")}, }, testAccountID) - assert.Error(t, err) - assert.Contains(t, err.Error(), "InvalidGroup.NotFound") + requireAWSError(t, err, awserrors.ErrorInvalidGroupNotFound, "The security group 'sg-0000000000000dead' does not exist") +} + +// The shapes below are the answers AWS gave for each ID; a 17-digit ID is +// always treated as unknown here, though AWS decodes some of those as Malformed. +func TestDescribeSecurityGroups_MalformedID(t *testing.T) { + t.Parallel() + svc := setupTestVPCService(t) + describe := func(ids ...string) error { + _, err := svc.DescribeSecurityGroups(context.Background(), &ec2.DescribeSecurityGroupsInput{ + GroupIds: aws.StringSlice(ids), + }, testAccountID) + return err + } + + for _, id := range []string{"sg-xyz", "sg-", "sg-0AAAAAAAAAAAAAAA1", "sg-000000001", "sg-0000000000000001", "sg-000000000000000001"} { + requireAWSError(t, describe(id), awserrors.ErrorInvalidGroupIdMalformed, fmt.Sprintf("Invalid id: %q", id)) + } + requireAWSError(t, describe("foo"), awserrors.ErrorInvalidGroupIdMalformed, `Invalid id: "foo" (expecting "sg-...")`) + for _, id := range []string{"sg-a", "sg-0aaaaaaa", "sg-01234567890abcdef"} { + requireAWSCode(t, describe(id), awserrors.ErrorInvalidGroupNotFound) + } + + // A malformed ID wins over an unknown one, wherever it sits. + requireAWSCode(t, describe("sg-0aaaaaaa", "sg-xyz"), awserrors.ErrorInvalidGroupIdMalformed) } // --- AuthorizeSecurityGroupIngress --- @@ -1550,8 +1574,7 @@ func TestCreateSecurityGroup_RejectsReservedDefaultName(t *testing.T) { Description: aws.String("user-supplied"), VpcId: aws.String(vpcID), }, testAccountID) - require.Error(t, err) - assert.Contains(t, err.Error(), "InvalidGroup.Reserved") + requireAWSError(t, err, awserrors.ErrorInvalidParameterValue, "Cannot use reserved security group name: default") } func TestCreateSecurityGroup_RejectsSgPrefixedName(t *testing.T) { diff --git a/spinifex/handlers/ec2/vpc/service_impl.go b/spinifex/handlers/ec2/vpc/service_impl.go index f0f881799..0d9f82dd9 100644 --- a/spinifex/handlers/ec2/vpc/service_impl.go +++ b/spinifex/handlers/ec2/vpc/service_impl.go @@ -255,22 +255,27 @@ func (s *VPCServiceImpl) nextVNI(ctx context.Context) (int64, error) { return allocated, nil } +// invalidCIDRBlockError is AWS's answer to a CidrBlock that does not parse. +func invalidCIDRBlockError(cidr string) error { + return awserrors.Errorf(awserrors.ErrorInvalidParameterValue, + "Value (%s) for parameter cidrBlock is invalid. This is not a valid CIDR block.", cidr) +} + // CreateVpc creates a new VPC. func (s *VPCServiceImpl) CreateVpc(ctx context.Context, input *ec2.CreateVpcInput, accountID string) (*ec2.CreateVpcOutput, error) { if input.CidrBlock == nil || *input.CidrBlock == "" { return nil, errors.New(awserrors.ErrorMissingParameter) } - // Validate CIDR block _, ipNet, err := net.ParseCIDR(*input.CidrBlock) if err != nil { - return nil, errors.New(awserrors.ErrorInvalidVpcRange) + return nil, invalidCIDRBlockError(*input.CidrBlock) } // AWS allows /16 to /28 for VPC CIDR blocks ones, _ := ipNet.Mask.Size() if ones < 16 || ones > 28 { - return nil, errors.New(awserrors.ErrorInvalidVpcRange) + return nil, awserrors.Errorf(awserrors.ErrorInvalidVpcRange, "The CIDR '%s' is invalid.", *input.CidrBlock) } // Allocate VNI for overlay network @@ -684,6 +689,12 @@ func (s *VPCServiceImpl) CreateSubnet(ctx context.Context, input *ec2.CreateSubn vpcID := *input.VpcId + // AWS rejects an unparseable CIDR before it looks the VPC up. + _, subnetNet, err := net.ParseCIDR(*input.CidrBlock) + if err != nil { + return nil, invalidCIDRBlockError(*input.CidrBlock) + } + // Verify VPC exists and belongs to this account vpcEntry, err := s.vpcKV.Get(ctx, utils.AccountKey(accountID, vpcID)) if err != nil { @@ -694,11 +705,6 @@ func (s *VPCServiceImpl) CreateSubnet(ctx context.Context, input *ec2.CreateSubn if err := json.Unmarshal(vpcEntry.Value(), &vpcRecord); err != nil { return nil, errors.New(awserrors.ErrorServerInternal) } - // Validate subnet CIDR - _, subnetNet, err := net.ParseCIDR(*input.CidrBlock) - if err != nil { - return nil, errors.New(awserrors.ErrorInvalidSubnetRange) - } // AWS allows /16 to /28 for subnet CIDR blocks ones, _ := subnetNet.Mask.Size() diff --git a/spinifex/handlers/ec2/vpc/service_impl_test.go b/spinifex/handlers/ec2/vpc/service_impl_test.go index e56a777ef..7ef67a41a 100644 --- a/spinifex/handlers/ec2/vpc/service_impl_test.go +++ b/spinifex/handlers/ec2/vpc/service_impl_test.go @@ -103,7 +103,8 @@ func TestCreateVpc_InvalidCidr(t *testing.T) { _, err := svc.CreateVpc(context.Background(), &ec2.CreateVpcInput{ CidrBlock: aws.String("not-a-cidr"), }, testAccountID) - assert.ErrorContains(t, err, "InvalidVpcRange") + requireAWSError(t, err, awserrors.ErrorInvalidParameterValue, + "Value (not-a-cidr) for parameter cidrBlock is invalid. This is not a valid CIDR block.") } func TestCreateVpc_CidrTooLarge(t *testing.T) { @@ -112,7 +113,7 @@ func TestCreateVpc_CidrTooLarge(t *testing.T) { _, err := svc.CreateVpc(context.Background(), &ec2.CreateVpcInput{ CidrBlock: aws.String("10.0.0.0/8"), }, testAccountID) - assert.ErrorContains(t, err, "InvalidVpcRange") + requireAWSError(t, err, "InvalidVpc.Range", "The CIDR '10.0.0.0/8' is invalid.") } func TestCreateVpc_CidrTooSmall(t *testing.T) { @@ -121,7 +122,7 @@ func TestCreateVpc_CidrTooSmall(t *testing.T) { _, err := svc.CreateVpc(context.Background(), &ec2.CreateVpcInput{ CidrBlock: aws.String("10.0.0.0/29"), }, testAccountID) - assert.ErrorContains(t, err, "InvalidVpcRange") + requireAWSError(t, err, "InvalidVpc.Range", "The CIDR '10.0.0.0/29' is invalid.") } func TestCreateVpc_WithTags(t *testing.T) { @@ -362,7 +363,15 @@ func TestCreateSubnet_InvalidCidr(t *testing.T) { VpcId: aws.String(vpcID), CidrBlock: aws.String("not-a-cidr"), }, testAccountID) - assert.ErrorContains(t, err, "InvalidSubnet.Range") + requireAWSError(t, err, awserrors.ErrorInvalidParameterValue, + "Value (not-a-cidr) for parameter cidrBlock is invalid. This is not a valid CIDR block.") + + // AWS rejects the CIDR before it looks the VPC up. + _, err = svc.CreateSubnet(context.Background(), &ec2.CreateSubnetInput{ + VpcId: aws.String("vpc-nonexistent"), + CidrBlock: aws.String("not-a-cidr"), + }, testAccountID) + requireAWSCode(t, err, awserrors.ErrorInvalidParameterValue) } func TestCreateSubnet_OutsideVpcCidr(t *testing.T) { diff --git a/tests/integration/routetable_test.go b/tests/integration/routetable_test.go index c07b3e377..fa45cfcb1 100644 --- a/tests/integration/routetable_test.go +++ b/tests/integration/routetable_test.go @@ -390,7 +390,7 @@ func TestReplaceRouteConvergence(t *testing.T) { DestinationCidrBlock: aws.String("198.51.100.0/24"), GatewayId: aws.String(igw2), }) - requireAWSErrorCode(t, err, "InvalidRoute.NotFound") + requireAWSErrorCode(t, err, "InvalidParameterValue") // Local route is immutable. _, err = c.ReplaceRoute(&ec2.ReplaceRouteInput{ @@ -413,7 +413,7 @@ func TestReplaceRouteConvergence(t *testing.T) { DestinationCidrBlock: aws.String(dest), GatewayId: aws.String("igw-00000000000000000"), }) - requireAWSErrorCode(t, err, "InvalidInternetGatewayID.NotFound") + requireAWSErrorCode(t, err, "InvalidGatewayID.NotFound") // IGW attached to a different VPC than the route table. _, err = c.ReplaceRoute(&ec2.ReplaceRouteInput{