From fe7df6d5c49f3788cc58bd7d58f2de57eaf62365 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 22:54:49 +0100 Subject: [PATCH] feat(planstep): refuse bound plans that lost their binding Slice A, step A1c of #1746. - Precheck refuses a plan where an enabled entry is bound (a "#" key, or any entry of a bound_groups plan) but has no bound identities. The new ErrBoundPlanMissingBinding wraps ErrUnsupportedPlan, so attach writes a failed step instead of buying by filter or retrying silently. - Writes that arm or widen auto-purchase run the same check and answer 409. Disarming, disabling and renaming never reach that gate, so an operator can still stop a stripped plan. - min_count 1 on a "#"-keyed entry is the rollback sentinel (older binaries refuse any min_count), so Precheck no longer treats it as a filter there. Any other filter, and min_count on a legacy key, is still refused. - The #1756 bound-marker test now also checks that the requested ramp shape is the one saved (CodeRabbit nitpick on #1756). 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 | 56 +++++++++++++++++ internal/api/auto_purchase_grant.go | 7 +++ .../api/handler_plans_bound_marker_test.go | 13 +++- internal/config/purchase_plan_providers.go | 9 +++ internal/planstep/bound_binding_test.go | 60 +++++++++++++++++++ internal/planstep/bound_groups_mode_test.go | 2 +- internal/planstep/precheck.go | 49 ++++++++++++--- 7 files changed, 183 insertions(+), 13 deletions(-) create mode 100644 internal/api/auto_purchase_bound_binding_test.go create mode 100644 internal/planstep/bound_binding_test.go diff --git a/internal/api/auto_purchase_bound_binding_test.go b/internal/api/auto_purchase_bound_binding_test.go new file mode 100644 index 000000000..3ead1cbce --- /dev/null +++ b/internal/api/auto_purchase_bound_binding_test.go @@ -0,0 +1,56 @@ +package api + +import ( + "context" + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/planstep" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Fixtures are hand-built plans, not real configuration (issue #1746). + +// strippedBoundPlan is a bound plan whose group fields an older binary's +// write removed: the "#" key and the min_count sentinel survive, the bound +// identities do not. +func strippedBoundPlan(armed bool) *config.PurchasePlan { + p := apgStoredPlan(armed, apgCap(1000)) + p.Services = map[string]config.ServiceConfig{"aws/rds#g1": {Provider: "aws", Service: "rds", Enabled: true, MinCount: 1}} + return p +} + +func boundPatch(t *testing.T, stored *config.PurchasePlan, body string) (*MockConfigStore, error) { + t.Helper() + store := new(MockConfigStore) + store.On("GetPurchasePlan", mock.Anything, apgPlanID).Return(stored, nil) + store.On("GetPlanAccounts", mock.Anything, apgPlanID).Return([]config.CloudAccount{{ID: apsAcctA, Provider: "aws"}}, nil).Maybe() + store.On("UpdatePurchasePlan", mock.Anything, mock.Anything).Return(nil).Maybe() + store.On("UpdatePurchasePlanGated", mock.Anything, mock.Anything, mock.Anything).Return(nil).Maybe() + h, req := apgCaller{perms: apsGrant(nil)}.handlerAndReq(store, body) + _, err := h.patchPlan(context.Background(), req, apgPlanID) + return store, err +} + +func TestPlanArm_RefusesStrippedBoundPlan(t *testing.T) { + store, err := boundPatch(t, strippedBoundPlan(false), `{"auto_purchase":true}`) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok, "must be a client error, not a 500: %v", err) + assert.Equal(t, 409, ce.code) + assert.Contains(t, ce.Error(), planstep.ErrBoundPlanMissingBinding.Error()) + assert.False(t, apgCalled(store, "UpdatePurchasePlan")) + assert.False(t, apgCalled(store, "UpdatePurchasePlanGated")) +} + +// De-escalating writes must always pass, so an operator can stop a plan that +// lost its binding. +func TestPlanDisarm_AllowsStrippedBoundPlan(t *testing.T) { + for _, body := range []string{`{"auto_purchase":false}`, `{"enabled":false}`, `{"name":"renamed"}`} { + store, err := boundPatch(t, strippedBoundPlan(true), body) + require.NoError(t, err, body) + assert.True(t, apgCalled(store, "UpdatePurchasePlan"), body) + } +} diff --git a/internal/api/auto_purchase_grant.go b/internal/api/auto_purchase_grant.go index 2fdb11936..84cffcf10 100644 --- a/internal/api/auto_purchase_grant.go +++ b/internal/api/auto_purchase_grant.go @@ -5,6 +5,7 @@ import ( "errors" "fmt" "math" + "net/http" "slices" "strconv" "time" @@ -12,6 +13,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-platform/internal/auth" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/LeanerCloud/cloud-commitments-platform/internal/planstep" "github.com/aws/aws-lambda-go/events" ) @@ -212,6 +214,11 @@ func (h *Handler) requirePlanArmGrant(ctx context.Context, req *events.LambdaFun // requirePlanArmGrantFor is requirePlanArmGrant for an escalating write whose // target accounts are already resolved. 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 := h.requireAutoPurchaseGrantForPlan(ctx, req, session, accountIDs, after); err != nil { return err } diff --git a/internal/api/handler_plans_bound_marker_test.go b/internal/api/handler_plans_bound_marker_test.go index e65c0c5d5..f831699ed 100644 --- a/internal/api/handler_plans_bound_marker_test.go +++ b/internal/api/handler_plans_bound_marker_test.go @@ -15,16 +15,18 @@ import ( func TestUpdatePlan_KeepsBoundGroupsMarker(t *testing.T) { const planID = "12345678-1234-1234-1234-123456789abc" weekly := config.PresetRampSchedules[config.RampWeekly25Pct] + monthly := config.PresetRampSchedules[config.RampMonthly10Pct] for _, tc := range []struct { name, body string stored config.RampAllocationMode want config.RampAllocationMode + wantRamp config.RampSchedule }{ - {"shape change keeps bound_groups", `{"name":"p","ramp_schedule":"monthly-10pct"}`, config.RampAllocationBoundGroups, config.RampAllocationBoundGroups}, + {"shape change keeps bound_groups", `{"name":"p","ramp_schedule":"monthly-10pct"}`, config.RampAllocationBoundGroups, config.RampAllocationBoundGroups, monthly}, // (guard) the same shape already carried every mode. - {"same shape keeps bound_groups", `{"name":"renamed","ramp_schedule":"weekly-25pct"}`, config.RampAllocationBoundGroups, config.RampAllocationBoundGroups}, + {"same shape keeps bound_groups", `{"name":"renamed","ramp_schedule":"weekly-25pct"}`, config.RampAllocationBoundGroups, config.RampAllocationBoundGroups, weekly}, // (guard) other modes still reset on a shape change, as today. - {"shape change still drops equal_count", `{"name":"p","ramp_schedule":"monthly-10pct"}`, config.RampAllocationEqualCount, ""}, + {"shape change still drops equal_count", `{"name":"p","ramp_schedule":"monthly-10pct"}`, config.RampAllocationEqualCount, "", monthly}, } { t.Run(tc.name, func(t *testing.T) { rs := weekly @@ -34,6 +36,11 @@ func TestUpdatePlan_KeepsBoundGroupsMarker(t *testing.T) { require.Equal(t, 200, resp.StatusCode, resp.Body) require.NotNil(t, saved) assert.Equal(t, tc.want, saved.RampSchedule.AllocationMode) + // The requested shape is what gets saved; only the marker is carried. + assert.Equal(t, tc.wantRamp.Type, saved.RampSchedule.Type) + assert.Equal(t, tc.wantRamp.PercentPerStep, saved.RampSchedule.PercentPerStep) + assert.Equal(t, tc.wantRamp.StepIntervalDays, saved.RampSchedule.StepIntervalDays) + assert.Equal(t, tc.wantRamp.TotalSteps, saved.RampSchedule.TotalSteps) }) } } diff --git a/internal/config/purchase_plan_providers.go b/internal/config/purchase_plan_providers.go index 6ccb00020..bd4af2829 100644 --- a/internal/config/purchase_plan_providers.go +++ b/internal/config/purchase_plan_providers.go @@ -70,3 +70,12 @@ func DerivePlanServices(plan *PurchasePlan) []string { sort.Strings(services) return services } + +// BoundKeySeparator joins a bound entry's "provider/service" key to its group +// id: "aws/ec2#" (issue #1746). +const BoundKeySeparator = "#" + +// IsBoundServiceKey reports whether a services key names a bound group entry. +func IsBoundServiceKey(key string) bool { + return strings.Contains(key, BoundKeySeparator) +} diff --git a/internal/planstep/bound_binding_test.go b/internal/planstep/bound_binding_test.go new file mode 100644 index 000000000..f5a9dc8c2 --- /dev/null +++ b/internal/planstep/bound_binding_test.go @@ -0,0 +1,60 @@ +package planstep + +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). + +func boundPlan(marked bool, entries map[string]config.ServiceConfig) *config.PurchasePlan { + p := &config.PurchasePlan{ID: "plan-bound", Services: entries, RampSchedule: config.RampSchedule{Type: "weekly", PercentPerStep: 25}} + if marked { + p.RampSchedule.AllocationMode = config.RampAllocationBoundGroups + } + return p +} + +func boundEntry(ids ...string) config.ServiceConfig { + return config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: true, MinCount: 1, GroupID: "g1", BoundIdentities: ids} +} + +func TestPrecheck_BoundBinding(t *testing.T) { + for _, tc := range []struct { + name string + plan *config.PurchasePlan + missing bool // want ErrBoundPlanMissingBinding + want string // other refusal substring; "" = accepted + }{ + {"marked plan with bindings and the sentinel", boundPlan(true, map[string]config.ServiceConfig{"aws/ec2#g1": boundEntry("k1")}), false, ""}, + {"marked plan, entry lost its binding", boundPlan(true, map[string]config.ServiceConfig{"aws/ec2#g1": boundEntry()}), true, ""}, + {"#-keyed entry without marker or binding", boundPlan(false, map[string]config.ServiceConfig{"aws/ec2#g1": boundEntry()}), true, ""}, + {"marked plan, legacy key without binding", boundPlan(true, map[string]config.ServiceConfig{"aws/ec2": {Provider: "aws", Service: "ec2", Enabled: true}}), true, ""}, + {"disabled entry without binding is not checked", boundPlan(true, map[string]config.ServiceConfig{ + "aws/ec2#g1": boundEntry("k1"), + "aws/rds#g2": {Provider: "aws", Service: "rds", MinCount: 1, GroupID: "g2"}, + }), false, ""}, + {"sentinel other than 1 on a #-key is still a filter", boundPlan(true, map[string]config.ServiceConfig{"aws/ec2#g1": withSvc(boundEntry("k1"), func(s *config.ServiceConfig) { s.MinCount = 2 })}), false, "min_count"}, + {"other filters on a #-key are still refused", boundPlan(true, map[string]config.ServiceConfig{"aws/ec2#g1": withSvc(boundEntry("k1"), func(s *config.ServiceConfig) { s.IncludeTypes = []string{"m5.large"} })}), false, "include_types"}, + // (guard) a legacy key keeps today's refusal of min_count 1. + {"min_count 1 on a legacy key is refused", boundPlan(false, map[string]config.ServiceConfig{"aws/ec2": {Provider: "aws", Service: "ec2", Enabled: true, MinCount: 1}}), false, "min_count"}, + } { + t.Run(tc.name, func(t *testing.T) { + err := Precheck(tc.plan) + switch { + case tc.missing: + require.ErrorIs(t, err, ErrBoundPlanMissingBinding) + require.ErrorIs(t, err, ErrUnsupportedPlan, "attach must record a failed row, not retry") + assert.ErrorContains(t, err, "plan-bound") + case tc.want != "": + require.ErrorIs(t, err, ErrUnsupportedPlan) + assert.ErrorContains(t, err, tc.want) + default: + assert.NoError(t, err) + } + }) + } +} diff --git a/internal/planstep/bound_groups_mode_test.go b/internal/planstep/bound_groups_mode_test.go index 33ff57632..fe79ed005 100644 --- a/internal/planstep/bound_groups_mode_test.go +++ b/internal/planstep/bound_groups_mode_test.go @@ -17,7 +17,7 @@ func boundRamp(r config.RampSchedule) config.RampSchedule { // bound_groups takes every default-mode branch of Precheck. func TestPrecheck_BoundGroupsModeActsLikeDefault(t *testing.T) { - ok := config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: true} + ok := config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: true, BoundIdentities: []string{"k1"}} assert.NoError(t, Precheck(planWith(ok, boundRamp(config.RampSchedule{Type: "weekly", PercentPerStep: 25})))) assert.NoError(t, Precheck(planWith(ok, boundRamp(config.RampSchedule{Type: "immediate", PercentPerStep: 100})))) err := Precheck(planWith(ok, boundRamp(config.RampSchedule{Type: "immediate", PercentPerStep: 50}))) diff --git a/internal/planstep/precheck.go b/internal/planstep/precheck.go index 695e81f22..a730aa120 100644 --- a/internal/planstep/precheck.go +++ b/internal/planstep/precheck.go @@ -14,6 +14,12 @@ import ( // with the setting silently ignored. var ErrUnsupportedPlan = errors.New("plan settings are not supported by recommendation resolution") +// ErrBoundPlanMissingBinding marks a bound plan entry with no bound +// recommendations, for example one stripped by an older binary's write +// (issue #1746). It wraps ErrUnsupportedPlan so attach records a failed step +// instead of buying by filter or retrying silently. +var ErrBoundPlanMissingBinding = fmt.Errorf("%w: bound plan entry has no bound recommendations", ErrUnsupportedPlan) + // 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 @@ -25,14 +31,12 @@ func Precheck(plan *config.PurchasePlan) error { if err := plan.RampSchedule.ValidateAllocationMode(); err != nil { return fmt.Errorf("%w: plan %s: %w", ErrUnsupportedPlan, plan.ID, err) } - keys := make([]string, 0, len(plan.Services)) - for k := range plan.Services { - keys = append(keys, k) + if err := CheckBoundBinding(plan); err != nil { + return err } - sort.Strings(keys) - for _, k := range keys { + for _, k := range sortedServiceKeys(plan) { svc := plan.Services[k] - if field := firstFilterSet(&svc); field != "" { + if field := firstFilterSet(&svc, config.IsBoundServiceKey(k)); field != "" { return fmt.Errorf("%w: plan %s service %q sets %s", ErrUnsupportedPlan, plan.ID, k, field) } } @@ -46,8 +50,35 @@ func Precheck(plan *config.PurchasePlan) error { return nil } -// firstFilterSet names the first filter field set on svc, or "". -func firstFilterSet(svc *config.ServiceConfig) string { +// CheckBoundBinding refuses a plan where an enabled entry is bound (a "#" key, +// or any entry of a plan with the bound_groups marker) but has no bound +// identities. Precheck runs it, and so do plan writes that arm or widen +// auto-purchase; de-escalating writes skip it so a stripped plan can still be +// stopped. +func CheckBoundBinding(plan *config.PurchasePlan) error { + marked := plan.RampSchedule.AllocationMode == config.RampAllocationBoundGroups + for _, k := range sortedServiceKeys(plan) { + svc := plan.Services[k] + if svc.Enabled && (marked || config.IsBoundServiceKey(k)) && len(svc.BoundIdentities) == 0 { + return fmt.Errorf("%w: plan %s service %q", ErrBoundPlanMissingBinding, plan.ID, k) + } + } + return nil +} + +func sortedServiceKeys(plan *config.PurchasePlan) []string { + keys := make([]string, 0, len(plan.Services)) + for k := range plan.Services { + keys = append(keys, k) + } + sort.Strings(keys) + return keys +} + +// firstFilterSet names the first filter field set on svc, or "". A bound +// entry's min_count of exactly 1 is its rollback sentinel (older binaries +// refuse any min_count), not a filter: every recommendation's count is >= 1. +func firstFilterSet(svc *config.ServiceConfig, bound bool) string { switch { case len(svc.IncludeEngines) > 0: return "include_engines" @@ -61,7 +92,7 @@ func firstFilterSet(svc *config.ServiceConfig) string { return "include_types" case len(svc.ExcludeTypes) > 0: return "exclude_types" - case svc.MinCount != 0: + case svc.MinCount != 0 && (!bound || svc.MinCount != 1): return "min_count" } return ""