From dd28b6d1a5a58ffa45341c6183feb6fda2d40a12 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 10 Oct 2026 05:27:38 +0200 Subject: [PATCH 1/2] fix(cmd): fail when extended-support exclusion data is unavailable (#2147) Region and engine lifecycle query failures used to be logged and turned into empty maps, so RDS purchases could proceed with the extended-support exclusion silently off. - queryRDSInstancesInRegions reports failed regions (API error on any page or worker panic) instead of treating them as an empty inventory. - queryMajorEngineVersionsWithClient returns an aggregated error when any engine lifecycle query fails (including the pagination cap). - fetchEngineVersionData returns an error and skips all queries when the exclusion is not needed (--include-extended-support, or no RDS in scope). - An RDS recommendation in a failed (or unknown) region aborts the run on both the API and --input-csv paths; healthy regions proceed with a warning. Closes #2147 Co-Authored-By: Claude Sonnet 5.5 --- CHANGELOG.md | 6 + cmd/main_test.go | 6 + cmd/multi_service.go | 49 +- cmd/multi_service_coverage_test.go | 12 +- cmd/multi_service_engine_versions.go | 53 ++- cmd/multi_service_engine_versions_test.go | 4 +- ...rvice_extended_support_unavailable_test.go | 445 ++++++++++++++++++ cmd/multi_service_helpers.go | 127 +++-- cmd/multi_service_max_instances_test.go | 8 +- cmd/recommendation_completeness_proxy_test.go | 11 +- cmd/reservation_expiry_command_test.go | 7 +- 11 files changed, 646 insertions(+), 82 deletions(-) create mode 100644 cmd/multi_service_extended_support_unavailable_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index e94c2e2b3..1b1ad08c0 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,12 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- Stop silently disabling the extended-support exclusion when AWS queries + fail. A failed engine lifecycle query or region listing now aborts the run + before any purchase; an RDS recommendation in a region whose instance + inventory could not be read aborts it too, while other regions proceed with + a warning. The queries are skipped entirely under `--include-extended-support` + or when no RDS recommendations are in scope (#2147). - Reject NaN and infinite values for `--coverage`, `--target-coverage`, `--min-pool-size` and `--min-savings-pct` before any API call. NaN used to pass validation and silently switch `--target-coverage` runs to diff --git a/cmd/main_test.go b/cmd/main_test.go index bb92fb449..87dc3ada0 100644 --- a/cmd/main_test.go +++ b/cmd/main_test.go @@ -1,6 +1,7 @@ package main import ( + "context" "fmt" "os" "path/filepath" @@ -25,6 +26,11 @@ import ( // AuditLog, which takes precedence within that test. func TestMain(m *testing.M) { toolCfg.AuditLog = filepath.Join(os.TempDir(), fmt.Sprintf("cudly-test-audit-%d.jsonl", os.Getpid())) + // Keep tests off AWS: the real fetcher is exercised explicitly against a + // local stub (useRealEngineVersionFetcher). + engineVersionFetcher = func(context.Context, Config, bool) (engineVersionData, error) { + return engineVersionData{}, nil + } code := m.Run() _ = os.Remove(toolCfg.AuditLog) os.Exit(code) diff --git a/cmd/multi_service.go b/cmd/multi_service.go index 8a8091308..dc8881a71 100644 --- a/cmd/multi_service.go +++ b/cmd/multi_service.go @@ -153,7 +153,8 @@ func runToolMultiService(ctx context.Context, cfg Config) { if adapter, ok := recClient.(*awsprovider.RecommendationsClientAdapter); ok && cfg.RecLookbackPeriod != "" { adapter.SetRecLookbackPeriod(cfg.RecLookbackPeriod) } - engineData := fetchEngineVersionData(ctx, cfg) + engineData, err := engineVersionFetcher(ctx, cfg, extendedSupportCheckNeeded(cfg, servicesToProcess)) + exitOnExclusionError(err) // Fetch existing-RI coverage so --target-coverage can subtract what // the user already owns. On a dry run, a failure logs a warning and @@ -169,7 +170,8 @@ func runToolMultiService(ctx context.Context, cfg Config) { // Phase 1: collect all recommendations without purchasing. AppLogger.Printf("\nšŸ“„ Fetching recommendations from all services...\n") - allRecs, drops := fetchAllRecs(ctx, awsCfg, recClient, accountCache, servicesToProcess, engineData, cfg, coverageMap) + allRecs, drops, err := fetchAllRecs(ctx, awsCfg, recClient, accountCache, servicesToProcess, engineData, cfg, coverageMap) + exitOnExclusionError(err) // Phase 2: score, enforce the run-wide instance cap, and display. scoredResult := scoreLimitAndDisplay(allRecs, cfg, drops) @@ -771,32 +773,18 @@ func checkDuplicatesForCSVRegion(ctx context.Context, recs []common.Recommendati // It returns an error when the cap cannot be enforced honestly; see // requireRankingSignal. func filterAndAdjustRecommendations(recs []common.Recommendation, csvModeCoverage float64, cfg Config) ([]common.Recommendation, error) { - // Query running instances for engine version validation - log.Printf("šŸ” Querying running RDS instances across all regions to validate engine versions...") - instanceVersions, err := queryRunningInstanceEngineVersions(context.Background(), cfg) + engineData, err := engineVersionFetcher(context.Background(), cfg, !cfg.IncludeExtendedSupport && hasDatabaseRecs(recs)) if err != nil { - log.Printf("āš ļø Warning: Failed to query running instances for engine version validation: %v", err) - log.Printf(" Continuing without engine version filtering") - instanceVersions = make(map[string][]InstanceEngineVersion) - } else { - log.Printf("āœ… Found %d instance types with version information across all regions", len(instanceVersions)) + return nil, err } - - // Query major engine versions for extended support detection - log.Printf("šŸ” Querying AWS RDS major engine versions for extended support information...") - versionInfo, err := queryMajorEngineVersions(context.Background(), cfg) - if err != nil { - log.Printf("āš ļø Warning: Failed to query major engine versions: %v", err) - log.Printf(" Continuing without extended support detection") - versionInfo = make(map[string]MajorEngineVersionInfo) - } else { - log.Printf("āœ… Found support information for %d major engine versions", len(versionInfo)) + if err := requireRegionInventory(recs, cfg, engineData); err != nil { + return nil, err } // Apply filters (empty currentRegion since we're processing from CSV, not iterating regions). // Drop tracking is skipped on the CSV path (nil drops). originalCount := len(recs) - recs = applyFilters(recs, &cfg, instanceVersions, versionInfo, "", nil) + recs = applyFilters(recs, &cfg, engineData.instanceVersions, engineData.versionInfo, "", nil) if len(recs) < originalCount { AppLogger.Printf("šŸ” After filters: %d recs (filtered out %d)\n", len(recs), originalCount-len(recs)) } @@ -907,3 +895,22 @@ func processPurchaseLoop(ctx context.Context, recs []common.Recommendation, regi return results } + +// hasDatabaseRecs reports whether any recommendation is one the extended-support +// exclusion applies to (the same *common.DatabaseDetails test the adjuster uses). +func hasDatabaseRecs(recs []common.Recommendation) bool { + for i := range recs { + if d, ok := recs[i].Details.(*common.DatabaseDetails); ok && d != nil { + return true + } + } + return false +} + +// exitOnExclusionError aborts the run before any purchase when the +// extended-support exclusion cannot be applied (issue #2147). +func exitOnExclusionError(err error) { + if err != nil { + log.Fatalf("Cannot apply extended-support exclusion: %v", err) + } +} diff --git a/cmd/multi_service_coverage_test.go b/cmd/multi_service_coverage_test.go index 30cea2dfa..eae28d5de 100644 --- a/cmd/multi_service_coverage_test.go +++ b/cmd/multi_service_coverage_test.go @@ -76,9 +76,10 @@ func TestQueryMajorEngineVersionsWithClient_Success(t *testing.T) { "lifecycle data must be carried through from the API response") } -// TestQueryMajorEngineVersionsWithClient_EngineErrorContinues asserts the -// warn-and-continue contract: one engine failing must not drop the results -// of the others, and the overall call still succeeds. +// TestQueryMajorEngineVersionsWithClient_EngineErrorContinues asserts that one +// engine failing does not stop the remaining engine queries, but the overall +// call fails: missing lifecycle data must not silently disable the +// extended-support exclusion (#2147). func TestQueryMajorEngineVersionsWithClient_EngineErrorContinues(t *testing.T) { stub := &engineKeyedRDSMajorVersionsStub{ versionsByEngine: map[string][]rdstypes.DBMajorEngineVersion{ @@ -90,14 +91,13 @@ func TestQueryMajorEngineVersionsWithClient_EngineErrorContinues(t *testing.T) { } result, err := queryMajorEngineVersionsWithClient(context.Background(), stub) - require.NoError(t, err, "per-engine API failures are warn-and-continue") + require.Error(t, err, "an unavailable engine lifecycle query must fail the call") + assert.Nil(t, result) assert.ElementsMatch(t, []string{"mysql", "postgres", "aurora-mysql", "aurora-postgresql"}, stub.enginesQueried, "a failing engine must not stop the remaining engine queries") - require.Len(t, result, 1) - assert.Equal(t, "15", result["postgres:15"].MajorEngineVersion) } // TestQueryMajorEngineVersions_ProfileSelection asserts validation-profile diff --git a/cmd/multi_service_engine_versions.go b/cmd/multi_service_engine_versions.go index 415407e29..46d497bfb 100644 --- a/cmd/multi_service_engine_versions.go +++ b/cmd/multi_service_engine_versions.go @@ -2,6 +2,7 @@ package main import ( "context" + "errors" "fmt" "log" "runtime" @@ -41,15 +42,20 @@ type MajorEngineVersionInfo struct { } // queryRunningInstanceEngineVersions queries all running RDS instances and returns their engine versions. -func queryRunningInstanceEngineVersions(ctx context.Context, cfg Config) (map[string][]InstanceEngineVersion, error) { +// +// The returned failedRegions maps each region whose inventory could not be read +// completely to the cause; the instance map still holds whatever was read. A +// non-nil error means the inventory is unusable as a whole (config, region +// listing or canceled context). +func queryRunningInstanceEngineVersions(ctx context.Context, cfg Config) (instances map[string][]InstanceEngineVersion, failedRegions map[string]error, err error) { awsCfg, err := loadValidationAWSConfig(ctx, cfg) if err != nil { - return nil, err + return nil, nil, err } regions, err := getAWSRegions(ctx, awsCfg) if err != nil { - return nil, err + return nil, nil, err } return queryRDSInstancesInRegions(ctx, awsCfg, regions) @@ -100,8 +106,12 @@ type RDSMajorVersionsClient interface { } // queryRDSInstancesInRegions queries RDS instances in all regions concurrently. -func queryRDSInstancesInRegions(ctx context.Context, awsCfg aws.Config, regions []ec2types.Region) (map[string][]InstanceEngineVersion, error) { - instanceVersions := make(map[string][]InstanceEngineVersion) +// A region that fails (API error on any page, or a worker panic) is reported in +// failedRegions rather than silently treated as an empty inventory. A canceled +// ctx is returned as the error. +func queryRDSInstancesInRegions(ctx context.Context, awsCfg aws.Config, regions []ec2types.Region) (instanceVersions map[string][]InstanceEngineVersion, failedRegions map[string]error, err error) { + instanceVersions = make(map[string][]InstanceEngineVersion) + failedRegions = make(map[string]error) var mu sync.Mutex var wg sync.WaitGroup @@ -118,18 +128,29 @@ func queryRDSInstancesInRegions(ctx context.Context, awsCfg aws.Config, regions buf := make([]byte, 4096) n := runtime.Stack(buf, false) log.Printf("ERROR: panic in region worker (region=%s): %v\n%s", regionName, r, buf[:n]) + mu.Lock() + failedRegions[regionName] = fmt.Errorf("panic in region worker: %v", r) + mu.Unlock() } }() - queryRDSInstancesInRegion(ctx, awsCfg, regionName, instanceVersions, &mu) + if regionErr := queryRDSInstancesInRegion(ctx, awsCfg, regionName, instanceVersions, &mu); regionErr != nil { + mu.Lock() + failedRegions[regionName] = regionErr + mu.Unlock() + } }(aws.ToString(region.RegionName)) } wg.Wait() - return instanceVersions, nil + if err = ctx.Err(); err != nil { + return nil, nil, err + } + return instanceVersions, failedRegions, nil } -// queryRDSInstancesInRegion queries RDS instances in a single region. -func queryRDSInstancesInRegion(ctx context.Context, awsCfg aws.Config, regionName string, instanceVersions map[string][]InstanceEngineVersion, mu *sync.Mutex) { +// queryRDSInstancesInRegion queries RDS instances in a single region and +// returns the first page error, leaving whatever earlier pages merged in place. +func queryRDSInstancesInRegion(ctx context.Context, awsCfg aws.Config, regionName string, instanceVersions map[string][]InstanceEngineVersion, mu *sync.Mutex) error { regionCfg := awsCfg.Copy() regionCfg.Region = regionName rdsClient := awsrds.NewFromConfig(regionCfg) @@ -138,8 +159,7 @@ func queryRDSInstancesInRegion(ctx context.Context, awsCfg aws.Config, regionNam for { localVersions, nextMarker, err := queryRDSInstancesPage(ctx, rdsClient, marker, regionName) if err != nil { - log.Printf("āš ļø Warning: Failed to describe RDS instances in %s: %v", regionName, err) - break + return fmt.Errorf("failed to describe RDS instances in %s: %w", regionName, err) } // Merge into shared map with mutex protection @@ -150,7 +170,7 @@ func queryRDSInstancesInRegion(ctx context.Context, awsCfg aws.Config, regionNam mu.Unlock() if nextMarker == nil { - break + return nil } marker = nextMarker } @@ -216,6 +236,7 @@ func queryMajorEngineVersionsWithClient(ctx context.Context, rdsClient RDSMajorV // Query all engine types we care about engines := []string{"mysql", "postgres", "aurora-mysql", "aurora-postgresql"} + var engineErrs []error for _, engine := range engines { if err := ctx.Err(); err != nil { @@ -227,13 +248,19 @@ func queryMajorEngineVersionsWithClient(ctx context.Context, rdsClient RDSMajorV if ctxErr := ctx.Err(); ctxErr != nil { return nil, ctxErr } - log.Printf("Warning: Failed to describe major engine versions for %s: %v", engine, err) + // Lifecycle data for an engine is required to tell whether its + // instances are on extended support; keep trying the remaining + // engines so the error reports every unavailable one (issue #2147). + engineErrs = append(engineErrs, fmt.Errorf("major engine versions for %s: %w", engine, err)) } } if err := ctx.Err(); err != nil { return nil, err } + if len(engineErrs) > 0 { + return nil, errors.Join(engineErrs...) + } return versionInfo, nil } diff --git a/cmd/multi_service_engine_versions_test.go b/cmd/multi_service_engine_versions_test.go index e431e9f6b..fda7e3782 100644 --- a/cmd/multi_service_engine_versions_test.go +++ b/cmd/multi_service_engine_versions_test.go @@ -818,7 +818,7 @@ func TestQueryMajorEngineVersions_ErrorHandling(t *testing.T) { // We expect an error in test environment (no real AWS creds) // The important thing is that the function doesn't panic if err != nil { - assert.Contains(t, err.Error(), "failed to load AWS config") + assert.Regexp(t, "failed to load AWS config|major engine versions for", err.Error()) } }) } @@ -863,7 +863,7 @@ func TestQueryRunningInstanceEngineVersions_ErrorHandling(t *testing.T) { t.Run(tt.name, func(t *testing.T) { // This will likely fail in test environment without real AWS credentials // but it validates the function signature and basic logic paths - _, err := queryRunningInstanceEngineVersions(ctx, tt.cfg) + _, _, err := queryRunningInstanceEngineVersions(ctx, tt.cfg) // We expect an error in test environment (no real AWS creds) // The important thing is that the function doesn't panic if err != nil { diff --git a/cmd/multi_service_extended_support_unavailable_test.go b/cmd/multi_service_extended_support_unavailable_test.go new file mode 100644 index 000000000..9d6178533 --- /dev/null +++ b/cmd/multi_service_extended_support_unavailable_test.go @@ -0,0 +1,445 @@ +package main + +import ( + "context" + "errors" + "fmt" + "io" + "net/http" + "net/http/httptest" + "net/url" + "os" + "path/filepath" + "strings" + "sync" + "testing" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/aws/aws-sdk-go-v2/aws" + awsrds "github.com/aws/aws-sdk-go-v2/service/rds" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" + "github.com/stretchr/testify/require" +) + +// Regression tests for #2147: unavailable RDS inventory or lifecycle data must +// not silently disable the extended-support exclusion. Nothing here reaches +// AWS: every request is answered by an in-process stub. + +// rdsAPIStub answers the three query-protocol calls the exclusion data needs. +// Behavior is selected by the fields; every request is recorded. +type rdsAPIStub struct { + mu sync.Mutex + actions []string + failRegions map[string]bool // DescribeDBInstances -> AccessDenied + failPage2 map[string]bool // first page returns a marker, second page fails + panicRegions map[string]bool // transport panics (direct transport use only) + failEngines map[string]bool // DescribeDBMajorEngineVersions -> AccessDenied + failRegionsEC bool // DescribeRegions -> AccessDenied + instances map[string]string + regionList []string +} + +func (s *rdsAPIStub) count(prefix string) int { + s.mu.Lock() + defer s.mu.Unlock() + n := 0 + for _, a := range s.actions { + if strings.HasPrefix(a, prefix) { + n++ + } + } + return n +} + +func (s *rdsAPIStub) total() int { + s.mu.Lock() + defer s.mu.Unlock() + return len(s.actions) +} + +const rdsStubDenied = `SenderAccessDenieddenied by stubr` + +func (s *rdsAPIStub) respond(authz, body string) (int, string) { + form, _ := url.ParseQuery(body) + action := form.Get("Action") + region := "" + if parts := strings.Split(authz, "/"); len(parts) > 2 { + region = parts[2] + } + s.mu.Lock() + s.actions = append(s.actions, action) + s.mu.Unlock() + + switch action { + case "DescribeRegions": + if s.failRegionsEC { + return http.StatusForbidden, rdsStubDenied + } + var items strings.Builder + for _, r := range s.regionList { + fmt.Fprintf(&items, "%s", r) + } + return http.StatusOK, `` + items.String() + `` + case "DescribeDBInstances": + if s.panicRegions[region] { + panic("stub panic in " + region) + } + if s.failRegions[region] || (s.failPage2[region] && form.Get("Marker") != "") { + return http.StatusForbidden, rdsStubDenied + } + marker := "" + if s.failPage2[region] { + marker = "page2" + } + inst := "" + if class := s.instances[region]; class != "" { + inst = fmt.Sprintf("%smysql5.7.44", class) + } + return http.StatusOK, `` + inst + `` + marker + `` + case "DescribeDBMajorEngineVersions": + if s.failEngines[form.Get("Engine")] { + return http.StatusForbidden, rdsStubDenied + } + return http.StatusOK, `` + } + return http.StatusBadRequest, rdsStubDenied +} + +// RoundTrip lets tests hand the stub to an aws.Config directly. +func (s *rdsAPIStub) RoundTrip(r *http.Request) (*http.Response, error) { + b, _ := io.ReadAll(r.Body) + status, body := s.respond(r.Header.Get("Authorization"), string(b)) + return &http.Response{ + StatusCode: status, + Header: http.Header{"Content-Type": []string{"text/xml"}}, + Body: io.NopCloser(strings.NewReader(body)), + Request: r, + }, nil +} + +func (s *rdsAPIStub) awsConfig() aws.Config { + return aws.Config{ + Region: "us-east-1", + HTTPClient: &http.Client{Transport: s}, + Credentials: aws.CredentialsProviderFunc(func(context.Context) (aws.Credentials, error) { + return aws.Credentials{AccessKeyID: "AKIDEXAMPLE", SecretAccessKey: "secret"}, nil + }), + Retryer: func() aws.Retryer { return aws.NopRetryer{} }, + } +} + +// serveEnv starts the stub as a local HTTP endpoint and points the default AWS +// config chain at it, isolated from any developer credentials or config. +func (s *rdsAPIStub) serveEnv(t *testing.T) { + t.Helper() + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + b, _ := io.ReadAll(r.Body) + status, body := s.respond(r.Header.Get("Authorization"), string(b)) + w.Header().Set("Content-Type", "text/xml") + w.WriteHeader(status) + _, _ = io.WriteString(w, body) + })) + t.Cleanup(srv.Close) + + empty := filepath.Join(t.TempDir(), "empty") + require.NoError(t, os.WriteFile(empty, nil, 0o600)) + t.Setenv("AWS_CONFIG_FILE", empty) + t.Setenv("AWS_SHARED_CREDENTIALS_FILE", empty) + t.Setenv("AWS_EC2_METADATA_DISABLED", "true") + t.Setenv("AWS_PROFILE", "") + t.Setenv("AWS_IGNORE_CONFIGURED_ENDPOINT_URLS", "") + t.Setenv("AWS_ACCESS_KEY_ID", "AKIDEXAMPLE") + t.Setenv("AWS_SECRET_ACCESS_KEY", "secret") + t.Setenv("AWS_SESSION_TOKEN", "") + t.Setenv("AWS_MAX_ATTEMPTS", "1") + t.Setenv("AWS_ENDPOINT_URL", srv.URL) +} + +// requireStubReached fails the test when the seam fell through to somewhere +// other than the stub, and when anything tried to purchase. +func (s *rdsAPIStub) requireStubReachedNoPurchase(t *testing.T) { + t.Helper() + assert.Positive(t, s.total(), "stub must have been reached") + assert.Zero(t, s.count("Purchase"), "no purchase call may be made") +} + +func newRDSStub(regions ...string) *rdsAPIStub { + return &rdsAPIStub{ + regionList: regions, + failRegions: map[string]bool{}, + failPage2: map[string]bool{}, + panicRegions: map[string]bool{}, + failEngines: map[string]bool{}, + instances: map[string]string{}, + } +} + +func queryRegions(t *testing.T, stub *rdsAPIStub, ctx context.Context) (map[string][]InstanceEngineVersion, map[string]error, error) { + t.Helper() + regions, err := getAWSRegions(ctx, stub.awsConfig()) + require.NoError(t, err) + return queryRDSInstancesInRegions(ctx, stub.awsConfig(), regions) +} + +func TestQueryRDSInstancesInRegions_AllRegionsFail(t *testing.T) { + stub := newRDSStub("us-east-1", "eu-west-1") + stub.failRegions["us-east-1"], stub.failRegions["eu-west-1"] = true, true + + inst, failed, err := queryRegions(t, stub, context.Background()) + + require.NoError(t, err) + assert.Empty(t, inst) + assert.Len(t, failed, 2, "failed regions must be reported, not treated as an empty inventory") + assert.ErrorContains(t, failed["us-east-1"], "us-east-1") +} + +func TestQueryRDSInstancesInRegions_FailureMidPaginationMarksRegionFailed(t *testing.T) { + stub := newRDSStub("us-east-1", "eu-west-1") + stub.failPage2["us-east-1"] = true + stub.instances["us-east-1"], stub.instances["eu-west-1"] = "db.r5.large", "db.t3.small" + + inst, failed, err := queryRegions(t, stub, context.Background()) + + require.NoError(t, err) + assert.Len(t, failed, 1) + assert.Contains(t, failed, "us-east-1") + assert.Contains(t, inst, "db.t3.small", "healthy region data is kept") +} + +func TestQueryRDSInstancesInRegions_WorkerPanicMarksRegionFailed(t *testing.T) { + stub := newRDSStub("us-east-1", "eu-west-1") + stub.panicRegions["us-east-1"] = true + stub.instances["eu-west-1"] = "db.t3.small" + + _, failed, err := queryRegions(t, stub, context.Background()) + + require.NoError(t, err) + assert.Contains(t, failed, "us-east-1") + assert.NotContains(t, failed, "eu-west-1") +} + +func TestQueryRDSInstancesInRegions_GenuineEmptyInventoryIsNotAFailure(t *testing.T) { + stub := newRDSStub("us-east-1", "eu-west-1") + + inst, failed, err := queryRegions(t, stub, context.Background()) + + require.NoError(t, err) + assert.Empty(t, inst) + assert.Empty(t, failed) +} + +func TestQueryRDSInstancesInRegions_CancelledContextIsTerminal(t *testing.T) { + stub := newRDSStub("us-east-1") + ctx, cancel := context.WithCancel(context.Background()) + regions, err := getAWSRegions(ctx, stub.awsConfig()) + require.NoError(t, err) + cancel() + + _, _, err = queryRDSInstancesInRegions(ctx, stub.awsConfig(), regions) + + require.Error(t, err) + assert.True(t, errors.Is(err, context.Canceled)) +} + +func TestQueryMajorEngineVersionsWithClient_EngineFailuresAreAggregated(t *testing.T) { + denied := errors.New("AccessDenied") + t.Run("all engines fail", func(t *testing.T) { + stub := &engineKeyedRDSMajorVersionsStub{errByEngine: map[string]error{ + "mysql": denied, "postgres": denied, "aurora-mysql": denied, "aurora-postgresql": denied, + }} + got, err := queryMajorEngineVersionsWithClient(context.Background(), stub) + require.Error(t, err) + assert.Nil(t, got) + for _, e := range []string{"mysql", "postgres", "aurora-mysql", "aurora-postgresql"} { + assert.ErrorContains(t, err, "major engine versions for "+e+":") + } + }) + t.Run("one engine fails", func(t *testing.T) { + stub := &engineKeyedRDSMajorVersionsStub{errByEngine: map[string]error{"postgres": denied}} + _, err := queryMajorEngineVersionsWithClient(context.Background(), stub) + require.Error(t, err) + assert.ErrorContains(t, err, "postgres") + assert.ErrorIs(t, err, denied) + }) + t.Run("genuinely empty lists are not an error", func(t *testing.T) { + got, err := queryMajorEngineVersionsWithClient(context.Background(), &engineKeyedRDSMajorVersionsStub{}) + require.NoError(t, err) + assert.Empty(t, got) + }) + t.Run("canceled context stays context.Canceled", func(t *testing.T) { + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _, err := queryMajorEngineVersionsWithClient(ctx, &engineKeyedRDSMajorVersionsStub{errByEngine: map[string]error{"mysql": denied}}) + assert.True(t, errors.Is(err, context.Canceled)) + }) +} + +func TestQueryMajorEngineVersionsWithClient_PaginationCapIsAnError(t *testing.T) { + _, err := queryMajorEngineVersionsWithClient(context.Background(), &alwaysMarkerRDSStub{}) + require.Error(t, err) + assert.ErrorContains(t, err, "pagination cap") +} + +// alwaysMarkerRDSStub never stops paginating. +type alwaysMarkerRDSStub struct{} + +func (alwaysMarkerRDSStub) DescribeDBMajorEngineVersions(context.Context, *awsrds.DescribeDBMajorEngineVersionsInput, ...func(*awsrds.Options)) (*awsrds.DescribeDBMajorEngineVersionsOutput, error) { + return &awsrds.DescribeDBMajorEngineVersionsOutput{Marker: aws.String("more")}, nil +} + +func TestFetchEngineVersionData_ExclusionDataUnavailable(t *testing.T) { + ctx := context.Background() + + t.Run("exclusion not needed makes no request", func(t *testing.T) { + stub := newRDSStub("us-east-1") + stub.failRegionsEC = true + stub.serveEnv(t) + data, err := fetchEngineVersionData(ctx, Config{IncludeExtendedSupport: true}, false) + require.NoError(t, err) + assert.Empty(t, data.instanceVersions) + assert.Zero(t, stub.total()) + }) + t.Run("region listing failure is fatal", func(t *testing.T) { + stub := newRDSStub("us-east-1") + stub.failRegionsEC = true + stub.serveEnv(t) + _, err := fetchEngineVersionData(ctx, Config{}, true) + require.Error(t, err) + assert.ErrorContains(t, err, "--include-extended-support") + stub.requireStubReachedNoPurchase(t) + }) + t.Run("engine lifecycle failure is fatal", func(t *testing.T) { + stub := newRDSStub("us-east-1") + stub.failEngines["postgres"] = true + stub.serveEnv(t) + _, err := fetchEngineVersionData(ctx, Config{}, true) + require.Error(t, err) + assert.ErrorContains(t, err, "postgres") + assert.ErrorContains(t, err, "--include-extended-support") + stub.requireStubReachedNoPurchase(t) + }) + t.Run("one failed region is reported, not fatal", func(t *testing.T) { + stub := newRDSStub("us-east-1", "eu-west-1") + stub.failRegions["eu-west-1"] = true + stub.serveEnv(t) + data, err := fetchEngineVersionData(ctx, Config{}, true) + require.NoError(t, err) + assert.Contains(t, data.failedRegions, "eu-west-1") + assert.NotContains(t, data.failedRegions, "us-east-1") + stub.requireStubReachedNoPurchase(t) + }) + t.Run("healthy and genuinely empty", func(t *testing.T) { + stub := newRDSStub("us-east-1") + stub.serveEnv(t) + data, err := fetchEngineVersionData(ctx, Config{}, true) + require.NoError(t, err) + assert.Empty(t, data.failedRegions) + stub.requireStubReachedNoPurchase(t) + }) +} + +func TestFetchEngineVersionData_CancelledContextKeepsContextError(t *testing.T) { + stub := newRDSStub("us-east-1") + stub.serveEnv(t) + ctx, cancel := context.WithCancel(context.Background()) + cancel() + _, err := fetchEngineVersionData(ctx, Config{}, true) + require.Error(t, err) + assert.True(t, errors.Is(err, context.Canceled), "cancel must stay detectable through the wrapping") +} + +func rdsRec(region string) common.Recommendation { + return common.Recommendation{ + Service: common.ServiceRDS, ResourceType: "db.t3.small", Count: 2, Region: region, + Details: &common.DatabaseDetails{Engine: "mysql"}, + } +} + +func TestRequireRegionInventory(t *testing.T) { + data := engineVersionData{failedRegions: map[string]error{"eu-west-1": errors.New("denied")}} + + assert.ErrorContains(t, requireRegionInventory([]common.Recommendation{rdsRec("eu-west-1")}, Config{}, data), "eu-west-1") + assert.ErrorContains(t, requireRegionInventory([]common.Recommendation{rdsRec("")}, Config{}, data), "eu-west-1", "empty region cannot be proven healthy") + assert.NoError(t, requireRegionInventory([]common.Recommendation{rdsRec("us-east-1")}, Config{}, data)) + assert.NoError(t, requireRegionInventory([]common.Recommendation{{Service: common.ServiceEC2, Region: "eu-west-1"}}, Config{}, data), "non-RDS recs are unaffected") + assert.NoError(t, requireRegionInventory([]common.Recommendation{rdsRec("eu-west-1")}, Config{IncludeExtendedSupport: true}, data), "opt-in skips the check") +} + +// useRealEngineVersionFetcher restores the AWS-backed fetcher (TestMain stubs it). +func useRealEngineVersionFetcher(t *testing.T) { + t.Helper() + orig := engineVersionFetcher + engineVersionFetcher = fetchEngineVersionData + t.Cleanup(func() { engineVersionFetcher = orig }) +} + +func TestFilterAndAdjustRecommendations_ExtendedSupportDataUnavailable(t *testing.T) { + useRealEngineVersionFetcher(t) + + t.Run("engine lifecycle failure aborts before any rec is returned", func(t *testing.T) { + stub := newRDSStub("us-east-1") + stub.failEngines["mysql"] = true + stub.serveEnv(t) + got, err := filterAndAdjustRecommendations([]common.Recommendation{rdsRec("us-east-1")}, 100, Config{}) + require.Error(t, err) + assert.Empty(t, got) + stub.requireStubReachedNoPurchase(t) + }) + t.Run("rec in a failed region aborts", func(t *testing.T) { + stub := newRDSStub("us-east-1", "eu-west-1") + stub.failRegions["eu-west-1"] = true + stub.serveEnv(t) + got, err := filterAndAdjustRecommendations([]common.Recommendation{rdsRec("eu-west-1")}, 100, Config{}) + require.Error(t, err) + assert.ErrorContains(t, err, "eu-west-1") + assert.Empty(t, got) + }) + t.Run("rec in a healthy region proceeds despite another failed region", func(t *testing.T) { + stub := newRDSStub("us-east-1", "eu-west-1") + stub.failRegions["eu-west-1"] = true + stub.serveEnv(t) + got, err := filterAndAdjustRecommendations([]common.Recommendation{rdsRec("us-east-1")}, 100, Config{}) + require.NoError(t, err) + assert.Len(t, got, 1) + }) + t.Run("genuinely empty inventory proceeds", func(t *testing.T) { + stub := newRDSStub("us-east-1") + stub.serveEnv(t) + got, err := filterAndAdjustRecommendations([]common.Recommendation{rdsRec("us-east-1")}, 100, Config{}) + require.NoError(t, err) + assert.Len(t, got, 1) + }) + t.Run("IncludeExtendedSupport opt-in makes no query and proceeds", func(t *testing.T) { + stub := newRDSStub("us-east-1") + stub.failRegionsEC = true + stub.serveEnv(t) + got, err := filterAndAdjustRecommendations([]common.Recommendation{rdsRec("us-east-1")}, 100, Config{IncludeExtendedSupport: true}) + require.NoError(t, err) + assert.Len(t, got, 1) + assert.Zero(t, stub.total()) + }) +} + +func TestFetchAllRecs_RDSRecInFailedRegionAborts(t *testing.T) { + ctx := context.Background() + awsCfg := aws.Config{Region: "us-east-1"} + saved := saveGlobalVars() + defer saved.restore() + toolCfg.Coverage = 100 + toolCfg.Regions = []string{"us-east-1", "eu-west-1"} + + client := &MockRecommendationsClient{} + client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { + return p != nil && p.Region == "us-east-1" + })).Return([]common.Recommendation{rdsRec("us-east-1")}, nil) + client.On("GetRecommendations", mock.Anything, mock.MatchedBy(func(p *common.RecommendationParams) bool { + return p != nil && p.Region == "eu-west-1" + })).Return([]common.Recommendation{rdsRec("eu-west-1")}, nil) + + data := engineVersionData{failedRegions: map[string]error{"eu-west-1": errors.New("denied")}} + _, _, err := fetchAllRecs(ctx, awsCfg, client, NewAccountAliasCache(awsCfg), []common.ServiceType{common.ServiceRDS}, data, toolCfg, nil) + + require.Error(t, err) + assert.ErrorContains(t, err, "eu-west-1") +} diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index d490afc2b..0a8ee8d76 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -5,6 +5,8 @@ import ( "errors" "fmt" "log" + "maps" + "slices" "sort" "strings" "time" @@ -296,53 +298,118 @@ func handleRegionDiscoveryError(ctx context.Context, recClient provider.Recommen } // engineVersionData holds the results of engine version queries. +// failedRegions lists regions whose RDS inventory could not be read +// completely, mapped to the cause; instanceVersions then holds only what was +// read, so recommendations in those regions cannot be checked (issue #2147). type engineVersionData struct { instanceVersions map[string][]InstanceEngineVersion versionInfo map[string]MajorEngineVersionInfo + failedRegions map[string]error } -// fetchEngineVersionData queries running instances and major engine versions for validation. -func fetchEngineVersionData(ctx context.Context, cfg Config) engineVersionData { +// engineVersionFetcher is the seam tests replace so they never reach AWS. +var engineVersionFetcher = fetchEngineVersionData + +// extendedSupportCheckNeeded reports whether extended-support exclusion will +// run for the given services: it is on by default and applies to RDS only. +func extendedSupportCheckNeeded(cfg Config, services []common.ServiceType) bool { + if cfg.IncludeExtendedSupport { + return false + } + return slices.Contains(services, common.ServiceRDS) +} + +// fetchEngineVersionData queries running instances and major engine versions +// for extended-support exclusion. When needed is false nothing is queried. +// It fails when exclusion data is unavailable as a whole (region listing or +// any engine lifecycle query); per-region instance failures are returned in +// failedRegions and enforced per recommendation by requireRegionInventory. +func fetchEngineVersionData(ctx context.Context, cfg Config, needed bool) (engineVersionData, error) { data := engineVersionData{ instanceVersions: make(map[string][]InstanceEngineVersion), versionInfo: make(map[string]MajorEngineVersionInfo), + failedRegions: make(map[string]error), + } + if !needed { + return data, nil } - // Query running instances for engine version validation - data.instanceVersions = queryInstanceVersions(ctx, cfg) - - // Query major engine versions for extended support detection - data.versionInfo = queryMajorVersions(ctx, cfg) - - return data + var err error + data.instanceVersions, data.failedRegions, err = queryInstanceVersions(ctx, cfg) + if err != nil { + return engineVersionData{}, fmt.Errorf("extended-support exclusion data unavailable: %w (re-run, or pass --include-extended-support to skip the check)", err) + } + data.versionInfo, err = queryMajorVersions(ctx, cfg) + if err != nil { + return engineVersionData{}, fmt.Errorf("extended-support exclusion data unavailable: %w (re-run, or pass --include-extended-support to skip the check)", err) + } + return data, nil } // queryInstanceVersions queries running instances for engine version validation. -func queryInstanceVersions(ctx context.Context, cfg Config) map[string][]InstanceEngineVersion { +func queryInstanceVersions(ctx context.Context, cfg Config) (instances map[string][]InstanceEngineVersion, failedRegions map[string]error, err error) { AppLogger.Printf("šŸ” Querying running RDS instances across all regions to validate engine versions...\n") - instanceVersions, err := queryRunningInstanceEngineVersions(ctx, cfg) + instanceVersions, failedRegions, err := queryRunningInstanceEngineVersions(ctx, cfg) if err != nil { - AppLogger.Printf("āš ļø Warning: Failed to query running instances for engine version validation: %v\n", err) - AppLogger.Printf(" Continuing without engine version filtering\n") - return make(map[string][]InstanceEngineVersion) + return nil, nil, err } AppLogger.Printf("āœ… Found %d instance types with version information across all regions\n", len(instanceVersions)) - return instanceVersions + if len(failedRegions) > 0 { + AppLogger.Printf("āš ļø Warning: RDS inventory unavailable in %s; RDS recommendations in those regions will abort the run, other regions proceed\n", describeFailedRegions(failedRegions)) + } + return instanceVersions, failedRegions, nil } // queryMajorVersions queries major engine versions for extended support detection. -func queryMajorVersions(ctx context.Context, cfg Config) map[string]MajorEngineVersionInfo { +func queryMajorVersions(ctx context.Context, cfg Config) (map[string]MajorEngineVersionInfo, error) { AppLogger.Printf("šŸ” Querying AWS RDS major engine versions for extended support information...\n") versionInfo, err := queryMajorEngineVersions(ctx, cfg) if err != nil { - AppLogger.Printf("āš ļø Warning: Failed to query major engine versions: %v\n", err) - AppLogger.Printf(" Continuing without extended support detection\n") - return make(map[string]MajorEngineVersionInfo) + return nil, err } AppLogger.Printf("āœ… Found support information for %d major engine versions\n", len(versionInfo)) - return versionInfo + return versionInfo, nil +} + +// describeFailedRegions renders failed regions and causes in a stable order. +func describeFailedRegions(failed map[string]error) string { + names := make([]string, 0, len(failed)) + for region := range failed { + names = append(names, region) + } + sort.Strings(names) + parts := make([]string, 0, len(names)) + for _, region := range names { + parts = append(parts, fmt.Sprintf("%s (%v)", region, failed[region])) + } + return strings.Join(parts, "; ") +} + +// requireRegionInventory fails when an RDS recommendation sits in a region +// (or has no region) whose instance inventory could not be read, because the +// extended-support exclusion cannot be applied to it. Recommendations in +// healthy regions are unaffected. +func requireRegionInventory(recs []common.Recommendation, cfg Config, data engineVersionData) error { + if cfg.IncludeExtendedSupport || len(data.failedRegions) == 0 { + return nil + } + affected := make(map[string]error) + for i := range recs { + if d, ok := recs[i].Details.(*common.DatabaseDetails); !ok || d == nil { + continue + } + if recs[i].Region == "" { + maps.Copy(affected, data.failedRegions) + } else if cause, failed := data.failedRegions[recs[i].Region]; failed { + affected[recs[i].Region] = cause + } + } + if len(affected) == 0 { + return nil + } + return fmt.Errorf("extended-support exclusion cannot be applied: RDS inventory unavailable in %s (re-run, or pass --include-extended-support to skip the check)", describeFailedRegions(affected)) } // regionRecommendations holds the processed recommendations for a single region. @@ -657,21 +724,24 @@ func fetchAndFilterRegionRecs( cfg Config, coverageMap recommendations.PoolCoverageMap, drops *common.DropSummary, -) []common.Recommendation { +) ([]common.Recommendation, error) { AppLogger.Printf("\n šŸ“ [%d/%d] Region: %s\n", regionIndex, totalRegions, region) recs := fetchRecommendationsForRegion(ctx, recClient, service, region, cfg) if len(recs) == 0 { AppLogger.Printf(" ā„¹ļø No recommendations found\n") - return nil + return nil, nil } AppLogger.Printf(" āœ… Found %d recommendations\n", len(recs)) populateRecommendationAccountNames(ctx, recs, accountCache) + if err := requireRegionInventory(recs, cfg, engineData); err != nil { + return nil, err + } recs = applyRegionFilters(recs, engineData, region, cfg, drops) if len(recs) == 0 { AppLogger.Printf(" ā„¹ļø No recommendations after applying filters\n") - return nil + return nil, nil } // Build the regional service client once and reuse for both expiry-aware @@ -703,7 +773,7 @@ func fetchAndFilterRegionRecs( recs = checkDuplicates(ctx, recs, serviceClient, effectiveDryRun(cfg), drops) } - return recs + return recs, nil } // fetchAllRecs collects recommendations from all services and regions without @@ -720,7 +790,7 @@ func fetchAllRecs( engineData engineVersionData, cfg Config, coverageMap recommendations.PoolCoverageMap, -) ([]common.Recommendation, *common.DropSummary) { +) ([]common.Recommendation, *common.DropSummary, error) { all := make([]common.Recommendation, 0) drops := common.NewDropSummary() for _, service := range servicesToProcess { @@ -734,9 +804,12 @@ func fetchAllRecs( continue } for i, region := range regions { - recs := fetchAndFilterRegionRecs(ctx, awsCfg, recClient, accountCache, service, region, i+1, len(regions), engineData, cfg, coverageMap, drops) + recs, err := fetchAndFilterRegionRecs(ctx, awsCfg, recClient, accountCache, service, region, i+1, len(regions), engineData, cfg, coverageMap, drops) + if err != nil { + return nil, nil, err + } all = append(all, recs...) } } - return all, drops + return all, drops, nil } diff --git a/cmd/multi_service_max_instances_test.go b/cmd/multi_service_max_instances_test.go index 0da06c8f1..435d3c0b3 100644 --- a/cmd/multi_service_max_instances_test.go +++ b/cmd/multi_service_max_instances_test.go @@ -92,7 +92,7 @@ func TestMaxInstancesCapsWholeRunAcrossServicesAndRegions(t *testing.T) { t.Cleanup(func() { mockClient.AssertExpectations(t) }) accountCache := NewAccountAliasCache(awsCfg) - allRecs, drops := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, + allRecs, drops, _ := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) pairs := len(maxInstancesFixtureServices) * len(maxInstancesFixtureRegions) @@ -172,7 +172,7 @@ func TestMaxInstancesKeepsHighestSavingsNotFirstFetched(t *testing.T) { t.Cleanup(func() { mockClient.AssertExpectations(t) }) accountCache := NewAccountAliasCache(awsCfg) - allRecs, drops := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, + allRecs, drops, _ := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) require.Len(t, allRecs, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) @@ -241,7 +241,7 @@ func TestMaxInstancesNeverPurchasesBelowMinCount(t *testing.T) { t.Cleanup(func() { mockClient.AssertExpectations(t) }) accountCache := NewAccountAliasCache(awsCfg) - allRecs, drops := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, + allRecs, drops, _ := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) require.Len(t, allRecs, len(maxInstancesFixtureServices)*len(maxInstancesFixtureRegions)) @@ -327,7 +327,7 @@ func TestMaxInstancesNotAppliedWhenUnset(t *testing.T) { t.Cleanup(func() { mockClient.AssertExpectations(t) }) accountCache := NewAccountAliasCache(awsCfg) - allRecs, drops := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, + allRecs, drops, _ := fetchAllRecs(ctx, awsCfg, mockClient, accountCache, maxInstancesFixtureServices, engineVersionData{}, toolCfg, nil) scored := scoreLimitAndDisplay(allRecs, toolCfg, drops) diff --git a/cmd/recommendation_completeness_proxy_test.go b/cmd/recommendation_completeness_proxy_test.go index eb2d8b8f5..d4926dace 100644 --- a/cmd/recommendation_completeness_proxy_test.go +++ b/cmd/recommendation_completeness_proxy_test.go @@ -235,7 +235,7 @@ func (p *completenessProxy) operation(host, op string, call int) (int, string, s return 200, "application/x-amz-json-1.1", `{"Recommendations":[{"RecommendationDetails":[` + details[p.details] + `]}]}` } if host == "ec2.us-east-1.amazonaws.com" && op == "DescribeRegions" { - if p.regions == "fallback" && call == 2 { + if p.regions == "fallback" && call == 1 { return 400, "text/xml", `UnauthorizedOperationfixture denied region discovery` } return 200, "text/xml", `us-east-1` @@ -265,9 +265,10 @@ func (p *completenessProxy) assertRequests(t *testing.T) { } } require.Equal(t, ceCalls, p.requests["AWSInsightsIndexService.GetReservationPurchaseRecommendation"], "operations=%v", p.requests) - wantRegions := 2 + // --include-extended-support skips the RDS exclusion queries, so only region discovery lists regions. + wantRegions := 1 if p.regions == "explicit" { - wantRegions = 1 + wantRegions = 0 } require.Equal(t, wantRegions, p.requests["DescribeRegions"], "region calls") t.Logf("actual root command, config loader, SDK and CSV; synthetic operations=%v", p.requests) @@ -346,11 +347,11 @@ func (p *completenessProxy) assertSPRequests(t *testing.T) { rows = 0 } } - operations := map[string]int{"DescribeRegions": 1, "DescribeDBInstances": 1, "DescribeDBMajorEngineVersions": 4, "AWSInsightsIndexService.GetSavingsPlansPurchaseRecommendation": len(expected)} + operations := map[string]int{"AWSInsightsIndexService.GetSavingsPlansPurchaseRecommendation": len(expected)} if rows > 0 { operations["DescribeSavingsPlans"] = rows } require.Equal(t, operations, p.requests, "unexpected read or purchase") - require.Equal(t, []string{"mysql", "postgres", "aurora-mysql", "aurora-postgresql"}, p.engines, "ancillary RDS engine reads") + require.Empty(t, p.engines, "--include-extended-support must skip the RDS lifecycle reads") t.Logf("actual root command, SP SDK and CSV; synthetic operations=%v", p.requests) } diff --git a/cmd/reservation_expiry_command_test.go b/cmd/reservation_expiry_command_test.go index 0512eb8ac..837475ecc 100644 --- a/cmd/reservation_expiry_command_test.go +++ b/cmd/reservation_expiry_command_test.go @@ -272,9 +272,8 @@ func (p *completenessProxy) assertReservationExpiryRequests(t *testing.T) { "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationPurchaseRecommendation": 1, "ce.us-east-1.amazonaws.com/AWSInsightsIndexService.GetReservationCoverage": 12, "organizations.us-east-1.amazonaws.com/AWSOrganizationsV20161128.DescribeAccount": s.rows, - "ec2.us-east-1.amazonaws.com/DescribeRegions": 1, "ec2.us-east-1.amazonaws.com/DescribeInstanceTypes": 1, - "ec2.us-east-1.amazonaws.com/DescribeReservedInstances": 2, - "rds.us-east-1.amazonaws.com/DescribeDBInstances": 1, "rds.us-east-1.amazonaws.com/DescribeDBMajorEngineVersions": 4, + "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) @@ -284,6 +283,6 @@ func (p *completenessProxy) assertReservationExpiryRequests(t *testing.T) { } require.ElementsMatch(t, wantCoverage, p.expiryCoverage) require.ElementsMatch(t, []string{"111111111111", "222222222222", "333333333333"}[:s.rows], p.expiryAccounts) - require.ElementsMatch(t, []string{"mysql", "postgres", "aurora-mysql", "aurora-postgresql"}, p.engines) + require.Empty(t, p.engines, "--include-extended-support must skip the RDS lifecycle reads") t.Logf("actual root command, SDK and CSV; synthetic expiry operations=%v coverage=%v accounts=%v", p.requests, p.expiryCoverage, p.expiryAccounts) } From 003422df30230539c76ad37bc7d9fa1583d966a4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 10 Oct 2026 09:05:08 +0200 Subject: [PATCH 2/2] test(cmd): cover extended-support exclusion wiring in the main pipeline (#2147) Re-exec the command against an in-process stub endpoint and assert exit 1 with the exclusion message and zero Purchase calls for a purchase run with a denied engine lifecycle query, a dry run with the same denial, and a dry run with an RDS rec in a region whose inventory is denied; with --include-extended-support no exclusion query is made. Also make TestFetchAllRecs_RDSRecInFailedRegionAborts use the stub transport and fix a stale comment. Co-Authored-By: Claude Sonnet 5.5 --- cmd/multi_service_engine_versions.go | 2 +- ...rvice_extended_support_unavailable_test.go | 97 ++++++++++++++++--- 2 files changed, 83 insertions(+), 16 deletions(-) diff --git a/cmd/multi_service_engine_versions.go b/cmd/multi_service_engine_versions.go index 740c1bebd..bd38e1451 100644 --- a/cmd/multi_service_engine_versions.go +++ b/cmd/multi_service_engine_versions.go @@ -244,7 +244,7 @@ func queryMajorEngineVersionsWithClient(ctx context.Context, rdsClient RDSMajorV } if err := fetchMajorEngineVersionsForEngine(ctx, rdsClient, engine, versionInfo); err != nil { // Canceled caller context is terminal (issue #1325); check ctx.Err(), - // not the wrapped API error, so SDK-internal timeouts stay warnings. + // not the wrapped API error, so SDK-internal timeouts are reported as engine failures. if ctxErr := ctx.Err(); ctxErr != nil { return nil, ctxErr } diff --git a/cmd/multi_service_extended_support_unavailable_test.go b/cmd/multi_service_extended_support_unavailable_test.go index f96b9124b..8c3e0dc86 100644 --- a/cmd/multi_service_extended_support_unavailable_test.go +++ b/cmd/multi_service_extended_support_unavailable_test.go @@ -9,6 +9,7 @@ import ( "net/http/httptest" "net/url" "os" + "os/exec" "path/filepath" "strings" "sync" @@ -23,8 +24,8 @@ import ( ) // Regression tests for #2147: unavailable RDS inventory or lifecycle data must -// not silently disable the extended-support exclusion. Nothing here reaches -// AWS: every request is answered by an in-process stub. +// not silently disable the extended-support exclusion. Requests are answered by +// an in-process stub (AWS_ENDPOINT_URL or an injected transport). // rdsAPIStub answers the three query-protocol calls the exclusion data needs. // Behavior is selected by the fields; every request is recorded. @@ -58,35 +59,47 @@ func (s *rdsAPIStub) total() int { return len(s.actions) } +const ( + xmlContentType = "text/xml" + jsonContentType = "application/x-amz-json-1.1" +) + const rdsStubDenied = `SenderAccessDenieddenied by stubr` -func (s *rdsAPIStub) respond(authz, body string) (int, string) { +func (s *rdsAPIStub) respond(authz, target, body string) (int, string, string) { form, _ := url.ParseQuery(body) action := form.Get("Action") region := "" if parts := strings.Split(authz, "/"); len(parts) > 2 { region = parts[2] } + if target != "" { + action = strings.TrimPrefix(target, "AWSInsightsIndexService.") + } s.mu.Lock() s.actions = append(s.actions, action) s.mu.Unlock() switch action { + case "GetReservationCoverage": + return http.StatusOK, jsonContentType, `{"CoveragesByTime":[]}` + case "GetReservationPurchaseRecommendation": + return http.StatusOK, jsonContentType, `{"Recommendations":[{"RecommendationDetails":[{"RecommendedNumberOfInstancesToPurchase":"2","EstimatedMonthlySavingsAmount":"10","EstimatedMonthlyOnDemandCost":"30","InstanceDetails":{"RDSInstanceDetails":{"InstanceType":"db.t3.medium","Region":"us-east-1","DeploymentOption":"Single-AZ","DatabaseEngine":"MySQL"}}}]}]}` case "DescribeRegions": if s.failRegionsEC { - return http.StatusForbidden, rdsStubDenied + return http.StatusForbidden, xmlContentType, rdsStubDenied } var items strings.Builder for _, r := range s.regionList { fmt.Fprintf(&items, "%s", r) } - return http.StatusOK, `` + items.String() + `` + return http.StatusOK, xmlContentType, `` + items.String() + `` case "DescribeDBInstances": if s.panicRegions[region] { panic("stub panic in " + region) } if s.failRegions[region] || (s.failPage2[region] && form.Get("Marker") != "") { - return http.StatusForbidden, rdsStubDenied + return http.StatusForbidden, xmlContentType, rdsStubDenied } marker := "" if s.failPage2[region] { @@ -96,23 +109,23 @@ func (s *rdsAPIStub) respond(authz, body string) (int, string) { if class := s.instances[region]; class != "" { inst = fmt.Sprintf("%smysql5.7.44", class) } - return http.StatusOK, `` + inst + `` + marker + `` + return http.StatusOK, xmlContentType, `` + inst + `` + marker + `` case "DescribeDBMajorEngineVersions": if s.failEngines[form.Get("Engine")] { - return http.StatusForbidden, rdsStubDenied + return http.StatusForbidden, xmlContentType, rdsStubDenied } - return http.StatusOK, `` + return http.StatusOK, xmlContentType, `` } - return http.StatusBadRequest, rdsStubDenied + return http.StatusBadRequest, xmlContentType, rdsStubDenied } // RoundTrip lets tests hand the stub to an aws.Config directly. func (s *rdsAPIStub) RoundTrip(r *http.Request) (*http.Response, error) { b, _ := io.ReadAll(r.Body) - status, body := s.respond(r.Header.Get("Authorization"), string(b)) + status, contentType, body := s.respond(r.Header.Get("Authorization"), r.Header.Get("X-Amz-Target"), string(b)) return &http.Response{ StatusCode: status, - Header: http.Header{"Content-Type": []string{"text/xml"}}, + Header: http.Header{"Content-Type": []string{contentType}}, Body: io.NopCloser(strings.NewReader(body)), Request: r, }, nil @@ -135,8 +148,8 @@ func (s *rdsAPIStub) serveEnv(t *testing.T) { t.Helper() srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { b, _ := io.ReadAll(r.Body) - status, body := s.respond(r.Header.Get("Authorization"), string(b)) - w.Header().Set("Content-Type", "text/xml") + status, contentType, body := s.respond(r.Header.Get("Authorization"), r.Header.Get("X-Amz-Target"), string(b)) + w.Header().Set("Content-Type", contentType) w.WriteHeader(status) _, _ = io.WriteString(w, body) })) @@ -154,6 +167,7 @@ func (s *rdsAPIStub) serveEnv(t *testing.T) { t.Setenv("AWS_SESSION_TOKEN", "") t.Setenv("AWS_MAX_ATTEMPTS", "1") t.Setenv("AWS_ENDPOINT_URL", srv.URL) + t.Setenv("AWS_ENDPOINT_URL_COST_EXPLORER", srv.URL) } // requireStubReached fails the test when the seam fell through to somewhere @@ -423,7 +437,7 @@ func TestFilterAndAdjustRecommendations_ExtendedSupportDataUnavailable(t *testin func TestFetchAllRecs_RDSRecInFailedRegionAborts(t *testing.T) { ctx := context.Background() - awsCfg := aws.Config{Region: "us-east-1"} + awsCfg := newRDSStub("us-east-1").awsConfig() // injected stub transport: no outbound connection saved := saveGlobalVars() defer saved.restore() toolCfg.Coverage = 100 @@ -443,3 +457,56 @@ func TestFetchAllRecs_RDSRecInFailedRegionAborts(t *testing.T) { require.Error(t, err) assert.ErrorContains(t, err, "eu-west-1") } + +// The main pipeline wiring (runToolMultiService) is exercised in a re-exec'd +// child: the child runs the real command with the real engine-version fetcher +// against this process's stub endpoint, and exits through log.Fatalf. +func TestRunToolMultiService_ExtendedSupportExclusionWiring(t *testing.T) { + if os.Getenv("CUDLY_EXCL_CHILD") == "1" { + engineVersionFetcher = fetchEngineVersionData + rootCmd.SetArgs(strings.Fields(os.Getenv("CUDLY_EXCL_ARGS"))) + require.NoError(t, rootCmd.Execute()) + return + } + + const denyHint = "Cannot apply extended-support exclusion" + tests := []struct { + name string + setup func(*rdsAPIStub) + args string + wantExit1 bool + wantRDS bool // exclusion queries expected + }{ + {"purchase run, engine lifecycle denied", func(s *rdsAPIStub) { s.failEngines["mysql"] = true }, "--purchase", true, true}, + {"dry run, engine lifecycle denied", func(s *rdsAPIStub) { s.failEngines["mysql"] = true }, "", true, true}, + {"dry run, rec in a region whose inventory is denied", func(s *rdsAPIStub) { s.failRegions["us-east-1"] = true }, "", true, true}, + {"opt-in skips every exclusion query", func(s *rdsAPIStub) { s.failEngines["mysql"], s.failRegions["us-east-1"] = true, true }, "--include-extended-support", false, false}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + stub := newRDSStub("us-east-1") + tt.setup(stub) + stub.serveEnv(t) + dir := t.TempDir() + args := "--services rds --term 1 --coverage 100 --regions us-east-1 --output " + filepath.Join(dir, "out.csv") + + " --audit-log " + filepath.Join(dir, "audit.jsonl") + " " + tt.args + child := exec.Command(os.Args[0], "-test.run=^TestRunToolMultiService_ExtendedSupportExclusionWiring$") + child.Env = append(os.Environ(), "CUDLY_EXCL_CHILD=1", "CUDLY_EXCL_ARGS="+args, "AWS_REGION=us-east-1") + out, err := child.CombinedOutput() + + assert.Positive(t, stub.total(), "child must have reached the stub:\n%s", out) + assert.Zero(t, stub.count("Purchase"), "no purchase call may be made") + if tt.wantExit1 { + var exitErr *exec.ExitError + require.ErrorAs(t, err, &exitErr, "output:\n%s", out) + assert.Equal(t, 1, exitErr.ExitCode()) + assert.Contains(t, string(out), denyHint) + assert.Contains(t, string(out), "--include-extended-support") + return + } + require.NoError(t, err, "output:\n%s", out) + assert.Zero(t, stub.count("DescribeDBInstances")) + assert.Zero(t, stub.count("DescribeDBMajorEngineVersions")) + }) + } +}