From b8ecf511edfb48cdf99fd317a5a4dcf2072050e3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 19:17:25 +0200 Subject: [PATCH 1/2] fix(cmd): refuse an EC2 Instance Savings Plan without a family in the dry run too validatePurchasePreconditions required only a region for an EC2 Instance Savings Plan. With no instance family and no offering ID, the real run refuses in the offering lookup (ambiguous or missing offerings) while the dry run reported success. Require the family unless an offering ID is set. The completeness fixture now carries a family, as a real Cost Explorer EC2 Instance recommendation does. Closes #2164 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +++ cmd/recommendation_completeness_proxy_test.go | 2 +- cmd/savings_plan_purchase.go | 15 ++++++++++++++- cmd/savings_plan_purchase_test.go | 13 +++++++++++-- 4 files changed, 29 insertions(+), 4 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5bab5c6b3..68d8361c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - Recommendations with no account ID or name are now reported in the drop summary when an account filter (`--include-accounts` or `--exclude-accounts`) removes them (#1613). +- A dry run now refuses an EC2 Instance Savings Plan that has no instance family + and no offering ID, the same way the real run does, instead of reporting + success for a purchase the offering lookup would reject (#2164). - A purchase result with `Success=true` and a non-nil `Error` is now treated as a failure in the Ctrl-C summary, the audit-stop message, the per-service success/failed counts and the non-zero exit. The CSV `Success` column keeps the diff --git a/cmd/recommendation_completeness_proxy_test.go b/cmd/recommendation_completeness_proxy_test.go index d5d3a4f11..2964491d3 100644 --- a/cmd/recommendation_completeness_proxy_test.go +++ b/cmd/recommendation_completeness_proxy_test.go @@ -305,7 +305,7 @@ func (p *completenessProxy) spRecommendation(body []byte) (int, string, string) const invalid = `{"HourlyCommitmentToPurchase":"not-a-number","EstimatedMonthlySavingsAmount":"10","UpfrontCost":"3"}` details := valid if request.SavingsPlansType == "EC2_INSTANCE_SP" { - details = strings.TrimSuffix(valid, "}") + `,"SavingsPlansDetails":{"Region":"us-east-1"}}` + details = strings.TrimSuffix(valid, "}") + `,"SavingsPlansDetails":{"InstanceFamily":"m5","Region":"us-east-1"}}` } switch p.details { case "mixed": diff --git a/cmd/savings_plan_purchase.go b/cmd/savings_plan_purchase.go index d293b839b..1c9aa122d 100644 --- a/cmd/savings_plan_purchase.go +++ b/cmd/savings_plan_purchase.go @@ -151,9 +151,22 @@ func validateSavingsPlanPreconditions(rec common.Recommendation) error { if fmt.Sprintf(spCommitmentFormat, d.HourlyCommitment) == "0.00" { return fmt.Errorf("%s recommendation's hourly commitment %v would be sent as 0.00; it must be at least 0.005", rec.Service, d.HourlyCommitment) } - if rec.Service == common.ServiceSavingsPlansEC2Instance && d.Region == "" { + if rec.Service == common.ServiceSavingsPlansEC2Instance { + return validateEC2InstanceSPOffering(d) + } + return nil +} + +// validateEC2InstanceSPOffering mirrors the library's offering lookup: an +// explicit offering ID skips it, otherwise it needs the region and family to +// pick exactly one offering. +func validateEC2InstanceSPOffering(d *common.SavingsPlanDetails) error { + if d.Region == "" { return fmt.Errorf("EC2 Instance Savings Plan needs a region to pick the offering") } + if d.OfferingID == "" && d.InstanceFamily == "" { + return fmt.Errorf("EC2 Instance Savings Plan needs an instance family or an offering ID to pick the offering") + } return nil } diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index 41d59a33d..f745e271c 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -258,8 +258,14 @@ func TestValidatePurchasePreconditions(t *testing.T) { ok := map[string]common.Recommendation{ "compute sp": parserShapedSP(), "ec2 instance sp": validEC2InstanceSP(), - "ec2": validEC2Rec(), - "rds": {Service: common.ServiceRDS, Region: "us-east-1"}, + "ec2 instance sp offering id no family": mut(validEC2InstanceSP(), func(r *common.Recommendation) { + r.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1, Region: "eu-west-1", OfferingID: "o-1"} + }), + "compute sp no family": mut(parserShapedSP(), func(r *common.Recommendation) { + r.Details = &common.SavingsPlanDetails{PlanType: "Compute", HourlyCommitment: 1} + }), + "ec2": validEC2Rec(), + "rds": {Service: common.ServiceRDS, Region: "us-east-1"}, } for name, rec := range ok { assert.NoError(t, validatePurchasePreconditions(rec, rec.Region, defaultTestAccount), name) @@ -274,6 +280,9 @@ func TestValidatePurchasePreconditions(t *testing.T) { "ec2 instance sp no region": mut(validEC2InstanceSP(), func(r *common.Recommendation) { r.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1, InstanceFamily: "m5"} }), + "ec2 instance sp no family no offering": mut(validEC2InstanceSP(), func(r *common.Recommendation) { + r.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1, Region: "eu-west-1"} + }), "sp all": mut(parserShapedSP(), func(r *common.Recommendation) { r.Service = common.ServiceSavingsPlansAll }), "ec2 nil": mut(validEC2Rec(), func(r *common.Recommendation) { r.Details = nil }), "ec2 tenancy": mut(validEC2Rec(), func(r *common.Recommendation) { r.Details.(*common.ComputeDetails).Tenancy = "" }), From 6f8b09d81eda7d1971de990402f41abbe34314ab Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 18 Oct 2026 01:05:39 +0200 Subject: [PATCH 2/2] fix(cmd): reject whitespace-only EC2 Instance family in SP precondition validateEC2InstanceSPOffering checked InstanceFamily == "", so a family of spaces passed the dry run when OfferingID was empty; the real lookup then sent it as the instanceFamily filter. Trim before the empty check and cover it in TestValidatePurchasePreconditions. --- cmd/savings_plan_purchase.go | 3 ++- cmd/savings_plan_purchase_test.go | 3 +++ 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/cmd/savings_plan_purchase.go b/cmd/savings_plan_purchase.go index 1c9aa122d..1d37727b1 100644 --- a/cmd/savings_plan_purchase.go +++ b/cmd/savings_plan_purchase.go @@ -3,6 +3,7 @@ package main import ( "fmt" "strconv" + "strings" "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" @@ -164,7 +165,7 @@ func validateEC2InstanceSPOffering(d *common.SavingsPlanDetails) error { if d.Region == "" { return fmt.Errorf("EC2 Instance Savings Plan needs a region to pick the offering") } - if d.OfferingID == "" && d.InstanceFamily == "" { + if d.OfferingID == "" && strings.TrimSpace(d.InstanceFamily) == "" { return fmt.Errorf("EC2 Instance Savings Plan needs an instance family or an offering ID to pick the offering") } return nil diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index f745e271c..af4f05e68 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -283,6 +283,9 @@ func TestValidatePurchasePreconditions(t *testing.T) { "ec2 instance sp no family no offering": mut(validEC2InstanceSP(), func(r *common.Recommendation) { r.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1, Region: "eu-west-1"} }), + "ec2 instance sp whitespace family no offering": mut(validEC2InstanceSP(), func(r *common.Recommendation) { + r.Details = &common.SavingsPlanDetails{PlanType: "EC2Instance", HourlyCommitment: 1, Region: "eu-west-1", InstanceFamily: " "} + }), "sp all": mut(parserShapedSP(), func(r *common.Recommendation) { r.Service = common.ServiceSavingsPlansAll }), "ec2 nil": mut(validEC2Rec(), func(r *common.Recommendation) { r.Details = nil }), "ec2 tenancy": mut(validEC2Rec(), func(r *common.Recommendation) { r.Details.(*common.ComputeDetails).Tenancy = "" }),