diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index e7fd1921f..5139ff9d5 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -299,9 +299,17 @@ func (c *CacheClient) PurchaseCommitment(ctx context.Context, rec common.Recomme "billingScopeId": fmt.Sprintf("/subscriptions/%s", c.subscriptionID), "term": fmt.Sprintf("P%dY", termYears), "quantity": rec.Count, - "displayName": fmt.Sprintf("Redis Cache Reservation - %s", rec.ResourceType), - "appliedScopeType": "Shared", - "renew": false, + "displayName": reservations.BuildDisplayName(reservations.DisplayNameFields{ + Service: "redis", + Region: c.region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }), + "appliedScopeType": "Shared", + "renew": false, }, } applyPurchaseAutomationTag(requestBody, opts.Source) diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 930f39885..3380d6083 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -1145,3 +1145,54 @@ func TestCacheClient_PurchaseCommitment_RequiresSource(t *testing.T) { // No HTTP call may be issued when the guard rejects the request. mockHTTP.AssertNotCalled(t, "Do", mock.Anything) } + +// TestCacheClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist guards +// against regression: displayName in the calculatePrice body must match +// [A-Za-z0-9_-]{1,64} (Azure rejects DisplayNameInvalid otherwise). +func TestCacheClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t *testing.T) { + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + const orderID = "azure-cache-displayname" + var capturedDisplayName string + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + if r.Method != http.MethodPost || r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { + return false + } + if r.Body == nil { + return true + } + bodyBytes, _ := io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) + var body map[string]interface{} + if err := json.Unmarshal(bodyBytes, &body); err == nil { + if props, ok := body["properties"].(map[string]interface{}); ok { + if dn, ok := props["displayName"].(string); ok { + capturedDisplayName = dn + } + } + } + return true + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && + r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + + rec := common.Recommendation{ + ResourceType: "Premium_P1", + Term: "1yr", + Count: 1, + CommitmentCost: 1000.0, + } + _, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + assert.NotEmpty(t, capturedDisplayName) + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, capturedDisplayName) + // Rich-format guards: service code is correct and SKU is preserved + // (see providers/azure/services/internal/reservations/displayname.go). + assert.Regexp(t, `^redis-`, capturedDisplayName) + assert.Contains(t, capturedDisplayName, "Premium_P1") +} diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index e016e963f..4638bba3a 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -378,9 +378,17 @@ func (c *ComputeClient) buildReservationBody(rec common.Recommendation, source s "billingScopeId": fmt.Sprintf("/subscriptions/%s", c.subscriptionID), "term": fmt.Sprintf("P%dY", termYears), "quantity": rec.Count, - "displayName": fmt.Sprintf("VM Reservation - %s", rec.ResourceType), - "appliedScopeType": "Shared", - "renew": false, + "displayName": reservations.BuildDisplayName(reservations.DisplayNameFields{ + Service: "vm", + Region: c.region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }), + "appliedScopeType": "Shared", + "renew": false, }, } if source != "" { diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index fb9b2076b..5daf697b9 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -1064,3 +1064,56 @@ func TestComputeClient_CachedSKULookup_FetchedOnce(t *testing.T) { } assert.Equal(t, 1, mockPager.pageHits, "catalogue must be fetched ONCE regardless of lookup count") } + +// TestComputeClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist guards +// against regression: displayName in the calculatePrice body must match +// [A-Za-z0-9_-]{1,64} (Azure rejects DisplayNameInvalid otherwise). +func TestComputeClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t *testing.T) { + ctx := context.Background() + mockHTTP := &mocks.MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + mockCapacityProviderCheck(mockHTTP) + + const orderID = "azure-vm-displayname" + var capturedDisplayName string + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + if r.Method != http.MethodPost || r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { + return false + } + if r.Body == nil { + return true + } + bodyBytes, _ := io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) + var body map[string]interface{} + if err := json.Unmarshal(bodyBytes, &body); err == nil { + if props, ok := body["properties"].(map[string]interface{}); ok { + if dn, ok := props["displayName"].(string); ok { + capturedDisplayName = dn + } + } + } + return true + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && + r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" + })).Return(mocks.CreateMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + + rec := common.Recommendation{ + ResourceType: "Standard_D2s_v3", + Term: "1yr", + Count: 1, + CommitmentCost: 2000.0, + } + _, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + assert.NotEmpty(t, capturedDisplayName) + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, capturedDisplayName) + // Rich-format guards: service code is correct and SKU is preserved + // (see providers/azure/services/internal/reservations/displayname.go). + assert.Regexp(t, `^vm-`, capturedDisplayName) + assert.Contains(t, capturedDisplayName, "Standard_D2s_v3") +} diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index b77ad88f7..bb42424ce 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -292,9 +292,17 @@ func (c *CosmosDBClient) PurchaseCommitment(ctx context.Context, rec common.Reco "billingScopeId": fmt.Sprintf("/subscriptions/%s", c.subscriptionID), "term": fmt.Sprintf("P%dY", termYears), "quantity": rec.Count, - "displayName": fmt.Sprintf("Cosmos DB Reservation - %s", rec.ResourceType), - "appliedScopeType": "Shared", - "renew": false, + "displayName": reservations.BuildDisplayName(reservations.DisplayNameFields{ + Service: "cosmos", + Region: c.region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }), + "appliedScopeType": "Shared", + "renew": false, }, } applyPurchaseAutomationTag(requestBody, opts.Source) diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index 7dd73df61..b4e596665 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -1214,3 +1214,54 @@ func TestDetailsFromCosmosSKU(t *testing.T) { }) } } + +// TestCosmosDBClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist guards +// against regression: displayName in the calculatePrice body must match +// [A-Za-z0-9_-]{1,64} (Azure rejects DisplayNameInvalid otherwise). +func TestCosmosDBClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t *testing.T) { + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + const orderID = "azure-cosmos-displayname" + var capturedDisplayName string + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + if r.Method != http.MethodPost || r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { + return false + } + if r.Body == nil { + return true + } + bodyBytes, _ := io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) + var body map[string]interface{} + if err := json.Unmarshal(bodyBytes, &body); err == nil { + if props, ok := body["properties"].(map[string]interface{}); ok { + if dn, ok := props["displayName"].(string); ok { + capturedDisplayName = dn + } + } + } + return true + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && + r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + + rec := common.Recommendation{ + ResourceType: "EnableCassandra", + Term: "1yr", + Count: 100, + CommitmentCost: 5000.0, + } + _, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + assert.NotEmpty(t, capturedDisplayName) + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, capturedDisplayName) + // Rich-format guards: service code is correct and SKU is preserved + // (see providers/azure/services/internal/reservations/displayname.go). + assert.Regexp(t, `^cosmos-`, capturedDisplayName) + assert.Contains(t, capturedDisplayName, "EnableCassandra") +} diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index f0238c00c..63c72175c 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -300,9 +300,17 @@ func (c *DatabaseClient) PurchaseCommitment(ctx context.Context, rec common.Reco "billingScopeId": fmt.Sprintf("/subscriptions/%s", c.subscriptionID), "term": fmt.Sprintf("P%dY", termYears), "quantity": rec.Count, - "displayName": fmt.Sprintf("SQL DB Reservation - %s", rec.ResourceType), - "appliedScopeType": "Shared", - "renew": false, + "displayName": reservations.BuildDisplayName(reservations.DisplayNameFields{ + Service: "sql", + Region: c.region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }), + "appliedScopeType": "Shared", + "renew": false, }, } applyPurchaseAutomationTag(requestBody, opts.Source) diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index c26628466..1e6d3ef21 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -1141,3 +1141,54 @@ func TestDatabaseClient_ValidateOffering_CaseInsensitive(t *testing.T) { assert.NoError(t, err) }) } + +// TestDatabaseClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist guards +// against regression: displayName in the calculatePrice body must match +// [A-Za-z0-9_-]{1,64} (Azure rejects DisplayNameInvalid otherwise). +func TestDatabaseClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t *testing.T) { + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + const orderID = "azure-db-displayname" + var capturedDisplayName string + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + if r.Method != http.MethodPost || r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { + return false + } + if r.Body == nil { + return true + } + bodyBytes, _ := io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) + var body map[string]interface{} + if err := json.Unmarshal(bodyBytes, &body); err == nil { + if props, ok := body["properties"].(map[string]interface{}); ok { + if dn, ok := props["displayName"].(string); ok { + capturedDisplayName = dn + } + } + } + return true + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && + r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + + rec := common.Recommendation{ + ResourceType: "GP_Gen5_2", + Term: "1yr", + Count: 1, + CommitmentCost: 1500.0, + } + _, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + assert.NotEmpty(t, capturedDisplayName) + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, capturedDisplayName) + // Rich-format guards: service code is correct and SKU is preserved + // (see providers/azure/services/internal/reservations/displayname.go). + assert.Regexp(t, `^sql-`, capturedDisplayName) + assert.Contains(t, capturedDisplayName, "GP_Gen5_2") +} diff --git a/providers/azure/services/internal/reservations/displayname.go b/providers/azure/services/internal/reservations/displayname.go new file mode 100644 index 000000000..97b55d881 --- /dev/null +++ b/providers/azure/services/internal/reservations/displayname.go @@ -0,0 +1,235 @@ +// Package reservations provides shared helpers for Azure Reservations API operations. +package reservations + +import ( + "crypto/rand" + "encoding/hex" + "fmt" + "strings" + "time" +) + +// azureDisplayNameMaxLen is Azure's hard cap on Reservation DisplayName length. +// Azure rejects longer values with HTTP 400 DisplayNameInvalid. +const azureDisplayNameMaxLen = 64 + +// isAllowedDisplayNameChar reports whether r is in Azure's base allowlist +// [A-Za-z0-9-]. Underscores are handled separately by the caller +// (SanitizeDisplayName passes '_' through verbatim alongside the chars +// matched here). +func isAllowedDisplayNameChar(r rune) bool { + return (r >= 'A' && r <= 'Z') || (r >= 'a' && r <= 'z') || (r >= '0' && r <= '9') || r == '-' +} + +// SanitizeDisplayName returns s with any character outside [A-Za-z0-9_-] +// replaced by '_', truncated to 64 chars. Azure rejects DisplayName fields +// that don't match this allowlist with HTTP 400 DisplayNameInvalid. +// Runs of non-conforming characters are collapsed into a single '_'. +func SanitizeDisplayName(s string) string { + var b strings.Builder + b.Grow(len(s)) + lastWasUnderscore := false + for _, r := range s { + if isAllowedDisplayNameChar(r) || r == '_' { + b.WriteRune(r) + lastWasUnderscore = r == '_' + } else if !lastWasUnderscore { + b.WriteByte('_') + lastWasUnderscore = true + } + } + result := b.String() + if len(result) > azureDisplayNameMaxLen { + // All output chars are ASCII so byte index == rune index. + result = result[:azureDisplayNameMaxLen] + } + return result +} + +// DisplayNameFields carries the inputs needed by BuildDisplayName. +// +// Now and randSource are exposed so tests can pin time and randomness for +// deterministic assertions. Production callers leave randSource nil (the +// builder then uses crypto/rand) and pass time.Now(). +type DisplayNameFields struct { + // Service is a short identifier for the Azure service, e.g. "vm", + // "redis", "cosmos", "sql", "search". Set per-call by the service + // client; the builder treats it as opaque and sanitizes it. + Service string + + // Region is the Azure location string (e.g. "eastus", "westeurope"). + Region string + + // ResourceType is the Azure SKU name (e.g. "Standard_D2a_v4"). + ResourceType string + + // Count is the reservation quantity. Always rendered as "{N}x". + Count int + + // Term is the commitment term, normalized to "1yr" / "3yr" by upstream + // recommendation parsers. Pass through as-is; the builder collapses + // it to "1yr"/"3yr" when possible, and sanitizes otherwise. + Term string + + // Payment is the payment option string from the recommendation + // ("all-upfront", "upfront", "no-upfront", "monthly", "partial-upfront"). + // The builder normalizes to a short form ("allup", "noup", "partup", + // "monthly") so the segment stays under 8 chars. + Payment string + + // Now is the timestamp baseline. Tests inject a fixed value for + // determinism; production callers should pass time.Now(). A zero + // time.Time is replaced with time.Now() by the builder so the + // timestamp segment never emits the placeholder "00010101T000000". + Now time.Time + + // randSource is an optional 4-byte source for the random suffix. + // When nil (production), the builder reads from crypto/rand. Tests set + // it via WithRandSource to make output deterministic. + randSource []byte +} + +// WithRandSource returns a copy of f with the given bytes used as the +// random suffix source (test hook). Production code does not call this. +func (f DisplayNameFields) WithRandSource(b []byte) DisplayNameFields { + f.randSource = b + return f +} + +// BuildDisplayName composes a rich, parseable identifier for an Azure +// reservation purchase. The format mirrors the AWS RI CSV's ReservationId +// column shape: +// +// {svc}-{region}-{sku}-{count}x-{term}-{paymt}-{ts}-{rand} +// +// e.g. "vm-eastus-Standard_D2a_v4-1x-1yr-allup-20260522T190000-a1b2c3d4". +// +// The result is always sanitized to [A-Za-z0-9_-] and never longer than +// 64 characters. If the composed string would exceed 64, fields are +// progressively dropped from the tail (random suffix first, then +// timestamp, then payment-option) until it fits. The service code, +// region, SKU, count, and term are NEVER dropped — those are the +// high-signal segments operators rely on to identify the reservation in +// the Azure portal. +func BuildDisplayName(f DisplayNameFields) string { + svc := normalizeSegment(f.Service) + region := normalizeSegment(f.Region) + sku := normalizeSegment(f.ResourceType) + count := fmt.Sprintf("%dx", f.Count) + term := normalizeTerm(f.Term) + paymt := normalizePayment(f.Payment) + // Guard against a zero-value Now (which would emit the nonsensical + // "00010101T000000"). Tests that want determinism pin Now explicitly; + // production callers that forget get a real timestamp, not a placeholder. + now := f.Now + if now.IsZero() { + now = time.Now() + } + ts := now.UTC().Format("20060102T150405") + randHex := generateRandSuffix(f.randSource) + + // Required segments (order matters — never dropped, never reordered). + required := []string{svc, region, sku, count, term} + + // Optional tail segments, in drop priority. The slice order here is + // "keep" order; dropping happens from the right. + tail := []string{paymt, ts, randHex} + + // Try full -> drop random -> drop timestamp -> drop payment. Check the + // PRE-sanitized length so the cap actually gates segment-dropping; calling + // SanitizeDisplayName inside the loop would make the cap vacuously true + // (the sanitizer hard-truncates to 64) and short-circuit the drop logic. + // Each segment is already allowlist-conformant via normalizeSegment, so we + // only need to sanitize once on the exit path as a defensive invariant. + 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) + } + } + + // All optional segments dropped and we still bust the cap — fall back + // to truncating the joined required segments via SanitizeDisplayName. + // This path is reachable only with pathologically long inputs (e.g. + // an impossibly long SKU name) but the builder must never return >64. + return SanitizeDisplayName(joinNonEmpty(required, "-")) +} + +// normalizeSegment strips disallowed characters from a single segment. +// The dash separator is the only allowlist character we reserve for joins, +// so embedded dashes inside a segment are converted to underscores to +// avoid ambiguity at parse-time (consumers split on "-"). +func normalizeSegment(s string) string { + // Replace dashes with underscores first so they don't collide with + // the join separator, then sanitize the rest normally. + s = strings.ReplaceAll(s, "-", "_") + return SanitizeDisplayName(s) +} + +// normalizeTerm maps "1"/"1yr"/"P1Y" -> "1yr" and "3"/"3yr"/"P3Y" -> "3yr". +// Anything else falls back to a sanitized passthrough. +func normalizeTerm(s string) string { + switch strings.ToLower(strings.TrimSpace(s)) { + case "1", "1yr", "1y", "p1y": + return "1yr" + case "3", "3yr", "3y", "p3y": + return "3yr" + default: + return normalizeSegment(s) + } +} + +// normalizePayment maps known payment-option strings to short forms. +// Unknown values are sanitized and truncated to keep the segment ≤6 chars. +func normalizePayment(s string) string { + switch strings.ToLower(strings.TrimSpace(s)) { + case "all-upfront", "allupfront", "upfront": + return "allup" + case "no-upfront", "noupfront": + return "noup" + case "partial-upfront", "partialupfront", "partial": + return "partup" + case "monthly": + return "monthly" + case "": + return "" + default: + out := normalizeSegment(s) + if len(out) > 6 { + out = out[:6] + } + return out + } +} + +// joinNonEmpty joins parts with sep, skipping any empty strings so callers +// don't produce double-separator artifacts ("svc--region") when an optional +// segment is missing. +func joinNonEmpty(parts []string, sep string) string { + out := make([]string, 0, len(parts)) + for _, p := range parts { + if p != "" { + out = append(out, p) + } + } + return strings.Join(out, sep) +} + +// generateRandSuffix returns 8 hex chars from src (test hook) or crypto/rand. +// If src is non-nil but shorter than 4 bytes it is treated as if nil and +// crypto/rand is used as the fallback; callers that want deterministic output +// must pass at least 4 bytes. If randomness can't be obtained (extremely +// unlikely on supported platforms), returns an empty string, which the +// builder treats as a dropped suffix. +func generateRandSuffix(src []byte) string { + if len(src) >= 4 { + return hex.EncodeToString(src[:4]) + } + var b [4]byte + if _, err := rand.Read(b[:]); err != nil { + return "" + } + return hex.EncodeToString(b[:]) +} diff --git a/providers/azure/services/internal/reservations/displayname_test.go b/providers/azure/services/internal/reservations/displayname_test.go new file mode 100644 index 000000000..22b695dc8 --- /dev/null +++ b/providers/azure/services/internal/reservations/displayname_test.go @@ -0,0 +1,509 @@ +package reservations + +import ( + "regexp" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +func TestSanitizeDisplayName(t *testing.T) { + tests := []struct { + name string + input string + want string + }{ + { + name: "already conformant", + input: "VM_Reservation_Standard_D2a_v4", + want: "VM_Reservation_Standard_D2a_v4", + }, + { + name: "spaces replaced by underscore", + input: "VM Reservation Standard D2s v3", + want: "VM_Reservation_Standard_D2s_v3", + }, + { + name: "special chars replaced", + input: "Redis@Cache#Reservation!foo", + want: "Redis_Cache_Reservation_foo", + }, + { + name: "runs of non-conforming chars collapsed to single underscore", + input: "foo bar!!baz", + want: "foo_bar_baz", + }, + { + name: "empty input", + input: "", + want: "", + }, + { + name: "exact 64 chars unchanged", + input: strings.Repeat("a", 64), + want: strings.Repeat("a", 64), + }, + { + name: "65 chars truncated to 64", + input: strings.Repeat("b", 65), + want: strings.Repeat("b", 64), + }, + { + name: "100 chars truncated to 64", + input: strings.Repeat("c", 100), + want: strings.Repeat("c", 64), + }, + { + name: "hyphens preserved", + input: "Standard-D2a-v4", + want: "Standard-D2a-v4", + }, + { + name: "mixed case preserved", + input: "Redis_Cache_Reservation_Standard_D2s_v3", + want: "Redis_Cache_Reservation_Standard_D2s_v3", + }, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + got := SanitizeDisplayName(tc.input) + assert.Equal(t, tc.want, got) + // Output must always match the Azure allowlist. + if got != "" { + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, got) + } + }) + } +} + +// fixedTime is a constant timestamp used across BuildDisplayName tests for +// deterministic comparison. 2026-05-22 19:00:00 UTC. +var fixedTime = time.Date(2026, 5, 22, 19, 0, 0, 0, time.UTC) + +// fixedRand is a 4-byte slice yielding the hex suffix "a1b2c3d4". +var fixedRand = []byte{0xa1, 0xb2, 0xc3, 0xd4} + +func TestBuildDisplayName_HappyPath(t *testing.T) { + got := BuildDisplayName(DisplayNameFields{ + Service: "vm", + Region: "eastus", + ResourceType: "Standard_D2a_v4", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: fixedTime, + }.WithRandSource(fixedRand)) + + want := "vm-eastus-Standard_D2a_v4-1x-1yr-allup-20260522T190000-a1b2c3d4" + assert.Equal(t, want, got) + assert.LessOrEqual(t, len(got), 64) + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, got) +} + +func TestBuildDisplayName_PerServiceExamples(t *testing.T) { + // One realistic example per service — the call sites use these + // service codes. Test catches accidental swaps in the literal strings. + cases := []struct { + svc string + region string + sku string + wantHead string + }{ + {"vm", "eastus", "Standard_D2a_v4", "vm-eastus-Standard_D2a_v4-"}, + {"redis", "westeurope", "Premium_P1", "redis-westeurope-Premium_P1-"}, + {"cosmos", "northeurope", "EnableCassandra", "cosmos-northeurope-EnableCassandra-"}, + {"sql", "centralus", "GP_Gen5_2", "sql-centralus-GP_Gen5_2-"}, + {"search", "westus2", "standard2", "search-westus2-standard2-"}, + } + for _, tc := range cases { + t.Run(tc.svc, func(t *testing.T) { + got := BuildDisplayName(DisplayNameFields{ + Service: tc.svc, + Region: tc.region, + ResourceType: tc.sku, + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: fixedTime, + }.WithRandSource(fixedRand)) + assert.True(t, strings.HasPrefix(got, tc.wantHead), + "want prefix %q, got %q", tc.wantHead, got) + assert.LessOrEqual(t, len(got), 64) + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, got) + }) + } +} + +func TestBuildDisplayName_LengthFitDropsRandomFirst(t *testing.T) { + // Long but realistic input: huge SKU + long region. Full format is + // ~75 chars; builder must drop the random suffix first, then the + // timestamp, keeping payment + the required segments. + got := BuildDisplayName(DisplayNameFields{ + Service: "search", + Region: "australiaeast", + ResourceType: "Standard_NV24ads_A10_v5", + Count: 999, + Term: "1yr", + Payment: "allup", + Now: fixedTime, + }.WithRandSource(fixedRand)) + + assert.LessOrEqual(t, len(got), 64) + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, got) + // All required segments must survive truncation. + for _, must := range []string{"search", "australiaeast", "Standard_NV24ads_A10_v5", "999x", "1yr"} { + assert.Contains(t, got, must, "required segment %q must survive truncation", must) + } + // Random suffix must be the first to go. + assert.NotContains(t, got, "a1b2c3d4", "random suffix should be dropped to fit length cap") +} + +func TestBuildDisplayName_LengthFitDropsTimestampNext(t *testing.T) { + // Push beyond just dropping random — also need to drop timestamp. + // Use a long SKU that pushes the total above 64 even without random. + got := BuildDisplayName(DisplayNameFields{ + Service: "search", + Region: "germanywestcentral", // 18 chars + ResourceType: "Standard_NV24ads_A10_v5", + Count: 999, + Term: "1yr", + Payment: "allup", + Now: fixedTime, + }.WithRandSource(fixedRand)) + + assert.LessOrEqual(t, len(got), 64) + for _, must := range []string{"search", "germanywestcentral", "Standard_NV24ads_A10_v5", "999x", "1yr"} { + assert.Contains(t, got, must) + } + assert.NotContains(t, got, "a1b2c3d4") + assert.NotContains(t, got, "20260522T190000") + // Payment must still survive (drops after timestamp). + assert.Contains(t, got, "allup") +} + +func TestBuildDisplayName_LengthFitDropsPaymentLast(t *testing.T) { + // Push beyond ts+random drops so paymt must also go, but keep total + // short enough that all required segments still survive. + // Sizes: "search"(6) + "germanywestcentral"(18) + SKU(25) + "9999x"(5) + // + "1yr"(3) + 4 separators = 61 -- the longest combo where required + // segments still fit and all optional ones must drop. + got := BuildDisplayName(DisplayNameFields{ + Service: "search", + Region: "germanywestcentral", + ResourceType: strings.Repeat("X", 25), + Count: 9999, + Term: "1yr", + Payment: "all-upfront", + Now: fixedTime, + }.WithRandSource(fixedRand)) + + assert.LessOrEqual(t, len(got), 64) + for _, must := range []string{"search", "germanywestcentral", strings.Repeat("X", 25), "9999x", "1yr"} { + assert.Contains(t, got, must) + } + // All optional segments dropped. + assert.NotContains(t, got, "a1b2c3d4") + assert.NotContains(t, got, "20260522T190000") + assert.NotContains(t, got, "allup") +} + +func TestBuildDisplayName_LengthFitTruncatesRequiredAsLastResort(t *testing.T) { + // Even the required segments alone exceed 64. Builder must still + // produce a ≤64-char allowlist-conformant string rather than panicking + // or returning a too-long value. + got := BuildDisplayName(DisplayNameFields{ + Service: "search", + Region: "germanywestcentral", + ResourceType: strings.Repeat("X", 80), + Count: 9999, + Term: "1yr", + Payment: "all-upfront", + Now: fixedTime, + }.WithRandSource(fixedRand)) + + assert.LessOrEqual(t, len(got), 64) + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, got) +} + +func TestBuildDisplayName_PaymentNormalization(t *testing.T) { + cases := []struct { + in string + want string + }{ + {"all-upfront", "allup"}, + {"All-Upfront", "allup"}, + {"upfront", "allup"}, + {"no-upfront", "noup"}, + {"No-Upfront", "noup"}, + {"partial-upfront", "partup"}, + {"Partial-Upfront", "partup"}, + {"monthly", "monthly"}, + {"Monthly", "monthly"}, + {"", ""}, + } + for _, tc := range cases { + t.Run(tc.in, func(t *testing.T) { + assert.Equal(t, tc.want, normalizePayment(tc.in)) + }) + } +} + +func TestBuildDisplayName_PaymentNormalizationVisibleInOutput(t *testing.T) { + cases := []struct { + paymt string + wantSeg string + }{ + {"all-upfront", "allup"}, + {"no-upfront", "noup"}, + {"partial-upfront", "partup"}, + {"monthly", "monthly"}, + } + for _, tc := range cases { + t.Run(tc.paymt, func(t *testing.T) { + got := BuildDisplayName(DisplayNameFields{ + Service: "vm", + Region: "eastus", + ResourceType: "Standard_D2a_v4", + Count: 1, + Term: "1yr", + Payment: tc.paymt, + Now: fixedTime, + }.WithRandSource(fixedRand)) + // Payment segment is bracketed by dashes in the output. + assert.Contains(t, got, "-"+tc.wantSeg+"-", + "payment %q should normalize to segment %q in %q", tc.paymt, tc.wantSeg, got) + }) + } +} + +func TestBuildDisplayName_TermNormalization(t *testing.T) { + cases := []struct { + in string + want string + }{ + {"1yr", "1yr"}, + {"1", "1yr"}, + {"1y", "1yr"}, + {"P1Y", "1yr"}, + {"3yr", "3yr"}, + {"3", "3yr"}, + {"P3Y", "3yr"}, + {"", ""}, + } + for _, tc := range cases { + t.Run(tc.in, func(t *testing.T) { + assert.Equal(t, tc.want, normalizeTerm(tc.in)) + }) + } +} + +func TestBuildDisplayName_SanitizesDirtyInput(t *testing.T) { + // Unexpected chars in any field must be sanitized to underscores. + got := BuildDisplayName(DisplayNameFields{ + Service: "v m", // space + Region: "east/us", + ResourceType: "Standard@D2a v4", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: fixedTime, + }.WithRandSource(fixedRand)) + + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, got) + // Should not contain spaces, slashes, or @. + assert.NotContains(t, got, " ") + assert.NotContains(t, got, "/") + assert.NotContains(t, got, "@") +} + +func TestBuildDisplayName_EmbeddedDashInSegmentBecomesUnderscore(t *testing.T) { + // A SKU containing a dash would create ambiguity with the join + // separator. The builder collapses internal dashes to underscores. + got := BuildDisplayName(DisplayNameFields{ + Service: "vm", + Region: "eastus", + ResourceType: "Standard-D2a-v4", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: fixedTime, + }.WithRandSource(fixedRand)) + + // The SKU's dashes should be underscores in the output. + assert.Contains(t, got, "Standard_D2a_v4") + // And the segment boundaries remain dashes. + assert.True(t, strings.HasPrefix(got, "vm-eastus-Standard_D2a_v4-")) +} + +func TestBuildDisplayName_Deterministic(t *testing.T) { + // Same fields + same Now + same randSource -> identical output. + f := DisplayNameFields{ + Service: "vm", + Region: "eastus", + ResourceType: "Standard_D2a_v4", + Count: 2, + Term: "1yr", + Payment: "all-upfront", + Now: fixedTime, + }.WithRandSource(fixedRand) + first := BuildDisplayName(f) + second := BuildDisplayName(f) + assert.Equal(t, first, second) +} + +func TestBuildDisplayName_DifferentRandsProduceDifferentOutputs(t *testing.T) { + base := DisplayNameFields{ + Service: "vm", + Region: "eastus", + ResourceType: "Standard_D2a_v4", + Count: 2, + Term: "1yr", + Payment: "all-upfront", + Now: fixedTime, + } + a := BuildDisplayName(base.WithRandSource([]byte{0x01, 0x02, 0x03, 0x04})) + b := BuildDisplayName(base.WithRandSource([]byte{0xff, 0xee, 0xdd, 0xcc})) + assert.NotEqual(t, a, b) + assert.Contains(t, a, "01020304") + assert.Contains(t, b, "ffeeddcc") +} + +func TestBuildDisplayName_ProductionUsesCryptoRand(t *testing.T) { + // No randSource set: builder reads from crypto/rand. The two calls + // should differ in their 8-hex suffix with extremely high probability + // (2^-32 collision), and both should still conform to the allowlist. + base := DisplayNameFields{ + Service: "vm", + Region: "eastus", + ResourceType: "Standard_D2a_v4", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: fixedTime, + } + a := BuildDisplayName(base) + b := BuildDisplayName(base) + assert.NotEqual(t, a, b) + allowlist := regexp.MustCompile(`^[A-Za-z0-9_-]{1,64}$`) + require.Regexp(t, allowlist, a) + require.Regexp(t, allowlist, b) +} + +func TestBuildDisplayName_EmptyPaymentSegmentIsSkipped(t *testing.T) { + got := BuildDisplayName(DisplayNameFields{ + Service: "vm", + Region: "eastus", + ResourceType: "Standard_D2a_v4", + Count: 1, + Term: "1yr", + Payment: "", // no payment info + Now: fixedTime, + }.WithRandSource(fixedRand)) + + // Must not contain double-dash from the missing payment segment. + assert.NotContains(t, got, "--") + // Order is preserved: 1yr is directly followed by the timestamp. + assert.Contains(t, got, "-1yr-20260522T190000-a1b2c3d4") +} + +// TestBuildDisplayName_DropLoopActuallyDropsTimestamp is a regression guard +// against a previously-broken progressive-drop loop. The old implementation +// called SanitizeDisplayName(candidate) INSIDE the loop before the length +// check, which short-circuited the cap (the sanitizer hard-truncates to 64) +// and returned a mid-string-truncated full format instead of cleanly dropping +// the random suffix and timestamp. The fixed loop must produce a string that +// ends exactly at the payment segment with no partial timestamp bytes. +// +// Input: full format = 83 chars, requires dropping both the 8-char random +// suffix AND the 15-char timestamp; the result must end with "-allup" and +// contain NO trace of the timestamp digits. +func TestBuildDisplayName_DropLoopActuallyDropsTimestamp(t *testing.T) { + got := BuildDisplayName(DisplayNameFields{ + Service: "vm", + Region: "germanywestcentral", // 18 chars + ResourceType: "Standard_NV24ads_A10_v5", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: fixedTime, + }.WithRandSource(fixedRand)) + + // With the fixed drop loop, both optional ts and random are dropped + // cleanly. The broken loop returned a 64-char mid-truncated value that + // happened to include "20260" (start of the timestamp). + want := "vm-germanywestcentral-Standard_NV24ads_A10_v5-1x-1yr-allup" + assert.Equal(t, want, got) + assert.LessOrEqual(t, len(got), 64) + // Defensive: any partial timestamp prefix would surface as digits after + // "allup-"; the fixed builder must not emit any of them. + assert.NotContains(t, got, "allup-20") + assert.NotContains(t, got, "20260") +} + +// TestBuildDisplayName_DropLoopActuallyDropsPayment is a second regression +// guard for the drop loop, this time forcing the loop to drop the payment +// segment too (keep=0, required-only). The broken loop again would short- +// circuit at iteration 1 and mid-truncate, leaving partial payment bytes. +func TestBuildDisplayName_DropLoopActuallyDropsPayment(t *testing.T) { + got := BuildDisplayName(DisplayNameFields{ + Service: "vm", + Region: "germanywestcentral", + ResourceType: strings.Repeat("X", 30), // forces required-only fallback + Count: 9999, + Term: "1yr", + Payment: "all-upfront", + Now: fixedTime, + }.WithRandSource(fixedRand)) + + // With the fixed drop loop, all optional segments drop cleanly and the + // result is exactly the required segments joined by dashes. + want := "vm-germanywestcentral-" + strings.Repeat("X", 30) + "-9999x-1yr" + assert.Equal(t, want, got) + assert.LessOrEqual(t, len(got), 64) + // No payment fragment anywhere: the broken loop left "...1yr-a" at the + // 64-char boundary. + assert.NotContains(t, got, "allup") + assert.NotContains(t, got, "1yr-a") +} + +// TestBuildDisplayName_ZeroNowReplacedByWallClock guards the zero-time guard: +// a zero-value Now must not emit the placeholder "00010101T000000" segment. +// The builder substitutes time.Now() so the timestamp is always meaningful. +func TestBuildDisplayName_ZeroNowReplacedByWallClock(t *testing.T) { + got := BuildDisplayName(DisplayNameFields{ + Service: "vm", + Region: "eastus", + ResourceType: "Standard_D2a_v4", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + // Now intentionally left as zero value. + }.WithRandSource(fixedRand)) + + assert.NotContains(t, got, "00010101T000000", + "zero-value Now must be replaced by wall-clock time, not emitted as placeholder") + // Sanity: still allowlist-conformant and within length cap. + assert.LessOrEqual(t, len(got), 64) + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, got) +} + +func TestBuildDisplayName_TimestampUTC(t *testing.T) { + // Even if the caller passes a local-zone time, the builder must + // normalize to UTC so identifiers are comparable across hosts. + loc, err := time.LoadLocation("America/Los_Angeles") + require.NoError(t, err) + local := time.Date(2026, 5, 22, 12, 0, 0, 0, loc) // 19:00 UTC + got := BuildDisplayName(DisplayNameFields{ + Service: "vm", + Region: "eastus", + ResourceType: "Standard_D2a_v4", + Count: 1, + Term: "1yr", + Payment: "all-upfront", + Now: local, + }.WithRandSource(fixedRand)) + assert.Contains(t, got, "20260522T190000") +} diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index 7573577be..fa0dd1994 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -272,9 +272,17 @@ func (c *SearchClient) PurchaseCommitment(ctx context.Context, rec common.Recomm "billingScopeId": fmt.Sprintf("/subscriptions/%s", c.subscriptionID), "term": fmt.Sprintf("P%dY", termYears), "quantity": rec.Count, - "displayName": fmt.Sprintf("Search Service Reservation - %s", rec.ResourceType), - "appliedScopeType": "Shared", - "renew": false, + "displayName": reservations.BuildDisplayName(reservations.DisplayNameFields{ + Service: "search", + Region: c.region, + ResourceType: rec.ResourceType, + Count: rec.Count, + Term: rec.Term, + Payment: rec.PaymentOption, + Now: time.Now(), + }), + "appliedScopeType": "Shared", + "renew": false, }, } applyPurchaseAutomationTag(requestBody, opts.Source) diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index bfee43ce7..5f4be4920 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -955,3 +955,54 @@ func TestSearchClient_ValidateOffering_CaseInsensitive(t *testing.T) { assert.NoError(t, err) }) } + +// TestSearchClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist guards +// against regression: displayName in the calculatePrice body must match +// [A-Za-z0-9_-]{1,64} (Azure rejects DisplayNameInvalid otherwise). +func TestSearchClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t *testing.T) { + ctx := context.Background() + mockHTTP := &MockHTTPClient{} + mockCred := &MockTokenCredential{token: "test-token"} + client := NewClientWithHTTP(mockCred, "test-subscription", "eastus", mockHTTP) + + const orderID = "azure-search-displayname" + var capturedDisplayName string + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + if r.Method != http.MethodPost || r.URL.Path != "/providers/Microsoft.Capacity/calculatePrice" { + return false + } + if r.Body == nil { + return true + } + bodyBytes, _ := io.ReadAll(r.Body) + r.Body = io.NopCloser(bytes.NewReader(bodyBytes)) + var body map[string]interface{} + if err := json.Unmarshal(bodyBytes, &body); err == nil { + if props, ok := body["properties"].(map[string]interface{}); ok { + if dn, ok := props["displayName"].(string); ok { + capturedDisplayName = dn + } + } + } + return true + })).Return(createMockHTTPResponse(http.StatusOK, calcPriceRespJSON(orderID)), nil).Once() + mockHTTP.On("Do", mock.MatchedBy(func(r *http.Request) bool { + return r.Method == http.MethodPost && + r.URL.Path == "/providers/Microsoft.Capacity/reservationOrders/"+orderID+"/purchase" + })).Return(createMockHTTPResponse(http.StatusOK, `{}`), nil).Once() + + rec := common.Recommendation{ + ResourceType: "standard2", + Term: "1yr", + Count: 1, + CommitmentCost: 800.0, + } + _, err := client.PurchaseCommitment(ctx, rec, common.PurchaseOptions{Source: common.PurchaseSourceCLI}) + require.NoError(t, err) + assert.NotEmpty(t, capturedDisplayName) + assert.Regexp(t, `^[A-Za-z0-9_-]{1,64}$`, capturedDisplayName) + // Rich-format guards: service code is correct and SKU is preserved + // (see providers/azure/services/internal/reservations/displayname.go). + assert.Regexp(t, `^search-`, capturedDisplayName) + assert.Contains(t, capturedDisplayName, "standard2") +}