Skip to content

fix(api/validation): provider-canonical payment-option whitelist (closes #717) #793

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
cristim merged 1 commit into from
Jun 5, 2026
Merged

fix(api/validation): provider-canonical payment-option whitelist (closes #717) #793

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
53 changes: 51 additions & 2 deletions internal/api/handler_purchases_guards_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,15 +43,48 @@ func TestValidatePurchaseRecommendation(t *testing.T) {
rec config.RecommendationRecord
wantError bool
}{
// --- AWS canonical set ---
{"valid aws all-upfront 3y", validRec(), false},
{"valid aws no-upfront 1y", mutate(func(r *config.RecommendationRecord) { r.Payment = "no-upfront"; r.Term = 1 }), false},
{"valid aws partial-upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "partial-upfront" }), false},
{"aws rejects azure-only monthly", mutate(func(r *config.RecommendationRecord) { r.Payment = "monthly" }), true},
{"aws rejects azure-only upfront", mutate(func(r *config.RecommendationRecord) { r.Payment = "upfront" }), true},
// --- Azure canonical set ---
{"valid azure upfront", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "upfront" }), false},
{"valid azure monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "azure"; r.Payment = "monthly" }), false},
{"valid gcp upfront", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp"; r.Payment = "upfront" }), false},
// Legacy AWS-style tokens on Azure are normalized to Azure-canonical before validation.
{"azure accepts legacy all-upfront (coerced to upfront)", mutate(func(r *config.RecommendationRecord) {
r.Provider = "azure"; r.Payment = "all-upfront"
}), false},
{"azure accepts legacy no-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) {
r.Provider = "azure"; r.Payment = "no-upfront"
}), false},
{"azure accepts legacy partial-upfront (coerced to upfront)", mutate(func(r *config.RecommendationRecord) {
r.Provider = "azure"; r.Payment = "partial-upfront"
}), false},
{"azure rejects unknown token", mutate(func(r *config.RecommendationRecord) {
r.Provider = "azure"; r.Payment = "foo"
}), true},
// --- GCP canonical set (monthly-only) ---
{"valid gcp monthly", mutate(func(r *config.RecommendationRecord) { r.Provider = "gcp"; r.Payment = "monthly" }), false},
// Legacy tokens on GCP are all normalized to monthly.
{"gcp accepts legacy upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) {
r.Provider = "gcp"; r.Payment = "upfront"
}), false},
{"gcp accepts legacy all-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) {
r.Provider = "gcp"; r.Payment = "all-upfront"
}), false},
{"gcp accepts legacy no-upfront (coerced to monthly)", mutate(func(r *config.RecommendationRecord) {
r.Provider = "gcp"; r.Payment = "no-upfront"
}), false},
{"gcp rejects unknown token", mutate(func(r *config.RecommendationRecord) {
r.Provider = "gcp"; r.Payment = "foo"
}), true},
// --- General ---
{"payment case-insensitive", mutate(func(r *config.RecommendationRecord) { r.Payment = "All-Upfront" }), false},
{"invalid term 7", mutate(func(r *config.RecommendationRecord) { r.Term = 7 }), true},
{"invalid term 0", mutate(func(r *config.RecommendationRecord) { r.Term = 0 }), true},
{"invalid payment foo", mutate(func(r *config.RecommendationRecord) { r.Payment = "foo" }), true},
{"aws rejects azure-only monthly", mutate(func(r *config.RecommendationRecord) { r.Payment = "monthly" }), true},
{"negative count", mutate(func(r *config.RecommendationRecord) { r.Count = -1 }), true},
{"zero count", mutate(func(r *config.RecommendationRecord) { r.Count = 0 }), true},
{"empty service", mutate(func(r *config.RecommendationRecord) { r.Service = "" }), true},
Expand All @@ -74,6 +107,22 @@ func TestValidatePurchaseRecommendation(t *testing.T) {
}
}

// TestValidatePurchaseRecommendation_ErrorMessage verifies that a mismatched
// (provider, payment-option) pair produces a 400 error whose message names the
// provider and lists the valid options, matching the plan-validator shape
// required by issue #717.
func TestValidatePurchaseRecommendation_ErrorMessage(t *testing.T) {
t.Parallel()
rec := validRec()
rec.Provider = "azure"
rec.Payment = "foo" // not in azure canonical set and has no normalization alias
err := validatePurchaseRecommendation(&rec, 0)
require.Error(t, err)
assert.Contains(t, err.Error(), "azure")
assert.Contains(t, err.Error(), "upfront")
assert.Contains(t, err.Error(), "monthly")
}

