fix(cli): close the remaining #1613 findings

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 `<name>-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`

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")

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)
+}

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)

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

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))
+	})
+}

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. "+