From 8dd39ecae01bd6f2a0354e6aca330c9a741601dc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 01:58:56 +0200 Subject: [PATCH 1/2] fix(plans): scope Savings Plans out of multi-step ramps Refuse an armed multi-step plan with an enabled Savings Plans service at plan create, update and arm (HTTP 400), via PurchasePlan.Validate and the step Precheck. At attach, route ErrUnsupportedPlan to failStep so an already-armed plan gets one visible failed row instead of erroring every tick. Single-step SP plans are unchanged. Adds an owner runbook query for armed hits. Refs #631 Co-Authored-By: Claude Sonnet 5.5 --- .../api/handler_plans_sp_multistep_test.go | 95 +++++++++++++++++++ internal/config/validation.go | 24 +++++ internal/config/validation_test.go | 36 +++++++ internal/planstep/precheck.go | 6 +- internal/planstep/precheck_test.go | 6 ++ internal/purchase/plan_step_attach.go | 3 + internal/purchase/plan_step_attach_test.go | 22 +++++ .../armed-multi-step-savings-plans-plans.md | 29 ++++++ 8 files changed, 220 insertions(+), 1 deletion(-) create mode 100644 internal/api/handler_plans_sp_multistep_test.go create mode 100644 runbooks/armed-multi-step-savings-plans-plans.md diff --git a/internal/api/handler_plans_sp_multistep_test.go b/internal/api/handler_plans_sp_multistep_test.go new file mode 100644 index 000000000..b8d816614 --- /dev/null +++ b/internal/api/handler_plans_sp_multistep_test.go @@ -0,0 +1,95 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Savings Plans are scoped out of multi-step ramps (platform#631 Q7): create, +// PUT and PATCH (arm) refuse an armed multi-step plan with an SP service with +// a 400, and leave single-step SP and multi-step non-SP plans alone. + +const spPlanID = "11111111-1111-1111-1111-111111111111" + +func spHandler(t *testing.T) (*Handler, *MockConfigStore) { + t.Helper() + ctx := context.Background() + store := new(MockConfigStore) + auth := new(MockAuthService) + auth.On("ValidateSession", ctx, "admin-token").Return(&Session{UserID: "aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa", Email: "admin@example.com"}, nil) + auth.grantAdmin() + return &Handler{config: store, auth: auth}, store +} + +func spReq(body string) *events.LambdaFunctionURLRequest { + return &events.LambdaFunctionURLRequest{Headers: map[string]string{"Authorization": "Bearer admin-token"}, Body: body} +} + +func requireSP400(t *testing.T, err error) { + t.Helper() + ce, ok := IsClientError(err) + require.True(t, ok, "want ClientError, got %T: %v", err, err) + assert.Equal(t, 400, ce.code) + assert.Contains(t, ce.Error(), "multi-step ramps do not support Savings Plans") +} + +func TestCreatePlan_RejectsArmedMultiStepSavingsPlan(t *testing.T) { + h, _ := spHandler(t) + body := `{"name":"P","provider":"aws","service":"savingsplans","ramp_schedule":"weekly-25pct","enabled":true,"auto_purchase":true,"target_accounts":["bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb"]}` + _, err := h.createPlan(context.Background(), spReq(body)) + requireSP400(t, err) +} + +func TestMergeUpdatedPlan_RejectsArmedMultiStepSavingsPlan(t *testing.T) { + existing := &config.PurchasePlan{Name: "p", RampSchedule: config.RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1}} + _, err := mergeUpdatedPlan(existing, &PlanRequest{Name: "p", Provider: "aws", Service: "savingsplans", + RampSchedule: "weekly-25pct", Enabled: true, AutoPurchase: true}, "id") + requireSP400(t, err) +} + +func patchSP(t *testing.T, existing *config.PurchasePlan, body string) (any, error) { + t.Helper() + ctx := context.Background() + h, store := spHandler(t) + store.On("GetPurchasePlan", ctx, spPlanID).Return(existing, nil) + store.On("UpdatePurchasePlan", ctx, mock.AnythingOfType("*config.PurchasePlan")).Return(nil) + return h.patchPlan(ctx, spReq(body), spPlanID) +} + +func spPlan(service string, ramp config.RampSchedule) *config.PurchasePlan { + return &config.PurchasePlan{ID: spPlanID, Name: "p", NotificationDaysBefore: 3, RampSchedule: ramp, + Services: map[string]config.ServiceConfig{"aws/" + service: {Provider: "aws", Service: service, Enabled: true, Term: 3, Payment: "no-upfront", Coverage: 80}}} +} + +func TestPatchPlan_ArmingMultiStepSavingsPlanIs400(t *testing.T) { + weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4} + _, err := patchSP(t, spPlan("savingsplans", weekly), `{"enabled":true,"auto_purchase":true}`) + requireSP400(t, err) +} + +func TestPatchPlan_SingleStepSavingsPlanAndMultiStepNonSPArmFine(t *testing.T) { + immediate := config.RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1} + weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4} + for name, plan := range map[string]*config.PurchasePlan{ + "single-step SP": spPlan("savingsplans", immediate), + "multi-step RDS": spPlan("rds", weekly), + "single custom SP (100%)": spPlan("savingsplans", config.RampSchedule{Type: "custom", PercentPerStep: 100, StepIntervalDays: 7, TotalSteps: 1}), + } { + t.Run(name, func(t *testing.T) { + _, err := patchSP(t, plan, `{"enabled":true,"auto_purchase":true}`) + require.NoError(t, err) + }) + } +} + +func TestPatchPlan_UnarmedMultiStepSavingsPlanIsAllowed(t *testing.T) { + weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4} + _, err := patchSP(t, spPlan("savingsplans", weekly), `{"name":"renamed"}`) + require.NoError(t, err) +} diff --git a/internal/config/validation.go b/internal/config/validation.go index 8080dfbcd..bb4c88277 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -3,6 +3,7 @@ package config import ( "encoding/json" + "errors" "fmt" "math" "net/mail" @@ -10,6 +11,7 @@ import ( "strconv" "strings" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/ladder" ) @@ -597,6 +599,28 @@ func (p *PurchasePlan) Validate() error { } } + return p.CheckNoMultiStepSavingsPlans() +} + +// ErrMultiStepSavingsPlans marks an armed multi-step plan with an enabled +// Savings Plans service. Step 1 suppresses the whole SP recommendation for 7 +// days, so later steps find nothing and the ramp strands (platform#631, Q7). +var ErrMultiStepSavingsPlans = errors.New("multi-step ramps do not support Savings Plans: use a single-step (immediate) ramp or remove the Savings Plans service") + +// CheckNoMultiStepSavingsPlans refuses an armed (enabled and auto-purchase) +// plan that has more than one ramp step and an enabled Savings Plans service. +// It is the one definition, shared by plan validation (create, update, arm) and +// the step precheck at attach. Single-step plans are unaffected. +func (p *PurchasePlan) CheckNoMultiStepSavingsPlans() error { + if !p.Enabled || !p.AutoPurchase || p.RampSchedule.TotalSteps <= 1 { + return nil + } + for key := range p.Services { + svc := p.Services[key] + if svc.Enabled && common.IsSavingsPlan(common.ServiceType(svc.Service)) { + return fmt.Errorf("%w (service %q, total_steps %d)", ErrMultiStepSavingsPlans, key, p.RampSchedule.TotalSteps) + } + } return nil } diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 96b3d39f1..2b9301b13 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -1109,3 +1109,39 @@ func TestLadderCapMicros(t *testing.T) { assert.Error(t, err, in) } } + +func TestPurchasePlan_CheckNoMultiStepSavingsPlans(t *testing.T) { + svc := func(name string, enabled bool) map[string]ServiceConfig { + return map[string]ServiceConfig{"aws/" + name: {Provider: "aws", Service: name, Enabled: enabled}} + } + cases := []struct { + name string + enabled bool + auto bool + steps int + svcs map[string]ServiceConfig + wantErr bool + }{ + {"armed 4-step SP", true, true, 4, svc("savingsplans", true), true}, + {"armed 2-step SP (boundary)", true, true, 2, svc("savingsplans", true), true}, + {"armed 1-step SP", true, true, 1, svc("savingsplans", true), false}, + {"armed 0-step SP", true, true, 0, svc("savingsplans", true), false}, + {"armed 4-step compute SP", true, true, 4, svc("savings-plans-compute", true), true}, + {"armed 4-step disabled SP service", true, true, 4, svc("savingsplans", false), false}, + {"armed 4-step non-SP", true, true, 4, svc("rds", true), false}, + {"not auto-purchase", true, false, 4, svc("savingsplans", true), false}, + {"disabled", false, true, 4, svc("savingsplans", true), false}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + p := &PurchasePlan{Enabled: tc.enabled, AutoPurchase: tc.auto, Services: tc.svcs, + RampSchedule: RampSchedule{TotalSteps: tc.steps}} + err := p.CheckNoMultiStepSavingsPlans() + if tc.wantErr { + require.ErrorIs(t, err, ErrMultiStepSavingsPlans) + return + } + require.NoError(t, err) + }) + } +} diff --git a/internal/planstep/precheck.go b/internal/planstep/precheck.go index 1926d0519..72d5dd846 100644 --- a/internal/planstep/precheck.go +++ b/internal/planstep/precheck.go @@ -17,7 +17,8 @@ var ErrUnsupportedPlan = errors.New("plan settings are not supported by recommen // Precheck refuses, with a plan-specific error, the plan settings Resolve does // not apply: any Include*/Exclude*/MinCount filter on a service config (the // plan-step request never sets them, so honoring them would buy something other -// than what the plan says), and an immediate ramp whose PercentPerStep is not +// than what the plan says), an armed multi-step plan with a Savings Plans +// service (platform#631 Q7), and an immediate ramp whose PercentPerStep is not // 100 (an immediate ramp ignores that field when validated but Resolve sizes // with it). func Precheck(plan *config.PurchasePlan) error { @@ -36,6 +37,9 @@ func Precheck(plan *config.PurchasePlan) error { return fmt.Errorf("%w: plan %s has an immediate ramp with percent_per_step %v (must be 100)", ErrUnsupportedPlan, plan.ID, plan.RampSchedule.PercentPerStep) } + if err := plan.CheckNoMultiStepSavingsPlans(); err != nil { + return fmt.Errorf("%w: plan %s: %w", ErrUnsupportedPlan, plan.ID, err) + } return nil } diff --git a/internal/planstep/precheck_test.go b/internal/planstep/precheck_test.go index 935a60b91..2138dd337 100644 --- a/internal/planstep/precheck_test.go +++ b/internal/planstep/precheck_test.go @@ -17,6 +17,9 @@ func planWith(svc config.ServiceConfig, ramp config.RampSchedule) *config.Purcha func TestPrecheck(t *testing.T) { ok := config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: true} weekly := config.RampSchedule{Type: "weekly", PercentPerStep: 25} + sp := config.ServiceConfig{Provider: "aws", Service: "savingsplans", Enabled: true} + weekly4 := config.RampSchedule{Type: "weekly", PercentPerStep: 25, TotalSteps: 4} + armed := func(p *config.PurchasePlan) *config.PurchasePlan { p.Enabled, p.AutoPurchase = true, true; return p } cases := []struct { name string plan *config.PurchasePlan @@ -35,6 +38,9 @@ func TestPrecheck(t *testing.T) { {"exclude types", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.ExcludeTypes = []string{"m5.large"} }), weekly), "exclude_types"}, {"min count", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.MinCount = 1 }), weekly), "min_count"}, {"negative min count", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.MinCount = -1 }), weekly), "min_count"}, + {"armed multi-step savings plan", armed(planWith(sp, weekly4)), "multi-step ramps do not support Savings Plans"}, + {"armed single-step savings plan", armed(planWith(sp, config.RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1})), ""}, + {"armed multi-step non-SP", armed(planWith(ok, weekly4)), ""}, {"filter on a disabled service still refuses", planWith(withSvc(ok, func(s *config.ServiceConfig) { s.Enabled = false; s.MinCount = 2 }), weekly), "min_count"}, } for _, tc := range cases { diff --git a/internal/purchase/plan_step_attach.go b/internal/purchase/plan_step_attach.go index f6a0a5ba7..5d289c4b4 100644 --- a/internal/purchase/plan_step_attach.go +++ b/internal/purchase/plan_step_attach.go @@ -153,6 +153,9 @@ func (m *Manager) attachStep(ctx context.Context, tx pgx.Tx, locked *config.Purc if errors.Is(err, planstep.ErrNoMatch) { return m.noMatch(ctx, tx, locked, claim, root) } + if errors.Is(err, planstep.ErrUnsupportedPlan) { + return m.failStep(ctx, tx, locked, claim, root, err.Error()) + } if err != nil { return stepClaim{}, fmt.Errorf("plan %s step %d: %w", locked.ID, claim.number, err) } diff --git a/internal/purchase/plan_step_attach_test.go b/internal/purchase/plan_step_attach_test.go index 390cf1276..1f6b7f593 100644 --- a/internal/purchase/plan_step_attach_test.go +++ b/internal/purchase/plan_step_attach_test.go @@ -237,3 +237,25 @@ func TestPlanUnchanged_ComparesDatesAtMicrosecondPrecision(t *testing.T) { locked.NextExecutionDate = &moved assert.False(t, planUnchanged(listed, locked, providers), "a microsecond is a change") } + +// An already-armed multi-step Savings Plans plan (platform#631 Q7) must produce +// one visible failed row with the reason, not an error on every tick. +func TestAttach_UnsupportedPlanFailsTheStepVisibly(t *testing.T) { + listed := attachPlan(time.Now().Add(48*time.Hour), 1e6) + locked := attachPlan(*listed.NextExecutionDate, 1e6) + for _, p := range []*config.PurchasePlan{listed, locked} { + p.Services = map[string]config.ServiceConfig{"aws/savingsplans": {Enabled: true, Provider: "aws", Service: "savingsplans"}} + p.RampSchedule = config.RampSchedule{Type: "weekly", PercentPerStep: 25, TotalSteps: 4} + } + res := &fakeResolver{err: planstep.Precheck(locked)} + require.ErrorIs(t, res.err, planstep.ErrUnsupportedPlan) + m, store := attachManager(t, listed, locked, res) + store.On("SavePurchaseExecution", mock.Anything, mock.Anything).Return(nil) + + _, _, _, err := m.getOrCreateExecution(context.Background(), listed) + require.ErrorIs(t, err, errExecutionNotNotifiable) + saved := savedRows(store) + require.Len(t, saved, 1) + assert.Equal(t, "failed", saved[0].Status) + assert.Contains(t, saved[0].Error, "multi-step ramps do not support Savings Plans") +} diff --git a/runbooks/armed-multi-step-savings-plans-plans.md b/runbooks/armed-multi-step-savings-plans-plans.md new file mode 100644 index 000000000..32b4637a0 --- /dev/null +++ b/runbooks/armed-multi-step-savings-plans-plans.md @@ -0,0 +1,29 @@ +# Runbook: armed multi-step plans that include Savings Plans + +Multi-step ramps do not support Savings Plans (platform#631). Step 1 suppresses +the whole SP recommendation for 7 days, so later steps find nothing and the ramp +strands. Plan create, update and arm now return HTTP 400 for an armed plan +(enabled and auto_purchase) with `total_steps > 1` and an enabled Savings Plans +service, and an already-armed one fails its next step with a visible failed row. + +## Before deploying this change (read-only, run on DEV and prod) + +Lists the armed plans that would start failing. Decide each hit first: re-create +it as a single-step (immediate) plan, remove the SP service, or disarm it. + +```sql +SELECT p.id, p.name, p.ramp_schedule->>'type' AS ramp_type, + (p.ramp_schedule->>'total_steps')::int AS total_steps, + (p.ramp_schedule->>'current_step')::int AS current_step, + p.next_execution_date, + array_agg(s.key) AS sp_services +FROM purchase_plans p +CROSS JOIN LATERAL jsonb_each(p.services) AS s(key, val) +WHERE p.enabled AND p.auto_purchase + AND COALESCE((p.ramp_schedule->>'total_steps')::int, 0) > 1 + AND COALESCE((s.val->>'enabled')::boolean, false) + AND (s.val->>'service' = 'savingsplans' OR s.val->>'service' LIKE 'savings-plans-%') +GROUP BY p.id, p.name, p.ramp_schedule, p.next_execution_date; +``` + +Zero rows means nothing changes for existing plans. From 847f140fb0a3b31cf2b66f3ebb0aa187ee0eefdd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 01:59:19 +0200 Subject: [PATCH 2/2] docs(plans): point the SP scope-out error at the owner runbook Co-Authored-By: Claude Sonnet 5.5 --- internal/config/validation.go | 1 + 1 file changed, 1 insertion(+) diff --git a/internal/config/validation.go b/internal/config/validation.go index bb4c88277..96af8a796 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -605,6 +605,7 @@ func (p *PurchasePlan) Validate() error { // ErrMultiStepSavingsPlans marks an armed multi-step plan with an enabled // Savings Plans service. Step 1 suppresses the whole SP recommendation for 7 // days, so later steps find nothing and the ramp strands (platform#631, Q7). +// Read-only query for already-armed hits: runbooks/armed-multi-step-savings-plans-plans.md. var ErrMultiStepSavingsPlans = errors.New("multi-step ramps do not support Savings Plans: use a single-step (immediate) ramp or remove the Savings Plans service") // CheckNoMultiStepSavingsPlans refuses an armed (enabled and auto-purchase)