diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index 57973a0f..037053cd 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -300,6 +300,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. @@ -371,9 +377,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) @@ -418,7 +425,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) @@ -442,14 +449,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) +}