diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index de1900bad..e04c6e6e1 100644 --- a/internal/api/handler_purchases.go +++ b/internal/api/handler_purchases.go @@ -1504,7 +1504,7 @@ func (h *Handler) requirePlanStepCap(ctx context.Context, execution *config.Purc if err != nil { return fmt.Errorf("failed to read plan %s for the approval cap check: %w", execution.PlanID, err) } - if !plan.AutoPurchase && plan.MaxStepCommitment == nil { + if !purchase.StepCapApplies(plan) { return nil } if capErr := purchase.CheckStepCommitmentCap(plan, execution.Recommendations); capErr != nil { diff --git a/internal/api/handler_purchases_plan_step_approve_test.go b/internal/api/handler_purchases_plan_step_approve_test.go index 460620706..f6bd6b2c0 100644 --- a/internal/api/handler_purchases_plan_step_approve_test.go +++ b/internal/api/handler_purchases_plan_step_approve_test.go @@ -279,3 +279,33 @@ func TestApproveConstraintsForSession_NilSession_NonUSDCappedPlanStep(t *testing _, err := newTokenApprove(t, nil, autoPlan(&capAmount)).handler.approveViaToken(ctx, &events.LambdaFunctionURLRequest{}, exec, "valid-token") requireStatus(t, err, 404) // the 401 is concealed as not-found } + +// A manual bound plan with group caps and no plan cap: the approval gate runs +// only because it shares StepCapApplies with the attach path (#1746), so an +// over-cap group, or a rec outside every group, is refused for the email-link +// approver too. +func TestApproveViaToken_PlanStep_ManualBoundPlanGroupCaps(t *testing.T) { + exec, total := planStepExecution(t) + key, err := config.RecIdentityKey(&exec.Recommendations[0]) + require.NoError(t, err) + bound := func(groupCap *float64, ids ...string) *config.PurchasePlan { + return &config.PurchasePlan{ + ID: stepPlanID, + RampSchedule: config.RampSchedule{AllocationMode: config.RampAllocationBoundGroups}, + Services: map[string]config.ServiceConfig{"azure/compute#g1": { + Provider: "azure", Service: "compute", Enabled: true, MinCount: 1, GroupID: "g1", + BoundIdentities: ids, MaxStepCommitment: groupCap, + }}, + } + } + under, over := total+1, total-1 + + f := newTokenApprove(t, nil, bound(&under, key)) + requireApproveReached(t, f.purchase, f.approve(exec)) + + f = newTokenApprove(t, nil, bound(&over, key)) + requireApproveRefused(t, f.purchase, f.approve(exec), 403) + + f = newTokenApprove(t, nil, bound(&under, "some-other-identity")) + requireApproveRefused(t, f.purchase, f.approve(exec), 403) +} diff --git a/internal/purchase/errnotes.go b/internal/purchase/errnotes.go index 770ae7bbd..98efa74c1 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -25,7 +25,7 @@ var serverNotePatterns = []string{ regexp.QuoteMeta(armedRedriveLookupRefusal), `stale Azure recommendation pricing; refresh recommendations and submit a new purchase(?:: the latest collection state is missing, so Azure prices cannot be confirmed current)?`, `(?:execution ` + uuidPattern + `: )?plan step has no recommendations attached, so it cannot buy anything(?: \(platform#631: plan steps cannot yet carry recommendations\))?`, - `plan step refused by commitment cap(?:: plan has no max_step_commitment set \(auto-purchase of steps with recommendations is refused until one is set\)|: step total commitment \d+\.\d{2} exceeds max_step_commitment \d+\.\d{2})?`, + `plan step refused by commitment cap(?:: plan has no max_step_commitment set \(auto-purchase of steps with recommendations is refused until one is set\)|: step total commitment \d+\.\d{2} exceeds max_step_commitment \d+\.\d{2}|` + boundGroupCapNotePatterns + `)?`, `scheduled purchase skipped: it was due too long ago(?:: it was due at \d{4}-\d{2}-\d{2}T[0-9:.]+Z, [0-9a-z.]+ ago, which is more than the [0-9a-z.]+ limit \(SCHEDULED_PURCHASE_MAX_OVERDUE\); nothing was submitted, retry it if the purchase is still wanted)?`, `purchase refused after claim: the claimed execution no longer passes the authorization checks`, `refusing to execute ladder purchase: (?:global config unavailable|ladder planning is switched off|ladder execution is switched off|its tranche status is unknown|its tranche is [a-z_]+|its ladder config no longer exists|its ladder config is disabled|ladder config mode is "[a-z_]*", not auto_approve|its ladder run no longer exists|its ladder run was canceled|its ladder run status "[a-z_]{1,32}" is unknown)`, @@ -101,6 +101,16 @@ const ( // whitespace, quotes or newline. Anything wider falls back to the fixed head. amountPattern = `\d+(?:\.\d+)?` shortValuePattern = `[A-Za-z0-9._+-]{1,32}` + // groupKeyPattern is a bound group's services key ("aws/ec2#g1"); a key with + // any other character falls back to the fixed head. + groupKeyPattern = `[A-Za-z0-9._:/#+-]{1,128}` + // boundGroupCapNotePatterns are the group-level refusals of a bound plan's step + // cap (step_cap.go): a group over or without a usable cap, or a rec in two + // groups. They name only group keys and numbers. "Outside every group" names a + // rec id, which carries an account, so it keeps only the head. + boundGroupCapNotePatterns = `: group "` + groupKeyPattern + `" step commitment \d+\.\d{2} exceeds its max_step_commitment \d+\.\d{2}` + + `|: group "` + groupKeyPattern + `" has no usable max_step_commitment \((?:NaN|[+-]?Inf|-?\d+(?:\.\d+)?(?:e[+-]\d+)?)\)` + + `|: a recommendation is bound by two groups \("` + groupKeyPattern + `" and "` + groupKeyPattern + `"\)` // uuidPattern is a Postgres UUID as it renders (plan, execution and cloud // account IDs). commitmentIDPattern covers provider commitment IDs and ARNs; // identityKeyPattern a RecIdentityKey with "/" separators (skippedNote). diff --git a/internal/purchase/plan_step_attach.go b/internal/purchase/plan_step_attach.go index d4754b67b..8450eaae7 100644 --- a/internal/purchase/plan_step_attach.go +++ b/internal/purchase/plan_step_attach.go @@ -184,7 +184,7 @@ func (m *Manager) attachStep(ctx context.Context, tx pgx.Tx, locked *config.Purc } func (m *Manager) attachWithinCap(ctx context.Context, tx pgx.Tx, locked *config.PurchasePlan, claim stepClaim, root *config.PurchaseExecution, recs []config.RecommendationRecord) (stepClaim, error) { - if locked.AutoPurchase || locked.MaxStepCommitment != nil { + if StepCapApplies(locked) { if capErr := CheckStepCommitmentCap(locked, recs); capErr != nil { return m.failStep(ctx, tx, locked, claim, root, fmt.Sprintf("%v (recommendations: %s)", capErr, recIDs(recs))) } diff --git a/internal/purchase/plan_step_attach_integration_test.go b/internal/purchase/plan_step_attach_integration_test.go index a10e63b65..45c49713b 100644 --- a/internal/purchase/plan_step_attach_integration_test.go +++ b/internal/purchase/plan_step_attach_integration_test.go @@ -152,6 +152,40 @@ func TestAttachIntegration_NoMatchAndCap(t *testing.T) { assert.Contains(t, rows[0].Error, "rec-a") assert.Contains(t, rows[0].Error, "exceeds max_step_commitment 1.00") }) + t.Run("a manual bound plan with an over-cap group and no plan cap fails the step", func(t *testing.T) { + // Only StepCapApplies makes the attach path run the cap check for this plan + // (manual, no plan cap): its group cap and its group mapping are held here too. + rec := attachRec("rec-a", 5) + key, err := config.RecIdentityKey(&rec) + require.NoError(t, err) + next := time.Now().Add(24 * time.Hour).Truncate(time.Microsecond) + plan := &config.PurchasePlan{ + Name: "attach-bound-manual", Enabled: true, NotificationDaysBefore: 30, + Services: map[string]config.ServiceConfig{"aws/ec2#g1": { + Provider: "aws", Service: "ec2", Enabled: true, Term: 1, Payment: "no-upfront", Coverage: 80, + MinCount: 1, GroupID: "g1", BoundIdentities: []string{key}, MaxStepCommitment: f64(1), + }}, + RampSchedule: config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4, StartDate: next.AddDate(0, 0, -7), AllocationMode: config.RampAllocationBoundGroups}, + NextExecutionDate: &next, + } + require.NoError(t, plan.Validate()) + require.NoError(t, store.CreatePurchasePlan(ctx, plan)) + // A manual plan attaches only into its pre-created empty root row. + root := &config.PurchaseExecution{PlanID: plan.ID, ExecutionID: "00000000-0000-4000-8000-0000000000a2", Status: "pending", StepNumber: 1, ScheduledDate: time.Now().Add(40 * 24 * time.Hour)} + require.NoError(t, store.SavePurchaseExecution(ctx, root)) + sender := newCountingSender(ctx) + m := &Manager{config: store, email: sender, resolver: &fakeResolver{recs: []config.RecommendationRecord{rec}}} + + require.False(t, m.sendPlanNotification(ctx, plan, false)) + + rows := stepRowsInDB(ctx, t, store, plan.ID) + require.Len(t, rows, 1) + assert.Equal(t, root.ExecutionID, rows[0].ExecutionID) + assert.Equal(t, "failed", rows[0].Status) + assert.Empty(t, rows[0].Recommendations) + assert.Contains(t, rows[0].Error, `group "aws/ec2#g1"`) + assert.Contains(t, rows[0].Error, "exceeds its max_step_commitment 1.00") + }) t.Run("pre-created empty root row is attached in place", func(t *testing.T) { plan := attachPlanInDB(ctx, t, store, "attach-root", time.Now().Add(24*time.Hour)) root := &config.PurchaseExecution{PlanID: plan.ID, ExecutionID: "00000000-0000-4000-8000-0000000000a1", Status: "pending", StepNumber: 1, ScheduledDate: time.Now().Add(40 * 24 * time.Hour)} diff --git a/internal/purchase/step_cap.go b/internal/purchase/step_cap.go index be0649733..180bcb04f 100644 --- a/internal/purchase/step_cap.go +++ b/internal/purchase/step_cap.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "math" + "sort" "strings" "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" @@ -14,6 +15,23 @@ import ( // ErrStepCommitmentCap marks a plan step refused by the per-plan spend cap. var ErrStepCommitmentCap = errors.New("plan step refused by commitment cap") +// ErrRecOutsideBoundGroups marks a bound plan step (issue #1746) holding a +// recommendation that no group bound. It wraps ErrStepCommitmentCap, so every +// cap site refuses it the same way. +var ErrRecOutsideBoundGroups = fmt.Errorf("%w: recommendation is outside every bound group", ErrStepCommitmentCap) + +// StepCapApplies reports whether CheckStepCommitmentCap must run for plan: +// auto-purchase plans, plans with a cap, and bound plans, whose recommendations +// must map to a group whether or not any cap is set. The attach and email +// approval sites share it so they cannot drift. +func StepCapApplies(plan *config.PurchasePlan) bool { + return plan != nil && (plan.AutoPurchase || plan.MaxStepCommitment != nil || isBoundPlan(plan)) +} + +func isBoundPlan(plan *config.PurchasePlan) bool { + return plan.RampSchedule.AllocationMode == config.RampAllocationBoundGroups +} + // CheckStepCommitmentCap refuses a plan step's recommendations unless the plan // carries a cap and the step's TOTAL commitment (upfront + monthly * term * 12, // see RecommendationRecord.TotalCommitment) fits under it. It fails closed: @@ -32,8 +50,11 @@ func CheckStepCommitmentCap(plan *config.PurchasePlan, recs []config.Recommendat if len(recs) == 0 { return nil } - if plan == nil || plan.MaxStepCommitment == nil || !(*plan.MaxStepCommitment > 0) || math.IsInf(*plan.MaxStepCommitment, 0) { - return fmt.Errorf("%w: plan has no max_step_commitment set (auto-purchase of steps with recommendations is refused until one is set)", ErrStepCommitmentCap) + if plan != nil && isBoundPlan(plan) { + return checkBoundStepCap(plan, recs) + } + if !planCapUsable(plan) { + return errNoPlanCap() } var total float64 for i := range recs { @@ -49,6 +70,121 @@ func CheckStepCommitmentCap(plan *config.PurchasePlan, recs []config.Recommendat return nil } +func planCapUsable(plan *config.PurchasePlan) bool { + return plan != nil && plan.MaxStepCommitment != nil && capUsable(*plan.MaxStepCommitment) +} + +// capUsable reports whether a cap is a positive finite number. NaN is not: it +// fails every comparison, so as a cap it would silently let any step through. +func capUsable(c float64) bool { + return c > 0 && !math.IsInf(c, 0) +} + +func errNoPlanCap() error { + return fmt.Errorf("%w: plan has no max_step_commitment set (auto-purchase of steps with recommendations is refused until one is set)", ErrStepCommitmentCap) +} + +// checkBoundStepCap is CheckStepCommitmentCap for a bound plan (issue #1746). +// Every rec must belong to a group by its identity. An auto-purchase plan +// still needs a plan cap; a manual one (a human approves each row) may have +// none. A rec's commitment is checked only where a cap applies to it: the +// plan cap, or its group's cap. Each group stays under its own cap, and the +// total under the plan cap, so the effective cap is min(group, plan). +func checkBoundStepCap(plan *config.PurchasePlan, recs []config.RecommendationRecord) error { + planCap := planCapUsable(plan) + if !planCap && (plan.AutoPurchase || plan.MaxStepCommitment != nil) { + return errNoPlanCap() + } + groupOf, err := boundGroupOf(plan) + if err != nil { + return err + } + totals, total, err := boundStepTotals(plan, recs, groupOf, planCap) + if err != nil { + return err + } + if planCap && total > *plan.MaxStepCommitment { + return fmt.Errorf("%w: step total commitment %.2f exceeds max_step_commitment %.2f", ErrStepCommitmentCap, total, *plan.MaxStepCommitment) + } + return checkBoundGroupCaps(plan, totals) +} + +// boundGroupOf maps each bound identity key to the service key of its group. +// A disabled group is skipped, as the resolver skips it, so its identities are +// outside every group. An identity bound by two groups is refused: validation +// rejects it, but a stored plan may hold one. Keys are visited in order so the +// pair named in the refusal is the same on every check. +func boundGroupOf(plan *config.PurchasePlan) (map[string]string, error) { + keys := make([]string, 0, len(plan.Services)) + for k := range plan.Services { + keys = append(keys, k) + } + sort.Strings(keys) + groupOf := make(map[string]string) + for _, k := range keys { + if !plan.Services[k].Enabled { + continue + } + for _, id := range plan.Services[k].BoundIdentities { + if prev, dup := groupOf[id]; dup && prev != k { + return nil, fmt.Errorf("%w: a recommendation is bound by two groups (%q and %q)", ErrStepCommitmentCap, prev, k) + } + groupOf[id] = k + } + } + return groupOf, nil +} + +// boundStepTotals maps every rec to its bound group and sums the commitments a +// cap applies to: all of them under a plan cap, otherwise only those whose +// group has its own cap. A rec outside every group is refused. +func boundStepTotals(plan *config.PurchasePlan, recs []config.RecommendationRecord, groupOf map[string]string, planCap bool) (totals map[string]float64, total float64, err error) { + totals = map[string]float64{} + for i := range recs { + key, keyErr := config.RecIdentityKey(&recs[i]) + if keyErr != nil { + return nil, 0, fmt.Errorf("%w: recommendation %q: %w", ErrStepCommitmentCap, recs[i].ID, keyErr) + } + g, ok := groupOf[key] + if !ok { + return nil, 0, fmt.Errorf("%w (recommendation %q)", ErrRecOutsideBoundGroups, recs[i].ID) + } + if !planCap && plan.Services[g].MaxStepCommitment == nil { + continue + } + c, capErr := capCommitment(&recs[i]) + if capErr != nil { + return nil, 0, capErr + } + totals[g] += c + total += c + } + return totals, total, nil +} + +// checkBoundGroupCaps refuses the first group, in key order, whose cap is +// unusable or that is over its own cap. +func checkBoundGroupCaps(plan *config.PurchasePlan, totals map[string]float64) error { + keys := make([]string, 0, len(totals)) + for g := range totals { + keys = append(keys, g) + } + sort.Strings(keys) + for _, g := range keys { + gc := plan.Services[g].MaxStepCommitment + if gc == nil { + continue + } + if !capUsable(*gc) { + return fmt.Errorf("%w: group %q has no usable max_step_commitment (%v)", ErrStepCommitmentCap, g, *gc) + } + if totals[g] > *gc { + return fmt.Errorf("%w: group %q step commitment %.2f exceeds its max_step_commitment %.2f", ErrStepCommitmentCap, g, totals[g], *gc) + } + } + return nil +} + // capCommitment returns a rec's total commitment for the cap comparison, or // refuses it. The cap is USD with no FX conversion, so a set non-USD currency // is refused; an empty (unknown) currency passes. diff --git a/internal/purchase/step_cap_bound_test.go b/internal/purchase/step_cap_bound_test.go new file mode 100644 index 000000000..791c64a13 --- /dev/null +++ b/internal/purchase/step_cap_bound_test.go @@ -0,0 +1,253 @@ +package purchase + +import ( + "errors" + "math" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Fixtures are hand-built plans and recs, not real configuration (issue #1746). + +func capRec(region string, upfront float64, currency string) config.RecommendationRecord { + acct := "acct-1" + return config.RecommendationRecord{ + ID: "aws|acct-1|ec2|" + region, Provider: "aws", Service: "ec2", Region: region, ResourceType: "m5.large", + Term: 1, Payment: "all-upfront", Count: 1, UpfrontCost: upfront, CloudAccountID: &acct, CommitmentCurrency: currency, + } +} + +func identity(t *testing.T, r config.RecommendationRecord) string { + t.Helper() + k, err := config.RecIdentityKey(&r) + require.NoError(t, err) + return k +} + +func boundCapPlan(auto bool, planCap *float64, groups map[string]config.ServiceConfig) *config.PurchasePlan { + return &config.PurchasePlan{ID: "plan-c", Enabled: true, AutoPurchase: auto, MaxStepCommitment: planCap, + RampSchedule: config.RampSchedule{AllocationMode: config.RampAllocationBoundGroups}, Services: groups} +} + +func group(cap *float64, ids ...string) config.ServiceConfig { + return config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: true, MinCount: 1, BoundIdentities: ids, MaxStepCommitment: cap} +} + +func usd(v float64) *float64 { return &v } + +func TestCheckStepCommitmentCap_BoundPlans(t *testing.T) { + east, west := capRec("us-east-1", 100, ""), capRec("us-west-2", 100, "") + ke, kw := identity(t, east), identity(t, west) + recs := []config.RecommendationRecord{east, west} + + t.Run("a group over its own cap is refused, naming the group", func(t *testing.T) { + p := boundCapPlan(true, usd(1000), map[string]config.ServiceConfig{"aws/ec2#g1": group(usd(150), ke, kw)}) + err := CheckStepCommitmentCap(p, recs) + require.ErrorIs(t, err, ErrStepCommitmentCap) + assert.ErrorContains(t, err, "aws/ec2#g1") + }) + t.Run("groups within their caps and the plan cap pass", func(t *testing.T) { + p := boundCapPlan(true, usd(1000), map[string]config.ServiceConfig{ + "aws/ec2#g1": group(usd(150), ke), "aws/ec2#g2": group(usd(150), kw), + }) + assert.NoError(t, CheckStepCommitmentCap(p, recs)) + }) + t.Run("the plan cap still applies to the total", func(t *testing.T) { + p := boundCapPlan(true, usd(150), map[string]config.ServiceConfig{ + "aws/ec2#g1": group(usd(150), ke), "aws/ec2#g2": group(usd(150), kw), + }) + require.ErrorIs(t, CheckStepCommitmentCap(p, recs), ErrStepCommitmentCap) + }) + t.Run("a rec in no group's bound set is refused", func(t *testing.T) { + p := boundCapPlan(true, usd(1000), map[string]config.ServiceConfig{"aws/ec2#g1": group(nil, ke)}) + err := CheckStepCommitmentCap(p, recs) + require.ErrorIs(t, err, ErrRecOutsideBoundGroups) + require.ErrorIs(t, err, ErrStepCommitmentCap, "refused like any cap failure at every site") + }) + t.Run("an auto plan with no plan cap is refused even with group caps", func(t *testing.T) { + p := boundCapPlan(true, nil, map[string]config.ServiceConfig{"aws/ec2#g1": group(usd(500), ke, kw)}) + require.ErrorIs(t, CheckStepCommitmentCap(p, recs), ErrStepCommitmentCap) + }) + t.Run("a manual plan with no caps only checks the mapping", func(t *testing.T) { + nonUSD := capRec("eu-west-1", 100, "EUR") + p := boundCapPlan(false, nil, map[string]config.ServiceConfig{"aws/ec2#g1": group(nil, ke, identity(t, nonUSD))}) + assert.NoError(t, CheckStepCommitmentCap(p, []config.RecommendationRecord{east, nonUSD})) + p.Services["aws/ec2#g1"] = group(nil, ke) + require.ErrorIs(t, CheckStepCommitmentCap(p, []config.RecommendationRecord{east, west}), ErrRecOutsideBoundGroups) + }) + t.Run("a manual plan with no plan cap still checks group caps", func(t *testing.T) { + p := boundCapPlan(false, nil, map[string]config.ServiceConfig{"aws/ec2#g1": group(usd(150), ke, kw)}) + require.ErrorIs(t, CheckStepCommitmentCap(p, recs), ErrStepCommitmentCap) + nonUSD := capRec("eu-west-1", 10, "EUR") + p.Services["aws/ec2#g1"] = group(usd(150), identity(t, nonUSD)) + require.ErrorIs(t, CheckStepCommitmentCap(p, []config.RecommendationRecord{nonUSD}), ErrStepCommitmentCap, "a capped group still needs USD prices") + }) +} + +func TestStepCapApplies(t *testing.T) { + cap := usd(100) + assert.False(t, StepCapApplies(&config.PurchasePlan{}), "(guard) a manual uncapped plan skips the check, as before") + assert.True(t, StepCapApplies(&config.PurchasePlan{AutoPurchase: true})) + assert.True(t, StepCapApplies(&config.PurchasePlan{MaxStepCommitment: cap})) + assert.True(t, StepCapApplies(&config.PurchasePlan{RampSchedule: config.RampSchedule{AllocationMode: config.RampAllocationBoundGroups}}), + "a bound plan always runs the mapping check") + assert.False(t, StepCapApplies(nil)) +} + +// (guard) An unmarked plan keeps today's rules exactly. +func TestCheckStepCommitmentCap_UnmarkedUnchanged(t *testing.T) { + recs := []config.RecommendationRecord{capRec("us-east-1", 100, "")} + p := &config.PurchasePlan{AutoPurchase: true, Services: map[string]config.ServiceConfig{"aws/ec2": {Provider: "aws", Service: "ec2", Enabled: true}}} + require.ErrorIs(t, CheckStepCommitmentCap(p, recs), ErrStepCommitmentCap) + p.MaxStepCommitment = usd(150) + assert.NoError(t, CheckStepCommitmentCap(p, recs)) + assert.False(t, errors.Is(CheckStepCommitmentCap(p, recs), ErrRecOutsideBoundGroups)) +} + +// A step exactly at a cap passes, as for an unbound plan; one cent over is +// refused. The same holds for the plan cap and for a group cap. +func TestCheckStepCommitmentCap_BoundCapBoundary(t *testing.T) { + rec := capRec("us-east-1", 100, "") + recs := []config.RecommendationRecord{rec} + key := identity(t, rec) + for _, tc := range []struct { + name string + plan func(cap float64) *config.PurchasePlan + wantErr bool + }{ + {"plan cap", func(c float64) *config.PurchasePlan { + return boundCapPlan(true, usd(c), map[string]config.ServiceConfig{"aws/ec2#g1": group(nil, key)}) + }, false}, + {"group cap", func(c float64) *config.PurchasePlan { + return boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#g1": group(usd(c), key)}) + }, false}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.NoError(t, CheckStepCommitmentCap(tc.plan(100.01), recs), "the step is one cent under the cap") + assert.NoError(t, CheckStepCommitmentCap(tc.plan(100), recs), "exactly at the cap passes") + require.ErrorIs(t, CheckStepCommitmentCap(tc.plan(99.99), recs), ErrStepCommitmentCap, "one cent over is refused") + }) + } +} + +// A group cap that is not a usable positive finite number refuses the step, as +// the plan cap does (planCapUsable), instead of silently turning the cap off. +func TestCheckStepCommitmentCap_BoundGroupCapUnusableFailsClosed(t *testing.T) { + rec := capRec("us-east-1", 100, "") + recs := []config.RecommendationRecord{rec} + key := identity(t, rec) + for name, c := range map[string]float64{"NaN": math.NaN(), "+Inf": math.Inf(1), "zero": 0, "negative": -5} { + t.Run(name, func(t *testing.T) { + p := boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#g1": group(usd(c), key)}) + err := CheckStepCommitmentCap(p, recs) + require.ErrorIs(t, err, ErrStepCommitmentCap) + assert.ErrorContains(t, err, "aws/ec2#g1") + assert.ErrorContains(t, err, "no usable max_step_commitment", "refused as unusable, not as an over-cap step") + }) + } +} + +// A rec's commitment is upfront plus monthly * term * 12, so a cap that only +// fits the upfront part must refuse it, and a cap equal to the whole passes. +func TestCheckStepCommitmentCap_BoundCountsMonthlyCost(t *testing.T) { + rec := capRec("us-east-1", 50, "") + monthly := 10.0 + rec.MonthlyCost = &monthly + rec.Payment = "partial-upfront" + require.InDelta(t, 170.0, rec.TotalCommitment(), 0.001, "50 upfront + 10 * 12 months") + recs := []config.RecommendationRecord{rec} + key := identity(t, rec) + t.Run("group cap", func(t *testing.T) { + p := boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#g1": group(usd(150), key)}) + require.ErrorIs(t, CheckStepCommitmentCap(p, recs), ErrStepCommitmentCap, "150 fits the upfront 50 but not the 170 total") + p.Services["aws/ec2#g1"] = group(usd(170), key) + assert.NoError(t, CheckStepCommitmentCap(p, recs)) + }) + t.Run("plan cap", func(t *testing.T) { + p := boundCapPlan(true, usd(150), map[string]config.ServiceConfig{"aws/ec2#g1": group(nil, key)}) + require.ErrorIs(t, CheckStepCommitmentCap(p, recs), ErrStepCommitmentCap) + p.MaxStepCommitment = usd(170) + assert.NoError(t, CheckStepCommitmentCap(p, recs)) + }) +} + +// A group with no cap of its own is held only by the plan cap: its recs count +// toward the plan total, and it adds no group limit. +func TestCheckStepCommitmentCap_BoundNilGroupCapUnderAPlanCap(t *testing.T) { + east, west := capRec("us-east-1", 100, ""), capRec("us-west-2", 100, "") + ke, kw := identity(t, east), identity(t, west) + recs := []config.RecommendationRecord{east, west} + p := boundCapPlan(true, usd(150), map[string]config.ServiceConfig{ + "aws/ec2#g1": group(nil, ke), "aws/ec2#g2": group(usd(150), kw), + }) + require.ErrorIs(t, CheckStepCommitmentCap(p, recs), ErrStepCommitmentCap, "the nil-cap group's 100 counts toward the plan total of 200") + p.MaxStepCommitment = usd(200) + assert.NoError(t, CheckStepCommitmentCap(p, recs), "and it is not itself group-capped") + p.MaxStepCommitment = usd(1e9) + assert.NoError(t, CheckStepCommitmentCap(p, []config.RecommendationRecord{east}), "a nil group cap limits nothing alone") +} + +// The resolver skips a disabled group, so a rec bound only to one is outside +// every group here too, instead of being held to a cap that never applies. +func TestCheckStepCommitmentCap_BoundDisabledGroupIsOutside(t *testing.T) { + rec := capRec("us-east-1", 100, "") + key := identity(t, rec) + disabled := group(usd(1e9), key) + disabled.Enabled = false + p := boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#g1": disabled}) + require.ErrorIs(t, CheckStepCommitmentCap(p, []config.RecommendationRecord{rec}), ErrRecOutsideBoundGroups) +} + +// An identity bound by two groups is refused, not assigned to whichever group +// the map visits last (validation refuses it, but a stored plan may hold one). +func TestCheckStepCommitmentCap_BoundIdentityInTwoGroupsIsRefused(t *testing.T) { + rec := capRec("us-east-1", 100, "") + key := identity(t, rec) + p := boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{ + "aws/ec2#g1": group(usd(1e9), key), "aws/ec2#g2": group(usd(1e9), key), + }) + for i := 0; i < 20; i++ { + err := CheckStepCommitmentCap(p, []config.RecommendationRecord{rec}) + require.ErrorIs(t, err, ErrStepCommitmentCap) + assert.ErrorContains(t, err, `"aws/ec2#g1" and "aws/ec2#g2"`, "the same pair is named every time") + } +} + +// The group-level refusals reach the History note with their group key, which +// is closed text (a key and amounts), so the operator sees which group failed. +// A rec id carries an account number, so "outside every group" keeps only the +// fixed head. +func TestCheckStepCommitmentCap_BoundRefusalNotesKeepTheGroup(t *testing.T) { + rec := capRec("us-east-1", 100, "") + key := identity(t, rec) + recs := []config.RecommendationRecord{rec} + for name, plan := range map[string]*config.PurchasePlan{ + "over its cap": boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#g1": group(usd(50), key)}), + "NaN cap": boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#g1": group(usd(math.NaN()), key)}), + "negative cap": boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#g1": group(usd(-5), key)}), + "infinite cap": boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#g1": group(usd(math.Inf(1)), key)}), + "huge negative": boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#g1": group(usd(-1e21), key)}), + "in two groups": boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#g1": group(nil, key), "aws/ec2#g2": group(nil, key)}), + "plan cap total": boundCapPlan(true, usd(50), map[string]config.ServiceConfig{"aws/ec2#g1": group(nil, key)}), + } { + t.Run(name, func(t *testing.T) { + err := CheckStepCommitmentCap(plan, recs) + require.ErrorIs(t, err, ErrStepCommitmentCap) + assert.Equal(t, err.Error(), SanitizeStoredError(err.Error()), "a closed note is shown whole") + }) + } + outside := CheckStepCommitmentCap(boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#g1": group(nil, "other")}), recs) + require.ErrorIs(t, outside, ErrRecOutsideBoundGroups) + assert.Equal(t, "plan step refused by commitment cap", SanitizeStoredError(outside.Error()), "the rec id and its account stay out of History") + t.Run("an odd group key is not shown", func(t *testing.T) { + p := boundCapPlan(true, usd(1e9), map[string]config.ServiceConfig{"aws/ec2#team 7; drop": group(usd(50), key)}) + err := CheckStepCommitmentCap(p, recs) + require.ErrorIs(t, err, ErrStepCommitmentCap) + shown := SanitizeStoredError(err.Error()) + assert.NotContains(t, shown, "team 7") + assert.NotContains(t, shown, "drop") + }) +}