Repository navigation
sec(azure): harden HTTP clients + fix fabricated reservation prices in managedredis/savingsplans and 5 pricing clients - #1045
Merged
cristim merged 9 commits intoJun 7, 2026
Conversation
…agination
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
… 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).
cristim
added
triaged
Member
Author
|
@coderabbitai review |
Contributor
|
Warning Review limit reached
More reviews will be available in 26 minutes and 27 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
📝 WalkthroughWalkthroughThis PR implements comprehensive validation hardening and HTTP security improvements across all Azure service clients, introducing a shared ChangesAzure Validation and HTTP Hardening
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
Contributor
✅ Action performedReview finished.
|
This was referenced Jun 7, 2026
Closed
#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.
…ns 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.
Member
Author
FOLD-1045 additions (Medium/Low/Nit from report 09)Two new commits (73f540a, 4c9c858) add the remaining findings from the FOLD-1045 bucket in Functional changes
Deferred / documented decisions
Verification
@coderabbitai review |
Contributor
|
🧠 Learnings used✅ Action performedReview finished.
|
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).
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
…lias 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.
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).
Member
Author
Money-path verification pass + 2 fixesRan a deep money-path correctness verification of the Azure provider (reservation recommendation -> purchase) across all services, probing the GCP money-path bug classes. Two real defects found and fixed in this PR; everything else verified correct (details in the PR description's verification section). Commit
Commit
Verification: providers/azure |
Member
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
providers/azure/services/cosmosdb/client.go (1)
534-559:⚠️ Potential issue | 🟠 Major | ⚡ Quick winScope the Cosmos retail-price lookup to the requested SKU.
getCosmosPricing()still ignoressku, andextractCosmosPricing()picks the last reservation/on-demand row for the term across the whole region. In regions with multiple Cosmos reservation SKUs,GetOfferingDetailscan return a different SKU's quote instead of surfacing "no reservation pricing found".Suggested direction
func (c *CosmosDBClient) getCosmosPricing(ctx context.Context, sku, region string, termYears int) (*CosmosPricing, error) { - filter := fmt.Sprintf("serviceName eq 'Azure Cosmos DB' and armRegionName eq '%s'", region) + filter := fmt.Sprintf( + "serviceName eq 'Azure Cosmos DB' and armRegionName eq '%s' and (armSkuName eq '%s' or skuName eq '%s')", + region, sku, sku, + ) @@ - onDemandPrice, reservationPrice, currency := extractCosmosPricing(priceData.Items, termYears) + onDemandPrice, reservationPrice, currency := extractCosmosPricing(priceData.Items, sku, termYears) } -func extractCosmosPricing(items []CosmosRetailPriceItem, termYears int) (onDemand, reservation float64, currency string) { +func extractCosmosPricing(items []CosmosRetailPriceItem, sku string, termYears int) (onDemand, reservation float64, currency string) { currency = "USD" termStr := fmt.Sprintf("%d Years", termYears) for _, item := range items { + if item.ArmSKUName != "" && !strings.EqualFold(item.ArmSKUName, sku) && + item.SKUName != "" && !strings.EqualFold(item.SKUName, sku) { + continue + } + if item.CurrencyCode != "" { currency = item.CurrencyCode }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/services/cosmosdb/client.go` around lines 534 - 559, getCosmosPricing ignores the sku parameter so pricing is selected across all SKUs in a region; update the Azure retail-price filter and downstream selection so only entries for the requested SKU are considered. Specifically, include the SKU identifier in the filter passed to fetchAzurePricing (so filter string combines serviceName, armRegionName and the sku field used by the pricing API) and change extractCosmosPricing (or call-site logic) to restrict candidate items to the requested sku and termYears before computing onDemandPrice/reservationPrice/currency so reservationPrice truly corresponds to the requested SKU.providers/azure/services/search/client.go (1)
480-548:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftThe Search pricing lookup still isn't keyed to the requested SKU or full result set.
getSearchPricing()ignoressku, andfetchAzurePricing()still reads only the first page despite the response carryingNextPageLink. With multiple Search SKUs in one region or paginated price results,GetOfferingDetailscan either quote the wrong SKU or incorrectly raise the new "no reservation pricing found" error.Suggested direction
+type SearchRetailPriceItem 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"` +} + -func (c *SearchClient) fetchAzurePricing(ctx context.Context, filter string) (*AzureRetailPrice, error) { - ... - resp, err := c.httpClient.Do(req) - ... - var priceData AzureRetailPrice - if err := json.NewDecoder(resp.Body).Decode(&priceData); err != nil { - return nil, fmt.Errorf("failed to decode pricing response: %w", err) - } - return &priceData, nil +func (c *SearchClient) fetchAzurePricing(ctx context.Context, filter string) ([]SearchRetailPriceItem, error) { + 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() + return pricing.FetchAll[SearchRetailPriceItem](ctx, c.httpClient, initialURL, pricing.DefaultPageTimeout, pricing.DefaultMaxPages) } -func extractSearchPricing(items []struct { ... }, termYears int) ... +func extractSearchPricing(items []SearchRetailPriceItem, sku string, termYears int) ...and filter
extractSearchPricingonArmSKUName == skubefore choosing the reservation/on-demand rows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/services/search/client.go` around lines 480 - 548, getSearchPricing currently ignores sku and fetchAzurePricing only decodes the first page, so you can get wrong SKU results or miss reservation rows; update fetchAzurePricing to follow pagination by checking AzureRetailPrice.NextPageLink and repeatedly GET subsequent pages (accumulating Items into a single AzureRetailPrice.Items slice) before returning, and either include sku in the initial $filter or (preferably) make extractSearchPricing filter the returned priceData.Items by ArmSKUName == sku before computing onDemand/reservation prices; adjust getSearchPricing to pass sku through unchanged to extractSearchPricing so the pricing selection is correctly keyed to the requested SKU.
🧹 Nitpick comments (1)
providers/azure/services/synapse/client_test.go (1)
453-490: ⚡ Quick winPlease add a regression test for the new unsupported-payment branch.
GetOfferingDetailsnow errors on unrecognized payment options, but this suite still only covers the two happy paths plus transport failure. A one-case negative test here would pin the no-silent-fallback money-path change.Suggested test
+func TestGetOfferingDetails_unsupportedPaymentOption(t *testing.T) { + mHTTP := &mockHTTPClient{} + mHTTP.On("Do", mock.Anything).Return(newHTTPResponse(http.StatusOK, sampleSynapsePricingJSON), nil) + + c := &SynapseClient{subscriptionID: "sub-123", region: "eastus", httpClient: mHTTP} + + rec := common.Recommendation{ResourceType: "DW100c", Term: "1yr", PaymentOption: "weekly-bananas"} + details, err := c.GetOfferingDetails(context.Background(), rec) + require.Error(t, err) + assert.Nil(t, details) + assert.Contains(t, err.Error(), "unsupported payment option") +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@providers/azure/services/synapse/client_test.go` around lines 453 - 490, Add a negative regression test that asserts GetOfferingDetails returns an error for unrecognized payment options: create a test (e.g., TestGetOfferingDetails_unsupportedPayment) that sets up mockHTTP to return the same sampleSynapsePricingJSON, constructs a SynapseClient and a common.Recommendation with PaymentOption set to an invalid value like "unsupported", calls c.GetOfferingDetails(context.Background(), rec) and asserts an error is returned (and optionally that the error message mentions unsupported or payment). This ensures the new unsupported-payment branch in GetOfferingDetails is exercised instead of silently falling back.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@providers/azure/recommendations_test.go`:
- Around line 425-454: The test TestMergeServiceResults_OrderIsStable can
falsely pass because both computeRec and advisorRec set Service to
common.ServiceCompute; modify the mkRec fixture usage (or mkRec itself) to
include a unique identifying field on the Recommendation (e.g., set ResourceType
or Account on the computeRec and a different value on advisorRec) so the two
compute-type entries are distinguishable, then update the assertions after
calling mergeServiceResults to check the unique marker (ResourceType or Account)
for result[0] and result[5] instead of relying only on Service equality; this
targets mkRec, computeRec, advisorRec and the mergeServiceResults result checks.
In `@providers/azure/services/cache/client.go`:
- Around line 560-566: The error is triggered because extractRedisPricing only
matches the plural "%d Years" string so a singular "1 Year" row leaves
reservationPrice at 0 and causes a false-positive error; update
extractRedisPricing (and the same logic around reservationPrice handling) to
accept both singular and plural forms—e.g., match "%d Year" and "%d Years" or
use a regex/normalized comparison for termYears == 1 vs >1—so the code correctly
finds reservation pricing for 1-year entries and does not return the
fabricated-error branch.
In `@providers/azure/services/compute/client.go`:
- Around line 693-699: The new error on zero reservationPrice incorrectly flags
rows where Azure uses the singular term "1 Year" because extractVMPricing
currently only matches "%d Years"; update extractVMPricing (and any parsing used
by the reservationPrice calculation) to accept both singular and plural forms
(e.g., "1 Year" and "%d Years"/"%d Year") or normalize ReservationTerm before
parsing so that ReservationTerm == "1 Year" yields termYears==1 and sets
reservationPrice correctly; verify the same change is applied to the other
parsing site referenced in the comment to avoid false no-pricing errors.
---
Outside diff comments:
In `@providers/azure/services/cosmosdb/client.go`:
- Around line 534-559: getCosmosPricing ignores the sku parameter so pricing is
selected across all SKUs in a region; update the Azure retail-price filter and
downstream selection so only entries for the requested SKU are considered.
Specifically, include the SKU identifier in the filter passed to
fetchAzurePricing (so filter string combines serviceName, armRegionName and the
sku field used by the pricing API) and change extractCosmosPricing (or call-site
logic) to restrict candidate items to the requested sku and termYears before
computing onDemandPrice/reservationPrice/currency so reservationPrice truly
corresponds to the requested SKU.
In `@providers/azure/services/search/client.go`:
- Around line 480-548: getSearchPricing currently ignores sku and
fetchAzurePricing only decodes the first page, so you can get wrong SKU results
or miss reservation rows; update fetchAzurePricing to follow pagination by
checking AzureRetailPrice.NextPageLink and repeatedly GET subsequent pages
(accumulating Items into a single AzureRetailPrice.Items slice) before
returning, and either include sku in the initial $filter or (preferably) make
extractSearchPricing filter the returned priceData.Items by ArmSKUName == sku
before computing onDemand/reservation prices; adjust getSearchPricing to pass
sku through unchanged to extractSearchPricing so the pricing selection is
correctly keyed to the requested SKU.
---
Nitpick comments:
In `@providers/azure/services/synapse/client_test.go`:
- Around line 453-490: Add a negative regression test that asserts
GetOfferingDetails returns an error for unrecognized payment options: create a
test (e.g., TestGetOfferingDetails_unsupportedPayment) that sets up mockHTTP to
return the same sampleSynapsePricingJSON, constructs a SynapseClient and a
common.Recommendation with PaymentOption set to an invalid value like
"unsupported", calls c.GetOfferingDetails(context.Background(), rec) and asserts
an error is returned (and optionally that the error message mentions unsupported
or payment). This ensures the new unsupported-payment branch in
GetOfferingDetails is exercised instead of silently falling back.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1abf8e13-a463-4a14-8bba-29a3c05da976
📒 Files selected for processing (26)
pkg/exchange/reshape_crossfamily_test.goproviders/azure/internal/recommendations/converter.goproviders/azure/mocks/azure_mocks.goproviders/azure/provider.goproviders/azure/provider_test.goproviders/azure/recommendations.goproviders/azure/recommendations_test.goproviders/azure/services/cache/client.goproviders/azure/services/cache/client_test.goproviders/azure/services/compute/client.goproviders/azure/services/compute/client_test.goproviders/azure/services/compute/exchange.goproviders/azure/services/cosmosdb/client.goproviders/azure/services/cosmosdb/client_test.goproviders/azure/services/database/client.goproviders/azure/services/database/client_test.goproviders/azure/services/internal/reservations/purchase.goproviders/azure/services/internal/reservations/purchase_test.goproviders/azure/services/managedredis/client.goproviders/azure/services/managedredis/client_test.goproviders/azure/services/savingsplans/client.goproviders/azure/services/savingsplans/client_test.goproviders/azure/services/search/client.goproviders/azure/services/search/client_test.goproviders/azure/services/synapse/client.goproviders/azure/services/synapse/client_test.go
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.
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.
Member
Author
|
Addressed CR pass findings:
All 692 tests pass ( @coderabbitai review |
Contributor
|
Triggering an incremental review of commits ✏️ Learnings added
🧠 Learnings used✅ Action performedReview finished.
|
7 of 8 tasks
11 tasks
This was referenced Aug 3, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
H1 (sec):
managedredisandsavingsplansboth built bare&http.Client{Timeout:30s}instead ofhttpclient.New(), bypassing theblockIMDSDialerthat prevents SSRF credential exfiltration via the Azure IMDS endpoint. BothNewClientandNewClientWithHTTP(nil fallback) are now hardened.H2 (sec):
managedredis.getRedisPricinghand-rolled afor nextURL != ""pagination loop without the seen-URL guard, max-pages cap, or per-page timeout thatpricing.FetchAllenforces. Replaced withpricing.FetchAll[redisPriceItem](named type required to satisfy the generic constraint).H3 (fix):
savingsplans.fetchOnDemandRateinterpolated the$filterstring directly into the URL with only partialQueryEscape, relied on a single-page read, and returned(0, nil)on not-found -- conflating "rate is zero" with "lookup found nothing". Rewritten to useurl.Values{}.Encode(),pricing.FetchAll, and error-on-not-found.H4 (fix):
compute,cache,cosmosdb,database,searcheach fabricated a reservation price from a hardcoded discount multiplier (0.62 / 0.45 / 0.35 / 0.65 / 0.65) and surfaced it as a realTotalCost/SavingsPercentagewhen the Retail Prices API returned no reservation line. All five now return an error matching the patternmanagedredisalready uses.Test plan
go build ./...inproviders/azurepasses (verified)go test ./...inproviders/azurepasses: 685 tests, 0 failures (up from 667 pre-fix; 18 new regression tests added)managedredis/savingsplansconstructors;TestFetchOnDemandRate_NotFound/TestFetchOnDemandRate_URLEncoding;NoReservationPricingtest for all 5 H4 services;TestGetOfferingDetails_Paginatedalready existed in managedredis*_3YearTermtests that were previously passing via the fabricated fallback now pass with real 3-year pricing data added to their mock responsesScope expanded
Two additional commits fold in the remaining Medium/Low/Nit findings from the providers/azure code review (FOLD-1045 from docs/code-review/16-remaining-findings-plan.md):
M4: lifted
ParseTermYearsintoservices/internal/reservationsas the canonical erroring parser. Five clients (compute, cache, cosmosdb, database, search) previously used a literalrec.Term == "3yr" || rec.Term == "3"check that silently coerced unknown terms to 1yr. The private parsers inmanagedredisandsynapseare removed in favour of the shared one.M6: added
rec.Count <= 0guard at the top of everyPurchaseCommitmentimplementation. An Advisor recommendation with a missingqtyfield defaults to 0; without this guard a zero-quantity purchase reaches Azure and produces a confusing 400.M3:
checkAndRegisterCapacityProvidernow checksresp.StatusCodebefore decoding the body. A 403/429/500 previously decoded to an emptyRegistrationState, triggering a spurious register POST; the register POST response status is also now checked.M1/M2: added comment to
ExtractedFields.Scopedocumenting that the field is populated but not yet threaded into the purchase body. The recommendation filter already constrains scope toSharedso the hardcodedappliedScopeType: "Shared"is always correct; the comment preserves the field for future wiring.M5: documented the
sync.Oncesticky-failure contract onIsConfiguredso operators know a transient auth failure at startup requires a process restart.N2: lowercased leading word of error strings in
provider.goper Gorevive error-stringsconvention.N3: inlined the
contains()one-line alias; callstrings.Containsdirectly at the 4 call sites.L2: added comment at
convertToExchangeableReservationdocumenting that callers MUST filter emptyReservationOrderIDentries before initiating an exchange.L3: added
TestMergeServiceResults_OrderIsStableto pin the canonical compute->database->cache->cosmosdb->savingsplans->advisor merge order.L4: included accumulated recommendation count in the Advisor pagination error
Warnfso operators can assess completeness of partial results.Closes #1021
Also closes #1055
Addresses the Azure portion of #1020 (the GCP half is handled by a separate agent/PR).
Deep money-path verification pass (Azure provider)
A full money-path correctness verification of the Azure reservation recommendation -> purchase path was run across all services (compute, cache, cosmosdb, database, search, managedredis, savingsplans, reservations), applying the GCP money-path bug classes (invalid SDK enum literals; per-hour-vs-term savings unit mismatch; term not propagated; PaymentOption not honoured; fabricated prices; non-idempotent purchase; under-counted commitments; hardcoded magic ratios; silent money-affecting defaults) as the probe list.
Result: two real defects found and fixed; all other classes verified CORRECT.
retailPriceas the full-term price keyed byreservationTerm == "N Years"; the on-demandunitPriceis per-hour and is scaled to the same window viahoursInTerm = 8760 * termYears. Both sides of(onDemandPrice*hoursInTerm - reservationPrice)are full-term. The ~99.99%-bogus-savings GCP class is not present in Azure.reservations.ParseTermYears); savingsplans uses real SDK enum constants (noMEMORY_MB-class literal; ARM REST literals likeVirtualMachines/P%dY/Sharedare valid); the H4 hardcoded-discount fallback is fully removed;GetExistingCommitmentsand the reservation-order lookup paginate fully; errgroup uses per-leaf semaphore + post-Waitctx.Err();8760 * termYearsis a correct calendar constant (no fabricated memory/vCPU ratio);go test -raceis clean.savingsplans.PurchaseCommitmentdiscardedPurchaseOptionsand named the order alias withtime.Now().UnixNano(). Since the alias is created with an HTTP PUT (the name is the idempotency handle), a re-drive double-bought a savings plan. Fixed by deriving the alias name fromopts.IdempotencyTokenviacommon.ReservationOrderID, mirroring the reservation clients. Regression tests fail pre-fix, pass post-fix.GetOfferingDetailspayment-option switch in compute/cache/cosmosdb/database/search/synapse silently treated an unrecognisedPaymentOptionas all-upfront (default: upfrontCost = totalCost), surfacing a plausible-but-wrong cost split. Per the no-silent-fallbacks policy for money-affecting fields, the default branch now returns an explicit error. Unreachable on the live path today (converters pin "upfront"), so a defence-in-depth fix with a compute regression test that fails pre-fix.Verification: providers/azure
go build ./...OK,go vet ./...clean,go test -race ./...688 passed; repo-rootgo build ./...OK.Also closes #1079
Summary by CodeRabbit
Bug Fixes
Tests
Refactor