From dc8dab97b4dd1fb97253b9c718703f2a8c518ad7 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 20:48:02 +0200 Subject: [PATCH 1/3] fix(recommendations): switch to pointer params and shared rate limiter Change GetRecommendations signature to *common.RecommendationParams across all AWS/Azure/GCP providers, and replace the per-call newRateLimiter factory on the AWS recommendations Client with a single shared *RateLimiter reset before each call. Update all callers, mocks, and tests to match. Fixes type mismatches introduced when origin/main moved after this branch diverged: serviceRecsGetter interface updated to pointer params, fakeServiceClient in azure/recommendations_test.go updated, and scheduler test matcher changed from AnythingOfType("common.RecommendationParams") to pointer type. Preserves the always-empty GetRecommendations for Azure Search (H1 regression test) while keeping convertAzureSearchRecommendation for GetOfferingDetails. --- cmd/multi_service_coverage_test.go | 35 ++-------- cmd/multi_service_helpers.go | 2 +- cmd/multi_service_test.go | 64 +++---------------- cmd/multi_service_test_common_test.go | 4 +- internal/purchase/mocks_test.go | 2 +- internal/scheduler/scheduler.go | 7 +- internal/scheduler/scheduler_test.go | 6 +- pkg/provider/interface.go | 4 +- providers/aws/recommendations/client.go | 49 +++++++------- providers/aws/recommendations/client_test.go | 34 +++++----- providers/aws/recommendations/coverage.go | 6 +- .../aws/recommendations/ondemand_series.go | 6 +- providers/aws/recommendations/parser_sp.go | 6 +- providers/aws/recommendations/sku.go | 21 +++--- providers/aws/recommendations/sku_test.go | 31 ++++++--- providers/aws/recommendations/sp_coverage.go | 28 ++++---- providers/aws/recommendations/utilization.go | 6 +- providers/aws/service_client.go | 4 +- providers/aws/service_client_test.go | 6 +- providers/aws/services/ec2/client.go | 2 +- providers/aws/services/ec2/client_test.go | 2 +- providers/aws/services/elasticache/client.go | 2 +- .../aws/services/elasticache/client_test.go | 2 +- providers/aws/services/memorydb/client.go | 2 +- .../aws/services/memorydb/client_test.go | 2 +- providers/aws/services/opensearch/client.go | 2 +- .../aws/services/opensearch/client_test.go | 2 +- providers/aws/services/rds/client.go | 2 +- providers/aws/services/rds/client_test.go | 2 +- providers/aws/services/redshift/client.go | 2 +- .../aws/services/redshift/client_test.go | 2 +- providers/aws/services/savingsplans/client.go | 2 +- .../aws/services/savingsplans/client_test.go | 2 +- providers/azure/recommendations.go | 21 +++--- providers/azure/recommendations_test.go | 11 ++-- providers/azure/services/cache/client.go | 2 +- providers/azure/services/cache/client_test.go | 4 +- providers/azure/services/compute/client.go | 2 +- .../azure/services/compute/client_test.go | 4 +- providers/azure/services/cosmosdb/client.go | 2 +- .../azure/services/cosmosdb/client_test.go | 6 +- providers/azure/services/database/client.go | 2 +- .../azure/services/database/client_test.go | 4 +- .../azure/services/managedredis/client.go | 2 +- .../services/managedredis/client_test.go | 4 +- .../azure/services/savingsplans/client.go | 2 +- .../services/savingsplans/client_test.go | 2 +- providers/azure/services/search/client.go | 29 ++++----- .../azure/services/search/client_test.go | 4 +- providers/azure/services/synapse/client.go | 2 +- .../azure/services/synapse/client_test.go | 12 ++-- providers/gcp/recommendations.go | 18 ++++-- providers/gcp/recommendations_test.go | 2 +- providers/gcp/services/cloudsql/client.go | 32 ++++++---- .../gcp/services/cloudsql/client_test.go | 6 +- providers/gcp/services/cloudstorage/client.go | 32 ++++++---- .../gcp/services/cloudstorage/client_test.go | 6 +- .../gcp/services/computeengine/client.go | 32 ++++++---- .../gcp/services/computeengine/client_test.go | 16 ++--- providers/gcp/services/memorystore/client.go | 29 ++++++--- .../gcp/services/memorystore/client_test.go | 2 +- 61 files changed, 312 insertions(+), 327 deletions(-) diff --git a/cmd/multi_service_coverage_test.go b/cmd/multi_service_coverage_test.go index 1de248cc4..1adebebea 100644 --- a/cmd/multi_service_coverage_test.go +++ b/cmd/multi_service_coverage_test.go @@ -12,6 +12,7 @@ import ( 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/mock" "github.com/stretchr/testify/require" ) @@ -638,16 +639,7 @@ func TestProcessService_GetRegionsError(t *testing.T) { toolCfg.Regions = []string{"us-east-1"} // Setup mock to return empty recommendations - params := common.RecommendationParams{ - Service: common.ServiceRDS, - Region: "us-east-1", - PaymentOption: "all-upfront", - Term: "3yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } - mockClient.On("GetRecommendations", ctx, params).Return([]common.Recommendation{}, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return([]common.Recommendation{}, nil) recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{}) @@ -674,16 +666,7 @@ func TestProcessService_GetRecommendationsError(t *testing.T) { accountCache := NewAccountAliasCache(awsCfg) // Setup mock to return error - params := common.RecommendationParams{ - Service: common.ServiceEC2, - Region: "us-east-1", - PaymentOption: "partial-upfront", - Term: "1yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } - mockClient.On("GetRecommendations", ctx, params).Return([]common.Recommendation(nil), errors.New("API error")) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return([]common.Recommendation(nil), errors.New("API error")) recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceEC2, true, toolCfg, engineVersionData{}) @@ -710,22 +693,12 @@ func TestProcessService_AllRecommendationsFilteredOut(t *testing.T) { mockClient := &MockRecommendationsClient{} accountCache := NewAccountAliasCache(awsCfg) - params := common.RecommendationParams{ - Service: common.ServiceRDS, - Region: "us-east-1", - PaymentOption: "no-upfront", - Term: "1yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } - // Return recommendations that don't match the filter mockRecs := []common.Recommendation{ {ResourceType: "db.t3.small", Count: 5, Region: "us-east-1", EstimatedSavings: 100}, {ResourceType: "db.t3.medium", Count: 3, Region: "us-east-1", EstimatedSavings: 200}, } - mockClient.On("GetRecommendations", ctx, params).Return(mockRecs, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil) recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{}) diff --git a/cmd/multi_service_helpers.go b/cmd/multi_service_helpers.go index dd9e75542..aa57122e2 100644 --- a/cmd/multi_service_helpers.go +++ b/cmd/multi_service_helpers.go @@ -438,7 +438,7 @@ func fetchRecommendationsForRegion( ExcludeSPTypes: cfg.ExcludeSPTypes, } - recs, err := recClient.GetRecommendations(ctx, params) + recs, err := recClient.GetRecommendations(ctx, ¶ms) if err != nil { AppLogger.Printf(" ❌ Failed to fetch recommendations: %v\n", err) return nil diff --git a/cmd/multi_service_test.go b/cmd/multi_service_test.go index 38b57358e..7826496b3 100644 --- a/cmd/multi_service_test.go +++ b/cmd/multi_service_test.go @@ -272,21 +272,8 @@ func TestProcessServiceWithMocks(t *testing.T) { mockClient := &MockRecommendationsClient{} // Setup expectations - termStr := "1yr" - if toolCfg.TermYears == 3 { - termStr = "3yr" - } - for _, region := range tt.testRegions { - params := common.RecommendationParams{ - Service: tt.service, - Region: region, - PaymentOption: toolCfg.PaymentOption, - Term: termStr, - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } - mockClient.On("GetRecommendations", ctx, params).Return(tt.mockRecs, nil) + for range tt.testRegions { + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(tt.mockRecs, nil) } // Set regions in toolCfg for this test @@ -342,19 +329,10 @@ func TestProcessService_SavingsPlansAccountLevel(t *testing.T) { // Savings Plans should only query us-east-1 once (account-level). Use the // per-plan-type Compute slug now that the legacy umbrella has been // retired from createServiceClient dispatch. - params := common.RecommendationParams{ - Service: common.ServiceSavingsPlansCompute, - Region: "us-east-1", - PaymentOption: "all-upfront", - Term: "3yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } mockRecs := []common.Recommendation{ {Service: common.ServiceSavingsPlansCompute, ResourceType: "ComputeSP", Count: 1, Region: "us-east-1", EstimatedSavings: 1000}, } - mockClient.On("GetRecommendations", ctx, params).Return(mockRecs, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil) accountCache := NewAccountAliasCache(awsCfg) recs, results := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceSavingsPlansCompute, true, toolCfg, engineVersionData{}) @@ -388,20 +366,11 @@ func TestProcessService_WithInstanceLimit(t *testing.T) { mockClient := &MockRecommendationsClient{} - params := common.RecommendationParams{ - Service: common.ServiceRDS, - Region: "us-east-1", - PaymentOption: "partial-upfront", - Term: "1yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } mockRecs := []common.Recommendation{ {ResourceType: "db.t3.micro", Count: 10, Region: "us-east-1", EstimatedSavings: 100}, {ResourceType: "db.t3.small", Count: 10, Region: "us-east-1", EstimatedSavings: 200}, } - mockClient.On("GetRecommendations", ctx, params).Return(mockRecs, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil) accountCache := NewAccountAliasCache(awsCfg) recs, _ := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceRDS, true, toolCfg, engineVersionData{}) @@ -430,20 +399,11 @@ func TestProcessService_WithOverrideCount(t *testing.T) { mockClient := &MockRecommendationsClient{} - params := common.RecommendationParams{ - Service: common.ServiceElastiCache, - Region: "us-east-1", - PaymentOption: "no-upfront", - Term: "1yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } mockRecs := []common.Recommendation{ {ResourceType: "cache.t3.micro", Count: 10, Region: "us-east-1", EstimatedSavings: 100}, {ResourceType: "cache.t3.small", Count: 5, Region: "us-east-1", EstimatedSavings: 200}, } - mockClient.On("GetRecommendations", ctx, params).Return(mockRecs, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil) accountCache := NewAccountAliasCache(awsCfg) recs, _ := processService(ctx, awsCfg, mockClient, accountCache, common.ServiceElastiCache, true, toolCfg, engineVersionData{}) @@ -470,21 +430,13 @@ func TestProcessService_MultipleRegions(t *testing.T) { mockClient := &MockRecommendationsClient{} - // Setup mock for each region + // Setup mock for each region call in order; Once() ensures testify cycles + // through the returns so each region's call gets region-appropriate results. for _, region := range toolCfg.Regions { - params := common.RecommendationParams{ - Service: common.ServiceRDS, - Region: region, - PaymentOption: "all-upfront", - Term: "3yr", - LookbackPeriod: "7d", - IncludeSPTypes: toolCfg.IncludeSPTypes, - ExcludeSPTypes: toolCfg.ExcludeSPTypes, - } mockRecs := []common.Recommendation{ {ResourceType: "db.t3.small", Count: 2, Region: region, EstimatedSavings: 100}, } - mockClient.On("GetRecommendations", ctx, params).Return(mockRecs, nil) + mockClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(mockRecs, nil).Once() } accountCache := NewAccountAliasCache(awsCfg) diff --git a/cmd/multi_service_test_common_test.go b/cmd/multi_service_test_common_test.go index b77d5bf4f..24097be8d 100644 --- a/cmd/multi_service_test_common_test.go +++ b/cmd/multi_service_test_common_test.go @@ -31,7 +31,7 @@ type MockRecommendationsClient struct { mock.Mock } -func (m *MockRecommendationsClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (m *MockRecommendationsClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { args := m.Called(ctx, params) if args.Get(0) == nil { return nil, args.Error(1) @@ -70,7 +70,7 @@ func (m *MockServiceClient) GetRegion() string { return args.String(0) } -func (m *MockServiceClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (m *MockServiceClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { args := m.Called(ctx, params) if args.Get(0) == nil { return nil, args.Error(1) diff --git a/internal/purchase/mocks_test.go b/internal/purchase/mocks_test.go index aeb1047f7..9d82f253e 100644 --- a/internal/purchase/mocks_test.go +++ b/internal/purchase/mocks_test.go @@ -110,7 +110,7 @@ func (m *MockServiceClient) PurchaseCommitment(ctx context.Context, rec common.R return args.Get(0).(common.PurchaseResult), args.Error(1) } -func (m *MockServiceClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (m *MockServiceClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { args := m.Called(ctx, params) if args.Get(0) == nil { return nil, args.Error(1) diff --git a/internal/scheduler/scheduler.go b/internal/scheduler/scheduler.go index 5cde7ec9f..19edbd007 100644 --- a/internal/scheduler/scheduler.go +++ b/internal/scheduler/scheduler.go @@ -879,13 +879,14 @@ func (s *Scheduler) fetchAndConvert(ctx context.Context, prov provider.Provider, PaymentOption: globalCfg.DefaultPayment, LookbackPeriod: fmt.Sprintf("%dd", lookbackDays), } - recs, err = recClient.GetRecommendations(ctx, params) - if err != nil { + var recErr error + recs, recErr = recClient.GetRecommendations(ctx, ¶ms) + if recErr != nil { // Fail loud: a misconfigured DefaultPayment/DefaultTerm or a CE // failure on this fallback must surface to the operator instead // of silently presenting as "zero recommendations". return nil, fmt.Errorf("failed to get %s recommendations with default term/payment fallback (term=%s, payment=%s, lookback=%s): %w", - providerName, params.Term, params.PaymentOption, params.LookbackPeriod, err) + providerName, params.Term, params.PaymentOption, params.LookbackPeriod, recErr) } } result := s.convertRecommendations(recs, providerName) diff --git a/internal/scheduler/scheduler_test.go b/internal/scheduler/scheduler_test.go index 0fc012ca0..cc24576e7 100644 --- a/internal/scheduler/scheduler_test.go +++ b/internal/scheduler/scheduler_test.go @@ -701,7 +701,7 @@ type MockRecommendationsClient struct { mock.Mock } -func (m *MockRecommendationsClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (m *MockRecommendationsClient) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { args := m.Called(ctx, params) if args.Get(0) == nil { return nil, args.Error(1) @@ -1630,7 +1630,7 @@ func TestScheduler_CollectAWSRecommendations_FallbackToFiltered(t *testing.T) { mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil) mockProvider.On("GetRecommendationsClient", ctx).Return(mockRecClient, nil) mockRecClient.On("GetAllRecommendations", ctx).Return([]common.Recommendation{}, nil) // Empty - mockRecClient.On("GetRecommendations", ctx, mock.AnythingOfType("common.RecommendationParams")).Return(filteredRecommendations, nil) + mockRecClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(filteredRecommendations, nil) scheduler := &Scheduler{ config: mockStore, @@ -1668,7 +1668,7 @@ func TestScheduler_CollectAWSRecommendations_FallbackError(t *testing.T) { mockFactory.On("CreateAndValidateProvider", mock.Anything, "aws", mock.Anything).Return(mockProvider, nil) mockProvider.On("GetRecommendationsClient", ctx).Return(mockRecClient, nil) mockRecClient.On("GetAllRecommendations", ctx).Return([]common.Recommendation{}, nil) // Empty -> triggers fallback - mockRecClient.On("GetRecommendations", ctx, mock.AnythingOfType("common.RecommendationParams")).Return(nil, fallbackErr) + mockRecClient.On("GetRecommendations", ctx, mock.AnythingOfType("*common.RecommendationParams")).Return(nil, fallbackErr) scheduler := &Scheduler{ config: mockStore, diff --git a/pkg/provider/interface.go b/pkg/provider/interface.go index f2a02159c..5554fd2fe 100644 --- a/pkg/provider/interface.go +++ b/pkg/provider/interface.go @@ -41,7 +41,7 @@ type ServiceClient interface { GetRegion() string // Recommendations - GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) + GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) // Commitments (RI/SP/CUD/etc) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) @@ -56,7 +56,7 @@ type ServiceClient interface { // RecommendationsClient provides centralized recommendations across all services type RecommendationsClient interface { // Get recommendations with filtering - GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) + GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) // Get recommendations for a specific service GetRecommendationsForService(ctx context.Context, service common.ServiceType) ([]common.Recommendation, error) diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index fc46f18f5..a9305d234 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -49,21 +49,20 @@ type CostExplorerAPI interface { // Client wraps the AWS Cost Explorer client for RI recommendations. type Client struct { costExplorerClient CostExplorerAPI - region string - - // newRateLimiter is called once per API call (not shared across goroutines). - // Tests can replace it with a factory returning a faster limiter. - newRateLimiter func() *RateLimiter // ec2API is the EC2 client used to build the DescribeInstanceTypes paginator. // Populated by NewClient from aws.Config; nil when created via NewClientWithAPI. ec2API DescribeInstanceTypesAPI + rateLimiter *RateLimiter + // instanceTypePagerFactory creates a new InstanceTypePager on demand. // Set by NewClient to wrap ec2API; overridable via SetInstanceTypePagerFactory // for hermetic tests. When nil, instanceTypeLookup returns (0,0). instanceTypePagerFactory func() InstanceTypePager + region string + // skuCatalog caches the per-instance-type vCPU/memory catalog, fetched // lazily once per Client lifetime via sync.Once (one DescribeInstanceTypes // fan-out per scheduler tick). @@ -85,7 +84,7 @@ func NewClient(cfg *aws.Config) *Client { return &Client{ costExplorerClient: costexplorer.NewFromConfig(ceConfig), region: cfg.Region, - newRateLimiter: NewRateLimiter, + rateLimiter: NewRateLimiter(), ec2API: ec2Client, // Factory wraps the EC2 client so the paginator is created lazily // on the first EC2 recommendation parse (not at construction time). @@ -100,7 +99,7 @@ func NewClientWithAPI(api CostExplorerAPI, region string) *Client { return &Client{ costExplorerClient: api, region: region, - newRateLimiter: NewRateLimiter, + rateLimiter: NewRateLimiter(), // ec2API left nil: instanceTypeLookup falls back to VCPU=0/MemoryGB=0 // unless the caller sets instanceTypePagerFactory. } @@ -117,7 +116,7 @@ func (c *Client) SetInstanceTypePagerFactory(f func() InstanceTypePager) { // instanceTypeLookup returns the cached SKU entry for instanceType. // On the first call the catalog is built by calling the pager factory. // ok=false when no factory is configured, the catalog fetch failed, or -// the instance type was not in the catalog - the caller falls back to +// the instance type was not in the catalog -- the caller falls back to // VCPU=0/MemoryGB=0 (graceful-degradation contract from Azure PR #810). func (c *Client) instanceTypeLookup(ctx context.Context, instanceType string) (instanceTypeSKUEntry, bool) { if c.instanceTypePagerFactory == nil { @@ -134,14 +133,14 @@ func (c *Client) SetRecLookbackPeriod(period string) { } // GetRecommendations fetches Reserved Instance recommendations for any service. -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { // Handle Savings Plans separately — they use a different Cost Explorer API // (GetSavingsPlansPurchaseRecommendation, not GetReservationPurchaseRecommendation). // Match any SP slug — the legacy umbrella plus the four per-plan-type slugs — // via the IsSavingsPlan family predicate so the dispatch keeps working as // callers migrate. if common.IsSavingsPlan(params.Service) { - return c.getSavingsPlansRecommendations(ctx, ¶ms) + return c.getSavingsPlansRecommendations(ctx, params) } input := &costexplorer.GetReservationPurchaseRecommendationInput{ @@ -157,7 +156,7 @@ func (c *Client) GetRecommendations(ctx context.Context, params common.Recommend return nil, err } - return c.parseRecommendations(ctx, allRecs, params) + return c.parseRecommendations(ctx, allRecs, *params) } // fetchRIAllPages paginates over all pages of RI recommendations for a single @@ -207,12 +206,12 @@ func (c *Client) fetchRIPageWithRetry( ctx context.Context, input *costexplorer.GetReservationPurchaseRecommendationInput, ) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { - rl := c.newRateLimiter() + c.rateLimiter.Reset() var result *costexplorer.GetReservationPurchaseRecommendationOutput var err error for { - if waitErr := rl.Wait(ctx); waitErr != nil { + if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) } @@ -221,13 +220,13 @@ func (c *Client) fetchRIPageWithRetry( } result, err = c.costExplorerClient.GetReservationPurchaseRecommendation(ctx, input) concurrency.Release(ctx) - if !rl.ShouldRetry(err) { + if !c.rateLimiter.ShouldRetry(err) { break } } if err != nil { - return nil, fmt.Errorf("failed to get RI recommendations after %d retries: %w", rl.GetRetryCount(), err) + return nil, fmt.Errorf("failed to get RI recommendations after %d retries: %w", c.rateLimiter.GetRetryCount(), err) } return result, nil @@ -276,7 +275,7 @@ func (c *Client) fetchSingleComboRecs(ctx context.Context, service common.Servic LookbackPeriod: lookback, Region: "", } - recs, err := c.GetRecommendations(ctx, params) + recs, err := c.GetRecommendations(ctx, ¶ms) if err != nil { // A canceled / deadline-exceeded ctx is NOT a per-combo // failure to be tolerated -- every subsequent combo @@ -429,9 +428,9 @@ func (c *Client) GetAllRecommendations(ctx context.Context) ([]common.Recommenda // stays under the gocyclo gate (.golangci.yml min-complexity: 15) after the // post-Wait ctx.Err() block was added. type serviceResult struct { + err error name string recs []common.Recommendation - err error } // mergeServiceResults logs per-service errors at WARN and appends successful @@ -452,20 +451,20 @@ func mergeServiceResults(results ...serviceResult) ([]common.Recommendation, err total := 0 failures := 0 var lastErr error - for _, r := range results { - total += len(r.recs) - if r.err != nil { + for i := range results { + total += len(results[i].recs) + if results[i].err != nil { failures++ - lastErr = r.err + lastErr = results[i].err } } out := make([]common.Recommendation, 0, total) - for _, r := range results { - if r.err != nil { - logging.Warnf("AWS %s recommendations: %v", r.name, r.err) + for i := range results { + if results[i].err != nil { + logging.Warnf("AWS %s recommendations: %v", results[i].name, results[i].err) continue } - out = append(out, r.recs...) + out = append(out, results[i].recs...) } if failures == len(results) && failures > 0 { return nil, fmt.Errorf("all %d AWS recommendation services failed: %w", failures, lastErr) diff --git a/providers/aws/recommendations/client_test.go b/providers/aws/recommendations/client_test.go index fb4fef8ce..44c000233 100644 --- a/providers/aws/recommendations/client_test.go +++ b/providers/aws/recommendations/client_test.go @@ -83,7 +83,7 @@ func TestNewClient(t *testing.T) { assert.NotNil(t, client) assert.NotNil(t, client.costExplorerClient) - assert.NotNil(t, client.newRateLimiter) + assert.NotNil(t, client.rateLimiter) assert.Equal(t, "us-west-2", client.region) } @@ -96,7 +96,7 @@ func TestNewClientWithAPI(t *testing.T) { assert.NotNil(t, client) assert.Equal(t, mockAPI, client.costExplorerClient) assert.Equal(t, region, client.region) - assert.NotNil(t, client.newRateLimiter) + assert.NotNil(t, client.rateLimiter) } func TestGetRecommendations_EC2_Success(t *testing.T) { @@ -134,7 +134,7 @@ func TestGetRecommendations_EC2_Success(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Len(t, recs, 1) @@ -178,7 +178,7 @@ func TestGetRecommendations_RDS_Success(t *testing.T) { LookbackPeriod: "30d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Len(t, recs, 1) @@ -224,7 +224,7 @@ func TestGetRecommendations_ElastiCache_Success(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Len(t, recs, 1) @@ -263,7 +263,7 @@ func TestGetRecommendations_SavingsPlans_Success(t *testing.T) { IncludeSPTypes: []string{"Compute"}, } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Len(t, recs, 1) @@ -284,11 +284,9 @@ func TestGetRecommendations_Error(t *testing.T) { riError: newThrottleError(), } - // Use custom rate limiter factory to speed up test + // Use custom rate limiter to speed up test client := NewClientWithAPI(mockAPI, "us-east-1") - client.newRateLimiter = func() *RateLimiter { - return NewRateLimiterWithOptions(1*time.Millisecond, 10*time.Millisecond, 2) - } + client.rateLimiter = NewRateLimiterWithOptions(1*time.Millisecond, 10*time.Millisecond, 2) params := common.RecommendationParams{ Service: common.ServiceEC2, @@ -297,7 +295,7 @@ func TestGetRecommendations_Error(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) assert.Error(t, err) assert.Nil(t, recs) @@ -321,7 +319,7 @@ func TestGetRecommendations_EmptyResult(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Empty(t, recs) @@ -530,9 +528,7 @@ func TestGetRecommendations_ContextCancellation(t *testing.T) { } client := NewClientWithAPI(mockAPI, "us-east-1") - client.newRateLimiter = func() *RateLimiter { - return NewRateLimiterWithOptions(100*time.Millisecond, 1*time.Second, 5) - } + client.rateLimiter = NewRateLimiterWithOptions(100*time.Millisecond, 1*time.Second, 5) ctx, cancel := context.WithCancel(context.Background()) cancel() // Cancel immediately @@ -544,7 +540,7 @@ func TestGetRecommendations_ContextCancellation(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(ctx, params) + recs, err := client.GetRecommendations(ctx, ¶ms) // 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 @@ -774,7 +770,7 @@ func TestGetRecommendations_RI_Paginates(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) 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") @@ -803,7 +799,7 @@ func TestGetRecommendations_RI_EmptyTokenTerminates(t *testing.T) { LookbackPeriod: "7d", } - recs, err := client.GetRecommendations(context.Background(), params) + recs, err := client.GetRecommendations(context.Background(), ¶ms) require.NoError(t, err) assert.Len(t, recs, 1) assert.Equal(t, 1, mock.calls, "empty-string token must terminate pagination after page 1") @@ -875,7 +871,7 @@ func TestGetRecommendations_RI_PaginationCapError(t *testing.T) { LookbackPeriod: "7d", } - _, err := client.GetRecommendations(context.Background(), params) + _, err := client.GetRecommendations(context.Background(), ¶ms) require.Error(t, err) assert.Contains(t, err.Error(), "pagination cap reached") assert.Equal(t, maxRecommendationPages, mock.calls, diff --git a/providers/aws/recommendations/coverage.go b/providers/aws/recommendations/coverage.go index d45b519e9..6c7ac438f 100644 --- a/providers/aws/recommendations/coverage.go +++ b/providers/aws/recommendations/coverage.go @@ -324,9 +324,9 @@ func serviceRegionFilter(service, region string) *types.Expression { // held only across the outbound SDK call, matching fetchRIPageWithRetry, // so the coverage-enrichment fan-out stays inside the global IO cap. func (c *Client) fetchCoveragePage(ctx context.Context, input *costexplorer.GetReservationCoverageInput) (*costexplorer.GetReservationCoverageOutput, error) { - rl := c.newRateLimiter() + c.rateLimiter.Reset() for { - if waitErr := rl.Wait(ctx); waitErr != nil { + 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 { @@ -334,7 +334,7 @@ func (c *Client) fetchCoveragePage(ctx context.Context, input *costexplorer.GetR } result, err := c.costExplorerClient.GetReservationCoverage(ctx, input) concurrency.Release(ctx) - if !rl.ShouldRetry(err) { + if !c.rateLimiter.ShouldRetry(err) { if err != nil { return nil, fmt.Errorf("failed to get reservation coverage: %w", err) } diff --git a/providers/aws/recommendations/ondemand_series.go b/providers/aws/recommendations/ondemand_series.go index d6b09dc5c..1ea1a6a34 100644 --- a/providers/aws/recommendations/ondemand_series.go +++ b/providers/aws/recommendations/ondemand_series.go @@ -174,13 +174,13 @@ func validateOnDemandSeriesArgs(region string, lookbackDays int) error { // utilization.go. Each attempt acquires one rate-limiter slot at the API call // site (not at goroutine creation) per feedback_semaphore_at_api_call. func (c *Client) fetchOnDemandPage(ctx context.Context, input *costexplorer.GetCostAndUsageInput) (*costexplorer.GetCostAndUsageOutput, error) { - rl := c.newRateLimiter() + c.rateLimiter.Reset() for { - if waitErr := rl.Wait(ctx); waitErr != nil { + if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait: %w", waitErr) } out, err := c.costExplorerClient.GetCostAndUsage(ctx, input) - if !rl.ShouldRetry(err) { + if !c.rateLimiter.ShouldRetry(err) { if err != nil { return nil, fmt.Errorf("GetCostAndUsage: %w", err) } diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index 33d977cd9..1cc03c2c0 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -133,12 +133,12 @@ func (c *Client) fetchSPPageWithRetry( ctx context.Context, input *costexplorer.GetSavingsPlansPurchaseRecommendationInput, ) (*costexplorer.GetSavingsPlansPurchaseRecommendationOutput, error) { - rl := c.newRateLimiter() + c.rateLimiter.Reset() var result *costexplorer.GetSavingsPlansPurchaseRecommendationOutput var err error for { - if waitErr := rl.Wait(ctx); waitErr != nil { + if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) } @@ -147,7 +147,7 @@ func (c *Client) fetchSPPageWithRetry( } result, err = c.costExplorerClient.GetSavingsPlansPurchaseRecommendation(ctx, input) concurrency.Release(ctx) - if !rl.ShouldRetry(err) { + if !c.rateLimiter.ShouldRetry(err) { break } } diff --git a/providers/aws/recommendations/sku.go b/providers/aws/recommendations/sku.go index d5f4f7da5..f73c2e920 100644 --- a/providers/aws/recommendations/sku.go +++ b/providers/aws/recommendations/sku.go @@ -32,17 +32,17 @@ type instanceTypeSKUEntry struct { memoryGB float64 } -// skuCatalog holds a lazily-built per-Client instance-type catalogue. -// The catalogue is fetched ONCE per client lifetime via sync.Once +// skuCatalog holds a lazily-built per-Client instance-type catalog. +// The catalog is fetched ONCE per client lifetime via sync.Once // so a single recommendations refresh issues at most one // DescribeInstanceTypes fan-out regardless of how many EC2 recs are returned. type skuCatalog struct { + m map[string]instanceTypeSKUEntry once sync.Once - m map[string]instanceTypeSKUEntry // nil means fetch failed } -// lookup returns the catalogue entry for instanceType, building the -// catalogue on the first call. ok=false on cache miss or fetch failure; +// lookup returns the catalog entry for instanceType, building the +// catalog on the first call. ok=false on cache miss or fetch failure; // the caller falls back to VCPU=0 / MemoryGB=0 and does NOT fail the // conversion (graceful-degradation contract from Azure PR #810). func (s *skuCatalog) lookup(ctx context.Context, instanceType string, newPager func() InstanceTypePager) (instanceTypeSKUEntry, bool) { @@ -65,17 +65,17 @@ func (s *skuCatalog) lookup(ctx context.Context, instanceType string, newPager f // WARN so operators can detect it. // // Any page-fetch error also returns nil (partial results are discarded so -// callers never see a half-populated catalogue). +// callers never see a half-populated catalog). func fetchInstanceTypeCatalogue(ctx context.Context, pager InstanceTypePager) map[string]instanceTypeSKUEntry { out := make(map[string]instanceTypeSKUEntry) for pager.HasMorePages() { if err := ctx.Err(); err != nil { - logging.Warnf("aws ec2: instance type catalogue fetch interrupted: %v — Details.VCPU/MemoryGB left at 0", err) + logging.Warnf("aws ec2: instance type catalog fetch interrupted: %v -- Details.VCPU/MemoryGB left at 0", err) return nil } page, err := pager.NextPage(ctx) if err != nil { - logging.Warnf("aws ec2: instance type catalogue page fetch failed: %v — Details.VCPU/MemoryGB left at 0", err) + logging.Warnf("aws ec2: instance type catalog page fetch failed: %v -- Details.VCPU/MemoryGB left at 0", err) return nil } populateInstanceTypeSKUMap(out, page.InstanceTypes) @@ -86,7 +86,8 @@ func fetchInstanceTypeCatalogue(ctx context.Context, pager InstanceTypePager) ma // populateInstanceTypeSKUMap writes one instanceTypeSKUEntry per item in // instanceTypes into out. First-write-wins on duplicate names. func populateInstanceTypeSKUMap(out map[string]instanceTypeSKUEntry, instanceTypes []ec2types.InstanceTypeInfo) { - for _, info := range instanceTypes { + for i := range instanceTypes { + info := &instanceTypes[i] name := string(info.InstanceType) if name == "" { continue @@ -101,7 +102,7 @@ func populateInstanceTypeSKUMap(out map[string]instanceTypeSKUEntry, instanceTyp // extractInstanceTypeSKUEntry reads the vCPU count and memory size from // InstanceTypeInfo. Returns (0, 0) when either field is absent or nil; // callers treat 0 as "unknown". -func extractInstanceTypeSKUEntry(info ec2types.InstanceTypeInfo) instanceTypeSKUEntry { +func extractInstanceTypeSKUEntry(info *ec2types.InstanceTypeInfo) instanceTypeSKUEntry { var vCPUs int var memoryGB float64 diff --git a/providers/aws/recommendations/sku_test.go b/providers/aws/recommendations/sku_test.go index 64842842b..54f612c46 100644 --- a/providers/aws/recommendations/sku_test.go +++ b/providers/aws/recommendations/sku_test.go @@ -45,7 +45,7 @@ func newStubPager(entries ...ec2types.InstanceTypeInfo) *stubInstanceTypePager { } // knownInstanceTypes returns a slice with two well-known instance types for -// use in tests that need a populated catalogue. +// use in tests that need a populated catalog. func knownInstanceTypes() []ec2types.InstanceTypeInfo { return []ec2types.InstanceTypeInfo{ { @@ -111,11 +111,20 @@ func TestExtractInstanceTypeSKUEntry(t *testing.T) { wantVCPU: 1, wantMemGB: 0.5, }, + { + name: "MemoryInfo present but SizeInMiB nil -- MemGB zero", + info: ec2types.InstanceTypeInfo{ + VCpuInfo: &ec2types.VCpuInfo{DefaultVCpus: aws.Int32(8)}, + MemoryInfo: &ec2types.MemoryInfo{}, + }, + wantVCPU: 8, + wantMemGB: 0.0, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - entry := extractInstanceTypeSKUEntry(tt.info) + entry := extractInstanceTypeSKUEntry(&tt.info) assert.Equal(t, tt.wantVCPU, entry.vCPUs) assert.InDelta(t, tt.wantMemGB, entry.memoryGB, 0.001) }) @@ -132,12 +141,12 @@ func TestFetchInstanceTypeCatalogue_PopulatesMap(t *testing.T) { assert.Len(t, m, 2) m5, ok := m["m5.large"] - require.True(t, ok, "m5.large must be in catalogue") + require.True(t, ok, "m5.large must be in catalog") assert.Equal(t, 2, m5.vCPUs) assert.InDelta(t, 8.0, m5.memoryGB, 0.001) r5, ok := m["r5.xlarge"] - require.True(t, ok, "r5.xlarge must be in catalogue") + require.True(t, ok, "r5.xlarge must be in catalog") assert.Equal(t, 4, r5.vCPUs) assert.InDelta(t, 32.0, r5.memoryGB, 0.001) } @@ -146,7 +155,7 @@ func TestFetchInstanceTypeCatalogue_PopulatesMap(t *testing.T) { func TestFetchInstanceTypeCatalogue_PageError(t *testing.T) { errPager := &errorOnFirstPagePager{} m := fetchInstanceTypeCatalogue(context.Background(), errPager) - assert.Nil(t, m, "catalogue must be nil on page fetch error") + assert.Nil(t, m, "catalog must be nil on page fetch error") } // TestFetchInstanceTypeCatalogue_ContextCanceled returns nil when ctx is canceled. @@ -156,7 +165,9 @@ func TestFetchInstanceTypeCatalogue_ContextCanceled(t *testing.T) { pager := newStubPager(knownInstanceTypes()...) m := fetchInstanceTypeCatalogue(ctx, pager) - assert.Nil(t, m, "catalogue must be nil when ctx is already canceled") + assert.Nil(t, m, "catalog must be nil when ctx is already canceled") + // NextPage must not have been called on a pre-canceled ctx. + assert.Equal(t, int32(0), atomic.LoadInt32(&pager.callCount)) } // TestInstanceTypeLookup_CachedOnce asserts that a single GetRecommendations @@ -183,7 +194,7 @@ func TestInstanceTypeLookup_CachedOnce(t *testing.T) { } // TestParseEC2Details_VCPUAndMemoryPopulated asserts that parseEC2Details -// enriches ComputeDetails.VCPU and MemoryGB from the catalogue. +// enriches ComputeDetails.VCPU and MemoryGB from the catalog. func TestParseEC2Details_VCPUAndMemoryPopulated(t *testing.T) { pager := newStubPager(knownInstanceTypes()...) client := NewClientWithAPI(&mockCostExplorerAPI{}, "us-east-1") @@ -228,12 +239,12 @@ func TestParseEC2Details_CatalogueMiss(t *testing.T) { rec := &common.Recommendation{} err := client.parseEC2Details(context.Background(), rec, details) - require.NoError(t, err, "catalogue miss must not fail the conversion") + require.NoError(t, err, "catalog miss must not fail the conversion") cd, ok := rec.Details.(*common.ComputeDetails) require.True(t, ok) - assert.Equal(t, 0, cd.VCPU, "VCPU must be 0 on catalogue miss") - assert.InDelta(t, 0.0, cd.MemoryGB, 0.001, "MemoryGB must be 0 on catalogue miss") + assert.Equal(t, 0, cd.VCPU, "VCPU must be 0 on catalog miss") + assert.InDelta(t, 0.0, cd.MemoryGB, 0.001, "MemoryGB must be 0 on catalog miss") } // TestParseEC2Details_NoCatalogueConfigured leaves VCPU/MemoryGB at zero diff --git a/providers/aws/recommendations/sp_coverage.go b/providers/aws/recommendations/sp_coverage.go index f72d8873b..808c25662 100644 --- a/providers/aws/recommendations/sp_coverage.go +++ b/providers/aws/recommendations/sp_coverage.go @@ -298,9 +298,11 @@ func (a *spCoverageAccumulator) summarize(windowHours float64) SPCoverageSummary // Days==0 as "no data for this scope", not "no SPs in the account", and // check Days before dereferencing the pointer fields. // -// Concurrency: each fetch builds its own RateLimiter via c.newRateLimiter() -// (see feedback_rate_limiter_per_call), so concurrent SP calls no longer -// race on a shared retry counter. +// Concurrency: Client is NOT safe for concurrent SP calls - the shared +// rateLimiter's Reset() is unsynchronized (see +// feedback_rate_limiter_per_call), so concurrent callers race on the retry +// counter. Call from a single goroutine per Client; the ladder consumer +// (PR 5) is sequential by design. func (c *Client) GetSPCoverageSummary(ctx context.Context, region string, lookbackDays int) (SPCoverageSummary, error) { if lookbackDays <= 0 { return SPCoverageSummary{}, fmt.Errorf("sp coverage: lookbackDays must be positive, got %d", lookbackDays) @@ -359,13 +361,13 @@ func (c *Client) GetSPCoverageSummary(ctx context.Context, region string, lookba // fetchSPCoveragePage calls GetSavingsPlansCoverage with rate-limit retry. // Mirrors fetchCoveragePage in coverage.go so both paths back off consistently. func (c *Client) fetchSPCoveragePage(ctx context.Context, input *costexplorer.GetSavingsPlansCoverageInput) (*costexplorer.GetSavingsPlansCoverageOutput, error) { - rl := c.newRateLimiter() + c.rateLimiter.Reset() for { - if waitErr := rl.Wait(ctx); waitErr != nil { + if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) } result, err := c.costExplorerClient.GetSavingsPlansCoverage(ctx, input) - if !rl.ShouldRetry(err) { + if !c.rateLimiter.ShouldRetry(err) { if err != nil { return nil, fmt.Errorf("failed to get SP coverage: %w", err) } @@ -410,9 +412,11 @@ func (c *Client) fetchSPCoveragePage(ctx context.Context, input *costexplorer.Ge // real GetSavingsPlansUtilization per plan type) and confirm the returned // dimension values match the map. // -// Concurrency: each fetch builds its own RateLimiter via c.newRateLimiter() -// (see feedback_rate_limiter_per_call), so concurrent SP calls no longer -// race on a shared retry counter. +// Concurrency: Client is NOT safe for concurrent SP calls - the shared +// rateLimiter's Reset() is unsynchronized (see +// feedback_rate_limiter_per_call), so concurrent callers race on the retry +// counter. Call from a single goroutine per Client; the ladder consumer +// (PR 5) is sequential by design. func (c *Client) GetSPUtilization(ctx context.Context, planType types.SupportedSavingsPlansType, region string, lookbackDays int) (SPUtilizationSummary, error) { if err := validateSPPlanType(planType); err != nil { return SPUtilizationSummary{}, fmt.Errorf("sp utilization: %w", err) @@ -448,13 +452,13 @@ func (c *Client) GetSPUtilization(ctx context.Context, planType types.SupportedS // fetchSPUtilizationPage calls GetSavingsPlansUtilization with rate-limit retry. // Mirrors fetchUtilizationPage in utilization.go so both paths back off consistently. func (c *Client) fetchSPUtilizationPage(ctx context.Context, input *costexplorer.GetSavingsPlansUtilizationInput) (*costexplorer.GetSavingsPlansUtilizationOutput, error) { - rl := c.newRateLimiter() + c.rateLimiter.Reset() for { - if waitErr := rl.Wait(ctx); waitErr != nil { + if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) } result, err := c.costExplorerClient.GetSavingsPlansUtilization(ctx, input) - if !rl.ShouldRetry(err) { + if !c.rateLimiter.ShouldRetry(err) { if err != nil { return nil, fmt.Errorf("failed to get SP utilization: %w", err) } diff --git a/providers/aws/recommendations/utilization.go b/providers/aws/recommendations/utilization.go index c7307d0bb..4fd7c8449 100644 --- a/providers/aws/recommendations/utilization.go +++ b/providers/aws/recommendations/utilization.go @@ -103,9 +103,9 @@ func buildUtilizations(agg map[string]*riAccumulator) []RIUtilization { // across the outbound SDK call, matching fetchRIPageWithRetry, so callers // running under a semaphore-carrying context stay inside the global IO cap. func (c *Client) fetchUtilizationPage(ctx context.Context, input *costexplorer.GetReservationUtilizationInput) (*costexplorer.GetReservationUtilizationOutput, error) { - rl := c.newRateLimiter() + c.rateLimiter.Reset() for { - if waitErr := rl.Wait(ctx); waitErr != nil { + if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) } @@ -114,7 +114,7 @@ func (c *Client) fetchUtilizationPage(ctx context.Context, input *costexplorer.G } result, err := c.costExplorerClient.GetReservationUtilization(ctx, input) concurrency.Release(ctx) - if !rl.ShouldRetry(err) { + if !c.rateLimiter.ShouldRetry(err) { if err != nil { return nil, fmt.Errorf("failed to get reservation utilization: %w", err) } diff --git a/providers/aws/service_client.go b/providers/aws/service_client.go index 51c9a595c..de10e47d7 100644 --- a/providers/aws/service_client.go +++ b/providers/aws/service_client.go @@ -71,13 +71,13 @@ func NewRecommendationsClient(cfg aws.Config) provider.RecommendationsClient { } // GetRecommendations gets recommendations with filtering -func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { recs, err := r.client.GetRecommendations(ctx, params) if err != nil { return nil, err } - recs = applyRecommendationFilters(recs, params) + recs = applyRecommendationFilters(recs, *params) return recs, nil } diff --git a/providers/aws/service_client_test.go b/providers/aws/service_client_test.go index 8df394714..3c86ec429 100644 --- a/providers/aws/service_client_test.go +++ b/providers/aws/service_client_test.go @@ -146,12 +146,12 @@ func TestRecommendationsClientAdapter_GetRecommendationsForService(t *testing.T) // testRecommendationsClientAdapter is a test-only version of RecommendationsClientAdapter // that uses an interface for easier mocking type testRecommendationsClientAdapter struct { - getRecommendationsFunc func(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) + getRecommendationsFunc func(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) getRecommendationsForServiceFunc func(ctx context.Context, service common.ServiceType) ([]common.Recommendation, error) getAllRecommendationsFunc func(ctx context.Context) ([]common.Recommendation, error) } -func (t *testRecommendationsClientAdapter) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (t *testRecommendationsClientAdapter) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { if t.getRecommendationsFunc != nil { return t.getRecommendationsFunc(ctx, params) } @@ -191,7 +191,7 @@ func TestRecommendationsClientAdapter_GetRecommendations_Integration(t *testing. // This will call the real adapter method which exercises the filtering code // Even though the underlying client returns no recommendations, // this test ensures the adapter's GetRecommendations method is covered - _, err := adapter.GetRecommendations(context.Background(), params) + _, err := adapter.GetRecommendations(context.Background(), ¶ms) // We expect no error even with empty results require.NoError(t, err) }) diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index ba1816ad7..9ac9e632b 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -66,7 +66,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as EC2 uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { // EC2 recommendations come from Cost Explorer API via RecommendationsClient return []common.Recommendation{}, nil } diff --git a/providers/aws/services/ec2/client_test.go b/providers/aws/services/ec2/client_test.go index 920ea0198..11689b9dc 100644 --- a/providers/aws/services/ec2/client_test.go +++ b/providers/aws/services/ec2/client_test.go @@ -143,7 +143,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { t.Parallel() client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/elasticache/client.go b/providers/aws/services/elasticache/client.go index c6d8cd16d..803b0b46c 100644 --- a/providers/aws/services/elasticache/client.go +++ b/providers/aws/services/elasticache/client.go @@ -57,7 +57,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as ElastiCache uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/elasticache/client_test.go b/providers/aws/services/elasticache/client_test.go index 1666cb2af..20d3bdd28 100644 --- a/providers/aws/services/elasticache/client_test.go +++ b/providers/aws/services/elasticache/client_test.go @@ -68,7 +68,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/memorydb/client.go b/providers/aws/services/memorydb/client.go index 567d76030..db7fa7d10 100644 --- a/providers/aws/services/memorydb/client.go +++ b/providers/aws/services/memorydb/client.go @@ -57,7 +57,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as MemoryDB uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/memorydb/client_test.go b/providers/aws/services/memorydb/client_test.go index a04ce8d60..5e10cb36c 100644 --- a/providers/aws/services/memorydb/client_test.go +++ b/providers/aws/services/memorydb/client_test.go @@ -68,7 +68,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/opensearch/client.go b/providers/aws/services/opensearch/client.go index a5a08032e..eca1fb838 100644 --- a/providers/aws/services/opensearch/client.go +++ b/providers/aws/services/opensearch/client.go @@ -76,7 +76,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as OpenSearch uses centralized Cost Explorer recommendations. -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/opensearch/client_test.go b/providers/aws/services/opensearch/client_test.go index 1e922e451..328ec41ea 100644 --- a/providers/aws/services/opensearch/client_test.go +++ b/providers/aws/services/opensearch/client_test.go @@ -92,7 +92,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/rds/client.go b/providers/aws/services/rds/client.go index 5c9b37581..78269e642 100644 --- a/providers/aws/services/rds/client.go +++ b/providers/aws/services/rds/client.go @@ -59,7 +59,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as RDS uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/rds/client_test.go b/providers/aws/services/rds/client_test.go index 434e38f19..6f383b081 100644 --- a/providers/aws/services/rds/client_test.go +++ b/providers/aws/services/rds/client_test.go @@ -69,7 +69,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/redshift/client.go b/providers/aws/services/redshift/client.go index a9d51dcc7..ba42caac6 100644 --- a/providers/aws/services/redshift/client.go +++ b/providers/aws/services/redshift/client.go @@ -80,7 +80,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as Redshift uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/redshift/client_test.go b/providers/aws/services/redshift/client_test.go index 407c5ce1d..bd0176ec7 100644 --- a/providers/aws/services/redshift/client_test.go +++ b/providers/aws/services/redshift/client_test.go @@ -97,7 +97,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/aws/services/savingsplans/client.go b/providers/aws/services/savingsplans/client.go index 88bf91471..eeb064c73 100644 --- a/providers/aws/services/savingsplans/client.go +++ b/providers/aws/services/savingsplans/client.go @@ -101,7 +101,7 @@ func (c *Client) GetRegion() string { } // GetRecommendations returns empty as Savings Plans uses centralized Cost Explorer recommendations -func (c *Client) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/aws/services/savingsplans/client_test.go b/providers/aws/services/savingsplans/client_test.go index df59a7769..6594c37a5 100644 --- a/providers/aws/services/savingsplans/client_test.go +++ b/providers/aws/services/savingsplans/client_test.go @@ -90,7 +90,7 @@ func TestClient_GetRegion(t *testing.T) { func TestClient_GetRecommendations(t *testing.T) { client := &Client{region: "us-east-1"} - recs, err := client.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := client.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index b6ea2ad39..5217b4fc9 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -27,7 +27,7 @@ import ( // allow tests to substitute fake implementations; production code uses the // concrete types via the newXxxClientFn variables below. type serviceRecsGetter interface { - GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) + GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) } // newComputeClientFn, newDatabaseClientFn, newCacheClientFn, @@ -108,7 +108,7 @@ func NewRecommendationsClientAdapter(cred azcore.TokenCredential, subscriptionID // its own error and returns nil to the group so that a single service failure // does not cancel sibling calls. Results are appended in a deterministic order // (compute → database → cache → cosmosdb → savingsplans → advisor) after all goroutines finish. -func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { var ( computeRecs, dbRecs, cacheRecs, cosmosRecs, advisorRecs, spRecs []common.Recommendation computeErr, dbErr, cacheErr, cosmosErr, advisorErr, spErr error @@ -143,10 +143,11 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // Record which services the params filter lets through so the merge can // distinguish "skipped by filter" from "attempted and succeeded with zero // recommendations" when applying the all-attempted-failed guard. - includeCompute := shouldIncludeService(params, common.ServiceCompute) - includeDB := shouldIncludeService(params, common.ServiceRelationalDB) - includeCache := shouldIncludeService(params, common.ServiceCache) - includeCosmos := shouldIncludeService(params, common.ServiceNoSQL) + includeCompute := shouldIncludeService(*params, common.ServiceCompute) + includeDB := shouldIncludeService(*params, common.ServiceRelationalDB) + includeCache := shouldIncludeService(*params, common.ServiceCache) + includeCosmos := shouldIncludeService(*params, common.ServiceNoSQL) + includeSP := shouldIncludeService(*params, common.ServiceSavingsPlansAll) // Compute (VM) recommendations — subscription-wide. if includeCompute { @@ -185,7 +186,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p // The call returns an empty slice so the service appears in the fan-out // and will start returning data once the API stabilizes without requiring // a scheduler change. - if shouldIncludeService(params, common.ServiceSavingsPlansAll) { + if includeSP { goService(&spErr, func() { spClient := newSavingsPlansClientFn(r.cred, r.subscriptionID, "") spRecs, spErr = spClient.GetRecommendations(gctx, params) @@ -203,7 +204,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p advisorFn = r.getAdvisorRecommendations } goService(&advisorErr, func() { - advisorRecs, advisorErr = advisorFn(gctx, params) + advisorRecs, advisorErr = r.getAdvisorRecommendations(gctx, *params) }) // Wait for all goroutines. g.Wait() always returns nil because every @@ -313,13 +314,13 @@ func (r *RecommendationsClientAdapter) GetRecommendationsForService(ctx context. params := common.RecommendationParams{ Service: service, } - return r.GetRecommendations(ctx, params) + return r.GetRecommendations(ctx, ¶ms) } // GetAllRecommendations retrieves all Azure reservation recommendations across all services func (r *RecommendationsClientAdapter) GetAllRecommendations(ctx context.Context) ([]common.Recommendation, error) { params := common.RecommendationParams{} - return r.GetRecommendations(ctx, params) + return r.GetRecommendations(ctx, ¶ms) } // getAdvisorRecommendations retrieves cost optimization recommendations from Azure Advisor diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index 24fb68b8a..da3797c47 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -325,7 +325,8 @@ func TestRecommendationsClientAdapter_GetRecommendations_PropagatesContextCancel ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := adapter.GetRecommendations(ctx, common.RecommendationParams{}) + emptyParams := common.RecommendationParams{} + _, err := adapter.GetRecommendations(ctx, &emptyParams) require.Error(t, err, "expected context.Canceled to propagate from GetRecommendations") assert.ErrorIs(t, err, context.Canceled, "GetRecommendations must propagate the parent ctx error after g.Wait()") @@ -616,7 +617,7 @@ type fakeServiceClient struct { err error } -func (f *fakeServiceClient) GetRecommendations(ctx context.Context, _ common.RecommendationParams) ([]common.Recommendation, error) { +func (f *fakeServiceClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { select { case <-time.After(f.sleepDur): case <-ctx.Done(): @@ -679,7 +680,7 @@ func TestGetRecommendations_Parallelism(t *testing.T) { } start := time.Now() - _, err := adapter.GetRecommendations(context.Background(), common.RecommendationParams{}) + _, err := adapter.GetRecommendations(context.Background(), &common.RecommendationParams{}) elapsed := time.Since(start) require.NoError(t, err) @@ -731,7 +732,7 @@ func TestGetRecommendations_OrderPreservation(t *testing.T) { getAdvisorRecsFn: noopAdvisorFn, } - recs, err := adapter.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := adapter.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) // savingsplans and advisor are no-ops; we expect exactly 4 recs (one per @@ -780,7 +781,7 @@ func TestGetRecommendations_ErrorIsolation(t *testing.T) { getAdvisorRecsFn: noopAdvisorFn, } - recs, err := adapter.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := adapter.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err, "a per-service error must not surface as a GetRecommendations error") require.Len(t, recs, 3, "expected recs from the 3 healthy injectable services") diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 1e5cdb111..753ae2e82 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -138,7 +138,7 @@ func (c *CacheClient) GetRegion() string { type AzureRetailPrice = pricing.Page[pricing.RetailPriceItem] // GetRecommendations gets Redis Cache reservation recommendations from Azure Consumption API -func (c *CacheClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *CacheClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) // Use injected pager if available (for testing) diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index f75cbe5c8..a95814ed7 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -484,7 +484,7 @@ func TestCacheClient_GetRecommendations_WithMockPager(t *testing.T) { client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } @@ -511,7 +511,7 @@ func TestCacheClient_GetRecommendations_MultiplePages(t *testing.T) { client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index 70a9a6d91..fd447817b 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -168,7 +168,7 @@ type AzureRetailPriceItem = pricing.RetailPriceItem type AzureRetailPrice = pricing.Page[pricing.RetailPriceItem] // GetRecommendations gets VM RI recommendations from Azure Consumption API -func (c *ComputeClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *ComputeClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) // Use injected pager if available (for testing) diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index 266c8020a..97862ea94 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -214,7 +214,7 @@ func TestComputeClient_GetRecommendations_WithMock(t *testing.T) { Region: "eastus", } - recommendations, err := client.GetRecommendations(ctx, params) + recommendations, err := client.GetRecommendations(ctx, ¶ms) require.NoError(t, err) assert.Empty(t, recommendations) } @@ -242,7 +242,7 @@ func TestComputeClient_GetRecommendations_EmitsBothPaymentVariants(t *testing.T) } client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, recs, 2, "one API rec must expand to two payment-variant entries") diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index 8d1d93ac0..620b35476 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -140,7 +140,7 @@ type CosmosRetailPriceItem = pricing.RetailPriceItem type AzureRetailPrice = pricing.Page[pricing.RetailPriceItem] // GetRecommendations gets Cosmos DB reservation recommendations from Azure Consumption API -func (c *CosmosDBClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *CosmosDBClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) // Use injected pager if available (for testing) diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index a75354eb0..5f75b1761 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -421,7 +421,7 @@ func TestCosmosDBClient_GetRecommendations_WithMockPager(t *testing.T) { } client.SetRecommendationsPager(mockPager) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.NotNil(t, recommendations) } @@ -437,7 +437,7 @@ func TestCosmosDBClient_GetRecommendations_PagerError(t *testing.T) { client.SetRecommendationsPager(mockPager) - _, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + _, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) assert.Error(t, err) assert.Contains(t, err.Error(), "failed to get Cosmos DB recommendations") } @@ -463,7 +463,7 @@ func TestCosmosDBClient_GetRecommendations_MultiplePages(t *testing.T) { } client.SetRecommendationsPager(mockPager) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.NotNil(t, recommendations) assert.Equal(t, 2, mockPager.index) // Verify both pages were consumed diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index 50df7c5db..88614306c 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -170,7 +170,7 @@ func (c *DatabaseClient) GetRegion() string { type AzureRetailPrice = pricing.Page[pricing.RetailPriceItem] // GetRecommendations gets SQL Database reservation recommendations from Azure Consumption API -func (c *DatabaseClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *DatabaseClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) // Use injected pager if available (for testing) diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index 92e219a7e..7b0d84173 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -436,7 +436,7 @@ func TestDatabaseClient_GetRecommendations_WithMockPager(t *testing.T) { client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } @@ -463,7 +463,7 @@ func TestDatabaseClient_GetRecommendations_MultiplePages(t *testing.T) { client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index 3e7bb3a01..92d77b468 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -132,7 +132,7 @@ func (c *ManagedRedisClient) GetRegion() string { type AzureRetailPrice = pricing.Page[pricing.RetailPriceItem] // GetRecommendations gets Redis Cache reservation recommendations from the Azure Consumption API. -func (c *ManagedRedisClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *ManagedRedisClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recommendations := make([]common.Recommendation, 0) var pager RecommendationsPager diff --git a/providers/azure/services/managedredis/client_test.go b/providers/azure/services/managedredis/client_test.go index 0e4f99748..f0d48ad30 100644 --- a/providers/azure/services/managedredis/client_test.go +++ b/providers/azure/services/managedredis/client_test.go @@ -341,7 +341,7 @@ func TestGetRecommendations_EmptyPager(t *testing.T) { }}, }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } @@ -358,7 +358,7 @@ func TestGetRecommendations_MultiplePages(t *testing.T) { }}, }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index c94d46e4b..d3b30c792 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -132,7 +132,7 @@ func (c *Client) GetRegion() string { // (providers/azure/recommendations.go, COR-03). When this method starts // making real API calls, flip that call site's attempted flag back to the // params-filter value so its failures count toward the guard. -func (c *Client) GetRecommendations(_ context.Context, _ common.RecommendationParams) ([]common.Recommendation, error) { +func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index 1ce975968..0f5192833 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -114,7 +114,7 @@ func TestGetRegion(t *testing.T) { func TestGetRecommendations_AlwaysEmpty(t *testing.T) { c := NewClient(nil, "sub", "eastus") - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index 504e78915..2d02c19aa 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -111,21 +111,20 @@ func (c *SearchClient) GetRegion() string { return c.region } -// AzureRetailPrice is the response envelope for the Azure Retail Prices API. -type AzureRetailPrice = pricing.Page[pricing.RetailPriceItem] - -// GetRecommendations returns empty for Azure Search because the Azure Consumption -// ReservationRecommendations API has no Search-specific resourceType. The -// valid resourceType values are: VirtualMachines, SQLDatabases, PostgreSQL, -// ManagedDisk, MySQL, RedHat, MariaDB, RedisCache, CosmosDB, SqlDataWarehouse, -// SUSELinux, AppService, BlockBlob, AzureDataExplorer, VMwareCloudSimple. -// -// Querying without a resourceType filter (or with an invalid value) causes the -// API to default to VirtualMachines and return VM reservation recommendations, -// which would be mislabelled as Search recommendations. Returning empty is the -// correct behaviour until Azure exposes a Search reservation recommendation -// stream via this API. -func (c *SearchClient) GetRecommendations(_ context.Context, _ common.RecommendationParams) ([]common.Recommendation, error) { +// AzureRetailPrice represents pricing information from Azure Retail Prices API +type AzureRetailPrice struct { + Items []pricing.RetailPriceItem `json:"Items"` + NextPageLink string `json:"NextPageLink"` + Count int `json:"Count"` +} + +// GetRecommendations returns an empty slice for Azure Search. "AzureSearch" is not +// a valid resourceType in the Consumption ReservationRecommendations API (valid list: +// VirtualMachines, SQLDatabases, CosmosDB, SqlDataWarehouse, etc.), so any call to +// that API returns VM recommendations that would be mislabeled as Search recommendations. +// Until Azure exposes a stable Search reservation recommendations API, this method +// is intentionally a no-op. +func (c *SearchClient) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { return []common.Recommendation{}, nil } diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index 05d3b63de..94ab59196 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -430,7 +430,7 @@ func TestSearchClient_GetRecommendations_WithMockPager(t *testing.T) { client.SetRecommendationsPager(mockPager) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } @@ -471,7 +471,7 @@ func TestSearchClient_GetRecommendations_AlwaysEmpty(t *testing.T) { } client.SetRecommendationsPager(pagerWithVMRecs) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs, "Azure Search has no Consumption API reservation stream; GetRecommendations "+ diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index b11e793ec..876c65600 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -123,7 +123,7 @@ type SynapseRetailPriceItem struct { // GetRecommendations retrieves Synapse reservation recommendations from the // Azure Consumption API. -func (c *SynapseClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *SynapseClient) GetRecommendations(ctx context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { recs := make([]common.Recommendation, 0) var pager RecommendationsPager diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index 257f3cce0..c58bb98c5 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -152,7 +152,7 @@ func TestGetRecommendations_empty(t *testing.T) { c := newTestClient() c.SetRecommendationsPager(&fakeRecommendationsPager{}) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recs) } @@ -176,7 +176,7 @@ func TestGetRecommendations_singlePage(t *testing.T) { }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, recs, 1) @@ -228,7 +228,7 @@ func TestGetRecommendations_multiPage(t *testing.T) { }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) assert.Len(t, recs, 2) } @@ -237,7 +237,7 @@ func TestGetRecommendations_pagerError(t *testing.T) { c := newTestClient() c.SetRecommendationsPager(&errorRecommendationsPager{}) - _, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + _, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) assert.Error(t, err) } @@ -266,7 +266,7 @@ func TestGetRecommendations_regionFilter(t *testing.T) { }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, recs, 1) assert.Equal(t, "DW500c", recs[0].ResourceType) @@ -292,7 +292,7 @@ func TestGetRecommendations_modernShape(t *testing.T) { }, }) - recs, err := c.GetRecommendations(context.Background(), common.RecommendationParams{}) + recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, recs, 1) assert.Equal(t, "DW2000c", recs[0].ResourceType) diff --git a/providers/gcp/recommendations.go b/providers/gcp/recommendations.go index d51f3d70e..48ea1cc78 100644 --- a/providers/gcp/recommendations.go +++ b/providers/gcp/recommendations.go @@ -101,7 +101,11 @@ type RecommendationsClientAdapter struct { // Mirrors the Azure parallelisation in // providers/azure/recommendations.go (closes #258, commit b10326c5) and the // AWS service-loop parallelisation (closes #266). -func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + if p == nil { + return nil, fmt.Errorf("params cannot be nil") + } + params := *p // Context cancellation is terminal: bail out before any API fan-out. // Newer cloud.google.com/go/compute REST clients can complete a regions // List call (and return a real 403) even when ctx is already cancelled, @@ -218,7 +222,7 @@ func (r *RecommendationsClientAdapter) collectComputeRecs(ctx context.Context, p if err != nil { return nil, err } - return client.GetRecommendations(ctx, params) + return client.GetRecommendations(ctx, ¶ms) } // collectSQLRecs fetches Cloud SQL CUD recommendations for one region. @@ -231,7 +235,7 @@ func (r *RecommendationsClientAdapter) collectSQLRecs(ctx context.Context, param if err != nil { return nil, err } - return client.GetRecommendations(ctx, params) + return client.GetRecommendations(ctx, ¶ms) } // collectCacheRecs fetches Memorystore recommendations for one region. @@ -244,7 +248,7 @@ func (r *RecommendationsClientAdapter) collectCacheRecs(ctx context.Context, par if err != nil { return nil, err } - return client.GetRecommendations(ctx, params) + return client.GetRecommendations(ctx, ¶ms) } // collectStorageRecs fetches Cloud Storage recommendations for one region. @@ -257,7 +261,7 @@ func (r *RecommendationsClientAdapter) collectStorageRecs(ctx context.Context, p if err != nil { return nil, err } - return client.GetRecommendations(ctx, params) + return client.GetRecommendations(ctx, ¶ms) } // collectRegion fetches recommendations for all four GCP services @@ -351,13 +355,13 @@ func (r *RecommendationsClientAdapter) GetRecommendationsForService(ctx context. params := common.RecommendationParams{ Service: service, } - return r.GetRecommendations(ctx, params) + return r.GetRecommendations(ctx, ¶ms) } // GetAllRecommendations retrieves all GCP commitment recommendations across all services func (r *RecommendationsClientAdapter) GetAllRecommendations(ctx context.Context) ([]common.Recommendation, error) { params := common.RecommendationParams{} - return r.GetRecommendations(ctx, params) + return r.GetRecommendations(ctx, ¶ms) } // getRegions retrieves available GCP regions for the project diff --git a/providers/gcp/recommendations_test.go b/providers/gcp/recommendations_test.go index 94f9c3eb6..b27d1739e 100644 --- a/providers/gcp/recommendations_test.go +++ b/providers/gcp/recommendations_test.go @@ -155,7 +155,7 @@ func TestRecommendationsClientAdapter_GetRecommendations_PropagatesContextCancel ctx, cancel := context.WithCancel(context.Background()) cancel() - _, err := adapter.GetRecommendations(ctx, common.RecommendationParams{}) + _, err := adapter.GetRecommendations(ctx, &common.RecommendationParams{}) require.Error(t, err, "expected context.Canceled to propagate from GetRecommendations") assert.ErrorIs(t, err, context.Canceled, "GetRecommendations must propagate the parent ctx error") diff --git a/providers/gcp/services/cloudsql/client.go b/providers/gcp/services/cloudsql/client.go index 6a102dca0..8cef72e51 100644 --- a/providers/gcp/services/cloudsql/client.go +++ b/providers/gcp/services/cloudsql/client.go @@ -138,20 +138,30 @@ func (c *CloudSQLClient) GetRegion() string { return c.region } +// resolveRecommenderClient returns the injected client (for testing) or creates +// a new one from the stored options. +func (c *CloudSQLClient) resolveRecommenderClient(ctx context.Context) (RecommenderClient, error) { + if c.recommenderClient != nil { + return c.recommenderClient, nil + } + client, err := recommender.NewClient(ctx, c.clientOpts...) + if err != nil { + return nil, fmt.Errorf("failed to create recommender client: %w", err) + } + return &realRecommenderClient{client: client}, nil +} + // GetRecommendations gets Cloud SQL recommendations from GCP Recommender API -func (c *CloudSQLClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *CloudSQLClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + if p == nil { + return nil, fmt.Errorf("params cannot be nil") + } + params := *p recommendations := make([]common.Recommendation, 0) - // Use injected client if available (for testing) - var recClient RecommenderClient - if c.recommenderClient != nil { - recClient = c.recommenderClient - } else { - client, err := recommender.NewClient(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create recommender client: %w", err) - } - recClient = &realRecommenderClient{client: client} + recClient, err := c.resolveRecommenderClient(ctx) + if err != nil { + return nil, err } defer recClient.Close() diff --git a/providers/gcp/services/cloudsql/client_test.go b/providers/gcp/services/cloudsql/client_test.go index 98ac60938..f857f60b2 100644 --- a/providers/gcp/services/cloudsql/client_test.go +++ b/providers/gcp/services/cloudsql/client_test.go @@ -617,7 +617,7 @@ func TestCloudSQLClient_GetRecommendations_WithMock(t *testing.T) { } client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Len(t, recommendations, 1) assert.Equal(t, common.ProviderGCP, recommendations[0].Provider) @@ -635,7 +635,7 @@ func TestCloudSQLClient_GetRecommendations_IteratorError(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.Error(t, err) assert.Contains(t, err.Error(), "cloudsql: iterate recommendations") assert.Nil(t, recs, "partial data must not leak on iterator failure") @@ -655,7 +655,7 @@ func TestCloudSQLClient_GetRecommendations_Empty(t *testing.T) { } client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recommendations) } diff --git a/providers/gcp/services/cloudstorage/client.go b/providers/gcp/services/cloudstorage/client.go index 5993132d0..94a1cc175 100644 --- a/providers/gcp/services/cloudstorage/client.go +++ b/providers/gcp/services/cloudstorage/client.go @@ -158,20 +158,30 @@ func (c *CloudStorageClient) GetRegion() string { return c.region } +// resolveRecommenderClient returns the injected client (for testing) or creates +// a new one from the stored options. +func (c *CloudStorageClient) resolveRecommenderClient(ctx context.Context) (RecommenderClient, error) { + if c.recommenderClient != nil { + return c.recommenderClient, nil + } + client, err := recommender.NewClient(ctx, c.clientOpts...) + if err != nil { + return nil, fmt.Errorf("failed to create recommender client: %w", err) + } + return &realRecommenderClient{client: client}, nil +} + // GetRecommendations gets Cloud Storage recommendations from GCP Recommender API -func (c *CloudStorageClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *CloudStorageClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + if p == nil { + return nil, fmt.Errorf("params cannot be nil") + } + params := *p recommendations := make([]common.Recommendation, 0) - // Use injected client if available (for testing) - var recClient RecommenderClient - if c.recommenderClient != nil { - recClient = c.recommenderClient - } else { - client, err := recommender.NewClient(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create recommender client: %w", err) - } - recClient = &realRecommenderClient{client: client} + recClient, err := c.resolveRecommenderClient(ctx) + if err != nil { + return nil, err } defer recClient.Close() diff --git a/providers/gcp/services/cloudstorage/client_test.go b/providers/gcp/services/cloudstorage/client_test.go index c8c1f2058..85e1da2e3 100644 --- a/providers/gcp/services/cloudstorage/client_test.go +++ b/providers/gcp/services/cloudstorage/client_test.go @@ -376,7 +376,7 @@ func TestCloudStorageClient_GetRecommendations_WithMock(t *testing.T) { } client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Len(t, recommendations, 1) assert.Equal(t, common.ProviderGCP, recommendations[0].Provider) @@ -394,7 +394,7 @@ func TestCloudStorageClient_GetRecommendations_Empty(t *testing.T) { } client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recommendations) } @@ -411,7 +411,7 @@ func TestCloudStorageClient_GetRecommendations_IteratorError(t *testing.T) { // Iterator errors now propagate (issue #1022 H2 fix) -- they must not be // silently swallowed, as that would mask auth/quota failures and cause callers // to act on a partial (empty) recommendation list. - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.Error(t, err) assert.Contains(t, err.Error(), "cloudstorage: iterate recommendations") assert.Nil(t, recommendations) diff --git a/providers/gcp/services/computeengine/client.go b/providers/gcp/services/computeengine/client.go index 506e0c8f6..fa3d85900 100644 --- a/providers/gcp/services/computeengine/client.go +++ b/providers/gcp/services/computeengine/client.go @@ -224,20 +224,30 @@ func (c *ComputeEngineClient) GetRegion() string { return c.region } +// resolveRecommenderClient returns the injected client (for testing) or creates +// a new one from the stored options. +func (c *ComputeEngineClient) resolveRecommenderClient(ctx context.Context) (RecommenderClient, error) { + if c.recommenderClient != nil { + return c.recommenderClient, nil + } + client, err := recommender.NewClient(ctx, c.clientOpts...) + if err != nil { + return nil, fmt.Errorf("failed to create recommender client: %w", err) + } + return &realRecommenderClient{client: client}, nil +} + // GetRecommendations gets CUD recommendations from GCP Recommender API -func (c *ComputeEngineClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { +func (c *ComputeEngineClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + if p == nil { + return nil, fmt.Errorf("params cannot be nil") + } + params := *p recommendations := make([]common.Recommendation, 0) - // Use injected client if available (for testing) - var recClient RecommenderClient - if c.recommenderClient != nil { - recClient = c.recommenderClient - } else { - client, err := recommender.NewClient(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create recommender client: %w", err) - } - recClient = &realRecommenderClient{client: client} + recClient, err := c.resolveRecommenderClient(ctx) + if err != nil { + return nil, err } defer recClient.Close() diff --git a/providers/gcp/services/computeengine/client_test.go b/providers/gcp/services/computeengine/client_test.go index 990694358..b736ee4cc 100644 --- a/providers/gcp/services/computeengine/client_test.go +++ b/providers/gcp/services/computeengine/client_test.go @@ -790,7 +790,7 @@ func TestComputeEngineClient_GetRecommendations_WithMock(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Len(t, recommendations, 1) assert.Equal(t, common.ProviderGCP, recommendations[0].Provider) @@ -814,7 +814,7 @@ func TestComputeEngineClient_GetRecommendations_IteratorError(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.Error(t, err) assert.Contains(t, err.Error(), "computeengine: iterate recommendations") assert.Nil(t, recs, "partial data must not leak on iterator failure") @@ -832,7 +832,7 @@ func TestComputeEngineClient_GetRecommendations_Empty(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - recommendations, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recommendations, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) assert.Empty(t, recommendations) } @@ -936,8 +936,8 @@ func TestComputeEngineClient_GetRecommendations_CtxCancelReturnsError(t *testing require.NoError(t, err) client.SetRecommenderClient(&infiniteRecommenderClient{}) - _, err = client.GetRecommendations(ctx, common.RecommendationParams{}) - require.Error(t, err, "canceled context must surface an error, not a partial result set") + _, err = client.GetRecommendations(ctx, &common.RecommendationParams{}) + require.Error(t, err, "cancelled context must surface an error, not a partial result set") } // TestComputeEngineClient_GetRecommendations_PageCapFires asserts that the @@ -947,7 +947,7 @@ func TestComputeEngineClient_GetRecommendations_PageCapFires(t *testing.T) { require.NoError(t, err) client.SetRecommenderClient(&infiniteRecommenderClient{}) - _, err = client.GetRecommendations(context.Background(), common.RecommendationParams{}) + _, err = client.GetRecommendations(context.Background(), &common.RecommendationParams{}) require.Error(t, err, "page cap must surface an error when the iterator never terminates") } @@ -1562,7 +1562,7 @@ func TestGetRecommendations_FiltersNonActiveStates(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - results, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + results, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, results, 1, "only the ACTIVE recommendation must be returned; CLAIMED/SUCCEEDED/FAILED/DISMISSED must be filtered (H-1)") @@ -1595,7 +1595,7 @@ func TestGetRecommendations_ActiveRecIncluded(t *testing.T) { mockClient := &MockRecommenderClient{iterator: mockIterator} client.SetRecommenderClient(mockClient) - results, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + results, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) require.NoError(t, err) require.Len(t, results, 1, "an ACTIVE recommendation must be included") } diff --git a/providers/gcp/services/memorystore/client.go b/providers/gcp/services/memorystore/client.go index f54726824..4c49a988a 100644 --- a/providers/gcp/services/memorystore/client.go +++ b/providers/gcp/services/memorystore/client.go @@ -151,15 +151,28 @@ func (c *MemorystoreClient) GetRegion() string { return c.region } +// resolveRecommenderClient returns the injected client (for testing) or creates +// a new one from the stored options. +func (c *MemorystoreClient) resolveRecommenderClient(ctx context.Context) (RecommenderClient, error) { + if c.recommenderClient != nil { + return c.recommenderClient, nil + } + client, err := recommender.NewClient(ctx, c.clientOpts...) + if err != nil { + return nil, fmt.Errorf("failed to create recommender client: %w", err) + } + return &realRecommenderClient{client: client}, nil +} + // GetRecommendations gets Memorystore Redis recommendations from GCP Recommender API -func (c *MemorystoreClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { - recClient := c.recommenderClient - if recClient == nil { - client, err := recommender.NewClient(ctx, c.clientOpts...) - if err != nil { - return nil, fmt.Errorf("failed to create recommender client: %w", err) - } - recClient = &realRecommenderClient{client: client} +func (c *MemorystoreClient) GetRecommendations(ctx context.Context, p *common.RecommendationParams) ([]common.Recommendation, error) { + if p == nil { + return nil, fmt.Errorf("params cannot be nil") + } + params := *p + recClient, err := c.resolveRecommenderClient(ctx) + if err != nil { + return nil, err } defer recClient.Close() diff --git a/providers/gcp/services/memorystore/client_test.go b/providers/gcp/services/memorystore/client_test.go index 1d3c7e365..a01c1c768 100644 --- a/providers/gcp/services/memorystore/client_test.go +++ b/providers/gcp/services/memorystore/client_test.go @@ -574,7 +574,7 @@ func TestMemorystoreClient_GetRecommendations_WithMockClient(t *testing.T) { } client.SetRecommenderClient(mockClient) - recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + recs, err := client.GetRecommendations(ctx, &common.RecommendationParams{}) if tt.wantErr { require.Error(t, err) From 506c06015cb12c23cbbb08dfcc24c391a4578c18 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 19 Jul 2026 22:34:24 +0200 Subject: [PATCH 2/3] fix(recommendations): isolate retry state and guard inputs Use operation-local AWS retry counters so concurrent recommendation, coverage, and utilization requests cannot corrupt each other's backoff. Honor the injected Azure Advisor function and reject nil recommendation parameters at the AWS and Azure provider boundaries. --- providers/aws/recommendations/client.go | 11 +++++--- providers/aws/recommendations/client_test.go | 20 +++++++++++++ providers/aws/recommendations/coverage.go | 6 ++-- .../aws/recommendations/ondemand_series.go | 6 ++-- providers/aws/recommendations/parser_sp.go | 6 ++-- providers/aws/recommendations/ratelimiter.go | 7 +++++ providers/aws/recommendations/sp_coverage.go | 28 ++++++++----------- providers/aws/recommendations/utilization.go | 6 ++-- providers/aws/service_client.go | 4 +++ providers/aws/service_client_test.go | 9 ++++++ providers/azure/recommendations.go | 5 +++- providers/azure/recommendations_test.go | 27 ++++++++++++++++++ 12 files changed, 102 insertions(+), 33 deletions(-) diff --git a/providers/aws/recommendations/client.go b/providers/aws/recommendations/client.go index a9305d234..22df623fc 100644 --- a/providers/aws/recommendations/client.go +++ b/providers/aws/recommendations/client.go @@ -134,6 +134,9 @@ func (c *Client) SetRecLookbackPeriod(period string) { // GetRecommendations fetches Reserved Instance recommendations for any service. func (c *Client) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { + if params == nil { + return nil, fmt.Errorf("params cannot be nil") + } // Handle Savings Plans separately — they use a different Cost Explorer API // (GetSavingsPlansPurchaseRecommendation, not GetReservationPurchaseRecommendation). // Match any SP slug — the legacy umbrella plus the four per-plan-type slugs — @@ -206,12 +209,12 @@ func (c *Client) fetchRIPageWithRetry( ctx context.Context, input *costexplorer.GetReservationPurchaseRecommendationInput, ) (*costexplorer.GetReservationPurchaseRecommendationOutput, error) { - c.rateLimiter.Reset() + rateLimiter := c.rateLimiter.newOperation() var result *costexplorer.GetReservationPurchaseRecommendationOutput var err error for { - if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { + if waitErr := rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) } @@ -220,13 +223,13 @@ func (c *Client) fetchRIPageWithRetry( } result, err = c.costExplorerClient.GetReservationPurchaseRecommendation(ctx, input) concurrency.Release(ctx) - if !c.rateLimiter.ShouldRetry(err) { + if !rateLimiter.ShouldRetry(err) { break } } if err != nil { - return nil, fmt.Errorf("failed to get RI recommendations after %d retries: %w", c.rateLimiter.GetRetryCount(), err) + return nil, fmt.Errorf("failed to get RI recommendations after %d retries: %w", rateLimiter.GetRetryCount(), err) } return result, nil diff --git a/providers/aws/recommendations/client_test.go b/providers/aws/recommendations/client_test.go index 44c000233..8c63316e5 100644 --- a/providers/aws/recommendations/client_test.go +++ b/providers/aws/recommendations/client_test.go @@ -303,6 +303,26 @@ func TestGetRecommendations_Error(t *testing.T) { assert.Equal(t, 3, mockAPI.callCount) } +func TestGetRecommendations_NilParams(t *testing.T) { + client := NewClientWithAPI(&mockCostExplorerAPI{}, "us-east-1") + + recs, err := client.GetRecommendations(context.Background(), nil) + + require.EqualError(t, err, "params cannot be nil") + assert.Nil(t, recs) +} + +func TestRateLimiter_NewOperationHasIndependentRetryState(t *testing.T) { + policy := NewRateLimiterWithOptions(time.Millisecond, 10*time.Millisecond, 2) + first := policy.newOperation() + second := policy.newOperation() + + require.True(t, first.ShouldRetry(newThrottleError())) + assert.Equal(t, 1, first.GetRetryCount()) + assert.Zero(t, second.GetRetryCount()) + assert.Zero(t, policy.GetRetryCount()) +} + func TestGetRecommendations_EmptyResult(t *testing.T) { mockAPI := &mockCostExplorerAPI{ riRecommendations: &costexplorer.GetReservationPurchaseRecommendationOutput{ diff --git a/providers/aws/recommendations/coverage.go b/providers/aws/recommendations/coverage.go index 6c7ac438f..9e5f10bda 100644 --- a/providers/aws/recommendations/coverage.go +++ b/providers/aws/recommendations/coverage.go @@ -324,9 +324,9 @@ func serviceRegionFilter(service, region string) *types.Expression { // held only across the outbound SDK call, matching fetchRIPageWithRetry, // so the coverage-enrichment fan-out stays inside the global IO cap. func (c *Client) fetchCoveragePage(ctx context.Context, input *costexplorer.GetReservationCoverageInput) (*costexplorer.GetReservationCoverageOutput, error) { - c.rateLimiter.Reset() + rateLimiter := c.rateLimiter.newOperation() for { - if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { + if waitErr := rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) } if acqErr := concurrency.Acquire(ctx); acqErr != nil { @@ -334,7 +334,7 @@ func (c *Client) fetchCoveragePage(ctx context.Context, input *costexplorer.GetR } result, err := c.costExplorerClient.GetReservationCoverage(ctx, input) concurrency.Release(ctx) - if !c.rateLimiter.ShouldRetry(err) { + if !rateLimiter.ShouldRetry(err) { if err != nil { return nil, fmt.Errorf("failed to get reservation coverage: %w", err) } diff --git a/providers/aws/recommendations/ondemand_series.go b/providers/aws/recommendations/ondemand_series.go index 1ea1a6a34..2b22b8bd2 100644 --- a/providers/aws/recommendations/ondemand_series.go +++ b/providers/aws/recommendations/ondemand_series.go @@ -174,13 +174,13 @@ func validateOnDemandSeriesArgs(region string, lookbackDays int) error { // utilization.go. Each attempt acquires one rate-limiter slot at the API call // site (not at goroutine creation) per feedback_semaphore_at_api_call. func (c *Client) fetchOnDemandPage(ctx context.Context, input *costexplorer.GetCostAndUsageInput) (*costexplorer.GetCostAndUsageOutput, error) { - c.rateLimiter.Reset() + rateLimiter := c.rateLimiter.newOperation() for { - if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { + if waitErr := rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait: %w", waitErr) } out, err := c.costExplorerClient.GetCostAndUsage(ctx, input) - if !c.rateLimiter.ShouldRetry(err) { + if !rateLimiter.ShouldRetry(err) { if err != nil { return nil, fmt.Errorf("GetCostAndUsage: %w", err) } diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index 1cc03c2c0..3a71a7d75 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -133,12 +133,12 @@ func (c *Client) fetchSPPageWithRetry( ctx context.Context, input *costexplorer.GetSavingsPlansPurchaseRecommendationInput, ) (*costexplorer.GetSavingsPlansPurchaseRecommendationOutput, error) { - c.rateLimiter.Reset() + rateLimiter := c.rateLimiter.newOperation() var result *costexplorer.GetSavingsPlansPurchaseRecommendationOutput var err error for { - if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { + if waitErr := rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) } @@ -147,7 +147,7 @@ func (c *Client) fetchSPPageWithRetry( } result, err = c.costExplorerClient.GetSavingsPlansPurchaseRecommendation(ctx, input) concurrency.Release(ctx) - if !c.rateLimiter.ShouldRetry(err) { + if !rateLimiter.ShouldRetry(err) { break } } diff --git a/providers/aws/recommendations/ratelimiter.go b/providers/aws/recommendations/ratelimiter.go index d24ab4c06..0853158e1 100644 --- a/providers/aws/recommendations/ratelimiter.go +++ b/providers/aws/recommendations/ratelimiter.go @@ -57,6 +57,13 @@ func NewRateLimiterWithOptions(baseDelay, maxDelay time.Duration, maxRetries int } } +// newOperation returns a limiter with the same retry policy and independent +// retry state. A Client is shared across concurrent service sweeps, so retry +// counters must be scoped to one API operation. +func (r *RateLimiter) newOperation() *RateLimiter { + return NewRateLimiterWithOptions(r.baseDelay, r.maxDelay, r.maxRetries) +} + // Wait implements exponential backoff delay func (r *RateLimiter) Wait(ctx context.Context) error { if r.retryCount == 0 { diff --git a/providers/aws/recommendations/sp_coverage.go b/providers/aws/recommendations/sp_coverage.go index 808c25662..33dfcffb4 100644 --- a/providers/aws/recommendations/sp_coverage.go +++ b/providers/aws/recommendations/sp_coverage.go @@ -298,11 +298,9 @@ func (a *spCoverageAccumulator) summarize(windowHours float64) SPCoverageSummary // Days==0 as "no data for this scope", not "no SPs in the account", and // check Days before dereferencing the pointer fields. // -// Concurrency: Client is NOT safe for concurrent SP calls - the shared -// rateLimiter's Reset() is unsynchronized (see -// feedback_rate_limiter_per_call), so concurrent callers race on the retry -// counter. Call from a single goroutine per Client; the ladder consumer -// (PR 5) is sequential by design. +// Concurrent calls are safe: fetchSPCoveragePage derives operation-local +// retry state from the client's shared retry policy, so callers never mutate +// the same retry counter. func (c *Client) GetSPCoverageSummary(ctx context.Context, region string, lookbackDays int) (SPCoverageSummary, error) { if lookbackDays <= 0 { return SPCoverageSummary{}, fmt.Errorf("sp coverage: lookbackDays must be positive, got %d", lookbackDays) @@ -361,13 +359,13 @@ func (c *Client) GetSPCoverageSummary(ctx context.Context, region string, lookba // fetchSPCoveragePage calls GetSavingsPlansCoverage with rate-limit retry. // Mirrors fetchCoveragePage in coverage.go so both paths back off consistently. func (c *Client) fetchSPCoveragePage(ctx context.Context, input *costexplorer.GetSavingsPlansCoverageInput) (*costexplorer.GetSavingsPlansCoverageOutput, error) { - c.rateLimiter.Reset() + rateLimiter := c.rateLimiter.newOperation() for { - if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { + if waitErr := rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) } result, err := c.costExplorerClient.GetSavingsPlansCoverage(ctx, input) - if !c.rateLimiter.ShouldRetry(err) { + if !rateLimiter.ShouldRetry(err) { if err != nil { return nil, fmt.Errorf("failed to get SP coverage: %w", err) } @@ -412,11 +410,9 @@ func (c *Client) fetchSPCoveragePage(ctx context.Context, input *costexplorer.Ge // real GetSavingsPlansUtilization per plan type) and confirm the returned // dimension values match the map. // -// Concurrency: Client is NOT safe for concurrent SP calls - the shared -// rateLimiter's Reset() is unsynchronized (see -// feedback_rate_limiter_per_call), so concurrent callers race on the retry -// counter. Call from a single goroutine per Client; the ladder consumer -// (PR 5) is sequential by design. +// Concurrent calls are safe: fetchSPUtilizationPage derives operation-local +// retry state from the client's shared retry policy, so callers never mutate +// the same retry counter. func (c *Client) GetSPUtilization(ctx context.Context, planType types.SupportedSavingsPlansType, region string, lookbackDays int) (SPUtilizationSummary, error) { if err := validateSPPlanType(planType); err != nil { return SPUtilizationSummary{}, fmt.Errorf("sp utilization: %w", err) @@ -452,13 +448,13 @@ func (c *Client) GetSPUtilization(ctx context.Context, planType types.SupportedS // fetchSPUtilizationPage calls GetSavingsPlansUtilization with rate-limit retry. // Mirrors fetchUtilizationPage in utilization.go so both paths back off consistently. func (c *Client) fetchSPUtilizationPage(ctx context.Context, input *costexplorer.GetSavingsPlansUtilizationInput) (*costexplorer.GetSavingsPlansUtilizationOutput, error) { - c.rateLimiter.Reset() + rateLimiter := c.rateLimiter.newOperation() for { - if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { + if waitErr := rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) } result, err := c.costExplorerClient.GetSavingsPlansUtilization(ctx, input) - if !c.rateLimiter.ShouldRetry(err) { + if !rateLimiter.ShouldRetry(err) { if err != nil { return nil, fmt.Errorf("failed to get SP utilization: %w", err) } diff --git a/providers/aws/recommendations/utilization.go b/providers/aws/recommendations/utilization.go index 4fd7c8449..26090d6f3 100644 --- a/providers/aws/recommendations/utilization.go +++ b/providers/aws/recommendations/utilization.go @@ -103,9 +103,9 @@ func buildUtilizations(agg map[string]*riAccumulator) []RIUtilization { // across the outbound SDK call, matching fetchRIPageWithRetry, so callers // running under a semaphore-carrying context stay inside the global IO cap. func (c *Client) fetchUtilizationPage(ctx context.Context, input *costexplorer.GetReservationUtilizationInput) (*costexplorer.GetReservationUtilizationOutput, error) { - c.rateLimiter.Reset() + rateLimiter := c.rateLimiter.newOperation() for { - if waitErr := c.rateLimiter.Wait(ctx); waitErr != nil { + if waitErr := rateLimiter.Wait(ctx); waitErr != nil { return nil, fmt.Errorf("rate limiter wait failed: %w", waitErr) } @@ -114,7 +114,7 @@ func (c *Client) fetchUtilizationPage(ctx context.Context, input *costexplorer.G } result, err := c.costExplorerClient.GetReservationUtilization(ctx, input) concurrency.Release(ctx) - if !c.rateLimiter.ShouldRetry(err) { + if !rateLimiter.ShouldRetry(err) { if err != nil { return nil, fmt.Errorf("failed to get reservation utilization: %w", err) } diff --git a/providers/aws/service_client.go b/providers/aws/service_client.go index de10e47d7..42cf84fef 100644 --- a/providers/aws/service_client.go +++ b/providers/aws/service_client.go @@ -3,6 +3,7 @@ package aws import ( "context" + "fmt" "github.com/aws/aws-sdk-go-v2/aws" sptypes "github.com/aws/aws-sdk-go-v2/service/savingsplans/types" @@ -72,6 +73,9 @@ func NewRecommendationsClient(cfg aws.Config) provider.RecommendationsClient { // GetRecommendations gets recommendations with filtering func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { + if params == nil { + return nil, fmt.Errorf("params cannot be nil") + } recs, err := r.client.GetRecommendations(ctx, params) if err != nil { return nil, err diff --git a/providers/aws/service_client_test.go b/providers/aws/service_client_test.go index 3c86ec429..ab0f11191 100644 --- a/providers/aws/service_client_test.go +++ b/providers/aws/service_client_test.go @@ -53,6 +53,15 @@ func newTestRecommendationsClient(ce *mockCostExplorerClient) *recommendations.C return recommendations.NewClientWithAPI(ce, "us-east-1") } +func TestRecommendationsClientAdapter_GetRecommendations_NilParams(t *testing.T) { + adapter := &RecommendationsClientAdapter{client: newTestRecommendationsClient(&mockCostExplorerClient{})} + + recs, err := adapter.GetRecommendations(context.Background(), nil) + + require.EqualError(t, err, "params cannot be nil") + assert.Nil(t, recs) +} + func TestNewEC2Client(t *testing.T) { cfg := aws.Config{Region: "us-east-1"} client := NewEC2Client(cfg) diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 5217b4fc9..8d143ef7b 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -109,6 +109,9 @@ func NewRecommendationsClientAdapter(cred azcore.TokenCredential, subscriptionID // does not cancel sibling calls. Results are appended in a deterministic order // (compute → database → cache → cosmosdb → savingsplans → advisor) after all goroutines finish. func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { + if params == nil { + return nil, fmt.Errorf("params cannot be nil") + } var ( computeRecs, dbRecs, cacheRecs, cosmosRecs, advisorRecs, spRecs []common.Recommendation computeErr, dbErr, cacheErr, cosmosErr, advisorErr, spErr error @@ -204,7 +207,7 @@ func (r *RecommendationsClientAdapter) GetRecommendations(ctx context.Context, p advisorFn = r.getAdvisorRecommendations } goService(&advisorErr, func() { - advisorRecs, advisorErr = r.getAdvisorRecommendations(gctx, *params) + advisorRecs, advisorErr = advisorFn(gctx, *params) }) // Wait for all goroutines. g.Wait() always returns nil because every diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index da3797c47..317152d05 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -265,6 +265,33 @@ func TestRecommendationsClientAdapter_GetAllRecommendations(t *testing.T) { }, recs) } +func TestRecommendationsClientAdapter_GetRecommendations_NilParams(t *testing.T) { + adapter := &RecommendationsClientAdapter{} + + recs, err := adapter.GetRecommendations(context.Background(), nil) + + require.EqualError(t, err, "params cannot be nil") + assert.Nil(t, recs) +} + +func TestGetRecommendations_UsesInjectedAdvisor(t *testing.T) { + want := common.Recommendation{Provider: common.ProviderAzure, Service: common.ServiceCompute} + called := false + adapter := &RecommendationsClientAdapter{ + getAdvisorRecsFn: func(_ context.Context, _ common.RecommendationParams) ([]common.Recommendation, error) { + called = true + return []common.Recommendation{want}, nil + }, + } + params := &common.RecommendationParams{Service: common.ServiceType("advisor-only")} + + recs, err := adapter.GetRecommendations(context.Background(), params) + + require.NoError(t, err) + require.True(t, called) + require.Equal(t, []common.Recommendation{want}, recs) +} + // TestGetRecommendations_SavingsPlansServiceIncluded pins that shouldIncludeService // allows ServiceSavingsPlansAll through both when params.Service is empty (all-services // sweep) and when explicitly set to ServiceSavingsPlansAll, and does not include it From ce401574a40834c90c528726688da2e04188fa84 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 21 Jul 2026 18:17:25 +0200 Subject: [PATCH 3/3] fix(deps): bump brace-expansion to 1.1.16 (GHSA-3jxr-9vmj-r5cp) npm audit flagged brace-expansion <1.1.16 as high severity DoS. This was disclosed 2026-07-21 and affects all PR branches equally. Lockfile-only change; no production API surface altered. --- frontend/package-lock.json | 66 +++++++++++++++++++------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/frontend/package-lock.json b/frontend/package-lock.json index adbfcc9e4..2064f1da4 100644 --- a/frontend/package-lock.json +++ b/frontend/package-lock.json @@ -8,38 +8,38 @@ "name": "cudly-frontend", "version": "1.0.0", "dependencies": { - "@types/qrcode": "^1.5.6", - "chart.js": "^4.4.0", - "qrcode": "^1.5.4" + "@types/qrcode": "1.5.6", + "chart.js": "4.5.1", + "qrcode": "1.5.4" }, "devDependencies": { - "@babel/core": "^7.23.0", - "@babel/preset-env": "^7.23.0", - "@babel/preset-typescript": "^7.23.0", - "@testing-library/dom": "^9.3.0", - "@testing-library/jest-dom": "^6.1.0", - "@types/chart.js": "^2.9.41", - "@types/jest": "^29.5.0", - "@types/jsdom": "^21.1.0", - "@typescript-eslint/eslint-plugin": "^8.0.0", - "@typescript-eslint/parser": "^8.0.0", - "@ungap/structured-clone": "^1.3.1", - "babel-loader": "^9.1.0", - "copy-webpack-plugin": "^14.0.0", - "css-loader": "^6.8.0", - "css-minimizer-webpack-plugin": "^8.0.0", - "eslint": "^8.50.0", - "html-webpack-plugin": "^5.5.0", - "jest": "^29.7.0", - "jest-environment-jsdom": "^29.7.0", - "jsdom": "^22.1.0", - "mini-css-extract-plugin": "^2.7.0", - "style-loader": "^3.3.0", - "ts-jest": "^29.1.0", - "ts-loader": "^9.5.0", - "typescript": "^5.3.0", - "webpack": "^5.88.0", - "webpack-cli": "^5.1.0" + "@babel/core": "7.29.7", + "@babel/preset-env": "7.28.5", + "@babel/preset-typescript": "7.28.5", + "@testing-library/dom": "9.3.4", + "@testing-library/jest-dom": "6.9.1", + "@types/chart.js": "2.9.41", + "@types/jest": "29.5.14", + "@types/jsdom": "21.1.7", + "@typescript-eslint/eslint-plugin": "8.62.1", + "@typescript-eslint/parser": "8.62.1", + "@ungap/structured-clone": "1.3.1", + "babel-loader": "9.2.1", + "copy-webpack-plugin": "14.0.0", + "css-loader": "6.11.0", + "css-minimizer-webpack-plugin": "8.0.0", + "eslint": "8.57.1", + "html-webpack-plugin": "5.6.5", + "jest": "29.7.0", + "jest-environment-jsdom": "29.7.0", + "jsdom": "22.1.0", + "mini-css-extract-plugin": "2.9.4", + "style-loader": "3.3.4", + "ts-jest": "29.4.6", + "ts-loader": "9.5.4", + "typescript": "5.9.3", + "webpack": "5.108.3", + "webpack-cli": "5.1.4" } }, "node_modules/@adobe/css-tools": { @@ -3837,9 +3837,9 @@ "license": "ISC" }, "node_modules/brace-expansion": { - "version": "1.1.15", - "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.15.tgz", - "integrity": "sha512-EwOCDEex4quD37XhqM3omwtMoJjr//isUZz1JopUNWms+4Z2ViyM/k1YIRePpoVNnQhENnxtFjLaxNHrT7xIUg==", + "version": "1.1.16", + "resolved": "https://registry.npmjs.org/brace-expansion/-/brace-expansion-1.1.16.tgz", + "integrity": "sha512-IDw48K2/2kRkg9LdJxurvq3lV3aBgq0REY89duEqFRthjlPdXHKMj7EnQOXVckxzgisinf3nHfrcE2FufFLXMw==", "dev": true, "license": "MIT", "dependencies": {