fix(providers/aws): fail loud on unrecognized RI term strings

providers/aws/services/elasticache/client.go

@@ -286,7 +286,10 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation,
 // 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 := c.getDurationString(rec.Term)
+	duration, err := c.getDurationString(rec.Term)
+	if err != nil {
+		return "", err
+	}
 	tag := execID
 	if tag == "" {
 		tag = "no-exec"
@@ -440,12 +443,19 @@ const (
 	ThreeYearSeconds = 94608000 // 3 * 365 days in seconds
 )
 
-// getDurationString converts term string to duration string
-func (c *Client) getDurationString(term string) string {
-	if term == "3yr" || term == "3" {
-		return fmt.Sprintf("%d", ThreeYearSeconds)
+// getDurationString converts a term string to the duration string the
+// ElastiCache API expects. Returns an error on any unrecognized or empty
+// input so callers fail loud rather than silently buying a 1-year
+// reservation when another commitment length was intended.
+func (c *Client) getDurationString(term string) (string, error) {
+	switch term {
+	case "3yr", "3":
+		return fmt.Sprintf("%d", ThreeYearSeconds), nil
+	case "1yr", "1":
+		return fmt.Sprintf("%d", OneYearSeconds), nil
+	default:
+		return "", fmt.Errorf("unsupported ElastiCache reservation term %q: must be one of 1yr, 1, 3yr, 3", term)
 	}
-	return fmt.Sprintf("%d", OneYearSeconds)
 }
 
 // convertPaymentOption converts payment option to AWS string.

providers/aws/services/elasticache/client_test.go

@@ -372,19 +372,35 @@ func TestClient_GetDurationString(t *testing.T) {
 	client := &Client{}
 
 	tests := []struct {
-		name     string
-		term     string
-		expected string
+		name      string
+		term      string
+		expected  string
+		expectErr bool
 	}{
-		{"1 year", "1yr", "31536000"},
-		{"3 years", "3yr", "94608000"},
-		{"3 numeric", "3", "94608000"},
-		{"default for invalid", "invalid", "31536000"},
+		{"1 year", "1yr", "31536000", false},
+		{"1 numeric", "1", "31536000", false},
+		{"3 years", "3yr", "94608000", false},
+		{"3 numeric", "3", "94608000", false},
+		// Regression for ARCH-04 (issue #1192): unrecognized or empty terms
+		// must error instead of silently mapping to a 1-year purchase. An
+		// empty term is what a 0/NULL Term DB row produces on the scheduler
+		// purchase path.
+		{"invalid term errors", "invalid", "", true},
+		{"empty term errors", "", "", true},
+		{"zero term errors", "0", "", true},
+		{"2yr term errors", "2yr", "", true},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := client.getDurationString(tt.term)
+			result, err := client.getDurationString(tt.term)
+			if tt.expectErr {
+				if assert.Error(t, err) {
+					assert.Contains(t, err.Error(), "unsupported ElastiCache reservation term")
+				}
+				return
+			}
+			assert.NoError(t, err)
 			assert.Equal(t, tt.expected, result)
 		})
 	}
@@ -680,3 +696,28 @@ func TestFindOfferingID_HappyPath(t *testing.T) {
 	assert.NoError(t, err)
 	assert.Equal(t, "offering-ok", id)
 }
+
+// TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall is the ARCH-04 (issue
+// #1192) call-path regression test: an unrecognized or empty term must abort
+// the offering lookup before any DescribeReservedCacheNodesOfferings call,
+// rather than silently matching (and buying) a 1-year offering. A "0" term is
+// what a 0/NULL Term DB row produces on the scheduler purchase path.
+func TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall(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:          "0",
+		Details:       &common.CacheDetails{Engine: "redis"},
+	}
+
+	_, err := client.findOfferingID(context.Background(), rec, "")
+
+	if assert.Error(t, err, "findOfferingID must error on an unrecognized term (ARCH-04)") {
+		assert.Contains(t, err.Error(), "unsupported ElastiCache reservation term")
+	}
+	mockEC.AssertNotCalled(t, "DescribeReservedCacheNodesOfferings", mock.Anything, mock.Anything)
+}

providers/aws/services/memorydb/client.go

@@ -288,7 +288,10 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation,
 		return "", err
 	}
 
