From 48ce12880f8c6a0586893b25bf22080c50702397 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:21:04 +0200 Subject: [PATCH 01/17] fix(azure): make Enter skip the create-service-principal prompt Enter ran Entra app, password and role-assignment creation. Skip is now the default, matching the GCP wizard (#1613, A10-030). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +++ cmd/configure_azure.go | 6 ++++-- cmd/configure_azure_secret_test.go | 20 ++++++++++++++++++++ 3 files changed, 27 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 4e1bd23a3..e064a7919 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `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 diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 2cc5cca73..983cb6788 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -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..8873ed3fc 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. From 49ae5908393ddfd48e85959a183ef6ef881eb405 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:29:23 +0200 Subject: [PATCH 02/17] fix(rds): cap and cancel DescribeDBInstances paging per region queryRDSInstancesInRegion paged without a cap or ctx check. Add a 200-page cap that errors naming the region, a ctx.Err() check, and a client interface seam for tests (#1613, L2). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 ++ cmd/multi_service_engine_versions.go | 27 ++++++++++-- cmd/multi_service_rds_instances_pages_test.go | 44 +++++++++++++++++++ 3 files changed, 71 insertions(+), 3 deletions(-) create mode 100644 cmd/multi_service_rds_instances_pages_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e064a7919..ba55eb91c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- 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). 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_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) +} From 4f649ab511eb5dad090fc0e93843b1371846c260 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:43:41 +0200 Subject: [PATCH 03/17] fix(configure): select the exact-named secret, not the first prefix match Secrets Manager name filters are prefix matches in unspecified order, so -old could be updated instead of . Share one resolveSecretID helper between the Azure and GCP wizards; two exact matches now error (#1613, M5). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 ++ cmd/configure_azure.go | 10 ++---- cmd/configure_gcp.go | 10 ++---- cmd/secrets_store.go | 33 ++++++++++++++++++++ cmd/secrets_store_test.go | 65 +++++++++++++++++++++++++++++++++++++++ 5 files changed, 107 insertions(+), 14 deletions(-) create mode 100644 cmd/secrets_store_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index ba55eb91c..a2e0af80d 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `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). diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index 983cb6788..b68d48f5f 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -144,13 +144,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 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/secrets_store.go b/cmd/secrets_store.go index b03b56e8d..952989f71 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" @@ -65,3 +67,34 @@ 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 (or a failed listing) it returns name itself, so +// UpdateSecret reports a missing secret; more than one exact match is an error. +func resolveSecretID(ctx context.Context, store SecretsStore, name string) (string, error) { + arns, err := store.ListSecrets(ctx, name) + if err != nil { + return name, nil + } + 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..3f518ce08 --- /dev/null +++ b/cmd/secrets_store_test.go @@ -0,0 +1,65 @@ +package main + +import ( + "context" + "testing" + + "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) +} From 0352e94454fe46abb3859e6429c10dbad911c037 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:54:07 +0200 Subject: [PATCH 04/17] docs(azure): drop --client-secret from the configure-azure help example Point to the interactive prompt instead so the secret stays out of shell history and process listings (#1613, L1). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 ++ cmd/configure_azure.go | 5 +++-- cmd/configure_azure_secret_test.go | 5 +++++ 3 files changed, 10 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index a2e0af80d..261f672c5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- 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). diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index b68d48f5f..e8a967279 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: diff --git a/cmd/configure_azure_secret_test.go b/cmd/configure_azure_secret_test.go index 8873ed3fc..a9df2feb4 100644 --- a/cmd/configure_azure_secret_test.go +++ b/cmd/configure_azure_secret_test.go @@ -360,3 +360,8 @@ 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") +} From 75f5254a16b3a943f80c1e5cf61d261f7695a1db Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 00:58:45 +0200 Subject: [PATCH 05/17] feat(azure): deprecate the configure-azure --client-secret flag A secret on the command line leaks into shell history and process listings. The flag still works; the deprecation message points to the interactive prompt (#1613, L1). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 2 ++ cmd/configure_azure.go | 3 +++ cmd/configure_azure_secret_test.go | 6 ++++++ docs/cli/cloud-setup.md | 4 ++-- 4 files changed, 13 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 261f672c5..a1027e8d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,8 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- `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 diff --git a/cmd/configure_azure.go b/cmd/configure_azure.go index e8a967279..69f17990b 100644 --- a/cmd/configure_azure.go +++ b/cmd/configure_azure.go @@ -116,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)") diff --git a/cmd/configure_azure_secret_test.go b/cmd/configure_azure_secret_test.go index a9df2feb4..9b755abf5 100644 --- a/cmd/configure_azure_secret_test.go +++ b/cmd/configure_azure_secret_test.go @@ -365,3 +365,9 @@ func TestCollectAzureCredentials_PromptWording(t *testing.T) { 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/docs/cli/cloud-setup.md b/docs/cli/cloud-setup.md index c22b0024a..1eac93263 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 \ From f7d5ac0394d3506a3a9b290bf09c7a2b3aefc39d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 01:04:51 +0200 Subject: [PATCH 06/17] fix(filters): exempt Savings Plans from --max-instances and --min-count A Savings Plan recommendation always has Count 1, which is not an instance count. It no longer uses cap budget, is not reduced by the cap, and is not dropped by the cli-side min-count floors (#1613, M7). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 + cmd/helpers.go | 45 ++++++++++++++ cmd/instance_limit_savings_plan_test.go | 83 +++++++++++++++++++++++++ cmd/main.go | 4 +- cmd/multi_service.go | 15 +++-- cmd/multi_service_csv_cap.go | 6 +- docs/cli/README.md | 4 +- docs/cli/filtering.md | 8 +-- 8 files changed, 153 insertions(+), 15 deletions(-) create mode 100644 cmd/instance_limit_savings_plan_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a1027e8d3..083c38f25 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -13,6 +13,9 @@ 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 --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; diff --git a/cmd/helpers.go b/cmd/helpers.go index 1b99b15ae..ba99f61d5 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -146,6 +146,44 @@ 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 +} + +// splitSavingsPlans separates Savings Plans from instance-based recommendations, +// keeping each group's order. +func splitSavingsPlans(recs []common.Recommendation) (sps, others []common.Recommendation) { + for i := range recs { + if isSavingsPlan(recs[i]) { + sps = append(sps, recs[i]) + } else { + others = append(others, recs[i]) + } + } + return sps, 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 +240,9 @@ 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 and consume no budget. func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []common.Recommendation { if maxInstances <= 0 { return recs @@ -212,6 +253,10 @@ 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 } diff --git a/cmd/instance_limit_savings_plan_test.go b/cmd/instance_limit_savings_plan_test.go new file mode 100644 index 000000000..a04d61e7a --- /dev/null +++ b/cmd/instance_limit_savings_plan_test.go @@ -0,0 +1,83 @@ +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) + 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) + 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) +} 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..bfbe56cf2 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 := splitSavingsPlans(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 := splitSavingsPlans(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..fc50f8f10 100644 --- a/cmd/multi_service_csv_cap.go +++ b/cmd/multi_service_csv_cap.go @@ -56,13 +56,15 @@ func applyMinCountFloor(recs []common.Recommendation, minCount int) []common.Rec if minCount <= 0 { return recs } - scored := scorer.Score(recs, scorer.Config{MinCount: minCount}) + // Savings Plans carry a placeholder Count of 1, so the floor does not apply. + sps, others := splitSavingsPlans(recs) + scored := scorer.Score(others, 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) } - return scored.Passed + return append(scored.Passed, sps...) } // applyThresholdGates drops recommendations that fail --min-savings-pct or 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/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. From 8f5b463eaaa3485ba6ef333c5d53d3dc38d374b3 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 01:08:05 +0200 Subject: [PATCH 07/17] fix(filters): keep input order in applyMinCountFloor Scoring each row separately keeps mixed RI and Savings Plan sets in their input order instead of the scorer sort or SPs appended last. Tests assert the order (#1613, M7). Co-Authored-By: Claude Opus 5.5 --- cmd/instance_limit_savings_plan_test.go | 15 +++++++++++++++ cmd/multi_service_csv_cap.go | 23 ++++++++++++++++------- 2 files changed, 31 insertions(+), 7 deletions(-) diff --git a/cmd/instance_limit_savings_plan_test.go b/cmd/instance_limit_savings_plan_test.go index a04d61e7a..f8ba12589 100644 --- a/cmd/instance_limit_savings_plan_test.go +++ b/cmd/instance_limit_savings_plan_test.go @@ -59,6 +59,7 @@ func TestApplyGlobalInstanceLimit_SavingsPlanExempt(t *testing.T) { } } assert.Equal(t, tt.wantRIs, ris) + assertSameRelativeOrder(t, tt.recs, got) assert.Equal(t, tt.wantSP, sp) assert.Len(t, got, tt.wantSize) }) @@ -69,6 +70,7 @@ 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) { @@ -81,3 +83,16 @@ func TestApplyMinCountFloor_SavingsPlanExempt(t *testing.T) { 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++ + } +} diff --git a/cmd/multi_service_csv_cap.go b/cmd/multi_service_csv_cap.go index fc50f8f10..1bb54003a 100644 --- a/cmd/multi_service_csv_cap.go +++ b/cmd/multi_service_csv_cap.go @@ -57,14 +57,23 @@ func applyMinCountFloor(recs []common.Recommendation, minCount int) []common.Rec return recs } // Savings Plans carry a placeholder Count of 1, so the floor does not apply. - sps, others := splitSavingsPlans(recs) - scored := scorer.Score(others, 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) + // 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 append(scored.Passed, sps...) + return kept } // applyThresholdGates drops recommendations that fail --min-savings-pct or From 4956e8e0edcf899df8edfac074007bfb0dcb4022 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 01:09:21 +0200 Subject: [PATCH 08/17] refactor(filters): return only instance rows from the Savings Plan split The sps result was unused and failed the unparam lint (#1613). Co-Authored-By: Claude Opus 5.5 --- cmd/helpers.go | 12 +++++------- cmd/multi_service.go | 4 ++-- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/cmd/helpers.go b/cmd/helpers.go index ba99f61d5..be685b241 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -152,17 +152,15 @@ func isSavingsPlan(rec common.Recommendation) bool { return rec.CommitmentType == common.CommitmentSavingsPlan } -// splitSavingsPlans separates Savings Plans from instance-based recommendations, -// keeping each group's order. -func splitSavingsPlans(recs []common.Recommendation) (sps, others []common.Recommendation) { +// 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]) { - sps = append(sps, recs[i]) - } else { + if !isSavingsPlan(recs[i]) { others = append(others, recs[i]) } } - return sps, others + return others } // mergeKeepingOrder returns the entries of original that survive, in original diff --git a/cmd/multi_service.go b/cmd/multi_service.go index bfbe56cf2..56848e503 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -389,7 +389,7 @@ 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 { - _, others := splitSavingsPlans(recs) + others := withoutSavingsPlans(recs) return cfg.MaxInstances > 0 && CalculateTotalInstances(others) > int(cfg.MaxInstances) } @@ -421,7 +421,7 @@ func applyGlobalInstanceLimit(passed []common.Recommendation, cfg Config, rule r // 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 := splitSavingsPlans(passed) + others := withoutSavingsPlans(passed) totalBefore := CalculateTotalInstances(others) limited := ApplyInstanceLimit(others, cfg.MaxInstances) limited, belowMin := dropTruncatedBelowMinCount(limited, cfg.MinCount) From d7f95026ca47abaff784d2cc7cafa49957e09cec Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 01:23:51 +0200 Subject: [PATCH 09/17] build(deps): bump cloud-commitments-go to the scorer Savings Plan exemption The pinned pkg predates go#536, so --min-count >= 2 still dropped every Savings Plan on the scoring path. Bump pkg and the three providers to cfe8ac29 (the providers must move with pkg: the old azure provider no longer compiles against it). The rds expiry log line is no longer printed when the real deduction is 0 (go#533), so that scenario drops its log assertion (#1613, M7). Co-Authored-By: Claude Opus 5.5 --- cmd/instance_limit_savings_plan_test.go | 12 ++++++++++++ cmd/rds_family_expiry_command_test.go | 4 +++- go.mod | 8 ++++---- go.sum | 16 ++++++++-------- 4 files changed, 27 insertions(+), 13 deletions(-) diff --git a/cmd/instance_limit_savings_plan_test.go b/cmd/instance_limit_savings_plan_test.go index f8ba12589..e563fac65 100644 --- a/cmd/instance_limit_savings_plan_test.go +++ b/cmd/instance_limit_savings_plan_test.go @@ -96,3 +96,15 @@ func assertSameRelativeOrder(t *testing.T, in, got []common.Recommendation) { 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) + + var types []common.CommitmentType + for _, r := range result.Passed { + types = append(types, r.CommitmentType) + } + assert.ElementsMatch(t, []common.CommitmentType{common.CommitmentSavingsPlan, common.CommitmentReservedInstance}, types) +} diff --git a/cmd/rds_family_expiry_command_test.go b/cmd/rds_family_expiry_command_test.go index a82cecddc..1b7fa6bf5 100644 --- a/cmd/rds_family_expiry_command_test.go +++ b/cmd/rds_family_expiry_command_test.go @@ -181,7 +181,9 @@ 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", + // No logLine: since go#533 ExpiringNU is the deduction actually applied, + // which is 0 here once the replacements are credited, so the + // "considered against family coverage" line is not printed. } case "control-replacement-outside-lookback": // Same inventory but the replacements started 45 days ago, outside the 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= From ff86a2a62bfe49eb440028e088277440f786e266 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 01:24:46 +0200 Subject: [PATCH 10/17] fix(filters): keep Savings Plans after the instance budget is spent ApplyInstanceLimit stopped scanning once the budget hit 0, dropping any Savings Plan later in the slice. Skip instance rows instead of breaking (#1613, M7). Co-Authored-By: Claude Opus 5.5 --- cmd/helpers.go | 5 +++-- cmd/instance_limit_savings_plan_test.go | 8 ++++++++ 2 files changed, 11 insertions(+), 2 deletions(-) diff --git a/cmd/helpers.go b/cmd/helpers.go index be685b241..7fef0217e 100644 --- a/cmd/helpers.go +++ b/cmd/helpers.go @@ -240,7 +240,8 @@ func applySizing(recs []common.Recommendation, cfg Config, coverage float64, dro // Count by refreshCountLinear (#1845). // // Savings Plans are exempt: their Count is always 1 and is not an instance -// count, so they pass through unchanged and consume no budget. +// 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 @@ -256,7 +257,7 @@ func ApplyInstanceLimit(recs []common.Recommendation, maxInstances int32) []comm 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 index e563fac65..6681e1b03 100644 --- a/cmd/instance_limit_savings_plan_test.go +++ b/cmd/instance_limit_savings_plan_test.go @@ -108,3 +108,11 @@ func TestScoreLimitAndDisplay_MinCountKeepsSavingsPlan(t *testing.T) { } 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])) +} From 6e87c4243fbc936ee90a2ced82bdbcafa4c00893 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 01:27:28 +0200 Subject: [PATCH 11/17] test(filters): assert exact output order for interleaved Savings Plans The subsequence check let a mutation that emits SPs last pass. Add exact slices for the cap, cap plus min-count, and the floor, and preallocate a slice flagged by prealloc (#1613). Co-Authored-By: Claude Opus 5.5 --- cmd/instance_limit_savings_plan_test.go | 39 ++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) diff --git a/cmd/instance_limit_savings_plan_test.go b/cmd/instance_limit_savings_plan_test.go index 6681e1b03..e2e27538d 100644 --- a/cmd/instance_limit_savings_plan_test.go +++ b/cmd/instance_limit_savings_plan_test.go @@ -102,7 +102,7 @@ func assertSameRelativeOrder(t *testing.T, in, got []common.Recommendation) { func TestScoreLimitAndDisplay_MinCountKeepsSavingsPlan(t *testing.T) { result := scoreLimitAndDisplay([]common.Recommendation{mkSP(), mkRI("a", 3)}, Config{MinCount: 2}, nil) - var types []common.CommitmentType + types := make([]common.CommitmentType, 0, len(result.Passed)) for _, r := range result.Passed { types = append(types, r.CommitmentType) } @@ -116,3 +116,40 @@ func TestApplyInstanceLimit_SavingsPlanAfterBudgetSpent(t *testing.T) { 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)) + }) +} From cfaa43b88199a06b333bb857822902d7e7051a2d Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 01:29:39 +0200 Subject: [PATCH 12/17] docs(safety): note that --max-instances does not cap Savings Plans Co-Authored-By: Claude Opus 5.5 --- docs/cli/purchase-safety.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) 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). From ae7f62b5f856186d35e515fe229b7b37e070724c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 01:32:30 +0200 Subject: [PATCH 13/17] test(rds): assert the expiry log line is absent when the deduction is 0 Since go#533 ExpiringNU is the deduction actually applied; the replacement-bought rerun deducts 0, so the line must not appear. Update the logFamilyExpiry comment to match (#1613). Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service_helpers.go | 4 ++-- cmd/rds_family_expiry_command_test.go | 12 +++++++++--- 2 files changed, 11 insertions(+), 5 deletions(-) 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/rds_family_expiry_command_test.go b/cmd/rds_family_expiry_command_test.go index 1b7fa6bf5..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,9 +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}, - // No logLine: since go#533 ExpiringNU is the deduction actually applied, - // which is 0 here once the replacements are credited, so the - // "considered against family coverage" line is not printed. + // 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 @@ -222,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) From fa1feaf33d8a7bd30d6eef58e232101c1241ae9a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 01:36:33 +0200 Subject: [PATCH 14/17] docs: describe the GCP provider bump and drop the stale recommender note The bumped gcp provider no longer queries the Cloud SQL and Memorystore PerformanceRecommenders. Fix the cloud-setup bullet and add a CHANGELOG entry for the library bump (#1613). Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 6 ++++++ docs/cli/cloud-setup.md | 9 ++++----- 2 files changed, 10 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 083c38f25..d42eab59c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -137,6 +137,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/docs/cli/cloud-setup.md b/docs/cli/cloud-setup.md index 1eac93263..9ba312610 100644 --- a/docs/cli/cloud-setup.md +++ b/docs/cli/cloud-setup.md @@ -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. From 1175bd2307e6c437e0e4a1037981e08caa7f7e7f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 02:08:01 +0200 Subject: [PATCH 15/17] fix(cap): ignore Savings Plans in requireRankingSignal A Savings Plan neither consumes the --max-instances cap nor needs ranking, so a zero-savings SP no longer fails a run whose cap binds on other rows. Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service_csv_cap.go | 10 ++++++---- cmd/multi_service_csv_cap_ranking_test.go | 22 ++++++++++++++++++++++ 2 files changed, 28 insertions(+), 4 deletions(-) create mode 100644 cmd/multi_service_csv_cap_ranking_test.go diff --git a/cmd/multi_service_csv_cap.go b/cmd/multi_service_csv_cap.go index 1bb54003a..2aeae92cf 100644 --- a/cmd/multi_service_csv_cap.go +++ b/cmd/multi_service_csv_cap.go @@ -163,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 { @@ -184,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})) +} From 76a456bffbade678328720ebaa95aee1045c180f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 02:08:58 +0200 Subject: [PATCH 16/17] fix(secrets): paginate ListSecrets in AWSSecretsStore ListSecrets read only the first page and ignored NextToken, so the exact secret on a later page was missed. Loop on NextToken with a page cap and a ctx check, failing loudly at the cap. Co-Authored-By: Claude Opus 5.5 --- cmd/secrets_store.go | 44 +++++++++++++++++++++++---------- cmd/secrets_store_test.go | 52 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 83 insertions(+), 13 deletions(-) diff --git a/cmd/secrets_store.go b/cmd/secrets_store.go index 952989f71..4c82d675c 100644 --- a/cmd/secrets_store.go +++ b/cmd/secrets_store.go @@ -20,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{ @@ -41,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. diff --git a/cmd/secrets_store_test.go b/cmd/secrets_store_test.go index 3f518ce08..a515d36e2 100644 --- a/cmd/secrets_store_test.go +++ b/cmd/secrets_store_test.go @@ -4,6 +4,9 @@ import ( "context" "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" ) @@ -63,3 +66,52 @@ func TestStoreGCPCredentials_SkipsPrefixMatch(t *testing.T) { 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) +} From 161295dacae0be24f3baf1d59bb59535f17f1ff8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 20 Oct 2026 02:09:53 +0200 Subject: [PATCH 17/17] fix(secrets): fail configure when the secret listing fails resolveSecretID returned the plain name on a ListSecrets error, so a denied lookup proceeded to UpdateSecret and could look like success. Return the wrapped error; keep the name fallback only for a successful listing with no exact match. Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 3 +++ cmd/secrets_store.go | 7 ++++--- cmd/secrets_store_test.go | 11 +++++++++++ 3 files changed, 18 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d42eab59c..5f47b112c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -16,6 +16,9 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - `--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; diff --git a/cmd/secrets_store.go b/cmd/secrets_store.go index 4c82d675c..a20bed1f8 100644 --- a/cmd/secrets_store.go +++ b/cmd/secrets_store.go @@ -93,12 +93,13 @@ 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 (or a failed listing) it returns name itself, so -// UpdateSecret reports a missing secret; more than one exact match is an error. +// 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 name, nil + return "", fmt.Errorf("listing secrets named %q: %w", name, err) } var exact []string for _, arn := range arns { diff --git a/cmd/secrets_store_test.go b/cmd/secrets_store_test.go index a515d36e2..55f424a18 100644 --- a/cmd/secrets_store_test.go +++ b/cmd/secrets_store_test.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -115,3 +116,13 @@ func TestAWSSecretsStore_ListSecretsHonorsCanceledContext(t *testing.T) { _, 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) +}