From b1183e294cfe49e0e4bdcd1e7582af3eb10b58e0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 01:34:05 +0100 Subject: [PATCH 1/2] feat(purchase): apply the step cap to bound plans per group A bound plan (allocation_mode bound_groups) must map every recommendation in a step to a group, whether or not a cap is set. StepCapApplies now includes bound plans, and the attach and email approval sites share it so they cannot drift. checkBoundStepCap refuses a recommendation outside every bound group (ErrRecOutsideBoundGroups, which wraps ErrStepCommitmentCap), keeps each group under its own cap and the step total under the plan cap, so the effective cap is min(group, plan). An auto-purchase bound plan still needs a plan cap; a manual one, where a human approves each row, may have none. Part of #1746 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_purchases.go | 2 +- internal/purchase/plan_step_attach.go | 2 +- internal/purchase/step_cap.go | 107 ++++++++++++++++++++++- internal/purchase/step_cap_bound_test.go | 107 +++++++++++++++++++++++ 4 files changed, 214 insertions(+), 4 deletions(-) create mode 100644 internal/purchase/step_cap_bound_test.go diff --git a/internal/api/handler_purchases.go b/internal/api/handler_purchases.go index e07241d09..8a92132cd 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/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/step_cap.go b/internal/purchase/step_cap.go index be0649733..1d2951fff 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,88 @@ func CheckStepCommitmentCap(plan *config.PurchasePlan, recs []config.Recommendat return nil } +func planCapUsable(plan *config.PurchasePlan) bool { + return plan != nil && plan.MaxStepCommitment != nil && *plan.MaxStepCommitment > 0 && !math.IsInf(*plan.MaxStepCommitment, 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() + } + totals, total, err := boundStepTotals(plan, recs, boundGroupOf(plan), 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. +func boundGroupOf(plan *config.PurchasePlan) map[string]string { + groupOf := make(map[string]string) + for k := range plan.Services { + for _, id := range plan.Services[k].BoundIdentities { + groupOf[id] = k + } + } + return groupOf +} + +// 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, 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 { + if gc := plan.Services[g].MaxStepCommitment; gc != nil && 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..d907c603f --- /dev/null +++ b/internal/purchase/step_cap_bound_test.go @@ -0,0 +1,107 @@ +package purchase + +import ( + "errors" + "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)) +} From 8c6e7dbc266b4d74b61d2aab7ea9a82e6d685b00 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 18:43:45 +0100 Subject: [PATCH 2/2] fix(purchase): fail closed on an unusable group cap and pin the bound step cap A bound group cap that is NaN or +Inf compared false against every step and so switched the cap off, while the plan cap already failed closed. capUsable now holds both: a group cap that is not a positive finite number refuses the step. boundGroupOf skips a disabled group, as the resolver does, so a rec bound only to one is outside every group, and refuses an identity bound by two groups instead of taking whichever the map visits last. The group-level refusals now reach the History note with their group key (closed text: a key and amounts); "outside every group" keeps only the head because its message carries a rec id and so an account. Tests pin the cap boundaries (cap minus a cent, the cap, plus a cent) on the plan cap and a group cap, monthly cost in the total, a nil group cap under a plan cap, and both approval sites: the email-link approval and the attach path run the group check for a manual bound plan with no plan cap only because they share StepCapApplies. Part of #1746 Co-Authored-By: Claude Sonnet 5.5 --- ...andler_purchases_plan_step_approve_test.go | 30 ++++ internal/purchase/errnotes.go | 12 +- .../plan_step_attach_integration_test.go | 34 ++++ internal/purchase/step_cap.go | 47 +++++- internal/purchase/step_cap_bound_test.go | 146 ++++++++++++++++++ 5 files changed, 261 insertions(+), 8 deletions(-) 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 4ff7e75c6..6102eabb6 100644 --- a/internal/purchase/errnotes.go +++ b/internal/purchase/errnotes.go @@ -24,7 +24,7 @@ var serverNotePatterns = []string{ `refusing to execute retry attempt \d+: (?:` + redriveReasonPattern + `)`, `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)`, @@ -100,6 +100,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_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 1d2951fff..180bcb04f 100644 --- a/internal/purchase/step_cap.go +++ b/internal/purchase/step_cap.go @@ -71,7 +71,13 @@ func CheckStepCommitmentCap(plan *config.PurchasePlan, recs []config.Recommendat } func planCapUsable(plan *config.PurchasePlan) bool { - return plan != nil && plan.MaxStepCommitment != nil && *plan.MaxStepCommitment > 0 && !math.IsInf(*plan.MaxStepCommitment, 0) + 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 { @@ -89,7 +95,11 @@ func checkBoundStepCap(plan *config.PurchasePlan, recs []config.RecommendationRe if !planCap && (plan.AutoPurchase || plan.MaxStepCommitment != nil) { return errNoPlanCap() } - totals, total, err := boundStepTotals(plan, recs, boundGroupOf(plan), planCap) + groupOf, err := boundGroupOf(plan) + if err != nil { + return err + } + totals, total, err := boundStepTotals(plan, recs, groupOf, planCap) if err != nil { return err } @@ -100,14 +110,29 @@ func checkBoundStepCap(plan *config.PurchasePlan, recs []config.RecommendationRe } // boundGroupOf maps each bound identity key to the service key of its group. -func boundGroupOf(plan *config.PurchasePlan) map[string]string { - groupOf := make(map[string]string) +// 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 + return groupOf, nil } // boundStepTotals maps every rec to its bound group and sums the commitments a @@ -137,7 +162,8 @@ func boundStepTotals(plan *config.PurchasePlan, recs []config.RecommendationReco return totals, total, nil } -// checkBoundGroupCaps refuses the first group, in key order, over its own cap. +// 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 { @@ -145,7 +171,14 @@ func checkBoundGroupCaps(plan *config.PurchasePlan, totals map[string]float64) e } sort.Strings(keys) for _, g := range keys { - if gc := plan.Services[g].MaxStepCommitment; gc != nil && totals[g] > *gc { + 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) } } diff --git a/internal/purchase/step_cap_bound_test.go b/internal/purchase/step_cap_bound_test.go index d907c603f..791c64a13 100644 --- a/internal/purchase/step_cap_bound_test.go +++ b/internal/purchase/step_cap_bound_test.go @@ -2,6 +2,7 @@ package purchase import ( "errors" + "math" "testing" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" @@ -105,3 +106,148 @@ func TestCheckStepCommitmentCap_UnmarkedUnchanged(t *testing.T) { 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") + }) +}