feat(planstep): resolve bound groups and re-check arming when they widen

internal/api/auto_purchase_bound_escalation.go

@@ -0,0 +1,82 @@
+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 := range before.Services {
+		svc := before.Services[k]
+		old[svc.EffectiveGroupID(k)] = svc
+	}
+	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
+		}
+	}
+	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
+}

internal/api/auto_purchase_bound_escalation_test.go

@@ -0,0 +1,96 @@
+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("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()))
+	})
+}
+
+// (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")))
+}

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 {
@@ -133,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)

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"))
+		})
+	}
+}

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{}{}
 		}
 	}
@@ -53,12 +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 plan's "#<group_id>" 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 {
+		if bound {
+			k, _, _ = strings.Cut(k, BoundKeySeparator)
+		}
 		if i := strings.IndexAny(k, "/:"); i > 0 && i < len(k)-1 {
 			seen[k[i+1:]] = struct{}{}
 		}

internal/config/purchase_plan_providers_test.go

@@ -100,3 +100,41 @@ 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 "#<group_id>" 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))
+	})
+}
+
+// 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)
+}

internal/config/purchase_plan_services_bound_test.go

@@ -0,0 +1,30 @@
+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": {},
+	}}
+	p.RampSchedule.AllocationMode = RampAllocationBoundGroups
+	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))
+}
+
+// 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))
+}