From bd8168aab8ec3e71adb2a083cdb986e0bfb290c9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 10 Jun 2026 12:25:52 -0700 Subject: [PATCH 1/3] fix(providers/aws): fail loud on unrecognized RI term strings The term converters in the elasticache, memorydb, rds, redshift, and opensearch RI clients silently mapped any unrecognized or empty term to a 1-year duration before selecting the offering PurchaseCommitment buys. A malformed or NULL term reaching the purchase path (for example a 0 Term row executed by the scheduler, which does not re-validate) would silently buy a 1-year reservation when 3-year was intended. Mirror the savingsplans client fix: converters now return an explicit error for any term outside the supported set, and the error propagates from the offering lookup before any API call. Redshift and OpenSearch validate the term once up front via requiredMonthsForTerm and thread the resolved month count into matchesDuration. Regression tests cover empty, "0", "2yr", and garbage terms in all five packages. Closes #1192 --- providers/aws/services/elasticache/client.go | 22 ++++++--- .../aws/services/elasticache/client_test.go | 31 ++++++++---- providers/aws/services/memorydb/client.go | 21 ++++++-- .../aws/services/memorydb/client_test.go | 39 +++++++++++++++ providers/aws/services/opensearch/client.go | 34 +++++++++---- .../aws/services/opensearch/client_test.go | 48 ++++++++++++++++--- providers/aws/services/rds/client.go | 22 ++++++--- providers/aws/services/rds/client_test.go | 31 ++++++++---- providers/aws/services/redshift/client.go | 34 +++++++++---- .../aws/services/redshift/client_test.go | 48 ++++++++++++++++--- 10 files changed, 265 insertions(+), 65 deletions(-) diff --git a/providers/aws/services/elasticache/client.go b/providers/aws/services/elasticache/client.go index 987e32e96..250b9347c 100644 --- a/providers/aws/services/elasticache/client.go +++ b/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. diff --git a/providers/aws/services/elasticache/client_test.go b/providers/aws/services/elasticache/client_test.go index 69db4d4d9..57982fa43 100644 --- a/providers/aws/services/elasticache/client_test.go +++ b/providers/aws/services/elasticache/client_test.go @@ -372,19 +372,34 @@ 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 { + assert.Error(t, err) + assert.Contains(t, err.Error(), "unsupported ElastiCache reservation term") + return + } + assert.NoError(t, err) assert.Equal(t, tt.expected, result) }) } diff --git a/providers/aws/services/memorydb/client.go b/providers/aws/services/memorydb/client.go index 2e4542fe1..918a48c46 100644 --- a/providers/aws/services/memorydb/client.go +++ b/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 diff --git a/providers/aws/services/memorydb/client_test.go b/providers/aws/services/memorydb/client_test.go index 769954e81..2ff942668 100644 --- a/providers/aws/services/memorydb/client_test.go +++ b/providers/aws/services/memorydb/client_test.go @@ -877,3 +877,42 @@ 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 { + assert.Error(t, err) + assert.Contains(t, err.Error(), "unsupported MemoryDB reservation term") + return + } + assert.NoError(t, err) + assert.Equal(t, tt.expected, result) + }) + } +} diff --git a/providers/aws/services/opensearch/client.go b/providers/aws/services/opensearch/client.go index e4071aedf..e599cde12 100644 --- a/providers/aws/services/opensearch/client.go +++ b/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 } diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index 8edcb4a05..915d489b1 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -257,19 +257,53 @@ 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 { + assert.Error(t, err) + assert.Contains(t, err.Error(), "unsupported OpenSearch reservation term") + return + } + assert.NoError(t, err) assert.Equal(t, tt.expected, result) }) } diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index 58e16a3dd..c0f7950a2 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -374,7 +374,10 @@ func (c *Client) paginateRDSOfferings(ctx context.Context, rec common.Recommenda if err != nil { return "", fmt.Errorf("cannot look up RDS offering: %w", err) } - duration := c.getDurationString(rec.Term) + duration, err := c.getDurationString(rec.Term) + if err != nil { + return "", err + } tag := execID if tag == "" { @@ -528,12 +531,19 @@ const ( ThreeYearSeconds = 94608000 // 3 * 365 days in seconds ) -// getDurationString converts term string to duration string for RDS API -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 RDS +// 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 RDS reservation term %q: must be one of 1yr, 1, 3yr, 3", term) } - return fmt.Sprintf("%d", OneYearSeconds) } // convertPaymentOption converts payment option to AWS string diff --git a/providers/aws/services/rds/client_test.go b/providers/aws/services/rds/client_test.go index 6c4f4bb74..e190c191c 100644 --- a/providers/aws/services/rds/client_test.go +++ b/providers/aws/services/rds/client_test.go @@ -561,19 +561,34 @@ 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", "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 { + assert.Error(t, err) + assert.Contains(t, err.Error(), "unsupported RDS reservation term") + return + } + assert.NoError(t, err) assert.Equal(t, tt.expected, result) }) } diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index a7b49b864..65a96b847 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -412,6 +412,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" @@ -448,7 +452,7 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, log.Printf("purchase[%s]: Redshift findOfferingID page %d: %d offerings in %s", tag, page, len(result.ReservedNodeOfferings), time.Since(pageStart)) - if id, scanErr := c.scanRedshiftOfferingPage(result.ReservedNodeOfferings, rec); scanErr != nil { + if id, scanErr := c.scanRedshiftOfferingPage(result.ReservedNodeOfferings, rec, requiredMonths); scanErr != nil { return "", scanErr } else if id != "" { log.Printf("purchase[%s]: Redshift findOfferingID found match on page %d after %s total", @@ -480,12 +484,12 @@ func (c *Client) findOfferingID(ctx context.Context, rec common.Recommendation, // expressed through the offering's FixedPrice (upfront) and recurring charge, // so an offering whose price shape does not match the operator's chosen payment // option is skipped rather than purchased on the wrong terms. -func (c *Client) scanRedshiftOfferingPage(offerings []redshifttypes.ReservedNodeOffering, rec common.Recommendation) (string, error) { +func (c *Client) scanRedshiftOfferingPage(offerings []redshifttypes.ReservedNodeOffering, rec common.Recommendation, requiredMonths int) (string, error) { for _, offering := range offerings { if offering.NodeType == nil || *offering.NodeType != rec.ResourceType { continue } - if !c.matchesDuration(offering.Duration, rec.Term) { + if !c.matchesDuration(offering.Duration, requiredMonths) { continue } offeringTypeStr := string(offering.ReservedNodeOfferingType) @@ -545,17 +549,29 @@ func matchesPaymentOption(offering redshifttypes.ReservedNodeOffering, paymentOp } } -// matchesDuration checks if the offering duration matches -func (c *Client) matchesDuration(offeringDuration *int32, term string) bool { +// 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 Redshift 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 { if offeringDuration == nil { return false } offeringMonths := *offeringDuration / 2592000 - requiredMonths := 12 - if term == "3yr" || term == "3" { - requiredMonths = 36 - } return int(offeringMonths) == requiredMonths } diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index 9920f575b..b0ab9dc15 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -463,19 +463,53 @@ func TestClient_MatchesDuration(t *testing.T) { tests := []struct { name string offeringDuration *int32 - term string + requiredMonths int expected bool }{ - {"1 year match", aws.Int32(31536000), "1yr", true}, - {"3 years match", aws.Int32(94608000), "3yr", true}, - {"3 numeric term", aws.Int32(94608000), "3", true}, - {"no match", aws.Int32(31536000), "3yr", false}, - {"nil duration", nil, "1yr", false}, + {"1 year match", aws.Int32(31536000), 12, true}, + {"3 years match", aws.Int32(94608000), 36, true}, + {"no match", aws.Int32(31536000), 36, false}, + {"nil duration", nil, 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 { + assert.Error(t, err) + assert.Contains(t, err.Error(), "unsupported Redshift reservation term") + return + } + assert.NoError(t, err) assert.Equal(t, tt.expected, result) }) } From e19126d993645a58a4ffb11cd40033d17adb7523 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 10 Jun 2026 17:04:22 -0700 Subject: [PATCH 2/3] test(providers/aws): harden ARCH-04 term regression tests Address CodeRabbit review feedback on PR #1207: - Guard the error-message Contains assertions behind the result of assert.Error in all five term-converter table tests, so a failed error expectation reports cleanly instead of panicking on a nil error. - Add TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall to the RDS client tests: an unrecognized term ("0", as produced by a 0/NULL Term DB row on the scheduler purchase path) must abort the offering lookup before any DescribeReservedDBInstancesOfferings call, which exercises the fail-loud path end to end rather than only the helper. Part of #1192. --- .../aws/services/elasticache/client_test.go | 5 +-- .../aws/services/memorydb/client_test.go | 5 +-- .../aws/services/opensearch/client_test.go | 5 +-- providers/aws/services/rds/client_test.go | 33 +++++++++++++++++-- .../aws/services/redshift/client_test.go | 5 +-- 5 files changed, 43 insertions(+), 10 deletions(-) diff --git a/providers/aws/services/elasticache/client_test.go b/providers/aws/services/elasticache/client_test.go index 57982fa43..32ed58c85 100644 --- a/providers/aws/services/elasticache/client_test.go +++ b/providers/aws/services/elasticache/client_test.go @@ -395,8 +395,9 @@ func TestClient_GetDurationString(t *testing.T) { t.Run(tt.name, func(t *testing.T) { result, err := client.getDurationString(tt.term) if tt.expectErr { - assert.Error(t, err) - assert.Contains(t, err.Error(), "unsupported ElastiCache reservation term") + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "unsupported ElastiCache reservation term") + } return } assert.NoError(t, err) diff --git a/providers/aws/services/memorydb/client_test.go b/providers/aws/services/memorydb/client_test.go index 2ff942668..442ddfd29 100644 --- a/providers/aws/services/memorydb/client_test.go +++ b/providers/aws/services/memorydb/client_test.go @@ -907,8 +907,9 @@ func TestClient_GetDurationStringForAPI(t *testing.T) { t.Run(tt.name, func(t *testing.T) { result, err := client.getDurationStringForAPI(tt.term) if tt.expectErr { - assert.Error(t, err) - assert.Contains(t, err.Error(), "unsupported MemoryDB reservation term") + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "unsupported MemoryDB reservation term") + } return } assert.NoError(t, err) diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index 915d489b1..058c38617 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -299,8 +299,9 @@ func TestRequiredMonthsForTerm(t *testing.T) { t.Run(tt.name, func(t *testing.T) { result, err := requiredMonthsForTerm(tt.term) if tt.expectErr { - assert.Error(t, err) - assert.Contains(t, err.Error(), "unsupported OpenSearch reservation term") + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "unsupported OpenSearch reservation term") + } return } assert.NoError(t, err) diff --git a/providers/aws/services/rds/client_test.go b/providers/aws/services/rds/client_test.go index e190c191c..01fe1f385 100644 --- a/providers/aws/services/rds/client_test.go +++ b/providers/aws/services/rds/client_test.go @@ -584,8 +584,9 @@ func TestClient_GetDurationString(t *testing.T) { t.Run(tt.name, func(t *testing.T) { result, err := client.getDurationString(tt.term) if tt.expectErr { - assert.Error(t, err) - assert.Contains(t, err.Error(), "unsupported RDS reservation term") + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "unsupported RDS reservation term") + } return } assert.NoError(t, err) @@ -893,6 +894,34 @@ func TestFindOfferingID_InvalidAZConfig_Errors(t *testing.T) { assert.Contains(t, err.Error(), "typo-az") } +// 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 DescribeReservedDBInstancesOfferings 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) { + mockRDS := &MockRDSClient{} + t.Cleanup(func() { mockRDS.AssertExpectations(t) }) + client := &Client{client: mockRDS, region: "us-east-1"} + + rec := common.Recommendation{ + Service: common.ServiceRelationalDB, + ResourceType: "db.r5.large", + PaymentOption: "all-upfront", + Term: "0", + Details: &common.DatabaseDetails{ + Engine: "mysql", + AZConfig: "single-az", + }, + } + + _, err := client.findOfferingID(context.Background(), rec, "") + + require.Error(t, err, "findOfferingID must error on an unrecognized term (ARCH-04)") + assert.Contains(t, err.Error(), "unsupported RDS reservation term") + mockRDS.AssertNotCalled(t, "DescribeReservedDBInstancesOfferings", mock.Anything, mock.Anything) +} + // TestNormalizeEngineName_AmbiguousErrors is the M6 regression test: // normalizeEngineName must error for bare Oracle/SQL Server/Aurora inputs. // CE returns "Oracle" (title-case) for any Oracle engine when no edition is diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index b0ab9dc15..4343493da 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -505,8 +505,9 @@ func TestRequiredMonthsForTerm(t *testing.T) { t.Run(tt.name, func(t *testing.T) { result, err := requiredMonthsForTerm(tt.term) if tt.expectErr { - assert.Error(t, err) - assert.Contains(t, err.Error(), "unsupported Redshift reservation term") + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "unsupported Redshift reservation term") + } return } assert.NoError(t, err) From 0c3cba52a6951929f8883c56a7d8145dce1c27ed Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 19 Jun 2026 16:48:23 +0200 Subject: [PATCH 3/3] test(providers/aws): add findOfferingID call-path regression for ARCH-04 For the four remaining ARCH-04 clients (ElastiCache, MemoryDB, OpenSearch, Redshift), add TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall tests that assert an invalid/zero term aborts before any describe-offerings API call. Mirrors the existing RDS call-path test added in the second commit on this PR. Each test uses AssertNotCalled to verify the AWS API is never reached when the term string is "0" (the scheduler-path 0/NULL Term DB row scenario from issue #1192/ARCH-04). --- .../aws/services/elasticache/client_test.go | 25 +++++++++++++++++++ .../aws/services/memorydb/client_test.go | 24 ++++++++++++++++++ .../aws/services/opensearch/client_test.go | 24 ++++++++++++++++++ .../aws/services/redshift/client_test.go | 24 ++++++++++++++++++ 4 files changed, 97 insertions(+) diff --git a/providers/aws/services/elasticache/client_test.go b/providers/aws/services/elasticache/client_test.go index 32ed58c85..186ff9479 100644 --- a/providers/aws/services/elasticache/client_test.go +++ b/providers/aws/services/elasticache/client_test.go @@ -696,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) +} diff --git a/providers/aws/services/memorydb/client_test.go b/providers/aws/services/memorydb/client_test.go index 442ddfd29..f68018212 100644 --- a/providers/aws/services/memorydb/client_test.go +++ b/providers/aws/services/memorydb/client_test.go @@ -917,3 +917,27 @@ func TestClient_GetDurationStringForAPI(t *testing.T) { }) } } + +// 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) +} diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index 058c38617..e9662fd5a 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -984,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) +} diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index 4343493da..2899fc1f9 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -1467,3 +1467,27 @@ func TestDerivePaymentOption(t *testing.T) { assert.Equal(t, "partial-upfront", derivePaymentOption(rsOffering("x", 500, 0.05))) assert.Equal(t, "unknown", derivePaymentOption(rsOffering("x", 0, 0))) } + +// 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 DescribeReservedNodeOfferings 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) { + mockRS := &MockRedshiftClient{} + t.Cleanup(func() { mockRS.AssertExpectations(t) }) + client := &Client{client: mockRS, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "dc2.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 Redshift reservation term") + } + mockRS.AssertNotCalled(t, "DescribeReservedNodeOfferings", mock.Anything, mock.Anything) +}