From dd9373765027d2817cbe0553e1d341ff51468408 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 21 Oct 2026 00:56:22 +0200 Subject: [PATCH] fix(aws/elasticache): guard the offering lookup against a missing or wrong engine findOfferingID sent the Cost Explorer ProductDescription verbatim as the engine filter, including an empty value, and the page scan checked only the payment option of the first offering, so an offering for a different engine, node type or term could be bought. Refuse an empty engine with an error naming the missing field, send the engine lowercased as ElastiCache reports it, and fail on a first offering whose node type, duration or engine differs from the request. Other #53 items are not covered here. Part of #53 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 + providers/aws/services/elasticache/client.go | 58 +++++++++--- .../aws/services/elasticache/client_test.go | 2 + .../elasticache/offering_guard_test.go | 91 +++++++++++++++++++ .../elasticache/offering_price_test.go | 2 +- 5 files changed, 142 insertions(+), 15 deletions(-) create mode 100644 providers/aws/services/elasticache/offering_guard_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 5f448ea0..dd1633cc 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -216,6 +216,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- AWS ElastiCache purchases refuse a recommendation with no engine instead of + sending an empty `ProductDescription` filter, send the engine lowercased as + ElastiCache reports it, and verify the chosen offering's node type, duration + and engine before buying it (#53). - **`ReshapeBuffer` rejects nil payment caps** instead of mapping them to `math.MaxFloat64`: `MaxPaymentPerExchangeUSD` and `MaxPaymentDailyUSD` are now required, so an omitted cap can no longer mean unlimited spend on diff --git a/providers/aws/services/elasticache/client.go b/providers/aws/services/elasticache/client.go index 4712ef07..66df52a8 100644 --- a/providers/aws/services/elasticache/client.go +++ b/providers/aws/services/elasticache/client.go @@ -7,6 +7,8 @@ import ( "fmt" "log" "sort" + "strconv" + "strings" "time" "github.com/aws/aws-sdk-go-v2/aws" @@ -280,21 +282,25 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, if err != nil { return "", err } - return c.paginateElastiCacheOfferings(ctx, rec, details, offeringType, execID) + engine := strings.ToLower(strings.TrimSpace(details.Engine)) + if engine == "" { + return "", fmt.Errorf("ElastiCache recommendation for %s has no engine (Cost Explorer omitted ProductDescription); refusing to guess between redis, valkey and memcached", rec.ResourceType) + } + return c.paginateElastiCacheOfferings(ctx, rec, engine, offeringType, execID) } // 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, offeringType, execID string) (string, error) { - duration, err := c.getDurationString(rec.Term) +func (c *Client) paginateElastiCacheOfferings(ctx context.Context, rec common.Recommendation, engine, offeringType, execID string) (string, error) { + duration, wantSeconds, err := c.getDurationSeconds(rec.Term) if err != nil { return "", err } tag := purchasecfg.ResolveTag(execID) t0 := time.Now() log.Printf("purchase[%s]: ElastiCache findOfferingID starting (nodeType=%s engine=%s duration=%s payment=%s)", - tag, rec.ResourceType, details.Engine, duration, offeringType) + tag, rec.ResourceType, engine, duration, offeringType) var marker *string page := 0 @@ -305,11 +311,11 @@ func (c *Client) paginateElastiCacheOfferings(ctx context.Context, rec common.Re 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) + maxOfferingPages, rec.ResourceType, engine, rec.PaymentOption) } input := &elasticache.DescribeReservedCacheNodesOfferingsInput{ CacheNodeType: aws.String(rec.ResourceType), - ProductDescription: aws.String(details.Engine), + ProductDescription: aws.String(engine), Duration: aws.String(duration), OfferingType: aws.String(offeringType), MaxRecords: aws.Int32(100), @@ -324,7 +330,7 @@ func (c *Client) paginateElastiCacheOfferings(ctx context.Context, rec common.Re } log.Printf("purchase[%s]: ElastiCache findOfferingID page %d: %d offerings in %s", tag, page, len(result.ReservedCacheNodesOfferings), time.Since(pageStart)) - if id, scanErr := scanElastiCacheOfferingPage(result.ReservedCacheNodesOfferings, rec, offeringType); scanErr != nil { + if id, scanErr := scanElastiCacheOfferingPage(result.ReservedCacheNodesOfferings, rec, offeringType, engine, wantSeconds); scanErr != nil { return "", scanErr } else if id != "" { log.Printf("purchase[%s]: ElastiCache findOfferingID found match on page %d after %s total", @@ -339,26 +345,36 @@ func (c *Client) paginateElastiCacheOfferings(ctx context.Context, rec common.Re log.Printf("purchase[%s]: ElastiCache findOfferingID exhausted %d page(s) in %s -- no match", tag, page, time.Since(t0)) return "", fmt.Errorf("no offerings found for ElastiCache %s %s %s after %d page(s) (issue #688)", - rec.ResourceType, details.Engine, rec.PaymentOption, page) + rec.ResourceType, engine, rec.PaymentOption, page) } // 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) { +func scanElastiCacheOfferingPage(offerings []types.ReservedCacheNodesOffering, rec common.Recommendation, wantType, wantEngine string, wantSeconds int32) (string, error) { if len(offerings) == 0 { return "", nil } // The DescribeReservedCacheNodesOfferings call already filters server-side - // by offering type, so only the first result is examined; a mismatch here - // indicates an AWS API-side anomaly worth a hard error, not a scan target. + // by node type, engine, duration and offering type, so only the first + // result is examined; a mismatch on any of them indicates an AWS API-side + // anomaly worth a hard error, not a scan target. o := &offerings[0] + id := aws.ToString(o.ReservedCacheNodesOfferingId) 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) + id, got, wantType, rec.ResourceType, rec.PaymentOption) + } + if gotNode := aws.ToString(o.CacheNodeType); gotNode != rec.ResourceType { + return "", fmt.Errorf("ElastiCache offering %s has node type %q, want %q -- API filter mismatch", id, gotNode, rec.ResourceType) + } + if gotSeconds := aws.ToInt32(o.Duration); gotSeconds != wantSeconds { + return "", fmt.Errorf("ElastiCache offering %s has duration %ds, want %ds -- API filter mismatch", id, gotSeconds, wantSeconds) } - return aws.ToString(o.ReservedCacheNodesOfferingId), nil + if gotEngine := aws.ToString(o.ProductDescription); !strings.EqualFold(gotEngine, wantEngine) { + return "", fmt.Errorf("ElastiCache offering %s has engine %q, want %q -- API filter mismatch", id, gotEngine, wantEngine) + } + return id, nil } // ValidateOffering checks if an offering exists without purchasing. @@ -482,6 +498,20 @@ func (c *Client) getDurationString(term string) (string, error) { return purchasecfg.DurationSecondsString(term, "ElastiCache") } +// getDurationSeconds returns the request's duration string and the same value +// as the int32 an offering reports. +func (c *Client) getDurationSeconds(term string) (duration string, seconds int32, err error) { + duration, err = c.getDurationString(term) + if err != nil { + return "", 0, err + } + parsed, err := strconv.ParseInt(duration, 10, 32) + if err != nil { + return "", 0, fmt.Errorf("ElastiCache duration %q is not a number of seconds: %w", duration, err) + } + return duration, int32(parsed), nil +} + // 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" diff --git a/providers/aws/services/elasticache/client_test.go b/providers/aws/services/elasticache/client_test.go index 0114a5ab..b299a11f 100644 --- a/providers/aws/services/elasticache/client_test.go +++ b/providers/aws/services/elasticache/client_test.go @@ -806,6 +806,8 @@ func TestFindOfferingID_HappyPath(t *testing.T) { { ReservedCacheNodesOfferingId: aws.String("offering-ok"), CacheNodeType: aws.String("cache.r6g.large"), + Duration: aws.Int32(31536000), + ProductDescription: aws.String("redis"), OfferingType: aws.String("No Upfront"), }, }, diff --git a/providers/aws/services/elasticache/offering_guard_test.go b/providers/aws/services/elasticache/offering_guard_test.go new file mode 100644 index 00000000..4be5b869 --- /dev/null +++ b/providers/aws/services/elasticache/offering_guard_test.go @@ -0,0 +1,91 @@ +package elasticache + +import ( + "context" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/elasticache" + "github.com/aws/aws-sdk-go-v2/service/elasticache/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func guardRec(engine string) common.Recommendation { + return common.Recommendation{ + ResourceType: "cache.r6g.large", PaymentOption: "no-upfront", Term: "1yr", + Details: &common.CacheDetails{Engine: engine}, + } +} + +func guardOffering(mut func(*types.ReservedCacheNodesOffering)) types.ReservedCacheNodesOffering { + o := types.ReservedCacheNodesOffering{ + ReservedCacheNodesOfferingId: aws.String("off"), + CacheNodeType: aws.String("cache.r6g.large"), + Duration: aws.Int32(31536000), + ProductDescription: aws.String("redis"), + OfferingType: aws.String("No Upfront"), + } + if mut != nil { + mut(&o) + } + return o +} + +// CE can omit ProductDescription; the lookup must name the missing field +// instead of sending an empty engine filter. +func TestFindOfferingID_EmptyEngineRefusedBeforeAPICall(t *testing.T) { + t.Parallel() + for _, engine := range []string{"", " "} { + mockEC := &MockElastiCacheClient{} + client := &Client{client: mockEC, region: "us-east-1"} + _, err := client.findOfferingID(context.Background(), guardRec(engine), "") + require.ErrorContains(t, err, "no engine", "engine %q", engine) + mockEC.AssertNotCalled(t, "DescribeReservedCacheNodesOfferings", mock.Anything, mock.Anything) + } +} + +// ElastiCache reports lowercase engines; a title-cased CE value must be sent +// lowercased and still match the lowercase offering. +func TestFindOfferingID_EngineFilterIsLowercased(t *testing.T) { + t.Parallel() + mockEC := &MockElastiCacheClient{} + client := &Client{client: mockEC, region: "us-east-1"} + mockEC.On("DescribeReservedCacheNodesOfferings", mock.Anything, mock.MatchedBy(func(in *elasticache.DescribeReservedCacheNodesOfferingsInput) bool { + return aws.ToString(in.ProductDescription) == "redis" + })).Return(&elasticache.DescribeReservedCacheNodesOfferingsOutput{ + ReservedCacheNodesOfferings: []types.ReservedCacheNodesOffering{guardOffering(nil)}, + }, nil).Once() + id, err := client.findOfferingID(context.Background(), guardRec(" Redis "), "") + require.NoError(t, err) + assert.Equal(t, "off", id) + mockEC.AssertExpectations(t) +} + +// The request filters these fields, but an offering that differs on any of +// them must not be bought just because it is first on the page. +func TestFindOfferingID_RejectsOfferingForWrongNodeDurationOrEngine(t *testing.T) { + t.Parallel() + for name, mut := range map[string]func(*types.ReservedCacheNodesOffering){ + "node type": func(o *types.ReservedCacheNodesOffering) { o.CacheNodeType = aws.String("cache.m6g.large") }, + "duration": func(o *types.ReservedCacheNodesOffering) { o.Duration = aws.Int32(94608000) }, + "engine": func(o *types.ReservedCacheNodesOffering) { o.ProductDescription = aws.String("memcached") }, + "no engine": func(o *types.ReservedCacheNodesOffering) { o.ProductDescription = nil }, + "no duration": func(o *types.ReservedCacheNodesOffering) { o.Duration = nil }, + } { + t.Run(name, func(t *testing.T) { + t.Parallel() + mockEC := &MockElastiCacheClient{} + client := &Client{client: mockEC, region: "us-east-1"} + mockEC.On("DescribeReservedCacheNodesOfferings", mock.Anything, mock.Anything). + Return(&elasticache.DescribeReservedCacheNodesOfferingsOutput{ + ReservedCacheNodesOfferings: []types.ReservedCacheNodesOffering{guardOffering(mut)}, + }, nil).Once() + _, err := client.findOfferingID(context.Background(), guardRec("redis"), "") + require.ErrorContains(t, err, "API filter mismatch") + }) + } +} diff --git a/providers/aws/services/elasticache/offering_price_test.go b/providers/aws/services/elasticache/offering_price_test.go index b77f746a..456dde29 100644 --- a/providers/aws/services/elasticache/offering_price_test.go +++ b/providers/aws/services/elasticache/offering_price_test.go @@ -42,7 +42,7 @@ func TestGetOfferingDetails_Pricing(t *testing.T) { {name: "3yr term", term: "3yr", duration: threeYear, usage: 0, charges: []float64{0.05}, currency: "USD", region: "", err: "", hourly: 0.05, hours: 26280}, {name: "usage only, no charges", term: "1yr", duration: oneYear, usage: 0.05, charges: nil, currency: "USD", region: "", err: "", hourly: 0.05, hours: 8760}, {name: "two hourly charges are summed", term: "1yr", duration: oneYear, usage: 0, charges: []float64{0.03, 0.02}, currency: "USD", region: "", err: "", hourly: 0.05, hours: 8760}, - {name: "3yr offering vs 1yr request", term: "1yr", duration: threeYear, usage: 0, charges: []float64{0.05}, currency: "USD", region: "", err: "does not match", hourly: 0, hours: 0}, + {name: "3yr offering vs 1yr request", term: "1yr", duration: threeYear, usage: 0, charges: []float64{0.05}, currency: "USD", region: "", err: "API filter mismatch", hourly: 0, hours: 0}, {name: "usage and charge both nonzero", term: "1yr", duration: oneYear, usage: 0.05, charges: []float64{0.05}, currency: "USD", region: "", err: "refusing to guess", hourly: 0, hours: 0}, {name: "cn region has no assumable currency", term: "1yr", duration: oneYear, region: "cn-north-1", err: "not billed in USD"}, } {