Skip to content

Commit 1e3fabe

Browse files
committed
fix(azure/reservations): repair progressive-drop loop in BuildDisplayName
Address CodeRabbit findings on PR #686. Critical: BuildDisplayName's progressive-drop loop was broken. It called SanitizeDisplayName(candidate) inside the loop before checking the length cap. Because the sanitizer hard-truncates to 64 chars, the condition len(candidate) <= 64 was vacuously true on iteration 1 and the loop exited immediately, returning a mid-string-truncated full format instead of cleanly dropping the random suffix, timestamp, and payment segments. The existing tests passed only by coincidence (all test inputs happened to land their required segments within the first 64 bytes of the full format). Fix: check the pre-sanitized length to gate segment-dropping, and sanitize only on the exit path. Each segment is already conformant via normalizeSegment, so the in-loop sanitization was both wrong and redundant. Add three regression tests that fail against the pre-fix code: - DropLoopActuallyDropsTimestamp: forces the loop to drop both random and timestamp; asserts the exact clean output and that no timestamp digits leak in. - DropLoopActuallyDropsPayment: forces required-only fallback; asserts no payment fragment survives. - ZeroNowReplacedByWallClock: asserts a zero-value Now is replaced by time.Now() rather than emitting the placeholder "00010101T000000". Also address three minor doc/safety findings on the same file: - Clarify isAllowedDisplayNameChar godoc so the [A-Za-z0-9-] description matches reality (underscores are passed through by the caller). - Guard the zero-value Now case so production callers that forget to set it get a real timestamp instead of "00010101T000000". - Document generateRandSuffix's silent fallback for non-nil src shorter than 4 bytes. Verification: stash-revert the production code change with the new tests in place; all three regression tests fail with exactly the expected mid-truncated outputs. Restore the fix; all 68 reservations tests pass and all 620 azure-package tests pass.
1 parent 8add0cb commit 1e3fabe

2 files changed

Lines changed: 109 additions & 13 deletions

File tree

‎providers/azure/services/internal/reservations/displayname.go‎

