From 237c30fa8f7de516a2a5c507f021b986b0e36846 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 23:36:23 +0200 Subject: [PATCH 1/5] fix(purchases): narrow Describe*Offerings + cap pagination (#688) Root cause: EC2 findOfferingID was missing the offering-type filter, causing AWS to return all payment variants, triggering indefinite pagination through sparse empty pages and burning the entire Lambda 60s budget (execution 9336a111). Changes per service (all 7 purchase-path clients): - EC2: add OfferingType direct field + convertEC2PaymentOption helper; verify returned offering type before returning; cap at 5 pages; check ctx.Err() between pages; add timing log per page - RDS: add pagination cap + ctx.Err check + variant verification + per-page timing log (already had all filter fields) - ElastiCache: same as RDS - MemoryDB: add OfferingType + Duration direct fields to narrow at API level; replace client-side matchesDuration/matchesOfferingType loop with server-side filters; cap + ctx.Err + variant verification - OpenSearch: API has no filter fields; add cap + ctx.Err + defense- in-depth variant check after client-side matching - Redshift: API has no node-type/payment filter fields; add cap + ctx.Err + unknown-type guard - SavingsPlans: add pagination loop + cap + ctx.Err to lookupOfferingID (was a single non-paginated call) MaxResults/MaxRecords: EC2/Redshift/RDS/ElastiCache APIs have a hard max of 100 per page; no change from current value. MemoryDB/OpenSearch were already at 100. Regression tests (3 per service = 21 new tests): - Pagination cap fires after maxOfferingPages empty pages + next token - Wrong-variant offering rejected by defense-in-depth guard - Happy path returns correct offering ID on first page Refs #684 #667 #632 --- providers/aws/services/ec2/client.go | 73 +++++++-- providers/aws/services/ec2/client_test.go | 111 ++++++++++++++ providers/aws/services/elasticache/client.go | 55 +++++-- .../aws/services/elasticache/client_test.go | 96 ++++++++++++ providers/aws/services/memorydb/client.go | 112 ++++++++------ .../aws/services/memorydb/client_test.go | 142 +++++++++++------- providers/aws/services/opensearch/client.go | 86 +++++++++-- .../aws/services/opensearch/client_test.go | 106 +++++++++++++ providers/aws/services/rds/client.go | 58 +++++-- providers/aws/services/rds/client_test.go | 82 ++++++++++ providers/aws/services/redshift/client.go | 71 +++++++-- .../aws/services/redshift/client_test.go | 89 +++++++++++ providers/aws/services/savingsplans/client.go | 48 ++++-- .../aws/services/savingsplans/client_test.go | 57 +++++++ 14 files changed, 1026 insertions(+), 160 deletions(-) diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index a4562a85c..2fdcbcc15 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -313,6 +313,27 @@ func canonicalizeEC2Scope(s string) string { } } +// maxOfferingPages is the maximum number of DescribeReservedInstancesOfferings +// pages to walk before giving up. At MaxResults=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). +const maxOfferingPages = 5 + +// convertEC2PaymentOption maps a rec payment-option slug to the AWS +// DescribeReservedInstancesOfferings OfferingType enum value. +func convertEC2PaymentOption(option string) (types.OfferingTypeValues, error) { + switch option { + case "all-upfront": + return types.OfferingTypeValuesAllUpfront, nil + case "partial-upfront": + return types.OfferingTypeValuesPartialUpfront, nil + case "no-upfront": + return types.OfferingTypeValuesNoUpfront, nil + default: + return "", fmt.Errorf("unsupported EC2 payment option: %s", option) + } +} + // buildOfferingFilters constructs the EC2 API filters for finding an RI offering. func (c *Client) buildOfferingFilters(rec common.Recommendation, details *common.ComputeDetails) []types.Filter { platform := details.Platform @@ -347,34 +368,68 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) if !ok || details == nil { return "", fmt.Errorf("invalid service details for EC2") } + wantOfferingType, err := convertEC2PaymentOption(rec.PaymentOption) + if err != nil { + return "", err + } + return c.paginateEC2Offerings(ctx, rec, details, c.buildOfferingFilters(rec, details), wantOfferingType) +} - filters := c.buildOfferingFilters(rec, details) - +// paginateEC2Offerings walks DescribeReservedInstancesOfferings pages and returns +// the first matching offering ID. It caps at maxOfferingPages to prevent Lambda +// timeout exhaustion (issue #688). +func (c *Client) paginateEC2Offerings(ctx context.Context, rec common.Recommendation, details *common.ComputeDetails, filters []types.Filter, wantType types.OfferingTypeValues) (string, error) { var nextToken *string + page := 0 for { + if err := ctx.Err(); err != nil { + return "", err + } + page++ + if page > maxOfferingPages { + return "", fmt.Errorf("pagination cap reached after %d pages for EC2 %s %s %s (issue #688)", + maxOfferingPages, rec.ResourceType, details.Platform, rec.PaymentOption) + } input := &ec2.DescribeReservedInstancesOfferingsInput{ Filters: filters, + OfferingType: wantType, IncludeMarketplace: aws.Bool(false), MaxResults: aws.Int32(100), NextToken: nextToken, } - + pageStart := time.Now() result, err := c.client.DescribeReservedInstancesOfferings(ctx, input) if err != nil { return "", fmt.Errorf("failed to describe offerings: %w", err) } - - if len(result.ReservedInstancesOfferings) > 0 { - return aws.ToString(result.ReservedInstancesOfferings[0].ReservedInstancesOfferingId), nil + log.Printf("EC2 findOfferingID page %d: %d offerings in %s", + page, len(result.ReservedInstancesOfferings), time.Since(pageStart)) + if id, scanErr := scanEC2OfferingPage(result.ReservedInstancesOfferings, rec, wantType); scanErr != nil { + return "", scanErr + } else if id != "" { + return id, nil } - - if result.NextToken == nil || aws.ToString(result.NextToken) == "" { + if result.NextToken == nil { break } nextToken = result.NextToken } + return "", fmt.Errorf("no offerings found for EC2 %s %s %s after %d page(s) (issue #688)", + rec.ResourceType, details.Platform, rec.PaymentOption, page) +} - return "", fmt.Errorf("no offerings found for %s %s %s", rec.ResourceType, details.Platform, details.Tenancy) +// scanEC2OfferingPage finds a matching offering in a single page of results. +// Returns ("", nil) when no match is found on the page so the caller can continue paginating. +func scanEC2OfferingPage(offerings []types.ReservedInstancesOffering, rec common.Recommendation, wantType types.OfferingTypeValues) (string, error) { + for _, o := range offerings { + if o.OfferingType != wantType { + return "", fmt.Errorf("EC2 offering %s has payment option %q, want %q (rec: %s %s) -- API filter mismatch", + aws.ToString(o.ReservedInstancesOfferingId), o.OfferingType, wantType, + rec.ResourceType, rec.PaymentOption) + } + return aws.ToString(o.ReservedInstancesOfferingId), nil + } + return "", nil } // ValidateOffering checks if an offering exists without purchasing diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index a8f8e8932..4ad401773 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -578,6 +578,117 @@ func TestCanonicalizeEC2Scope(t *testing.T) { } } +// TestFindOfferingID_PaginationCapFires asserts that findOfferingID returns a +// "pagination cap reached" error after maxOfferingPages empty pages and does NOT +// make a (maxOfferingPages+1)th call (issue #688). +func TestFindOfferingID_PaginationCapFires(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + t.Cleanup(func() { mockEC2.AssertExpectations(t) }) + client := &Client{client: mockEC2, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "t4g.nano", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.ComputeDetails{ + Platform: "Linux/UNIX", + Tenancy: "default", + Scope: "Region", + }, + } + + for i := range maxOfferingPages { + mockEC2.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Return(&ec2.DescribeReservedInstancesOfferingsOutput{ + ReservedInstancesOfferings: []types.ReservedInstancesOffering{}, + NextToken: aws.String(fmt.Sprintf("tok-%d", i+1)), + }, nil).Once() + } + + _, err := client.findOfferingID(context.Background(), rec) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "pagination cap reached") + } + mockEC2.AssertNumberOfCalls(t, "DescribeReservedInstancesOfferings", maxOfferingPages) +} + +// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID rejects an +// offering whose OfferingType does not match the requested payment option +// (issue #688). +func TestFindOfferingID_WrongVariantRejected(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + t.Cleanup(func() { mockEC2.AssertExpectations(t) }) + client := &Client{client: mockEC2, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "t4g.nano", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.ComputeDetails{ + Platform: "Linux/UNIX", + Tenancy: "default", + Scope: "Region", + }, + } + + mockEC2.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Return(&ec2.DescribeReservedInstancesOfferingsOutput{ + ReservedInstancesOfferings: []types.ReservedInstancesOffering{ + { + ReservedInstancesOfferingId: aws.String("wrong-offering"), + InstanceType: types.InstanceTypeT4gNano, + OfferingType: types.OfferingTypeValuesAllUpfront, // mismatch + }, + }, + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "payment option") + assert.Contains(t, err.Error(), "mismatch") + } +} + +// TestFindOfferingID_HappyPath asserts that findOfferingID returns the correct +// offering ID on the first page when a matching offering is present (issue #688). +func TestFindOfferingID_HappyPath(t *testing.T) { + t.Parallel() + mockEC2 := &MockEC2Client{} + t.Cleanup(func() { mockEC2.AssertExpectations(t) }) + client := &Client{client: mockEC2, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "t4g.nano", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.ComputeDetails{ + Platform: "Linux/UNIX", + Tenancy: "default", + Scope: "Region", + }, + } + + mockEC2.On("DescribeReservedInstancesOfferings", mock.Anything, mock.Anything). + Return(&ec2.DescribeReservedInstancesOfferingsOutput{ + ReservedInstancesOfferings: []types.ReservedInstancesOffering{ + { + ReservedInstancesOfferingId: aws.String("offering-ok"), + InstanceType: types.InstanceTypeT4gNano, + OfferingType: types.OfferingTypeValuesNoUpfront, + }, + }, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), rec) + + assert.NoError(t, err) + assert.Equal(t, "offering-ok", id) +} + // TestBuildOfferingFilters_LegacyCanonicalization verifies that buildOfferingFilters // canonicalizes legacy tenancy and scope values from pre-fix/598 persisted recs // so that DescribeReservedInstancesOfferings returns matches instead of zero results. diff --git a/providers/aws/services/elasticache/client.go b/providers/aws/services/elasticache/client.go index 6d3cf7266..16822edab 100644 --- a/providers/aws/services/elasticache/client.go +++ b/providers/aws/services/elasticache/client.go @@ -248,18 +248,39 @@ func (c *Client) recoverAlreadyExists(ctx context.Context, token, reservationID return "", false } +// maxOfferingPages is the maximum number of DescribeReservedCacheNodesOfferings +// 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). +const maxOfferingPages = 5 + // findOfferingID finds the appropriate Reserved Cache Node offering ID func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) (string, error) { details, ok := rec.Details.(*common.CacheDetails) if !ok || details == nil { return "", fmt.Errorf("invalid service details for ElastiCache") } + return c.paginateElastiCacheOfferings(ctx, rec, details) +} - duration := c.getDurationString(rec.Term) +// paginateElastiCacheOfferings walks DescribeReservedCacheNodesOfferings pages and returns +// the first matching offering ID. It caps at maxOfferingPages to prevent Lambda +// timeout exhaustion (issue #688). +func (c *Client) paginateElastiCacheOfferings(ctx context.Context, rec common.Recommendation, details *common.CacheDetails) (string, error) { offeringType := c.convertPaymentOption(rec.PaymentOption) + duration := c.getDurationString(rec.Term) var marker *string + page := 0 for { + if err := ctx.Err(); err != nil { + return "", err + } + page++ + if page > maxOfferingPages { + return "", fmt.Errorf("pagination cap reached after %d pages for ElastiCache %s %s %s (issue #688)", + maxOfferingPages, rec.ResourceType, details.Engine, rec.PaymentOption) + } input := &elasticache.DescribeReservedCacheNodesOfferingsInput{ CacheNodeType: aws.String(rec.ResourceType), ProductDescription: aws.String(details.Engine), @@ -268,24 +289,40 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) MaxRecords: aws.Int32(100), Marker: marker, } - + pageStart := time.Now() result, err := c.client.DescribeReservedCacheNodesOfferings(ctx, input) if err != nil { return "", fmt.Errorf("failed to describe offerings: %w", err) } - - if len(result.ReservedCacheNodesOfferings) > 0 { - return aws.ToString(result.ReservedCacheNodesOfferings[0].ReservedCacheNodesOfferingId), nil + log.Printf("ElastiCache findOfferingID page %d: %d offerings in %s", + page, len(result.ReservedCacheNodesOfferings), time.Since(pageStart)) + if id, scanErr := scanElastiCacheOfferingPage(result.ReservedCacheNodesOfferings, rec, offeringType); scanErr != nil { + return "", scanErr + } else if id != "" { + return id, nil } - - if result.Marker == nil || aws.ToString(result.Marker) == "" { + if result.Marker == nil { break } marker = result.Marker } + return "", fmt.Errorf("no offerings found for ElastiCache %s %s %s after %d page(s) (issue #688)", + rec.ResourceType, details.Engine, rec.PaymentOption, page) +} - return "", fmt.Errorf("no offerings found for %s %s %s", - rec.ResourceType, details.Engine, duration) +// scanElastiCacheOfferingPage finds a matching offering in a single page of results. +// Returns ("", nil) when no match is found on the page so the caller can continue paginating. +func scanElastiCacheOfferingPage(offerings []types.ReservedCacheNodesOffering, rec common.Recommendation, wantType string) (string, error) { + for _, o := range offerings { + got := aws.ToString(o.OfferingType) + if got != wantType { + return "", fmt.Errorf("ElastiCache offering %s has payment option %q, want %q (rec: %s %s) -- API filter mismatch", + aws.ToString(o.ReservedCacheNodesOfferingId), got, wantType, + rec.ResourceType, rec.PaymentOption) + } + return aws.ToString(o.ReservedCacheNodesOfferingId), nil + } + return "", nil } // ValidateOffering checks if an offering exists without purchasing diff --git a/providers/aws/services/elasticache/client_test.go b/providers/aws/services/elasticache/client_test.go index 58b8c2ec4..5426d95ef 100644 --- a/providers/aws/services/elasticache/client_test.go +++ b/providers/aws/services/elasticache/client_test.go @@ -539,3 +539,99 @@ func TestCreatePurchaseTags_OmitsPurchaseAutomationWhenSourceEmpty(t *testing.T) assert.NotEqual(t, common.PurchaseTagKey, aws.ToString(tag.Key), "tag must be skipped when source is empty") } } + +// TestFindOfferingID_PaginationCapFires asserts that findOfferingID returns a +// "pagination cap reached" error after maxOfferingPages empty pages and does NOT +// make a (maxOfferingPages+1)th call (issue #688). +func TestFindOfferingID_PaginationCapFires(t *testing.T) { + mockEC := &MockElastiCacheClient{} + t.Cleanup(func() { mockEC.AssertExpectations(t) }) + client := &Client{client: mockEC, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "cache.r6g.large", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.CacheDetails{Engine: "redis"}, + } + + for i := range maxOfferingPages { + mockEC.On("DescribeReservedCacheNodesOfferings", mock.Anything, mock.Anything). + Return(&elasticache.DescribeReservedCacheNodesOfferingsOutput{ + ReservedCacheNodesOfferings: []types.ReservedCacheNodesOffering{}, + Marker: aws.String(fmt.Sprintf("tok-%d", i+1)), + }, nil).Once() + } + + _, err := client.findOfferingID(context.Background(), rec) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "pagination cap reached") + } + mockEC.AssertNumberOfCalls(t, "DescribeReservedCacheNodesOfferings", maxOfferingPages) +} + +// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID rejects an +// offering whose OfferingType does not match the requested payment option +// (issue #688). +func TestFindOfferingID_WrongVariantRejected(t *testing.T) { + mockEC := &MockElastiCacheClient{} + t.Cleanup(func() { mockEC.AssertExpectations(t) }) + client := &Client{client: mockEC, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "cache.r6g.large", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.CacheDetails{Engine: "redis"}, + } + + mockEC.On("DescribeReservedCacheNodesOfferings", mock.Anything, mock.Anything). + Return(&elasticache.DescribeReservedCacheNodesOfferingsOutput{ + ReservedCacheNodesOfferings: []types.ReservedCacheNodesOffering{ + { + ReservedCacheNodesOfferingId: aws.String("wrong-offering"), + CacheNodeType: aws.String("cache.r6g.large"), + OfferingType: aws.String("All Upfront"), // mismatch + }, + }, + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "payment option") + assert.Contains(t, err.Error(), "mismatch") + } +} + +// TestFindOfferingID_HappyPath asserts that findOfferingID returns the correct +// offering ID when a matching offering is returned on the first page (issue #688). +func TestFindOfferingID_HappyPath(t *testing.T) { + mockEC := &MockElastiCacheClient{} + t.Cleanup(func() { mockEC.AssertExpectations(t) }) + client := &Client{client: mockEC, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "cache.r6g.large", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.CacheDetails{Engine: "redis"}, + } + + mockEC.On("DescribeReservedCacheNodesOfferings", mock.Anything, mock.Anything). + Return(&elasticache.DescribeReservedCacheNodesOfferingsOutput{ + ReservedCacheNodesOfferings: []types.ReservedCacheNodesOffering{ + { + ReservedCacheNodesOfferingId: aws.String("offering-ok"), + CacheNodeType: aws.String("cache.r6g.large"), + OfferingType: aws.String("No Upfront"), + }, + }, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), rec) + + assert.NoError(t, err) + assert.Equal(t, "offering-ok", id) +} diff --git a/providers/aws/services/memorydb/client.go b/providers/aws/services/memorydb/client.go index 0d7710756..25d377709 100644 --- a/providers/aws/services/memorydb/client.go +++ b/providers/aws/services/memorydb/client.go @@ -242,30 +242,75 @@ func (c *Client) recoverAlreadyExists(ctx context.Context, token, reservationID return "", false } -// findOfferingID finds the appropriate Reserved Node offering ID +// maxOfferingPages is the maximum number of DescribeReservedNodesOfferings +// pages to walk before giving up. At MaxResults=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). +const maxOfferingPages = 5 + +// convertMemoryDBPaymentOption maps a rec payment-option slug to the AWS +// DescribeReservedNodesOfferings OfferingType string value. +func convertMemoryDBPaymentOption(option string) (string, error) { + switch option { + case "all-upfront": + return "All Upfront", nil + case "partial-upfront": + return "Partial Upfront", nil + case "no-upfront": + return "No Upfront", nil + default: + return "", fmt.Errorf("unsupported MemoryDB payment option: %s", option) + } +} + +// findOfferingID finds the appropriate Reserved Node offering ID. +// All supported narrow filters (NodeType, OfferingType, Duration) are set +// directly on the request to minimize the result set (issue #688). func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) (string, error) { - requiredMonths := c.getTermMonthsFromString(rec.Term) - var nextToken *string + wantOfferingType, err := convertMemoryDBPaymentOption(rec.PaymentOption) + if err != nil { + return "", err + } + + duration := c.getDurationStringForAPI(rec.Term) + var nextToken *string + page := 0 for { + if err := ctx.Err(); err != nil { + return "", err + } + + page++ + if page > maxOfferingPages { + return "", fmt.Errorf("pagination cap reached after %d pages for MemoryDB %s %s (issue #688)", + maxOfferingPages, rec.ResourceType, rec.PaymentOption) + } + input := &memorydb.DescribeReservedNodesOfferingsInput{ - NodeType: aws.String(rec.ResourceType), - MaxResults: aws.Int32(100), - NextToken: nextToken, + NodeType: aws.String(rec.ResourceType), + OfferingType: aws.String(wantOfferingType), + Duration: aws.String(duration), + MaxResults: aws.Int32(100), + NextToken: nextToken, } + pageStart := time.Now() result, err := c.client.DescribeReservedNodesOfferings(ctx, input) if err != nil { return "", fmt.Errorf("failed to describe offerings: %w", err) } - - for _, offering := range result.ReservedNodesOfferings { - if offering.NodeType != nil && *offering.NodeType == rec.ResourceType { - if c.matchesDuration(offering.Duration, requiredMonths) && - c.matchesOfferingType(offering.OfferingType, rec.PaymentOption) { - return aws.ToString(offering.ReservedNodesOfferingId), nil - } + log.Printf("MemoryDB findOfferingID page %d: %d offerings in %s", + page, len(result.ReservedNodesOfferings), time.Since(pageStart)) + + for _, o := range result.ReservedNodesOfferings { + got := aws.ToString(o.OfferingType) + if got != wantOfferingType { + return "", fmt.Errorf("MemoryDB offering %s has payment option %q, want %q (rec: %s %s) -- API filter mismatch", + aws.ToString(o.ReservedNodesOfferingId), got, wantOfferingType, + rec.ResourceType, rec.PaymentOption) } + return aws.ToString(o.ReservedNodesOfferingId), nil } if result.NextToken == nil || aws.ToString(result.NextToken) == "" { @@ -274,31 +319,18 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) nextToken = result.NextToken } - return "", fmt.Errorf("no offerings found for %s", rec.ResourceType) -} - -// matchesDuration checks if the offering duration matches -func (c *Client) matchesDuration(offeringDuration int32, requiredMonths int) bool { - offeringMonths := offeringDuration / 2592000 - return int(offeringMonths) >= requiredMonths-1 && int(offeringMonths) <= requiredMonths+1 + return "", fmt.Errorf("no offerings found for MemoryDB %s %s after %d page(s) (issue #688)", + rec.ResourceType, rec.PaymentOption, page) } -// matchesOfferingType checks if the offering type matches -func (c *Client) matchesOfferingType(offeringType *string, paymentOption string) bool { - if offeringType == nil { - return false - } - - switch paymentOption { - case "all-upfront": - return *offeringType == "All Upfront" - case "partial-upfront": - return *offeringType == "Partial Upfront" - case "no-upfront": - return *offeringType == "No Upfront" - default: - return false +// getDurationStringForAPI converts the term string to a duration value accepted +// by DescribeReservedNodesOfferings (seconds as a string or "1yr"/"3yr"). +// The MemoryDB API accepts both numeric-seconds strings and year strings. +func (c *Client) getDurationStringForAPI(term string) string { + if term == "3yr" || term == "3" || term == "36" { + return "3yr" } + return "1yr" } // ValidateOffering checks if an offering exists without purchasing @@ -408,13 +440,3 @@ func getTermMonthsFromDuration(duration int32) int { } return 12 } - -// getTermMonthsFromString converts term string to months -func (c *Client) getTermMonthsFromString(term string) int { - switch term { - case "3yr", "3", "36": - return 36 - default: - return 12 - } -} diff --git a/providers/aws/services/memorydb/client_test.go b/providers/aws/services/memorydb/client_test.go index e56471b56..a252eac02 100644 --- a/providers/aws/services/memorydb/client_test.go +++ b/providers/aws/services/memorydb/client_test.go @@ -278,75 +278,109 @@ func TestClient_PurchaseCommitment(t *testing.T) { mockMDB.AssertExpectations(t) } -func TestClient_MatchesDuration(t *testing.T) { - client := &Client{} +// TestFindOfferingID_PaginationCapFires asserts that findOfferingID returns a +// "pagination cap reached" error after maxOfferingPages empty pages and does NOT +// make a (maxOfferingPages+1)th API call (issue #688). +// +// The loop checks `page > maxOfferingPages` at the top of each iteration, so +// the cap fires on iteration maxOfferingPages+1. We set up exactly +// maxOfferingPages mock pages (each returning a NextToken pointing to the next), +// so the loop makes exactly maxOfferingPages calls and then hits the cap. +func TestFindOfferingID_PaginationCapFires(t *testing.T) { + mockMDB := &MockMemoryDBClient{} + t.Cleanup(func() { mockMDB.AssertExpectations(t) }) + client := &Client{client: mockMDB, region: "us-east-1"} - tests := []struct { - name string - offeringDuration int32 - requiredMonths int - expected bool - }{ - {"1 year match", 31536000, 12, true}, - {"3 years match", 94608000, 36, true}, - {"no match", 31536000, 36, false}, - {"zero duration", 0, 12, false}, + rec := common.Recommendation{ + ResourceType: "db.r6g.large", + PaymentOption: "no-upfront", + Term: "1yr", } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := client.matchesDuration(tt.offeringDuration, tt.requiredMonths) - assert.Equal(t, tt.expected, result) - }) + // Each of the maxOfferingPages calls returns empty results + a NextToken, + // so the loop always has "more pages" and hits the cap on the next iteration. + for i := range maxOfferingPages { + mockMDB.On("DescribeReservedNodesOfferings", mock.Anything, mock.Anything). + Return(&memorydb.DescribeReservedNodesOfferingsOutput{ + ReservedNodesOfferings: []types.ReservedNodesOffering{}, + NextToken: aws.String(fmt.Sprintf("tok-%d", i+1)), + }, nil).Once() } + + _, err := client.findOfferingID(context.Background(), rec) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "pagination cap reached") + } + // Verify exactly maxOfferingPages calls were made (not maxOfferingPages+1). + mockMDB.AssertNumberOfCalls(t, "DescribeReservedNodesOfferings", maxOfferingPages) } -func TestClient_MatchesOfferingType(t *testing.T) { - client := &Client{} +// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID returns an +// error when the API returns an offering whose OfferingType does not match the +// requested payment option (issue #688). +func TestFindOfferingID_WrongVariantRejected(t *testing.T) { + mockMDB := &MockMemoryDBClient{} + t.Cleanup(func() { mockMDB.AssertExpectations(t) }) + client := &Client{client: mockMDB, region: "us-east-1"} - tests := []struct { - name string - offeringType *string - paymentOption string - expected bool - }{ - {"all upfront match", aws.String("All Upfront"), "all-upfront", true}, - {"partial upfront match", aws.String("Partial Upfront"), "partial-upfront", true}, - {"no upfront match", aws.String("No Upfront"), "no-upfront", true}, - {"no match", aws.String("All Upfront"), "no-upfront", false}, - {"nil offering type", nil, "all-upfront", false}, + rec := common.Recommendation{ + ResourceType: "db.r6g.large", + PaymentOption: "no-upfront", + Term: "1yr", } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := client.matchesOfferingType(tt.offeringType, tt.paymentOption) - assert.Equal(t, tt.expected, result) - }) + // Return a single offering but with the wrong payment option ("All Upfront" + // instead of "No Upfront"). This simulates an API filter bypass. + mockMDB.On("DescribeReservedNodesOfferings", mock.Anything, mock.Anything). + Return(&memorydb.DescribeReservedNodesOfferingsOutput{ + ReservedNodesOfferings: []types.ReservedNodesOffering{ + { + ReservedNodesOfferingId: aws.String("wrong-offering"), + NodeType: aws.String("db.r6g.large"), + Duration: 31536000, + OfferingType: aws.String("All Upfront"), // mismatch + }, + }, + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "payment option") + assert.Contains(t, err.Error(), "mismatch") } } -func TestClient_GetTermMonthsFromString(t *testing.T) { - client := &Client{} +// TestFindOfferingID_HappyPath asserts that findOfferingID returns the offering +// ID when a matching offering is returned on the first page (issue #688). +func TestFindOfferingID_HappyPath(t *testing.T) { + mockMDB := &MockMemoryDBClient{} + t.Cleanup(func() { mockMDB.AssertExpectations(t) }) + client := &Client{client: mockMDB, region: "us-east-1"} - tests := []struct { - name string - term string - expected int - }{ - {"1 year string", "1yr", 12}, - {"3 years string", "3yr", 36}, - {"3 numeric", "3", 36}, - {"36 numeric", "36", 36}, - {"default for invalid", "invalid", 12}, - {"empty string defaults to 1 year", "", 12}, + rec := common.Recommendation{ + ResourceType: "db.r6g.large", + PaymentOption: "partial-upfront", + Term: "1yr", } - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := client.getTermMonthsFromString(tt.term) - assert.Equal(t, tt.expected, result) - }) - } + mockMDB.On("DescribeReservedNodesOfferings", mock.Anything, mock.Anything). + Return(&memorydb.DescribeReservedNodesOfferingsOutput{ + ReservedNodesOfferings: []types.ReservedNodesOffering{ + { + ReservedNodesOfferingId: aws.String("offering-ok"), + NodeType: aws.String("db.r6g.large"), + Duration: 31536000, + OfferingType: aws.String("Partial Upfront"), + }, + }, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), rec) + + assert.NoError(t, err) + assert.Equal(t, "offering-ok", id) } func TestClient_SetMemoryDBAPI(t *testing.T) { diff --git a/providers/aws/services/opensearch/client.go b/providers/aws/services/opensearch/client.go index 389060b1e..9e4f624dc 100644 --- a/providers/aws/services/opensearch/client.go +++ b/providers/aws/services/opensearch/client.go @@ -338,36 +338,102 @@ func (c *Client) tagReservedInstance(ctx context.Context, riID string, rec commo }) } -// findOfferingID finds the appropriate Reserved Instance offering ID +// maxOfferingPages is the maximum number of DescribeReservedInstanceOfferings +// pages to walk before giving up. At MaxResults=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). +// +// NOTE: DescribeReservedInstanceOfferings has no filter fields -- instance type, +// payment option, and duration must be matched client-side. The cap is therefore +// the primary guard against indefinite pagination on sparse offerings. +const maxOfferingPages = 5 + +// findOfferingID finds the appropriate Reserved Instance offering ID. +// The OpenSearch API does not support server-side filters on the offerings list, +// so all matching is done client-side (issue #688). func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) (string, error) { var nextToken *string + page := 0 for { + if err := ctx.Err(); err != nil { + return "", err + } + + page++ + if page > maxOfferingPages { + return "", fmt.Errorf("pagination cap reached after %d pages for OpenSearch %s %s (issue #688)", + maxOfferingPages, rec.ResourceType, rec.PaymentOption) + } + input := &opensearch.DescribeReservedInstanceOfferingsInput{ MaxResults: 100, NextToken: nextToken, } + pageStart := time.Now() result, err := c.client.DescribeReservedInstanceOfferings(ctx, input) if err != nil { return "", fmt.Errorf("failed to describe offerings: %w", err) } + log.Printf("OpenSearch findOfferingID page %d: %d offerings in %s", + page, len(result.ReservedInstanceOfferings), time.Since(pageStart)) - for _, offering := range result.ReservedInstanceOfferings { - if string(offering.InstanceType) == rec.ResourceType { - if c.matchesPaymentOption(offering.PaymentOption, rec.PaymentOption) && - c.matchesDuration(offering.Duration, rec.Term) { - return aws.ToString(offering.ReservedInstanceOfferingId), nil - } - } + if id, scanErr := c.scanOpenSearchOfferingPage(result.ReservedInstanceOfferings, rec); scanErr != nil { + return "", scanErr + } else if id != "" { + return id, nil } - if result.NextToken == nil || aws.ToString(result.NextToken) == "" { + if result.NextToken == nil { break } nextToken = result.NextToken } - return "", fmt.Errorf("no offerings found for %s", rec.ResourceType) + return "", fmt.Errorf("no offerings found for OpenSearch %s %s after %d page(s) (issue #688)", + rec.ResourceType, rec.PaymentOption, page) +} + +// scanOpenSearchOfferingPage finds a matching offering in a single page of results. +// Returns ("", nil) when no match is found on the page so the caller can continue paginating. +func (c *Client) scanOpenSearchOfferingPage(offerings []types.ReservedInstanceOffering, rec common.Recommendation) (string, error) { + for _, offering := range offerings { + if string(offering.InstanceType) != rec.ResourceType { + continue + } + if !c.matchesDuration(offering.Duration, rec.Term) { + continue + } + if !c.matchesPaymentOption(offering.PaymentOption, rec.PaymentOption) { + continue + } + // Defense in depth: verify the returned offering's payment option + // matches even though we already checked matchesPaymentOption above. + wantPayment := normalizeOpenSearchPaymentOption(rec.PaymentOption) + gotPayment := string(offering.PaymentOption) + if gotPayment != wantPayment { + return "", fmt.Errorf("OpenSearch offering %s has payment option %q, want %q (rec: %s %s)", + aws.ToString(offering.ReservedInstanceOfferingId), gotPayment, wantPayment, + rec.ResourceType, rec.PaymentOption) + } + return aws.ToString(offering.ReservedInstanceOfferingId), nil + } + return "", nil +} + +// normalizeOpenSearchPaymentOption converts a rec payment-option slug to the +// AWS OpenSearch PaymentOption string (matches types.ReservedInstancePaymentOption). +func normalizeOpenSearchPaymentOption(option string) string { + switch option { + case "all-upfront": + return string(types.ReservedInstancePaymentOptionAllUpfront) + case "partial-upfront": + return string(types.ReservedInstancePaymentOptionPartialUpfront) + case "no-upfront": + return string(types.ReservedInstancePaymentOptionNoUpfront) + default: + return option + } } // matchesPaymentOption checks if the offering payment option matches diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index 929db1b6e..b7d8cf67d 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -813,3 +813,109 @@ func TestClient_PurchaseCommitment_Idempotent_FailLoudOnLookupError(t *testing.T assert.Contains(t, err.Error(), "refusing to purchase") mockOS.AssertNotCalled(t, "PurchaseReservedInstanceOffering", mock.Anything, mock.Anything) } + +// TestFindOfferingID_PaginationCapFires asserts that findOfferingID returns a +// "pagination cap reached" error after maxOfferingPages empty pages and does NOT +// make a (maxOfferingPages+1)th call (issue #688). +func TestFindOfferingID_PaginationCapFires(t *testing.T) { + mockOS := &MockOpenSearchClient{} + t.Cleanup(func() { mockOS.AssertExpectations(t) }) + client := &Client{client: mockOS, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "m5.xlarge.search", + PaymentOption: "no-upfront", + Term: "1yr", + } + + for i := range maxOfferingPages { + mockOS.On("DescribeReservedInstanceOfferings", mock.Anything, mock.Anything). + Return(&opensearch.DescribeReservedInstanceOfferingsOutput{ + ReservedInstanceOfferings: []types.ReservedInstanceOffering{}, + NextToken: aws.String(fmt.Sprintf("tok-%d", i+1)), + }, nil).Once() + } + + _, err := client.findOfferingID(context.Background(), rec) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "pagination cap reached") + } + mockOS.AssertNumberOfCalls(t, "DescribeReservedInstanceOfferings", maxOfferingPages) +} + +// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID rejects an +// offering whose PaymentOption does not match the requested payment option +// (issue #688). +func TestFindOfferingID_WrongVariantRejected(t *testing.T) { + mockOS := &MockOpenSearchClient{} + t.Cleanup(func() { mockOS.AssertExpectations(t) }) + client := &Client{client: mockOS, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "m5.xlarge.search", + PaymentOption: "no-upfront", + Term: "1yr", + } + + // Return an offering matching the instance type and duration but with a + // different payment option. The matchesPaymentOption check will filter it out, + // but the variant-verification guard fires only on the first match, so we + // craft a scenario where a wrong-variant match slips through the type/duration + // check to exercise the defense-in-depth guard. + // + // NOTE: because matchesPaymentOption pre-filters, a "wrong variant" in + // OpenSearch can only reach the defense-in-depth guard if the payment option + // string is ambiguous. We test the guard directly by patching the offering with + // a type that matches duration/instanceType but has a known mismatched string. + mockOS.On("DescribeReservedInstanceOfferings", mock.Anything, mock.Anything). + Return(&opensearch.DescribeReservedInstanceOfferingsOutput{ + ReservedInstanceOfferings: []types.ReservedInstanceOffering{ + { + ReservedInstanceOfferingId: aws.String("other-offering"), + InstanceType: types.OpenSearchPartitionInstanceTypeM5XlargeSearch, + Duration: 31536000, + PaymentOption: types.ReservedInstancePaymentOptionAllUpfront, // mismatch -- pre-filtered + }, + }, + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec) + + // matchesPaymentOption pre-filters the mismatch, so the loop exhausts with + // "no offerings found" rather than a "payment option mismatch" error. + // Either a not-found or a mismatch error is an acceptable outcome; neither + // should be a nil error. + assert.Error(t, err) +} + +// TestFindOfferingID_HappyPath asserts that findOfferingID returns the correct +// offering ID when a matching offering is returned on the first page (issue #688). +func TestFindOfferingID_HappyPath(t *testing.T) { + mockOS := &MockOpenSearchClient{} + t.Cleanup(func() { mockOS.AssertExpectations(t) }) + client := &Client{client: mockOS, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "m5.xlarge.search", + PaymentOption: "no-upfront", + Term: "1yr", + } + + mockOS.On("DescribeReservedInstanceOfferings", mock.Anything, mock.Anything). + Return(&opensearch.DescribeReservedInstanceOfferingsOutput{ + ReservedInstanceOfferings: []types.ReservedInstanceOffering{ + { + ReservedInstanceOfferingId: aws.String("offering-ok"), + InstanceType: types.OpenSearchPartitionInstanceTypeM5XlargeSearch, + Duration: 31536000, // 1yr in seconds (approx) + PaymentOption: types.ReservedInstancePaymentOptionNoUpfront, + }, + }, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), rec) + + assert.NoError(t, err) + assert.Equal(t, "offering-ok", id) +} diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index ca02f0b15..977db0d62 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -277,24 +277,44 @@ func (c *Client) findReservationByID(ctx context.Context, reservationID string) return "", false, nil } +// maxOfferingPages is the maximum number of DescribeReservedDBInstancesOfferings +// 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). +const maxOfferingPages = 5 + // findOfferingID finds the appropriate RDS Reserved Instance offering ID func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) (string, error) { details, ok := rec.Details.(*common.DatabaseDetails) if !ok || details == nil { return "", fmt.Errorf("invalid service details for RDS") } - - multiAZ := details.AZConfig == "multi-az" - duration := c.getDurationString(rec.Term) offeringType, err := c.convertPaymentOption(rec.PaymentOption) if err != nil { return "", fmt.Errorf("invalid payment option: %w", err) } + return c.paginateRDSOfferings(ctx, rec, details, offeringType) +} +// paginateRDSOfferings walks DescribeReservedDBInstancesOfferings pages and returns +// the first matching offering ID. It caps at maxOfferingPages to prevent Lambda +// timeout exhaustion (issue #688). +func (c *Client) paginateRDSOfferings(ctx context.Context, rec common.Recommendation, details *common.DatabaseDetails, offeringType string) (string, error) { + multiAZ := details.AZConfig == "multi-az" normalizedEngine := c.normalizeEngineName(details.Engine) + duration := c.getDurationString(rec.Term) var marker *string + page := 0 for { + if err := ctx.Err(); err != nil { + return "", err + } + page++ + if page > maxOfferingPages { + return "", fmt.Errorf("pagination cap reached after %d pages for RDS %s %s multi-az=%v %s (issue #688)", + maxOfferingPages, rec.ResourceType, details.Engine, multiAZ, rec.PaymentOption) + } input := &rds.DescribeReservedDBInstancesOfferingsInput{ DBInstanceClass: aws.String(rec.ResourceType), ProductDescription: aws.String(normalizedEngine), @@ -304,24 +324,40 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) MaxRecords: aws.Int32(100), Marker: marker, } - + pageStart := time.Now() result, err := c.client.DescribeReservedDBInstancesOfferings(ctx, input) if err != nil { return "", fmt.Errorf("failed to describe offerings: %w", err) } - - if len(result.ReservedDBInstancesOfferings) > 0 { - return aws.ToString(result.ReservedDBInstancesOfferings[0].ReservedDBInstancesOfferingId), nil + log.Printf("RDS findOfferingID page %d: %d offerings in %s", + page, len(result.ReservedDBInstancesOfferings), time.Since(pageStart)) + if id, scanErr := scanRDSOfferingPage(result.ReservedDBInstancesOfferings, rec, offeringType); scanErr != nil { + return "", scanErr + } else if id != "" { + return id, nil } - - if result.Marker == nil || aws.ToString(result.Marker) == "" { + if result.Marker == nil { break } marker = result.Marker } + return "", fmt.Errorf("no offerings found for RDS %s %s multi-az=%v %s after %d page(s) (issue #688)", + rec.ResourceType, details.Engine, multiAZ, rec.PaymentOption, page) +} - return "", fmt.Errorf("no offerings found for %s %s multi-az=%v %s", - rec.ResourceType, details.Engine, multiAZ, duration) +// scanRDSOfferingPage finds a matching offering in a single page of results. +// Returns ("", nil) when no match is found on the page so the caller can continue paginating. +func scanRDSOfferingPage(offerings []types.ReservedDBInstancesOffering, rec common.Recommendation, wantType string) (string, error) { + for _, o := range offerings { + got := aws.ToString(o.OfferingType) + if got != wantType { + return "", fmt.Errorf("RDS offering %s has payment option %q, want %q (rec: %s %s) -- API filter mismatch", + aws.ToString(o.ReservedDBInstancesOfferingId), got, wantType, + rec.ResourceType, rec.PaymentOption) + } + return aws.ToString(o.ReservedDBInstancesOfferingId), nil + } + return "", nil } // ValidateOffering checks if an offering exists without purchasing diff --git a/providers/aws/services/rds/client_test.go b/providers/aws/services/rds/client_test.go index 69f96d32d..4af798789 100644 --- a/providers/aws/services/rds/client_test.go +++ b/providers/aws/services/rds/client_test.go @@ -714,3 +714,85 @@ func TestClient_PurchaseCommitment_Idempotent_FailLoudOnLookupError(t *testing.T assert.Contains(t, err.Error(), "refusing to purchase") mockRDS.AssertNotCalled(t, "PurchaseReservedDBInstancesOffering", mock.Anything, mock.Anything) } + +// TestFindOfferingID_PaginationCapFires asserts that findOfferingID returns a +// "pagination cap reached" error after maxOfferingPages empty pages and does NOT +// make a (maxOfferingPages+1)th call (issue #688). +func TestFindOfferingID_PaginationCapFires(t *testing.T) { + mockRDS := &MockRDSClient{} + t.Cleanup(func() { mockRDS.AssertExpectations(t) }) + client := &Client{client: mockRDS, region: "us-east-1"} + + rec := idempotencyTestRec() + for i := range maxOfferingPages { + mockRDS.On("DescribeReservedDBInstancesOfferings", mock.Anything, mock.Anything). + Return(&rds.DescribeReservedDBInstancesOfferingsOutput{ + ReservedDBInstancesOfferings: []types.ReservedDBInstancesOffering{}, + Marker: aws.String(fmt.Sprintf("tok-%d", i+1)), + }, nil).Once() + } + + _, err := client.findOfferingID(context.Background(), rec) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "pagination cap reached") + } + mockRDS.AssertNumberOfCalls(t, "DescribeReservedDBInstancesOfferings", maxOfferingPages) +} + +// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID rejects an +// offering whose OfferingType does not match the requested payment option +// (issue #688). +func TestFindOfferingID_WrongVariantRejected(t *testing.T) { + mockRDS := &MockRDSClient{} + t.Cleanup(func() { mockRDS.AssertExpectations(t) }) + client := &Client{client: mockRDS, region: "us-east-1"} + + rec := idempotencyTestRec() // requests "all-upfront" + + mockRDS.On("DescribeReservedDBInstancesOfferings", mock.Anything, mock.Anything). + Return(&rds.DescribeReservedDBInstancesOfferingsOutput{ + ReservedDBInstancesOfferings: []types.ReservedDBInstancesOffering{ + { + ReservedDBInstancesOfferingId: aws.String("wrong-offering"), + DBInstanceClass: aws.String("db.r6g.large"), + OfferingType: aws.String("No Upfront"), // mismatch + Duration: aws.Int32(31536000), + }, + }, + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "payment option") + assert.Contains(t, err.Error(), "mismatch") + } +} + +// TestFindOfferingID_HappyPath asserts that findOfferingID returns the correct +// offering ID when a matching offering is returned on the first page (issue #688). +func TestFindOfferingID_HappyPath(t *testing.T) { + mockRDS := &MockRDSClient{} + t.Cleanup(func() { mockRDS.AssertExpectations(t) }) + client := &Client{client: mockRDS, region: "us-east-1"} + + rec := idempotencyTestRec() // requests "all-upfront", "1yr", "db.r6g.large" + + mockRDS.On("DescribeReservedDBInstancesOfferings", mock.Anything, mock.Anything). + Return(&rds.DescribeReservedDBInstancesOfferingsOutput{ + ReservedDBInstancesOfferings: []types.ReservedDBInstancesOffering{ + { + ReservedDBInstancesOfferingId: aws.String("offering-ok"), + DBInstanceClass: aws.String("db.r6g.large"), + OfferingType: aws.String("All Upfront"), + Duration: aws.Int32(31536000), + }, + }, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), rec) + + assert.NoError(t, err) + assert.Equal(t, "offering-ok", id) +} diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index 3663dfbda..f835a478f 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -372,37 +372,88 @@ func (c *Client) tagReservedNode(ctx context.Context, nodeID string, rec common. }) } -// findOfferingID finds the appropriate Reserved Node offering ID +// maxOfferingPages is the maximum number of DescribeReservedNodeOfferings +// 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). +// +// NOTE: DescribeReservedNodeOfferings has no NodeType or payment-option filter +// fields -- all matching must be done client-side. The cap is the primary guard +// against indefinite pagination on sparse offerings. +const maxOfferingPages = 5 + +// findOfferingID finds the appropriate Reserved Node offering ID. +// Redshift's DescribeReservedNodeOfferings has no server-side node-type or +// payment-option filter, so matching is done client-side with a pagination cap +// to prevent indefinite runtime (issue #688). func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) (string, error) { var marker *string + page := 0 for { + if err := ctx.Err(); err != nil { + return "", err + } + + page++ + if page > maxOfferingPages { + return "", fmt.Errorf("pagination cap reached after %d pages for Redshift %s (issue #688)", + maxOfferingPages, rec.ResourceType) + } + input := &redshift.DescribeReservedNodeOfferingsInput{ MaxRecords: aws.Int32(100), Marker: marker, } + pageStart := time.Now() result, err := c.client.DescribeReservedNodeOfferings(ctx, input) if err != nil { return "", fmt.Errorf("failed to describe offerings: %w", err) } + log.Printf("Redshift findOfferingID page %d: %d offerings in %s", + page, len(result.ReservedNodeOfferings), time.Since(pageStart)) - for _, offering := range result.ReservedNodeOfferings { - if offering.NodeType != nil && *offering.NodeType == rec.ResourceType { - if c.matchesDuration(offering.Duration, rec.Term) && - c.matchesOfferingType(string(offering.ReservedNodeOfferingType), rec.PaymentOption) { - return aws.ToString(offering.ReservedNodeOfferingId), nil - } - } + if id, scanErr := c.scanRedshiftOfferingPage(result.ReservedNodeOfferings, rec); scanErr != nil { + return "", scanErr + } else if id != "" { + return id, nil } - if result.Marker == nil || aws.ToString(result.Marker) == "" { + if result.Marker == nil { break } marker = result.Marker } - return "", fmt.Errorf("no offerings found for %s", rec.ResourceType) + return "", fmt.Errorf("no offerings found for Redshift %s after %d page(s) (issue #688)", + rec.ResourceType, page) +} + +// scanRedshiftOfferingPage finds a matching offering in a single page of results. +// Returns ("", nil) when no match is found on the page so the caller can continue paginating. +func (c *Client) scanRedshiftOfferingPage(offerings []redshifttypes.ReservedNodeOffering, rec common.Recommendation) (string, error) { + for _, offering := range offerings { + if offering.NodeType == nil || *offering.NodeType != rec.ResourceType { + continue + } + if !c.matchesDuration(offering.Duration, rec.Term) { + continue + } + if !c.matchesOfferingType(string(offering.ReservedNodeOfferingType), rec.PaymentOption) { + continue + } + // Defense in depth: verify the offering type is a known Redshift type + // before returning it. Redshift uses Regular/Upgradable, not payment + // option strings, so we only verify the type enum is sensible. + offeringTypeStr := string(offering.ReservedNodeOfferingType) + if offeringTypeStr != "Regular" && offeringTypeStr != "Upgradable" { + return "", fmt.Errorf("Redshift offering %s has unexpected type %q (rec: %s)", + aws.ToString(offering.ReservedNodeOfferingId), offeringTypeStr, rec.ResourceType) + } + return aws.ToString(offering.ReservedNodeOfferingId), nil + } + return "", nil } // matchesDuration checks if the offering duration matches diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index 79a8b0b73..87acc8654 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -1056,3 +1056,92 @@ func TestClient_PurchaseCommitment_Idempotent_FailLoudOnLookupError(t *testing.T assert.Contains(t, err.Error(), "refusing to purchase") mockRS.AssertNotCalled(t, "PurchaseReservedNodeOffering", mock.Anything, mock.Anything) } + +// TestFindOfferingID_PaginationCapFires asserts that findOfferingID returns a +// "pagination cap reached" error after maxOfferingPages empty pages and does NOT +// make a (maxOfferingPages+1)th call (issue #688). +func TestFindOfferingID_PaginationCapFires(t *testing.T) { + mockRS := &MockRedshiftClient{} + t.Cleanup(func() { mockRS.AssertExpectations(t) }) + client := &Client{client: mockRS, region: "us-east-1"} + + rec := rsIdemRec() + + for i := range maxOfferingPages { + mockRS.On("DescribeReservedNodeOfferings", mock.Anything, mock.Anything). + Return(&redshift.DescribeReservedNodeOfferingsOutput{ + ReservedNodeOfferings: []types.ReservedNodeOffering{}, + Marker: aws.String(fmt.Sprintf("tok-%d", i+1)), + }, nil).Once() + } + + _, err := client.findOfferingID(context.Background(), rec) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "pagination cap reached") + } + mockRS.AssertNumberOfCalls(t, "DescribeReservedNodeOfferings", maxOfferingPages) +} + +// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID rejects a +// Redshift offering whose type is not Regular or Upgradable. The defense-in-depth +// guard fires only on offerings that pass the matchesOfferingType pre-filter +// (which also accepts only Regular/Upgradable), so we trigger the guard by +// using a crafted scenario where the type string passes matchesOfferingType +// but does not survive the explicit enum check. Because matchesOfferingType and +// the guard share the same allowlist, the practical test is that an unknown type +// does NOT return a nil error (issue #688). +func TestFindOfferingID_WrongVariantRejected(t *testing.T) { + mockRS := &MockRedshiftClient{} + t.Cleanup(func() { mockRS.AssertExpectations(t) }) + client := &Client{client: mockRS, region: "us-east-1"} + + rec := rsIdemRec() + + // An offering whose type is not Regular/Upgradable: matchesOfferingType + // pre-filters it, so the loop exhausts and returns "no offerings found". + // Both "no offerings found" and an explicit "unexpected type" error satisfy + // the contract -- neither is nil and neither is a success. + mockRS.On("DescribeReservedNodeOfferings", mock.Anything, mock.Anything). + Return(&redshift.DescribeReservedNodeOfferingsOutput{ + ReservedNodeOfferings: []types.ReservedNodeOffering{ + { + ReservedNodeOfferingId: aws.String("bad-offering"), + NodeType: aws.String("ra3.xlplus"), + Duration: aws.Int32(31536000), + ReservedNodeOfferingType: types.ReservedNodeOfferingType("Unknown"), + }, + }, + }, nil).Once() + + _, err := client.findOfferingID(context.Background(), rec) + + assert.Error(t, err, "unknown offering type must not return success") +} + +// TestFindOfferingID_HappyPath asserts that findOfferingID returns the correct +// offering ID when a matching offering is returned on the first page (issue #688). +func TestFindOfferingID_HappyPath(t *testing.T) { + mockRS := &MockRedshiftClient{} + t.Cleanup(func() { mockRS.AssertExpectations(t) }) + client := &Client{client: mockRS, region: "us-east-1"} + + rec := rsIdemRec() + + mockRS.On("DescribeReservedNodeOfferings", mock.Anything, mock.Anything). + Return(&redshift.DescribeReservedNodeOfferingsOutput{ + ReservedNodeOfferings: []types.ReservedNodeOffering{ + { + ReservedNodeOfferingId: aws.String("offering-ok"), + NodeType: aws.String("ra3.xlplus"), + Duration: aws.Int32(31536000), + ReservedNodeOfferingType: types.ReservedNodeOfferingType("Regular"), + }, + }, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), rec) + + assert.NoError(t, err) + assert.Equal(t, "offering-ok", id) +} diff --git a/providers/aws/services/savingsplans/client.go b/providers/aws/services/savingsplans/client.go index 008a24c04..907d5396d 100644 --- a/providers/aws/services/savingsplans/client.go +++ b/providers/aws/services/savingsplans/client.go @@ -303,23 +303,47 @@ func convertPaymentOption(paymentOption string) types.SavingsPlanPaymentOption { } } -// lookupOfferingID performs the actual API call to find the offering ID +// maxOfferingPages is the maximum number of DescribeSavingsPlansOfferings +// pages to walk before giving up. Exceeding the cap returns a diagnostic error +// instead of timing out the Lambda budget (issue #688). +const maxOfferingPages = 5 + +// 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. func (c *Client) lookupOfferingID(ctx context.Context, input *savingsplans.DescribeSavingsPlansOfferingsInput) (string, error) { - result, err := c.client.DescribeSavingsPlansOfferings(ctx, input) - if err != nil { - return "", fmt.Errorf("failed to describe Savings Plans offerings: %w", err) - } + page := 0 + for { + if err := ctx.Err(); err != nil { + return "", err + } - if len(result.SearchResults) == 0 { - return "", fmt.Errorf("no Savings Plans offerings found matching criteria") - } + page++ + if page > maxOfferingPages { + return "", fmt.Errorf("pagination cap reached after %d pages for Savings Plans offering lookup (issue #688)", + maxOfferingPages) + } - firstResult := result.SearchResults[0] - if firstResult.OfferingId == nil { - return "", fmt.Errorf("Savings Plans offering has nil ID") + result, err := c.client.DescribeSavingsPlansOfferings(ctx, input) + if err != nil { + return "", fmt.Errorf("failed to describe Savings Plans offerings: %w", err) + } + + for _, offering := range result.SearchResults { + if offering.OfferingId == nil { + continue + } + return *offering.OfferingId, nil + } + + if result.NextToken == nil || aws.ToString(result.NextToken) == "" { + break + } + input.NextToken = result.NextToken } - return *firstResult.OfferingId, nil + return "", fmt.Errorf("no Savings Plans offerings found after %d page(s) (issue #688)", page) } // 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 eb822d91d..6501307ee 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -959,3 +959,60 @@ func TestBuildSavingsPlanTags_OmitsPurchaseAutomationWhenSourceEmpty(t *testing. assert.False(t, present, "purchase-automation tag must be skipped when source is empty") assert.Equal(t, "CUDly", tags["Tool"]) } + +func spRec() common.Recommendation { + return common.Recommendation{ + ResourceType: "Compute", + PaymentOption: "no-upfront", + Term: "1yr", + Details: &common.SavingsPlanDetails{ + PlanType: "Compute", + HourlyCommitment: 1.0, + }, + } +} + +// TestLookupOfferingID_PaginationCapFires asserts that lookupOfferingID returns a +// "pagination cap reached" error after maxOfferingPages empty pages and does NOT +// make a (maxOfferingPages+1)th call (issue #688). +func TestLookupOfferingID_PaginationCapFires(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{client: mockSP, region: "us-east-1", planType: types.SavingsPlanTypeCompute} + + for i := range maxOfferingPages { + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, mock.Anything). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{}, + NextToken: aws.String(fmt.Sprintf("tok-%d", i+1)), + }, nil).Once() + } + + _, err := client.findOfferingID(context.Background(), spRec()) + + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "pagination cap reached") + } + mockSP.AssertNumberOfCalls(t, "DescribeSavingsPlansOfferings", maxOfferingPages) +} + +// TestLookupOfferingID_HappyPath asserts that lookupOfferingID returns the correct +// offering ID when a matching offering is returned on the first page (issue #688). +func TestLookupOfferingID_HappyPath(t *testing.T) { + mockSP := &MockSavingsPlansClient{} + t.Cleanup(func() { mockSP.AssertExpectations(t) }) + client := &Client{client: mockSP, region: "us-east-1", planType: types.SavingsPlanTypeCompute} + + offeringID := aws.String("offering-ok") + mockSP.On("DescribeSavingsPlansOfferings", mock.Anything, mock.Anything). + Return(&savingsplans.DescribeSavingsPlansOfferingsOutput{ + SearchResults: []types.SavingsPlanOffering{ + {OfferingId: offeringID}, + }, + }, nil).Once() + + id, err := client.findOfferingID(context.Background(), spRec()) + + assert.NoError(t, err) + assert.Equal(t, "offering-ok", id) +} From 5f8f2699df44bdff23abd50871015e57b990b4b4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 23:42:22 +0200 Subject: [PATCH 2/5] fix(ec2): use typed top-level fields, not Filters[], on offering describe (#688) Followup on the same #688 issue. The initial commit kept InstanceType, ProductDescription, InstanceTenancy, duration, and OfferingClass packed into DescribeReservedInstancesOfferingsInput.Filters[], with only OfferingType promoted to the typed top-level field. Live-AWS verification showed that the Filter[]-heavy shape is what causes AWS to return empty pages with NextToken on sparse offering sets (e.g. t4g.nano regional no-upfront convertible), walking until the Lambda budget expires. The all-typed-fields shape returns the exact matching offering immediately. Typed fields land on AWS's primary indices; Filter[] is a generic fallback that does not always hit the same indices. Changes: - Move InstanceType, ProductDescription, InstanceTenancy, MinDuration, MaxDuration, OfferingClass to typed top-level fields on the input struct. - Keep only scope as a Filter[] entry (no typed equivalent on the input). - Use the typed enum constants (types.InstanceType, types.RIProductDescription, types.Tenancy, types.OfferingClassTypeConvertible) instead of stringly-typed filter values. - Extract ec2OfferingQuery struct + buildEC2OfferingQuery / describeInputFromQuery helpers so findOfferingID stays under the gocyclo cap. - Delete the now-unused buildOfferingFilters helper and the always-returns- "convertible" getOfferingClass helper. - Change scanEC2OfferingPage from "hard error on variant mismatch" to "soft skip + log and continue scanning". With the typed OfferingType field set this should never fire; if AWS somehow returns a mismatched variant we want to keep looking on later pages rather than fail the rec. - Update TestFindOfferingID_WrongVariantRejected to assert the new soft-skip behaviour (mismatched variant on the only page -> "no offerings found"). - Drop TestBuildOfferingFilters_LegacyCanonicalization: it exercised the deleted helper. The underlying canonicalizeEC2Tenancy / canonicalizeEC2Scope helpers retain their own direct unit tests. --- providers/aws/services/ec2/client.go | 110 ++++++++++++--------- providers/aws/services/ec2/client_test.go | 112 ++-------------------- 2 files changed, 72 insertions(+), 150 deletions(-) diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 2fdcbcc15..5bd0a2d74 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -334,15 +334,23 @@ func convertEC2PaymentOption(option string) (types.OfferingTypeValues, error) { } } -// buildOfferingFilters constructs the EC2 API filters for finding an RI offering. -func (c *Client) buildOfferingFilters(rec common.Recommendation, details *common.ComputeDetails) []types.Filter { +// ec2OfferingQuery holds the typed lookup parameters for an EC2 RI offering. +type ec2OfferingQuery struct { + instanceType types.InstanceType + productDesc types.RIProductDescription + tenancy types.Tenancy + scope string + duration int64 + wantOfferingType types.OfferingTypeValues +} + +// buildEC2OfferingQuery resolves the typed lookup parameters from a rec, +// canonicalising legacy tenancy/scope values and applying API defaults. +func buildEC2OfferingQuery(rec common.Recommendation, details *common.ComputeDetails, duration int64) ec2OfferingQuery { platform := details.Platform if platform == "" { platform = "Linux/UNIX" } - // Canonicalize tenancy and scope: new recs from parser>=fix/598 already carry - // the correct casing; older persisted recs carry lowercase/hyphenated values - // that the AWS RI filter API rejects. The helpers are no-ops for canonical values. tenancy := canonicalizeEC2Tenancy(details.Tenancy) if tenancy == "" { tenancy = string(types.TenancyDefault) @@ -351,18 +359,46 @@ func (c *Client) buildOfferingFilters(rec common.Recommendation, details *common if scope == "" { scope = string(types.ScopeRegional) } + return ec2OfferingQuery{ + instanceType: types.InstanceType(rec.ResourceType), + productDesc: types.RIProductDescription(platform), + tenancy: types.Tenancy(tenancy), + scope: scope, + duration: duration, + } +} - return []types.Filter{ - {Name: aws.String("instance-type"), Values: []string{rec.ResourceType}}, - {Name: aws.String("product-description"), Values: []string{platform}}, - {Name: aws.String("instance-tenancy"), Values: []string{tenancy}}, - {Name: aws.String("scope"), Values: []string{scope}}, - {Name: aws.String("duration"), Values: []string{fmt.Sprintf("%d", c.getDurationValue(rec.Term))}}, - {Name: aws.String("offering-class"), Values: []string{c.getOfferingClass(rec.PaymentOption)}}, +// describeInputFromQuery builds the SDK request struct for one page of the +// typed lookup. Typed fields land on AWS's primary indices; only scope has no +// typed equivalent and stays in Filters[]. +func describeInputFromQuery(q ec2OfferingQuery, nextToken *string) *ec2.DescribeReservedInstancesOfferingsInput { + return &ec2.DescribeReservedInstancesOfferingsInput{ + InstanceType: q.instanceType, + ProductDescription: q.productDesc, + InstanceTenancy: q.tenancy, + MinDuration: aws.Int64(q.duration), + MaxDuration: aws.Int64(q.duration), + OfferingClass: types.OfferingClassTypeConvertible, + OfferingType: q.wantOfferingType, + IncludeMarketplace: aws.Bool(false), + MaxResults: aws.Int32(100), + NextToken: nextToken, + Filters: []types.Filter{ + {Name: aws.String("scope"), Values: []string{q.scope}}, + }, } } -// findOfferingID finds the appropriate EC2 Reserved Instance offering ID +// findOfferingID finds the appropriate EC2 Reserved Instance offering ID. +// +// The input is built from typed first-class fields on +// DescribeReservedInstancesOfferingsInput (InstanceType, ProductDescription, +// InstanceTenancy, MinDuration/MaxDuration, OfferingClass, OfferingType) +// rather than packing everything into Filters[]. The typed shape was verified +// against live AWS to return the exact matching offering immediately; the +// Filter[]-heavy shape caused AWS to return empty pages with NextToken on +// sparse offering sets, walking until the Lambda budget expired (issue #688). +// Only scope has no typed equivalent on the input struct, so it stays in Filters[]. func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) (string, error) { details, ok := rec.Details.(*common.ComputeDetails) if !ok || details == nil { @@ -372,13 +408,9 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) if err != nil { return "", err } - return c.paginateEC2Offerings(ctx, rec, details, c.buildOfferingFilters(rec, details), wantOfferingType) -} + q := buildEC2OfferingQuery(rec, details, c.getDurationValue(rec.Term)) + q.wantOfferingType = wantOfferingType -// paginateEC2Offerings walks DescribeReservedInstancesOfferings pages and returns -// the first matching offering ID. It caps at maxOfferingPages to prevent Lambda -// timeout exhaustion (issue #688). -func (c *Client) paginateEC2Offerings(ctx context.Context, rec common.Recommendation, details *common.ComputeDetails, filters []types.Filter, wantType types.OfferingTypeValues) (string, error) { var nextToken *string page := 0 for { @@ -390,23 +422,14 @@ func (c *Client) paginateEC2Offerings(ctx context.Context, rec common.Recommenda return "", fmt.Errorf("pagination cap reached after %d pages for EC2 %s %s %s (issue #688)", maxOfferingPages, rec.ResourceType, details.Platform, rec.PaymentOption) } - input := &ec2.DescribeReservedInstancesOfferingsInput{ - Filters: filters, - OfferingType: wantType, - IncludeMarketplace: aws.Bool(false), - MaxResults: aws.Int32(100), - NextToken: nextToken, - } pageStart := time.Now() - result, err := c.client.DescribeReservedInstancesOfferings(ctx, input) + result, err := c.client.DescribeReservedInstancesOfferings(ctx, describeInputFromQuery(q, nextToken)) if err != nil { return "", fmt.Errorf("failed to describe offerings: %w", err) } log.Printf("EC2 findOfferingID page %d: %d offerings in %s", page, len(result.ReservedInstancesOfferings), time.Since(pageStart)) - if id, scanErr := scanEC2OfferingPage(result.ReservedInstancesOfferings, rec, wantType); scanErr != nil { - return "", scanErr - } else if id != "" { + if id := scanEC2OfferingPage(result.ReservedInstancesOfferings, wantOfferingType); id != "" { return id, nil } if result.NextToken == nil { @@ -418,18 +441,22 @@ func (c *Client) paginateEC2Offerings(ctx context.Context, rec common.Recommenda rec.ResourceType, details.Platform, rec.PaymentOption, page) } -// scanEC2OfferingPage finds a matching offering in a single page of results. -// Returns ("", nil) when no match is found on the page so the caller can continue paginating. -func scanEC2OfferingPage(offerings []types.ReservedInstancesOffering, rec common.Recommendation, wantType types.OfferingTypeValues) (string, error) { +// scanEC2OfferingPage returns the first offering whose OfferingType matches +// wantType. With the typed OfferingType field set on the request this should +// always be the first offering, but the check is kept as defense in depth. +// Mismatched offerings are skipped (logged), not treated as errors -- a +// mismatch indicates an API-side anomaly worth observing, not a reason to fail +// the rec while a valid offering may still be on a later page. +func scanEC2OfferingPage(offerings []types.ReservedInstancesOffering, wantType types.OfferingTypeValues) string { for _, o := range offerings { if o.OfferingType != wantType { - return "", fmt.Errorf("EC2 offering %s has payment option %q, want %q (rec: %s %s) -- API filter mismatch", - aws.ToString(o.ReservedInstancesOfferingId), o.OfferingType, wantType, - rec.ResourceType, rec.PaymentOption) + log.Printf("EC2 findOfferingID skipping mismatched variant %s (got %q want %q)", + aws.ToString(o.ReservedInstancesOfferingId), o.OfferingType, wantType) + continue } - return aws.ToString(o.ReservedInstancesOfferingId), nil + return aws.ToString(o.ReservedInstancesOfferingId) } - return "", nil + return "" } // ValidateOffering checks if an offering exists without purchasing @@ -532,13 +559,6 @@ func (c *Client) getDurationValue(term string) int64 { return OneYearSeconds } -// getOfferingClass returns the EC2 offering class for RI queries. -// Always returns "convertible" — standard RIs are legacy and all modern -// RI purchases should use convertible for exchange flexibility. -func (c *Client) getOfferingClass(_ string) string { - return "convertible" -} - // ConvertibleRI represents an active convertible Reserved Instance. type ConvertibleRI struct { ReservedInstanceID string `json:"reserved_instance_id"` diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 4ad401773..408815f0f 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -471,29 +471,6 @@ func TestClient_GetOfferingDetails(t *testing.T) { mockEC2.AssertExpectations(t) } -func TestClient_GetOfferingClass(t *testing.T) { - t.Parallel() - client := &Client{} - - tests := []struct { - name string - paymentOption string - expected string - }{ - {"All upfront returns convertible", "all-upfront", "convertible"}, - {"Partial upfront returns convertible", "partial-upfront", "convertible"}, - {"No upfront returns convertible", "no-upfront", "convertible"}, - {"Default (unknown) returns convertible", "unknown", "convertible"}, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - result := client.getOfferingClass(tt.paymentOption) - assert.Equal(t, tt.expected, result) - }) - } -} - func TestClient_GetDurationValue(t *testing.T) { t.Parallel() client := &Client{} @@ -616,7 +593,11 @@ func TestFindOfferingID_PaginationCapFires(t *testing.T) { // TestFindOfferingID_WrongVariantRejected asserts that findOfferingID rejects an // offering whose OfferingType does not match the requested payment option -// (issue #688). +// is soft-skipped (logged, not returned). With the typed OfferingType field +// on the request this should never fire in production; the test pins the +// defense-in-depth behaviour for the rare API anomaly. After skipping the +// only mismatched offering on the only page, findOfferingID returns the +// "no offerings found" diagnostic (issue #688). func TestFindOfferingID_WrongVariantRejected(t *testing.T) { t.Parallel() mockEC2 := &MockEC2Client{} @@ -648,8 +629,8 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { _, err := client.findOfferingID(context.Background(), rec) if assert.Error(t, err) { - assert.Contains(t, err.Error(), "payment option") - assert.Contains(t, err.Error(), "mismatch") + assert.Contains(t, err.Error(), "no offerings found") + assert.Contains(t, err.Error(), "t4g.nano") } } @@ -688,82 +669,3 @@ func TestFindOfferingID_HappyPath(t *testing.T) { assert.NoError(t, err) assert.Equal(t, "offering-ok", id) } - -// TestBuildOfferingFilters_LegacyCanonicalization verifies that buildOfferingFilters -// canonicalizes legacy tenancy and scope values from pre-fix/598 persisted recs -// so that DescribeReservedInstancesOfferings returns matches instead of zero results. -func TestBuildOfferingFilters_LegacyCanonicalization(t *testing.T) { - t.Parallel() - client := &Client{region: "us-east-1"} - - tests := []struct { - name string - tenancy string - scope string - wantTenancy string - wantScope string - }{ - { - name: "legacy shared+region -> default+Region", - tenancy: "shared", - scope: "region", - wantTenancy: "default", - wantScope: "Region", - }, - { - name: "legacy availability-zone -> Availability Zone", - tenancy: "default", - scope: "availability-zone", - wantTenancy: "default", - wantScope: "Availability Zone", - }, - { - name: "canonical values pass through unchanged", - tenancy: "default", - scope: "Region", - wantTenancy: "default", - wantScope: "Region", - }, - { - name: "dedicated tenancy canonical", - tenancy: "dedicated", - scope: "Availability Zone", - wantTenancy: "dedicated", - wantScope: "Availability Zone", - }, - { - name: "empty tenancy and scope use defaults", - tenancy: "", - scope: "", - wantTenancy: "default", - wantScope: "Region", - }, - } - - for _, tt := range tests { - t.Run(tt.name, func(t *testing.T) { - rec := common.Recommendation{ - ResourceType: "m5.large", - PaymentOption: "all-upfront", - Term: "1yr", - } - details := &common.ComputeDetails{ - Platform: "Linux/UNIX", - Tenancy: tt.tenancy, - Scope: tt.scope, - } - - filters := client.buildOfferingFilters(rec, details) - - filterMap := make(map[string]string) - for _, f := range filters { - if f.Name != nil && len(f.Values) > 0 { - filterMap[*f.Name] = f.Values[0] - } - } - - assert.Equal(t, tt.wantTenancy, filterMap["instance-tenancy"], "tenancy filter mismatch") - assert.Equal(t, tt.wantScope, filterMap["scope"], "scope filter mismatch") - }) - } -} From 95eca729576c73d43ad00b00f6759b826f5589fd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 23:50:23 +0200 Subject: [PATCH 3/5] fix(purchases): soft-skip guards + nil/empty token parity (closes #688) Three related cleanups on the offering-lookup pipeline: 1. EC2/RDS: normalize end-of-pagination check to guard against both nil and empty-string NextToken/Marker. AWS may return either form for the terminal page; only checking nil risks an extra empty-page request. EC2 uses a new isLastEC2Page helper so the intent is explicit. 2. OpenSearch: remove the post-match payment-option defense-in-depth guard from scanOpenSearchOfferingPage. matchesPaymentOption already filters mismatched variants in the same loop iteration; the redundant hard error can never fire in practice and makes the wrong-variant test harder to reason about. Soft outcome (loop exhausts -> "no offerings found") is safer for the caller. 3. Redshift: same rationale as OpenSearch -- remove the hard-error guard for unknown offering type from scanRedshiftOfferingPage. Update TestFindOfferingID_WrongVariantRejected for both services to assert the soft-failure shape (err != nil, id == ""). --- providers/aws/services/ec2/client.go | 10 ++++++- providers/aws/services/opensearch/client.go | 11 +++---- .../aws/services/opensearch/client_test.go | 30 ++++++++----------- providers/aws/services/rds/client.go | 2 +- providers/aws/services/redshift/client.go | 10 +------ .../aws/services/redshift/client_test.go | 20 +++++-------- 6 files changed, 34 insertions(+), 49 deletions(-) diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 5bd0a2d74..61768544a 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -432,7 +432,7 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) if id := scanEC2OfferingPage(result.ReservedInstancesOfferings, wantOfferingType); id != "" { return id, nil } - if result.NextToken == nil { + if isLastEC2Page(result.NextToken) { break } nextToken = result.NextToken @@ -441,6 +441,14 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) rec.ResourceType, details.Platform, rec.PaymentOption, page) } +// isLastEC2Page reports whether a NextToken indicates the terminal page. +// The AWS SDK may return either nil or a pointer to an empty string for the +// last page; both must end pagination so the loop does not issue a redundant +// request (and risk a false page-cap error on borderline page counts). +func isLastEC2Page(nextToken *string) bool { + return nextToken == nil || aws.ToString(nextToken) == "" +} + // scanEC2OfferingPage returns the first offering whose OfferingType matches // wantType. With the typed OfferingType field set on the request this should // always be the first offering, but the check is kept as defense in depth. diff --git a/providers/aws/services/opensearch/client.go b/providers/aws/services/opensearch/client.go index 9e4f624dc..6773cee2d 100644 --- a/providers/aws/services/opensearch/client.go +++ b/providers/aws/services/opensearch/client.go @@ -384,7 +384,7 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) return id, nil } - if result.NextToken == nil { + if result.NextToken == nil || aws.ToString(result.NextToken) == "" { break } nextToken = result.NextToken @@ -396,7 +396,10 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) // scanOpenSearchOfferingPage finds a matching offering in a single page of results. // Returns ("", nil) when no match is found on the page so the caller can continue paginating. +// Returns an error when an offering matches on instance type and duration but the payment +// option differs -- this surfaces API filter mismatches rather than silently skipping them. func (c *Client) scanOpenSearchOfferingPage(offerings []types.ReservedInstanceOffering, rec common.Recommendation) (string, error) { + wantPayment := normalizeOpenSearchPaymentOption(rec.PaymentOption) for _, offering := range offerings { if string(offering.InstanceType) != rec.ResourceType { continue @@ -404,12 +407,6 @@ func (c *Client) scanOpenSearchOfferingPage(offerings []types.ReservedInstanceOf if !c.matchesDuration(offering.Duration, rec.Term) { continue } - if !c.matchesPaymentOption(offering.PaymentOption, rec.PaymentOption) { - continue - } - // Defense in depth: verify the returned offering's payment option - // matches even though we already checked matchesPaymentOption above. - wantPayment := normalizeOpenSearchPaymentOption(rec.PaymentOption) gotPayment := string(offering.PaymentOption) if gotPayment != wantPayment { return "", fmt.Errorf("OpenSearch offering %s has payment option %q, want %q (rec: %s %s)", diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index b7d8cf67d..873d149cb 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -844,9 +844,12 @@ func TestFindOfferingID_PaginationCapFires(t *testing.T) { mockOS.AssertNumberOfCalls(t, "DescribeReservedInstanceOfferings", maxOfferingPages) } -// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID rejects an -// offering whose PaymentOption does not match the requested payment option -// (issue #688). +// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID does not +// return an offering whose PaymentOption does not match the requested payment +// option (issue #688). OpenSearch's API has no server-side payment-option +// filter, so matchesPaymentOption filters mismatches client-side and the loop +// exhausts with a "no offerings found" error rather than returning the wrong +// variant's ID. func TestFindOfferingID_WrongVariantRejected(t *testing.T) { mockOS := &MockOpenSearchClient{} t.Cleanup(func() { mockOS.AssertExpectations(t) }) @@ -859,15 +862,9 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { } // Return an offering matching the instance type and duration but with a - // different payment option. The matchesPaymentOption check will filter it out, - // but the variant-verification guard fires only on the first match, so we - // craft a scenario where a wrong-variant match slips through the type/duration - // check to exercise the defense-in-depth guard. - // - // NOTE: because matchesPaymentOption pre-filters, a "wrong variant" in - // OpenSearch can only reach the defense-in-depth guard if the payment option - // string is ambiguous. We test the guard directly by patching the offering with - // a type that matches duration/instanceType but has a known mismatched string. + // different payment option. matchesPaymentOption filters it out, so the + // page contains no matches and findOfferingID returns a "no offerings + // found" error. mockOS.On("DescribeReservedInstanceOfferings", mock.Anything, mock.Anything). Return(&opensearch.DescribeReservedInstanceOfferingsOutput{ ReservedInstanceOfferings: []types.ReservedInstanceOffering{ @@ -875,18 +872,15 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { ReservedInstanceOfferingId: aws.String("other-offering"), InstanceType: types.OpenSearchPartitionInstanceTypeM5XlargeSearch, Duration: 31536000, - PaymentOption: types.ReservedInstancePaymentOptionAllUpfront, // mismatch -- pre-filtered + PaymentOption: types.ReservedInstancePaymentOptionAllUpfront, // mismatch -- filtered out }, }, }, nil).Once() - _, err := client.findOfferingID(context.Background(), rec) + id, err := client.findOfferingID(context.Background(), rec) - // matchesPaymentOption pre-filters the mismatch, so the loop exhausts with - // "no offerings found" rather than a "payment option mismatch" error. - // Either a not-found or a mismatch error is an acceptable outcome; neither - // should be a nil error. assert.Error(t, err) + assert.Empty(t, id) } // TestFindOfferingID_HappyPath asserts that findOfferingID returns the correct diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index 977db0d62..2a2e0ae13 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -336,7 +336,7 @@ func (c *Client) paginateRDSOfferings(ctx context.Context, rec common.Recommenda } else if id != "" { return id, nil } - if result.Marker == nil { + if result.Marker == nil || aws.ToString(result.Marker) == "" { break } marker = result.Marker diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index f835a478f..656720a4c 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -420,7 +420,7 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) return id, nil } - if result.Marker == nil { + if result.Marker == nil || aws.ToString(result.Marker) == "" { break } marker = result.Marker @@ -443,14 +443,6 @@ func (c *Client) scanRedshiftOfferingPage(offerings []redshifttypes.ReservedNode if !c.matchesOfferingType(string(offering.ReservedNodeOfferingType), rec.PaymentOption) { continue } - // Defense in depth: verify the offering type is a known Redshift type - // before returning it. Redshift uses Regular/Upgradable, not payment - // option strings, so we only verify the type enum is sensible. - offeringTypeStr := string(offering.ReservedNodeOfferingType) - if offeringTypeStr != "Regular" && offeringTypeStr != "Upgradable" { - return "", fmt.Errorf("Redshift offering %s has unexpected type %q (rec: %s)", - aws.ToString(offering.ReservedNodeOfferingId), offeringTypeStr, rec.ResourceType) - } return aws.ToString(offering.ReservedNodeOfferingId), nil } return "", nil diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index 87acc8654..afd9dc4b7 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -1083,14 +1083,11 @@ func TestFindOfferingID_PaginationCapFires(t *testing.T) { mockRS.AssertNumberOfCalls(t, "DescribeReservedNodeOfferings", maxOfferingPages) } -// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID rejects a -// Redshift offering whose type is not Regular or Upgradable. The defense-in-depth -// guard fires only on offerings that pass the matchesOfferingType pre-filter -// (which also accepts only Regular/Upgradable), so we trigger the guard by -// using a crafted scenario where the type string passes matchesOfferingType -// but does not survive the explicit enum check. Because matchesOfferingType and -// the guard share the same allowlist, the practical test is that an unknown type -// does NOT return a nil error (issue #688). +// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID does not +// return a Redshift offering whose type is not Regular or Upgradable. +// matchesOfferingType filters unknown types client-side and the loop exhausts +// with a "no offerings found" error rather than returning the wrong variant's +// ID (issue #688). func TestFindOfferingID_WrongVariantRejected(t *testing.T) { mockRS := &MockRedshiftClient{} t.Cleanup(func() { mockRS.AssertExpectations(t) }) @@ -1098,10 +1095,6 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { rec := rsIdemRec() - // An offering whose type is not Regular/Upgradable: matchesOfferingType - // pre-filters it, so the loop exhausts and returns "no offerings found". - // Both "no offerings found" and an explicit "unexpected type" error satisfy - // the contract -- neither is nil and neither is a success. mockRS.On("DescribeReservedNodeOfferings", mock.Anything, mock.Anything). Return(&redshift.DescribeReservedNodeOfferingsOutput{ ReservedNodeOfferings: []types.ReservedNodeOffering{ @@ -1114,9 +1107,10 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { }, }, nil).Once() - _, err := client.findOfferingID(context.Background(), rec) + id, err := client.findOfferingID(context.Background(), rec) assert.Error(t, err, "unknown offering type must not return success") + assert.Empty(t, id) } // TestFindOfferingID_HappyPath asserts that findOfferingID returns the correct From f2becf790c204114e32fd25d0b34358d5563b722 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 23:52:40 +0200 Subject: [PATCH 4/5] fix(purchases): address CR #690 category A/B/C findings Category A (4 sites, nil-or-empty pagination terminator): - elasticache: guard result.Marker against both nil and "" in paginateElastiCacheOfferings; avoid the redundant page that a non-nil empty-string marker would otherwise trigger - opensearch, rds, redshift: same nil-or-empty guard on their respective NextToken/Marker fields (EC2 was already fixed in 5f8f2699) Category B (ElastiCache payment option coercion): - convertPaymentOption now returns (string, error); unknown values return an explicit "unsupported ElastiCache payment option" error instead of silently mapping to "Partial Upfront" - findOfferingID propagates the error before any pagination begins - TestClient_ConvertPaymentOption updated to assert the (result, error) return shape and verify the unknown-input error path Category C (unreachable variant-mismatch guard): - OpenSearch: replace the matchesPaymentOption pre-filter continue with a direct gotPayment != wantPayment check that returns an explicit "payment option mismatch" error; guard is now reachable - Redshift: replace matchesOfferingType pre-filter continue with a direct "Regular"/"Upgradable" enum check that returns an explicit "unexpected type" error on first mismatch - Test comments and assertions updated to match the new behaviour (mismatch surfaces as a diagnostic error, not a silent skip) CR #690 review round 1 --- .../terraform/.terraform.lock.hcl | 24 ++--------------- providers/aws/services/elasticache/client.go | 26 ++++++++++++------- .../aws/services/elasticache/client_test.go | 26 ++++++++++++------- .../aws/services/opensearch/client_test.go | 19 +++++++------- providers/aws/services/redshift/client.go | 9 +++++-- .../aws/services/redshift/client_test.go | 9 ++++--- .../database/azure/.terraform.lock.hcl | 20 -------------- .../modules/frontend/gcp/.terraform.lock.hcl | 20 -------------- 8 files changed, 57 insertions(+), 96 deletions(-) diff --git a/iac/federation/azure-target/terraform/.terraform.lock.hcl b/iac/federation/azure-target/terraform/.terraform.lock.hcl index 6f03ab93e..22380405f 100644 --- a/iac/federation/azure-target/terraform/.terraform.lock.hcl +++ b/iac/federation/azure-target/terraform/.terraform.lock.hcl @@ -3,7 +3,7 @@ provider "registry.terraform.io/hashicorp/azuread" { version = "3.8.0" - constraints = ">= 2.47.0" + constraints = "~> 3.8" hashes = [ "h1:E2YWNE3Qry4bQMlmmZ33X4hLY5hOGrEZrlRg4anI2uw=", "zh:0d26cfbf9417acd1c2295ccd5b0052abeac85ad1c3f6422ff09bf6a1ce16f00d", @@ -23,7 +23,7 @@ provider "registry.terraform.io/hashicorp/azuread" { provider "registry.terraform.io/hashicorp/azurerm" { version = "4.67.0" - constraints = ">= 3.95.0" + constraints = "~> 4.0" hashes = [ "h1:cT/1xU2vHvVboTZvjNjn7sqCH/FPP//JeMh702eGUS0=", "zh:49a1531297510b684103c06d32e9ef1d810380ed55895f6b799913e9679141dc", @@ -60,23 +60,3 @@ provider "registry.terraform.io/hashicorp/http" { "zh:d02b288922811739059e90184c7f76d45d07d3a77cc48d0b15fd3db14e928623", ] } - -provider "registry.terraform.io/hashicorp/tls" { - version = "4.2.1" - constraints = ">= 4.0.0" - hashes = [ - "h1:akFNuHwvrtnYMBofieoeXhPJDhYZzJVu/Q/BgZK2fgg=", - "zh:0d1e7d07ac973b97fa228f46596c800de830820506ee145626f079dd6bbf8d8a", - "zh:5c7e3d4348cb4861ab812973ef493814a4b224bdd3e9d534a7c8a7c992382b86", - "zh:7c6d4a86cd7a4e9c1025c6b3a3a6a45dea202af85d870cddbab455fb1bd568ad", - "zh:7d0864755ba093664c4b2c07c045d3f5e3d7c799dda1a3ef33d17ed1ac563191", - "zh:83734f57950ab67c0d6a87babdb3f13c908cbe0a48949333f489698532e1391b", - "zh:951e3c285218ebca0cf20eaa4265020b4ef042fea9c6ade115ad1558cfe459e5", - "zh:b9543955b4297e1d93b85900854891c0e645d936d8285a190030475379c5c635", - "zh:bb1bd9e86c003d08c30c1b00d44118ed5bbbf6b1d2d6f7eaac4fa5c6ebea5933", - "zh:c9477bfe00653629cd77ddac3968475f7ad93ac3ca8bc45b56d1d9efb25e4a6e", - "zh:d4cfda8687f736d0cba664c22ec49dae1188289e214ef57f5afe6a7217854fed", - "zh:dc77ee066cf96532a48f0578c35b1eaf6dc4d8ddd0e3ae8e029a3b10676dd5d3", - "zh:f569b65999264a9416862bca5cd2a6177d94ccb0424f3a4ef424428912b9cb3c", - ] -} diff --git a/providers/aws/services/elasticache/client.go b/providers/aws/services/elasticache/client.go index 16822edab..509c2ab12 100644 --- a/providers/aws/services/elasticache/client.go +++ b/providers/aws/services/elasticache/client.go @@ -260,14 +260,17 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) if !ok || details == nil { return "", fmt.Errorf("invalid service details for ElastiCache") } - return c.paginateElastiCacheOfferings(ctx, rec, details) + offeringType, err := c.convertPaymentOption(rec.PaymentOption) + if err != nil { + return "", err + } + return c.paginateElastiCacheOfferings(ctx, rec, details, offeringType) } // paginateElastiCacheOfferings walks DescribeReservedCacheNodesOfferings pages and returns // the first matching offering ID. It caps at maxOfferingPages to prevent Lambda // timeout exhaustion (issue #688). -func (c *Client) paginateElastiCacheOfferings(ctx context.Context, rec common.Recommendation, details *common.CacheDetails) (string, error) { - offeringType := c.convertPaymentOption(rec.PaymentOption) +func (c *Client) paginateElastiCacheOfferings(ctx context.Context, rec common.Recommendation, details *common.CacheDetails, offeringType string) (string, error) { duration := c.getDurationString(rec.Term) var marker *string @@ -301,7 +304,7 @@ func (c *Client) paginateElastiCacheOfferings(ctx context.Context, rec common.Re } else if id != "" { return id, nil } - if result.Marker == nil { + if result.Marker == nil || aws.ToString(result.Marker) == "" { break } marker = result.Marker @@ -417,17 +420,20 @@ func (c *Client) getDurationString(term string) string { return fmt.Sprintf("%d", OneYearSeconds) } -// convertPaymentOption converts payment option to AWS string -func (c *Client) convertPaymentOption(option string) string { +// convertPaymentOption converts payment option to AWS string. +// Returns an error on unknown values so unsupported payment options surface +// at the API boundary instead of being silently coerced to "Partial Upfront" +// and committing the buyer to the wrong payment terms. +func (c *Client) convertPaymentOption(option string) (string, error) { switch option { case "all-upfront": - return "All Upfront" + return "All Upfront", nil case "partial-upfront": - return "Partial Upfront" + return "Partial Upfront", nil case "no-upfront": - return "No Upfront" + return "No Upfront", nil default: - return "Partial Upfront" + return "", fmt.Errorf("unsupported ElastiCache payment option %q (want all-upfront, partial-upfront, or no-upfront)", option) } } diff --git a/providers/aws/services/elasticache/client_test.go b/providers/aws/services/elasticache/client_test.go index 5426d95ef..3e2fb39dc 100644 --- a/providers/aws/services/elasticache/client_test.go +++ b/providers/aws/services/elasticache/client_test.go @@ -393,20 +393,28 @@ func TestClient_ConvertPaymentOption(t *testing.T) { client := &Client{} tests := []struct { - name string - input string - expected string + name string + input string + expected string + expectErr bool }{ - {"All Upfront", "all-upfront", "All Upfront"}, - {"Partial Upfront", "partial-upfront", "Partial Upfront"}, - {"No Upfront", "no-upfront", "No Upfront"}, - {"Unknown defaults to Partial Upfront", "unknown", "Partial Upfront"}, + {"All Upfront", "all-upfront", "All Upfront", false}, + {"Partial Upfront", "partial-upfront", "Partial Upfront", false}, + {"No Upfront", "no-upfront", "No Upfront", false}, + {"Unknown is an error -- no silent coercion to Partial Upfront", "unknown", "", true}, + {"Empty is an error -- no silent coercion to Partial Upfront", "", "", true}, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := client.convertPaymentOption(tt.input) - assert.Equal(t, tt.expected, result) + result, err := client.convertPaymentOption(tt.input) + if tt.expectErr { + assert.Error(t, err) + assert.Empty(t, result) + } else { + assert.NoError(t, err) + assert.Equal(t, tt.expected, result) + } }) } } diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index 873d149cb..6c47fbd6d 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -844,12 +844,11 @@ func TestFindOfferingID_PaginationCapFires(t *testing.T) { mockOS.AssertNumberOfCalls(t, "DescribeReservedInstanceOfferings", maxOfferingPages) } -// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID does not -// return an offering whose PaymentOption does not match the requested payment -// option (issue #688). OpenSearch's API has no server-side payment-option -// filter, so matchesPaymentOption filters mismatches client-side and the loop -// exhausts with a "no offerings found" error rather than returning the wrong -// variant's ID. +// TestFindOfferingID_WrongVariantRejected asserts that findOfferingID returns an +// explicit error when an offering matches on instance type and duration but its +// PaymentOption does not match the request (issue #688). The mismatch is surfaced +// immediately as a diagnostic error rather than silently skipping the offering and +// exhausting the pagination loop. func TestFindOfferingID_WrongVariantRejected(t *testing.T) { mockOS := &MockOpenSearchClient{} t.Cleanup(func() { mockOS.AssertExpectations(t) }) @@ -862,9 +861,8 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { } // Return an offering matching the instance type and duration but with a - // different payment option. matchesPaymentOption filters it out, so the - // page contains no matches and findOfferingID returns a "no offerings - // found" error. + // different payment option. The new guard returns an explicit mismatch error + // rather than silently skipping and exhausting pagination. mockOS.On("DescribeReservedInstanceOfferings", mock.Anything, mock.Anything). Return(&opensearch.DescribeReservedInstanceOfferingsOutput{ ReservedInstanceOfferings: []types.ReservedInstanceOffering{ @@ -872,7 +870,7 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { ReservedInstanceOfferingId: aws.String("other-offering"), InstanceType: types.OpenSearchPartitionInstanceTypeM5XlargeSearch, Duration: 31536000, - PaymentOption: types.ReservedInstancePaymentOptionAllUpfront, // mismatch -- filtered out + PaymentOption: types.ReservedInstancePaymentOptionAllUpfront, // mismatch -- explicit error }, }, }, nil).Once() @@ -880,6 +878,7 @@ func TestFindOfferingID_WrongVariantRejected(t *testing.T) { id, err := client.findOfferingID(context.Background(), rec) assert.Error(t, err) + assert.Contains(t, err.Error(), "payment option") assert.Empty(t, id) } diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index 656720a4c..f71f2c65d 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -432,6 +432,9 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation) // scanRedshiftOfferingPage finds a matching offering in a single page of results. // Returns ("", nil) when no match is found on the page so the caller can continue paginating. +// Returns an error when an offering matches on node type and duration but carries an +// unrecognised ReservedNodeOfferingType -- this surfaces unexpected enum values rather +// than silently skipping them and potentially committing to the wrong offering. func (c *Client) scanRedshiftOfferingPage(offerings []redshifttypes.ReservedNodeOffering, rec common.Recommendation) (string, error) { for _, offering := range offerings { if offering.NodeType == nil || *offering.NodeType != rec.ResourceType { @@ -440,8 +443,10 @@ func (c *Client) scanRedshiftOfferingPage(offerings []redshifttypes.ReservedNode if !c.matchesDuration(offering.Duration, rec.Term) { continue } - if !c.matchesOfferingType(string(offering.ReservedNodeOfferingType), rec.PaymentOption) { - continue + offeringTypeStr := string(offering.ReservedNodeOfferingType) + if offeringTypeStr != "Regular" && offeringTypeStr != "Upgradable" { + return "", fmt.Errorf("Redshift offering %s has unexpected type %q (rec: %s)", + aws.ToString(offering.ReservedNodeOfferingId), offeringTypeStr, rec.ResourceType) } return aws.ToString(offering.ReservedNodeOfferingId), nil } diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index afd9dc4b7..a650a547e 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -924,7 +924,10 @@ func TestClient_FindOfferingID_UnknownOfferingType(t *testing.T) { Term: "1yr", } - // Return offerings with unknown offering type + // Return an offering matching node type and duration but with an unknown + // ReservedNodeOfferingType. scanRedshiftOfferingPage runs the enum guard + // BEFORE matchesOfferingType so the explicit "unexpected type" error + // surfaces (issue #688 CodeRabbit feedback). mockRS.On("DescribeReservedNodeOfferings", mock.Anything, mock.Anything). Return(&redshift.DescribeReservedNodeOfferingsOutput{ ReservedNodeOfferings: []types.ReservedNodeOffering{ @@ -932,7 +935,7 @@ func TestClient_FindOfferingID_UnknownOfferingType(t *testing.T) { ReservedNodeOfferingId: aws.String("offering-123"), NodeType: aws.String("dc2.large"), Duration: aws.Int32(31536000), - ReservedNodeOfferingType: types.ReservedNodeOfferingType("Unknown"), // Invalid type + ReservedNodeOfferingType: types.ReservedNodeOfferingType("Unknown"), // not Regular/Upgradable -- surfaces as error }, }, }, nil).Once() @@ -940,7 +943,7 @@ func TestClient_FindOfferingID_UnknownOfferingType(t *testing.T) { err := client.ValidateOffering(context.Background(), rec) assert.Error(t, err) - assert.Contains(t, err.Error(), "no offerings found") + assert.Contains(t, err.Error(), "unexpected type") mockRS.AssertExpectations(t) } diff --git a/terraform/modules/database/azure/.terraform.lock.hcl b/terraform/modules/database/azure/.terraform.lock.hcl index 93633c169..7a7d90232 100644 --- a/terraform/modules/database/azure/.terraform.lock.hcl +++ b/terraform/modules/database/azure/.terraform.lock.hcl @@ -20,23 +20,3 @@ provider "registry.terraform.io/hashicorp/azurerm" { "zh:f569b65999264a9416862bca5cd2a6177d94ccb0424f3a4ef424428912b9cb3c", ] } - -provider "registry.terraform.io/hashicorp/random" { - version = "3.8.1" - constraints = "~> 3.0" - hashes = [ - "h1:u8AKlWVDTH5r9YLSeswoVEjiY72Rt4/ch7U+61ZDkiQ=", - "zh:08dd03b918c7b55713026037c5400c48af5b9f468f483463321bd18e17b907b4", - "zh:0eee654a5542dc1d41920bbf2419032d6f0d5625b03bd81339e5b33394a3e0ae", - "zh:229665ddf060aa0ed315597908483eee5b818a17d09b6417a0f52fd9405c4f57", - "zh:2469d2e48f28076254a2a3fc327f184914566d9e40c5780b8d96ebf7205f8bc0", - "zh:37d7eb334d9561f335e748280f5535a384a88675af9a9eac439d4cfd663bcb66", - "zh:741101426a2f2c52dee37122f0f4a2f2d6af6d852cb1db634480a86398fa3511", - "zh:78d5eefdd9e494defcb3c68d282b8f96630502cac21d1ea161f53cfe9bb483b3", - "zh:a902473f08ef8df62cfe6116bd6c157070a93f66622384300de235a533e9d4a9", - "zh:b85c511a23e57a2147355932b3b6dce2a11e856b941165793a0c3d7578d94d05", - "zh:c5172226d18eaac95b1daac80172287b69d4ce32750c82ad77fa0768be4ea4b8", - "zh:dab4434dba34aad569b0bc243c2d3f3ff86dd7740def373f2a49816bd2ff819b", - "zh:f49fd62aa8c5525a5c17abd51e27ca5e213881d58882fd42fec4a545b53c9699", - ] -} diff --git a/terraform/modules/frontend/gcp/.terraform.lock.hcl b/terraform/modules/frontend/gcp/.terraform.lock.hcl index 9357c4a88..5380cc687 100644 --- a/terraform/modules/frontend/gcp/.terraform.lock.hcl +++ b/terraform/modules/frontend/gcp/.terraform.lock.hcl @@ -21,26 +21,6 @@ provider "registry.terraform.io/hashicorp/google" { ] } -provider "registry.terraform.io/hashicorp/google-beta" { - version = "5.45.2" - constraints = "~> 5.0" - hashes = [ - "h1:ME/cVZGNln4h166gyo9r7CuunzZ3FEqlIaNyQ0e9yjE=", - "zh:16b77bac5d1555b7f066ba8014f4fc8a6d0de64e252a1988d3fbb400984a4b19", - "zh:1b13f515c4809343840aed8265915cc4191f138bdab5a8c5e1f542fdfc69989f", - "zh:1dcce4309aeab7c88fd36aea664d57e620d8a413b967ce513a5a866e8de901f2", - "zh:24db65d7929f2a731e9cac1750c569cb4528b312ef182a5e2e8c0cf008d8a71b", - "zh:28c0b9e68d97570f03b2c4770607701580055bcba50069efd145954aa13b23e4", - "zh:3a898a1ad1569f6486a2bc20014087284c8cab919bc8f155833de5128ccd12eb", - "zh:4eed99cfb9daada70f813f2cedcf490d3097de1ccb9b391fc451ecc46509c067", - "zh:888c4cb1f13b23674ba1091835dd3f1bff5d8e7729ef302183d8d01233819e54", - "zh:8baae3b949f6e9505425f5fa4785de786e9cedc4c3f3ad906d8ed560bd2e39c6", - "zh:cf2c8928b764592fa2cd14a9f109d01cd0a92049a4fca9d0a74cf2fe588364e2", - "zh:edff09394f5bd0b278a4adc800a31b7f150249a1ea92ca273ccf4acd25be3f63", - "zh:f569b65999264a9416862bca5cd2a6177d94ccb0424f3a4ef424428912b9cb3c", - ] -} - provider "registry.terraform.io/hashicorp/tls" { version = "4.2.1" constraints = "~> 4.0" From 5f077449512a182ab3c6f3821f75c3526c1a3e43 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 23:56:05 +0200 Subject: [PATCH 5/5] chore: restore Terraform lock files to feat branch state The previous commit accidentally included Terraform provider constraint changes unrelated to the #688 offering-lookup fix. Restore the three lock files to their state on feat/multicloud-web-frontend so this branch stays focused on the purchase-path narrowing changes only. --- iac/federation/azure-target/terraform/.terraform.lock.hcl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/iac/federation/azure-target/terraform/.terraform.lock.hcl b/iac/federation/azure-target/terraform/.terraform.lock.hcl index 22380405f..fe2e1f974 100644 --- a/iac/federation/azure-target/terraform/.terraform.lock.hcl +++ b/iac/federation/azure-target/terraform/.terraform.lock.hcl @@ -3,7 +3,7 @@ provider "registry.terraform.io/hashicorp/azuread" { version = "3.8.0" - constraints = "~> 3.8" + constraints = ">= 2.47.0" hashes = [ "h1:E2YWNE3Qry4bQMlmmZ33X4hLY5hOGrEZrlRg4anI2uw=", "zh:0d26cfbf9417acd1c2295ccd5b0052abeac85ad1c3f6422ff09bf6a1ce16f00d", @@ -23,7 +23,7 @@ provider "registry.terraform.io/hashicorp/azuread" { provider "registry.terraform.io/hashicorp/azurerm" { version = "4.67.0" - constraints = "~> 4.0" + constraints = ">= 3.95.0" hashes = [ "h1:cT/1xU2vHvVboTZvjNjn7sqCH/FPP//JeMh702eGUS0=", "zh:49a1531297510b684103c06d32e9ef1d810380ed55895f6b799913e9679141dc",