diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 19edbd007..b7c0dfa03 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1299,15 +1299,21 @@ func (s *Scheduler) convertRecommendations(recs []common.Recommendation, provide providerName, rec.PaymentOption, rec.Account, rec.Service, rec.ResourceType) } - // Parse term to integer (e.g., "3yr" -> 3) - term := 3 - if rec.Term != "" { - termStr := strings.TrimSuffix(rec.Term, "yr") - if parsed, err := strconv.Atoi(termStr); err == nil && parsed > 0 { - term = parsed - } else { - logging.Warnf("Invalid term value %q, defaulting to 3 years", rec.Term) - } + // Parse term to integer (e.g., "3yr" -> 3). An empty or unparseable + // term must NOT be coerced to a valid default: rec.Term round-trips + // through this int on the purchase path (internal/purchase/execution.go + // formats it back as "%dyr"), so silently defaulting here would let a + // malformed term launder into a valid, purchasable commitment length + // downstream, bypassing the ARCH-04 whitelist checks in the provider + // clients entirely, since "3yr" is itself a whitelisted value (issue + // #1207 follow-up). Drop the recommendation instead of persisting a + // fabricated term. + termStr := strings.TrimSuffix(rec.Term, "yr") + term, err := strconv.Atoi(termStr) + if err != nil || term <= 0 { + logging.Errorf("convertRecommendations: dropping %s rec (account=%s service=%s sku=%s region=%s): invalid term %q", + providerName, rec.Account, rec.Service, rec.ResourceType, rec.Region, rec.Term) + continue } // The ID must be unique per logically-distinct rec — otherwise diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index cc24576e7..931096829 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -1190,6 +1190,66 @@ func TestScheduler_ConvertRecommendations_Empty(t *testing.T) { assert.Len(t, records, 0) } +// TestScheduler_ConvertRecommendations_InvalidTermDropped is the ARCH-04 +// follow-up (issue #1207) regression test: a rec with an empty or +// unparseable Term must be dropped rather than silently persisted with +// Term=3. rec.Term round-trips through this int on the purchase path +// (internal/purchase/execution.go formats it back as "%dyr"), so a +// fabricated default here would let a malformed term launder into a real +// 3-year purchase, bypassing every provider client's ARCH-04 whitelist +// (since "3yr" is itself a valid, purchasable term). This test fails on +// the pre-fix code (which appends a record with Term=3 for both bad recs) +// and passes once the invalid-term recs are skipped. +func TestScheduler_ConvertRecommendations_InvalidTermDropped(t *testing.T) { + scheduler := &Scheduler{} + + recommendations := []common.Recommendation{ + // Empty Term: what a 0/NULL Term DB row produces on the scheduler + // purchase path. + { + Provider: common.ProviderAWS, + Service: common.ServiceEC2, + Region: "us-east-1", + ResourceType: "m5.large", + Count: 1, + Term: "", + PaymentOption: "all-upfront", + CommitmentCost: 1000.0, + EstimatedSavings: 500.0, + }, + // Garbage Term. + { + Provider: common.ProviderAWS, + Service: common.ServiceEC2, + Region: "us-east-1", + ResourceType: "m5.xlarge", + Count: 1, + Term: "banana", + PaymentOption: "all-upfront", + CommitmentCost: 2000.0, + EstimatedSavings: 900.0, + }, + // Valid rec in the same batch must still be persisted. + { + Provider: common.ProviderAWS, + Service: common.ServiceEC2, + Region: "us-east-1", + ResourceType: "m5.2xlarge", + Count: 1, + Term: "1yr", + PaymentOption: "all-upfront", + CommitmentCost: 3000.0, + EstimatedSavings: 1400.0, + }, + } + + records := scheduler.convertRecommendations(recommendations, "aws") + + require.Len(t, records, 1, "both invalid-term recs must be dropped, not persisted with a fabricated Term") + assert.Equal(t, "m5.2xlarge", records[0].ResourceType) + assert.Equal(t, 1, records[0].Term) +} + // TestScheduler_ConvertRecommendations_OnDemandCost pins #274: the // canonical on-demand baseline must round-trip from common.Recommendation // through to the persisted RecommendationRecord so the frontend can use diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 9ac9e632b..bbad6c33e 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -461,7 +461,11 @@ func (c *Client) buildEC2QueryFromRec(rec common.Recommendation) (ec2OfferingQue if err != nil { return ec2OfferingQuery{}, err } - q, err := buildEC2OfferingQuery(rec, details, c.getDurationValue(rec.Term)) + duration, err := c.getDurationValue(rec.Term) + if err != nil { + return ec2OfferingQuery{}, err + } + q, err := buildEC2OfferingQuery(rec, details, duration) if err != nil { return ec2OfferingQuery{}, err } @@ -661,12 +665,19 @@ const ( ThreeYearSeconds = 94608000 // 3 * 365 days in seconds ) -// getDurationValue converts term string to seconds for EC2 API -func (c *Client) getDurationValue(term string) int64 { - if term == "3yr" || term == "3" { - return ThreeYearSeconds +// getDurationValue converts a term string to seconds for the EC2 API. +// 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) getDurationValue(term string) (int64, error) { + switch term { + case "3yr", "3": + return ThreeYearSeconds, nil + case "1yr", "1": + return OneYearSeconds, nil + default: + return 0, fmt.Errorf("unsupported EC2 reservation term %q: must be one of 1yr, 1, 3yr, 3", term) } - return OneYearSeconds } // ConvertibleRI represents an active convertible Reserved Instance. diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 11689b9dc..cfdfa6949 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -15,6 +15,7 @@ import ( "github.com/aws/aws-sdk-go-v2/service/ec2/types" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" ) // MockEC2Client implements EC2API for testing @@ -516,19 +517,35 @@ func TestClient_GetDurationValue(t *testing.T) { client := &Client{} tests := []struct { - name string - term string - expected int64 + name string + term string + expected int64 + 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 follow-up (issue #1207): 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", 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 := client.getDurationValue(tt.term) + result, err := client.getDurationValue(tt.term) + if tt.expectErr { + if assert.Error(t, err) { + assert.Contains(t, err.Error(), "unsupported EC2 reservation term") + } + return + } + assert.NoError(t, err) assert.Equal(t, tt.expected, result) }) } @@ -710,6 +727,36 @@ func TestFindOfferingID_HappyPath(t *testing.T) { assert.Equal(t, "offering-ok", id) } +// TestFindOfferingID_InvalidTerm_ErrorsBeforeAPICall is the ARCH-04 +// follow-up (issue #1207) call-path regression test: an unrecognized or +// empty term must abort the offering lookup before any +// DescribeReservedInstancesOfferings 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) { + t.Parallel() + mockEC2 := &MockEC2Client{} + t.Cleanup(func() { mockEC2.AssertExpectations(t) }) + client := &Client{client: mockEC2, region: "us-east-1"} + + rec := common.Recommendation{ + ResourceType: "t4g.nano", + PaymentOption: "no-upfront", + Term: "0", + Details: &common.ComputeDetails{ + Platform: "Linux/UNIX", + Tenancy: "default", + Scope: "Region", + }, + } + + _, 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 EC2 reservation term") + mockEC2.AssertNotCalled(t, "DescribeReservedInstancesOfferings", mock.Anything, mock.Anything) +} + // TestClient_tagReservedInstance_NameTagPresent asserts that a purchase on the // no-token CLI path (issue #687) stamps a self-describing Name tag on the EC2 // RI. EC2 PurchaseReservedInstancesOfferingInput has no customer-supplied name