From cd80e338b7ca1b06b144cd96a1f04f7a17e4f706 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 8 Aug 2026 09:30:39 +0200 Subject: [PATCH] fix(azure): pair the recommended SKU with the count in its own units Azure's consumption API carries two quantities on a reservation recommendation and they are not interchangeable: RecommendedQuantity counts the actual SKU (SKUProperties[SKUName]), RecommendedQuantityNormalized counts NormalizedSize. The resource type and the count were resolved independently, so the Legacy/EA ladder took its SKU from NormalizedSize while the count came from RecommendedQuantity. Azure recommending 4 x Standard_D8s_v3, normalized to 16 x Standard_D2s_v3, was converted to Standard_D2s_v3 x4 -- a quarter of the recommended capacity. Verified end to end: it reaches the wire as sku.name=Standard_D2s_v3 with quantity=4 on both calculatePrice and purchase, which Azure accepts because both values are individually valid. The recommendation's savings figure is Azure's number for the FULL quantity and rode along unchanged, so $400/mo was reported against a purchase delivering roughly a quarter of it. Under-buy, not over-buy: it fails safe on commitment risk -- nobody is locked into capacity they cannot use -- but unsafe on reported savings, because the number shown to whoever approves the purchase is wrong. A decision-quality defect rather than a spend defect. The two are now resolved together so no branch can emit a mixed pair. The actual SKU is preferred, matching the Modern ladder, so EA and MCA subscriptions with identical usage yield the same resource type; its partner RecommendedQuantity is also the *float64 field, avoiding a *float32 rounding question on a purchase path. When only NormalizedSize is available its normalized count is the only valid partner, and if that is absent the count is left unset rather than borrowed -- an unpaired count is the defect itself, and zero is refused by the Count <= 0 guard every service applies before purchasing. Modern was not the reported instance and its first rung is unchanged, but with SKUName absent its NormalizedSize rung mixed the same two fields. Paired here rather than left as the one branch that can still emit mismatched units. The fixture builders now set both quantities, because a fixture naming a NormalizedSize while setting only RecommendedQuantity describes a payload with no valid pairing. Refs #1540 --- .../internal/recommendations/converter.go | 103 ++++++++++--- .../recommendations/converter_test.go | 144 ++++++++++++++++-- .../azure/mocks/recommendation_fixtures.go | 76 +++++++-- .../azure/services/compute/client_test.go | 86 +++++++++++ 4 files changed, 363 insertions(+), 46 deletions(-) diff --git a/providers/azure/internal/recommendations/converter.go b/providers/azure/internal/recommendations/converter.go index 962fdfe4c..3071bf529 100644 --- a/providers/azure/internal/recommendations/converter.go +++ b/providers/azure/internal/recommendations/converter.go @@ -141,15 +141,16 @@ func extractLegacy(rec *armconsumption.LegacyReservationRecommendation) *Extract return nil } + resourceType, quantity := resolveLegacySKUAndQuantity(props) out := &ExtractedFields{ Region: strDeref(rec.Location), - ResourceType: resolveLegacyResourceType(props), + ResourceType: resourceType, Term: normaliseTerm(props.Term), Scope: strDeref(props.Scope), } - if props.RecommendedQuantity != nil { - out.Count = int(*props.RecommendedQuantity) + if quantity != nil { + out.Count = int(*quantity) } if props.CostWithNoReservedInstances != nil { out.OnDemandCost = *props.CostWithNoReservedInstances @@ -194,15 +195,16 @@ func extractModern(rec *armconsumption.ModernReservationRecommendation) *Extract region = strDeref(props.Location) } + resourceType, quantity := resolveModernSKUAndQuantity(props) out := &ExtractedFields{ Region: region, - ResourceType: resolveModernResourceType(props), + ResourceType: resourceType, Term: normaliseTerm(props.Term), Scope: strDeref(props.Scope), } - if props.RecommendedQuantity != nil { - out.Count = int(*props.RecommendedQuantity) + if quantity != nil { + out.Count = int(*quantity) } out.OnDemandCost = amountValue(props.CostWithNoReservedInstances) out.CommitmentCost = amountValue(props.TotalCostWithReservedInstances) @@ -222,29 +224,86 @@ func extractModern(rec *armconsumption.ModernReservationRecommendation) *Extract return out } -// resolveLegacyResourceType follows the Legacy field ladder: -// NormalizedSize → SKUProperties[Name==SKUName|skuName].Value → first -// non-empty SKUProperty.Value. -func resolveLegacyResourceType(props *armconsumption.LegacyReservationRecommendationProperties) string { - if s := strDeref(props.NormalizedSize); s != "" { - return s +// resolveLegacySKUAndQuantity returns the resource type together with the +// quantity that counts THAT resource type, so the two are always in the same +// units. +// +// Azure carries two quantities on a Legacy recommendation and they are not +// interchangeable: RecommendedQuantity counts the actual SKU +// (SKUProperties[SKUName]), while RecommendedQuantityNormalized counts +// NormalizedSize. Resolving the two independently let the SKU come from one +// and the count from the other, understating the purchase by the family's +// instance-size-flexibility ratio -- 4 x Standard_D8s_v3 normalized to +// 16 x Standard_D2s_v3 was bought as 4 x Standard_D2s_v3, a quarter of the +// recommended capacity, with the full recommendation's savings still shown +// against it (issue #1540). They are resolved together here so no branch can +// produce a mixed pair. +// +// The actual SKU is preferred, matching resolveModernSKUAndQuantity, so EA and +// MCA subscriptions with identical usage yield the same ResourceType. Its +// partner RecommendedQuantity is also the *float64 field, avoiding the +// *float32 rounding question on a purchase path. +// +// When only NormalizedSize is available the normalized quantity is its only +// valid partner. If that partner is absent the count is left unset rather than +// borrowed from RecommendedQuantity: an unpaired count is precisely the defect +// above, and a zero count is refused downstream by the `Count <= 0` guard every +// service applies before purchasing. +func resolveLegacySKUAndQuantity(props *armconsumption.LegacyReservationRecommendationProperties) (resourceType string, quantity *float64) { + if s := resourceTypeFromSKUProperties(props.SKUProperties); s != "" { + return s, props.RecommendedQuantity } - return resourceTypeFromSKUProperties(props.SKUProperties) + if normalized := strDeref(props.NormalizedSize); normalized != "" { + if props.RecommendedQuantityNormalized == nil { + logging.Warnf( + "azure recommendations: %q has a normalized size but no RecommendedQuantityNormalized "+ + "and no SKU properties to pair RecommendedQuantity with; leaving the count unset "+ + "rather than pairing mismatched units", + normalized, + ) + return normalized, nil + } + return normalized, float64Ptr(float64(*props.RecommendedQuantityNormalized)) + } + // No resource type from either source: there is no normalized size for the + // count to disagree with, so this degenerate payload keeps the behavior it + // had before the pairing fix. + return "", props.RecommendedQuantity } -// resolveModernResourceType follows the Modern field ladder. Modern adds -// a top-level SKUName pointer (the cleanest source), so the preference is -// SKUName → NormalizedSize → SKUProperties fallback. The SKUProperties -// fallback matches Legacy's contract so a switch between billing-account -// types doesn't change ResourceType semantics. -func resolveModernResourceType(props *armconsumption.ModernReservationRecommendationProperties) string { +// resolveModernSKUAndQuantity is the Modern counterpart of +// resolveLegacySKUAndQuantity, pairing each rung of the ladder with the +// quantity expressed in that rung's units. +// +// The ladder is unchanged: Modern adds a top-level SKUName pointer (the +// cleanest source), so the preference stays SKUName → NormalizedSize → +// SKUProperties fallback, and the SKUProperties rung still matches Legacy's +// contract so a switch between billing-account types does not change +// ResourceType semantics. +// +// Both SKU rungs count the actual SKU and so pair with RecommendedQuantity; +// only the NormalizedSize rung is counted by RecommendedQuantityNormalized. +// Modern was not the reported instance of #1540 -- real MCA payloads carry +// SKUName and take the first rung -- but its NormalizedSize rung mixed the +// same two fields, so it is paired here rather than left as the one branch +// that can still emit mismatched units. +func resolveModernSKUAndQuantity(props *armconsumption.ModernReservationRecommendationProperties) (resourceType string, quantity *float64) { if s := strDeref(props.SKUName); s != "" { - return s + return s, props.RecommendedQuantity } if s := strDeref(props.NormalizedSize); s != "" { - return s + if props.RecommendedQuantityNormalized == nil { + logging.Warnf( + "azure recommendations: modern recommendation %q has a normalized size but no "+ + "RecommendedQuantityNormalized; leaving the count unset rather than pairing "+ + "mismatched units", + s, + ) + return s, nil + } + return s, float64Ptr(float64(*props.RecommendedQuantityNormalized)) } - return resourceTypeFromSKUProperties(props.SKUProperties) + return resourceTypeFromSKUProperties(props.SKUProperties), props.RecommendedQuantity } // resourceTypeFromSKUProperties scans a SKUProperties key/value list for diff --git a/providers/azure/internal/recommendations/converter_test.go b/providers/azure/internal/recommendations/converter_test.go index 84d34d442..938532734 100644 --- a/providers/azure/internal/recommendations/converter_test.go +++ b/providers/azure/internal/recommendations/converter_test.go @@ -96,20 +96,21 @@ func TestExtract_QuantityRoundsDown(t *testing.T) { } } -func TestExtract_ResourceTypePrefersNormalizedSize(t *testing.T) { - // When both NormalizedSize and SKUProperties are populated, NormalizedSize wins. +func TestExtract_ResourceTypePrefersActualSKUOverNormalizedSize(t *testing.T) { + // When both are populated the actual SKU wins, because RecommendedQuantity + // counts THAT SKU. Preferring NormalizedSize here was issue #1540: the + // normalized size was paired with the un-normalized count. rec := mocks.BuildLegacyReservationRecommendation( mocks.WithNormalizedSize("Standard_D2s_v3"), - mocks.WithSKU("SHOULD_NOT_WIN"), + mocks.WithSKU("Standard_D8s_v3"), ) f := Extract(rec) require.NotNil(t, f) - assert.Equal(t, "Standard_D2s_v3", f.ResourceType) + assert.Equal(t, "Standard_D8s_v3", f.ResourceType) } -func TestExtract_ResourceTypeFallsBackToSKUName(t *testing.T) { +func TestExtract_ResourceTypeUsesSKUName(t *testing.T) { rec := mocks.BuildLegacyReservationRecommendation( - // No NormalizedSize → should fall back to SKUProperties entry with Name="SKUName". mocks.WithSKU("Standard_E4s_v5"), ) f := Extract(rec) @@ -118,8 +119,9 @@ func TestExtract_ResourceTypeFallsBackToSKUName(t *testing.T) { } func TestExtract_ResourceTypeFallsBackToFirstPropertyValue(t *testing.T) { - // No NormalizedSize, no SKUName-keyed property — fall back to the - // first non-empty value in the list (last-ditch fallback). + // No SKUName-keyed property — fall back to the first non-empty value in + // the list (last-ditch fallback). Still an actual-SKU rung, so it keeps + // RecommendedQuantity as its partner. rec := mocks.BuildLegacyReservationRecommendation( mocks.WithSKUProperty("Cores", "4"), mocks.WithSKUProperty("MemoryGB", "16"), @@ -129,6 +131,113 @@ func TestExtract_ResourceTypeFallsBackToFirstPropertyValue(t *testing.T) { assert.Equal(t, "4", f.ResourceType) } +func TestExtract_ResourceTypeFallsBackToNormalizedSize(t *testing.T) { + // No SKU properties at all — NormalizedSize is the only source left, and + // it is counted by RecommendedQuantityNormalized. + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithNormalizedSize("Standard_D2s_v3"), + mocks.WithQuantity(4), + mocks.WithNormalizedQuantity(16), + ) + f := Extract(rec) + require.NotNil(t, f) + assert.Equal(t, "Standard_D2s_v3", f.ResourceType) + assert.Equal(t, 16, f.Count, + "the normalized size must be counted by RecommendedQuantityNormalized, not RecommendedQuantity") +} + +// TestExtract_LegacySKUAndQuantityStayInMatchingUnits is the regression test +// for issue #1540. Azure carries two quantities that are NOT interchangeable: +// RecommendedQuantity counts SKUProperties[SKUName], RecommendedQuantityNormalized +// counts NormalizedSize. Pairing the normalized SKU with the un-normalized count +// understated every EA purchase by the family's instance-size-flexibility ratio, +// while the full recommendation's savings were still reported against it. +// +// Both directions are asserted: the mismatched pair must be gone, AND each +// valid pair must still be produced. Asserting only the first passes against a +// converter that returns nothing at all. +func TestExtract_LegacySKUAndQuantityStayInMatchingUnits(t *testing.T) { + // Azure recommends 4 x Standard_D8s_v3, normalized to 16 x Standard_D2s_v3. + const ( + actualSKU = "Standard_D8s_v3" + normalized = "Standard_D2s_v3" + actualQty = 4 + normQty = 16 + ) + + tests := []struct { + name string + opts []mocks.LegacyOpt + wantType string + wantQty int + }{ + { + name: "actual SKU present: paired with the un-normalized count", + opts: []mocks.LegacyOpt{ + mocks.WithSKU(actualSKU), + mocks.WithNormalizedSize(normalized), + mocks.WithQuantity(actualQty), + mocks.WithNormalizedQuantity(normQty), + }, + wantType: actualSKU, + wantQty: actualQty, + }, + { + name: "only the normalized size: paired with the normalized count", + opts: []mocks.LegacyOpt{ + mocks.WithNormalizedSize(normalized), + mocks.WithQuantity(actualQty), + mocks.WithNormalizedQuantity(normQty), + }, + wantType: normalized, + wantQty: normQty, + }, + { + name: "no normalization: the single quantity pairs with the single SKU", + opts: []mocks.LegacyOpt{ + mocks.WithSKU(actualSKU), + mocks.WithQuantity(actualQty), + }, + wantType: actualSKU, + wantQty: actualQty, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + f := Extract(mocks.BuildLegacyReservationRecommendation(tt.opts...)) + require.NotNil(t, f) + assert.Equal(t, tt.wantType, f.ResourceType) + assert.Equal(t, tt.wantQty, f.Count) + + // The invariant the pairing exists to hold: never the normalized + // SKU counted by the un-normalized quantity. + if f.ResourceType == normalized { + assert.NotEqual(t, actualQty, f.Count, + "normalized size %q must never carry the un-normalized count %d (issue #1540)", + normalized, actualQty) + } + }) + } +} + +// TestExtract_LegacyNormalizedSizeWithoutNormalizedQuantityLeavesCountUnset +// pins the one state with no valid pairing. Borrowing RecommendedQuantity here +// would reintroduce exactly the mismatch #1540 is about, so the count is left +// at zero, which every service's `Count <= 0` guard refuses before purchasing. +func TestExtract_LegacyNormalizedSizeWithoutNormalizedQuantityLeavesCountUnset(t *testing.T) { + rec := mocks.BuildLegacyReservationRecommendation( + mocks.WithNormalizedSize("Standard_D2s_v3"), + mocks.WithQuantity(4), + mocks.WithoutNormalizedQuantity(), + ) + f := Extract(rec) + require.NotNil(t, f) + assert.Equal(t, "Standard_D2s_v3", f.ResourceType) + assert.Equal(t, 0, f.Count, + "an unpairable count must be left unset, not borrowed from RecommendedQuantity") +} + func TestExtract_MissingCostsReadAsZero(t *testing.T) { // Cost fields are *float64 in the SDK — nil must become 0.0, not panic. rec := mocks.BuildLegacyReservationRecommendation() // no WithCosts @@ -228,24 +337,37 @@ func TestExtract_Modern_RegionFallsBackToInnerProperties(t *testing.T) { func TestExtract_Modern_ResourceTypePrefersSKUNameOverNormalizedSize(t *testing.T) { // Both populated — Modern's top-level SKUName wins over NormalizedSize - // (matches the Modern field-preference documented in resolveModernResourceType). + // (matches the Modern field-preference documented in + // resolveModernSKUAndQuantity), and is counted by RecommendedQuantity. rec := mocks.BuildModernReservationRecommendation( mocks.WithModernSKUName("Standard_E4s_v5"), mocks.WithModernNormalizedSize("Standard_D2"), + mocks.WithModernQuantity(4), + mocks.WithModernNormalizedQuantity(16), ) f := Extract(rec) require.NotNil(t, f) assert.Equal(t, "Standard_E4s_v5", f.ResourceType) + assert.Equal(t, 4, f.Count, + "the actual SKU is counted by RecommendedQuantity; this is the rung real MCA payloads take") } -func TestExtract_Modern_ResourceTypeFallsBackToNormalizedSize(t *testing.T) { - // No SKUName — should fall back to NormalizedSize (second preference). +// TestExtract_Modern_NormalizedSizeRungIsCountedNormalized covers the one +// Modern rung that had #1540's mismatch. Modern was not the reported instance +// -- real MCA payloads carry SKUName and take the first rung, asserted above -- +// but with SKUName absent it paired NormalizedSize with RecommendedQuantity +// just as Legacy did. +func TestExtract_Modern_NormalizedSizeRungIsCountedNormalized(t *testing.T) { rec := mocks.BuildModernReservationRecommendation( mocks.WithModernNormalizedSize("Standard_D2"), + mocks.WithModernQuantity(4), + mocks.WithModernNormalizedQuantity(16), ) f := Extract(rec) require.NotNil(t, f) assert.Equal(t, "Standard_D2", f.ResourceType) + assert.Equal(t, 16, f.Count, + "the normalized size must be counted by RecommendedQuantityNormalized") } func TestExtract_Modern_MissingCostAmountsReadAsZero(t *testing.T) { diff --git a/providers/azure/mocks/recommendation_fixtures.go b/providers/azure/mocks/recommendation_fixtures.go index a7fa534a2..c80f3bc21 100644 --- a/providers/azure/mocks/recommendation_fixtures.go +++ b/providers/azure/mocks/recommendation_fixtures.go @@ -25,10 +25,13 @@ func BuildLegacyReservationRecommendation(opts ...LegacyOpt) *armconsumption.Leg term := "P1Y" qty := float64(1) + normQty := float32(qty) + props := &armconsumption.LegacyReservationRecommendationProperties{ - Scope: &scope, - Term: &term, - RecommendedQuantity: &qty, + Scope: &scope, + Term: &term, + RecommendedQuantity: &qty, + RecommendedQuantityNormalized: &normQty, } rec := &armconsumption.LegacyReservationRecommendation{ Location: &location, @@ -67,15 +70,45 @@ func WithTerm(term string) LegacyOpt { } } -// WithQuantity overrides RecommendedQuantity. Use float values (e.g. 0.5, -// 2.7) to cover the float→int truncation contract. +// WithQuantity overrides BOTH recommended quantities, modeling the common +// case where the recommended SKU is already the family's base size so the +// normalized and un-normalized counts agree. Use float values (e.g. 0.5, 2.7) +// to cover the float→int truncation contract. +// +// Setting both matters because the converter pairs each resource-type source +// with the quantity expressed in that source's units (issue #1540): a fixture +// that set only RecommendedQuantity while naming a NormalizedSize would +// describe a payload with no valid pairing. Use WithNormalizedQuantity to make +// the two differ deliberately. func WithQuantity(qty float64) LegacyOpt { return func(_ *armconsumption.LegacyReservationRecommendation, props *armconsumption.LegacyReservationRecommendationProperties) { + normQty := float32(qty) props.RecommendedQuantity = &qty + props.RecommendedQuantityNormalized = &normQty + } +} + +// WithNormalizedQuantity overrides RecommendedQuantityNormalized alone, so a +// test can express the real shape the normalization ratio produces: e.g. +// 4 x Standard_D8s_v3 recommended, 16 x Standard_D2s_v3 normalized. Pass it +// after WithQuantity, which sets both. +func WithNormalizedQuantity(qty float32) LegacyOpt { + return func(_ *armconsumption.LegacyReservationRecommendation, props *armconsumption.LegacyReservationRecommendationProperties) { + props.RecommendedQuantityNormalized = &qty } } -// WithNormalizedSize populates the preferred ResourceType source. +// WithoutNormalizedQuantity clears RecommendedQuantityNormalized, modeling a +// payload that names a NormalizedSize but gives no count in its units. +func WithoutNormalizedQuantity() LegacyOpt { + return func(_ *armconsumption.LegacyReservationRecommendation, props *armconsumption.LegacyReservationRecommendationProperties) { + props.RecommendedQuantityNormalized = nil + } +} + +// WithNormalizedSize populates NormalizedSize, the ResourceType source used +// when no SKU property names the actual SKU. It is counted by +// RecommendedQuantityNormalized, not RecommendedQuantity (issue #1540). func WithNormalizedSize(size string) LegacyOpt { return func(_ *armconsumption.LegacyReservationRecommendation, props *armconsumption.LegacyReservationRecommendationProperties) { props.NormalizedSize = &size @@ -83,8 +116,8 @@ func WithNormalizedSize(size string) LegacyOpt { } // WithSKU is a convenience that seeds SKUProperties with a single -// `{Name: "SKUName", Value: sku}` entry — the fallback path the converter -// uses when NormalizedSize is unset. +// `{Name: "SKUName", Value: sku}` entry — the actual SKU, which the converter +// prefers over NormalizedSize because RecommendedQuantity counts it. func WithSKU(sku string) LegacyOpt { return func(_ *armconsumption.LegacyReservationRecommendation, props *armconsumption.LegacyReservationRecommendationProperties) { name := "SKUName" @@ -144,10 +177,13 @@ func BuildModernReservationRecommendation(opts ...ModernOpt) *armconsumption.Mod term := "P1Y" qty := float64(1) + normQty := float32(qty) + props := &armconsumption.ModernReservationRecommendationProperties{ - Scope: &scope, - Term: &term, - RecommendedQuantity: &qty, + Scope: &scope, + Term: &term, + RecommendedQuantity: &qty, + RecommendedQuantityNormalized: &normQty, } rec := &armconsumption.ModernReservationRecommendation{ Location: &location, @@ -195,10 +231,23 @@ func WithModernTerm(term string) ModernOpt { } } -// WithModernQuantity overrides RecommendedQuantity. +// WithModernQuantity overrides BOTH recommended quantities, for the same +// reason as the Legacy WithQuantity: the converter pairs each resource-type +// source with the quantity in that source's units (issue #1540). func WithModernQuantity(qty float64) ModernOpt { return func(_ *armconsumption.ModernReservationRecommendation, props *armconsumption.ModernReservationRecommendationProperties) { + normQty := float32(qty) props.RecommendedQuantity = &qty + props.RecommendedQuantityNormalized = &normQty + } +} + +// WithModernNormalizedQuantity overrides RecommendedQuantityNormalized alone, +// so a test can make the normalized and un-normalized counts differ. Pass it +// after WithModernQuantity, which sets both. +func WithModernNormalizedQuantity(qty float32) ModernOpt { + return func(_ *armconsumption.ModernReservationRecommendation, props *armconsumption.ModernReservationRecommendationProperties) { + props.RecommendedQuantityNormalized = &qty } } @@ -211,7 +260,8 @@ func WithModernSKUName(sku string) ModernOpt { } // WithModernNormalizedSize populates NormalizedSize (second-preference -// source for ResourceType on Modern). +// source for ResourceType on Modern). Counted by +// RecommendedQuantityNormalized, not RecommendedQuantity (issue #1540). func WithModernNormalizedSize(size string) ModernOpt { return func(_ *armconsumption.ModernReservationRecommendation, props *armconsumption.ModernReservationRecommendationProperties) { props.NormalizedSize = &size diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index 8c551fd29..5f2ed6846 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -16,6 +16,7 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption" + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -1497,3 +1498,88 @@ func TestComputeClient_PurchaseCommitment_ZeroCountRejected(t *testing.T) { assert.Contains(t, err.Error(), "quantity must be greater than zero") mockHTTP.AssertNotCalled(t, "Do", mock.Anything) } + +// TestPurchaseBody_SKUAndQuantityStayInMatchingUnits is the downstream half of +// the issue #1540 regression. The pairing is fixed in the shared converter, but +// the defect only mattered because it reached the wire: the same fields become +// `sku.name` and `quantity` on the calculatePrice and purchase requests. A test +// that only exercised the converter would look right while the bytes Azure +// receives stayed wrong, which is exactly how the defect survived. +// +// Both directions are asserted per case: the SKU that must be sent AND the +// quantity that must accompany it. Asserting only that the old mismatched pair +// is gone would pass against a converter that produced nothing. +func TestPurchaseBody_SKUAndQuantityStayInMatchingUnits(t *testing.T) { + // Azure recommends 4 x Standard_D8s_v3, normalized to 16 x Standard_D2s_v3. + const ( + actualSKU = "Standard_D8s_v3" + normalized = "Standard_D2s_v3" + ) + + tests := []struct { + name string + opts []mocks.LegacyOpt + wantSKU string + wantQty float64 + }{ + { + name: "actual SKU present: buys the recommended SKU at the recommended count", + opts: []mocks.LegacyOpt{ + mocks.WithSKU(actualSKU), + mocks.WithNormalizedSize(normalized), + mocks.WithQuantity(4), + mocks.WithNormalizedQuantity(16), + }, + wantSKU: actualSKU, + wantQty: 4, + }, + { + name: "only the normalized size: buys the normalized SKU at the normalized count", + opts: []mocks.LegacyOpt{ + mocks.WithNormalizedSize(normalized), + mocks.WithQuantity(4), + mocks.WithNormalizedQuantity(16), + }, + wantSKU: normalized, + wantQty: 16, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + apiRec := mocks.BuildLegacyReservationRecommendation(append(tt.opts, + mocks.WithRegion("eastus"), + mocks.WithCosts(1000, 600, 400), + )...) + + c := &ComputeClient{subscriptionID: "sub-1", region: "eastus"} + rec := c.convertAzureVMRecommendation(context.Background(), apiRec) + require.NotNil(t, rec) + + body, err := c.buildReservationBody(*rec, armreservations.ReservationBillingPlanUpfront, "test", "tok") + require.NoError(t, err) + + var payload struct { + SKU struct { + Name string `json:"name"` + } `json:"sku"` + Properties struct { + Quantity float64 `json:"quantity"` + } `json:"properties"` + } + require.NoError(t, json.Unmarshal(body, &payload)) + + assert.Equal(t, tt.wantSKU, payload.SKU.Name, + "sku.name is what Azure reserves") + assert.Equal(t, tt.wantQty, payload.Properties.Quantity, + "quantity must be expressed in units of sku.name, never the other size's count (issue #1540)") + + // The specific mismatch #1540 shipped: the normalized (smaller) SKU + // carrying the un-normalized count, i.e. a quarter of the capacity. + if payload.SKU.Name == normalized { + assert.NotEqual(t, float64(4), payload.Properties.Quantity, + "normalized SKU %q must never be bought at the un-normalized count", normalized) + } + }) + } +}