From c0a268d1b631ca92b626595a53ae269fcbf89e94 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 15:27:39 +0200 Subject: [PATCH 1/3] fix(deps): adopt reviewed cloud-commitments-go purchase safeguards Bump the AWS and GCP cloud-commitments-go provider modules to v0.0.0-20261006205158-7ff8c1aee1bb. main already pins pkg at 90e61e668b99, Azure at 58c25f04c49b and AWS at 945a4045d11f, all of which contain the reviewed commit a32fd1a178e9 (#155-#158); both 7ff8c1aee1bb providers require pkg 90e61e668b99, so pkg and Azure stay. The GCP bump brings the commitment-family dedupe (#155) and the later SKU, amount, currency and idempotency-token validation fixes. The AWS bump brings adopted-commitment flagging, EC2 tenancy/scope validation, the fractional RI quantity warning and Organizations paging error propagation. Add consumer regression tests through the CLI's own seams (checkDuplicates, NewDuplicateChecker, ApplyTargetCoverage, executePurchase, executePurchasePipeline) with fake cloud boundaries. The GCP family dedupe test fails with GCP ce9513612901 and passes with the new pin; the others guard the dedupe, sizing, explicit-zero vs absent purchase-cost and fail-closed interruption contracts that the CLI relies on. Closes #2121 --- cmd/gcp_cud_dedupe_2121_test.go | 131 ++++++++++ cmd/purchase_safeguards_2121_test.go | 346 +++++++++++++++++++++++++++ go.mod | 4 +- go.sum | 8 +- 4 files changed, 483 insertions(+), 6 deletions(-) create mode 100644 cmd/gcp_cud_dedupe_2121_test.go create mode 100644 cmd/purchase_safeguards_2121_test.go diff --git a/cmd/gcp_cud_dedupe_2121_test.go b/cmd/gcp_cud_dedupe_2121_test.go new file mode 100644 index 000000000..0c9db566c --- /dev/null +++ b/cmd/gcp_cud_dedupe_2121_test.go @@ -0,0 +1,131 @@ +package main + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/LeanerCloud/cloud-commitments-go/providers/gcp/services/computeengine" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// Consumer regression test for issue #2121, library #155: a recently +// purchased GCP committed-use discount suppresses retries for the same +// commitment family. The CLI reaches this through its duplicate-purchase +// filter (NewDuplicateChecker in helpers.go), which dispatches to the +// provider client's recent-commitment filter when the pinned library +// provides one. +// +// The test holds the pinned library's real *computeengine.Client behind an +// interface probe instead of naming the method statically: with the pre-#155 +// pins the type does not implement the filter, the probe fails at runtime, +// and this test fails; with the new pins the real family-level filter runs. +// Either way the file compiles, so the sibling pin-sensitive tests keep +// running under both pin sets. + +// gcpRecentCommitmentFilter mirrors the unexported recfilter dispatch +// interface so the probe can detect whether the pinned library client +// implements the family-level suppression. +type gcpRecentCommitmentFilter interface { + FilterRecommendationsForRecentCommitments(recs []common.Recommendation, existing []common.Commitment) (passed, filtered []common.Recommendation, err error) +} + +// fakeGCPServiceClient is a provider.ServiceClient whose existing +// commitments are fixed and whose recent-commitment filter delegates to the +// pinned library's real compute engine client, so the family-matching logic +// under test is the library's, not a reimplementation. +type fakeGCPServiceClient struct { + libraryClient any + commitments []common.Commitment +} + +func (f *fakeGCPServiceClient) GetServiceType() common.ServiceType { return common.ServiceCompute } +func (f *fakeGCPServiceClient) GetRegion() string { return "us-central1" } + +func (f *fakeGCPServiceClient) GetRecommendations(context.Context, *common.RecommendationParams) ([]common.Recommendation, error) { + return nil, errors.New("not used by this test") +} + +func (f *fakeGCPServiceClient) GetExistingCommitments(context.Context) ([]common.Commitment, error) { + return f.commitments, nil +} + +func (f *fakeGCPServiceClient) PurchaseCommitment(context.Context, common.Recommendation, common.PurchaseOptions) (common.PurchaseResult, error) { + return common.PurchaseResult{}, errors.New("purchase must never be attempted by the duplicate checker") +} + +func (f *fakeGCPServiceClient) ValidateOffering(context.Context, common.Recommendation) error { + return errors.New("not used by this test") +} + +func (f *fakeGCPServiceClient) GetOfferingDetails(context.Context, common.Recommendation) (*common.OfferingDetails, error) { + return nil, errors.New("not used by this test") +} + +func (f *fakeGCPServiceClient) GetValidResourceTypes(context.Context) ([]string, error) { + return nil, errors.New("not used by this test") +} + +func (f *fakeGCPServiceClient) FilterRecommendationsForRecentCommitments(recs []common.Recommendation, existing []common.Commitment) ([]common.Recommendation, []common.Recommendation, error) { + filter, ok := f.libraryClient.(gcpRecentCommitmentFilter) + if !ok { + return nil, nil, errors.New("pinned gcp provider client has no recent-commitment filter (pre-#155 library)") + } + return filter.FilterRecommendationsForRecentCommitments(recs, existing) +} + +var _ provider.ServiceClient = (*fakeGCPServiceClient)(nil) + +// TestDuplicateChecker_RecentGPCCUDSuppressesFamilyRetry_2121 drives the +// CLI's duplicate-purchase seam with a recent GCP CUD record: an n2 +// recommendation for the same account and region as the recent +// GENERAL_PURPOSE_N2 commitment is suppressed (a retry of the purchase that +// already happened), while an n4 recommendation in the same region belongs +// to a different commitment family and still passes. With the old pins the +// gcp client implements no such filter and the generic per-resource-type +// matching lets the n2 retry through. +func TestDuplicateChecker_RecentGPCCUDSuppressesFamilyRetry_2121(t *testing.T) { + ctx := context.Background() + + recentCUD := common.Commitment{ + Provider: common.ProviderGCP, + Account: "proj-1", + CommitmentID: "cud-recent-1", + CommitmentType: common.CommitmentCUD, + Service: common.ServiceCompute, + Region: "us-central1", + ResourceType: "GENERAL_PURPOSE_N2", // what GetExistingCommitments reports: the commitment Type + Count: 8, + State: common.CommitmentStateActive, + StartDate: time.Now().Add(-2 * time.Hour), + } + cudRec := func(machineType string) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderGCP, + Account: "proj-1", + Service: common.ServiceCompute, + CommitmentType: common.CommitmentCUD, + Region: "us-central1", + ResourceType: machineType, + Count: 2, + } + } + + client := &fakeGCPServiceClient{ + libraryClient: &computeengine.Client{}, // zero value: the filter uses only its arguments + commitments: []common.Commitment{recentCUD}, + } + + recs := []common.Recommendation{cudRec("n2-standard-4"), cudRec("n4-standard-4")} + passed, filtered, err := NewDuplicateChecker(0).AdjustRecommendationsForExisting(ctx, recs, client) + + require.NoError(t, err) + require.Len(t, filtered, 1, "recent GENERAL_PURPOSE_N2 purchase must suppress the n2 retry") + assert.Equal(t, "n2-standard-4", filtered[0].ResourceType) + require.Len(t, passed, 1, "a different commitment family must not be suppressed") + assert.Equal(t, "n4-standard-4", passed[0].ResourceType) +} diff --git a/cmd/purchase_safeguards_2121_test.go b/cmd/purchase_safeguards_2121_test.go new file mode 100644 index 000000000..c8e31aaac --- /dev/null +++ b/cmd/purchase_safeguards_2121_test.go @@ -0,0 +1,346 @@ +package main + +import ( + "context" + "encoding/json" + "errors" + "os" + "path/filepath" + "strings" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Consumer regression tests for issue #2121 (adopt reviewed +// cloud-commitments-go purchase safeguards). Each test names the library +// issue it pins down and the CLI seam it exercises. The pin-sensitive ones +// fail with the pre-#2121 library pins and pass with +// v0.0.0-20261006205158-7ff8c1aee1bb; the proof lives in the PR body. + +// TestApplyTargetCoverage_NonPositiveCountDropped_2121 covers library #221 +// through the cmd ApplyTargetCoverage wrapper (helpers.go). A recommendation +// with no positive Count has no per-unit cost, so the safeguard drops it +// instead of scaling its money fields by an invented ratio. With the old +// pins a Count=0 RI rec was kept and its costs multiplied by nTarget. +func TestApplyTargetCoverage_NonPositiveCountDropped_2121(t *testing.T) { + for _, count := range []int{0, -1} { + rec := common.Recommendation{ + Provider: common.ProviderAWS, + Service: common.ServiceRDS, + Region: "us-east-1", + ResourceType: "db.r6g.large", + CommitmentType: common.CommitmentReservedInstance, + Count: count, + AverageInstancesUsedPerHour: 4, + ExistingCoveragePct: 0, + CommitmentCost: 1000, + OnDemandCost: 2000, + EstimatedSavings: 500, + } + drops := common.NewDropSummary() + out := ApplyTargetCoverage([]common.Recommendation{rec}, 75, drops) + + assert.Empty(t, out, "count=%d must be dropped, not scaled", count) + assert.Contains(t, drops.FormatOneLine(), "target-input-invalid=1", "count=%d", count) + } +} + +// TestApplyTargetCoverage_InvalidInputsDropped_2121 covers library #204 +// through the cmd ApplyTargetCoverage wrapper. A negative existing coverage +// makes the sized count meaningless (the gap exceeds the target), so the rec +// is dropped rather than scaled up past what AWS proposed. With the old pins +// this rec was kept and scaled by gap/target arithmetic. +func TestApplyTargetCoverage_InvalidInputsDropped_2121(t *testing.T) { + rec := common.Recommendation{ + Provider: common.ProviderAWS, + Service: common.ServiceRDS, + Region: "us-east-1", + ResourceType: "db.r6g.large", + CommitmentType: common.CommitmentReservedInstance, + Count: 2, + AverageInstancesUsedPerHour: 4, + ExistingCoveragePct: -10, + CommitmentCost: 1000, + OnDemandCost: 2000, + EstimatedSavings: 500, + } + drops := common.NewDropSummary() + out := ApplyTargetCoverage([]common.Recommendation{rec}, 75, drops) + + assert.Empty(t, out, "negative existing coverage must drop the rec") + assert.Contains(t, drops.FormatOneLine(), "target-input-invalid=1") +} + +// TestDuplicateChecker_ValkeyDoesNotCoverRedis_2121 covers library #158/#208 +// through cmd's NewDuplicateChecker (helpers.go) and the CLI's +// recommendation-to-purchase duplicate filter. Redis OSS reservations cover +// Valkey nodes, never the reverse, so a recent Valkey reservation must not +// suppress a Redis recommendation. With the old pins Valkey was folded into +// Redis and the Redis rec was wrongly suppressed. +func TestDuplicateChecker_ValkeyDoesNotCoverRedis_2121(t *testing.T) { + ctx := context.Background() + recent := time.Now().Add(-1 * time.Hour) + + commitment := func(engine string) common.Commitment { + return common.Commitment{ + Provider: common.ProviderAWS, + Service: common.ServiceCache, + ResourceType: "cache.r6g.large", + Region: "us-east-1", + Engine: engine, + Count: 1, + State: common.CommitmentStateActive, + StartDate: recent, + } + } + rec := func(engine string) common.Recommendation { + return common.Recommendation{ + Provider: common.ProviderAWS, + Service: common.ServiceElastiCache, + ResourceType: "cache.r6g.large", + Region: "us-east-1", + Count: 1, + Details: &common.CacheDetails{Engine: engine}, + } + } + + t.Run("recent valkey reservation keeps redis recommendation", func(t *testing.T) { + mockClient := &MockServiceClient{} + mockClient.On("GetExistingCommitments", ctx). + Return([]common.Commitment{commitment("valkey")}, nil) + + passed, filtered, err := NewDuplicateChecker(0). + AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec("redis")}, mockClient) + + require.NoError(t, err) + require.Len(t, passed, 1, "valkey reservation must not suppress a redis recommendation") + assert.Equal(t, 1, passed[0].Count) + assert.Empty(t, filtered) + }) + + t.Run("recent redis reservation still suppresses valkey recommendation", func(t *testing.T) { + mockClient := &MockServiceClient{} + mockClient.On("GetExistingCommitments", ctx). + Return([]common.Commitment{commitment("redis")}, nil) + + passed, filtered, err := NewDuplicateChecker(0). + AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec("valkey")}, mockClient) + + require.NoError(t, err) + assert.Empty(t, passed, "redis OSS reservations cover valkey nodes") + require.Len(t, filtered, 1) + }) + + t.Run("recent reservation with missing engine still wildcards", func(t *testing.T) { + mockClient := &MockServiceClient{} + mockClient.On("GetExistingCommitments", ctx). + Return([]common.Commitment{commitment("")}, nil) + + passed, filtered, err := NewDuplicateChecker(0). + AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec("redis")}, mockClient) + + require.NoError(t, err) + assert.Empty(t, passed, "a reservation with unknown engine matches any cache engine") + require.Len(t, filtered, 1) + }) +} + +// TestDuplicateChecker_UnknownReservationStateRemainsOwned_2121 covers the +// CLI-side half of library #156: the duplicate checker keeps commitments in +// an unrecognized state as owned (fail closed against double purchases), +// while states known to release capacity do not suppress. The matching +// provider-side change (#156, providers/aws internal reservationstate) is +// only reachable through live AWS APIs and is covered in the provider repo. +func TestDuplicateChecker_UnknownReservationStateRemainsOwned_2121(t *testing.T) { + ctx := context.Background() + recs := []common.Recommendation{{ + Provider: common.ProviderAWS, + Service: common.ServiceRDS, + ResourceType: "db.r6g.large", + Region: "us-east-1", + Count: 1, + Details: &common.DatabaseDetails{Engine: "mysql"}, + }} + commitment := func(state common.CommitmentState) common.Commitment { + return common.Commitment{ + Provider: common.ProviderAWS, + Service: common.ServiceRDS, + ResourceType: "db.r6g.large", + Region: "us-east-1", + Engine: "mysql", + Count: 1, + State: state, + StartDate: time.Now().Add(-1 * time.Hour), + } + } + + t.Run("unrecognized state suppresses", func(t *testing.T) { + mockClient := &MockServiceClient{} + mockClient.On("GetExistingCommitments", ctx). + Return([]common.Commitment{commitment("some-future-state")}, nil) + + passed, filtered, err := NewDuplicateChecker(0). + AdjustRecommendationsForExisting(ctx, recs, mockClient) + + require.NoError(t, err) + assert.Empty(t, passed, "unknown states stay owned: skipping a purchase is recoverable, a duplicate is not") + require.Len(t, filtered, 1) + }) + + t.Run("retired state does not suppress", func(t *testing.T) { + mockClient := &MockServiceClient{} + mockClient.On("GetExistingCommitments", ctx). + Return([]common.Commitment{commitment(common.CommitmentStateRetired)}, nil) + + passed, filtered, err := NewDuplicateChecker(0). + AdjustRecommendationsForExisting(ctx, recs, mockClient) + + require.NoError(t, err) + require.Len(t, passed, 1) + assert.Empty(t, filtered) + }) +} + +// TestExecutePurchase_PreservesExplicitZeroVsAbsentCost_2121 covers library +// #157 at the CLI purchase dispatch (executePurchase in +// multi_service_helpers.go): PurchaseResult.Cost is the total upfront charge, +// a non-nil zero means "no upfront charge" and nil means "unknown". The CLI +// must pass both through untouched, and the two must stay distinguishable in +// the JSON form downstream consumers (audit tooling, MCP) read. +func TestExecutePurchase_PreservesExplicitZeroVsAbsentCost_2121(t *testing.T) { + ctx := context.Background() + rec := common.Recommendation{ + Provider: common.ProviderAWS, + Service: common.ServiceEC2, + ResourceType: "m6i.large", + Region: "us-east-1", + Count: 2, + } + + t.Run("explicit zero upfront cost stays a non-nil zero", func(t *testing.T) { + zero := 0.0 + mockClient := &MockServiceClient{} + mockClient.On("PurchaseCommitment", ctx, rec, mock.Anything). + Return(common.PurchaseResult{Recommendation: rec, Success: true, Cost: &zero}, nil) + + result := executePurchase(ctx, rec, rec.Region, 1, mockClient, toolCfg) + + require.True(t, result.Success) + require.NotNil(t, result.Cost, "explicit zero must not collapse to absent") + assert.Equal(t, 0.0, *result.Cost) + + data, err := json.Marshal(result) + require.NoError(t, err) + assert.Contains(t, string(data), `"cost":0`) + }) + + t.Run("absent upfront cost stays nil", func(t *testing.T) { + mockClient := &MockServiceClient{} + mockClient.On("PurchaseCommitment", ctx, rec, mock.Anything). + Return(common.PurchaseResult{Recommendation: rec, Success: true, Cost: nil}, nil) + + result := executePurchase(ctx, rec, rec.Region, 1, mockClient, toolCfg) + + require.True(t, result.Success) + assert.Nil(t, result.Cost, "unknown cost must not be invented as zero") + + data, err := json.Marshal(result) + require.NoError(t, err) + assert.Contains(t, string(data), `"cost":null`) + }) +} + +// TestWritePurchaseAuditRecord_ZeroAndUnknownCost_2121 checks the CLI output +// path (writePurchaseAuditRecord in multi_service.go): a successful purchase +// with an explicit-zero or an absent upfront cost is audited as a success, +// and the fail-closed error case is audited as an error, so a re-driven run +// reconciles against the right per-recommendation outcome. +func TestWritePurchaseAuditRecord_ZeroAndUnknownCost_2121(t *testing.T) { + auditPath := filepath.Join(t.TempDir(), "audit.jsonl") + rec := common.Recommendation{ + Provider: common.ProviderAWS, + Service: common.ServiceRDS, + ResourceType: "db.r6g.large", + Region: "us-east-1", + Count: 1, + } + zero := 0.0 + + cases := []struct { + name string + result common.PurchaseResult + status string + }{ + {"explicit zero cost success", common.PurchaseResult{Recommendation: rec, Success: true, CommitmentID: "ri-zero", Cost: &zero}, "success"}, + {"absent cost success", common.PurchaseResult{Recommendation: rec, Success: true, CommitmentID: "ri-unknown"}, "success"}, + {"provider error stays fail-closed", common.PurchaseResult{Recommendation: rec, Success: false, Error: errors.New("throttling")}, "error"}, + } + + for _, tc := range cases { + writePurchaseAuditRecord("run-2121", rec, tc.result, tc.status, false, auditPath) + } + + data, err := os.ReadFile(auditPath) //nolint:gosec // G304: test-controlled temp path + require.NoError(t, err) + lines := strings.Split(strings.TrimSpace(string(data)), "\n") + require.Len(t, lines, len(cases)) + for i, tc := range cases { + var record common.AuditRecord + require.NoError(t, json.Unmarshal([]byte(lines[i]), &record), tc.name) + assert.Equal(t, tc.status, record.Status, tc.name) + assert.Equal(t, "run-2121", record.RunID, tc.name) + } +} + +// TestExecutePurchasePipeline_InterruptsFailClosed_2121 checks the +// interruption half of the fail-closed contract: once shutdown is requested, +// the purchase pipeline stops before the next recommendation instead of +// driving further purchases, and an uninterrupted dry run audits every rec. +// Dry-run mode only: no service clients are created and nothing is purchased. +func TestExecutePurchasePipeline_InterruptsFailClosed_2121(t *testing.T) { + ctx := context.Background() + recs := []common.Recommendation{ + {Provider: common.ProviderAWS, Service: common.ServiceRDS, ResourceType: "db.r6g.large", Region: "us-east-1", Count: 1}, + {Provider: common.ProviderAWS, Service: common.ServiceRDS, ResourceType: "db.r6g.xlarge", Region: "us-east-1", Count: 2}, + } + + t.Run("shutdown before start purchases nothing", func(t *testing.T) { + auditPath := filepath.Join(t.TempDir(), "audit.jsonl") + shutdownRequested.Store(true) + defer shutdownRequested.Store(false) + + results := executePurchasePipeline(ctx, aws.Config{}, recs, true, "run-2121-int", Config{AuditLog: auditPath}) + + assert.Empty(t, results, "a requested shutdown must stop the pipeline before the first purchase") + _, err := os.Stat(auditPath) + assert.True(t, os.IsNotExist(err), "no audit records without purchase attempts") + }) + + t.Run("uninterrupted dry run audits every rec as skipped", func(t *testing.T) { + auditPath := filepath.Join(t.TempDir(), "audit.jsonl") + results := executePurchasePipeline(ctx, aws.Config{}, recs, true, "run-2121-dry", Config{AuditLog: auditPath}) + + require.Len(t, results, len(recs)) + for _, result := range results { + assert.True(t, result.Success) + assert.True(t, result.DryRun) + } + + data, err := os.ReadFile(auditPath) //nolint:gosec // G304: test-controlled temp path + require.NoError(t, err) + lines := strings.Split(strings.TrimSpace(string(data)), "\n") + require.Len(t, lines, len(recs)) + for _, line := range lines { + var record common.AuditRecord + require.NoError(t, json.Unmarshal([]byte(line), &record)) + assert.Equal(t, "skipped", record.Status) + assert.True(t, record.DryRun) + } + }) +} diff --git a/go.mod b/go.mod index 649c56986..f37b27dac 100644 --- a/go.mod +++ b/go.mod @@ -83,9 +83,9 @@ require ( require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/authorization/armauthorization/v2 v2.2.0 github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261006104817-90e61e668b99 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261005004231-945a4045d11f + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261006205158-7ff8c1aee1bb github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007133317-58c25f04c49b - github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901 + github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261006205158-7ff8c1aee1bb github.com/aws/aws-sdk-go-v2/service/organizations v1.45.3 github.com/aws/aws-sdk-go-v2/service/secretsmanager v1.40.3 github.com/google/uuid v1.6.0 diff --git a/go.sum b/go.sum index 4a8d06f5c..26dd82266 100644 --- a/go.sum +++ b/go.sum @@ -80,12 +80,12 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapp github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261006104817-90e61e668b99 h1:inu/nYSAVY6cLsTn4qc64RIbCsymURLlEdgkhha79cw= github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261006104817-90e61e668b99/go.mod h1:ApWBliDXe099f3oDXBz41K/I9v4bHvn1dG/BGoRmHlw= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261005004231-945a4045d11f h1:c3K60juwiY6t25Gr0HWAxajIDsXH4LzDO4e3ENGAsAM= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261005004231-945a4045d11f/go.mod h1:wpW9/TvGFUUOqBEleJL639loh6Ue5aVIcZKdubRnYPQ= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261006205158-7ff8c1aee1bb h1:gtfWXNBATJDSS+q3+5Jxt2+HYijOSVEnpGkXmeCkudY= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261006205158-7ff8c1aee1bb/go.mod h1:zgv5QhLOIk9Hqw1xcmr1lSFg4ZeChwTCMwxETFVcfj4= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007133317-58c25f04c49b h1:yurugmfdZjf+dqjfqfDyfoBR3IAiFUpMKQ8URnZnJzQ= github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007133317-58c25f04c49b/go.mod h1:iIvPuWLWd+SXqLHAQNdodYMUAAcq7PnwfScXWgARt5A= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901 h1:i6OwXLUheudN3GfwnYXdKuEq8vPdE9qkNJ+r71LRLKA= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901/go.mod h1:UnMqDe2mBUHHF6gnKAgJn9/JeXa7WZqQxfWPs4unGrg= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261006205158-7ff8c1aee1bb h1:AJ4nKDtttjiTDemo9PDp2esoc35ymVaiCSr0zIewwpY= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261006205158-7ff8c1aee1bb/go.mod h1:vLtjCWuAcgFc6vlY4l4wpWOYvlj4itk25qjreylWpDQ= github.com/aws/aws-sdk-go-v2 v1.41.5 h1:dj5kopbwUsVUVFgO4Fi5BIT3t4WyqIDjGKCangnV/yY= github.com/aws/aws-sdk-go-v2 v1.41.5/go.mod h1:mwsPRE8ceUUpiTgF7QmQIJ7lgsKUPQOUl3o72QBrE1o= github.com/aws/aws-sdk-go-v2/config v1.29.12 h1:Y/2a+jLPrPbHpFkpAAYkVEtJmxORlXoo5k2g1fa2sUo= From 32e85810b4fb2fd73cb9954623d1ab8b4b4a62a8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 19:16:27 +0200 Subject: [PATCH 2/3] test(cli): trim purchase safeguard test narration --- cmd/gcp_cud_dedupe_2121_test.go | 33 +----------- cmd/purchase_safeguards_2121_test.go | 78 +--------------------------- 2 files changed, 2 insertions(+), 109 deletions(-) diff --git a/cmd/gcp_cud_dedupe_2121_test.go b/cmd/gcp_cud_dedupe_2121_test.go index 0c9db566c..d41525188 100644 --- a/cmd/gcp_cud_dedupe_2121_test.go +++ b/cmd/gcp_cud_dedupe_2121_test.go @@ -13,31 +13,11 @@ import ( "github.com/stretchr/testify/require" ) -// Consumer regression test for issue #2121, library #155: a recently -// purchased GCP committed-use discount suppresses retries for the same -// commitment family. The CLI reaches this through its duplicate-purchase -// filter (NewDuplicateChecker in helpers.go), which dispatches to the -// provider client's recent-commitment filter when the pinned library -// provides one. -// -// The test holds the pinned library's real *computeengine.Client behind an -// interface probe instead of naming the method statically: with the pre-#155 -// pins the type does not implement the filter, the probe fails at runtime, -// and this test fails; with the new pins the real family-level filter runs. -// Either way the file compiles, so the sibling pin-sensitive tests keep -// running under both pin sets. - -// gcpRecentCommitmentFilter mirrors the unexported recfilter dispatch -// interface so the probe can detect whether the pinned library client -// implements the family-level suppression. +// Probe the library interface at runtime so this test also compiles with the old pins. type gcpRecentCommitmentFilter interface { FilterRecommendationsForRecentCommitments(recs []common.Recommendation, existing []common.Commitment) (passed, filtered []common.Recommendation, err error) } -// fakeGCPServiceClient is a provider.ServiceClient whose existing -// commitments are fixed and whose recent-commitment filter delegates to the -// pinned library's real compute engine client, so the family-matching logic -// under test is the library's, not a reimplementation. type fakeGCPServiceClient struct { libraryClient any commitments []common.Commitment @@ -80,14 +60,6 @@ func (f *fakeGCPServiceClient) FilterRecommendationsForRecentCommitments(recs [] var _ provider.ServiceClient = (*fakeGCPServiceClient)(nil) -// TestDuplicateChecker_RecentGPCCUDSuppressesFamilyRetry_2121 drives the -// CLI's duplicate-purchase seam with a recent GCP CUD record: an n2 -// recommendation for the same account and region as the recent -// GENERAL_PURPOSE_N2 commitment is suppressed (a retry of the purchase that -// already happened), while an n4 recommendation in the same region belongs -// to a different commitment family and still passes. With the old pins the -// gcp client implements no such filter and the generic per-resource-type -// matching lets the n2 retry through. func TestDuplicateChecker_RecentGPCCUDSuppressesFamilyRetry_2121(t *testing.T) { ctx := context.Background() @@ -114,15 +86,12 @@ func TestDuplicateChecker_RecentGPCCUDSuppressesFamilyRetry_2121(t *testing.T) { Count: 2, } } - client := &fakeGCPServiceClient{ libraryClient: &computeengine.Client{}, // zero value: the filter uses only its arguments commitments: []common.Commitment{recentCUD}, } - recs := []common.Recommendation{cudRec("n2-standard-4"), cudRec("n4-standard-4")} passed, filtered, err := NewDuplicateChecker(0).AdjustRecommendationsForExisting(ctx, recs, client) - require.NoError(t, err) require.Len(t, filtered, 1, "recent GENERAL_PURPOSE_N2 purchase must suppress the n2 retry") assert.Equal(t, "n2-standard-4", filtered[0].ResourceType) diff --git a/cmd/purchase_safeguards_2121_test.go b/cmd/purchase_safeguards_2121_test.go index c8e31aaac..e1d4ed8e1 100644 --- a/cmd/purchase_safeguards_2121_test.go +++ b/cmd/purchase_safeguards_2121_test.go @@ -17,17 +17,6 @@ import ( "github.com/stretchr/testify/require" ) -// Consumer regression tests for issue #2121 (adopt reviewed -// cloud-commitments-go purchase safeguards). Each test names the library -// issue it pins down and the CLI seam it exercises. The pin-sensitive ones -// fail with the pre-#2121 library pins and pass with -// v0.0.0-20261006205158-7ff8c1aee1bb; the proof lives in the PR body. - -// TestApplyTargetCoverage_NonPositiveCountDropped_2121 covers library #221 -// through the cmd ApplyTargetCoverage wrapper (helpers.go). A recommendation -// with no positive Count has no per-unit cost, so the safeguard drops it -// instead of scaling its money fields by an invented ratio. With the old -// pins a Count=0 RI rec was kept and its costs multiplied by nTarget. func TestApplyTargetCoverage_NonPositiveCountDropped_2121(t *testing.T) { for _, count := range []int{0, -1} { rec := common.Recommendation{ @@ -45,17 +34,11 @@ func TestApplyTargetCoverage_NonPositiveCountDropped_2121(t *testing.T) { } drops := common.NewDropSummary() out := ApplyTargetCoverage([]common.Recommendation{rec}, 75, drops) - assert.Empty(t, out, "count=%d must be dropped, not scaled", count) assert.Contains(t, drops.FormatOneLine(), "target-input-invalid=1", "count=%d", count) } } -// TestApplyTargetCoverage_InvalidInputsDropped_2121 covers library #204 -// through the cmd ApplyTargetCoverage wrapper. A negative existing coverage -// makes the sized count meaningless (the gap exceeds the target), so the rec -// is dropped rather than scaled up past what AWS proposed. With the old pins -// this rec was kept and scaled by gap/target arithmetic. func TestApplyTargetCoverage_InvalidInputsDropped_2121(t *testing.T) { rec := common.Recommendation{ Provider: common.ProviderAWS, @@ -72,21 +55,13 @@ func TestApplyTargetCoverage_InvalidInputsDropped_2121(t *testing.T) { } drops := common.NewDropSummary() out := ApplyTargetCoverage([]common.Recommendation{rec}, 75, drops) - assert.Empty(t, out, "negative existing coverage must drop the rec") assert.Contains(t, drops.FormatOneLine(), "target-input-invalid=1") } -// TestDuplicateChecker_ValkeyDoesNotCoverRedis_2121 covers library #158/#208 -// through cmd's NewDuplicateChecker (helpers.go) and the CLI's -// recommendation-to-purchase duplicate filter. Redis OSS reservations cover -// Valkey nodes, never the reverse, so a recent Valkey reservation must not -// suppress a Redis recommendation. With the old pins Valkey was folded into -// Redis and the Redis rec was wrongly suppressed. func TestDuplicateChecker_ValkeyDoesNotCoverRedis_2121(t *testing.T) { ctx := context.Background() recent := time.Now().Add(-1 * time.Hour) - commitment := func(engine string) common.Commitment { return common.Commitment{ Provider: common.ProviderAWS, @@ -109,54 +84,39 @@ func TestDuplicateChecker_ValkeyDoesNotCoverRedis_2121(t *testing.T) { Details: &common.CacheDetails{Engine: engine}, } } - t.Run("recent valkey reservation keeps redis recommendation", func(t *testing.T) { mockClient := &MockServiceClient{} mockClient.On("GetExistingCommitments", ctx). Return([]common.Commitment{commitment("valkey")}, nil) - passed, filtered, err := NewDuplicateChecker(0). AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec("redis")}, mockClient) - require.NoError(t, err) require.Len(t, passed, 1, "valkey reservation must not suppress a redis recommendation") assert.Equal(t, 1, passed[0].Count) assert.Empty(t, filtered) }) - t.Run("recent redis reservation still suppresses valkey recommendation", func(t *testing.T) { mockClient := &MockServiceClient{} mockClient.On("GetExistingCommitments", ctx). Return([]common.Commitment{commitment("redis")}, nil) - passed, filtered, err := NewDuplicateChecker(0). AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec("valkey")}, mockClient) - require.NoError(t, err) assert.Empty(t, passed, "redis OSS reservations cover valkey nodes") require.Len(t, filtered, 1) }) - t.Run("recent reservation with missing engine still wildcards", func(t *testing.T) { mockClient := &MockServiceClient{} mockClient.On("GetExistingCommitments", ctx). Return([]common.Commitment{commitment("")}, nil) - passed, filtered, err := NewDuplicateChecker(0). AdjustRecommendationsForExisting(ctx, []common.Recommendation{rec("redis")}, mockClient) - require.NoError(t, err) assert.Empty(t, passed, "a reservation with unknown engine matches any cache engine") require.Len(t, filtered, 1) }) } -// TestDuplicateChecker_UnknownReservationStateRemainsOwned_2121 covers the -// CLI-side half of library #156: the duplicate checker keeps commitments in -// an unrecognized state as owned (fail closed against double purchases), -// while states known to release capacity do not suppress. The matching -// provider-side change (#156, providers/aws internal reservationstate) is -// only reachable through live AWS APIs and is covered in the provider repo. func TestDuplicateChecker_UnknownReservationStateRemainsOwned_2121(t *testing.T) { ctx := context.Background() recs := []common.Recommendation{{ @@ -179,40 +139,28 @@ func TestDuplicateChecker_UnknownReservationStateRemainsOwned_2121(t *testing.T) StartDate: time.Now().Add(-1 * time.Hour), } } - t.Run("unrecognized state suppresses", func(t *testing.T) { mockClient := &MockServiceClient{} mockClient.On("GetExistingCommitments", ctx). Return([]common.Commitment{commitment("some-future-state")}, nil) - passed, filtered, err := NewDuplicateChecker(0). AdjustRecommendationsForExisting(ctx, recs, mockClient) - require.NoError(t, err) assert.Empty(t, passed, "unknown states stay owned: skipping a purchase is recoverable, a duplicate is not") require.Len(t, filtered, 1) }) - t.Run("retired state does not suppress", func(t *testing.T) { mockClient := &MockServiceClient{} mockClient.On("GetExistingCommitments", ctx). Return([]common.Commitment{commitment(common.CommitmentStateRetired)}, nil) - passed, filtered, err := NewDuplicateChecker(0). AdjustRecommendationsForExisting(ctx, recs, mockClient) - require.NoError(t, err) require.Len(t, passed, 1) assert.Empty(t, filtered) }) } -// TestExecutePurchase_PreservesExplicitZeroVsAbsentCost_2121 covers library -// #157 at the CLI purchase dispatch (executePurchase in -// multi_service_helpers.go): PurchaseResult.Cost is the total upfront charge, -// a non-nil zero means "no upfront charge" and nil means "unknown". The CLI -// must pass both through untouched, and the two must stay distinguishable in -// the JSON form downstream consumers (audit tooling, MCP) read. func TestExecutePurchase_PreservesExplicitZeroVsAbsentCost_2121(t *testing.T) { ctx := context.Background() rec := common.Recommendation{ @@ -222,45 +170,32 @@ func TestExecutePurchase_PreservesExplicitZeroVsAbsentCost_2121(t *testing.T) { Region: "us-east-1", Count: 2, } - t.Run("explicit zero upfront cost stays a non-nil zero", func(t *testing.T) { zero := 0.0 mockClient := &MockServiceClient{} mockClient.On("PurchaseCommitment", ctx, rec, mock.Anything). Return(common.PurchaseResult{Recommendation: rec, Success: true, Cost: &zero}, nil) - result := executePurchase(ctx, rec, rec.Region, 1, mockClient, toolCfg) - require.True(t, result.Success) require.NotNil(t, result.Cost, "explicit zero must not collapse to absent") assert.Equal(t, 0.0, *result.Cost) - data, err := json.Marshal(result) require.NoError(t, err) assert.Contains(t, string(data), `"cost":0`) }) - t.Run("absent upfront cost stays nil", func(t *testing.T) { mockClient := &MockServiceClient{} mockClient.On("PurchaseCommitment", ctx, rec, mock.Anything). Return(common.PurchaseResult{Recommendation: rec, Success: true, Cost: nil}, nil) - result := executePurchase(ctx, rec, rec.Region, 1, mockClient, toolCfg) - require.True(t, result.Success) assert.Nil(t, result.Cost, "unknown cost must not be invented as zero") - data, err := json.Marshal(result) require.NoError(t, err) assert.Contains(t, string(data), `"cost":null`) }) } -// TestWritePurchaseAuditRecord_ZeroAndUnknownCost_2121 checks the CLI output -// path (writePurchaseAuditRecord in multi_service.go): a successful purchase -// with an explicit-zero or an absent upfront cost is audited as a success, -// and the fail-closed error case is audited as an error, so a re-driven run -// reconciles against the right per-recommendation outcome. func TestWritePurchaseAuditRecord_ZeroAndUnknownCost_2121(t *testing.T) { auditPath := filepath.Join(t.TempDir(), "audit.jsonl") rec := common.Recommendation{ @@ -285,7 +220,6 @@ func TestWritePurchaseAuditRecord_ZeroAndUnknownCost_2121(t *testing.T) { for _, tc := range cases { writePurchaseAuditRecord("run-2121", rec, tc.result, tc.status, false, auditPath) } - data, err := os.ReadFile(auditPath) //nolint:gosec // G304: test-controlled temp path require.NoError(t, err) lines := strings.Split(strings.TrimSpace(string(data)), "\n") @@ -298,40 +232,30 @@ func TestWritePurchaseAuditRecord_ZeroAndUnknownCost_2121(t *testing.T) { } } -// TestExecutePurchasePipeline_InterruptsFailClosed_2121 checks the -// interruption half of the fail-closed contract: once shutdown is requested, -// the purchase pipeline stops before the next recommendation instead of -// driving further purchases, and an uninterrupted dry run audits every rec. -// Dry-run mode only: no service clients are created and nothing is purchased. +// Dry-run mode creates no service clients and makes no purchases. func TestExecutePurchasePipeline_InterruptsFailClosed_2121(t *testing.T) { ctx := context.Background() recs := []common.Recommendation{ {Provider: common.ProviderAWS, Service: common.ServiceRDS, ResourceType: "db.r6g.large", Region: "us-east-1", Count: 1}, {Provider: common.ProviderAWS, Service: common.ServiceRDS, ResourceType: "db.r6g.xlarge", Region: "us-east-1", Count: 2}, } - t.Run("shutdown before start purchases nothing", func(t *testing.T) { auditPath := filepath.Join(t.TempDir(), "audit.jsonl") shutdownRequested.Store(true) defer shutdownRequested.Store(false) - results := executePurchasePipeline(ctx, aws.Config{}, recs, true, "run-2121-int", Config{AuditLog: auditPath}) - assert.Empty(t, results, "a requested shutdown must stop the pipeline before the first purchase") _, err := os.Stat(auditPath) assert.True(t, os.IsNotExist(err), "no audit records without purchase attempts") }) - t.Run("uninterrupted dry run audits every rec as skipped", func(t *testing.T) { auditPath := filepath.Join(t.TempDir(), "audit.jsonl") results := executePurchasePipeline(ctx, aws.Config{}, recs, true, "run-2121-dry", Config{AuditLog: auditPath}) - require.Len(t, results, len(recs)) for _, result := range results { assert.True(t, result.Success) assert.True(t, result.DryRun) } - data, err := os.ReadFile(auditPath) //nolint:gosec // G304: test-controlled temp path require.NoError(t, err) lines := strings.Split(strings.TrimSpace(string(data)), "\n") From ebfdbb102cfb6729c9d4ccd08dea7c45cd1e8bd8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 7 Oct 2026 22:25:32 +0200 Subject: [PATCH 3/3] test(cli): exercise GCP duplicate filtering entry path --- cmd/gcp_cud_dedupe_2121_test.go | 16 +++++++++------- 1 file changed, 9 insertions(+), 7 deletions(-) diff --git a/cmd/gcp_cud_dedupe_2121_test.go b/cmd/gcp_cud_dedupe_2121_test.go index d41525188..4b4297885 100644 --- a/cmd/gcp_cud_dedupe_2121_test.go +++ b/cmd/gcp_cud_dedupe_2121_test.go @@ -60,8 +60,11 @@ func (f *fakeGCPServiceClient) FilterRecommendationsForRecentCommitments(recs [] var _ provider.ServiceClient = (*fakeGCPServiceClient)(nil) -func TestDuplicateChecker_RecentGPCCUDSuppressesFamilyRetry_2121(t *testing.T) { +func TestDuplicateChecker_RecentGCPCUDSuppressesFamilyRetry_2121(t *testing.T) { ctx := context.Background() + previousWindow := toolCfg.IdempotencyWindowHours + toolCfg.IdempotencyWindowHours = 0 + t.Cleanup(func() { toolCfg.IdempotencyWindowHours = previousWindow }) recentCUD := common.Commitment{ Provider: common.ProviderGCP, @@ -91,10 +94,9 @@ func TestDuplicateChecker_RecentGPCCUDSuppressesFamilyRetry_2121(t *testing.T) { commitments: []common.Commitment{recentCUD}, } recs := []common.Recommendation{cudRec("n2-standard-4"), cudRec("n4-standard-4")} - passed, filtered, err := NewDuplicateChecker(0).AdjustRecommendationsForExisting(ctx, recs, client) - require.NoError(t, err) - require.Len(t, filtered, 1, "recent GENERAL_PURPOSE_N2 purchase must suppress the n2 retry") - assert.Equal(t, "n2-standard-4", filtered[0].ResourceType) - require.Len(t, passed, 1, "a different commitment family must not be suppressed") - assert.Equal(t, "n4-standard-4", passed[0].ResourceType) + drops := common.NewDropSummary() + adjusted := checkDuplicates(ctx, recs, client, false, drops) + assert.Equal(t, "Dropped 1 recs: duplicate-dedup=1", drops.FormatOneLine()) + require.Len(t, adjusted, 1, "a different commitment family must not be suppressed") + assert.Equal(t, "n4-standard-4", adjusted[0].ResourceType) }