diff --git a/CHANGELOG.md b/CHANGELOG.md index bde5b9b6..50172fa8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -140,6 +140,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Changed +- **GCP: one strict commitment term parser** (#15). Compute Engine now uses + `skumatch.TermYears` like Cloud SQL, Memorystore and Cloud Storage; it accepts + `1yr`/`1`/`12mo`/`3yr`/`3`/`36mo` case-insensitively. An unrecognized term + now errors in `GetOfferingDetails` and skips pricing in `enrichRecWithPricing` + instead of being priced as one year. - **`aws/recommendations`:** the four Cost Explorer coverage and utilization page fetchers share one generic `fetchWithRetry` helper (rate limiter plus concurrency cap); behaviour and error messages are unchanged (#120). - **`aws/rds`:** the RDS purchase path uses the shared `purchaseguard` idempotency guard and AlreadyExists recovery, like ElastiCache. Behavior for valid purchases is unchanged (#10). - **`aws/memorydb`:** the MemoryDB purchase path uses the shared `purchaseguard` idempotency guard and AlreadyExists recovery, like ElastiCache. Behavior for valid purchases is unchanged; only the lookup-failure error wording changes to "before MemoryDB reservation purchase" (#10). diff --git a/providers/gcp/internal/skumatch/term.go b/providers/gcp/internal/skumatch/term.go index 81737e5e..e3c4b202 100644 --- a/providers/gcp/internal/skumatch/term.go +++ b/providers/gcp/internal/skumatch/term.go @@ -1,16 +1,20 @@ package skumatch -import "fmt" +import ( + "fmt" + "strings" +) -// TermYears parses a recommendation term label into commitment years. It -// accepts "1yr", "1" and "" (an unset term has always meant one year) for one -// year and "3yr", "3" for three. Anything else is an error: guessing a term +// TermYears parses a recommendation term label into commitment years. It is +// case-insensitive, ignores surrounding whitespace, and accepts "1yr", "1", +// "12mo" and "" (an unset term has always meant one year) for one year and +// "3yr", "3", "36mo" for three. Anything else is an error: guessing a term // would price the wrong commitment. func TermYears(term string) (int, error) { - switch term { - case "", "1yr", "1": + switch strings.ToLower(strings.TrimSpace(term)) { + case "", "1yr", "1", "12mo": return 1, nil - case "3yr", "3": + case "3yr", "3", "36mo": return 3, nil } return 0, fmt.Errorf("unsupported commitment term %q, want 1yr or 3yr", term) diff --git a/providers/gcp/internal/skumatch/term_test.go b/providers/gcp/internal/skumatch/term_test.go index 19827451..2426d248 100644 --- a/providers/gcp/internal/skumatch/term_test.go +++ b/providers/gcp/internal/skumatch/term_test.go @@ -8,12 +8,16 @@ import ( ) func TestTermYears(t *testing.T) { - for term, want := range map[string]int{"": 1, "1yr": 1, "1": 1, "3yr": 3, "3": 3} { + accepted := map[string]int{ + "": 1, "1yr": 1, "1": 1, "12mo": 1, "1YR": 1, "1Yr": 1, " 1yr ": 1, "12MO": 1, " 12mo": 1, + "3yr": 3, "3": 3, "36mo": 3, "3YR": 3, " 3yr": 3, "36MO": 3, " 36mo ": 3, " 3 ": 3, + } + for term, want := range accepted { got, err := TermYears(term) require.NoError(t, err, term) assert.Equal(t, want, got, term) } - for _, term := range []string{"2yr", "2", "5yr", "3yrs", "1Yr", " 3yr", "monthly"} { + for _, term := range []string{"2yr", "2", "5yr", "3yrs", "24mo", "P1Y", "abc", "monthly"} { _, err := TermYears(term) require.Error(t, err, term) assert.Contains(t, err.Error(), "unsupported commitment term") diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index c214b7c7..b23c0964 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -31,6 +31,7 @@ import ( "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/LeanerCloud/cloud-commitments-go/pkg/retry" "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skulist" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) // maxRecsPages caps GCP Recommender API iteration to avoid burning a Lambda @@ -44,23 +45,21 @@ const maxCommitmentsPages = 50 const maxMachineTypeItems = 20 // termPlan converts a commitment term string to the canonical GCP Compute API -// commitment plan value derived from the SDK enum constants. -// -// Accepted forms: -// - 1-year: "1yr", "1", "12mo" -// - 3-year: "3yr", "3", "36mo" -// -// An empty or unrecognized term returns an error rather than silently defaulting -// to 12 months; a silent mis-default can purchase the wrong term and waste money. +// commitment plan value derived from the SDK enum constants. Term parsing is +// delegated to skumatch.TermYears; unlike it, an empty term is an error here +// because a purchase must name its plan explicitly. func termPlan(term string) (string, error) { - switch strings.ToLower(strings.TrimSpace(term)) { - case "3yr", "3", "36mo": + if strings.TrimSpace(term) == "" { + return "", fmt.Errorf("termPlan: unrecognized commitment term %q (empty)", term) + } + years, err := skumatch.TermYears(term) + if err != nil { + return "", fmt.Errorf("termPlan: unrecognized commitment term %q: %w", term, err) + } + if years == 3 { return computepb.Commitment_THIRTY_SIX_MONTH.String(), nil - case "1yr", "1", "12mo": - return computepb.Commitment_TWELVE_MONTH.String(), nil - default: - return "", fmt.Errorf("termPlan: unrecognized commitment term %q (accepted: 1yr/1/12mo or 3yr/3/36mo)", term) } + return computepb.Commitment_TWELVE_MONTH.String(), nil } // machineFamilyCommitmentType maps a GCP machine series (the segment of a @@ -202,17 +201,6 @@ func commitmentTypeForMachineType(machineType string) (computepb.Commitment_Type return commitType, nil } -// termYearsFromTerm returns the commitment length in years for a term label, -// defaulting to 1 for any 1-year or unrecognized value and 3 for 3-year labels. -func termYearsFromTerm(term string) int { - switch strings.ToLower(strings.TrimSpace(term)) { - case "3yr", "3", "36mo": - return 3 - default: - return 1 - } -} - // CommitmentsService interface for commitments operations (enables mocking). type CommitmentsService interface { List(ctx context.Context, req *computepb.ListRegionCommitmentsRequest) CommitmentsIterator @@ -897,9 +885,9 @@ func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation // GetOfferingDetails retrieves CUD offering details from GCP Billing API. func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { - termYears := 1 - if rec.Term == "3yr" || rec.Term == "3" { - termYears = 3 + termYears, err := skumatch.TermYears(rec.Term) + if err != nil { + return nil, fmt.Errorf("failed to get pricing: %w", err) } res, err := resourcesFromRecommendation(rec) @@ -1079,7 +1067,11 @@ func (c *Client) enrichRecWithPricing(ctx context.Context, rec *common.Recommend if rec.ResourceType == "" { return } - termYears := termYearsFromTerm(rec.Term) + termYears, err := skumatch.TermYears(rec.Term) + if err != nil { + log.Printf("computeengine: pricing unavailable for %s in %s: %v", rec.ResourceType, c.region, err) + return + } res, err := resourcesFromRecommendation(*rec) if err != nil { log.Printf("computeengine: pricing unavailable for %s in %s: %v", rec.ResourceType, c.region, err) diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 1da26b6b..b14f220a 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -1035,6 +1035,38 @@ func TestComputeEngineClient_GetOfferingDetails_WithMock(t *testing.T) { assert.InDelta(t, details.TotalCost/12, details.RecurringCost, 1e-6) } +func TestComputeEngineClient_GetOfferingDetails_Terms(t *testing.T) { + t.Parallel() + ctx := context.Background() + client, _ := NewClient("test-project", "us-central1") + client.SetBillingService(mockBillingWithCommitment()) + rec := common.Recommendation{ResourceType: "n1-standard-4", Count: 4, Details: common.ComputeDetails{MemoryGB: 15}} + + rec.Term = "1yr" + one, err := client.GetOfferingDetails(ctx, rec) + require.NoError(t, err) + for _, term := range []string{"12mo", "12MO", "1"} { + rec.Term = term + got, err := client.GetOfferingDetails(ctx, rec) + require.NoError(t, err, term) + assert.InDelta(t, one.TotalCost, got.TotalCost, 1e-6, term) + } + // The fixture has no 3-year SKUs, so a recognized 3-year label fails on + // missing pricing, not on the term (3-year pricing is covered by the enrich test). + for _, term := range []string{"36mo", "36MO", " 3 ", "3yr"} { + rec.Term = term + _, err := client.GetOfferingDetails(ctx, rec) + require.Error(t, err, term) + assert.NotContains(t, err.Error(), "unsupported commitment term", term) + } + for _, term := range []string{"2yr", "24mo", "P1Y"} { + rec.Term = term + _, err := client.GetOfferingDetails(ctx, rec) + require.Error(t, err, term) + assert.Contains(t, err.Error(), "unsupported commitment term", term) + } +} + func TestComputeEngineClient_GetOfferingDetails_NoPricing(t *testing.T) { t.Parallel() ctx := context.Background() diff --git a/providers/gcp/services/computeengine/plan_test.go b/providers/gcp/services/computeengine/plan_test.go index 1b641f22..f488b4b4 100644 --- a/providers/gcp/services/computeengine/plan_test.go +++ b/providers/gcp/services/computeengine/plan_test.go @@ -10,6 +10,7 @@ import ( "google.golang.org/protobuf/types/known/structpb" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/internal/skumatch" ) func planClient(t *testing.T, recs ...*recommenderpb.Recommendation) (*Client, *MockCommitmentsService) { @@ -142,7 +143,9 @@ func TestExplicitTermIsCanonicalized(t *testing.T) { 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)) + years, yErr := skumatch.TermYears(recs[0].Term) + require.NoError(t, yErr) + assert.Equal(t, 3, years) } } assert.Equal(t, "3yr", canonicalTerm("36mo")) diff --git a/providers/gcp/services/computeengine/pricing_test.go b/providers/gcp/services/computeengine/pricing_test.go index 0bbd1a74..e638d85f 100644 --- a/providers/gcp/services/computeengine/pricing_test.go +++ b/providers/gcp/services/computeengine/pricing_test.go @@ -168,6 +168,33 @@ func TestEnrichRecWithPricing_MonthlyFiguresPerTerm(t *testing.T) { } } +func TestEnrichRecWithPricing_TermSpellingsAndUnknown(t *testing.T) { + t.Parallel() + price := func(term string) *float64 { + client, _ := NewClient("p", "us-central1") + client.SetBillingService(&MockBillingService{skus: &cloudbilling.ListSkusResponse{Skus: n2AmericasSKUs()}}) + rec := newRec("n2-standard-8", 8, 32) + rec.Term = term + client.enrichRecWithPricing(context.Background(), &rec) + return rec.RecurringMonthlyCost + } + one, three := price("1yr"), price("3yr") + require.NotNil(t, one) + require.NotNil(t, three) + for _, term := range []string{"12mo", "12MO", " 1 "} { + got := price(term) + require.NotNil(t, got, term) + assert.InDelta(t, *one, *got, 1e-9, term) + } + for _, term := range []string{"36mo", "36MO", " 3 "} { + got := price(term) + require.NotNil(t, got, term) + assert.InDelta(t, *three, *got, 1e-9, term) + } + assert.Nil(t, price("2yr"), "unknown term must not be priced as 1yr") + assert.Nil(t, price("24mo"), "unknown term must not be priced as 1yr") +} + func TestEnrichRecWithPricing_InvalidCurrency(t *testing.T) { t.Parallel() for _, currency := range []string{"", "EUR"} {