diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e1bd23a3..5f47b112c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,25 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `--max-instances` and `--min-count` no longer apply to Savings Plans, whose + Count of 1 is not an instance count: a Savings Plan neither uses up the + instance cap nor is dropped by the floor (#1613). +- `configure-azure` and `configure-gcp` now fail when listing the credentials + secret fails (for example access denied) instead of proceeding to the update + and looking successful; `ListSecrets` also reads every result page (#1613). +- `configure-azure --client-secret` is deprecated (it still works): omit it and + enter the secret at the interactive prompt (#1613). +- The `configure-azure` help no longer shows `--client-secret` in its example; + the client secret is read from the interactive prompt (#1613). +- `configure-azure` and `configure-gcp` now store credentials in the secret whose + name matches exactly instead of the first prefix match from Secrets Manager + (which could be `-old`), and fail when two secrets share the name (#1613). +- The RDS engine-version scan now stops on cancellation and fails with an error + naming the region after 200 pages of `DescribeDBInstances` instead of paging + without bound (#1613). +- `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). - 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 @@ -121,6 +140,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Changed +- Bumped `cloud-commitments-go` (pkg and the aws, azure and gcp providers) to + `cfe8ac29`. User-visible effects: recommendations reported in a non-USD + currency are skipped at ingestion and the skip is reported; RDS SQL Server + RIs are sized per instance size; GCP no longer emits Cloud SQL and Memorystore + CUD recommendations from the performance recommenders; Azure Savings Plan + purchases refuse a malformed idempotency token (#1613). - BREAKING: `--input-csv` now requires `Term` and `PaymentOption` columns in addition to `Service`, `Region`, `ResourceType` and `Count`. `Term` must be `1yr` or `3yr` and `PaymentOption` must be `all-upfront`, `partial-upfront` diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 2cc5cca73..69f17990b 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -97,8 +97,9 @@ var configureAzureCmd = &cobra.Command{ This command stores your Azure credentials in AWS Secrets Manager for use by CUDly. -You can provide credentials via flags or interactively: - cudly configure-azure --stack-name my-cudly --tenant-id xxx --client-id xxx --client-secret xxx --subscription-id xxx +You can provide credentials via flags or interactively (the client secret is +read from a prompt, so it stays out of shell history and process listings): + cudly configure-azure --stack-name my-cudly --tenant-id xxx --client-id xxx --subscription-id xxx cudly configure-azure --stack-name my-cudly --interactive To create an Azure Service Principal manually: @@ -115,6 +116,9 @@ func init() { configureAzureCmd.Flags().StringVar(&azureOpts.TenantID, "tenant-id", "", "Azure AD Tenant ID") configureAzureCmd.Flags().StringVar(&azureOpts.ClientID, "client-id", "", "Azure Service Principal Client ID") configureAzureCmd.Flags().StringVar(&azureOpts.ClientSecret, "client-secret", "", "Azure Service Principal Client Secret") + if err := configureAzureCmd.Flags().MarkDeprecated("client-secret", "a secret on the command line leaks into shell history and process listings; omit it and enter it at the interactive Client Secret prompt"); err != nil { + panic(err) + } configureAzureCmd.Flags().StringVar(&azureOpts.SubscriptionID, "subscription-id", "", "Azure Subscription ID") configureAzureCmd.Flags().BoolVarP(&azureOpts.Interactive, "interactive", "i", false, "Prompt for credentials interactively") configureAzureCmd.Flags().BoolVar(&azureOpts.SkipSetup, "skip-setup", false, "Skip Azure CLI setup commands (az login, create service principal)") @@ -144,13 +148,9 @@ func storeAzureCredentials(ctx context.Context, store SecretsStore, stackName st // Build expected secret name pattern secretName := fmt.Sprintf("%s-AzureCredentials", stackName) - // Try to find the actual secret ARN by listing secrets - arns, err := store.ListSecrets(ctx, secretName) - - // Use the ARN if found, otherwise use the name (will fail if secret doesn't exist) - secretID := secretName - if err == nil && len(arns) > 0 { - secretID = arns[0] + secretID, err := resolveSecretID(ctx, store, secretName) + if err != nil { + return err } // Marshal credentials to JSON @@ -527,14 +527,16 @@ func azureStepCreateServicePrincipal(ctx context.Context, reader *bufio.Reader, fmt.Printf("%q role at subscription scope, via the Microsoft Graph SDK.\n", azureSPRoleName) fmt.Println() fmt.Printf("Create service principal %q with role %q at /subscriptions/%s?\n", azureSPName, azureSPRoleName, subscriptionID) - fmt.Printf("[R]un, [S]kip? ") + fmt.Printf("[r]un, [S]kip (default)? ") choice, err := readTrimmedLine(reader) if err != nil { return nil, fmt.Errorf("failed to read service-principal choice: %w", err) } + // Creating an Entra app, password and role assignment is not harmless, so + // Enter skips, like the GCP wizard. choice = strings.ToLower(choice) - if choice != "r" && choice != "run" && choice != "" { + if choice != "r" && choice != "run" { fmt.Println("Skipping Create Service Principal") fmt.Println() fmt.Println("Provide the appId, client secret and tenant ID for an existing") diff --git a/cmd/configure_azure_secret_test.go b/cmd/configure_azure_secret_test.go index 4888e464a..9b755abf5 100644 --- a/cmd/configure_azure_secret_test.go +++ b/cmd/configure_azure_secret_test.go @@ -179,6 +179,26 @@ func TestAzureWizard_SkipCreateSP_ReturnsNil(t *testing.T) { assert.Nil(t, created) } +// TestAzureWizard_EmptyInputSkipsCreateSP checks that pressing Enter at the +// create-SP prompt skips (the default), so no Entra app, password or role +// assignment is created by accident. +func TestAzureWizard_EmptyInputSkipsCreateSP(t *testing.T) { + stubAzureSPConstructors(t, nil) + newAzureSPProvisionerFn = func(_ string) (azureSPProvisioner, error) { + t.Fatal("provisioner must not be built on empty input") + return nil, nil + } + + var created *AzureCredentials + var err error + captureAllOutput(t, func() { + created, err = azureStepCreateServicePrincipal(context.Background(), + bufio.NewReader(strings.NewReader("\n")), testSubID) + }) + require.NoError(t, err) + assert.Nil(t, created) +} + // TestAzureWizard_FailureAfterCreate_NoSecretInError covers the rollback path // and the post-create recovery hint: neither error text nor output may carry // the secret. @@ -340,3 +360,14 @@ func TestCollectAzureCredentials_PromptWording(t *testing.T) { assert.Contains(t, out, "Enter the credentials of an existing Azure Service Principal:") assert.NotContains(t, out, "output above") } + +// The help example must not teach passing the secret on the command line. +func TestConfigureAzureHelp_NoClientSecretFlagExample(t *testing.T) { + assert.NotContains(t, configureAzureCmd.Long, "--client-secret") +} + +func TestConfigureAzure_ClientSecretFlagDeprecated(t *testing.T) { + flag := configureAzureCmd.Flags().Lookup("client-secret") + require.NotNil(t, flag) + assert.NotEmpty(t, flag.Deprecated) +} diff --git a/cmd/configure_gcp.go b/cmd/configure_gcp.go index 9b6e39514..ea9cb3387 100644 --- a/cmd/configure_gcp.go +++ b/cmd/configure_gcp.go @@ -121,13 +121,9 @@ func storeGCPCredentials(ctx context.Context, store SecretsStore, stackName, cre // Build expected secret name pattern secretName := fmt.Sprintf("%s-GCPCredentials", stackName) - // Try to find the actual secret ARN by listing secrets - arns, err := store.ListSecrets(ctx, secretName) - - // Use the ARN if found, otherwise use the name (will fail if secret doesn't exist) - secretID := secretName - if err == nil && len(arns) > 0 { - secretID = arns[0] + secretID, err := resolveSecretID(ctx, store, secretName) + if err != nil { + return err } // Store credentials in Secrets Manager (using the original JSON format) diff --git a/cmd/helpers.go b/cmd/helpers.go index 1b99b15ae..7fef0217e 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -146,6 +146,42 @@ func CalculateTotalInstances(recs []common.Recommendation) int { return total } +// isSavingsPlan reports whether rec is a Savings Plan, whose Count is a +// placeholder 1 rather than a number of instances. +func isSavingsPlan(rec common.Recommendation) bool { + return rec.CommitmentType == common.CommitmentSavingsPlan +} + +// withoutSavingsPlans returns the instance-based recommendations, in order. +func withoutSavingsPlans(recs []common.Recommendation) []common.Recommendation { + var others []common.Recommendation + for i := range recs { + if !isSavingsPlan(recs[i]) { + others = append(others, recs[i]) + } + } + return others +} + +// mergeKeepingOrder returns the entries of original that survive, in original +// order: every Savings Plan, plus the leading len(keptOthers) instance-based +// entries replaced by their (possibly reduced) kept versions. keptOthers must be +// a prefix-ordered result of the instance-based entries. +func mergeKeepingOrder(original, keptOthers []common.Recommendation) []common.Recommendation { + out := make([]common.Recommendation, 0, len(original)) + next := 0 + for i := range original { + switch { + case isSavingsPlan(original[i]): + out = append(out, original[i]) + case next < len(keptOthers): + out = append(out, keptOthers[next]) + next++ + } + } + return out +} + // ApplyCoverage delegates to recfilter.ApplyCoverage, wiring AppLogger as the // logging sink. Substantive documentation lives on recfilter.ApplyCoverage. func ApplyCoverage(recs []common.Recommendation, coverage float64) []common.Recommendation { @@ -202,6 +238,10 @@ func applySizing(recs []common.Recommendation, cfg Config, coverage float64, dro // path (#1830). The count-linear fields that are not money (ProjectedCoverage, // ProjectedUtilization, Redshift NumberOfNodes) are re-derived from the new // Count by refreshCountLinear (#1845). +// +// Savings Plans are exempt: their Count is always 1 and is not an instance +// count, so they pass through unchanged, wherever they sit in recs, and consume +// no budget. func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []common.Recommendation { if maxInstances <= 0 { return recs @@ -212,8 +252,12 @@ func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []comm for _rvc := range recs { rec := recs[_rvc] + if isSavingsPlan(rec) { + result = append(result, rec) + continue + } if remaining <= 0 { - break + continue // keep scanning: later Savings Plans still pass through } adjusted := rec // rec.Count > remaining and remaining >= 1 together imply rec.Count diff --git a/cmd/instance_limit_savings_plan_test.go b/cmd/instance_limit_savings_plan_test.go new file mode 100644 index 000000000..e2e27538d --- /dev/null +++ b/cmd/instance_limit_savings_plan_test.go @@ -0,0 +1,155 @@ +package main + +import ( + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func mkRI(resourceType string, count int) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceRDS, Region: "eu-west-1", ResourceType: resourceType, + Count: count, CommitmentType: common.CommitmentReservedInstance, EstimatedSavings: 100, + } +} + +func mkSP() common.Recommendation { + return common.Recommendation{ + Service: common.ServiceCompute, Region: "eu-west-1", ResourceType: "Compute", + Count: 1, CommitmentType: common.CommitmentSavingsPlan, EstimatedSavings: 100, + } +} + +func TestApplyInstanceLimit_SavingsPlanExempt(t *testing.T) { + got := ApplyInstanceLimit([]common.Recommendation{mkRI("db.r5.large", 3), mkSP(), mkRI("db.r5.xlarge", 2)}, 1) + + require.Len(t, got, 2) + assert.Equal(t, 1, got[0].Count) + assert.Equal(t, common.CommitmentSavingsPlan, got[1].CommitmentType) + assert.Equal(t, 1, got[1].Count) +} + +func TestApplyGlobalInstanceLimit_SavingsPlanExempt(t *testing.T) { + tests := []struct { + name string + cfg Config + recs []common.Recommendation + wantRIs []int + wantSP bool + wantSize int + }{ + {"cap binds on RIs only", Config{MaxInstances: 1}, []common.Recommendation{mkRI("a", 3), mkSP(), mkRI("b", 2)}, []int{1}, true, 2}, + {"SP alone does not bind the cap", Config{MaxInstances: 1}, []common.Recommendation{mkRI("a", 1), mkSP()}, []int{1}, true, 2}, + {"SP survives cap with min-count above 1", Config{MaxInstances: 1, MinCount: 2}, []common.Recommendation{mkRI("a", 3), mkSP()}, nil, true, 1}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got := applyGlobalInstanceLimit(tt.recs, tt.cfg, rankBySavingsPercentage, nil) + + var ris []int + sp := false + for _, r := range got { + if isSavingsPlan(r) { + sp = true + assert.Equal(t, 1, r.Count) + } else { + ris = append(ris, r.Count) + } + } + assert.Equal(t, tt.wantRIs, ris) + assertSameRelativeOrder(t, tt.recs, got) + assert.Equal(t, tt.wantSP, sp) + assert.Len(t, got, tt.wantSize) + }) + } +} + +func TestApplyMinCountFloor_SavingsPlanExempt(t *testing.T) { + got := applyMinCountFloor([]common.Recommendation{mkRI("a", 1), mkSP(), mkRI("b", 5)}, 3) + + require.Len(t, got, 2) + assert.Equal(t, []string{"Compute", "b"}, []string{got[0].ResourceType, got[1].ResourceType}, "input order kept") + var sps, ris int + for _, r := range got { + if isSavingsPlan(r) { + sps++ + } else { + ris++ + assert.Equal(t, 5, r.Count) + } + } + assert.Equal(t, 1, sps) + assert.Equal(t, 1, ris) +} + +// assertSameRelativeOrder checks got is a subsequence of in by ResourceType. +func assertSameRelativeOrder(t *testing.T, in, got []common.Recommendation) { + t.Helper() + i := 0 + for _, g := range got { + for i < len(in) && in[i].ResourceType != g.ResourceType { + i++ + } + require.Less(t, i, len(in), "output order differs from input order") + i++ + } +} + +// scoreLimitAndDisplay hands MinCount to scorer.Score, which must exempt +// Savings Plans (Count 1 is a placeholder) at the pinned library version. +func TestScoreLimitAndDisplay_MinCountKeepsSavingsPlan(t *testing.T) { + result := scoreLimitAndDisplay([]common.Recommendation{mkSP(), mkRI("a", 3)}, Config{MinCount: 2}, nil) + + types := make([]common.CommitmentType, 0, len(result.Passed)) + for _, r := range result.Passed { + types = append(types, r.CommitmentType) + } + assert.ElementsMatch(t, []common.CommitmentType{common.CommitmentSavingsPlan, common.CommitmentReservedInstance}, types) +} + +func TestApplyInstanceLimit_SavingsPlanAfterBudgetSpent(t *testing.T) { + got := ApplyInstanceLimit([]common.Recommendation{mkRI("a", 3), mkRI("b", 2), mkSP()}, 3) + + require.Len(t, got, 2) + assert.Equal(t, "a", got[0].ResourceType) + assert.True(t, isSavingsPlan(got[1])) +} + +func mkNamedSP(name string) common.Recommendation { + r := mkSP() + r.ResourceType = name + return r +} + +// mixedOrderInput interleaves Savings Plans with instance rows. +func mixedOrderInput() []common.Recommendation { + return []common.Recommendation{ + mkNamedSP("S1"), mkRI("a", 3), mkNamedSP("S2"), mkRI("b", 2), + mkNamedSP("S3"), mkRI("c", 5), mkNamedSP("S4"), + } +} + +func resourceTypes(recs []common.Recommendation) []string { + out := make([]string, 0, len(recs)) + for _, r := range recs { + out = append(out, r.ResourceType) + } + return out +} + +func TestMixedSavingsPlanOrder_Exact(t *testing.T) { + t.Run("cap 4", func(t *testing.T) { + got := applyGlobalInstanceLimit(mixedOrderInput(), Config{MaxInstances: 4}, rankBySavingsPercentage, nil) + assert.Equal(t, []string{"S1", "a", "S2", "b", "S3", "S4"}, resourceTypes(got)) + }) + t.Run("cap 4 min-count 2", func(t *testing.T) { + got := applyGlobalInstanceLimit(mixedOrderInput(), Config{MaxInstances: 4, MinCount: 2}, rankBySavingsPercentage, nil) + assert.Equal(t, []string{"S1", "a", "S2", "S3", "S4"}, resourceTypes(got)) + }) + t.Run("min-count floor 3", func(t *testing.T) { + got := applyMinCountFloor(mixedOrderInput(), 3) + assert.Equal(t, []string{"S1", "a", "S2", "S3", "c", "S4"}, resourceTypes(got)) + }) +} diff --git a/cmd/main.go b/cmd/main.go index 740f2f77d..c78d8debe 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -134,7 +134,7 @@ func init() { rootCmd.Flags().StringSliceVar(&toolCfg.ExcludeEngines, "exclude-engines", []string{}, "Exclude these engines (comma-separated)") rootCmd.Flags().StringSliceVar(&toolCfg.IncludeAccounts, "include-accounts", []string{}, "Only include recommendations for these accounts, matched exactly (case-insensitive) on account name or ID (comma-separated)") rootCmd.Flags().StringSliceVar(&toolCfg.ExcludeAccounts, "exclude-accounts", []string{}, "Exclude recommendations for these accounts, matched exactly (case-insensitive) on account name or ID; a value that only partly matches account names stops the run (comma-separated)") - rootCmd.Flags().Int32Var(&toolCfg.MaxInstances, "max-instances", 0, "Maximum total number of instances to purchase (0 = no limit)") + rootCmd.Flags().Int32Var(&toolCfg.MaxInstances, "max-instances", 0, "Maximum total number of instances to purchase (0 = no limit); does not apply to Savings Plans") rootCmd.Flags().Int32Var(&toolCfg.OverrideCount, "override-count", 0, "Override recommendation count with fixed number for all selected RIs (0 = use recommendation or coverage)") rootCmd.Flags().StringVar(&toolCfg.ValidationProfile, "validation-profile", "", "AWS profile to use for validating running instances (if different from main profile)") rootCmd.Flags().BoolVar(&toolCfg.IncludeExtendedSupport, "include-extended-support", false, "Include instances running on extended support engine versions (by default they are excluded)") @@ -150,7 +150,7 @@ func init() { rootCmd.Flags().StringVar(&toolCfg.IdempotencyWindow, "idempotency-window", "24h", "Lookback window for duplicate purchase detection") rootCmd.Flags().Float64Var(&toolCfg.MinSavingsPct, "min-savings-pct", 0, "Minimum savings percentage to include a recommendation (0 = no filter)") rootCmd.Flags().IntVar(&toolCfg.MaxBreakEvenMonths, "max-break-even-months", 0, "Maximum break-even period in months (0 = no filter)") - rootCmd.Flags().IntVar(&toolCfg.MinCount, "min-count", 0, "Minimum instance count to include a recommendation (0 = no filter)") + rootCmd.Flags().IntVar(&toolCfg.MinCount, "min-count", 0, "Minimum instance count to include a recommendation (0 = no filter); does not apply to Savings Plans") rootCmd.Flags().IntVar(&toolCfg.CoverageLookbackDays, "coverage-lookback-days", 30, "Number of calendar days of historical demand fed to GetReservationCoverage "+ "when computing the existing-RI coverage map for --target-coverage sizing. "+ diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 0fb0eb9e1..56848e503 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -389,7 +389,8 @@ const ( // conditions drift apart, either a run is refused for a cap that changes // nothing, or an unrankable run reaches the cap after all. func capBinds(recs []common.Recommendation, cfg Config) bool { - return cfg.MaxInstances > 0 && CalculateTotalInstances(recs) > int(cfg.MaxInstances) + others := withoutSavingsPlans(recs) + return cfg.MaxInstances > 0 && CalculateTotalInstances(others) > int(cfg.MaxInstances) } // applyGlobalInstanceLimit enforces --max-instances once across the entire run. @@ -417,12 +418,16 @@ func applyGlobalInstanceLimit(passed []common.Recommendation, cfg Config, rule r return passed } - totalBefore := CalculateTotalInstances(passed) - limited := ApplyInstanceLimit(passed, cfg.MaxInstances) + // Savings Plans are exempt from the cap and the post-cap floor: their Count + // is a placeholder 1, not an instance count. The report and the floor see + // only the instance-based rows, whose capped result is a prefix of its input. + others := withoutSavingsPlans(passed) + totalBefore := CalculateTotalInstances(others) + limited := ApplyInstanceLimit(others, cfg.MaxInstances) limited, belowMin := dropTruncatedBelowMinCount(limited, cfg.MinCount) - reportInstanceLimit(passed, limited, len(belowMin), totalBefore, cfg.MaxInstances, rule, drops) + reportInstanceLimit(others, limited, len(belowMin), totalBefore, cfg.MaxInstances, rule, drops) reportMinCountDrops(belowMin, cfg.MinCount, drops) - return limited + return mergeKeepingOrder(passed, limited) } // dropTruncatedBelowMinCount removes recommendations that the cap truncated to diff --git a/cmd/multi_service_csv_cap.go b/cmd/multi_service_csv_cap.go index 3113fc6d9..2aeae92cf 100644 --- a/cmd/multi_service_csv_cap.go +++ b/cmd/multi_service_csv_cap.go @@ -56,13 +56,24 @@ func applyMinCountFloor(recs []common.Recommendation, minCount int) []common.Rec if minCount <= 0 { return recs } - scored := scorer.Score(recs, scorer.Config{MinCount: minCount}) - for i := range scored.Filtered { - f := scored.Filtered[i] - AppLogger.Printf("🔒 --min-count dropped %s %s %s: %s\n", - f.Recommendation.Service, f.Recommendation.Region, f.Recommendation.ResourceType, f.FilterReason) + // Savings Plans carry a placeholder Count of 1, so the floor does not apply. + // Rows are scored one at a time so the output keeps the input order + // (scorer.Score sorts its Passed list). + var kept []common.Recommendation + for i := range recs { + if isSavingsPlan(recs[i]) { + kept = append(kept, recs[i]) + continue + } + scored := scorer.Score(recs[i:i+1], scorer.Config{MinCount: minCount}) + for j := range scored.Filtered { + f := scored.Filtered[j] + AppLogger.Printf("🔒 --min-count dropped %s %s %s: %s\n", + f.Recommendation.Service, f.Recommendation.Region, f.Recommendation.ResourceType, f.FilterReason) + } + kept = append(kept, scored.Passed...) } - return scored.Passed + return kept } // applyThresholdGates drops recommendations that fail --min-savings-pct or @@ -152,11 +163,13 @@ func requireRankingSignal(recs []common.Recommendation, cfg Config) error { return nil } + // Savings Plans are exempt from the cap, so they are never ranked. + others := withoutSavingsPlans(recs) unrankable := make([]string, 0) - for i := range recs { - if recs[i].EstimatedSavings <= 0 { + for i := range others { + if others[i].EstimatedSavings <= 0 { unrankable = append(unrankable, fmt.Sprintf("%s %s %s", - recs[i].Service, recs[i].Region, recs[i].ResourceType)) + others[i].Service, others[i].Region, others[i].ResourceType)) } } if len(unrankable) == 0 { @@ -173,5 +186,5 @@ func requireRankingSignal(recs []common.Recommendation, cfg Config) error { "--max-instances=%d has to choose which of %d recommendations to buy, but %d row(s) of %s carry no usable EstimatedSavings value: %s%s. "+ "A blank or missing EstimatedSavings cell is indistinguishable from $0 of savings, so capping on it would pick by instance-type name rather than by value. "+ "Populate EstimatedSavings for every row, or drop --max-instances and cap the file itself", - cfg.MaxInstances, len(recs), len(unrankable), cfg.CSVInput, strings.Join(named, ", "), suffix) + cfg.MaxInstances, len(others), len(unrankable), cfg.CSVInput, strings.Join(named, ", "), suffix) } diff --git a/cmd/multi_service_csv_cap_ranking_test.go b/cmd/multi_service_csv_cap_ranking_test.go new file mode 100644 index 000000000..4e5cf1a58 --- /dev/null +++ b/cmd/multi_service_csv_cap_ranking_test.go @@ -0,0 +1,22 @@ +package main + +import ( + "testing" + + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +func TestRequireRankingSignal_IgnoresZeroSavingsSavingsPlan(t *testing.T) { + recs := []common.Recommendation{ + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.micro", Count: 5, EstimatedSavings: 100}, + {Service: common.ServiceRDS, Region: "us-east-1", ResourceType: "db.t3.small", Count: 5, EstimatedSavings: 50}, + {Service: common.ServiceCompute, Region: "us-east-1", ResourceType: "compute", Count: 1, + CommitmentType: common.CommitmentSavingsPlan, EstimatedSavings: 0}, + } + require.NoError(t, requireRankingSignal(recs, Config{MaxInstances: 6})) + + recs[1].EstimatedSavings = 0 + require.Error(t, requireRankingSignal(recs, Config{MaxInstances: 6})) +} diff --git a/cmd/multi_service_engine_versions.go b/cmd/multi_service_engine_versions.go index 97c8c05a5..c4cae83d1 100644 --- a/cmd/multi_service_engine_versions.go +++ b/cmd/multi_service_engine_versions.go @@ -99,6 +99,17 @@ const maxConcurrentRegionQueries = 10 // 20 pages x ~100 records/page = ~2000 records, enough for any engine list (issue #692). const maxEngineVersionPages = 20 +// maxInstancePages caps DescribeDBInstances pagination per region. A page holds +// 100 instances by default, so 200 pages covers 20k instances per region, far +// above the RDS per-account instance quota, and bounds a non-terminating marker. +const maxInstancePages = 200 + +// RDSInstancesClient is the subset of the RDS API needed by +// queryRDSInstancesPaged, extracted so tests can inject a fake. +type RDSInstancesClient interface { + DescribeDBInstances(ctx context.Context, params *awsrds.DescribeDBInstancesInput, optFns ...func(*awsrds.Options)) (*awsrds.DescribeDBInstancesOutput, error) +} + // RDSMajorVersionsClient is the subset of the RDS API needed by // queryMajorEngineVersionsWithClient, extracted so tests can inject a mock. type RDSMajorVersionsClient interface { @@ -153,10 +164,20 @@ func queryRDSInstancesInRegions(ctx context.Context, awsCfg aws.Config, regions func queryRDSInstancesInRegion(ctx context.Context, awsCfg aws.Config, regionName string, instanceVersions map[string][]InstanceEngineVersion, mu *sync.Mutex) error { regionCfg := awsCfg.Copy() regionCfg.Region = regionName - rdsClient := awsrds.NewFromConfig(regionCfg) + return queryRDSInstancesPaged(ctx, awsrds.NewFromConfig(regionCfg), regionName, instanceVersions, mu) +} +// queryRDSInstancesPaged pages through DescribeDBInstances, failing loudly on a +// canceled ctx or when maxInstancePages is reached rather than truncating. +func queryRDSInstancesPaged(ctx context.Context, rdsClient RDSInstancesClient, regionName string, instanceVersions map[string][]InstanceEngineVersion, mu *sync.Mutex) error { var marker *string - for { + for pageIdx := 0; ; pageIdx++ { + if err := ctx.Err(); err != nil { + return err + } + if pageIdx >= maxInstancePages { + return fmt.Errorf("RDS instance pagination cap reached after %d pages in %s", maxInstancePages, regionName) + } localVersions, nextMarker, err := queryRDSInstancesPage(ctx, rdsClient, marker, regionName) if err != nil { return fmt.Errorf("failed to describe RDS instances in %s: %w", regionName, err) @@ -177,7 +198,7 @@ func queryRDSInstancesInRegion(ctx context.Context, awsCfg aws.Config, regionNam } // queryRDSInstancesPage queries a single page of RDS instances. -func queryRDSInstancesPage(ctx context.Context, rdsClient *awsrds.Client, marker *string, regionName string) (versions map[string][]InstanceEngineVersion, nextMarker *string, err error) { +func queryRDSInstancesPage(ctx context.Context, rdsClient RDSInstancesClient, marker *string, regionName string) (versions map[string][]InstanceEngineVersion, nextMarker *string, err error) { input := &awsrds.DescribeDBInstancesInput{Marker: marker} output, err := rdsClient.DescribeDBInstances(ctx, input) if err != nil { diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index a8bb73d9a..4a107384f 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -646,8 +646,8 @@ func applyCoverageAndOverrides(recs []common.Recommendation, cfg Config, coverag // warned about rather than read as "no expiry". func logFamilyExpiry(expiry recommendations.FamilyExpiryReport, windowDays, lookbackDays int) { if expiry.ExpiringNU > 0 { - // The exact deduction is capped by the covered and active NU per family - // (go#342); report the inputs rather than claim a net figure. + // ExpiringNU is the deduction actually applied after the per-family + // covered/active caps (go#342, go#533), so nothing is logged when it is 0. AppLogger.Printf(" ⏰ RDS family sizing: %.0f normalized units of RIs expiring within %d days considered against family coverage; %.0f units of replacement RIs started in the last %d days credited back\n", expiry.ExpiringNU, windowDays, expiry.ReplacementNU, lookbackDays) } if expiry.Unusable > 0 { diff --git a/cmd/multi_service_rds_instances_pages_test.go b/cmd/multi_service_rds_instances_pages_test.go new file mode 100644 index 000000000..4e7b0218c --- /dev/null +++ b/cmd/multi_service_rds_instances_pages_test.go @@ -0,0 +1,44 @@ +package main + +import ( + "context" + "sync" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + awsrds "github.com/aws/aws-sdk-go-v2/service/rds" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// loopingRDSInstancesClient always returns a next marker. +type loopingRDSInstancesClient struct { + calls int + onCall func() +} + +func (c *loopingRDSInstancesClient) DescribeDBInstances(_ context.Context, _ *awsrds.DescribeDBInstancesInput, _ ...func(*awsrds.Options)) (*awsrds.DescribeDBInstancesOutput, error) { + c.calls++ + if c.onCall != nil { + c.onCall() + } + return &awsrds.DescribeDBInstancesOutput{Marker: aws.String("next")}, nil +} + +func TestQueryRDSInstancesPaged_PageCapErrors(t *testing.T) { + client := &loopingRDSInstancesClient{} + var mu sync.Mutex + err := queryRDSInstancesPaged(context.Background(), client, "eu-west-1", map[string][]InstanceEngineVersion{}, &mu) + require.Error(t, err) + assert.Contains(t, err.Error(), "eu-west-1") + assert.Equal(t, maxInstancePages, client.calls) +} + +func TestQueryRDSInstancesPaged_CanceledContextStops(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + client := &loopingRDSInstancesClient{onCall: cancel} + var mu sync.Mutex + err := queryRDSInstancesPaged(ctx, client, "eu-west-1", map[string][]InstanceEngineVersion{}, &mu) + require.ErrorIs(t, err, context.Canceled) + assert.Equal(t, 1, client.calls) +} diff --git a/cmd/rds_family_expiry_command_test.go b/cmd/rds_family_expiry_command_test.go index a82cecddc..b21b20194 100644 --- a/cmd/rds_family_expiry_command_test.go +++ b/cmd/rds_family_expiry_command_test.go @@ -68,6 +68,8 @@ type rdsExpiryScenario struct { unusable int // logLine, when set, must appear in the run log. logLine string + // absentLogLine, when set, must NOT appear in the run log. + absentLogLine string // parent documents the count the commit before the fix produced. parent []int } @@ -181,7 +183,10 @@ func rdsExpiryCase(name string) rdsExpiryScenario { {class: "db.r6g.large", engine: "mysql", count: 4, endDays: 363, startedDaysAgo: 2}, }, recs: one("db.r6g.large", 8), want: []int{0}, parent: []int{3}, - logLine: "16 normalized units of RIs expiring within 30 days considered against family coverage; 16 units of replacement RIs started in the last 30 days credited back", + // Since go#533 ExpiringNU is the deduction actually applied. The + // replacements cancel the expiring RIs here, so it is 0 and the + // "considered against family coverage" line must not be printed. + absentLogLine: "normalized units of RIs expiring within 30 days considered against family coverage", } case "control-replacement-outside-lookback": // Same inventory but the replacements started 45 days ago, outside the @@ -220,6 +225,9 @@ func assertRDSExpiryCSV(t *testing.T, output, name, logs string) { if s.logLine != "" { require.Contains(t, logs, s.logLine) } + if s.absentLogLine != "" { + require.NotContains(t, logs, s.absentLogLine) + } data, err := os.ReadFile(output) if countNonZero(s.want) == 0 { require.ErrorIs(t, err, os.ErrNotExist, "nothing to buy must write no CSV; logs=%s", logs) diff --git a/cmd/secrets_store.go b/cmd/secrets_store.go index b03b56e8d..a20bed1f8 100644 --- a/cmd/secrets_store.go +++ b/cmd/secrets_store.go @@ -2,6 +2,8 @@ package main import ( "context" + "fmt" + "strings" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/secretsmanager" @@ -18,9 +20,19 @@ type SecretsStore interface { // AWSSecretsStore implements SecretsStore using AWS Secrets Manager. type AWSSecretsStore struct { - client *secretsmanager.Client + client secretsListUpdater } +// secretsListUpdater is the subset of the Secrets Manager client the store uses. +type secretsListUpdater interface { + ListSecrets(ctx context.Context, in *secretsmanager.ListSecretsInput, opts ...func(*secretsmanager.Options)) (*secretsmanager.ListSecretsOutput, error) + UpdateSecret(ctx context.Context, in *secretsmanager.UpdateSecretInput, opts ...func(*secretsmanager.Options)) (*secretsmanager.UpdateSecretOutput, error) +} + +// maxSecretListPages caps ListSecrets pagination; a name filter matching more +// than this many pages is a bug or a runaway token, not a real account. +const maxSecretListPages = 200 + // NewAWSSecretsStore creates a new AWS Secrets Manager store. func NewAWSSecretsStore(client *secretsmanager.Client) *AWSSecretsStore { return &AWSSecretsStore{ @@ -39,20 +51,28 @@ func (s *AWSSecretsStore) ListSecrets(ctx context.Context, filter string) ([]str }, } - result, err := s.client.ListSecrets(ctx, input) - if err != nil { - return nil, err - } - - arns := make([]string, 0, len(result.SecretList)) - for _rvc := range result.SecretList { - secret := result.SecretList[_rvc] - if secret.ARN != nil { - arns = append(arns, *secret.ARN) + var arns []string + for page := 0; ; page++ { + if err := ctx.Err(); err != nil { + return nil, err + } + if page >= maxSecretListPages { + return nil, fmt.Errorf("ListSecrets pagination cap reached after %d pages for %q", maxSecretListPages, filter) + } + result, err := s.client.ListSecrets(ctx, input) + if err != nil { + return nil, err + } + for i := range result.SecretList { + if result.SecretList[i].ARN != nil { + arns = append(arns, *result.SecretList[i].ARN) + } + } + if result.NextToken == nil || *result.NextToken == "" { + return arns, nil } + input.NextToken = result.NextToken } - - return arns, nil } // UpdateSecret updates a secret with the given value. @@ -65,3 +85,35 @@ func (s *AWSSecretsStore) UpdateSecret(ctx context.Context, secretID, secretValu _, err := s.client.UpdateSecret(ctx, input) return err } + +// secretARNSuffixLen is the "-" plus six random characters Secrets Manager +// appends to a secret's name in its ARN. +const secretARNSuffixLen = 7 + +// resolveSecretID returns the ARN of the secret named exactly name. The +// ListSecrets name filter is a prefix match with unspecified order, so +// "" also matches "-old"; only ARNs whose secret name equals name +// count. With no exact match from a successful listing it returns name itself, +// so UpdateSecret reports a missing secret; a failed listing and more than one +// exact match are errors, so a denied lookup cannot pass as success. +func resolveSecretID(ctx context.Context, store SecretsStore, name string) (string, error) { + arns, err := store.ListSecrets(ctx, name) + if err != nil { + return "", fmt.Errorf("listing secrets named %q: %w", name, err) + } + var exact []string + for _, arn := range arns { + _, secretName, ok := strings.Cut(arn, ":secret:") + if ok && len(secretName) > secretARNSuffixLen && secretName[:len(secretName)-secretARNSuffixLen] == name { + exact = append(exact, arn) + } + } + switch len(exact) { + case 0: + return name, nil + case 1: + return exact[0], nil + default: + return "", fmt.Errorf("found %d secrets named %q: %v", len(exact), name, exact) + } +} diff --git a/cmd/secrets_store_test.go b/cmd/secrets_store_test.go new file mode 100644 index 000000000..55f424a18 --- /dev/null +++ b/cmd/secrets_store_test.go @@ -0,0 +1,128 @@ +package main + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/secretsmanager" + secretsmgrtypes "github.com/aws/aws-sdk-go-v2/service/secretsmanager/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +const ( + arnPrefix = "arn:aws:secretsmanager:eu-west-1:123456789012:secret:" + wantedARN = arnPrefix + "cudly-AzureCredentials-AbCdEf" + oldARN = arnPrefix + "cudly-AzureCredentials-old-GhIjKl" + wantedName = "cudly-AzureCredentials" +) + +func listStore(arns ...string) *MockSecretsStore { + m := NewMockSecretsStore() + m.listSecretsFunc = func(context.Context, string) ([]string, error) { return arns, nil } + return m +} + +func TestResolveSecretID(t *testing.T) { + tests := []struct { + name string + arns []string + want string + wantErr bool + }{ + {"exact match after prefix match", []string{oldARN, wantedARN}, wantedARN, false}, + {"prefix match only falls back to name", []string{oldARN}, wantedName, false}, + {"no results falls back to name", nil, wantedName, false}, + {"two exact matches error", []string{wantedARN, arnPrefix + "cudly-AzureCredentials-MnOpQr"}, "", true}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := resolveSecretID(context.Background(), listStore(tt.arns...), wantedName) + if tt.wantErr { + require.Error(t, err) + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } +} + +func TestStoreAzureCredentials_SkipsPrefixMatch(t *testing.T) { + store := listStore(oldARN, wantedARN) + creds := AzureCredentials{TenantID: testTenantID, ClientID: testSPAppID, ClientSecret: testSPSecret, SubscriptionID: testSubID} + require.NoError(t, storeAzureCredentials(context.Background(), store, "cudly", creds)) + assert.Contains(t, store.updatedSecrets, wantedARN) + assert.NotContains(t, store.updatedSecrets, oldARN) +} + +func TestStoreGCPCredentials_SkipsPrefixMatch(t *testing.T) { + gcpOld := arnPrefix + "cudly-GCPCredentials-old-GhIjKl" + gcpWanted := arnPrefix + "cudly-GCPCredentials-AbCdEf" + store := listStore(gcpOld, gcpWanted) + creds := `{"type":"service_account","project_id":"p","client_email":"a@b.c","private_key":"k"}` + require.NoError(t, storeGCPCredentials(context.Background(), store, "cudly", creds)) + assert.Contains(t, store.updatedSecrets, gcpWanted) + assert.NotContains(t, store.updatedSecrets, gcpOld) +} + +type fakeSecretsClient struct { + pages []*secretsmanager.ListSecretsOutput + calls int + loop bool +} + +func (f *fakeSecretsClient) ListSecrets(_ context.Context, in *secretsmanager.ListSecretsInput, _ ...func(*secretsmanager.Options)) (*secretsmanager.ListSecretsOutput, error) { + f.calls++ + if f.loop { + return &secretsmanager.ListSecretsOutput{NextToken: aws.String("again")}, nil + } + idx := 0 + if in.NextToken != nil { + idx = 1 + } + return f.pages[idx], nil +} + +func (f *fakeSecretsClient) UpdateSecret(context.Context, *secretsmanager.UpdateSecretInput, ...func(*secretsmanager.Options)) (*secretsmanager.UpdateSecretOutput, error) { + return &secretsmanager.UpdateSecretOutput{}, nil +} + +func TestAWSSecretsStore_ListSecretsPaginates(t *testing.T) { + fake := &fakeSecretsClient{pages: []*secretsmanager.ListSecretsOutput{ + {SecretList: []secretsmgrtypes.SecretListEntry{{ARN: aws.String(oldARN)}}, NextToken: aws.String("t2")}, + {SecretList: []secretsmgrtypes.SecretListEntry{{ARN: aws.String(wantedARN)}}}, + }} + store := &AWSSecretsStore{client: fake} + got, err := resolveSecretID(context.Background(), store, wantedName) + require.NoError(t, err) + assert.Equal(t, wantedARN, got) + assert.Equal(t, 2, fake.calls) +} + +func TestAWSSecretsStore_ListSecretsFailsAtPageCap(t *testing.T) { + fake := &fakeSecretsClient{loop: true} + _, err := (&AWSSecretsStore{client: fake}).ListSecrets(context.Background(), wantedName) + require.ErrorContains(t, err, "pagination cap") + assert.Equal(t, maxSecretListPages, fake.calls) +} + +func TestAWSSecretsStore_ListSecretsHonorsCanceledContext(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + fake := &fakeSecretsClient{loop: true} + _, err := (&AWSSecretsStore{client: fake}).ListSecrets(ctx, wantedName) + require.ErrorIs(t, err, context.Canceled) +} + +func TestResolveSecretID_ListErrorFails(t *testing.T) { + m := NewMockSecretsStore() + m.listSecretsFunc = func(context.Context, string) ([]string, error) { return nil, errors.New("AccessDenied") } + _, err := resolveSecretID(context.Background(), m, wantedName) + require.ErrorContains(t, err, "AccessDenied") + creds := AzureCredentials{TenantID: testTenantID, ClientID: testSPAppID, ClientSecret: testSPSecret, SubscriptionID: testSubID} + require.Error(t, storeAzureCredentials(context.Background(), m, "cudly", creds)) + assert.Empty(t, m.updatedSecrets) +} diff --git a/docs/cli/README.md b/docs/cli/README.md index 8313ef871..93583b87a 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -36,7 +36,7 @@ All flags belong to the root command unless noted otherwise. | `--target-coverage` | `-u` | `0` (disabled) | Target percentage (0-100) of historical average hourly usage to cover with commitments. Sizes each recommendation to `floor(avg * target/100)`, leaving the remainder on-demand. Overrides `--coverage` when non-zero. Pairs with `--rebuy-window-days` and `--min-pool-size` (see [filtering.md](filtering.md)). | | `--coverage-lookback-days` | | `30` | Calendar days of historical demand fed to `GetReservationCoverage` when computing the existing-RI coverage map for `--target-coverage` sizing. Match this to your AWS console coverage report window to reconcile cudly's `ExistingCoverage` column against the console export. Only affects `--target-coverage`. | | `--override-count` | | `0` (disabled) | Replace every recommendation's count with this fixed number, rescaling that row's costs and savings by the same ratio so they describe the quantity that will actually be bought. Savings Plans are exempt and pass through unchanged: an SP commitment is priced in dollars per hour, not in instances. Rows with a non-positive count are exempt too, having no quantity to rescale from. Overriding above the quantity the provider's figures cover keeps the costs accurate but extrapolates the savings past the observed demand; all three cases are named on stdout. Applied before `--max-instances`. Useful when testing a specific purchase size. | -| `--max-instances` | | `0` (no limit) | Hard cap on the total number of instances purchased across all recommendations. Applied after coverage scaling, once per run across every service and region (not per service or per region). The cap is run-wide on both paths, but the two rank on different keys: recommendation-driven runs keep the highest savings *percentage*, while `--input-csv` rows are ranked on savings *per instance* (`EstimatedSavings / Count`), never on file order. A binding cap is refused if any row lacks an `EstimatedSavings` value to rank on. A recommendation the cap would truncate below `--min-count` is dropped instead. See [filtering.md](filtering.md). | +| `--max-instances` | | `0` (no limit) | Hard cap on the total number of instances purchased across all recommendations. Applied after coverage scaling, once per run across every service and region (not per service or per region). The cap is run-wide on both paths, but the two rank on different keys: recommendation-driven runs keep the highest savings *percentage*, while `--input-csv` rows are ranked on savings *per instance* (`EstimatedSavings / Count`), never on file order. A binding cap is refused if any row lacks an `EstimatedSavings` value to rank on. A recommendation the cap would truncate below `--min-count` is dropped instead. Does not apply to Savings Plans. See [filtering.md](filtering.md). | ### Purchase terms @@ -59,7 +59,7 @@ All flags belong to the root command unless noted otherwise. |------|---------|-------------| | `--min-savings-pct` | `0` (no filter) | Drop recommendations whose estimated savings percentage is below this threshold. This is a **percentage** (e.g. `10` = 10%), not a dollar amount. See [filtering.md](filtering.md) for the naming distinction from the GUI `min_savings` dollar filter. | | `--max-break-even-months` | `0` (no filter) | Drop recommendations whose break-even period exceeds this many months. `0` disables the filter. See [filtering.md](filtering.md). | -| `--min-count` | `0` (no filter) | Drop recommendations for fewer than this many instances. `0` disables the filter. See [filtering.md](filtering.md). | +| `--min-count` | `0` (no filter) | Drop recommendations for fewer than this many instances. `0` disables the filter. Does not apply to Savings Plans. See [filtering.md](filtering.md). | ### Scoping filters diff --git a/docs/cli/cloud-setup.md b/docs/cli/cloud-setup.md index c22b0024a..9ba312610 100644 --- a/docs/cli/cloud-setup.md +++ b/docs/cli/cloud-setup.md @@ -31,7 +31,7 @@ cudly configure-azure [flags] | `--profile` | (AWS default chain) | AWS profile to use when writing to Secrets Manager. | | `--tenant-id` | | Azure AD Tenant ID (UUID format). | | `--client-id` | | Azure Service Principal Client ID (UUID format). | -| `--client-secret` | | Azure Service Principal Client Secret. Read securely from stdin if omitted. | +| `--client-secret` | | Deprecated: a secret on the command line leaks into shell history and process listings. Omit it and enter it at the interactive Client Secret prompt. The flag still works. | | `--subscription-id` | | Azure Subscription ID (UUID format). | | `--interactive` / `-i` | `false` | Prompt for all credential fields interactively, even if some are provided as flags. | | `--skip-setup` | `false` | Skip the guided Azure CLI steps (az login, az account list, az ad sp create-for-rbac). Use when you already have a Service Principal and just want to store the credentials. | @@ -53,7 +53,7 @@ If step 3 creates the Service Principal, its credentials (including the subscrip ### Non-interactive usage ```bash -# Provide all credentials as flags (--client-secret is read from a variable to avoid shell history) +# Provide all credentials as flags (--client-secret is deprecated; it is read from a variable to avoid shell history) AZURE_SECRET="$(cat /run/secrets/azure-client-secret)" cudly configure-azure \ --stack-name prod-cudly \ @@ -177,11 +177,10 @@ Scope notes: billing account, folder or organization, because the pinned client never queries those parents. Billing-account roles such as `roles/billing.viewer` are not needed for this call. -- Other services: the GCP provider also queries Cloud SQL and Memorystore - recommenders (`google.cloudsql.instance.PerformanceRecommender`, - `google.memorystore.redis.PerformanceRecommender`). The wizard provisions - nothing for them; those calls return a permission error, which is reported - as a warning for that service while other services continue. +- Other services: the GCP provider no longer queries the Cloud SQL and + Memorystore recommenders. Those commitments are spend-based and scoped to a + billing account, so they are handled only on explicit request, and the wizard + provisions nothing for them. Rerunning the wizard does not remove broad grants from older installations. diff --git a/docs/cli/filtering.md b/docs/cli/filtering.md index 3d31ec06b..512a081df 100644 --- a/docs/cli/filtering.md +++ b/docs/cli/filtering.md @@ -135,10 +135,10 @@ Threshold filters drop individual recommendations that fall below a quality or s ### --min-count ```text ---min-count Minimum instance count (0 = no filter) +--min-count Minimum instance count (0 = no filter; does not apply to Savings Plans) ``` -Drop recommendations where the adjusted instance count (after coverage scaling) is below this number. Useful for ignoring one-off or very small recommendations that are not worth the operational overhead. +Drop recommendations where the adjusted instance count (after coverage scaling) is below this number. Useful for ignoring one-off or very small recommendations that are not worth the operational overhead. Savings Plans are priced in dollars per hour, not instances, so the floor does not apply to them. The floor applies on both paths, including `--input-csv`: a row already below it is dropped, and a row `--max-instances` would truncate below it is dropped rather than purchased short. On `--input-csv` runs it is re-checked after existing matching commitments are deducted, so a row whose remaining count falls under the floor is dropped rather than purchased at the smaller size. @@ -189,10 +189,10 @@ cudly --services ec2 --max-break-even-months 18 ### --max-instances ```text ---max-instances Hard cap on total instances purchased (0 = no limit) +--max-instances Hard cap on total instances purchased (0 = no limit; does not apply to Savings Plans) ``` -Cap the total number of instances purchased across all recommendations after coverage scaling. Applied as a final cut after all other filters. This is a safety net to prevent unexpectedly large batch purchases, not a primary sizing control. +Cap the total number of instances purchased across all recommendations after coverage scaling. Applied as a final cut after all other filters. This is a safety net to prevent unexpectedly large batch purchases, not a primary sizing control. It does not apply to Savings Plans: they are priced in dollars per hour, so they neither count against the cap nor get reduced or dropped by it. The cap covers the whole run on both code paths: it is applied once to the combined set of recommendations, never once per service or per region. diff --git a/docs/cli/purchase-safety.md b/docs/cli/purchase-safety.md index 2be678926..5c36d2d64 100644 --- a/docs/cli/purchase-safety.md +++ b/docs/cli/purchase-safety.md @@ -131,6 +131,6 @@ Before any real purchase run: 2. Check that `--audit-log` points to a readable, writable, durable location. 3. If using `--target-coverage`, verify `--rebuy-window-days` is set appropriately for your RI renewal cadence. 4. Narrow the scope with `--include-regions`, `--include-accounts`, or `--min-savings-pct` before buying across all services. -5. Consider `--max-instances` as a final safety cap for a first run. +5. Consider `--max-instances` as a final safety cap for a first run. It limits instance-based reservations only; Savings Plans are exempt, so it does not cap their hourly commitment spend. 6. Set `--idempotency-window` to cover the time since the earlier run (e.g. `72h` for a re-run two days later), and note that a failed existing-commitments lookup makes a dry run proceed un-deduplicated with a warning, while a `--purchase` run refuses to purchase for that (service, region) instead ([#1941](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1941)); watch the log for either signal and check the audit log afterward. 7. If an AI agent or other automation drives `cudly`, have it hand off the dry-run recommendation to a human, who reviews the totals and confirms `--purchase` at a terminal. See [Automation and AI agents](#automation-and-ai-agents). diff --git a/go.mod b/go.mod index e256a846a..596c8284f 100644 --- a/go.mod +++ b/go.mod @@ -82,10 +82,10 @@ 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-20261016175124-570d168c6bc7 - github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261016165653-512e42fe37c6 - github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261013212804-9b35837e78d1 - github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261013212804-9b35837e78d1 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261019225641-cfe8ac29cee7 + github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261019225641-cfe8ac29cee7 + github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261019225641-cfe8ac29cee7 + github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261019225641-cfe8ac29cee7 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/aws/aws-sdk-go-v2/service/sts v1.33.17 diff --git a/go.sum b/go.sum index 83477ee9e..17f053fe9 100644 --- a/go.sum +++ b/go.sum @@ -78,14 +78,14 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.54.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0 h1:s0WlVbf9qpvkh1c/uDAPElam0WrL7fHRIidgZJ7UqZI= 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-20261016175124-570d168c6bc7 h1:mAvqQSMkTuw4P8m8o+/wdyoxGE9L6eiEzZFanGXqYcI= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261016175124-570d168c6bc7/go.mod h1:SCDEgHG6evjPh0RsoXof+J3aMmGXNFMrwbJIle7rMpQ= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261016165653-512e42fe37c6 h1:nxcsaVj7BKH0/I9wiTvraQPivKABf/GzE7Ecwm47u9U= -github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261016165653-512e42fe37c6/go.mod h1:uOUuTVRjTm0Fhu/Vra2M2NES0w4pvZUSfZLsGyCd18s= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261013212804-9b35837e78d1 h1:aYYu7JDJQcoriUKg/m+IgTJruc2ugfBLXafAC2LQy00= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261013212804-9b35837e78d1/go.mod h1:8vqO2ZlDzjX/Z4bESZVp/IenhTifkySeF0VJwQKihLA= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261013212804-9b35837e78d1 h1:LPI8x3lOnrvr4w3oX8cHsCLoXuCSLSUhcHbe4WCMUSQ= -github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261013212804-9b35837e78d1/go.mod h1:J4H0b34EwQQ4p9cM3amsIU9tFmbzecS8E00ybjVp56g= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261019225641-cfe8ac29cee7 h1:DpaGgEGdWEp4M9TbOgk7/n01bgaHi+JMqRv69KFr3S8= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261019225641-cfe8ac29cee7/go.mod h1:SCDEgHG6evjPh0RsoXof+J3aMmGXNFMrwbJIle7rMpQ= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261019225641-cfe8ac29cee7 h1:38G9GPC1NTtFSJJpXidzw984Jr7ahcjC1ktZaU18b4s= +github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261019225641-cfe8ac29cee7/go.mod h1:p6TrYRa4+lvcdsPCAJ+sa0tI0sRfGQ/0YT1Af3fZk3w= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261019225641-cfe8ac29cee7 h1:cIrFuU5ZBAhZEqnwmecOJcRDttBdPyjakC/9G040tRA= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261019225641-cfe8ac29cee7/go.mod h1:iOwl8e9EKGb3VQ4u/YjiqVzglhD6/GX1ABC3tBkqu0A= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261019225641-cfe8ac29cee7 h1:AHtImpEpBxhmwK1Hs5rV+QlcvXnPx1DJCmtD2k+aCog= +github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20261019225641-cfe8ac29cee7/go.mod h1:03vjHt5wlHtZEFAFOhosA+paDSqacWnKg9cVa68QnJ0= 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=