From 1acdd72d231b989d9274509b4e6f990605cc239d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 12:56:23 +0100 Subject: [PATCH 1/7] feat(planstep): resolve a bound group from its selected recommendations Slice A, steps A2 and A3 of #1746. - DerivePlanServices drops a bound entry's "#" suffix, so the arming checks and grant constraints see "ec2" for "aws/ec2#g1". - resolveService keeps only a bound group's own identities from the strict read, before the plan match, suppression and account checks that still apply. A matching recommendation that was not selected is never bought and never enters the frozen baseline. - Bound identities the strict read didn't show are returned as Result.MissingBound and logged at warn with plan and service. Some missing: the rest are sized. All missing: the empty result takes the existing ErrNoMatch grace path. - On a bound plan, planServiceOf matches a baselined identity to the enabled group that bound it, so targetMet judges each group against its own coverage, including groups that differ only in payment. A key no group bound is not judged. Unmarked plans keep today's first-entry match (a regression test covers the payment-split case). 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 --- internal/config/purchase_plan_providers.go | 2 + .../purchase_plan_services_bound_test.go | 22 ++++ internal/planstep/bound_resolve_test.go | 102 +++++++++++++++++ internal/planstep/resolve.go | 103 ++++++++++++++++-- 4 files changed, 219 insertions(+), 10 deletions(-) create mode 100644 internal/config/purchase_plan_services_bound_test.go create mode 100644 internal/planstep/bound_resolve_test.go diff --git a/internal/config/purchase_plan_providers.go b/internal/config/purchase_plan_providers.go index bd4af2829..35882a919 100644 --- a/internal/config/purchase_plan_providers.go +++ b/internal/config/purchase_plan_providers.go @@ -53,12 +53,14 @@ func serviceEntryProvider(key, fieldProvider string) string { // DerivePlanServices extracts the distinct service names a plan targets from // the "provider/service" and "provider:service" keys of plan.Services, sorted. +// A bound entry's "#" suffix is not part of its service name. func DerivePlanServices(plan *PurchasePlan) []string { if plan == nil { return nil } seen := make(map[string]struct{}, len(plan.Services)) for k := range plan.Services { + k, _, _ = strings.Cut(k, BoundKeySeparator) if i := strings.IndexAny(k, "/:"); i > 0 && i < len(k)-1 { seen[k[i+1:]] = struct{}{} } diff --git a/internal/config/purchase_plan_services_bound_test.go b/internal/config/purchase_plan_services_bound_test.go new file mode 100644 index 000000000..fafe5f088 --- /dev/null +++ b/internal/config/purchase_plan_services_bound_test.go @@ -0,0 +1,22 @@ +package config + +import ( + "testing" + + "github.com/stretchr/testify/assert" +) + +// Fixtures are hand-built plans, not real configuration (issue #1746). + +func TestDerivePlanServices_CutsBoundGroupSuffix(t *testing.T) { + p := &PurchasePlan{Services: map[string]ServiceConfig{ + "aws/ec2#g1": {}, "aws/ec2#g2": {}, "aws/rds#g3": {}, + }} + assert.Equal(t, []string{"ec2", "rds"}, DerivePlanServices(p)) + providers, err := DerivePlanProviders(p) + assert.NoError(t, err) + assert.Equal(t, []string{"aws"}, providers) + // (guard) legacy keys are unchanged. + legacy := &PurchasePlan{Services: map[string]ServiceConfig{"aws/ec2": {}, "aws:savings-plans-compute": {}}} + assert.Equal(t, []string{"ec2", "savings-plans-compute"}, DerivePlanServices(legacy)) +} diff --git a/internal/planstep/bound_resolve_test.go b/internal/planstep/bound_resolve_test.go new file mode 100644 index 000000000..c6dbcdb03 --- /dev/null +++ b/internal/planstep/bound_resolve_test.go @@ -0,0 +1,102 @@ +package planstep + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Fixtures are in-memory fakes, not a real database or cloud (issue #1746). + +func azureRecIn(region, payment string) config.RecommendationRecord { + r := azureRec(10, 100) + r.Region, r.Payment = region, payment + r.ID = "azure|acct-T|compute|" + region + "|Standard_D2s_v3||1|" + payment + return r +} + +func boundAzurePlan(entries map[string]config.ServiceConfig) *config.PurchasePlan { + return &config.PurchasePlan{ID: "plan-b", Services: entries, RampSchedule: config.RampSchedule{ + Type: "custom", PercentPerStep: 50, TotalSteps: 2, AllocationMode: config.RampAllocationBoundGroups, + }} +} + +func azureGroup(payment string, coverage float64, ids ...string) config.ServiceConfig { + return config.ServiceConfig{Provider: "azure", Service: "compute", Enabled: true, Term: 1, Payment: payment, + Coverage: coverage, MinCount: 1, BoundIdentities: ids} +} + +func TestResolve_BoundGroupBuysOnlyItsSelection(t *testing.T) { + west, north := azureRecIn("westeurope", "monthly"), azureRecIn("northeurope", "monthly") + rows := []config.RecommendationRecord{west, north} + plan := boundAzurePlan(map[string]config.ServiceConfig{"azure/compute#g1": azureGroup("monthly", 100, cumKey(t, west))}) + res, err := Resolve(t.Context(), fakeSource{shown: rows}, fakeStore{rows: rows}, plan, accts, 1, nil, nil) + require.NoError(t, err) + require.Len(t, res.Recs, 1, "the matching but unselected northeurope rec is not bought") + assert.Equal(t, "westeurope", res.Recs[0].Region) + assert.Empty(t, res.MissingBound) + assert.Contains(t, res.Baseline, cumKey(t, west)) + assert.NotContains(t, res.Baseline, cumKey(t, north), "an unselected rec never enters the baseline") +} + +func TestResolve_BoundIdentitiesMissingFromTheStrictRead(t *testing.T) { + west, north := azureRecIn("westeurope", "monthly"), azureRecIn("northeurope", "monthly") + gone := azureRecIn("uksouth", "monthly") + // Some missing: the present one is sized, the missing one is reported. + plan := boundAzurePlan(map[string]config.ServiceConfig{"azure/compute#g1": azureGroup("monthly", 100, cumKey(t, west), cumKey(t, gone))}) + rows := []config.RecommendationRecord{west, north} + res, err := Resolve(t.Context(), fakeSource{shown: rows}, fakeStore{rows: rows}, plan, accts, 1, nil, nil) + require.NoError(t, err) + require.Len(t, res.Recs, 1) + assert.Equal(t, []string{cumKey(t, gone)}, res.MissingBound) + // All missing: nothing to buy takes the ErrNoMatch grace path. + plan = boundAzurePlan(map[string]config.ServiceConfig{"azure/compute#g1": azureGroup("monthly", 100, cumKey(t, gone))}) + res, err = Resolve(t.Context(), fakeSource{shown: rows}, fakeStore{rows: rows}, plan, accts, 1, nil, nil) + require.ErrorIs(t, err, ErrNoMatch) + assert.Equal(t, []string{cumKey(t, gone)}, res.MissingBound) +} + +// targetMet judges a baselined identity only against the group that bound it. +func TestResolve_TargetMetJudgesByBoundGroup(t *testing.T) { + monthly, upfront := azureRecIn("westeurope", "monthly"), azureRecIn("westeurope", "upfront") + km, ku := cumKey(t, monthly), cumKey(t, upfront) + baseline := config.PlanRampBaseline{km: {Count: 10}, ku: {Count: 10}} + committed := map[string]config.PlanRampQuantity{km: {Count: 10}, ku: {Count: 5}} + // Step 2 at 50%/step is cumulative 100%: g1 (coverage 100) needs 10, g2 (coverage 50) needs 5. + bound := boundAzurePlan(map[string]config.ServiceConfig{ + "azure/compute#g1": azureGroup("monthly", 100, km), + "azure/compute#g2": azureGroup("upfront", 50, ku), + }) + _, err := Resolve(t.Context(), fakeSource{}, fakeStore{}, bound, accts, 2, baseline, committed) + require.ErrorIs(t, err, ErrTargetMet, "each group is judged against its own coverage") + + // (guard) an unmarked plan with the same payment split keeps today's + // first-entry match: the upfront key is judged at coverage 100 and is short. + legacy := &config.PurchasePlan{ID: "plan-l", RampSchedule: config.RampSchedule{Type: "custom", PercentPerStep: 50, TotalSteps: 2}, + Services: map[string]config.ServiceConfig{ + "azure:compute": {Provider: "azure", Service: "compute", Enabled: true, Term: 1, Payment: "monthly", Coverage: 100}, + "azure:compute-u": {Provider: "azure", Service: "compute", Enabled: true, Term: 1, Payment: "upfront", Coverage: 50}, + }} + _, err = Resolve(t.Context(), fakeSource{}, fakeStore{}, legacy, accts, 2, baseline, committed) + require.ErrorIs(t, err, ErrNoMatch) + + // A disabled group's identities are not judged, as for any disabled entry. + g2 := bound.Services["azure/compute#g2"] + g2.Enabled = false + bound.Services["azure/compute#g2"] = g2 + short := map[string]config.PlanRampQuantity{km: {Count: 10}, ku: {Count: 0}} + _, err = Resolve(t.Context(), fakeSource{}, fakeStore{}, bound, accts, 2, baseline, short) + require.ErrorIs(t, err, ErrTargetMet) + g2.Enabled = true + bound.Services["azure/compute#g2"] = g2 + + // A baselined key that no group bound is not judged, even though its + // attributes match g1: with nothing judged the step is not met. + other := azureRecIn("northeurope", "monthly") + ko := cumKey(t, other) + onlyOther := config.PlanRampBaseline{ko: {Count: 10}} + _, err = Resolve(t.Context(), fakeSource{}, fakeStore{}, bound, accts, 2, onlyOther, map[string]config.PlanRampQuantity{ko: {Count: 10}}) + require.ErrorIs(t, err, ErrNoMatch) +} diff --git a/internal/planstep/resolve.go b/internal/planstep/resolve.go index b0ca1ca2a..55b33ea47 100644 --- a/internal/planstep/resolve.go +++ b/internal/planstep/resolve.go @@ -10,11 +10,13 @@ import ( "fmt" "math" "math/big" + "slices" "sort" "strconv" "strings" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/logging" "github.com/LeanerCloud/cloud-commitments-platform/internal/config" ) @@ -61,6 +63,9 @@ type Result struct { // what the plan already holds. Set only with ErrTargetMet, for the no-op // row's note. Target, Committed int + // MissingBound lists bound identities (issue #1746) the strict read did + // not show, through churn or suppression. They are not bought this step. + MissingBound []string } // ramp carries the cumulative-sizing inputs and accumulates its outputs. @@ -146,7 +151,11 @@ func (r *ramp) resolveServices(ctx context.Context, src Source, store Store, pla if !svc.Enabled { continue } - recs, err := resolveService(ctx, src, store, &svc, plan.RampSchedule.PercentPerStep, r, accountIDs, allowed, enabledProviders) + recs, missing, err := resolveService(ctx, src, store, &svc, plan.RampSchedule.PercentPerStep, r, accountIDs, allowed, enabledProviders) + if len(missing) > 0 { + logging.Warnf("plan %s service %q: %d bound recommendation(s) not shown, not bought this step: %v", plan.ID, k, len(missing), missing) + r.out.MissingBound = append(r.out.MissingBound, missing...) + } if err == nil { err = claimIdentities(seen, recs) } @@ -190,9 +199,14 @@ func (r *ramp) targetMet(plan *config.PurchasePlan) (bool, error) { return judged > 0, nil } -// planServiceOf finds the enabled plan service an identity key belongs to -// (provider, service and term are fields 0, 2 and 6 of the key). +// planServiceOf finds the enabled plan service an identity key belongs to. On +// a bound plan (issue #1746) that is the group whose bound identities hold the +// key, and a key no group bound is not judged. Otherwise it is the first entry +// with the key's provider, service and term (fields 0, 2 and 6), as before. func planServiceOf(plan *config.PurchasePlan, key string) (config.ServiceConfig, bool) { + if plan.RampSchedule.AllocationMode == config.RampAllocationBoundGroups { + return boundServiceOf(plan, key) + } f := strings.Split(key, "\x1f") if len(f) < 7 { return config.ServiceConfig{}, false @@ -206,6 +220,18 @@ func planServiceOf(plan *config.PurchasePlan, key string) (config.ServiceConfig, return config.ServiceConfig{}, false } +// boundServiceOf is planServiceOf for a bound plan: the enabled group whose +// bound identities hold key. +func boundServiceOf(plan *config.PurchasePlan, key string) (config.ServiceConfig, bool) { + for _, k := range sortedKeys(plan.Services) { + svc := plan.Services[k] + if svc.Enabled && slices.Contains(svc.BoundIdentities, key) { + return svc, true + } + } + return config.ServiceConfig{}, false +} + // Legacy targets multiply coverage*cum first; marked targets use exact shares. func (r *ramp) target(base int, coverage float64) (int, error) { if r.mode == config.RampAllocationEqualCount { @@ -246,41 +272,98 @@ func claimIdentities(seen map[string]struct{}, recs []config.RecommendationRecor return nil } +// resolveService sizes one plan service. A bound group (issue #1746) only +// considers its bound identities, and returns those the strict read did not +// show as missing. func resolveService(ctx context.Context, src Source, store Store, svc *config.ServiceConfig, percentPerStep float64, r *ramp, - accountIDs []string, allowed map[string]struct{}, enabledProviders []string) ([]config.RecommendationRecord, error) { + accountIDs []string, allowed map[string]struct{}, enabledProviders []string) ([]config.RecommendationRecord, []string, error) { wantPayment, err := r.validateInputs(svc, percentPerStep) if err != nil { - return nil, err + return nil, nil, err } filter := config.RecommendationFilter{Provider: svc.Provider, Service: svc.Service, AccountIDs: accountIDs} shown, err := src.ListRecommendationsStrict(ctx, filter) if err != nil { - return nil, err + return nil, nil, err } shown = config.FilterRecommendationsByEnabledProviders(shown, enabledProviders) stored, err := storedByIdentity(ctx, store, filter) if err != nil { - return nil, err + return nil, nil, err + } + bound := newBoundSet(svc.BoundIdentities) + if shown, err = bound.filter(shown); err != nil { + return nil, nil, err } var out []config.RecommendationRecord for i := range shown { rec := shown[i] match, err := matchesPlan(&rec, svc.Term, wantPayment, allowed) if err != nil { - return nil, err + return nil, nil, err } if !match { continue } sized, keep, err := r.sizeFromStored(&rec, stored, svc.Coverage, percentPerStep) if err != nil { - return nil, fmt.Errorf("recommendation %s: %w", rec.ID, err) + return nil, nil, fmt.Errorf("recommendation %s: %w", rec.ID, err) } if keep { out = append(out, sized) } } - return out, nil + return out, bound.missing(), nil +} + +// boundSet restricts a bound group to its bound identities and records which +// of them the strict read showed. A nil set (an unbound entry) admits every rec. +type boundSet struct { + ids []string + want map[string]bool // identity -> shown +} + +func newBoundSet(ids []string) *boundSet { + if len(ids) == 0 { + return nil + } + b := &boundSet{ids: ids, want: make(map[string]bool, len(ids))} + for _, id := range ids { + b.want[id] = false + } + return b +} + +// filter keeps the recs whose identity is bound and marks them shown. +func (b *boundSet) filter(recs []config.RecommendationRecord) ([]config.RecommendationRecord, error) { + if b == nil { + return recs, nil + } + var kept []config.RecommendationRecord + for i := range recs { + key, err := config.RecIdentityKey(&recs[i]) + if err != nil { + return nil, err + } + if _, ok := b.want[key]; ok { + b.want[key] = true + kept = append(kept, recs[i]) + } + } + return kept, nil +} + +func (b *boundSet) missing() []string { + if b == nil { + return nil + } + var out []string + for _, id := range b.ids { + if !b.want[id] { + out = append(out, id) + } + } + return out } // validateInputs rejects sizing inputs outside (0,100] (custom ramps too) and From 737c9b712a1d8bb15c106ed5a4d96cd83ffc2b5a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Thu, 5 Nov 2026 13:51:25 +0100 Subject: [PATCH 2/7] feat(api): re-check the arming grant when a bound plan's groups widen Slice A, step A5 of #1746. When the stored or the new plan carries the bound_groups marker, these writes to an armed plan also count as escalations, so the execute:purchases grant's constraints are checked again: - a new effective group id; - a bound set that isn't a subset of the old one, including bound to unbound (an empty set means filter re-match); - a group cap raised or removed (a nil group cap is unbounded, unlike capCents(nil)); - a group's term or normalized payment changing; - a disabled group re-enabled; - the bound_groups marker removed. The provider and service sets can't show any of these. Unmarked plans keep today's rules exactly; a guard test pins that term-only and payment-only edits don't escalate. 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_escalation.go | 77 ++++++++++++++++++ .../auto_purchase_bound_escalation_test.go | 78 +++++++++++++++++++ internal/api/auto_purchase_grant.go | 3 +- 3 files changed, 157 insertions(+), 1 deletion(-) create mode 100644 internal/api/auto_purchase_bound_escalation.go create mode 100644 internal/api/auto_purchase_bound_escalation_test.go diff --git a/internal/api/auto_purchase_bound_escalation.go b/internal/api/auto_purchase_bound_escalation.go new file mode 100644 index 000000000..4e30c0a0b --- /dev/null +++ b/internal/api/auto_purchase_bound_escalation.go @@ -0,0 +1,77 @@ +package api + +import ( + "slices" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" +) + +// boundGroupsEscalate reports whether a write widens what an armed bound plan +// (issue #1746) buys in a way the provider and service sets cannot show, so +// the arming grant's constraints must be checked again. It applies when the +// stored or the new plan carries the bound_groups marker; unmarked plans keep +// today's rules. +func boundGroupsEscalate(before, after *config.PurchasePlan) bool { + beforeBound := isBoundPlan(before) + afterBound := isBoundPlan(after) + if !beforeBound && !afterBound { + return false + } + if beforeBound && !afterBound { + return true // the marker was removed + } + old := make(map[string]config.ServiceConfig, len(before.Services)) + for k, svc := range before.Services { + old[svc.EffectiveGroupID(k)] = svc + } + for k := range after.Services { + svc := after.Services[k] + prev, ok := old[svc.EffectiveGroupID(k)] + if !ok || groupWidens(&prev, &svc) { + return true + } + } + return false +} + +func isBoundPlan(p *config.PurchasePlan) bool { + return p != nil && p.RampSchedule.AllocationMode == config.RampAllocationBoundGroups +} + +// groupWidens compares one group before and after a write. +func groupWidens(prev, next *config.ServiceConfig) bool { + if next.Enabled && !prev.Enabled { + return true + } + // An empty set means filter re-match, the widest binding there is. + if len(prev.BoundIdentities) > 0 && len(next.BoundIdentities) == 0 { + return true + } + for _, id := range next.BoundIdentities { + if !slices.Contains(prev.BoundIdentities, id) { + return true + } + } + if groupCapRaised(prev.MaxStepCommitment, next.MaxStepCommitment) { + return true + } + return next.Term != prev.Term || canonicalPayment(next) != canonicalPayment(prev) +} + +// groupCapRaised treats a nil group cap as unbounded, unlike capCents(nil). +func groupCapRaised(prev, next *float64) bool { + if next == nil { + return prev != nil + } + if prev == nil { + return false + } + return capCents(next) > capCents(prev) +} + +func canonicalPayment(svc *config.ServiceConfig) string { + if canon, ok := config.NormalizePaymentOption(svc.Provider, svc.Payment); ok { + return canon + } + return svc.Payment +} diff --git a/internal/api/auto_purchase_bound_escalation_test.go b/internal/api/auto_purchase_bound_escalation_test.go new file mode 100644 index 000000000..7bc6980c1 --- /dev/null +++ b/internal/api/auto_purchase_bound_escalation_test.go @@ -0,0 +1,78 @@ +package api + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-platform/internal/config" + "github.com/stretchr/testify/assert" +) + +// Fixtures are hand-built plans, not real configuration (issue #1746). + +// armedBoundPlan is an armed bound plan with one group g1. +func armedBoundPlan() *config.PurchasePlan { + p := apgStoredPlan(true, apgCap(1000)) + p.RampSchedule.AllocationMode = config.RampAllocationBoundGroups + p.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{"k1", "k2"}, MaxStepCommitment: apgCap(100), + }} + return p +} + +// edited returns a copy of the armed bound plan with f applied to group g1. +func edited(f func(p *config.PurchasePlan, g *config.ServiceConfig)) *config.PurchasePlan { + p := armedBoundPlan() + g := p.Services["aws/ec2#g1"] + f(p, &g) + p.Services["aws/ec2#g1"] = g + return p +} + +func TestPlanArmEscalates_BoundGroups(t *testing.T) { + for _, tc := range []struct { + name string + after *config.PurchasePlan + want bool + }{ + {"unchanged", armedBoundPlan(), false}, + {"narrowed bound set", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.BoundIdentities = []string{"k1"} }), false}, + {"lowered group cap", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.MaxStepCommitment = apgCap(50) }), false}, + {"coverage edit", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.Coverage = 60 }), false}, + {"new group", edited(func(p *config.PurchasePlan, _ *config.ServiceConfig) { + p.Services["aws/ec2#g2"] = config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: true, Term: 3, Payment: "all-upfront", GroupID: "g2", BoundIdentities: []string{"k9"}} + }), true}, + {"added bound identity", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.BoundIdentities = []string{"k1", "k2", "k3"} }), true}, + {"bound to unbound", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.BoundIdentities = nil }), true}, + {"raised group cap", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.MaxStepCommitment = apgCap(200) }), true}, + {"removed group cap (nil is unbounded)", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.MaxStepCommitment = nil }), true}, + {"term changed", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.Term = 3 }), true}, + {"payment changed", edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.Payment = "all-upfront" }), true}, + {"marker removed", edited(func(p *config.PurchasePlan, _ *config.ServiceConfig) { p.RampSchedule.AllocationMode = "" }), true}, + } { + t.Run(tc.name, func(t *testing.T) { + assert.Equal(t, tc.want, planArmEscalates(armedBoundPlan(), tc.after)) + }) + } + t.Run("re-enabling a disabled group", func(t *testing.T) { + before := edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.Enabled = false }) + assert.True(t, planArmEscalates(before, armedBoundPlan())) + assert.False(t, planArmEscalates(armedBoundPlan(), before), "disabling a group narrows") + }) + t.Run("a group cap added where none was set narrows", func(t *testing.T) { + before := edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.MaxStepCommitment = nil }) + assert.False(t, planArmEscalates(before, armedBoundPlan())) + }) +} + +// (guard) Unmarked armed plans keep today's rules: a term- or payment-only +// edit is not an escalation, so admin-key callers are unaffected. +func TestPlanArmEscalates_UnmarkedTermAndPaymentEditsDoNotEscalate(t *testing.T) { + plan := func(term int, payment string) *config.PurchasePlan { + p := apgStoredPlan(true, apgCap(1000)) + p.Services = map[string]config.ServiceConfig{"aws/rds": {Provider: "aws", Service: "rds", Enabled: true, Term: term, Payment: payment}} + return p + } + assert.False(t, planArmEscalates(plan(1, "no-upfront"), plan(3, "no-upfront"))) + assert.False(t, planArmEscalates(plan(1, "no-upfront"), plan(1, "all-upfront"))) +} diff --git a/internal/api/auto_purchase_grant.go b/internal/api/auto_purchase_grant.go index dfc6952a2..46c972d12 100644 --- a/internal/api/auto_purchase_grant.go +++ b/internal/api/auto_purchase_grant.go @@ -72,7 +72,8 @@ func planArmEscalates(before, after *config.PurchasePlan) bool { return true } return !subsetOf(afterProviders, beforeProviders) || - !subsetOf(config.DerivePlanServices(after), config.DerivePlanServices(before)) + !subsetOf(config.DerivePlanServices(after), config.DerivePlanServices(before)) || + boundGroupsEscalate(before, after) } func subsetOf(sub, super []string) bool { From fea2b01fdadc8edf61e65a102e320df632737816 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 01:38:21 +0100 Subject: [PATCH 3/7] fix(api): avoid copying service configs when comparing bound groups Part of #1746. Co-Authored-By: Claude Opus 5.5 --- internal/api/auto_purchase_bound_escalation.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/internal/api/auto_purchase_bound_escalation.go b/internal/api/auto_purchase_bound_escalation.go index 4e30c0a0b..56462279a 100644 --- a/internal/api/auto_purchase_bound_escalation.go +++ b/internal/api/auto_purchase_bound_escalation.go @@ -21,7 +21,8 @@ func boundGroupsEscalate(before, after *config.PurchasePlan) bool { return true // the marker was removed } old := make(map[string]config.ServiceConfig, len(before.Services)) - for k, svc := range before.Services { + for k := range before.Services { + svc := before.Services[k] old[svc.EffectiveGroupID(k)] = svc } for k := range after.Services { From 56c54f8f146ec6aa79afe208ef6157ab19406e90 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 04:59:28 +0100 Subject: [PATCH 4/7] fix(api): ignore disabled bound groups when checking arming escalation A disabled group buys nothing, so adding or widening one no longer counts as an escalation; enabling it still does. Tests also pin that arming a disarmed bound plan escalates even when the write narrows it. Part of #1746. Co-Authored-By: Claude Opus 5.5 --- internal/api/auto_purchase_bound_escalation.go | 4 ++++ .../api/auto_purchase_bound_escalation_test.go | 18 ++++++++++++++++++ 2 files changed, 22 insertions(+) diff --git a/internal/api/auto_purchase_bound_escalation.go b/internal/api/auto_purchase_bound_escalation.go index 56462279a..1c251a85f 100644 --- a/internal/api/auto_purchase_bound_escalation.go +++ b/internal/api/auto_purchase_bound_escalation.go @@ -27,6 +27,10 @@ func boundGroupsEscalate(before, after *config.PurchasePlan) bool { } for k := range after.Services { svc := after.Services[k] + // A disabled group buys nothing; enabling it later is checked then. + if !svc.Enabled { + continue + } prev, ok := old[svc.EffectiveGroupID(k)] if !ok || groupWidens(&prev, &svc) { return true diff --git a/internal/api/auto_purchase_bound_escalation_test.go b/internal/api/auto_purchase_bound_escalation_test.go index 7bc6980c1..a2c5c19a6 100644 --- a/internal/api/auto_purchase_bound_escalation_test.go +++ b/internal/api/auto_purchase_bound_escalation_test.go @@ -59,6 +59,24 @@ func TestPlanArmEscalates_BoundGroups(t *testing.T) { assert.True(t, planArmEscalates(before, armedBoundPlan())) assert.False(t, planArmEscalates(armedBoundPlan(), before), "disabling a group narrows") }) + t.Run("arming a disarmed bound plan escalates even when it narrows", func(t *testing.T) { + disarmed := armedBoundPlan() + disarmed.AutoPurchase = false + assert.True(t, planArmEscalates(disarmed, armedBoundPlan()), "unchanged") + narrowed := edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.BoundIdentities = []string{"k1"} }) + assert.True(t, planArmEscalates(disarmed, narrowed), "narrowed") + }) + t.Run("a disabled group buys nothing, so its edits wait for re-enabling", func(t *testing.T) { + withG2 := func(enabled bool, ids ...string) *config.PurchasePlan { + return edited(func(p *config.PurchasePlan, _ *config.ServiceConfig) { + p.Services["aws/ec2#g2"] = config.ServiceConfig{Provider: "aws", Service: "ec2", Enabled: enabled, Term: 1, Payment: "no-upfront", GroupID: "g2", BoundIdentities: ids} + }) + } + assert.False(t, planArmEscalates(armedBoundPlan(), withG2(false, "k9")), "new disabled group") + assert.False(t, planArmEscalates(withG2(false, "k9"), withG2(false, "k9", "k8")), "disabled group widened") + assert.True(t, planArmEscalates(withG2(false, "k9"), withG2(true, "k9")), "enabling it") + assert.True(t, planArmEscalates(armedBoundPlan(), withG2(true, "k9")), "new enabled group") + }) t.Run("a group cap added where none was set narrows", func(t *testing.T) { before := edited(func(_ *config.PurchasePlan, g *config.ServiceConfig) { g.MaxStepCommitment = nil }) assert.False(t, planArmEscalates(before, armedBoundPlan())) From aaaff7307498b3316608c040044dfe3b0ebd8c3e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 15:01:38 +0100 Subject: [PATCH 5/7] fix(config): require a bound key to name its own provider and service The arming grant derives its provider and service scopes from a plan's services keys, while sizing buys by each entry's own provider and service fields. A bound key with no "provider/" prefix, such as "ec2#g/1", slipped through: its group id was read as part of the provider ("ec2#g") and its service was dropped from the scopes, so a grant restricted to another service could arm a plan that buys ec2. - checkGroupKey now requires the key prefix to be the entry's own "provider/service" (or "provider:service"). - serviceEntryProvider cuts the "#" suffix before it looks for a separator, so a "/" inside a group id is never a provider boundary. Found by CodeRabbit on #1834. Part of #1746. Co-Authored-By: Claude Sonnet 5.5 --- internal/config/purchase_plan_providers.go | 1 + .../config/purchase_plan_providers_test.go | 27 +++++++++++++++++++ internal/config/validation_bound_groups.go | 16 ++++++++--- .../config/validation_bound_groups_test.go | 6 +++++ 4 files changed, 46 insertions(+), 4 deletions(-) diff --git a/internal/config/purchase_plan_providers.go b/internal/config/purchase_plan_providers.go index 35882a919..e51bd5666 100644 --- a/internal/config/purchase_plan_providers.go +++ b/internal/config/purchase_plan_providers.go @@ -45,6 +45,7 @@ func DerivePlanProviders(plan *PurchasePlan) ([]string, error) { // serviceEntryProvider returns the provider a services entry names: the key's // prefix when the key has one, else the entry's Provider field, else "". func serviceEntryProvider(key, fieldProvider string) string { + key, _, _ = strings.Cut(key, BoundKeySeparator) // the group id may hold a "/" if i := strings.IndexAny(key, "/:"); i > 0 { return key[:i] } diff --git a/internal/config/purchase_plan_providers_test.go b/internal/config/purchase_plan_providers_test.go index 6bdb8c6cd..71ade329b 100644 --- a/internal/config/purchase_plan_providers_test.go +++ b/internal/config/purchase_plan_providers_test.go @@ -100,3 +100,30 @@ func TestDerivePlanProviders_LegacyKeyUsesProviderField(t *testing.T) { require.NoError(t, err) assert.Equal(t, []string{"gcp"}, got, "a key that names a provider still wins, as before") } + +// A bound key's "#" suffix is never part of its provider or service, +// even when the group id holds a "/" (issue #1746). +func TestDerivePlan_BoundGroupIDIsNeitherProviderNorService(t *testing.T) { + bound := func(entries map[string]ServiceConfig) *PurchasePlan { + p := &PurchasePlan{Services: entries} + p.RampSchedule.AllocationMode = RampAllocationBoundGroups + return p + } + t.Run("a key with no provider prefix takes its provider from the entry", func(t *testing.T) { + providers, err := DerivePlanProviders(bound(map[string]ServiceConfig{ + "ec2#g/1": {Provider: "aws", Service: "ec2", GroupID: "g/1"}, + })) + require.NoError(t, err) + assert.Equal(t, []string{"aws"}, providers) + }) + t.Run("a slash in the group id does not hide a service", func(t *testing.T) { + plan := bound(map[string]ServiceConfig{ + "aws/ec2#g/1": {Provider: "aws", Service: "ec2", GroupID: "g/1"}, + "aws/rds#g2": {Provider: "aws", Service: "rds", GroupID: "g2"}, + }) + providers, err := DerivePlanProviders(plan) + require.NoError(t, err) + assert.Equal(t, []string{"aws"}, providers) + assert.Equal(t, []string{"ec2", "rds"}, DerivePlanServices(plan)) + }) +} diff --git a/internal/config/validation_bound_groups.go b/internal/config/validation_bound_groups.go index ffa11f69b..0a5eed4c8 100644 --- a/internal/config/validation_bound_groups.go +++ b/internal/config/validation_bound_groups.go @@ -99,11 +99,19 @@ func checkGroupBindings(svc *ServiceConfig, gid string, seen map[string]string) // 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. +// field can never name different groups. The key's provider and service must +// also be the entry's own: the arming grant's scopes are derived from the key, +// while sizing buys by the entry's fields. 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) + prefix, suffix, ok := strings.Cut(key, BoundKeySeparator) + if !ok || svc.GroupID == "" || suffix != svc.GroupID || !namesEntry(prefix, svc) { + return fmt.Errorf("%w: entry %q must be keyed \"%s/%s%s%s\" (its own provider, service and group_id)", ErrInvalidBoundGroup, key, svc.Provider, svc.Service, BoundKeySeparator, svc.GroupID) } return nil } + +// namesEntry reports whether a key prefix is the entry's "provider/service" or +// "provider:service". +func namesEntry(prefix string, svc *ServiceConfig) bool { + return prefix == svc.Provider+"/"+svc.Service || prefix == svc.Provider+":"+svc.Service +} diff --git a/internal/config/validation_bound_groups_test.go b/internal/config/validation_bound_groups_test.go index b780301e1..0cf466a76 100644 --- a/internal/config/validation_bound_groups_test.go +++ b/internal/config/validation_bound_groups_test.go @@ -43,6 +43,12 @@ func TestValidate_BoundGroups(t *testing.T) { {"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`}, + {"key without a provider prefix", map[string]ServiceConfig{"ec2#g1": ec2a}, `must be keyed`}, + {"key without a provider prefix and a slash in the group id", map[string]ServiceConfig{"ec2#g/1": withGroup(ec2a, "g/1")}, `must be keyed`}, + {"key provider differs from the entry's", map[string]ServiceConfig{"azure/ec2#g1": ec2a}, `must be keyed`}, + {"key service differs from the entry's", map[string]ServiceConfig{"aws/rds#g1": ec2a}, `must be keyed`}, + {"colon-separated key", map[string]ServiceConfig{"aws:ec2#g1": ec2a}, ""}, + {"slash in the group id", map[string]ServiceConfig{"aws/ec2#g/1": withGroup(ec2a, "g/1")}, ""}, {"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"}, From de3ea4b56c719531ea63096a53bc0d5b1b63fd92 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 16:20:58 +0100 Subject: [PATCH 6/7] fix(config): derive provider and service scopes from group-free keys only for bound plans The "#" cut added for bound plans also changed how unmarked plans derive their arming-grant scopes: a legacy key such as "team#west/ec2" read its provider from the entry field and lost its service. Unmarked plans keep exactly the parsing main has; only a plan carrying the bound_groups marker cuts the group suffix, in both DerivePlanProviders and DerivePlanServices. Found by CodeRabbit and an independent review of #1834. Part of #1746. Co-Authored-By: Claude Sonnet 5.5 --- internal/config/purchase_plan_providers.go | 15 +++++++++++---- internal/config/purchase_plan_providers_test.go | 11 +++++++++++ .../config/purchase_plan_services_bound_test.go | 8 ++++++++ 3 files changed, 30 insertions(+), 4 deletions(-) diff --git a/internal/config/purchase_plan_providers.go b/internal/config/purchase_plan_providers.go index e51bd5666..1f342e73b 100644 --- a/internal/config/purchase_plan_providers.go +++ b/internal/config/purchase_plan_providers.go @@ -26,8 +26,13 @@ func DerivePlanProviders(plan *PurchasePlan) ([]string, error) { return nil, nil } seen := make(map[string]struct{}, len(plan.Services)) + bound := plan.RampSchedule.AllocationMode == RampAllocationBoundGroups for k := range plan.Services { - if provider := serviceEntryProvider(k, plan.Services[k].Provider); provider != "" { + key := k + if bound { + key, _, _ = strings.Cut(key, BoundKeySeparator) // the group id may hold a "/" + } + if provider := serviceEntryProvider(key, plan.Services[k].Provider); provider != "" { seen[provider] = struct{}{} } } @@ -45,7 +50,6 @@ func DerivePlanProviders(plan *PurchasePlan) ([]string, error) { // serviceEntryProvider returns the provider a services entry names: the key's // prefix when the key has one, else the entry's Provider field, else "". func serviceEntryProvider(key, fieldProvider string) string { - key, _, _ = strings.Cut(key, BoundKeySeparator) // the group id may hold a "/" if i := strings.IndexAny(key, "/:"); i > 0 { return key[:i] } @@ -54,14 +58,17 @@ func serviceEntryProvider(key, fieldProvider string) string { // DerivePlanServices extracts the distinct service names a plan targets from // the "provider/service" and "provider:service" keys of plan.Services, sorted. -// A bound entry's "#" suffix is not part of its service name. +// A bound plan's "#" suffix is not part of its service name. func DerivePlanServices(plan *PurchasePlan) []string { if plan == nil { return nil } seen := make(map[string]struct{}, len(plan.Services)) + bound := plan.RampSchedule.AllocationMode == RampAllocationBoundGroups for k := range plan.Services { - k, _, _ = strings.Cut(k, BoundKeySeparator) + if bound { + k, _, _ = strings.Cut(k, BoundKeySeparator) + } if i := strings.IndexAny(k, "/:"); i > 0 && i < len(k)-1 { seen[k[i+1:]] = struct{}{} } diff --git a/internal/config/purchase_plan_providers_test.go b/internal/config/purchase_plan_providers_test.go index 71ade329b..e3a00ccb1 100644 --- a/internal/config/purchase_plan_providers_test.go +++ b/internal/config/purchase_plan_providers_test.go @@ -127,3 +127,14 @@ func TestDerivePlan_BoundGroupIDIsNeitherProviderNorService(t *testing.T) { assert.Equal(t, []string{"ec2", "rds"}, DerivePlanServices(plan)) }) } + +// An unmarked plan keeps today's provider parsing: its key is read up to the +// first separator, "#" or not (#1746 changes bound plans only). +func TestDerivePlanProviders_UnmarkedKeyWithHashKeepsItsParsing(t *testing.T) { + plan := &PurchasePlan{Services: map[string]ServiceConfig{ + "team#west/ec2": {Provider: "aws", Service: "ec2"}, + }} + providers, err := DerivePlanProviders(plan) + require.NoError(t, err) + assert.Equal(t, []string{"team#west"}, providers) +} diff --git a/internal/config/purchase_plan_services_bound_test.go b/internal/config/purchase_plan_services_bound_test.go index fafe5f088..8ad27bbff 100644 --- a/internal/config/purchase_plan_services_bound_test.go +++ b/internal/config/purchase_plan_services_bound_test.go @@ -12,6 +12,7 @@ func TestDerivePlanServices_CutsBoundGroupSuffix(t *testing.T) { p := &PurchasePlan{Services: map[string]ServiceConfig{ "aws/ec2#g1": {}, "aws/ec2#g2": {}, "aws/rds#g3": {}, }} + p.RampSchedule.AllocationMode = RampAllocationBoundGroups assert.Equal(t, []string{"ec2", "rds"}, DerivePlanServices(p)) providers, err := DerivePlanProviders(p) assert.NoError(t, err) @@ -20,3 +21,10 @@ func TestDerivePlanServices_CutsBoundGroupSuffix(t *testing.T) { legacy := &PurchasePlan{Services: map[string]ServiceConfig{"aws/ec2": {}, "aws:savings-plans-compute": {}}} assert.Equal(t, []string{"ec2", "savings-plans-compute"}, DerivePlanServices(legacy)) } + +// An unmarked plan keeps today's service parsing: a "#" in its key is not a +// group suffix, so the entry still names its service (#1746 changes bound plans only). +func TestDerivePlanServices_UnmarkedKeyWithHashKeepsItsService(t *testing.T) { + p := &PurchasePlan{Services: map[string]ServiceConfig{"aws/rds": {}, "team#west/ec2": {}}} + assert.Equal(t, []string{"ec2", "rds"}, DerivePlanServices(p)) +} From f5cefddf54aa167f8ee21597391c81460c351f45 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 9 Nov 2026 16:50:39 +0100 Subject: [PATCH 7/7] fix(api): refuse a stored bound plan whose key names another entry before deriving grant scopes The arming grant's scopes come from a plan's keys, while sizing buys by each entry's own provider and service. The retarget gate reads the stored plan without validating it, so a bound plan keyed "gcp/compute#g1" whose entry buys aws/ec2 could pass a grant scoped to compute and rds. requireAutoPurchaseGrantForPlan now runs PurchasePlan.CheckBoundKeys first and answers 409, before any scope is derived. Both of its callers are covered: the validated create and update gate (where the check is a no-op) and the unvalidated retarget gate. Found by CodeRabbit on #1834. Part of #1746. Co-Authored-By: Claude Sonnet 5.5 --- internal/api/auto_purchase_grant.go | 4 ++ internal/api/auto_purchase_scope_test.go | 45 ++++++++++++++++++++++ internal/config/validation_bound_groups.go | 21 ++++++++++ 3 files changed, 70 insertions(+) diff --git a/internal/api/auto_purchase_grant.go b/internal/api/auto_purchase_grant.go index 46c972d12..598abc271 100644 --- a/internal/api/auto_purchase_grant.go +++ b/internal/api/auto_purchase_grant.go @@ -134,6 +134,10 @@ func (h *Handler) requireAutoPurchaseGrantFor(ctx context.Context, req *events.L // providers and services the plan targets. A plan whose providers cannot be // derived fails closed: an empty provider set would read as no restriction. func (h *Handler) requireAutoPurchaseGrantForPlan(ctx context.Context, req *events.LambdaFunctionURLRequest, session *Session, accountIDs []string, plan *config.PurchasePlan) error { + // The scopes below come from the keys; sizing buys by each entry's own fields. + if err := plan.CheckBoundKeys(); err != nil { + return NewClientError(http.StatusConflict, err.Error()) + } providers, err := config.DerivePlanProviders(plan) if err != nil { logging.Errorf("requireAutoPurchaseGrantForPlan: %v", err) diff --git a/internal/api/auto_purchase_scope_test.go b/internal/api/auto_purchase_scope_test.go index 8cfd77dbf..e3dd1040f 100644 --- a/internal/api/auto_purchase_scope_test.go +++ b/internal/api/auto_purchase_scope_test.go @@ -295,3 +295,48 @@ func TestAutoPurchaseScope_RetargetProviderMismatchBeforeArmingGate(t *testing.T assert.Equal(t, 400, clientCode(t, err)) assert.False(t, apgCalled(store, "SetPlanAccounts")) } + +// The grant's scopes come from a plan's keys, while sizing buys by each entry's +// own provider and service. The retarget gate reads the stored plan without +// validating it, so a bound plan whose key names another provider or service is +// refused before any scope is derived (#1746): a grant covering the key's +// gcp/compute and aws/rds must not move a plan that buys aws/ec2. +func TestAutoPurchaseScope_RetargetRefusesBoundKeyNamingAnotherEntry(t *testing.T) { + grant := apsGrant(&auth.PermissionConstraints{Providers: []string{"aws", "gcp"}, Services: []string{"compute", "rds"}}) + bound := func(key string, svc config.ServiceConfig) *config.PurchasePlan { + p := apgStoredPlan(true, apgCap(1000)) + p.RampSchedule.AllocationMode = config.RampAllocationBoundGroups + p.Services = map[string]config.ServiceConfig{ + key: svc, + "aws/rds#g2": {Provider: "aws", Service: "rds", MinCount: 1, GroupID: "g2", BoundIdentities: []string{"k2"}}, + } + return p + } + entry := func(provider, service string) config.ServiceConfig { + return config.ServiceConfig{Provider: provider, Service: service, Enabled: true, MinCount: 1, GroupID: "g1", BoundIdentities: []string{"k1"}} + } + for _, tc := range []struct { + name string + stored *config.PurchasePlan + wantRefused bool + }{ + {"key names another provider and service", bound("gcp/compute#g1", entry("aws", "ec2")), true}, + {"key names another service", bound("aws/compute#g1", entry("aws", "ec2")), true}, + {"key without a provider prefix", bound("ec2#g1", entry("aws", "ec2")), true}, + {"a key naming its own entry still moves", bound("aws/rds#g1", entry("aws", "rds")), false}, + } { + t.Run(tc.name, func(t *testing.T) { + store, err := apsSetAccounts(t, grant, tc.stored, apsAcctA) + if tc.wantRefused { + 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(), "must be keyed") + assert.False(t, apgCalled(store, "SetPlanAccounts")) + return + } + require.NoError(t, err) + assert.True(t, apgCalled(store, "SetPlanAccounts")) + }) + } +} diff --git a/internal/config/validation_bound_groups.go b/internal/config/validation_bound_groups.go index 0a5eed4c8..b849e87b5 100644 --- a/internal/config/validation_bound_groups.go +++ b/internal/config/validation_bound_groups.go @@ -23,6 +23,27 @@ func (c *ServiceConfig) EffectiveGroupID(key string) string { return key } +// CheckBoundKeys refuses a bound plan whose entries are not keyed by their own +// provider, service and group (checkGroupKey). A gate that reads a stored plan +// without validating it calls this before it derives grant scopes from the keys. +func (p *PurchasePlan) CheckBoundKeys() error { + if p == nil || 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) // the error names the first bad key in a stable order + for _, k := range keys { + svc := p.Services[k] + if err := checkGroupKey(k, &svc); err != nil { + return err + } + } + return nil +} + // 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