fix: match AWS's CreateRoute check order and messages, icmp duplicate-rule text and untagged default subnets

spinifex/handlers/ec2/routetable/service_impl.go

@@ -6,6 +6,7 @@ import (
 	"errors"
 	"fmt"
 	"log/slog"
+	"net/netip"
 	"strings"
 	"time"
 
@@ -688,6 +689,30 @@ 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.
+// checkRouteOutsideVPC rejects a gateway route whose destination is, or lies
+// inside, one of the VPC's CIDR blocks (the table's local routes). AWS allows
+// only an interface or instance there, which Spinifex does not route to.
+func checkRouteOutsideVPC(record *RouteTableRecord, destCidr string) error {
+	dest, parseErr := netip.ParsePrefix(destCidr)
+	if parseErr != nil {
+		return nil //nolint:nilerr // an unparseable destination is not a VPC-range conflict
+	}
+	for _, r := range record.Routes {
+		if r.GatewayId != "local" {
+			continue
+		}
+		vpc, err := netip.ParsePrefix(r.DestinationCidrBlock)
+		if err != nil {
+			continue
+		}
+		if dest.Bits() >= vpc.Bits() && vpc.Contains(dest.Addr()) {
+			return awserrors.Errorf(awserrors.ErrorInvalidParameterValue,
+				"The destination CIDR block %s is equal to or more specific than one of this VPC's CIDR blocks. This route can target only an interface or an instance.", destCidr)
+		}
+	}
+	return nil
+}
+
 func rtbNotFoundError(id string) error {
 	return awserrors.IDNotFound(awserrors.ErrorInvalidRouteTableIDNotFound, "routeTable", id)
 }
@@ -716,19 +741,10 @@ func (s *RouteTableServiceImpl) CreateRoute(ctx context.Context, input *ec2.Crea
 		return nil, err
 	}
 
-	// AWS treats a repeat of an existing route (same destination, same target)
-	// as a successful no-op; only a different target is a conflict.
-	for _, r := range record.Routes {
-		if r.DestinationCidrBlock != destCidr {
-			continue
-		}
-		if r.GatewayId != "local" && r.GatewayId == aws.StringValue(input.GatewayId) && r.NatGatewayId == aws.StringValue(input.NatGatewayId) {
-			return &ec2.CreateRouteOutput{Return: aws.Bool(true)}, nil
-		}
-		return nil, errors.New(awserrors.ErrorRouteAlreadyExists)
-	}
-
+	// AWS validates the target, then the destination, then looks for an
+	// existing route, so events are held back until all three pass.
 	var route RouteRecord
+	var publish func()
 
 	switch {
 	case input.GatewayId != nil && *input.GatewayId != "":
@@ -757,7 +773,9 @@ func (s *RouteTableServiceImpl) CreateRoute(ctx context.Context, input *ec2.Crea
 
 		// Publish vpc.add-igw-route events for each subnet associated with this
 		// route table so the network subscriber installs per-subnet egress policies.
-		s.publishIGWRouteEvents(ctx, accountID, "vpc.add-igw-route", record, igwRecord.VpcId, igwID, destCidr)
+		publish = func() {
+			s.publishIGWRouteEvents(ctx, accountID, "vpc.add-igw-route", record, igwRecord.VpcId, igwID, destCidr)
+		}
 
 	case input.NatGatewayId != nil && *input.NatGatewayId != "":
 		natgwID := *input.NatGatewayId
@@ -785,12 +803,32 @@ func (s *RouteTableServiceImpl) CreateRoute(ctx context.Context, input *ec2.Crea
 		}
 
 		// Publish vpc.add-nat-gateway events for each subnet associated with this route table
-		s.publishNatGatewayEvents(ctx, accountID, record, natgwRecord.VpcId, natgwID, natgwRecord.PublicIp, destCidr)
+		publish = func() {
+			s.publishNatGatewayEvents(ctx, accountID, record, natgwRecord.VpcId, natgwID, natgwRecord.PublicIp, destCidr)
+		}
 
 	default:
 		return nil, errors.New(awserrors.ErrorInvalidParameterValue)
 	}
 
+	if err := checkRouteOutsideVPC(record, destCidr); err != nil {
+		return nil, err
+	}
+
+	// A repeat of an existing route (same destination, same target) is a
+	// successful no-op; a different target is a conflict.
+	for _, r := range record.Routes {
+		if r.DestinationCidrBlock != destCidr {
+			continue
+		}
+		if r.GatewayId == route.GatewayId && r.NatGatewayId == route.NatGatewayId {
+			return &ec2.CreateRouteOutput{Return: aws.Bool(true)}, nil
+		}
+		return nil, awserrors.Errorf(awserrors.ErrorRouteAlreadyExists, "The route identified by %s already exists.", destCidr)
+	}
+
+	publish()
+
 	record.Routes = append(record.Routes, route)
 
 	if err := s.putRouteTable(ctx, accountID, record); err != nil {

spinifex/handlers/ec2/routetable/service_impl_test.go

@@ -427,15 +427,27 @@ func TestCreateRoute_DuplicateDestination(t *testing.T) {
 		DestinationCidrBlock: aws.String("0.0.0.0/0"),
 		NatGatewayId:         aws.String("nat-test1"),
 	}, testAccountID)
-	assert.EqualError(t, err, awserrors.ErrorRouteAlreadyExists)
+	requireAWSError(t, err, awserrors.ErrorRouteAlreadyExists, "The route identified by 0.0.0.0/0 already exists.")
 
-	// The local route's destination is never a no-op, even with target "local"
+	// AWS checks the target before the destination: "local" is no gateway
 	_, err = svc.CreateRoute(t.Context(), &ec2.CreateRouteInput{
 		RouteTableId:         aws.String(rtbID),
 		DestinationCidrBlock: aws.String("10.0.0.0/16"),
 		GatewayId:            aws.String("local"),
 	}, testAccountID)
-	assert.EqualError(t, err, awserrors.ErrorRouteAlreadyExists)
+	requireAWSError(t, err, awserrors.ErrorInvalidGatewayIDNotFound, "The gateway ID 'local' does not exist")
+
+	// A gateway route to the VPC's CIDR, or inside it, is refused before the
+	// existing local route is considered
+	for _, dest := range []string{"10.0.0.0/16", "10.0.5.0/24"} {
+		_, err = svc.CreateRoute(t.Context(), &ec2.CreateRouteInput{
+			RouteTableId:         aws.String(rtbID),
+			DestinationCidrBlock: aws.String(dest),
+			GatewayId:            aws.String("igw-test1"),
+		}, testAccountID)
+		requireAWSError(t, err, awserrors.ErrorInvalidParameterValue,
+			"The destination CIDR block "+dest+" is equal to or more specific than one of this VPC's CIDR blocks. This route can target only an interface or an instance.")
+	}
 
 	record, err := svc.getRouteTable(t.Context(), testAccountID, rtbID)
 	require.NoError(t, err)

spinifex/handlers/ec2/vpc/default_vpc.go

@@ -291,7 +291,6 @@ func (s *VPCServiceImpl) buildDefaultVPC(ctx context.Context, accountID string,
 		State:               "available",
 		IsDefault:           true,
 		MapPublicIpOnLaunch: !s.disableDefaultPublicIP, // AWS default subnets auto-assign public IPs (unless external mode has none)
-		Tags:                map[string]string{"Name": "default"},
 		CreatedAt:           time.Now(),
 	})
 	if err != nil {

spinifex/handlers/ec2/vpc/default_vpc_test.go

@@ -271,7 +271,7 @@ func TestEnsureDefaultVPC_UsesBootstrapIDs(t *testing.T) {
 	}
 }
 
-// AWS leaves the default VPC untagged.
+// AWS leaves the default VPC and its default subnet untagged.
 func TestEnsureDefaultVPC_LeavesVPCUntagged(t *testing.T) {
 	t.Parallel()
 	svcs, _, _ := newNodeServices(t, 1)
@@ -283,4 +283,9 @@ func TestEnsureDefaultVPC_LeavesVPCUntagged(t *testing.T) {
 	require.NoError(t, err)
 	require.Len(t, out.Vpcs, 1)
 	assert.Empty(t, out.Vpcs[0].Tags)
+
+	subnets, err := svcs[0].DescribeSubnets(t.Context(), &ec2.DescribeSubnetsInput{SubnetIds: []*string{aws.String(info.SubnetId)}}, "000000000001")
+	require.NoError(t, err)
+	require.Len(t, subnets.Subnets, 1)
+	assert.Empty(t, subnets.Subnets[0].Tags)
 }

spinifex/handlers/ec2/vpc/security_group.go

@@ -459,13 +459,26 @@ func sgRuleDuplicateError(rule SGRule) error {
 	if peer == "" {
 		peer = rule.SourceSG
 	}
-	desc := fmt.Sprintf("peer: %s, ALL, ALLOW", peer)
-	if rule.IpProtocol != allProtocols {
+	var desc string
+	switch rule.IpProtocol {
+	case allProtocols:
+		desc = fmt.Sprintf("peer: %s, ALL, ALLOW", peer)
+	case "icmp":
+		desc = fmt.Sprintf("peer: %s, ICMP, type: %s, code: %s, ALLOW", peer, icmpField(rule.FromPort), icmpField(rule.ToPort))
+	default:
 		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)
 }
 
+// icmpField renders an ICMP type or code as AWS does, with -1 as ALL.
+func icmpField(v int64) string {
+	if v == -1 {
+		return "ALL"
+	}
+	return strconv.FormatInt(v, 10)
+}
+
 // describeSecurityGroupsValidFilters defines the set of filter names accepted by DescribeSecurityGroups.
 var describeSecurityGroupsValidFilters = map[string]bool{
 	"group-id":           true,

spinifex/handlers/ec2/vpc/security_group_test.go

@@ -2407,6 +2407,17 @@ func TestAuthorizeSecurityGroupIngress_RejectsContentDuplicate(t *testing.T) {
 	}, testAccountID)
 	requireAWSError(t, err, awserrors.ErrorInvalidPermissionDuplicate, `the specified rule "peer: 0.0.0.0/0, ALL, ALLOW" already exists`)
 
+	icmp := &ec2.IpPermission{
+		IpProtocol: aws.String("icmp"), FromPort: aws.Int64(8), ToPort: aws.Int64(-1),
+		IpRanges: []*ec2.IpRange{{CidrIp: aws.String("10.0.0.0/24")}},
+	}
+	for range 2 {
+		_, err = svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{
+			GroupId: aws.String(sgID), IpPermissions: []*ec2.IpPermission{icmp},
+		}, testAccountID)
+	}
+	requireAWSError(t, err, awserrors.ErrorInvalidPermissionDuplicate, `the specified rule "peer: 10.0.0.0/24, ICMP, type: 8, code: ALL, ALLOW" already exists`)
+
 	_, err = svc.AuthorizeSecurityGroupIngress(context.Background(), &ec2.AuthorizeSecurityGroupIngressInput{
 		GroupId: aws.String(sgID),
 		IpPermissions: []*ec2.IpPermission{{