From 202f70f0d02cc3c22e2864efcc2c15244146615d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 02:56:55 -0700 Subject: [PATCH 1/3] fix(aws/savingsplans): paginate GetExistingCommitments via NextToken loop DescribeSavingsPlans paginates (NextToken on both input and output) but GetExistingCommitments was making a single un-paginated call, silently truncating the SP list to the first page. On accounts with more SPs than one page, CUDly under-reported existing commitments, causing duplicate- detection and expiry-coverage math to see lower coverage than reality and recommend or auto-purchase redundant SPs (financial overspend). Add the standard NextToken accumulator loop consistent with the MemoryDB and OpenSearch commitment fetches: ctx.Err() guard at the top of each iteration (cancellation is terminal, not accumulated), MaxResults=100 per page, and a maxCommitmentPages=100 page cap that logs a warning and returns partial results (data loss is worse than a hard error for the commitment listing path). Also add two regression tests: - two-page mock asserts all items are accumulated (fails pre-fix, passes post-fix) - cancellation test asserts ctx.Err() propagates verbatim on cancel Fix pre-existing build break: two TestLookupOfferingID_* tests called findOfferingID with 2 args after its signature grew to 3 args in the same branch; pass empty string as execID to match ValidateOffering. Closes #1019 --- providers/aws/services/savingsplans/client.go | 109 ++++++++++++------ .../aws/services/savingsplans/client_test.go | 102 +++++++++++++++- 2 files changed, 172 insertions(+), 39 deletions(-) diff --git a/providers/aws/services/savingsplans/client.go b/providers/aws/services/savingsplans/client.go index 355b0503a..6779bb208 100644 --- a/providers/aws/services/savingsplans/client.go +++ b/providers/aws/services/savingsplans/client.go @@ -104,21 +104,13 @@ func (c *Client) GetRecommendations(ctx context.Context, params common.Recommend return []common.Recommendation{}, nil } -// GetExistingCommitments retrieves existing Savings Plans +// GetExistingCommitments retrieves existing Savings Plans across all pages. +// DescribeSavingsPlans is paginated (NextToken on both input and output); the +// original single-call implementation silently truncated to the first page, +// causing CUDly to under-report existing SPs and recommend redundant purchases +// (issue #1019). The loop mirrors the NextToken accumulator used in the +// MemoryDB and OpenSearch commitment fetches. func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) { - input := &savingsplans.DescribeSavingsPlansInput{ - States: []types.SavingsPlanState{ - types.SavingsPlanStateActive, - types.SavingsPlanStatePendingReturn, - types.SavingsPlanStateQueued, - }, - } - - result, err := c.client.DescribeSavingsPlans(ctx, input) - if err != nil { - return nil, fmt.Errorf("failed to describe Savings Plans: %w", err) - } - // Each client is scoped to one plan type, so partition the API result by // SavingsPlanType and only return commitments matching this client's type. // The provider registers four SP services and calls GetExistingCommitments @@ -128,40 +120,75 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen // `case common.ServiceSavingsPlans` branch in provider.go's // GetServiceClient): in that mode, return every commitment unfiltered // to match pre-split behaviour. Per-plan-type clients still partition. - commitments := make([]common.Commitment, 0, len(result.SavingsPlans)) + commitments := make([]common.Commitment, 0) service := c.GetServiceType() + page := 0 + var nextToken *string - for _, sp := range result.SavingsPlans { - if sp.SavingsPlanId == nil { - continue + for { + if err := ctx.Err(); err != nil { + return nil, err } - if c.planType != "" && sp.SavingsPlanType != c.planType { - continue + + page++ + if page > maxCommitmentPages { + log.Printf("WARNING: savingsplans.GetExistingCommitments reached page cap (%d) — returning partial results (issue #1019)", + maxCommitmentPages) + break } - commitment := common.Commitment{ - Provider: common.ProviderAWS, - CommitmentID: *sp.SavingsPlanId, - CommitmentType: common.CommitmentSavingsPlan, - Service: service, - Region: aws.ToString(sp.Region), - ResourceType: string(sp.SavingsPlanType), - Count: 1, // Savings Plans don't have a count - State: string(sp.State), + input := &savingsplans.DescribeSavingsPlansInput{ + States: []types.SavingsPlanState{ + types.SavingsPlanStateActive, + types.SavingsPlanStatePendingReturn, + types.SavingsPlanStateQueued, + }, + NextToken: nextToken, + MaxResults: aws.Int32(100), } - if sp.Start != nil { - if startTime, err := time.Parse(time.RFC3339, *sp.Start); err == nil { - commitment.StartDate = startTime - } + result, err := c.client.DescribeSavingsPlans(ctx, input) + if err != nil { + return nil, fmt.Errorf("failed to describe Savings Plans: %w", err) } - if sp.End != nil { - if endTime, err := time.Parse(time.RFC3339, *sp.End); err == nil { - commitment.EndDate = endTime + + for _, sp := range result.SavingsPlans { + if sp.SavingsPlanId == nil { + continue + } + if c.planType != "" && sp.SavingsPlanType != c.planType { + continue + } + + commitment := common.Commitment{ + Provider: common.ProviderAWS, + CommitmentID: *sp.SavingsPlanId, + CommitmentType: common.CommitmentSavingsPlan, + Service: service, + Region: aws.ToString(sp.Region), + ResourceType: string(sp.SavingsPlanType), + Count: 1, // Savings Plans don't have a count + State: string(sp.State), + } + + if sp.Start != nil { + if startTime, err := time.Parse(time.RFC3339, *sp.Start); err == nil { + commitment.StartDate = startTime + } } + if sp.End != nil { + if endTime, err := time.Parse(time.RFC3339, *sp.End); err == nil { + commitment.EndDate = endTime + } + } + + commitments = append(commitments, commitment) } - commitments = append(commitments, commitment) + if result.NextToken == nil || aws.ToString(result.NextToken) == "" { + break + } + nextToken = result.NextToken } return commitments, nil @@ -327,6 +354,14 @@ func convertPaymentOption(paymentOption string) types.SavingsPlanPaymentOption { // instead of timing out the Lambda budget (issue #688). const maxOfferingPages = 5 +// maxCommitmentPages is the maximum number of DescribeSavingsPlans pages to +// walk when listing existing commitments. 100 pages * 100 items/page = 10,000 +// SPs, which is a realistic upper bound for even large enterprise accounts. +// Exceeding the cap logs a warning but still returns the commitments collected +// so far, consistent with the #691 guidance (data loss is worse than a +// truncation warning for the commitment path). +const maxCommitmentPages = 100 + // lookupOfferingID performs the API call(s) to find the offering ID. // DescribeSavingsPlansOfferings already accepts PlanTypes/Durations/PaymentOptions // filters that narrow the result set, so only a handful of results are expected diff --git a/providers/aws/services/savingsplans/client_test.go b/providers/aws/services/savingsplans/client_test.go index 813d753e8..ccb7eb31d 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -257,6 +257,104 @@ func TestClient_GetExistingCommitments_UmbrellaMode(t *testing.T) { mockClient.AssertExpectations(t) } +// TestClient_GetExistingCommitments_Pagination verifies that +// GetExistingCommitments drives DescribeSavingsPlans across all pages and +// accumulates every item. This is the regression test for issue #1019: the +// original single-call implementation silently dropped page 2+ commitments, +// causing CUDly to under-count existing SPs and recommend redundant purchases. +// +// The test fails on pre-fix code (only 1 item returned from page 1) and passes +// after the NextToken accumulator loop is in place (both items returned). +func TestClient_GetExistingCommitments_Pagination(t *testing.T) { + mockClient := &MockSavingsPlansClient{} + t.Cleanup(func() { mockClient.AssertExpectations(t) }) + + // Page 1: returns one SP and a NextToken signalling more pages. + mockClient.On("DescribeSavingsPlans", mock.Anything, mock.MatchedBy(func(input *savingsplans.DescribeSavingsPlansInput) bool { + return input.NextToken == nil + })).Return(&savingsplans.DescribeSavingsPlansOutput{ + SavingsPlans: []types.SavingsPlan{ + { + SavingsPlanId: aws.String("sp-page1"), + SavingsPlanType: types.SavingsPlanTypeCompute, + State: types.SavingsPlanStateActive, + Region: aws.String("us-east-1"), + }, + }, + NextToken: aws.String("token-page2"), + }, nil).Once() + + // Page 2: returns the second SP with no further NextToken. + mockClient.On("DescribeSavingsPlans", mock.Anything, mock.MatchedBy(func(input *savingsplans.DescribeSavingsPlansInput) bool { + return input.NextToken != nil && *input.NextToken == "token-page2" + })).Return(&savingsplans.DescribeSavingsPlansOutput{ + SavingsPlans: []types.SavingsPlan{ + { + SavingsPlanId: aws.String("sp-page2"), + SavingsPlanType: types.SavingsPlanTypeCompute, + State: types.SavingsPlanStateActive, + Region: aws.String("us-west-2"), + }, + }, + NextToken: nil, + }, nil).Once() + + client := &Client{ + client: mockClient, + region: "us-east-1", + planType: types.SavingsPlanTypeCompute, + } + + result, err := client.GetExistingCommitments(context.Background()) + require.NoError(t, err) + // Both pages must be accumulated: pre-fix returns 1, post-fix returns 2. + require.Len(t, result, 2, "GetExistingCommitments must paginate and accumulate all SPs") + ids := []string{result[0].CommitmentID, result[1].CommitmentID} + assert.Contains(t, ids, "sp-page1") + assert.Contains(t, ids, "sp-page2") +} + +// TestClient_GetExistingCommitments_CtxCancellation verifies that a cancelled +// context is treated as a hard stop in the pagination loop (not accumulated as +// lastErr while the loop continues). Per feedback_ctx_cancel_terminal: context +// cancellation is terminal in API fan-out loops. +func TestClient_GetExistingCommitments_CtxCancellation(t *testing.T) { + mockClient := &MockSavingsPlansClient{} + t.Cleanup(func() { mockClient.AssertExpectations(t) }) + + ctx, cancel := context.WithCancel(context.Background()) + + // First page succeeds and returns a NextToken; the cancel fires before + // the loop re-checks ctx.Err() at the top of the second iteration. + mockClient.On("DescribeSavingsPlans", mock.Anything, mock.MatchedBy(func(input *savingsplans.DescribeSavingsPlansInput) bool { + return input.NextToken == nil + })).Return(&savingsplans.DescribeSavingsPlansOutput{ + SavingsPlans: []types.SavingsPlan{ + { + SavingsPlanId: aws.String("sp-1"), + SavingsPlanType: types.SavingsPlanTypeCompute, + State: types.SavingsPlanStateActive, + }, + }, + NextToken: aws.String("token-page2"), + }, nil).Run(func(args mock.Arguments) { + // Cancel after the first page is returned so the loop's ctx.Err() + // check at the top of iteration 2 fires before any second API call. + cancel() + }).Once() + + client := &Client{ + client: mockClient, + region: "us-east-1", + planType: types.SavingsPlanTypeCompute, + } + + _, err := client.GetExistingCommitments(ctx) + require.Error(t, err) + assert.ErrorIs(t, err, context.Canceled, + "GetExistingCommitments must propagate ctx.Err() verbatim on cancellation") +} + // TestClient_findOfferingID_RejectsMismatchedPlanType pins the // per-plan-type isolation post-split: a client scoped to one plan type // must refuse to look up an offering for a different plan type, even if @@ -988,7 +1086,7 @@ func TestLookupOfferingID_PaginationCapFires(t *testing.T) { }, nil).Once() } - _, err := client.findOfferingID(context.Background(), spRec()) + _, err := client.findOfferingID(context.Background(), spRec(), "") if assert.Error(t, err) { assert.Contains(t, err.Error(), "pagination cap reached") @@ -1011,7 +1109,7 @@ func TestLookupOfferingID_HappyPath(t *testing.T) { }, }, nil).Once() - id, err := client.findOfferingID(context.Background(), spRec()) + id, err := client.findOfferingID(context.Background(), spRec(), "") assert.NoError(t, err) assert.Equal(t, "offering-ok", id) From 34cdbe3bd6848e01750b1978b57d0970b4cf467d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:00:58 -0700 Subject: [PATCH 2/3] refactor(aws/savingsplans): extract toCommitment to satisfy gocyclo The pagination loop added in this branch pushed GetExistingCommitments to cyclomatic complexity 15 (budget 10), failing the pre-commit gocyclo hook in CI. Pull the per-SavingsPlan conversion (nil-ID guard, plan-type filter, and the two RFC3339 time-parse branches) into a toCommitment helper that returns (Commitment, ok). This is a behavior-preserving extraction: the loop now skips entries when ok is false, identical to the previous continue branches. GetExistingCommitments drops back under the limit; the existing two-page and cancellation regression tests still pass unchanged. --- providers/aws/services/savingsplans/client.go | 69 +++++++++++-------- 1 file changed, 40 insertions(+), 29 deletions(-) diff --git a/providers/aws/services/savingsplans/client.go b/providers/aws/services/savingsplans/client.go index 6779bb208..2c94446af 100644 --- a/providers/aws/services/savingsplans/client.go +++ b/providers/aws/services/savingsplans/client.go @@ -153,36 +153,9 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen } for _, sp := range result.SavingsPlans { - if sp.SavingsPlanId == nil { - continue - } - if c.planType != "" && sp.SavingsPlanType != c.planType { - continue + if commitment, ok := c.toCommitment(sp, service); ok { + commitments = append(commitments, commitment) } - - commitment := common.Commitment{ - Provider: common.ProviderAWS, - CommitmentID: *sp.SavingsPlanId, - CommitmentType: common.CommitmentSavingsPlan, - Service: service, - Region: aws.ToString(sp.Region), - ResourceType: string(sp.SavingsPlanType), - Count: 1, // Savings Plans don't have a count - State: string(sp.State), - } - - if sp.Start != nil { - if startTime, err := time.Parse(time.RFC3339, *sp.Start); err == nil { - commitment.StartDate = startTime - } - } - if sp.End != nil { - if endTime, err := time.Parse(time.RFC3339, *sp.End); err == nil { - commitment.EndDate = endTime - } - } - - commitments = append(commitments, commitment) } if result.NextToken == nil || aws.ToString(result.NextToken) == "" { @@ -194,6 +167,44 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen return commitments, nil } +// toCommitment converts a single DescribeSavingsPlans entry into a +// common.Commitment, returning ok=false for entries that should be skipped +// (missing ID, or a plan type that does not match this client's scope). It is +// split out of GetExistingCommitments to keep that function under the +// cyclomatic limit. +func (c *Client) toCommitment(sp types.SavingsPlan, service common.ServiceType) (common.Commitment, bool) { + if sp.SavingsPlanId == nil { + return common.Commitment{}, false + } + if c.planType != "" && sp.SavingsPlanType != c.planType { + return common.Commitment{}, false + } + + commitment := common.Commitment{ + Provider: common.ProviderAWS, + CommitmentID: *sp.SavingsPlanId, + CommitmentType: common.CommitmentSavingsPlan, + Service: service, + Region: aws.ToString(sp.Region), + ResourceType: string(sp.SavingsPlanType), + Count: 1, // Savings Plans don't have a count + State: string(sp.State), + } + + if sp.Start != nil { + if startTime, err := time.Parse(time.RFC3339, *sp.Start); err == nil { + commitment.StartDate = startTime + } + } + if sp.End != nil { + if endTime, err := time.Parse(time.RFC3339, *sp.End); err == nil { + commitment.EndDate = endTime + } + } + + return commitment, true +} + // PurchaseCommitment purchases a Savings Plan func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ From 9d292dcf592a51af22341a85e2805c114933f483 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:21:09 -0700 Subject: [PATCH 3/3] fix(aws/savingsplans): add USD filter, EC2Instance region filter, and deterministic sort to findOfferingID DescribeSavingsPlansOfferings was called without currency or region constraints, so non-USD offerings and (for EC2Instance SPs) off-region offerings could appear in SearchResults. The first element was then returned without sorting, making the selected offering ID nondeterministic across API calls. - Set Currencies: [USD] on DescribeSavingsPlansOfferingsInput so non-USD currency variants are excluded server-side. - For EC2Instance SPs (the only region-scoped SP type), add a SavingsPlanOfferingFilterAttributeRegion filter element using the client's region. Compute, SageMaker, and Database SPs are global and receive no region filter. - In lookupOfferingID, accumulate all matching offering IDs across pages, sort lexicographically, then return the first so the result is stable regardless of AWS response ordering. Closes #1066. Finding 08-L1 (SP portion). --- providers/aws/services/savingsplans/client.go | 37 +++- .../aws/services/savingsplans/client_test.go | 161 ++++++++++++++++++ 2 files changed, 193 insertions(+), 5 deletions(-) diff --git a/providers/aws/services/savingsplans/client.go b/providers/aws/services/savingsplans/client.go index 2c94446af..c970a286e 100644 --- a/providers/aws/services/savingsplans/client.go +++ b/providers/aws/services/savingsplans/client.go @@ -5,6 +5,7 @@ import ( "context" "fmt" "log" + "sort" "time" "github.com/aws/aws-sdk-go-v2/aws" @@ -303,10 +304,27 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, log.Printf("purchase[%s]: SavingsPlans findOfferingID starting (planType=%s term=%s payment=%s)", tag, planType, rec.Term, rec.PaymentOption) + // Pin to USD so non-USD currency offerings are excluded server-side. + // EC2Instance SPs are region-scoped; add a region filter so only the + // offering for the client's region is returned. Compute, SageMaker, and + // Database SPs are global and do not carry a region property. input := &savingsplans.DescribeSavingsPlansOfferingsInput{ PlanTypes: []types.SavingsPlanType{planType}, Durations: []int64{termSeconds}, PaymentOptions: []types.SavingsPlanPaymentOption{paymentOption}, + Currencies: []types.CurrencyCode{types.CurrencyCodeUsd}, + } + if planType == types.SavingsPlanTypeEc2Instance { + if c.region == "" { + log.Printf("purchase[%s]: SavingsPlans findOfferingID: client region is empty; skipping region filter", tag) + } else { + input.Filters = []types.SavingsPlanOfferingFilterElement{ + { + Name: types.SavingsPlanOfferingFilterAttributeRegion, + Values: []string{c.region}, + }, + } + } } offeringID, err := c.lookupOfferingID(ctx, input) @@ -374,12 +392,15 @@ const maxOfferingPages = 5 const maxCommitmentPages = 100 // lookupOfferingID performs the API call(s) to find the offering ID. -// DescribeSavingsPlansOfferings already accepts PlanTypes/Durations/PaymentOptions -// filters that narrow the result set, so only a handful of results are expected -// on the first page. Pagination with a cap is added as a safety net. +// DescribeSavingsPlansOfferings accepts PlanTypes/Durations/PaymentOptions/ +// Currencies/Filters that narrow the result set to at most a handful of +// results. All pages are accumulated so the caller receives a stable, +// lexicographically-sorted first offering rather than a result that depends +// on AWS response ordering (finding 08-L1). func (c *Client) lookupOfferingID(ctx context.Context, input *savingsplans.DescribeSavingsPlansOfferingsInput) (string, error) { t0 := time.Now() page := 0 + var offeringIDs []string for { if err := ctx.Err(); err != nil { return "", err @@ -404,7 +425,7 @@ func (c *Client) lookupOfferingID(ctx context.Context, input *savingsplans.Descr if offering.OfferingId == nil { continue } - return *offering.OfferingId, nil + offeringIDs = append(offeringIDs, *offering.OfferingId) } if result.NextToken == nil || aws.ToString(result.NextToken) == "" { @@ -413,7 +434,13 @@ func (c *Client) lookupOfferingID(ctx context.Context, input *savingsplans.Descr input.NextToken = result.NextToken } - return "", fmt.Errorf("no Savings Plans offerings found after %d page(s) (issue #688)", page) + if len(offeringIDs) == 0 { + return "", fmt.Errorf("no Savings Plans offerings found after %d page(s) (issue #688)", page) + } + // Sort for a stable, deterministic tie-break when multiple offerings + // survive the server-side filters. The first ID after sorting is returned. + sort.Strings(offeringIDs) + return offeringIDs[0], nil } // ValidateOffering checks if a Savings Plans offering exists diff --git a/providers/aws/services/savingsplans/client_test.go b/providers/aws/services/savingsplans/client_test.go index ccb7eb31d..1f0eb6164 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -1114,3 +1114,164 @@ func TestLookupOfferingID_HappyPath(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "offering-ok", id) } + +// TestFindOfferingID_SendsUSDCurrencyFilter asserts that findOfferingID always +// includes a USD currency filter in the DescribeSavingsPlansOfferings request +// (finding 08-L1). Without the filter the API may return non-USD offerings +// and a blind [0] pick selects the wrong offering. +func TestFindOfferingID_SendsUSDCurrencyFilter(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{client: mockSP, region: "us-east-1", planType: types.SavingsPlanTypeCompute} + + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, + mock.MatchedBy(func(in *savingsplans.DescribeSavingsPlansOfferingsInput) bool { + return len(in.Currencies) == 1 && in.Currencies[0] == types.CurrencyCodeUsd + })). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + {OfferingId: aws.String("offering-usd")}, + }, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), spRec(), "") + require.NoError(t, err) + assert.Equal(t, "offering-usd", id) +} + +// TestFindOfferingID_EC2InstanceSendsRegionFilter asserts that an EC2Instance +// client adds a region filter element to the request (finding 08-L1). EC2 +// Instance SPs are region-scoped; picking without a region filter risks +// selecting an offering for the wrong region. +func TestFindOfferingID_EC2InstanceSendsRegionFilter(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{client: mockSP, region: "eu-west-1", planType: types.SavingsPlanTypeEc2Instance} + + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, + mock.MatchedBy(func(in *savingsplans.DescribeSavingsPlansOfferingsInput) bool { + for _, f := range in.Filters { + if f.Name == types.SavingsPlanOfferingFilterAttributeRegion && + len(f.Values) == 1 && f.Values[0] == "eu-west-1" { + return true + } + } + return false + })). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + {OfferingId: aws.String("offering-eu")}, + }, + }, nil).Once() + + rec := common.Recommendation{ + Service: common.ServiceSavingsPlansEC2Instance, + ResourceType: "EC2Instance", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.SavingsPlanDetails{ + PlanType: "EC2Instance", + HourlyCommitment: 1.0, + }, + } + id, err := client.findOfferingID(context.Background(), rec, "") + require.NoError(t, err) + assert.Equal(t, "offering-eu", id) +} + +// TestFindOfferingID_ComputeNoRegionFilter asserts that a Compute SP client +// does NOT attach a region filter (Compute SPs are global, not region-scoped). +func TestFindOfferingID_ComputeNoRegionFilter(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{client: mockSP, region: "us-east-1", planType: types.SavingsPlanTypeCompute} + + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, + mock.MatchedBy(func(in *savingsplans.DescribeSavingsPlansOfferingsInput) bool { + // Compute SPs must not carry any region filter. + for _, f := range in.Filters { + if f.Name == types.SavingsPlanOfferingFilterAttributeRegion { + return false + } + } + return true + })). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + {OfferingId: aws.String("offering-global")}, + }, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), spRec(), "") + require.NoError(t, err) + assert.Equal(t, "offering-global", id) +} + +// TestLookupOfferingID_DeterministicSort asserts that when multiple offerings +// survive the server-side filters, lookupOfferingID returns the lexicographically +// smallest offering ID rather than relying on unspecified AWS response ordering +// (finding 08-L1). Pre-fix code returned SearchResults[0] without sorting, +// making the result nondeterministic across API calls. +func TestLookupOfferingID_DeterministicSort(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{client: mockSP, region: "us-east-1", planType: types.SavingsPlanTypeCompute} + + // Return three offerings in reverse-alphabetical order. The fix must sort + // them so "offering-aaa" (the lexicographically smallest) is returned, not + // "offering-zzz" (the one AWS happened to put first). + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, mock.Anything). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + {OfferingId: aws.String("offering-zzz")}, + {OfferingId: aws.String("offering-mmm")}, + {OfferingId: aws.String("offering-aaa")}, + }, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), spRec(), "") + require.NoError(t, err) + // Pre-fix: would return "offering-zzz" (first in response). + // Post-fix: returns "offering-aaa" (lexicographically smallest). + assert.Equal(t, "offering-aaa", id, + "findOfferingID must sort results deterministically before selecting") +} + +// TestLookupOfferingID_DeterministicSortAcrossPages asserts that the +// deterministic sort operates across ALL accumulated pages, not just within a +// single page (finding 08-L1 multi-page variant). +func TestLookupOfferingID_DeterministicSortAcrossPages(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{client: mockSP, region: "us-east-1", planType: types.SavingsPlanTypeCompute} + + // Page 1 returns a "later" offering ID. + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, + mock.MatchedBy(func(in *savingsplans.DescribeSavingsPlansOfferingsInput) bool { + return in.NextToken == nil + })). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + {OfferingId: aws.String("offering-zzz")}, + }, + NextToken: aws.String("tok-2"), + }, nil).Once() + + // Page 2 returns an "earlier" offering ID. Without cross-page sort, the + // pre-fix code would have already returned "offering-zzz" from page 1. + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, + mock.MatchedBy(func(in *savingsplans.DescribeSavingsPlansOfferingsInput) bool { + return in.NextToken != nil && *in.NextToken == "tok-2" + })). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + {OfferingId: aws.String("offering-aaa")}, + }, + NextToken: nil, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), spRec(), "") + require.NoError(t, err) + assert.Equal(t, "offering-aaa", id, + "findOfferingID must sort across pages before selecting") +}