diff --git a/cmd/multi_service_engine_versions.go b/cmd/multi_service_engine_versions.go index 94bef3ee0..1e1c96746 100644 --- a/cmd/multi_service_engine_versions.go +++ b/cmd/multi_service_engine_versions.go @@ -14,6 +14,7 @@ import ( awsec2 "github.com/aws/aws-sdk-go-v2/service/ec2" ec2types "github.com/aws/aws-sdk-go-v2/service/ec2/types" awsrds "github.com/aws/aws-sdk-go-v2/service/rds" + rdstypes "github.com/aws/aws-sdk-go-v2/service/rds/types" ) // InstanceEngineVersion stores engine version information for an instance @@ -87,6 +88,16 @@ func getAWSRegions(ctx context.Context, awsCfg aws.Config) ([]ec2types.Region, e // maxConcurrentRegionQueries limits the number of concurrent AWS API calls across regions const maxConcurrentRegionQueries = 10 +// maxEngineVersionPages caps DescribeDBMajorEngineVersions pagination per engine. +// 20 pages x ~100 records/page = ~2000 records, enough for any engine list (issue #692). +const maxEngineVersionPages = 20 + +// RDSMajorVersionsClient is the subset of the RDS API needed by +// queryMajorEngineVersionsWithClient, extracted so tests can inject a mock. +type RDSMajorVersionsClient interface { + DescribeDBMajorEngineVersions(ctx context.Context, params *awsrds.DescribeDBMajorEngineVersionsInput, optFns ...func(*awsrds.Options)) (*awsrds.DescribeDBMajorEngineVersionsOutput, error) +} + // 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) @@ -186,8 +197,12 @@ func queryMajorEngineVersions(ctx context.Context, cfg Config) (map[string]Major return nil, fmt.Errorf("failed to load AWS config: %w", err) } - rdsClient := awsrds.NewFromConfig(awsCfg) + return queryMajorEngineVersionsWithClient(ctx, awsrds.NewFromConfig(awsCfg)) +} +// queryMajorEngineVersionsWithClient is the testable core of queryMajorEngineVersions. +// It accepts a RDSMajorVersionsClient so tests can inject a mock without real AWS creds. +func queryMajorEngineVersionsWithClient(ctx context.Context, rdsClient RDSMajorVersionsClient) (map[string]MajorEngineVersionInfo, error) { // Map of "engine:majorVersion" -> MajorEngineVersionInfo versionInfo := make(map[string]MajorEngineVersionInfo) @@ -195,42 +210,80 @@ func queryMajorEngineVersions(ctx context.Context, cfg Config) (map[string]Major engines := []string{"mysql", "postgres", "aurora-mysql", "aurora-postgresql"} for _, engine := range engines { + if err := fetchMajorEngineVersionsForEngine(ctx, rdsClient, engine, versionInfo); err != nil { + log.Printf("Warning: Failed to describe major engine versions for %s: %v", engine, err) + } + } + + return versionInfo, nil +} + +// fetchMajorEngineVersionsForEngine fetches all pages of major engine version +// info for a single engine and merges results into versionInfo. Returns an error +// only on API failure or pagination cap exceeded (issue #692). +func fetchMajorEngineVersionsForEngine(ctx context.Context, rdsClient RDSMajorVersionsClient, engine string, versionInfo map[string]MajorEngineVersionInfo) error { + var marker *string + + for pageIdx := 0; ; pageIdx++ { + if err := ctx.Err(); err != nil { + return err + } + if pageIdx >= maxEngineVersionPages { + return fmt.Errorf( + "pagination cap reached after %d pages for engine %s (issue #692)", + maxEngineVersionPages, engine, + ) + } + output, err := rdsClient.DescribeDBMajorEngineVersions(ctx, &awsrds.DescribeDBMajorEngineVersionsInput{ Engine: aws.String(engine), + Marker: marker, }) if err != nil { - log.Printf("⚠️ Warning: Failed to describe major engine versions for %s: %v", engine, err) - continue + return err } for _, version := range output.DBMajorEngineVersions { - info := MajorEngineVersionInfo{ - Engine: aws.ToString(version.Engine), - MajorEngineVersion: aws.ToString(version.MajorEngineVersion), - } + info := parseDBMajorEngineVersion(version) + key := fmt.Sprintf("%s:%s", info.Engine, info.MajorEngineVersion) + versionInfo[key] = info + } - // Parse lifecycle support dates - for _, lifecycle := range version.SupportedEngineLifecycles { - lifecycleInfo := EngineLifecycleInfo{ - LifecycleSupportName: string(lifecycle.LifecycleSupportName), - } + if output.Marker == nil || aws.ToString(output.Marker) == "" { + break + } + marker = output.Marker + } - if lifecycle.LifecycleSupportStartDate != nil { - lifecycleInfo.LifecycleSupportStartDate = *lifecycle.LifecycleSupportStartDate - } - if lifecycle.LifecycleSupportEndDate != nil { - lifecycleInfo.LifecycleSupportEndDate = *lifecycle.LifecycleSupportEndDate - } + return nil +} - info.SupportedEngineLifecycles = append(info.SupportedEngineLifecycles, lifecycleInfo) - } +// parseDBMajorEngineVersion converts an RDS DBMajorEngineVersion into a +// MajorEngineVersionInfo, extracting lifecycle support dates. Extracted from +// fetchMajorEngineVersionsForEngine to keep its cyclomatic complexity below +// the gocyclo cap. +func parseDBMajorEngineVersion(version rdstypes.DBMajorEngineVersion) MajorEngineVersionInfo { + info := MajorEngineVersionInfo{ + Engine: aws.ToString(version.Engine), + MajorEngineVersion: aws.ToString(version.MajorEngineVersion), + } - key := fmt.Sprintf("%s:%s", info.Engine, info.MajorEngineVersion) - versionInfo[key] = info + for _, lifecycle := range version.SupportedEngineLifecycles { + lifecycleInfo := EngineLifecycleInfo{ + LifecycleSupportName: string(lifecycle.LifecycleSupportName), + } + + if lifecycle.LifecycleSupportStartDate != nil { + lifecycleInfo.LifecycleSupportStartDate = *lifecycle.LifecycleSupportStartDate + } + if lifecycle.LifecycleSupportEndDate != nil { + lifecycleInfo.LifecycleSupportEndDate = *lifecycle.LifecycleSupportEndDate } + + info.SupportedEngineLifecycles = append(info.SupportedEngineLifecycles, lifecycleInfo) } - return versionInfo, nil + return info } // extractMajorVersion extracts the major version from a full engine version string diff --git a/cmd/multi_service_engine_versions_paginate_test.go b/cmd/multi_service_engine_versions_paginate_test.go new file mode 100644 index 000000000..cb68aa097 --- /dev/null +++ b/cmd/multi_service_engine_versions_paginate_test.go @@ -0,0 +1,153 @@ +package main + +import ( + "context" + "fmt" + "testing" + "time" + + "github.com/aws/aws-sdk-go-v2/aws" + awsrds "github.com/aws/aws-sdk-go-v2/service/rds" + rdstypes "github.com/aws/aws-sdk-go-v2/service/rds/types" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// multiPageRDSMajorVersionsMock implements RDSMajorVersionsClient and returns +// distinct pages based on the Marker in the incoming request. +type multiPageRDSMajorVersionsMock struct { + pages []*awsrds.DescribeDBMajorEngineVersionsOutput + tokens []string // tokens[i] triggers pages[i+1]; first call has empty marker + calls int +} + +func (m *multiPageRDSMajorVersionsMock) DescribeDBMajorEngineVersions( + _ context.Context, + params *awsrds.DescribeDBMajorEngineVersionsInput, + _ ...func(*awsrds.Options), +) (*awsrds.DescribeDBMajorEngineVersionsOutput, error) { + idx := 0 + incoming := aws.ToString(params.Marker) + for i, tok := range m.tokens { + if tok == incoming { + idx = i + 1 + break + } + } + if incoming == "" { + idx = 0 + } + m.calls++ + if idx >= len(m.pages) { + return nil, fmt.Errorf("unexpected RDS Marker %q", incoming) + } + return m.pages[idx], nil +} + +// rdsMajorVersion builds a minimal DBMajorEngineVersion for tests. +func rdsMajorVersion(engine, major string) rdstypes.DBMajorEngineVersion { + return rdstypes.DBMajorEngineVersion{ + Engine: aws.String(engine), + MajorEngineVersion: aws.String(major), + SupportedEngineLifecycles: []rdstypes.SupportedEngineLifecycle{ + { + LifecycleSupportName: "open-source-rds-extended-support", + LifecycleSupportStartDate: aws.Time(time.Now().AddDate(-1, 0, 0)), + LifecycleSupportEndDate: aws.Time(time.Now().AddDate(2, 0, 0)), + }, + }, + } +} + +// TestFetchMajorEngineVersionsForEngine_Paginates asserts that all pages are +// fetched and results accumulated (issue #692 regression test). +func TestFetchMajorEngineVersionsForEngine_Paginates(t *testing.T) { + mock := &multiPageRDSMajorVersionsMock{ + pages: []*awsrds.DescribeDBMajorEngineVersionsOutput{ + { + DBMajorEngineVersions: []rdstypes.DBMajorEngineVersion{ + rdsMajorVersion("mysql", "5.7"), + rdsMajorVersion("mysql", "8.0"), + }, + Marker: aws.String("tok1"), + }, + { + DBMajorEngineVersions: []rdstypes.DBMajorEngineVersion{ + rdsMajorVersion("mysql", "8.4"), + rdsMajorVersion("mysql", "9.0"), + }, + Marker: aws.String("tok2"), + }, + { + DBMajorEngineVersions: []rdstypes.DBMajorEngineVersion{ + rdsMajorVersion("mysql", "9.1"), + }, + Marker: nil, + }, + }, + tokens: []string{"tok1", "tok2"}, + } + + versionInfo := make(map[string]MajorEngineVersionInfo) + err := fetchMajorEngineVersionsForEngine(context.Background(), mock, "mysql", versionInfo) + require.NoError(t, err) + // 2 + 2 + 1 = 5 versions across 3 pages + assert.Len(t, versionInfo, 5, "must accumulate all versions across pages") + assert.Equal(t, 3, mock.calls, "must call API once per page") + assert.Contains(t, versionInfo, "mysql:5.7") + assert.Contains(t, versionInfo, "mysql:9.1") +} + +// TestFetchMajorEngineVersionsForEngine_EmptyMarkerTerminates asserts that an +// empty-string Marker is treated as terminal (parity with PR #690). +func TestFetchMajorEngineVersionsForEngine_EmptyMarkerTerminates(t *testing.T) { + mock := &multiPageRDSMajorVersionsMock{ + pages: []*awsrds.DescribeDBMajorEngineVersionsOutput{ + { + DBMajorEngineVersions: []rdstypes.DBMajorEngineVersion{ + rdsMajorVersion("mysql", "8.0"), + }, + Marker: aws.String(""), // empty string -- must terminate + }, + }, + tokens: []string{}, + } + + versionInfo := make(map[string]MajorEngineVersionInfo) + err := fetchMajorEngineVersionsForEngine(context.Background(), mock, "mysql", versionInfo) + require.NoError(t, err) + assert.Len(t, versionInfo, 1) + assert.Equal(t, 1, mock.calls, "empty-string Marker must terminate after page 1") +} + +// alwaysNextPageRDSMock returns pages each carrying a non-nil non-empty Marker. +type alwaysNextPageRDSMock struct { + calls int +} + +func (m *alwaysNextPageRDSMock) DescribeDBMajorEngineVersions( + _ context.Context, + _ *awsrds.DescribeDBMajorEngineVersionsInput, + _ ...func(*awsrds.Options), +) (*awsrds.DescribeDBMajorEngineVersionsOutput, error) { + m.calls++ + return &awsrds.DescribeDBMajorEngineVersionsOutput{ + DBMajorEngineVersions: []rdstypes.DBMajorEngineVersion{ + rdsMajorVersion("mysql", fmt.Sprintf("5.%d", m.calls)), + }, + Marker: aws.String(fmt.Sprintf("tok%d", m.calls)), + }, nil +} + +// TestFetchMajorEngineVersionsForEngine_PaginationCapError asserts that +// exceeding maxEngineVersionPages returns a diagnostic error (issue #692). +func TestFetchMajorEngineVersionsForEngine_PaginationCapError(t *testing.T) { + mock := &alwaysNextPageRDSMock{} + versionInfo := make(map[string]MajorEngineVersionInfo) + + err := fetchMajorEngineVersionsForEngine(context.Background(), mock, "mysql", versionInfo) + require.Error(t, err) + assert.Contains(t, err.Error(), "pagination cap reached") + assert.Equal(t, maxEngineVersionPages, mock.calls, + "must stop exactly at the cap") +} diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index efc7ed314..9d94dc454 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -15,6 +15,12 @@ import ( "github.com/LeanerCloud/CUDly/pkg/logging" ) +// maxRecommendationPages caps the number of pages fetched per Cost Explorer +// GetReservationPurchaseRecommendation or GetSavingsPlansPurchaseRecommendation +// call. 20 pages x ~100 items/page = ~2000 items, enough headroom for any +// payer org we have seen. Exceeding the cap returns a diagnostic error (issue #692). +const maxRecommendationPages = 20 + // CostExplorerAPI defines the interface for Cost Explorer operations type CostExplorerAPI interface { GetReservationPurchaseRecommendation(ctx context.Context, params *costexplorer.GetReservationPurchaseRecommendationInput, optFns ...func(*costexplorer.Options)) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) @@ -72,16 +78,65 @@ func (c *Client) GetRecommendations(ctx context.Context, params common.Recommend AccountScope: types.AccountScopeLinked, } - // Implement rate limiting with exponential backoff. The shared semaphore - // (if any) on ctx bounds aggregate concurrent Cost Explorer requests; we - // Acquire/Release around the SDK call itself rather than around the whole - // service sweep so a goroutine waiting on rate-limiter backoff or - // processing a response doesn't monopolise a permit while no request is - // in flight. See pkg/concurrency. + allRecs, err := c.fetchRIAllPages(ctx, input, params.Service) + if err != nil { + return nil, err + } + + return c.parseRecommendations(allRecs, params) +} + +// fetchRIAllPages paginates over all pages of RI recommendations for a single +// (service, term, payment) combination. ctx.Err() is checked at the top of +// each iteration so cancellation is terminal (per feedback_ctx_cancel_terminal.md, +// issue #692). +func (c *Client) fetchRIAllPages( + ctx context.Context, + input *costexplorer.GetReservationPurchaseRecommendationInput, + service common.ServiceType, +) ([]types.ReservationPurchaseRecommendation, error) { + var allRecs []types.ReservationPurchaseRecommendation + var nextPageToken *string + + for pageIdx := 0; ; pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, err + } + if pageIdx >= maxRecommendationPages { + return nil, fmt.Errorf( + "pagination cap reached after %d pages for RI %s (issue #692)", + maxRecommendationPages, service, + ) + } + input.NextPageToken = nextPageToken + + result, err := c.fetchRIPageWithRetry(ctx, input) + if err != nil { + return nil, err + } + + allRecs = append(allRecs, result.Recommendations...) + + if result.NextPageToken == nil || aws.ToString(result.NextPageToken) == "" { + break + } + nextPageToken = result.NextPageToken + } + + return allRecs, nil +} + +// fetchRIPageWithRetry executes a single GetReservationPurchaseRecommendation +// call with rate-limiter exponential back-off. Extracted so the pagination loop +// in fetchRIAllPages stays below the gocyclo cap. +func (c *Client) fetchRIPageWithRetry( + ctx context.Context, + input *costexplorer.GetReservationPurchaseRecommendationInput, +) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { + c.rateLimiter.Reset() var result *costexplorer.GetReservationPurchaseRecommendationOutput var err error - c.rateLimiter.Reset() for { if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) @@ -101,7 +156,7 @@ func (c *Client) GetRecommendations(ctx context.Context, params common.Recommend return nil, fmt.Errorf("failed to get RI recommendations after %d retries: %w", c.rateLimiter.GetRetryCount(), err) } - return c.parseRecommendations(result.Recommendations, params) + return result, nil } // defaultDiscoveryTerms enumerates the term lengths the discovery flow diff --git a/providers/aws/recommendations/client_test.go b/providers/aws/recommendations/client_test.go index a8ddee1e6..b95f7960a 100644 --- a/providers/aws/recommendations/client_test.go +++ b/providers/aws/recommendations/client_test.go @@ -2,6 +2,7 @@ package recommendations import ( "context" + "fmt" "testing" "time" @@ -519,9 +520,13 @@ func TestGetRecommendations_ContextCancellation(t *testing.T) { recs, err := client.GetRecommendations(ctx, params) + // With the pagination loop added (issue #692), ctx.Err() is checked at + // the top of the first page iteration before the rate-limiter runs. A + // pre-cancelled context therefore returns context.Canceled directly, which + // is the correct behavior per feedback_ctx_cancel_terminal.md. assert.Error(t, err) assert.Nil(t, recs) - assert.Contains(t, err.Error(), "rate limiter wait failed") + assert.ErrorIs(t, err, context.Canceled) } // TestGetAllRecommendations_PropagatesContextCancellation pins the contract @@ -559,3 +564,195 @@ func TestGetAllRecommendations_PropagatesContextCancellation(t *testing.T) { "GetAllRecommendations must propagate the parent ctx error after g.Wait()") assert.Nil(t, recs) } + +// multiPageRIMock returns distinct pages for GetReservationPurchaseRecommendation +// based on the NextPageToken in the incoming request. Implements CostExplorerAPI. +type multiPageRIMock struct { + // pages is an ordered list of outputs to return. The first call (token=="") + // returns pages[0], the call with token "tok1" returns pages[1], etc. + // tokens[i] is the NextPageToken value that triggers pages[i+1]. + pages []*costexplorer.GetReservationPurchaseRecommendationOutput + tokens []string // len == len(pages)-1; pages[last] has nil token + calls int +} + +func (m *multiPageRIMock) GetReservationPurchaseRecommendation( + ctx context.Context, + params *costexplorer.GetReservationPurchaseRecommendationInput, + _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { + idx := 0 + incoming := aws.ToString(params.NextPageToken) + for i, tok := range m.tokens { + if tok == incoming { + idx = i + 1 + break + } + } + // First call has empty/nil token + if incoming == "" { + idx = 0 + } + m.calls++ + if idx >= len(m.pages) { + return nil, fmt.Errorf("unexpected page token %q", incoming) + } + return m.pages[idx], nil +} + +func (m *multiPageRIMock) GetSavingsPlansPurchaseRecommendation( + _ context.Context, _ *costexplorer.GetSavingsPlansPurchaseRecommendationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetSavingsPlansPurchaseRecommendationOutput, error) { + return &costexplorer.GetSavingsPlansPurchaseRecommendationOutput{}, nil +} + +func (m *multiPageRIMock) GetReservationUtilization( + _ context.Context, _ *costexplorer.GetReservationUtilizationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationUtilizationOutput, error) { + return &costexplorer.GetReservationUtilizationOutput{}, nil +} + +func (m *multiPageRIMock) GetReservationCoverage( + _ context.Context, _ *costexplorer.GetReservationCoverageInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationCoverageOutput, error) { + return &costexplorer.GetReservationCoverageOutput{}, nil +} + +// riDetail returns a minimal ReservationPurchaseRecommendation with n EC2 details. +func riDetail(n int) types.ReservationPurchaseRecommendation { + details := make([]types.ReservationPurchaseRecommendationDetail, n) + for i := range details { + details[i] = types.ReservationPurchaseRecommendationDetail{ + RecommendedNumberOfInstancesToPurchase: aws.String("1"), + EstimatedMonthlySavingsAmount: aws.String("10.00"), + EstimatedMonthlySavingsPercentage: aws.String("10.0"), + InstanceDetails: &types.InstanceDetails{ + EC2InstanceDetails: &types.EC2InstanceDetails{ + InstanceType: aws.String("m5.large"), + Platform: aws.String("Linux/UNIX"), + Region: aws.String("us-east-1"), + }, + }, + } + } + return types.ReservationPurchaseRecommendation{RecommendationDetails: details} +} + +// TestGetRecommendations_RI_Paginates asserts that GetRecommendations accumulates +// items across all pages (issue #692 regression test). +func TestGetRecommendations_RI_Paginates(t *testing.T) { + mock := &multiPageRIMock{ + pages: []*costexplorer.GetReservationPurchaseRecommendationOutput{ + { + Recommendations: []types.ReservationPurchaseRecommendation{riDetail(2)}, + NextPageToken: aws.String("tok1"), + }, + { + Recommendations: []types.ReservationPurchaseRecommendation{riDetail(3)}, + NextPageToken: aws.String("tok2"), + }, + { + Recommendations: []types.ReservationPurchaseRecommendation{riDetail(4)}, + NextPageToken: nil, + }, + }, + tokens: []string{"tok1", "tok2"}, + } + + client := NewClientWithAPI(mock, "us-east-1") + params := common.RecommendationParams{ + Service: common.ServiceEC2, + PaymentOption: "partial-upfront", + Term: "1yr", + LookbackPeriod: "7d", + } + + recs, err := client.GetRecommendations(context.Background(), params) + require.NoError(t, err) + // 2 + 3 + 4 = 9 recommendation details, each yielding one rec + assert.Len(t, recs, 9, "must accumulate recs across all 3 pages") + assert.Equal(t, 3, mock.calls, "must call CE exactly once per page") +} + +// TestGetRecommendations_RI_EmptyTokenTerminates asserts that an empty-string +// NextPageToken is treated as terminal (no extra call). Parity with PR #690 +// CR category-A fix. +func TestGetRecommendations_RI_EmptyTokenTerminates(t *testing.T) { + mock := &multiPageRIMock{ + pages: []*costexplorer.GetReservationPurchaseRecommendationOutput{ + { + Recommendations: []types.ReservationPurchaseRecommendation{riDetail(1)}, + NextPageToken: aws.String(""), // empty string -- must terminate + }, + }, + tokens: []string{}, + } + + client := NewClientWithAPI(mock, "us-east-1") + params := common.RecommendationParams{ + Service: common.ServiceEC2, + PaymentOption: "partial-upfront", + Term: "1yr", + LookbackPeriod: "7d", + } + + recs, err := client.GetRecommendations(context.Background(), params) + require.NoError(t, err) + assert.Len(t, recs, 1) + assert.Equal(t, 1, mock.calls, "empty-string token must terminate pagination after page 1") +} + +// alwaysNextPageRIMock returns pages each carrying a non-empty NextPageToken, +// used to exercise the maxRecommendationPages cap. +type alwaysNextPageRIMock struct { + calls int +} + +func (m *alwaysNextPageRIMock) GetReservationPurchaseRecommendation( + _ context.Context, + _ *costexplorer.GetReservationPurchaseRecommendationInput, + _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { + m.calls++ + return &costexplorer.GetReservationPurchaseRecommendationOutput{ + Recommendations: []types.ReservationPurchaseRecommendation{riDetail(1)}, + NextPageToken: aws.String(fmt.Sprintf("tok%d", m.calls)), + }, nil +} + +func (m *alwaysNextPageRIMock) GetSavingsPlansPurchaseRecommendation( + _ context.Context, _ *costexplorer.GetSavingsPlansPurchaseRecommendationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetSavingsPlansPurchaseRecommendationOutput, error) { + return &costexplorer.GetSavingsPlansPurchaseRecommendationOutput{}, nil +} + +func (m *alwaysNextPageRIMock) GetReservationUtilization( + _ context.Context, _ *costexplorer.GetReservationUtilizationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationUtilizationOutput, error) { + return &costexplorer.GetReservationUtilizationOutput{}, nil +} + +func (m *alwaysNextPageRIMock) GetReservationCoverage( + _ context.Context, _ *costexplorer.GetReservationCoverageInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationCoverageOutput, error) { + return &costexplorer.GetReservationCoverageOutput{}, nil +} + +// TestGetRecommendations_RI_PaginationCapError asserts that exceeding +// maxRecommendationPages returns a diagnostic error (issue #692). +func TestGetRecommendations_RI_PaginationCapError(t *testing.T) { + mock := &alwaysNextPageRIMock{} + client := NewClientWithAPI(mock, "us-east-1") + params := common.RecommendationParams{ + Service: common.ServiceEC2, + PaymentOption: "partial-upfront", + Term: "1yr", + LookbackPeriod: "7d", + } + + _, err := client.GetRecommendations(context.Background(), params) + require.Error(t, err) + assert.Contains(t, err.Error(), "pagination cap reached") + assert.Equal(t, maxRecommendationPages, mock.calls, + "must stop exactly at the cap, not one page later") +} diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index 800c5a41b..231e1dc0a 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -45,32 +45,11 @@ func (c *Client) getSavingsPlansRecommendations(ctx context.Context, params comm AccountScope: types.AccountScopeLinked, } - // Acquire/Release the shared semaphore (if any on ctx) around each - // individual SDK call so rate-limiter backoff waits don't tie up a - // permit while no request is actually in flight. See pkg/concurrency. - c.rateLimiter.Reset() - var result *costexplorer.GetSavingsPlansPurchaseRecommendationOutput - var err error - - for { - if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { - return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) - } - - if acqErr := concurrency.Acquire(ctx); acqErr != nil { - return nil, fmt.Errorf("concurrency acquire failed: %w", acqErr) - } - result, err = c.costExplorerClient.GetSavingsPlansPurchaseRecommendation(ctx, input) - concurrency.Release(ctx) - if !c.rateLimiter.ShouldRetry(err) { - break - } - } - + recs, err := c.fetchSPAllPages(ctx, input, params, planType) if err != nil { // When the caller scoped the request to one plan type // (post-issue-#22 split), a Cost Explorer failure means an - // entire SP service collection returns nothing — silently + // entire SP service collection returns nothing -- silently // dropping that as "0 recommendations" hides real outages. // Propagate. The umbrella iterate-all path keeps logging // and continuing so a transient failure on one plan type @@ -82,13 +61,86 @@ func (c *Client) getSavingsPlansRecommendations(ctx context.Context, params comm continue } + allRecommendations = append(allRecommendations, recs...) + } + + return allRecommendations, nil +} + +// fetchSPAllPages paginates over all pages of SP recommendations for a single +// plan type. ctx.Err() is checked at the top of each iteration so cancellation +// is terminal (per feedback_ctx_cancel_terminal.md, issue #692). +func (c *Client) fetchSPAllPages( + ctx context.Context, + input *costexplorer.GetSavingsPlansPurchaseRecommendationInput, + params common.RecommendationParams, + planType types.SupportedSavingsPlansType, +) ([]common.Recommendation, error) { + var allRecs []common.Recommendation + var nextPageToken *string + + for pageIdx := 0; ; pageIdx++ { + if err := ctx.Err(); err != nil { + return nil, err + } + if pageIdx >= maxRecommendationPages { + return nil, fmt.Errorf( + "pagination cap reached after %d pages for SP %s (issue #692)", + maxRecommendationPages, planType, + ) + } + input.NextPageToken = nextPageToken + + result, err := c.fetchSPPageWithRetry(ctx, input) + if err != nil { + return nil, err + } + if result.SavingsPlansPurchaseRecommendation != nil { recs := c.parseSavingsPlansRecommendations(result.SavingsPlansPurchaseRecommendation, params, planType) - allRecommendations = append(allRecommendations, recs...) + allRecs = append(allRecs, recs...) + } + + if result.NextPageToken == nil || aws.ToString(result.NextPageToken) == "" { + break } + nextPageToken = result.NextPageToken } - return allRecommendations, nil + return allRecs, nil +} + +// fetchSPPageWithRetry executes a single GetSavingsPlansPurchaseRecommendation +// call with rate-limiter exponential back-off. Extracted so the pagination loop +// in fetchSPAllPages stays below the gocyclo cap. +func (c *Client) fetchSPPageWithRetry( + ctx context.Context, + input *costexplorer.GetSavingsPlansPurchaseRecommendationInput, +) (*costexplorer.GetSavingsPlansPurchaseRecommendationOutput, error) { + c.rateLimiter.Reset() + var result *costexplorer.GetSavingsPlansPurchaseRecommendationOutput + var err error + + for { + if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { + return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) + } + + if acqErr := concurrency.Acquire(ctx); acqErr != nil { + return nil, fmt.Errorf("concurrency acquire failed: %w", acqErr) + } + result, err = c.costExplorerClient.GetSavingsPlansPurchaseRecommendation(ctx, input) + concurrency.Release(ctx) + if !c.rateLimiter.ShouldRetry(err) { + break + } + } + + if err != nil { + return nil, err + } + + return result, nil } // parseSavingsPlansRecommendations converts Savings Plans recommendations diff --git a/providers/aws/recommendations/parser_sp_additional_test.go b/providers/aws/recommendations/parser_sp_additional_test.go index 68e6331d0..cbe28a128 100644 --- a/providers/aws/recommendations/parser_sp_additional_test.go +++ b/providers/aws/recommendations/parser_sp_additional_test.go @@ -2,6 +2,7 @@ package recommendations import ( "context" + "fmt" "testing" "github.com/aws/aws-sdk-go-v2/aws" @@ -361,3 +362,175 @@ func TestParseSavingsPlanDetail_OnDemandCost(t *testing.T) { }) } } + +// spDetail returns a minimal SavingsPlansPurchaseRecommendationDetail for pagination tests. +func spDetail(n int) []types.SavingsPlansPurchaseRecommendationDetail { + details := make([]types.SavingsPlansPurchaseRecommendationDetail, n) + for i := range details { + details[i] = types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("1.00"), + EstimatedMonthlySavingsAmount: aws.String("50.00"), + EstimatedSavingsPercentage: aws.String("20.0"), + } + } + return details +} + +func spOutput(n int, nextToken *string) *costexplorer.GetSavingsPlansPurchaseRecommendationOutput { + return &costexplorer.GetSavingsPlansPurchaseRecommendationOutput{ + SavingsPlansPurchaseRecommendation: &types.SavingsPlansPurchaseRecommendation{ + SavingsPlansPurchaseRecommendationDetails: spDetail(n), + }, + NextPageToken: nextToken, + } +} + +// multiPageSPMock returns distinct pages for GetSavingsPlansPurchaseRecommendation +// based on the NextPageToken in the incoming request. +type multiPageSPMock struct { + pages []*costexplorer.GetSavingsPlansPurchaseRecommendationOutput + tokens []string // tokens[i] triggers pages[i+1] + calls int +} + +func (m *multiPageSPMock) GetReservationPurchaseRecommendation( + _ context.Context, _ *costexplorer.GetReservationPurchaseRecommendationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { + return &costexplorer.GetReservationPurchaseRecommendationOutput{}, nil +} + +func (m *multiPageSPMock) GetSavingsPlansPurchaseRecommendation( + _ context.Context, + params *costexplorer.GetSavingsPlansPurchaseRecommendationInput, + _ ...func(*costexplorer.Options), +) (*costexplorer.GetSavingsPlansPurchaseRecommendationOutput, error) { + idx := 0 + incoming := aws.ToString(params.NextPageToken) + for i, tok := range m.tokens { + if tok == incoming { + idx = i + 1 + break + } + } + if incoming == "" { + idx = 0 + } + m.calls++ + if idx >= len(m.pages) { + return nil, fmt.Errorf("unexpected SP page token %q", incoming) + } + return m.pages[idx], nil +} + +func (m *multiPageSPMock) GetReservationUtilization( + _ context.Context, _ *costexplorer.GetReservationUtilizationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationUtilizationOutput, error) { + return &costexplorer.GetReservationUtilizationOutput{}, nil +} + +func (m *multiPageSPMock) GetReservationCoverage( + _ context.Context, _ *costexplorer.GetReservationCoverageInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationCoverageOutput, error) { + return &costexplorer.GetReservationCoverageOutput{}, nil +} + +// alwaysNextPageSPMock returns pages each carrying a non-nil non-empty NextPageToken. +type alwaysNextPageSPMock struct { + calls int +} + +func (m *alwaysNextPageSPMock) GetReservationPurchaseRecommendation( + _ context.Context, _ *costexplorer.GetReservationPurchaseRecommendationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { + return &costexplorer.GetReservationPurchaseRecommendationOutput{}, nil +} + +func (m *alwaysNextPageSPMock) GetSavingsPlansPurchaseRecommendation( + _ context.Context, + _ *costexplorer.GetSavingsPlansPurchaseRecommendationInput, + _ ...func(*costexplorer.Options), +) (*costexplorer.GetSavingsPlansPurchaseRecommendationOutput, error) { + m.calls++ + return spOutput(1, aws.String(fmt.Sprintf("tok%d", m.calls))), nil +} + +func (m *alwaysNextPageSPMock) GetReservationUtilization( + _ context.Context, _ *costexplorer.GetReservationUtilizationInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationUtilizationOutput, error) { + return &costexplorer.GetReservationUtilizationOutput{}, nil +} + +func (m *alwaysNextPageSPMock) GetReservationCoverage( + _ context.Context, _ *costexplorer.GetReservationCoverageInput, _ ...func(*costexplorer.Options), +) (*costexplorer.GetReservationCoverageOutput, error) { + return &costexplorer.GetReservationCoverageOutput{}, nil +} + +// TestGetSavingsPlansRecommendations_Paginates asserts multi-page accumulation (issue #692). +func TestGetSavingsPlansRecommendations_Paginates(t *testing.T) { + mock := &multiPageSPMock{ + pages: []*costexplorer.GetSavingsPlansPurchaseRecommendationOutput{ + spOutput(2, aws.String("tok1")), + spOutput(3, aws.String("tok2")), + spOutput(4, nil), + }, + tokens: []string{"tok1", "tok2"}, + } + + client := NewClientWithAPI(mock, "us-east-1") + params := common.RecommendationParams{ + Service: common.ServiceSavingsPlansCompute, + PaymentOption: "partial-upfront", + Term: "1yr", + LookbackPeriod: "7d", + } + + recs, err := client.getSavingsPlansRecommendations(context.Background(), params) + require.NoError(t, err) + // 2 + 3 + 4 = 9 detail items + assert.Len(t, recs, 9, "must accumulate recs across all 3 SP pages") + assert.Equal(t, 3, mock.calls, "must call CE exactly once per page") +} + +// TestGetSavingsPlansRecommendations_EmptyTokenTerminates asserts that an +// empty-string NextPageToken is treated as terminal (parity with PR #690). +func TestGetSavingsPlansRecommendations_EmptyTokenTerminates(t *testing.T) { + mock := &multiPageSPMock{ + pages: []*costexplorer.GetSavingsPlansPurchaseRecommendationOutput{ + spOutput(2, aws.String("")), // empty string -- must terminate + }, + tokens: []string{}, + } + + client := NewClientWithAPI(mock, "us-east-1") + params := common.RecommendationParams{ + Service: common.ServiceSavingsPlansCompute, + PaymentOption: "partial-upfront", + Term: "1yr", + LookbackPeriod: "7d", + } + + recs, err := client.getSavingsPlansRecommendations(context.Background(), params) + require.NoError(t, err) + assert.Len(t, recs, 2) + assert.Equal(t, 1, mock.calls, "empty-string token must terminate pagination after page 1") +} + +// TestGetSavingsPlansRecommendations_PaginationCapError asserts that exceeding +// maxRecommendationPages returns a diagnostic error (issue #692). +func TestGetSavingsPlansRecommendations_PaginationCapError(t *testing.T) { + mock := &alwaysNextPageSPMock{} + client := NewClientWithAPI(mock, "us-east-1") + params := common.RecommendationParams{ + Service: common.ServiceSavingsPlansCompute, + PaymentOption: "partial-upfront", + Term: "1yr", + LookbackPeriod: "7d", + } + + _, err := client.getSavingsPlansRecommendations(context.Background(), params) + require.Error(t, err) + assert.Contains(t, err.Error(), "pagination cap reached") + assert.Equal(t, maxRecommendationPages, mock.calls, + "must stop exactly at the cap") +}