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..186ff9479 100644 --- a/providers/aws/services/elasticache/client_test.go +++ b/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) +} 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..f68018212 100644 --- a/providers/aws/services/memorydb/client_test.go +++ b/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) +} 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..e9662fd5a 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/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) +} 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..01fe1f385 100644 --- a/providers/aws/services/rds/client_test.go +++ b/providers/aws/services/rds/client_test.go @@ -561,19 +561,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", "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 RDS reservation term") + } + return + } + assert.NoError(t, err) assert.Equal(t, tt.expected, result) }) } @@ -878,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.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..2899fc1f9 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -463,19 +463,54 @@ 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 { + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "unsupported Redshift reservation term") + } + return + } + assert.NoError(t, err) assert.Equal(t, tt.expected, result) }) } @@ -1432,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) +}