amadan

carlos / platform Public

awsinv: fix Route53 identifier-blind stall check and cross-service dedup collision

Two new Important findings from round-2 review, both real:

- The pagination stall check compared only NextRecordName and
  NextRecordType, so a weighted/latency/geolocation/failover record set —
  which legitimately repeats the same name and type across a page boundary
  while only NextRecordIdentifier advances — was misclassified as stalled,
  truncating the record set after one page and appending a false 'cursor
  did not advance' line. The stall check now requires all three of name,
  type, and identifier to be unchanged.

- The bare-trailing-component seeding added to add() in the previous fix
  was a global cross-service dedup key: catchAll's parsed ids are always
  an ARN's bare tail, so seeding every typed resource's tail meant an IAM
  role or user named e.g. 'worker' silently swallowed an unrelated SQS
  queue, Lambda function, or SNS topic sharing that same tail — a common
  naming collision. Removed the global seeding; fixed the Route53 zone
  case that motivated it at the source instead, by using the bare
  hosted-zone id (stripping the '/hostedzone/' prefix AWS's own Id field
  carries) as the Resource's ID, so it matches parseARN's output directly
  with no cross-service rule. seen is keyed on the literal Resource.ID only
  again.

Regression tests: extended the Route53 pagination fake to a 3-page run
where the 2nd->3rd page boundary repeats name+type (the case a 2-page test
can't distinguish, since the 2nd page can coincidentally also be the last);
added TestCatchAllSurvivesTrailingNameCollision, verified to fail against
the removed global-seeding code. Re-verified
TestCatchAllDedupesAgainstTypedResources still fails when the
global-mappings-before-region-loop ordering from the previous fix is
reverted, so that regression stays load-bearing after this change.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Paul Campbell pushed by paul@keymail.dev ddb24ccc9dad1debef7b00fcae848a77117ad413 parent 13b3fe5
4 files changed, +181 −16
  • internal/awsinv/awsinv.go +11 −9
  • internal/awsinv/awsinv_test.go +39 −0
  • internal/awsinv/rest.go +21 −6
  • internal/awsinv/rest_test.go +110 −1
diff --git a/internal/awsinv/awsinv.go b/internal/awsinv/awsinv.go
index 1eaa3c2ed..cdb733f58 100644
--- a/internal/awsinv/awsinv.go
+++ b/internal/awsinv/awsinv.go
@@ -105,18 +105,20 @@ func (p *Provider) Inventory(ctx context.Context) (infraview.Inventory, error) {
note := func(lines []string) {
inv.Partial = append(inv.Partial, lines...)
}
+ // seen is keyed on the literal Resource.ID only. A global bare-trailing-
+ // component seeding was tried here and removed: catchAll's parsed id is
+ // always an ARN's bare tail, so seeding every typed resource's tail too
+ // meant an IAM role or user named "worker" would seed "worker" into
+ // seen and silently swallow an unrelated SQS queue, Lambda function, or
+ // SNS topic sharing that same tail — a common naming collision. Any
+ // mapping whose typed ID needs to equal the catch-all's parsed id (the
+ // Route53 zone case that motivated the tail-seeding in the first place)
+ // fixes it at the source instead: route53Resources now uses the bare
+ // hosted-zone id as the Resource's ID, so it matches parseARN's output
+ // directly with no cross-service seeding rule.
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)
}
}
diff --git a/internal/awsinv/awsinv_test.go b/internal/awsinv/awsinv_test.go
index a6bcc8fca..0a2f1847b 100644
--- a/internal/awsinv/awsinv_test.go
+++ b/internal/awsinv/awsinv_test.go
@@ -145,3 +145,42 @@ func TestPartialSurvivesWhenSomethingWasDiscovered(t *testing.T) {
t.Fatal("failures must be named in Partial")
}
}
+
+// A tagged resource that happens to share its trailing ARN component with
+// an unrelated typed resource's name must NOT be swallowed by dedup: only
+// a literal Resource.ID match may suppress a catch-all entry. Before this
+// fix, add() also seeded the bare trailing component of every resource's
+// ID into seen, so an IAM role named "worker" silently dropped an SQS
+// queue also named "worker" — a common naming collision, not a rare one.
+func TestCatchAllSurvivesTrailingNameCollision(t *testing.T) {
+ factory := func(region string) Clients {
+ return Clients{
+ IAM: &fakeIAM{
+ roles: &iam.ListRolesOutput{Roles: []iamtypes.Role{
+ {RoleName: aws.String("worker"), Arn: aws.String("arn:aws:iam::1234:role/worker")},
+ }},
+ users: &iam.ListUsersOutput{},
+ attached: &iam.ListAttachedRolePoliciesOutput{},
+ },
+ Tagging: &fakeTagging{out: &resourcegroupstaggingapi.GetResourcesOutput{
+ ResourceTagMappingList: []rgtypes.ResourceTagMapping{
+ {ResourceARN: aws.String("arn:aws:sqs:eu-west-1:1234:worker")},
+ },
+ }},
+ }
+ }
+ p := newWithFactory(factory, "eu-west-1", []string{"eu-west-1"}, "1234")
+
+ inv, err := p.Inventory(context.Background())
+ if err != nil {
+ t.Fatalf("Inventory: %v", err)
+ }
+ identities := inv.ByKind(infraview.KindIdentity)
+ if len(identities) != 1 {
+ t.Fatalf("identities = %d, want 1 (the IAM role)", len(identities))
+ }
+ others := inv.ByKind(infraview.KindOther)
+ if len(others) != 1 || others[0].NativeType != "sqs" {
+ t.Fatalf("others = %+v, want the SQS queue to survive sharing the trailing name %q with the IAM role", others, "worker")
+ }
+}
diff --git a/internal/awsinv/rest.go b/internal/awsinv/rest.go
index 2db750dbd..46073bfbf 100644
--- a/internal/awsinv/rest.go
+++ b/internal/awsinv/rest.go
@@ -72,7 +72,15 @@ func route53Resources(ctx context.Context, api Route53API) (out []infraview.Reso
return nil, partial
}
for _, z := range zones.HostedZones {
- id := aws.ToString(z.Id)
+ // AWS's own ID field carries an "/hostedzone/" prefix
+ // (aws.ToString(z.Id) == "/hostedzone/Z1"); the bare "Z1" is the
+ // canonical hosted-zone id used everywhere else (the console, the
+ // CLI, parseARN's tail), and nothing downstream depends on the
+ // prefixed form — the console joins records to their zone by NAME,
+ // not by id. Using the bare id here is what lets the catch-all's
+ // dedup, keyed on the literal Resource.ID, find this zone without
+ // a second, service-specific seeding rule.
+ id := strings.TrimPrefix(aws.ToString(z.Id), "/hostedzone/")
name := strings.TrimSuffix(aws.ToString(z.Name), ".")
zr := res(infraview.KindDNSZone, "route53:hosted-zone", id, name, "")
if z.Config != nil && z.Config.PrivateZone {
@@ -123,13 +131,20 @@ func route53Resources(ctx context.Context, api Route53API) (out []infraview.Reso
if !recs.IsTruncated {
break
}
- // A nil next-name, or one that (together with the type) is
+ // A nil next-name, or a next name+type+identifier triple
// 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.
+ // advance. All three must match to call it a stall: weighted,
+ // latency, geolocation and failover record sets legitimately
+ // repeat the same name and type across a page boundary while
+ // only the identifier moves, and treating that as a stall would
+ // truncate the record set. Trusting IsTruncated forever on a
+ // genuine non-advance 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) {
+ (aws.ToString(recs.NextRecordName) == aws.ToString(nextName) &&
+ recs.NextRecordType == nextType &&
+ aws.ToString(recs.NextRecordIdentifier) == aws.ToString(nextIdentifier)) {
partial = append(partial, fmt.Sprintf("records in %s: pagination cursor did not advance, stopped short", name))
break
}
diff --git a/internal/awsinv/rest_test.go b/internal/awsinv/rest_test.go
index 37445fb8d..0e11c59b6 100644
--- a/internal/awsinv/rest_test.go
+++ b/internal/awsinv/rest_test.go
@@ -3,6 +3,7 @@ package awsinv
import (
"context"
"errors"
+ "fmt"
"strings"
"testing"
"time"
@@ -213,12 +214,120 @@ func TestRoute53PaginationStopsWhenCursorDoesNotAdvance(t *testing.T) {
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")
+ zone := find(rs, infraview.KindDNSZone, "Z2")
if zone == nil {
t.Fatal("the zone itself must still be returned even though pagination stalled")
}
}
+// fakeRoute53AdvancingIdentifier simulates a weighted/latency/geolocation/
+// failover record set spanning THREE pages, where the second-to-third page
+// boundary repeats the SAME name and type and only NextRecordIdentifier
+// moves. Two pages alone can't distinguish the fix (the second page can
+// coincidentally end the response too), so this needs a run where the
+// name+type also repeats mid-stream, which is exactly what a stall check
+// blind to the identifier misreads as "the cursor didn't move".
+type fakeRoute53AdvancingIdentifier struct {
+ zones *route53.ListHostedZonesOutput
+ calls int
+}
+
+func (f *fakeRoute53AdvancingIdentifier) ListHostedZones(context.Context, *route53.ListHostedZonesInput, ...func(*route53.Options)) (*route53.ListHostedZonesOutput, error) {
+ return f.zones, nil
+}
+
+func (f *fakeRoute53AdvancingIdentifier) ListResourceRecordSets(_ context.Context, in *route53.ListResourceRecordSetsInput, _ ...func(*route53.Options)) (*route53.ListResourceRecordSetsOutput, error) {
+ f.calls++
+ const name = "weighted.carlosframework.com."
+ switch f.calls {
+ case 1:
+ return &route53.ListResourceRecordSetsOutput{
+ ResourceRecordSets: []r53types.ResourceRecordSet{
+ {
+ Name: aws.String(name), Type: r53types.RRTypeA,
+ SetIdentifier: aws.String("primary"),
+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("52.0.0.10")}},
+ },
+ },
+ IsTruncated: true,
+ NextRecordName: aws.String(name),
+ NextRecordType: r53types.RRTypeA,
+ NextRecordIdentifier: aws.String("secondary"),
+ }, nil
+ case 2:
+ if aws.ToString(in.StartRecordIdentifier) != "secondary" {
+ return nil, fmt.Errorf("want StartRecordIdentifier=secondary, got %q", aws.ToString(in.StartRecordIdentifier))
+ }
+ // SAME name and type as the cursor we were just handed — only the
+ // identifier is different. A stall check that ignores the
+ // identifier sees name+type match and stops here, one page short.
+ return &route53.ListResourceRecordSetsOutput{
+ ResourceRecordSets: []r53types.ResourceRecordSet{
+ {
+ Name: aws.String(name), Type: r53types.RRTypeA,
+ SetIdentifier: aws.String("secondary"),
+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("52.0.0.11")}},
+ },
+ },
+ IsTruncated: true,
+ NextRecordName: aws.String(name),
+ NextRecordType: r53types.RRTypeA,
+ NextRecordIdentifier: aws.String("tertiary"),
+ }, nil
+ case 3:
+ if aws.ToString(in.StartRecordIdentifier) != "tertiary" {
+ return nil, fmt.Errorf("want StartRecordIdentifier=tertiary, got %q", aws.ToString(in.StartRecordIdentifier))
+ }
+ return &route53.ListResourceRecordSetsOutput{
+ ResourceRecordSets: []r53types.ResourceRecordSet{
+ {
+ Name: aws.String(name), Type: r53types.RRTypeA,
+ SetIdentifier: aws.String("tertiary"),
+ ResourceRecords: []r53types.ResourceRecord{{Value: aws.String("52.0.0.12")}},
+ },
+ },
+ IsTruncated: false,
+ }, nil
+ default:
+ return nil, fmt.Errorf("unexpected extra call %d", f.calls)
+ }
+}
+
+// The identical-name-and-type case at a page boundary must keep paginating
+// (and then terminate normally) rather than being misclassified as a
+// stall — the name/type/identifier triple only counts as stalled when ALL
+// THREE are unchanged from the request just made.
+func TestRoute53PaginationAdvancesOnIdentifierAlone(t *testing.T) {
+ f := &fakeRoute53AdvancingIdentifier{
+ zones: &route53.ListHostedZonesOutput{HostedZones: []r53types.HostedZone{
+ {Id: aws.String("/hostedzone/Z3"), Name: aws.String("carlosframework.com.")},
+ }},
+ }
+ rs, partial := route53Resources(context.Background(), f)
+ if len(partial) != 0 {
+ t.Fatalf("partial = %v, want none (an identifier-only advance must not be treated as a stall)", partial)
+ }
+ if f.calls != 3 {
+ t.Fatalf("ListResourceRecordSets calls = %d, want 3 (all three pages fetched)", f.calls)
+ }
+ dnsRecords := 0
+ for _, r := range rs {
+ if r.Kind == infraview.KindDNSRecord {
+ dnsRecords++
+ }
+ }
+ if dnsRecords != 3 {
+ t.Fatalf("dns_record resources = %d, want 3 (one per weighted record set member across all three pages)", dnsRecords)
+ }
+ zone := find(rs, infraview.KindDNSZone, "Z3")
+ if zone == nil {
+ t.Fatal("no zone produced")
+ }
+ if zone.Attr("record_count") != "3" {
+ t.Errorf("record_count = %q, want 3", zone.Attr("record_count"))
+ }
+}
+
type fakeIAM struct {
roles *iam.ListRolesOutput
users *iam.ListUsersOutput