From 3573ee5b0efe5c41d699393c7c3b6f17a1572dd1 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Mon, 19 Oct 2026 12:45:56 +0100 Subject: [PATCH 1/2] fix(csv): round-trip the EC2 AvailabilityZone and refuse zonal rows without one in the dry run The CSV report carried EC2 Tenancy and Scope but not AvailabilityZone, so a zonal EC2 row exported and replayed with --input-csv lost its AZ. Since the go pin bump (go#305) the library buys a zonal RI only in the rec's AZ and refuses a zonal rec without one, so the replayed row was refused at purchase time while validatePurchasePreconditions, which does not look at the AZ, let the dry run report success. Append an AvailabilityZone detail column after Scope (blank for regional rows and other services; columns are looked up by name, so older CSVs still load) and read it back into ComputeDetails. Mirror the library's scope/AZ check in the EC2 preconditions so the dry run refuses a zonal row with no AZ, or a regional row that names one, exactly as the real run does. Closes #2174 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 + cmd/csv_zonal_az_test.go | 242 +++++++++++++++++++++++++++++++ cmd/multi_service_csv_details.go | 18 ++- cmd/savings_plan_purchase.go | 36 ++++- docs/cli/README.md | 2 +- 5 files changed, 292 insertions(+), 12 deletions(-) create mode 100644 cmd/csv_zonal_az_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 68d8361c5..f5650621b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- Zonal EC2 rows now keep their Availability Zone through the CSV report and + `--input-csv`: reports gain an `AvailabilityZone` column, appended after + `Scope` (blank for regional rows and other services). A dry run now refuses a + zonal EC2 row without an AZ, or a regional row that names one, the same way + the real run does, instead of reporting success. A zonal row from a CSV + written before this column existed is refused until its AZ is added (#2174). - 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). diff --git a/cmd/csv_zonal_az_test.go b/cmd/csv_zonal_az_test.go new file mode 100644 index 000000000..0460efc0f --- /dev/null +++ b/cmd/csv_zonal_az_test.go @@ -0,0 +1,242 @@ +package main + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Issue #2174: a zonal EC2 row must keep its AvailabilityZone through the CSV +// export and --input-csv import, and a dry run must refuse the zonal rows the +// real run refuses. Every AWS call here goes to awsFixtureStub; nothing can buy. + +const ( + ec2Host = "ec2.us-east-1.amazonaws.com" + ec2Purchase = "PurchaseReservedInstancesOffering" + ec2ZonalRespID = "ec2-ri-zonal-1" + ec2Offering1a = "ec2-offering-1a" + ec2Offering1b = "ec2-offering-1b" + ec2FixtureXMLNS = `xmlns="http://ec2.amazonaws.com/doc/2016-11-15/"` +) + +func ec2ZonalOffering(id, az string) string { + return `` + id + `m5.large` + + `` + az + `31536000500.0` + + `0.0Linux/UNIXdefault` + + `USDPartial Upfrontconvertible` + + `Availability Zonefalse` +} + +// ec2ZonalFixtures answers the EC2 calls of one CSV purchase: an empty +// inventory, two zonal offerings (us-east-1a listed first, so buying from it +// would mean the AZ was lost), the purchase and the post-purchase tagging. +func ec2ZonalFixtures() map[string]awsFixture { + return map[string]awsFixture{ + "ec2 DescribeReservedInstances": {body: `fixture` + + ``}, + "ec2 DescribeReservedInstancesOfferings": {body: `` + + `fixture` + ec2ZonalOffering(ec2Offering1a, "us-east-1a") + + ec2ZonalOffering(ec2Offering1b, "us-east-1b") + ``}, + "ec2 " + ec2Purchase: {body: `fixture` + + `` + ec2ZonalRespID + ``}, + "ec2 CreateTags": {body: `fixturetrue`}, + } +} + +// exportCSV writes recs through the real report writer, the file a user would +// pass back with --input-csv, and returns its contents. +func exportCSV(t *testing.T, recs ...common.Recommendation) string { + t.Helper() + results := make([]common.PurchaseResult, 0, len(recs)) + for _, rec := range recs { + results = append(results, common.PurchaseResult{Recommendation: rec, DryRun: true, Success: true, Timestamp: time.Now()}) + } + path := filepath.Join(t.TempDir(), "export.csv") + require.NoError(t, writeMultiServiceCSVReport(results, path)) + data, err := os.ReadFile(path) + require.NoError(t, err) + return string(data) +} + +func TestCSVRoundTrip_ZonalEC2KeepsAvailabilityZone(t *testing.T) { + zonal := zonalEC2Rec(ec2types.ScopeAvailabilityZone, "us-east-1b") + zonal.EstimatedSavings = 20 + regional := zonalEC2Rec(ec2types.ScopeRegional, "") + regional.ResourceType = "c5.large" + regional.Details.(*common.ComputeDetails).InstanceType = "c5.large" + + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, exportCSV(t, zonal, regional))) + require.NoError(t, err) + require.Len(t, recs, 2) + + byType := map[string]*common.ComputeDetails{} + for _, rec := range recs { + d, ok := rec.Details.(*common.ComputeDetails) + require.True(t, ok, "%s: EC2 row must load ComputeDetails", rec.ResourceType) + byType[rec.ResourceType] = d + } + assert.Equal(t, "us-east-1b", byType["m5.large"].AvailabilityZone) + assert.Equal(t, string(ec2types.ScopeAvailabilityZone), byType["m5.large"].Scope) + assert.Empty(t, byType["c5.large"].AvailabilityZone, "a regional row must not gain an AZ") + assert.Equal(t, string(ec2types.ScopeRegional), byType["c5.large"].Scope) +} + +// TestCSVRealPurchase_ZonalEC2BuysInItsAZ is the write -> read -> purchase +// path: the exported zonal row is loaded with --input-csv semantics and bought +// through runCSVPurchases against the stub. Before the fix the AZ was dropped +// on export and the library refused the row before any offering lookup. +func TestCSVRealPurchase_ZonalEC2BuysInItsAZ(t *testing.T) { + env := newCSVPurchaseEnvFor(t, exportCSV(t, zonalEC2Rec(ec2types.ScopeAvailabilityZone, "us-east-1b")), ec2ZonalFixtures()) + env.run("report1.csv") + + rep := env.report(t, "report1.csv") + assert.Equal(t, []string{"m5.large"}, rep["ResourceType"]) + assert.Equal(t, []string{"true"}, rep["Success"]) + assert.Equal(t, []string{""}, rep["Error"]) + assert.Equal(t, []string{ec2ZonalRespID}, rep["CommitmentID"]) + + env.stub.mu.Lock() + describes := env.stub.forms[ec2Host+" DescribeReservedInstancesOfferings"] + env.stub.mu.Unlock() + require.NotEmpty(t, describes) + for _, form := range describes { + assert.Equal(t, "us-east-1b", form.Get("AvailabilityZone"), "the offering lookup must be pinned to the row's AZ") + } + buy := env.purchaseForm(t, ec2Host+" "+ec2Purchase) + assert.Equal(t, ec2Offering1b, buy.Get("ReservedInstancesOfferingId"), "must buy the us-east-1b offering, not the first listed") + assert.Equal(t, "1", buy.Get("InstanceCount")) + + header, rows := readPurchaseReport(t, filepath.Join(env.dir, "report1.csv")) + assert.Equal(t, []string{"us-east-1b"}, reportColumn(t, header, rows, "AvailabilityZone"), "the purchase report keeps the AZ too") + assert.Zero(t, env.denyHits.Load(), "nothing may reach the deny endpoint") +} + +// runCSVDryRun loads csvData and dry-runs it through runCSVPurchases against +// the stub, returning the report rows. +func runCSVDryRun(t *testing.T, csvData string) (header []string, rows [][]string, stub *awsFixtureStub) { + t.Helper() + origCfg := toolCfg + t.Cleanup(func() { toolCfg = origCfg }) + isolateAWSEnv(t) + awsCfg, stub := newAWSFixtureConfig(t) + for k, v := range ec2ZonalFixtures() { + if !strings.Contains(k, "Purchase") { + stub.fixtures[k] = v + } + } + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, csvData)) + require.NoError(t, err) + dir := t.TempDir() + cfg := Config{CallerAccountID: defaultTestAccount, CSVOutput: filepath.Join(dir, "dry.csv"), AuditLog: filepath.Join(dir, "audit.jsonl")} + require.NoError(t, runCSVPurchases(context.Background(), awsCfg, recs, true, cfg, "dry-run")) + header, rows = readPurchaseReport(t, cfg.CSVOutput) + return header, rows, stub +} + +func TestCSVDryRun_RefusesZonalEC2RowsTheRealRunRefuses(t *testing.T) { + const legacyHeader = "Service,Region,ResourceType,Engine,Count,Term,PaymentOption,Tenancy,Scope" + tests := []struct { + name string + csv string + wantErr string // "" means the dry run must succeed + }{ + { + name: "zonal row from a CSV without the AvailabilityZone column", + csv: legacyHeader + "\nec2,us-east-1,m5.large,Linux/UNIX,1,1yr,partial-upfront,default,Availability Zone\n", + wantErr: "zonal scope but no AvailabilityZone", + }, + { + name: "zonal row with a blank AvailabilityZone", + csv: legacyHeader + ",AvailabilityZone\nec2,us-east-1,m5.large,Linux/UNIX,1,1yr,partial-upfront,default,Availability Zone, \n", + wantErr: "zonal scope but no AvailabilityZone", + }, + { + name: "regional row that names an AZ", + csv: legacyHeader + ",AvailabilityZone\nec2,us-east-1,m5.large,Linux/UNIX,1,1yr,partial-upfront,default,Region,us-east-1b\n", + wantErr: `regional scope but AvailabilityZone "us-east-1b"`, + }, + { + name: "zonal row with its AZ", + csv: legacyHeader + ",AvailabilityZone\nec2,us-east-1,m5.large,Linux/UNIX,1,1yr,partial-upfront,default,Availability Zone,us-east-1b\n", + }, + { + name: "regional row without an AZ", + csv: legacyHeader + ",AvailabilityZone\nec2,us-east-1,m5.large,Linux/UNIX,1,1yr,partial-upfront,default,Region,\n", + }, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + header, rows, stub := runCSVDryRun(t, tt.csv) + require.Len(t, rows, 1) + success := reportColumn(t, header, rows, "Success")[0] + errCell := reportColumn(t, header, rows, "Error")[0] + if tt.wantErr == "" { + assert.Equal(t, "true", success) + assert.Empty(t, errCell) + } else { + assert.Equal(t, "false", success, "the dry run must not report success for a row the real run refuses") + assert.Contains(t, errCell, tt.wantErr) + } + stub.mu.Lock() + defer stub.mu.Unlock() + for k := range stub.received { + assert.NotContains(t, k, "Offerings", "a dry run makes no offering lookup") + assert.NotContains(t, k, ec2Purchase) + } + }) + } +} + +// TestDetailCells_AvailabilityZone covers both ComputeDetails shapes the +// writer accepts: the parser's pointer and a plain value. +func TestDetailCells_AvailabilityZone(t *testing.T) { + azCol := len(csvDetailColumns) - 1 + require.Equal(t, "AvailabilityZone", csvDetailColumns[azCol]) + ptr := zonalEC2Rec(ec2types.ScopeAvailabilityZone, "us-east-1b") + val := ptr + val.Details = *ptr.Details.(*common.ComputeDetails) + for name, rec := range map[string]common.Recommendation{"pointer": ptr, "value": val} { + cells := detailCells(rec) + assert.Equal(t, "us-east-1b", cells[azCol], name) + assert.Equal(t, string(ec2types.ScopeAvailabilityZone), cells[azCol-1], name) + } +} + +// TestValidatePurchasePreconditions_EC2AvailabilityZone pins the dry-run check +// to the library's refusal (providers/aws ec2 buildEC2OfferingQuery), including +// its legacy scope spellings. +func TestValidatePurchasePreconditions_EC2AvailabilityZone(t *testing.T) { + withScope := func(scope, az string) common.Recommendation { + return zonalEC2Rec(ec2types.Scope(scope), az) + } + ok := map[string]common.Recommendation{ + "zonal with AZ": withScope("Availability Zone", "us-east-1b"), + "legacy zonal spelling with AZ": withScope("availability-zone", "us-east-1b"), + "regional without AZ": withScope("Region", ""), + "regional with blank AZ": withScope("Region", " "), + } + for name, rec := range ok { + assert.NoError(t, validatePurchasePreconditions(rec, rec.Region, defaultTestAccount), name) + } + bad := map[string]common.Recommendation{ + "zonal without AZ": withScope("Availability Zone", ""), + "zonal with blank AZ": withScope("Availability Zone", " \t"), + "legacy zonal spelling no AZ": withScope("availability-zone", ""), + "lowercase zonal spelling no AZ": withScope("availability zone", ""), + "regional with AZ": withScope("Region", "us-east-1b"), + "lowercase regional with AZ": withScope("region", "us-east-1b"), + } + for name, rec := range bad { + err := validatePurchasePreconditions(rec, rec.Region, defaultTestAccount) + require.Error(t, err, name) + assert.Contains(t, err.Error(), "AvailabilityZone", name) + } +} diff --git a/cmd/multi_service_csv_details.go b/cmd/multi_service_csv_details.go index f948986b1..cdca72f31 100644 --- a/cmd/multi_service_csv_details.go +++ b/cmd/multi_service_csv_details.go @@ -10,10 +10,12 @@ import ( // csvDetailColumns are the report columns that carry the fields of a // recommendation's polymorphic Details that Engine/Deployment cannot: Savings -// Plan identity and EC2 tenancy and scope. They are appended after the -// original columns so existing readers, which look columns up by name, keep -// working. The writer and csvDetails below must stay in step. -var csvDetailColumns = []string{"PlanType", "HourlyCommitment", "InstanceFamily", "OfferingID", "DetailsRegion", "Tenancy", "Scope"} +// Plan identity and EC2 tenancy, scope and Availability Zone. They are +// appended after the original columns so existing readers, which look columns +// up by name, keep working. The writer and csvDetails below must stay in step. +// AvailabilityZone is set only for zonal EC2 rows: the library buys a zonal RI +// in that AZ and refuses one without it (#2174). +var csvDetailColumns = []string{"PlanType", "HourlyCommitment", "InstanceFamily", "OfferingID", "DetailsRegion", "Tenancy", "Scope", "AvailabilityZone"} // detailCells renders rec.Details for csvDetailColumns, in that order. func detailCells(rec common.Recommendation) []string { @@ -29,10 +31,10 @@ func detailCells(rec common.Recommendation) []string { } case *common.ComputeDetails: if d != nil { - cells[5], cells[6] = d.Tenancy, d.Scope + cells[5], cells[6], cells[7] = d.Tenancy, d.Scope, d.AvailabilityZone } case common.ComputeDetails: - cells[5], cells[6] = d.Tenancy, d.Scope + cells[5], cells[6], cells[7] = d.Tenancy, d.Scope, d.AvailabilityZone } return cells } @@ -75,6 +77,10 @@ func csvDetails(rec common.Recommendation, record []string, colIdx map[string]in Platform: engine, Tenancy: getCSVField(record, colIdx, "Tenancy"), Scope: getCSVField(record, colIdx, "Scope"), + // Blank for regional rows and for CSVs written before the column + // existed; validatePurchasePreconditions refuses a zonal row + // without it, as the library does. + AvailabilityZone: getCSVField(record, colIdx, "AvailabilityZone"), }, nil } return nil, nil diff --git a/cmd/savings_plan_purchase.go b/cmd/savings_plan_purchase.go index 1d37727b1..057d033d7 100644 --- a/cmd/savings_plan_purchase.go +++ b/cmd/savings_plan_purchase.go @@ -110,12 +110,38 @@ func validatePurchasePreconditions(rec common.Recommendation, region, callerAcco case common.IsSavingsPlan(rec.Service): return validateSavingsPlanPreconditions(rec) case rec.Service == common.ServiceEC2: - d, ok := rec.Details.(*common.ComputeDetails) - if !ok || d == nil { - return fmt.Errorf("EC2 recommendation for %s has no compute details (platform, tenancy, scope)", rec.ResourceType) + return validateEC2Preconditions(rec) + } + return nil +} + +func validateEC2Preconditions(rec common.Recommendation) error { + d, ok := rec.Details.(*common.ComputeDetails) + if !ok || d == nil { + return fmt.Errorf("EC2 recommendation for %s has no compute details (platform, tenancy, scope)", rec.ResourceType) + } + if d.Platform == "" || d.Tenancy == "" || d.Scope == "" { + return fmt.Errorf("EC2 recommendation for %s is missing platform, tenancy or scope (got %q, %q, %q)", rec.ResourceType, d.Platform, d.Tenancy, d.Scope) + } + return validateEC2AvailabilityZone(rec.ResourceType, d) +} + +// validateEC2AvailabilityZone mirrors the library's zonal check +// (buildEC2OfferingQuery): a zonal RI is bought in the recommendation's AZ, so +// a zonal rec without one is refused, and a regional rec that names an AZ is +// contradictory. Scope spellings match the library's parseEC2Scope. Without +// this a dry run of a zonal CSV row with no AvailabilityZone reported success +// while the real run refused it (#2174). +func validateEC2AvailabilityZone(resourceType string, d *common.ComputeDetails) error { + az := strings.TrimSpace(d.AvailabilityZone) + switch strings.ToLower(d.Scope) { + case "availability-zone", "availability zone": + if az == "" { + return fmt.Errorf("EC2 recommendation for %s has zonal scope but no AvailabilityZone; a zonal RI is bought in a specific AZ, so re-export the CSV with the AvailabilityZone column or re-fetch the recommendation", resourceType) } - if d.Platform == "" || d.Tenancy == "" || d.Scope == "" { - return fmt.Errorf("EC2 recommendation for %s is missing platform, tenancy or scope (got %q, %q, %q)", rec.ResourceType, d.Platform, d.Tenancy, d.Scope) + case "region": + if az != "" { + return fmt.Errorf("EC2 recommendation for %s has regional scope but AvailabilityZone %q; clear the AvailabilityZone or re-fetch the recommendation", resourceType, az) } } return nil diff --git a/docs/cli/README.md b/docs/cli/README.md index c79a8938c..a782f848f 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -88,7 +88,7 @@ All flags belong to the root command unless noted otherwise. | Flag | Short | Default | Description | |------|-------|---------|-------------| | `--output` | `-o` | auto-generated | Output CSV file path. Auto-name format: `ri-helper-dryrun-YYYYMMDD-HHMMSS.csv` or `ri-helper-purchase-YYYYMMDD-HHMMSS.csv`. An explicit path that already exists (file, directory or symlink) is refused instead of overwritten; choose a new path or remove the old file. The `Success` column is the raw flag the provider returned; a provider can report `true` together with an error, so read the `Error` column too (a row with an error counts as failed in the run summary). | -| `--input-csv` | `-i` | (none) | Read recommendations from an existing CSV instead of fetching from cloud APIs. The file must have a `.csv` extension and exist at the path given. The header must contain `Service`, `Region`, `ResourceType`, `Count`, `Term` and `PaymentOption`; a missing column is a load error that names it. `Term` must be `1yr` or `3yr` and `PaymentOption` must be `all-upfront`, `partial-upfront` or `no-upfront` on every row; a blank or unknown value fails the load with the row number. CSVs with only the older four columns are rejected. | +| `--input-csv` | `-i` | (none) | Read recommendations from an existing CSV instead of fetching from cloud APIs. The file must have a `.csv` extension and exist at the path given. The header must contain `Service`, `Region`, `ResourceType`, `Count`, `Term` and `PaymentOption`; a missing column is a load error that names it. `Term` must be `1yr` or `3yr` and `PaymentOption` must be `all-upfront`, `partial-upfront` or `no-upfront` on every row; a blank or unknown value fails the load with the row number. CSVs with only the older four columns are rejected. An EC2 row also needs `Engine` (platform), `Tenancy` and `Scope`; a zonal row (`Scope` = `Availability Zone`) must name its AZ in the optional `AvailabilityZone` column, the last column of the reports cudly writes, and a regional row must leave it blank. A zonal row without an AZ (for example from a CSV written before the column existed) is refused in both the dry run and the real run; re-export it or add the AZ. | ### Authentication From 427d481f1e608113d5c9461432febcd8399b40f1 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 14:48:16 +0100 Subject: [PATCH 2/2] fix(purchase): say "availability zone" in the CLI's EC2 scope refusals Main's TestPurchaseSingleRec_RefusesInconsistentEC2ScopeBeforeAnyAWSCall (from #2173) expects the refusal to mention "availability zone". This branch's pre-check now answers first, so its messages carry the phrase too. Co-Authored-By: Claude Opus 5.5 --- cmd/savings_plan_purchase.go | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/cmd/savings_plan_purchase.go b/cmd/savings_plan_purchase.go index fc4cda432..c61bd2332 100644 --- a/cmd/savings_plan_purchase.go +++ b/cmd/savings_plan_purchase.go @@ -142,11 +142,11 @@ func validateEC2AvailabilityZone(resourceType string, d *common.ComputeDetails) switch strings.ToLower(d.Scope) { case "availability-zone", "availability zone": if az == "" { - return fmt.Errorf("EC2 recommendation for %s has zonal scope but no AvailabilityZone; a zonal RI is bought in a specific AZ, so re-export the CSV with the AvailabilityZone column or re-fetch the recommendation", resourceType) + return fmt.Errorf("EC2 recommendation for %s has zonal scope but no AvailabilityZone; a zonal RI is bought in a specific availability zone, so re-export the CSV with the AvailabilityZone column or re-fetch the recommendation", resourceType) } case "region": if az != "" { - return fmt.Errorf("EC2 recommendation for %s has regional scope but AvailabilityZone %q; clear the AvailabilityZone or re-fetch the recommendation", resourceType, az) + return fmt.Errorf("EC2 recommendation for %s has regional scope but AvailabilityZone %q; a regional RI has no availability zone, so clear the AvailabilityZone or re-fetch the recommendation", resourceType, az) } } return nil