diff --git a/providers/aws/ladder/commitment_rates.go b/providers/aws/ladder/commitment_rates.go index f7908d1e..34922a98 100644 --- a/providers/aws/ladder/commitment_rates.go +++ b/providers/aws/ladder/commitment_rates.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "math" "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" @@ -15,8 +16,15 @@ type spCommitmentRateSource interface { GetSPCommitmentRate(context.Context, recommendations.SPCommitmentRateParams) (float64, error) } -// GetCommitmentRates leaves Compute absent pending the caller's sizing policy; -// regional EC2 Instance recommendations are not a Compute pricing proxy. +// rateLayer is one layer whose commitment rate is fetched independently. +type rateLayer struct { + layer ladder.LayerType + plan types.SupportedSavingsPlansType + region string +} + +// GetCommitmentRates fetches one rate per layer; a layer without a rate is +// absent (hold) and never affects the others. func (a *AWSLadder) GetCommitmentRates(ctx context.Context, scope ladder.Scope, term ladder.Term, payment ladder.PaymentOption) (map[ladder.LayerType]float64, error) { if err := a.validateScope(scope); err != nil { return nil, err @@ -30,17 +38,35 @@ func (a *AWSLadder) GetCommitmentRates(ctx context.Context, scope ladder.Scope, if a.spRates == nil { return nil, errors.New("GetCommitmentRates: Savings Plan rate source not wired") } - rate, err := a.spRates.GetSPCommitmentRate(ctx, recommendations.SPCommitmentRateParams{ - PlanType: types.SupportedSavingsPlansTypeEc2InstanceSp, - AccountID: a.cfg.AccountID, Region: a.cfg.Region, - Term: string(term), PaymentOption: string(payment), + // Compute SP stays absent until the owner decides (#349). Its account-wide + // blend over EC2+Lambda+Fargate biases sizing of a regional EC2-only gap. + layers := []rateLayer{ + {layer: ladder.LayerEC2InstanceSP, plan: types.SupportedSavingsPlansTypeEc2InstanceSp, region: a.cfg.Region}, + } + return fetchLayerRates(ctx, a.spRates, layers, recommendations.SPCommitmentRateParams{ + AccountID: a.cfg.AccountID, + Term: string(term), PaymentOption: string(payment), LookbackPeriod: fmt.Sprintf("%dd", a.cfg.lookbackDays()), }) - if errors.Is(err, recommendations.ErrNoSPCommitmentRate) { - return map[ladder.LayerType]float64{}, nil - } - if err != nil { - return nil, fmt.Errorf("GetCommitmentRates: %w", err) +} + +// fetchLayerRates sets PlanType and Region from each layer on a copy of base. +func fetchLayerRates(ctx context.Context, src spCommitmentRateSource, layers []rateLayer, base recommendations.SPCommitmentRateParams) (map[ladder.LayerType]float64, error) { + rates := make(map[ladder.LayerType]float64, len(layers)) + for _, l := range layers { + p := base + p.PlanType, p.Region = l.plan, l.region + rate, err := src.GetSPCommitmentRate(ctx, p) + if errors.Is(err, recommendations.ErrNoSPCommitmentRate) { + continue + } + if err != nil { + return nil, fmt.Errorf("GetCommitmentRates: layer %s: %w", l.layer, err) + } + if math.IsNaN(rate) || math.IsInf(rate, 0) || rate <= 0 { + return nil, fmt.Errorf("GetCommitmentRates: layer %s: unusable rate %v", l.layer, rate) + } + rates[l.layer] = rate } - return map[ladder.LayerType]float64{ladder.LayerEC2InstanceSP: rate}, nil + return rates, nil } diff --git a/providers/aws/ladder/commitment_rates_test.go b/providers/aws/ladder/commitment_rates_test.go index 261477b3..30564db9 100644 --- a/providers/aws/ladder/commitment_rates_test.go +++ b/providers/aws/ladder/commitment_rates_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "fmt" + "math" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -16,9 +17,21 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" ) +type fakeRateResult struct { + rate float64 + err error +} + +// fakeSPRateSource answers per plan type; an unlisted plan type is a test bug. type fakeSPRateSource struct { - err error - params []recommendations.SPCommitmentRateParams + results map[types.SupportedSavingsPlansType]fakeRateResult + params []recommendations.SPCommitmentRateParams +} + +func ec2Fake(rate float64, err error) *fakeSPRateSource { + return &fakeSPRateSource{results: map[types.SupportedSavingsPlansType]fakeRateResult{ + types.SupportedSavingsPlansTypeEc2InstanceSp: {rate, err}, + }} } func TestNewFromConfig_ValidatesAndScopesClients(t *testing.T) { @@ -39,7 +52,11 @@ func TestNewFromConfig_ValidatesAndScopesClients(t *testing.T) { func (f *fakeSPRateSource) GetSPCommitmentRate(_ context.Context, p recommendations.SPCommitmentRateParams) (float64, error) { f.params = append(f.params, p) - return 0.7, f.err + r, ok := f.results[p.PlanType] + if !ok { + return 0, fmt.Errorf("unexpected plan type %s", p.PlanType) + } + return r.rate, r.err } func TestGetCommitmentRates_ValidatesBeforeSource(t *testing.T) { @@ -56,7 +73,7 @@ func TestGetCommitmentRates_ValidatesBeforeSource(t *testing.T) { {"payment", testScope(), ladder.Term1Year, ladder.PaymentOption("bad")}, } { t.Run(tc.name, func(t *testing.T) { - f := &fakeSPRateSource{} + f := ec2Fake(0.7, nil) a := &AWSLadder{cfg: Config{Region: "us-east-1", AccountID: testScope().AccountID}, spRates: f} rates, err := a.GetCommitmentRates(context.Background(), tc.scope, tc.term, tc.payment) require.Error(t, err) @@ -70,7 +87,7 @@ func TestGetCommitmentRates_ForwardsSelection(t *testing.T) { t.Parallel() for _, days := range []int{0, 7, 60} { t.Run(fmt.Sprint(days), func(t *testing.T) { - f := &fakeSPRateSource{} + f := ec2Fake(0.7, nil) a := &AWSLadder{cfg: Config{Region: "eu-west-1", AccountID: testScope().AccountID, LookbackDays: days}, spRates: f} rates, err := a.GetCommitmentRates(context.Background(), testScope(), ladder.Term3Year, ladder.PaymentPartialUpfront) require.NoError(t, err) @@ -96,8 +113,8 @@ func TestGetCommitmentRates_AbsenceAndErrors(t *testing.T) { want error }{ {"unwired", nil, nil}, - {"absent", &fakeSPRateSource{err: fmt.Errorf("wrapped: %w", recommendations.ErrNoSPCommitmentRate)}, nil}, - {"hard", &fakeSPRateSource{err: hard}, hard}, + {"absent", ec2Fake(0, fmt.Errorf("wrapped: %w", recommendations.ErrNoSPCommitmentRate)), nil}, + {"hard", ec2Fake(0, hard), hard}, } { t.Run(tc.name, func(t *testing.T) { a := newTestLadder(t, &fakeRILister{}, &fakeSPLister{}, &fakeCoverageSource{}, &fakeUtilizationSource{}) @@ -117,3 +134,64 @@ func TestGetCommitmentRates_AbsenceAndErrors(t *testing.T) { }) } } + +func TestGetCommitmentRates_RejectsUnusableRate(t *testing.T) { + t.Parallel() + for name, rate := range map[string]float64{"nan": math.NaN(), "inf": math.Inf(1), "zero": 0, "negative": -0.3} { + t.Run(name, func(t *testing.T) { + a := &AWSLadder{cfg: Config{Region: "us-east-1", AccountID: testScope().AccountID}, spRates: ec2Fake(rate, nil)} + rates, err := a.GetCommitmentRates(context.Background(), testScope(), ladder.Term1Year, ladder.PaymentNoUpfront) + require.ErrorContains(t, err, string(ladder.LayerEC2InstanceSP)) + assert.Nil(t, rates) + }) + } +} + +func TestGetCommitmentRates_HardErrorNamesLayer(t *testing.T) { + t.Parallel() + hard := errors.New("source failed") + a := &AWSLadder{cfg: Config{Region: "us-east-1", AccountID: testScope().AccountID}, spRates: ec2Fake(0, hard)} + _, err := a.GetCommitmentRates(context.Background(), testScope(), ladder.Term1Year, ladder.PaymentNoUpfront) + require.ErrorIs(t, err, hard) + assert.ErrorContains(t, err, string(ladder.LayerEC2InstanceSP)) +} + +func TestFetchLayerRates_LayersAreIndependent(t *testing.T) { + t.Parallel() + const ( + planA = types.SupportedSavingsPlansTypeEc2InstanceSp + planB = types.SupportedSavingsPlansTypeComputeSp + ) + layers := []rateLayer{ + {layer: ladder.LayerEC2InstanceSP, plan: planA, region: "eu-west-1"}, + {layer: ladder.LayerComputeSP, plan: planB, region: ""}, + } + missing := fmt.Errorf("none: %w", recommendations.ErrNoSPCommitmentRate) + hard := errors.New("source failed") + for _, tc := range []struct { + name string + a, b fakeRateResult + want map[ladder.LayerType]float64 + wantErr error + }{ + {"first missing second present", fakeRateResult{err: missing}, fakeRateResult{rate: 0.8}, map[ladder.LayerType]float64{ladder.LayerComputeSP: 0.8}, nil}, + {"first present second hard", fakeRateResult{rate: 0.7}, fakeRateResult{err: hard}, nil, hard}, + {"both missing", fakeRateResult{err: missing}, fakeRateResult{err: missing}, map[ladder.LayerType]float64{}, nil}, + {"both present", fakeRateResult{rate: 0.7}, fakeRateResult{rate: 0.8}, map[ladder.LayerType]float64{ladder.LayerEC2InstanceSP: 0.7, ladder.LayerComputeSP: 0.8}, nil}, + } { + t.Run(tc.name, func(t *testing.T) { + f := &fakeSPRateSource{results: map[types.SupportedSavingsPlansType]fakeRateResult{planA: tc.a, planB: tc.b}} + got, err := fetchLayerRates(context.Background(), f, layers, recommendations.SPCommitmentRateParams{AccountID: "acct"}) + if tc.wantErr != nil { + require.ErrorIs(t, err, tc.wantErr) + assert.Nil(t, got) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + require.Len(t, f.params, 2) + assert.Equal(t, "eu-west-1", f.params[0].Region) + assert.Equal(t, planB, f.params[1].PlanType) + }) + } +}