fix(aws/elasticache): guard the offering lookup against a missing or wrong engine

CHANGELOG.md

@@ -215,6 +215,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

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"

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"),
 				},
 			},

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")
+		})
+	}
+}

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"},
 	} {