From e86270aaf23c4cf42c82cc36820a19dbb0181ac9 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 15:33:02 +0200 Subject: [PATCH 1/7] fix(gcp): take the compute CUD term from the recommendation plan The converter used params.Term (default 1yr) and ignored the root Commitment plan, so a 36-month recommendation could be bought as 12 months and vice versa. The root plan is now authoritative. A conflicting params.Term, a root with no plan and no term, or a malformed plan skips that recommendation (errBadPlan) without aborting the batch. Closes #291 Co-Authored-By: Claude Sonnet 5.5 --- known_issues/01_gcp_compute_nonroot_term.md | 17 +++ .../gcp/services/computeengine/client.go | 20 ++-- .../services/computeengine/commitment_type.go | 81 ++++++++++++++ .../gcp/services/computeengine/dedupe_test.go | 14 ++- .../gcp/services/computeengine/plan_test.go | 105 ++++++++++++++++++ 5 files changed, 227 insertions(+), 10 deletions(-) create mode 100644 known_issues/01_gcp_compute_nonroot_term.md create mode 100644 providers/gcp/services/computeengine/plan_test.go diff --git a/known_issues/01_gcp_compute_nonroot_term.md b/known_issues/01_gcp_compute_nonroot_term.md new file mode 100644 index 00000000..5088dd81 --- /dev/null +++ b/known_issues/01_gcp_compute_nonroot_term.md @@ -0,0 +1,17 @@ +# Known Issues: GCP Compute Engine CUD term on the non-root payload path + +> **Audit status (2026-10-09):** `1 needs triage ยท 0 resolved` + +## MEDIUM: Non-root recommendation payloads still guess the term + +**Files:** `providers/gcp/services/computeengine/commitment_type.go` +(`resolveRecTerm`, `populateRecommendationResources`) + +**Description:** The fix for #291 reads the commitment `plan` from the root +Commitment operation. A recommendation with no root Commitment op has no plan +to read, so its term still comes from `params.Term`, defaulting to "1yr". + +**Why deferred:** The Recommender payload shape for non-root operations is +unverified, and #291 was scoped to the root path. Tracked in #304. + +**Status:** Open. diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index c857d51c..d3f61062 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -476,7 +476,7 @@ func (c *Client) GetRecommendations(ctx context.Context, p *common.Recommendatio // Structural payload errors are still returned and abort the batch. func (c *Client) convertOrSkipBadAmount(ctx context.Context, rec *recommenderpb.Recommendation, params common.RecommendationParams) (*common.Recommendation, error) { converted, err := c.convertGCPRecommendation(ctx, rec, params) - if errors.Is(err, errBadAmount) { + if errors.Is(err, errBadAmount) || errors.Is(err, errBadPlan) { log.Printf("computeengine: skipping recommendation %q: %v", rec.GetName(), err) return nil, nil } @@ -1182,8 +1182,11 @@ func skuMatchesMachineType(sku *cloudbilling.Sku, machineType, region string) bo // BreakEvenMonths so the scorer can filter and rank GCP recommendations correctly // (issue #1022 C2). Pricing failures are logged but do not discard the recommendation: // EstimatedSavings from the Recommender payload is the authoritative savings signal. -// Returns nil when the params.Term is unrecognized so the caller skips an -// unroutable recommendation rather than queuing a purchase with an invalid plan. +// The term comes from the root Commitment plan when present (issue #291); an +// explicit params.Term that disagrees, or a root without a plan and without +// params.Term, returns errBadPlan so the caller skips the recommendation. +// Returns nil when the term is unrecognized so the caller skips an unroutable +// recommendation rather than queuing a purchase with an invalid plan. func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) (*common.Recommendation, error) { // GCP CUDs are billed monthly with no upfront option; force "monthly" // unconditionally and log any non-monthly input so scheduler @@ -1195,11 +1198,12 @@ func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommend } paymentOption := "monthly" - // H-3: propagate params.Term (default "1yr") and validate it so an - // unrecognized term fails loud here rather than reaching buildInsertRequest. - term := params.Term - if term == "" { - term = "1yr" + // The root Commitment plan is authoritative (issue #291); resolveRecTerm + // reconciles it with params.Term and validates the result so an unrecognized + // term never reaches buildInsertRequest. + term, err := resolveRecTerm(gcpRec, params) + if err != nil { + return nil, err } if _, err := termPlan(term); err != nil { log.Printf("computeengine: skipping recommendation with unrecognized term %q: %v", term, err) diff --git a/providers/gcp/services/computeengine/commitment_type.go b/providers/gcp/services/computeengine/commitment_type.go index 9ed0d512..66a962f1 100644 --- a/providers/gcp/services/computeengine/commitment_type.go +++ b/providers/gcp/services/computeengine/commitment_type.go @@ -1,6 +1,7 @@ package computeengine import ( + "errors" "fmt" "strings" @@ -157,3 +158,83 @@ func populateRecommendationResources(gcpRec *recommenderpb.Recommendation, rec * rec.Count = count return extractMemoryMBFromRecommendation(gcpRec, rec) } + +// errBadPlan marks a recommendation whose commitment plan is malformed, +// missing where required, or in conflict with the caller's term. The +// recommendation is skipped rather than purchased with a guessed term. +var errBadPlan = errors.New("bad recommendation plan") + +// rootCommitmentTerm reads the root Commitment's plan and returns the +// canonical term ("1yr" or "3yr"). found is false when the root has no plan +// key. A present but malformed plan returns an error wrapping errBadPlan. +func rootCommitmentTerm(content *recommenderpb.RecommendationContent) (term string, found bool, err error) { + root, err := selectRootCommitment(content) + if err != nil || root == nil { + return "", false, err + } + object := root.GetValue().GetStructValue() + if object == nil { + return "", false, fmt.Errorf("root Commitment value must be an object") + } + value, present := object.Fields["plan"] + if !present { + return "", false, nil + } + name, ok := value.GetKind().(*structpb.Value_StringValue) + if !ok { + return "", false, fmt.Errorf("%w: root Commitment plan must be a string", errBadPlan) + } + switch name.StringValue { + case computepb.Commitment_TWELVE_MONTH.String(): + return "1yr", true, nil + case computepb.Commitment_THIRTY_SIX_MONTH.String(): + return "3yr", true, nil + default: + return "", false, fmt.Errorf("%w: unsupported root Commitment plan %q", errBadPlan, name.StringValue) + } +} + +// resolveRecTerm picks the term for a recommendation. A root Commitment plan +// is authoritative: an explicit params.Term must agree with it. A root +// Commitment without a plan needs an explicit params.Term. A payload with no +// root Commitment keeps the params.Term / "1yr" fallback (issue #291 follow-up). +func resolveRecTerm(gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) (string, error) { + planTerm, found, err := rootCommitmentTerm(gcpRec.GetContent()) + if err != nil { + return "", err + } + if !found { + return termWithoutPlan(gcpRec.GetContent(), params.Term) + } + if params.Term == "" { + return planTerm, nil + } + want, err := termPlan(params.Term) + if err != nil { + // Unrecognized explicit term: the caller skips it, as before. + return params.Term, nil + } + got, err := termPlan(planTerm) + if err != nil { + return "", err + } + if want != got { + return "", fmt.Errorf("%w: params term %q conflicts with recommendation plan %s", errBadPlan, params.Term, got) + } + return planTerm, nil +} + +// termWithoutPlan resolves the term when the payload carries no plan. +func termWithoutPlan(content *recommenderpb.RecommendationContent, paramTerm string) (string, error) { + root, err := selectRootCommitment(content) + if err != nil { + return "", err + } + if paramTerm != "" { + return paramTerm, nil + } + if root != nil { + return "", fmt.Errorf("%w: root Commitment has no plan and no explicit term was given", errBadPlan) + } + return "1yr", nil +} diff --git a/providers/gcp/services/computeengine/dedupe_test.go b/providers/gcp/services/computeengine/dedupe_test.go index e41653af..9074ac4e 100644 --- a/providers/gcp/services/computeengine/dedupe_test.go +++ b/providers/gcp/services/computeengine/dedupe_test.go @@ -18,12 +18,22 @@ import ( // Schema fixture, not a captured service response. func rootCUDRecommendation(kind string) *recommenderpb.Recommendation { - value, _ := structpb.NewStruct(map[string]any{ + return rootCUDRecommendationWithPlan(kind, "TWELVE_MONTH") +} + +// rootCUDRecommendationWithPlan builds the fixture with the given root plan; +// an empty plan omits the key. +func rootCUDRecommendationWithPlan(kind, plan string) *recommenderpb.Recommendation { + fields := map[string]any{ "type": kind, "resources": []any{ map[string]any{"type": "MEMORY", "amount": "6291456"}, map[string]any{"type": "VCPU", "amount": "480"}, }, - }) + } + if plan != "" { + fields["plan"] = plan + } + value, _ := structpb.NewStruct(fields) return &recommenderpb.Recommendation{ StateInfo: &recommenderpb.RecommendationStateInfo{State: recommenderpb.RecommendationStateInfo_ACTIVE}, Content: &recommenderpb.RecommendationContent{OperationGroups: []*recommenderpb.OperationGroup{{Operations: []*recommenderpb.Operation{{ diff --git a/providers/gcp/services/computeengine/plan_test.go b/providers/gcp/services/computeengine/plan_test.go new file mode 100644 index 00000000..20b449fe --- /dev/null +++ b/providers/gcp/services/computeengine/plan_test.go @@ -0,0 +1,105 @@ +package computeengine + +import ( + "context" + "testing" + + "cloud.google.com/go/recommender/apiv1/recommenderpb" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + "google.golang.org/protobuf/types/known/structpb" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func planClient(t *testing.T, recs ...*recommenderpb.Recommendation) (*Client, *MockCommitmentsService) { + t.Helper() + client, err := NewClient(context.Background(), "test-project", "us-central1") + require.NoError(t, err) + client.SetBillingService(&MockBillingService{err: assert.AnError}) + client.SetRecommenderClient(&MockRecommenderClient{iterator: &MockRecommenderIterator{recommendations: recs}}) + service := &MockCommitmentsService{operation: &MockOperation{}} + client.SetCommitmentsService(service) + return client, service +} + +// Issue #291: a THIRTY_SIX_MONTH recommendation with no caller term must be +// bought as THIRTY_SIX_MONTH. Pre-fix it was labelled 1yr and bought TWELVE_MONTH. +func TestRecommendationPlanDrivesPurchaseTerm(t *testing.T) { + cases := []struct { + name, plan, paramTerm, wantTerm, wantPlan string + }{ + {"36mo plan, empty term", "THIRTY_SIX_MONTH", "", "3yr", "THIRTY_SIX_MONTH"}, + {"12mo plan, empty term", "TWELVE_MONTH", "", "1yr", "TWELVE_MONTH"}, + {"36mo plan, 3yr", "THIRTY_SIX_MONTH", "3yr", "3yr", "THIRTY_SIX_MONTH"}, + {"36mo plan, 36mo spelling", "THIRTY_SIX_MONTH", "36mo", "3yr", "THIRTY_SIX_MONTH"}, + {"36mo plan, bare 3", "THIRTY_SIX_MONTH", " 3 ", "3yr", "THIRTY_SIX_MONTH"}, + {"no plan, explicit 3yr", "", "3yr", "3yr", "THIRTY_SIX_MONTH"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + client, service := planClient(t, rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", tc.plan)) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: tc.paramTerm}) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, tc.wantTerm, recs[0].Term) + _, err = client.PurchaseCommitment(context.Background(), recs[0], common.PurchaseOptions{}) + require.NoError(t, err) + assert.Equal(t, tc.wantPlan, service.lastInsertReq.CommitmentResource.GetPlan()) + }) + } +} + +func TestRecommendationPlanSkips(t *testing.T) { + malformed := func(v *structpb.Value) *recommenderpb.Recommendation { + rec := rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "") + rec.Content.OperationGroups[0].Operations[0].GetValue().GetStructValue().Fields["plan"] = v + return rec + } + cases := map[string]struct { + rec *recommenderpb.Recommendation + term string + }{ + "conflict 12mo vs 3yr": {rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "TWELVE_MONTH"), "3yr"}, + "conflict 36mo vs 1yr": {rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "THIRTY_SIX_MONTH"), "1yr"}, + "missing plan, no term": {rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", ""), ""}, + "UNDEFINED_PLAN": {malformed(structpb.NewStringValue("UNDEFINED_PLAN")), ""}, + "unknown plan": {malformed(structpb.NewStringValue("FIVE_YEAR")), ""}, + "empty plan": {malformed(structpb.NewStringValue("")), ""}, + "numeric plan": {malformed(structpb.NewNumberValue(36)), ""}, + "null plan": {malformed(structpb.NewNullValue()), ""}, + } + for name, tc := range cases { + t.Run(name, func(t *testing.T) { + client, service := planClient(t, tc.rec) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: tc.term}) + require.NoError(t, err) + assert.Empty(t, recs) + assert.Empty(t, service.insertReqs) + }) + } +} + +// A conflicting recommendation is skipped without dropping its good neighbour. +func TestRecommendationPlanConflictKeepsGoodRecommendation(t *testing.T) { + bad := rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "TWELVE_MONTH") + bad.Name = "bad" + good := rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "THIRTY_SIX_MONTH") + good.Name = "good" + client, service := planClient(t, bad, good) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: "3yr"}) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, "3yr", recs[0].Term) + assert.Empty(t, service.insertReqs, "listing must not insert") +} + +// Mutation probe: the plan-to-term mapping must not be swappable. +func TestRootCommitmentTermMapping(t *testing.T) { + for plan, want := range map[string]string{"TWELVE_MONTH": "1yr", "THIRTY_SIX_MONTH": "3yr"} { + got, found, err := rootCommitmentTerm(rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", plan).GetContent()) + require.NoError(t, err) + assert.True(t, found) + assert.Equal(t, want, got) + } +} From 7c5c0d2c0bcfde7891576677df84ac6a1b9990dc Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 15:38:15 +0200 Subject: [PATCH 2/7] fix(gcp): canonicalise the compute CUD term and add plan to the SDK fixture Add a plan to the sdkRootCommitment fixture, return the canonical 1yr/3yr term for explicit params.Term too, and drop the unreachable second root lookup in termWithoutPlan. Co-Authored-By: Claude Sonnet 5.5 --- providers/gcp/recommendations_sdk_test.go | 2 +- .../services/computeengine/commitment_type.go | 64 +++++++++++-------- .../gcp/services/computeengine/plan_test.go | 16 +++++ 3 files changed, 56 insertions(+), 26 deletions(-) diff --git a/providers/gcp/recommendations_sdk_test.go b/providers/gcp/recommendations_sdk_test.go index f696d747..5c953930 100644 --- a/providers/gcp/recommendations_sdk_test.go +++ b/providers/gcp/recommendations_sdk_test.go @@ -246,7 +246,7 @@ func sdkCostRecommendation(savings int64, state recommenderpb.RecommendationStat // Schema root fixture, not a captured service response. func sdkRootCommitment(kind, region string) *recommenderpb.RecommendationContent { value, _ := structpb.NewStruct(map[string]any{ - "type": kind, "resources": []any{ + "type": kind, "plan": "TWELVE_MONTH", "resources": []any{ map[string]any{"type": "MEMORY", "amount": "6291456"}, map[string]any{"type": "VCPU", "amount": "480"}, }, diff --git a/providers/gcp/services/computeengine/commitment_type.go b/providers/gcp/services/computeengine/commitment_type.go index 66a962f1..6a8ea972 100644 --- a/providers/gcp/services/computeengine/commitment_type.go +++ b/providers/gcp/services/computeengine/commitment_type.go @@ -169,9 +169,16 @@ var errBadPlan = errors.New("bad recommendation plan") // key. A present but malformed plan returns an error wrapping errBadPlan. func rootCommitmentTerm(content *recommenderpb.RecommendationContent) (term string, found bool, err error) { root, err := selectRootCommitment(content) - if err != nil || root == nil { + if err != nil { return "", false, err } + return rootPlanTerm(root) +} + +func rootPlanTerm(root *recommenderpb.Operation) (term string, found bool, err error) { + if root == nil { + return "", false, nil + } object := root.GetValue().GetStructValue() if object == nil { return "", false, fmt.Errorf("root Commitment value must be an object") @@ -194,46 +201,53 @@ func rootCommitmentTerm(content *recommenderpb.RecommendationContent) (term stri } } -// resolveRecTerm picks the term for a recommendation. A root Commitment plan -// is authoritative: an explicit params.Term must agree with it. A root -// Commitment without a plan needs an explicit params.Term. A payload with no -// root Commitment keeps the params.Term / "1yr" fallback (issue #291 follow-up). +// canonicalTerm maps any accepted term spelling to "1yr" or "3yr". An +// unrecognized term is returned unchanged so the caller skips it, as before. +func canonicalTerm(term string) string { + plan, err := termPlan(term) + if err != nil { + return term + } + if plan == computepb.Commitment_THIRTY_SIX_MONTH.String() { + return "3yr" + } + return "1yr" +} + +// resolveRecTerm picks the canonical term ("1yr"/"3yr") for a recommendation. +// A root Commitment plan is authoritative: an explicit params.Term must agree +// with it. A root Commitment without a plan needs an explicit params.Term. A +// payload with no root Commitment keeps the params.Term / "1yr" fallback +// (issue #291 follow-up). func resolveRecTerm(gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) (string, error) { - planTerm, found, err := rootCommitmentTerm(gcpRec.GetContent()) + root, err := selectRootCommitment(gcpRec.GetContent()) + if err != nil { + return "", err + } + planTerm, found, err := rootPlanTerm(root) if err != nil { return "", err } if !found { - return termWithoutPlan(gcpRec.GetContent(), params.Term) + return termWithoutPlan(root != nil, params.Term) } if params.Term == "" { return planTerm, nil } - want, err := termPlan(params.Term) - if err != nil { - // Unrecognized explicit term: the caller skips it, as before. - return params.Term, nil - } - got, err := termPlan(planTerm) - if err != nil { - return "", err - } - if want != got { - return "", fmt.Errorf("%w: params term %q conflicts with recommendation plan %s", errBadPlan, params.Term, got) + if want := canonicalTerm(params.Term); want != "1yr" && want != "3yr" { + return want, nil + } else if want != planTerm { + return "", fmt.Errorf("%w: params term %q conflicts with recommendation plan term %s", errBadPlan, params.Term, planTerm) } return planTerm, nil } // termWithoutPlan resolves the term when the payload carries no plan. -func termWithoutPlan(content *recommenderpb.RecommendationContent, paramTerm string) (string, error) { - root, err := selectRootCommitment(content) - if err != nil { - return "", err - } +func termWithoutPlan(hasRoot bool, paramTerm string) (string, error) { if paramTerm != "" { - return paramTerm, nil + return canonicalTerm(paramTerm), nil } - if root != nil { + if hasRoot { return "", fmt.Errorf("%w: root Commitment has no plan and no explicit term was given", errBadPlan) } return "1yr", nil diff --git a/providers/gcp/services/computeengine/plan_test.go b/providers/gcp/services/computeengine/plan_test.go index 20b449fe..90350c3a 100644 --- a/providers/gcp/services/computeengine/plan_test.go +++ b/providers/gcp/services/computeengine/plan_test.go @@ -103,3 +103,19 @@ func TestRootCommitmentTermMapping(t *testing.T) { assert.Equal(t, want, got) } } + +// An explicit non-canonical Term must be canonicalised so pricing and grouping +// see "3yr", not "36mo" (which GetOfferingDetails would price as one year). +func TestExplicitTermIsCanonicalised(t *testing.T) { + for _, raw := range []string{"36mo", " 3 ", "3yr"} { + for _, plan := range []string{"", "THIRTY_SIX_MONTH"} { + client, _ := planClient(t, rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", plan)) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: raw}) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, "3yr", recs[0].Term, "raw %q plan %q", raw, plan) + assert.Equal(t, 3, termYearsFromTerm(recs[0].Term)) + } + } + assert.Equal(t, "3yr", canonicalTerm("36mo")) +} From 01b974dc8d5d9e03fe81215d2cf30b2e18a50d71 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 16:14:53 +0200 Subject: [PATCH 3/7] fix(gcp): use US spelling in plan test comments to satisfy misspell Co-Authored-By: Claude Sonnet 5.5 --- providers/gcp/services/computeengine/plan_test.go | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/providers/gcp/services/computeengine/plan_test.go b/providers/gcp/services/computeengine/plan_test.go index 90350c3a..2ed7b199 100644 --- a/providers/gcp/services/computeengine/plan_test.go +++ b/providers/gcp/services/computeengine/plan_test.go @@ -80,7 +80,7 @@ func TestRecommendationPlanSkips(t *testing.T) { } } -// A conflicting recommendation is skipped without dropping its good neighbour. +// A conflicting recommendation is skipped without dropping its good neighbor. func TestRecommendationPlanConflictKeepsGoodRecommendation(t *testing.T) { bad := rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "TWELVE_MONTH") bad.Name = "bad" @@ -104,9 +104,9 @@ func TestRootCommitmentTermMapping(t *testing.T) { } } -// An explicit non-canonical Term must be canonicalised so pricing and grouping +// An explicit non-canonical Term must be canonicalized so pricing and grouping // see "3yr", not "36mo" (which GetOfferingDetails would price as one year). -func TestExplicitTermIsCanonicalised(t *testing.T) { +func TestExplicitTermIsCanonicalized(t *testing.T) { for _, raw := range []string{"36mo", " 3 ", "3yr"} { for _, plan := range []string{"", "THIRTY_SIX_MONTH"} { client, _ := planClient(t, rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", plan)) From ee7c750092a5b29d7a7be06059577e844134190e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 9 Oct 2026 18:25:21 +0200 Subject: [PATCH 4/7] fix(gcp): use US spelling in plan test comments to satisfy misspell Co-Authored-By: Claude Sonnet 5.5 --- providers/gcp/services/computeengine/plan_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/providers/gcp/services/computeengine/plan_test.go b/providers/gcp/services/computeengine/plan_test.go index 2ed7b199..6f5a60f1 100644 --- a/providers/gcp/services/computeengine/plan_test.go +++ b/providers/gcp/services/computeengine/plan_test.go @@ -24,7 +24,7 @@ func planClient(t *testing.T, recs ...*recommenderpb.Recommendation) (*Client, * } // Issue #291: a THIRTY_SIX_MONTH recommendation with no caller term must be -// bought as THIRTY_SIX_MONTH. Pre-fix it was labelled 1yr and bought TWELVE_MONTH. +// bought as THIRTY_SIX_MONTH. Pre-fix it was labeled 1yr and bought TWELVE_MONTH. func TestRecommendationPlanDrivesPurchaseTerm(t *testing.T) { cases := []struct { name, plan, paramTerm, wantTerm, wantPlan string From 422a545cd2d621318b09e09ba11355174199afce Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 23:48:28 +0200 Subject: [PATCH 5/7] feat(gcp): gate the plan-derived CUD term behind an experimental opt-in The root Commitment plan payload shape is unverified against a live Recommender. Default keeps params.Term / 1yr; ExperimentalGCPPlanTerm enables the plan path, with docs and a log line marking it experimental. Co-Authored-By: Claude Sonnet 5.5 --- known_issues/01_gcp_compute_nonroot_term.md | 4 +++ pkg/common/types.go | 5 +++ .../gcp/services/computeengine/client.go | 3 ++ .../services/computeengine/commitment_type.go | 14 ++++++-- .../gcp/services/computeengine/plan_test.go | 32 ++++++++++++++++--- 5 files changed, 52 insertions(+), 6 deletions(-) diff --git a/known_issues/01_gcp_compute_nonroot_term.md b/known_issues/01_gcp_compute_nonroot_term.md index 5088dd81..3ebb3f8c 100644 --- a/known_issues/01_gcp_compute_nonroot_term.md +++ b/known_issues/01_gcp_compute_nonroot_term.md @@ -15,3 +15,7 @@ to read, so its term still comes from `params.Term`, defaulting to "1yr". unverified, and #291 was scoped to the root path. Tracked in #304. **Status:** Open. + +**Experimental gate:** the plan-derived term path is EXPERIMENTAL and off by +default (`RecommendationParams.ExperimentalGCPPlanTerm`). The root `plan` +payload shape is unverified against a live Recommender. diff --git a/pkg/common/types.go b/pkg/common/types.go index 9cfefe07..7be0d3c3 100644 --- a/pkg/common/types.go +++ b/pkg/common/types.go @@ -529,6 +529,11 @@ type RecommendationParams struct { // recommendations: "linked" or "payer" (case-insensitive). Empty means // linked. Payer scope only applies to a management account. SPAccountScope string + // ExperimentalGCPPlanTerm (GCP Compute Engine only, ignored elsewhere) + // takes the CUD term from the recommendation's root Commitment plan + // instead of Term. EXPERIMENTAL: the payload shape is unverified against a + // live Recommender. Default false keeps Term, defaulting to "1yr". + ExperimentalGCPPlanTerm bool } // Account represents a cloud account/subscription/project. diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index d3f61062..6525feea 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -1205,6 +1205,9 @@ func (c *Client) convertGCPRecommendation(ctx context.Context, gcpRec *recommend if err != nil { return nil, err } + if params.ExperimentalGCPPlanTerm { + log.Printf("computeengine: EXPERIMENTAL plan-term path: term %q for %q; root plan payload shape is unverified", term, gcpRec.GetName()) + } if _, err := termPlan(term); err != nil { log.Printf("computeengine: skipping recommendation with unrecognized term %q: %v", term, err) return nil, nil diff --git a/providers/gcp/services/computeengine/commitment_type.go b/providers/gcp/services/computeengine/commitment_type.go index 6a8ea972..fae983d2 100644 --- a/providers/gcp/services/computeengine/commitment_type.go +++ b/providers/gcp/services/computeengine/commitment_type.go @@ -159,12 +159,12 @@ func populateRecommendationResources(gcpRec *recommenderpb.Recommendation, rec * return extractMemoryMBFromRecommendation(gcpRec, rec) } -// errBadPlan marks a recommendation whose commitment plan is malformed, +// errBadPlan (EXPERIMENTAL plan-term path, see resolveRecTerm) marks a recommendation whose commitment plan is malformed, // missing where required, or in conflict with the caller's term. The // recommendation is skipped rather than purchased with a guessed term. var errBadPlan = errors.New("bad recommendation plan") -// rootCommitmentTerm reads the root Commitment's plan and returns the +// rootCommitmentTerm (EXPERIMENTAL, unverified payload shape) reads the root Commitment's plan and returns the // canonical term ("1yr" or "3yr"). found is false when the root has no plan // key. A present but malformed plan returns an error wrapping errBadPlan. func rootCommitmentTerm(content *recommenderpb.RecommendationContent) (term string, found bool, err error) { @@ -219,7 +219,17 @@ func canonicalTerm(term string) string { // with it. A root Commitment without a plan needs an explicit params.Term. A // payload with no root Commitment keeps the params.Term / "1yr" fallback // (issue #291 follow-up). +// +// EXPERIMENTAL: the plan path runs only when params.ExperimentalGCPPlanTerm is +// set. The root-plan payload shape is unverified against a live Recommender. +// When unset, the pre-#291 behaviour applies: params.Term, default "1yr". func resolveRecTerm(gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) (string, error) { + if !params.ExperimentalGCPPlanTerm { + if params.Term == "" { + return "1yr", nil + } + return params.Term, nil + } root, err := selectRootCommitment(gcpRec.GetContent()) if err != nil { return "", err diff --git a/providers/gcp/services/computeengine/plan_test.go b/providers/gcp/services/computeengine/plan_test.go index 6f5a60f1..bfbf94cc 100644 --- a/providers/gcp/services/computeengine/plan_test.go +++ b/providers/gcp/services/computeengine/plan_test.go @@ -39,7 +39,7 @@ func TestRecommendationPlanDrivesPurchaseTerm(t *testing.T) { for _, tc := range cases { t.Run(tc.name, func(t *testing.T) { client, service := planClient(t, rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", tc.plan)) - recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: tc.paramTerm}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: tc.paramTerm, ExperimentalGCPPlanTerm: true}) require.NoError(t, err) require.Len(t, recs, 1) assert.Equal(t, tc.wantTerm, recs[0].Term) @@ -72,7 +72,7 @@ func TestRecommendationPlanSkips(t *testing.T) { for name, tc := range cases { t.Run(name, func(t *testing.T) { client, service := planClient(t, tc.rec) - recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: tc.term}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: tc.term, ExperimentalGCPPlanTerm: true}) require.NoError(t, err) assert.Empty(t, recs) assert.Empty(t, service.insertReqs) @@ -87,7 +87,7 @@ func TestRecommendationPlanConflictKeepsGoodRecommendation(t *testing.T) { good := rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", "THIRTY_SIX_MONTH") good.Name = "good" client, service := planClient(t, bad, good) - recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: "3yr"}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: "3yr", ExperimentalGCPPlanTerm: true}) require.NoError(t, err) require.Len(t, recs, 1) assert.Equal(t, "3yr", recs[0].Term) @@ -110,7 +110,7 @@ func TestExplicitTermIsCanonicalized(t *testing.T) { for _, raw := range []string{"36mo", " 3 ", "3yr"} { for _, plan := range []string{"", "THIRTY_SIX_MONTH"} { client, _ := planClient(t, rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", plan)) - recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: raw}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: raw, ExperimentalGCPPlanTerm: true}) require.NoError(t, err) require.Len(t, recs, 1) assert.Equal(t, "3yr", recs[0].Term, "raw %q plan %q", raw, plan) @@ -119,3 +119,27 @@ func TestExplicitTermIsCanonicalized(t *testing.T) { } assert.Equal(t, "3yr", canonicalTerm("36mo")) } + +// Default (flag off) keeps the pre-#291 behaviour: Term or "1yr", plan ignored, +// nothing skipped. +func TestPlanTermOffByDefault(t *testing.T) { + cases := []struct { + name, plan, paramTerm, wantTerm, wantPlan string + }{ + {"36mo plan, empty term", "THIRTY_SIX_MONTH", "", "1yr", "TWELVE_MONTH"}, + {"12mo plan, 3yr term (conflict kept)", "TWELVE_MONTH", "3yr", "3yr", "THIRTY_SIX_MONTH"}, + {"missing plan, no term", "", "", "1yr", "TWELVE_MONTH"}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + client, service := planClient(t, rootCUDRecommendationWithPlan("MEMORY_OPTIMIZED_M4_6TB", tc.plan)) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{Term: tc.paramTerm}) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, tc.wantTerm, recs[0].Term) + _, err = client.PurchaseCommitment(context.Background(), recs[0], common.PurchaseOptions{}) + require.NoError(t, err) + assert.Equal(t, tc.wantPlan, service.lastInsertReq.CommitmentResource.GetPlan()) + }) + } +} From 4c0b5fd6e9cffc803e4e454fb75a1124b6ed0b4a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 23:55:34 +0200 Subject: [PATCH 6/7] fix(gcp): use US spelling in comments for misspell lint Co-Authored-By: Claude Sonnet 5.5 --- providers/gcp/services/computeengine/commitment_type.go | 2 +- providers/gcp/services/computeengine/plan_test.go | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/providers/gcp/services/computeengine/commitment_type.go b/providers/gcp/services/computeengine/commitment_type.go index fae983d2..a811f992 100644 --- a/providers/gcp/services/computeengine/commitment_type.go +++ b/providers/gcp/services/computeengine/commitment_type.go @@ -222,7 +222,7 @@ func canonicalTerm(term string) string { // // EXPERIMENTAL: the plan path runs only when params.ExperimentalGCPPlanTerm is // set. The root-plan payload shape is unverified against a live Recommender. -// When unset, the pre-#291 behaviour applies: params.Term, default "1yr". +// When unset, the pre-#291 behavior applies: params.Term, default "1yr". func resolveRecTerm(gcpRec *recommenderpb.Recommendation, params common.RecommendationParams) (string, error) { if !params.ExperimentalGCPPlanTerm { if params.Term == "" { diff --git a/providers/gcp/services/computeengine/plan_test.go b/providers/gcp/services/computeengine/plan_test.go index bfbf94cc..e685b6e3 100644 --- a/providers/gcp/services/computeengine/plan_test.go +++ b/providers/gcp/services/computeengine/plan_test.go @@ -120,7 +120,7 @@ func TestExplicitTermIsCanonicalized(t *testing.T) { assert.Equal(t, "3yr", canonicalTerm("36mo")) } -// Default (flag off) keeps the pre-#291 behaviour: Term or "1yr", plan ignored, +// Default (flag off) keeps the pre-#291 behavior: Term or "1yr", plan ignored, // nothing skipped. func TestPlanTermOffByDefault(t *testing.T) { cases := []struct { From 32289566fe6a3974b544ed0f73342f13381cf177 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 14 Oct 2026 00:13:42 +0200 Subject: [PATCH 7/7] chore(deps): bump pkg pin in providers/gcp for ExperimentalGCPPlanTerm Co-Authored-By: Claude Sonnet 5.5 --- providers/gcp/go.mod | 2 +- providers/gcp/go.sum | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/providers/gcp/go.mod b/providers/gcp/go.mod index 1cdb1020..b09ba6d6 100644 --- a/providers/gcp/go.mod +++ b/providers/gcp/go.mod @@ -8,7 +8,7 @@ require ( cloud.google.com/go/redis v1.18.3 cloud.google.com/go/resourcemanager v1.10.7 cloud.google.com/go/storage v1.59.2 - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261012192523-3838efd56668 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261013221254-77bc8a993eb8 github.com/googleapis/gax-go/v2 v2.21.0 github.com/stretchr/testify v1.11.1 golang.org/x/oauth2 v0.36.0 diff --git a/providers/gcp/go.sum b/providers/gcp/go.sum index 32e56b49..9ba08105 100644 --- a/providers/gcp/go.sum +++ b/providers/gcp/go.sum @@ -36,8 +36,8 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.54.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0 h1:s0WlVbf9qpvkh1c/uDAPElam0WrL7fHRIidgZJ7UqZI= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261012192523-3838efd56668 h1:/m3hxkSHGd8AJlhBaPTttRDwtwC1ycYH7IbRkRKR9bM= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261012192523-3838efd56668/go.mod h1:8RTmzPazA8IeVZfn7wNaU7ILx0VK5eHZQozO21Mvtq8= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261013221254-77bc8a993eb8 h1:0EMP5PHw8RnQ5Xl8B2firZ8CwRSpmxitMZ4cdYuL8p4= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261013221254-77bc8a993eb8/go.mod h1:8RTmzPazA8IeVZfn7wNaU7ILx0VK5eHZQozO21Mvtq8= github.com/aws/aws-sdk-go-v2 v1.41.5 h1:dj5kopbwUsVUVFgO4Fi5BIT3t4WyqIDjGKCangnV/yY= github.com/aws/aws-sdk-go-v2 v1.41.5/go.mod h1:mwsPRE8ceUUpiTgF7QmQIJ7lgsKUPQOUl3o72QBrE1o= github.com/aws/smithy-go v1.24.2 h1:FzA3bu/nt/vDvmnkg+R8Xl46gmzEDam6mZ1hzmwXFng=