fix(azure): sanitize Azure DisplayName to [A-Za-z0-9_-] for calculatePrice (closes #685)

[cristim]

Summary

• Azure's Reservations API rejects DisplayName values containing characters outside [A-Za-z0-9_-] with HTTP 400 DisplayNameInvalid — blocking every approve for affected SKUs (e.g. Standard_D2a_v4)
• Adds SanitizeDisplayName helper in providers/azure/internal/reservations/ that replaces non-allowlisted characters with _, collapses runs, and truncates to 64 chars
• Replaces space-containing format strings in all 5 service clients (cache, compute, cosmosdb, database, search) with underscore-form prefixes piped through the sanitizer

Test plan

• go test ./... -count=1 passes (362 tests, all green)
• 11 new unit tests for SanitizeDisplayName covering: spaces, special chars, run-collapsing, exact-64/65/100-char truncation, already-conformant input, empty input
• 5 new per-service PurchaseCommitment_DisplayNameConformsToAzureAllowlist tests capture the PUT body and assert ^[A-Za-z0-9_-]{1,64}$ — regression guard for future format changes
• gofmt, go vet, gocyclo all pass via pre-commit hooks

After merge + deploy, the user can retry the previously-failing execution d73b0ffe-8ada-42e0-902b-27a63ce54088 (Standard_D2a_v4 VM).

Summary by CodeRabbit

• Bug Fixes

  • Fixed display name formatting for Azure reservations across cache, compute, Cosmos DB, database, and search services to comply with Azure's character and length requirements, preventing potential submission errors.

• Tests

  • Added comprehensive tests to validate that display names conform to Azure platform requirements across all affected services.

[coderabbitai]

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0d6ba1fc-1028-48d9-9e1d-12a5538fcdcf

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

• 🔍 Trigger review

📝 Walkthrough

Walkthrough

This PR introduces a centralized utility to sanitize Azure Reservation display names across multiple service clients, ensuring all reservation requests conform to Azure's allowlist constraints of letters, digits, hyphens, and underscores, with a 64-character maximum length.

Changes

Display Name Sanitization for Azure Reservations

Layer / File(s)	Summary
Sanitizer utility implementation and validation providers/azure/internal/reservations/displayname.go, providers/azure/internal/reservations/displayname_test.go	Core SanitizeDisplayName function filters input strings to allowed Azure characters [A-Za-z0-9_-], collapses consecutive invalid characters into underscores, and truncates to 64 characters. Comprehensive table-driven tests validate character transformations, run collapsing, and regex conformance ^[A-Za-z0-9_-]{1,64}$.
Cache and Compute service client integration providers/azure/services/cache/client.go, providers/azure/services/cache/client_test.go, providers/azure/services/compute/client.go, providers/azure/services/compute/client_test.go	Cache and Compute clients import the sanitizer, update PurchaseCommitment request payloads to sanitize displayName before sending to Azure, and introduce HTTP request interception tests that validate the outgoing displayName field matches the Azure allowlist regex.
Cosmos DB, Database, and Search service client integration providers/azure/services/cosmosdb/client.go, providers/azure/services/cosmosdb/client_test.go, providers/azure/services/database/client.go, providers/azure/services/database/client_test.go, providers/azure/services/search/client.go, providers/azure/services/search/client_test.go	Cosmos DB, Database, and Search clients apply the same pattern: import the sanitizer, wrap reservation request displayName values with SanitizeDisplayName, and add conformance tests that intercept HTTP bodies to validate the displayName matches the allowed regex and is non-empty.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~8 minutes

Possibly related issues

• bug(providers/azure): displayName contains spaces — every Azure approve fails with DisplayNameInvalid #685: Both PRs add the same SanitizeDisplayName helper and update the same Azure service clients and tests to produce allowlist-conformant displayName strings across cache, compute, Cosmos DB, database, and search services.

Suggested labels

priority/p3, severity/low, urgency/eventually, impact/few, effort/m

Poem

🐰 A rabbit hops through Azure clouds so blue,
With names now trimmed to fit the allowlist true.
No spaces, no dashes beyond the pair—
Display names sanitized with utmost care! ✨

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly and specifically describes the main change: sanitizing Azure DisplayName to a specific character set [A-Za-z0-9_-] to fix validation failures, and references the related issue (#685).
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch fix/issue-685-azure-displayname

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Rebased onto latest feat/multicloud-web-frontend. Resolved 7 conflicts across 5 service clients: 5 client.go import-line conflicts (#680's opts.Source guard now sits alongside our displayName sanitization, both invariants apply at PurchaseCommitment entry) and 2 client_test.go conflicts (database/search). Also consolidated the new displayname.go helper into providers/azure/services/internal/reservations/ (same package as DoPurchaseTwoStep) so the displayName line can resolve the symbol without a second reservations import. The 5 PurchaseCommitment_DisplayNameConformsToAzureAllowlist regression tests were adapted to use the two-step calculatePrice mock pattern established by #680 and now pass on the rebased base.

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Extended this PR with a richer DisplayName format that mirrors the AWS RI CSV's ReservationId column shape so operators can identify reservations in the Azure portal without cross-referencing the purchase audit log.

Format (each segment separated by -, sanitized to [A-Za-z0-9_-]{1,64}):

{svc}-{region}-{sku}-{count}x-{term}-{paymt}-{ts}-{rand}

Example (compute / VM):

vm-eastus-Standard_D2a_v4-1x-1yr-allup-20260522T190000-a1b2c3d4

Per-service codes: vm, redis, cosmos, sql, search.

Length-fit strategy. When the composed string would exceed 64 chars (long region + long SKU + high count), the builder progressively drops the optional tail segments — random first → timestamp → payment-option. Service code, region, SKU, count, and term are never dropped (those are the high-signal segments). If even the joined required segments would exceed 64, they're truncated via SanitizeDisplayName as a last-resort safety net so the builder never emits >64 chars. The TestBuildDisplayName_LengthFit* family exercises each drop priority.

Backward-compat note. This changes the operator-visible DisplayName from VM_Reservation_<SKU> (and equivalents per service) to the rich format above. Existing reservations are unaffected; only newly-purchased reservations get the new identifier.

Code: providers/azure/services/internal/reservations/displayname.go (new BuildDisplayName alongside the existing SanitizeDisplayName); wired into all 5 service clients (cache / compute / cosmosdb / database / search).

Tests: helper has 14 sub-tests covering happy-path, per-service shape, three length-fit drop priorities, the pathological-input truncation fallback, payment normalization (both unit and visible-in-output), term normalization, dirty-input sanitization, embedded-dash collapse, determinism, different-rand-different-output, production crypto/rand path, empty-payment skip, and UTC normalization. Per-service _DisplayNameConformsToAzureAllowlist tests are extended to also assert the service-code prefix and SKU presence so accidental swaps in the literal strings are caught.

@coderabbitai review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Fetch the PR diff
gh pr view 686 --repo LeanerCloud/CUDly --json files,headRefName,baseRefName

# Get the actual displayname.go file
fd -e go displayname providers/azure/

Length of output: 1580

---

🏁 Script executed:

#!/bin/bash
cat providers/azure/services/internal/reservations/displayname.go
echo "---FILE_SEPARATOR---"
cat providers/azure/services/internal/reservations/displayname_test.go
echo "---FILE_SEPARATOR---"
cat providers/azure/services/compute/client.go
echo "---FILE_SEPARATOR---"
cat providers/azure/services/cache/client.go
echo "---FILE_SEPARATOR---"
cat providers/azure/services/cosmosdb/client.go
echo "---FILE_SEPARATOR---"
cat providers/azure/services/database/client.go
echo "---FILE_SEPARATOR---"
cat providers/azure/services/search/client.go

Length of output: 146191

---

@cristim — the rich BuildDisplayName design is excellent from an operator-experience standpoint. One critical correctness issue in the loop, plus a few minor points below.

---

🔴 Critical: Progressive-drop loop is broken — it always truncates instead of dropping whole segments

BuildDisplayName (providers/azure/services/internal/reservations/displayname.go) calls SanitizeDisplayName(candidate) inside the loop before checking the length. Because SanitizeDisplayName always returns ≤ 64 chars (via its hard truncation), the condition len(candidate) <= azureDisplayNameMaxLen is vacuously true on the very first iteration (keep == len(tail) == 3). The loop exits immediately every time, and the function returns a mid-string-truncated full format instead of a cleanly segmented fallback.

The tests still pass, but by coincidence: for every test input the required segments happen to land entirely within the first 64 bytes of the full format, and the optional segments are naturally chopped off by truncation. The structured drop the comments describe never executes.

Example (germanywestcentral + Standard_NV24ads_A10_v5 + 999x):

Full (89 chars): search-germanywestcentral-Standard_NV24ads_A10_v5-999x-1yr-allup-20260522T190000-a1b2c3d4
Truncated (64):  search-germanywestcentral-Standard_NV24ads_A10_v5-999x-1yr-allup
Intended drop-ts: search-germanywestcentral-Standard_NV24ads_A10_v5-999x-1yr-allup

These happen to be identical here, but won't be for shorter SKUs where the timestamp fits in 64 chars (e.g. vm-eastus-Standard_D2a_v4-1x-1yr-allup-20260522T190000-a1b2c3d4 = 63 chars) — in that case the loop would never drop the random suffix even when the intended strategy calls for it at 65+ chars.

Fix — check the pre-sanitized length, then sanitize only when returning:

for keep := len(tail); keep >= 0; keep-- {
    segments := append([]string{}, required...)
    segments = append(segments, tail[:keep]...)
    candidate := joinNonEmpty(segments, "-")
    if len(candidate) <= azureDisplayNameMaxLen {
        return SanitizeDisplayName(candidate)   // sanitize once, on the exit path
    }
}
// last-resort: truncate required-only via sanitizer
return SanitizeDisplayName(joinNonEmpty(required, "-"))

This also eliminates the redundant SanitizeDisplayName call on every loop iteration (since every segment is already individually sanitized by normalizeSegment).

---

🟡 Minor: isAllowedDisplayNameChar godoc is inconsistent with the actual allowlist

// isAllowedDisplayNameChar reports whether r is in Azure's DisplayName allowlist
// ([A-Za-z0-9-]).

The function excludes _ intentionally (the caller in SanitizeDisplayName handles _ via the r == '_' branch), but the godoc omits this explanation. The comment on SanitizeDisplayName correctly states [A-Za-z0-9_-]. Consider updating:

// isAllowedDisplayNameChar reports whether r is in Azure's base allowlist
// [A-Za-z0-9-]. Underscores are handled separately by the caller.

---

🟡 Minor: Zero-value Now produces a nonsensical timestamp segment

f.Now.UTC().Format("20060102T150405") on a zero time.Time emits "00010101T000000". The field godoc says "Must be non-zero for production calls" but this is silent. Consider adding a guard or replacing zero-time with time.Now():

ts := f.Now
if ts.IsZero() {
    ts = time.Now()
}

---

🟡 Minor: generateRandSuffix silently falls back for 1–3 byte slices

The doc says "When nil (production), the builder reads from crypto/rand." But a non-nil src with fewer than 4 bytes also silently falls back to crypto/rand, which could surprise a test author who accidentally passes a short slice. Consider returning an error or panic in that case, or document the behaviour:

// If src is non-nil but shorter than 4 bytes, crypto/rand is used as fallback.

---

🟢 Everything else looks solid

• normalizeSegment correctly converts embedded dashes → underscores before the join, preventing parse-time ambiguity. ✅
• joinNonEmpty correctly eliminates double-dash artifacts for empty payment. ✅
• UTC normalisation, crypto/rand production path, and the 14-test suite structure are all well-designed. ✅
• All five service-client wiring points (vm/redis/cosmos/sql/search) are consistent. ✅

The loop bug is the only item that changes observable behaviour in production — specifically for SKUs where the full format fits within 64 chars (the vast majority of common SKUs), the function works correctly by coincidence. The bug surfaces only for the long-input cases where structured dropping was intended.

[cristim]

Addressed in 1e3fabe:

1. Critical (drop loop): fixed by moving sanitization to the exit path so the length cap actually gates segment-dropping. Added 3 regression tests (DropLoopActuallyDropsTimestamp, DropLoopActuallyDropsPayment, ZeroNowReplacedByWallClock) that fail against the pre-fix code with exactly the broken-loop mid-truncated outputs, and pass after the fix. Verified via stash-revert.
2. isAllowedDisplayNameChar godoc: fixed; clarified that underscores are handled separately by the caller (SanitizeDisplayName).
3. Zero-value Now: fixed; guards a zero time.Time with time.Now() so the placeholder 00010101T000000 is never emitted. Updated the Now field godoc to reflect this.
4. generateRandSuffix silent fallback: documented; godoc now states that a non-nil src shorter than 4 bytes falls back to crypto/rand, with guidance to pass at least 4 bytes for deterministic output.

All 68 reservations tests and all 620 azure-package tests pass.

@coderabbitai review

[coderabbitai]

@cristim — thank you for the thorough follow-up! All four points are addressed cleanly:

1. Drop loop (critical) — moving sanitization to the exit path is exactly the right fix, and the three targeted regression tests (DropLoopActuallyDropsTimestamp, DropLoopActuallyDropsPayment, ZeroNowReplacedByWallClock) that were verified to fail against the pre-fix code give strong confidence the invariant is now locked in. ✅
2. isAllowedDisplayNameChar godoc — the clarification eliminates the apparent contradiction with SanitizeDisplayName's doc. ✅
3. Zero Now guard — replacing zero-time with time.Now() is the right default; the updated field godoc reflects it. ✅
4. generateRandSuffix silent fallback — documented behaviour is much better than silent surprise. ✅

Triggering a fresh review of the rebased branch now.

🐇 ✨

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

@cristim — thank you for the thorough follow-up! All four points are addressed cleanly:

1. Drop loop (critical) — moving sanitization to the exit path is exactly the right fix, and the three targeted regression tests (DropLoopActuallyDropsTimestamp, DropLoopActuallyDropsPayment, ZeroNowReplacedByWallClock) that were verified to fail against the pre-fix code give strong confidence the invariant is now locked in. ✅
2. isAllowedDisplayNameChar godoc — the clarification eliminates the apparent contradiction with SanitizeDisplayName's doc. ✅
3. Zero Now guard — replacing zero-time with time.Now() is the right default; the updated field godoc reflects it. ✅
4. generateRandSuffix silent fallback — documented behaviour is much better than silent surprise. ✅

Triggering a fresh review of the rebased branch now.

🐇 ✨

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.