From 9f1cd40f28535de3bceb3c2339604599e85731a1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 01:33:12 +0100 Subject: [PATCH 1/3] feat(config): validate bound plan groups and refuse more stripped-plan writes Slice A, step A1d of #1746. - PurchasePlan.Validate checks plans that carry the bound_groups marker: - effective group ids are unique; - no two enabled groups share provider, service, term and payment; - no recommendation is bound twice, in one group or across groups; - a group binds at most 1000 recommendations; - a group cap is a valid cents amount (StepCapCents). A plan cap below a group cap stays valid, because the effective cap is min(group, plan). Unmarked plans skip these checks, so migration 000040's savings-plans siblings validate as before. - ServiceConfig.EffectiveGroupID returns GroupID, or the map key for a legacy entry. - Writes that leave a stripped bound plan armed now answer 409 on two more paths: - re-enabling an entry, which planArmEscalates can't see because DerivePlanServices ignores Enabled; - retargeting an armed plan through PUT /api/plans/{id}/accounts. Both gaps were found in the #1760 reviews. Precheck already refused both cases at execution. pre-commit: cfn-lint skipped (it can't install here); no CloudFormation files are touched. Part of #1746 Co-Authored-By: Claude Opus 5.5 --- .../api/auto_purchase_bound_binding_test.go | 43 ++++++++ internal/api/auto_purchase_grant.go | 29 ++++- internal/api/handler_plans.go | 5 + internal/config/validation.go | 3 + internal/config/validation_bound_groups.go | 94 ++++++++++++++++ .../config/validation_bound_groups_test.go | 100 ++++++++++++++++++ 6 files changed, 272 insertions(+), 2 deletions(-) create mode 100644 internal/config/validation_bound_groups.go create mode 100644 internal/config/validation_bound_groups_test.go diff --git a/internal/api/auto_purchase_bound_binding_test.go b/internal/api/auto_purchase_bound_binding_test.go index 3ead1cbce..fe098cdb0 100644 --- a/internal/api/auto_purchase_bound_binding_test.go +++ b/internal/api/auto_purchase_bound_binding_test.go @@ -54,3 +54,46 @@ func TestPlanDisarm_AllowsStrippedBoundPlan(t *testing.T) { assert.True(t, apgCalled(store, "UpdatePurchasePlan"), body) } } + +// Re-enabling a stripped entry widens an armed plan even though +// planArmEscalates cannot see it (its service sets ignore Enabled). +func TestUpdatePlanGated_RefusesReenablingStrippedEntry(t *testing.T) { + before := strippedBoundPlan(true) + before.Services = map[string]config.ServiceConfig{ + "aws/ec2#g1": {Provider: "aws", Service: "ec2", Enabled: true, MinCount: 1, GroupID: "g1", BoundIdentities: []string{"k1"}}, + "aws/rds#g2": {Provider: "aws", Service: "rds", Enabled: false, MinCount: 1}, + } + after := *before + after.Services = map[string]config.ServiceConfig{"aws/ec2#g1": before.Services["aws/ec2#g1"], "aws/rds#g2": before.Services["aws/rds#g2"]} + g2 := after.Services["aws/rds#g2"] + g2.Enabled = true + after.Services["aws/rds#g2"] = g2 + require.False(t, planArmEscalates(before, &after), "the gap: the arming gate alone misses this write") + + store := new(MockConfigStore) + h, req := apgCaller{perms: apsGrant(nil)}.handlerAndReq(store, `{}`) + err := h.updatePlanGated(context.Background(), req, &Session{UserID: apgUserID}, before, &after, "updatePlan") + ce, ok := IsClientError(err) + require.True(t, ok, "must be a client error: %v", err) + assert.Equal(t, 409, ce.code) + assert.False(t, apgCalled(store, "UpdatePurchasePlan")) + + // (guard) re-enabling a bound entry that kept its binding still saves. + g2.BoundIdentities = []string{"k2"} + after.Services["aws/rds#g2"] = g2 + store.On("UpdatePurchasePlan", mock.Anything, mock.Anything).Return(nil) + require.NoError(t, h.updatePlanGated(context.Background(), req, &Session{UserID: apgUserID}, before, &after, "updatePlan")) +} + +func TestRetargetArmedPlan_RefusesStrippedBoundPlan(t *testing.T) { + store, err := apsSetAccounts(t, apsGrant(nil), strippedBoundPlan(true), apsAcctB) + ce, ok := IsClientError(err) + require.True(t, ok, "must be a client error: %v", err) + assert.Equal(t, 409, ce.code) + assert.False(t, apgCalled(store, "SetPlanAccounts")) + + // (guard) an unarmed stripped plan can still be retargeted. + store, err = apsSetAccounts(t, apsGrant(nil), strippedBoundPlan(false), apsAcctB) + require.NoError(t, err) + assert.True(t, apgCalled(store, "SetPlanAccounts")) +} diff --git a/internal/api/auto_purchase_grant.go b/internal/api/auto_purchase_grant.go index 84cffcf10..dfc6952a2 100644 --- a/internal/api/auto_purchase_grant.go +++ b/internal/api/auto_purchase_grant.go @@ -216,8 +216,8 @@ func (h *Handler) requirePlanArmGrant(ctx context.Context, req *events.LambdaFun func (h *Handler) requirePlanArmGrantFor(ctx context.Context, req *events.LambdaFunctionURLRequest, session *Session, before, after *config.PurchasePlan, accountIDs []string) error { // A bound plan that lost its binding must not be armed or widened // (#1746); disarming and disabling it never reach this gate. - if err := planstep.CheckBoundBinding(after); err != nil { - return NewClientError(http.StatusConflict, err.Error()) + if err := refuseUnboundBinding(after); err != nil { + return err } if err := h.requireAutoPurchaseGrantForPlan(ctx, req, session, accountIDs, after); err != nil { return err @@ -298,9 +298,34 @@ func (h *Handler) requireRetargetOfArmedPlan(ctx context.Context, req *events.La if !planArmed(plan) { return plan, nil } + if err := refuseUnboundBinding(plan); err != nil { + return nil, err + } return plan, h.requireAutoPurchaseGrantForPlan(ctx, req, session, accountIDs, plan) } +// refuseUnboundBinding answers 409 for a bound plan that lost its binding +// (#1746), so a write that arms it, widens it or moves where it buys is +// refused at save time instead of failing its next step. +func refuseUnboundBinding(plan *config.PurchasePlan) error { + if err := planstep.CheckBoundBinding(plan); err != nil { + return NewClientError(http.StatusConflict, err.Error()) + } + return nil +} + +// enablesEntry reports whether the write turns on a services entry that was +// off or absent. DerivePlanServices ignores Enabled, so planArmEscalates +// cannot see this widening. +func enablesEntry(before, after *config.PurchasePlan) bool { + for k := range after.Services { + if after.Services[k].Enabled && !before.Services[k].Enabled { + return true + } + } + return false +} + // errPlanMovedUnderGate means the plan changed between the grant check and the // plan row lock, so the check no longer describes what would be written. var errPlanMovedUnderGate = errors.New("plan changed under the arming gate") diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 9434fe58e..3d7c8d1d2 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -345,6 +345,11 @@ func (h *Handler) updatePlan(ctx context.Context, httpReq *events.LambdaFunction // same lock). before is the snapshot plan was built from; if the row moved on, // the write fails its optimistic check instead. func (h *Handler) updatePlanGated(ctx context.Context, httpReq *events.LambdaFunctionURLRequest, session *Session, before, plan *config.PurchasePlan, op string) error { + if planArmed(plan) && enablesEntry(before, plan) { + if err := refuseUnboundBinding(plan); err != nil { + return err + } + } if !planArmEscalates(before, plan) { return h.mapPlanUpdateError(h.config.UpdatePurchasePlan(ctx, plan), op) } diff --git a/internal/config/validation.go b/internal/config/validation.go index 57b134a8f..4dc348211 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -648,6 +648,9 @@ func (p *PurchasePlan) Validate() error { return fmt.Errorf("invalid service config '%s': %w", key, err) } } + if err := p.validateBoundGroups(); err != nil { + return err + } return p.CheckNoMultiStepSavingsPlans() } diff --git a/internal/config/validation_bound_groups.go b/internal/config/validation_bound_groups.go new file mode 100644 index 000000000..98bee5477 --- /dev/null +++ b/internal/config/validation_bound_groups.go @@ -0,0 +1,94 @@ +package config + +import ( + "errors" + "fmt" + "sort" +) + +// MaxBoundIdentitiesPerGroup caps the recommendations one bound group may +// hold (issue #1746). +const MaxBoundIdentitiesPerGroup = 1000 + +// ErrInvalidBoundGroup marks a bound plan whose config groups conflict. +var ErrInvalidBoundGroup = errors.New("invalid bound plan group") + +// EffectiveGroupID is the entry's group id: GroupID when set, otherwise its +// services map key (a legacy entry is its own one-entry group). +func (c *ServiceConfig) EffectiveGroupID(key string) string { + if c.GroupID != "" { + return c.GroupID + } + return key +} + +// validateBoundGroups checks a bound_groups plan's entries against each other. +// Unmarked plans are skipped, so existing plans (including migration 000040's +// savings-plans siblings, which share one identity) validate as before. The +// marker-required and missing-binding rules are not here: they live in +// planstep.Precheck and the arming gate, so a de-escalating PATCH always +// passes. +func (p *PurchasePlan) validateBoundGroups() error { + if p.RampSchedule.AllocationMode != RampAllocationBoundGroups { + return nil + } + keys := make([]string, 0, len(p.Services)) + for k := range p.Services { + keys = append(keys, k) + } + sort.Strings(keys) + groups := map[string]string{} // effective group id -> key + identities := map[string]string{} // provider|service|term|payment -> group + bound := map[string]string{} // bound identity -> group + for _, k := range keys { + svc := p.Services[k] + gid := svc.EffectiveGroupID(k) + if prev, dup := groups[gid]; dup { + return fmt.Errorf("%w: group %q is used by both %q and %q", ErrInvalidBoundGroup, gid, prev, k) + } + groups[gid] = k + if err := checkGroupIdentity(&svc, gid, identities); err != nil { + return err + } + if err := checkGroupBindings(&svc, gid, bound); err != nil { + return err + } + if svc.MaxStepCommitment != nil { + if _, err := StepCapCents(*svc.MaxStepCommitment); err != nil { + return fmt.Errorf("%w: group %q: %w", ErrInvalidBoundGroup, gid, err) + } + } + } + return nil +} + +// checkGroupIdentity refuses two enabled groups with the same provider, +// service, term and payment: their recommendations could not be told apart +// when sizing. ServiceConfig.Validate has already required each payment to be +// its provider's canonical token, so equal payments compare equal here. +func checkGroupIdentity(svc *ServiceConfig, gid string, seen map[string]string) error { + if !svc.Enabled { + return nil + } + id := fmt.Sprintf("%s|%s|%d|%s", svc.Provider, svc.Service, svc.Term, svc.Payment) + if prev, dup := seen[id]; dup { + return fmt.Errorf("%w: groups %q and %q have the same provider, service, term and payment", ErrInvalidBoundGroup, prev, gid) + } + seen[id] = gid + return nil +} + +// checkGroupBindings refuses a group with too many bound identities, or an +// identity bound twice, within the group or across groups. +func checkGroupBindings(svc *ServiceConfig, gid string, seen map[string]string) error { + if len(svc.BoundIdentities) > MaxBoundIdentitiesPerGroup { + return fmt.Errorf("%w: group %q binds %d recommendations (at most %d)", ErrInvalidBoundGroup, gid, len(svc.BoundIdentities), MaxBoundIdentitiesPerGroup) + } + for _, ident := range svc.BoundIdentities { + if prev, dup := seen[ident]; dup { + return fmt.Errorf("%w: recommendation %q is bound by group %q and group %q", ErrInvalidBoundGroup, ident, prev, gid) + } + seen[ident] = gid + } + return nil +} diff --git a/internal/config/validation_bound_groups_test.go b/internal/config/validation_bound_groups_test.go new file mode 100644 index 000000000..eb8cb9c87 --- /dev/null +++ b/internal/config/validation_bound_groups_test.go @@ -0,0 +1,100 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Fixtures are hand-built plans, not real configuration (issue #1746). + +func boundGroupsPlan(entries map[string]ServiceConfig) *PurchasePlan { + return &PurchasePlan{ + Name: "bound", Enabled: true, Services: entries, + RampSchedule: RampSchedule{Type: "weekly", PercentPerStep: 25, StepIntervalDays: 7, TotalSteps: 4, AllocationMode: RampAllocationBoundGroups}, + } +} + +func groupEntry(gid, service string, term int, payment string, ids ...string) ServiceConfig { + return ServiceConfig{Provider: "aws", Service: service, Enabled: true, Term: term, Payment: payment, Coverage: 80, + MinCount: 1, GroupID: gid, BoundIdentities: ids} +} + +func TestValidate_BoundGroups(t *testing.T) { + ec2a := groupEntry("g1", "ec2", 1, "no-upfront", "k1", "k2") + ec2b := groupEntry("g2", "ec2", 3, "all-upfront", "k3") + capOf := func(s ServiceConfig, v float64) ServiceConfig { s.MaxStepCommitment = &v; return s } + many := make([]string, MaxBoundIdentitiesPerGroup+1) + for i := range many { + many[i] = "k" + string(rune('a'+i%26)) + string(rune('0'+i/26%10)) + string(rune('0'+i/260)) + } + for _, tc := range []struct { + name string + entries map[string]ServiceConfig + want string // substring of the error; "" = valid + }{ + {"two groups differing in term", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": ec2b}, ""}, + {"two groups differing only in term", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": groupEntry("g2", "ec2", 3, "no-upfront", "k3")}, ""}, + {"duplicate group id", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": withGroup(ec2b, "g1")}, `group "g1"`}, + {"duplicate identity", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": groupEntry("g2", "ec2", 1, "no-upfront", "k3")}, "same provider, service, term and payment"}, + {"same identity on another provider", map[string]ServiceConfig{"azure/compute#g1": azureEntry("g1", "monthly", "k1"), "azure/compute#g2": azureEntry("g2", "monthly", "k2")}, "same provider, service, term and payment"}, + {"disabled duplicate identity is fine", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": disabled(groupEntry("g2", "ec2", 1, "no-upfront", "k3"))}, ""}, + {"identity bound twice in a group", map[string]ServiceConfig{"aws/ec2#g1": groupEntry("g1", "ec2", 1, "no-upfront", "k1", "k1")}, "k1"}, + {"identity bound in two groups", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": groupEntry("g2", "ec2", 3, "all-upfront", "k2")}, "k2"}, + {"too many identities", map[string]ServiceConfig{"aws/ec2#g1": groupEntry("g1", "ec2", 1, "no-upfront", many...)}, "at most 1000"}, + {"valid group cap", map[string]ServiceConfig{"aws/ec2#g1": capOf(ec2a, 250.5)}, ""}, + {"group cap below a cent", map[string]ServiceConfig{"aws/ec2#g1": capOf(ec2a, 0.001)}, "max_step_commitment"}, + {"group cap with three decimals", map[string]ServiceConfig{"aws/ec2#g1": capOf(ec2a, 1.005)}, "max_step_commitment"}, + } { + t.Run(tc.name, func(t *testing.T) { + err := boundGroupsPlan(tc.entries).Validate() + if tc.want == "" { + assert.NoError(t, err) + return + } + require.ErrorIs(t, err, ErrInvalidBoundGroup) + assert.ErrorContains(t, err, tc.want) + }) + } +} + +// The effective cap is min(group, plan): a plan cap below a group cap is valid. +func TestValidate_BoundGroups_PlanCapBelowGroupCap(t *testing.T) { + g := groupEntry("g1", "ec2", 1, "no-upfront", "k1") + groupCap, planCap := 500.0, 100.0 + g.MaxStepCommitment = &groupCap + p := boundGroupsPlan(map[string]ServiceConfig{"aws/ec2#g1": g}) + p.MaxStepCommitment = &planCap + assert.NoError(t, p.Validate()) +} + +// (guard) Unmarked plans validate exactly as today, including migration +// 000040's four aws:savings-plans-* siblings that share one identity. +func TestValidate_UnmarkedPlansSkipBoundGroupChecks(t *testing.T) { + sp := func(service string) ServiceConfig { + return ServiceConfig{Provider: "aws", Service: service, Enabled: true, Term: 1, Payment: "no-upfront", Coverage: 80} + } + p := &PurchasePlan{Name: "legacy", Enabled: true, RampSchedule: RampSchedule{Type: "immediate", PercentPerStep: 100, TotalSteps: 1}, + Services: map[string]ServiceConfig{ + "aws:savings-plans-compute": sp("savings-plans"), "aws:savings-plans-ec2instance": sp("savings-plans"), + "aws:savings-plans-sagemaker": sp("savings-plans"), "aws:savings-plans-database": sp("savings-plans"), + }} + assert.NoError(t, p.Validate()) + p.Services["aws:dup"] = ServiceConfig{Provider: "aws", Service: "ec2", Enabled: true, Term: 1, Payment: "no-upfront", Coverage: 80, GroupID: "x", BoundIdentities: []string{"k1", "k1"}} + assert.NoError(t, p.Validate(), "no marker: group fields are not validated here (Precheck guards buying)") +} + +func TestServiceConfig_EffectiveGroupID(t *testing.T) { + assert.Equal(t, "g1", (&ServiceConfig{GroupID: "g1"}).EffectiveGroupID("aws/ec2#g1")) + assert.Equal(t, "aws/ec2", (&ServiceConfig{}).EffectiveGroupID("aws/ec2")) +} + +func azureEntry(gid, payment string, ids ...string) ServiceConfig { + e := groupEntry(gid, "compute", 1, payment, ids...) + e.Provider = "azure" + return e +} + +func withGroup(s ServiceConfig, gid string) ServiceConfig { s.GroupID = gid; return s } +func disabled(s ServiceConfig) ServiceConfig { s.Enabled = false; return s } From b2341195301cb1ed21d0564c97057451671ad495 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 16:14:12 +0100 Subject: [PATCH 2/3] fix(api): let a disarm or disable PATCH stop any bound plan Review round on #1774 (part of #1746). - A PATCH that sets enabled=false or auto_purchase=false now validates with the new PurchasePlan.ValidateDeescalation. It runs every Validate check except the bound-group ones, so a stored bound plan with a conflicting group can always be stopped (CodeRabbit). A rename or an arming PATCH still runs the full Validate. - Validate now requires each entry of a bound plan to be keyed "provider/service#" with a matching group_id, so the key and the field can't name different groups (qa-ui-3). - New tests (qa-ui-3's gaps and CodeRabbit's nitpick): - an unarmed stripped plan can re-enable an entry; - an armed bound plan that gains an unbound group of a service it already has gets 409, with enablesEntry pinned directly; - exactly 1000 bound identities are accepted; - groups that differ only in payment, service or provider are valid. Part of #1746 Co-Authored-By: Claude Opus 5.5 --- .../api/auto_purchase_bound_binding_test.go | 40 +++++++++++++++++++ internal/api/handler_plans.go | 16 +++++++- internal/api/handler_plans_deescalate_test.go | 37 +++++++++++++++++ internal/config/validation.go | 20 +++++++++- internal/config/validation_bound_groups.go | 15 +++++++ .../config/validation_bound_groups_test.go | 26 +++++++++++- 6 files changed, 151 insertions(+), 3 deletions(-) create mode 100644 internal/api/handler_plans_deescalate_test.go diff --git a/internal/api/auto_purchase_bound_binding_test.go b/internal/api/auto_purchase_bound_binding_test.go index fe098cdb0..6758f50f3 100644 --- a/internal/api/auto_purchase_bound_binding_test.go +++ b/internal/api/auto_purchase_bound_binding_test.go @@ -97,3 +97,43 @@ func TestRetargetArmedPlan_RefusesStrippedBoundPlan(t *testing.T) { require.NoError(t, err) assert.True(t, apgCalled(store, "SetPlanAccounts")) } + +// (guard) The re-enable gate applies only while the plan stays armed: an +// unarmed stripped plan can still re-enable an entry. +func TestUpdatePlanGated_UnarmedReenableIsAccepted(t *testing.T) { + before := strippedBoundPlan(false) + before.Services = map[string]config.ServiceConfig{"aws/rds#g2": {Provider: "aws", Service: "rds", Enabled: false, MinCount: 1}} + after := *before + after.Services = map[string]config.ServiceConfig{"aws/rds#g2": {Provider: "aws", Service: "rds", Enabled: true, MinCount: 1}} + store := new(MockConfigStore) + store.On("UpdatePurchasePlan", mock.Anything, mock.Anything).Return(nil) + h, req := apgCaller{perms: apsGrant(nil)}.handlerAndReq(store, `{}`) + require.NoError(t, h.updatePlanGated(context.Background(), req, &Session{UserID: apgUserID}, before, &after, "updatePlan")) + assert.True(t, apgCalled(store, "UpdatePurchasePlan")) +} + +// A new entry counts as enabled: an armed bound plan gaining a second group of a +// service it already has (DerivePlanServices unchanged, so no escalation) is +// refused when that group has no binding. +func TestUpdatePlanGated_RefusesNewUnboundGroupOnArmedPlan(t *testing.T) { + before := strippedBoundPlan(true) + before.RampSchedule.AllocationMode = config.RampAllocationBoundGroups + before.Services = map[string]config.ServiceConfig{ + "aws/ec2#g1": {Provider: "aws", Service: "ec2", Enabled: true, MinCount: 1, GroupID: "g1", BoundIdentities: []string{"k1"}}, + } + after := *before + after.Services = map[string]config.ServiceConfig{ + "aws/ec2#g1": before.Services["aws/ec2#g1"], + "aws/ec2#g2": {Provider: "aws", Service: "ec2", Enabled: true, MinCount: 1, GroupID: "g2"}, + } + // Pinned directly: once DerivePlanServices drops the "#" suffix (slice A + // step A2), this gate is the only save-time refusal for a new group. + require.True(t, enablesEntry(before, &after), "a newly added enabled entry counts as enabling") + store := new(MockConfigStore) + h, req := apgCaller{perms: apsGrant(nil)}.handlerAndReq(store, `{}`) + err := h.updatePlanGated(context.Background(), req, &Session{UserID: apgUserID}, before, &after, "updatePlan") + ce, ok := IsClientError(err) + require.True(t, ok, "must be a client error: %v", err) + assert.Equal(t, 409, ce.code) + assert.False(t, apgCalled(store, "UpdatePurchasePlan")) +} diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 3d7c8d1d2..59cf03e8d 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -874,6 +874,20 @@ type PatchPlanRequest struct { NotificationDaysBefore *int `json:"notification_days_before,omitempty"` } +// patchDeescalates reports whether a PATCH disables or disarms the plan. Such a +// write skips the bound-group checks (ValidateDeescalation), so a plan can +// always be stopped. +func patchDeescalates(req PatchPlanRequest) bool { + return (req.Enabled != nil && !*req.Enabled) || (req.AutoPurchase != nil && !*req.AutoPurchase) +} + +func validatePatchedPlan(plan *config.PurchasePlan, req PatchPlanRequest) error { + if patchDeescalates(req) { + return plan.ValidateDeescalation() + } + return plan.Validate() +} + // applyPatchFields applies validated partial-update fields to a plan. func applyPatchFields(plan *config.PurchasePlan, req PatchPlanRequest) error { if req.Name != nil { @@ -941,7 +955,7 @@ func (h *Handler) patchPlan(ctx context.Context, httpReq *events.LambdaFunctionU return nil, err } - if err := plan.Validate(); err != nil { + if err := validatePatchedPlan(plan, req); err != nil { return nil, NewClientError(400, fmt.Sprintf("validation error: %s", err)) } diff --git a/internal/api/handler_plans_deescalate_test.go b/internal/api/handler_plans_deescalate_test.go new file mode 100644 index 000000000..0be1df1f6 --- /dev/null +++ b/internal/api/handler_plans_deescalate_test.go @@ -0,0 +1,37 @@ +package api + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Fixtures are hand-built plans, not real configuration (issue #1746). + +// conflictedBoundPlan is an armed bound plan whose stored groups fail the +// bound-group checks: one recommendation is bound twice. +func conflictedBoundPlan() *config.PurchasePlan { + p := apgStoredPlan(true, apgCap(1000)) + p.RampSchedule.AllocationMode = config.RampAllocationBoundGroups + p.Services = map[string]config.ServiceConfig{"aws/rds#g1": { + Provider: "aws", Service: "rds", Enabled: true, Term: 1, Payment: "no-upfront", Coverage: 80, + MinCount: 1, GroupID: "g1", BoundIdentities: []string{"k1", "k1"}, + }} + return p +} + +func TestPatchPlan_StoppingABoundPlanSkipsGroupChecks(t *testing.T) { + for _, body := range []string{`{"auto_purchase":false}`, `{"enabled":false}`, `{"enabled":false,"name":"stopped"}`} { + store, err := boundPatch(t, conflictedBoundPlan(), body) + require.NoError(t, err, body) + assert.True(t, apgCalled(store, "UpdatePurchasePlan"), body) + } + // Any other write still runs every check. + store, err := boundPatch(t, conflictedBoundPlan(), `{"name":"renamed"}`) + ce, ok := IsClientError(err) + require.True(t, ok, "must be a client error: %v", err) + assert.Equal(t, 400, ce.code) + assert.False(t, apgCalled(store, "UpdatePurchasePlan")) +} diff --git a/internal/config/validation.go b/internal/config/validation.go index 4dc348211..9ba687625 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -621,6 +621,24 @@ func (p *PurchasePlan) validateNotificationAndCap() error { // Validate validates the PurchasePlan. func (p *PurchasePlan) Validate() error { + return p.validate(true) +} + +// ValidateDeescalation is Validate without the bound-group checks, for a write +// that only disables or disarms the plan: stopping a plan must never be blocked +// by a group conflict it already stores (issue #1746). Every other check runs. +func (p *PurchasePlan) ValidateDeescalation() error { + return p.validate(false) +} + +func (p *PurchasePlan) checkBoundGroups(enabled bool) error { + if !enabled { + return nil + } + return p.validateBoundGroups() +} + +func (p *PurchasePlan) validate(boundGroups bool) error { // Name is required if p.Name == "" { return fmt.Errorf("plan name is required") @@ -648,7 +666,7 @@ func (p *PurchasePlan) Validate() error { return fmt.Errorf("invalid service config '%s': %w", key, err) } } - if err := p.validateBoundGroups(); err != nil { + if err := p.checkBoundGroups(boundGroups); err != nil { return err } diff --git a/internal/config/validation_bound_groups.go b/internal/config/validation_bound_groups.go index 98bee5477..ffa11f69b 100644 --- a/internal/config/validation_bound_groups.go +++ b/internal/config/validation_bound_groups.go @@ -4,6 +4,7 @@ import ( "errors" "fmt" "sort" + "strings" ) // MaxBoundIdentitiesPerGroup caps the recommendations one bound group may @@ -42,6 +43,9 @@ func (p *PurchasePlan) validateBoundGroups() error { bound := map[string]string{} // bound identity -> group for _, k := range keys { svc := p.Services[k] + if err := checkGroupKey(k, &svc); err != nil { + return err + } gid := svc.EffectiveGroupID(k) if prev, dup := groups[gid]; dup { return fmt.Errorf("%w: group %q is used by both %q and %q", ErrInvalidBoundGroup, gid, prev, k) @@ -92,3 +96,14 @@ func checkGroupBindings(svc *ServiceConfig, gid string, seen map[string]string) } return nil } + +// checkGroupKey requires a bound plan's entry to be keyed +// "provider/service#" with that same GroupID, so the key and the +// field can never name different groups. +func checkGroupKey(key string, svc *ServiceConfig) error { + _, suffix, ok := strings.Cut(key, BoundKeySeparator) + if !ok || svc.GroupID == "" || suffix != svc.GroupID { + return fmt.Errorf("%w: entry %q must be keyed \"provider/service%s\" with a matching group_id (got %q)", ErrInvalidBoundGroup, key, BoundKeySeparator, svc.GroupID) + } + return nil +} diff --git a/internal/config/validation_bound_groups_test.go b/internal/config/validation_bound_groups_test.go index eb8cb9c87..b780301e1 100644 --- a/internal/config/validation_bound_groups_test.go +++ b/internal/config/validation_bound_groups_test.go @@ -35,8 +35,15 @@ func TestValidate_BoundGroups(t *testing.T) { want string // substring of the error; "" = valid }{ {"two groups differing in term", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": ec2b}, ""}, + {"two groups differing only in payment", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": groupEntry("g2", "ec2", 1, "all-upfront", "k3")}, ""}, + {"two groups differing only in service", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/rds#g2": groupEntry("g2", "rds", 1, "no-upfront", "k3")}, ""}, + {"two groups differing only in provider", map[string]ServiceConfig{"azure/compute#g1": azureEntry("g1", "monthly", "k1"), "gcp/compute#g2": gcpEntry("g2", "k2")}, ""}, {"two groups differing only in term", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": groupEntry("g2", "ec2", 3, "no-upfront", "k3")}, ""}, - {"duplicate group id", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": withGroup(ec2b, "g1")}, `group "g1"`}, + {"duplicate group id", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/rds#g1": groupEntry("g1", "rds", 3, "all-upfront", "k3")}, `group "g1"`}, + {"key suffix differs from group_id", map[string]ServiceConfig{"aws/ec2#g1": withGroup(ec2a, "g2")}, `must be keyed`}, + {"bound entry without a # key", map[string]ServiceConfig{"aws/ec2": ec2a}, `must be keyed`}, + {"bound entry without a group_id", map[string]ServiceConfig{"aws/ec2#g1": withGroup(ec2a, "")}, `must be keyed`}, + {"exactly 1000 identities is accepted", map[string]ServiceConfig{"aws/ec2#g1": groupEntry("g1", "ec2", 1, "no-upfront", many[:MaxBoundIdentitiesPerGroup]...)}, ""}, {"duplicate identity", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": groupEntry("g2", "ec2", 1, "no-upfront", "k3")}, "same provider, service, term and payment"}, {"same identity on another provider", map[string]ServiceConfig{"azure/compute#g1": azureEntry("g1", "monthly", "k1"), "azure/compute#g2": azureEntry("g2", "monthly", "k2")}, "same provider, service, term and payment"}, {"disabled duplicate identity is fine", map[string]ServiceConfig{"aws/ec2#g1": ec2a, "aws/ec2#g2": disabled(groupEntry("g2", "ec2", 1, "no-upfront", "k3"))}, ""}, @@ -98,3 +105,20 @@ func azureEntry(gid, payment string, ids ...string) ServiceConfig { func withGroup(s ServiceConfig, gid string) ServiceConfig { s.GroupID = gid; return s } func disabled(s ServiceConfig) ServiceConfig { s.Enabled = false; return s } + +func gcpEntry(gid string, ids ...string) ServiceConfig { + e := groupEntry(gid, "compute", 1, "monthly", ids...) + e.Provider = "gcp" + return e +} + +// A write that only stops the plan skips the bound-group checks, and keeps +// every other check. +func TestValidateDeescalation_SkipsOnlyBoundGroupChecks(t *testing.T) { + g := groupEntry("g1", "ec2", 1, "no-upfront", "k1", "k1") + p := boundGroupsPlan(map[string]ServiceConfig{"aws/ec2#g1": g}) + require.ErrorIs(t, p.Validate(), ErrInvalidBoundGroup) + assert.NoError(t, p.ValidateDeescalation()) + p.Name = "" + assert.ErrorContains(t, p.ValidateDeescalation(), "plan name is required") +} From e8058a2d818b3c0b7fa92f9d5f0fc04e96070124 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 17:14:50 +0100 Subject: [PATCH 3/3] fix(api): a PATCH that turns anything on isn't a stop Review round on #1774 (ui-fix-1's CHANGES at b2341195; part of #1746). patchDeescalates treated a PATCH as a stop if either field turned off, so {"enabled":true,"auto_purchase":false} on a disabled plan re-enabled it while skipping the bound-group checks. A PATCH now counts as a stop only if it disables or disarms the plan and enables or arms nothing. Any PATCH that turns something on runs the full Validate. The test adds ui-fix-1's repro (re-enable while disarming is refused, nothing saved) and the mirror case (arm while disabling is refused). Part of #1746 Co-Authored-By: Claude Opus 5.5 --- internal/api/handler_plans.go | 11 ++++++---- internal/api/handler_plans_deescalate_test.go | 22 +++++++++++++++++-- 2 files changed, 27 insertions(+), 6 deletions(-) diff --git a/internal/api/handler_plans.go b/internal/api/handler_plans.go index 59cf03e8d..d8c1acdeb 100644 --- a/internal/api/handler_plans.go +++ b/internal/api/handler_plans.go @@ -874,11 +874,14 @@ type PatchPlanRequest struct { NotificationDaysBefore *int `json:"notification_days_before,omitempty"` } -// patchDeescalates reports whether a PATCH disables or disarms the plan. Such a -// write skips the bound-group checks (ValidateDeescalation), so a plan can -// always be stopped. +// patchDeescalates reports whether a PATCH only stops the plan: it disables or +// disarms it and turns nothing on. Such a write skips the bound-group checks +// (ValidateDeescalation), so a plan can always be stopped; a PATCH that also +// enables or arms the plan runs every check. func patchDeescalates(req PatchPlanRequest) bool { - return (req.Enabled != nil && !*req.Enabled) || (req.AutoPurchase != nil && !*req.AutoPurchase) + off := func(b *bool) bool { return b != nil && !*b } + on := func(b *bool) bool { return b != nil && *b } + return (off(req.Enabled) || off(req.AutoPurchase)) && !on(req.Enabled) && !on(req.AutoPurchase) } func validatePatchedPlan(plan *config.PurchasePlan, req PatchPlanRequest) error { diff --git a/internal/api/handler_plans_deescalate_test.go b/internal/api/handler_plans_deescalate_test.go index 0be1df1f6..0a4bb40d3 100644 --- a/internal/api/handler_plans_deescalate_test.go +++ b/internal/api/handler_plans_deescalate_test.go @@ -28,10 +28,28 @@ func TestPatchPlan_StoppingABoundPlanSkipsGroupChecks(t *testing.T) { require.NoError(t, err, body) assert.True(t, apgCalled(store, "UpdatePurchasePlan"), body) } - // Any other write still runs every check. - store, err := boundPatch(t, conflictedBoundPlan(), `{"name":"renamed"}`) + // A PATCH that turns anything on is not a stop, even if it also disarms: + // re-enabling a disabled conflicted plan must run the group checks + // (ui-fix-1's repro on #1774). + disabled := conflictedBoundPlan() + disabled.Enabled = false + store, err := boundPatch(t, disabled, `{"enabled":true,"auto_purchase":false}`) ce, ok := IsClientError(err) require.True(t, ok, "must be a client error: %v", err) assert.Equal(t, 400, ce.code) assert.False(t, apgCalled(store, "UpdatePurchasePlan")) + + // Arming while disabling isn't a stop either. + store, err = boundPatch(t, conflictedBoundPlan(), `{"enabled":false,"auto_purchase":true}`) + ce, ok = IsClientError(err) + require.True(t, ok, "must be a client error: %v", err) + assert.Equal(t, 400, ce.code) + assert.False(t, apgCalled(store, "UpdatePurchasePlan")) + + // Any other write still runs every check. + store, err = boundPatch(t, conflictedBoundPlan(), `{"name":"renamed"}`) + ce, ok = IsClientError(err) + require.True(t, ok, "must be a client error: %v", err) + assert.Equal(t, 400, ce.code) + assert.False(t, apgCalled(store, "UpdatePurchasePlan")) }