From dfe695d2be55ac7439a2b6d799e650e483144517 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 15:24:20 +0100 Subject: [PATCH 01/10] test(cmd): extract startCompletenessChild and count linked-account probes The scope refusal scenarios run the root command against the same recorded-HTTP proxy and assert on a refusal, not a CSV, so the child start is shared. The proxy counts the probe's LINKED_ACCOUNT requests. go.mod lists costexplorer as a direct dependency. Refs #2131 Co-Authored-By: Claude Opus 5.5 --- cmd/recommendation_completeness_proxy_test.go | 2 ++ cmd/recommendation_completeness_test.go | 25 +++++++++++++------ go.mod | 2 +- 3 files changed, 20 insertions(+), 9 deletions(-) diff --git a/cmd/recommendation_completeness_proxy_test.go b/cmd/recommendation_completeness_proxy_test.go index 2964491d3..5ac5a2f93 100644 --- a/cmd/recommendation_completeness_proxy_test.go +++ b/cmd/recommendation_completeness_proxy_test.go @@ -39,6 +39,8 @@ type completenessProxy struct { cert tls.Certificate expiryCoverage []string expiryAccounts []string + // linkedAccountCalls counts the rebuy scope probe's LINKED_ACCOUNT requests. + linkedAccountCalls int } type completenessSPRequest struct { diff --git a/cmd/recommendation_completeness_test.go b/cmd/recommendation_completeness_test.go index 54923e7c2..52e1c79c8 100644 --- a/cmd/recommendation_completeness_test.go +++ b/cmd/recommendation_completeness_test.go @@ -44,11 +44,14 @@ func TestRecommendationCompletenessCommand(t *testing.T) { } } -func runCompletenessScenario(t *testing.T, service, regionMode, details string) { +// startCompletenessChild re-executes the test binary as the root command +// against a recorded-HTTP proxy and returns what the child printed. The +// output path is the CSV the child would write. +func startCompletenessChild(t *testing.T, service, regionMode, details string) (fixture *completenessProxy, output, stdout, stderr string, runErr error) { t.Helper() dir := t.TempDir() fixture, proxyURL, caPath := newCompletenessProxy(t, dir, regionMode, details, service) - output := filepath.Join(dir, "recommendations.csv") + output = filepath.Join(dir, "recommendations.csv") ctx, cancel := context.WithTimeout(context.Background(), 45*time.Second) defer cancel() child := exec.CommandContext(ctx, os.Args[0], "-test.run=^TestRecommendationCompletenessCommand$", "-test.v") @@ -65,10 +68,16 @@ func runCompletenessScenario(t *testing.T, service, regionMode, details string) "AWS_SHARED_CREDENTIALS_FILE=" + filepath.Join(dir, "absent-credentials"), "HTTPS_PROXY=" + proxyURL, "HTTP_PROXY=" + proxyURL, "AWS_CA_BUNDLE=" + caPath, } - var stdout, stderr bytes.Buffer - child.Stdout, child.Stderr = &stdout, &stderr - runErr := child.Run() - logs := stdout.String() + stderr.String() + var out, errOut bytes.Buffer + child.Stdout, child.Stderr = &out, &errOut + runErr = child.Run() + return fixture, output, out.String(), errOut.String(), runErr +} + +func runCompletenessScenario(t *testing.T, service, regionMode, details string) { + t.Helper() + fixture, output, stdout, stderr, runErr := startCompletenessChild(t, service, regionMode, details) + logs := stdout + stderr if details == "api-error" || details == "failed-type" { // A provider failure is a failed run, not "nothing to buy". var exitErr *exec.ExitError @@ -79,7 +88,7 @@ func runCompletenessScenario(t *testing.T, service, regionMode, details string) require.NoFileExists(t, output) return } - require.NoError(t, runErr, "stdout: %s\nstderr: %s", &stdout, &stderr) + require.NoError(t, runErr, "stdout: %s\nstderr: %s", stdout, stderr) fixture.assertRequests(t) for _, failure := range []string{"Could not check", "Failed to query", "request send failed", "certificate", "TLS handshake"} { require.NotContains(t, logs, failure, "unexpected ancillary error") @@ -90,7 +99,7 @@ func runCompletenessScenario(t *testing.T, service, regionMode, details string) case rdsExpiryService: assertRDSExpiryCSV(t, output, details, logs) case "rds": - assertCompletenessDiagnostics(t, details, stdout.String(), stderr.String()) + assertCompletenessDiagnostics(t, details, stdout, stderr) assertCompletenessCSV(t, output, details, logs) default: assertSPCompleteness(t, output, service, details, logs) diff --git a/go.mod b/go.mod index 31a485dda..112ab8bdf 100644 --- a/go.mod +++ b/go.mod @@ -5,7 +5,7 @@ go 1.26.10 require ( github.com/aws/aws-sdk-go-v2 v1.47.5 github.com/aws/aws-sdk-go-v2/config v1.33.13 - github.com/aws/aws-sdk-go-v2/service/costexplorer v1.61.0 // indirect + github.com/aws/aws-sdk-go-v2/service/costexplorer v1.61.0 github.com/aws/aws-sdk-go-v2/service/ec2 v1.343.0 github.com/aws/aws-sdk-go-v2/service/elasticache v1.50.3 // indirect github.com/aws/aws-sdk-go-v2/service/memorydb v1.31.4 // indirect From e6baf3e90a3ab73873c9597df84b71f77fc37e98 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 15:24:31 +0100 Subject: [PATCH 02/10] fix(rebuy): refuse gated rebuy unless the account owns all in-scope usage With --rebuy-window-days and --target-coverage on an RI service, the run now exits 1 with no CSV, on dry runs too, unless it can show that the credentials' account owns all the in-scope usage. The checks run in this order: - The STS lookup finds the caller's account. - organizations:DescribeOrganization refuses member accounts; standalone and management accounts continue. - A Cost Explorer GetReservationCoverage query, grouped by linked account over the same window, service and regions as sizing, refuses on any other account. - Recommendation rows for other accounts refuse. - A failed RI inventory read exits 1 instead of warning. The owner chose this refusal for member accounts (option 2), which adds the one new read-only permission. Refs #2131 Co-Authored-By: Claude Opus 5.5 --- cmd/multi_service.go | 4 + cmd/multi_service_fetch_all.go | 20 ++-- cmd/rebuy_scope.go | 150 ++++++++++++++++++++++++++++++ cmd/rebuy_scope_org.go | 62 +++++++++++++ cmd/rebuy_scope_probe.go | 161 +++++++++++++++++++++++++++++++++ 5 files changed, 385 insertions(+), 12 deletions(-) create mode 100644 cmd/rebuy_scope.go create mode 100644 cmd/rebuy_scope_org.go create mode 100644 cmd/rebuy_scope_probe.go diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 69d528063..7c961546a 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -172,6 +172,7 @@ func runToolMultiService(ctx context.Context, cfg Config) { if adapter, ok := recClient.(*awsprovider.RecommendationsClientAdapter); ok && cfg.RecLookbackPeriod != "" { adapter.SetRecLookbackPeriod(cfg.RecLookbackPeriod) } + cfg = mustEnforceRebuyScope(ctx, awsCfg, cfg, servicesToProcess) engineData := mustFetchEngineVersionData(ctx, cfg, extendedSupportCheckNeeded(cfg, servicesToProcess)) // Fetch existing-RI coverage so --target-coverage can subtract what @@ -385,6 +386,9 @@ func exitOnFetchError(err error) { if errors.Is(err, errRecommendationFetch) { log.Fatalf("Cannot complete the recommendation fetch: %v", err) } + if errors.Is(err, errRebuyScope) { + log.Fatalf("Cannot verify the rebuy scope: %v", err) + } exitOnExclusionError(err) } diff --git a/cmd/multi_service_fetch_all.go b/cmd/multi_service_fetch_all.go index 0d7599063..908f3f835 100644 --- a/cmd/multi_service_fetch_all.go +++ b/cmd/multi_service_fetch_all.go @@ -104,7 +104,7 @@ func fetchAndFilterRegionRecs( if err := populateRecommendationAccountNames(ctx, recs, accountCache, cfg); err != nil { return nil, fmt.Errorf("%w: %w", errRecommendationFetch, err) } - if err := requireRegionInventory(recs, cfg, engineData); err != nil { + if err := requireRecPreconditions(recs, cfg, engineData); err != nil { return nil, err } recs = applyRegionFilters(recs, engineData, region, cfg, drops) @@ -120,17 +120,13 @@ func fetchAndFilterRegionRecs( serviceClient := createServiceClient(service, regionalCfg) // Fetch existing commitments up front when --rebuy-window-days is set so - // the sizing step can deduct expiring RIs from ExistingCoveragePct. - // Best-effort: a failure here logs and continues; expiry adjustment - // becomes a no-op for this region. - var expiringCommitments []common.Commitment - if cfg.RebuyWindowDays > 0 && serviceClient != nil { - commits, err := serviceClient.GetExistingCommitments(ctx) - if err != nil { - AppLogger.Printf(" ⚠️ Could not fetch existing commitments for expiry check: %v\n", err) - } else { - expiringCommitments = commits - } + // the sizing step can deduct expiring RIs from ExistingCoveragePct. Under + // the rebuy scope gate (RI service, coverage target) a failed read refuses + // the run; otherwise it warns and the expiry adjustment is a no-op for this + // region. + expiringCommitments, inventoryErr := fetchRebuyInventory(ctx, serviceClient, service, cfg) + if inventoryErr != nil { + return nil, inventoryErr } recs = applyCoverageAndOverrides(recs, cfg, coverageMap, expiringCommitments, drops) diff --git a/cmd/rebuy_scope.go b/cmd/rebuy_scope.go new file mode 100644 index 000000000..9ae8d64c9 --- /dev/null +++ b/cmd/rebuy_scope.go @@ -0,0 +1,150 @@ +package main + +import ( + "context" + "errors" + "fmt" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/provider" + "github.com/aws/aws-sdk-go-v2/aws" +) + +// errRebuyScope marks a gated --rebuy-window-days run that cannot prove the +// credentials' account owns all the in-scope usage: either another account's +// usage is in scope, or the check itself could not run. Both refuse the run. +var errRebuyScope = errors.New("rebuy scope") + +// rebuyScopeError carries the operator-facing text unchanged (the exit message +// supplies the "Cannot verify the rebuy scope" prefix) while matching +// errRebuyScope under errors.Is. +type rebuyScopeError struct{ err error } + +func (e rebuyScopeError) Error() string { return e.err.Error() } +func (e rebuyScopeError) Unwrap() error { return e.err } +func (e rebuyScopeError) Is(target error) bool { return target == errRebuyScope } + +func refuseRebuyScope(format string, args ...any) error { + return rebuyScopeError{fmt.Errorf(format, args...)} +} + +// rebuyScopeRemedy ends every refusal that names another account's usage. +const rebuyScopeRemedy = "gated rebuy needs a standalone account, or a management account whose member accounts have no in-scope usage; otherwise omit --rebuy-window-days" + +// scopeCheckFailure is the refusal for a check that could not run, as opposed +// to one that ran and found another account's usage. +func scopeCheckFailure(cause error) error { + return refuseRebuyScope("the scope check could not run (%w); fix access or retry, or omit --rebuy-window-days", cause) +} + +// rebuyGated reports whether --rebuy-window-days is enforced for the service: +// it adjusts RI sizing only, and only with a coverage target. +func rebuyGated(cfg Config, service common.ServiceType) bool { + return cfg.RebuyWindowDays > 0 && cfg.TargetCoverage > 0 && !common.IsSavingsPlan(service) +} + +// rebuyScopeActive reports whether any selected service is gated. +func rebuyScopeActive(cfg Config, services []common.ServiceType) bool { + for _, service := range services { + if rebuyGated(cfg, service) { + return true + } + } + return false +} + +// enforceRebuyScope resolves the credentials' account when the rebuy gate +// holds, refuses a member account, refuses if the scope probe finds usage in +// another account, and returns cfg carrying the account for the +// per-recommendation scope check. An unresolvable or empty account refuses. +func enforceRebuyScope(ctx context.Context, awsCfg aws.Config, cfg Config, services []common.ServiceType) (Config, error) { + if !rebuyScopeActive(cfg, services) { + return cfg, nil + } + account, err := callerAccountFetcher(ctx, awsCfg) + if err != nil { + return cfg, scopeCheckFailure(fmt.Errorf("cannot determine the AWS account of the credentials: %w", err)) + } + out := cfg + out.CallerAccountID = account + if out.CallerAccountID == "" { + return cfg, scopeCheckFailure(errors.New("the AWS account of the credentials is unknown")) + } + if orgErr := requireNotMember(ctx, rebuyOrgClient(awsCfg), out.CallerAccountID); orgErr != nil { + return cfg, orgErr + } + if probeErr := probeRebuyScope(ctx, rebuyCoverageClient(awsCfg), out, services); probeErr != nil { + return cfg, probeErr + } + return out, nil +} + +// mustEnforceRebuyScope is enforceRebuyScope for the main pipeline; it exits +// through the same route as fetch errors. +func mustEnforceRebuyScope(ctx context.Context, awsCfg aws.Config, cfg Config, services []common.ServiceType) Config { + out, err := enforceRebuyScope(ctx, awsCfg, cfg, services) + if err != nil { + exitIfInterrupted(ctx, "checking the rebuy scope") + exitOnFetchError(err) + } + return out +} + +// requireRecPreconditions runs the checks every fetched batch must pass +// before filtering: the scope check first, then the RDS inventory check. +func requireRecPreconditions(recs []common.Recommendation, cfg Config, data engineVersionData) error { + if err := requireRebuyRecScope(recs, cfg); err != nil { + return err + } + return requireRegionInventory(recs, cfg, data) +} + +// requireRebuyRecScope refuses a gated run when an RI recommendation belongs to +// an account other than the credentials'. The existing-commitment inventory +// reads only the credentials' own account, so expiry netting against another +// account's demand would be wrong. A recommendation with no account passes +// here; the scope probe covers it. +func requireRebuyRecScope(recs []common.Recommendation, cfg Config) error { + for i := range recs { + if !rebuyGated(cfg, recs[i].Service) { + continue + } + err := checkPurchaseAccount(recs[i], cfg.CallerAccountID) + if err == nil { + continue + } + if cfg.CallerAccountID == "" { + return scopeCheckFailure(err) + } + return refuseRebuyScope("%s recommendation for account %s in %s, but the credentials are for account %s; %s", + recs[i].Service, recs[i].Account, recs[i].Region, cfg.CallerAccountID, rebuyScopeRemedy) + } + return nil +} + +// fetchRebuyInventory reads the owned commitments the sizing step deducts +// expiring RIs from. Under the gate a missing client or a failed read refuses: +// sizing without the inventory would silently skip the expiry netting. +// Savings Plans and a gate that is off keep the warning and continue with no +// inventory. +func fetchRebuyInventory(ctx context.Context, client provider.ServiceClient, service common.ServiceType, cfg Config) ([]common.Commitment, error) { + if cfg.RebuyWindowDays <= 0 { + return nil, nil + } + gated := rebuyGated(cfg, service) + if client == nil { + if gated { + return nil, scopeCheckFailure(fmt.Errorf("no %s client to read existing commitments", service)) + } + return nil, nil + } + commits, err := client.GetExistingCommitments(ctx) + if err == nil { + return commits, nil + } + if gated { + return nil, scopeCheckFailure(fmt.Errorf("reading existing %s commitments: %w", service, err)) + } + AppLogger.Printf(" ⚠️ Could not fetch existing commitments for expiry check: %v\n", err) + return nil, nil +} diff --git a/cmd/rebuy_scope_org.go b/cmd/rebuy_scope_org.go new file mode 100644 index 000000000..7c80d04de --- /dev/null +++ b/cmd/rebuy_scope_org.go @@ -0,0 +1,62 @@ +package main + +import ( + "context" + "errors" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/organizations" + "github.com/aws/aws-sdk-go-v2/service/organizations/types" +) + +// orgDescribeAPI is the slice of the Organizations client the role check uses. +type orgDescribeAPI interface { + DescribeOrganization(ctx context.Context, in *organizations.DescribeOrganizationInput, optFns ...func(*organizations.Options)) (*organizations.DescribeOrganizationOutput, error) +} + +// rebuyOrgClient is the seam tests replace so they never reach AWS. +var rebuyOrgClient = func(cfg aws.Config) orgDescribeAPI { return organizations.NewFromConfig(cfg) } + +// orgRoleKind is the credentials' account's place in AWS Organizations. +type orgRoleKind int + +const ( + orgRoleStandalone orgRoleKind = iota + 1 + orgRoleManagement + orgRoleMember +) + +// orgRole classifies the caller with organizations:DescribeOrganization. +// Only AWSOrganizationsNotInUseException means standalone; an empty answer, +// an access denial or any other error cannot prove the role and is an error. +func orgRole(ctx context.Context, client orgDescribeAPI, caller string) (orgRoleKind, error) { + out, err := client.DescribeOrganization(ctx, &organizations.DescribeOrganizationInput{}) + if err != nil { + var notInUse *types.AWSOrganizationsNotInUseException + if errors.As(err, ¬InUse) { + return orgRoleStandalone, nil + } + return 0, err + } + if out == nil || out.Organization == nil || aws.ToString(out.Organization.MasterAccountId) == "" { + return 0, errors.New("the organization answer has no management account") + } + if aws.ToString(out.Organization.MasterAccountId) == caller { + return orgRoleManagement, nil + } + return orgRoleMember, nil +} + +// requireNotMember refuses a gated run from a member account: replacements +// bought centrally are invisible to it, so its expiry netting would be wrong. +// Standalone and management callers pass on to the usage probe. +func requireNotMember(ctx context.Context, client orgDescribeAPI, caller string) error { + role, err := orgRole(ctx, client, caller) + if err != nil { + return refuseRebuyScope("the scope check could not run (%w); fix access or retry, grant organizations:DescribeOrganization (read-only), or omit --rebuy-window-days", err) + } + if role == orgRoleMember { + return refuseRebuyScope("account %s is a member of an AWS Organization: member accounts can't run gated rebuy safely (replacements bought centrally aren't visible to this account); gated rebuy only runs from a standalone account, or a management account whose member accounts have no in-scope usage; omit --rebuy-window-days", caller) + } + return nil +} diff --git a/cmd/rebuy_scope_probe.go b/cmd/rebuy_scope_probe.go new file mode 100644 index 000000000..d2958f6b9 --- /dev/null +++ b/cmd/rebuy_scope_probe.go @@ -0,0 +1,161 @@ +package main + +import ( + "context" + "errors" + "strings" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/costexplorer" + "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" +) + +// coverageAPI is the slice of the Cost Explorer client the scope probe uses. +type coverageAPI interface { + GetReservationCoverage(ctx context.Context, in *costexplorer.GetReservationCoverageInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationCoverageOutput, error) +} + +// rebuyCoverageClient is the seam tests replace so they never reach AWS. +var rebuyCoverageClient = func(cfg aws.Config) coverageAPI { return costexplorer.NewFromConfig(cfg) } + +// maxProbePages bounds the probe's paging; exceeding it refuses. +const maxProbePages = 100 + +// rebuyCoverageService maps an RI service to Cost Explorer's SERVICE dimension +// value. It mirrors the unexported constants in cloud-commitments-go +// providers/aws/recommendations/coverage.go, so the probe sees exactly the pool +// the coverage map sizes against. An unmapped service has no safe filter, so +// the probe refuses instead of widening it. +func rebuyCoverageService(service common.ServiceType) (string, bool) { + switch service { + case common.ServiceEC2: + return "Amazon Elastic Compute Cloud - Compute", true + case common.ServiceRDS: + return "Amazon Relational Database Service", true + case common.ServiceElastiCache: + return "Amazon ElastiCache", true + case common.ServiceOpenSearch: + return "Amazon OpenSearch Service", true + case common.ServiceRedshift: + return "Amazon Redshift", true + case common.ServiceMemoryDB: + return "Amazon MemoryDB", true + } + return "", false +} + +// probeRebuyScope runs the per-account usage probe for every gated service. +func probeRebuyScope(ctx context.Context, client coverageAPI, cfg Config, services []common.ServiceType) error { + for _, service := range services { + if !rebuyGated(cfg, service) { + continue + } + if err := probeServiceAccounts(ctx, client, service, cfg); err != nil { + return err + } + } + return nil +} + +// probeServiceAccounts asks Cost Explorer which accounts have usage in the +// same pool the coverage map sizes (same window, SERVICE and, when --regions +// is set, REGION filters), grouped by LINKED_ACCOUNT. It refuses on the first +// account other than the credentials'. Without --regions there is no REGION +// filter: that is broader, so it can only refuse more. A member account sees +// only itself and a standalone account sees no other accounts, so both pass. +func probeServiceAccounts(ctx context.Context, client coverageAPI, service common.ServiceType, cfg Config) error { + ceService, ok := rebuyCoverageService(service) + if !ok { + return refuseRebuyScope("no Cost Explorer service mapping for %s, so its usage scope cannot be checked; omit --rebuy-window-days", service) + } + input := rebuyProbeInput(ceService, cfg) + for range maxProbePages { + out, err := client.GetReservationCoverage(ctx, input) + if err != nil { + return probeCallFailure(err) + } + if groupErr := requireOwnAccountGroups(out, service, cfg.CallerAccountID); groupErr != nil { + return groupErr + } + if aws.ToString(out.NextPageToken) == "" { + return nil + } + next := *input + next.NextPageToken = out.NextPageToken + input = &next + } + return refuseRebuyScope("the scope check could not run (more than %d pages of %s usage groups); omit --rebuy-window-days", maxProbePages, service) +} + +// rebuyProbeInput builds the probe request: the coverage lookback window +// (Metrics "Hour" as the coverage map), SERVICE and optional REGION filters. +func rebuyProbeInput(ceService string, cfg Config) *costexplorer.GetReservationCoverageInput { + end := time.Now().UTC() + start := end.AddDate(0, 0, -coverageLookbackDays(cfg)) + filters := []types.Expression{{Dimensions: &types.DimensionValues{Key: types.DimensionService, Values: []string{ceService}}}} + if len(cfg.Regions) > 0 { + filters = append(filters, types.Expression{Dimensions: &types.DimensionValues{Key: types.DimensionRegion, Values: cfg.Regions}}) + } + return &costexplorer.GetReservationCoverageInput{ + TimePeriod: &types.DateInterval{Start: aws.String(start.Format(time.DateOnly)), End: aws.String(end.Format(time.DateOnly))}, + GroupBy: []types.GroupDefinition{{Type: types.GroupDefinitionTypeDimension, Key: aws.String(string(types.DimensionLinkedAccount))}}, + Filter: &types.Expression{And: filters}, + Metrics: []string{"Hour"}, + } +} + +// requireOwnAccountGroups refuses when a page names an account other than the +// caller, or a group whose account cannot be read. +func requireOwnAccountGroups(out *costexplorer.GetReservationCoverageOutput, service common.ServiceType, caller string) error { + for _, byTime := range out.CoveragesByTime { + for _, group := range byTime.Groups { + account, err := groupAccount(group.Attributes) + if err != nil { + return scopeCheckFailure(err) + } + if account != caller { + return refuseRebuyScope("account %s has in-scope %s usage, but the credentials are for account %s; %s", account, service, caller, rebuyScopeRemedy) + } + } + } + return nil +} + +// groupAccount reads the linked account from a coverage group's attributes. +// Cost Explorer returns attribute keys in camelCase ("linkedAccount") although +// GroupBy takes LINKED_ACCOUNT, so keys compare with underscores removed and +// case folded; the value is used exactly as returned. A missing or empty +// value, or two keys that normalize alike, cannot be trusted and fail closed. +// Source of truth: the AWS Cost Explorer API reference for +// ReservationCoverageGroup (Attributes); no real-account response verified. +func groupAccount(attrs map[string]string) (string, error) { + var account string + matches := 0 + for key, value := range attrs { + if strings.ToLower(strings.ReplaceAll(key, "_", "")) == "linkedaccount" { + account = value + matches++ + } + } + switch { + case matches == 0: + return "", errors.New("a coverage group has no linked-account attribute") + case matches > 1: + return "", errors.New("a coverage group has duplicate linked-account attributes") + case account == "": + return "", errors.New("a coverage group has an empty linked-account attribute") + } + return account, nil +} + +// probeCallFailure is the refusal for a failed Cost Explorer call. An access +// denial adds the usual fix. +func probeCallFailure(err error) error { + var coded interface{ ErrorCode() string } + if errors.As(err, &coded) && coded.ErrorCode() == "AccessDeniedException" { + return refuseRebuyScope("the scope check could not run (%w); fix access or retry, grant ce:GetReservationCoverage or enable Cost Explorer access for this account, or omit --rebuy-window-days", err) + } + return scopeCheckFailure(err) +} From c64a6990ac28163d27124ab01632c4d849a4093f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 15:24:43 +0100 Subject: [PATCH 03/10] test(rebuy): cover the scope gate and the organization role check Refs #2131 Co-Authored-By: Claude Opus 5.5 --- cmd/rebuy_scope_org_test.go | 134 +++++++++++++++++++++++ cmd/rebuy_scope_test.go | 213 ++++++++++++++++++++++++++++++++++++ 2 files changed, 347 insertions(+) create mode 100644 cmd/rebuy_scope_org_test.go create mode 100644 cmd/rebuy_scope_test.go diff --git a/cmd/rebuy_scope_org_test.go b/cmd/rebuy_scope_org_test.go new file mode 100644 index 000000000..9f5812bd9 --- /dev/null +++ b/cmd/rebuy_scope_org_test.go @@ -0,0 +1,134 @@ +package main + +import ( + "context" + "errors" + "testing" + + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/costexplorer" + "github.com/aws/aws-sdk-go-v2/service/organizations" + "github.com/aws/aws-sdk-go-v2/service/organizations/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeOrg answers DescribeOrganization from a fixed output or error. +type fakeOrg struct { + out *organizations.DescribeOrganizationOutput + err error + calls int +} + +func (f *fakeOrg) DescribeOrganization(context.Context, *organizations.DescribeOrganizationInput, ...func(*organizations.Options)) (*organizations.DescribeOrganizationOutput, error) { + f.calls++ + return f.out, f.err +} + +func orgOf(master *string) *fakeOrg { + return &fakeOrg{out: &organizations.DescribeOrganizationOutput{Organization: &types.Organization{MasterAccountId: master}}} +} + +// managementOrg is an organization whose management account is the given one. +func managementOrg(account string) *fakeOrg { return orgOf(aws.String(account)) } + +// stubOrg replaces the Organizations seam for one test. +func stubOrg(t *testing.T, fake *fakeOrg) { + t.Helper() + orig := rebuyOrgClient + t.Cleanup(func() { rebuyOrgClient = orig }) + rebuyOrgClient = func(aws.Config) orgDescribeAPI { return fake } +} + +func TestOrgRole(t *testing.T) { + ctx := context.Background() + for name, tc := range map[string]struct { + fake *fakeOrg + want orgRoleKind + wantErr string + }{ + "standalone": {&fakeOrg{err: &types.AWSOrganizationsNotInUseException{}}, orgRoleStandalone, ""}, + "management": {managementOrg("111111111111"), orgRoleManagement, ""}, + "member": {managementOrg("999999999999"), orgRoleMember, ""}, + "access denied": {&fakeOrg{err: &types.AccessDeniedException{}}, 0, "AccessDeniedException"}, + "other error": {&fakeOrg{err: errors.New("throttled")}, 0, "throttled"}, + "nil organization": {&fakeOrg{out: &organizations.DescribeOrganizationOutput{}}, 0, "no management account"}, + "empty management": {orgOf(aws.String("")), 0, "no management account"}, + "missing management": {orgOf(nil), 0, "no management account"}, + "not-in-use by text": {&fakeOrg{err: errors.New("AWSOrganizationsNotInUseException")}, 0, "AWSOrganizationsNotInUseException"}, + } { + t.Run(name, func(t *testing.T) { + got, err := orgRole(ctx, tc.fake, "111111111111") + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + } else { + require.NoError(t, err) + } + assert.Equal(t, tc.want, got) + }) + } +} + +func TestRequireNotMember(t *testing.T) { + ctx := context.Background() + t.Run("standalone and management pass", func(t *testing.T) { + require.NoError(t, requireNotMember(ctx, &fakeOrg{err: &types.AWSOrganizationsNotInUseException{}}, "111111111111")) + require.NoError(t, requireNotMember(ctx, managementOrg("111111111111"), "111111111111")) + }) + t.Run("member refuses with the scope text", func(t *testing.T) { + err := requireNotMember(ctx, managementOrg("999999999999"), "111111111111") + require.ErrorIs(t, err, errRebuyScope) + assert.Equal(t, "account 111111111111 is a member of an AWS Organization: member accounts can't run gated rebuy safely (replacements bought centrally aren't visible to this account); gated rebuy only runs from a standalone account, or a management account whose member accounts have no in-scope usage; omit --rebuy-window-days", err.Error()) + }) + for name, fake := range map[string]*fakeOrg{ + "access denied": {err: &types.AccessDeniedException{}}, + "throttled": {err: errors.New("throttled")}, + "empty answer": {out: &organizations.DescribeOrganizationOutput{}}, + } { + t.Run(name+" is a check failure", func(t *testing.T) { + err := requireNotMember(ctx, fake, "111111111111") + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "the scope check could not run (") + assert.Contains(t, err.Error(), "fix access or retry, grant organizations:DescribeOrganization (read-only), or omit --rebuy-window-days") + }) + } +} + +func TestEnforceRebuyScopeOrg(t *testing.T) { + run := func(t *testing.T, org *fakeOrg, probe *fakeCoverage) error { + t.Helper() + stubCaller(t, "111111111111", nil) + stubOrg(t, org) + stubProbe(t, probe) + _, err := enforceRebuyScope(context.Background(), aws.Config{}, gatedCfg(), riServices) + return err + } + t.Run("member refuses before the probe", func(t *testing.T) { + org := managementOrg("999999999999") + err := run(t, org, &fakeCoverage{t: t, noCalls: true}) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "member of an AWS Organization") + assert.Equal(t, 1, org.calls) + }) + t.Run("an org error refuses before the probe", func(t *testing.T) { + err := run(t, &fakeOrg{err: &types.AccessDeniedException{}}, &fakeCoverage{t: t, noCalls: true}) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "organizations:DescribeOrganization") + }) + t.Run("management proceeds to the probe", func(t *testing.T) { + probe := &fakeCoverage{t: t} + require.NoError(t, run(t, managementOrg("111111111111"), probe)) + assert.Len(t, probe.inputs, 1) + }) + t.Run("standalone proceeds to the probe", func(t *testing.T) { + probe := &fakeCoverage{t: t} + require.NoError(t, run(t, &fakeOrg{err: &types.AWSOrganizationsNotInUseException{}}, probe)) + assert.Len(t, probe.inputs, 1) + }) + t.Run("standalone with a foreign linked account still refuses", func(t *testing.T) { + probe := &fakeCoverage{t: t, pages: []*costexplorer.GetReservationCoverageOutput{groupsPage("", "111111111111", "222222222222")}} + err := run(t, &fakeOrg{err: &types.AWSOrganizationsNotInUseException{}}, probe) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "222222222222") + }) +} diff --git a/cmd/rebuy_scope_test.go b/cmd/rebuy_scope_test.go new file mode 100644 index 000000000..7b409f15d --- /dev/null +++ b/cmd/rebuy_scope_test.go @@ -0,0 +1,213 @@ +package main + +import ( + "context" + "errors" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/costexplorer" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +var ( + riServices = []common.ServiceType{common.ServiceEC2} + spServices = []common.ServiceType{common.ServiceSavingsPlansCompute} +) + +func gatedCfg() Config { return Config{RebuyWindowDays: 30, TargetCoverage: 80} } + +// stubCaller replaces the STS seam for one test and counts its calls. +func stubCaller(t *testing.T, account string, err error) *int { + t.Helper() + orig := callerAccountFetcher + t.Cleanup(func() { callerAccountFetcher = orig }) + calls := 0 + callerAccountFetcher = func(context.Context, aws.Config) (string, error) { + calls++ + return account, err + } + stubProbe(t, &fakeCoverage{t: t, noCalls: true}) + stubOrg(t, managementOrg(account)) + return &calls +} + +// stubProbe replaces the Cost Explorer seam for one test. +func stubProbe(t *testing.T, fake *fakeCoverage) { + t.Helper() + orig := rebuyCoverageClient + t.Cleanup(func() { rebuyCoverageClient = orig }) + rebuyCoverageClient = func(aws.Config) coverageAPI { return fake } +} + +func TestEnforceRebuyScope(t *testing.T) { + t.Run("gate on resolves the caller", func(t *testing.T) { + calls := stubCaller(t, "111111111111", nil) + probe := &fakeCoverage{t: t} + stubProbe(t, probe) + out, err := enforceRebuyScope(context.Background(), aws.Config{}, gatedCfg(), riServices) + require.NoError(t, err) + assert.Equal(t, "111111111111", out.CallerAccountID) + assert.Equal(t, 1, *calls) + require.Len(t, probe.inputs, 1, "one probe call for one RI service") + }) + t.Run("a mixed RI and SP list is gated", func(t *testing.T) { + calls := stubCaller(t, "111111111111", nil) + probe := &fakeCoverage{t: t} + stubProbe(t, probe) + _, err := enforceRebuyScope(context.Background(), aws.Config{}, gatedCfg(), append(spServices, riServices...)) + require.NoError(t, err) + assert.Equal(t, 1, *calls) + require.Len(t, probe.inputs, 1, "the SP service is not probed") + }) + for name, tc := range map[string]struct { + cfg Config + services []common.ServiceType + }{ + "rebuy without a target": {Config{RebuyWindowDays: 30}, riServices}, + "target without rebuy": {Config{TargetCoverage: 80}, riServices}, + "Savings Plans only": {gatedCfg(), spServices}, + "no services": {gatedCfg(), nil}, + "legacy coverage, no gates": {Config{Coverage: 80}, riServices}, + } { + t.Run("gate off: "+name, func(t *testing.T) { + calls := stubCaller(t, "", errors.New("must not be called")) + out, err := enforceRebuyScope(context.Background(), aws.Config{}, tc.cfg, tc.services) + require.NoError(t, err) + assert.Equal(t, tc.cfg, out) + assert.Zero(t, *calls, "no STS call when the gate is off") + // stubCaller's probe fake fails the test if it is called. + }) + } + t.Run("STS failure refuses", func(t *testing.T) { + stubCaller(t, "", errors.New("expired token")) + _, err := enforceRebuyScope(context.Background(), aws.Config{}, gatedCfg(), riServices) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "the scope check could not run (") + assert.Contains(t, err.Error(), "cannot determine the AWS account of the credentials: expired token") + assert.NotContains(t, err.Error(), "needed to check purchases") + assert.Contains(t, err.Error(), "fix access or retry, or omit --rebuy-window-days") + }) + t.Run("empty caller refuses", func(t *testing.T) { + stubCaller(t, "", nil) + _, err := enforceRebuyScope(context.Background(), aws.Config{}, gatedCfg(), riServices) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "the AWS account of the credentials is unknown") + }) +} + +func TestEnforceRebuyScopeProbe(t *testing.T) { + t.Run("foreign usage refuses", func(t *testing.T) { + stubCaller(t, "111111111111", nil) + stubProbe(t, &fakeCoverage{t: t, pages: []*costexplorer.GetReservationCoverageOutput{groupsPage("", "111111111111", "222222222222")}}) + _, err := enforceRebuyScope(context.Background(), aws.Config{}, gatedCfg(), riServices) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "222222222222") + }) + t.Run("probe failure refuses", func(t *testing.T) { + stubCaller(t, "111111111111", nil) + stubProbe(t, &fakeCoverage{t: t, err: errors.New("throttled")}) + _, err := enforceRebuyScope(context.Background(), aws.Config{}, gatedCfg(), riServices) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "the scope check could not run (throttled)") + }) + t.Run("STS failure never reaches the probe", func(t *testing.T) { + stubCaller(t, "", errors.New("expired token")) + _, err := enforceRebuyScope(context.Background(), aws.Config{}, gatedCfg(), riServices) + require.ErrorIs(t, err, errRebuyScope) + }) +} + +func TestRebuyScopeActive(t *testing.T) { + assert.True(t, rebuyScopeActive(gatedCfg(), riServices)) + assert.False(t, rebuyScopeActive(gatedCfg(), spServices)) + assert.False(t, rebuyScopeActive(Config{RebuyWindowDays: 30}, riServices)) + assert.False(t, rebuyScopeActive(Config{TargetCoverage: 80}, riServices)) +} + +func ec2AccountRec(account string) common.Recommendation { + return common.Recommendation{Service: common.ServiceEC2, Account: account, Region: "us-east-1"} +} + +func TestRequireRebuyRecScope(t *testing.T) { + cfg := gatedCfg() + cfg.CallerAccountID = "111111111111" + t.Run("own and empty accounts pass", func(t *testing.T) { + require.NoError(t, requireRebuyRecScope([]common.Recommendation{ec2AccountRec("111111111111"), ec2AccountRec("")}, cfg)) + }) + t.Run("foreign RI rec refuses with account, service, region and remedy", func(t *testing.T) { + err := requireRebuyRecScope([]common.Recommendation{ec2AccountRec("111111111111"), ec2AccountRec("222222222222")}, cfg) + require.ErrorIs(t, err, errRebuyScope) + assert.Equal(t, "ec2 recommendation for account 222222222222 in us-east-1, but the credentials are for account 111111111111; gated rebuy needs a standalone account, or a management account whose member accounts have no in-scope usage; otherwise omit --rebuy-window-days", err.Error()) + }) + t.Run("foreign SP rec is ignored", func(t *testing.T) { + sp := ec2AccountRec("222222222222") + sp.Service = common.ServiceSavingsPlansCompute + require.NoError(t, requireRebuyRecScope([]common.Recommendation{sp}, cfg)) + }) + t.Run("gate off ignores foreign RI recs", func(t *testing.T) { + off := cfg + off.TargetCoverage = 0 + require.NoError(t, requireRebuyRecScope([]common.Recommendation{ec2AccountRec("222222222222")}, off)) + off = cfg + off.RebuyWindowDays = 0 + require.NoError(t, requireRebuyRecScope([]common.Recommendation{ec2AccountRec("222222222222")}, off)) + }) + t.Run("empty caller refuses as a check failure", func(t *testing.T) { + noCaller := gatedCfg() + err := requireRebuyRecScope([]common.Recommendation{ec2AccountRec("")}, noCaller) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "the scope check could not run (") + }) +} + +func TestFetchRebuyInventory(t *testing.T) { + ctx := context.Background() + owned := []common.Commitment{{CommitmentID: "ri-1"}} + t.Run("returns the inventory", func(t *testing.T) { + client := &MockServiceClient{} + client.On("GetExistingCommitments", ctx).Return(owned, nil) + got, err := fetchRebuyInventory(ctx, client, common.ServiceEC2, gatedCfg()) + require.NoError(t, err) + assert.Equal(t, owned, got) + }) + t.Run("RI failure under the gate refuses", func(t *testing.T) { + client := &MockServiceClient{} + client.On("GetExistingCommitments", ctx).Return(nil, errors.New("throttled")) + got, err := fetchRebuyInventory(ctx, client, common.ServiceEC2, gatedCfg()) + require.ErrorIs(t, err, errRebuyScope) + assert.Nil(t, got) + assert.Contains(t, err.Error(), "the scope check could not run (reading existing ec2 commitments: throttled); fix access or retry, or omit --rebuy-window-days") + }) + t.Run("nil client for an RI service under the gate refuses", func(t *testing.T) { + _, err := fetchRebuyInventory(ctx, nil, common.ServiceEC2, gatedCfg()) + require.ErrorIs(t, err, errRebuyScope) + }) + t.Run("SP failure warns", func(t *testing.T) { + client := &MockServiceClient{} + client.On("GetExistingCommitments", ctx).Return(nil, errors.New("throttled")) + got, err := fetchRebuyInventory(ctx, client, common.ServiceSavingsPlansCompute, gatedCfg()) + require.NoError(t, err) + assert.Nil(t, got) + }) + t.Run("gate off warns", func(t *testing.T) { + for _, cfg := range []Config{{RebuyWindowDays: 30}, {RebuyWindowDays: 30, TargetCoverage: 0}} { + client := &MockServiceClient{} + client.On("GetExistingCommitments", ctx).Return(nil, errors.New("throttled")) + got, err := fetchRebuyInventory(ctx, client, common.ServiceEC2, cfg) + require.NoError(t, err) + assert.Nil(t, got) + } + _, err := fetchRebuyInventory(ctx, nil, common.ServiceEC2, Config{RebuyWindowDays: 30}) + require.NoError(t, err, "nil client with the gate off is not a refusal") + }) + t.Run("rebuy off never reads", func(t *testing.T) { + client := &MockServiceClient{} + got, err := fetchRebuyInventory(ctx, client, common.ServiceEC2, Config{TargetCoverage: 80}) + require.NoError(t, err) + assert.Nil(t, got) + client.AssertNotCalled(t, "GetExistingCommitments", ctx) + }) +} From 3cc73d3c04c16054418adca9005ac77203ac82a1 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 15:24:49 +0100 Subject: [PATCH 04/10] test(rebuy): cover the usage probe and the command-level refusals The command scenarios answer DescribeOrganization in every gated run and assert that a member account sends no Cost Explorer request. Refs #2131 Co-Authored-By: Claude Opus 5.5 --- cmd/rebuy_scope_command_test.go | 154 ++++++++++++++++++++++++ cmd/rebuy_scope_probe_test.go | 205 ++++++++++++++++++++++++++++++++ 2 files changed, 359 insertions(+) create mode 100644 cmd/rebuy_scope_command_test.go create mode 100644 cmd/rebuy_scope_probe_test.go diff --git a/cmd/rebuy_scope_command_test.go b/cmd/rebuy_scope_command_test.go new file mode 100644 index 000000000..dc1466180 --- /dev/null +++ b/cmd/rebuy_scope_command_test.go @@ -0,0 +1,154 @@ +package main + +// Command-level tests for cli#2131: a gated rebuy run (--rebuy-window-days and +// --target-coverage, RI services) refuses, exit 1 and no CSV, when it cannot +// prove the credentials' account owns the in-scope usage. The caller account +// is 111111111111 (TestMain stubs STS in the child process). Every gated +// scenario answers DescribeOrganization as a management account (caller) +// unless it names another. +// +// Scenario dispositions (router answers are the proxy's existing ones): +// +// file scenario disposition +// ec2 pool-demand reshape: 1 row (caller), average=demand=90, count 12 -> 36 +// ec2 outside-window reshape: 1 row, count 6 -> 18 +// ec2 replaced-inside-lookback reshape: 1 row, count 6 -> 18 +// ec2 missing-demand-two-each-six-total reshape + rename missing-demand-two-total: 1 row, count 2, warning "1 recommendations" +// ec2 filtered-subset now refuses (c), renamed exclude-others-refuses: 3 rows, exclude accounts 2,3 (kills (c)-after-filter) +// ec2 exact-boundary reshape: 1 row, average 30, count 4 -> 8 +// ec2 below-boundary reshape: 1 row, average 30, count 3 -> 7 +// ec2 inventory-error (new) refuses (d): 1 row, DescribeReservedInstances answers 403 +// rds already-at-target keep (rec account 111) +// rds below-target keep +// rds large-ri-xlarge-usage keep +// rds multi-size-linked-accounts now refuses (c): accounts 111/222/333 +// rds partial-inventory now refuses (b): the only rec is the caller, the router reports account 999999999999 +// ec2 member-account-refuses (new) refuses (e): 1 row, DescribeOrganization names another management account +// ec2 foreign-usage-default-region (new) refuses (b) in default region mode: 1 caller row, router reports 999999999999 +// rds unusable-expiring-warned keep +// rds control-outside-window keep +// rds control-engine-isolation keep +// rds rerun-replacement-bought keep (rec account 111) +// rds control-replacement-outside-lookback keep + +import ( + "encoding/json" + "fmt" + "os/exec" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +// orgDescribeKey is the request-count key of the organization role check. +const orgDescribeKey = "organizations.us-east-1.amazonaws.com/AWSOrganizationsV20161128.DescribeOrganization" + +// describeOrganization answers the role check: the organization is managed by +// master, or by the caller (111111111111) when master is empty. It reports +// ok=false for any other operation. The caller holds p.mu. +func (p *completenessProxy) describeOrganization(host, op, master string) (string, bool) { + if host != "organizations.us-east-1.amazonaws.com" || op != "AWSOrganizationsV20161128.DescribeOrganization" { + return "", false + } + if master == "" { + master = "111111111111" + } + return fmt.Sprintf(`{"Organization":{"Id":"o-fixture","MasterAccountId":%q}}`, master), true +} + +func TestRebuyScopeRefusalCommand(t *testing.T) { + for _, tc := range []struct { + service, name, regionMode string + wantText []string + requests map[string]int + }{ + {"ec2", "exclude-others-refuses", "explicit", []string{"Cannot verify the rebuy scope", "222222222222", "ec2"}, map[string]int{"ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationPurchaseRecommendation": 1, "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage": 13, orgDescribeKey: 1, "organizations.us-east-1.amazonaws.com/AWSOrganizationsV20161128.DescribeAccount": 3, "ec2.us-east-1.amazonaws.com/DescribeInstanceTypes": 1}}, + {"ec2", "inventory-error", "explicit", []string{"Cannot verify the rebuy scope", "fixture denied reserved instances", "ec2"}, map[string]int{"ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationPurchaseRecommendation": 1, "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage": 13, orgDescribeKey: 1, "organizations.us-east-1.amazonaws.com/AWSOrganizationsV20161128.DescribeAccount": 1, "ec2.us-east-1.amazonaws.com/DescribeInstanceTypes": 1, "ec2.us-east-1.amazonaws.com/DescribeReservedInstances": 1}}, + {rdsExpiryService, "multi-size-linked-accounts", "explicit", []string{"Cannot verify the rebuy scope", "222222222222", "rds"}, map[string]int{"ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationPurchaseRecommendation": 1, "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage": 13, orgDescribeKey: 1, "organizations.us-east-1.amazonaws.com/AWSOrganizationsV20161128.DescribeAccount": 3}}, + // (b): the caller owns every rec row; the probe alone sees the foreign account. + // The probe runs before everything else: no coverage-map request + // (INSTANCE_TYPE), no recommendations, no DescribeRegions, no inventory. + {rdsExpiryService, "partial-inventory", "explicit", []string{"Cannot verify the rebuy scope", "999999999999", "rds"}, map[string]int{ + orgDescribeKey: 1, "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage": 1}}, + {"ec2", "foreign-usage-default-region", "default", []string{"Cannot verify the rebuy scope", "999999999999", "ec2"}, map[string]int{ + orgDescribeKey: 1, "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage": 1}}, + // A member account is refused after the role check and before any Cost Explorer request. + {"ec2", "member-account-refuses", "default", []string{"Cannot verify the rebuy scope", "member of an AWS Organization", "111111111111"}, map[string]int{ + orgDescribeKey: 1}}, + } { + t.Run(tc.service+"/"+tc.name, func(t *testing.T) { + fixture, output, stdout, stderr, runErr := startCompletenessChild(t, tc.service, tc.regionMode, tc.name) + logs := stdout + stderr + var exitErr *exec.ExitError + require.ErrorAs(t, runErr, &exitErr, logs) + require.Equal(t, 1, exitErr.ExitCode(), logs) + for _, text := range tc.wantText { + require.Contains(t, logs, text) + } + require.NoFileExists(t, output) + fixture.handlers.Wait() + fixture.mu.Lock() + defer fixture.mu.Unlock() + require.Equal(t, tc.requests, fixture.requests, "no other operation, including purchases, is permitted") + wantProbes := tc.requests["ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage"] + require.Equal(t, wantProbes-len(fixture.expiryCoverage), fixture.linkedAccountCalls, "coverage requests are the probe plus the coverage map") + if tc.name == "partial-inventory" || tc.name == "foreign-usage-default-region" { + require.Empty(t, fixture.expiryCoverage, "the probe must refuse before the coverage map is built") + } + }) + } +} + +// linkedAccountCoverage is the shared LINKED_ACCOUNT router: it answers the +// scope probe (GroupBy LINKED_ACCOUNT) and reports ok=false for any other +// coverage request. It pins the probe's request shape: the 30-day window, +// Metrics Hour, one page, the SERVICE filter of the run's service, and the +// REGION filter only in explicit region mode. The caller (111111111111) is +// always reported; foreign, when set, is reported too. The caller holds p.mu. +func (p *completenessProxy) linkedAccountCoverage(body []byte, ceService, foreign string) (string, bool) { + var request struct { + TimePeriod map[string]string + GroupBy []map[string]string + Filter struct { + And []struct { + Dimensions struct { + Key string + Values []string + } + } + } + Metrics []string + NextPageToken string + } + require.NoError(p.t, json.Unmarshal(body, &request)) + if len(request.GroupBy) == 0 || request.GroupBy[0]["Key"] != "LINKED_ACCOUNT" { + return "", false + } + p.linkedAccountCalls++ + require.Equal(p.t, []map[string]string{{"Type": "DIMENSION", "Key": "LINKED_ACCOUNT"}}, request.GroupBy) + require.Equal(p.t, []string{"Hour"}, request.Metrics) + require.Empty(p.t, request.NextPageToken) + start, err := time.Parse(time.DateOnly, request.TimePeriod["Start"]) + require.NoError(p.t, err) + end, err := time.Parse(time.DateOnly, request.TimePeriod["End"]) + require.NoError(p.t, err) + require.Equal(p.t, 30*24*time.Hour, end.Sub(start)) + filters := make(map[string][]string) + for _, expression := range request.Filter.And { + require.NotContains(p.t, filters, expression.Dimensions.Key) + filters[expression.Dimensions.Key] = expression.Dimensions.Values + } + want := map[string][]string{"SERVICE": {ceService}} + if p.regions == "explicit" { + want["REGION"] = []string{"us-east-1"} + } + require.Equal(p.t, want, filters) + groups := []any{map[string]any{"Attributes": map[string]string{"linkedAccount": "111111111111"}}} + if foreign != "" { + groups = append(groups, map[string]any{"Attributes": map[string]string{"linkedAccount": foreign}}) + } + payload, err := json.Marshal(map[string]any{"CoveragesByTime": []any{map[string]any{"Groups": groups}}}) + require.NoError(p.t, err) + return string(payload), true +} diff --git a/cmd/rebuy_scope_probe_test.go b/cmd/rebuy_scope_probe_test.go new file mode 100644 index 000000000..e1e383421 --- /dev/null +++ b/cmd/rebuy_scope_probe_test.go @@ -0,0 +1,205 @@ +package main + +import ( + "context" + "errors" + "testing" + "time" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/aws-sdk-go-v2/service/costexplorer" + "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// fakeCoverage serves canned pages in order (repeating the last one) and +// records the requests it receives. +type fakeCoverage struct { + t *testing.T + pages []*costexplorer.GetReservationCoverageOutput + err error + noCalls bool + inputs []costexplorer.GetReservationCoverageInput +} + +func (f *fakeCoverage) GetReservationCoverage(_ context.Context, in *costexplorer.GetReservationCoverageInput, _ ...func(*costexplorer.Options)) (*costexplorer.GetReservationCoverageOutput, error) { + if f.noCalls { + f.t.Error("unexpected Cost Explorer call") + } + f.inputs = append(f.inputs, *in) + if f.err != nil { + return nil, f.err + } + if len(f.pages) == 0 { + return &costexplorer.GetReservationCoverageOutput{}, nil + } + return f.pages[min(len(f.inputs), len(f.pages))-1], nil +} + +// groupsPage is one response page whose groups carry the given accounts under +// Cost Explorer's camelCase attribute key, with an optional next-page token. +func groupsPage(next string, accounts ...string) *costexplorer.GetReservationCoverageOutput { + groups := make([]types.ReservationCoverageGroup, 0, len(accounts)) + for _, account := range accounts { + groups = append(groups, types.ReservationCoverageGroup{Attributes: map[string]string{"linkedAccount": account}}) + } + out := &costexplorer.GetReservationCoverageOutput{CoveragesByTime: []types.CoverageByTime{{Groups: groups}}} + if next != "" { + out.NextPageToken = aws.String(next) + } + return out +} + +type codedError struct{ code string } + +func (e codedError) Error() string { return e.code } +func (e codedError) ErrorCode() string { return e.code } + +func probeCfg() Config { + cfg := gatedCfg() + cfg.CallerAccountID = "111111111111" + return cfg +} + +func TestProbeServiceAccounts(t *testing.T) { + ctx := context.Background() + t.Run("caller only across pages passes", func(t *testing.T) { + fake := &fakeCoverage{t: t, pages: []*costexplorer.GetReservationCoverageOutput{ + groupsPage("p2", "111111111111"), groupsPage("", "111111111111", "111111111111")}} + require.NoError(t, probeServiceAccounts(ctx, fake, common.ServiceEC2, probeCfg())) + require.Len(t, fake.inputs, 2) + assert.Nil(t, fake.inputs[0].NextPageToken) + assert.Equal(t, "p2", aws.ToString(fake.inputs[1].NextPageToken)) + }) + t.Run("a member that sees only itself passes", func(t *testing.T) { + fake := &fakeCoverage{t: t, pages: []*costexplorer.GetReservationCoverageOutput{groupsPage("", "111111111111")}} + require.NoError(t, probeServiceAccounts(ctx, fake, common.ServiceEC2, probeCfg())) + }) + t.Run("a standalone account with no groups passes", func(t *testing.T) { + fake := &fakeCoverage{t: t} + require.NoError(t, probeServiceAccounts(ctx, fake, common.ServiceEC2, probeCfg())) + }) + t.Run("foreign account refuses at the first foreign page without paging on", func(t *testing.T) { + fake := &fakeCoverage{t: t, pages: []*costexplorer.GetReservationCoverageOutput{groupsPage("more", "111111111111", "222222222222")}} + err := probeServiceAccounts(ctx, fake, common.ServiceEC2, probeCfg()) + require.ErrorIs(t, err, errRebuyScope) + assert.Equal(t, "account 222222222222 has in-scope ec2 usage, but the credentials are for account 111111111111; gated rebuy needs a standalone account, or a management account whose member accounts have no in-scope usage; otherwise omit --rebuy-window-days", err.Error()) + assert.Len(t, fake.inputs, 1) + }) + t.Run("foreign account only in the RDS pool refuses with the RDS filter", func(t *testing.T) { + fake := &fakeCoverage{t: t, pages: []*costexplorer.GetReservationCoverageOutput{groupsPage("", "333333333333")}} + err := probeServiceAccounts(ctx, fake, common.ServiceRDS, probeCfg()) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "333333333333") + assert.Contains(t, err.Error(), "rds") + require.Len(t, fake.inputs, 1) + assert.Equal(t, []string{"Amazon Relational Database Service"}, fake.inputs[0].Filter.And[0].Dimensions.Values) + }) + t.Run("an account compared exactly: padding is foreign", func(t *testing.T) { + fake := &fakeCoverage{t: t, pages: []*costexplorer.GetReservationCoverageOutput{groupsPage("", " 111111111111")}} + require.ErrorIs(t, probeServiceAccounts(ctx, fake, common.ServiceEC2, probeCfg()), errRebuyScope) + }) + t.Run("a group without the attribute refuses", func(t *testing.T) { + page := &costexplorer.GetReservationCoverageOutput{CoveragesByTime: []types.CoverageByTime{{Groups: []types.ReservationCoverageGroup{{Attributes: map[string]string{"instanceType": "m5.large"}}}}}} + fake := &fakeCoverage{t: t, pages: []*costexplorer.GetReservationCoverageOutput{page}} + err := probeServiceAccounts(ctx, fake, common.ServiceEC2, probeCfg()) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "no linked-account attribute") + }) + t.Run("the page cap refuses", func(t *testing.T) { + fake := &fakeCoverage{t: t, pages: []*costexplorer.GetReservationCoverageOutput{groupsPage("again", "111111111111")}} + err := probeServiceAccounts(ctx, fake, common.ServiceEC2, probeCfg()) + require.ErrorIs(t, err, errRebuyScope) + assert.Len(t, fake.inputs, maxProbePages) + assert.Contains(t, err.Error(), "more than 100 pages") + }) + t.Run("a call error refuses", func(t *testing.T) { + fake := &fakeCoverage{t: t, err: errors.New("throttled")} + err := probeServiceAccounts(ctx, fake, common.ServiceEC2, probeCfg()) + require.ErrorIs(t, err, errRebuyScope) + assert.Equal(t, "the scope check could not run (throttled); fix access or retry, or omit --rebuy-window-days", err.Error()) + }) + t.Run("AccessDenied adds the Cost Explorer remedy", func(t *testing.T) { + fake := &fakeCoverage{t: t, err: codedError{"AccessDeniedException"}} + err := probeServiceAccounts(ctx, fake, common.ServiceEC2, probeCfg()) + require.ErrorIs(t, err, errRebuyScope) + assert.Equal(t, "the scope check could not run (AccessDeniedException); fix access or retry, grant ce:GetReservationCoverage or enable Cost Explorer access for this account, or omit --rebuy-window-days", err.Error()) + }) + t.Run("an unmapped service refuses without a call", func(t *testing.T) { + fake := &fakeCoverage{t: t, noCalls: true} + err := probeServiceAccounts(ctx, fake, common.ServiceCompute, probeCfg()) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "no Cost Explorer service mapping") + }) +} + +func TestProbeRequestShape(t *testing.T) { + ctx := context.Background() + services := map[common.ServiceType]string{ + common.ServiceEC2: "Amazon Elastic Compute Cloud - Compute", + common.ServiceRDS: "Amazon Relational Database Service", + common.ServiceElastiCache: "Amazon ElastiCache", + common.ServiceOpenSearch: "Amazon OpenSearch Service", + common.ServiceRedshift: "Amazon Redshift", + common.ServiceMemoryDB: "Amazon MemoryDB", + } + for service, want := range services { + fake := &fakeCoverage{t: t} + require.NoError(t, probeServiceAccounts(ctx, fake, service, probeCfg())) + require.Len(t, fake.inputs, 1) + in := fake.inputs[0] + assert.Equal(t, []types.Expression{{Dimensions: &types.DimensionValues{Key: types.DimensionService, Values: []string{want}}}}, in.Filter.And, "%s: default mode has no REGION filter", service) + assert.Equal(t, []types.GroupDefinition{{Type: types.GroupDefinitionTypeDimension, Key: aws.String("LINKED_ACCOUNT")}}, in.GroupBy) + assert.Equal(t, []string{"Hour"}, in.Metrics) + } + t.Run("explicit regions are one REGION expression with all of them", func(t *testing.T) { + cfg := probeCfg() + cfg.Regions = []string{"us-east-1", "eu-west-1"} + fake := &fakeCoverage{t: t} + require.NoError(t, probeServiceAccounts(ctx, fake, common.ServiceEC2, cfg)) + require.Len(t, fake.inputs[0].Filter.And, 2) + assert.Equal(t, &types.DimensionValues{Key: types.DimensionRegion, Values: []string{"us-east-1", "eu-west-1"}}, fake.inputs[0].Filter.And[1].Dimensions) + }) + t.Run("the window is the coverage lookback", func(t *testing.T) { + for lookback, wantDays := range map[int]int{0: 30, 7: 7} { + cfg := probeCfg() + cfg.CoverageLookbackDays = lookback + fake := &fakeCoverage{t: t} + require.NoError(t, probeServiceAccounts(ctx, fake, common.ServiceEC2, cfg)) + start, err := time.Parse(time.DateOnly, aws.ToString(fake.inputs[0].TimePeriod.Start)) + require.NoError(t, err) + end, err := time.Parse(time.DateOnly, aws.ToString(fake.inputs[0].TimePeriod.End)) + require.NoError(t, err) + assert.Equal(t, time.Duration(wantDays)*24*time.Hour, end.Sub(start)) + } + }) +} + +func TestGroupAccount(t *testing.T) { + for name, tc := range map[string]struct { + attrs map[string]string + want string + wantErr string + }{ + "camelCase key": {map[string]string{"linkedAccount": "111111111111"}, "111111111111", ""}, + "SNAKE_CASE key": {map[string]string{"LINKED_ACCOUNT": "111111111111"}, "111111111111", ""}, + "other attributes": {map[string]string{"instanceType": "m5.large", "linkedAccount": "111111111111"}, "111111111111", ""}, + "value is not trimmed": {map[string]string{"linkedAccount": " 111111111111 "}, " 111111111111 ", ""}, + "missing": {map[string]string{"instanceType": "m5.large"}, "", "no linked-account attribute"}, + "nil attributes": {nil, "", "no linked-account attribute"}, + "empty value": {map[string]string{"linkedAccount": ""}, "", "empty linked-account attribute"}, + "duplicate keys": {map[string]string{"linkedAccount": "111111111111", "LINKED_ACCOUNT": "222222222222"}, "", "duplicate linked-account attributes"}, + } { + t.Run(name, func(t *testing.T) { + got, err := groupAccount(tc.attrs) + if tc.wantErr != "" { + require.ErrorContains(t, err, tc.wantErr) + return + } + require.NoError(t, err) + assert.Equal(t, tc.want, got) + }) + } +} From 723692a4de90289e4e98d9028f68aa7d72c102ff Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 15:24:55 +0100 Subject: [PATCH 05/10] test(cmd): run the expiry suites under the scope gate Gated success scenarios now answer the scope checks. Multi-account filtered-subset scenarios that the gate refuses no longer expect a CSV. Refs #2131 Co-Authored-By: Claude Opus 5.5 --- cmd/rds_family_expiry_command_test.go | 35 ++++++++++---- cmd/reservation_expiry_command_test.go | 64 ++++++++++++++++++-------- 2 files changed, 71 insertions(+), 28 deletions(-) diff --git a/cmd/rds_family_expiry_command_test.go b/cmd/rds_family_expiry_command_test.go index b21b20194..654ddcc6f 100644 --- a/cmd/rds_family_expiry_command_test.go +++ b/cmd/rds_family_expiry_command_test.go @@ -72,6 +72,9 @@ type rdsExpiryScenario struct { absentLogLine string // parent documents the count the commit before the fix produced. parent []int + // foreignUsage is an account the scope probe reports besides the caller. + foreignUsage string + orgMaster string } const rdsExpiryTarget = 80 @@ -120,6 +123,8 @@ func rdsExpiryCase(name string) rdsExpiryScenario { recs: one("db.r6g.xlarge", 4), want: []int{2}, existing: 60, parent: []int{0}, } case "multi-size-linked-accounts": + // Refused since cli#2131 (rebuy_scope_command_test.go): the recs span + // three accounts. Sizing math kept for the day the probe allows it. // Two sizes and three linked-account rows in one family: the 16 expiring // NU are deducted once (family 40% -> 20%), so the 60% gap (48 NU) splits // 2 + 4 + 2. A double deduction would buy 2 + 5 + 2; no deduction, 1 + 2 + 1. @@ -132,14 +137,17 @@ func rdsExpiryCase(name string) rdsExpiryScenario { want: []int{2, 4, 2}, existing: 20, parent: []int{1, 2, 1}, } case "partial-inventory": - // Cost Explorer covers 64 NU org-wide but the single-account inventory - // lists only the 4 expiring large RIs (16 NU); the other accounts' RIs - // are missing. The deduction is capped at the 16 expiring NU (80% -> - // 60%, buy 4 large), never the whole supplied-vs-covered difference. + // Refused since cli#2131: Cost Explorer covers 64 NU org-wide but the + // single-account inventory lists only the 4 expiring large RIs (16 NU); + // the other accounts' RIs are missing, which the scope probe detects as + // another account's usage in the pool. The sizing math below is kept for + // the day the probe allows it (deduction capped at the 16 expiring NU: + // 80% -> 60%, buy 4 large). return rdsExpiryScenario{ - usage: []rdsExpiryUsage{mysql("db.r6g.large", 20, 80)}, - ris: []rdsExpiryRI{ri("db.r6g.large", 4, 15)}, - recs: one("db.r6g.large", 8), want: []int{4}, existing: 60, parent: []int{0}, + foreignUsage: "999999999999", + usage: []rdsExpiryUsage{mysql("db.r6g.large", 20, 80)}, + ris: []rdsExpiryRI{ri("db.r6g.large", 4, 15)}, + recs: one("db.r6g.large", 8), want: []int{4}, existing: 60, parent: []int{0}, } case "unusable-expiring-warned": // Expiring RIs the family pass cannot use (multi-AZ with no coverage @@ -207,8 +215,8 @@ func rdsExpiryCase(name string) rdsExpiryScenario { func TestRDSFamilyExpiryCommand(t *testing.T) { for _, name := range []string{ - "already-at-target", "below-target", "large-ri-xlarge-usage", "multi-size-linked-accounts", - "partial-inventory", "unusable-expiring-warned", "control-outside-window", "control-engine-isolation", + "already-at-target", "below-target", "large-ri-xlarge-usage", + "unusable-expiring-warned", "control-outside-window", "control-engine-isolation", "rerun-replacement-bought", "control-replacement-outside-lookback", } { t.Run(name, func(t *testing.T) { runCompletenessScenario(t, rdsExpiryService, "explicit", name) }) @@ -324,6 +332,9 @@ func (p *completenessProxy) respondRDSExpiry(req *http.Request, body []byte) *ht func (p *completenessProxy) rdsExpiryResponse(host, op string, body []byte, values url.Values) (int, string, string) { s := rdsExpiryCase(p.details) + if payload, ok := p.describeOrganization(host, op, s.orgMaster); ok { + return 200, "application/x-amz-json-1.1", payload + } switch { case host == "ce.us-east-1.amazonaws.com" && op == "AWSInsightsIndexService.GetReservationPurchaseRecommendation": var request map[string]any @@ -345,6 +356,9 @@ func (p *completenessProxy) rdsExpiryResponse(host, op string, body []byte, valu require.NoError(p.t, err) return 200, "application/x-amz-json-1.1", string(encoded) case host == "ce.us-east-1.amazonaws.com" && op == "AWSInsightsIndexService.GetReservationCoverage": + if payload, ok := p.linkedAccountCoverage(body, "Amazon Relational Database Service", s.foreignUsage); ok { + return 200, "application/x-amz-json-1.1", payload + } return 200, "application/x-amz-json-1.1", p.rdsExpiryCoverage(s, body) case host == "organizations.us-east-1.amazonaws.com" && op == "AWSOrganizationsV20161128.DescribeAccount": var request map[string]string @@ -433,6 +447,7 @@ func (p *completenessProxy) assertRDSExpiryRequests(t *testing.T) { t.Logf("rds expiry op %s x%d", op, n) } require.Equal(t, 1, p.requests["ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationPurchaseRecommendation"]) - require.Equal(t, 12, p.requests["ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage"]) + require.Equal(t, 13, p.requests["ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage"], "12 coverage-map calls plus the scope probe") + require.Equal(t, 1, p.requests[orgDescribeKey], "one organization role check") require.Positive(t, p.requests["rds.us-east-1.amazonaws.com/DescribeReservedDBInstances"]) } diff --git a/cmd/reservation_expiry_command_test.go b/cmd/reservation_expiry_command_test.go index 8a9f795ea..8a6a4260b 100644 --- a/cmd/reservation_expiry_command_test.go +++ b/cmd/reservation_expiry_command_test.go @@ -24,28 +24,50 @@ type reservationExpiryScenario struct { average, demand, target, existing float64 missing, filtered bool replacedDays, replacedCount int + // inventoryFails makes DescribeReservedInstances answer AccessDenied. + inventoryFails bool + // foreignUsage is an account the scope probe reports besides the caller. + foreignUsage string + // orgMaster is the management account DescribeOrganization reports; + // empty means the caller (a management account). + orgMaster string } +// reservationExpiryCase defines each scenario. Since cli#2131 a gated rebuy run +// refuses a recommendation owned by another account, so every success scenario +// is a single account (the caller, 111111111111) whose usage is the whole pool: +// average equals demand, so Cost Explorer's pool rescaling is the identity. func reservationExpiryCase(name string) reservationExpiryScenario { - s := reservationExpiryScenario{rows: 3, expiry: 18, endDays: 15, count: 12, average: 30, demand: 90, target: 80, existing: 40} + s := reservationExpiryScenario{rows: 1, expiry: 18, endDays: 15, count: 36, average: 90, demand: 90, target: 80, existing: 40} switch name { case "pool-demand": case "outside-window": - s.endDays, s.count, s.existing = 180, 6, 60 + s.endDays, s.count, s.existing = 180, 18, 60 case "replaced-inside-lookback": // Replacements bought 5 days ago are inside the 30-day coverage // lookback, so the expiring RIs net to zero and the run must not // re-buy them (cli#2235). - s.count, s.existing, s.replacedDays, s.replacedCount = 6, 60, 5, 18 - case "missing-demand-two-each-six-total": + s.count, s.existing, s.replacedDays, s.replacedCount = 18, 60, 5, 18 + case "missing-demand-two-total": s.average, s.demand, s.count, s.existing, s.missing = 10, 0, 2, 60, true - case "filtered-subset": - s.count, s.filtered = 18, true case "exact-boundary", "below-boundary": - s.rows, s.expiry, s.count, s.average, s.demand, s.existing = 2, 2, 4, 15, 30, 160.0/3 + s.expiry, s.count, s.average, s.demand, s.existing = 2, 8, 30, 30, 160.0/3 if name == "below-boundary" { - s.target, s.count = math.Nextafter(80, 0), 3 + s.target, s.count = math.Nextafter(80, 0), 7 } + case "exclude-others-refuses": + // Three accounts, the run excludes the two foreign ones: the scope check + // still refuses because it runs before the exclusion filter. + s.rows, s.average, s.filtered = 3, 30, true + case "inventory-error": + s.inventoryFails = true + case "member-account-refuses": + // The caller belongs to an organization managed by another account. + s.orgMaster = "999999999999" + case "foreign-usage-default-region": + // Every rec row is the caller's, but Cost Explorer reports another + // account's usage in the pool: only the probe can see it. + s.foreignUsage = "999999999999" default: panic("unknown expiry scenario: " + name) } @@ -53,7 +75,7 @@ func reservationExpiryCase(name string) reservationExpiryScenario { } func TestReservationExpiryCommand(t *testing.T) { - for _, name := range []string{"pool-demand", "outside-window", "replaced-inside-lookback", "missing-demand-two-each-six-total", "filtered-subset", "exact-boundary", "below-boundary"} { + for _, name := range []string{"pool-demand", "outside-window", "replaced-inside-lookback", "missing-demand-two-total", "exact-boundary", "below-boundary"} { t.Run(name, func(t *testing.T) { runCompletenessScenario(t, "ec2", "explicit", name) }) } } @@ -62,7 +84,7 @@ func reservationExpiryArgs(name string) []string { s := reservationExpiryCase(name) args := []string{"--payment", "partial-upfront", "--target-coverage", strconv.FormatFloat(s.target, 'g', -1, 64), "--rebuy-window-days", "30"} if s.filtered { - args = append(args, "--exclude-accounts", "expiry-account-3") + args = append(args, "--exclude-accounts", "expiry-account-2,expiry-account-3") } return args } @@ -78,7 +100,7 @@ func assertReservationExpiryCSV(t *testing.T, output, name, logs string) { warnings := 0 if s.missing { warnings = 1 - require.Contains(t, logs, "3 recommendations because pool demand was unavailable; coverage was left unchanged") + require.Contains(t, logs, "1 recommendations because pool demand was unavailable; coverage was left unchanged") } require.Equal(t, warnings, strings.Count(logs, "Skipped expiry adjustment for"), logs) for _, failure := range []string{"Failed to fetch", "incomplete AWS", "Could not fetch", "failed to fetch", "WARNING:", "no signal", "catalog fetch"} { @@ -86,10 +108,6 @@ func assertReservationExpiryCSV(t *testing.T, output, name, logs string) { } count := s.rows avg := s.average - if s.filtered { - count-- - avg = s.demand / float64(count) - } require.Len(t, rows, count+2, logs) require.NotContains(t, rows[0], "ExistingCoveragePercentExact") seen := make(map[string]bool) @@ -149,6 +167,9 @@ func (p *completenessProxy) respondReservationExpiry(req *http.Request, body []b func (p *completenessProxy) reservationExpiryResponse(host, op string, body []byte, values url.Values) (int, string, string) { s := reservationExpiryCase(p.details) + if payload, ok := p.describeOrganization(host, op, s.orgMaster); ok { + return 200, "application/x-amz-json-1.1", payload + } if host == "ce.us-east-1.amazonaws.com" { switch op { case "AWSInsightsIndexService.GetReservationPurchaseRecommendation": @@ -195,6 +216,9 @@ func (p *completenessProxy) reservationExpiryResponse(host, op string, body []by case "DescribeInstanceTypes": return 200, "text/xml", `m5.large28192` case "DescribeReservedInstances": + if s.inventoryFails { + return 403, "text/xml", `UnauthorizedOperationfixture denied reserved instancesfixture` + } return 200, "text/xml", reservationExpiryReservedInstances(s) } } @@ -232,6 +256,9 @@ func reservationExpiryReservedInstances(s reservationExpiryScenario) string { } func (p *completenessProxy) reservationExpiryCoverage(body []byte) string { + if payload, ok := p.linkedAccountCoverage(body, "Amazon Elastic Compute Cloud - Compute", reservationExpiryCase(p.details).foreignUsage); ok { + return payload + } var request struct { TimePeriod map[string]string GroupBy []map[string]string @@ -294,10 +321,11 @@ func (p *completenessProxy) assertReservationExpiryRequests(t *testing.T) { s := reservationExpiryCase(p.details) want := map[string]int{ "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationPurchaseRecommendation": 1, - "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage": 12, + "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage": 13, "organizations.us-east-1.amazonaws.com/AWSOrganizationsV20161128.DescribeAccount": s.rows, - "ec2.us-east-1.amazonaws.com/DescribeInstanceTypes": 1, - "ec2.us-east-1.amazonaws.com/DescribeReservedInstances": 2, + orgDescribeKey: 1, + "ec2.us-east-1.amazonaws.com/DescribeInstanceTypes": 1, + "ec2.us-east-1.amazonaws.com/DescribeReservedInstances": 2, } require.Equal(t, want, p.requests, "no other operation, including purchases, is permitted") wantCoverage := make([]string, 0, 12) From 9db2308528267a14f3468fa0de26b47eb37605fd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 15:25:02 +0100 Subject: [PATCH 06/10] docs(rebuy): document the scope refusal and the new read-only permission Covers the --rebuy-window-days help text, the README flag row, the Scope section of purchase-safety.md, the AWS permissions section of cloud-setup.md and the CHANGELOG. Closes #2131 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 8 ++++++++ cmd/main.go | 7 ++++++- docs/cli/README.md | 2 +- docs/cli/cloud-setup.md | 12 ++++++++++++ docs/cli/purchase-safety.md | 26 +++++++++++++++++++++++--- 5 files changed, 50 insertions(+), 5 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 26614fa17..ac49ae1d3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -54,6 +54,14 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). - `--target-coverage` with `--rebuy-window-days` no longer buys replacement EC2 and other non-RDS RIs again when they were already bought inside the coverage lookback: the expiring RIs are netted against those replacements (#2235). +- `--rebuy-window-days` with `--target-coverage` now refuses to run (exit 1, no + CSV, dry runs included) when the credentials' account does not own all the + in-scope RI usage, instead of netting one account's expiring RIs against + broader demand. Member accounts of an AWS Organization are refused. A failed + STS lookup, role check, scope probe (new Cost Explorer + `GetReservationCoverage` query grouped by linked account) or RI inventory + read also exits 1 on these runs. These runs now need the read-only + `organizations:DescribeOrganization` permission (#2131). - `--min-savings-pct` and `--max-break-even-months` now apply on `--input-csv` runs instead of being refused. The CSV report gains `SavingsPercentage` and `BreakEvenMonths` columns; replaying a report that lacks a column a set flag diff --git a/cmd/main.go b/cmd/main.go index e9ccbd136..f07f554ae 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -161,7 +161,12 @@ func init() { rootCmd.Flags().IntVar(&toolCfg.RebuyWindowDays, "rebuy-window-days", 0, "When >0, treat existing RIs expiring within this many days as already "+ "uncovered, so --target-coverage sizes recommendations to replace them "+ - "before they expire. Default 0 = trust existing coverage fully.") + "before they expire. Default 0 = trust existing coverage fully. "+ + "With --target-coverage, refuses to run from a member account of an AWS "+ + "Organization, when the credentials' account does not own all in-scope "+ + "usage, or when a scope check or the RI inventory read fails. Needs "+ + "read-only sts:GetCallerIdentity, organizations:DescribeOrganization "+ + "and ce:GetReservationCoverage; see docs/cli/purchase-safety.md.") rootCmd.Flags().Float64Var(&toolCfg.MinPoolSize, "min-pool-size", 0, "When >0, drop RI recommendations for pools with AverageInstancesUsedPerHour "+ "below this threshold. Useful with --target-coverage to skip tiny pools "+ diff --git a/docs/cli/README.md b/docs/cli/README.md index c603f2c09..6a7d40d29 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -83,7 +83,7 @@ All flags belong to the root command unless noted otherwise. | Flag | Default | Description | |------|---------|-------------| -| `--rebuy-window-days` | `0` (disabled) | When set, treat existing RIs whose remaining term is at most this many days as already uncovered, so `--target-coverage` sizes replacements before they expire. `0` fully trusts existing coverage. See [purchase-safety.md](purchase-safety.md). | +| `--rebuy-window-days` | `0` (disabled) | When set, treat existing RIs whose remaining term is at most this many days as already uncovered, so `--target-coverage` sizes replacements before they expire. `0` fully trusts existing coverage. With `--target-coverage`, refuses to run from a member account of an AWS Organization, when the account does not own all in-scope usage, or when a scope check or the RI inventory read fails; needs read-only `sts:GetCallerIdentity`, `organizations:DescribeOrganization` and `ce:GetReservationCoverage`. See [purchase-safety.md](purchase-safety.md) and [cloud-setup.md](cloud-setup.md#aws-permissions-for---rebuy-window-days). | | `--min-pool-size` | `0` (no filter) | When set, drop RI recommendations for pools whose `AverageInstancesUsedPerHour` is below this threshold. Prevents integer-arithmetic over-coverage on tiny pools (e.g. average=1 cannot approximate 80% coverage without effectively buying 100%). SPs and recommendations without a per-hour signal pass through unfiltered. See [filtering.md](filtering.md). | ### Input / output diff --git a/docs/cli/cloud-setup.md b/docs/cli/cloud-setup.md index 9ba312610..cf82d4305 100644 --- a/docs/cli/cloud-setup.md +++ b/docs/cli/cloud-setup.md @@ -323,6 +323,18 @@ The command expects a standard GCP Service Account JSON key file (type `service_ Any missing required field causes the command to exit with an error before writing to Secrets Manager. +### AWS permissions for --rebuy-window-days + +A run that sets `--rebuy-window-days` with `--target-coverage` on an RI service first checks that the credentials' account owns all the in-scope usage (see [purchase-safety.md](purchase-safety.md#scope)). The check needs one new read-only permission, `organizations:DescribeOrganization`. It also calls two actions the run already uses: + +| Action | Used for | If the call fails | +|--------|----------|------------| +| `sts:GetCallerIdentity` | Identifies the credentials' account (IAM cannot deny it) | The run exits 1 and names the failed lookup | +| `organizations:DescribeOrganization` | Tells a standalone, management or member account apart; member accounts are refused | The run exits 1 and asks you to grant it or omit the flag | +| `ce:GetReservationCoverage` | Already needed by `--target-coverage`; here it also lists which accounts have usage in the sized pool | The run exits 1 and names the cause; a Cost Explorer denial also asks you to grant it or enable Cost Explorer access for the account | + +`organizations:DescribeOrganization` is not needed when `--rebuy-window-days` is not set. + ## Usage notes | Scenario | Notes | diff --git a/docs/cli/purchase-safety.md b/docs/cli/purchase-safety.md index 5c36d2d64..618344412 100644 --- a/docs/cli/purchase-safety.md +++ b/docs/cli/purchase-safety.md @@ -81,7 +81,7 @@ A duplicate check runs before every purchase, on both the `--services` and `--in The window is a Go duration string that must be a positive whole number of hours (e.g. `24h`, `48h`, `72h`). A value that doesn't parse, is zero or negative, or isn't whole hours (e.g. `90m`, `1h30m`) is rejected at startup, before any API call, rather than rounded or replaced with the default. -If the existing-commitments lookup itself fails (a transient API error), the two modes diverge: a dry run continues with a warning printed to the log (nothing is bought, so reporting fidelity wins), while a `--purchase` run refuses that (service, region) - it prints a "Refusing to purchase" line and buys nothing there, rather than falling back to the un-deduplicated counts ([#1941](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1941)). +If the existing-commitments lookup itself fails (a transient API error), the two modes diverge: a dry run continues with a warning printed to the log (nothing is bought, so reporting fidelity wins), while a `--purchase` run refuses that (service, region) - it prints a "Refusing to purchase" line and buys nothing there, rather than falling back to the un-deduplicated counts ([#1941](https://github.com/LeanerCloud/cloud-commitments-cli/issues/1941)). The exception is `--rebuy-window-days` with `--target-coverage` on an RI service: there a failed existing-RI inventory read exits 1 first, on dry runs too (see [Scope](#scope)). The audit status value `skipped_covered` (idempotency hit) is defined in the audit record schema for use by the server-side scheduler path and is not emitted by this CLI's dedup check. @@ -111,6 +111,26 @@ cudly --services rds \ `--rebuy-window-days` has no effect when `--target-coverage` is not set. +### Scope + +Rebuy reads the existing-RI inventory of the credentials' own account only. The `--target-coverage` demand it sizes against can be broader: a payer or management account sees the usage of its member accounts. Netting one account's expiring RIs against another account's demand would be wrong, so a run with `--rebuy-window-days` and `--target-coverage` on an RI service refuses when it cannot show that the credentials' account owns all the in-scope usage. Savings Plans are never expiry-adjusted and are not checked. + +The run exits 1 and writes no CSV, on dry runs and `--purchase` runs alike, in these cases: + +- the credentials belong to a member account of an AWS Organization. Replacements bought centrally are not visible to a member account, so its expiry netting could buy them again. Gated rebuy runs from a standalone account, or from a management account whose member accounts have no in-scope usage; +- a recommendation row belongs to another account; +- a Cost Explorer coverage query (grouped by linked account, with the same lookback window, service and, when `--regions` is set, region filters as sizing) shows usage in another account, so recommendations hidden behind your own account's rows are caught too; +- the check itself could not run: the STS identity lookup, the `organizations:DescribeOrganization` role check, the Cost Explorer scope query or the RI inventory read failed. Before this check, a failed RI inventory read on a dry run only warned and sized without the expiry netting; now it exits 1. + +A scope refusal names the other account and the service. To recover, run from a standalone account or from a management account whose member accounts have no in-scope usage, or omit `--rebuy-window-days`. A check failure says "the scope check could not run" and the cause: fix access or retry, or omit `--rebuy-window-days`. A failed role check also asks you to grant `organizations:DescribeOrganization` (read-only); see [cloud-setup.md](cloud-setup.md#aws-permissions-for---rebuy-window-days). A Cost Explorer AccessDenied also asks you to grant `ce:GetReservationCoverage` or enable Cost Explorer access for the account. A Cost Explorer failure while building the coverage map on a dry run still only warns, as described above. `--input-csv` runs never apply rebuy. + +Limits of the check, in both directions, for a management account that passes it: + +- expiring RIs owned by member accounts are missing from the inventory, so the run can under-replace; +- a replacement bought by a member account and shared through RI sharing to the management account's usage is not credited against the management account's own expiring RIs, so the run can buy it again: an over-purchase. This is rarer than the member case the check refuses, and it cannot be detected from the management account. + +Purchases for other accounts are still refused regardless. + ## Between-purchase delay cudly inserts a short delay between consecutive purchases to respect AWS API rate limits. This delay is hardcoded (a few seconds) and cannot be configured via a user-facing flag. @@ -129,8 +149,8 @@ Before any real purchase run: 1. Run without `--purchase` first to review the dry-run CSV output. 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. +3. If using `--target-coverage`, verify `--rebuy-window-days` is set appropriately for your RI renewal cadence, and that the credentials' account owns all in-scope usage (see [Scope](#scope)). 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. 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. +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. With `--rebuy-window-days` and `--target-coverage`, a failed RI inventory read exits 1 instead. 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 e2a1d12d0ab016c841a97331601a3d56c324b596 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 19:32:55 +0100 Subject: [PATCH 07/10] fix(rebuy): send the probe's lone SERVICE filter as a bare dimension Cost Explorer rejects an And with fewer than two operands. Without --regions the scope probe sent And{SERVICE}, so every default-mode gated run would fail the check and exit 1. A single filter is now the bare dimension, and SERVICE plus REGION stays an And, as in cloud-commitments-go's CE filter builders (spUtilizationFilter). The command fake now accepts a bare dimension and fails a one-operand And. The probe unit tests pin both shapes. Refs #2131 Co-Authored-By: Claude Opus 5.5 --- cmd/rebuy_scope_command_test.go | 22 +++++++++++++++++----- cmd/rebuy_scope_probe.go | 11 ++++++++--- cmd/rebuy_scope_probe_test.go | 6 ++++-- 3 files changed, 29 insertions(+), 10 deletions(-) diff --git a/cmd/rebuy_scope_command_test.go b/cmd/rebuy_scope_command_test.go index dc1466180..bca141e02 100644 --- a/cmd/rebuy_scope_command_test.go +++ b/cmd/rebuy_scope_command_test.go @@ -100,6 +100,12 @@ func TestRebuyScopeRefusalCommand(t *testing.T) { } } +// ceDimension is a Cost Explorer DimensionValues as the JSON protocol sends it. +type ceDimension struct { + Key string + Values []string +} + // linkedAccountCoverage is the shared LINKED_ACCOUNT router: it answers the // scope probe (GroupBy LINKED_ACCOUNT) and reports ok=false for any other // coverage request. It pins the probe's request shape: the 30-day window, @@ -111,11 +117,9 @@ func (p *completenessProxy) linkedAccountCoverage(body []byte, ceService, foreig TimePeriod map[string]string GroupBy []map[string]string Filter struct { - And []struct { - Dimensions struct { - Key string - Values []string - } + Dimensions *ceDimension + And []struct { + Dimensions ceDimension } } Metrics []string @@ -134,7 +138,15 @@ func (p *completenessProxy) linkedAccountCoverage(body []byte, ceService, foreig end, err := time.Parse(time.DateOnly, request.TimePeriod["End"]) require.NoError(p.t, err) require.Equal(p.t, 30*24*time.Hour, end.Sub(start)) + // Cost Explorer rejects an And with fewer than two operands, so one filter + // must be a bare dimension (as in cloud-commitments-go's CE filter builders). filters := make(map[string][]string) + if d := request.Filter.Dimensions; d != nil { + require.Empty(p.t, request.Filter.And, "a filter is a bare dimension or an And, not both") + filters[d.Key] = d.Values + } else { + require.GreaterOrEqual(p.t, len(request.Filter.And), 2, "Cost Explorer rejects an And with fewer than two operands") + } for _, expression := range request.Filter.And { require.NotContains(p.t, filters, expression.Dimensions.Key) filters[expression.Dimensions.Key] = expression.Dimensions.Values diff --git a/cmd/rebuy_scope_probe.go b/cmd/rebuy_scope_probe.go index d2958f6b9..f3a488fcd 100644 --- a/cmd/rebuy_scope_probe.go +++ b/cmd/rebuy_scope_probe.go @@ -91,17 +91,22 @@ func probeServiceAccounts(ctx context.Context, client coverageAPI, service commo // rebuyProbeInput builds the probe request: the coverage lookback window // (Metrics "Hour" as the coverage map), SERVICE and optional REGION filters. +// Cost Explorer rejects an And with fewer than two operands, so SERVICE alone +// is a bare dimension, as in cloud-commitments-go's CE filter builders. func rebuyProbeInput(ceService string, cfg Config) *costexplorer.GetReservationCoverageInput { end := time.Now().UTC() start := end.AddDate(0, 0, -coverageLookbackDays(cfg)) - filters := []types.Expression{{Dimensions: &types.DimensionValues{Key: types.DimensionService, Values: []string{ceService}}}} + filter := &types.Expression{Dimensions: &types.DimensionValues{Key: types.DimensionService, Values: []string{ceService}}} if len(cfg.Regions) > 0 { - filters = append(filters, types.Expression{Dimensions: &types.DimensionValues{Key: types.DimensionRegion, Values: cfg.Regions}}) + filter = &types.Expression{And: []types.Expression{ + *filter, + {Dimensions: &types.DimensionValues{Key: types.DimensionRegion, Values: cfg.Regions}}, + }} } return &costexplorer.GetReservationCoverageInput{ TimePeriod: &types.DateInterval{Start: aws.String(start.Format(time.DateOnly)), End: aws.String(end.Format(time.DateOnly))}, GroupBy: []types.GroupDefinition{{Type: types.GroupDefinitionTypeDimension, Key: aws.String(string(types.DimensionLinkedAccount))}}, - Filter: &types.Expression{And: filters}, + Filter: filter, Metrics: []string{"Hour"}, } } diff --git a/cmd/rebuy_scope_probe_test.go b/cmd/rebuy_scope_probe_test.go index e1e383421..4d700c151 100644 --- a/cmd/rebuy_scope_probe_test.go +++ b/cmd/rebuy_scope_probe_test.go @@ -95,7 +95,7 @@ func TestProbeServiceAccounts(t *testing.T) { assert.Contains(t, err.Error(), "333333333333") assert.Contains(t, err.Error(), "rds") require.Len(t, fake.inputs, 1) - assert.Equal(t, []string{"Amazon Relational Database Service"}, fake.inputs[0].Filter.And[0].Dimensions.Values) + assert.Equal(t, []string{"Amazon Relational Database Service"}, fake.inputs[0].Filter.Dimensions.Values) }) t.Run("an account compared exactly: padding is foreign", func(t *testing.T) { fake := &fakeCoverage{t: t, pages: []*costexplorer.GetReservationCoverageOutput{groupsPage("", " 111111111111")}} @@ -150,7 +150,7 @@ func TestProbeRequestShape(t *testing.T) { require.NoError(t, probeServiceAccounts(ctx, fake, service, probeCfg())) require.Len(t, fake.inputs, 1) in := fake.inputs[0] - assert.Equal(t, []types.Expression{{Dimensions: &types.DimensionValues{Key: types.DimensionService, Values: []string{want}}}}, in.Filter.And, "%s: default mode has no REGION filter", service) + assert.Equal(t, &types.Expression{Dimensions: &types.DimensionValues{Key: types.DimensionService, Values: []string{want}}}, in.Filter, "%s: default mode is the bare SERVICE dimension (no single-operand And)", service) assert.Equal(t, []types.GroupDefinition{{Type: types.GroupDefinitionTypeDimension, Key: aws.String("LINKED_ACCOUNT")}}, in.GroupBy) assert.Equal(t, []string{"Hour"}, in.Metrics) } @@ -160,6 +160,8 @@ func TestProbeRequestShape(t *testing.T) { fake := &fakeCoverage{t: t} require.NoError(t, probeServiceAccounts(ctx, fake, common.ServiceEC2, cfg)) require.Len(t, fake.inputs[0].Filter.And, 2) + assert.Nil(t, fake.inputs[0].Filter.Dimensions) + assert.Equal(t, &types.DimensionValues{Key: types.DimensionService, Values: []string{"Amazon Elastic Compute Cloud - Compute"}}, fake.inputs[0].Filter.And[0].Dimensions) assert.Equal(t, &types.DimensionValues{Key: types.DimensionRegion, Values: []string{"us-east-1", "eu-west-1"}}, fake.inputs[0].Filter.And[1].Dimensions) }) t.Run("the window is the coverage lookback", func(t *testing.T) { From 5c7f8cd0a5a257830aa3221dc548644c7789fa1c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 6 Nov 2026 19:33:05 +0100 Subject: [PATCH 08/10] docs(rebuy): say exactly when DescribeOrganization is needed Refs #2131 Co-Authored-By: Claude Opus 5.5 --- docs/cli/cloud-setup.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/cli/cloud-setup.md b/docs/cli/cloud-setup.md index cf82d4305..304cd0c7c 100644 --- a/docs/cli/cloud-setup.md +++ b/docs/cli/cloud-setup.md @@ -333,7 +333,7 @@ A run that sets `--rebuy-window-days` with `--target-coverage` on an RI service | `organizations:DescribeOrganization` | Tells a standalone, management or member account apart; member accounts are refused | The run exits 1 and asks you to grant it or omit the flag | | `ce:GetReservationCoverage` | Already needed by `--target-coverage`; here it also lists which accounts have usage in the sized pool | The run exits 1 and names the cause; a Cost Explorer denial also asks you to grant it or enable Cost Explorer access for the account | -`organizations:DescribeOrganization` is not needed when `--rebuy-window-days` is not set. +`organizations:DescribeOrganization` is needed only when the check runs: `--rebuy-window-days` and `--target-coverage` are both set and the run includes an RI service. Runs without either flag, and Savings Plans-only runs, never call it. ## Usage notes From 31f7c435e2f5f0a382f8286b60705047b06ca8a0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:10:35 +0100 Subject: [PATCH 09/10] fix(rebuy): widen the scope probe to the recommendation lookback The usage probe queried only --coverage-lookback-days, so a 60d recommendation could include another account usage from days 31-60 that the probe never saw. The probe now spans the larger of the coverage and recommendation lookbacks, and refuses when --rec-lookback-period cannot be read. Co-Authored-By: Claude Opus 5.5 --- cmd/rebuy_scope.go | 3 +- cmd/rebuy_scope_command_test.go | 17 +++++++--- cmd/rebuy_scope_probe.go | 43 +++++++++++++++++++++----- cmd/rebuy_scope_probe_test.go | 24 +++++++++++--- cmd/reservation_expiry_command_test.go | 9 ++++++ docs/cli/purchase-safety.md | 4 +-- 6 files changed, 82 insertions(+), 18 deletions(-) diff --git a/cmd/rebuy_scope.go b/cmd/rebuy_scope.go index 9ae8d64c9..385f0ad6b 100644 --- a/cmd/rebuy_scope.go +++ b/cmd/rebuy_scope.go @@ -103,7 +103,8 @@ func requireRecPreconditions(recs []common.Recommendation, cfg Config, data engi // an account other than the credentials'. The existing-commitment inventory // reads only the credentials' own account, so expiry netting against another // account's demand would be wrong. A recommendation with no account passes -// here; the scope probe covers it. +// here; the scope probe covers it, over the longer of --coverage-lookback-days +// and --rec-lookback-period, so usage the recommendations were sized on is seen. func requireRebuyRecScope(recs []common.Recommendation, cfg Config) error { for i := range recs { if !rebuyGated(cfg, recs[i].Service) { diff --git a/cmd/rebuy_scope_command_test.go b/cmd/rebuy_scope_command_test.go index bca141e02..d727f0335 100644 --- a/cmd/rebuy_scope_command_test.go +++ b/cmd/rebuy_scope_command_test.go @@ -73,6 +73,10 @@ func TestRebuyScopeRefusalCommand(t *testing.T) { orgDescribeKey: 1, "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage": 1}}, {"ec2", "foreign-usage-default-region", "default", []string{"Cannot verify the rebuy scope", "999999999999", "ec2"}, map[string]int{ orgDescribeKey: 1, "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage": 1}}, + // The foreign usage is only in days 31-60; the 60-day recommendation + // lookback must widen the probe to see it. + {"ec2", "foreign-usage-days-31-60", "default", []string{"Cannot verify the rebuy scope", "999999999999", "ec2"}, map[string]int{ + orgDescribeKey: 1, "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage": 1}}, // A member account is refused after the role check and before any Cost Explorer request. {"ec2", "member-account-refuses", "default", []string{"Cannot verify the rebuy scope", "member of an AWS Organization", "111111111111"}, map[string]int{ orgDescribeKey: 1}}, @@ -93,7 +97,7 @@ func TestRebuyScopeRefusalCommand(t *testing.T) { require.Equal(t, tc.requests, fixture.requests, "no other operation, including purchases, is permitted") wantProbes := tc.requests["ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage"] require.Equal(t, wantProbes-len(fixture.expiryCoverage), fixture.linkedAccountCalls, "coverage requests are the probe plus the coverage map") - if tc.name == "partial-inventory" || tc.name == "foreign-usage-default-region" { + if tc.name == "partial-inventory" || tc.name == "foreign-usage-default-region" || tc.name == "foreign-usage-days-31-60" { require.Empty(t, fixture.expiryCoverage, "the probe must refuse before the coverage map is built") } }) @@ -108,7 +112,7 @@ type ceDimension struct { // linkedAccountCoverage is the shared LINKED_ACCOUNT router: it answers the // scope probe (GroupBy LINKED_ACCOUNT) and reports ok=false for any other -// coverage request. It pins the probe's request shape: the 30-day window, +// coverage request. It pins the probe's request shape: the window (30 days, or 60 for the recommendation-lookback case), // Metrics Hour, one page, the SERVICE filter of the run's service, and the // REGION filter only in explicit region mode. The caller (111111111111) is // always reported; foreign, when set, is reported too. The caller holds p.mu. @@ -137,7 +141,12 @@ func (p *completenessProxy) linkedAccountCoverage(body []byte, ceService, foreig require.NoError(p.t, err) end, err := time.Parse(time.DateOnly, request.TimePeriod["End"]) require.NoError(p.t, err) - require.Equal(p.t, 30*24*time.Hour, end.Sub(start)) + wantDays, foreignVisible := 30, true + if p.details == "foreign-usage-days-31-60" { + wantDays = 60 + foreignVisible = end.Sub(start) > 30*24*time.Hour + } + require.Equal(p.t, time.Duration(wantDays)*24*time.Hour, end.Sub(start)) // Cost Explorer rejects an And with fewer than two operands, so one filter // must be a bare dimension (as in cloud-commitments-go's CE filter builders). filters := make(map[string][]string) @@ -157,7 +166,7 @@ func (p *completenessProxy) linkedAccountCoverage(body []byte, ceService, foreig } require.Equal(p.t, want, filters) groups := []any{map[string]any{"Attributes": map[string]string{"linkedAccount": "111111111111"}}} - if foreign != "" { + if foreign != "" && foreignVisible { groups = append(groups, map[string]any{"Attributes": map[string]string{"linkedAccount": foreign}}) } payload, err := json.Marshal(map[string]any{"CoveragesByTime": []any{map[string]any{"Groups": groups}}}) diff --git a/cmd/rebuy_scope_probe.go b/cmd/rebuy_scope_probe.go index f3a488fcd..60df38093 100644 --- a/cmd/rebuy_scope_probe.go +++ b/cmd/rebuy_scope_probe.go @@ -7,6 +7,7 @@ import ( "time" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/aws/recommendations" "github.com/aws/aws-sdk-go-v2/aws" "github.com/aws/aws-sdk-go-v2/service/costexplorer" "github.com/aws/aws-sdk-go-v2/service/costexplorer/types" @@ -60,7 +61,8 @@ func probeRebuyScope(ctx context.Context, client coverageAPI, cfg Config, servic } // probeServiceAccounts asks Cost Explorer which accounts have usage in the -// same pool the coverage map sizes (same window, SERVICE and, when --regions +// same pool the recommendations and coverage map size (the longer of their +// two windows, same SERVICE and, when --regions // is set, REGION filters), grouped by LINKED_ACCOUNT. It refuses on the first // account other than the credentials'. Without --regions there is no REGION // filter: that is broader, so it can only refuse more. A member account sees @@ -70,7 +72,10 @@ func probeServiceAccounts(ctx context.Context, client coverageAPI, service commo if !ok { return refuseRebuyScope("no Cost Explorer service mapping for %s, so its usage scope cannot be checked; omit --rebuy-window-days", service) } - input := rebuyProbeInput(ceService, cfg) + input, err := rebuyProbeInput(ceService, cfg) + if err != nil { + return err + } for range maxProbePages { out, err := client.GetReservationCoverage(ctx, input) if err != nil { @@ -89,13 +94,37 @@ func probeServiceAccounts(ctx context.Context, client coverageAPI, service commo return refuseRebuyScope("the scope check could not run (more than %d pages of %s usage groups); omit --rebuy-window-days", maxProbePages, service) } -// rebuyProbeInput builds the probe request: the coverage lookback window -// (Metrics "Hour" as the coverage map), SERVICE and optional REGION filters. +// recLookbackDays maps --rec-lookback-period to days. An empty value is the +// pipeline's default; any other unrecognized value cannot be sized, so the +// probe refuses rather than guess a window. +func recLookbackDays(period string) (int, error) { + if period == "" { + period = recommendations.DefaultRecLookbackPeriod + } + switch period { + case "7d": + return 7, nil + case "30d": + return 30, nil + case "60d": + return 60, nil + } + return 0, refuseRebuyScope("cannot read --rec-lookback-period %q, so the scope window is unknown; use 7d, 30d or 60d, or omit --rebuy-window-days", period) +} + +// rebuyProbeInput builds the probe request: the larger of the coverage and +// recommendation lookback windows, so an account that contributed usage to +// either the recommendations or the coverage demand is seen (Metrics "Hour" as +// the coverage map), SERVICE and optional REGION filters. // Cost Explorer rejects an And with fewer than two operands, so SERVICE alone // is a bare dimension, as in cloud-commitments-go's CE filter builders. -func rebuyProbeInput(ceService string, cfg Config) *costexplorer.GetReservationCoverageInput { +func rebuyProbeInput(ceService string, cfg Config) (*costexplorer.GetReservationCoverageInput, error) { + recDays, err := recLookbackDays(cfg.RecLookbackPeriod) + if err != nil { + return nil, err + } end := time.Now().UTC() - start := end.AddDate(0, 0, -coverageLookbackDays(cfg)) + start := end.AddDate(0, 0, -max(coverageLookbackDays(cfg), recDays)) filter := &types.Expression{Dimensions: &types.DimensionValues{Key: types.DimensionService, Values: []string{ceService}}} if len(cfg.Regions) > 0 { filter = &types.Expression{And: []types.Expression{ @@ -108,7 +137,7 @@ func rebuyProbeInput(ceService string, cfg Config) *costexplorer.GetReservationC GroupBy: []types.GroupDefinition{{Type: types.GroupDefinitionTypeDimension, Key: aws.String(string(types.DimensionLinkedAccount))}}, Filter: filter, Metrics: []string{"Hour"}, - } + }, nil } // requireOwnAccountGroups refuses when a page names an account other than the diff --git a/cmd/rebuy_scope_probe_test.go b/cmd/rebuy_scope_probe_test.go index 4d700c151..eaf142aac 100644 --- a/cmd/rebuy_scope_probe_test.go +++ b/cmd/rebuy_scope_probe_test.go @@ -164,19 +164,35 @@ func TestProbeRequestShape(t *testing.T) { assert.Equal(t, &types.DimensionValues{Key: types.DimensionService, Values: []string{"Amazon Elastic Compute Cloud - Compute"}}, fake.inputs[0].Filter.And[0].Dimensions) assert.Equal(t, &types.DimensionValues{Key: types.DimensionRegion, Values: []string{"us-east-1", "eu-west-1"}}, fake.inputs[0].Filter.And[1].Dimensions) }) - t.Run("the window is the coverage lookback", func(t *testing.T) { - for lookback, wantDays := range map[int]int{0: 30, 7: 7} { + t.Run("the window is the larger of the coverage and recommendation lookbacks", func(t *testing.T) { + for _, tc := range []struct { + coverage int + rec string + wantDays int + }{ + {0, "", 30}, {7, "", 7}, {30, "7d", 30}, {30, "30d", 30}, {30, "60d", 60}, {90, "60d", 90}, + } { cfg := probeCfg() - cfg.CoverageLookbackDays = lookback + cfg.CoverageLookbackDays = tc.coverage + cfg.RecLookbackPeriod = tc.rec fake := &fakeCoverage{t: t} require.NoError(t, probeServiceAccounts(ctx, fake, common.ServiceEC2, cfg)) start, err := time.Parse(time.DateOnly, aws.ToString(fake.inputs[0].TimePeriod.Start)) require.NoError(t, err) end, err := time.Parse(time.DateOnly, aws.ToString(fake.inputs[0].TimePeriod.End)) require.NoError(t, err) - assert.Equal(t, time.Duration(wantDays)*24*time.Hour, end.Sub(start)) + assert.Equal(t, time.Duration(tc.wantDays)*24*time.Hour, end.Sub(start), "coverage %d rec %q", tc.coverage, tc.rec) } }) + t.Run("an unparsable recommendation lookback refuses before any request", func(t *testing.T) { + cfg := probeCfg() + cfg.RecLookbackPeriod = "SIXTY_DAYS" + fake := &fakeCoverage{t: t} + err := probeServiceAccounts(ctx, fake, common.ServiceEC2, cfg) + require.ErrorIs(t, err, errRebuyScope) + assert.Contains(t, err.Error(), "SIXTY_DAYS") + assert.Empty(t, fake.inputs) + }) } func TestGroupAccount(t *testing.T) { diff --git a/cmd/reservation_expiry_command_test.go b/cmd/reservation_expiry_command_test.go index 8a6a4260b..3f5883093 100644 --- a/cmd/reservation_expiry_command_test.go +++ b/cmd/reservation_expiry_command_test.go @@ -31,6 +31,8 @@ type reservationExpiryScenario struct { // orgMaster is the management account DescribeOrganization reports; // empty means the caller (a management account). orgMaster string + // recLookback is the --rec-lookback-period; empty keeps the default. + recLookback string } // reservationExpiryCase defines each scenario. Since cli#2131 a gated rebuy run @@ -68,6 +70,10 @@ func reservationExpiryCase(name string) reservationExpiryScenario { // Every rec row is the caller's, but Cost Explorer reports another // account's usage in the pool: only the probe can see it. s.foreignUsage = "999999999999" + case "foreign-usage-days-31-60": + // The foreign account has usage only before the 30-day coverage window, + // which a 60-day recommendation lookback still includes. + s.foreignUsage, s.recLookback = "999999999999", "60d" default: panic("unknown expiry scenario: " + name) } @@ -83,6 +89,9 @@ func TestReservationExpiryCommand(t *testing.T) { func reservationExpiryArgs(name string) []string { s := reservationExpiryCase(name) args := []string{"--payment", "partial-upfront", "--target-coverage", strconv.FormatFloat(s.target, 'g', -1, 64), "--rebuy-window-days", "30"} + if s.recLookback != "" { + args = append(args, "--rec-lookback-period", s.recLookback) + } if s.filtered { args = append(args, "--exclude-accounts", "expiry-account-2,expiry-account-3") } diff --git a/docs/cli/purchase-safety.md b/docs/cli/purchase-safety.md index 618344412..ad0139173 100644 --- a/docs/cli/purchase-safety.md +++ b/docs/cli/purchase-safety.md @@ -119,8 +119,8 @@ The run exits 1 and writes no CSV, on dry runs and `--purchase` runs alike, in t - the credentials belong to a member account of an AWS Organization. Replacements bought centrally are not visible to a member account, so its expiry netting could buy them again. Gated rebuy runs from a standalone account, or from a management account whose member accounts have no in-scope usage; - a recommendation row belongs to another account; -- a Cost Explorer coverage query (grouped by linked account, with the same lookback window, service and, when `--regions` is set, region filters as sizing) shows usage in another account, so recommendations hidden behind your own account's rows are caught too; -- the check itself could not run: the STS identity lookup, the `organizations:DescribeOrganization` role check, the Cost Explorer scope query or the RI inventory read failed. Before this check, a failed RI inventory read on a dry run only warned and sized without the expiry netting; now it exits 1. +- a Cost Explorer coverage query (grouped by linked account, over the longer of `--coverage-lookback-days` and `--rec-lookback-period`, with the same service and, when `--regions` is set, region filters as sizing) shows usage in another account, so recommendations hidden behind your own account's rows are caught too; +- the check itself could not run: an unrecognized `--rec-lookback-period`, the STS identity lookup, the `organizations:DescribeOrganization` role check, the Cost Explorer scope query or the RI inventory read failed. Before this check, a failed RI inventory read on a dry run only warned and sized without the expiry netting; now it exits 1. A scope refusal names the other account and the service. To recover, run from a standalone account or from a management account whose member accounts have no in-scope usage, or omit `--rebuy-window-days`. A check failure says "the scope check could not run" and the cause: fix access or retry, or omit `--rebuy-window-days`. A failed role check also asks you to grant `organizations:DescribeOrganization` (read-only); see [cloud-setup.md](cloud-setup.md#aws-permissions-for---rebuy-window-days). A Cost Explorer AccessDenied also asks you to grant `ce:GetReservationCoverage` or enable Cost Explorer access for the account. A Cost Explorer failure while building the coverage map on a dry run still only warns, as described above. `--input-csv` runs never apply rebuy. From c7ca126bd57d875d9cea3ae3aa4575e35564981e Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 7 Nov 2026 05:27:39 +0100 Subject: [PATCH 10/10] fix(cli): cap --coverage-lookback-days at 365 Cost Explorer keeps about 13 months of history, so a longer window made GetReservationCoverage fail. The probe and the coverage map both use this value. Co-Authored-By: Claude Opus 5.5 --- cmd/main.go | 2 +- cmd/validators.go | 7 +++++++ cmd/validators_coverage_test.go | 9 ++++++++- docs/cli/README.md | 2 +- 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/cmd/main.go b/cmd/main.go index f07f554ae..649edbb63 100644 --- a/cmd/main.go +++ b/cmd/main.go @@ -157,7 +157,7 @@ func init() { "Number of 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. Default 30.") + "CUDly's ExistingCoverage column against the console export. Range 1-365, default 30.") rootCmd.Flags().IntVar(&toolCfg.RebuyWindowDays, "rebuy-window-days", 0, "When >0, treat existing RIs expiring within this many days as already "+ "uncovered, so --target-coverage sizes recommendations to replace them "+ diff --git a/cmd/validators.go b/cmd/validators.go index abc337d38..048e8290e 100644 --- a/cmd/validators.go +++ b/cmd/validators.go @@ -91,6 +91,10 @@ func validateIdempotencyWindow() error { return nil } +// maxCoverageLookbackDays keeps the Cost Explorer window inside its roughly +// 13 months of history. +const maxCoverageLookbackDays = 365 + // validateRecLookbackPeriod validates the --rec-lookback-period flag. func validateRecLookbackPeriod() error { valid := map[string]bool{"7d": true, "30d": true, "60d": true} @@ -117,6 +121,9 @@ func validateNumericRanges(cmd *cobra.Command) error { if toolCfg.CoverageLookbackDays < 1 { return fmt.Errorf("coverage-lookback-days must be >= 1, got: %d", toolCfg.CoverageLookbackDays) } + if toolCfg.CoverageLookbackDays > maxCoverageLookbackDays { + return fmt.Errorf("coverage-lookback-days must be <= %d (Cost Explorer keeps about 13 months of history), got: %d", maxCoverageLookbackDays, toolCfg.CoverageLookbackDays) + } // Validate max instances if toolCfg.MaxInstances < 0 { diff --git a/cmd/validators_coverage_test.go b/cmd/validators_coverage_test.go index f0d8ac3b2..db52ba67d 100644 --- a/cmd/validators_coverage_test.go +++ b/cmd/validators_coverage_test.go @@ -77,7 +77,7 @@ func TestValidateTargetCoverage(t *testing.T) { } // TestValidateCoverageLookbackDays verifies that validateNumericRanges rejects -// non-positive --coverage-lookback-days and accepts positive values. +// --coverage-lookback-days outside 1..365 and accepts values inside it. func TestValidateCoverageLookbackDays(t *testing.T) { tests := []struct { name string @@ -88,6 +88,13 @@ func TestValidateCoverageLookbackDays(t *testing.T) { {name: "default 30 is valid", days: 30, wantErr: false}, {name: "1 day is valid", days: 1, wantErr: false}, {name: "90 days is valid", days: 90, wantErr: false}, + {name: "365 days is valid", days: 365, wantErr: false}, + { + name: "366 days rejected", + days: 366, + wantErr: true, + errSubstr: "coverage-lookback-days must be <= 365", + }, { name: "zero rejected", days: 0, diff --git a/docs/cli/README.md b/docs/cli/README.md index 6a7d40d29..cff0e62df 100644 --- a/docs/cli/README.md +++ b/docs/cli/README.md @@ -34,7 +34,7 @@ All flags belong to the root command unless noted otherwise. |------|-------|---------|-------------| | `--coverage` | `-c` | `80` | Percentage (0-100) of each recommendation's instance count to purchase (`rec.Count * coverage/100`). Ignored when `--target-coverage` is also set. With `--input-csv`, omitting the flag buys each row's full `Count` (100%); any explicitly passed value, including `80`, is applied. | | `--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`. | +| `--coverage-lookback-days` | | `30` | Calendar days (1-365) 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. Does not apply to Savings Plans. See [filtering.md](filtering.md). |