diff --git a/cmd/multi_service.go b/cmd/multi_service.go index bd3847456..6ec97eaaf 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -22,11 +22,8 @@ import ( // fetchExistingCoverage retrieves the existing-RI coverage map from Cost // Explorer so --target-coverage sizing can subtract what's already owned in -// each pool. Best-effort: a transient CE failure logs a warning and returns -// an empty map, which the sizing path treats as "no signal" — recs sized -// without subtracting existing commitments. Skipping the fetch entirely -// when --target-coverage is not in play avoids the per-region CE charges -// for users on the --coverage path. +// each pool. Skipping the fetch entirely when --target-coverage is not in +// play avoids the per-region CE charges for users on the --coverage path. // // Coverage is fetched per-region per-account so CE's org-wide aggregate // doesn't bleed one account's coverage into another in multi-account orgs. @@ -35,15 +32,26 @@ import ( // The lookback window is cfg.CoverageLookbackDays (default 30, matching the // CE UI default). Operators reconciling against the AWS console coverage // report should match this value to the report's own time window. -func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient provider.RecommendationsClient, cfg Config) recommendations.PoolCoverageMap { +// +// A failed fetch is not survivable on a real purchase run: the sizing +// formula computes gapPct := target - ExistingCoveragePct, and a nil map +// leaves every recommendation at ExistingCoveragePct == 0, so a fetch failure +// silently sizes as if the account owns nothing. Against an account already +// at 75% coverage, --target-coverage 80 would then buy another 80% on top and +// land near 155%. So a real purchase run returns the error and the caller +// must abort rather than proceed with a nil map. A dry run logs a loud +// warning and returns a nil map with no error, matching the previous +// best-effort behavior, since nothing is bought and reporting fidelity wins. +func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient provider.RecommendationsClient, cfg Config) (recommendations.PoolCoverageMap, error) { if cfg.TargetCoverage <= 0 { - return nil + return nil, nil } adapter, ok := recClient.(*awsprovider.RecommendationsClientAdapter) if !ok { // Non-AWS provider: feature not wired up. Sizing degenerates to - // the no-existing-commitments path. - return nil + // the no-existing-commitments path. Not a fetch failure, so this is + // not subject to the fail-closed behavior below. + return nil, nil } lookbackDays := cfg.CoverageLookbackDays if lookbackDays <= 0 { @@ -53,19 +61,29 @@ func fetchExistingCoverage(ctx context.Context, awsCfg aws.Config, recClient pro if len(regions) == 0 { allRegions, err := getAllAWSRegions(ctx, awsCfg) if err != nil { - AppLogger.Printf(" āš ļø Could not list AWS regions for coverage fetch (%v); skipping existing-coverage subtraction\n", err) - return nil + return nil, coverageFetchFailure(cfg, fmt.Errorf("could not list AWS regions for coverage fetch: %w", err)) } regions = allRegions } AppLogger.Printf("\nšŸ”Ž Fetching existing-RI coverage from Cost Explorer per-account across %d regions (lookback %d days)...\n", len(regions), lookbackDays) cov, err := adapter.GetRICoverageMap(ctx, lookbackDays, regions) if err != nil { - AppLogger.Printf(" āš ļø Could not fetch existing-RI coverage (%v); sizing will assume zero existing coverage\n", err) - return nil + return nil, coverageFetchFailure(cfg, fmt.Errorf("could not fetch existing-RI coverage: %w", err)) } AppLogger.Printf(" āœ… Fetched coverage for %d (region, instance-type, engine, account) entries\n", len(cov)) - return cov + return cov, nil +} + +// coverageFetchFailure decides how fetchExistingCoverage reports a failed +// coverage fetch: nil (best-effort, matching the pre-#1942 behavior) on a dry +// run, or the error itself on a real purchase run so the caller aborts +// instead of sizing every recommendation as if nothing is owned. +func coverageFetchFailure(cfg Config, err error) error { + if effectiveDryRun(cfg) { + AppLogger.Printf(" āš ļø %v; sizing will assume zero existing coverage (dry run only — a real --purchase run aborts instead, since --target-coverage would overbuy on top of what is already owned)\n", err) + return nil + } + return err } // shutdownRequested is set to true when SIGINT is received during a purchase run. @@ -126,11 +144,16 @@ func runToolMultiService(ctx context.Context, cfg Config) { engineData := fetchEngineVersionData(ctx, cfg) // Fetch existing-RI coverage so --target-coverage can subtract what - // the user already owns. Best-effort: a failure here logs a warning - // and continues with an empty map, which makes sizing degenerate to - // the no-existing-commitments path (matches behavior when no recs - // are matched in the map). - coverageMap := fetchExistingCoverage(ctx, awsCfg, recClient, cfg) + // the user already owns. On a dry run, a failure logs a warning and + // continues with an empty map (sizing degenerates to the + // no-existing-commitments path). On a real purchase run, a failure + // aborts instead: sizing against a nil map would treat every + // recommendation as if nothing is owned and overbuy on top of the + // account's existing coverage. + coverageMap, err := fetchExistingCoverage(ctx, awsCfg, recClient, cfg) + if err != nil { + log.Fatalf("Cannot size --target-coverage: %v", err) + } // Phase 1: collect all recommendations without purchasing. AppLogger.Printf("\nšŸ“„ Fetching recommendations from all services...\n") diff --git a/cmd/multi_service_coverage_test.go b/cmd/multi_service_coverage_test.go index c66e46d6d..5151c4751 100644 --- a/cmd/multi_service_coverage_test.go +++ b/cmd/multi_service_coverage_test.go @@ -8,6 +8,7 @@ import ( "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + awsprovider "github.com/LeanerCloud/cloud-commitments-go/providers/aws" "github.com/aws/aws-sdk-go-v2/aws" awsrds "github.com/aws/aws-sdk-go-v2/service/rds" rdstypes "github.com/aws/aws-sdk-go-v2/service/rds/types" @@ -822,13 +823,15 @@ func TestFetchExistingCoverage_LookbackDays(t *testing.T) { t.Run("zero TargetCoverage returns nil regardless of lookback", func(t *testing.T) { cfg := Config{TargetCoverage: 0, CoverageLookbackDays: 14, Regions: []string{"us-east-1"}} - got := fetchExistingCoverage(ctx, awsCfg, mockClient, cfg) + got, err := fetchExistingCoverage(ctx, awsCfg, mockClient, cfg) + require.NoError(t, err) assert.Nil(t, got, "TargetCoverage=0 must short-circuit before any CE call") }) t.Run("non-AWS adapter returns nil, lookback not needed", func(t *testing.T) { cfg := Config{TargetCoverage: 80, CoverageLookbackDays: 14, Regions: []string{"us-east-1"}} - got := fetchExistingCoverage(ctx, awsCfg, mockClient, cfg) + got, err := fetchExistingCoverage(ctx, awsCfg, mockClient, cfg) + require.NoError(t, err) assert.Nil(t, got, "non-AWS provider must return nil (no CE integration)") }) @@ -840,3 +843,31 @@ func TestFetchExistingCoverage_LookbackDays(t *testing.T) { // providers/aws/recommendations/coverage_test.go, which directly verifies // end-start == lookbackDays on the actual CE input. No redundant subcase here. } + +// TestFetchExistingCoverage_FetchFailure reproduces #1942: a failed +// existing-coverage fetch must not silently size every recommendation as if +// nothing is owned (ExistingCoveragePct == 0) on a real purchase run -- +// against an account already at 75% coverage, --target-coverage 80 would +// then buy another 80% on top and land near 155%. It uses a real +// *awsprovider.RecommendationsClientAdapter with no credentials so +// GetRICoverageMap fails the same way a real CE throttle or a missing +// ce:GetReservationCoverage permission would. +func TestFetchExistingCoverage_FetchFailure(t *testing.T) { + ctx := context.Background() + awsCfg := aws.Config{Region: "us-east-1"} + realClient := awsprovider.NewRecommendationsClientDirect(awsCfg) + + t.Run("purchase run aborts: returns the error and a nil map", func(t *testing.T) { + cfg := Config{TargetCoverage: 80, Regions: []string{"us-east-1"}, ActualPurchase: true} + got, err := fetchExistingCoverage(ctx, awsCfg, realClient, cfg) + require.Error(t, err, "a real purchase run must abort rather than size against a nil coverage map") + assert.Nil(t, got) + }) + + t.Run("dry run keeps the previous best-effort behavior: nil map, no error", func(t *testing.T) { + cfg := Config{TargetCoverage: 80, Regions: []string{"us-east-1"}, ActualPurchase: false} + got, err := fetchExistingCoverage(ctx, awsCfg, realClient, cfg) + require.NoError(t, err, "a dry run must not abort; nothing is bought") + assert.Nil(t, got) + }) +}