Lines changed: 28 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -13,8 +13,10 @@ import (
1313
// Azure rejects longer values with HTTP 400 DisplayNameInvalid.
1414
const azureDisplayNameMaxLen = 64
1515

16-
// isAllowedDisplayNameChar reports whether r is in Azure's DisplayName allowlist
17-
// ([A-Za-z0-9-]). Underscores are emitted by the sanitizer, not passed through here.
16+
// isAllowedDisplayNameChar reports whether r is in Azure's base allowlist
17+
// [A-Za-z0-9-]. Underscores are handled separately by the caller
18+
// (SanitizeDisplayName passes '_' through verbatim alongside the chars
19+
// matched here).
1820
func isAllowedDisplayNameChar(r rune) bool {
1921
return (r >= 'A' && r <= 'Z') || (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-'
2022
}
@@ -75,8 +77,10 @@ type DisplayNameFields struct {
7577
// "monthly") so the segment stays under 8 chars.
7678
Payment string
7779

78-
// Now is the timestamp baseline. Must be non-zero for production calls.
79-
// Tests inject a fixed value for determinism.
80+
// Now is the timestamp baseline. Tests inject a fixed value for
81+
// determinism; production callers should pass time.Now(). A zero
82+
// time.Time is replaced with time.Now() by the builder so the
83+
// timestamp segment never emits the placeholder "00010101T000000".
8084
Now time.Time
8185

8286
// randSource is an optional 4-byte source for the random suffix.
@@ -114,7 +118,14 @@ func BuildDisplayName(f DisplayNameFields) string {
114118
count := fmt.Sprintf("%dx", f.Count)
115119
term := normalizeTerm(f.Term)
116120
paymt := normalizePayment(f.Payment)
117-
ts := f.Now.UTC().Format("20060102T150405")
121+
// Guard against a zero-value Now (which would emit the nonsensical
122+
// "00010101T000000"). Tests that want determinism pin Now explicitly;
123+
// production callers that forget get a real timestamp, not a placeholder.
124+
now := f.Now
125+
if now.IsZero() {
126+
now = time.Now()
127+
}
128+
ts := now.UTC().Format("20060102T150405")
118129
randHex := generateRandSuffix(f.randSource)
119130

120131
// Required segments (order matters — never dropped, never reordered).
@@ -124,17 +135,18 @@ func BuildDisplayName(f DisplayNameFields) string {
124135
// "keep" order; dropping happens from the right.
125136
tail := []string{paymt, ts, randHex}
126137

127-
// Try full -> drop random -> drop timestamp -> drop payment.
138+
// Try full -> drop random -> drop timestamp -> drop payment. Check the
139+
// PRE-sanitized length so the cap actually gates segment-dropping; calling
140+
// SanitizeDisplayName inside the loop would make the cap vacuously true
141+
// (the sanitizer hard-truncates to 64) and short-circuit the drop logic.
142+
// Each segment is already allowlist-conformant via normalizeSegment, so we
143+
// only need to sanitize once on the exit path as a defensive invariant.
128144
for keep := len(tail); keep >= 0; keep-- {
129145
segments := append([]string{}, required...)
130146
segments = append(segments, tail[:keep]...)
131147
candidate := joinNonEmpty(segments, "-")
132-
// Defensive sanitization: even though each segment is already
133-
// allowlist-conformant, SanitizeDisplayName guarantees the
134-
// invariant in one place rather than relying on every caller.
135-
candidate = SanitizeDisplayName(candidate)
136148
if len(candidate) <= azureDisplayNameMaxLen {
137-
return candidate
149+
return SanitizeDisplayName(candidate)
138150
}
139151
}
140152

@@ -206,8 +218,11 @@ func joinNonEmpty(parts []string, sep string) string {
206218
}
207219

208220
// generateRandSuffix returns 8 hex chars from src (test hook) or crypto/rand.
209-
// If randomness can't be obtained (extremely unlikely on supported platforms),
210-
// returns an empty string — the builder treats that as a dropped suffix.
221+
// If src is non-nil but shorter than 4 bytes it is treated as if nil and
222+
// crypto/rand is used as the fallback; callers that want deterministic output
223+
// must pass at least 4 bytes. If randomness can't be obtained (extremely
224+
// unlikely on supported platforms), returns an empty string, which the
225+
// builder treats as a dropped suffix.
211226
func generateRandSuffix(src []byte) string {
212227
if len(src) >= 4 {
213228
return hex.EncodeToString(src[:4])

‎providers/azure/services/internal/reservations/displayname_test.go‎

Lines changed: 81 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -409,6 +409,87 @@ func TestBuildDisplayName_EmptyPaymentSegmentIsSkipped(t *testing.T) {
409409
assert.Contains(t, got, "-1yr-20260522T190000-a1b2c3d4")
410410
}
411411

412+
// TestBuildDisplayName_DropLoopActuallyDropsTimestamp is a regression guard
413+
// against a previously-broken progressive-drop loop. The old implementation
414+
// called SanitizeDisplayName(candidate) INSIDE the loop before the length
415+
// check, which short-circuited the cap (the sanitizer hard-truncates to 64)
416+
// and returned a mid-string-truncated full format instead of cleanly dropping
417+
// the random suffix and timestamp. The fixed loop must produce a string that
418+
// ends exactly at the payment segment with no partial timestamp bytes.
419+
//
420+
// Input: full format = 83 chars, requires dropping both the 8-char random
421+
// suffix AND the 15-char timestamp; the result must end with "-allup" and
422+
// contain NO trace of the timestamp digits.
423+
func TestBuildDisplayName_DropLoopActuallyDropsTimestamp(t *testing.T) {
424+
got := BuildDisplayName(DisplayNameFields{
425+
Service: "vm",
426+
Region: "germanywestcentral", // 18 chars
427+
ResourceType: "Standard_NV24ads_A10_v5",
428+
Count: 1,
429+
Term: "1yr",
430+
Payment: "all-upfront",
431+
Now: fixedTime,
432+
}.WithRandSource(fixedRand))
433+
434+
// With the fixed drop loop, both optional ts and random are dropped
435+
// cleanly. The broken loop returned a 64-char mid-truncated value that
436+
// happened to include "20260" (start of the timestamp).
437+
want := "vm-germanywestcentral-Standard_NV24ads_A10_v5-1x-1yr-allup"
438+
assert.Equal(t, want, got)
439+
assert.LessOrEqual(t, len(got), 64)
440+
// Defensive: any partial timestamp prefix would surface as digits after
441+
// "allup-"; the fixed builder must not emit any of them.
442+
assert.NotContains(t, got, "allup-20")
443+
assert.NotContains(t, got, "20260")
444+
}
445+
446+
// TestBuildDisplayName_DropLoopActuallyDropsPayment is a second regression
447+
// guard for the drop loop, this time forcing the loop to drop the payment
448+
// segment too (keep=0, required-only). The broken loop again would short-
449+
// circuit at iteration 1 and mid-truncate, leaving partial payment bytes.
450+
func TestBuildDisplayName_DropLoopActuallyDropsPayment(t *testing.T) {
451+
got := BuildDisplayName(DisplayNameFields{
452+
Service: "vm",
453+
Region: "germanywestcentral",
454+
ResourceType: strings.Repeat("X", 30), // forces required-only fallback
455+
Count: 9999,
456+
Term: "1yr",
457+
Payment: "all-upfront",
458+
Now: fixedTime,
459+
}.WithRandSource(fixedRand))
460+
461+
// With the fixed drop loop, all optional segments drop cleanly and the
462+
// result is exactly the required segments joined by dashes.
463+
want := "vm-germanywestcentral-" + strings.Repeat("X", 30) + "-9999x-1yr"
464+
assert.Equal(t, want, got)
465+
assert.LessOrEqual(t, len(got), 64)
466+
// No payment fragment anywhere: the broken loop left "...1yr-a" at the
467+
// 64-char boundary.
468+
assert.NotContains(t, got, "allup")
469+
assert.NotContains(t, got, "1yr-a")
470+
}
471+
472+
// TestBuildDisplayName_ZeroNowReplacedByWallClock guards the zero-time guard:
473+
// a zero-value Now must not emit the placeholder "00010101T000000" segment.
474+
// The builder substitutes time.Now() so the timestamp is always meaningful.
475+
func TestBuildDisplayName_ZeroNowReplacedByWallClock(t *testing.T) {
476+
got := BuildDisplayName(DisplayNameFields{
477+
Service: "vm",
478+
Region: "eastus",
479+
ResourceType: "Standard_D2a_v4",
480+
Count: 1,
481+
Term: "1yr",
482+
Payment: "all-upfront",
483+
// Now intentionally left as zero value.
484+
}.WithRandSource(fixedRand))
485+
486+
assert.NotContains(t, got, "00010101T000000",
487+
"zero-value Now must be replaced by wall-clock time, not emitted as placeholder")
488+
// Sanity: still allowlist-conformant and within length cap.
489+
assert.LessOrEqual(t, len(got), 64)
490+
assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, got)
491+
}
492+
412493
func TestBuildDisplayName_TimestampUTC(t *testing.T) {
413494
// Even if the caller passes a local-zone time, the builder must
414495
// normalize to UTC so identifiers are comparable across hosts.

0 commit comments

Comments
 (0)