From 0bf078531bd99cac3a0de96da90124546ce196b8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:06:49 -0700 Subject: [PATCH 1/9] sec(azure): harden managedredis and savingsplans HTTP clients + fix pagination Both services/managedredis and services/savingsplans built bare &http.Client{Timeout: 30s} instead of httpclient.New(), bypassing the blockIMDSDialer that every other Azure service client uses to prevent SSRF attacks that could exfiltrate managed-identity credentials (issue #1021 H1). managedredis.getRedisPricing also hand-rolled a NextPageLink loop without the seen-URL guard, max-pages cap, or per-page timeout that pricing.FetchAll enforces -- a self-referential NextPageLink would loop forever (H2). savingsplans.fetchOnDemandRate interpolated the $filter value directly into the URL with only a partial QueryEscape, bypassing proper percent-encoding for spaces and quotes; it also read only the first page and returned (0, nil) on not-found, conflating "rate is zero" with "lookup found nothing" (H3). Changes: - Replace &http.Client{} with httpclient.New() in NewClient and NewClientWithHTTP for both managedredis and savingsplans (nil fallback also hardened). - Introduce redisPriceItem named type in managedredis to satisfy the pricing.FetchAll[T] type constraint; replace the hand-rolled loop with pricing.FetchAll (seen-URL guard + max-pages cap + per-page timeout). - Rewrite savingsplans.fetchOnDemandRate: build $filter with url.Values.Encode, route through pricing.FetchAll, return an error on not-found rather than (0, nil). - Add regression tests: IMDS-block assertion for both constructors; not-found error for savingsplans; URL-encoding guard; paginated happy-path for managedredis. Closes #1021 --- .../azure/services/managedredis/client.go | 99 +++++----------- .../services/managedredis/client_test.go | 67 +++++++++-- .../azure/services/savingsplans/client.go | 90 ++++++--------- .../services/savingsplans/client_test.go | 108 ++++++++++++++++++ 4 files changed, 233 insertions(+), 131 deletions(-) diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index c0bf315f6..9fa5bc1e7 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. @@ -454,11 +453,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 +465,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) - } + initialURL := "https://prices.azure.com/api/retail/prices?" + params.Encode() - 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)) - } - - 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) @@ -526,18 +500,7 @@ func (c *ManagedRedisClient) getRedisPricing(ctx context.Context, sku, region st // 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) for _, item := range items { diff --git a/providers/azure/services/managedredis/client_test.go b/providers/azure/services/managedredis/client_test.go index 8c187235b..e11ac3e38 100644 --- a/providers/azure/services/managedredis/client_test.go +++ b/providers/azure/services/managedredis/client_test.go @@ -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) @@ -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) { diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index 6c9bf481a..cad66d974 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, @@ -368,22 +369,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 +441,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), - ) - apiURL := fmt.Sprintf( - "https://prices.azure.com/api/retail/prices?$filter=%s", - filter, + planType, ) + 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() - req, err := http.NewRequestWithContext(ctx, http.MethodGet, apiURL, nil) + items, err := pricing.FetchAll[savingsPlanPriceItem](ctx, c.httpClient, initialURL, pricing.DefaultPageTimeout, pricing.DefaultMaxPages) 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) - } - } - - resp, err := c.httpClient.Do(req) - 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..6ff203d5e 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -1,13 +1,17 @@ package savingsplans import ( + "bytes" "context" "errors" + "io" + "net/http" "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" @@ -411,6 +415,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) { From 3d409506b803b820e7d45cb4141c12c826966e0f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 03:07:06 -0700 Subject: [PATCH 2/9] fix(azure): remove fabricated reservation price fallback in 5 pricing clients When the Retail Prices API returned no reservation line, compute, cache, cosmosdb, database, and search each invented a reservation price from a hardcoded discount multiplier (0.62, 0.45, 0.35/0.65, 0.65, 0.65) and surfaced the result as a real OfferingDetails.TotalCost/SavingsPercentage. This can justify a reservation purchase that is not actually economical (issue #1020 H4, feedback_nullable_not_zero). managedredis already returns an error in this case and is the model here. Changes: - Replace the fabrication branches with fmt.Errorf("no reservation pricing found for ...") in compute.getVMPricing, cache.getRedisPricing, cosmosdb.getCosmosPricing, database.getSQLPricing, search.getSearchPricing. - Remove the now-dead estimateCosmosReservationPrice and estimateSearchReservationPrice helpers. - Update mock pricing responses to include "3 Years" reservation entries so the existing 3YearTerm tests exercise real pricing rather than the former fabricated fallback. - Add regression tests (NoReservationPricing) for all five services that fail pre-fix and pass post-fix. Addresses the Azure portion of #1020 (GCP half is handled separately). --- providers/azure/mocks/azure_mocks.go | 15 +++++- providers/azure/services/cache/client.go | 7 ++- providers/azure/services/cache/client_test.go | 48 +++++++++++++++++ providers/azure/services/compute/client.go | 7 ++- .../azure/services/compute/client_test.go | 38 ++++++++++++++ providers/azure/services/cosmosdb/client.go | 14 +++-- .../azure/services/cosmosdb/client_test.go | 49 ++++++++++++++++++ providers/azure/services/database/client.go | 7 ++- .../azure/services/database/client_test.go | 51 +++++++++++++++++++ providers/azure/services/search/client.go | 14 +++-- .../azure/services/search/client_test.go | 47 +++++++++++++++++ 11 files changed, 277 insertions(+), 20 deletions(-) diff --git a/providers/azure/mocks/azure_mocks.go b/providers/azure/mocks/azure_mocks.go index 23e9b3d6e..c69231c53 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": [ @@ -221,6 +223,17 @@ func CreateSampleVMPricingResponse() string { "reservationTerm": "1 Years", "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" + }, { "currencyCode": "USD", "retailPrice": 0.096, diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 9e45643ac..770a066f3 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -549,8 +549,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 diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 9333c510e..17b2c0cbb 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -125,6 +125,18 @@ func createSampleRedisPricingResponse() string { "reservationTerm": "1 Years", "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" + }, { "currencyCode": "USD", "retailPrice": 0.125, @@ -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") diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index aeb974da9..57fcd9803 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -657,8 +657,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 diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index 59dc60ad6..515d453d6 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -506,6 +506,44 @@ func TestComputeClient_GetOfferingDetails_NoPricing(t *testing.T) { assert.Contains(t, err.Error(), "no pricing data found") } +// 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 diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index e1bbaa5d9..b906be57f 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -541,8 +541,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) @@ -593,13 +598,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..fa97a42b0 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -136,6 +136,18 @@ func createSampleCosmosPricingResponse() string { "reservationTerm": "1 Years", "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" + }, { "currencyCode": "USD", "retailPrice": 0.008, @@ -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") diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index c775c10b2..4e67aeab9 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -535,8 +535,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 diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index 1e6d3ef21..f7f8d738e 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -120,6 +120,21 @@ func createSampleSQLPricingResponse() string { "armSkuName": "Standard_S0", "reservationTerm": "1 Years" }, + { + "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", "retailPrice": 0.096, @@ -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") diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index dadd1c986..6aa728ad1 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -487,8 +487,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) @@ -566,13 +571,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..38b90dc3d 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -123,6 +123,18 @@ func createSampleSearchPricingResponse() string { "reservationTerm": "1 Years", "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" + }, { "currencyCode": "USD", "retailPrice": 0.20, @@ -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") From 73f540a8b5f1fc95e59f144f4c00a5c4e20daf93 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:31:32 -0700 Subject: [PATCH 3/9] fix(azure): standardise term parsing and add Count/status guards (closes #1055) M4: lift ParseTermYears into services/internal/reservations so all service clients share one erroring parser. Five clients (compute, cache, cosmosdb, database, search) previously used a literal "3yr"||"3" check that silently coerced unknown terms to 1yr instead of returning an error. managedredis and synapse used private erroring parsers that are now removed in favour of the shared one. M6: add rec.Count <= 0 guard at the top of every PurchaseCommitment implementation. An Advisor recommendation with a missing qty field defaults to 0; without this guard a zero-quantity purchase reaches the Azure API and produces a confusing 400. Synapse already had the guard; it is now added to compute, cache, cosmosdb, database, search, and managedredis. M3: fix checkAndRegisterCapacityProvider to check resp.StatusCode before decoding the body. A 403/429/500 previously decoded to an empty RegistrationState, triggering a spurious register POST. The register POST response status is now also checked. Tests: added ParseTermYears table test, ZeroCountRejected regression tests for managedredis and compute, and a NonTwoxx regression test for the capacity-provider status check. --- providers/azure/services/cache/client.go | 17 ++++--- providers/azure/services/cache/client_test.go | 3 ++ providers/azure/services/compute/client.go | 33 +++++++++---- .../azure/services/compute/client_test.go | 46 +++++++++++++++++++ providers/azure/services/cosmosdb/client.go | 17 ++++--- .../azure/services/cosmosdb/client_test.go | 3 ++ providers/azure/services/database/client.go | 17 ++++--- .../azure/services/database/client_test.go | 3 ++ .../internal/reservations/purchase.go | 18 ++++++++ .../internal/reservations/purchase_test.go | 35 ++++++++++++++ .../azure/services/managedredis/client.go | 22 +++------ .../services/managedredis/client_test.go | 27 +++++++++-- providers/azure/services/search/client.go | 17 ++++--- .../azure/services/search/client_test.go | 3 ++ providers/azure/services/synapse/client.go | 18 +------- .../azure/services/synapse/client_test.go | 3 +- 16 files changed, 213 insertions(+), 69 deletions(-) diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 770a066f3..f76bccd4c 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) diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 17b2c0cbb..112ef09a7 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -1078,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}) @@ -1099,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}) @@ -1123,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}) diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index 57fcd9803..b3062e301 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" @@ -355,10 +356,18 @@ func (c *ComputeClient) checkAndRegisterCapacityProvider(ctx context.Context) er if err != nil { return fmt.Errorf("check provider: %w", err) } - defer resp.Body.Close() + body, _ := io.ReadAll(resp.Body) + resp.Body.Close() + + 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 fmt.Errorf("check Microsoft.Capacity provider status returned HTTP %d: %s", resp.StatusCode, string(body)) + } var state providerRegistrationState - if err := json.NewDecoder(resp.Body).Decode(&state); err != nil { + if err := json.Unmarshal(body, &state); err != nil { return fmt.Errorf("decode provider state: %w", err) } @@ -380,7 +389,11 @@ func (c *ComputeClient) checkAndRegisterCapacityProvider(ctx context.Context) er 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) return nil @@ -393,9 +406,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 +467,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 +520,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) diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index 515d453d6..eb43d9154 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -679,6 +679,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}) @@ -702,6 +703,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}) @@ -731,6 +733,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}) @@ -1259,3 +1262,46 @@ 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") +} + +// 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/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index b906be57f..82446fd52 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) diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index fa97a42b0..50885ad12 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -900,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}) @@ -921,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}) @@ -945,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 4e67aeab9..adca284b6 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) diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index f7f8d738e..bb1d08128 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -972,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}) @@ -993,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}) @@ -1017,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 9fa5bc1e7..fba80ada3 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -224,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) { @@ -257,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 @@ -325,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 } diff --git a/providers/azure/services/managedredis/client_test.go b/providers/azure/services/managedredis/client_test.go index e11ac3e38..87115cbf0 100644 --- a/providers/azure/services/managedredis/client_test.go +++ b/providers/azure/services/managedredis/client_test.go @@ -620,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) @@ -632,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) @@ -648,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) @@ -667,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) @@ -833,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/search/client.go b/providers/azure/services/search/client.go index 6aa728ad1..0eac7c9ca 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) diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index 38b90dc3d..7ce615ae3 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -782,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}) @@ -803,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}) @@ -827,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..bf1c01f75 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 } 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 { From 4c9c858c7024abfd2d62e65951d19a9cedda5755 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 04:31:56 -0700 Subject: [PATCH 4/9] fix(azure): document scope handling, fix error strings, inline contains alias M1/M2: add comment to ExtractedFields.Scope documenting that the field is populated but not yet threaded into the purchase body. The recommendation filter already constrains scope to Shared so the hardcoded value is always correct; the comment preserves the field for future wiring. M5: document the sync.Once sticky-failure contract on IsConfigured so operators know a transient auth failure at startup requires a process restart to recover. N2: lowercase the leading word of error strings in provider.go per Go revive/golint error-strings convention ("Azure is not configured" -> "azure provider is not configured", "Azure credentials validation failed" -> "azure credentials validation failed"). Update assertion strings in tests. N3: inline the contains() one-line alias in recommendations.go; call strings.Contains directly at the 4 call sites. The test for contains() is updated to call strings.Contains directly. L2: add a comment at convertToExchangeableReservation documenting that reservations with an empty ReservationOrderID (from malformed ARM IDs) are returned but callers MUST filter them before initiating an exchange. L3: add TestMergeServiceResults_OrderIsStable to pin the canonical compute->database->cache->cosmosdb->savingsplans->advisor merge order. L4: include accumulated recommendation count in the Advisor pagination error Warnf so operators can assess the completeness of partial results. --- .../internal/recommendations/converter.go | 10 ++++- providers/azure/provider.go | 25 ++++++++---- providers/azure/provider_test.go | 16 ++++---- providers/azure/recommendations.go | 18 ++++----- providers/azure/recommendations_test.go | 40 ++++++++++++++++++- providers/azure/services/compute/exchange.go | 9 ++++- 6 files changed, 90 insertions(+), 28 deletions(-) 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/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..996edd173 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 "" @@ -459,7 +463,3 @@ func shouldIncludeService(params common.RecommendationParams, service common.Ser 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..226964791 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) common.Recommendation { + return common.Recommendation{Service: svc, Provider: common.ProviderAzure} + } + + computeRec := mkRec(common.ServiceCompute) + dbRec := mkRec(common.ServiceRelationalDB) + cacheRec := mkRec(common.ServiceCache) + cosmosRec := mkRec(common.ServiceNoSQL) + spRec := mkRec(common.ServiceSavingsPlans) + advisorRec := mkRec(common.ServiceCompute) // 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, common.ServiceCompute, result[0].Service, "first must be compute") + assert.Equal(t, common.ServiceRelationalDB, result[1].Service, "second must be database") + assert.Equal(t, common.ServiceCache, result[2].Service, "third must be cache") + assert.Equal(t, common.ServiceNoSQL, result[3].Service, "fourth must be cosmosdb") + assert.Equal(t, common.ServiceSavingsPlans, result[4].Service, "fifth must be savingsplans") + assert.Equal(t, common.ServiceCompute, result[5].Service, "sixth must be advisor (compute-type)") +} 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, From f0759c7939dca9b32045f4f496e7f22515151908 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 06:24:45 -0700 Subject: [PATCH 5/9] fix(azure/compute): reduce cyclomatic complexity and fix gofmt Extract fetchCapacityProviderState and triggerCapacityProviderRegistration from checkAndRegisterCapacityProvider to bring complexity from 13 to under 10 (gocyclo limit). Also apply gofmt to recommendations.go (trailing newline) and pkg/exchange/reshape_crossfamily_test.go (field alignment). --- pkg/exchange/reshape_crossfamily_test.go | 16 ++++----- providers/azure/recommendations.go | 1 - providers/azure/services/compute/client.go | 41 ++++++++++++++-------- 3 files changed, 35 insertions(+), 23 deletions(-) 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/recommendations.go b/providers/azure/recommendations.go index 996edd173..8fe3719cd 100644 --- a/providers/azure/recommendations.go +++ b/providers/azure/recommendations.go @@ -462,4 +462,3 @@ func shouldIncludeService(params common.RecommendationParams, service common.Ser // Check if this is the requested service return params.Service == service } - diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index b3062e301..76025bff4 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -340,21 +340,33 @@ 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) } body, _ := io.ReadAll(resp.Body) resp.Body.Close() @@ -363,19 +375,19 @@ func (c *ComputeClient) checkAndRegisterCapacityProvider(ctx context.Context) er // 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 fmt.Errorf("check Microsoft.Capacity provider status returned HTTP %d: %s", resp.StatusCode, string(body)) + return providerRegistrationState{}, fmt.Errorf("check Microsoft.Capacity provider status returned HTTP %d: %s", resp.StatusCode, string(body)) } var state providerRegistrationState if err := json.Unmarshal(body, &state); err != nil { - return fmt.Errorf("decode provider state: %w", err) - } - - if state.RegistrationState == "Registered" { - return 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, @@ -384,7 +396,8 @@ 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) @@ -395,7 +408,7 @@ func (c *ComputeClient) checkAndRegisterCapacityProvider(ctx context.Context) er 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 } From 9d46d6ebe2a2652fd23f75ee3020654c65d17058 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 10:55:05 -0700 Subject: [PATCH 6/9] fix(azure/savingsplans): make purchase idempotent via deterministic alias name PurchaseCommitment discarded PurchaseOptions and named the SavingsPlanOrderAlias with time.Now().UnixNano(). Because the alias is created with an HTTP PUT on its name (armbillingbenefits SavingsPlanOrderAliasClient.BeginCreate uses PUT on .../savingsPlanOrderAliases/{name}), the name is the idempotency handle: a non-deterministic name means a re-drive of a stranded execution (issue #636) creates a second savings plan and double-charges the customer. Derive the alias name from opts.IdempotencyToken via common.ReservationOrderID so a re-drive PUTs the same alias (idempotent), falling back to the prior timestamp-based name on the CLI path where no token is supplied. This mirrors the reservation clients, which already route opts.IdempotencyToken through reservations.DoIdempotentPurchaseTwoStep. Regression tests assert two re-drives with the same token reuse the GUID alias name (fail pre-fix) and the empty-token CLI path keeps the timestamp name. --- .../azure/services/savingsplans/client.go | 17 +++++- .../services/savingsplans/client_test.go | 59 ++++++++++++++++++- 2 files changed, 73 insertions(+), 3 deletions(-) diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index cad66d974..8d98cf6b3 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -207,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, @@ -260,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) diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index 6ff203d5e..aa0b624e0 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -6,6 +6,7 @@ import ( "errors" "io" "net/http" + "strings" "testing" "time" @@ -44,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 } @@ -266,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) { From da76c08651f2796d2e394727fee7d159032cce11 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 11:05:22 -0700 Subject: [PATCH 7/9] fix(azure): fail loud on unrecognised payment option in offering details The GetOfferingDetails payment-option switch in compute, cache, cosmosdb, database, search and synapse silently treated any unrecognised PaymentOption as all-upfront (default: upfrontCost = totalCost), so a bad value would produce a plausible-but-wrong cost split surfaced as a real quote. Per the no-silent- fallbacks policy for money-affecting fields, the default branch now returns an explicit error instead of degrading to upfront. The branch is unreachable today (every converter pins PaymentOption to "upfront"), so this is a defence-in-depth fix with no behaviour change on the live path; it prevents a future monthly/other-payment recommendation from being silently mispriced. Regression test (compute, the canonical client): an unrecognised payment option with fully-present pricing now returns an error and a nil OfferingDetails; verified to fail pre-fix (returned a valid details with no error). --- providers/azure/services/cache/client.go | 5 +++- providers/azure/services/compute/client.go | 5 +++- .../azure/services/compute/client_test.go | 28 +++++++++++++++++++ providers/azure/services/cosmosdb/client.go | 5 +++- providers/azure/services/database/client.go | 5 +++- providers/azure/services/search/client.go | 5 +++- providers/azure/services/synapse/client.go | 5 +++- 7 files changed, 52 insertions(+), 6 deletions(-) diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index f76bccd4c..aa69f30fe 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -405,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{ diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index 76025bff4..127a9b40d 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -554,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{ diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index eb43d9154..0ce66e594 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -506,6 +506,34 @@ 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 diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index 82446fd52..0d27cafe7 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -398,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{ diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index adca284b6..dfd2ebe43 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -403,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{ diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index 0eac7c9ca..1095d561c 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -379,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{ diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index bf1c01f75..44110ac88 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -344,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{ From 5e938d9dd0f630b341482eb8392d20f8f011e79a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 11:32:57 -0700 Subject: [PATCH 8/9] fix(azure): handle singular "1 Year" form in all pricing extractors The Azure Retail Prices API returns "1 Year" (singular) for 1-year reservation terms and "N Years" (plural) for 2+ year terms. All six pricing extractors (compute, cache, database, cosmosdb, search, managedredis) used fmt.Sprintf("%d Years", termYears) unconditionally, so a 1-year reservation row was silently skipped and reservationPrice stayed 0. This caused a false "no reservation pricing found" error even when the reservation pricing row was present in the API response. Add azureTermString(termYears int) string helper to each affected package, returning "1 Year" for termYears==1 and "%d Years" otherwise. Synapse already handled the singular/plural distinction independently. Update the existing "1 Years" test fixtures in mock data and client_test.go files for all six packages to use the correct "1 Year" form, and add TestExtractVMPricing_SingularOneYear, TestExtractVMPricing_PluralThreeYears, TestExtractRedisPricing_SingularOneYear, TestExtractRedisPricing_PluralThreeYears regression tests that fail on the pre-fix code and pass after. Addresses CR finding on providers/azure/services/compute/client.go:699 and providers/azure/services/cache/client.go:566 from PR #1045 review. --- providers/azure/mocks/azure_mocks.go | 2 +- providers/azure/services/cache/client.go | 12 ++++- providers/azure/services/cache/client_test.go | 46 +++++++++++++++++- providers/azure/services/compute/client.go | 12 ++++- .../azure/services/compute/client_test.go | 48 +++++++++++++++++++ providers/azure/services/cosmosdb/client.go | 12 ++++- .../azure/services/cosmosdb/client_test.go | 2 +- providers/azure/services/database/client.go | 12 ++++- .../azure/services/database/client_test.go | 2 +- .../azure/services/managedredis/client.go | 12 ++++- .../services/managedredis/client_test.go | 4 +- providers/azure/services/search/client.go | 12 ++++- .../azure/services/search/client_test.go | 2 +- 13 files changed, 165 insertions(+), 13 deletions(-) diff --git a/providers/azure/mocks/azure_mocks.go b/providers/azure/mocks/azure_mocks.go index c69231c53..d5e11e1d3 100644 --- a/providers/azure/mocks/azure_mocks.go +++ b/providers/azure/mocks/azure_mocks.go @@ -220,7 +220,7 @@ func CreateSampleVMPricingResponse() string { "productName": "Virtual Machines D Series", "serviceName": "Virtual Machines", "armSkuName": "Standard_D2s_v3", - "reservationTerm": "1 Years", + "reservationTerm": "1 Year", "type": "Reservation" }, { diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index aa69f30fe..266e27816 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -596,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 112ef09a7..14397c747 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -122,7 +122,7 @@ func createSampleRedisPricingResponse() string { "serviceName": "Azure Cache for Redis", "armSkuName": "Premium_P1", "meterName": "P1 Instance", - "reservationTerm": "1 Years", + "reservationTerm": "1 Year", "type": "Reservation" }, { @@ -1285,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 127a9b40d..77d175b4a 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -734,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 0ce66e594..520bf0393 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -1316,6 +1316,54 @@ func TestCheckAndRegisterCapacityProvider_NonTwoxx(t *testing.T) { 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) { diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index 0d27cafe7..5c619e3b5 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -586,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 != "" { diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index 50885ad12..1b77ddfba 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -133,7 +133,7 @@ func createSampleCosmosPricingResponse() string { "serviceName": "Azure Cosmos DB", "skuName": "100RU", "meterName": "100 RU/s", - "reservationTerm": "1 Years", + "reservationTerm": "1 Year", "type": "Reservation" }, { diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index dfd2ebe43..acd598904 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -581,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 bb1d08128..95d2d8351 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -118,7 +118,7 @@ func createSampleSQLPricingResponse() string { "unitOfMeasure": "1 DTU/Hour", "type": "Reservation", "armSkuName": "Standard_S0", - "reservationTerm": "1 Years" + "reservationTerm": "1 Year" }, { "currencyCode": "USD", diff --git a/providers/azure/services/managedredis/client.go b/providers/azure/services/managedredis/client.go index fba80ada3..e38722c8c 100644 --- a/providers/azure/services/managedredis/client.go +++ b/providers/azure/services/managedredis/client.go @@ -488,11 +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 []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 87115cbf0..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" }, { @@ -541,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" } ], diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index 1095d561c..0ec7fb77b 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -548,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"` @@ -562,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 != "" { diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index 7ce615ae3..766570204 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -120,7 +120,7 @@ func createSampleSearchPricingResponse() string { "serviceName": "Azure Cognitive Search", "armSkuName": "Standard_S1", "meterName": "S1 Search Unit", - "reservationTerm": "1 Years", + "reservationTerm": "1 Year", "type": "Reservation" }, { From c873b35e8e7780fdfc260c0b14ef7a67fe0123b2 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sun, 7 Jun 2026 11:33:00 -0700 Subject: [PATCH 9/9] test(azure): disambiguate compute vs advisor in OrderIsStable test TestMergeServiceResults_OrderIsStable set Service: ServiceCompute on both computeRec and advisorRec. A compute/advisor position swap in mergeServiceResults would still pass since both assertions checked the same Service value. Add a marker string via a new ResourceType parameter to mkRec and assert on ResourceType instead of Service for result[0] and result[5] so compute and advisor entries are distinguishable and a position swap is caught immediately. Addresses CR finding on providers/azure/recommendations_test.go:454 from PR #1045 review. --- providers/azure/recommendations_test.go | 28 ++++++++++++------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/providers/azure/recommendations_test.go b/providers/azure/recommendations_test.go index 226964791..efdca18dc 100644 --- a/providers/azure/recommendations_test.go +++ b/providers/azure/recommendations_test.go @@ -423,16 +423,16 @@ func strPtr(s string) *string { // 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) common.Recommendation { - return common.Recommendation{Service: svc, Provider: common.ProviderAzure} + mkRec := func(svc common.ServiceType, marker string) common.Recommendation { + return common.Recommendation{Service: svc, Provider: common.ProviderAzure, ResourceType: marker} } - computeRec := mkRec(common.ServiceCompute) - dbRec := mkRec(common.ServiceRelationalDB) - cacheRec := mkRec(common.ServiceCache) - cosmosRec := mkRec(common.ServiceNoSQL) - spRec := mkRec(common.ServiceSavingsPlans) - advisorRec := mkRec(common.ServiceCompute) // Advisor produces Compute recs + 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( @@ -445,10 +445,10 @@ func TestMergeServiceResults_OrderIsStable(t *testing.T) { ) require.Len(t, result, 6, "all six services must be represented") - assert.Equal(t, common.ServiceCompute, result[0].Service, "first must be compute") - assert.Equal(t, common.ServiceRelationalDB, result[1].Service, "second must be database") - assert.Equal(t, common.ServiceCache, result[2].Service, "third must be cache") - assert.Equal(t, common.ServiceNoSQL, result[3].Service, "fourth must be cosmosdb") - assert.Equal(t, common.ServiceSavingsPlans, result[4].Service, "fifth must be savingsplans") - assert.Equal(t, common.ServiceCompute, result[5].Service, "sixth must be advisor (compute-type)") + 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)") }