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..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" @@ -151,9 +152,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 == "" && 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 41d59a33d..af4f05e68 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,12 @@ 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"} + }), + "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 = "" }),