From c0d1a1f77455b694f1e57f4f93c8b1a0bd01687c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 19:45:10 +0200 Subject: [PATCH 1/7] fix(config): accept provider-canonical payment options in plan validator ServiceConfig.validatePayment was using the AWS-only ValidPaymentOptions slice regardless of provider, rejecting Azure ("upfront", "monthly") and GCP ("upfront", "monthly") payment tokens that their recommendation emitters stamp after PR #682. Add ValidPaymentOptionsByProvider (per-provider canonical sets), validPaymentOptionsFor lookup, and update ServiceConfig.validatePayment to route through the provider's own set with a clear error message. A cross-provider token (e.g. "partial-upfront" on Azure) is rejected even though it is valid somewhere else. GlobalConfig.DefaultPayment validation switches to the union of all provider sets so a global default can hold any provider-canonical token. Closes #698 --- internal/config/validation.go | 65 ++++++++++++++++-- internal/config/validation_test.go | 107 +++++++++++++++++++++++++++++ 2 files changed, 165 insertions(+), 7 deletions(-) diff --git a/internal/config/validation.go b/internal/config/validation.go index 42a858756..610406bb8 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -10,9 +10,45 @@ import ( // ValidProviders lists all supported cloud providers var ValidProviders = []string{"aws", "azure", "gcp"} -// ValidPaymentOptions lists all supported payment options +// ValidPaymentOptions lists the AWS-canonical payment options. Kept for +// backwards compatibility; prefer ValidPaymentOptionsByProvider for +// provider-aware validation. var ValidPaymentOptions = []string{"no-upfront", "partial-upfront", "all-upfront"} +// ValidPaymentOptionsByProvider maps each provider to the payment option +// tokens it accepts. Azure and GCP reservations use "upfront" (all-upfront +// billing) and "monthly" (no-upfront / spread billing). AWS RIs/SPs use the +// three classic tiers. See providers/{azure,gcp}/services/*/client.go for +// the case statements that consume these values. +var ValidPaymentOptionsByProvider = map[string][]string{ + "aws": {"no-upfront", "partial-upfront", "all-upfront"}, + "azure": {"all-upfront", "no-upfront", "upfront", "monthly"}, + "gcp": {"all-upfront", "no-upfront", "upfront", "monthly"}, +} + +// validPaymentOptionsUnion is the union of all provider payment option sets, +// used for global-config default validation where no provider context is +// available. Accepts any token that is valid for at least one provider. +var validPaymentOptionsUnion = func() []string { + seen := map[string]bool{} + var all []string + for _, opts := range ValidPaymentOptionsByProvider { + for _, o := range opts { + if !seen[o] { + seen[o] = true + all = append(all, o) + } + } + } + return all +}() + +// validPaymentOptionsFor returns the provider-canonical payment option slice +// for the given provider (lowercase). Returns nil when the provider is unknown. +func validPaymentOptionsFor(provider string) []string { + return ValidPaymentOptionsByProvider[provider] +} + // ValidRampScheduleTypes lists all supported ramp schedule types var ValidRampScheduleTypes = []string{"immediate", "weekly", "monthly", "custom"} @@ -142,10 +178,12 @@ func validateGlobalTerm(term int) error { return nil } -// validatePaymentOption validates that the payment option is valid if set +// validatePaymentOption validates that the payment option is in the union of +// all provider sets. Used by GlobalConfig.Validate where no provider context +// is available; any token valid for at least one provider is accepted. func validatePaymentOption(payment string) error { if payment != "" && !isValidPaymentOption(payment) { - return fmt.Errorf("invalid payment option: %s (valid: %s)", payment, strings.Join(ValidPaymentOptions, ", ")) + return fmt.Errorf("invalid payment option: %s (valid: %s)", payment, strings.Join(validPaymentOptionsUnion, ", ")) } return nil } @@ -200,10 +238,23 @@ func (c *ServiceConfig) validateTerm() error { } func (c *ServiceConfig) validatePayment() error { - if c.Payment != "" && !isValidPaymentOption(c.Payment) { - return fmt.Errorf("invalid payment option: %s (valid: %s)", c.Payment, strings.Join(ValidPaymentOptions, ", ")) + if c.Payment == "" { + return nil } - return nil + // Provider-canonical validation: each provider accepts only its own token + // set. A cross-provider token (e.g. "all-upfront" on an Azure service) is + // rejected even though that token is valid somewhere else. + opts := validPaymentOptionsFor(c.Provider) + if opts == nil { + // Provider was already validated; unknown provider here is a bug. + return fmt.Errorf("internal error: no payment options defined for provider %q", c.Provider) + } + for _, v := range opts { + if c.Payment == v { + return nil + } + } + return fmt.Errorf("invalid payment option: %s (valid for %s: %s)", c.Payment, c.Provider, strings.Join(opts, ", ")) } func (c *ServiceConfig) validateConfigCoverage() error { @@ -297,7 +348,7 @@ func isValidProvider(p string) bool { } func isValidPaymentOption(p string) bool { - for _, valid := range ValidPaymentOptions { + for _, valid := range validPaymentOptionsUnion { if p == valid { return true } diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index f3ab646ce..a52a2b053 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -72,6 +72,23 @@ func TestGlobalConfig_Validate(t *testing.T) { wantErr: true, errMsg: "invalid payment option", }, + // Issue #698: GlobalConfig accepts union of all provider payment tokens + { + name: "global config accepts azure/gcp upfront token", + config: GlobalConfig{ + DefaultTerm: 3, + DefaultPayment: "upfront", + }, + wantErr: false, + }, + { + name: "global config accepts azure/gcp monthly token", + config: GlobalConfig{ + DefaultTerm: 3, + DefaultPayment: "monthly", + }, + wantErr: false, + }, { name: "coverage too low", config: GlobalConfig{ @@ -334,6 +351,91 @@ func TestServiceConfig_Validate(t *testing.T) { wantErr: true, errMsg: "invalid payment option", }, + // Issue #698: provider-canonical payment validation + { + name: "aws all-upfront is valid", + config: ServiceConfig{ + Provider: "aws", + Service: "ec2", + Payment: "all-upfront", + }, + wantErr: false, + }, + { + name: "aws monthly is rejected", + config: ServiceConfig{ + Provider: "aws", + Service: "ec2", + Payment: "monthly", + }, + wantErr: true, + errMsg: "invalid payment option", + }, + { + name: "azure upfront is valid", + config: ServiceConfig{ + Provider: "azure", + Service: "vm", + Payment: "upfront", + }, + wantErr: false, + }, + { + name: "azure monthly is valid", + config: ServiceConfig{ + Provider: "azure", + Service: "vm", + Payment: "monthly", + }, + wantErr: false, + }, + { + name: "azure all-upfront is valid", + config: ServiceConfig{ + Provider: "azure", + Service: "vm", + Payment: "all-upfront", + }, + wantErr: false, + }, + { + name: "azure partial-upfront is rejected (aws-only token)", + config: ServiceConfig{ + Provider: "azure", + Service: "vm", + Payment: "partial-upfront", + }, + wantErr: true, + errMsg: "invalid payment option", + }, + { + name: "gcp upfront is valid", + config: ServiceConfig{ + Provider: "gcp", + Service: "computeengine", + Payment: "upfront", + }, + wantErr: false, + }, + { + name: "gcp monthly is valid", + config: ServiceConfig{ + Provider: "gcp", + Service: "computeengine", + Payment: "monthly", + }, + wantErr: false, + }, + { + name: "gcp partial-upfront is rejected (aws-only token)", + config: ServiceConfig{ + Provider: "gcp", + Service: "computeengine", + Payment: "partial-upfront", + }, + wantErr: true, + errMsg: "invalid payment option", + }, { name: "coverage too low", config: ServiceConfig{ @@ -689,9 +791,14 @@ func TestIsValidProvider(t *testing.T) { } func TestIsValidPaymentOption(t *testing.T) { + // AWS tokens assert.True(t, isValidPaymentOption("no-upfront")) assert.True(t, isValidPaymentOption("partial-upfront")) assert.True(t, isValidPaymentOption("all-upfront")) + // Azure/GCP tokens (union set) + assert.True(t, isValidPaymentOption("upfront")) + assert.True(t, isValidPaymentOption("monthly")) + // Unknown tokens rejected assert.False(t, isValidPaymentOption("invalid")) assert.False(t, isValidPaymentOption("")) } From 5ffc7a3726d77a9371fa2c22904afd6772810192 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 22:21:27 +0200 Subject: [PATCH 2/7] docs(config): add Go docs for new payment-validator helpers Add missing docstring for isValidPaymentOption to complete docstring coverage for the new payment-option validation helpers introduced in PR #698. All new package-level helpers now have proper Go-style doc comments (ValidPaymentOptionsByProvider, validPaymentOptionsUnion, validPaymentOptionsFor, isValidPaymentOption). Resolves CodeRabbit docstring coverage pre-merge check (44.44% -> 100%). --- internal/config/validation.go | 2 ++ 1 file changed, 2 insertions(+) diff --git a/internal/config/validation.go b/internal/config/validation.go index 610406bb8..51e2de1d3 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -347,6 +347,8 @@ func isValidProvider(p string) bool { return false } +// isValidPaymentOption reports whether p is a valid payment option for any +// provider. It checks against the union of all provider payment option sets. func isValidPaymentOption(p string) bool { for _, valid := range validPaymentOptionsUnion { if p == valid { From e92adb09e8726b1a5e90bff8b6f20ad47dd5f567 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 23:03:09 +0200 Subject: [PATCH 3/7] fix(config): tighten provider-canonical payment-option sets to verified provider semantics PR #709 originally accepted both AWS-style tokens (all-upfront, no-upfront) and the provider-canonical (upfront, monthly) for Azure and GCP, because the per-service switches at providers/{azure,gcp}/services/*/client.go alias the two pairs. That over-acceptance hid input bugs: a plan that mistakenly stamped an AWS-style token on a non-AWS service would validate successfully and only surface the mismatch later in pricing math. Tighten the validator to the canonical token each provider semantically models, verified by reading every per-service purchase/pricing switch: - AWS : {no-upfront, partial-upfront, all-upfront} (unchanged) - Azure: {upfront, monthly} - GCP : {upfront, monthly} The validator now rejects cross-provider tokens with the existing "invalid payment option: (valid for : )" error, giving callers an immediately actionable list of accepted values. The Azure savingsplans switch at services/savingsplans/client.go:418-429 DOES mirror AWS's three-tier set, but Azure savings-plan recommendations are not currently emitted (GetRecommendations returns []) - the canonical set follows the only path that emits today and can be expanded later when SP recs land. Tests updated to assert the new rejections and that the error message carries the canonical set. The GlobalConfig union test still passes - the union of {no-upfront,partial-upfront,all-upfront,upfront,monthly} is unchanged. --- internal/config/validation.go | 42 +++++++++++++++++++---- internal/config/validation_test.go | 55 ++++++++++++++++++++++++++++-- 2 files changed, 89 insertions(+), 8 deletions(-) diff --git a/internal/config/validation.go b/internal/config/validation.go index 51e2de1d3..07ee5cc7a 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -16,14 +16,44 @@ var ValidProviders = []string{"aws", "azure", "gcp"} var ValidPaymentOptions = []string{"no-upfront", "partial-upfront", "all-upfront"} // ValidPaymentOptionsByProvider maps each provider to the payment option -// tokens it accepts. Azure and GCP reservations use "upfront" (all-upfront -// billing) and "monthly" (no-upfront / spread billing). AWS RIs/SPs use the -// three classic tiers. See providers/{azure,gcp}/services/*/client.go for -// the case statements that consume these values. +// tokens it accepts. The sets are tightened to the canonical tokens each +// provider actually models semantically; AWS-style aliases the service +// clients also accept (for legacy frontend compat) are deliberately not +// surfaced here — config-layer validation rejects them so input bugs aren't +// hidden behind silent alias coercion in the service-client switches. +// Callers that need to translate legacy/cross-provider tokens into the +// canonical form should use NormalizePaymentOption at the emission boundary +// (see internal/scheduler/scheduler.go:convertRecommendations). +// +// Canonical sets, verified against the per-service purchase/pricing switches: +// +// - AWS : {no-upfront, partial-upfront, all-upfront} +// Three distinct billing tiers exposed by RI/SP offering APIs. +// +// - Azure: {upfront, monthly} +// Reservation purchases only model two billing plans. See: +// providers/azure/services/compute/client.go:493-497 +// providers/azure/services/cache/client.go:375-379 +// providers/azure/services/cosmosdb/client.go:368-372 +// providers/azure/services/database/client.go:376-380 +// providers/azure/services/search/client.go:349-353 +// providers/azure/services/synapse/client.go:354-358 +// providers/azure/services/managedredis/client.go:345-349 +// (The savingsplans switch at providers/azure/services/savingsplans/ +// client.go:418-429 mirrors AWS's three-tier set, but Azure savings-plan +// recommendations are not currently emitted — GetRecommendations returns +// []; the canonical set follows the only path that emits today.) +// +// - GCP : {upfront, monthly} +// CUD purchase model has only one-time upfront vs monthly recurring. See: +// providers/gcp/services/computeengine/client.go:587-591 +// providers/gcp/services/cloudsql/client.go:288-292 +// providers/gcp/services/cloudstorage/client.go:297-301 +// providers/gcp/services/memorystore/client.go:245-249 var ValidPaymentOptionsByProvider = map[string][]string{ "aws": {"no-upfront", "partial-upfront", "all-upfront"}, - "azure": {"all-upfront", "no-upfront", "upfront", "monthly"}, - "gcp": {"all-upfront", "no-upfront", "upfront", "monthly"}, + "azure": {"upfront", "monthly"}, + "gcp": {"upfront", "monthly"}, } // validPaymentOptionsUnion is the union of all provider payment option sets, diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index a52a2b053..36b558371 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -390,13 +390,24 @@ func TestServiceConfig_Validate(t *testing.T) { wantErr: false, }, { - name: "azure all-upfront is valid", + name: "azure all-upfront is rejected (aws-only token)", config: ServiceConfig{ Provider: "azure", Service: "vm", Payment: "all-upfront", }, - wantErr: false, + wantErr: true, + errMsg: "invalid payment option", + }, + { + name: "azure no-upfront is rejected (aws-only token)", + config: ServiceConfig{ + Provider: "azure", + Service: "vm", + Payment: "no-upfront", + }, + wantErr: true, + errMsg: "invalid payment option", }, { name: "azure partial-upfront is rejected (aws-only token)", @@ -408,6 +419,16 @@ func TestServiceConfig_Validate(t *testing.T) { wantErr: true, errMsg: "invalid payment option", }, + { + name: "azure error message includes canonical set", + config: ServiceConfig{ + Provider: "azure", + Service: "vm", + Payment: "all-upfront", + }, + wantErr: true, + errMsg: "valid for azure: upfront, monthly", + }, { name: "gcp upfront is valid", config: ServiceConfig{ @@ -436,6 +457,36 @@ func TestServiceConfig_Validate(t *testing.T) { wantErr: true, errMsg: "invalid payment option", }, + { + name: "gcp all-upfront is rejected (aws-only token)", + config: ServiceConfig{ + Provider: "gcp", + Service: "computeengine", + Payment: "all-upfront", + }, + wantErr: true, + errMsg: "invalid payment option", + }, + { + name: "gcp no-upfront is rejected (aws-only token)", + config: ServiceConfig{ + Provider: "gcp", + Service: "computeengine", + Payment: "no-upfront", + }, + wantErr: true, + errMsg: "invalid payment option", + }, + { + name: "gcp error message includes canonical set", + config: ServiceConfig{ + Provider: "gcp", + Service: "computeengine", + Payment: "no-upfront", + }, + wantErr: true, + errMsg: "valid for gcp: upfront, monthly", + }, { name: "coverage too low", config: ServiceConfig{ From eaac7f913040c1c36bb60b579b83509f8f9b77a1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 23:08:19 +0200 Subject: [PATCH 4/7] fix(rec-emission): normalize AWS-style payment tokens for GCP/Azure before validation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The validator tightened in the previous commit now rejects AWS-style payment-option tokens ("all-upfront", "no-upfront", "partial-upfront") on non-AWS service configs. That's correct as a config-layer invariant but brittle if a recommendation-emission code path (or a globally-default payment-option setting fanned out across providers) stamps an AWS-style token onto a non-AWS rec — the rec persists with a token that the validator later refuses, surfacing as a confusing "invalid payment option" downstream rather than at the source. Add config.NormalizePaymentOption(provider, raw) -> (canonical, ok) and apply it at the shared emission boundary in scheduler.convertRecommendations, where every common.Recommendation from every provider passes through on its way to the RecommendationRecord table. Mapping: - AWS: passthrough. - Azure/GCP: all-upfront → upfront no-upfront → monthly partial-upfront → upfront (coerce-to-nearest with WARN log) Rationale for the partial-upfront coercion (vs drop-the-rec): partial-upfront has no semantic equivalent in Azure's or GCP's reservation/CUD models — both only model "one-time upfront" vs "monthly recurring". Dropping the rec would be a silent data loss for the user. Coercing to "upfront" preserves the rec at the closest billing tier the provider actually offers, and the WARN log surfaces the upstream stamping bug so an operator can fix it. This is the same tradeoff the existing per-service pricing switches make when their default branch falls back to "upfront" treatment. Tests cover all 3 providers x {canonical, AWS-style, AWS-only, Azure/GCP-style cross-feeds, empty, garbage} plus the unknown-provider fast-fail path. The GlobalConfig union-set test is unchanged — the union of tightened sets is still {no-upfront, partial-upfront, all-upfront, upfront, monthly} since AWS's three tokens stay distinct. Verified emission sites today already use canonical tokens (every PaymentOption: "upfront" / "monthly" literal in providers/azure/ and providers/gcp/). The normalizer is defensive belt-and-braces — its real value is at the scheduler.convertRecommendations boundary where any future code path or operator-set globalCfg.DefaultPayment that flows into a non-AWS rec gets canonicalized once before persistence. --- internal/config/validation.go | 60 ++++++++++++++++++++++++++++++ internal/config/validation_test.go | 57 ++++++++++++++++++++++++++++ internal/scheduler/scheduler.go | 20 ++++++++++ 3 files changed, 137 insertions(+) diff --git a/internal/config/validation.go b/internal/config/validation.go index 07ee5cc7a..b24ce1c13 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -79,6 +79,66 @@ func validPaymentOptionsFor(provider string) []string { return ValidPaymentOptionsByProvider[provider] } +// NormalizePaymentOption maps a raw payment-option token onto the canonical +// token the given provider semantically models (see ValidPaymentOptionsByProvider). +// It exists so the recommendation-emission boundary (see +// internal/scheduler/scheduler.go:convertRecommendations) can defensively +// canonicalize any AWS-style token that a code path or a globally-default +// payment-option setting might stamp onto a non-AWS rec, before the rec is +// persisted and later validated against the provider-canonical set. +// +// Returns (canonical, true) when raw is already canonical for the provider +// or has an unambiguous canonical mapping. Returns ("", false) only for +// unknown providers — every known cross-provider AWS-style token maps to a +// canonical value: +// +// - AWS : passthrough (AWS already speaks the three-tier set). +// - Azure: all-upfront → upfront, no-upfront → monthly, +// partial-upfront → upfront (no semantic equivalent — coerce to nearest +// all-upfront tier rather than drop the rec; caller may log). +// - GCP : all-upfront → upfront, no-upfront → monthly, +// partial-upfront → upfront (same rationale as Azure). +// +// The partial-upfront coercion is deliberate: dropping the rec would be a +// silent data loss for the user, while coercing to the all-upfront tier +// preserves the rec at the closest billing model the provider offers. The +// caller is expected to log a warning so an operator notices the input bug. +// +// Empty raw passes through as ("", true) — callers that distinguish "unset" +// from "invalid" can check the returned bool only when raw is non-empty. +func NormalizePaymentOption(provider, raw string) (string, bool) { + if _, known := ValidPaymentOptionsByProvider[provider]; !known { + return "", false + } + if raw == "" { + return "", true + } + // Already canonical for this provider: passthrough. + for _, v := range ValidPaymentOptionsByProvider[provider] { + if raw == v { + return raw, true + } + } + // Cross-provider AWS-style tokens that have a canonical equivalent in + // the Azure/GCP two-tier model. + if provider == "azure" || provider == "gcp" { + switch raw { + case "all-upfront": + return "upfront", true + case "no-upfront": + return "monthly", true + case "partial-upfront": + // No semantic equivalent — coerce to the all-upfront tier so the + // rec survives validation; caller should WARN-log the substitution + // so an operator can fix the upstream stamping bug. + return "upfront", true + } + } + // Anything else (including Azure/GCP-style tokens on AWS) is left as-is + // and will surface as a validation error at the next boundary. + return raw, false +} + // ValidRampScheduleTypes lists all supported ramp schedule types var ValidRampScheduleTypes = []string{"immediate", "weekly", "monthly", "custom"} diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index 36b558371..c72a54a86 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -854,6 +854,63 @@ func TestIsValidPaymentOption(t *testing.T) { assert.False(t, isValidPaymentOption("")) } +func TestNormalizePaymentOption(t *testing.T) { + tests := []struct { + name string + provider string + raw string + want string + ok bool + }{ + // AWS: passthrough for every canonical token. + {"aws no-upfront passthrough", "aws", "no-upfront", "no-upfront", true}, + {"aws partial-upfront passthrough", "aws", "partial-upfront", "partial-upfront", true}, + {"aws all-upfront passthrough", "aws", "all-upfront", "all-upfront", true}, + // AWS: Azure/GCP-style tokens are left as-is and flagged for the + // next validator boundary to surface. + {"aws upfront left as-is", "aws", "upfront", "upfront", false}, + {"aws monthly left as-is", "aws", "monthly", "monthly", false}, + + // Azure: canonical passthrough. + {"azure upfront passthrough", "azure", "upfront", "upfront", true}, + {"azure monthly passthrough", "azure", "monthly", "monthly", true}, + // Azure: AWS-style aliases coerced to canonical. + {"azure all-upfront → upfront", "azure", "all-upfront", "upfront", true}, + {"azure no-upfront → monthly", "azure", "no-upfront", "monthly", true}, + {"azure partial-upfront → upfront (nearest)", "azure", "partial-upfront", "upfront", true}, + + // GCP: canonical passthrough. + {"gcp upfront passthrough", "gcp", "upfront", "upfront", true}, + {"gcp monthly passthrough", "gcp", "monthly", "monthly", true}, + // GCP: AWS-style aliases coerced to canonical. + {"gcp all-upfront → upfront", "gcp", "all-upfront", "upfront", true}, + {"gcp no-upfront → monthly", "gcp", "no-upfront", "monthly", true}, + {"gcp partial-upfront → upfront (nearest)", "gcp", "partial-upfront", "upfront", true}, + + // Empty raw: passthrough on any known provider. + {"empty raw on aws", "aws", "", "", true}, + {"empty raw on azure", "azure", "", "", true}, + {"empty raw on gcp", "gcp", "", "", true}, + + // Unknown provider: ok=false, no canonicalization. + {"unknown provider", "ibm", "all-upfront", "", false}, + {"empty provider", "", "monthly", "", false}, + + // Garbage tokens on known providers: left as-is, ok=false. + {"azure garbage", "azure", "ohai", "ohai", false}, + {"gcp garbage", "gcp", "ohai", "ohai", false}, + {"aws garbage", "aws", "ohai", "ohai", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, ok := NormalizePaymentOption(tt.provider, tt.raw) + assert.Equal(t, tt.want, got) + assert.Equal(t, tt.ok, ok) + }) + } +} + func TestIsValidRampScheduleType(t *testing.T) { assert.True(t, isValidRampScheduleType("immediate")) assert.True(t, isValidRampScheduleType("weekly")) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index a555b4a94..1b8c0305a 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -1102,6 +1102,26 @@ func (s *Scheduler) convertRecommendations(recs []common.Recommendation, provide engine := extractEngine(rec.Details) detailsBlob := marshalRecDetails(rec, providerName) + // Canonicalize PaymentOption at the emission boundary so a downstream + // plan-validator round-trip never sees a cross-provider/AWS-style + // token on a non-AWS rec (issue #698). The provider service clients + // historically aliased "all-upfront"/"no-upfront" to the canonical + // "upfront"/"monthly" inside their pricing switches; the validator + // no longer accepts those aliases. Normalize once here so persisted + // recs always carry the provider-canonical token. + if canon, ok := config.NormalizePaymentOption(providerName, rec.PaymentOption); ok { + if canon != rec.PaymentOption { + logging.Warnf("convertRecommendations: coerced %s payment_option %q to canonical %q (account=%s service=%s sku=%s)", + providerName, rec.PaymentOption, canon, rec.Account, rec.Service, rec.ResourceType) + rec.PaymentOption = canon + } + } else if rec.PaymentOption != "" { + // Unknown provider or unmapped token: leave as-is. The next + // validator boundary will surface the issue with a clear error. + logging.Warnf("convertRecommendations: cannot canonicalize %s payment_option %q (account=%s service=%s sku=%s) — leaving as-is", + providerName, rec.PaymentOption, rec.Account, rec.Service, rec.ResourceType) + } + // Parse term to integer (e.g., "3yr" -> 3) term := 3 if rec.Term != "" { From 5036b11d56b7f32919936d38bd254e3989eb3af3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 25 May 2026 23:38:54 +0200 Subject: [PATCH 5/7] fix(config): tighten GCP payment-option set to monthly-only per GCP CUD semantics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #709's first revision tightened GCP to {upfront, monthly} matching the per-service pricing switches at providers/gcp/services/*/client.go. Closer inspection of the actual GCP CUD purchase path shows that's still an over-acceptance: the GCP CUD API only takes a Plan discriminator (TWELVE_MONTH / THIRTY_SIX_MONTH) and never reads a payment-option field — see buildCommitmentRequests at providers/gcp/services/computeengine/client.go :350-373. GCP commitments are inherently monthly-billed across the term; "upfront" exists in the codebase only as a misnomer that one GCP emitter (computeengine/client.go:804) still stamps, while the other three GCP services (cloudsql, cloudstorage, memorystore) already stamp "monthly". Tighten the validator's GCP set to {monthly} alone, with the doc comment pointing at both the purchase path (where Plan is the only knob) and the pricing switches (which alias "upfront"/"all-upfront" only for compatibility with downstream code expecting a token). Azure remains {upfront, monthly} — Azure reservations DO model both billing plans, verified against the seven service-client switches at compute/cache/cosmosdb/database/search/synapse/ managedredis client.go (all accept {all-upfront, upfront} and {monthly, no-upfront} as aliased pairs). AWS is unchanged at the canonical three-tier {no-upfront, partial-upfront, all-upfront}. Update NormalizePaymentOption's GCP branch to coerce every non-monthly token — including the legacy "upfront" that computeengine/client.go:804 stamps — to "monthly". This keeps the existing GCP emission path safe: the scheduler.convertRecommendations boundary now canonicalizes "upfront" → "monthly" with a WARN log before persistence, so the rec carries the provider-canonical token downstream and the validator no longer trips on its way to the plan-validator. The Azure mapping is unchanged (all-upfront → upfront, no-upfront → monthly, partial-upfront → upfront). Existing helper survey before adding the new mapping: - internal/commitmentopts/normalizePayment is unexported and parses AWS API response spellings ("All Upfront" / "ALL_UPFRONT") into the canonical AWS three-tier set — different concern (within-AWS spelling normalization, not cross-provider canonicalization). - internal/api/validation.go:purchasePaymentWhitelist is the purchase-execute boundary, currently still permissive (accepts AWS-style tokens on Azure/GCP) — explicitly out of scope per the issue spec. - pkg/common/reservation_name.go:normalizeReservationPayment produces short-form name segments ("allup" / "noup" / "partup"), not canonical tokens — different concern. No prior cross-provider canonicalization helper exists; the new function stays in internal/config/validation.go alongside ValidPaymentOptionsByProvider. Tests: - ServiceConfig: gcp-monthly stays valid, gcp-upfront is now rejected, error message asserts "valid for gcp: monthly". - NormalizePaymentOption: GCP canonical set is {monthly}; "upfront", "all-upfront", "no-upfront", "partial-upfront" all coerce to "monthly". - GlobalConfig union test unchanged — "upfront" is still in the union because Azure still has it. --- internal/config/validation.go | 96 +++++++++++++++++++++--------- internal/config/validation_test.go | 24 ++++---- 2 files changed, 80 insertions(+), 40 deletions(-) diff --git a/internal/config/validation.go b/internal/config/validation.go index b24ce1c13..2369b7d12 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -44,16 +44,24 @@ var ValidPaymentOptions = []string{"no-upfront", "partial-upfront", "all-upfront // recommendations are not currently emitted — GetRecommendations returns // []; the canonical set follows the only path that emits today.) // -// - GCP : {upfront, monthly} -// CUD purchase model has only one-time upfront vs monthly recurring. See: -// providers/gcp/services/computeengine/client.go:587-591 -// providers/gcp/services/cloudsql/client.go:288-292 -// providers/gcp/services/cloudstorage/client.go:297-301 -// providers/gcp/services/memorystore/client.go:245-249 +// - GCP : {monthly} +// GCP CUDs are inherently monthly-billed across the term — the GCP CUD +// purchase API only takes a Plan (TWELVE_MONTH / THIRTY_SIX_MONTH), not a +// payment-option discriminator (see +// providers/gcp/services/computeengine/client.go:350-373 where +// buildCommitmentRequests sets Plan from rec.Term and never reads +// PaymentOption). The per-service pricing switches at +// providers/gcp/services/computeengine/client.go:587-591, +// providers/gcp/services/cloudsql/client.go:288-292, +// providers/gcp/services/cloudstorage/client.go:297-301, +// providers/gcp/services/memorystore/client.go:245-249 alias "upfront" +// and "all-upfront" only for compatibility with downstream code that +// expects a payment-option token; semantically GCP exposes one billing +// plan and the validator surfaces that explicitly. var ValidPaymentOptionsByProvider = map[string][]string{ "aws": {"no-upfront", "partial-upfront", "all-upfront"}, "azure": {"upfront", "monthly"}, - "gcp": {"upfront", "monthly"}, + "gcp": {"monthly"}, } // validPaymentOptionsUnion is the union of all provider payment option sets, @@ -88,21 +96,30 @@ func validPaymentOptionsFor(provider string) []string { // persisted and later validated against the provider-canonical set. // // Returns (canonical, true) when raw is already canonical for the provider -// or has an unambiguous canonical mapping. Returns ("", false) only for -// unknown providers — every known cross-provider AWS-style token maps to a -// canonical value: +// or has an unambiguous canonical mapping. Returns ("", false) for unknown +// providers and (raw, false) for tokens that have no canonical mapping on +// the given provider (e.g. an Azure/GCP-style "upfront" on AWS) — callers +// can use ok=false to surface the unmapped token at the next validator +// boundary. Per-provider mapping: // // - AWS : passthrough (AWS already speaks the three-tier set). // - Azure: all-upfront → upfront, no-upfront → monthly, -// partial-upfront → upfront (no semantic equivalent — coerce to nearest +// partial-upfront → upfront (no semantic equivalent — coerce to the // all-upfront tier rather than drop the rec; caller may log). -// - GCP : all-upfront → upfront, no-upfront → monthly, -// partial-upfront → upfront (same rationale as Azure). +// - GCP : all-upfront → monthly, no-upfront → monthly, +// partial-upfront → monthly, upfront → monthly (GCP CUDs are +// inherently monthly-billed — every non-monthly token collapses to the +// one billing plan GCP actually models). The collapse from "upfront" to +// "monthly" is what makes the existing +// providers/gcp/services/computeengine/client.go:804 stamp safe: the +// scheduler.convertRecommendations boundary coerces it once before +// persistence so the rec carries the canonical token downstream. // -// The partial-upfront coercion is deliberate: dropping the rec would be a -// silent data loss for the user, while coercing to the all-upfront tier -// preserves the rec at the closest billing model the provider offers. The -// caller is expected to log a warning so an operator notices the input bug. +// The partial-upfront coercion is deliberate on both Azure and GCP: dropping +// the rec would be a silent data loss for the user, while coercing to the +// closest billing model the provider offers preserves the rec. The caller is +// expected to log a warning when raw != canonical so an operator notices the +// upstream input bug. // // Empty raw passes through as ("", true) — callers that distinguish "unset" // from "invalid" can check the returned bool only when raw is non-empty. @@ -119,24 +136,45 @@ func NormalizePaymentOption(provider, raw string) (string, bool) { return raw, true } } - // Cross-provider AWS-style tokens that have a canonical equivalent in - // the Azure/GCP two-tier model. - if provider == "azure" || provider == "gcp" { + // Cross-provider tokens that have a canonical equivalent in the target + // provider's billing model. Each provider's mapping is delegated to a + // helper to keep cyclomatic complexity below the project limit. + if canon, ok := crossProviderPaymentAlias(provider, raw); ok { + return canon, true + } + // Anything else (including Azure/GCP-style tokens on AWS) is left as-is + // and will surface as a validation error at the next boundary. + return raw, false +} + +// crossProviderPaymentAlias maps an AWS-style (or Azure-style on GCP) token +// onto the target provider's canonical token. Returns (canonical, true) when +// a mapping exists, ("", false) when none does. Split out of +// NormalizePaymentOption purely to keep that function under the project's +// gocyclo limit; the policy lives here. +func crossProviderPaymentAlias(provider, raw string) (string, bool) { + switch provider { + case "azure": + // Azure reservations model both billing plans; coerce AWS-style tokens + // to the Azure-canonical spelling. partial-upfront has no Azure + // equivalent — coerce to the all-upfront tier so the rec survives + // validation rather than dropping silently (caller WARN-logs). switch raw { - case "all-upfront": + case "all-upfront", "partial-upfront": return "upfront", true case "no-upfront": return "monthly", true - case "partial-upfront": - // No semantic equivalent — coerce to the all-upfront tier so the - // rec survives validation; caller should WARN-log the substitution - // so an operator can fix the upstream stamping bug. - return "upfront", true + } + case "gcp": + // GCP commitments are monthly-only — every non-monthly token (AWS-style + // or the legacy "upfront" some GCP code paths still stamp) collapses + // to "monthly", the one billing plan GCP semantically models. + switch raw { + case "all-upfront", "no-upfront", "partial-upfront", "upfront": + return "monthly", true } } - // Anything else (including Azure/GCP-style tokens on AWS) is left as-is - // and will surface as a validation error at the next boundary. - return raw, false + return "", false } // ValidRampScheduleTypes lists all supported ramp schedule types diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index c72a54a86..a3ac597e9 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -430,22 +430,23 @@ func TestServiceConfig_Validate(t *testing.T) { errMsg: "valid for azure: upfront, monthly", }, { - name: "gcp upfront is valid", + name: "gcp monthly is valid", config: ServiceConfig{ Provider: "gcp", Service: "computeengine", - Payment: "upfront", + Payment: "monthly", }, wantErr: false, }, { - name: "gcp monthly is valid", + name: "gcp upfront is rejected (gcp is monthly-only)", config: ServiceConfig{ Provider: "gcp", Service: "computeengine", - Payment: "monthly", + Payment: "upfront", }, - wantErr: false, + wantErr: true, + errMsg: "invalid payment option", }, { name: "gcp partial-upfront is rejected (aws-only token)", @@ -485,7 +486,7 @@ func TestServiceConfig_Validate(t *testing.T) { Payment: "no-upfront", }, wantErr: true, - errMsg: "valid for gcp: upfront, monthly", + errMsg: "valid for gcp: monthly", }, { name: "coverage too low", @@ -879,13 +880,14 @@ func TestNormalizePaymentOption(t *testing.T) { {"azure no-upfront → monthly", "azure", "no-upfront", "monthly", true}, {"azure partial-upfront → upfront (nearest)", "azure", "partial-upfront", "upfront", true}, - // GCP: canonical passthrough. - {"gcp upfront passthrough", "gcp", "upfront", "upfront", true}, + // GCP: canonical passthrough (monthly-only — every non-monthly token + // collapses to monthly because GCP CUDs only model one billing plan). {"gcp monthly passthrough", "gcp", "monthly", "monthly", true}, - // GCP: AWS-style aliases coerced to canonical. - {"gcp all-upfront → upfront", "gcp", "all-upfront", "upfront", true}, + {"gcp upfront → monthly (gcp is monthly-only)", "gcp", "upfront", "monthly", true}, + // GCP: AWS-style aliases coerced to the one canonical token. + {"gcp all-upfront → monthly", "gcp", "all-upfront", "monthly", true}, {"gcp no-upfront → monthly", "gcp", "no-upfront", "monthly", true}, - {"gcp partial-upfront → upfront (nearest)", "gcp", "partial-upfront", "upfront", true}, + {"gcp partial-upfront → monthly", "gcp", "partial-upfront", "monthly", true}, // Empty raw: passthrough on any known provider. {"empty raw on aws", "aws", "", "", true}, From 3fb9cb9ed859fb5606e5d71fc51f43ceb1678f46 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 26 May 2026 00:10:26 +0200 Subject: [PATCH 6/7] fix(config): deterministic ordering for validPaymentOptionsUnion Map iteration in Go is non-deterministic, so validPaymentOptionsUnion could be built in a different order each run, causing validation error messages listing valid payment options to vary across runs and making test assertions on error strings brittle. Add sort.Strings(all) before returning the union slice so the order is stable. Add TestValidPaymentOptionsUnionDeterministic to assert the specific expected sorted order and the pairwise ordering invariant. --- internal/config/validation.go | 4 ++++ internal/config/validation_test.go | 22 ++++++++++++++++++++++ 2 files changed, 26 insertions(+) diff --git a/internal/config/validation.go b/internal/config/validation.go index 2369b7d12..d44a8d009 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -4,6 +4,7 @@ package config import ( "fmt" "net/mail" + "sort" "strings" ) @@ -67,6 +68,8 @@ var ValidPaymentOptionsByProvider = map[string][]string{ // validPaymentOptionsUnion is the union of all provider payment option sets, // used for global-config default validation where no provider context is // available. Accepts any token that is valid for at least one provider. +// The slice is sorted so that validation error messages are deterministic +// across runs (map iteration order in Go is non-deterministic). var validPaymentOptionsUnion = func() []string { seen := map[string]bool{} var all []string @@ -78,6 +81,7 @@ var validPaymentOptionsUnion = func() []string { } } } + sort.Strings(all) return all }() diff --git a/internal/config/validation_test.go b/internal/config/validation_test.go index a3ac597e9..5e2d42190 100644 --- a/internal/config/validation_test.go +++ b/internal/config/validation_test.go @@ -855,6 +855,28 @@ func TestIsValidPaymentOption(t *testing.T) { assert.False(t, isValidPaymentOption("")) } +func TestValidPaymentOptionsUnionDeterministic(t *testing.T) { + // validPaymentOptionsUnion must be sorted so that validation error messages + // that include the list of valid options are reproducible across runs. + // Map iteration in Go is non-deterministic; without an explicit sort the + // slice order can vary, making test assertions on error strings brittle. + expected := []string{"all-upfront", "monthly", "no-upfront", "partial-upfront", "upfront"} + assert.Equal(t, expected, validPaymentOptionsUnion, + "validPaymentOptionsUnion must be in sorted order for deterministic error messages") + + // Double-check: building the union a second time (simulating another init + // call) must produce the same sorted result. We verify by sorting a fresh + // copy of the current value and confirming it is byte-equal. + sorted := make([]string, len(validPaymentOptionsUnion)) + copy(sorted, validPaymentOptionsUnion) + // sorted is already sorted by construction; assert the slice is in order. + for i := 1; i < len(sorted); i++ { + assert.LessOrEqual(t, sorted[i-1], sorted[i], + "validPaymentOptionsUnion[%d] %q must be <= validPaymentOptionsUnion[%d] %q", + i-1, sorted[i-1], i, sorted[i]) + } +} + func TestNormalizePaymentOption(t *testing.T) { tests := []struct { name string From 944c74454cabb6a2bcf0128e7d5fa453be0fd5f5 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 26 May 2026 00:13:54 +0200 Subject: [PATCH 7/7] docs(config): clarify ValidPaymentOptionsByProvider canonical-sets framing + sort union for deterministic errors N2 (doc comment): replace the "accepts synonyms" framing with an explicit canonical-sets description referencing the per-provider service-client switch statements. Clarifies that cross-provider tokens are rejected loudly and that NormalizePaymentOption canonicalizes at the rec-emission boundary before the validator is reached. N3/N4 (sort union): sort.Strings was already present in the init func and the determinism test already existed; no code change needed. N1 (synonym rejection tests): all rejection cases CR suggested as acceptance cases (azure no-upfront, azure partial-upfront, gcp all-upfront, gcp no-upfront, gcp upfront) were already present in the test suite as rejection assertions, consistent with the tightened canonical-sets design decision. --- internal/config/validation.go | 57 ++++++++++++----------------------- 1 file changed, 19 insertions(+), 38 deletions(-) diff --git a/internal/config/validation.go b/internal/config/validation.go index d44a8d009..08b25718f 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -17,48 +17,29 @@ var ValidProviders = []string{"aws", "azure", "gcp"} var ValidPaymentOptions = []string{"no-upfront", "partial-upfront", "all-upfront"} // ValidPaymentOptionsByProvider maps each provider to the payment option -// tokens it accepts. The sets are tightened to the canonical tokens each -// provider actually models semantically; AWS-style aliases the service -// clients also accept (for legacy frontend compat) are deliberately not -// surfaced here — config-layer validation rejects them so input bugs aren't -// hidden behind silent alias coercion in the service-client switches. -// Callers that need to translate legacy/cross-provider tokens into the -// canonical form should use NormalizePaymentOption at the emission boundary -// (see internal/scheduler/scheduler.go:convertRecommendations). +// tokens it accepts. Each provider's set is the canonical set verified +// against the provider's service-client switch statements: // -// Canonical sets, verified against the per-service purchase/pricing switches: +// - aws: {no-upfront, partial-upfront, all-upfront}: the three RI/SP +// billing tiers exposed by AWS APIs. // -// - AWS : {no-upfront, partial-upfront, all-upfront} -// Three distinct billing tiers exposed by RI/SP offering APIs. +// - azure: {upfront, monthly}: verified against the 7 Azure service-client +// switches in providers/azure/services/{compute,cache,cosmosdb,database, +// search,synapse,managedredis}/client.go. (The savingsplans client mirrors +// AWS's three-tier set, but Azure savings-plan recs are not currently +// emitted; GetRecommendations returns []. The canonical set follows the +// only path that emits today.) // -// - Azure: {upfront, monthly} -// Reservation purchases only model two billing plans. See: -// providers/azure/services/compute/client.go:493-497 -// providers/azure/services/cache/client.go:375-379 -// providers/azure/services/cosmosdb/client.go:368-372 -// providers/azure/services/database/client.go:376-380 -// providers/azure/services/search/client.go:349-353 -// providers/azure/services/synapse/client.go:354-358 -// providers/azure/services/managedredis/client.go:345-349 -// (The savingsplans switch at providers/azure/services/savingsplans/ -// client.go:418-429 mirrors AWS's three-tier set, but Azure savings-plan -// recommendations are not currently emitted — GetRecommendations returns -// []; the canonical set follows the only path that emits today.) +// - gcp: {monthly}: GCP CUDs are billed monthly across the commitment +// term; there is no upfront billing tier. See +// providers/gcp/services/computeengine/client.go:buildCommitmentRequests +// which takes only a Plan (TWELVE_MONTH/THIRTY_SIX_MONTH) and never reads +// PaymentOption. // -// - GCP : {monthly} -// GCP CUDs are inherently monthly-billed across the term — the GCP CUD -// purchase API only takes a Plan (TWELVE_MONTH / THIRTY_SIX_MONTH), not a -// payment-option discriminator (see -// providers/gcp/services/computeengine/client.go:350-373 where -// buildCommitmentRequests sets Plan from rec.Term and never reads -// PaymentOption). The per-service pricing switches at -// providers/gcp/services/computeengine/client.go:587-591, -// providers/gcp/services/cloudsql/client.go:288-292, -// providers/gcp/services/cloudstorage/client.go:297-301, -// providers/gcp/services/memorystore/client.go:245-249 alias "upfront" -// and "all-upfront" only for compatibility with downstream code that -// expects a payment-option token; semantically GCP exposes one billing -// plan and the validator surfaces that explicitly. +// Cross-provider tokens are rejected loudly by the validator. Legacy AWS-style +// tokens emitted by older code paths are canonicalized via NormalizePaymentOption +// at the rec-emission boundary BEFORE reaching this validator (see +// internal/scheduler/scheduler.go:convertRecommendations). var ValidPaymentOptionsByProvider = map[string][]string{ "aws": {"no-upfront", "partial-upfront", "all-upfront"}, "azure": {"upfront", "monthly"},