From 37ed8fbb3670a4431435a7564f948d54641434db Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 21:39:34 +0200 Subject: [PATCH 1/4] fix(azure/reservations): add SanitizeDisplayName helper for Azure DisplayName allowlist Azure's Reservations API rejects DisplayName values that contain characters outside [A-Za-z0-9_-] with HTTP 400 DisplayNameInvalid. Add a shared helper in services/internal/reservations that replaces non-conforming characters with '_', collapses consecutive non-conforming chars into a single '_', and truncates to 64 characters. --- .../internal/reservations/displayname.go | 35 +++++++++ .../internal/reservations/displayname_test.go | 77 +++++++++++++++++++ 2 files changed, 112 insertions(+) create mode 100644 providers/azure/services/internal/reservations/displayname.go create mode 100644 providers/azure/services/internal/reservations/displayname_test.go diff --git a/providers/azure/services/internal/reservations/displayname.go b/providers/azure/services/internal/reservations/displayname.go new file mode 100644 index 000000000..145b42bff --- /dev/null +++ b/providers/azure/services/internal/reservations/displayname.go @@ -0,0 +1,35 @@ +// Package reservations provides shared helpers for Azure Reservations API operations. +package reservations + +import "strings" + +// isAllowedDisplayNameChar reports whether r is in Azure's DisplayName allowlist +// ([A-Za-z0-9-]). Underscores are emitted by the sanitizer, not passed through 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) > 64 { + // All output chars are ASCII so byte index == rune index. + result = result[:64] + } + return result +} 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..770acee67 --- /dev/null +++ b/providers/azure/services/internal/reservations/displayname_test.go @@ -0,0 +1,77 @@ +package reservations + +import ( + "strings" + "testing" + + "github.com/stretchr/testify/assert" +) + +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) + } + }) + } +} From 35bac3cb18d3a366ab519ed1a0220c9dc9d824f0 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 21:40:09 +0200 Subject: [PATCH 2/4] fix(azure): use SanitizeDisplayName + underscore-form displayName across 5 services (closes #685) Replace space-containing displayName format strings with underscore-form prefixes and pipe through SanitizeDisplayName in all 5 Azure service clients (cache, compute, cosmosdb, database, search). Fixes HTTP 400 DisplayNameInvalid errors from Azure's Reservations API when SKU names contain unusual characters. Extends each service's PurchaseCommitment test with a body-capture assertion that guards the [A-Za-z0-9_-]{1,64} allowlist going forward. The capture mocks the calculatePrice POST so it works correctly with the two-step calculatePrice->purchase flow established by #680. --- providers/azure/services/cache/client.go | 2 +- providers/azure/services/cache/client_test.go | 47 ++++++++++++++++++ providers/azure/services/compute/client.go | 2 +- .../azure/services/compute/client_test.go | 49 +++++++++++++++++++ providers/azure/services/cosmosdb/client.go | 2 +- .../azure/services/cosmosdb/client_test.go | 47 ++++++++++++++++++ providers/azure/services/database/client.go | 2 +- .../azure/services/database/client_test.go | 47 ++++++++++++++++++ providers/azure/services/search/client.go | 2 +- .../azure/services/search/client_test.go | 47 ++++++++++++++++++ 10 files changed, 242 insertions(+), 5 deletions(-) diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index e7fd1921f..70f6e5286 100644 --- a/providers/azure/services/cache/client.go +++ b/providers/azure/services/cache/client.go @@ -299,7 +299,7 @@ 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), + "displayName": reservations.SanitizeDisplayName(fmt.Sprintf("Redis_Cache_Reservation_%s", rec.ResourceType)), "appliedScopeType": "Shared", "renew": false, }, diff --git a/providers/azure/services/cache/client_test.go b/providers/azure/services/cache/client_test.go index 930f39885..39cb0cb16 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -1145,3 +1145,50 @@ 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) +} diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index e016e963f..f5a8c4674 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -378,7 +378,7 @@ 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), + "displayName": reservations.SanitizeDisplayName(fmt.Sprintf("VM_Reservation_%s", rec.ResourceType)), "appliedScopeType": "Shared", "renew": false, }, diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index fb9b2076b..0650c5dd1 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -1064,3 +1064,52 @@ 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) +} diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index b77ad88f7..e43134cbb 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -292,7 +292,7 @@ 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), + "displayName": reservations.SanitizeDisplayName(fmt.Sprintf("Cosmos_DB_Reservation_%s", rec.ResourceType)), "appliedScopeType": "Shared", "renew": false, }, diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index 7dd73df61..f31091330 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -1214,3 +1214,50 @@ 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) +} diff --git a/providers/azure/services/database/client.go b/providers/azure/services/database/client.go index f0238c00c..80a28460e 100644 --- a/providers/azure/services/database/client.go +++ b/providers/azure/services/database/client.go @@ -300,7 +300,7 @@ 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), + "displayName": reservations.SanitizeDisplayName(fmt.Sprintf("SQL_DB_Reservation_%s", rec.ResourceType)), "appliedScopeType": "Shared", "renew": false, }, diff --git a/providers/azure/services/database/client_test.go b/providers/azure/services/database/client_test.go index c26628466..16c8c54c2 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -1141,3 +1141,50 @@ 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) +} diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index 7573577be..e853db703 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -272,7 +272,7 @@ 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), + "displayName": reservations.SanitizeDisplayName(fmt.Sprintf("Search_Service_Reservation_%s", rec.ResourceType)), "appliedScopeType": "Shared", "renew": false, }, diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index bfee43ce7..227f99f27 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -955,3 +955,50 @@ 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) +} From 8add0cb3b81b2dbd7ef18ef18851aacfce494095 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 22:32:49 +0200 Subject: [PATCH 3/4] feat(azure/reservations): rich self-describing displayName for purchases MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Extend the Azure DisplayName fix from #686 with a richer, parseable 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 '-'): {svc}-{region}-{sku}-{count}x-{term}-{paymt}-{ts}-{rand} vm -eastus -Standard_D2a_v4-1x-1yr-allup -20260522T190000-a1b2c3d4 redis -westeurope-Premium_P1 -1x-1yr-allup -20260522T190000-a1b2c3d4 cosmos -northeurope-EnableCassandra-1x-1yr-allup-20260522T190000-a1b2c3d4 sql -centralus -GP_Gen5_2 -1x-1yr-allup -20260522T190000-a1b2c3d4 search -westus2 -standard2 -1x-1yr-allup -20260522T190000-a1b2c3d4 Output is always sanitized to Azure's [A-Za-z0-9_-]{1,64} allowlist. When the composed string would exceed 64 chars (long region + long SKU + high count), the builder progressively drops the optional tail segments — random suffix first, then timestamp, then payment-option. The service code, region, SKU, count, and term are NEVER dropped: those are the high-signal segments operators rely on. With every optional segment dropped, the joined required segments are truncated as a last-resort safety net so the builder never emits >64 chars. Previous format was `VM_Reservation_` (and equivalents per service) — operators reported these were indistinguishable across purchases. The new identifier carries enough context to identify the specific reservation purchase from the portal alone. Implementation lives in providers/azure/services/internal/reservations/displayname.go alongside the existing SanitizeDisplayName helper, and is wired into all 5 service clients (cache, compute, cosmosdb, database, search). Time and random sources are injectable so tests can pin them; tests cover happy-path, per-service shape, length-fit truncation at each drop priority (random → timestamp → payment), payment normalization, sanitization of dirty input, embedded-dash collapse, determinism, and UTC normalization. The 5 per-service allowlist regression tests are extended to also assert the service-code prefix and SKU presence so accidental swaps in the literal strings are caught. Refs #685 --- providers/azure/services/cache/client.go | 14 +- providers/azure/services/cache/client_test.go | 4 + providers/azure/services/compute/client.go | 14 +- .../azure/services/compute/client_test.go | 4 + providers/azure/services/cosmosdb/client.go | 14 +- .../azure/services/cosmosdb/client_test.go | 4 + providers/azure/services/database/client.go | 14 +- .../azure/services/database/client_test.go | 4 + .../internal/reservations/displayname.go | 191 +++++++++- .../internal/reservations/displayname_test.go | 351 ++++++++++++++++++ providers/azure/services/search/client.go | 14 +- .../azure/services/search/client_test.go | 4 + 12 files changed, 614 insertions(+), 18 deletions(-) diff --git a/providers/azure/services/cache/client.go b/providers/azure/services/cache/client.go index 70f6e5286..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": reservations.SanitizeDisplayName(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 39cb0cb16..3380d6083 100644 --- a/providers/azure/services/cache/client_test.go +++ b/providers/azure/services/cache/client_test.go @@ -1191,4 +1191,8 @@ func TestCacheClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t *t 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 f5a8c4674..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": reservations.SanitizeDisplayName(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 0650c5dd1..5daf697b9 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -1112,4 +1112,8 @@ func TestComputeClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t 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 e43134cbb..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": reservations.SanitizeDisplayName(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 f31091330..b4e596665 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -1260,4 +1260,8 @@ func TestCosmosDBClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t 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 80a28460e..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": reservations.SanitizeDisplayName(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 16c8c54c2..1e6d3ef21 100644 --- a/providers/azure/services/database/client_test.go +++ b/providers/azure/services/database/client_test.go @@ -1187,4 +1187,8 @@ func TestDatabaseClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t 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 index 145b42bff..bdcd348a8 100644 --- a/providers/azure/services/internal/reservations/displayname.go +++ b/providers/azure/services/internal/reservations/displayname.go @@ -1,7 +1,17 @@ // Package reservations provides shared helpers for Azure Reservations API operations. package reservations -import "strings" +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 DisplayName allowlist // ([A-Za-z0-9-]). Underscores are emitted by the sanitizer, not passed through here. @@ -27,9 +37,184 @@ func SanitizeDisplayName(s string) string { } } result := b.String() - if len(result) > 64 { + if len(result) > azureDisplayNameMaxLen { // All output chars are ASCII so byte index == rune index. - result = result[:64] + 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. Must be non-zero for production calls. + // Tests inject a fixed value for determinism. + 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) + ts := f.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. + for keep := len(tail); keep >= 0; keep-- { + segments := append([]string{}, required...) + segments = append(segments, tail[:keep]...) + candidate := joinNonEmpty(segments, "-") + // Defensive sanitization: even though each segment is already + // allowlist-conformant, SanitizeDisplayName guarantees the + // invariant in one place rather than relying on every caller. + candidate = SanitizeDisplayName(candidate) + if len(candidate) <= azureDisplayNameMaxLen { + return 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 randomness can't be obtained (extremely unlikely on supported platforms), +// returns an empty string — the builder treats that 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 index 770acee67..14a11d6c2 100644 --- a/providers/azure/services/internal/reservations/displayname_test.go +++ b/providers/azure/services/internal/reservations/displayname_test.go @@ -1,10 +1,13 @@ package reservations import ( + "regexp" "strings" "testing" + "time" "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" ) func TestSanitizeDisplayName(t *testing.T) { @@ -75,3 +78,351 @@ func TestSanitizeDisplayName(t *testing.T) { }) } } + +// 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") +} + +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 e853db703..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": reservations.SanitizeDisplayName(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 227f99f27..5f4be4920 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -1001,4 +1001,8 @@ func TestSearchClient_PurchaseCommitment_DisplayNameConformsToAzureAllowlist(t * 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") } From 1e3fabebfe9abce862c4da465ee00a683e69bcbd Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Fri, 22 May 2026 23:01:37 +0200 Subject: [PATCH 4/4] 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. --- .../internal/reservations/displayname.go | 41 +++++++--- .../internal/reservations/displayname_test.go | 81 +++++++++++++++++++ 2 files changed, 109 insertions(+), 13 deletions(-) diff --git a/providers/azure/services/internal/reservations/displayname.go b/providers/azure/services/internal/reservations/displayname.go index bdcd348a8..97b55d881 100644 --- a/providers/azure/services/internal/reservations/displayname.go +++ b/providers/azure/services/internal/reservations/displayname.go @@ -13,8 +13,10 @@ import ( // Azure rejects longer values with HTTP 400 DisplayNameInvalid. const azureDisplayNameMaxLen = 64 -// isAllowedDisplayNameChar reports whether r is in Azure's DisplayName allowlist -// ([A-Za-z0-9-]). Underscores are emitted by the sanitizer, not passed through here. +// 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 == '-' } @@ -75,8 +77,10 @@ type DisplayNameFields struct { // "monthly") so the segment stays under 8 chars. Payment string - // Now is the timestamp baseline. Must be non-zero for production calls. - // Tests inject a fixed value for determinism. + // 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. @@ -114,7 +118,14 @@ func BuildDisplayName(f DisplayNameFields) string { count := fmt.Sprintf("%dx", f.Count) term := normalizeTerm(f.Term) paymt := normalizePayment(f.Payment) - ts := f.Now.UTC().Format("20060102T150405") + // 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). @@ -124,17 +135,18 @@ func BuildDisplayName(f DisplayNameFields) string { // "keep" order; dropping happens from the right. tail := []string{paymt, ts, randHex} - // Try full -> drop random -> drop timestamp -> drop payment. + // 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, "-") - // Defensive sanitization: even though each segment is already - // allowlist-conformant, SanitizeDisplayName guarantees the - // invariant in one place rather than relying on every caller. - candidate = SanitizeDisplayName(candidate) if len(candidate) <= azureDisplayNameMaxLen { - return candidate + return SanitizeDisplayName(candidate) } } @@ -206,8 +218,11 @@ func joinNonEmpty(parts []string, sep string) string { } // generateRandSuffix returns 8 hex chars from src (test hook) or crypto/rand. -// If randomness can't be obtained (extremely unlikely on supported platforms), -// returns an empty string — the builder treats that as a dropped suffix. +// 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]) diff --git a/providers/azure/services/internal/reservations/displayname_test.go b/providers/azure/services/internal/reservations/displayname_test.go index 14a11d6c2..22b695dc8 100644 --- a/providers/azure/services/internal/reservations/displayname_test.go +++ b/providers/azure/services/internal/reservations/displayname_test.go @@ -409,6 +409,87 @@ func TestBuildDisplayName_EmptyPaymentSegmentIsSkipped(t *testing.T) { 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.