From ec3b18586fdc2a9652545c82ef5119926422a9c7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 16 Jul 2026 16:28:05 +0300 Subject: [PATCH 1/5] feat(recommendations): add GetCostAndUsage to CostExplorerAPI + on-demand series Extend CostExplorerAPI with GetCostAndUsage so the ladder's usage-baseline adapter can source daily on-demand EC2 spend from CE without breaking the existing mock graph. New GetOnDemandSeries on *Client: - CE DAILY granularity, UNBLENDED_COST metric, 3-clause AND filter (SERVICE=EC2 compute, PURCHASE_TYPE=On Demand Instances, REGION=) - Paginates via NextPageToken, capped at maxOnDemandSeriesPages=20 - Divides daily USD by 24 (derived from granularity, not magic) - Oldest-first sort; errors on completely empty result - TODO(#1365) markers for the []DailyPoint upgrade once that PR merges Add GetCostAndUsage stubs to all existing CostExplorerAPI mocks (mockCostExplorerAPI, multiPageRIMock, alwaysNextPageRIMock, mockCostExplorerForSP, multiPageSPMock, alwaysNextPageSPMock, mockCostExplorerClient) so the interface extension does not break existing test suites. --- providers/aws/recommendations/client.go | 4 + providers/aws/recommendations/client_test.go | 16 + .../aws/recommendations/ondemand_series.go | 220 ++++++++++++++ .../recommendations/ondemand_series_test.go | 285 ++++++++++++++++++ .../parser_sp_additional_test.go | 16 + providers/aws/service_client_test.go | 4 + 6 files changed, 545 insertions(+) create mode 100644 providers/aws/recommendations/ondemand_series.go create mode 100644 providers/aws/recommendations/ondemand_series_test.go diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index 95102165f..7a1274f7f 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -30,6 +30,10 @@ type CostExplorerAPI interface { GetReservationCoverage(ctx context.Context, params *costexplorer.GetReservationCoverageInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationCoverageOutput, error) GetSavingsPlansCoverage(ctx context.Context, params *costexplorer.GetSavingsPlansCoverageInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetSavingsPlansCoverageOutput, error) GetSavingsPlansUtilization(ctx context.Context, params *costexplorer.GetSavingsPlansUtilizationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetSavingsPlansUtilizationOutput, error) + // GetCostAndUsage fetches cost and usage data for arbitrary time periods + // and granularities. Added for the daily on-demand series adapter that + // powers GetUsageBaseline (L2). + GetCostAndUsage(ctx context.Context, params *costexplorer.GetCostAndUsageInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetCostAndUsageOutput, error) } // Client wraps the AWS Cost Explorer client for RI recommendations diff --git a/providers/aws/recommendations/client_test.go b/providers/aws/recommendations/client_test.go index 2d83b44ab..54beed2de 100644 --- a/providers/aws/recommendations/client_test.go +++ b/providers/aws/recommendations/client_test.go @@ -60,6 +60,10 @@ func (m *mockCostExplorerAPI) GetSavingsPlansUtilization(ctx context.Context, pa return &costexplorer.GetSavingsPlansUtilizationOutput{}, nil } +func (m *mockCostExplorerAPI) GetCostAndUsage(_ context.Context, _ *costexplorer.GetCostAndUsageInput, _ ...func(*costexplorer.Options)) (*costexplorer.GetCostAndUsageOutput, error) { + return &costexplorer.GetCostAndUsageOutput{}, nil +} + func TestNewClient(t *testing.T) { cfg := aws.Config{ Region: "us-west-2", @@ -701,6 +705,12 @@ func (m *multiPageRIMock) GetSavingsPlansUtilization( return &costexplorer.GetSavingsPlansUtilizationOutput{}, nil } +func (m *multiPageRIMock) GetCostAndUsage( + _ context.Context, _ *costexplorer.GetCostAndUsageInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetCostAndUsageOutput, error) { + return &costexplorer.GetCostAndUsageOutput{}, nil +} + // riDetail returns a minimal ReservationPurchaseRecommendation with n EC2 details. func riDetail(n int) types.ReservationPurchaseRecommendation { details := make([]types.ReservationPurchaseRecommendationDetail, n) @@ -833,6 +843,12 @@ func (m *alwaysNextPageRIMock) GetSavingsPlansUtilization( return &costexplorer.GetSavingsPlansUtilizationOutput{}, nil } +func (m *alwaysNextPageRIMock) GetCostAndUsage( + _ context.Context, _ *costexplorer.GetCostAndUsageInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetCostAndUsageOutput, error) { + return &costexplorer.GetCostAndUsageOutput{}, nil +} + // TestGetRecommendations_RI_PaginationCapError asserts that exceeding // maxRecommendationPages returns a diagnostic error (issue #692). func TestGetRecommendations_RI_PaginationCapError(t *testing.T) { diff --git a/providers/aws/recommendations/ondemand_series.go b/providers/aws/recommendations/ondemand_series.go new file mode 100644 index 000000000..147048e12 --- /dev/null +++ b/providers/aws/recommendations/ondemand_series.go @@ -0,0 +1,220 @@ +package recommendations + +import ( + "context" + "fmt" + "sort" + "strconv" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/costexplorer" + "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" +) + +// ec2ComputeService is the CE SERVICE dimension value for EC2 compute. +// Matches the exact string CE uses; deviations silently return empty results. +const ec2ComputeService = "Amazon Elastic Compute Cloud - Compute" + +// purchaseTypeOnDemand is the CE PURCHASE_TYPE dimension value for on-demand +// instances. Filters to usage billed at on-demand rates (excludes RI-covered, +// SP-covered, and Spot). Using the uncovered on-demand spend as the baseline +// is conservative: existing coverage is separately counted by GetLayerStates, +// so the engine correctly nets it into the gap calculation. +const purchaseTypeOnDemand = "On Demand Instances" + +// onDemandMetric is the CE metric for GetCostAndUsage. UNBLENDED_COST is the +// actual charge to the account at the billed rate; for on-demand instances +// this equals on-demand rate x hours. Named constant (not magic string) per +// feedback_no_hardcoded_magic_values; the SDK types.MetricUnblendedCost is +// used to derive it so it cannot drift from the CE enum vocabulary. +const onDemandMetric = string(types.MetricUnblendedCost) + +// maxOnDemandSeriesPages caps the GetCostAndUsage NextPageToken loop. +// Daily granularity over a 30-day window produces at most 1 page; the cap +// guards against a runaway token loop or API misbehavior and returns a +// diagnostic error instead of billing $0.01/call indefinitely. +const maxOnDemandSeriesPages = 20 + +// GetOnDemandSeries fetches daily on-demand EC2 compute cost for the given +// region over the past lookbackDays days from CE GetCostAndUsage and returns +// a slice of len(returned_days) USD/hr values ordered oldest-to-newest. +// +// CE query parameters: +// - Granularity: DAILY (one entry per calendar day) +// - Metric: UNBLENDED_COST (actual on-demand billed rate; spec: unblended) +// - Filter: SERVICE = EC2 compute AND PURCHASE_TYPE = On Demand Instances +// AND REGION = region (three-clause AND) +// - Time window: [now-lookbackDays, now) exclusive end +// +// Each day's total USD is divided by 24.0 to yield USD/hr. Dividing by 24 is +// derived from the DAILY granularity contract (one day = 24 hours), not a +// magic constant. +// +// CE typically lags ~24-48h, so the returned series may be shorter than +// lookbackDays. The caller (baseline.GetUsageBaseline) enforces the minimum- +// length requirement (minBaselineSeriesDays); this function only errors on a +// completely empty result. +// +// TODO(#1365): when the feat/ladder-stale-series PR merges, change the +// interface to return []DailyPoint{Date time.Time; USDPerHour float64} instead +// of []float64. The accumulator below already builds per-date entries; the +// final flatten step is the only change needed. +func (c *Client) GetOnDemandSeries(ctx context.Context, region string, lookbackDays int) ([]float64, error) { + if lookbackDays <= 0 { + return nil, fmt.Errorf("GetOnDemandSeries: lookbackDays must be > 0, got %d", lookbackDays) + } + if region == "" { + return nil, fmt.Errorf("GetOnDemandSeries: region must not be empty") + } + + end := time.Now().UTC().Truncate(24 * time.Hour) // midnight today (exclusive end for CE) + start := end.AddDate(0, 0, -lookbackDays) + + input := &costexplorer.GetCostAndUsageInput{ + TimePeriod: &types.DateInterval{ + Start: aws.String(start.Format("2006-01-02")), + End: aws.String(end.Format("2006-01-02")), + }, + Granularity: types.GranularityDaily, + Metrics: []string{onDemandMetric}, + Filter: onDemandSeriesFilter(region), + } + + // TODO(#1365): build []struct{Date time.Time; USDPerHour float64} here + // and flatten to []float64 at the return site once the interface changes. + byDate := make(map[string]float64) + + var nextToken *string + page := 0 + for { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("GetOnDemandSeries: context cancelled during pagination: %w", err) + } + page++ + if page > maxOnDemandSeriesPages { + return nil, fmt.Errorf("GetOnDemandSeries: exceeded %d page cap; possible CE token loop", maxOnDemandSeriesPages) + } + input.NextPageToken = nextToken + + out, err := c.fetchOnDemandPage(ctx, input) + if err != nil { + return nil, fmt.Errorf("GetOnDemandSeries: %w", err) + } + + if err := accumulateDailyResults(byDate, out); err != nil { + return nil, fmt.Errorf("GetOnDemandSeries: %w", err) + } + + // aws.ToString returns "" for nil, so this single check covers both nil + // and empty-string tokens. + if aws.ToString(out.NextPageToken) == "" { + break + } + nextToken = out.NextPageToken + } + + if len(byDate) == 0 { + return nil, fmt.Errorf("GetOnDemandSeries: CE returned no on-demand data for region %q over the past %d days (account may have no on-demand EC2 spend, or CE data not yet available)", + region, lookbackDays) + } + + return sortedDailySeries(byDate), nil +} + +// fetchOnDemandPage calls GetCostAndUsage with rate-limit retry, mirroring +// the fetchCoveragePage / fetchUtilizationPage pattern in coverage.go and +// utilization.go. Each attempt acquires one rate-limiter slot at the API call +// site (not at goroutine creation) per feedback_semaphore_at_api_call. +func (c *Client) fetchOnDemandPage(ctx context.Context, input *costexplorer.GetCostAndUsageInput) (*costexplorer.GetCostAndUsageOutput, error) { + c.rateLimiter.Reset() + for { + if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { + return nil, fmt.Errorf("rate limiter wait: %w", waitErr) + } + out, err := c.costExplorerClient.GetCostAndUsage(ctx, input) + if !c.rateLimiter.ShouldRetry(err) { + if err != nil { + return nil, fmt.Errorf("GetCostAndUsage: %w", err) + } + return out, nil + } + } +} + +// onDemandSeriesFilter builds the three-clause AND filter for +// GetCostAndUsage: EC2 compute service, on-demand purchase type, and the +// given region. All three clauses are required: +// - SERVICE scopes to EC2 compute (excludes RDS, ElastiCache, etc.). +// - PURCHASE_TYPE scopes to on-demand charges (excludes RI-covered, +// SP-covered, and Spot; those are accounted for via GetLayerStates). +// - REGION scopes to the ladder's configured region. +// +// Per feedback_verify_api_filter_contracts: GetCostAndUsage supports SERVICE, +// PURCHASE_TYPE, and REGION as valid filter dimensions (CE API reference, +// GetCostAndUsageInput.Filter). SAVINGS_PLANS_TYPE is NOT supported here +// (that is a GetSavingsPlansCoverage/Utilization dimension only). +func onDemandSeriesFilter(region string) *types.Expression { + return &types.Expression{ + And: []types.Expression{ + {Dimensions: &types.DimensionValues{ + Key: types.DimensionService, + Values: []string{ec2ComputeService}, + }}, + {Dimensions: &types.DimensionValues{ + Key: types.DimensionPurchaseType, + Values: []string{purchaseTypeOnDemand}, + }}, + {Dimensions: &types.DimensionValues{ + Key: types.DimensionRegion, + Values: []string{region}, + }}, + }, + } +} + +// accumulateDailyResults extracts daily USD/hr values from one page of +// GetCostAndUsage results and merges them into byDate. Days with a missing or +// nil metric are stored as 0 (CE may omit a day if spend was exactly $0). +// Days with an unparseable amount string fail loud (feedback_strict_int_parse). +// Dividing by 24 converts daily USD to USD/hr; 24 is derived from the DAILY +// granularity contract (one day = 24 hours), not a magic constant. +func accumulateDailyResults(byDate map[string]float64, out *costexplorer.GetCostAndUsageOutput) error { + for _, r := range out.ResultsByTime { + if r.TimePeriod == nil || r.TimePeriod.Start == nil { + continue + } + dateStr := aws.ToString(r.TimePeriod.Start) + mv, ok := r.Total[onDemandMetric] + if !ok || mv.Amount == nil { + byDate[dateStr] = 0 + continue + } + usd, err := strconv.ParseFloat(aws.ToString(mv.Amount), 64) + if err != nil { + return fmt.Errorf("cannot parse CE amount %q for day %s: %w", + aws.ToString(mv.Amount), dateStr, err) + } + // Divide total daily USD by 24 to get USD/hr. Derived from DAILY + // granularity: 1 day = 24 hours; not a magic constant. + byDate[dateStr] = usd / 24.0 + } + return nil +} + +// sortedDailySeries converts a date-keyed cost map to a []float64 sorted +// oldest-to-newest (lexicographic sort on "2006-01-02" strings is +// chronological). The baseline contract requires oldest-first ordering. +func sortedDailySeries(byDate map[string]float64) []float64 { + dates := make([]string, 0, len(byDate)) + for d := range byDate { + dates = append(dates, d) + } + sort.Strings(dates) + + series := make([]float64, len(dates)) + for i, d := range dates { + series[i] = byDate[d] + } + return series +} diff --git a/providers/aws/recommendations/ondemand_series_test.go b/providers/aws/recommendations/ondemand_series_test.go new file mode 100644 index 000000000..4641d697d --- /dev/null +++ b/providers/aws/recommendations/ondemand_series_test.go @@ -0,0 +1,285 @@ +package recommendations + +import ( + "context" + "errors" + "fmt" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/costexplorer" + "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// mockOnDemandCE is a hermetic mock for GetCostAndUsage. Only GetCostAndUsage +// is behaviourally wired; all other CostExplorerAPI methods are stubs that +// return empty non-nil outputs (the base mockCostExplorerAPI already provides +// those stubs via embedding). +type mockOnDemandCE struct { + mockCostExplorerAPI // provides all stub methods except GetCostAndUsage + pages []*costexplorer.GetCostAndUsageOutput + tokens []string // tokens[i] triggers pages[i+1] + gotInputs []*costexplorer.GetCostAndUsageInput + apiErr error +} + +func (m *mockOnDemandCE) GetCostAndUsage( + _ context.Context, + params *costexplorer.GetCostAndUsageInput, + _ ...func(*costexplorer.Options), +) (*costexplorer.GetCostAndUsageOutput, error) { + m.gotInputs = append(m.gotInputs, params) + if m.apiErr != nil { + return nil, m.apiErr + } + // Determine which page to return based on the incoming NextPageToken. + idx := 0 + incoming := aws.ToString(params.NextPageToken) + for i, tok := range m.tokens { + if tok == incoming { + idx = i + 1 + break + } + } + if idx >= len(m.pages) { + return nil, fmt.Errorf("unexpected page token %q", incoming) + } + return m.pages[idx], nil +} + +// dailyResult builds one ResultByTime entry for testing. +// dateStr must be in "2006-01-02" format; totalUSD is the day's UNBLENDED_COST. +func dailyResult(dateStr string, totalUSD float64) types.ResultByTime { + return types.ResultByTime{ + TimePeriod: &types.DateInterval{ + Start: aws.String(dateStr), + End: aws.String(dateStr), // end is exclusive, not relevant for mock + }, + Total: map[string]types.MetricValue{ + onDemandMetric: {Amount: aws.String(fmt.Sprintf("%.10f", totalUSD))}, + }, + } +} + +// dailyResultNoMetric builds a ResultByTime entry with a missing metric key, +// exercising the "metric absent" branch that falls back to 0. +func dailyResultNoMetric(dateStr string) types.ResultByTime { + return types.ResultByTime{ + TimePeriod: &types.DateInterval{Start: aws.String(dateStr)}, + Total: map[string]types.MetricValue{}, + } +} + +// newOnDemandClient creates a recommendations.Client wired to the given mock. +func newOnDemandClient(m *mockOnDemandCE) *Client { + return NewClientWithAPI(m, "us-east-1") +} + +// generate30Days builds a single-page mock returning 30 daily entries, +// each billing totalPerDay USD total (i.e. totalPerDay/24 USD/hr per entry). +func generate30DayPage(startDate time.Time, totalPerDay float64) []*costexplorer.GetCostAndUsageOutput { + results := make([]types.ResultByTime, 30) + for i := range results { + day := startDate.AddDate(0, 0, i) + results[i] = dailyResult(day.Format("2006-01-02"), totalPerDay) + } + return []*costexplorer.GetCostAndUsageOutput{ + {ResultsByTime: results}, + } +} + +// TestGetOnDemandSeries_HappyPath verifies that 30 daily entries at +// $240/day each return 30 elements of $10/hr ($240/24h). +func TestGetOnDemandSeries_HappyPath(t *testing.T) { + start := time.Now().UTC().Truncate(24*time.Hour).AddDate(0, 0, -30) + mock := &mockOnDemandCE{pages: generate30DayPage(start, 240.0)} + client := newOnDemandClient(mock) + + series, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 30) + + require.NoError(t, err) + assert.Len(t, series, 30, "series must have one element per returned day") + for i, v := range series { + assert.InDelta(t, 10.0, v, 1e-9, "day %d: expected $10/hr ($240/24h)", i) + } +} + +// TestGetOnDemandSeries_OldestFirst verifies that days are returned in +// chronological (oldest-to-newest) order regardless of map iteration order. +func TestGetOnDemandSeries_OldestFirst(t *testing.T) { + // Provide 3 days with distinct costs so order is detectable. + pages := []*costexplorer.GetCostAndUsageOutput{{ + ResultsByTime: []types.ResultByTime{ + dailyResult("2026-01-03", 72.0), // $3/hr + dailyResult("2026-01-01", 24.0), // $1/hr + dailyResult("2026-01-02", 48.0), // $2/hr + }, + }} + mock := &mockOnDemandCE{pages: pages} + client := newOnDemandClient(mock) + + series, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 7) + + require.NoError(t, err) + require.Len(t, series, 3) + assert.InDelta(t, 1.0, series[0], 1e-9, "oldest day first") + assert.InDelta(t, 2.0, series[1], 1e-9, "middle day second") + assert.InDelta(t, 3.0, series[2], 1e-9, "newest day last") +} + +// TestGetOnDemandSeries_Paginated verifies that two pages are fetched and +// their results merged into a single sorted series. +func TestGetOnDemandSeries_Paginated(t *testing.T) { + pages := []*costexplorer.GetCostAndUsageOutput{ + { + ResultsByTime: []types.ResultByTime{ + dailyResult("2026-01-01", 24.0), + dailyResult("2026-01-02", 48.0), + }, + NextPageToken: aws.String("tok1"), + }, + { + ResultsByTime: []types.ResultByTime{ + dailyResult("2026-01-03", 72.0), + }, + }, + } + mock := &mockOnDemandCE{pages: pages, tokens: []string{"tok1"}} + client := newOnDemandClient(mock) + + series, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 7) + + require.NoError(t, err) + require.Len(t, series, 3, "both pages must be merged") + assert.InDelta(t, 1.0, series[0], 1e-9) + assert.InDelta(t, 2.0, series[1], 1e-9) + assert.InDelta(t, 3.0, series[2], 1e-9) + // Verify exactly 2 CE calls were made (one per page). + assert.Len(t, mock.gotInputs, 2, "should have fetched 2 pages") + // Second call must carry the token from the first response. + assert.Equal(t, "tok1", aws.ToString(mock.gotInputs[1].NextPageToken)) +} + +// TestGetOnDemandSeries_EmptyResultErrors verifies a hard error when CE +// returns zero daily entries (no on-demand spend or CE not yet available). +func TestGetOnDemandSeries_EmptyResultErrors(t *testing.T) { + mock := &mockOnDemandCE{pages: []*costexplorer.GetCostAndUsageOutput{{}}} + client := newOnDemandClient(mock) + + _, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 30) + + require.Error(t, err) + assert.Contains(t, err.Error(), "no on-demand data", "error should describe the empty-data condition") +} + +// TestGetOnDemandSeries_APIErrorPropagated verifies that a CE API error is +// returned to the caller without swallowing. +func TestGetOnDemandSeries_APIErrorPropagated(t *testing.T) { + sentinel := errors.New("CE rate limit") + mock := &mockOnDemandCE{ + pages: []*costexplorer.GetCostAndUsageOutput{{}}, + apiErr: sentinel, + } + client := newOnDemandClient(mock) + + _, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 30) + + require.Error(t, err) + assert.ErrorIs(t, err, sentinel) +} + +// TestGetOnDemandSeries_ParseErrorFails verifies that a non-numeric Amount +// from CE triggers a fail-loud error (feedback_strict_int_parse). +func TestGetOnDemandSeries_ParseErrorFails(t *testing.T) { + pages := []*costexplorer.GetCostAndUsageOutput{{ + ResultsByTime: []types.ResultByTime{{ + TimePeriod: &types.DateInterval{Start: aws.String("2026-01-01")}, + Total: map[string]types.MetricValue{onDemandMetric: {Amount: aws.String("not-a-number")}}, + }}, + }} + mock := &mockOnDemandCE{pages: pages} + client := newOnDemandClient(mock) + + _, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 7) + + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot parse CE amount") +} + +// TestGetOnDemandSeries_CorrectFilterParams verifies the CE query uses the +// correct granularity, metric, and three-clause AND filter +// (feedback_verify_api_filter_contracts). +func TestGetOnDemandSeries_CorrectFilterParams(t *testing.T) { + start := time.Now().UTC().Truncate(24*time.Hour).AddDate(0, 0, -7) + mock := &mockOnDemandCE{pages: generate30DayPage(start, 24.0)} + client := newOnDemandClient(mock) + + _, err := client.GetOnDemandSeries(context.Background(), "eu-west-1", 7) + require.NoError(t, err) + require.NotEmpty(t, mock.gotInputs, "at least one CE call must have been made") + + input := mock.gotInputs[0] + + assert.Equal(t, types.GranularityDaily, input.Granularity, + "granularity must be DAILY for the daily series") + assert.Equal(t, []string{onDemandMetric}, input.Metrics, + "metric must be UNBLENDED_COST") + + require.NotNil(t, input.Filter, "filter must be present") + require.Len(t, input.Filter.And, 3, "filter must be a three-clause AND") + + // Extract dimension keys from the AND clauses. + dimKeys := make([]types.Dimension, 0, 3) + dimVals := make(map[types.Dimension][]string, 3) + for _, clause := range input.Filter.And { + require.NotNil(t, clause.Dimensions) + dimKeys = append(dimKeys, clause.Dimensions.Key) + dimVals[clause.Dimensions.Key] = clause.Dimensions.Values + } + assert.Contains(t, dimKeys, types.DimensionService, "SERVICE dimension required") + assert.Contains(t, dimKeys, types.DimensionPurchaseType, "PURCHASE_TYPE dimension required") + assert.Contains(t, dimKeys, types.DimensionRegion, "REGION dimension required") + + assert.Equal(t, []string{ec2ComputeService}, dimVals[types.DimensionService]) + assert.Equal(t, []string{purchaseTypeOnDemand}, dimVals[types.DimensionPurchaseType]) + assert.Equal(t, []string{"eu-west-1"}, dimVals[types.DimensionRegion]) +} + +// TestGetOnDemandSeries_ContextCancelled verifies that a cancelled context is +// propagated before the first CE call (ctx-cancel-is-terminal rule). +func TestGetOnDemandSeries_ContextCancelled(t *testing.T) { + mock := &mockOnDemandCE{pages: generate30DayPage(time.Now(), 24.0)} + client := newOnDemandClient(mock) + + ctx, cancel := context.WithCancel(context.Background()) + cancel() // cancel immediately before the call + + _, err := client.GetOnDemandSeries(ctx, "us-east-1", 30) + + require.Error(t, err) +} + +// TestGetOnDemandSeries_MissingMetricKey verifies that a day with a missing +// metric key is treated as $0 rather than causing an error. +func TestGetOnDemandSeries_MissingMetricKey(t *testing.T) { + pages := []*costexplorer.GetCostAndUsageOutput{{ + ResultsByTime: []types.ResultByTime{ + dailyResult("2026-01-01", 24.0), + dailyResultNoMetric("2026-01-02"), // missing metric -> treated as 0 + dailyResult("2026-01-03", 48.0), + }, + }} + mock := &mockOnDemandCE{pages: pages} + client := newOnDemandClient(mock) + + series, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 7) + + require.NoError(t, err) + require.Len(t, series, 3) + assert.InDelta(t, 1.0, series[0], 1e-9) + assert.InDelta(t, 0.0, series[1], 1e-9, "missing metric key is treated as $0/hr") + assert.InDelta(t, 2.0, series[2], 1e-9) +} diff --git a/providers/aws/recommendations/parser_sp_additional_test.go b/providers/aws/recommendations/parser_sp_additional_test.go index 25252d13f..70f864d22 100644 --- a/providers/aws/recommendations/parser_sp_additional_test.go +++ b/providers/aws/recommendations/parser_sp_additional_test.go @@ -253,6 +253,10 @@ func (m *mockCostExplorerForSP) GetSavingsPlansUtilization(_ context.Context, _ return &costexplorer.GetSavingsPlansUtilizationOutput{}, nil } +func (m *mockCostExplorerForSP) GetCostAndUsage(_ context.Context, _ *costexplorer.GetCostAndUsageInput, _ ...func(*costexplorer.Options)) (*costexplorer.GetCostAndUsageOutput, error) { + return &costexplorer.GetCostAndUsageOutput{}, nil +} + func TestGetSavingsPlansRecommendations_WithFilters(t *testing.T) { mockAPI := &mockCostExplorerForSP{ responses: map[types.SupportedSavingsPlansType]*costexplorer.GetSavingsPlansPurchaseRecommendationOutput{ @@ -454,6 +458,12 @@ func (m *multiPageSPMock) GetSavingsPlansUtilization( return &costexplorer.GetSavingsPlansUtilizationOutput{}, nil } +func (m *multiPageSPMock) GetCostAndUsage( + _ context.Context, _ *costexplorer.GetCostAndUsageInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetCostAndUsageOutput, error) { + return &costexplorer.GetCostAndUsageOutput{}, nil +} + // alwaysNextPageSPMock returns pages each carrying a non-nil non-empty NextPageToken. type alwaysNextPageSPMock struct { calls int @@ -498,6 +508,12 @@ func (m *alwaysNextPageSPMock) GetSavingsPlansUtilization( return &costexplorer.GetSavingsPlansUtilizationOutput{}, nil } +func (m *alwaysNextPageSPMock) GetCostAndUsage( + _ context.Context, _ *costexplorer.GetCostAndUsageInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetCostAndUsageOutput, error) { + return &costexplorer.GetCostAndUsageOutput{}, nil +} + // TestGetSavingsPlansRecommendations_Paginates asserts multi-page accumulation (issue #692). func TestGetSavingsPlansRecommendations_Paginates(t *testing.T) { mock := &multiPageSPMock{ diff --git a/providers/aws/service_client_test.go b/providers/aws/service_client_test.go index 35ae91c18..8df394714 100644 --- a/providers/aws/service_client_test.go +++ b/providers/aws/service_client_test.go @@ -44,6 +44,10 @@ func (m *mockCostExplorerClient) GetSavingsPlansUtilization(ctx context.Context, return &costexplorer.GetSavingsPlansUtilizationOutput{}, nil } +func (m *mockCostExplorerClient) GetCostAndUsage(_ context.Context, _ *costexplorer.GetCostAndUsageInput, _ ...func(*costexplorer.Options)) (*costexplorer.GetCostAndUsageOutput, error) { + return &costexplorer.GetCostAndUsageOutput{}, nil +} + // newTestRecommendationsClient creates a recommendations client with a mock CE client func newTestRecommendationsClient(ce *mockCostExplorerClient) *recommendations.Client { return recommendations.NewClientWithAPI(ce, "us-east-1") From 089bec53b720f271414f322ffeac459f20695937 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 16 Jul 2026 16:29:00 +0300 Subject: [PATCH 2/5] feat(ladder): wire real adapters into factory; DailyPoint series contract (L2) Replace all seven noop stubs in NewFromAWSConfig with real AWS client adapters so scheduled ladder runs produce plans instead of always erroring at GetUsageBaseline, and upgrade the on-demand series plumbing to the dated DailyPoint contract introduced by #1365. 7-source wiring: riLister -> ec2svc.Client.ListConvertibleReservedInstances spLister -> spListerAdapter{api: *sdksp.Client.DescribeSavingsPlans} riCoverageSource -> recommendations.Client.GetRICoverageMap onDemandSeries -> onDemandSeriesAdapter wrapping recommendations.Client.GetOnDemandSeries (CE GetCostAndUsage) utilizationSource -> recommendations.Client.GetRIUtilization spCoverageSource -> spCoverageAdapter{client: recommendations.Client} spUtilizationSource -> spUtilizationAdapter{client: recommendations.Client} DailyPoint plumbing (#1365 alignment): - recommendations.GetOnDemandSeries now returns []recommendations.DailyCost (Date = midnight-UTC calendar day parsed from the CE period start via ceDateLayout, USDPerHour = daily unblended USD / 24). Map accumulation dedupes by date and lexicographic sort of YYYY-MM-DD keys yields strictly increasing unique UTC days, satisfying the baseline's chronology, in-window coverage, and freshness validations. Malformed CE dates fail loud. All TODO(#1365) markers removed. - ladder.onDemandSeriesAdapter maps []DailyCost -> []DailyPoint at the seam (recommendations cannot import providers/aws/ladder; ladder already imports recommendations). New in adapters.go: - activeSPListAPI narrow interface (DescribeSavingsPlans only) - spListerAdapter: paginates DescribeSavingsPlans filtering to SavingsPlanStateActive, caps at maxSPListPages=20, fails loud on non-numeric Commitment strings (feedback_strict_int_parse) - spCoverageAdapter / spUtilizationAdapter: preserve nil pct when Days==0 Series tests now assert dates as well as values (oldest-first by Date, unique ascending UTC days, last point = most recent CE day), plus a new bad-date fail-loud test. Regression test TestGetLayerStates_RealSPLister_NonZeroExisting verifies a mocked active $2/hr Compute SP produces ExistingUSDPerHour=2.0 (was 0.0 from the noop). Write side (WithWriteSide) remains unwired; errWriteNotWired preserved (L6). Compatible with the #1368 widened exchangeRunner seam (read side untouched). --- providers/aws/ladder/adapters.go | 195 +++++++++++++ providers/aws/ladder/adapters_test.go | 265 ++++++++++++++++++ providers/aws/ladder/factory.go | 126 ++++----- .../aws/recommendations/ondemand_series.go | 86 ++++-- .../recommendations/ondemand_series_test.go | 98 +++++-- 5 files changed, 658 insertions(+), 112 deletions(-) create mode 100644 providers/aws/ladder/adapters.go create mode 100644 providers/aws/ladder/adapters_test.go diff --git a/providers/aws/ladder/adapters.go b/providers/aws/ladder/adapters.go new file mode 100644 index 000000000..ea40dc86d --- /dev/null +++ b/providers/aws/ladder/adapters.go @@ -0,0 +1,195 @@ +package ladder + +import ( + "context" + "fmt" + "strconv" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + sdksp "github.com/aws/aws-sdk-go-v2/service/savingsplans" + sptypes "github.com/aws/aws-sdk-go-v2/service/savingsplans/types" + + cetypes "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" + + "github.com/LeanerCloud/CUDly/providers/aws/recommendations" +) + +// maxSPListPages caps the DescribeSavingsPlans pagination loop to guard against +// a runaway token loop (mirrors the pattern from issue #692 / #1019). +const maxSPListPages = 20 + +// activeSPListAPI is the minimal interface for listing Savings Plans. +// Only DescribeSavingsPlans is needed; the full SavingsPlansAPI from +// providers/aws/services/savingsplans includes purchase and offering methods +// the read-only lister does not require (interface-segregation principle). +// Tests inject a hermetic fake implementing this narrow interface. +type activeSPListAPI interface { + DescribeSavingsPlans(ctx context.Context, params *sdksp.DescribeSavingsPlansInput, optFns ...func(*sdksp.Options)) (*sdksp.DescribeSavingsPlansOutput, error) +} + +// spListerAdapter implements the spLister interface using the AWS Savings Plans +// SDK directly. It filters to Active-state plans only (not queued or +// pending-return, which are not yet generating commitment costs). Commitment +// strings are parsed with strconv.ParseFloat; parse errors fail loud per +// feedback_strict_int_parse and feedback_no_silent_fallbacks. +type spListerAdapter struct { + api activeSPListAPI +} + +// ListActiveSPs lists all active Savings Plans and maps them to the AWSLadder +// view. Only types.SavingsPlanStateActive plans are included; queued and +// pending-return plans are excluded because they have not yet started billing. +// PlanType and Commitment are validated at the boundary: unknown plan types +// and unparseable commitment strings fail loud (feedback_prefer_typed_enums, +// feedback_strict_int_parse). Pagination is fully exhausted (issue #692). +func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error) { + var sps []ActiveSP + var nextToken *string + page := 0 + + for { + if err := ctx.Err(); err != nil { + return nil, fmt.Errorf("ListActiveSPs: context cancelled: %w", err) + } + page++ + if page > maxSPListPages { + return nil, fmt.Errorf("ListActiveSPs: exceeded %d page cap; possible API token loop", maxSPListPages) + } + + input := &sdksp.DescribeSavingsPlansInput{ + States: []sptypes.SavingsPlanState{sptypes.SavingsPlanStateActive}, + NextToken: nextToken, + MaxResults: aws.Int32(100), + } + out, err := a.api.DescribeSavingsPlans(ctx, input) + if err != nil { + return nil, fmt.Errorf("ListActiveSPs: DescribeSavingsPlans page %d: %w", page, err) + } + + for _, sp := range out.SavingsPlans { + entry, err := mapActiveSP(sp) + if err != nil { + return nil, err + } + sps = append(sps, entry) + } + + if out.NextToken == nil || aws.ToString(out.NextToken) == "" { + break + } + nextToken = out.NextToken + } + return sps, nil +} + +// mapActiveSP converts one DescribeSavingsPlans entry to ActiveSP. +// Fails loud on a missing SavingsPlanId (would make the entry un-identifiable) +// and on a non-numeric Commitment string (money path: cannot represent an +// absent number as 0, per feedback_nullable_not_zero and +// feedback_strict_int_parse). +func mapActiveSP(sp sptypes.SavingsPlan) (ActiveSP, error) { + if sp.SavingsPlanId == nil { + return ActiveSP{}, fmt.Errorf("ListActiveSPs: DescribeSavingsPlans returned entry with nil SavingsPlanId") + } + commitment := aws.ToString(sp.Commitment) + hourly, err := strconv.ParseFloat(commitment, 64) + if err != nil { + return ActiveSP{}, fmt.Errorf("ListActiveSPs: cannot parse Commitment %q for SP %s: %w", + commitment, *sp.SavingsPlanId, err) + } + + entry := ActiveSP{ + PlanID: *sp.SavingsPlanId, + PlanType: string(sp.SavingsPlanType), + State: string(sp.State), + Region: aws.ToString(sp.Region), + HourlyCommitmentUSD: hourly, + } + if sp.Start != nil { + if t, err := time.Parse(time.RFC3339, *sp.Start); err == nil { + entry.StartDate = t + } + } + if sp.End != nil { + if t, err := time.Parse(time.RFC3339, *sp.End); err == nil { + entry.EndDate = t + } + } + return entry, nil +} + +// onDemandSeriesAdapter implements onDemandSeriesSource by wrapping +// *recommendations.Client and mapping its []recommendations.DailyCost to this +// package's []DailyPoint. The two types have identical fields (Date midnight +// UTC + USDPerHour); the duplication exists because recommendations cannot +// import providers/aws/ladder (ladder already imports recommendations), so +// the mapping happens here at the seam. Ordering (oldest-first, strictly +// increasing unique UTC days) is guaranteed by the recommendations contract +// and preserved verbatim by the index-for-index copy. +type onDemandSeriesAdapter struct { + client *recommendations.Client +} + +// GetOnDemandSeries fetches the daily on-demand cost series from CE and maps +// each dated entry to a DailyPoint. Errors propagate unchanged (no silent +// fallback); an empty series is already rejected inside the client. +func (a *onDemandSeriesAdapter) GetOnDemandSeries(ctx context.Context, region string, lookbackDays int) ([]DailyPoint, error) { + costs, err := a.client.GetOnDemandSeries(ctx, region, lookbackDays) + if err != nil { + return nil, err + } + points := make([]DailyPoint, len(costs)) + for i, c := range costs { + points[i] = DailyPoint{Date: c.Date, USDPerHour: c.USDPerHour} + } + return points, nil +} + +// spCoverageAdapter implements spCoverageSource by wrapping *recommendations.Client +// and mapping its richer SPCoverageSummary to the local SPCoverageSummary type. +// The mapping preserves the nil-when-Days==0 contract: if CE returned no +// coverage data the recommendations summary has Days==0 and CoveragePct==nil; +// the adapter returns an empty local summary (CoveragePct stays nil, signalling +// "not measured" to the engine rather than "0% coverage"). +type spCoverageAdapter struct { + client *recommendations.Client +} + +// GetSPCoverageSummary fetches SP coverage from CE and maps the result to the +// local SPCoverageSummary type. CE does not support plan-type filtering for +// coverage (the filter contract allows only REGION, SERVICE, LINKED_ACCOUNT, +// and INSTANCE_FAMILY per the GetSavingsPlansCoverageInput SDK doc), so the +// returned summary applies to all SP types in the region. +func (a *spCoverageAdapter) GetSPCoverageSummary(ctx context.Context, region string, lookbackDays int) (SPCoverageSummary, error) { + rich, err := a.client.GetSPCoverageSummary(ctx, region, lookbackDays) + if err != nil { + return SPCoverageSummary{}, err + } + // Preserve nil-when-Days==0: recommendations.SPCoverageSummary.CoveragePct + // is nil when Days==0 (no CE data). The local type propagates that nil so + // the engine treats it as "not yet measured" rather than "zero coverage". + return SPCoverageSummary{CoveragePct: rich.CoveragePct}, nil +} + +// spUtilizationAdapter implements spUtilizationSource by wrapping +// *recommendations.Client and mapping its SPUtilizationSummary to the local +// type. The mapping follows the same nil-propagation contract as +// spCoverageAdapter. +type spUtilizationAdapter struct { + client *recommendations.Client +} + +// GetSPUtilization fetches SP utilization from CE for the given plan type and +// maps the result to the local SPUtilizationSummary type. planType must be a +// valid cetypes.SupportedSavingsPlansType (validated upstream in toSPUtilPlanType +// before this call). region=="" means all regions (for Compute SPs). +func (a *spUtilizationAdapter) GetSPUtilization(ctx context.Context, planType cetypes.SupportedSavingsPlansType, region string, lookbackDays int) (SPUtilizationSummary, error) { + rich, err := a.client.GetSPUtilization(ctx, planType, region, lookbackDays) + if err != nil { + return SPUtilizationSummary{}, err + } + // Propagate nil UtilizationPct: nil means CE returned no data ("not yet + // measured"), which the engine distinguishes from 0% (fully idle layer). + return SPUtilizationSummary{UtilizationPct: rich.UtilizationPct}, nil +} diff --git a/providers/aws/ladder/adapters_test.go b/providers/aws/ladder/adapters_test.go new file mode 100644 index 000000000..6c4866419 --- /dev/null +++ b/providers/aws/ladder/adapters_test.go @@ -0,0 +1,265 @@ +package ladder + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + sdksp "github.com/aws/aws-sdk-go-v2/service/savingsplans" + sptypes "github.com/aws/aws-sdk-go-v2/service/savingsplans/types" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + pkgladder "github.com/LeanerCloud/CUDly/pkg/ladder" +) + +// --------------------------------------------------------------------------- +// spListerAdapter helpers +// --------------------------------------------------------------------------- + +// mockDescribeSP is a hermetic fake for activeSPListAPI. +// pages[0] is served on the first call (no incoming token). +// tokens[i] is the incoming NextToken that selects pages[i+1]. apiErr is +// returned on every call if non-nil. +type mockDescribeSP struct { + pages []*sdksp.DescribeSavingsPlansOutput + tokens []string // tokens[i] = incoming token that selects pages[i+1] + apiErr error + calls int +} + +func (m *mockDescribeSP) DescribeSavingsPlans( + _ context.Context, + params *sdksp.DescribeSavingsPlansInput, + _ ...func(*sdksp.Options), +) (*sdksp.DescribeSavingsPlansOutput, error) { + m.calls++ + if m.apiErr != nil { + return nil, m.apiErr + } + idx := 0 + incoming := aws.ToString(params.NextToken) + for i, tok := range m.tokens { + if tok == incoming { + idx = i + 1 + break + } + } + if idx >= len(m.pages) { + return nil, errors.New("unexpected page token in mockDescribeSP") + } + return m.pages[idx], nil +} + +// makeSPEntry builds a minimal DescribeSavingsPlans response entry. +func makeSPEntry(id, planType, commitment string, state sptypes.SavingsPlanState) sptypes.SavingsPlan { + return sptypes.SavingsPlan{ + SavingsPlanId: aws.String(id), + SavingsPlanType: sptypes.SavingsPlanType(planType), + Commitment: aws.String(commitment), + State: state, + Start: aws.String("2025-01-01T00:00:00Z"), + End: aws.String("2026-01-01T00:00:00Z"), + } +} + +// capturingSPAPI wraps an activeSPListAPI and records the States filter from +// each DescribeSavingsPlans call. Used to assert active-only filtering. +type capturingSPAPI struct { + inner activeSPListAPI + gotStates *[][]sptypes.SavingsPlanState +} + +func (c *capturingSPAPI) DescribeSavingsPlans( + ctx context.Context, + params *sdksp.DescribeSavingsPlansInput, + optFns ...func(*sdksp.Options), +) (*sdksp.DescribeSavingsPlansOutput, error) { + *c.gotStates = append(*c.gotStates, params.States) + return c.inner.DescribeSavingsPlans(ctx, params, optFns...) +} + +// --------------------------------------------------------------------------- +// spListerAdapter unit tests +// --------------------------------------------------------------------------- + +func TestSPLister_HappyPath(t *testing.T) { + sp := makeSPEntry("sp-abc", string(sptypes.SavingsPlanTypeCompute), "1.50", sptypes.SavingsPlanStateActive) + mock := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, + } + lister := &spListerAdapter{api: mock} + + got, err := lister.ListActiveSPs(context.Background()) + + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, "sp-abc", got[0].PlanID) + assert.Equal(t, string(sptypes.SavingsPlanTypeCompute), got[0].PlanType) + assert.InDelta(t, 1.50, got[0].HourlyCommitmentUSD, 1e-9) + assert.Equal(t, string(sptypes.SavingsPlanStateActive), got[0].State) + assert.Equal(t, time.Date(2025, 1, 1, 0, 0, 0, 0, time.UTC), got[0].StartDate) + assert.Equal(t, time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), got[0].EndDate) +} + +// TestSPLister_ActiveOnlyFilter verifies that DescribeSavingsPlans is called +// with the typed SDK enum sptypes.SavingsPlanStateActive, not a string literal. +func TestSPLister_ActiveOnlyFilter(t *testing.T) { + var gotStates [][]sptypes.SavingsPlanState + inner := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{}}, + } + capturer := &capturingSPAPI{inner: inner, gotStates: &gotStates} + lister := &spListerAdapter{api: capturer} + + _, err := lister.ListActiveSPs(context.Background()) + require.NoError(t, err) + require.Len(t, gotStates, 1, "exactly one API call on an empty account") + assert.Equal(t, []sptypes.SavingsPlanState{sptypes.SavingsPlanStateActive}, gotStates[0]) +} + +// TestSPLister_PaginationExhausted verifies that two pages are fetched and +// merged (regression against issue #692 pattern: truncation at page 1). +func TestSPLister_PaginationExhausted(t *testing.T) { + sp1 := makeSPEntry("sp-1", string(sptypes.SavingsPlanTypeCompute), "1.00", sptypes.SavingsPlanStateActive) + sp2 := makeSPEntry("sp-2", string(sptypes.SavingsPlanTypeEc2Instance), "2.00", sptypes.SavingsPlanStateActive) + pages := []*sdksp.DescribeSavingsPlansOutput{ + {SavingsPlans: []sptypes.SavingsPlan{sp1}, NextToken: aws.String("tok1")}, + {SavingsPlans: []sptypes.SavingsPlan{sp2}}, + } + mock := &mockDescribeSP{pages: pages, tokens: []string{"tok1"}} + lister := &spListerAdapter{api: mock} + + got, err := lister.ListActiveSPs(context.Background()) + + require.NoError(t, err) + require.Len(t, got, 2, "both pages must be merged") + assert.Equal(t, "sp-1", got[0].PlanID) + assert.Equal(t, "sp-2", got[1].PlanID) + assert.Equal(t, 2, mock.calls, "two API calls expected") +} + +// TestSPLister_InvalidCommitmentFails verifies fail-loud on a non-numeric +// Commitment string (feedback_strict_int_parse, feedback_no_silent_fallbacks). +func TestSPLister_InvalidCommitmentFails(t *testing.T) { + sp := makeSPEntry("sp-bad", string(sptypes.SavingsPlanTypeCompute), "not-a-number", sptypes.SavingsPlanStateActive) + mock := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, + } + lister := &spListerAdapter{api: mock} + + _, err := lister.ListActiveSPs(context.Background()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot parse Commitment") +} + +// TestSPLister_APIErrorPropagated verifies that a DescribeSavingsPlans error +// is propagated to the caller (no silent swallowing). +func TestSPLister_APIErrorPropagated(t *testing.T) { + sentinel := errors.New("DescribeSavingsPlans failed") + mock := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{}}, + apiErr: sentinel, + } + lister := &spListerAdapter{api: mock} + + _, err := lister.ListActiveSPs(context.Background()) + + require.Error(t, err) + assert.ErrorIs(t, err, sentinel) +} + +// TestSPLister_EmptyResult verifies that an account with no active SPs returns +// an empty (non-nil) slice without error. +func TestSPLister_EmptyResult(t *testing.T) { + mock := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{}}, + } + lister := &spListerAdapter{api: mock} + + got, err := lister.ListActiveSPs(context.Background()) + + require.NoError(t, err) + assert.Empty(t, got) +} + +// TestSPLister_ContextCancelled verifies that a cancelled context terminates +// the listing loop before the first (or any subsequent) API call. +func TestSPLister_ContextCancelled(t *testing.T) { + // First page has a NextToken so a second call would occur without context + // cancellation; cancelling before the first call proves the loop exits. + pages := []*sdksp.DescribeSavingsPlansOutput{ + {NextToken: aws.String("tok1")}, + {}, + } + mock := &mockDescribeSP{pages: pages, tokens: []string{"tok1"}} + lister := &spListerAdapter{api: mock} + + ctx, cancel := context.WithCancel(context.Background()) + cancel() + + _, err := lister.ListActiveSPs(ctx) + require.Error(t, err, "cancelled context must produce an error") +} + +// TestSPLister_NilIDFails verifies fail-loud on a SP entry with a nil +// SavingsPlanId (un-identifiable entry; feedback_no_silent_fallbacks). +func TestSPLister_NilIDFails(t *testing.T) { + sp := sptypes.SavingsPlan{ + SavingsPlanId: nil, // nil: cannot identify + Commitment: aws.String("1.00"), + } + mock := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, + } + lister := &spListerAdapter{api: mock} + + _, err := lister.ListActiveSPs(context.Background()) + + require.Error(t, err) + assert.Contains(t, err.Error(), "nil SavingsPlanId") +} + +// --------------------------------------------------------------------------- +// Regression test: real spListerAdapter wired into New() must produce +// non-zero ExistingUSDPerHour for an active SP. +// Pre-fix behaviour: noop stub always returned 0.0. +// --------------------------------------------------------------------------- + +// TestGetLayerStates_RealSPLister_NonZeroExisting wires a real spListerAdapter +// backed by a hermetic DescribeSavingsPlans mock that returns one active +// Compute SP at $2/hr and asserts that GetLayerStates returns +// ExistingUSDPerHour == 2.0 for LayerComputeSP. +func TestGetLayerStates_RealSPLister_NonZeroExisting(t *testing.T) { + sp := makeSPEntry("sp-real", string(sptypes.SavingsPlanTypeCompute), "2.00", sptypes.SavingsPlanStateActive) + mockSPAPI := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, + } + + cov := &fakeCoverageSource{onDemandPoints: makeRecentPoints(7)} + a, err := New( + Config{Region: "us-east-1", AccountID: "123456789012"}, + &fakeRILister{}, + &spListerAdapter{api: mockSPAPI}, // real adapter, not the noop stub + cov, + cov, + &fakeUtilizationSource{}, + nil, // spCoverageSource: not needed for this assertion + nil, // spUtilizationSource: not needed for this assertion + ) + require.NoError(t, err) + + states, err := a.GetLayerStates(context.Background(), testScope()) + + require.NoError(t, err) + computeSP, ok := states[pkgladder.LayerComputeSP] + require.True(t, ok, "LayerComputeSP must be present") + require.NotNil(t, computeSP.ExistingUSDPerHour, + "ExistingUSDPerHour must not be nil for a layer with an active SP") + assert.InDelta(t, 2.0, *computeSP.ExistingUSDPerHour, 1e-9, + "ExistingUSDPerHour must equal the SP $2/hr commitment (was 0.0 pre-fix)") +} diff --git a/providers/aws/ladder/factory.go b/providers/aws/ladder/factory.go index 9626cdcd3..3156c10b5 100644 --- a/providers/aws/ladder/factory.go +++ b/providers/aws/ladder/factory.go @@ -4,99 +4,87 @@ import ( "context" "fmt" + awsconfig "github.com/aws/aws-sdk-go-v2/config" + sdksp "github.com/aws/aws-sdk-go-v2/service/savingsplans" + pkgladder "github.com/LeanerCloud/CUDly/pkg/ladder" "github.com/LeanerCloud/CUDly/providers/aws/recommendations" ec2svc "github.com/LeanerCloud/CUDly/providers/aws/services/ec2" ) -// NewFromAWSConfig constructs an AWSLadder for the given region and accountID. -// All read-side data-source adapters that are not yet wired in PR-2 are replaced -// by no-op stubs: -// - RI and SP listers return empty slices (no existing commitments is safe for -// a plan-only run: the engine correctly shows the full gap as unhedged). -// - RI coverage and utilization sources return empty results. -// - The on-demand series source returns an explicit error so GetUsageBaseline -// fails loud; executeLadderRun returns before persisting anything, so the -// config is counted Errored for the run (no ladder_runs row is written). -// Real wiring arrives in PR-4 (Cost Explorer data source). +// NewFromAWSConfig constructs a fully wired AWSLadder for the given region and +// accountID. All seven read-side data-source adapters are wired to real AWS +// clients so that scheduled ladder runs produce meaningful plans. +// +// Pre-L2 behaviour: every source was a no-op stub; GetUsageBaseline always +// errored; every run was recorded as Errored with no plan produced. +// +// Client wiring (7 sources): +// +// - riLister : ec2svc.Client.ListConvertibleReservedInstances +// - spLister : spListerAdapter wrapping *sdksp.Client.DescribeSavingsPlans +// - riCoverageSource : recommendations.Client.GetRICoverageMap +// - onDemandSeries : onDemandSeriesAdapter wrapping recommendations.Client.GetOnDemandSeries +// (CE GetCostAndUsage; maps recommendations.DailyCost to DailyPoint) +// - utilizationSource : recommendations.Client.GetRIUtilization +// - spCoverageSource : spCoverageAdapter wrapping recommendations.Client.GetSPCoverageSummary +// - spUtilizationSource: spUtilizationAdapter wrapping recommendations.Client.GetSPUtilization // -// The write side (PurchaseLayer / ReshapeBuffer) is not wired here; all writes -// are rejected with errWriteNotWired, which is correct for the plan-only phase. +// The write side (PurchaseLayer / ReshapeBuffer) remains unwired; all writes +// are rejected with errWriteNotWired. Write-side wiring arrives in L6. // -// NewFromAWSConfig matches the LadderCapabilityFactory signature on Application -// so it can be assigned directly: +// NewFromAWSConfig matches the LadderCapabilityFactory type on Application so +// it can be assigned directly: // // app.LadderCapabilityFactory = awsladder.NewFromAWSConfig -func NewFromAWSConfig(_ context.Context, region, accountID string) (pkgladder.LadderCapability, error) { +func NewFromAWSConfig(ctx context.Context, region, accountID string) (pkgladder.LadderCapability, error) { if region == "" { return nil, fmt.Errorf("awsladder.NewFromAWSConfig: region must not be empty") } if accountID == "" { return nil, fmt.Errorf("awsladder.NewFromAWSConfig: accountID must not be empty") } + + // Load the ambient AWS credentials (Lambda execution role or env vars). + // The ladder region is set here so EC2 and Savings Plans clients use the + // correct regional endpoint. recommendations.NewClient overrides the region + // for Cost Explorer (always us-east-1) internally. + awsCfg, err := awsconfig.LoadDefaultConfig(ctx, + awsconfig.WithRegion(region), + ) + if err != nil { + return nil, fmt.Errorf("awsladder.NewFromAWSConfig: load AWS config: %w", err) + } + + // recommendations.Client satisfies riCoverageSource and utilizationSource + // directly, and is the underlying client for the on-demand series and SP + // coverage/utilization adapters. + recoClient := recommendations.NewClient(awsCfg) + + // ec2svc.Client satisfies riLister (ListConvertibleReservedInstances). + ec2Client := ec2svc.NewClient(awsCfg) + + // sdksp.Client satisfies activeSPListAPI (DescribeSavingsPlans), which is the + // narrow interface spListerAdapter expects (interface-segregation: we do not + // need the offering and purchase methods the full SavingsPlansAPI exposes). + spSDKClient := sdksp.NewFromConfig(awsCfg) + cfg := Config{ Region: region, AccountID: accountID, } l, err := New( cfg, - noopRILister{}, - noopSPLister{}, - noopRICoverageSource{}, - noopOnDemandSeriesSource{}, - noopUtilizationSource{}, - nil, // spCoverageSource: wired when PR-4 lands - nil, // spUtilizationSource: wired when PR-4 lands + ec2Client, // riLister + &spListerAdapter{api: spSDKClient}, // spLister + recoClient, // riCoverageSource + &onDemandSeriesAdapter{client: recoClient}, // onDemandSeriesSource + recoClient, // utilizationSource + &spCoverageAdapter{client: recoClient}, // spCoverageSource + &spUtilizationAdapter{client: recoClient}, // spUtilizationSource ) if err != nil { return nil, fmt.Errorf("awsladder.NewFromAWSConfig: %w", err) } return l, nil } - -// noopRILister satisfies riLister by reporting no active convertible RIs. -// Returning an empty slice is safe: the engine counts ExistingUSDPerHour for -// RI layers as zero and plans the full gap. -type noopRILister struct{} - -func (noopRILister) ListConvertibleReservedInstances(_ context.Context) ([]ec2svc.ConvertibleRI, error) { - return []ec2svc.ConvertibleRI{}, nil -} - -// noopSPLister satisfies spLister by reporting no active Savings Plans. -// Returning an empty slice is safe for the same reason as noopRILister. -type noopSPLister struct{} - -func (noopSPLister) ListActiveSPs(_ context.Context) ([]ActiveSP, error) { - return []ActiveSP{}, nil -} - -// noopRICoverageSource satisfies riCoverageSource with an empty coverage map. -// The engine sets CoveragePct to nil for all RI pools when no coverage data is -// available, which is treated as "not yet measured" (not "zero coverage"). -type noopRICoverageSource struct{} - -func (noopRICoverageSource) GetRICoverageMap(_ context.Context, _ int, _ []string) (recommendations.PoolCoverageMap, error) { - return recommendations.PoolCoverageMap{}, nil -} - -// noopOnDemandSeriesSource satisfies onDemandSeriesSource by returning an -// explicit error. GetUsageBaseline requires a non-empty daily spend series to -// compute the low-water baseline; without it the engine cannot produce a -// meaningful plan. executeLadderRun returns early on the GetUsageBaseline -// error, so the config is counted Errored and no ladder_runs row is persisted. -// Real wiring (Cost Explorer adapter) arrives in PR-4. -type noopOnDemandSeriesSource struct{} - -func (noopOnDemandSeriesSource) GetOnDemandSeries(_ context.Context, _ string, _ int) ([]DailyPoint, error) { - return nil, fmt.Errorf("on-demand series source not yet wired: the Cost Explorer adapter is connected in PR-4; until then plan runs will be recorded as failed") -} - -// noopUtilizationSource satisfies utilizationSource by returning an empty -// slice. The engine treats nil/empty utilization as "not yet measured" and -// leaves UtilizationPct nil for RI layers. -type noopUtilizationSource struct{} - -func (noopUtilizationSource) GetRIUtilization(_ context.Context, _ int) ([]recommendations.RIUtilization, error) { - return []recommendations.RIUtilization{}, nil -} diff --git a/providers/aws/recommendations/ondemand_series.go b/providers/aws/recommendations/ondemand_series.go index 147048e12..466391828 100644 --- a/providers/aws/recommendations/ondemand_series.go +++ b/providers/aws/recommendations/ondemand_series.go @@ -30,15 +30,37 @@ const purchaseTypeOnDemand = "On Demand Instances" // used to derive it so it cannot drift from the CE enum vocabulary. const onDemandMetric = string(types.MetricUnblendedCost) +// ceDateLayout is the calendar-day format CE uses for DateInterval boundaries +// and ResultByTime period starts ("YYYY-MM-DD"). time.Parse with this layout +// yields midnight UTC, which is the normalization the ladder baseline expects. +const ceDateLayout = "2006-01-02" + // maxOnDemandSeriesPages caps the GetCostAndUsage NextPageToken loop. // Daily granularity over a 30-day window produces at most 1 page; the cap // guards against a runaway token loop or API misbehavior and returns a // diagnostic error instead of billing $0.01/call indefinitely. const maxOnDemandSeriesPages = 20 +// DailyCost is one calendar day of on-demand cost from CE GetCostAndUsage. +// Date is midnight UTC of the calendar day the entry covers; USDPerHour is +// the day's total unblended cost divided by 24. The ladder package maps this +// to its own DailyPoint type (identical fields) via a thin adapter; the type +// is duplicated because recommendations cannot import providers/aws/ladder +// (ladder already imports recommendations). +type DailyCost struct { + // Date is the UTC calendar day (midnight UTC) this entry covers. + Date time.Time + // USDPerHour is the on-demand-equivalent spend averaged over the day. + USDPerHour float64 +} + // GetOnDemandSeries fetches daily on-demand EC2 compute cost for the given // region over the past lookbackDays days from CE GetCostAndUsage and returns -// a slice of len(returned_days) USD/hr values ordered oldest-to-newest. +// one DailyCost per returned calendar day, ordered oldest-to-newest with +// strictly increasing unique UTC days (map accumulation dedupes by date; +// lexicographic sort of "YYYY-MM-DD" keys is chronological). The last element +// is the most recent day CE has data for, which the ladder baseline's +// freshness check relies on. // // CE query parameters: // - Granularity: DAILY (one entry per calendar day) @@ -52,20 +74,12 @@ const maxOnDemandSeriesPages = 20 // magic constant. // // CE typically lags ~24-48h, so the returned series may be shorter than -// lookbackDays. The caller (baseline.GetUsageBaseline) enforces the minimum- -// length requirement (minBaselineSeriesDays); this function only errors on a +// lookbackDays. The caller (baseline.GetUsageBaseline) enforces minimum +// length, in-window coverage, and freshness; this function only errors on a // completely empty result. -// -// TODO(#1365): when the feat/ladder-stale-series PR merges, change the -// interface to return []DailyPoint{Date time.Time; USDPerHour float64} instead -// of []float64. The accumulator below already builds per-date entries; the -// final flatten step is the only change needed. -func (c *Client) GetOnDemandSeries(ctx context.Context, region string, lookbackDays int) ([]float64, error) { - if lookbackDays <= 0 { - return nil, fmt.Errorf("GetOnDemandSeries: lookbackDays must be > 0, got %d", lookbackDays) - } - if region == "" { - return nil, fmt.Errorf("GetOnDemandSeries: region must not be empty") +func (c *Client) GetOnDemandSeries(ctx context.Context, region string, lookbackDays int) ([]DailyCost, error) { + if err := validateOnDemandSeriesArgs(region, lookbackDays); err != nil { + return nil, err } end := time.Now().UTC().Truncate(24 * time.Hour) // midnight today (exclusive end for CE) @@ -73,16 +87,14 @@ func (c *Client) GetOnDemandSeries(ctx context.Context, region string, lookbackD input := &costexplorer.GetCostAndUsageInput{ TimePeriod: &types.DateInterval{ - Start: aws.String(start.Format("2006-01-02")), - End: aws.String(end.Format("2006-01-02")), + Start: aws.String(start.Format(ceDateLayout)), + End: aws.String(end.Format(ceDateLayout)), }, Granularity: types.GranularityDaily, Metrics: []string{onDemandMetric}, Filter: onDemandSeriesFilter(region), } - // TODO(#1365): build []struct{Date time.Time; USDPerHour float64} here - // and flatten to []float64 at the return site once the interface changes. byDate := make(map[string]float64) var nextToken *string @@ -119,7 +131,24 @@ func (c *Client) GetOnDemandSeries(ctx context.Context, region string, lookbackD region, lookbackDays) } - return sortedDailySeries(byDate), nil + series, err := sortedDailyCosts(byDate) + if err != nil { + return nil, fmt.Errorf("GetOnDemandSeries: %w", err) + } + return series, nil +} + +// validateOnDemandSeriesArgs rejects out-of-range GetOnDemandSeries arguments +// at the boundary. Extracted to keep GetOnDemandSeries under the cyclomatic +// complexity limit. +func validateOnDemandSeriesArgs(region string, lookbackDays int) error { + if lookbackDays <= 0 { + return fmt.Errorf("GetOnDemandSeries: lookbackDays must be > 0, got %d", lookbackDays) + } + if region == "" { + return fmt.Errorf("GetOnDemandSeries: region must not be empty") + } + return nil } // fetchOnDemandPage calls GetCostAndUsage with rate-limit retry, mirroring @@ -202,19 +231,26 @@ func accumulateDailyResults(byDate map[string]float64, out *costexplorer.GetCost return nil } -// sortedDailySeries converts a date-keyed cost map to a []float64 sorted +// sortedDailyCosts converts a date-keyed cost map to a []DailyCost sorted // oldest-to-newest (lexicographic sort on "2006-01-02" strings is -// chronological). The baseline contract requires oldest-first ordering. -func sortedDailySeries(byDate map[string]float64) []float64 { +// chronological). Map keys are unique, so the result has strictly increasing +// unique UTC days, which the ladder baseline's chronology check requires. +// A date string that does not parse fails loud: it indicates a CE contract +// violation the caller must see, not skip (feedback_no_silent_fallbacks). +func sortedDailyCosts(byDate map[string]float64) ([]DailyCost, error) { dates := make([]string, 0, len(byDate)) for d := range byDate { dates = append(dates, d) } sort.Strings(dates) - series := make([]float64, len(dates)) + series := make([]DailyCost, len(dates)) for i, d := range dates { - series[i] = byDate[d] + day, err := time.Parse(ceDateLayout, d) + if err != nil { + return nil, fmt.Errorf("cannot parse CE period start date %q: %w", d, err) + } + series[i] = DailyCost{Date: day, USDPerHour: byDate[d]} } - return series + return series, nil } diff --git a/providers/aws/recommendations/ondemand_series_test.go b/providers/aws/recommendations/ondemand_series_test.go index 4641d697d..105b31c1b 100644 --- a/providers/aws/recommendations/ondemand_series_test.go +++ b/providers/aws/recommendations/ondemand_series_test.go @@ -78,13 +78,34 @@ func newOnDemandClient(m *mockOnDemandCE) *Client { return NewClientWithAPI(m, "us-east-1") } -// generate30Days builds a single-page mock returning 30 daily entries, +// utcDate parses a "2006-01-02" string into midnight UTC, failing the test +// on malformed input. Mirrors the production ceDateLayout parse so date +// assertions compare like with like. +func utcDate(t *testing.T, dateStr string) time.Time { + t.Helper() + d, err := time.Parse(ceDateLayout, dateStr) + require.NoError(t, err) + return d +} + +// requireStrictlyIncreasingDays asserts the series dates are unique ascending +// UTC calendar days (the ladder baseline's chronology contract). +func requireStrictlyIncreasingDays(t *testing.T, series []DailyCost) { + t.Helper() + for i := 1; i < len(series); i++ { + require.True(t, series[i].Date.After(series[i-1].Date), + "series dates must be strictly increasing: index %d (%s) not after index %d (%s)", + i, series[i].Date.Format(ceDateLayout), i-1, series[i-1].Date.Format(ceDateLayout)) + } +} + +// generate30DayPage builds a single-page mock returning 30 daily entries, // each billing totalPerDay USD total (i.e. totalPerDay/24 USD/hr per entry). func generate30DayPage(startDate time.Time, totalPerDay float64) []*costexplorer.GetCostAndUsageOutput { results := make([]types.ResultByTime, 30) for i := range results { day := startDate.AddDate(0, 0, i) - results[i] = dailyResult(day.Format("2006-01-02"), totalPerDay) + results[i] = dailyResult(day.Format(ceDateLayout), totalPerDay) } return []*costexplorer.GetCostAndUsageOutput{ {ResultsByTime: results}, @@ -92,7 +113,8 @@ func generate30DayPage(startDate time.Time, totalPerDay float64) []*costexplorer } // TestGetOnDemandSeries_HappyPath verifies that 30 daily entries at -// $240/day each return 30 elements of $10/hr ($240/24h). +// $240/day each return 30 dated points of $10/hr ($240/24h) with strictly +// increasing unique UTC days and a fresh (most recent CE day) tail. func TestGetOnDemandSeries_HappyPath(t *testing.T) { start := time.Now().UTC().Truncate(24*time.Hour).AddDate(0, 0, -30) mock := &mockOnDemandCE{pages: generate30DayPage(start, 240.0)} @@ -101,14 +123,22 @@ func TestGetOnDemandSeries_HappyPath(t *testing.T) { series, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 30) require.NoError(t, err) - assert.Len(t, series, 30, "series must have one element per returned day") - for i, v := range series { - assert.InDelta(t, 10.0, v, 1e-9, "day %d: expected $10/hr ($240/24h)", i) + require.Len(t, series, 30, "series must have one element per returned day") + for i, p := range series { + assert.InDelta(t, 10.0, p.USDPerHour, 1e-9, "day %d: expected $10/hr ($240/24h)", i) + assert.Equal(t, start.AddDate(0, 0, i), p.Date, + "day %d: Date must be the UTC day from the CE period start", i) } + requireStrictlyIncreasingDays(t, series) + // Freshness contract: the last point is the most recent CE day (start+29 + // = yesterday), within the baseline's maxSeriesAgeDays. + assert.Equal(t, start.AddDate(0, 0, 29), series[len(series)-1].Date, + "last point must be the most recent CE day") } // TestGetOnDemandSeries_OldestFirst verifies that days are returned in -// chronological (oldest-to-newest) order regardless of map iteration order. +// chronological (oldest-to-newest) order by Date regardless of the order CE +// returned them in. func TestGetOnDemandSeries_OldestFirst(t *testing.T) { // Provide 3 days with distinct costs so order is detectable. pages := []*costexplorer.GetCostAndUsageOutput{{ @@ -125,13 +155,17 @@ func TestGetOnDemandSeries_OldestFirst(t *testing.T) { require.NoError(t, err) require.Len(t, series, 3) - assert.InDelta(t, 1.0, series[0], 1e-9, "oldest day first") - assert.InDelta(t, 2.0, series[1], 1e-9, "middle day second") - assert.InDelta(t, 3.0, series[2], 1e-9, "newest day last") + assert.Equal(t, utcDate(t, "2026-01-01"), series[0].Date, "oldest day first") + assert.InDelta(t, 1.0, series[0].USDPerHour, 1e-9) + assert.Equal(t, utcDate(t, "2026-01-02"), series[1].Date, "middle day second") + assert.InDelta(t, 2.0, series[1].USDPerHour, 1e-9) + assert.Equal(t, utcDate(t, "2026-01-03"), series[2].Date, "newest day last") + assert.InDelta(t, 3.0, series[2].USDPerHour, 1e-9) + requireStrictlyIncreasingDays(t, series) } // TestGetOnDemandSeries_Paginated verifies that two pages are fetched and -// their results merged into a single sorted series. +// their results merged into a single date-sorted series. func TestGetOnDemandSeries_Paginated(t *testing.T) { pages := []*costexplorer.GetCostAndUsageOutput{ { @@ -154,9 +188,13 @@ func TestGetOnDemandSeries_Paginated(t *testing.T) { require.NoError(t, err) require.Len(t, series, 3, "both pages must be merged") - assert.InDelta(t, 1.0, series[0], 1e-9) - assert.InDelta(t, 2.0, series[1], 1e-9) - assert.InDelta(t, 3.0, series[2], 1e-9) + assert.Equal(t, utcDate(t, "2026-01-01"), series[0].Date) + assert.InDelta(t, 1.0, series[0].USDPerHour, 1e-9) + assert.Equal(t, utcDate(t, "2026-01-02"), series[1].Date) + assert.InDelta(t, 2.0, series[1].USDPerHour, 1e-9) + assert.Equal(t, utcDate(t, "2026-01-03"), series[2].Date) + assert.InDelta(t, 3.0, series[2].USDPerHour, 1e-9) + requireStrictlyIncreasingDays(t, series) // Verify exactly 2 CE calls were made (one per page). assert.Len(t, mock.gotInputs, 2, "should have fetched 2 pages") // Second call must carry the token from the first response. @@ -209,6 +247,24 @@ func TestGetOnDemandSeries_ParseErrorFails(t *testing.T) { assert.Contains(t, err.Error(), "cannot parse CE amount") } +// TestGetOnDemandSeries_BadDateFails verifies that a malformed CE period-start +// date fails loud instead of being skipped or misdated +// (feedback_no_silent_fallbacks). +func TestGetOnDemandSeries_BadDateFails(t *testing.T) { + pages := []*costexplorer.GetCostAndUsageOutput{{ + ResultsByTime: []types.ResultByTime{ + dailyResult("not-a-date", 24.0), + }, + }} + mock := &mockOnDemandCE{pages: pages} + client := newOnDemandClient(mock) + + _, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 7) + + require.Error(t, err) + assert.Contains(t, err.Error(), "cannot parse CE period start date") +} + // TestGetOnDemandSeries_CorrectFilterParams verifies the CE query uses the // correct granularity, metric, and three-clause AND filter // (feedback_verify_api_filter_contracts). @@ -263,7 +319,8 @@ func TestGetOnDemandSeries_ContextCancelled(t *testing.T) { } // TestGetOnDemandSeries_MissingMetricKey verifies that a day with a missing -// metric key is treated as $0 rather than causing an error. +// metric key is treated as $0 rather than causing an error, and keeps its +// correct calendar date in the series. func TestGetOnDemandSeries_MissingMetricKey(t *testing.T) { pages := []*costexplorer.GetCostAndUsageOutput{{ ResultsByTime: []types.ResultByTime{ @@ -279,7 +336,12 @@ func TestGetOnDemandSeries_MissingMetricKey(t *testing.T) { require.NoError(t, err) require.Len(t, series, 3) - assert.InDelta(t, 1.0, series[0], 1e-9) - assert.InDelta(t, 0.0, series[1], 1e-9, "missing metric key is treated as $0/hr") - assert.InDelta(t, 2.0, series[2], 1e-9) + assert.Equal(t, utcDate(t, "2026-01-01"), series[0].Date) + assert.InDelta(t, 1.0, series[0].USDPerHour, 1e-9) + assert.Equal(t, utcDate(t, "2026-01-02"), series[1].Date, + "the zero-spend day must keep its calendar date") + assert.InDelta(t, 0.0, series[1].USDPerHour, 1e-9, "missing metric key is treated as $0/hr") + assert.Equal(t, utcDate(t, "2026-01-03"), series[2].Date) + assert.InDelta(t, 2.0, series[2].USDPerHour, 1e-9) + requireStrictlyIncreasingDays(t, series) } From 1db9ad22318ce313b710a1ed806d56c3bf16a239 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 16 Jul 2026 18:42:05 +0300 Subject: [PATCH 3/5] fix(ladder): address L2 review blockers in CE series and SP lister 1. BLOCKER: GetCostAndUsage metric name. string(types.MetricUnblendedCost) yields "UNBLENDED_COST", but this operation's Metrics vocabulary is CamelCase "UnblendedCost" (the types.Metric enum belongs to other CE APIs); every production call would throw ValidationException. Use a plain "UnblendedCost" constant for both the request Metrics field and the per-day r.Total lookup; tests now assert the CamelCase literal so a "cleanup" back to the enum cannot pass. 2. HIGH: no fabricated zeros. A result row missing the metric key now fails loud (CE echoes every requested metric; genuine $0 days arrive as Amount:"0", so a missing key means a request-vocabulary bug). An all-zero series is also rejected: a wrong filter yields complete $0 rows (not an empty result) that would otherwise pass every downstream validation and let the engine size purchases from fabricated data. 3. HIGH: region scoping. DescribeSavingsPlans is account-wide but layer state is region-scoped: EC2Instance SPs bound to another region are now excluded (they would inflate ExistingUSDPerHour and under-purchase). Compute SPs are global and deliberately counted fully in every region (conservative: can only under-purchase); multi-region laddering (L18) will revisit. 4. MEDIUM: SP Start/End dates now fail loud on nil or unparseable values; a silently zero EndDate dropped the SP from sumExpiringSPHourlyCost and understated expiring commitment. 5. MEDIUM: payment-pending SPs are now included as existing commitment (a just-purchased SP must count immediately or the next run double-purchases); queued (future-dated) SPs stay excluded until L14. 6. LOW: added TimePeriod request assertions (today-exclusive window) and direct mapping tests for onDemandSeriesAdapter, spCoverageAdapter, and spUtilizationAdapter against a full CostExplorerAPI mock. Tests: 15 new/updated across adapters_test.go and ondemand_series_test.go. --- providers/aws/ladder/adapters.go | 130 +++++-- providers/aws/ladder/adapters_test.go | 323 ++++++++++++++++-- providers/aws/ladder/factory.go | 10 +- .../aws/recommendations/ondemand_series.go | 100 ++++-- .../recommendations/ondemand_series_test.go | 81 ++++- 5 files changed, 543 insertions(+), 101 deletions(-) diff --git a/providers/aws/ladder/adapters.go b/providers/aws/ladder/adapters.go index ea40dc86d..212436662 100644 --- a/providers/aws/ladder/adapters.go +++ b/providers/aws/ladder/adapters.go @@ -28,21 +28,50 @@ type activeSPListAPI interface { DescribeSavingsPlans(ctx context.Context, params *sdksp.DescribeSavingsPlansInput, optFns ...func(*sdksp.Options)) (*sdksp.DescribeSavingsPlansOutput, error) } -// spListerAdapter implements the spLister interface using the AWS Savings Plans -// SDK directly. It filters to Active-state plans only (not queued or -// pending-return, which are not yet generating commitment costs). Commitment -// strings are parsed with strconv.ParseFloat; parse errors fail loud per -// feedback_strict_int_parse and feedback_no_silent_fallbacks. +// spListerStates is the set of Savings Plan states counted as existing +// commitment. +// - active: currently billing. +// - payment-pending: purchase accepted, first payment in flight. A +// just-purchased SP sits in this state briefly; it MUST count as existing +// commitment immediately, or the next scheduled run would not see it and +// double-purchase. Safe default before the L6 write side lands. +// - queued (future-dated) SPs are deliberately EXCLUDED: they have not +// started, so counting them would suppress purchases the queued SP will +// not cover until its start date. Revisit with L14 (expiry alignment), +// which can reason about future-dated coverage explicitly. +var spListerStates = []sptypes.SavingsPlanState{ + sptypes.SavingsPlanStateActive, + sptypes.SavingsPlanStatePaymentPending, +} + +// spListerAdapter implements the spLister interface using the AWS Savings +// Plans SDK directly. It filters to spListerStates (active + payment-pending; +// see that var for rationale) and scopes EC2Instance SPs to the ladder's +// region. Commitment strings are parsed with strconv.ParseFloat; parse errors +// fail loud per feedback_strict_int_parse and feedback_no_silent_fallbacks. type spListerAdapter struct { api activeSPListAPI + // region is the ladder's configured region. DescribeSavingsPlans is an + // account-wide API, but the ladder's layer state is region-scoped: + // EC2Instance SPs from other regions must not inflate this region's + // ExistingUSDPerHour (that would understate the gap and under-purchase). + region string } -// ListActiveSPs lists all active Savings Plans and maps them to the AWSLadder -// view. Only types.SavingsPlanStateActive plans are included; queued and -// pending-return plans are excluded because they have not yet started billing. -// PlanType and Commitment are validated at the boundary: unknown plan types -// and unparseable commitment strings fail loud (feedback_prefer_typed_enums, -// feedback_strict_int_parse). Pagination is fully exhausted (issue #692). +// ListActiveSPs lists the account's Savings Plans in spListerStates and maps +// them to the AWSLadder view, region-scoped: +// +// - EC2Instance SPs are region-bound; entries whose Region differs from the +// adapter's region are EXCLUDED (they cover other regions' usage). +// - Compute SPs are global and are counted fully in every region. This is +// deliberately conservative: attributing the full global commitment to +// this region can only make the engine see MORE existing coverage and +// purchase LESS, never over-purchase. Multi-region laddering (L18) will +// revisit this attribution. +// +// Commitment strings and SP dates are validated at the boundary and fail +// loud (feedback_strict_int_parse, feedback_no_silent_fallbacks). +// Pagination is fully exhausted (issue #692). func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error) { var sps []ActiveSP var nextToken *string @@ -58,7 +87,7 @@ func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error) } input := &sdksp.DescribeSavingsPlansInput{ - States: []sptypes.SavingsPlanState{sptypes.SavingsPlanStateActive}, + States: spListerStates, NextToken: nextToken, MaxResults: aws.Int32(100), } @@ -67,12 +96,9 @@ func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error) return nil, fmt.Errorf("ListActiveSPs: DescribeSavingsPlans page %d: %w", page, err) } - for _, sp := range out.SavingsPlans { - entry, err := mapActiveSP(sp) - if err != nil { - return nil, err - } - sps = append(sps, entry) + sps, err = appendRegionScopedSPs(sps, out.SavingsPlans, a.region) + if err != nil { + return nil, err } if out.NextToken == nil || aws.ToString(out.NextToken) == "" { @@ -83,11 +109,33 @@ func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error) return sps, nil } +// appendRegionScopedSPs maps one DescribeSavingsPlans page onto the +// accumulated slice, applying the region-scoping rule: EC2Instance SPs bound +// to another region cover that region's usage, not ours; including them +// would inflate ExistingUSDPerHour and under-purchase. Compute SPs are +// global and always kept (see ListActiveSPs doc). Extracted to keep +// ListActiveSPs under the cyclomatic complexity limit. +func appendRegionScopedSPs(sps []ActiveSP, page []sptypes.SavingsPlan, region string) ([]ActiveSP, error) { + for _, sp := range page { + entry, err := mapActiveSP(sp) + if err != nil { + return nil, err + } + if entry.PlanType == spPlanTypeEC2Instance && entry.Region != region { + continue + } + sps = append(sps, entry) + } + return sps, nil +} + // mapActiveSP converts one DescribeSavingsPlans entry to ActiveSP. -// Fails loud on a missing SavingsPlanId (would make the entry un-identifiable) -// and on a non-numeric Commitment string (money path: cannot represent an -// absent number as 0, per feedback_nullable_not_zero and -// feedback_strict_int_parse). +// Fails loud on a missing SavingsPlanId (would make the entry +// un-identifiable), on a non-numeric Commitment string (money path: cannot +// represent an absent number as 0, per feedback_nullable_not_zero and +// feedback_strict_int_parse), and on missing or unparseable Start/End dates +// (a silently zero EndDate would drop the SP from sumExpiringSPHourlyCost +// and understate expiring commitment). func mapActiveSP(sp sptypes.SavingsPlan) (ActiveSP, error) { if sp.SavingsPlanId == nil { return ActiveSP{}, fmt.Errorf("ListActiveSPs: DescribeSavingsPlans returned entry with nil SavingsPlanId") @@ -98,25 +146,41 @@ func mapActiveSP(sp sptypes.SavingsPlan) (ActiveSP, error) { return ActiveSP{}, fmt.Errorf("ListActiveSPs: cannot parse Commitment %q for SP %s: %w", commitment, *sp.SavingsPlanId, err) } + start, err := parseSPDate("Start", sp.Start, *sp.SavingsPlanId) + if err != nil { + return ActiveSP{}, err + } + end, err := parseSPDate("End", sp.End, *sp.SavingsPlanId) + if err != nil { + return ActiveSP{}, err + } - entry := ActiveSP{ + return ActiveSP{ PlanID: *sp.SavingsPlanId, PlanType: string(sp.SavingsPlanType), State: string(sp.State), Region: aws.ToString(sp.Region), + StartDate: start, + EndDate: end, HourlyCommitmentUSD: hourly, + }, nil +} + +// parseSPDate parses a DescribeSavingsPlans RFC3339 date field, failing loud +// on a nil or unparseable value. Expiry math (sumExpiringSPHourlyCost) +// depends on EndDate; a silently zero date would exclude the SP from the +// expiring sum and understate expiring commitment +// (feedback_no_silent_fallbacks). +func parseSPDate(field string, value *string, planID string) (time.Time, error) { + if value == nil { + return time.Time{}, fmt.Errorf("ListActiveSPs: SP %s has nil %s date; expiry math requires it", planID, field) } - if sp.Start != nil { - if t, err := time.Parse(time.RFC3339, *sp.Start); err == nil { - entry.StartDate = t - } - } - if sp.End != nil { - if t, err := time.Parse(time.RFC3339, *sp.End); err == nil { - entry.EndDate = t - } + t, err := time.Parse(time.RFC3339, *value) + if err != nil { + return time.Time{}, fmt.Errorf("ListActiveSPs: cannot parse %s date %q for SP %s: %w", + field, *value, planID, err) } - return entry, nil + return t, nil } // onDemandSeriesAdapter implements onDemandSeriesSource by wrapping diff --git a/providers/aws/ladder/adapters_test.go b/providers/aws/ladder/adapters_test.go index 6c4866419..ec2b6b706 100644 --- a/providers/aws/ladder/adapters_test.go +++ b/providers/aws/ladder/adapters_test.go @@ -7,15 +7,22 @@ import ( "time" "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/costexplorer" sdksp "github.com/aws/aws-sdk-go-v2/service/savingsplans" sptypes "github.com/aws/aws-sdk-go-v2/service/savingsplans/types" + cetypes "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" + "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" pkgladder "github.com/LeanerCloud/CUDly/pkg/ladder" + "github.com/LeanerCloud/CUDly/providers/aws/recommendations" ) +// testRegion is the ladder region used across adapter tests. +const testRegion = "us-east-1" + // --------------------------------------------------------------------------- // spListerAdapter helpers // --------------------------------------------------------------------------- @@ -55,11 +62,13 @@ func (m *mockDescribeSP) DescribeSavingsPlans( } // makeSPEntry builds a minimal DescribeSavingsPlans response entry. -func makeSPEntry(id, planType, commitment string, state sptypes.SavingsPlanState) sptypes.SavingsPlan { +// region is the SP's bound region ("" for global Compute SPs). +func makeSPEntry(id, planType, commitment, region string, state sptypes.SavingsPlanState) sptypes.SavingsPlan { return sptypes.SavingsPlan{ SavingsPlanId: aws.String(id), SavingsPlanType: sptypes.SavingsPlanType(planType), Commitment: aws.String(commitment), + Region: aws.String(region), State: state, Start: aws.String("2025-01-01T00:00:00Z"), End: aws.String("2026-01-01T00:00:00Z"), @@ -67,7 +76,7 @@ func makeSPEntry(id, planType, commitment string, state sptypes.SavingsPlanState } // capturingSPAPI wraps an activeSPListAPI and records the States filter from -// each DescribeSavingsPlans call. Used to assert active-only filtering. +// each DescribeSavingsPlans call. Used to assert the states filter contract. type capturingSPAPI struct { inner activeSPListAPI gotStates *[][]sptypes.SavingsPlanState @@ -82,16 +91,21 @@ func (c *capturingSPAPI) DescribeSavingsPlans( return c.inner.DescribeSavingsPlans(ctx, params, optFns...) } +// newSPLister builds a region-scoped spListerAdapter over the given mock. +func newSPLister(api activeSPListAPI) *spListerAdapter { + return &spListerAdapter{api: api, region: testRegion} +} + // --------------------------------------------------------------------------- // spListerAdapter unit tests // --------------------------------------------------------------------------- func TestSPLister_HappyPath(t *testing.T) { - sp := makeSPEntry("sp-abc", string(sptypes.SavingsPlanTypeCompute), "1.50", sptypes.SavingsPlanStateActive) + sp := makeSPEntry("sp-abc", string(sptypes.SavingsPlanTypeCompute), "1.50", "", sptypes.SavingsPlanStateActive) mock := &mockDescribeSP{ pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, } - lister := &spListerAdapter{api: mock} + lister := newSPLister(mock) got, err := lister.ListActiveSPs(context.Background()) @@ -105,33 +119,84 @@ func TestSPLister_HappyPath(t *testing.T) { assert.Equal(t, time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), got[0].EndDate) } -// TestSPLister_ActiveOnlyFilter verifies that DescribeSavingsPlans is called -// with the typed SDK enum sptypes.SavingsPlanStateActive, not a string literal. -func TestSPLister_ActiveOnlyFilter(t *testing.T) { +// TestSPLister_StatesFilter verifies that DescribeSavingsPlans is called with +// exactly the typed SDK enums for active + payment-pending. payment-pending +// must be included so a just-purchased SP counts as existing commitment +// immediately (otherwise the next run double-purchases); queued stays +// excluded because a future-dated SP does not cover usage until its start. +func TestSPLister_StatesFilter(t *testing.T) { var gotStates [][]sptypes.SavingsPlanState inner := &mockDescribeSP{ pages: []*sdksp.DescribeSavingsPlansOutput{{}}, } capturer := &capturingSPAPI{inner: inner, gotStates: &gotStates} - lister := &spListerAdapter{api: capturer} + lister := newSPLister(capturer) _, err := lister.ListActiveSPs(context.Background()) require.NoError(t, err) require.Len(t, gotStates, 1, "exactly one API call on an empty account") - assert.Equal(t, []sptypes.SavingsPlanState{sptypes.SavingsPlanStateActive}, gotStates[0]) + assert.Equal(t, []sptypes.SavingsPlanState{ + sptypes.SavingsPlanStateActive, + sptypes.SavingsPlanStatePaymentPending, + }, gotStates[0]) + assert.NotContains(t, gotStates[0], sptypes.SavingsPlanStateQueued, + "queued (future-dated) SPs must not be requested") +} + +// TestSPLister_PaymentPendingIncluded verifies a payment-pending SP is mapped +// and returned as existing commitment. +func TestSPLister_PaymentPendingIncluded(t *testing.T) { + sp := makeSPEntry("sp-pending", string(sptypes.SavingsPlanTypeCompute), "3.00", "", sptypes.SavingsPlanStatePaymentPending) + mock := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, + } + lister := newSPLister(mock) + + got, err := lister.ListActiveSPs(context.Background()) + + require.NoError(t, err) + require.Len(t, got, 1, "payment-pending SP must count as existing commitment") + assert.Equal(t, string(sptypes.SavingsPlanStatePaymentPending), got[0].State) + assert.InDelta(t, 3.00, got[0].HourlyCommitmentUSD, 1e-9) +} + +// TestSPLister_OutOfRegionEC2InstanceExcluded verifies the region-scoping +// contract: an EC2Instance SP bound to another region is excluded (it covers +// that region's usage, and including it would inflate this region's +// ExistingUSDPerHour and under-purchase), while a global Compute SP is +// always included. +func TestSPLister_OutOfRegionEC2InstanceExcluded(t *testing.T) { + inRegion := makeSPEntry("sp-ec2-local", string(sptypes.SavingsPlanTypeEc2Instance), "1.00", testRegion, sptypes.SavingsPlanStateActive) + outOfRegion := makeSPEntry("sp-ec2-remote", string(sptypes.SavingsPlanTypeEc2Instance), "2.00", "eu-west-1", sptypes.SavingsPlanStateActive) + globalCompute := makeSPEntry("sp-compute", string(sptypes.SavingsPlanTypeCompute), "4.00", "", sptypes.SavingsPlanStateActive) + mock := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{ + SavingsPlans: []sptypes.SavingsPlan{inRegion, outOfRegion, globalCompute}, + }}, + } + lister := newSPLister(mock) + + got, err := lister.ListActiveSPs(context.Background()) + + require.NoError(t, err) + require.Len(t, got, 2, "out-of-region EC2Instance SP must be excluded") + ids := []string{got[0].PlanID, got[1].PlanID} + assert.Contains(t, ids, "sp-ec2-local", "in-region EC2Instance SP kept") + assert.Contains(t, ids, "sp-compute", "global Compute SP kept in every region") + assert.NotContains(t, ids, "sp-ec2-remote", "eu-west-1 EC2Instance SP must not leak into us-east-1") } // TestSPLister_PaginationExhausted verifies that two pages are fetched and // merged (regression against issue #692 pattern: truncation at page 1). func TestSPLister_PaginationExhausted(t *testing.T) { - sp1 := makeSPEntry("sp-1", string(sptypes.SavingsPlanTypeCompute), "1.00", sptypes.SavingsPlanStateActive) - sp2 := makeSPEntry("sp-2", string(sptypes.SavingsPlanTypeEc2Instance), "2.00", sptypes.SavingsPlanStateActive) + sp1 := makeSPEntry("sp-1", string(sptypes.SavingsPlanTypeCompute), "1.00", "", sptypes.SavingsPlanStateActive) + sp2 := makeSPEntry("sp-2", string(sptypes.SavingsPlanTypeEc2Instance), "2.00", testRegion, sptypes.SavingsPlanStateActive) pages := []*sdksp.DescribeSavingsPlansOutput{ {SavingsPlans: []sptypes.SavingsPlan{sp1}, NextToken: aws.String("tok1")}, {SavingsPlans: []sptypes.SavingsPlan{sp2}}, } mock := &mockDescribeSP{pages: pages, tokens: []string{"tok1"}} - lister := &spListerAdapter{api: mock} + lister := newSPLister(mock) got, err := lister.ListActiveSPs(context.Background()) @@ -145,11 +210,11 @@ func TestSPLister_PaginationExhausted(t *testing.T) { // TestSPLister_InvalidCommitmentFails verifies fail-loud on a non-numeric // Commitment string (feedback_strict_int_parse, feedback_no_silent_fallbacks). func TestSPLister_InvalidCommitmentFails(t *testing.T) { - sp := makeSPEntry("sp-bad", string(sptypes.SavingsPlanTypeCompute), "not-a-number", sptypes.SavingsPlanStateActive) + sp := makeSPEntry("sp-bad", string(sptypes.SavingsPlanTypeCompute), "not-a-number", "", sptypes.SavingsPlanStateActive) mock := &mockDescribeSP{ pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, } - lister := &spListerAdapter{api: mock} + lister := newSPLister(mock) _, err := lister.ListActiveSPs(context.Background()) @@ -157,6 +222,40 @@ func TestSPLister_InvalidCommitmentFails(t *testing.T) { assert.Contains(t, err.Error(), "cannot parse Commitment") } +// TestSPLister_BadDatesFail verifies fail-loud on missing or unparseable +// Start/End dates: a silently zero EndDate would drop the SP from +// sumExpiringSPHourlyCost and understate expiring commitment. +func TestSPLister_BadDatesFail(t *testing.T) { + base := func() sptypes.SavingsPlan { + return makeSPEntry("sp-dates", string(sptypes.SavingsPlanTypeCompute), "1.00", "", sptypes.SavingsPlanStateActive) + } + tests := []struct { + name string + mutate func(*sptypes.SavingsPlan) + wantMsg string + }{ + {"nil Start", func(sp *sptypes.SavingsPlan) { sp.Start = nil }, "nil Start date"}, + {"nil End", func(sp *sptypes.SavingsPlan) { sp.End = nil }, "nil End date"}, + {"garbage Start", func(sp *sptypes.SavingsPlan) { sp.Start = aws.String("garbage") }, "cannot parse Start date"}, + {"garbage End", func(sp *sptypes.SavingsPlan) { sp.End = aws.String("garbage") }, "cannot parse End date"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + sp := base() + tt.mutate(&sp) + mock := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, + } + lister := newSPLister(mock) + + _, err := lister.ListActiveSPs(context.Background()) + + require.Error(t, err) + assert.Contains(t, err.Error(), tt.wantMsg) + }) + } +} + // TestSPLister_APIErrorPropagated verifies that a DescribeSavingsPlans error // is propagated to the caller (no silent swallowing). func TestSPLister_APIErrorPropagated(t *testing.T) { @@ -165,7 +264,7 @@ func TestSPLister_APIErrorPropagated(t *testing.T) { pages: []*sdksp.DescribeSavingsPlansOutput{{}}, apiErr: sentinel, } - lister := &spListerAdapter{api: mock} + lister := newSPLister(mock) _, err := lister.ListActiveSPs(context.Background()) @@ -173,13 +272,13 @@ func TestSPLister_APIErrorPropagated(t *testing.T) { assert.ErrorIs(t, err, sentinel) } -// TestSPLister_EmptyResult verifies that an account with no active SPs returns -// an empty (non-nil) slice without error. +// TestSPLister_EmptyResult verifies that an account with no matching SPs +// returns an empty slice without error. func TestSPLister_EmptyResult(t *testing.T) { mock := &mockDescribeSP{ pages: []*sdksp.DescribeSavingsPlansOutput{{}}, } - lister := &spListerAdapter{api: mock} + lister := newSPLister(mock) got, err := lister.ListActiveSPs(context.Background()) @@ -190,14 +289,12 @@ func TestSPLister_EmptyResult(t *testing.T) { // TestSPLister_ContextCancelled verifies that a cancelled context terminates // the listing loop before the first (or any subsequent) API call. func TestSPLister_ContextCancelled(t *testing.T) { - // First page has a NextToken so a second call would occur without context - // cancellation; cancelling before the first call proves the loop exits. pages := []*sdksp.DescribeSavingsPlansOutput{ {NextToken: aws.String("tok1")}, {}, } mock := &mockDescribeSP{pages: pages, tokens: []string{"tok1"}} - lister := &spListerAdapter{api: mock} + lister := newSPLister(mock) ctx, cancel := context.WithCancel(context.Background()) cancel() @@ -216,7 +313,7 @@ func TestSPLister_NilIDFails(t *testing.T) { mock := &mockDescribeSP{ pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, } - lister := &spListerAdapter{api: mock} + lister := newSPLister(mock) _, err := lister.ListActiveSPs(context.Background()) @@ -224,6 +321,182 @@ func TestSPLister_NilIDFails(t *testing.T) { assert.Contains(t, err.Error(), "nil SavingsPlanId") } +// --------------------------------------------------------------------------- +// Direct mapping tests for the recommendations-backed adapters +// --------------------------------------------------------------------------- + +// ladderCEMock implements recommendations.CostExplorerAPI with canned +// responses so the ladder-side adapters can be exercised against a real +// *recommendations.Client without AWS. +type ladderCEMock struct { + costAndUsage *costexplorer.GetCostAndUsageOutput + spCoverage *costexplorer.GetSavingsPlansCoverageOutput + spUtilization *costexplorer.GetSavingsPlansUtilizationOutput +} + +func (m *ladderCEMock) GetReservationPurchaseRecommendation( + _ context.Context, _ *costexplorer.GetReservationPurchaseRecommendationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { + return &costexplorer.GetReservationPurchaseRecommendationOutput{}, nil +} + +func (m *ladderCEMock) GetSavingsPlansPurchaseRecommendation( + _ context.Context, _ *costexplorer.GetSavingsPlansPurchaseRecommendationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetSavingsPlansPurchaseRecommendationOutput, error) { + return &costexplorer.GetSavingsPlansPurchaseRecommendationOutput{}, nil +} + +func (m *ladderCEMock) GetReservationUtilization( + _ context.Context, _ *costexplorer.GetReservationUtilizationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationUtilizationOutput, error) { + return &costexplorer.GetReservationUtilizationOutput{}, nil +} + +func (m *ladderCEMock) GetReservationCoverage( + _ context.Context, _ *costexplorer.GetReservationCoverageInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationCoverageOutput, error) { + return &costexplorer.GetReservationCoverageOutput{}, nil +} + +func (m *ladderCEMock) GetSavingsPlansCoverage( + _ context.Context, _ *costexplorer.GetSavingsPlansCoverageInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetSavingsPlansCoverageOutput, error) { + if m.spCoverage != nil { + return m.spCoverage, nil + } + return &costexplorer.GetSavingsPlansCoverageOutput{}, nil +} + +func (m *ladderCEMock) GetSavingsPlansUtilization( + _ context.Context, _ *costexplorer.GetSavingsPlansUtilizationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetSavingsPlansUtilizationOutput, error) { + if m.spUtilization != nil { + return m.spUtilization, nil + } + return &costexplorer.GetSavingsPlansUtilizationOutput{}, nil +} + +func (m *ladderCEMock) GetCostAndUsage( + _ context.Context, _ *costexplorer.GetCostAndUsageInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetCostAndUsageOutput, error) { + if m.costAndUsage != nil { + return m.costAndUsage, nil + } + return &costexplorer.GetCostAndUsageOutput{}, nil +} + +// ceDailyRow builds one GetCostAndUsage daily row keyed by the CamelCase +// "UnblendedCost" metric name. The literal MUST stay CamelCase: it mirrors +// what real CE echoes back for a GetCostAndUsage request (the enum-derived +// "UNBLENDED_COST" belongs to other CE APIs). If production regressed to the +// SCREAMING_SNAKE lookup, the r.Total lookup would miss this key and the +// mapping test below would fail with a missing-metric error. +func ceDailyRow(dateStr, amount string) cetypes.ResultByTime { + return cetypes.ResultByTime{ + TimePeriod: &cetypes.DateInterval{Start: aws.String(dateStr), End: aws.String(dateStr)}, + Total: map[string]cetypes.MetricValue{ + "UnblendedCost": {Amount: aws.String(amount)}, + }, + } +} + +// TestOnDemandSeriesAdapter_MapsDailyCostToDailyPoint verifies the direct +// []recommendations.DailyCost -> []DailyPoint mapping: dates and values are +// copied index-for-index, oldest-first. +func TestOnDemandSeriesAdapter_MapsDailyCostToDailyPoint(t *testing.T) { + mock := &ladderCEMock{costAndUsage: &costexplorer.GetCostAndUsageOutput{ + ResultsByTime: []cetypes.ResultByTime{ + ceDailyRow("2026-01-02", "480"), // $20/hr, newer + ceDailyRow("2026-01-01", "240"), // $10/hr, older + }, + }} + adapter := &onDemandSeriesAdapter{client: recommendations.NewClientWithAPI(mock, testRegion)} + + points, err := adapter.GetOnDemandSeries(context.Background(), testRegion, 7) + + require.NoError(t, err) + require.Len(t, points, 2) + assert.Equal(t, time.Date(2026, 1, 1, 0, 0, 0, 0, time.UTC), points[0].Date, "oldest first") + assert.InDelta(t, 10.0, points[0].USDPerHour, 1e-9) + assert.Equal(t, time.Date(2026, 1, 2, 0, 0, 0, 0, time.UTC), points[1].Date) + assert.InDelta(t, 20.0, points[1].USDPerHour, 1e-9) +} + +// TestOnDemandSeriesAdapter_PropagatesError verifies errors pass through the +// adapter unchanged (no silent fallback). +func TestOnDemandSeriesAdapter_PropagatesError(t *testing.T) { + // Empty CE output -> the client errors on the empty series. + adapter := &onDemandSeriesAdapter{client: recommendations.NewClientWithAPI(&ladderCEMock{}, testRegion)} + + _, err := adapter.GetOnDemandSeries(context.Background(), testRegion, 7) + + require.Error(t, err) + assert.Contains(t, err.Error(), "no on-demand data") +} + +// TestSPCoverageAdapter_MapsCoveragePct verifies the direct +// recommendations.SPCoverageSummary -> local SPCoverageSummary mapping with a +// populated CE response: covered $120 + on-demand $120 over 1 day = 50%. +func TestSPCoverageAdapter_MapsCoveragePct(t *testing.T) { + mock := &ladderCEMock{spCoverage: &costexplorer.GetSavingsPlansCoverageOutput{ + SavingsPlansCoverages: []cetypes.SavingsPlansCoverage{{ + Coverage: &cetypes.SavingsPlansCoverageData{ + SpendCoveredBySavingsPlans: aws.String("120"), + OnDemandCost: aws.String("120"), + }, + }}, + }} + adapter := &spCoverageAdapter{client: recommendations.NewClientWithAPI(mock, testRegion)} + + got, err := adapter.GetSPCoverageSummary(context.Background(), testRegion, 1) + + require.NoError(t, err) + require.NotNil(t, got.CoveragePct) + assert.InDelta(t, 50.0, *got.CoveragePct, 1e-9, "covered/(covered+onDemand) = 120/240 = 50%") +} + +// TestSPCoverageAdapter_NilWhenNoData verifies the nil-when-Days==0 contract: +// an empty CE coverage response maps to CoveragePct == nil ("not measured"), +// never a fabricated 0. +func TestSPCoverageAdapter_NilWhenNoData(t *testing.T) { + adapter := &spCoverageAdapter{client: recommendations.NewClientWithAPI(&ladderCEMock{}, testRegion)} + + got, err := adapter.GetSPCoverageSummary(context.Background(), testRegion, 30) + + require.NoError(t, err) + assert.Nil(t, got.CoveragePct, "no CE data must map to nil, not 0%") +} + +// TestSPUtilizationAdapter_MapsUtilizationPct verifies the direct +// recommendations.SPUtilizationSummary -> local SPUtilizationSummary mapping. +func TestSPUtilizationAdapter_MapsUtilizationPct(t *testing.T) { + mock := &ladderCEMock{spUtilization: &costexplorer.GetSavingsPlansUtilizationOutput{ + Total: &cetypes.SavingsPlansUtilizationAggregates{ + Utilization: &cetypes.SavingsPlansUtilization{ + UtilizationPercentage: aws.String("85"), + }, + }, + }} + adapter := &spUtilizationAdapter{client: recommendations.NewClientWithAPI(mock, testRegion)} + + got, err := adapter.GetSPUtilization(context.Background(), cetypes.SupportedSavingsPlansTypeComputeSp, "", 30) + + require.NoError(t, err) + require.NotNil(t, got.UtilizationPct) + assert.InDelta(t, 85.0, *got.UtilizationPct, 1e-9) +} + +// TestSPUtilizationAdapter_NilWhenNoData verifies nil propagation on an empty +// CE utilization response. +func TestSPUtilizationAdapter_NilWhenNoData(t *testing.T) { + adapter := &spUtilizationAdapter{client: recommendations.NewClientWithAPI(&ladderCEMock{}, testRegion)} + + got, err := adapter.GetSPUtilization(context.Background(), cetypes.SupportedSavingsPlansTypeComputeSp, "", 30) + + require.NoError(t, err) + assert.Nil(t, got.UtilizationPct, "no CE data must map to nil, not 0%") +} + // --------------------------------------------------------------------------- // Regression test: real spListerAdapter wired into New() must produce // non-zero ExistingUSDPerHour for an active SP. @@ -235,16 +508,16 @@ func TestSPLister_NilIDFails(t *testing.T) { // Compute SP at $2/hr and asserts that GetLayerStates returns // ExistingUSDPerHour == 2.0 for LayerComputeSP. func TestGetLayerStates_RealSPLister_NonZeroExisting(t *testing.T) { - sp := makeSPEntry("sp-real", string(sptypes.SavingsPlanTypeCompute), "2.00", sptypes.SavingsPlanStateActive) + sp := makeSPEntry("sp-real", string(sptypes.SavingsPlanTypeCompute), "2.00", "", sptypes.SavingsPlanStateActive) mockSPAPI := &mockDescribeSP{ pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, } cov := &fakeCoverageSource{onDemandPoints: makeRecentPoints(7)} a, err := New( - Config{Region: "us-east-1", AccountID: "123456789012"}, + Config{Region: testRegion, AccountID: "123456789012"}, &fakeRILister{}, - &spListerAdapter{api: mockSPAPI}, // real adapter, not the noop stub + newSPLister(mockSPAPI), // real adapter, not the noop stub cov, cov, &fakeUtilizationSource{}, diff --git a/providers/aws/ladder/factory.go b/providers/aws/ladder/factory.go index 3156c10b5..57f3b802c 100644 --- a/providers/aws/ladder/factory.go +++ b/providers/aws/ladder/factory.go @@ -75,12 +75,12 @@ func NewFromAWSConfig(ctx context.Context, region, accountID string) (pkgladder. } l, err := New( cfg, - ec2Client, // riLister - &spListerAdapter{api: spSDKClient}, // spLister - recoClient, // riCoverageSource + ec2Client, // riLister + &spListerAdapter{api: spSDKClient, region: region}, // spLister (region-scoped) + recoClient, // riCoverageSource &onDemandSeriesAdapter{client: recoClient}, // onDemandSeriesSource - recoClient, // utilizationSource - &spCoverageAdapter{client: recoClient}, // spCoverageSource + recoClient, // utilizationSource + &spCoverageAdapter{client: recoClient}, // spCoverageSource &spUtilizationAdapter{client: recoClient}, // spUtilizationSource ) if err != nil { diff --git a/providers/aws/recommendations/ondemand_series.go b/providers/aws/recommendations/ondemand_series.go index 466391828..960c59350 100644 --- a/providers/aws/recommendations/ondemand_series.go +++ b/providers/aws/recommendations/ondemand_series.go @@ -23,12 +23,22 @@ const ec2ComputeService = "Amazon Elastic Compute Cloud - Compute" // so the engine correctly nets it into the gap calculation. const purchaseTypeOnDemand = "On Demand Instances" -// onDemandMetric is the CE metric for GetCostAndUsage. UNBLENDED_COST is the -// actual charge to the account at the billed rate; for on-demand instances -// this equals on-demand rate x hours. Named constant (not magic string) per -// feedback_no_hardcoded_magic_values; the SDK types.MetricUnblendedCost is -// used to derive it so it cannot drift from the CE enum vocabulary. -const onDemandMetric = string(types.MetricUnblendedCost) +// onDemandMetric is the CE metric name for GetCostAndUsage. UnblendedCost is +// the actual charge to the account at the billed rate; for on-demand +// instances this equals on-demand rate x hours. +// +// The valid Metrics vocabulary for THIS operation is CamelCase +// ("AmortizedCost", "BlendedCost", "NetAmortizedCost", "NetUnblendedCost", +// "NormalizedUsageAmount", "UnblendedCost", "UsageQuantity") per the +// GetCostAndUsageInput.Metrics SDK doc (api_op_GetCostAndUsage.go). Do NOT +// substitute string(types.MetricUnblendedCost): that enum yields +// SCREAMING_SNAKE "UNBLENDED_COST", which belongs to other CE APIs +// (recommendation/utilization), and GetCostAndUsage rejects it with a +// ValidationException on every call (feedback_verify_api_filter_contracts: +// verify against the actual API op's doc, not a same-SDK enum). The same +// name keys the per-day r.Total lookup because CE echoes request metric +// names as response keys. +const onDemandMetric = "UnblendedCost" // ceDateLayout is the calendar-day format CE uses for DateInterval boundaries // and ResultByTime period starts ("YYYY-MM-DD"). time.Parse with this layout @@ -64,7 +74,7 @@ type DailyCost struct { // // CE query parameters: // - Granularity: DAILY (one entry per calendar day) -// - Metric: UNBLENDED_COST (actual on-demand billed rate; spec: unblended) +// - Metric: UnblendedCost (actual on-demand billed rate; see onDemandMetric) // - Filter: SERVICE = EC2 compute AND PURCHASE_TYPE = On Demand Instances // AND REGION = region (three-clause AND) // - Time window: [now-lookbackDays, now) exclusive end @@ -75,8 +85,20 @@ type DailyCost struct { // // CE typically lags ~24-48h, so the returned series may be shorter than // lookbackDays. The caller (baseline.GetUsageBaseline) enforces minimum -// length, in-window coverage, and freshness; this function only errors on a -// completely empty result. +// length, in-window coverage, and freshness. +// +// Fail-loud conditions (feedback_no_silent_fallbacks): +// - Empty result: error. +// - A result row missing the requested metric key (or with a nil Amount): +// error. CE echoes every requested metric on every row (genuine $0 days +// arrive as Amount:"0"), so a missing key means the request vocabulary is +// wrong, and fabricating a 0 would silently corrupt the baseline. +// - An all-zero series: error. When a filter or metric name is wrong, CE +// returns a complete, fresh, chronological series of $0 rows that would +// pass every downstream validation and make the engine size purchases +// from fabricated data. An account with genuinely zero on-demand EC2 +// spend for the whole lookback has nothing to ladder, so erroring is +// correct there too. func (c *Client) GetOnDemandSeries(ctx context.Context, region string, lookbackDays int) ([]DailyCost, error) { if err := validateOnDemandSeriesArgs(region, lookbackDays); err != nil { return nil, err @@ -126,12 +148,7 @@ func (c *Client) GetOnDemandSeries(ctx context.Context, region string, lookbackD nextToken = out.NextPageToken } - if len(byDate) == 0 { - return nil, fmt.Errorf("GetOnDemandSeries: CE returned no on-demand data for region %q over the past %d days (account may have no on-demand EC2 spend, or CE data not yet available)", - region, lookbackDays) - } - - series, err := sortedDailyCosts(byDate) + series, err := buildDailySeries(byDate, region, lookbackDays) if err != nil { return nil, fmt.Errorf("GetOnDemandSeries: %w", err) } @@ -203,11 +220,15 @@ func onDemandSeriesFilter(region string) *types.Expression { } // accumulateDailyResults extracts daily USD/hr values from one page of -// GetCostAndUsage results and merges them into byDate. Days with a missing or -// nil metric are stored as 0 (CE may omit a day if spend was exactly $0). -// Days with an unparseable amount string fail loud (feedback_strict_int_parse). -// Dividing by 24 converts daily USD to USD/hr; 24 is derived from the DAILY -// granularity contract (one day = 24 hours), not a magic constant. +// GetCostAndUsage results and merges them into byDate. A row missing the +// requested metric key (or carrying a nil Amount) fails loud: CE echoes +// every requested metric on every returned row, with genuine $0 days arriving +// as Amount:"0" -- a missing key therefore signals a request-vocabulary bug, +// and writing a fabricated 0 would silently corrupt the baseline +// (feedback_no_silent_fallbacks). Unparseable amount strings also fail loud +// (feedback_strict_int_parse). Dividing by 24 converts daily USD to USD/hr; +// 24 is derived from the DAILY granularity contract (one day = 24 hours), +// not a magic constant. func accumulateDailyResults(byDate map[string]float64, out *costexplorer.GetCostAndUsageOutput) error { for _, r := range out.ResultsByTime { if r.TimePeriod == nil || r.TimePeriod.Start == nil { @@ -216,8 +237,8 @@ func accumulateDailyResults(byDate map[string]float64, out *costexplorer.GetCost dateStr := aws.ToString(r.TimePeriod.Start) mv, ok := r.Total[onDemandMetric] if !ok || mv.Amount == nil { - byDate[dateStr] = 0 - continue + return fmt.Errorf("CE result row for day %s is missing the %q metric; CE echoes every requested metric on every row, so this indicates a request-vocabulary bug (genuine $0 days arrive as Amount:\"0\")", + dateStr, onDemandMetric) } usd, err := strconv.ParseFloat(aws.ToString(mv.Amount), 64) if err != nil { @@ -231,26 +252,47 @@ func accumulateDailyResults(byDate map[string]float64, out *costexplorer.GetCost return nil } -// sortedDailyCosts converts a date-keyed cost map to a []DailyCost sorted -// oldest-to-newest (lexicographic sort on "2006-01-02" strings is -// chronological). Map keys are unique, so the result has strictly increasing -// unique UTC days, which the ladder baseline's chronology check requires. -// A date string that does not parse fails loud: it indicates a CE contract -// violation the caller must see, not skip (feedback_no_silent_fallbacks). -func sortedDailyCosts(byDate map[string]float64) ([]DailyCost, error) { +// buildDailySeries converts the date-keyed cost map into the final validated +// []DailyCost: +// - Empty map: error (no data from CE). +// - Sorted oldest-to-newest (lexicographic sort on "2006-01-02" strings is +// chronological). Map keys are unique, so the result has strictly +// increasing unique UTC days, which the ladder baseline's chronology +// check requires. +// - A date string that does not parse fails loud: it indicates a CE +// contract violation the caller must see (feedback_no_silent_fallbacks). +// - An all-zero series fails loud: a wrong filter or metric name makes CE +// return a complete series of $0 rows that would pass every downstream +// validation and let the engine size purchases from fabricated data. An +// account with genuinely zero on-demand EC2 spend over the whole window +// has nothing to ladder, so erroring is correct in that case too. +func buildDailySeries(byDate map[string]float64, region string, lookbackDays int) ([]DailyCost, error) { + if len(byDate) == 0 { + return nil, fmt.Errorf("CE returned no on-demand data for region %q over the past %d days (account may have no on-demand EC2 spend, or CE data not yet available)", + region, lookbackDays) + } + dates := make([]string, 0, len(byDate)) for d := range byDate { dates = append(dates, d) } sort.Strings(dates) + allZero := true series := make([]DailyCost, len(dates)) for i, d := range dates { day, err := time.Parse(ceDateLayout, d) if err != nil { return nil, fmt.Errorf("cannot parse CE period start date %q: %w", d, err) } + if byDate[d] != 0 { + allZero = false + } series[i] = DailyCost{Date: day, USDPerHour: byDate[d]} } + if allZero { + return nil, fmt.Errorf("CE returned an all-zero on-demand series for region %q over the past %d days; either the account has no on-demand EC2 spend to ladder, or the CE filter/metric vocabulary is wrong (a bad filter yields complete $0 rows, not an empty result)", + region, lookbackDays) + } return series, nil } diff --git a/providers/aws/recommendations/ondemand_series_test.go b/providers/aws/recommendations/ondemand_series_test.go index 105b31c1b..0f17c9e0b 100644 --- a/providers/aws/recommendations/ondemand_series_test.go +++ b/providers/aws/recommendations/ondemand_series_test.go @@ -281,8 +281,23 @@ func TestGetOnDemandSeries_CorrectFilterParams(t *testing.T) { assert.Equal(t, types.GranularityDaily, input.Granularity, "granularity must be DAILY for the daily series") - assert.Equal(t, []string{onDemandMetric}, input.Metrics, - "metric must be UNBLENDED_COST") + // The literal CamelCase value is asserted on purpose: GetCostAndUsage's + // Metrics vocabulary is CamelCase ("UnblendedCost"), NOT the + // types.MetricUnblendedCost enum's "UNBLENDED_COST" (that enum belongs to + // other CE APIs and triggers a ValidationException here). Locking the + // literal prevents a "cleanup" back to the enum from passing tests. + assert.Equal(t, []string{"UnblendedCost"}, input.Metrics, + "metric must be the CamelCase UnblendedCost accepted by GetCostAndUsage") + + // TimePeriod contract: [today-lookbackDays, today) with an exclusive end + // at midnight UTC today, so today's partial data is never included. + wantEnd := time.Now().UTC().Truncate(24 * time.Hour) + wantStart := wantEnd.AddDate(0, 0, -7) + require.NotNil(t, input.TimePeriod) + assert.Equal(t, wantStart.Format(ceDateLayout), aws.ToString(input.TimePeriod.Start), + "start must be today-lookbackDays (UTC)") + assert.Equal(t, wantEnd.Format(ceDateLayout), aws.ToString(input.TimePeriod.End), + "end must be midnight UTC today (exclusive: today's partial day excluded)") require.NotNil(t, input.Filter, "filter must be present") require.Len(t, input.Filter.And, 3, "filter must be a three-clause AND") @@ -318,14 +333,62 @@ func TestGetOnDemandSeries_ContextCancelled(t *testing.T) { require.Error(t, err) } -// TestGetOnDemandSeries_MissingMetricKey verifies that a day with a missing -// metric key is treated as $0 rather than causing an error, and keeps its -// correct calendar date in the series. -func TestGetOnDemandSeries_MissingMetricKey(t *testing.T) { +// TestGetOnDemandSeries_MissingMetricKeyFails verifies that a result row +// missing the requested metric key fails loud instead of fabricating a $0 +// day. CE echoes every requested metric on every row (genuine $0 days arrive +// as Amount:"0"), so a missing key means the request vocabulary is wrong; +// silently writing 0 would corrupt the baseline +// (feedback_no_silent_fallbacks). +func TestGetOnDemandSeries_MissingMetricKeyFails(t *testing.T) { + pages := []*costexplorer.GetCostAndUsageOutput{{ + ResultsByTime: []types.ResultByTime{ + dailyResult("2026-01-01", 24.0), + dailyResultNoMetric("2026-01-02"), // missing metric -> hard error + dailyResult("2026-01-03", 48.0), + }, + }} + mock := &mockOnDemandCE{pages: pages} + client := newOnDemandClient(mock) + + _, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 7) + + require.Error(t, err, "a missing metric key must fail loud, not fabricate $0") + assert.Contains(t, err.Error(), "missing the \"UnblendedCost\" metric") + assert.Contains(t, err.Error(), "2026-01-02", "error must name the offending day") +} + +// TestGetOnDemandSeries_AllZeroSeriesFails verifies that a complete series of +// $0 rows is rejected. When a filter or metric name is wrong, CE returns +// every day as a $0 row (not an empty result), producing a fresh, +// chronological all-zero series that would pass every downstream validation +// and make the engine size purchases from fabricated data. An account with +// genuinely zero on-demand spend all window has nothing to ladder, so +// erroring is correct there too. +func TestGetOnDemandSeries_AllZeroSeriesFails(t *testing.T) { + pages := []*costexplorer.GetCostAndUsageOutput{{ + ResultsByTime: []types.ResultByTime{ + dailyResult("2026-01-01", 0), + dailyResult("2026-01-02", 0), + dailyResult("2026-01-03", 0), + }, + }} + mock := &mockOnDemandCE{pages: pages} + client := newOnDemandClient(mock) + + _, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 7) + + require.Error(t, err, "an all-zero series must be rejected") + assert.Contains(t, err.Error(), "all-zero") +} + +// TestGetOnDemandSeries_MixedZeroDaysOK verifies that genuine $0 days +// (Amount:"0") interleaved with non-zero days are accepted and keep their +// calendar dates: only the ALL-zero case is rejected. +func TestGetOnDemandSeries_MixedZeroDaysOK(t *testing.T) { pages := []*costexplorer.GetCostAndUsageOutput{{ ResultsByTime: []types.ResultByTime{ dailyResult("2026-01-01", 24.0), - dailyResultNoMetric("2026-01-02"), // missing metric -> treated as 0 + dailyResult("2026-01-02", 0), // genuine $0 day: Amount:"0" dailyResult("2026-01-03", 48.0), }, }} @@ -334,13 +397,13 @@ func TestGetOnDemandSeries_MissingMetricKey(t *testing.T) { series, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 7) - require.NoError(t, err) + require.NoError(t, err, "genuine zero days mixed with spend must be accepted") require.Len(t, series, 3) assert.Equal(t, utcDate(t, "2026-01-01"), series[0].Date) assert.InDelta(t, 1.0, series[0].USDPerHour, 1e-9) assert.Equal(t, utcDate(t, "2026-01-02"), series[1].Date, "the zero-spend day must keep its calendar date") - assert.InDelta(t, 0.0, series[1].USDPerHour, 1e-9, "missing metric key is treated as $0/hr") + assert.InDelta(t, 0.0, series[1].USDPerHour, 1e-9) assert.Equal(t, utcDate(t, "2026-01-03"), series[2].Date) assert.InDelta(t, 2.0, series[2].USDPerHour, 1e-9) requireStrictlyIncreasingDays(t, series) From 54bee5d8e255a2270884292fbac35b76b349b26c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 16 Jul 2026 18:50:35 +0300 Subject: [PATCH 4/5] fix(ladder): fail loud on empty Region for EC2Instance SPs An EC2Instance-plan SP with an empty Region was silently excluded by the region-scope filter ("" != region), understating existing commitment and over-purchasing. AWS always populates Region for region-bound plans, so an empty one means corrupted data; mapActiveSP now returns an explicit error naming the SP. Adds TestSPLister_EmptyRegionEC2InstanceFails. Also fixes a stale test comment ("UNBLENDED_COST" -> "UnblendedCost") left over from the metric-name blocker fix. --- providers/aws/ladder/adapters.go | 12 ++++++++++-- providers/aws/ladder/adapters_test.go | 19 +++++++++++++++++++ .../recommendations/ondemand_series_test.go | 2 +- 3 files changed, 30 insertions(+), 3 deletions(-) diff --git a/providers/aws/ladder/adapters.go b/providers/aws/ladder/adapters.go index 212436662..28e175792 100644 --- a/providers/aws/ladder/adapters.go +++ b/providers/aws/ladder/adapters.go @@ -133,13 +133,21 @@ func appendRegionScopedSPs(sps []ActiveSP, page []sptypes.SavingsPlan, region st // Fails loud on a missing SavingsPlanId (would make the entry // un-identifiable), on a non-numeric Commitment string (money path: cannot // represent an absent number as 0, per feedback_nullable_not_zero and -// feedback_strict_int_parse), and on missing or unparseable Start/End dates +// feedback_strict_int_parse), on missing or unparseable Start/End dates // (a silently zero EndDate would drop the SP from sumExpiringSPHourlyCost -// and understate expiring commitment). +// and understate expiring commitment), and on an EC2Instance-plan SP with +// an empty Region (AWS always populates Region for region-bound plans, so +// an empty one means corrupted data; letting it fall through to the +// region-scope filter would silently exclude it, understating existing +// commitment and over-purchasing). func mapActiveSP(sp sptypes.SavingsPlan) (ActiveSP, error) { if sp.SavingsPlanId == nil { return ActiveSP{}, fmt.Errorf("ListActiveSPs: DescribeSavingsPlans returned entry with nil SavingsPlanId") } + if string(sp.SavingsPlanType) == spPlanTypeEC2Instance && aws.ToString(sp.Region) == "" { + return ActiveSP{}, fmt.Errorf("ListActiveSPs: EC2Instance SP %s has an empty Region; AWS always populates Region for region-bound plans, so this indicates corrupted data (silently excluding it would understate existing commitment)", + *sp.SavingsPlanId) + } commitment := aws.ToString(sp.Commitment) hourly, err := strconv.ParseFloat(commitment, 64) if err != nil { diff --git a/providers/aws/ladder/adapters_test.go b/providers/aws/ladder/adapters_test.go index ec2b6b706..11d226d95 100644 --- a/providers/aws/ladder/adapters_test.go +++ b/providers/aws/ladder/adapters_test.go @@ -256,6 +256,25 @@ func TestSPLister_BadDatesFail(t *testing.T) { } } +// TestSPLister_EmptyRegionEC2InstanceFails verifies fail-loud on an +// EC2Instance-plan SP with an empty Region: AWS always populates Region for +// region-bound plans, so an empty one means corrupted data. Letting it fall +// through to the region-scope filter would silently exclude it ("" != region), +// understating existing commitment and over-purchasing. +func TestSPLister_EmptyRegionEC2InstanceFails(t *testing.T) { + sp := makeSPEntry("sp-noregion", string(sptypes.SavingsPlanTypeEc2Instance), "1.00", "", sptypes.SavingsPlanStateActive) + mock := &mockDescribeSP{ + pages: []*sdksp.DescribeSavingsPlansOutput{{SavingsPlans: []sptypes.SavingsPlan{sp}}}, + } + lister := newSPLister(mock) + + _, err := lister.ListActiveSPs(context.Background()) + + require.Error(t, err, "empty Region on an EC2Instance SP must fail loud, not be silently excluded") + assert.Contains(t, err.Error(), "empty Region") + assert.Contains(t, err.Error(), "sp-noregion", "error must name the offending SP") +} + // TestSPLister_APIErrorPropagated verifies that a DescribeSavingsPlans error // is propagated to the caller (no silent swallowing). func TestSPLister_APIErrorPropagated(t *testing.T) { diff --git a/providers/aws/recommendations/ondemand_series_test.go b/providers/aws/recommendations/ondemand_series_test.go index 0f17c9e0b..f2d065d32 100644 --- a/providers/aws/recommendations/ondemand_series_test.go +++ b/providers/aws/recommendations/ondemand_series_test.go @@ -51,7 +51,7 @@ func (m *mockOnDemandCE) GetCostAndUsage( } // dailyResult builds one ResultByTime entry for testing. -// dateStr must be in "2006-01-02" format; totalUSD is the day's UNBLENDED_COST. +// dateStr must be in "2006-01-02" format; totalUSD is the day's UnblendedCost. func dailyResult(dateStr string, totalUSD float64) types.ResultByTime { return types.ResultByTime{ TimePeriod: &types.DateInterval{ From 23cac076eea988421e419047c6fc83ca659be89f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 16 Jul 2026 19:33:25 +0300 Subject: [PATCH 5/5] fix(ladder): address CodeRabbit round-1 findings on L2 PR 1. Major (adapters.go): remove the 20-page cap from DescribeSavingsPlans pagination. The API documents no page limit, so the cap silently truncated accounts with more than 2000 Savings Plans (MaxResults=100), understating existing commitment. Pagination now runs until NextToken is empty, guarded only against a REPEATED token (API misbehavior). Tests: TestSPLister_DeepPaginationNoCap (25 pages, above the removed cap) and TestSPLister_RepeatedTokenFails. 2. Major (ondemand_series.go): a CE result row missing its TimePeriod/Start was silently skipped; the undatable gap could still pass downstream minimum-length checks and yield an incorrect baseline. Now fails loud naming the row index (same class as the missing-metric-key fix). Test: TestGetOnDemandSeries_MissingPeriodStartFails. 3. Minor (ondemand_series_test.go): stale dailyResultNoMetric doc comment said the absent metric "falls back to zero"; production fails loud and the test asserts the failure. Wording corrected. --- providers/aws/ladder/adapters.go | 22 +++++---- providers/aws/ladder/adapters_test.go | 46 +++++++++++++++++++ .../aws/recommendations/ondemand_series.go | 26 ++++++----- .../recommendations/ondemand_series_test.go | 27 ++++++++++- 4 files changed, 100 insertions(+), 21 deletions(-) diff --git a/providers/aws/ladder/adapters.go b/providers/aws/ladder/adapters.go index 28e175792..4c2708f56 100644 --- a/providers/aws/ladder/adapters.go +++ b/providers/aws/ladder/adapters.go @@ -15,10 +15,6 @@ import ( "github.com/LeanerCloud/CUDly/providers/aws/recommendations" ) -// maxSPListPages caps the DescribeSavingsPlans pagination loop to guard against -// a runaway token loop (mirrors the pattern from issue #692 / #1019). -const maxSPListPages = 20 - // activeSPListAPI is the minimal interface for listing Savings Plans. // Only DescribeSavingsPlans is needed; the full SavingsPlansAPI from // providers/aws/services/savingsplans includes purchase and offering methods @@ -71,10 +67,16 @@ type spListerAdapter struct { // // Commitment strings and SP dates are validated at the boundary and fail // loud (feedback_strict_int_parse, feedback_no_silent_fallbacks). -// Pagination is fully exhausted (issue #692). +// +// Pagination is fully exhausted (issue #692): DescribeSavingsPlans documents +// no page limit, so a fixed page cap would silently truncate accounts with +// more Savings Plans than the cap (understating existing commitment). The +// only loop guard needed is against a REPEATED NextToken, which indicates +// API misbehavior rather than a legitimately long listing. func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error) { var sps []ActiveSP var nextToken *string + seenTokens := make(map[string]struct{}) page := 0 for { @@ -82,9 +84,6 @@ func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error) return nil, fmt.Errorf("ListActiveSPs: context cancelled: %w", err) } page++ - if page > maxSPListPages { - return nil, fmt.Errorf("ListActiveSPs: exceeded %d page cap; possible API token loop", maxSPListPages) - } input := &sdksp.DescribeSavingsPlansInput{ States: spListerStates, @@ -101,9 +100,14 @@ func (a *spListerAdapter) ListActiveSPs(ctx context.Context) ([]ActiveSP, error) return nil, err } - if out.NextToken == nil || aws.ToString(out.NextToken) == "" { + tok := aws.ToString(out.NextToken) + if tok == "" { break } + if _, dup := seenTokens[tok]; dup { + return nil, fmt.Errorf("ListActiveSPs: DescribeSavingsPlans returned a repeated pagination token on page %d; aborting to avoid an infinite loop", page) + } + seenTokens[tok] = struct{}{} nextToken = out.NextToken } return sps, nil diff --git a/providers/aws/ladder/adapters_test.go b/providers/aws/ladder/adapters_test.go index 11d226d95..743bedc36 100644 --- a/providers/aws/ladder/adapters_test.go +++ b/providers/aws/ladder/adapters_test.go @@ -3,6 +3,7 @@ package ladder import ( "context" "errors" + "fmt" "testing" "time" @@ -207,6 +208,51 @@ func TestSPLister_PaginationExhausted(t *testing.T) { assert.Equal(t, 2, mock.calls, "two API calls expected") } +// TestSPLister_DeepPaginationNoCap verifies there is NO fixed page cap: +// DescribeSavingsPlans documents no page limit, so an account with more SPs +// than any arbitrary cap must still be fully listed (a truncated listing +// would understate existing commitment). 25 pages > the removed 20-page cap. +func TestSPLister_DeepPaginationNoCap(t *testing.T) { + const n = 25 + pages := make([]*sdksp.DescribeSavingsPlansOutput, n) + tokens := make([]string, n-1) + for i := 0; i < n; i++ { + sp := makeSPEntry(fmt.Sprintf("sp-%02d", i), string(sptypes.SavingsPlanTypeCompute), "1.00", "", sptypes.SavingsPlanStateActive) + out := &sdksp.DescribeSavingsPlansOutput{SavingsPlans: []sptypes.SavingsPlan{sp}} + if i < n-1 { + tok := fmt.Sprintf("tok%02d", i+1) + out.NextToken = aws.String(tok) + tokens[i] = tok + } + pages[i] = out + } + mock := &mockDescribeSP{pages: pages, tokens: tokens} + lister := newSPLister(mock) + + got, err := lister.ListActiveSPs(context.Background()) + + require.NoError(t, err, "deep pagination must not hit an artificial page cap") + assert.Len(t, got, n, "all %d pages must be listed", n) + assert.Equal(t, n, mock.calls) +} + +// TestSPLister_RepeatedTokenFails verifies the loop guard that replaced the +// page cap: a NextToken that repeats indicates API misbehavior and must abort +// with an error instead of looping forever. +func TestSPLister_RepeatedTokenFails(t *testing.T) { + pages := []*sdksp.DescribeSavingsPlansOutput{ + {NextToken: aws.String("tok1")}, + {NextToken: aws.String("tok1")}, // same token again -> loop + } + mock := &mockDescribeSP{pages: pages, tokens: []string{"tok1"}} + lister := newSPLister(mock) + + _, err := lister.ListActiveSPs(context.Background()) + + require.Error(t, err, "a repeated pagination token must abort the loop") + assert.Contains(t, err.Error(), "repeated pagination token") +} + // TestSPLister_InvalidCommitmentFails verifies fail-loud on a non-numeric // Commitment string (feedback_strict_int_parse, feedback_no_silent_fallbacks). func TestSPLister_InvalidCommitmentFails(t *testing.T) { diff --git a/providers/aws/recommendations/ondemand_series.go b/providers/aws/recommendations/ondemand_series.go index 960c59350..abbac66b4 100644 --- a/providers/aws/recommendations/ondemand_series.go +++ b/providers/aws/recommendations/ondemand_series.go @@ -220,19 +220,23 @@ func onDemandSeriesFilter(region string) *types.Expression { } // accumulateDailyResults extracts daily USD/hr values from one page of -// GetCostAndUsage results and merges them into byDate. A row missing the -// requested metric key (or carrying a nil Amount) fails loud: CE echoes -// every requested metric on every returned row, with genuine $0 days arriving -// as Amount:"0" -- a missing key therefore signals a request-vocabulary bug, -// and writing a fabricated 0 would silently corrupt the baseline -// (feedback_no_silent_fallbacks). Unparseable amount strings also fail loud -// (feedback_strict_int_parse). Dividing by 24 converts daily USD to USD/hr; -// 24 is derived from the DAILY granularity contract (one day = 24 hours), -// not a magic constant. +// GetCostAndUsage results and merges them into byDate. All malformed rows +// fail loud (feedback_no_silent_fallbacks): +// - A row missing its TimePeriod/Start cannot be dated; silently skipping +// it would leave an incomplete series that can still pass downstream +// minimum-length checks and produce an incorrect baseline. +// - A row missing the requested metric key (or carrying a nil Amount) +// signals a request-vocabulary bug: CE echoes every requested metric on +// every returned row, with genuine $0 days arriving as Amount:"0"; +// writing a fabricated 0 would silently corrupt the baseline. +// - Unparseable amount strings fail loud (feedback_strict_int_parse). +// +// Dividing by 24 converts daily USD to USD/hr; 24 is derived from the DAILY +// granularity contract (one day = 24 hours), not a magic constant. func accumulateDailyResults(byDate map[string]float64, out *costexplorer.GetCostAndUsageOutput) error { - for _, r := range out.ResultsByTime { + for i, r := range out.ResultsByTime { if r.TimePeriod == nil || r.TimePeriod.Start == nil { - continue + return fmt.Errorf("CE result row %d is missing its period start; a row that cannot be dated would leave an undetectable gap in the daily series", i) } dateStr := aws.ToString(r.TimePeriod.Start) mv, ok := r.Total[onDemandMetric] diff --git a/providers/aws/recommendations/ondemand_series_test.go b/providers/aws/recommendations/ondemand_series_test.go index f2d065d32..b5f8d582c 100644 --- a/providers/aws/recommendations/ondemand_series_test.go +++ b/providers/aws/recommendations/ondemand_series_test.go @@ -65,7 +65,8 @@ func dailyResult(dateStr string, totalUSD float64) types.ResultByTime { } // dailyResultNoMetric builds a ResultByTime entry with a missing metric key, -// exercising the "metric absent" branch that falls back to 0. +// exercising the missing-metric error path (production fails loud rather +// than fabricating a $0 day). func dailyResultNoMetric(dateStr string) types.ResultByTime { return types.ResultByTime{ TimePeriod: &types.DateInterval{Start: aws.String(dateStr)}, @@ -357,6 +358,30 @@ func TestGetOnDemandSeries_MissingMetricKeyFails(t *testing.T) { assert.Contains(t, err.Error(), "2026-01-02", "error must name the offending day") } +// TestGetOnDemandSeries_MissingPeriodStartFails verifies that a result row +// missing its TimePeriod/Start fails loud instead of being silently skipped: +// an undatable row dropped from the series leaves a gap that can still pass +// downstream minimum-length checks and produce an incorrect baseline. +func TestGetOnDemandSeries_MissingPeriodStartFails(t *testing.T) { + pages := []*costexplorer.GetCostAndUsageOutput{{ + ResultsByTime: []types.ResultByTime{ + dailyResult("2026-01-01", 24.0), + { // undatable row: nil TimePeriod + TimePeriod: nil, + Total: map[string]types.MetricValue{onDemandMetric: {Amount: aws.String("48")}}, + }, + }, + }} + mock := &mockOnDemandCE{pages: pages} + client := newOnDemandClient(mock) + + _, err := client.GetOnDemandSeries(context.Background(), "us-east-1", 7) + + require.Error(t, err, "a row missing its period start must fail loud, not be skipped") + assert.Contains(t, err.Error(), "missing its period start") + assert.Contains(t, err.Error(), "row 1", "error must name the offending row index") +} + // TestGetOnDemandSeries_AllZeroSeriesFails verifies that a complete series of // $0 rows is rejected. When a filter or metric name is wrong, CE returns // every day as a $0 row (not an empty result), producing a fresh,