-	duration := c.getDurationStringForAPI(rec.Term)
+	duration, err := c.getDurationStringForAPI(rec.Term)
+	if err != nil {
+		return "", err
+	}
 	tag := execID
 	if tag == "" {
 		tag = "no-exec"
@@ -380,11 +383,19 @@ func isLastMemoryDBPage(nextToken *string) bool {
 // 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"
+// Returns an error on any unrecognized or empty input so callers fail loud
+// rather than silently buying a 1-year reservation when another commitment
+// length was intended. The month-count forms ("12", "36") were accepted by the
+// previous implementation and are kept for compatibility.
+func (c *Client) getDurationStringForAPI(term string) (string, error) {
+	switch term {
+	case "3yr", "3", "36":
+		return "3yr", nil
+	case "1yr", "1", "12":
+		return "1yr", nil
+	default:
+		return "", fmt.Errorf("unsupported MemoryDB reservation term %q: must be one of 1yr, 1, 12, 3yr, 3, 36", term)
 	}
-	return "1yr"
 }
 
 // ValidateOffering checks if an offering exists without purchasing

providers/aws/services/memorydb/client_test.go

@@ -877,3 +877,67 @@ func TestClient_PurchaseCommitment_NoToken_RichReservationName(t *testing.T) {
 	assert.Contains(t, capturedID, "3x-1yr", "count and term must be embedded: %q", capturedID)
 	assert.LessOrEqual(t, len(capturedID), 60, "must fit AWS reservation-ID cap")
 }
+
+func TestClient_GetDurationStringForAPI(t *testing.T) {
+	client := &Client{}
+
+	tests := []struct {
+		name      string
+		term      string
+		expected  string
+		expectErr bool
+	}{
+		{"1 year", "1yr", "1yr", false},
+		{"1 numeric", "1", "1yr", false},
+		{"12 months", "12", "1yr", false},
+		{"3 years", "3yr", "3yr", false},
+		{"3 numeric", "3", "3yr", false},
+		{"36 months", "36", "3yr", false},
+		// Regression for ARCH-04 (issue #1192): unrecognized or empty terms
+		// must error instead of silently mapping to a 1-year purchase. An
+		// empty term is what a 0/NULL Term DB row produces on the scheduler
+		// purchase path.
+		{"invalid term errors", "invalid", "", true},
+		{"empty term errors", "", "", true},
+		{"zero term errors", "0", "", true},
+		{"2yr term errors", "2yr", "", true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := client.getDurationStringForAPI(tt.term)
+			if tt.expectErr {
+				if assert.Error(t, err) {
+					assert.Contains(t, err.Error(), "unsupported MemoryDB reservation term")
+				}
+				return
+			}
+			assert.NoError(t, err)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
+// TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall is the ARCH-04 (issue
+// #1192) call-path regression test: an unrecognized or empty term must abort
+// the offering lookup before any DescribeReservedNodesOfferings call, rather
+// than silently matching (and buying) a 1-year offering. A "0" term is what a
+// 0/NULL Term DB row produces on the scheduler purchase path.
+func TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall(t *testing.T) {
+	mockMDB := &MockMemoryDBClient{}
+	t.Cleanup(func() { mockMDB.AssertExpectations(t) })
+	client := &Client{client: mockMDB, region: "us-east-1"}
+
+	rec := common.Recommendation{
+		ResourceType:  "db.r6g.large",
+		PaymentOption: "no-upfront",
+		Term:          "0",
+	}
+
+	_, err := client.findOfferingID(context.Background(), rec, "")
+
+	if assert.Error(t, err, "findOfferingID must error on an unrecognized term (ARCH-04)") {
+		assert.Contains(t, err.Error(), "unsupported MemoryDB reservation term")
+	}
+	mockMDB.AssertNotCalled(t, "DescribeReservedNodesOfferings", mock.Anything, mock.Anything)
+}

providers/aws/services/opensearch/client.go

@@ -367,6 +367,10 @@ const maxOfferingPages = 5
 // execID is the purchase execution UUID for log correlation; pass "" when
 // calling outside of a purchase flow (ValidateOffering, GetOfferingDetails).
 func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, execID string) (string, error) {
+	requiredMonths, err := requiredMonthsForTerm(rec.Term)
+	if err != nil {
+		return "", err
+	}
 	tag := execID
 	if tag == "" {
 		tag = "no-exec"
@@ -403,7 +407,7 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation,
 		log.Printf("purchase[%s]: OpenSearch findOfferingID page %d: %d offerings in %s",
 			tag, page, len(result.ReservedInstanceOfferings), time.Since(pageStart))
 
-		if id, scanErr := c.scanOpenSearchOfferingPage(result.ReservedInstanceOfferings, rec); scanErr != nil {
+		if id, scanErr := c.scanOpenSearchOfferingPage(result.ReservedInstanceOfferings, rec, requiredMonths); scanErr != nil {
 			return "", scanErr
 		} else if id != "" {
 			log.Printf("purchase[%s]: OpenSearch findOfferingID found match on page %d after %s total",
@@ -427,13 +431,13 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation,
 // 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) {
+func (c *Client) scanOpenSearchOfferingPage(offerings []types.ReservedInstanceOffering, rec common.Recommendation, requiredMonths int) (string, error) {
 	wantPayment := normalizeOpenSearchPaymentOption(rec.PaymentOption)
 	for _, offering := range offerings {
 		if string(offering.InstanceType) != rec.ResourceType {
 			continue
 		}
-		if !c.matchesDuration(offering.Duration, rec.Term) {
+		if !c.matchesDuration(offering.Duration, requiredMonths) {
 			continue
 		}
 		gotPayment := string(offering.PaymentOption)
@@ -476,13 +480,25 @@ func (c *Client) matchesPaymentOption(offeringOption types.ReservedInstancePayme
 	}
 }
 
-// matchesDuration checks if the offering duration matches
-func (c *Client) matchesDuration(offeringDuration int32, term string) bool {
-	offeringMonths := offeringDuration / 2592000 // 30 days in seconds
-	requiredMonths := 12
-	if term == "3yr" || term == "3" {
-		requiredMonths = 36
+// requiredMonthsForTerm converts a reservation term string to the offering
+// duration in months. Returns an error on any unrecognized or empty input so
+// callers fail loud rather than silently matching (and buying) a 1-year
+// offering when another commitment length was intended.
+func requiredMonthsForTerm(term string) (int, error) {
+	switch term {
+	case "3yr", "3":
+		return 36, nil
+	case "1yr", "1":
+		return 12, nil
+	default:
+		return 0, fmt.Errorf("unsupported OpenSearch reservation term %q: must be one of 1yr, 1, 3yr, 3", term)
 	}
+}
+
+// matchesDuration checks if the offering duration matches the required term
+// length in months (as produced by requiredMonthsForTerm).
+func (c *Client) matchesDuration(offeringDuration int32, requiredMonths int) bool {
+	offeringMonths := offeringDuration / 2592000 // 30 days in seconds
 	return int(offeringMonths) >= requiredMonths-1 && int(offeringMonths) <= requiredMonths+1
 }
 

providers/aws/services/opensearch/client_test.go

@@ -257,19 +257,54 @@ func TestClient_MatchesDuration(t *testing.T) {
 	tests := []struct {
 		name             string
 		offeringDuration int32
-		term             string
+		requiredMonths   int
 		expected         bool
 	}{
-		{"1 year match", 31536000, "1yr", true},
-		{"3 years match", 94608000, "3yr", true},
-		{"3 numeric term", 94608000, "3", true},
-		{"no match", 31536000, "3yr", false},
-		{"zero duration", 0, "1yr", false},
+		{"1 year match", 31536000, 12, true},
+		{"3 years match", 94608000, 36, true},
+		{"no match", 31536000, 36, false},
+		{"zero duration", 0, 12, false},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
-			result := client.matchesDuration(tt.offeringDuration, tt.term)
+			result := client.matchesDuration(tt.offeringDuration, tt.requiredMonths)
+			assert.Equal(t, tt.expected, result)
+		})
+	}
+}
+
+func TestRequiredMonthsForTerm(t *testing.T) {
+	tests := []struct {
+		name      string
+		term      string
+		expected  int
+		expectErr bool
+	}{
+		{"1 year", "1yr", 12, false},
+		{"1 numeric", "1", 12, false},
+		{"3 years", "3yr", 36, false},
+		{"3 numeric", "3", 36, false},
+		// Regression for ARCH-04 (issue #1192): unrecognized or empty terms
+		// must error instead of silently matching a 1-year offering. An empty
+		// term is what a 0/NULL Term DB row produces on the scheduler
+		// purchase path.
+		{"invalid term errors", "invalid", 0, true},
+		{"empty term errors", "", 0, true},
+		{"zero term errors", "0", 0, true},
+		{"2yr term errors", "2yr", 0, true},
+	}
+
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			result, err := requiredMonthsForTerm(tt.term)
+			if tt.expectErr {
+				if assert.Error(t, err) {
+					assert.Contains(t, err.Error(), "unsupported OpenSearch reservation term")
+				}
+				return
+			}
+			assert.NoError(t, err)
 			assert.Equal(t, tt.expected, result)
 		})
 	}
@@ -949,3 +984,27 @@ func TestClient_PurchaseCommitment_NoToken_RichReservationName(t *testing.T) {
 	assert.Contains(t, capturedName, "2x-3yr", "count and term must be embedded: %q", capturedName)
 	assert.LessOrEqual(t, len(capturedName), 60, "must fit AWS reservation-ID cap")
 }
+
+// TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall is the ARCH-04 (issue
+// #1192) call-path regression test: an unrecognized or empty term must abort
+// the offering lookup before any DescribeReservedInstanceOfferings call, rather
+// than silently matching (and buying) a 1-year offering. A "0" term is what a
+// 0/NULL Term DB row produces on the scheduler purchase path.
+func TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall(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:          "0",
+	}
+
+	_, err := client.findOfferingID(context.Background(), rec, "")
+
+	if assert.Error(t, err, "findOfferingID must error on an unrecognized term (ARCH-04)") {
+		assert.Contains(t, err.Error(), "unsupported OpenSearch reservation term")
+	}
+	mockOS.AssertNotCalled(t, "DescribeReservedInstanceOfferings", mock.Anything, mock.Anything)
+}