fix(csv): round-trip the EC2 AvailabilityZone and refuse zonal rows without one in the dry run

CHANGELOG.md

@@ -56,6 +56,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
   `BreakEvenMonths` columns are now parsed strictly (an invalid value fails the
   load) even when the threshold flags are unset. `--max-instances` still ranks
   on savings per instance (#1819).
+- 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).

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 `<item><reservedInstancesOfferingId>` + id + `</reservedInstancesOfferingId><instanceType>m5.large</instanceType>` +
+		`<availabilityZone>` + az + `</availabilityZone><duration>31536000</duration><fixedPrice>500.0</fixedPrice>` +
+		`<usagePrice>0.0</usagePrice><productDescription>Linux/UNIX</productDescription><instanceTenancy>default</instanceTenancy>` +
+		`<currencyCode>USD</currencyCode><offeringType>Partial Upfront</offeringType><offeringClass>convertible</offeringClass>` +
+		`<scope>Availability Zone</scope><marketplace>false</marketplace></item>`
+}
+
+// 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: `<DescribeReservedInstancesResponse ` + ec2FixtureXMLNS + `><requestId>fixture</requestId>` +
+			`<reservedInstancesSet/></DescribeReservedInstancesResponse>`},
+		"ec2 DescribeReservedInstancesOfferings": {body: `<DescribeReservedInstancesOfferingsResponse ` + ec2FixtureXMLNS + `>` +
+			`<requestId>fixture</requestId><reservedInstancesOfferingsSet>` + ec2ZonalOffering(ec2Offering1a, "us-east-1a") +
+			ec2ZonalOffering(ec2Offering1b, "us-east-1b") + `</reservedInstancesOfferingsSet></DescribeReservedInstancesOfferingsResponse>`},
+		"ec2 " + ec2Purchase: {body: `<PurchaseReservedInstancesOfferingResponse ` + ec2FixtureXMLNS + `><requestId>fixture</requestId>` +
+			`<reservedInstancesId>` + ec2ZonalRespID + `</reservedInstancesId></PurchaseReservedInstancesOfferingResponse>`},
+		"ec2 CreateTags": {body: `<CreateTagsResponse ` + ec2FixtureXMLNS + `><requestId>fixture</requestId><return>true</return></CreateTagsResponse>`},
+	}
+}
+
+// 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)
+	}
+}

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

cmd/savings_plan_purchase.go

@@ -115,12 +115,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 availability zone, 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; a regional RI has no availability zone, so clear the AvailabilityZone or re-fetch the recommendation", resourceType, az)
 		}
 	}
 	return nil

docs/cli/README.md

@@ -91,7 +91,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