diff --git a/spinifex/handlers/ec2/routetable/service_impl.go b/spinifex/handlers/ec2/routetable/service_impl.go index f3b54d20a..3926a54e8 100644 --- a/spinifex/handlers/ec2/routetable/service_impl.go +++ b/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 { diff --git a/spinifex/handlers/ec2/routetable/service_impl_test.go b/spinifex/handlers/ec2/routetable/service_impl_test.go index 155c7d94d..64abfdadf 100644 --- a/spinifex/handlers/ec2/routetable/service_impl_test.go +++ b/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) diff --git a/spinifex/handlers/ec2/vpc/default_vpc.go b/spinifex/handlers/ec2/vpc/default_vpc.go index 95a47b70c..34afbe999 100644 --- a/spinifex/handlers/ec2/vpc/default_vpc.go +++ b/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 { diff --git a/spinifex/handlers/ec2/vpc/default_vpc_test.go b/spinifex/handlers/ec2/vpc/default_vpc_test.go index b3b4f30d4..09fd648ea 100644 --- a/spinifex/handlers/ec2/vpc/default_vpc_test.go +++ b/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) } diff --git a/spinifex/handlers/ec2/vpc/security_group.go b/spinifex/handlers/ec2/vpc/security_group.go index 4ccb00d0c..eeb37b574 100644 --- a/spinifex/handlers/ec2/vpc/security_group.go +++ b/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, diff --git a/spinifex/handlers/ec2/vpc/security_group_test.go b/spinifex/handlers/ec2/vpc/security_group_test.go index 0ec21685e..cb6da1f5e 100644 --- a/spinifex/handlers/ec2/vpc/security_group_test.go +++ b/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{{