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 0000000..3ebb3f8 --- /dev/null +++ b/known_issues/01_gcp_compute_nonroot_term.md @@ -0,0 +1,21 @@ +# 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. + +**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/providers/gcp/go.mod b/providers/gcp/go.mod index 1cdb102..b09ba6d 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 32e56b4..9ba0810 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= diff --git a/providers/gcp/recommendations_sdk_test.go b/providers/gcp/recommendations_sdk_test.go index da524fe..f7af06b 100644 --- a/providers/gcp/recommendations_sdk_test.go +++ b/providers/gcp/recommendations_sdk_test.go @@ -248,7 +248,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/client.go b/providers/gcp/services/computeengine/client.go index 3373add..4e029b1 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -482,7 +482,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 } @@ -1082,8 +1082,11 @@ func (c *Client) getOrCreateBillingService(ctx context.Context) (BillingService, // 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 @@ -1095,11 +1098,15 @@ 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 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) diff --git a/providers/gcp/services/computeengine/commitment_type.go b/providers/gcp/services/computeengine/commitment_type.go index 9ed0d51..a811f99 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,107 @@ func populateRecommendationResources(gcpRec *recommenderpb.Recommendation, rec * rec.Count = count return extractMemoryMBFromRecommendation(gcpRec, rec) } + +// 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 (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) { + root, err := selectRootCommitment(content) + 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") + } + 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) + } +} + +// 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). +// +// 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 behavior 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 + } + planTerm, found, err := rootPlanTerm(root) + if err != nil { + return "", err + } + if !found { + return termWithoutPlan(root != nil, params.Term) + } + if params.Term == "" { + return planTerm, nil + } + 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(hasRoot bool, paramTerm string) (string, error) { + if paramTerm != "" { + return canonicalTerm(paramTerm), 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/dedupe_test.go b/providers/gcp/services/computeengine/dedupe_test.go index e41653a..9074ac4 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 0000000..e685b6e --- /dev/null +++ b/providers/gcp/services/computeengine/plan_test.go @@ -0,0 +1,145 @@ +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 labeled 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, ExperimentalGCPPlanTerm: true}) + 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, ExperimentalGCPPlanTerm: true}) + require.NoError(t, err) + assert.Empty(t, recs) + assert.Empty(t, service.insertReqs) + }) + } +} + +// 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" + 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", ExperimentalGCPPlanTerm: true}) + 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) + } +} + +// An explicit non-canonical Term must be canonicalized so pricing and grouping +// see "3yr", not "36mo" (which GetOfferingDetails would price as one year). +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, ExperimentalGCPPlanTerm: true}) + 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")) +} + +// Default (flag off) keeps the pre-#291 behavior: 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()) + }) + } +}