diff --git a/CHANGELOG.md b/CHANGELOG.md index 012bb03b..0fb5fe87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -216,6 +216,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed - Bump Go to 1.26.10 to pick up the fix for GO-2026-6809 (crypto/x509), which `govulncheck` now reports against 1.26.9. +- 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"}, } {