From f123297883d09dfd9f92d0e64a9b3170299181e4 Mon Sep 17 00:00:00 2001 From: Astrid Gealer Date: Tue, 20 Oct 2026 01:20:24 +0100 Subject: [PATCH] fix(cli): validate --input-csv Service cells and bound Count Term and PaymentOption cells are already checked against allow-lists (#2145), but the Service cell was cast straight to a ServiceType. A --services alias such as "elasticsearch", "RDS", "rds " or the "savingsplans" umbrella loaded fine, then the purchase loop found no service client for it and skipped the row with "Service client not yet implemented", so the run quietly bought less than the file asked for. Service is now parsed with parseCSVEnum against the canonical slugs from getAllServices() (what the CSV report writes), failing the load with the row and column. Count is also capped at MaxReasonableInstances, the ceiling validateFlags already applies to --max-instances and --override-count. Closes #379 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 ++ cmd/multi_service_csv.go | 27 ++++++++- cmd/multi_service_csv_strict_test.go | 90 ++++++++++++++++++++++++++++ docs/cli/README.md | 2 +- 4 files changed, 121 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e1bd23a3..cde5c0630 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `--input-csv` now rejects a row whose `Service` cell is not one of the service + names the CSV report writes (for example the `--services` alias + `elasticsearch`, `RDS` or `rds` with a trailing space), and a `Count` above + 10000, failing the load with the row and column. Such a row used to load and + then be skipped mid-run with "Service client not yet implemented" (#379). - Savings Plans purchases now send the hourly commitment truncated to 5 decimals (at least 2 shown) instead of rounded to cents, and accept amounts from 0.001 to 1,000,000, so a 0.001 to 0.004 commitment is no longer rejected (library diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index 7e99e0c73..455083268 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -83,12 +83,26 @@ func loadCSVRecommendations(csvPath string, need csvThresholdNeeds) ([]common.Re var requiredCSVColumns = []string{"Service", "Region", "ResourceType", "Count", "Term", "PaymentOption"} // Accepted Term values match fetchRecommendationsForRegion; accepted -// PaymentOption values match validatePaymentAndTerm. +// PaymentOption values match validatePaymentAndTerm. Accepted Service values +// are the canonical slugs writeMultiServiceCSV emits and createServiceClient +// builds a client for; --services flag aliases (elasticsearch, savingsplans, +// savingsplans-compute, ...) are not accepted, since a row the purchase loop +// cannot route would otherwise be skipped mid-run instead of failing the load. var ( validCSVTerms = []string{"1yr", "3yr"} validCSVPaymentOptions = []string{"all-upfront", "partial-upfront", "no-upfront"} + validCSVServices = csvServiceNames() ) +func csvServiceNames() []string { + services := getAllServices() + names := make([]string, len(services)) + for i, s := range services { + names[i] = string(s) + } + return names +} + // parseCSVEnum requires the cell to be exactly one of allowed. func parseCSVEnum(record []string, colIdx map[string]int, fieldName string, allowed []string) (string, error) { col := colIdx[fieldName] + 1 @@ -174,8 +188,13 @@ func parseCSVRecords(reader *csv.Reader, colIdx map[string]int, need csvThreshol func parseCSVRecord(record []string, colIdx map[string]int, need csvThresholdNeeds) (common.Recommendation, error) { rec := common.Recommendation{Provider: common.ProviderAWS} + service, err := parseCSVEnum(record, colIdx, "Service", validCSVServices) + if err != nil { + return rec, err + } + rec.Service = common.ServiceType(service) + // Parse string fields - rec.Service = common.ServiceType(getCSVField(record, colIdx, "Service")) rec.Region = getCSVField(record, colIdx, "Region") rec.ResourceType = getCSVField(record, colIdx, "ResourceType") rec.Account = getCSVField(record, colIdx, "Account") @@ -243,6 +262,10 @@ func parseCSVCount(record []string, colIdx map[string]int, target *int) error { if n < 1 { return fmt.Errorf("column %d %q: must be at least 1, got %d", idx+1, fieldName, n) } + // Same ceiling validateFlags puts on --max-instances and --override-count. + if n > MaxReasonableInstances { + return fmt.Errorf("column %d %q: must be at most %d, got %d", idx+1, fieldName, MaxReasonableInstances, n) + } *target = n return nil } diff --git a/cmd/multi_service_csv_strict_test.go b/cmd/multi_service_csv_strict_test.go index 62ced1c8c..eb8aae5dc 100644 --- a/cmd/multi_service_csv_strict_test.go +++ b/cmd/multi_service_csv_strict_test.go @@ -3,6 +3,7 @@ package main import ( "context" "path/filepath" + "strconv" "testing" "github.com/stretchr/testify/assert" @@ -172,3 +173,92 @@ func TestLoadRecommendationsFromCSV_TermAndPaymentAccepted(t *testing.T) { } } } + +// Service cells must be one of the canonical service types the purchase loop +// builds a client for (#379, audit A10-007). A flag alias such as +// "elasticsearch", a padded or capitalized slug, or the "savingsplans" +// umbrella used to load as an unknown ServiceType, and the row was then +// skipped with "Service client not yet implemented" instead of failing. +func TestLoadRecommendationsFromCSV_ServiceValues(t *testing.T) { + tests := []struct { + name string + service string + }{ + {"empty", ""}, + {"whitespace only", " "}, + {"flag alias", "elasticsearch"}, + {"trailing space", "rds "}, + {"uppercase", "RDS"}, + {"savings plans umbrella", "savingsplans"}, + {"flag-only savings plans spelling", "savingsplans-compute"}, + {"unknown", "dynamodb"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + content := "Service,Region,ResourceType,Count,Term,PaymentOption\n" + + "rds,us-east-1,db.t3.micro,1,1yr,all-upfront\n" + + "\"" + tt.service + "\",us-east-1,db.t3.small,2,1yr,all-upfront\n" + err := loadCSVContent(t, content) + require.Error(t, err) + assert.Contains(t, err.Error(), "line 3") + assert.Contains(t, err.Error(), `column 1 "Service"`) + }) + } +} + +// Every service the tool supports still loads, so the report a run writes can +// be fed back through --input-csv. +func TestLoadRecommendationsFromCSV_ServiceAccepted(t *testing.T) { + for _, service := range getAllServices() { + t.Run(string(service), func(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, + "Service,Region,ResourceType,Count,Term,PaymentOption\n"+ + string(service)+",us-east-1,m5.large,1,1yr,no-upfront\n")) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, service, recs[0].Service) + }) + } +} + +// Count is bounded by MaxReasonableInstances, the same ceiling validateFlags +// puts on --max-instances and --override-count (#379). +func TestLoadRecommendationsFromCSV_CountUpperBound(t *testing.T) { + row := func(count int) string { + return "Service,Region,ResourceType,Count,Term,PaymentOption\n" + + "rds,us-east-1,db.t3.micro," + strconv.Itoa(count) + ",3yr,partial-upfront\n" + } + + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, row(MaxReasonableInstances))) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, MaxReasonableInstances, recs[0].Count) + + err = loadCSVContent(t, row(MaxReasonableInstances+1)) + require.Error(t, err) + assert.Contains(t, err.Error(), "line 2") + assert.Contains(t, err.Error(), `column 4 "Count"`) + assert.Contains(t, err.Error(), strconv.Itoa(MaxReasonableInstances)) +} + +// End to end: a row with a bad Service cell stops the run before any AWS call +// and before a report is written, rather than being dropped mid-run. +func TestRunToolFromCSV_RejectsUnknownService(t *testing.T) { + awsHits := isolateAWSEnv(t) + cfg := Config{ + CSVInput: writeTestRecommendationsCSV(t, "Service,Region,ResourceType,Count,EstimatedSavings,Term,PaymentOption\n"+ + "rds,us-east-1,db.t3.micro,1,100,1yr,all-upfront\n"+ + "elasticsearch,us-east-1,m5.large.search,1,100,1yr,all-upfront\n"), + CSVOutput: filepath.Join(t.TempDir(), "report.csv"), + AuditLog: filepath.Join(t.TempDir(), "audit.jsonl"), + ActualPurchase: false, + Coverage: 100, + } + err := runToolFromCSV(context.Background(), cfg) + require.Error(t, err) + assert.Contains(t, err.Error(), "failed to read CSV file: CSV line 3:") + assert.Contains(t, err.Error(), `column 1 "Service"`) + assert.Contains(t, err.Error(), `"elasticsearch"`) + assert.NoFileExists(t, cfg.CSVOutput) + assert.Zero(t, awsHits.Load()) +} diff --git a/docs/cli/README.md b/docs/cli/README.md index 8313ef871..5aea9e87b 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -90,7 +90,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. `Service` must be one of `rds`, `elasticache`, `ec2`, `opensearch`, `redshift`, `memorydb`, `savings-plans-compute`, `savings-plans-ec2instance`, `savings-plans-sagemaker` or `savings-plans-database` (the names the CSV report writes; `--services` aliases such as `elasticsearch` or `savingsplans` are rejected), `Count` must be a whole number from 1 to 10000, `Term` must be `1yr` or `3yr` and `PaymentOption` must be `all-upfront`, `partial-upfront` or `no-upfront` on every row; a blank or invalid value fails the load with the row number. CSVs with only the older four columns are rejected. | ### Authentication