From ce62470f1db55d88ed541f586ba3a97e23f63bee Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 17 Jul 2026 20:20:37 +0300 Subject: [PATCH] fix(providers): correct Azure resourceType filters + fail-loud SP money parsing + nil covered-cost (adversarial-review) H1: Azure Search GetRecommendations now returns empty immediately; "AzureSearch" is not a valid Consumption API resourceType. Pre-fix queried with scope-only filter, causing Azure to return VirtualMachine recs mislabeled as Search recs. H2: Replace three hand-written Azure Consumption API resourceType strings with package-level typed constants: - Synapse: "SQLDatabaseDTU" -> reservationResourceTypeSynapse = "SqlDataWarehouse" - SQL DB: "SqlDatabase" -> reservationResourceTypeSQLDB = "SQLDatabases" - CosmosDB:"CosmosDb" -> reservationResourceTypeCosmosDB = "CosmosDB" All three pre-fix strings are invalid enum members; the API returned nothing. M1: ExpandPaymentVariants sets RecurringMonthlyCost to nil (not &0.0) when CommitmentCost is 0, distinguishing "data absent" from "known-zero cost" so the frontend renders "-" instead of fabricating "$0". M2: parseOptionalFloat now returns (float64, error) for money fields (HourlyCommitmentToPurchase, EstimatedMonthlySavingsAmount, UpfrontCost). Present-but-unparseable money fields propagate as errors and drop the recommendation; non-money fields (utilization percentages) still warn+0. Mirrors the RI path (parseAWSCostDetails) which already errored on this class. Extract spPlanTypeDisplayString helper to keep parseSavingsPlanDetail below the gocyclo-10 pre-commit threshold. Regression tests added for all four defects; all fail on pre-fix code and pass on post-fix code. --- providers/aws/recommendations/parser_sp.go | 105 +++++++++++++----- .../parser_sp_additional_test.go | 6 +- .../aws/recommendations/parser_sp_test.go | 58 +++++++++- .../internal/recommendations/converter.go | 24 +++- .../recommendations/converter_test.go | 12 +- providers/azure/services/cosmosdb/client.go | 9 +- .../azure/services/cosmosdb/client_test.go | 12 ++ providers/azure/services/database/client.go | 9 +- .../azure/services/database/client_test.go | 12 ++ providers/azure/services/search/client.go | 55 +++------ .../azure/services/search/client_test.go | 43 +++++++ providers/azure/services/synapse/client.go | 9 +- .../azure/services/synapse/client_test.go | 12 ++ 13 files changed, 285 insertions(+), 81 deletions(-) diff --git a/providers/aws/recommendations/parser_sp.go b/providers/aws/recommendations/parser_sp.go index f7913c805..87ec21468 100644 --- a/providers/aws/recommendations/parser_sp.go +++ b/providers/aws/recommendations/parser_sp.go @@ -167,7 +167,15 @@ func (c *Client) parseSavingsPlansRecommendations( var recommendations []common.Recommendation for _, detail := range spRec.SavingsPlansPurchaseRecommendationDetails { - rec := c.parseSavingsPlanDetail(&detail, params, planType) + rec, err := c.parseSavingsPlanDetail(&detail, params, planType) + if err != nil { + // present-but-unparseable money field: drop this recommendation + // rather than forwarding a corrupt $0 to the scheduler/frontend. + // Mirrors the RI path (parseAWSCostDetails) which errors on the + // same class. Log so operators can detect malformed CE responses. + log.Printf("WARNING: skipping SP recommendation (planType=%s): %v", planType, err) + continue + } if rec != nil { recommendations = append(recommendations, *rec) } @@ -176,16 +184,34 @@ func (c *Client) parseSavingsPlansRecommendations( return recommendations } -// parseSavingsPlanDetail converts a single Savings Plan recommendation detail -// parseOptionalFloat parses a string pointer as float64, logging a warning on failure. -// Returns 0 if the pointer is nil. -func parseOptionalFloat(field string, s *string) float64 { +// parseOptionalFloat parses a *string pointer as float64. +// - nil pointer (field absent from API response): returns (0, nil) — absent +// is acceptable; callers treat 0 as "not available". +// - non-nil pointer that fails ParseFloat (present but unparseable): returns +// (0, error) — mirrors the RI path (parseAWSCostDetails) which errors on +// the same class rather than silently substituting 0. Callers must propagate +// this error and drop the recommendation so a corrupt money figure never +// reaches the scheduler or frontend. +func parseOptionalFloat(field string, s *string) (float64, error) { if s == nil { - return 0 + return 0, nil } val, err := strconv.ParseFloat(*s, 64) if err != nil { - log.Printf("WARNING: failed to parse %s: %v", field, err) + return 0, fmt.Errorf("failed to parse %s %q: %w", field, *s, err) + } + return val, nil +} + +// parseOptionalFloatOrWarn parses a *string as float64 but treats present-but- +// unparseable as a non-fatal warning (logs and returns 0). Use ONLY for +// non-money fields (utilization percentages, averages) where a bad value +// degrades gracefully. For money fields use parseOptionalFloat and propagate +// the error. +func parseOptionalFloatOrWarn(field string, s *string) float64 { + val, err := parseOptionalFloat(field, s) + if err != nil { + log.Printf("WARNING: %v — treating as 0", err) return 0 } return val @@ -218,20 +244,57 @@ func extractEC2SPFields(planType types.SupportedSavingsPlansType, detail *types. } } +// spPlanTypeDisplayString converts a SupportedSavingsPlansType to a +// human-readable plan-type label used in SavingsPlanDetails.PlanType. +// Returns the raw SDK string for unrecognised types (forward-compat). +func spPlanTypeDisplayString(pt types.SupportedSavingsPlansType) string { + switch pt { + case types.SupportedSavingsPlansTypeComputeSp: + return "Compute" + case types.SupportedSavingsPlansTypeEc2InstanceSp: + return "EC2Instance" + case types.SupportedSavingsPlansTypeSagemakerSp: + return "SageMaker" + case types.SupportedSavingsPlansTypeDatabaseSp: + return "Database" + } + return string(pt) +} + +// parseSavingsPlanDetail converts a single Savings Plan recommendation detail +// into a *common.Recommendation. Returns (nil, error) when any money field +// (HourlyCommitmentToPurchase, EstimatedMonthlySavingsAmount, UpfrontCost) is +// present in the API response but cannot be parsed as float64 — mirroring the +// RI path (parseAWSCostDetails) which errors on the same class rather than +// silently substituting 0. Non-money fields (percentages, utilization) degrade +// to 0 with a log warning. func (c *Client) parseSavingsPlanDetail( detail *types.SavingsPlansPurchaseRecommendationDetail, params *common.RecommendationParams, planType types.SupportedSavingsPlansType, -) *common.Recommendation { - hourlyCommitment := parseOptionalFloat("HourlyCommitmentToPurchase", detail.HourlyCommitmentToPurchase) - monthlySavings := parseOptionalFloat("EstimatedMonthlySavingsAmount", detail.EstimatedMonthlySavingsAmount) - savingsPercent := parseOptionalFloat("EstimatedSavingsPercentage", detail.EstimatedSavingsPercentage) - upfrontCost := parseOptionalFloat("UpfrontCost", detail.UpfrontCost) +) (*common.Recommendation, error) { + hourlyCommitment, err := parseOptionalFloat("HourlyCommitmentToPurchase", detail.HourlyCommitmentToPurchase) + if err != nil { + return nil, err + } + monthlySavings, err := parseOptionalFloat("EstimatedMonthlySavingsAmount", detail.EstimatedMonthlySavingsAmount) + if err != nil { + return nil, err + } + upfrontCost, err := parseOptionalFloat("UpfrontCost", detail.UpfrontCost) + if err != nil { + return nil, err + } + + // Non-money fields: present-but-unparseable degrades to 0 with a warning + // rather than dropping the whole recommendation. The RI path uses the same + // two-tier treatment (hard error on cost, warn-and-continue on percentages). + savingsPercent := parseOptionalFloatOrWarn("EstimatedSavingsPercentage", detail.EstimatedSavingsPercentage) // EstimatedAverageUtilization carries the "if you buy exactly this commitment, // what % of it will AWS expect to be used" signal. Used by --target-coverage // sizing in cmd/helpers.go; zero (nil pointer or parse failure) means "no signal" // and the sizing path leaves the recommendation unchanged. - recommendedUtilization := parseOptionalFloat("EstimatedAverageUtilization", detail.EstimatedAverageUtilization) + recommendedUtilization := parseOptionalFloatOrWarn("EstimatedAverageUtilization", detail.EstimatedAverageUtilization) // onDemandCost is the canonical monthly on-demand baseline for this SP // recommendation. AWS Cost Explorer returns the average hourly on-demand // spend over the lookback period in CurrentAverageHourlyOnDemandSpend; @@ -242,7 +305,7 @@ func (c *Client) parseSavingsPlanDetail( // monthly_cost + savings + amortized (which is less accurate for SP rows // where monthly_cost reflects only the no-upfront recurring charge, not // the full on-demand baseline). See #303. - onDemandCost := parseOptionalFloat("CurrentAverageHourlyOnDemandSpend", detail.CurrentAverageHourlyOnDemandSpend) * hoursPerMonth + onDemandCost := parseOptionalFloatOrWarn("CurrentAverageHourlyOnDemandSpend", detail.CurrentAverageHourlyOnDemandSpend) * hoursPerMonth if detail.CurrentAverageHourlyOnDemandSpend == nil { // CurrentAverageHourlyOnDemandSpend absent from AWS CE response — onDemandCost // will be 0 and the scheduler's nonZeroPtr will store nil, causing the @@ -251,17 +314,7 @@ func (c *Client) parseSavingsPlanDetail( log.Printf("WARNING: CurrentAverageHourlyOnDemandSpend is nil for SP recommendation (planType=%s, account=%s) — Effective %% will use reconstruction fallback", planType, aws.ToString(detail.AccountId)) } - planTypeStr := string(planType) - switch planType { - case types.SupportedSavingsPlansTypeComputeSp: - planTypeStr = "Compute" - case types.SupportedSavingsPlansTypeEc2InstanceSp: - planTypeStr = "EC2Instance" - case types.SupportedSavingsPlansTypeSagemakerSp: - planTypeStr = "SageMaker" - case types.SupportedSavingsPlansTypeDatabaseSp: - planTypeStr = "Database" - } + planTypeStr := spPlanTypeDisplayString(planType) accountID := "" if detail.AccountId != nil { @@ -318,7 +371,7 @@ func (c *Client) parseSavingsPlanDetail( Region: ec2Fields.region, OfferingID: ec2Fields.offeringID, }, - } + }, nil } // planTypesForParams resolves which AWS Cost Explorer plan types to query diff --git a/providers/aws/recommendations/parser_sp_additional_test.go b/providers/aws/recommendations/parser_sp_additional_test.go index ae6914a4c..592b140ae 100644 --- a/providers/aws/recommendations/parser_sp_additional_test.go +++ b/providers/aws/recommendations/parser_sp_additional_test.go @@ -149,7 +149,8 @@ func TestParseSavingsPlanDetail(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - rec := client.parseSavingsPlanDetail(tt.detail, &tt.params, tt.planType) + rec, err := client.parseSavingsPlanDetail(tt.detail, &tt.params, tt.planType) + require.NoError(t, err) require.NotNil(t, rec) if tt.validate != nil { tt.validate(t, rec) @@ -367,7 +368,8 @@ func TestParseSavingsPlanDetail_OnDemandCost(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - rec := client.parseSavingsPlanDetail(tt.detail, ¶ms, types.SupportedSavingsPlansTypeComputeSp) + rec, err := client.parseSavingsPlanDetail(tt.detail, ¶ms, types.SupportedSavingsPlansTypeComputeSp) + require.NoError(t, err) require.NotNil(t, rec) assert.InDelta(t, tt.wantOnDemand, rec.OnDemandCost, 0.001, "OnDemandCost should equal CurrentAverageHourlyOnDemandSpend × 730") diff --git a/providers/aws/recommendations/parser_sp_test.go b/providers/aws/recommendations/parser_sp_test.go index e297a2318..3065fc32f 100644 --- a/providers/aws/recommendations/parser_sp_test.go +++ b/providers/aws/recommendations/parser_sp_test.go @@ -194,7 +194,9 @@ func TestParseSavingsPlanDetail_RecommendedUtilization(t *testing.T) { HourlyCommitmentToPurchase: aws.String("1.0"), EstimatedAverageUtilization: tt.utilizationStr, } - rec := client.parseSavingsPlanDetail(detail, ¶ms, types.SupportedSavingsPlansTypeComputeSp) + rec, err := client.parseSavingsPlanDetail(detail, ¶ms, types.SupportedSavingsPlansTypeComputeSp) + require.NoError(t, err, + "EstimatedAverageUtilization is a non-money field; parse failures must not propagate as errors") require.NotNil(t, rec) assert.Equal(t, tt.wantUtilization, rec.RecommendedUtilization, "SP utilization should be parsed into rec.RecommendedUtilization") @@ -293,7 +295,8 @@ func TestParseSavingsPlanDetail_EC2InstanceFieldsCaptured(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - rec := client.parseSavingsPlanDetail(tt.detail, params, tt.planType) + rec, err := client.parseSavingsPlanDetail(tt.detail, ¶ms, tt.planType) + require.NoError(t, err) require.NotNil(t, rec) spDetails, ok := rec.Details.(*common.SavingsPlanDetails) @@ -308,3 +311,54 @@ func TestParseSavingsPlanDetail_EC2InstanceFieldsCaptured(t *testing.T) { }) } } + +// TestParseSavingsPlanDetail_MoneyFieldUnparseable is the M2 regression test: +// a present-but-unparseable money field (HourlyCommitmentToPurchase, +// EstimatedMonthlySavingsAmount, UpfrontCost) must return an error, NOT a +// silently-fabricated $0. Pre-fix, parseOptionalFloat swallowed parse errors +// for all fields and substituted 0; the fix errors on money fields and +// uses warn+0 only for non-money fields (percentages/averages). +func TestParseSavingsPlanDetail_MoneyFieldUnparseable(t *testing.T) { + client := &Client{} + params := common.RecommendationParams{ + Service: common.ServiceSavingsPlansCompute, + PaymentOption: "no-upfront", + Term: "1yr", + } + + tests := []struct { + name string + detail *types.SavingsPlansPurchaseRecommendationDetail + }{ + { + name: "unparseable HourlyCommitmentToPurchase", + detail: &types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("not-a-number"), + }, + }, + { + name: "unparseable EstimatedMonthlySavingsAmount", + detail: &types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("1.0"), + EstimatedMonthlySavingsAmount: aws.String("bad-value"), + }, + }, + { + name: "unparseable UpfrontCost", + detail: &types.SavingsPlansPurchaseRecommendationDetail{ + HourlyCommitmentToPurchase: aws.String("1.0"), + UpfrontCost: aws.String("not-a-float"), + }, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + rec, err := client.parseSavingsPlanDetail(tt.detail, ¶ms, types.SupportedSavingsPlansTypeComputeSp) + require.Error(t, err, + "present-but-unparseable money field must return an error, not a silently-fabricated $0") + assert.Nil(t, rec, + "rec must be nil when a money field is unparseable") + }) + } +} diff --git a/providers/azure/internal/recommendations/converter.go b/providers/azure/internal/recommendations/converter.go index 1b65ca7d5..962fdfe4c 100644 --- a/providers/azure/internal/recommendations/converter.go +++ b/providers/azure/internal/recommendations/converter.go @@ -367,19 +367,37 @@ func ExpandPaymentVariants(base common.Recommendation) []common.Recommendation { } months := termToMonths(base.Term) - recurringMonthly := totalReservation / float64(months) + + // RecurringMonthlyCost semantics: + // - nil : CommitmentCost was absent from the provider response; the + // frontend renders "-" (data not available) rather than "$0". + // - &0.0 : all-upfront variant with a known non-zero CommitmentCost; the + // full charge was already paid upfront, so the recurring charge + // is a known zero. + // - &N : monthly variant; CommitmentCost spread evenly over term months. + // + // When CommitmentCost is 0 it means data was absent from the provider, NOT + // that the reservation is free. Using float64Ptr(0) in that case fabricates + // a non-nil &0.0 that the frontend renders as "$0" instead of "-", which is + // incorrect. Guard: only set non-nil pointers when we have real cost data. + var upfrontRecurring, monthlyRecurring *float64 + if totalReservation != 0 { + upfrontRecurring = float64Ptr(0) + monthly := totalReservation / float64(months) + monthlyRecurring = float64Ptr(monthly) + } allUpfront := base allUpfront.PaymentOption = "upfront" allUpfront.EstimatedSavings = savings allUpfront.SavingsPercentage = savingsPct - allUpfront.RecurringMonthlyCost = float64Ptr(0) + allUpfront.RecurringMonthlyCost = upfrontRecurring noUpfront := base noUpfront.PaymentOption = "monthly" noUpfront.EstimatedSavings = savings noUpfront.SavingsPercentage = savingsPct - noUpfront.RecurringMonthlyCost = float64Ptr(recurringMonthly) + noUpfront.RecurringMonthlyCost = monthlyRecurring return []common.Recommendation{allUpfront, noUpfront} } diff --git a/providers/azure/internal/recommendations/converter_test.go b/providers/azure/internal/recommendations/converter_test.go index 5c832e4ec..84d34d442 100644 --- a/providers/azure/internal/recommendations/converter_test.go +++ b/providers/azure/internal/recommendations/converter_test.go @@ -397,11 +397,17 @@ func TestExpandPaymentVariants_ZeroOnDemand_NonZeroCommitment(t *testing.T) { } func TestExpandPaymentVariants_ZeroCommitmentCost(t *testing.T) { - // Zero reservation total: both variants still emitted; no-upfront monthly = 0. + // M1 regression: when CommitmentCost is 0 (absent from provider response), + // both variants must carry nil RecurringMonthlyCost so the frontend renders + // "-" (data absent) rather than "$0" (known-zero). The pre-fix code + // unconditionally set RecurringMonthlyCost = &0.0 for both variants, + // fabricating a non-nil pointer even when no cost data was available. variants := ExpandPaymentVariants(baseRec(common.ServiceCompute, "1yr", 50, 0)) require.Len(t, variants, 2) - require.NotNil(t, variants[1].RecurringMonthlyCost) - assert.InDelta(t, 0.0, *variants[1].RecurringMonthlyCost, 1e-9) + assert.Nil(t, variants[0].RecurringMonthlyCost, + "all-upfront variant: RecurringMonthlyCost must be nil when CommitmentCost is absent") + assert.Nil(t, variants[1].RecurringMonthlyCost, + "no-upfront variant: RecurringMonthlyCost must be nil when CommitmentCost is absent") } func TestExpandPaymentVariants_SharedFieldsCarriedThrough(t *testing.T) { diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index 5c8cad242..b74393f14 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -27,6 +27,13 @@ import ( "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) +// reservationResourceTypeCosmosDB is the canonical resourceType value for +// Azure Cosmos DB in the Consumption API $filter. +// Source: Azure REST API spec for Microsoft.Consumption/reservationRecommendations +// (2021-10-01 stable). The previous hand-written value "CosmosDb" has wrong +// case; the correct value is "CosmosDB". +const reservationResourceTypeCosmosDB = "CosmosDB" + // maxRecsPages caps Consumption API recommendation pagination. const maxRecsPages = 10 @@ -166,7 +173,7 @@ func (c *CosmosDBClient) GetRecommendations(ctx context.Context, params common.R // filter — see the parallel comment in compute/client.go for the // failure mode that the wrong shape produced. scope := fmt.Sprintf("/subscriptions/%s", c.subscriptionID) - filter := "properties/scope eq 'Shared' and properties/resourceType eq 'CosmosDb'" + filter := "properties/scope eq 'Shared' and properties/resourceType eq '" + reservationResourceTypeCosmosDB + "'" pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter}) } diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index 1b77ddfba..a75354eb0 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -1317,3 +1317,15 @@ func TestCosmosDBClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t assert.Regexp(t, `^cosmos-`, capturedDisplayName) assert.Contains(t, capturedDisplayName, "EnableCassandra") } + +// TestReservationResourceTypeCosmosDB_IsCanonical is the H2c regression test. +// +// The Consumption ReservationRecommendations API $filter for Azure Cosmos DB +// requires exactly "CosmosDB" (uppercase DB). The pre-fix code used the +// hand-written string "CosmosDb" (lowercase d) which is not a valid resourceType +// enum member, causing the API to return no recommendations. +func TestReservationResourceTypeCosmosDB_IsCanonical(t *testing.T) { + assert.Equal(t, "CosmosDB", reservationResourceTypeCosmosDB, + "Cosmos DB filter value must be the Azure REST API canonical enum \"CosmosDB\"; "+ + "\"CosmosDb\" (pre-fix) is not a valid resourceType") +} diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index 28842f4c1..542300619 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -26,6 +26,13 @@ import ( "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) +// reservationResourceTypeSQLDB is the canonical resourceType value for Azure +// SQL Databases in the Consumption API $filter. +// Source: Azure REST API spec for Microsoft.Consumption/reservationRecommendations +// (2021-10-01 stable). The previous hand-written value "SqlDatabase" is not a +// valid enum member; the correct case is "SQLDatabases". +const reservationResourceTypeSQLDB = "SQLDatabases" + // maxRecsPages caps Consumption API recommendation pagination. const maxRecsPages = 10 @@ -171,7 +178,7 @@ func (c *DatabaseClient) GetRecommendations(ctx context.Context, params common.R // filter — see the parallel comment in compute/client.go for the // failure mode that the wrong shape produced. scope := fmt.Sprintf("/subscriptions/%s", c.subscriptionID) - filter := "properties/scope eq 'Shared' and properties/resourceType eq 'SqlDatabase'" + filter := "properties/scope eq 'Shared' and properties/resourceType eq '" + reservationResourceTypeSQLDB + "'" pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter}) } diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index a2d9f2a84..b366c3e18 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -1298,3 +1298,15 @@ func TestDatabaseClient_PurchaseCommitment_CanonicalReservedResourceType(t *test "reservedResourceType %q must be a member of armreservations.PossibleReservedResourceTypeValues()", capturedRRT) mockHTTP.AssertExpectations(t) } + +// TestReservationResourceTypeSQLDB_IsCanonical is the H2b regression test. +// +// The Consumption ReservationRecommendations API $filter for Azure SQL Databases +// requires exactly "SQLDatabases" (uppercase SQL, plural). The pre-fix code used +// the hand-written string "SqlDatabase" (lowercase SQL, singular) which is not a +// valid resourceType enum member, causing the API to return no recommendations. +func TestReservationResourceTypeSQLDB_IsCanonical(t *testing.T) { + assert.Equal(t, "SQLDatabases", reservationResourceTypeSQLDB, + "SQL Database filter value must be the Azure REST API canonical enum \"SQLDatabases\"; "+ + "\"SqlDatabase\" (pre-fix) is not a valid resourceType") +} diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index 07dce3006..4bd9587a2 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -129,48 +129,19 @@ type AzureRetailPrice struct { Count int `json:"Count"` } -// GetRecommendations gets Azure Search reservation recommendations from Azure Consumption API -func (c *SearchClient) GetRecommendations(ctx context.Context, params common.RecommendationParams) ([]common.Recommendation, error) { - recommendations := make([]common.Recommendation, 0) - - // Use injected pager if available (for testing) - var pager RecommendationsPager - if c.recommendationsPager != nil { - pager = c.recommendationsPager - } else { - client, err := armconsumption.NewReservationRecommendationsClient(c.cred, nil) - if err != nil { - return nil, fmt.Errorf("failed to create consumption client: %w", err) - } - // NewListPager's first argument is the billing scope, NOT the - // filter — see the parallel comment in compute/client.go for the - // failure mode that the wrong shape produced. - scope := fmt.Sprintf("/subscriptions/%s", c.subscriptionID) - filter := "properties/scope eq 'Shared'" - pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter}) - } - - for pageIdx := 0; pager.More(); pageIdx++ { - if err := ctx.Err(); err != nil { - return nil, fmt.Errorf("context cancelled during pagination: %w", err) - } - if pageIdx >= maxRecsPages { - return nil, fmt.Errorf("search: GetRecommendations pagination cap (%d pages) reached", maxRecsPages) - } - page, err := pager.NextPage(ctx) - if err != nil { - return nil, fmt.Errorf("failed to get Search recommendations: %w", err) - } - - for _, rec := range page.Value { - converted := c.convertAzureSearchRecommendation(ctx, rec) - if converted != nil { - recommendations = append(recommendations, azrecs.ExpandPaymentVariants(*converted)...) - } - } - } - - return recommendations, nil +// GetRecommendations returns empty for Azure Search because the Azure Consumption +// ReservationRecommendations API has no Search-specific resourceType. The +// valid resourceType values are: VirtualMachines, SQLDatabases, PostgreSQL, +// ManagedDisk, MySQL, RedHat, MariaDB, RedisCache, CosmosDB, SqlDataWarehouse, +// SUSELinux, AppService, BlockBlob, AzureDataExplorer, VMwareCloudSimple. +// +// Querying without a resourceType filter (or with an invalid value) causes the +// API to default to VirtualMachines and return VM reservation recommendations, +// which would be mislabelled as Search recommendations. Returning empty is the +// correct behaviour until Azure exposes a Search reservation recommendation +// stream via this API. +func (c *SearchClient) GetRecommendations(_ context.Context, _ common.RecommendationParams) ([]common.Recommendation, error) { + return []common.Recommendation{}, nil } // GetExistingCommitments retrieves existing Search reserved capacity diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index 766570204..91a434dfb 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -445,6 +445,49 @@ func TestSearchClient_GetRecommendations_WithMockPager(t *testing.T) { assert.Empty(t, recs) } +// TestSearchClient_GetRecommendations_AlwaysEmpty is the H1 regression test. +// +// "AzureSearch" is not a valid resourceType in the Consumption +// ReservationRecommendations API (valid list: VirtualMachines, SQLDatabases, +// CosmosDB, SqlDataWarehouse, etc.). The pre-fix code queried with only a +// scope filter, causing Azure to return VirtualMachine recommendations that +// were then mislabeled as Search recommendations. +// +// Post-fix, GetRecommendations returns an empty slice immediately regardless +// of what the pager contains. This test injects a non-empty pager (mimicking +// the VM recs Azure would return) and asserts the result is empty. +func TestSearchClient_GetRecommendations_AlwaysEmpty(t *testing.T) { + ctx := context.Background() + client := NewClient(nil, "test-subscription", "eastus") + + // Simulate what the Azure Consumption API returns when queried without a + // resourceType filter: a VirtualMachines recommendation that must NOT be + // forwarded as a Search recommendation. + vmRec := mocks.BuildLegacyReservationRecommendation( + mocks.WithRegion("eastus"), + mocks.WithTerm("P1Y"), + mocks.WithQuantity(1), + mocks.WithNormalizedSize("Standard_D2s_v3"), + mocks.WithCosts(100, 70, 30), + ) + pagerWithVMRecs := &MockRecommendationsPager{ + pages: []armconsumption.ReservationRecommendationsClientListResponse{ + { + ReservationRecommendationsListResult: armconsumption.ReservationRecommendationsListResult{ + Value: []armconsumption.ReservationRecommendationClassification{vmRec}, + }, + }, + }, + } + client.SetRecommendationsPager(pagerWithVMRecs) + + recs, err := client.GetRecommendations(ctx, common.RecommendationParams{}) + require.NoError(t, err) + assert.Empty(t, recs, + "Azure Search has no Consumption API reservation stream; GetRecommendations "+ + "must always return empty regardless of what the injected pager contains") +} + func TestSearchClient_GetExistingCommitments_WithMockPager(t *testing.T) { ctx := context.Background() client := NewClient(nil, "test-subscription", "eastus") diff --git a/providers/azure/services/synapse/client.go b/providers/azure/services/synapse/client.go index 652810d1d..b11e793ec 100644 --- a/providers/azure/services/synapse/client.go +++ b/providers/azure/services/synapse/client.go @@ -26,6 +26,13 @@ import ( "github.com/LeanerCloud/CUDly/providers/azure/services/internal/reservations" ) +// reservationResourceTypeSynapse is the canonical resourceType value for Azure +// Synapse Analytics (Dedicated SQL Pool) in the Consumption API $filter. +// Source: Azure REST API spec for Microsoft.Consumption/reservationRecommendations +// (2021-10-01 stable). The previous hand-written value "SQLDatabaseDTU" is not +// a valid enum member and caused the API to return no recommendations. +const reservationResourceTypeSynapse = "SqlDataWarehouse" + // HTTPClient interface for HTTP operations (enables mocking). type HTTPClient interface { Do(req *http.Request) (*http.Response, error) @@ -128,7 +135,7 @@ func (c *SynapseClient) GetRecommendations(ctx context.Context, params common.Re return nil, fmt.Errorf("failed to create consumption client: %w", err) } scope := fmt.Sprintf("/subscriptions/%s", c.subscriptionID) - filter := "properties/scope eq 'Shared' and properties/resourceType eq 'SQLDatabaseDTU'" + filter := "properties/scope eq 'Shared' and properties/resourceType eq '" + reservationResourceTypeSynapse + "'" pager = client.NewListPager(scope, &armconsumption.ReservationRecommendationsClientListOptions{Filter: &filter}) } diff --git a/providers/azure/services/synapse/client_test.go b/providers/azure/services/synapse/client_test.go index 14127491e..ed54aa522 100644 --- a/providers/azure/services/synapse/client_test.go +++ b/providers/azure/services/synapse/client_test.go @@ -906,6 +906,18 @@ func TestPurchaseCommitment_canonicalReservedResourceType(t *testing.T) { "reservedResourceType %q must be a member of armreservations.PossibleReservedResourceTypeValues()", capturedRRT) } +// TestReservationResourceTypeSynapse_IsCanonical is the H2a regression test. +// +// The Consumption ReservationRecommendations API $filter for Azure Synapse +// Analytics (Dedicated SQL Pool) requires exactly "SqlDataWarehouse". +// The pre-fix code used the hand-written string "SQLDatabaseDTU" which is not +// a valid resourceType enum member, causing the API to return no recommendations. +func TestReservationResourceTypeSynapse_IsCanonical(t *testing.T) { + assert.Equal(t, "SqlDataWarehouse", reservationResourceTypeSynapse, + "Synapse filter value must be the Azure REST API canonical enum \"SqlDataWarehouse\"; "+ + "\"SQLDatabaseDTU\" (pre-fix) is not a valid resourceType") +} + // ---- nil HTTP client fallback ---------------------------------------------- // TestNewClientWithHTTP_NilFallbackIsHardened is a regression test for the