From dbac61dabd4df8ddc554ea9ad600f2e8615fe3ef Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Fri, 16 Oct 2026 03:04:01 +0100 Subject: [PATCH] fix(aws/rds): refuse explicitly when the offering page cap hides uniqueness Since #300 the RDS offering lookup walks every page so a second distinct exact match fails loud, but the walk is still bounded by maxOfferingPages. An exact match seen before the cap was then reported only as a generic "pagination cap reached" error. Make that a documented refuse-only outcome: the cap stays at 5 pages (500 offerings, far above any realistic result for a request filtered on class, engine, multi-AZ, duration and payment option), and when the cap is hit after a match the error names the matched offering and states it cannot be proven unique, so nothing is purchased. Tests pin the refusal (match on the first and on the last allowed page with more pages remaining) and the boundary where the last allowed page ends the walk. Closes #303 Co-Authored-By: Claude Opus 5.5 --- providers/aws/services/rds/client.go | 29 ++++++++---- providers/aws/services/rds/client_test.go | 55 +++++++++++++++++++++++ 2 files changed, 76 insertions(+), 8 deletions(-) diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index c8887656..5a5f823c 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -296,6 +296,12 @@ func (c *Client) findReservationByID(ctx context.Context, reservationID string) // pages to walk before giving up. At MaxRecords=100 per page this caps the // search at 500 offerings. Exceeding the cap returns a diagnostic error instead // of timing out the Lambda budget (issue #688). +// +// The walk must reach the last page to prove an exact match is unique (issue +// #288), so hitting the cap is a refuse-only outcome even when a match was +// already found: the lookup errors and nothing is purchased (issue #303). The +// request filters on class, engine, multi-AZ, duration and payment option, so +// a result set anywhere near 500 rows is not expected. const maxOfferingPages = 5 // findOfferingID finds the appropriate RDS Reserved Instance offering ID. @@ -367,9 +373,10 @@ func (c *Client) fetchRDSOfferingPage(ctx context.Context, baseInput *rds.Descri return rdsOfferingPageResult{id: id, marker: nextMarker}, nil } -// paginateRDSOfferings walks DescribeReservedDBInstancesOfferings pages and returns -// the first matching offering ID. It caps at maxOfferingPages to prevent Lambda -// timeout exhaustion (issue #688). +// paginateRDSOfferings walks every DescribeReservedDBInstancesOfferings page and +// returns the single exact-match offering ID. It caps at maxOfferingPages to +// prevent Lambda timeout exhaustion (issue #688); reaching the cap errors even +// when a match was already seen (issue #303). func (c *Client) paginateRDSOfferings(ctx context.Context, rec common.Recommendation, details *common.DatabaseDetails, offeringType, execID string) (string, error) { multiAZ := details.AZConfig == "multi-az" normalizedEngine, err := c.lookupEngineName(details.Engine) @@ -414,7 +421,7 @@ func (c *Client) paginateRDSOfferings(ctx context.Context, rec common.Recommenda var found string pages := 0 for page := 1; ; page++ { - if err := offeringPageAllowed(ctx, page, desc); err != nil { + if err := offeringPageAllowed(ctx, page, desc, found); err != nil { return "", err } pr, err := c.fetchRDSOfferingPage(ctx, baseInput, marker, want, tag, page, t0) @@ -438,14 +445,20 @@ func (c *Client) paginateRDSOfferings(ctx context.Context, rec common.Recommenda } // offeringPageAllowed stops the page walk on cancellation or at the page cap. -func offeringPageAllowed(ctx context.Context, page int, desc string) error { +// found is the exact match seen so far, if any; at the cap it cannot be proven +// unique, so the walk refuses rather than return it (issue #303). +func offeringPageAllowed(ctx context.Context, page int, desc, found string) error { if err := ctx.Err(); err != nil { return err } - if page > maxOfferingPages { - return fmt.Errorf("pagination cap reached after %d pages for RDS %s (issue #688)", maxOfferingPages, desc) + if page <= maxOfferingPages { + return nil + } + if found != "" { + return fmt.Errorf("pagination cap reached after %d pages for RDS %s: offering %q matched but more pages remain, "+ + "so it cannot be proven unique; refusing to purchase (issue #303)", maxOfferingPages, desc, found) } - return nil + return fmt.Errorf("pagination cap reached after %d pages for RDS %s (issue #688)", maxOfferingPages, desc) } // pickOfferingID folds a newly found exact-match ID into the one found so far diff --git a/providers/aws/services/rds/client_test.go b/providers/aws/services/rds/client_test.go index 75a59ef7..e9aab1a7 100644 --- a/providers/aws/services/rds/client_test.go +++ b/providers/aws/services/rds/client_test.go @@ -1336,3 +1336,58 @@ func TestFindOfferingID_LicensedEnginesRefusedBeforeAPICall(t *testing.T) { }) } } + +// cappedOfferingPages returns maxOfferingPages pages, each with a next-page +// marker, so the walk always reaches the page cap. matchPage (1-based) carries +// an exact match; every other page is empty. +func cappedOfferingPages(mockRDS *MockRDSClient, matchPage int) { + for i := range maxOfferingPages { + out := &rds.DescribeReservedDBInstancesOfferingsOutput{ + ReservedDBInstancesOfferings: []types.ReservedDBInstancesOffering{}, + Marker: aws.String(fmt.Sprintf("tok-%d", i+1)), + } + if i+1 == matchPage { + out.ReservedDBInstancesOfferings = []types.ReservedDBInstancesOffering{exactOffering("offering-early")} + } + mockRDS.On("DescribeReservedDBInstancesOfferings", mock.Anything, mock.Anything).Return(out, nil).Once() + } +} + +// TestFindOfferingID_MatchBeforeCapRefusesWhenMorePagesRemain pins issue #303: +// an exact match seen before the page cap cannot be proven unique while more +// pages remain, so the lookup must refuse (never return the match) with an +// error that names the unverified match, and must not fetch past the cap. +func TestFindOfferingID_MatchBeforeCapRefusesWhenMorePagesRemain(t *testing.T) { + for _, matchPage := range []int{1, maxOfferingPages} { + t.Run(fmt.Sprintf("match on page %d", matchPage), func(t *testing.T) { + mockRDS := &MockRDSClient{} + t.Cleanup(func() { mockRDS.AssertExpectations(t) }) + client := &Client{client: mockRDS, region: "us-east-1"} + cappedOfferingPages(mockRDS, matchPage) + + id, err := client.findOfferingID(context.Background(), idempotencyTestRec(), "") + + assert.Empty(t, id) + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "pagination cap reached") + assert.Contains(t, err.Error(), `"offering-early"`) + assert.Contains(t, err.Error(), "cannot be proven unique") + assert.Contains(t, err.Error(), "refusing to purchase") + } + mockRDS.AssertNumberOfCalls(t, "DescribeReservedDBInstancesOfferings", maxOfferingPages) + }) + } +} + +// TestFindOfferingID_MatchOnLastAllowedPageSucceeds pins the cap boundary for +// issue #303: when the final page is the last one the cap allows (no further +// marker), the walk is complete and the unique match is returned. +func TestFindOfferingID_MatchOnLastAllowedPageSucceeds(t *testing.T) { + pages := make([][]types.ReservedDBInstancesOffering, maxOfferingPages) + pages[maxOfferingPages-1] = []types.ReservedDBInstancesOffering{exactOffering("offering-last")} + + id, err := findAcrossPages(t, pages...) + + assert.NoError(t, err) + assert.Equal(t, "offering-last", id) +}