carlos / platform Public

awsinv: fix catch-all double-counting, nil-output panics, Route53 pagination

Three review findings, all ruled to fix:

- Inventory ran the per-region catch-all before the global S3/Route53/IAM
  mappings, so a bucket, hosted zone, or IAM role surfaced through a
  region's tagging call was emitted twice: once typed, once as KindOther.
  Global mappings now run first. Route53 also needed seeding the bare
  trailing id component into seen, since a zone's ID (/hostedzone/Z1)
  doesn't match the catch-all's parsed ARN id (Z1).
- None of the six new rest.go mappings guarded a nil-but-error-free output,
  unlike ec2.go's else-if-o-!=-nil pattern. The fakes this task itself
  added return exactly that shape for an unset field, so a future test
  written the obvious way would panic. Mirrored ec2.go's guard everywhere,
  including the per-bucket loc/versioning/PAB reads and catchAll's page.
- Route53 record pagination resumed on StartRecordName alone; the API
  needs StartRecordType (and StartRecordIdentifier) to resume at the exact
  record, so a page boundary within one name double-counted records, or
  never advanced when a single name filled a whole page. Now carries the
  full cursor triple and bails if the next cursor is nil or unchanged.

Regression tests: extended TestCatchAllDedupesAgainstTypedResources to wire
S3/Route53/IAM (the prior version only covered EC2, which is why the
double-count slipped through), and added
TestRoute53PaginationStopsWhenCursorDoesNotAdvance.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>

Paul Campbell pushed by paul@keymail.dev 2 months ago 13b3fe5f9c4e61ff1f02740c49f0995006884a86 parent 8fd2609

4 files changed, +192 −32

internal/awsinv/awsinv.go +37 −19
internal/awsinv/awsinv_test.go +44 −1
internal/awsinv/rest.go +50 −12
internal/awsinv/rest_test.go +61 −0

diff --git a/internal/awsinv/awsinv.go b/internal/awsinv/awsinv.go

index 1cbe61ae3..1eaa3c2ed 100644

--- a/internal/awsinv/awsinv.go

+++ b/internal/awsinv/awsinv.go

@@ -80,8 +80,17 @@ func newWithFactory(f Factory, home string, regions []string, account string) *P

return &Provider{factory: f, home: home, regions: regions, account: account, now: time.Now}

}

-// Inventory walks every region's typed mappings, then the catch-all, then

-// the global services once from the home region.

+// Inventory walks the global services first, then every region's typed

+// mappings and catch-all.

+//

+// The global-first ordering matters: the tagging catch-all "only ever

+// ADDS" (never duplicates a typed resource), and it runs once per region —

+// including the bucket's own region for an S3 bucket, and whichever region

+// happens to be home for a hosted zone or an IAM role, since those are

+// global services surfaced through a regional tagging call. If the catch-

+// all for that region ran before the global mappings had populated seen,

+// the resource would be double-counted: once under its typed kind, once

+// again as KindOther. Running S3/Route53/IAM first closes that gap.

// The error contract: an error means "this deployment cannot inventory

// itself at all". Anything less — a throttled call, one dead region, a

