From eeface5f375a4ca0e57a601042e87e8560259324 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 17:13:03 +0200 Subject: [PATCH 1/4] test(cli): cover the CSV real-purchase path and replay idempotency for RDS and ElastiCache Drive runCSVPurchases with isDryRun=false through the offline AWS fixture stub (anonymous credentials, never delegates, unknown operations fail the test). Nothing reaches a real Purchase API and runToolFromCSV is never called with ActualPurchase=true. Covers: report CommitmentID equals the id AWS returned (the fixture id differs from the request id), purchase requests carry the offering id and count, Purchase calls match Success=true rows, a failed purchase carries the service error text, the audit log has one row per purchase, and a second run with the bought RIs visible buys nothing for RDS (with a non-matching-RI control and a fail-closed variant). The aws fixture stub now records request forms per operation. Known gap pinned in the replay test: CSV rows carry no Provider, so the ElastiCache duplicate key lacks the elasticache: engine prefix and the second run buys ElastiCache again. Flip cacheBuys to 0 when fixed. Refs #1326 Co-Authored-By: Claude Opus 5.5 --- cmd/aws_fixture_stub_test.go | 4 +- cmd/csv_real_purchase_test.go | 301 ++++++++++++++++++++++++++++++++++ 2 files changed, 304 insertions(+), 1 deletion(-) create mode 100644 cmd/csv_real_purchase_test.go diff --git a/cmd/aws_fixture_stub_test.go b/cmd/aws_fixture_stub_test.go index cb1353fe3..8a989c3ea 100644 --- a/cmd/aws_fixture_stub_test.go +++ b/cmd/aws_fixture_stub_test.go @@ -26,6 +26,7 @@ type awsFixtureStub struct { t *testing.T mu sync.Mutex received map[string]int + forms map[string][]url.Values // request forms per "host operation", in arrival order fixtures map[string]awsFixture log *bytes.Buffer } @@ -81,6 +82,7 @@ func (s *awsFixtureStub) RoundTrip(r *http.Request) (*http.Response, error) { op := awsOperation(r, form) s.mu.Lock() s.received[r.URL.Host+" "+op]++ + s.forms[r.URL.Host+" "+op] = append(s.forms[r.URL.Host+" "+op], form) s.mu.Unlock() svc, _, _ := strings.Cut(r.URL.Host, ".") fx, ok := s.fixtures[svc+" "+op] @@ -108,7 +110,7 @@ func (s *awsFixtureStub) RoundTrip(r *http.Request) (*http.Response, error) { // no environment. func newAWSFixtureConfig(t *testing.T) (aws.Config, *awsFixtureStub) { t.Helper() - stub := &awsFixtureStub{t: t, received: map[string]int{}, fixtures: maps.Clone(awsFixtures), log: captureAppLog(t)} + stub := &awsFixtureStub{t: t, received: map[string]int{}, forms: map[string][]url.Values{}, fixtures: maps.Clone(awsFixtures), log: captureAppLog(t)} return aws.Config{ Region: "us-east-1", HTTPClient: &http.Client{Transport: stub}, diff --git a/cmd/csv_real_purchase_test.go b/cmd/csv_real_purchase_test.go new file mode 100644 index 000000000..85380973e --- /dev/null +++ b/cmd/csv_real_purchase_test.go @@ -0,0 +1,301 @@ +package main + +import ( + "bytes" + "context" + "encoding/json" + "fmt" + "net/http" + "net/url" + "os" + "path/filepath" + "slices" + "strings" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// These tests drive the CSV purchase loop (runCSVPurchases with isDryRun=false) +// for RDS and ElastiCache. Nothing here can buy anything: the aws.Config is a +// literal whose HTTP client is awsFixtureStub (never delegates, unknown +// operations fail the test) with anonymous credentials, and no test calls +// runToolFromCSV with ActualPurchase=true. + +const ( + rdsHost = "rds.us-east-1.amazonaws.com" + cacheHost = "elasticache.us-west-2.amazonaws.com" + rdsPurchase = "PurchaseReservedDBInstancesOffering" + cachePurch = "PurchaseReservedCacheNodesOffering" + rdsRespID = "aws-ri-rds-1" // differs from any generated request id + cacheRespID = "aws-ri-cache-1" + csvPurchases = `Service,Region,ResourceType,Engine,Deployment,Count,Term,PaymentOption +rds,us-east-1,db.t3.small,postgres,single-az,2,1yr,all-upfront +elasticache,us-west-2,cache.t3.micro,redis,,1,1yr,all-upfront +` +) + +func queryErr(code, msg string) awsFixture { + return awsFixture{status: http.StatusBadRequest, body: `` + + `Sender` + code + `` + msg + `fixture`} +} + +func rdsFixtures() map[string]awsFixture { + return map[string]awsFixture{ + "rds DescribeReservedDBInstancesOfferings": {body: `` + + `` + + `rds-offering-1db.t3.small` + + `31536000postgresqlAll Upfront` + + `false` + + `fixture`}, + "rds " + rdsPurchase: {body: `` + + `` + rdsRespID + `` + + `fixture` + + ``}, + } +} + +func cacheFixtures() map[string]awsFixture { + return map[string]awsFixture{ + "elasticache DescribeReservedCacheNodesOfferings": {body: `` + + `` + + `cache-offering-1cache.t3.micro` + + `31536000redisAll Upfront` + + `` + + `fixture`}, + "elasticache " + cachePurch: {body: `` + + `` + cacheRespID + `` + + `fixture` + + ``}, + } +} + +// replayFixtures answers the existing-commitment Describe calls with one active +// RI per service bought just now: the state a second run sees after run 1. +func replayFixtures(rdsClass, cacheType string) map[string]awsFixture { + now := time.Now().UTC().Format(time.RFC3339) + return map[string]awsFixture{ + "rds DescribeReservedDBInstances": {body: `` + + `` + rdsRespID + `` + + `` + rdsClass + `postgresqlfalse` + + `231536000` + now + `active` + + `` + + `fixture`}, + "elasticache DescribeReservedCacheNodes": {body: `` + + `` + cacheRespID + `` + + `` + cacheType + `redis1` + + `31536000` + now + `active` + + `fixture`}, + } +} + +type csvPurchaseEnv struct { + stub *awsFixtureStub + log *bytes.Buffer + denyHits *atomic.Int64 + dir string + run func(report string) // buys the CSV rows against the stub, report written to the given name +} + +// newCSVPurchaseEnv wires the stub with the RDS and ElastiCache offering and +// purchase fixtures. Account stays empty in the CSV; CallerAccountID is set +// because entering at runCSVPurchases skips withCallerAccount. +func newCSVPurchaseEnv(t *testing.T) *csvPurchaseEnv { + t.Helper() + origCfg := toolCfg + t.Cleanup(func() { toolCfg = origCfg }) + denyHits := isolateAWSEnv(t) + t.Setenv("DISABLE_PURCHASE_DELAY", "true") + + awsCfg, stub := newAWSFixtureConfig(t) + // purchasecfg.NewConfig swaps any other HTTPClient type for the real network. + _, isStd := awsCfg.HTTPClient.(*http.Client) + require.True(t, isStd, "the fixture transport must ride on *http.Client") + for k, v := range rdsFixtures() { + stub.fixtures[k] = v + } + for k, v := range cacheFixtures() { + stub.fixtures[k] = v + } + dir := t.TempDir() + csvPath := writeTestRecommendationsCSV(t, csvPurchases) + env := &csvPurchaseEnv{stub: stub, log: stub.log, denyHits: denyHits, dir: dir} + env.run = func(report string) { + recs, err := loadRecommendationsFromCSV(csvPath) + require.NoError(t, err) + cfg := Config{CallerAccountID: defaultTestAccount, CSVOutput: filepath.Join(dir, report), AuditLog: filepath.Join(dir, "audit.jsonl")} + require.NoError(t, runCSVPurchases(context.Background(), awsCfg, recs, false, cfg, "run-"+report)) + } + return env +} + +func (e *csvPurchaseEnv) report(t *testing.T, name string) map[string][]string { + t.Helper() + header, rows := readPurchaseReport(t, filepath.Join(e.dir, name)) + cols := map[string][]string{} + for _, col := range []string{"ResourceType", "CommitmentID", "Success", "Error"} { + cols[col] = reportColumn(t, header, rows, col) + } + return cols +} + +func (e *csvPurchaseEnv) auditStatuses(t *testing.T) []string { + t.Helper() + rows := auditRows(t, filepath.Join(e.dir, "audit.jsonl")) + out := make([]string, 0, len(rows)) + for _, row := range rows { + var m map[string]any + require.NoError(t, json.Unmarshal([]byte(row), &m)) + out = append(out, fmt.Sprint(m["status"])) + } + return out +} + +func (e *csvPurchaseEnv) purchaseForm(t *testing.T, hostOp string) url.Values { + t.Helper() + e.stub.mu.Lock() + defer e.stub.mu.Unlock() + require.Len(t, e.stub.forms[hostOp], 1) + return e.stub.forms[hostOp][0] +} + +func firstRunRequests(rdsPurchases, cachePurchases int) map[string]int { + return map[string]int{ + rdsHost + " DescribeReservedDBInstances": 1, + rdsHost + " DescribeReservedDBInstancesOfferings": 1, + rdsHost + " " + rdsPurchase: rdsPurchases, + cacheHost + " DescribeReservedCacheNodes": 1, + cacheHost + " DescribeReservedCacheNodesOfferings": 1, + cacheHost + " " + cachePurch: cachePurchases, + } +} + +// TestAWSFixturesHaveNoPurchaseOps is a tripwire: the shared default fixtures +// must never answer a purchase call, so a test that forgets to install its own +// purchase fixture fails instead of "succeeding". +func TestAWSFixturesHaveNoPurchaseOps(t *testing.T) { + for key := range awsFixtures { + assert.NotContains(t, key, "Purchase") + } +} + +func TestCSVRealPurchase_RDSAndElastiCache(t *testing.T) { + env := newCSVPurchaseEnv(t) + env.run("report1.csv") + + rep := env.report(t, "report1.csv") + assert.ElementsMatch(t, []string{"db.t3.small", "cache.t3.micro"}, rep["ResourceType"]) + assert.Equal(t, []string{"true", "true"}, rep["Success"]) + // The report id is the one AWS returned, not the generated id sent in the request. + assert.ElementsMatch(t, []string{rdsRespID, cacheRespID}, rep["CommitmentID"]) + assert.Equal(t, []string{"", ""}, rep["Error"]) + + rdsForm := env.purchaseForm(t, rdsHost+" "+rdsPurchase) + assert.Equal(t, "rds-offering-1", rdsForm.Get("ReservedDBInstancesOfferingId")) + assert.Equal(t, "2", rdsForm.Get("DBInstanceCount")) + assert.NotEmpty(t, rdsForm.Get("ReservedDBInstanceId")) + assert.NotEqual(t, rdsRespID, rdsForm.Get("ReservedDBInstanceId")) + cacheForm := env.purchaseForm(t, cacheHost+" "+cachePurch) + assert.Equal(t, "cache-offering-1", cacheForm.Get("ReservedCacheNodesOfferingId")) + assert.Equal(t, "1", cacheForm.Get("CacheNodeCount")) + assert.NotEmpty(t, cacheForm.Get("ReservedCacheNodeId")) + assert.NotEqual(t, cacheRespID, cacheForm.Get("ReservedCacheNodeId")) + + // Purchase calls == Success=true rows, and no operation outside the six expected. + env.stub.requireRequests(t, firstRunRequests(1, 1)) + assert.Equal(t, []string{"success", "success"}, env.auditStatuses(t)) + assert.Zero(t, env.denyHits.Load(), "nothing may reach the deny endpoint") +} + +func TestCSVRealPurchase_FailedPurchaseCarriesError(t *testing.T) { + env := newCSVPurchaseEnv(t) + env.stub.fixtures["elasticache "+cachePurch] = queryErr("ReservedCacheNodeQuotaExceeded", "cache quota exceeded by fixture") + env.run("report1.csv") + + rep := env.report(t, "report1.csv") + for i, typ := range rep["ResourceType"] { + switch typ { + case "db.t3.small": + assert.Equal(t, "true", rep["Success"][i]) + assert.Equal(t, rdsRespID, rep["CommitmentID"][i]) + case "cache.t3.micro": + assert.Equal(t, "false", rep["Success"][i]) + assert.Contains(t, rep["Error"][i], "cache quota exceeded by fixture") + assert.NotContains(t, rep["Error"][i], "account of the credentials is unknown", "must fail at the purchase, not the precondition") + assert.NotEqual(t, cacheRespID, rep["CommitmentID"][i], "no AWS id exists for a failed purchase") + default: + t.Fatalf("unexpected report row %q", typ) + } + } + // Both purchases were attempted; only the RDS one succeeded. + env.stub.requireRequests(t, firstRunRequests(1, 1)) + assert.ElementsMatch(t, []string{"success", "error"}, env.auditStatuses(t)) + assert.Zero(t, env.denyHits.Load()) +} + +// TestCSVReplay_SecondRunBuysNothing runs the same CSV twice. Run 2 sees the +// RIs run 1 bought through the Describe fixture and must not buy again; the +// non-matching and failing variants prove the zero is caused by the match. +func TestCSVReplay_SecondRunBuysNothing(t *testing.T) { + tests := []struct { + name string + replay map[string]awsFixture + rdsBuys int // purchases in run 2 + cacheBuys int + wantAdjusted string // dedup log line proving the rows were removed by the match + wantRefused bool + }{ + // KNOWN GAP: CSV rows carry no Provider, so recfilter builds the + // ElastiCache rec key without the "elasticache:" engine prefix and never + // matches the existing node (Provider=AWS on the rec suppresses it). + // cacheBuys must become 0 and the log line "1 instances → 0 instances" when fixed. + {name: "matching RIs suppress the repurchase", replay: replayFixtures("db.t3.small", "cache.t3.micro"), cacheBuys: 1, wantAdjusted: "2 instances → 0 instances"}, + {name: "RIs of another class do not suppress it", replay: replayFixtures("db.m5.large", "cache.m5.large"), rdsBuys: 1, cacheBuys: 1}, + {name: "a failing check refuses to purchase", replay: map[string]awsFixture{ + "rds DescribeReservedDBInstances": queryErr("AccessDenied", "describe denied by fixture"), + "elasticache DescribeReservedCacheNodes": queryErr("AccessDenied", "describe denied by fixture"), + }, wantRefused: true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + env := newCSVPurchaseEnv(t) + env.run("report1.csv") + env.stub.requireRequests(t, firstRunRequests(1, 1)) + + for k, v := range tt.replay { + env.stub.fixtures[k] = v + } + env.log.Reset() + env.run("report2.csv") + + want := firstRunRequests(1+tt.rdsBuys, 1+tt.cacheBuys) + want[rdsHost+" DescribeReservedDBInstances"] = 2 + want[cacheHost+" DescribeReservedCacheNodes"] = 2 + want[rdsHost+" DescribeReservedDBInstancesOfferings"] = 1 + tt.rdsBuys + want[cacheHost+" DescribeReservedCacheNodesOfferings"] = 1 + tt.cacheBuys + env.stub.requireRequests(t, want) + + logText := env.log.String() + assert.NotContains(t, logText, "Could not check for existing RIs") + assert.Equal(t, tt.wantRefused, strings.Contains(logText, "Refusing to purchase")) + if tt.wantAdjusted != "" { + assert.Contains(t, logText, tt.wantAdjusted, "the dedup must be what removed the rows") + } else { + assert.NotContains(t, logText, "Adjusted recommendations") + } + rows := tt.rdsBuys + tt.cacheBuys + if rows == 0 { + // Fully covered rows never reach the loop, so no report and no audit row. + _, err := os.Stat(filepath.Join(env.dir, "report2.csv")) + assert.ErrorIs(t, err, os.ErrNotExist) + } else { + assert.Equal(t, slices.Repeat([]string{"true"}, rows), env.report(t, "report2.csv")["Success"]) + } + assert.Len(t, env.auditStatuses(t), 2+rows) + assert.Zero(t, env.denyHits.Load()) + }) + } +} From 1a8ec40ff89bd1ed42ca3c18fc6e09c2f2b02851 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 17:37:21 +0200 Subject: [PATCH 2/4] fix(csv): stamp AWS as the provider on --input-csv recommendations parseCSVRecord in cmd/multi_service_csv.go left Recommendation.Provider empty. The shared duplicate check adds the "elasticache:" engine prefix to its key only for ProviderAWS, so a CSV ElastiCache row never matched the node bought by an earlier run and a replay bought it again. RDS was not affected because its key has no prefix. Set Provider to AWS where the row is built. The replay test now expects the second run to buy nothing for ElastiCache as well, and a parser test pins the provider. The Savings Plan round-trip test asserted the old empty provider; it now asserts AWS and the purchase flow is unchanged. One side effect: a CSV Savings Plan row with an empty Region is now matched by --include-regions and --exclude-regions on its plan region (Details.Region, via EffectiveRegion), as on the main path (#1582). Before it was matched on the empty string, so an included region dropped it and an excluded one leaked it. --regions is still not applied to CSV Savings Plans. Closes #1326 Closes #2228 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 +++++ cmd/csv_real_purchase_test.go | 18 +++++++++++----- cmd/multi_service_csv.go | 2 +- cmd/multi_service_filters.go | 6 +++--- cmd/multi_service_filters_test.go | 34 +++++++++++++++++++++++++++++++ cmd/savings_plan_purchase_test.go | 2 +- 6 files changed, 57 insertions(+), 10 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 42151f622..543658c3f 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,11 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - `--min-savings-pct` now rejects values below 0 or above 100 at validation. Previously a negative value was accepted as no filter and a value above 100 silently dropped every recommendation (#2161). +- Replaying or retrying an `--input-csv` purchase no longer re-buys an + ElastiCache reservation that the first run already bought: CSV rows now carry + the AWS provider, so the duplicate check matches them. CSV Savings Plan rows + also now match `--include-regions` and `--exclude-regions` on their plan + region (#2228). - `configure-gcp`: skipping (or giving an unknown answer at) the "Create Service Account" step no longer continues with the default `cudly-service-account@` email that was never created. The wizard now diff --git a/cmd/csv_real_purchase_test.go b/cmd/csv_real_purchase_test.go index 85380973e..f0dda6361 100644 --- a/cmd/csv_real_purchase_test.go +++ b/cmd/csv_real_purchase_test.go @@ -15,6 +15,7 @@ import ( "testing" "time" + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" ) @@ -248,11 +249,7 @@ func TestCSVReplay_SecondRunBuysNothing(t *testing.T) { wantAdjusted string // dedup log line proving the rows were removed by the match wantRefused bool }{ - // KNOWN GAP: CSV rows carry no Provider, so recfilter builds the - // ElastiCache rec key without the "elasticache:" engine prefix and never - // matches the existing node (Provider=AWS on the rec suppresses it). - // cacheBuys must become 0 and the log line "1 instances → 0 instances" when fixed. - {name: "matching RIs suppress the repurchase", replay: replayFixtures("db.t3.small", "cache.t3.micro"), cacheBuys: 1, wantAdjusted: "2 instances → 0 instances"}, + {name: "matching RIs suppress the repurchase", replay: replayFixtures("db.t3.small", "cache.t3.micro"), wantAdjusted: "instances → 0 instances"}, {name: "RIs of another class do not suppress it", replay: replayFixtures("db.m5.large", "cache.m5.large"), rdsBuys: 1, cacheBuys: 1}, {name: "a failing check refuses to purchase", replay: map[string]awsFixture{ "rds DescribeReservedDBInstances": queryErr("AccessDenied", "describe denied by fixture"), @@ -299,3 +296,14 @@ func TestCSVReplay_SecondRunBuysNothing(t *testing.T) { }) } } + +// The dedupe key adds the "elasticache:" engine prefix only for ProviderAWS, +// so a CSV row must carry its provider or replays re-buy ElastiCache (#2228). +func TestLoadRecommendationsFromCSV_StampsAWSProvider(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, csvPurchases)) + require.NoError(t, err) + require.Len(t, recs, 2) + for _, rec := range recs { + assert.Equal(t, common.ProviderAWS, rec.Provider, rec.Service) + } +} diff --git a/cmd/multi_service_csv.go b/cmd/multi_service_csv.go index ce2025d8a..08621807a 100644 --- a/cmd/multi_service_csv.go +++ b/cmd/multi_service_csv.go @@ -162,7 +162,7 @@ func parseCSVRecords(reader *csv.Reader, colIdx map[string]int) ([]common.Recomm // parseCSVRecord parses a single CSV record into a Recommendation. func parseCSVRecord(record []string, colIdx map[string]int) (common.Recommendation, error) { - rec := common.Recommendation{} + rec := common.Recommendation{Provider: common.ProviderAWS} // Parse string fields rec.Service = common.ServiceType(getCSVField(record, colIdx, "Service")) diff --git a/cmd/multi_service_filters.go b/cmd/multi_service_filters.go index 978c79546..7fabf29ca 100644 --- a/cmd/multi_service_filters.go +++ b/cmd/multi_service_filters.go @@ -133,9 +133,9 @@ func shouldIncludePoolSize(rec *common.Recommendation, cfg *Config) bool { // Plans are fetched once, not per region (determineRegionsForService), so // --regions never reaches them through the fetch loop. An unknown region // does not match, so it is dropped rather than bought outside the scope. -// Not applied in --input-csv mode: CSV rows carry no Provider or -// CommitmentType, so their Details.Region is not readable here, and the rows -// are used as supplied. +// In --input-csv mode, rows carry Provider (AWS) but no CommitmentType, so +// EffectiveRegion reads Details.Region for --include/--exclude-regions. +// --regions is not applied to CSV Savings Plans, which are used as supplied. func shouldIncludeRecommendationRegion(rec *common.Recommendation, cfg *Config) bool { if awsprovider.IsRegionAgnostic(*rec) { return true diff --git a/cmd/multi_service_filters_test.go b/cmd/multi_service_filters_test.go index 5c913376e..f533e4660 100644 --- a/cmd/multi_service_filters_test.go +++ b/cmd/multi_service_filters_test.go @@ -838,3 +838,37 @@ savings-plans-ec2instance,,,1,1yr,no-upfront,EC2Instance,1.5,m5,us-east-1 got := applyFilters(recs, &cfg, nil, nil, "", common.NewDropSummary()) assert.Len(t, got, 3) } + +// TestApplyFilters_SavingsPlansIncludeRegionsCSVMode pins that a CSV Savings +// Plan row with an empty Region is matched by --include/--exclude-regions on +// its plan region (Details.Region), as on the main path, while --regions alone +// still leaves it alone (#2228). +func TestApplyFilters_SavingsPlansIncludeRegionsCSVMode(t *testing.T) { + csvPath := filepath.Join(t.TempDir(), "sp.csv") + content := `Service,Region,ResourceType,Count,Term,PaymentOption,PlanType,HourlyCommitment,InstanceFamily,DetailsRegion +savings-plans-ec2instance,,,1,1yr,no-upfront,EC2Instance,1.5,m5,us-east-1 +` + require.NoError(t, os.WriteFile(csvPath, []byte(content), 0o600)) + recs, err := loadRecommendationsFromCSV(csvPath) + require.NoError(t, err) + require.Len(t, recs, 1) + require.Empty(t, recs[0].Region) + + tests := []struct { + name string + cfg Config + want int + }{ + {"include another region drops it", Config{IncludeRegions: []string{"eu-west-1"}}, 0}, + {"include its region keeps it", Config{IncludeRegions: []string{"us-east-1"}}, 1}, + {"exclude its region drops it", Config{ExcludeRegions: []string{"us-east-1"}}, 0}, + {"--regions alone does not drop it", Config{Regions: []string{"eu-west-1"}}, 1}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tt.cfg.CSVInput = csvPath + got := applyFilters(recs, &tt.cfg, nil, nil, "", common.NewDropSummary()) + assert.Len(t, got, tt.want) + }) + } +} diff --git a/cmd/savings_plan_purchase_test.go b/cmd/savings_plan_purchase_test.go index dbf095228..21d662a97 100644 --- a/cmd/savings_plan_purchase_test.go +++ b/cmd/savings_plan_purchase_test.go @@ -396,7 +396,7 @@ func TestProcessCSVRegionPurchases_SavingsPlanRowFromWrittenCSV(t *testing.T) { require.NoError(t, err) require.Len(t, recs, 1) require.Empty(t, recs[0].Region) - require.Empty(t, recs[0].Provider) + require.Equal(t, common.ProviderAWS, recs[0].Provider) _, results, ok, _ := processCSVRegionPurchases(context.Background(), awsCfg, recs[0].Service, recs[0].Region, recs, false, cfg, "run-1") From b10bbe8de3ea38c7bd0e043783fecede1660b51f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 18:13:10 +0200 Subject: [PATCH 3/4] fix(csv): keep the MemoryDB engine on CSV replays so dedupe matches csvDetails had no MemoryDB case, so MemoryDB CSV rows loaded with nil Details and an empty duplicate-check engine, which never matched an existing node's "redis". An --input-csv replay re-bought them. Build CacheDetails from the CSV engine column, as for ElastiCache, and cover run and replay through the real MemoryDB client against the stub. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 5 ++- cmd/csv_real_purchase_test.go | 76 +++++++++++++++++++++++++++++--- cmd/multi_service_csv_details.go | 2 +- 3 files changed, 74 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 543658c3f..0bea2aae9 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,8 +17,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). Previously a negative value was accepted as no filter and a value above 100 silently dropped every recommendation (#2161). - Replaying or retrying an `--input-csv` purchase no longer re-buys an - ElastiCache reservation that the first run already bought: CSV rows now carry - the AWS provider, so the duplicate check matches them. CSV Savings Plan rows + ElastiCache or MemoryDB reservation that the first run already bought: CSV + rows now carry the AWS provider, and MemoryDB rows keep their engine, so the + duplicate check matches them. CSV Savings Plan rows also now match `--include-regions` and `--exclude-regions` on their plan region (#2228). - `configure-gcp`: skipping (or giving an unknown answer at) the "Create Service diff --git a/cmd/csv_real_purchase_test.go b/cmd/csv_real_purchase_test.go index f0dda6361..80fd4ec6b 100644 --- a/cmd/csv_real_purchase_test.go +++ b/cmd/csv_real_purchase_test.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "maps" "net/http" "net/url" "os" @@ -105,6 +106,12 @@ type csvPurchaseEnv struct { // purchase fixtures. Account stays empty in the CSV; CallerAccountID is set // because entering at runCSVPurchases skips withCallerAccount. func newCSVPurchaseEnv(t *testing.T) *csvPurchaseEnv { + t.Helper() + return newCSVPurchaseEnvFor(t, csvPurchases, rdsFixtures(), cacheFixtures()) +} + +// newCSVPurchaseEnvFor is newCSVPurchaseEnv with its own CSV and fixtures. +func newCSVPurchaseEnvFor(t *testing.T, csv string, fixtureSets ...map[string]awsFixture) *csvPurchaseEnv { t.Helper() origCfg := toolCfg t.Cleanup(func() { toolCfg = origCfg }) @@ -115,14 +122,11 @@ func newCSVPurchaseEnv(t *testing.T) *csvPurchaseEnv { // purchasecfg.NewConfig swaps any other HTTPClient type for the real network. _, isStd := awsCfg.HTTPClient.(*http.Client) require.True(t, isStd, "the fixture transport must ride on *http.Client") - for k, v := range rdsFixtures() { - stub.fixtures[k] = v - } - for k, v := range cacheFixtures() { - stub.fixtures[k] = v + for _, set := range fixtureSets { + maps.Copy(stub.fixtures, set) } dir := t.TempDir() - csvPath := writeTestRecommendationsCSV(t, csvPurchases) + csvPath := writeTestRecommendationsCSV(t, csv) env := &csvPurchaseEnv{stub: stub, log: stub.log, denyHits: denyHits, dir: dir} env.run = func(report string) { recs, err := loadRecommendationsFromCSV(csvPath) @@ -307,3 +311,63 @@ func TestLoadRecommendationsFromCSV_StampsAWSProvider(t *testing.T) { assert.Equal(t, common.ProviderAWS, rec.Provider, rec.Service) } } + +const ( + memdbHost = "memory-db.eu-west-1.amazonaws.com" + memdbPurchase = "PurchaseReservedNodesOffering" + memdbRespID = "aws-ri-memdb-1" + memdbCSV = `Service,Region,ResourceType,Engine,Deployment,Count,Term,PaymentOption +memorydb,eu-west-1,db.t4g.small,redis,,3,1yr,all-upfront +` +) + +// memdbFixtures answers MemoryDB (awsJson1.1) with no existing node, one +// offering and a purchase; existing, when set, is the node a replay finds. +func memdbFixtures(existing bool) map[string]awsFixture { + nodes := `[]` + if existing { + nodes = fmt.Sprintf(`[{"ReservationId":%q,"NodeType":"db.t4g.small","NodeCount":3,"Duration":31536000,"StartTime":%d,"State":"active"}]`, + memdbRespID, time.Now().Unix()) + } + return map[string]awsFixture{ + "memory-db DescribeReservedNodes": {body: `{"ReservedNodes":` + nodes + `}`}, + "memory-db DescribeReservedNodesOfferings": {body: `{"ReservedNodesOfferings":[{"ReservedNodesOfferingId":"memdb-offering-1",` + + `"NodeType":"db.t4g.small","Duration":31536000,"OfferingType":"All Upfront"}]}`}, + "memory-db " + memdbPurchase: {body: `{"ReservedNode":{"ReservationId":"` + memdbRespID + `","NodeCount":3}}`}, + } +} + +// The loader must keep the MemoryDB engine column: the dedupe key of a CSV row +// is only equal to an existing node's ("redis") when Details carry it. +func TestCSVReplay_MemoryDB(t *testing.T) { + env := newCSVPurchaseEnvFor(t, memdbCSV, memdbFixtures(false)) + env.run("report1.csv") + assert.Equal(t, []string{"true"}, env.report(t, "report1.csv")["Success"]) + assert.Equal(t, []string{memdbRespID}, env.report(t, "report1.csv")["CommitmentID"]) + env.stub.requireRequests(t, map[string]int{ + memdbHost + " DescribeReservedNodes": 1, + memdbHost + " DescribeReservedNodesOfferings": 1, + memdbHost + " " + memdbPurchase: 1, + }) + + maps.Copy(env.stub.fixtures, memdbFixtures(true)) + env.log.Reset() + env.run("report2.csv") + env.stub.requireRequests(t, map[string]int{ + memdbHost + " DescribeReservedNodes": 2, + memdbHost + " DescribeReservedNodesOfferings": 1, + memdbHost + " " + memdbPurchase: 1, + }) + assert.Contains(t, env.log.String(), "instances → 0 instances", "the dedup must be what removed the row") + assert.NotContains(t, env.log.String(), "Could not check for existing RIs") + _, err := os.Stat(filepath.Join(env.dir, "report2.csv")) + assert.ErrorIs(t, err, os.ErrNotExist) + assert.Zero(t, env.denyHits.Load()) +} + +func TestLoadRecommendationsFromCSV_KeepsMemoryDBEngine(t *testing.T) { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, memdbCSV)) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, &common.CacheDetails{Engine: "redis", NodeType: "db.t4g.small"}, recs[0].Details) +} diff --git a/cmd/multi_service_csv_details.go b/cmd/multi_service_csv_details.go index 7c44e20ee..f3e00579b 100644 --- a/cmd/multi_service_csv_details.go +++ b/cmd/multi_service_csv_details.go @@ -60,7 +60,7 @@ func csvDetails(rec common.Recommendation, record []string, colIdx map[string]in AZConfig: getCSVField(record, colIdx, "Deployment"), InstanceClass: rec.ResourceType, }, nil - case common.ServiceElastiCache, common.ServiceCache: + case common.ServiceElastiCache, common.ServiceCache, common.ServiceMemoryDB: return &common.CacheDetails{Engine: engine, NodeType: rec.ResourceType}, nil case common.ServiceEC2, common.ServiceCompute: return &common.ComputeDetails{ From c3644f5499cec3c14600cecd55088bdd7dd0f362 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 16 Oct 2026 18:52:00 +0200 Subject: [PATCH 4/4] fix(csv): canonicalize the MemoryDB engine so valkey and blank rows dedupe MemoryDB reservations are engine-agnostic for purchase and the inventory reports every node as "redis", so a CSV row with engine valkey or blank missed the dedupe key and a replay re-bought it. Always use the client's ReservedNodeEngine for MemoryDB CSV rows (CodeRabbit thread on #2230, follow-up for cli#2232). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 ++++---- cmd/csv_real_purchase_test.go | 34 +++++++++++++++++++++----------- cmd/multi_service_csv_details.go | 9 ++++++++- 3 files changed, 35 insertions(+), 16 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0bea2aae9..80ccb6dc2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,10 +18,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). silently dropped every recommendation (#2161). - Replaying or retrying an `--input-csv` purchase no longer re-buys an ElastiCache or MemoryDB reservation that the first run already bought: CSV - rows now carry the AWS provider, and MemoryDB rows keep their engine, so the - duplicate check matches them. CSV Savings Plan rows - also now match `--include-regions` and `--exclude-regions` on their plan - region (#2228). + rows now carry the AWS provider, and MemoryDB rows use the engine AWS reports + for existing nodes whatever the CSV says, so the duplicate check matches + them. CSV Savings Plan rows also now match `--include-regions` and + `--exclude-regions` on their plan region (#2228). - `configure-gcp`: skipping (or giving an unknown answer at) the "Create Service Account" step no longer continues with the default `cudly-service-account@` email that was never created. The wizard now diff --git a/cmd/csv_real_purchase_test.go b/cmd/csv_real_purchase_test.go index 80fd4ec6b..3211c4f5f 100644 --- a/cmd/csv_real_purchase_test.go +++ b/cmd/csv_real_purchase_test.go @@ -316,11 +316,15 @@ const ( memdbHost = "memory-db.eu-west-1.amazonaws.com" memdbPurchase = "PurchaseReservedNodesOffering" memdbRespID = "aws-ri-memdb-1" - memdbCSV = `Service,Region,ResourceType,Engine,Deployment,Count,Term,PaymentOption -memorydb,eu-west-1,db.t4g.small,redis,,3,1yr,all-upfront -` ) +const memdbCSVHeader = "Service,Region,ResourceType,Engine,Deployment,Count,Term,PaymentOption\n" + +// memdbCSVWithEngine is the MemoryDB row with the given Engine cell. +func memdbCSVWithEngine(engine string) string { + return memdbCSVHeader + "memorydb,eu-west-1,db.t4g.small," + engine + ",,3,1yr,all-upfront\n" +} + // memdbFixtures answers MemoryDB (awsJson1.1) with no existing node, one // offering and a purchase; existing, when set, is the node a replay finds. func memdbFixtures(existing bool) map[string]awsFixture { @@ -337,10 +341,16 @@ func memdbFixtures(existing bool) map[string]awsFixture { } } -// The loader must keep the MemoryDB engine column: the dedupe key of a CSV row -// is only equal to an existing node's ("redis") when Details carry it. +// The dedupe key of a CSV row only equals an existing node's ("redis") when +// Details carry that engine, whatever the CSV Engine cell says. func TestCSVReplay_MemoryDB(t *testing.T) { - env := newCSVPurchaseEnvFor(t, memdbCSV, memdbFixtures(false)) + for _, engine := range []string{"redis", "valkey", "Redis", ""} { + t.Run("engine="+engine, func(t *testing.T) { testCSVReplayMemoryDB(t, memdbCSVWithEngine(engine)) }) + } +} + +func testCSVReplayMemoryDB(t *testing.T, csv string) { + env := newCSVPurchaseEnvFor(t, csv, memdbFixtures(false)) env.run("report1.csv") assert.Equal(t, []string{"true"}, env.report(t, "report1.csv")["Success"]) assert.Equal(t, []string{memdbRespID}, env.report(t, "report1.csv")["CommitmentID"]) @@ -365,9 +375,11 @@ func TestCSVReplay_MemoryDB(t *testing.T) { assert.Zero(t, env.denyHits.Load()) } -func TestLoadRecommendationsFromCSV_KeepsMemoryDBEngine(t *testing.T) { - recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, memdbCSV)) - require.NoError(t, err) - require.Len(t, recs, 1) - assert.Equal(t, &common.CacheDetails{Engine: "redis", NodeType: "db.t4g.small"}, recs[0].Details) +func TestLoadRecommendationsFromCSV_MemoryDBEngineIsCanonical(t *testing.T) { + for _, engine := range []string{"redis", "valkey", ""} { + recs, err := loadRecommendationsFromCSV(writeTestRecommendationsCSV(t, memdbCSVWithEngine(engine))) + require.NoError(t, err) + require.Len(t, recs, 1) + assert.Equal(t, &common.CacheDetails{Engine: "redis", NodeType: "db.t4g.small"}, recs[0].Details, "engine=%q", engine) + } } diff --git a/cmd/multi_service_csv_details.go b/cmd/multi_service_csv_details.go index f3e00579b..f948986b1 100644 --- a/cmd/multi_service_csv_details.go +++ b/cmd/multi_service_csv_details.go @@ -4,6 +4,7 @@ import ( "strconv" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/memorydb" "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/savingsplans" ) @@ -49,6 +50,12 @@ func csvDetails(rec common.Recommendation, record []string, colIdx map[string]in if _, isSP := savingsplans.PlanTypeForServiceType(rec.Service); isSP { return csvSavingsPlanDetails(record, colIdx) } + if rec.Service == common.ServiceMemoryDB { + // MemoryDB reservations are engine-agnostic for purchase and the + // inventory reports every node as ReservedNodeEngine, so the CSV engine + // (blank, valkey, ...) must not reach the dedupe key or a replay re-buys. + return &common.CacheDetails{Engine: memorydb.ReservedNodeEngine, NodeType: rec.ResourceType}, nil + } engine := getCSVField(record, colIdx, "Engine") if engine == "" { return nil, nil @@ -60,7 +67,7 @@ func csvDetails(rec common.Recommendation, record []string, colIdx map[string]in AZConfig: getCSVField(record, colIdx, "Deployment"), InstanceClass: rec.ResourceType, }, nil - case common.ServiceElastiCache, common.ServiceCache, common.ServiceMemoryDB: + case common.ServiceElastiCache, common.ServiceCache: return &common.CacheDetails{Engine: engine, NodeType: rec.ResourceType}, nil case common.ServiceEC2, common.ServiceCompute: return &common.ComputeDetails{