// The per-rec #643 validation is wired into the web execute boundary
// (validateExecutePurchaseRequest), NOT the shared validateAndTotalRecommendations
// which the retry path also calls with replayed recs. This test pins that
Expand Down
68 changes: 38 additions & 30 deletions internal/api/validation.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -463,32 +463,21 @@ func parseAccountIDs(raw string) ([]string, error) {
return ids, nil
}

// purchasePaymentWhitelist maps a concrete provider to the set of payment
// options that provider's purchase path accepts. AWS RIs/SPs support the
// three classic upfront tiers; Azure/GCP reservations additionally accept
// the "upfront"/"monthly" spellings their SDK clients switch on (see
// providers/{azure,gcp}/services/*/client.go). Any value outside the set
// for the rec's provider is rejected at the API boundary so a malformed
// Payment never reaches the cloud SDK with a silent default substituted
// (issue #643).
var purchasePaymentWhitelist = map[string]map[string]bool{
"aws": {
"all-upfront": true,
"partial-upfront": true,
"no-upfront": true,
},
"azure": {
"all-upfront": true,
"upfront": true,
"no-upfront": true,
"monthly": true,
},
"gcp": {
"all-upfront": true,
"upfront": true,
"no-upfront": true,
"monthly": true,
},
// purchasePaymentSet returns the provider-canonical set of accepted payment
// option tokens for the given (already-lowercased) provider. It derives the
// set from config.ValidPaymentOptionsByProvider so that this boundary and the
// plan-validator share a single source of truth and cannot drift (issue #717).
// Returns nil when provider is unknown.
func purchasePaymentSet(provider string) map[string]bool {
opts, ok := config.ValidPaymentOptionsByProvider[provider]
if !ok {
return nil
}
set := make(map[string]bool, len(opts))
for _, o := range opts {
set[o] = true
}
return set
}

// purchaseTermWhitelist maps a concrete provider to the set of commitment
Expand All @@ -506,11 +495,20 @@ var purchaseTermWhitelist = map[string]map[int]bool{
// query-time validateProvider (which permits ""/"all"), the execute path
// requires a concrete provider because each rec triggers a real provider
// call. idx is the rec's position in the request slice, surfaced in the
// error so the caller can point at the offending row. Closes issue #643.
// error so the caller can point at the offending row. Closes issues #643,
// #717.
//
// Payment-option handling follows the same two-step approach as the
// plan-validator in internal/config/validation.go:
// 1. NormalizePaymentOption coerces any legacy AWS-style token onto the
// provider-canonical spelling (e.g. "all-upfront" on Azure -> "upfront").
// 2. The provider-canonical set (from config.ValidPaymentOptionsByProvider)
// rejects anything that has no canonical mapping, with an error that
// names the provider and lists the accepted tokens.
func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) error {
provider := strings.ToLower(strings.TrimSpace(rec.Provider))
payments, providerOK := purchasePaymentWhitelist[provider]
if !providerOK {
payments := purchasePaymentSet(provider)
if payments == nil {
return NewClientError(400, fmt.Sprintf("recommendation %d has invalid provider %q: must be one of aws, azure, gcp", idx, rec.Provider))
}
rec.Provider = provider
Expand All @@ -525,8 +523,18 @@ func validatePurchaseRecommendation(rec *config.RecommendationRecord, idx int) e
return NewClientError(400, fmt.Sprintf("recommendation %d has invalid term %d for provider %s: must be 1 or 3", idx, rec.Term, provider))
}
payment := strings.ToLower(strings.TrimSpace(rec.Payment))
// Coerce any legacy/cross-provider alias before the whitelist check so
// that callers using old AWS-style tokens are transparently redirected to
// the canonical token for the target provider.
if normalized, ok := config.NormalizePaymentOption(provider, payment); ok {
payment = normalized
}
if !payments[payment] {
return NewClientError(400, fmt.Sprintf("recommendation %d has invalid payment %q for provider %s", idx, rec.Payment, provider))
return NewClientError(400, fmt.Sprintf(
"invalid payment option for %s service: %q (valid for %s: %s)",
provider, rec.Payment, provider,
strings.Join(config.ValidPaymentOptionsByProvider[provider], ", "),
))
}
rec.Payment = payment
return nil
Expand Down
Loading