Skip to content

fix(aws/savingsplans): paginate GetExistingCommitments via NextToken loop #1039

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 3 commits into from
Jun 7, 2026
Merged

fix(aws/savingsplans): paginate GetExistingCommitments via NextToken loop #1039

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
202f70f
fix(aws/savingsplans): paginate GetExistingCommitments via NextToken …
cristim Jun 7, 2026
34cdbe3
refactor(aws/savingsplans): extract toCommitment to satisfy gocyclo
cristim Jun 7, 2026
9d292dc
fix(aws/savingsplans): add USD filter, EC2Instance region filter, and…
cristim Jun 7, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
157 changes: 115 additions & 42 deletions providers/aws/services/savingsplans/client.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (
"context"
"fmt"
"log"
"sort"
"time"

"github.com/aws/aws-sdk-go-v2/aws"
Expand Down Expand Up @@ -104,21 +105,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
Expand All @@ -128,45 +121,91 @@ 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 commitment, ok := c.toCommitment(sp, service); ok {
commitments = append(commitments, commitment)
}
}

commitments = append(commitments, commitment)
if result.NextToken == nil || aws.ToString(result.NextToken) == "" {
break
}
nextToken = result.NextToken
}

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{
Expand Down Expand Up @@ -265,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)
Expand Down Expand Up @@ -327,13 +383,24 @@ 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
// 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
Expand All @@ -358,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) == "" {
Expand All @@ -367,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
Expand Down
Loading
Loading