@@ -99,6 +108,15 @@ func (p *Provider) Inventory(ctx context.Context) (infraview.Inventory, error) {

add := func(rs []infraview.Resource) {

for _, r := range rs {

seen[r.ID] = true

+ // The catch-all derives its dedup key from an ARN's trailing

+ // path component (parseARN), while a typed resource's ID is not

+ // always that bare component — a Route53 zone's ID is

+ // "/hostedzone/Z1", not "Z1". Seeding the bare trailing

+ // component too is what lets the catch-all's parsed id match a

+ // typed ID that carries a path prefix.

+ if i := strings.LastIndex(r.ID, "/"); i >= 0 {

+ seen[r.ID[i+1:]] = true

+ }

inv.Resources = append(inv.Resources, r)

}

@@ -107,6 +125,23 @@ func (p *Provider) Inventory(ctx context.Context) (infraview.Inventory, error) {

// factory legitimately wires up only the service under test — a nil

// client here means "not exercised", not "denied", so it is skipped

// rather than dereferenced.

+ global := p.factory(p.home)

+ if global.S3 != nil {

+ rs, partial := s3Resources(ctx, global.S3)

+ add(rs)

+ note(partial)

+ }

+ if global.Route53 != nil {

+ rs, partial := route53Resources(ctx, global.Route53)

+ add(rs)

+ note(partial)

+ }

+ if global.IAM != nil {

+ rs, partial := iamResources(ctx, global.IAM)

+ add(rs)

+ note(partial)

+ }

for _, region := range p.regions {

c := p.factory(region)

if c.EC2 != nil {

@@ -131,23 +166,6 @@ func (p *Provider) Inventory(ctx context.Context) (infraview.Inventory, error) {

}

- global := p.factory(p.home)

- if global.S3 != nil {

- rs, partial := s3Resources(ctx, global.S3)

- add(rs)

- note(partial)

- }

- if global.Route53 != nil {

- rs, partial := route53Resources(ctx, global.Route53)

- add(rs)

- note(partial)

- }

- if global.IAM != nil {

- rs, partial := iamResources(ctx, global.IAM)

- add(rs)

- note(partial)

- }

for _, line := range inv.Partial {

if looksDenied(line) {

authDenied = true

diff --git a/internal/awsinv/awsinv_test.go b/internal/awsinv/awsinv_test.go

index 2dcb263ae..a6bcc8fca 100644

--- a/internal/awsinv/awsinv_test.go

+++ b/internal/awsinv/awsinv_test.go

@@ -8,8 +8,14 @@ import (

"github.com/aws/aws-sdk-go-v2/aws"

"github.com/aws/aws-sdk-go-v2/service/ec2"

ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types"

+ "github.com/aws/aws-sdk-go-v2/service/iam"

+ iamtypes "github.com/aws/aws-sdk-go-v2/service/iam/types"

"github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi"

rgtypes "github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi/types"

+ "github.com/aws/aws-sdk-go-v2/service/route53"

+ r53types "github.com/aws/aws-sdk-go-v2/service/route53/types"

+ "github.com/aws/aws-sdk-go-v2/service/s3"

+ s3types "github.com/aws/aws-sdk-go-v2/service/s3/types"

"github.com/carlosframework/platform/internal/infraview"

)

@@ -24,7 +30,13 @@ func (f *fakeTagging) GetResources(context.Context, *resourcegroupstaggingapi.Ge

// A resource the typed mappings already produced must NOT appear twice

// because the catch-all also saw it; anything the catch-all alone knows

-// about must appear as "other" rather than vanishing.

+// about must appear as "other" rather than vanishing. This covers all

+// three global services (S3, Route53, IAM) as well as EC2, because the

+// tagging catch-all runs once per region and a global service's resources

+// surface through whichever region happens to be home for it — a bucket

+// in its own region, a hosted zone or an IAM role through us-east-1 — so a

+// test that wires only EC2 would never exercise the global/regional

+// ordering the catch-all's dedup depends on.

func TestCatchAllDedupesAgainstTypedResources(t *testing.T) {

factory := func(region string) Clients {

return Clients{

@@ -33,10 +45,29 @@ func TestCatchAllDedupesAgainstTypedResources(t *testing.T) {

InstanceId: aws.String("i-123"),

}}}},

}},

+ S3: &fakeS3{

+ buckets: &s3.ListBucketsOutput{Buckets: []s3types.Bucket{{Name: aws.String("carlos-data-ie")}}},

+ },

+ Route53: &fakeRoute53{

+ zones: &route53.ListHostedZonesOutput{HostedZones: []r53types.HostedZone{

+ {Id: aws.String("/hostedzone/Z1"), Name: aws.String("carlosframework.com.")},

+ }},

+ recs: &route53.ListResourceRecordSetsOutput{},

+ },

+ IAM: &fakeIAM{

+ roles: &iam.ListRolesOutput{Roles: []iamtypes.Role{

+ {RoleName: aws.String("region-host"), Arn: aws.String("arn:aws:iam::1234:role/region-host")},

+ }},

+ users: &iam.ListUsersOutput{},

+ attached: &iam.ListAttachedRolePoliciesOutput{},

+ },

Tagging: &fakeTagging{out: &resourcegroupstaggingapi.GetResourcesOutput{

ResourceTagMappingList: []rgtypes.ResourceTagMapping{

{ResourceARN: aws.String("arn:aws:ec2:eu-west-1:1234:instance/i-123")},

{ResourceARN: aws.String("arn:aws:sqs:eu-west-1:1234:some-queue")},

+ {ResourceARN: aws.String("arn:aws:s3:::carlos-data-ie")},

+ {ResourceARN: aws.String("arn:aws:route53:::hostedzone/Z1")},

+ {ResourceARN: aws.String("arn:aws:iam::1234:role/region-host")},

}},

}

@@ -51,6 +82,18 @@ func TestCatchAllDedupesAgainstTypedResources(t *testing.T) {

if len(machines) != 1 {

t.Fatalf("machines = %d, want 1 (the catch-all must not duplicate a typed resource)", len(machines))

}

+ buckets := inv.ByKind(infraview.KindBucket)

+ if len(buckets) != 1 {

+ t.Fatalf("buckets = %d, want 1 (the catch-all must not duplicate the S3 mapping)", len(buckets))

+ }

+ zones := inv.ByKind(infraview.KindDNSZone)

+ if len(zones) != 1 {

+ t.Fatalf("zones = %d, want 1 (the catch-all must not duplicate the Route53 mapping)", len(zones))

+ }

+ identities := inv.ByKind(infraview.KindIdentity)

+ if len(identities) != 1 {

+ t.Fatalf("identities = %d, want 1 (the catch-all must not duplicate the IAM mapping)", len(identities))

+ }

others := inv.ByKind(infraview.KindOther)

if len(others) != 1 || others[0].NativeType != "sqs" {

t.Fatalf("others = %+v, want one sqs resource", others)

diff --git a/internal/awsinv/rest.go b/internal/awsinv/rest.go

index 70ccef1fe..2db750dbd 100644

--- a/internal/awsinv/rest.go

+++ b/internal/awsinv/rest.go

@@ -12,6 +12,7 @@ import (

"github.com/aws/aws-sdk-go-v2/service/iam"

"github.com/aws/aws-sdk-go-v2/service/resourcegroupstaggingapi"

"github.com/aws/aws-sdk-go-v2/service/route53"

+ r53types "github.com/aws/aws-sdk-go-v2/service/route53/types"

"github.com/aws/aws-sdk-go-v2/service/s3"

"github.com/carlosframework/platform/internal/infraview"

)

@@ -27,26 +28,29 @@ func s3Resources(ctx context.Context, api S3API) (out []infraview.Resource, part

if err != nil {

return nil, []string{fmt.Sprintf("buckets: %v", err)}

}

+ if o == nil {

+ return nil, partial

+ }

for _, b := range o.Buckets {

name := aws.ToString(b.Name)

r := res(infraview.KindBucket, "s3:bucket", name, name, "")

- if loc, err := api.GetBucketLocation(ctx, &s3.GetBucketLocationInput{Bucket: b.Name}); err == nil {

+ // Each of the four posture reads below is best-effort per bucket: a

+ // bucket whose versioning cannot be read still appears, with the

+ // attribute absent rather than a false "off". "off" is a claim.

+ if loc, err := api.GetBucketLocation(ctx, &s3.GetBucketLocationInput{Bucket: b.Name}); err == nil && loc != nil {

region := string(loc.LocationConstraint)

if region == "" {

region = "us-east-1" // the API's documented empty-means-us-east-1

}

r.Region = region

}

- // Each of the three posture reads is best-effort per bucket: a

- // bucket whose versioning cannot be read still appears, with the

- // attribute absent rather than a false "off". "off" is a claim.

- if v, err := api.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{Bucket: b.Name}); err == nil {

+ if v, err := api.GetBucketVersioning(ctx, &s3.GetBucketVersioningInput{Bucket: b.Name}); err == nil && v != nil {

set(&r, "versioning", strings.ToLower(string(v.Status)))

}

if _, err := api.GetBucketEncryption(ctx, &s3.GetBucketEncryptionInput{Bucket: b.Name}); err == nil {

set(&r, "encryption", "on")

}

- if p, err := api.GetPublicAccessBlock(ctx, &s3.GetPublicAccessBlockInput{Bucket: b.Name}); err == nil && p.PublicAccessBlockConfiguration != nil {

+ if p, err := api.GetPublicAccessBlock(ctx, &s3.GetPublicAccessBlockInput{Bucket: b.Name}); err == nil && p != nil && p.PublicAccessBlockConfiguration != nil {

c := p.PublicAccessBlockConfiguration

if aws.ToBool(c.BlockPublicAcls) && aws.ToBool(c.BlockPublicPolicy) {

set(&r, "public_access_block", "on")

@@ -64,6 +68,9 @@ func route53Resources(ctx context.Context, api Route53API) (out []infraview.Reso

if err != nil {

return nil, []string{fmt.Sprintf("hosted zones: %v", err)}

}

+ if zones == nil {

+ return nil, partial

+ }

for _, z := range zones.HostedZones {

id := aws.ToString(z.Id)

name := strings.TrimSuffix(aws.ToString(z.Name), ".")

@@ -72,15 +79,25 @@ func route53Resources(ctx context.Context, api Route53API) (out []infraview.Reso

set(&zr, "private", "yes")

}

count := 0

- var next *string

+ // Route53 needs the full StartRecordName+StartRecordType+

+ // StartRecordIdentifier triple to resume at the exact record; a

+ // name-only cursor restarts each page at the FIRST record carrying

+ // that name, which both double-counts records at the page boundary

+ // and, when a single name owns a whole page, never advances at all.

+ var nextName, nextIdentifier *string

+ var nextType r53types.RRType

for {

recs, err := api.ListResourceRecordSets(ctx, &route53.ListResourceRecordSetsInput{

- HostedZoneId: z.Id, StartRecordName: next,

+ HostedZoneId: z.Id, StartRecordName: nextName,

+ StartRecordType: nextType, StartRecordIdentifier: nextIdentifier,

})

if err != nil {

partial = append(partial, fmt.Sprintf("records in %s: %v", name, err))

break

}

+ if recs == nil {

+ break

+ }

for _, rec := range recs.ResourceRecordSets {

count++

rtype := string(rec.Type)

@@ -106,7 +123,19 @@ func route53Resources(ctx context.Context, api Route53API) (out []infraview.Reso

if !recs.IsTruncated {

break

}

- next = recs.NextRecordName

+ // A nil next-name, or one that (together with the type) is

+ // identical to what we just asked for, means the cursor did not

+ // advance. Trusting IsTruncated forever here would spin an

+ // unbounded loop against a live API on an hourly pass, so stop

+ // and name the shortfall rather than loop or restart the zone.

+ if recs.NextRecordName == nil ||

+ (aws.ToString(recs.NextRecordName) == aws.ToString(nextName) && recs.NextRecordType == nextType) {

+ partial = append(partial, fmt.Sprintf("records in %s: pagination cursor did not advance, stopped short", name))

+ break

+ }

+ nextName = recs.NextRecordName

+ nextType = recs.NextRecordType

+ nextIdentifier = recs.NextRecordIdentifier

}

set(&zr, "record_count", strconv.Itoa(count))

out = append(out, zr)

@@ -120,12 +149,12 @@ func route53Resources(ctx context.Context, api Route53API) (out []infraview.Reso

func iamResources(ctx context.Context, api IAMAPI) (out []infraview.Resource, partial []string) {

if o, err := api.ListRoles(ctx, &iam.ListRolesInput{}); err != nil {

partial = append(partial, fmt.Sprintf("IAM roles: %v", err))

- } else {

+ } else if o != nil {

for _, role := range o.Roles {

name := aws.ToString(role.RoleName)

r := res(infraview.KindIdentity, "iam:role", aws.ToString(role.Arn), name, "")

set(&r, "kind", "role")

- if pol, err := api.ListAttachedRolePolicies(ctx, &iam.ListAttachedRolePoliciesInput{RoleName: role.RoleName}); err == nil {

+ if pol, err := api.ListAttachedRolePolicies(ctx, &iam.ListAttachedRolePoliciesInput{RoleName: role.RoleName}); err == nil && pol != nil {

set(&r, "attached_policies", strconv.Itoa(len(pol.AttachedPolicies)))

}

out = append(out, r)

@@ -133,7 +162,7 @@ func iamResources(ctx context.Context, api IAMAPI) (out []infraview.Resource, pa

}

if o, err := api.ListUsers(ctx, &iam.ListUsersInput{}); err != nil {

partial = append(partial, fmt.Sprintf("IAM users: %v", err))

- } else {

+ } else if o != nil {

for _, u := range o.Users {

r := res(infraview.KindIdentity, "iam:user", aws.ToString(u.Arn), aws.ToString(u.UserName), "")

set(&r, "kind", "user")

@@ -148,6 +177,9 @@ func alarmResources(ctx context.Context, api CloudWatchAPI, region string) (out

if err != nil {

return nil, []string{fmt.Sprintf("alarms in %s: %v", region, err)}

}

+ if o == nil {

+ return nil, partial

+ }

for _, a := range o.MetricAlarms {

r := res(infraview.KindAlarm, "cloudwatch:alarm", aws.ToString(a.AlarmArn), aws.ToString(a.AlarmName), region)

set(&r, "state", string(a.StateValue))

@@ -167,6 +199,9 @@ func certResources(ctx context.Context, api ACMAPI, region string) (out []infrav

if err != nil {

return nil, []string{fmt.Sprintf("certificates in %s: %v", region, err)}

}

+ if o == nil {

+ return nil, partial

+ }

for _, c := range o.CertificateSummaryList {

r := res(infraview.KindCertificate, "acm:certificate", aws.ToString(c.CertificateArn), aws.ToString(c.DomainName), region)

set(&r, "domain", aws.ToString(c.DomainName))

@@ -191,6 +226,9 @@ func catchAll(ctx context.Context, api TaggingAPI, region string, seen map[strin

if err != nil {

return out, append(partial, fmt.Sprintf("resource inventory in %s: %v", region, err))

}

+ if o == nil {

+ return out, partial

+ }

for _, m := range o.ResourceTagMappingList {

arn := aws.ToString(m.ResourceARN)

service, id := parseARN(arn)

diff --git a/internal/awsinv/rest_test.go b/internal/awsinv/rest_test.go

index c257af99f..37445fb8d 100644

--- a/internal/awsinv/rest_test.go

+++ b/internal/awsinv/rest_test.go

@@ -158,6 +158,67 @@ func TestRoute53RecordsMaterialiseOnlyJoinable(t *testing.T) {

}

+// fakeRoute53Stuck always claims IsTruncated with the same next cursor, no

+// matter what StartRecordName/Type it was called with — simulating a

+// pagination cursor that never advances (a single record name owning a

+// whole page is the real-world case). route53Resources must stop rather

+// than spin forever against a live API on an hourly pass.

+type fakeRoute53Stuck struct {

+ zones *route53.ListHostedZonesOutput

+ calls int

+func (f *fakeRoute53Stuck) ListHostedZones(context.Context, *route53.ListHostedZonesInput, ...func(*route53.Options)) (*route53.ListHostedZonesOutput, error) {

+ return f.zones, nil

+func (f *fakeRoute53Stuck) ListResourceRecordSets(context.Context, *route53.ListResourceRecordSetsInput, ...func(*route53.Options)) (*route53.ListResourceRecordSetsOutput, error) {

+ f.calls++

+ return &route53.ListResourceRecordSetsOutput{

+ ResourceRecordSets: []r53types.ResourceRecordSet{

+ {

+ Name: aws.String("stuck.carlosframework.com."), Type: r53types.RRTypeA,

+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("52.0.0.9")}},

+ },

+ IsTruncated: true,

+ NextRecordName: aws.String("stuck.carlosframework.com."),

+ NextRecordType: r53types.RRTypeA,

+ }, nil

+func TestRoute53PaginationStopsWhenCursorDoesNotAdvance(t *testing.T) {

+ f := &fakeRoute53Stuck{

+ zones: &route53.ListHostedZonesOutput{HostedZones: []r53types.HostedZone{

+ {Id: aws.String("/hostedzone/Z2"), Name: aws.String("carlosframework.com.")},

+ }},

+ }

+ done := make(chan struct{})

+ var rs []infraview.Resource

+ var partial []string

+ go func() {

+ rs, partial = route53Resources(context.Background(), f)

+ close(done)

+ }()

+ select {

+ case <-done:

+ case <-time.After(2 * time.Second):

+ t.Fatal("route53Resources did not return — a non-advancing cursor spun forever")

+ }

+ if f.calls > 3 {

+ t.Fatalf("ListResourceRecordSets called %d times for a cursor that never advances, want it to stop quickly", f.calls)

+ }

+ if len(partial) == 0 {

+ t.Fatal("a stalled cursor must be named in partial, not silently truncate the zone")

+ }

+ zone := find(rs, infraview.KindDNSZone, "/hostedzone/Z2")

+ if zone == nil {

+ t.Fatal("the zone itself must still be returned even though pagination stalled")

+ }

type fakeIAM struct {

roles *iam.ListRolesOutput

users *iam.ListUsersOutput