diff --git a/CHANGELOG.md b/CHANGELOG.md index 1a2f57d5e..bc81850c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -41,6 +41,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - `configure-azure` no longer creates the Entra app, client secret and role assignment when you press Enter at the create-service-principal prompt; Enter now skips, as in the GCP wizard (#1613). +- `--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 087f67638..5722d831e 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" @@ -179,3 +180,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 2dafc6cff..faae38293 100644 --- a/docs/cli/README.md +++ b/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. `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