diff --git a/internal/config/validation.go b/internal/config/validation.go index 42a858756..08b25718f 100644 --- a/internal/config/validation.go +++ b/internal/config/validation.go @@ -4,15 +4,164 @@ package config import ( "fmt" "net/mail" + "sort" "strings" ) // 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. Each provider's set is the canonical set verified +// against the provider's service-client switch statements: +// +// - aws: {no-upfront, partial-upfront, all-upfront}: the three RI/SP +// billing tiers exposed by AWS 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.) +// +// - 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. +// +// 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"}, + "gcp": {"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. +// 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 + for _, opts := range ValidPaymentOptionsByProvider { + for _, o := range opts { + if !seen[o] { + seen[o] = true + all = append(all, o) + } + } + } + sort.Strings(all) + 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] +} + +// 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) 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 the +// all-upfront tier rather than drop the rec; caller may log). +// - 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 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. +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 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", "partial-upfront": + return "upfront", true + case "no-upfront": + return "monthly", 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 + } + } + return "", false +} + // ValidRampScheduleTypes lists all supported ramp schedule types var ValidRampScheduleTypes = []string{"immediate", "weekly", "monthly", "custom"} @@ -142,10 +291,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 +351,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 { @@ -296,8 +460,10 @@ 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 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..5e2d42190 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,143 @@ 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 rejected (aws-only token)", + config: ServiceConfig{ + Provider: "azure", + Service: "vm", + Payment: "all-upfront", + }, + 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)", + config: ServiceConfig{ + Provider: "azure", + Service: "vm", + Payment: "partial-upfront", + }, + 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 monthly is valid", + config: ServiceConfig{ + Provider: "gcp", + Service: "computeengine", + Payment: "monthly", + }, + wantErr: false, + }, + { + name: "gcp upfront is rejected (gcp is monthly-only)", + config: ServiceConfig{ + Provider: "gcp", + Service: "computeengine", + Payment: "upfront", + }, + wantErr: true, + errMsg: "invalid payment option", + }, + { + 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: "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: monthly", + }, { name: "coverage too low", config: ServiceConfig{ @@ -689,13 +843,98 @@ 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("")) } +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 + 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 (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 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 → monthly", "gcp", "partial-upfront", "monthly", 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 != "" {