diff --git a/pkg/exchange/reshape_crossfamily_test.go b/pkg/exchange/reshape_crossfamily_test.go index 7d4eb7f60..30f6d372d 100644 --- a/pkg/exchange/reshape_crossfamily_test.go +++ b/pkg/exchange/reshape_crossfamily_test.go @@ -465,7 +465,7 @@ func TestCompositeScore_SameGenOutranksTermMismatch(t *testing.T) { } // m6i.xlarge: same "m" prefix (gen jump bonus), exact NF, high confidence. nearPerfect := OfferingOption{ - InstanceType: "m6i.xlarge", + InstanceType: "m6i.xlarge", EffectiveMonthlyCost: 85, // slightly more expensive NormalizationFactor: 8, SavingsAbs: floatPtr(220), @@ -473,7 +473,7 @@ func TestCompositeScore_SameGenOutranksTermMismatch(t *testing.T) { } // r5.xlarge: different prefix (no family gen bonus), same NF, no confidence. crossFamily := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 80, // same cost as source -- better raw price NormalizationFactor: 8, } @@ -497,13 +497,13 @@ func TestCompositeScore_SameArchOutranksCrossArch(t *testing.T) { } // c6i: same "c" prefix (family gen bonus), Intel x86 (same arch). sameArch := OfferingOption{ - InstanceType: "c6i.xlarge", + InstanceType: "c6i.xlarge", EffectiveMonthlyCost: 90, NormalizationFactor: 8, } // c6g: same "c" prefix (family gen bonus), Graviton ARM (cross arch). crossArch := OfferingOption{ - InstanceType: "c6g.xlarge", + InstanceType: "c6g.xlarge", EffectiveMonthlyCost: 90, NormalizationFactor: 8, } @@ -522,14 +522,14 @@ func TestCompositeScore_HighConfidenceOutranksLow(t *testing.T) { MonthlyCost: 70, } highConf := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(250), RecommendationCount: 4, } lowConf := OfferingOption{ - InstanceType: "r5.xlarge", + InstanceType: "r5.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(10), @@ -550,13 +550,13 @@ func TestCompositeScore_AbsentSavingsIsNeutral(t *testing.T) { MonthlyCost: 70, } absentSavings := OfferingOption{ - InstanceType: "r6i.xlarge", + InstanceType: "r6i.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: nil, // not supplied } lowConf := OfferingOption{ - InstanceType: "r6i.xlarge", + InstanceType: "r6i.xlarge", EffectiveMonthlyCost: 70, NormalizationFactor: 8, SavingsAbs: floatPtr(5), // explicitly low confidence diff --git a/providers/azure/internal/recommendations/converter.go b/providers/azure/internal/recommendations/converter.go index a18736184..b9471b360 100644 --- a/providers/azure/internal/recommendations/converter.go +++ b/providers/azure/internal/recommendations/converter.go @@ -40,7 +40,15 @@ type ExtractedFields struct { CommitmentCost float64 EstimatedSavings float64 Term string - Scope string + // Scope is populated from the API response ("Shared" or a subscription ID) + // but is not yet threaded into the purchase body. All service clients + // currently hardcode "appliedScopeType": "Shared", which is correct because + // the recommendation filter in recommendations.go already asserts + // "properties/scope eq 'Shared'" — only Shared-scope recommendations are + // ever requested. This field is retained for future wiring if single- + // subscription scoped recommendations are ever requested. See finding M1/M2 + // in docs/code-review/09-provider-azure.md. + Scope string // RecurringMonthlyCost is the monthly recurring charge for this // commitment. Azure Reservation recommendations are all all-upfront // (a single payment, no recurring monthly charge), so this is always diff --git a/providers/azure/mocks/azure_mocks.go b/providers/azure/mocks/azure_mocks.go index 23e9b3d6e..d5e11e1d3 100644 --- a/providers/azure/mocks/azure_mocks.go +++ b/providers/azure/mocks/azure_mocks.go @@ -206,7 +206,9 @@ func CreateSampleReservationDetails(subscriptionID, region string) []*armconsump } } -// CreateSampleVMPricingResponse creates a sample VM pricing response for testing +// CreateSampleVMPricingResponse creates a sample VM pricing response for testing. +// Includes both 1-year and 3-year reservation entries so 3YearTerm tests exercise +// real pricing rather than a fabricated fallback. func CreateSampleVMPricingResponse() string { return `{ "Items": [ @@ -218,7 +220,18 @@ func CreateSampleVMPricingResponse() string { "productName": "Virtual Machines D Series", "serviceName": "Virtual Machines", "armSkuName": "Standard_D2s_v3", - "reservationTerm": "1 Years", + "reservationTerm": "1 Year", + "type": "Reservation" + }, + { + "currencyCode": "USD", + "retailPrice": 1200.0, + "unitPrice": 0.096, + "armRegionName": "eastus", + "productName": "Virtual Machines D Series", + "serviceName": "Virtual Machines", + "armSkuName": "Standard_D2s_v3", + "reservationTerm": "3 Years", "type": "Reservation" }, { diff --git a/providers/azure/provider.go b/providers/azure/provider.go index 568573158..2817e7120 100644 --- a/providers/azure/provider.go +++ b/providers/azure/provider.go @@ -165,6 +165,15 @@ func (p *AzureProvider) DisplayName() string { } // IsConfigured checks if Azure credentials are available. Thread-safe via sync.Once. +// +// Sticky-failure contract: when ambient credential resolution fails (e.g. a +// transient IMDS timeout during startup), the error is memoised under +// sync.Once. Subsequent calls return false for the process lifetime with no +// retry. For long-lived server deployments this means a transient auth failure +// at boot requires a process restart to recover. This is acceptable for the +// current Lambda/container deployment model where restarts are cheap; if a +// long-lived daemon pattern is introduced, replace the sync.Once with a +// time-bounded cache or single-flight retry. func (p *AzureProvider) IsConfigured() bool { // If credential was injected via SetCredential, skip the Once path. if p.cred != nil { @@ -191,7 +200,7 @@ func (p *AzureProvider) IsConfigured() bool { // GetCredentials returns Azure credentials func (p *AzureProvider) GetCredentials() (provider.Credentials, error) { if !p.IsConfigured() { - return nil, fmt.Errorf("Azure is not configured") + return nil, fmt.Errorf("azure provider is not configured") } // DefaultAzureCredential can use multiple sources @@ -206,7 +215,7 @@ func (p *AzureProvider) GetCredentials() (provider.Credentials, error) { // ValidateCredentials validates that Azure credentials are working func (p *AzureProvider) ValidateCredentials(ctx context.Context) error { if !p.IsConfigured() { - return fmt.Errorf("Azure is not configured") + return fmt.Errorf("azure provider is not configured") } // Use injected client if available (for testing) @@ -227,7 +236,7 @@ func (p *AzureProvider) ValidateCredentials(ctx context.Context) error { _, err := pager.NextPage(ctx) if err != nil { logging.Errorf("purchase[Azure]: subscriptions.NextPage failed after %s: %v", time.Since(t0), err) - return fmt.Errorf("Azure credentials validation failed: %w", err) + return fmt.Errorf("azure credentials validation failed: %w", err) } logging.Infof("purchase[Azure]: subscriptions.NextPage returned in %s", time.Since(t0)) return nil @@ -242,7 +251,7 @@ func (p *AzureProvider) ValidateCredentials(ctx context.Context) error { // where the STS-identified account is always the default). func (p *AzureProvider) GetAccounts(ctx context.Context) ([]common.Account, error) { if !p.IsConfigured() { - return nil, fmt.Errorf("Azure is not configured") + return nil, fmt.Errorf("azure provider is not configured") } // Use injected client if available (for testing) @@ -437,7 +446,7 @@ func (p *AzureProvider) GetSupportedServices() []common.ServiceType { // an extra GetAccounts round-trip per iteration. func (p *AzureProvider) GetServiceClient(ctx context.Context, service common.ServiceType, region string) (provider.ServiceClient, error) { if !p.IsConfigured() { - return nil, fmt.Errorf("Azure is not configured") + return nil, fmt.Errorf("azure provider is not configured") } // Use explicit subscription ID if configured; otherwise resolve from accounts. @@ -458,7 +467,7 @@ func (p *AzureProvider) GetServiceClient(ctx context.Context, service common.Ser // returned by GetAccounts to avoid O(n) redundant API calls. func (p *AzureProvider) GetServiceClientForAccount(ctx context.Context, service common.ServiceType, region, subscriptionID string) (provider.ServiceClient, error) { if !p.IsConfigured() { - return nil, fmt.Errorf("Azure is not configured") + return nil, fmt.Errorf("azure provider is not configured") } if subscriptionID == "" { return nil, fmt.Errorf("subscriptionID must not be empty") @@ -499,7 +508,7 @@ func (p *AzureProvider) newServiceClientForSubscription(service common.ServiceTy // GetRecommendationsClientForAccount. func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider.RecommendationsClient, error) { if !p.IsConfigured() { - return nil, fmt.Errorf("Azure is not configured") + return nil, fmt.Errorf("azure provider is not configured") } // Use explicit subscription ID if configured; otherwise resolve from accounts. @@ -520,7 +529,7 @@ func (p *AzureProvider) GetRecommendationsClient(ctx context.Context) (provider. // returned by GetAccounts to avoid O(n) redundant API calls. func (p *AzureProvider) GetRecommendationsClientForAccount(ctx context.Context, subscriptionID string) (provider.RecommendationsClient, error) { if !p.IsConfigured() { - return nil, fmt.Errorf("Azure is not configured") + return nil, fmt.Errorf("azure provider is not configured") } if subscriptionID == "" { return nil, fmt.Errorf("subscriptionID must not be empty") diff --git a/providers/azure/provider_test.go b/providers/azure/provider_test.go index f728f1889..432c9d86c 100644 --- a/providers/azure/provider_test.go +++ b/providers/azure/provider_test.go @@ -306,7 +306,7 @@ func TestAzureProvider_GetCredentials_NotConfigured(t *testing.T) { if !p.IsConfigured() { _, err := p.GetCredentials() assert.Error(t, err) - assert.Contains(t, err.Error(), "Azure is not configured") + assert.Contains(t, err.Error(), "azure provider is not configured") } } @@ -319,7 +319,7 @@ func TestAzureProvider_ValidateCredentials(t *testing.T) { }) err := p.ValidateCredentials(context.Background()) assert.Error(t, err) - assert.Contains(t, err.Error(), "Azure is not configured") + assert.Contains(t, err.Error(), "azure provider is not configured") }) t.Run("success with mock subscriptions client", func(t *testing.T) { @@ -370,7 +370,7 @@ func TestAzureProvider_ValidateCredentials(t *testing.T) { err := p.ValidateCredentials(context.Background()) assert.Error(t, err) - assert.Contains(t, err.Error(), "Azure credentials validation failed") + assert.Contains(t, err.Error(), "azure credentials validation failed") }) } @@ -380,7 +380,7 @@ func TestAzureProvider_GetServiceClient_NotConfigured(t *testing.T) { if !p.IsConfigured() { _, err := p.GetServiceClient(context.Background(), common.ServiceCompute, "eastus") assert.Error(t, err) - assert.Contains(t, err.Error(), "Azure is not configured") + assert.Contains(t, err.Error(), "azure provider is not configured") } } @@ -434,7 +434,7 @@ func TestAzureProvider_GetRecommendationsClient_NotConfigured(t *testing.T) { if !p.IsConfigured() { _, err := p.GetRecommendationsClient(context.Background()) assert.Error(t, err) - assert.Contains(t, err.Error(), "Azure is not configured") + assert.Contains(t, err.Error(), "azure provider is not configured") } } @@ -811,7 +811,7 @@ func TestAzureProvider_GetCredentials(t *testing.T) { }) _, err := p.GetCredentials() assert.Error(t, err) - assert.Contains(t, err.Error(), "Azure is not configured") + assert.Contains(t, err.Error(), "azure provider is not configured") }) t.Run("success returns credentials info", func(t *testing.T) { @@ -1194,7 +1194,7 @@ func TestAzureProvider_GetServiceClientForAccount(t *testing.T) { p.SetCredentialProvider(&mockCredentialProvider{err: errors.New("no cred")}) _, err := p.GetServiceClientForAccount(context.Background(), common.ServiceCompute, "eastus", "sub-1") assert.Error(t, err) - assert.Contains(t, err.Error(), "Azure is not configured") + assert.Contains(t, err.Error(), "azure provider is not configured") }) } @@ -1218,6 +1218,6 @@ func TestAzureProvider_GetRecommendationsClientForAccount(t *testing.T) { p.SetCredentialProvider(&mockCredentialProvider{err: errors.New("no cred")}) _, err := p.GetRecommendationsClientForAccount(context.Background(), "sub-1") assert.Error(t, err) - assert.Contains(t, err.Error(), "Azure is not configured") + assert.Contains(t, err.Error(), "azure provider is not configured") }) } diff --git a/providers/azure/recommendations.go b/providers/azure/recommendations.go index 6968c2113..8fe3719cd 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -236,7 +236,11 @@ func (r *RecommendationsClientAdapter) getAdvisorRecommendations(ctx context.Con for pager.More() { page, err := pager.NextPage(ctx) if err != nil { - logging.Warnf("Azure Advisor pagination error (partial results may be returned): %v", err) + // Advisor partial-OK is intentional: per-service reservation results + // (compute, database, etc.) remain useful on their own when Advisor + // pagination fails mid-stream. Warnf with the accumulated count so + // operators can assess the completeness of the Advisor slice. + logging.Warnf("Azure Advisor pagination error after %d recommendations (partial results returned): %v", len(recommendations), err) break } recommendations = r.appendAdvisorPageRecs(params, page, recommendations) @@ -378,13 +382,13 @@ func serviceFromImpactedField(field *string) string { } f := *field switch { - case contains(f, "Microsoft.Compute"): + case strings.Contains(f, "Microsoft.Compute"): return string(common.ServiceCompute) - case contains(f, "Microsoft.Sql"): + case strings.Contains(f, "Microsoft.Sql"): return string(common.ServiceRelationalDB) - case contains(f, "Microsoft.Cache"): + case strings.Contains(f, "Microsoft.Cache"): return string(common.ServiceCache) - case contains(f, "Microsoft.DBforMySQL"), contains(f, "Microsoft.DBforPostgreSQL"): + case strings.Contains(f, "Microsoft.DBforMySQL"), strings.Contains(f, "Microsoft.DBforPostgreSQL"): return string(common.ServiceRelationalDB) } return "" @@ -458,8 +462,3 @@ func shouldIncludeService(params common.RecommendationParams, service common.Ser // Check if this is the requested service return params.Service == service } - -// contains checks if a string contains a substring -func contains(s, substr string) bool { - return strings.Contains(s, substr) -} diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index ecbd1fd41..efdca18dc 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -2,6 +2,7 @@ package azure import ( "context" + "strings" "testing" "github.com/Azure/azure-sdk-for-go/sdk/azcore" @@ -126,7 +127,7 @@ func TestContains(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := contains(tt.s, tt.substr) + result := strings.Contains(tt.s, tt.substr) assert.Equal(t, tt.expected, result) }) } @@ -414,3 +415,40 @@ func TestConvertAdvisorRecommendation_UnknownService(t *testing.T) { func strPtr(s string) *string { return &s } + +// TestMergeServiceResults_OrderIsStable is a regression test for L3: +// the merged output of mergeServiceResults must preserve the canonical +// compute -> database -> cache -> cosmosdb -> savingsplans -> advisor order +// regardless of the order arguments are passed in. This test pins the actual +// argument order used in GetRecommendations so a reordering of the serviceResult +// literals will be caught immediately. +func TestMergeServiceResults_OrderIsStable(t *testing.T) { + mkRec := func(svc common.ServiceType, marker string) common.Recommendation { + return common.Recommendation{Service: svc, Provider: common.ProviderAzure, ResourceType: marker} + } + + computeRec := mkRec(common.ServiceCompute, "compute") + dbRec := mkRec(common.ServiceRelationalDB, "database") + cacheRec := mkRec(common.ServiceCache, "cache") + cosmosRec := mkRec(common.ServiceNoSQL, "cosmosdb") + spRec := mkRec(common.ServiceSavingsPlans, "savingsplans") + advisorRec := mkRec(common.ServiceCompute, "advisor") // Advisor produces Compute recs + + // Replicate the exact call order from GetRecommendations. + result := mergeServiceResults( + serviceResult{"compute", []common.Recommendation{computeRec}, nil}, + serviceResult{"database", []common.Recommendation{dbRec}, nil}, + serviceResult{"cache", []common.Recommendation{cacheRec}, nil}, + serviceResult{"cosmosdb", []common.Recommendation{cosmosRec}, nil}, + serviceResult{"savingsplans", []common.Recommendation{spRec}, nil}, + serviceResult{"advisor", []common.Recommendation{advisorRec}, nil}, + ) + + require.Len(t, result, 6, "all six services must be represented") + assert.Equal(t, "compute", result[0].ResourceType, "first must be compute") + assert.Equal(t, "database", result[1].ResourceType, "second must be database") + assert.Equal(t, "cache", result[2].ResourceType, "third must be cache") + assert.Equal(t, "cosmosdb", result[3].ResourceType, "fourth must be cosmosdb") + assert.Equal(t, "savingsplans", result[4].ResourceType, "fifth must be savingsplans") + assert.Equal(t, "advisor", result[5].ResourceType, "sixth must be advisor (compute-type)") +} diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 9e45643ac..266e27816 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -303,10 +303,15 @@ func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recomme result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } + if rec.Count <= 0 { + result.Error = fmt.Errorf("quantity must be greater than zero, got %d", rec.Count) + return result, result.Error + } - termYears := 1 - if rec.Term == "3yr" || rec.Term == "3" { - termYears = 3 + termYears, termErr := reservations.ParseTermYears(rec.Term) + if termErr != nil { + result.Error = termErr + return result, result.Error } requestBody := map[string]interface{}{ @@ -379,9 +384,9 @@ func (c *CacheClient) ValidateOffering(ctx context.Context, rec common.Recommend // GetOfferingDetails retrieves Redis Cache reservation offering details from Azure Retail Prices API func (c *CacheClient) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { - termYears := 1 - if rec.Term == "3yr" || rec.Term == "3" { - termYears = 3 + termYears, err := reservations.ParseTermYears(rec.Term) + if err != nil { + return nil, fmt.Errorf("invalid term: %w", err) } pricing, err := c.getRedisPricing(ctx, rec.ResourceType, c.region, termYears) @@ -400,7 +405,10 @@ func (c *CacheClient) GetOfferingDetails(ctx context.Context, rec common.Recomme upfrontCost = 0 recurringCost = totalCost / (float64(termYears) * 12) default: - upfrontCost = totalCost + // Fail loud on an unrecognised payment option rather than silently + // billing it as all-upfront (owner policy: no silent fallbacks on + // money-affecting fields). + return nil, fmt.Errorf("unsupported payment option for Azure Cache for Redis offering details: %q", rec.PaymentOption) } return &common.OfferingDetails{ @@ -549,8 +557,13 @@ func (c *CacheClient) getRedisPricing(ctx context.Context, sku, region string, t } hoursInTerm := 8760.0 * float64(termYears) + // Return an error rather than fabricating a reservation price from a + // hardcoded discount multiplier (issue #1020 H4). Presenting an + // estimated figure as a real TotalCost/SavingsPercentage is misleading + // and can justify uneconomical purchases. managedredis already uses + // this pattern as the model. if reservationPrice == 0 { - reservationPrice = onDemandPrice * hoursInTerm * 0.45 // 55% savings + return nil, fmt.Errorf("no reservation pricing found for Redis Cache SKU %s (%d year) in region %s", sku, termYears, region) } savingsPercentage := ((onDemandPrice*hoursInTerm - reservationPrice) / (onDemandPrice * hoursInTerm)) * 100 @@ -583,10 +596,20 @@ func (c *CacheClient) fetchAzurePricing(ctx context.Context, serviceName, sku, r return &AzureRetailPrice{Items: items}, nil } +// azureTermString returns the Retail Prices API ReservationTerm string for the +// given number of years. The API uses the singular form "1 Year" for one year +// and the plural form "N Years" for two or more years. +func azureTermString(termYears int) string { + if termYears == 1 { + return "1 Year" + } + return fmt.Sprintf("%d Years", termYears) +} + // extractRedisPricing extracts on-demand and reservation pricing from price items func extractRedisPricing(items []CacheRetailPriceItem, termYears int) (onDemand, reservation float64, currency string) { currency = "USD" - termStr := fmt.Sprintf("%d Years", termYears) + termStr := azureTermString(termYears) for _, item := range items { if item.CurrencyCode != "" { diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 9333c510e..14397c747 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -122,7 +122,19 @@ func createSampleRedisPricingResponse() string { "serviceName": "Azure Cache for Redis", "armSkuName": "Premium_P1", "meterName": "P1 Instance", - "reservationTerm": "1 Years", + "reservationTerm": "1 Year", + "type": "Reservation" + }, + { + "currencyCode": "USD", + "retailPrice": 850.0, + "unitPrice": 850.0, + "armRegionName": "eastus", + "productName": "Azure Cache for Redis", + "serviceName": "Azure Cache for Redis", + "armSkuName": "Premium_P1", + "meterName": "P1 Instance", + "reservationTerm": "3 Years", "type": "Reservation" }, { @@ -371,6 +383,42 @@ func TestCacheClient_GetOfferingDetails_NoPricing(t *testing.T) { assert.Contains(t, err.Error(), "no pricing data found") } +// TestCacheClient_GetOfferingDetails_NoReservationPricing verifies that when +// on-demand pricing is present but no reservation line is returned, the client +// returns an error rather than fabricating a price from the hardcoded 0.45 +// multiplier (issue #1020 H4). Pre-fix this would have silently surfaced a +// fabricated TotalCost/SavingsPercentage as a real quote. +func TestCacheClient_GetOfferingDetails_NoReservationPricing(t *testing.T) { + ctx := context.Background() + + onDemandOnly := `{ + "Items": [ + { + "currencyCode": "USD", + "retailPrice": 0.125, + "unitPrice": 0.125, + "armRegionName": "eastus", + "armSkuName": "Premium_P1", + "type": "Consumption" + } + ], + "NextPageLink": "" + }` + + mockHTTP := &MockHTTPClient{} + client := NewClientWithHTTP(nil, "test-subscription", "eastus", mockHTTP) + mockHTTP.On("Do", mock.Anything).Return(createMockHTTPResponse(http.StatusOK, onDemandOnly), nil) + + rec := common.Recommendation{ + ResourceType: "Premium_P1", + Term: "1yr", + PaymentOption: "upfront", + } + _, err := client.GetOfferingDetails(ctx, rec) + require.Error(t, err) + assert.Contains(t, err.Error(), "no reservation pricing found") +} + func TestCacheClient_GetExistingCommitments_Empty(t *testing.T) { ctx := context.Background() client := NewClient(nil, "test-subscription", "eastus") @@ -1030,6 +1078,7 @@ func TestCacheClient_PurchaseCommitment_TokenError(t *testing.T) { rec := common.Recommendation{ ResourceType: "Premium_P1", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -1051,6 +1100,7 @@ func TestCacheClient_PurchaseCommitment_HTTPError(t *testing.T) { rec := common.Recommendation{ ResourceType: "Premium_P1", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -1075,6 +1125,7 @@ func TestCacheClient_PurchaseCommitment_BadStatus(t *testing.T) { rec := common.Recommendation{ ResourceType: "Premium_P1", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -1234,3 +1285,47 @@ func TestCacheClient_GetValidResourceTypes_PageCapFires(t *testing.T) { require.NoError(t, err) require.NotEmpty(t, skus, "page cap must trigger fallback to common SKUs, not an empty result") } + +// TestExtractRedisPricing_SingularOneYear verifies that extractRedisPricing +// correctly recognises the "1 Year" singular form returned by the Azure Retail +// Prices API for 1-year reservation terms. Before the fix, the extractor used +// "%d Years" unconditionally, so the 1-year reservation line was silently +// skipped and reservationPrice remained 0, causing a false "no reservation +// pricing found" error even when the pricing row was present. +func TestExtractRedisPricing_SingularOneYear(t *testing.T) { + items := []CacheRetailPriceItem{ + { + CurrencyCode: "USD", + RetailPrice: 0.68, + UnitPrice: 0.68, + Type: "Consumption", + }, + { + CurrencyCode: "USD", + RetailPrice: 4500.0, + ReservationTerm: "1 Year", + Type: "Reservation", + }, + } + + onDemand, reservation, currency := extractRedisPricing(items, 1) + + assert.Equal(t, "USD", currency) + assert.Equal(t, 0.68, onDemand, "on-demand price must be extracted") + assert.Equal(t, 4500.0, reservation, "reservation price for '1 Year' term must be extracted") +} + +// TestExtractRedisPricing_PluralThreeYears verifies that the plural form +// "3 Years" continues to work for multi-year terms. +func TestExtractRedisPricing_PluralThreeYears(t *testing.T) { + items := []CacheRetailPriceItem{ + {CurrencyCode: "USD", RetailPrice: 0.68, UnitPrice: 0.68, Type: "Consumption"}, + {CurrencyCode: "USD", RetailPrice: 11000.0, ReservationTerm: "3 Years", Type: "Reservation"}, + } + + onDemand, reservation, currency := extractRedisPricing(items, 3) + + assert.Equal(t, "USD", currency) + assert.Equal(t, 0.68, onDemand) + assert.Equal(t, 11000.0, reservation, "reservation price for '3 Years' term must be extracted") +} diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index aeb974da9..77d175b4a 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -5,6 +5,7 @@ import ( "context" "encoding/json" "fmt" + "io" "log" "net/http" "net/url" @@ -339,34 +340,54 @@ func (c *ComputeClient) checkAndRegisterCapacityProvider(ctx context.Context) er return fmt.Errorf("get token: %w", err) } - apiVersion := "2021-04-01" + const apiVersion = "2021-04-01" + state, err := c.fetchCapacityProviderState(ctx, token.Token, apiVersion) + if err != nil { + return err + } + if state.RegistrationState == "Registered" { + return nil + } + return c.triggerCapacityProviderRegistration(ctx, token.Token, apiVersion, state.RegistrationState) +} + +// fetchCapacityProviderState queries the ARM providers API and returns the +// current registration state of Microsoft.Capacity. +func (c *ComputeClient) fetchCapacityProviderState(ctx context.Context, bearerToken, apiVersion string) (providerRegistrationState, error) { checkURL := fmt.Sprintf( "https://management.azure.com/subscriptions/%s/providers/Microsoft.Capacity?api-version=%s", c.subscriptionID, apiVersion, ) - req, err := http.NewRequestWithContext(ctx, http.MethodGet, checkURL, nil) if err != nil { - return fmt.Errorf("build request: %w", err) + return providerRegistrationState{}, fmt.Errorf("build request: %w", err) } - req.Header.Set("Authorization", "Bearer "+token.Token) + req.Header.Set("Authorization", "Bearer "+bearerToken) resp, err := c.httpClient.Do(req) if err != nil { - return fmt.Errorf("check provider: %w", err) + return providerRegistrationState{}, fmt.Errorf("check provider: %w", err) } - defer resp.Body.Close() + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() - var state providerRegistrationState - if err := json.NewDecoder(resp.Body).Decode(&state); err != nil { - return fmt.Errorf("decode provider state: %w", err) + if resp.StatusCode < 200 || resp.StatusCode >= 300 { + // Non-2xx from the provider check (e.g. 403 permissions, 429 throttle). + // Log and return so the purchase attempt can still proceed; a failed + // registration check is non-fatal. + return providerRegistrationState{}, fmt.Errorf("check Microsoft.Capacity provider status returned HTTP %d: %s", resp.StatusCode, string(body)) } - if state.RegistrationState == "Registered" { - return nil + var state providerRegistrationState + if err := json.Unmarshal(body, &state); err != nil { + return providerRegistrationState{}, fmt.Errorf("decode provider state: %w", err) } + return state, nil +} - // Trigger registration +// triggerCapacityProviderRegistration POSTs to the ARM register endpoint to +// initiate Microsoft.Capacity provider registration. +func (c *ComputeClient) triggerCapacityProviderRegistration(ctx context.Context, bearerToken, apiVersion, currentState string) error { registerURL := fmt.Sprintf( "https://management.azure.com/subscriptions/%s/providers/Microsoft.Capacity/register?api-version=%s", c.subscriptionID, apiVersion, @@ -375,14 +396,19 @@ func (c *ComputeClient) checkAndRegisterCapacityProvider(ctx context.Context) er if err != nil { return fmt.Errorf("build register request: %w", err) } - regReq.Header.Set("Authorization", "Bearer "+token.Token) + regReq.Header.Set("Authorization", "Bearer "+bearerToken) + regResp, err := c.httpClient.Do(regReq) if err != nil { return fmt.Errorf("register provider: %w", err) } + regBody, _ := io.ReadAll(regResp.Body) regResp.Body.Close() + if regResp.StatusCode < 200 || regResp.StatusCode >= 300 { + return fmt.Errorf("register Microsoft.Capacity provider returned HTTP %d: %s", regResp.StatusCode, string(regBody)) + } - log.Printf("Triggered Microsoft.Capacity provider registration (was: %q)", state.RegistrationState) + log.Printf("Triggered Microsoft.Capacity provider registration (was: %q)", currentState) return nil } @@ -393,9 +419,9 @@ func (c *ComputeClient) checkAndRegisterCapacityProvider(ctx context.Context) er // in the portal AND a re-driven purchase can find it via tag lookup before // buying a duplicate (issue #721). func (c *ComputeClient) buildReservationBody(rec common.Recommendation, source, idempotencyToken string) ([]byte, error) { - termYears := 1 - if rec.Term == "3yr" || rec.Term == "3" { - termYears = 3 + termYears, err := reservations.ParseTermYears(rec.Term) + if err != nil { + return nil, err } requestBody := map[string]interface{}{ "sku": map[string]string{"name": rec.ResourceType}, @@ -454,6 +480,10 @@ func (c *ComputeClient) PurchaseCommitment(ctx context.Context, rec common.Recom result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } + if rec.Count <= 0 { + result.Error = fmt.Errorf("quantity must be greater than zero, got %d", rec.Count) + return result, result.Error + } // Ensure Microsoft.Capacity provider is registered (cached after first call). c.ensureCapacityProviderRegistered(ctx) @@ -503,9 +533,9 @@ func (c *ComputeClient) ValidateOffering(ctx context.Context, rec common.Recomme // GetOfferingDetails retrieves VM RI offering details from Azure Retail Prices API func (c *ComputeClient) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { - termYears := 1 - if rec.Term == "3yr" || rec.Term == "3" { - termYears = 3 + termYears, err := reservations.ParseTermYears(rec.Term) + if err != nil { + return nil, fmt.Errorf("invalid term: %w", err) } pricing, err := c.getVMPricing(ctx, rec.ResourceType, c.region, termYears) @@ -524,7 +554,10 @@ func (c *ComputeClient) GetOfferingDetails(ctx context.Context, rec common.Recom upfrontCost = 0 recurringCost = totalCost / (float64(termYears) * 12) default: - upfrontCost = totalCost + // Fail loud on an unrecognised payment option rather than silently + // billing it as all-upfront (owner policy: no silent fallbacks on + // money-affecting fields). + return nil, fmt.Errorf("unsupported payment option for Azure VM offering details: %q", rec.PaymentOption) } return &common.OfferingDetails{ @@ -657,8 +690,13 @@ func (c *ComputeClient) getVMPricing(ctx context.Context, vmSize, region string, } hoursInTerm := 8760.0 * float64(termYears) + // Return an error rather than fabricating a reservation price from a + // hardcoded discount multiplier (issue #1020 H4). Presenting an + // estimated figure as a real TotalCost/SavingsPercentage is misleading + // and can justify uneconomical purchases. managedredis already uses + // this pattern as the model. if reservationPrice == 0 { - reservationPrice = onDemandPrice * hoursInTerm * 0.62 // Azure VMs typically 38% discount + return nil, fmt.Errorf("no reservation pricing found for VM size %s (%d year) in region %s", vmSize, termYears, region) } savingsPercentage := ((onDemandPrice*hoursInTerm - reservationPrice) / (onDemandPrice * hoursInTerm)) * 100 @@ -696,10 +734,20 @@ func (c *ComputeClient) fetchAzurePricing(ctx context.Context, filter string) (* return &AzureRetailPrice{Items: items}, nil } +// azureTermString returns the Retail Prices API ReservationTerm string for the +// given number of years. The API uses the singular form "1 Year" for one year +// and the plural form "N Years" for two or more years. +func azureTermString(termYears int) string { + if termYears == 1 { + return "1 Year" + } + return fmt.Sprintf("%d Years", termYears) +} + // extractVMPricing extracts on-demand and reservation pricing from price items func extractVMPricing(items []AzureRetailPriceItem, termYears int) (onDemand, reservation float64, currency string) { currency = "USD" - termStr := fmt.Sprintf("%d Years", termYears) + termStr := azureTermString(termYears) for _, item := range items { if item.CurrencyCode != "" { diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index 59dc60ad6..520bf0393 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -506,6 +506,72 @@ func TestComputeClient_GetOfferingDetails_NoPricing(t *testing.T) { assert.Contains(t, err.Error(), "no pricing data found") } +// TestComputeClient_GetOfferingDetails_UnsupportedPaymentOption verifies that an +// unrecognised payment option fails loud rather than being silently billed as +// all-upfront (owner policy: no silent fallbacks on money-affecting fields). +// Pricing is fully present, so the failure is solely on the payment-option +// branch. Pre-fix the default branch set upfrontCost = totalCost and returned a +// valid OfferingDetails with no error. +func TestComputeClient_GetOfferingDetails_UnsupportedPaymentOption(t *testing.T) { + ctx := context.Background() + + mockHTTP := &mocks.MockHTTPClient{} + client := NewClientWithHTTP(nil, "test-subscription", "eastus", mockHTTP) + mockHTTP.On("Do", mock.Anything).Return( + mocks.CreateMockHTTPResponse(http.StatusOK, mocks.CreateSampleVMPricingResponse()), + nil, + ) + + rec := common.Recommendation{ + ResourceType: "Standard_D2s_v3", + Term: "1yr", + PaymentOption: "weekly-bananas", + } + + details, err := client.GetOfferingDetails(ctx, rec) + require.Error(t, err) + assert.Nil(t, details) + assert.Contains(t, err.Error(), "unsupported payment option") +} + +// TestComputeClient_GetOfferingDetails_NoReservationPricing verifies that when +// on-demand pricing is present but no reservation line is returned, the client +// returns an error rather than fabricating a price from the hardcoded 0.62 +// multiplier (issue #1020 H4). Pre-fix this would have silently surfaced a +// fabricated TotalCost/SavingsPercentage as a real quote. +func TestComputeClient_GetOfferingDetails_NoReservationPricing(t *testing.T) { + ctx := context.Background() + + onDemandOnly := `{ + "Items": [ + { + "currencyCode": "USD", + "retailPrice": 0.096, + "unitPrice": 0.096, + "armRegionName": "eastus", + "armSkuName": "Standard_D2s_v3", + "type": "Consumption" + } + ], + "NextPageLink": "" + }` + + mockHTTP := &mocks.MockHTTPClient{} + client := NewClientWithHTTP(nil, "test-subscription", "eastus", mockHTTP) + mockHTTP.On("Do", mock.Anything).Return( + mocks.CreateMockHTTPResponse(http.StatusOK, onDemandOnly), nil, + ) + + rec := common.Recommendation{ + ResourceType: "Standard_D2s_v3", + Term: "1yr", + PaymentOption: "upfront", + } + _, err := client.GetOfferingDetails(ctx, rec) + require.Error(t, err) + assert.Contains(t, err.Error(), "no reservation pricing found") +} + // MockTokenCredential for testing PurchaseCommitment type MockTokenCredential struct { token string @@ -641,6 +707,7 @@ func TestComputeClient_PurchaseCommitment_TokenError(t *testing.T) { rec := common.Recommendation{ ResourceType: "Standard_D2s_v3", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -664,6 +731,7 @@ func TestComputeClient_PurchaseCommitment_HTTPError(t *testing.T) { rec := common.Recommendation{ ResourceType: "Standard_D2s_v3", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -693,6 +761,7 @@ func TestComputeClient_PurchaseCommitment_BadStatus(t *testing.T) { rec := common.Recommendation{ ResourceType: "Standard_D2s_v3", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -1221,3 +1290,94 @@ func TestComputeClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t assert.Regexp(t, `^vm-`, capturedDisplayName) assert.Contains(t, capturedDisplayName, "Standard_D2s_v3") } + +// TestCheckAndRegisterCapacityProvider_NonTwoxx is a regression test for M3: +// before this fix, checkAndRegisterCapacityProvider decoded the body +// unconditionally without checking resp.StatusCode, so a 403/429 with a +// non-JSON body would produce a misleading "decode provider state" error +// instead of a clear HTTP status error. +func TestCheckAndRegisterCapacityProvider_NonTwoxx(t *testing.T) { + ctx := context.Background() + mockHTTP := &mocks.MockHTTPClient{} + t.Cleanup(func() { mockHTTP.AssertExpectations(t) }) + cred := &MockTokenCredential{token: "tok"} + client := NewClientWithHTTP(cred, "sub", "eastus", mockHTTP) + + // Return a 403 Forbidden instead of 200 Registered. + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodGet && + strings.Contains(r.URL.Path, "providers/Microsoft.Capacity") && + !strings.Contains(r.URL.Path, "reservationOrders") && + !strings.Contains(r.URL.Path, "calculatePrice") + })).Return(mocks.CreateMockHTTPResponse(http.StatusForbidden, `{"error":"AuthorizationFailed"}`), nil).Once() + + err := client.checkAndRegisterCapacityProvider(ctx) + require.Error(t, err) + assert.Contains(t, err.Error(), "403", "error must surface the HTTP status code") +} + +// TestExtractVMPricing_SingularOneYear verifies that extractVMPricing correctly +// recognises the "1 Year" singular form returned by the Azure Retail Prices API +// for 1-year reservation terms. Before the fix, the extractor used "%d Years" +// unconditionally, so the 1-year reservation line was silently skipped and +// reservationPrice remained 0, causing a false "no reservation pricing found" +// error even when the pricing row was present. +func TestExtractVMPricing_SingularOneYear(t *testing.T) { + items := []AzureRetailPriceItem{ + { + CurrencyCode: "USD", + RetailPrice: 0.096, + UnitPrice: 0.096, + ArmRegionName: "eastus", + ArmSKUName: "Standard_D2s_v3", + Type: "Consumption", + }, + { + CurrencyCode: "USD", + RetailPrice: 730.0, + ArmRegionName: "eastus", + ArmSKUName: "Standard_D2s_v3", + ReservationTerm: "1 Year", + Type: "Reservation", + }, + } + + onDemand, reservation, currency := extractVMPricing(items, 1) + + assert.Equal(t, "USD", currency) + assert.Equal(t, 0.096, onDemand, "on-demand price must be extracted") + assert.Equal(t, 730.0, reservation, "reservation price for '1 Year' term must be extracted") +} + +// TestExtractVMPricing_PluralThreeYears verifies that the plural form "3 Years" +// continues to work for multi-year terms. +func TestExtractVMPricing_PluralThreeYears(t *testing.T) { + items := []AzureRetailPriceItem{ + {CurrencyCode: "USD", RetailPrice: 0.096, UnitPrice: 0.096, Type: "Consumption"}, + {CurrencyCode: "USD", RetailPrice: 1900.0, ReservationTerm: "3 Years", Type: "Reservation"}, + } + + onDemand, reservation, currency := extractVMPricing(items, 3) + + assert.Equal(t, "USD", currency) + assert.Equal(t, 0.096, onDemand) + assert.Equal(t, 1900.0, reservation, "reservation price for '3 Years' term must be extracted") +} + +// TestComputeClient_PurchaseCommitment_ZeroCountRejected is a regression test +// for M6: PurchaseCommitment must reject Count==0 before any HTTP call. +func TestComputeClient_PurchaseCommitment_ZeroCountRejected(t *testing.T) { + ctx := context.Background() + mockHTTP := &mocks.MockHTTPClient{} + t.Cleanup(func() { mockHTTP.AssertExpectations(t) }) + cred := &MockTokenCredential{token: "tok"} + client := NewClientWithHTTP(cred, "sub", "eastus", mockHTTP) + + result, err := client.PurchaseCommitment(ctx, common.Recommendation{ + ResourceType: "Standard_D2s_v3", Term: "1yr", Count: 0, + }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.Error(t, err) + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "quantity must be greater than zero") + mockHTTP.AssertNotCalled(t, "Do", mock.Anything) +} diff --git a/providers/azure/services/compute/exchange.go b/providers/azure/services/compute/exchange.go index 0cc73105e..18f2f9e37 100644 --- a/providers/azure/services/compute/exchange.go +++ b/providers/azure/services/compute/exchange.go @@ -188,8 +188,15 @@ func convertToExchangeableReservation(item *armreservations.ReservationResponse) return nil } id, sku, region, term, displayName, quantity, expiryDate := extractReservationFields(item) + orderID := parseReservationOrderID(id) + // parseReservationOrderID returns "" for IDs that do not contain the expected + // "/reservationOrders/" segment (malformed or unexpected format). Reservations + // with an empty order ID are still returned here so the caller can include them + // in the inventory view, but callers MUST filter out empty-order-ID entries + // before initiating an exchange operation -- the Azure exchange API requires a + // non-empty reservationOrderId. return &ExchangeableReservation{ - ReservationOrderID: parseReservationOrderID(id), + ReservationOrderID: orderID, ReservationID: id, SKU: sku, Quantity: quantity, diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index e1bbaa5d9..5c619e3b5 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -296,10 +296,15 @@ func (c *CosmosDBClient) PurchaseCommitment(ctx context.Context, rec common.Reco result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } + if rec.Count <= 0 { + result.Error = fmt.Errorf("quantity must be greater than zero, got %d", rec.Count) + return result, result.Error + } - termYears := 1 - if rec.Term == "3yr" || rec.Term == "3" { - termYears = 3 + termYears, termErr := reservations.ParseTermYears(rec.Term) + if termErr != nil { + result.Error = termErr + return result, result.Error } requestBody := map[string]interface{}{ @@ -372,9 +377,9 @@ func (c *CosmosDBClient) ValidateOffering(ctx context.Context, rec common.Recomm // GetOfferingDetails retrieves Cosmos DB reservation offering details from Azure Retail Prices API func (c *CosmosDBClient) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { - termYears := 1 - if rec.Term == "3yr" || rec.Term == "3" { - termYears = 3 + termYears, err := reservations.ParseTermYears(rec.Term) + if err != nil { + return nil, fmt.Errorf("invalid term: %w", err) } pricing, err := c.getCosmosPricing(ctx, rec.ResourceType, c.region, termYears) @@ -393,7 +398,10 @@ func (c *CosmosDBClient) GetOfferingDetails(ctx context.Context, rec common.Reco upfrontCost = 0 recurringCost = totalCost / (float64(termYears) * 12) default: - upfrontCost = totalCost + // Fail loud on an unrecognised payment option rather than silently + // billing it as all-upfront (owner policy: no silent fallbacks on + // money-affecting fields). + return nil, fmt.Errorf("unsupported payment option for Azure Cosmos DB offering details: %q", rec.PaymentOption) } return &common.OfferingDetails{ @@ -541,8 +549,13 @@ func (c *CosmosDBClient) getCosmosPricing(ctx context.Context, sku, region strin } hoursInTerm := 8760.0 * float64(termYears) + // Return an error rather than fabricating a reservation price from a + // hardcoded discount multiplier (issue #1020 H4). Presenting an + // estimated figure as a real TotalCost/SavingsPercentage is misleading + // and can justify uneconomical purchases. managedredis already uses + // this pattern as the model. if reservationPrice == 0 { - reservationPrice = estimateCosmosReservationPrice(onDemandPrice, hoursInTerm) + return nil, fmt.Errorf("no reservation pricing found for Cosmos DB (%d year) in region %s", termYears, region) } savingsPercentage := calculateCosmosSavingsPercentage(onDemandPrice, hoursInTerm, reservationPrice) @@ -573,10 +586,20 @@ func (c *CosmosDBClient) fetchAzurePricing(ctx context.Context, filter string) ( return &AzureRetailPrice{Items: items}, nil } +// azureTermString returns the Retail Prices API ReservationTerm string for the +// given number of years. The API uses the singular form "1 Year" for one year +// and the plural form "N Years" for two or more years. +func azureTermString(termYears int) string { + if termYears == 1 { + return "1 Year" + } + return fmt.Sprintf("%d Years", termYears) +} + // extractCosmosPricing extracts on-demand and reservation pricing from price items func extractCosmosPricing(items []CosmosRetailPriceItem, termYears int) (onDemand, reservation float64, currency string) { currency = "USD" - termStr := fmt.Sprintf("%d Years", termYears) + termStr := azureTermString(termYears) for _, item := range items { if item.CurrencyCode != "" { @@ -593,13 +616,6 @@ func extractCosmosPricing(items []CosmosRetailPriceItem, termYears int) (onDeman return onDemand, reservation, currency } -// estimateCosmosReservationPrice estimates reservation price when not available -func estimateCosmosReservationPrice(onDemandPrice, hoursInTerm float64) float64 { - onDemandTotal := onDemandPrice * hoursInTerm - // Azure Cosmos DB reservations typically offer 65% savings - return onDemandTotal * 0.35 -} - // calculateCosmosSavingsPercentage calculates the savings percentage func calculateCosmosSavingsPercentage(onDemandPrice, hoursInTerm, reservationPrice float64) float64 { onDemandTotal := onDemandPrice * hoursInTerm diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index b4e596665..1b77ddfba 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -133,7 +133,19 @@ func createSampleCosmosPricingResponse() string { "serviceName": "Azure Cosmos DB", "skuName": "100RU", "meterName": "100 RU/s", - "reservationTerm": "1 Years", + "reservationTerm": "1 Year", + "type": "Reservation" + }, + { + "currencyCode": "USD", + "retailPrice": 2400.0, + "unitPrice": 2400.0, + "armRegionName": "eastus", + "productName": "Azure Cosmos DB", + "serviceName": "Azure Cosmos DB", + "skuName": "100RU", + "meterName": "100 RU/s", + "reservationTerm": "3 Years", "type": "Reservation" }, { @@ -325,6 +337,43 @@ func TestCosmosDBClient_GetOfferingDetails_NoPricing(t *testing.T) { assert.Contains(t, err.Error(), "no pricing data found") } +// TestCosmosDBClient_GetOfferingDetails_NoReservationPricing verifies that when +// on-demand pricing is present but no reservation line is returned, the client +// returns an error rather than fabricating a price from a hardcoded multiplier +// (issue #1020 H4). Pre-fix this would have silently surfaced a fabricated +// TotalCost/SavingsPercentage as a real quote. +func TestCosmosDBClient_GetOfferingDetails_NoReservationPricing(t *testing.T) { + ctx := context.Background() + + onDemandOnly := `{ + "Items": [ + { + "currencyCode": "USD", + "retailPrice": 0.008, + "unitPrice": 0.008, + "armRegionName": "eastus", + "productName": "Azure Cosmos DB", + "serviceName": "Azure Cosmos DB", + "type": "Consumption" + } + ], + "NextPageLink": "" + }` + + mockHTTP := &MockHTTPClient{} + client := NewClientWithHTTP(nil, "test-subscription", "eastus", mockHTTP) + mockHTTP.On("Do", mock.Anything).Return(createMockHTTPResponse(http.StatusOK, onDemandOnly), nil) + + rec := common.Recommendation{ + ResourceType: "100RU", + Term: "1yr", + PaymentOption: "upfront", + } + _, err := client.GetOfferingDetails(ctx, rec) + require.Error(t, err) + assert.Contains(t, err.Error(), "no reservation pricing found") +} + func TestCosmosDBClient_GetExistingCommitments_Empty(t *testing.T) { ctx := context.Background() client := NewClient(nil, "test-subscription", "eastus") @@ -851,6 +900,7 @@ func TestCosmosDBClient_PurchaseCommitment_TokenError(t *testing.T) { rec := common.Recommendation{ ResourceType: "EnableCassandra", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -872,6 +922,7 @@ func TestCosmosDBClient_PurchaseCommitment_HTTPError(t *testing.T) { rec := common.Recommendation{ ResourceType: "EnableCassandra", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -896,6 +947,7 @@ func TestCosmosDBClient_PurchaseCommitment_BadStatus(t *testing.T) { rec := common.Recommendation{ ResourceType: "EnableCassandra", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index c775c10b2..acd598904 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -301,10 +301,15 @@ func (c *DatabaseClient) PurchaseCommitment(ctx context.Context, rec common.Reco result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } + if rec.Count <= 0 { + result.Error = fmt.Errorf("quantity must be greater than zero, got %d", rec.Count) + return result, result.Error + } - termYears := 1 - if rec.Term == "3yr" || rec.Term == "3" { - termYears = 3 + termYears, termErr := reservations.ParseTermYears(rec.Term) + if termErr != nil { + result.Error = termErr + return result, result.Error } requestBody := map[string]interface{}{ @@ -377,9 +382,9 @@ func (c *DatabaseClient) ValidateOffering(ctx context.Context, rec common.Recomm // GetOfferingDetails retrieves SQL Database reservation offering details from Azure Retail Prices API func (c *DatabaseClient) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { - termYears := 1 - if rec.Term == "3yr" || rec.Term == "3" { - termYears = 3 + termYears, err := reservations.ParseTermYears(rec.Term) + if err != nil { + return nil, fmt.Errorf("invalid term: %w", err) } pricing, err := c.getSQLPricing(ctx, rec.ResourceType, c.region, termYears) @@ -398,7 +403,10 @@ func (c *DatabaseClient) GetOfferingDetails(ctx context.Context, rec common.Reco upfrontCost = 0 recurringCost = totalCost / (float64(termYears) * 12) default: - upfrontCost = totalCost + // Fail loud on an unrecognised payment option rather than silently + // billing it as all-upfront (owner policy: no silent fallbacks on + // money-affecting fields). + return nil, fmt.Errorf("unsupported payment option for Azure SQL Database offering details: %q", rec.PaymentOption) } return &common.OfferingDetails{ @@ -535,8 +543,13 @@ func (c *DatabaseClient) getSQLPricing(ctx context.Context, sku, region string, } hoursInTerm := 8760.0 * float64(termYears) + // Return an error rather than fabricating a reservation price from a + // hardcoded discount multiplier (issue #1020 H4). Presenting an + // estimated figure as a real TotalCost/SavingsPercentage is misleading + // and can justify uneconomical purchases. managedredis already uses + // this pattern as the model. if reservationPrice == 0 { - reservationPrice = onDemandPrice * hoursInTerm * 0.65 + return nil, fmt.Errorf("no reservation pricing found for SQL Database SKU %s (%d year) in region %s", sku, termYears, region) } savingsPercentage := ((onDemandPrice*hoursInTerm - reservationPrice) / (onDemandPrice * hoursInTerm)) * 100 @@ -568,10 +581,20 @@ func (c *DatabaseClient) fetchAzurePricing(ctx context.Context, filter string) ( return &AzureRetailPrice{Items: items}, nil } +// azureTermString returns the Retail Prices API ReservationTerm string for the +// given number of years. The API uses the singular form "1 Year" for one year +// and the plural form "N Years" for two or more years. +func azureTermString(termYears int) string { + if termYears == 1 { + return "1 Year" + } + return fmt.Sprintf("%d Years", termYears) +} + // extractSQLPricing extracts on-demand and reservation pricing from price items func extractSQLPricing(items []DatabaseRetailPriceItem, termYears int) (onDemand, reservation float64, currency string) { currency = "USD" - termStr := fmt.Sprintf("%d Years", termYears) + termStr := azureTermString(termYears) for _, item := range items { if item.CurrencyCode != "" { diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index 1e6d3ef21..95d2d8351 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -118,7 +118,22 @@ func createSampleSQLPricingResponse() string { "unitOfMeasure": "1 DTU/Hour", "type": "Reservation", "armSkuName": "Standard_S0", - "reservationTerm": "1 Years" + "reservationTerm": "1 Year" + }, + { + "currencyCode": "USD", + "retailPrice": 1800.0, + "unitPrice": 1800.0, + "armRegionName": "eastus", + "location": "US East", + "meterName": "S0 DTUs", + "skuName": "Standard", + "productName": "SQL Database", + "serviceName": "SQL Database", + "unitOfMeasure": "1 DTU/Hour", + "type": "Reservation", + "armSkuName": "Standard_S0", + "reservationTerm": "3 Years" }, { "currencyCode": "USD", @@ -353,6 +368,42 @@ func TestDatabaseClient_GetOfferingDetails_NoPricing(t *testing.T) { assert.Contains(t, err.Error(), "no pricing data found") } +// TestDatabaseClient_GetOfferingDetails_NoReservationPricing verifies that when +// on-demand pricing is present but no reservation line is returned, the client +// returns an error rather than fabricating a price from the hardcoded 0.65 +// multiplier (issue #1020 H4). Pre-fix this would have silently surfaced a +// fabricated TotalCost/SavingsPercentage as a real quote. +func TestDatabaseClient_GetOfferingDetails_NoReservationPricing(t *testing.T) { + ctx := context.Background() + + onDemandOnly := `{ + "Items": [ + { + "currencyCode": "USD", + "retailPrice": 0.096, + "unitPrice": 0.096, + "armRegionName": "eastus", + "armSkuName": "Standard_S0", + "type": "Consumption" + } + ], + "NextPageLink": "" + }` + + mockHTTP := &MockHTTPClient{} + client := NewClientWithHTTP(nil, "test-subscription", "eastus", mockHTTP) + mockHTTP.On("Do", mock.Anything).Return(createMockHTTPResponse(http.StatusOK, onDemandOnly), nil) + + rec := common.Recommendation{ + ResourceType: "Standard_S0", + Term: "1yr", + PaymentOption: "upfront", + } + _, err := client.GetOfferingDetails(ctx, rec) + require.Error(t, err) + assert.Contains(t, err.Error(), "no reservation pricing found") +} + func TestDatabaseClient_GetExistingCommitments_Empty(t *testing.T) { ctx := context.Background() client := NewClient(nil, "test-subscription", "eastus") @@ -921,6 +972,7 @@ func TestDatabaseClient_PurchaseCommitment_TokenError(t *testing.T) { rec := common.Recommendation{ ResourceType: "GP_Gen5_8", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -942,6 +994,7 @@ func TestDatabaseClient_PurchaseCommitment_HTTPError(t *testing.T) { rec := common.Recommendation{ ResourceType: "GP_Gen5_8", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -966,6 +1019,7 @@ func TestDatabaseClient_PurchaseCommitment_BadStatus(t *testing.T) { rec := common.Recommendation{ ResourceType: "GP_Gen5_8", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) diff --git a/providers/azure/services/internal/reservations/purchase.go b/providers/azure/services/internal/reservations/purchase.go index d736dafdf..9602ecaa6 100644 --- a/providers/azure/services/internal/reservations/purchase.go +++ b/providers/azure/services/internal/reservations/purchase.go @@ -52,6 +52,24 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" ) +// ParseTermYears maps a term string to an integer year count. +// Returns an error for any value outside the explicit allowlist so that +// callers fail closed rather than silently coercing to a 1-year purchase. +// Recognised forms: "", "1", "1yr", "1y" -> 1; "3", "3yr", "3y" -> 3. +// This is the canonical parser; service clients that previously used a local +// literal comparison (rec.Term == "3yr" || rec.Term == "3") should call this +// instead to get consistent error reporting on unrecognised terms. +func ParseTermYears(term string) (int, error) { + switch strings.ToLower(strings.TrimSpace(term)) { + case "", "1", "1yr", "1y": + return 1, nil + case "3", "3yr", "3y": + return 3, nil + default: + return 0, fmt.Errorf("unsupported reservation term: %s", term) + } +} + // apiVersion is the GA api-version for the Microsoft.Capacity Reservations API. // Pinned to 2022-11-01 — the last stable version before Azure introduced the // calculatePrice requirement for new SKU families. diff --git a/providers/azure/services/internal/reservations/purchase_test.go b/providers/azure/services/internal/reservations/purchase_test.go index 3e81bfaca..e1a112148 100644 --- a/providers/azure/services/internal/reservations/purchase_test.go +++ b/providers/azure/services/internal/reservations/purchase_test.go @@ -675,3 +675,38 @@ func TestDoIdempotentPurchaseTwoStep_PreservesTwoStepFlow(t *testing.T) { assert.Equal(t, "second", orderID) m.AssertExpectations(t) } + +// TestParseTermYears verifies the canonical term parser (M4 regression: +// before this fix, five service clients used a literal "3yr"||"3" check that +// silently treated unrecognised terms as 1yr instead of returning an error). +func TestParseTermYears(t *testing.T) { + tests := []struct { + term string + want int + wantErr bool + }{ + {"1yr", 1, false}, + {"1", 1, false}, + {"1y", 1, false}, + {"", 1, false}, + {"3yr", 3, false}, + {"3", 3, false}, + {"3y", 3, false}, + {"1YR", 1, false}, // case-insensitive + {"3YR", 3, false}, // case-insensitive + {" 1yr ", 1, false}, // whitespace-tolerant + {"5yr", 0, true}, // unrecognised term must error (pre-fix: silently returned 1) + {"P1Y", 0, true}, // ISO 8601 form not supported by this parser + {"bogus", 0, true}, + } + for _, tc := range tests { + got, err := ParseTermYears(tc.term) + if tc.wantErr { + assert.Error(t, err, "term=%q should be an error", tc.term) + assert.Equal(t, 0, got, "term=%q error return should be 0", tc.term) + } else { + require.NoError(t, err, "term=%q should not error", tc.term) + assert.Equal(t, tc.want, got, "term=%q", tc.term) + } + } +} diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index c0bf315f6..e38722c8c 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -7,7 +7,6 @@ import ( "context" "encoding/json" "fmt" - "io" "net/http" "net/url" "strings" @@ -19,6 +18,8 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/redis/armredis/v3" "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient" + "github.com/LeanerCloud/CUDly/providers/azure/internal/pricing" "github.com/LeanerCloud/CUDly/providers/azure/internal/recommendations" "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) @@ -65,16 +66,16 @@ func NewClient(cred azcore.TokenCredential, subscriptionID, region string) *Mana cred: cred, subscriptionID: subscriptionID, region: region, - httpClient: &http.Client{Timeout: 30 * time.Second}, + httpClient: httpclient.New(), } } // NewClientWithHTTP creates a new ManagedRedisClient with a custom HTTP client (for testing). -// When httpClient is nil, a default *http.Client with a 30s timeout is used to avoid -// nil-deref panics on the first Do call. +// When httpClient is nil, the SSRF-hardened httpclient.New() is used so the nil +// fallback also blocks IMDS connections. func NewClientWithHTTP(cred azcore.TokenCredential, subscriptionID, region string, httpClient HTTPClient) *ManagedRedisClient { if httpClient == nil { - httpClient = &http.Client{Timeout: 30 * time.Second} + httpClient = httpclient.New() } return &ManagedRedisClient{ cred: cred, @@ -109,22 +110,20 @@ func (c *ManagedRedisClient) GetRegion() string { return c.region } -// AzureRetailPrice represents pricing information from the Azure Retail Prices API. -type AzureRetailPrice struct { - Items []struct { - CurrencyCode string `json:"currencyCode"` - RetailPrice float64 `json:"retailPrice"` - UnitPrice float64 `json:"unitPrice"` - ArmRegionName string `json:"armRegionName"` - ProductName string `json:"productName"` - ServiceName string `json:"serviceName"` - ArmSKUName string `json:"armSkuName"` - MeterName string `json:"meterName"` - ReservationTerm string `json:"reservationTerm"` - Type string `json:"type"` - } `json:"Items"` - NextPageLink string `json:"NextPageLink"` - Count int `json:"Count"` +// redisPriceItem is a single record from the Azure Retail Prices API used by +// pricing.FetchAll as the type parameter T. Named so it can satisfy the +// constraint; the anonymous struct in AzureRetailPrice was the blocker. +type redisPriceItem struct { + CurrencyCode string `json:"currencyCode"` + RetailPrice float64 `json:"retailPrice"` + UnitPrice float64 `json:"unitPrice"` + ArmRegionName string `json:"armRegionName"` + ProductName string `json:"productName"` + ServiceName string `json:"serviceName"` + ArmSKUName string `json:"armSkuName"` + MeterName string `json:"meterName"` + ReservationTerm string `json:"reservationTerm"` + Type string `json:"type"` } // GetRecommendations gets Redis Cache reservation recommendations from the Azure Consumption API. @@ -225,20 +224,6 @@ func (c *ManagedRedisClient) reservationDetailToCommitment(detail *armconsumptio return cm } -// parseTermYears maps a reservation term string to an integer year count. -// Returns an error for any value outside the explicit allowlist so callers -// fail closed rather than silently coercing to a 1-year purchase. -func parseTermYears(term string) (int, error) { - switch strings.ToLower(strings.TrimSpace(term)) { - case "", "1", "1yr", "1y": - return 1, nil - case "3", "3yr", "3y": - return 3, nil - default: - return 0, fmt.Errorf("unsupported reservation term: %s", term) - } -} - // PurchaseCommitment purchases Azure Cache for Redis reserved capacity using the two-step // calculatePrice->purchase flow required by Azure's Reservations API (issue #677). func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { @@ -258,8 +243,12 @@ func (c *ManagedRedisClient) PurchaseCommitment(ctx context.Context, rec common. result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } + if rec.Count <= 0 { + result.Error = fmt.Errorf("quantity must be greater than zero, got %d", rec.Count) + return result, result.Error + } - termYears, termErr := parseTermYears(rec.Term) + termYears, termErr := reservations.ParseTermYears(rec.Term) if termErr != nil { result.Error = termErr return result, result.Error @@ -326,7 +315,7 @@ func (c *ManagedRedisClient) ValidateOffering(ctx context.Context, rec common.Re // GetOfferingDetails retrieves reservation offering details from the Azure Retail Prices API. func (c *ManagedRedisClient) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { - termYears, err := parseTermYears(rec.Term) + termYears, err := reservations.ParseTermYears(rec.Term) if err != nil { return nil, err } @@ -454,11 +443,11 @@ type RedisPricing struct { SavingsPercentage float64 } -// getRedisPricing fetches pricing from the Azure Retail Prices API, following -// pagination via NextPageLink until all pages are consumed. +// getRedisPricing fetches pricing from the Azure Retail Prices API using the +// shared pricing.FetchAll walker, which enforces a seen-URL guard, a max-pages +// cap, and a per-page timeout independent of the caller's context budget. This +// replaces the former hand-rolled NextPageLink loop (issue #1021 H2). func (c *ManagedRedisClient) getRedisPricing(ctx context.Context, sku, region string, termYears int) (*RedisPricing, error) { - baseURL := "https://prices.azure.com/api/retail/prices" - filter := fmt.Sprintf("serviceName eq 'Azure Cache for Redis' and armRegionName eq '%s' and contains(armSkuName, '%s')", region, sku) @@ -466,43 +455,18 @@ func (c *ManagedRedisClient) getRedisPricing(ctx context.Context, sku, region st params.Add("$filter", filter) params.Add("api-version", "2023-01-01-preview") - nextURL := baseURL + "?" + params.Encode() - - var accumulated AzureRetailPrice - - for nextURL != "" { - req, err := http.NewRequestWithContext(ctx, "GET", nextURL, nil) - if err != nil { - return nil, fmt.Errorf("failed to create request: %w", err) - } - - resp, err := c.httpClient.Do(req) - if err != nil { - return nil, fmt.Errorf("failed to call pricing API: %w", err) - } - - if resp.StatusCode != http.StatusOK { - body, _ := io.ReadAll(resp.Body) - resp.Body.Close() - return nil, fmt.Errorf("pricing API returned status %d: %s", resp.StatusCode, string(body)) - } + initialURL := "https://prices.azure.com/api/retail/prices?" + params.Encode() - var page AzureRetailPrice - if err := json.NewDecoder(resp.Body).Decode(&page); err != nil { - resp.Body.Close() - return nil, fmt.Errorf("failed to decode pricing response: %w", err) - } - resp.Body.Close() - - accumulated.Items = append(accumulated.Items, page.Items...) - nextURL = page.NextPageLink + items, err := pricing.FetchAll[redisPriceItem](ctx, c.httpClient, initialURL, pricing.DefaultPageTimeout, pricing.DefaultMaxPages) + if err != nil { + return nil, fmt.Errorf("failed to fetch Redis Cache pricing: %w", err) } - if len(accumulated.Items) == 0 { + if len(items) == 0 { return nil, fmt.Errorf("no pricing data found for Redis Cache SKU %s in region %s", sku, region) } - onDemandPrice, reservationPrice, currency := parsePriceItems(accumulated.Items, termYears) + onDemandPrice, reservationPrice, currency := parsePriceItems(items, termYears) if onDemandPrice == 0 { return nil, fmt.Errorf("no on-demand pricing found for Redis Cache SKU %s", sku) @@ -524,22 +488,21 @@ func (c *ManagedRedisClient) getRedisPricing(ctx context.Context, sku, region st }, nil } +// azureTermString returns the Retail Prices API ReservationTerm string for the +// given number of years. The API uses the singular form "1 Year" for one year +// and the plural form "N Years" for two or more years. +func azureTermString(termYears int) string { + if termYears == 1 { + return "1 Year" + } + return fmt.Sprintf("%d Years", termYears) +} + // parsePriceItems extracts on-demand price, reservation price, and currency // from the flat list returned by the Azure Retail Prices API. -func parsePriceItems(items []struct { - CurrencyCode string `json:"currencyCode"` - RetailPrice float64 `json:"retailPrice"` - UnitPrice float64 `json:"unitPrice"` - ArmRegionName string `json:"armRegionName"` - ProductName string `json:"productName"` - ServiceName string `json:"serviceName"` - ArmSKUName string `json:"armSkuName"` - MeterName string `json:"meterName"` - ReservationTerm string `json:"reservationTerm"` - Type string `json:"type"` -}, termYears int) (onDemand, reservation float64, currency string) { +func parsePriceItems(items []redisPriceItem, termYears int) (onDemand, reservation float64, currency string) { currency = "USD" - termStr := fmt.Sprintf("%d Years", termYears) + termStr := azureTermString(termYears) for _, item := range items { if item.CurrencyCode != "" { currency = item.CurrencyCode diff --git a/providers/azure/services/managedredis/client_test.go b/providers/azure/services/managedredis/client_test.go index 8c187235b..0e5672947 100644 --- a/providers/azure/services/managedredis/client_test.go +++ b/providers/azure/services/managedredis/client_test.go @@ -107,7 +107,7 @@ func samplePricingJSON() string { "serviceName": "Azure Cache for Redis", "armSkuName": "Premium_P1", "meterName": "P1 Instance", - "reservationTerm": "1 Years", + "reservationTerm": "1 Year", "type": "Reservation" }, { @@ -158,6 +158,66 @@ func TestNewClient(t *testing.T) { assert.NotNil(t, c.httpClient) } +// TestNewClient_UsesHardenedHTTPClient verifies that NewClient installs the +// SSRF-hardened httpclient (blocks IMDS at 169.254.169.254) rather than a +// bare &http.Client{}. Pre-fix this would have passed with a bare client that +// allows IMDS connections (issue #1021 H1). +func TestNewClient_UsesHardenedHTTPClient(t *testing.T) { + c := NewClient(nil, "sub", "eastus") + require.NotNil(t, c.httpClient) + // The hardened client blocks IMDS; the bare client does not. + // Attempt a dial to the IMDS address and confirm it is rejected. + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://169.254.169.254/metadata/instance", nil) + require.NoError(t, err) + _, err = c.httpClient.Do(req) + require.Error(t, err, "hardened client must reject IMDS connections") + assert.Contains(t, err.Error(), "blocked") +} + +// TestNewClientWithHTTP_NilFallbackIsHardened verifies that passing nil as the +// httpClient falls back to httpclient.New() (SSRF-hardened), not bare &http.Client{}. +func TestNewClientWithHTTP_NilFallbackIsHardened(t *testing.T) { + c := NewClientWithHTTP(nil, "sub", "eastus", nil) + require.NotNil(t, c.httpClient) + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://169.254.169.254/metadata/instance", nil) + require.NoError(t, err) + _, err = c.httpClient.Do(req) + require.Error(t, err, "nil-fallback client must also reject IMDS connections") + assert.Contains(t, err.Error(), "blocked") +} + +// TestGetOfferingDetails_NoReservationPricing verifies that when the Retail +// Prices API returns on-demand pricing but no reservation line, GetOfferingDetails +// returns an error rather than fabricating a price from a hardcoded multiplier +// (issue #1020 H4). Pre-fix this would have returned a fabricated price silently. +func TestGetOfferingDetails_NoReservationPricing(t *testing.T) { + h := &mockHTTPClient{} + t.Cleanup(func() { h.AssertExpectations(t) }) + onDemandOnly := `{ + "Items": [ + { + "currencyCode": "USD", + "retailPrice": 0.125, + "unitPrice": 0.125, + "armRegionName": "eastus", + "productName": "Azure Cache for Redis", + "serviceName": "Azure Cache for Redis", + "armSkuName": "Premium_P1", + "type": "Consumption" + } + ], + "NextPageLink": "", + "Count": 1 + }` + h.On("Do", mock.Anything).Return(fakeHTTPResp(http.StatusOK, onDemandOnly), nil) + c := NewClientWithHTTP(nil, "sub", "eastus", h) + _, err := c.GetOfferingDetails(context.Background(), common.Recommendation{ + ResourceType: "Premium_P1", Term: "1yr", + }) + require.Error(t, err) + assert.Contains(t, err.Error(), "no reservation pricing found") +} + func TestNewClientWithHTTP(t *testing.T) { h := &mockHTTPClient{} c := NewClientWithHTTP(nil, "sub-123", "eastus", h) @@ -481,7 +541,7 @@ func TestGetOfferingDetails_Paginated(t *testing.T) { "serviceName": "Azure Cache for Redis", "armSkuName": "Premium_P1", "meterName": "P1 Instance", - "reservationTerm": "1 Years", + "reservationTerm": "1 Year", "type": "Reservation" } ], @@ -560,7 +620,7 @@ func TestPurchaseCommitment_Accepted(t *testing.T) { cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub", "eastus", h) result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ - ResourceType: "Premium_P1", Term: "1yr", + ResourceType: "Premium_P1", Term: "1yr", Count: 1, }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.NoError(t, err) assert.True(t, result.Success) @@ -572,7 +632,7 @@ func TestPurchaseCommitment_TokenError(t *testing.T) { cred := &mockTokenCredential{err: errors.New("token error")} c := NewClientWithHTTP(cred, "sub", "eastus", h) result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ - ResourceType: "Premium_P1", Term: "1yr", + ResourceType: "Premium_P1", Term: "1yr", Count: 1, }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) @@ -588,7 +648,7 @@ func TestPurchaseCommitment_HTTPError(t *testing.T) { cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub", "eastus", h) result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ - ResourceType: "Premium_P1", Term: "1yr", + ResourceType: "Premium_P1", Term: "1yr", Count: 1, }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) @@ -607,7 +667,7 @@ func TestPurchaseCommitment_BadStatus(t *testing.T) { cred := &mockTokenCredential{token: "tok"} c := NewClientWithHTTP(cred, "sub", "eastus", h) result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ - ResourceType: "Premium_P1", Term: "1yr", + ResourceType: "Premium_P1", Term: "1yr", Count: 1, }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) require.Error(t, err) assert.False(t, result.Success) @@ -722,13 +782,6 @@ func TestConvertRecommendation_legacy(t *testing.T) { assert.Equal(t, 0.0, *rec.RecurringMonthlyCost) } -// -- AzureRetailPrice struct -- - -func TestAzureRetailPriceStruct(t *testing.T) { - p := AzureRetailPrice{Count: 2} - assert.Equal(t, 2, p.Count) -} - // -- RedisPricing struct -- func TestRedisPricingStruct(t *testing.T) { @@ -780,3 +833,22 @@ func TestPurchaseCommitment_TagInjection(t *testing.T) { require.True(t, hasTags, "tags field must be present in purchase body when Source is set") assert.Equal(t, source, tags[common.PurchaseTagKey], "tag value must match opts.Source") } + +// TestPurchaseCommitment_ZeroCountRejected is a regression test for M6: +// PurchaseCommitment must reject Count==0 before issuing any HTTP call. +// An Advisor recommendation with a missing qty field defaults to 0 -- without +// this guard a zero-quantity purchase would reach the Azure API and produce a +// confusing 400. +func TestPurchaseCommitment_ZeroCountRejected(t *testing.T) { + h := &mockHTTPClient{} + t.Cleanup(func() { h.AssertExpectations(t) }) + cred := &mockTokenCredential{token: "tok"} + c := NewClientWithHTTP(cred, "sub", "eastus", h) + result, err := c.PurchaseCommitment(context.Background(), common.Recommendation{ + ResourceType: "Premium_P1", Term: "1yr", Count: 0, + }, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.Error(t, err) + assert.False(t, result.Success) + assert.Contains(t, err.Error(), "quantity must be greater than zero") + h.AssertNotCalled(t, "Do", mock.Anything) +} diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index 6c9bf481a..8d98cf6b3 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -3,19 +3,18 @@ package savingsplans import ( "context" - "encoding/json" "fmt" - "io" "net/http" "net/url" "time" "github.com/Azure/azure-sdk-for-go/sdk/azcore" - "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/azcore/runtime" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits" "github.com/LeanerCloud/CUDly/pkg/common" + "github.com/LeanerCloud/CUDly/providers/azure/internal/httpclient" + "github.com/LeanerCloud/CUDly/providers/azure/internal/pricing" ) // SavingsPlanOrderAliasAPI defines operations on SavingsPlanOrderAlias (enables mocking). @@ -77,14 +76,16 @@ func NewClient(cred azcore.TokenCredential, subscriptionID, region string) *Clie cred: cred, subscriptionID: subscriptionID, region: region, - httpClient: &http.Client{Timeout: 30 * time.Second}, + httpClient: httpclient.New(), } } // NewClientWithHTTP creates a new Azure Savings Plans client with a custom HTTP client (for testing). +// When httpClient is nil, the SSRF-hardened httpclient.New() is used so the nil +// fallback also blocks IMDS connections. func NewClientWithHTTP(cred azcore.TokenCredential, subscriptionID, region string, httpClient HTTPClient) *Client { if httpClient == nil { - httpClient = &http.Client{Timeout: 30 * time.Second} + httpClient = httpclient.New() } return &Client{ cred: cred, @@ -206,7 +207,17 @@ func convertSavingsPlan(sp *armbillingbenefits.SavingsPlanModel, subscriptionID } // PurchaseCommitment creates an Azure Savings Plan via the OrderAlias API. -func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, _ common.PurchaseOptions) (common.PurchaseResult, error) { +// +// Idempotency: the SavingsPlanOrderAlias is created with an HTTP PUT on the +// alias name (Microsoft.BillingBenefits/savingsPlanOrderAliases/{name}), so a +// PUT with a previously-used name returns the existing order alias rather than +// creating a second savings plan. When opts.IdempotencyToken is set (the +// purchase-execution path), the alias name is derived deterministically from it +// via common.ReservationOrderID so a re-drive of a stranded execution (issue +// #636) re-PUTs the same alias instead of double-buying. When the token is empty +// (the CLI path, which has no owning execution), the prior timestamp-based name +// is retained. +func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendation, opts common.PurchaseOptions) (common.PurchaseResult, error) { result := common.PurchaseResult{ Recommendation: rec, DryRun: false, @@ -259,7 +270,10 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati aliasClient = &realOrderAliasClient{client: real} } - aliasName := fmt.Sprintf("cudly-%d", time.Now().UnixNano()) + // Derive a deterministic alias name from the idempotency token when present + // so a re-drive PUTs the same alias (idempotent), falling back to the prior + // timestamp-based name on the CLI path (empty token). + aliasName := common.ReservationOrderID(opts.IdempotencyToken, fmt.Sprintf("cudly-%d", time.Now().UnixNano())) poller, err := aliasClient.BeginCreate(ctx, aliasName, body, nil) if err != nil { result.Error = fmt.Errorf("failed to begin savings plan purchase: %w", err) @@ -368,22 +382,20 @@ func checkValidateResponse(resp armbillingbenefits.RPClientValidatePurchaseRespo return nil } -// AzureRetailPrice represents a pricing record from the Azure Retail Prices API. -// -// NOTE: this struct is also defined in services/compute and services/search. -// Consolidating it into a shared internal package is tracked as a follow-up. -type AzureRetailPrice struct { - Items []struct { - CurrencyCode string `json:"currencyCode"` - RetailPrice float64 `json:"retailPrice"` - UnitPrice float64 `json:"unitPrice"` - ArmRegionName string `json:"armRegionName"` - ProductName string `json:"productName"` - ServiceName string `json:"serviceName"` - ArmSKUName string `json:"armSkuName"` - ReservationTerm string `json:"reservationTerm"` - Type string `json:"type"` - } `json:"Items"` +// savingsPlanPriceItem is a single record from the Azure Retail Prices API used +// by pricing.FetchAll as the type parameter T (requires a named type, not an +// anonymous struct). NOTE: this struct mirrors the one in services/compute and +// services/search; consolidating into internal/pricing is tracked as a follow-up. +type savingsPlanPriceItem struct { + CurrencyCode string `json:"currencyCode"` + RetailPrice float64 `json:"retailPrice"` + UnitPrice float64 `json:"unitPrice"` + ArmRegionName string `json:"armRegionName"` + ProductName string `json:"productName"` + ServiceName string `json:"serviceName"` + ArmSKUName string `json:"armSkuName"` + ReservationTerm string `json:"reservationTerm"` + Type string `json:"type"` } // GetOfferingDetails retrieves pricing details for an Azure Savings Plan offering. @@ -442,54 +454,33 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati } // fetchOnDemandRate queries the Azure Retail Prices API for the on-demand hourly -// rate for the given plan type. Returns 0 and an error if unavailable. +// rate for the given plan type. The $filter is built with url.Values so that +// spaces and quotes are properly percent-encoded (issue #1021 H3). Pagination is +// handled by pricing.FetchAll (seen-URL guard, max-pages cap, per-page timeout). +// Returns an error when no price record is found rather than (0, nil), which would +// conflate "zero rate" with "not found" (feedback_nullable_not_zero). func (c *Client) fetchOnDemandRate(ctx context.Context, planType string) (float64, error) { filter := fmt.Sprintf( "serviceFamily eq 'Compute' and priceType eq 'Consumption' and armSkuName eq '%s'", - url.QueryEscape(planType), + planType, ) - apiURL := fmt.Sprintf( - "https://prices.azure.com/api/retail/prices?$filter=%s", - filter, - ) - - req, err := http.NewRequestWithContext(ctx, http.MethodGet, apiURL, nil) - if err != nil { - return 0, err - } - - // Attach Bearer token if credential is available. - if c.cred != nil { - tokenOpts := policy.TokenRequestOptions{Scopes: []string{"https://management.azure.com/.default"}} - token, err := c.cred.GetToken(ctx, tokenOpts) - if err == nil { - req.Header.Set("Authorization", "Bearer "+token.Token) - } - } + params := url.Values{} + params.Add("$filter", filter) + params.Add("api-version", "2023-01-01-preview") + initialURL := "https://prices.azure.com/api/retail/prices?" + params.Encode() - resp, err := c.httpClient.Do(req) + items, err := pricing.FetchAll[savingsPlanPriceItem](ctx, c.httpClient, initialURL, pricing.DefaultPageTimeout, pricing.DefaultMaxPages) if err != nil { - return 0, err - } - defer resp.Body.Close() - - body, err := io.ReadAll(resp.Body) - if err != nil { - return 0, err - } - - var pricing AzureRetailPrice - if err := json.Unmarshal(body, &pricing); err != nil { - return 0, err + return 0, fmt.Errorf("failed to fetch on-demand rate for plan type %s: %w", planType, err) } - for _, item := range pricing.Items { + for _, item := range items { if item.RetailPrice > 0 { return item.RetailPrice, nil } } - return 0, nil + return 0, fmt.Errorf("no on-demand pricing found for savings plan type %s", planType) } // GetValidResourceTypes returns the Azure Savings Plan types. diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index bff5aa1c2..aa0b624e0 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -1,13 +1,18 @@ package savingsplans import ( + "bytes" "context" "errors" + "io" + "net/http" + "strings" "testing" "time" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" "github.com/LeanerCloud/CUDly/pkg/common" @@ -40,14 +45,18 @@ func (p *mockListAllPager) NextPage(_ context.Context) (armbillingbenefits.Savin type mockOrderAliasClient struct { createPoller SavingsPlanOrderAliasPoller createErr error + // capturedName records the alias name passed to BeginCreate so tests can + // assert the deterministic idempotency-derived name. + capturedName string } func (m *mockOrderAliasClient) BeginCreate( _ context.Context, - _ string, + savingsPlanOrderAliasName string, _ armbillingbenefits.SavingsPlanOrderAliasModel, _ *armbillingbenefits.SavingsPlanOrderAliasClientBeginCreateOptions, ) (SavingsPlanOrderAliasPoller, error) { + m.capturedName = savingsPlanOrderAliasName if m.createErr != nil { return nil, m.createErr } @@ -262,6 +271,58 @@ func TestPurchaseCommitment_BadTerm(t *testing.T) { assert.False(t, result.Success) } +// TestPurchaseCommitment_DeterministicAliasNameFromToken verifies that when an +// idempotency token is supplied, the SavingsPlanOrderAlias name is derived +// deterministically from it (the canonical GUID) rather than from a timestamp. +// Because the alias is created with an HTTP PUT, a re-drive with the same token +// re-PUTs the same alias instead of buying a second savings plan (issue #636). +// Pre-fix this test FAILS: PurchaseCommitment ignored PurchaseOptions and always +// used a time.Now().UnixNano() alias name, so two re-drives produced two distinct +// aliases (two purchases). +func TestPurchaseCommitment_DeterministicAliasNameFromToken(t *testing.T) { + orderID := "/subscriptions/sub/providers/Microsoft.BillingBenefits/savingsPlanOrders/order1" + resp := armbillingbenefits.SavingsPlanOrderAliasClientCreateResponse{ + SavingsPlanOrderAliasModel: armbillingbenefits.SavingsPlanOrderAliasModel{ID: &orderID}, + } + + token := common.DeriveIdempotencyToken("exec-1", 0) + wantName := common.ReservationOrderID(token, "") + require.NotEmpty(t, wantName, "token must yield a deterministic GUID") + + // First drive. + mock1 := &mockOrderAliasClient{createPoller: &mockPoller{resp: resp}} + c1 := NewClient(nil, "sub", "eastus") + c1.SetOrderAliasClient(mock1) + _, err := c1.PurchaseCommitment(context.Background(), makeRec("1yr"), common.PurchaseOptions{IdempotencyToken: token}) + require.NoError(t, err) + assert.Equal(t, wantName, mock1.capturedName) + + // Re-drive of the same execution: same token must yield the same alias name. + mock2 := &mockOrderAliasClient{createPoller: &mockPoller{resp: resp}} + c2 := NewClient(nil, "sub", "eastus") + c2.SetOrderAliasClient(mock2) + _, err = c2.PurchaseCommitment(context.Background(), makeRec("1yr"), common.PurchaseOptions{IdempotencyToken: token}) + require.NoError(t, err) + assert.Equal(t, mock1.capturedName, mock2.capturedName, + "re-drive with the same idempotency token must reuse the alias name (PUT is idempotent)") +} + +// TestPurchaseCommitment_EmptyTokenUsesTimestampName verifies the CLI path (no +// idempotency token) retains a non-deterministic timestamp-based alias name. +func TestPurchaseCommitment_EmptyTokenUsesTimestampName(t *testing.T) { + orderID := "/subscriptions/sub/providers/Microsoft.BillingBenefits/savingsPlanOrders/order1" + resp := armbillingbenefits.SavingsPlanOrderAliasClientCreateResponse{ + SavingsPlanOrderAliasModel: armbillingbenefits.SavingsPlanOrderAliasModel{ID: &orderID}, + } + mock := &mockOrderAliasClient{createPoller: &mockPoller{resp: resp}} + c := NewClient(nil, "sub", "eastus") + c.SetOrderAliasClient(mock) + _, err := c.PurchaseCommitment(context.Background(), makeRec("1yr"), common.PurchaseOptions{}) + require.NoError(t, err) + assert.True(t, strings.HasPrefix(mock.capturedName, "cudly-"), + "empty token should keep the timestamp-based cudly- alias name") +} + // --- ValidateOffering --- func TestValidateOffering_Valid(t *testing.T) { @@ -411,6 +472,110 @@ func TestGetOfferingDetails_WrongDetails(t *testing.T) { assert.Contains(t, err.Error(), "invalid service details") } +// --- HTTP client regression tests (issue #1021) --- + +// mockHTTPClient is a testify/mock-based HTTPClient stub for savingsplans tests. +type mockHTTPClient struct{ mock.Mock } + +func (m *mockHTTPClient) Do(req *http.Request) (*http.Response, error) { + args := m.Called(req) + if args.Get(0) == nil { + return nil, args.Error(1) + } + return args.Get(0).(*http.Response), args.Error(1) +} + +func fakeHTTPResp(status int, body string) *http.Response { + return &http.Response{ + StatusCode: status, + Body: io.NopCloser(bytes.NewBufferString(body)), + Header: make(http.Header), + } +} + +// TestNewClient_UsesHardenedHTTPClient verifies that NewClient installs the +// SSRF-hardened httpclient (blocks IMDS at 169.254.169.254) rather than a +// bare &http.Client{}. Pre-fix this would have passed with a bare client that +// allows IMDS connections (issue #1021 H1). +func TestNewClient_UsesHardenedHTTPClient(t *testing.T) { + c := NewClient(nil, "sub", "eastus") + require.NotNil(t, c.httpClient) + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://169.254.169.254/metadata/instance", nil) + require.NoError(t, err) + _, err = c.httpClient.Do(req) + require.Error(t, err, "hardened client must reject IMDS connections") + assert.Contains(t, err.Error(), "blocked") +} + +// TestNewClientWithHTTP_NilFallbackIsHardened verifies that passing nil as the +// httpClient falls back to httpclient.New() (SSRF-hardened), not bare &http.Client{}. +func TestNewClientWithHTTP_NilFallbackIsHardened(t *testing.T) { + c := NewClientWithHTTP(nil, "sub", "eastus", nil) + require.NotNil(t, c.httpClient) + req, err := http.NewRequestWithContext(context.Background(), http.MethodGet, "http://169.254.169.254/metadata/instance", nil) + require.NoError(t, err) + _, err = c.httpClient.Do(req) + require.Error(t, err, "nil-fallback client must also reject IMDS connections") + assert.Contains(t, err.Error(), "blocked") +} + +// TestFetchOnDemandRate_NotFound verifies that fetchOnDemandRate returns an +// error (not (0, nil)) when no pricing item is found. Pre-fix the function +// returned (0, nil) which conflated "not found" with "rate is zero" and would +// silently corrupt downstream savings calculations (issue #1021 H3, +// feedback_nullable_not_zero, feedback_empty_string_vs_error). +func TestFetchOnDemandRate_NotFound(t *testing.T) { + h := &mockHTTPClient{} + t.Cleanup(func() { h.AssertExpectations(t) }) + h.On("Do", mock.Anything).Return(fakeHTTPResp(http.StatusOK, `{"Items":[],"NextPageLink":""}`), nil) + + c := NewClientWithHTTP(nil, "sub", "eastus", h) + _, err := c.fetchOnDemandRate(context.Background(), "Compute") + require.Error(t, err) + assert.Contains(t, err.Error(), "no on-demand pricing found") +} + +// TestFetchOnDemandRate_ReturnsFirstPositivePrice verifies the happy path: +// when at least one item with RetailPrice > 0 exists, it is returned without error. +func TestFetchOnDemandRate_ReturnsFirstPositivePrice(t *testing.T) { + h := &mockHTTPClient{} + t.Cleanup(func() { h.AssertExpectations(t) }) + body := `{ + "Items": [ + {"currencyCode":"USD","retailPrice":1.5,"unitPrice":1.5,"type":"Consumption","armSkuName":"Compute"} + ], + "NextPageLink": "" + }` + h.On("Do", mock.Anything).Return(fakeHTTPResp(http.StatusOK, body), nil) + + c := NewClientWithHTTP(nil, "sub", "eastus", h) + rate, err := c.fetchOnDemandRate(context.Background(), "Compute") + require.NoError(t, err) + assert.InDelta(t, 1.5, rate, 0.001) +} + +// TestFetchOnDemandRate_URLEncoding verifies that the $filter value is built +// with url.Values.Encode() so that spaces and quotes are percent-encoded rather +// than passed raw in the URL (issue #1021 H3). +func TestFetchOnDemandRate_URLEncoding(t *testing.T) { + h := &mockHTTPClient{} + t.Cleanup(func() { h.AssertExpectations(t) }) + // Capture the request URL and assert it is properly encoded. + var capturedURL string + h.On("Do", mock.MatchedBy(func(req *http.Request) bool { + capturedURL = req.URL.RawQuery + return true + })).Return(fakeHTTPResp(http.StatusOK, `{"Items":[],"NextPageLink":""}`), nil) + + c := NewClientWithHTTP(nil, "sub", "eastus", h) + _, _ = c.fetchOnDemandRate(context.Background(), "Compute") + + // The raw query must not contain unencoded spaces or single-quotes. + assert.NotContains(t, capturedURL, " ", "URL must not contain unencoded spaces") + assert.NotContains(t, capturedURL, "'", "URL must not contain unencoded single-quotes") + assert.Contains(t, capturedURL, "%24filter", "URL must contain percent-encoded $filter key") +} + // --- toAzureTerm --- func TestToAzureTerm(t *testing.T) { diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index dadd1c986..0ec7fb77b 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -277,10 +277,15 @@ func (c *SearchClient) PurchaseCommitment(ctx context.Context, rec common.Recomm result.Error = fmt.Errorf("purchase source is required for Azure reservation purchases") return result, result.Error } + if rec.Count <= 0 { + result.Error = fmt.Errorf("quantity must be greater than zero, got %d", rec.Count) + return result, result.Error + } - termYears := 1 - if rec.Term == "3yr" || rec.Term == "3" { - termYears = 3 + termYears, termErr := reservations.ParseTermYears(rec.Term) + if termErr != nil { + result.Error = termErr + return result, result.Error } requestBody := map[string]interface{}{ @@ -353,9 +358,9 @@ func (c *SearchClient) ValidateOffering(ctx context.Context, rec common.Recommen // GetOfferingDetails retrieves Search reservation offering details from Azure Retail Prices API func (c *SearchClient) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { - termYears := 1 - if rec.Term == "3yr" || rec.Term == "3" { - termYears = 3 + termYears, err := reservations.ParseTermYears(rec.Term) + if err != nil { + return nil, fmt.Errorf("invalid term: %w", err) } pricing, err := c.getSearchPricing(ctx, rec.ResourceType, c.region, termYears) @@ -374,7 +379,10 @@ func (c *SearchClient) GetOfferingDetails(ctx context.Context, rec common.Recomm upfrontCost = 0 recurringCost = totalCost / (float64(termYears) * 12) default: - upfrontCost = totalCost + // Fail loud on an unrecognised payment option rather than silently + // billing it as all-upfront (owner policy: no silent fallbacks on + // money-affecting fields). + return nil, fmt.Errorf("unsupported payment option for Azure AI Search offering details: %q", rec.PaymentOption) } return &common.OfferingDetails{ @@ -487,8 +495,13 @@ func (c *SearchClient) getSearchPricing(ctx context.Context, sku, region string, } hoursInTerm := 8760.0 * float64(termYears) + // Return an error rather than fabricating a reservation price from a + // hardcoded discount multiplier (issue #1020 H4). Presenting an + // estimated figure as a real TotalCost/SavingsPercentage is misleading + // and can justify uneconomical purchases. managedredis already uses + // this pattern as the model. if reservationPrice == 0 { - reservationPrice = estimateSearchReservationPrice(onDemandPrice, hoursInTerm) + return nil, fmt.Errorf("no reservation pricing found for Azure Search SKU %s (%d year) in region %s", sku, termYears, region) } savingsPercentage := calculateSearchSavingsPercentage(onDemandPrice, hoursInTerm, reservationPrice) @@ -535,6 +548,16 @@ func (c *SearchClient) fetchAzurePricing(ctx context.Context, filter string) (*A return &priceData, nil } +// azureTermString returns the Retail Prices API ReservationTerm string for the +// given number of years. The API uses the singular form "1 Year" for one year +// and the plural form "N Years" for two or more years. +func azureTermString(termYears int) string { + if termYears == 1 { + return "1 Year" + } + return fmt.Sprintf("%d Years", termYears) +} + // extractSearchPricing extracts on-demand and reservation pricing from price items func extractSearchPricing(items []struct { CurrencyCode string `json:"currencyCode"` @@ -549,7 +572,7 @@ func extractSearchPricing(items []struct { Type string `json:"type"` }, termYears int) (onDemand, reservation float64, currency string) { currency = "USD" - termStr := fmt.Sprintf("%d Years", termYears) + termStr := azureTermString(termYears) for _, item := range items { if item.CurrencyCode != "" { @@ -566,13 +589,6 @@ func extractSearchPricing(items []struct { return onDemand, reservation, currency } -// estimateSearchReservationPrice estimates reservation price when not available -func estimateSearchReservationPrice(onDemandPrice, hoursInTerm float64) float64 { - onDemandTotal := onDemandPrice * hoursInTerm - // Azure Search reservations typically offer 30-40% savings - return onDemandTotal * 0.65 -} - // calculateSearchSavingsPercentage calculates the savings percentage func calculateSearchSavingsPercentage(onDemandPrice, hoursInTerm, reservationPrice float64) float64 { onDemandTotal := onDemandPrice * hoursInTerm diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index fcfa8ba5b..766570204 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -120,7 +120,19 @@ func createSampleSearchPricingResponse() string { "serviceName": "Azure Cognitive Search", "armSkuName": "Standard_S1", "meterName": "S1 Search Unit", - "reservationTerm": "1 Years", + "reservationTerm": "1 Year", + "type": "Reservation" + }, + { + "currencyCode": "USD", + "retailPrice": 1200.0, + "unitPrice": 1200.0, + "armRegionName": "eastus", + "productName": "Azure Cognitive Search", + "serviceName": "Azure Cognitive Search", + "armSkuName": "Standard_S1", + "meterName": "S1 Search Unit", + "reservationTerm": "3 Years", "type": "Reservation" }, { @@ -312,6 +324,41 @@ func TestSearchClient_GetOfferingDetails_NoPricing(t *testing.T) { assert.Contains(t, err.Error(), "no pricing data found") } +// TestSearchClient_GetOfferingDetails_NoReservationPricing verifies that when +// on-demand pricing is present but no reservation line is returned, the client +// returns an error rather than fabricating a price from a hardcoded multiplier +// (issue #1020 H4). Pre-fix this would have silently surfaced a fabricated +// TotalCost/SavingsPercentage as a real quote. +func TestSearchClient_GetOfferingDetails_NoReservationPricing(t *testing.T) { + ctx := context.Background() + + onDemandOnly := `{ + "Items": [ + { + "currencyCode": "USD", + "retailPrice": 0.20, + "unitPrice": 0.20, + "armRegionName": "eastus", + "armSkuName": "Standard_S1", + "type": "Consumption" + } + ] + }` + + mockHTTP := &MockHTTPClient{} + client := NewClientWithHTTP(nil, "test-subscription", "eastus", mockHTTP) + mockHTTP.On("Do", mock.Anything).Return(createMockHTTPResponse(http.StatusOK, onDemandOnly), nil) + + rec := common.Recommendation{ + ResourceType: "Standard_S1", + Term: "1yr", + PaymentOption: "upfront", + } + _, err := client.GetOfferingDetails(ctx, rec) + require.Error(t, err) + assert.Contains(t, err.Error(), "no reservation pricing found") +} + func TestSearchClient_GetExistingCommitments_Empty(t *testing.T) { ctx := context.Background() client := NewClient(nil, "test-subscription", "eastus") @@ -735,6 +782,7 @@ func TestSearchClient_PurchaseCommitment_TokenError(t *testing.T) { rec := common.Recommendation{ ResourceType: "standard", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -756,6 +804,7 @@ func TestSearchClient_PurchaseCommitment_HTTPError(t *testing.T) { rec := common.Recommendation{ ResourceType: "standard", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) @@ -780,6 +829,7 @@ func TestSearchClient_PurchaseCommitment_BadStatus(t *testing.T) { rec := common.Recommendation{ ResourceType: "standard", Term: "1yr", + Count: 1, } result, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index 6611cccee..44110ac88 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -226,20 +226,6 @@ func (c *SynapseClient) convertSynapseReservation(detail *armconsumption.Reserva return commitment } -// parseReservationTermYears maps a term string to an integer year count. -// Returns an error for any value outside the explicit allowlist so that -// callers fail closed rather than silently coercing to a 1-year purchase. -func parseReservationTermYears(term string) (int, error) { - switch strings.ToLower(strings.TrimSpace(term)) { - case "", "1", "1yr", "1y": - return 1, nil - case "3", "3yr", "3y": - return 3, nil - default: - return 0, fmt.Errorf("unsupported reservation term: %s", term) - } -} - // PurchaseCommitment purchases Synapse reserved capacity via the Azure // Reservations API two-step flow (calculatePrice -> purchase). The reserved // resource type is "SqlDW" which covers Dedicated SQL Pool DWU reservations. @@ -270,7 +256,7 @@ func (c *SynapseClient) PurchaseCommitment(ctx context.Context, rec common.Recom return result, result.Error } - termYears, err := parseReservationTermYears(rec.Term) + termYears, err := reservations.ParseTermYears(rec.Term) if err != nil { result.Error = err return result, result.Error @@ -337,7 +323,7 @@ func (c *SynapseClient) ValidateOffering(ctx context.Context, rec common.Recomme // GetOfferingDetails retrieves Synapse reservation offering details from the // Azure Retail Prices API. func (c *SynapseClient) GetOfferingDetails(ctx context.Context, rec common.Recommendation) (*common.OfferingDetails, error) { - termYears, err := parseReservationTermYears(rec.Term) + termYears, err := reservations.ParseTermYears(rec.Term) if err != nil { return nil, err } @@ -358,7 +344,10 @@ func (c *SynapseClient) GetOfferingDetails(ctx context.Context, rec common.Recom upfrontCost = 0 recurringCost = totalCost / (float64(termYears) * 12) default: - upfrontCost = totalCost + // Fail loud on an unrecognised payment option rather than silently + // billing it as all-upfront (owner policy: no silent fallbacks on + // money-affecting fields). + return nil, fmt.Errorf("unsupported payment option for Azure Synapse offering details: %q", rec.PaymentOption) } return &common.OfferingDetails{ diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index 48b726006..8a7e41719 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -18,6 +18,7 @@ import ( "github.com/LeanerCloud/CUDly/pkg/common" "github.com/LeanerCloud/CUDly/providers/azure/mocks" + "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) // ---- credential mock ------------------------------------------------------- @@ -746,7 +747,7 @@ func TestParseReservationTermYears(t *testing.T) { {"bogus", 0, true}, } for _, tc := range tests { - got, err := parseReservationTermYears(tc.term) + got, err := reservations.ParseTermYears(tc.term) if tc.wantErr { assert.Error(t, err, "term=%q should be an error", tc.term) } else {