diff --git a/providers/azure/services/compute/inventory_scope_sdk_test.go b/providers/azure/services/compute/inventory_scope_sdk_test.go index 5cd9337d..a3d1580a 100644 --- a/providers/azure/services/compute/inventory_scope_sdk_test.go +++ b/providers/azure/services/compute/inventory_scope_sdk_test.go @@ -319,3 +319,116 @@ func TestGetExistingCommitmentsSDKExpiryDateControls(t *testing.T) { }) } } + +// Issue #315: the applied-scope attribution check must only gate live +// reservations. A terminal (canceled, expired, failed, split, merged) Shared +// or scope-less VM reservation no longer counts as coverage, so it must not +// fail the whole inventory and block purchases through the duplicate guard. +func TestGetExistingCommitmentsSDKTerminalSharedVMDoesNotFail(t *testing.T) { + now := time.Now().UTC() + cases := []struct { + name string + state string + scopeType string + expiry time.Time + wantState common.CommitmentState + }{ + {name: "canceled shared", state: string(armreservations.ProvisioningStateCancelled), scopeType: `"Shared"`, expiry: now.AddDate(1, 0, 0), wantState: common.CommitmentStateCanceled}, + {name: "expired shared", state: string(armreservations.ProvisioningStateExpired), scopeType: `"Shared"`, expiry: now.AddDate(0, 0, -3), wantState: common.CommitmentStateExpired}, + {name: "failed shared", state: string(armreservations.ProvisioningStateFailed), scopeType: `"Shared"`, expiry: now.AddDate(1, 0, 0), wantState: common.CommitmentStateFailed}, + {name: "split shared", state: string(armreservations.ProvisioningStateSplit), scopeType: `"Shared"`, expiry: now.AddDate(1, 0, 0), wantState: common.CommitmentStateRetired}, + {name: "merged shared", state: string(armreservations.ProvisioningStateMerged), scopeType: `"Shared"`, expiry: now.AddDate(1, 0, 0), wantState: common.CommitmentStateRetired}, + {name: "succeeded shared past expiry", state: "Succeeded", scopeType: `"Shared"`, expiry: now.AddDate(0, 0, -3), wantState: common.CommitmentStateExpired}, + {name: "canceled missing scope type", state: string(armreservations.ProvisioningStateCancelled), scopeType: `null`, expiry: now.AddDate(1, 0, 0), wantState: common.CommitmentStateCanceled}, + } + rec := common.Recommendation{Provider: common.ProviderAzure, Service: common.ServiceCompute, + Account: "sub-a", Region: "eastus", ResourceType: "Standard_D2s_v3", Count: 1} + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + child := sdkReservationChild(sdkChild{ + id: "terminal-shared", resourceType: "VirtualMachines", scopeType: tc.scopeType, scopes: `null`, + region: "eastus", state: tc.state, quantity: 1, + }, now.AddDate(-1, 0, 0), tc.expiry) + client := sdkInventoryClient(t, "sub-a", inventorySDKTransport{child: child}) + inventory, err := client.GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.Len(t, inventory, 1) + require.Equal(t, tc.wantState, inventory[0].State) + + passed, filtered, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting( + context.Background(), []common.Recommendation{rec}, client) + require.NoError(t, err) + require.Len(t, passed, 1) + require.Empty(t, filtered) + }) + } +} + +// A terminal Shared row next to a live Single row must not hide the live +// coverage, and a live Shared row next to a terminal one still fails closed. +func TestGetExistingCommitmentsSDKTerminalSharedMixedWithLive(t *testing.T) { + now := time.Now().UTC() + canceledShared := sdkReservationChild(sdkChild{ + id: "canceled-shared", resourceType: "VirtualMachines", scopeType: `"Shared"`, scopes: `null`, + region: "eastus", state: string(armreservations.ProvisioningStateCancelled), quantity: 1, + }, now.AddDate(-1, 0, 0), now.AddDate(1, 0, 0)) + liveSingle := sdkReservationChild(sdkChild{ + id: "live-single", resourceType: "VirtualMachines", scopeType: `"Single"`, scopes: `["/subscriptions/sub-a"]`, + region: "eastus", state: "Succeeded", quantity: 1, + }, now, now.AddDate(1, 0, 0)) + client := sdkInventoryClient(t, "sub-a", inventorySDKTransport{child: canceledShared + "," + liveSingle}) + rec := common.Recommendation{Provider: common.ProviderAzure, Service: common.ServiceCompute, + Account: "sub-a", Region: "eastus", ResourceType: "Standard_D2s_v3", Count: 1} + passed, filtered, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting( + context.Background(), []common.Recommendation{rec}, client) + require.NoError(t, err) + require.Empty(t, passed) + require.Len(t, filtered, 1) + + for _, state := range []string{"Succeeded", string(armreservations.ProvisioningStatePendingBilling)} { + t.Run("live shared "+state, func(t *testing.T) { + liveShared := sdkReservationChild(sdkChild{ + id: "live-shared", resourceType: "VirtualMachines", scopeType: `"Shared"`, scopes: `null`, + region: "eastus", state: state, quantity: 1, + }, now, now.AddDate(1, 0, 0)) + client := sdkInventoryClient(t, "sub-a", inventorySDKTransport{child: canceledShared + "," + liveShared}) + inventory, err := client.GetExistingCommitments(context.Background()) + require.ErrorContains(t, err, "cannot attribute") + require.Nil(t, inventory) + }) + } +} + +// The date-only expiry ambiguity is not a scope check: a Succeeded Shared +// reservation expiring today converts to Expired but may still be live, so +// it must keep failing closed after the terminal exemption. +func TestGetExistingCommitmentsSDKSharedExpiryTodayStillFails(t *testing.T) { + now := time.Now().UTC() + child := sdkReservationChild(sdkChild{ + id: "shared-expiry-today", resourceType: "VirtualMachines", scopeType: `"Shared"`, scopes: `null`, + region: "eastus", state: "Succeeded", quantity: 1, + }, now.AddDate(-1, 0, 0), now) + client := sdkInventoryClient(t, "sub-a", inventorySDKTransport{child: child}) + inventory, err := client.GetExistingCommitments(context.Background()) + require.ErrorContains(t, err, "date-only expiryDate") + require.Nil(t, inventory) +} + +// A Split parent is exempt from the scope check only because its children +// are listed as their own rows. A live Shared child of that split must still +// fail closed, so the exemption cannot hide unattributable live coverage. +func TestGetExistingCommitmentsSDKSplitParentLiveSharedChildStillFails(t *testing.T) { + now := time.Now().UTC() + splitParent := sdkReservationChild(sdkChild{ + id: "split-parent", resourceType: "VirtualMachines", scopeType: `"Shared"`, scopes: `null`, + region: "eastus", state: string(armreservations.ProvisioningStateSplit), quantity: 2, + }, now.AddDate(-1, 0, 0), now.AddDate(1, 0, 0)) + liveSharedChild := sdkReservationChild(sdkChild{ + id: "split-child", resourceType: "VirtualMachines", scopeType: `"Shared"`, scopes: `null`, + region: "eastus", state: "Succeeded", quantity: 1, + }, now, now.AddDate(1, 0, 0)) + client := sdkInventoryClient(t, "sub-a", inventorySDKTransport{child: splitParent + "," + liveSharedChild}) + inventory, err := client.GetExistingCommitments(context.Background()) + require.ErrorContains(t, err, "cannot attribute") + require.Nil(t, inventory) +} diff --git a/providers/azure/services/internal/reservations/strict.go b/providers/azure/services/internal/reservations/strict.go index 07995477..bb178b9f 100644 --- a/providers/azure/services/internal/reservations/strict.go +++ b/providers/azure/services/internal/reservations/strict.go @@ -33,18 +33,12 @@ func StrictCommitment(r *armreservations.ReservationResponse, subscriptionID str if commitment == nil { return nil, nil } - owned, err := reservationOwnedBy(r.Properties, commitment.CommitmentID, subscriptionID, labels) - if err != nil { - return nil, err - } - if !owned { - log.Printf("%s: skipping Shared %s reservation %q: billed to %q, not subscription %q", - labels.Service, labels.Resource, commitment.CommitmentID, *r.Properties.BillingScopeID, subscriptionID) - return nil, nil - } - if err := validateReservationExpiry(r.Properties, commitment.CommitmentID, subscriptionID, labels, now); err != nil { + keep, err := attributeReservation(r.Properties, commitment, subscriptionID, labels, now) + if err != nil || !keep { return nil, err } + // A terminal reservation no longer counts as coverage, so its field + // completeness cannot affect the duplicate guard (#317). if !isTerminalCommitmentState(commitment.State) { if err := validateLiveReservationFields(r, commitment.CommitmentID, subscriptionID, labels); err != nil { return nil, err @@ -53,6 +47,32 @@ func StrictCommitment(r *armreservations.ReservationResponse, subscriptionID str return commitment, nil } +// attributeReservation decides whether a reservation counts for +// subscriptionID, in this order: +// - A Shared reservation billed to another subscription is skipped whatever +// its state: it is never this subscription's coverage (#293). +// - The expiry-today check runs on every other row: a Succeeded reservation +// expiring today converts to Expired but may still be live. +// - A scope that cannot be attributed (missing, unknown, or Shared without a +// subscription billingScopeId) fails closed only on a live reservation. A +// terminal one no longer counts as coverage, so failing on it would block +// purchases for the subscription indefinitely (#315). +func attributeReservation(props *armreservations.Properties, commitment *common.Commitment, subscriptionID string, labels StrictLabels, now time.Time) (bool, error) { + owned, ownErr := reservationOwnedBy(props, commitment.CommitmentID, subscriptionID, labels) + if ownErr == nil && !owned { + log.Printf("%s: skipping Shared %s reservation %q: billed to %q, not subscription %q", + labels.Service, labels.Resource, commitment.CommitmentID, *props.BillingScopeID, subscriptionID) + return false, nil + } + if err := validateReservationExpiry(props, commitment.CommitmentID, subscriptionID, labels, now); err != nil { + return false, err + } + if ownErr != nil && !isTerminalCommitmentState(commitment.State) { + return false, ownErr + } + return true, nil +} + func reservationLabel(r *armreservations.ReservationResponse) string { if r == nil || r.ID == nil { return "(no id)" @@ -63,6 +83,14 @@ func reservationLabel(r *armreservations.ReservationResponse) string { // isTerminalCommitmentState reports lifecycle states that no longer count as // coverage. It works on the converted state so a Succeeded reservation with a // past expiry is terminal too. +// +// Retired (Split/Merged parents) is exempt from the scope failure on the +// assumption, shared with #317's field-completeness gate, that Azure lists the +// resulting child reservations as separate rows, so each live child is +// scope-checked on its own. That is an API-shape assumption not verifiable offline; if a +// live child were ever omitted from the listing, its coverage would be +// missing rather than misattributed, so the duplicate guard would err toward +// buying, not toward a false "already covered". func isTerminalCommitmentState(s common.CommitmentState) bool { switch s { case common.CommitmentStateCanceled, common.CommitmentStateExpired, @@ -143,6 +171,8 @@ func billingSubscriptionID(scope *string) (string, bool) { return sub, true } +// validateReservationExpiry rejects a Succeeded reservation whose date-only +// expiryDate is the current UTC date: it may or may not still be live. func validateReservationExpiry(props *armreservations.Properties, reservationID, subscriptionID string, labels StrictLabels, now time.Time) error { if props.ProvisioningState != nil && *props.ProvisioningState == armreservations.ProvisioningStateSucceeded && props.ExpiryDate != nil && props.ExpiryDate.UTC().Format(time.DateOnly) == now.UTC().Format(time.DateOnly) { diff --git a/providers/azure/services/internal/reservations/strict_test.go b/providers/azure/services/internal/reservations/strict_test.go index 74eacc12..0a249680 100644 --- a/providers/azure/services/internal/reservations/strict_test.go +++ b/providers/azure/services/internal/reservations/strict_test.go @@ -95,3 +95,52 @@ func TestStrictCommitment_SharedScopeAttributedToBillingSubscription(t *testing. _, err = StrictCommitment(r, "sub-a", common.ServiceCompute, armreservations.ReservedResourceTypeVirtualMachines, testLabels, now) require.ErrorContains(t, err, "Shared scope without a subscription billingScopeId") } + +// #293 and #315 together: a Shared reservation billed to another subscription +// is skipped whatever its lifecycle, so neither a terminal state nor an +// expiry-today date makes it this subscription's row or its error. One billed +// to this subscription still hits the expiry-today check, and a terminal one +// whose purchaser cannot be identified no longer fails closed. +func TestStrictCommitment_SharedAttributionAcrossLifecycle(t *testing.T) { + t.Parallel() + now := time.Now() + shared := armreservations.AppliedScopeTypeShared + canceled := armreservations.ProvisioningStateCancelled + sharedFixture := func(billingScope *string) *armreservations.ReservationResponse { + r := strictFixture("VirtualMachines") + r.Properties.AppliedScopeType = &shared + r.Properties.AppliedScopes = nil + r.Properties.BillingScopeID = billingScope + return r + } + strict := func(r *armreservations.ReservationResponse, sub string) (*common.Commitment, error) { + return StrictCommitment(r, sub, common.ServiceCompute, armreservations.ReservedResourceTypeVirtualMachines, testLabels, now) + } + + r := sharedFixture(strPtr("/subscriptions/sub-a")) + r.Properties.ProvisioningState = &canceled + c, err := strict(r, "sub-b") + require.NoError(t, err) + assert.Nil(t, c, "a terminal Shared reservation bought by another subscription is not attributed to this one") + + r = sharedFixture(strPtr("/subscriptions/sub-a")) + r.Properties.ExpiryDate = &now + c, err = strict(r, "sub-b") + require.NoError(t, err, "another subscription's reservation expiring today must not block this one") + assert.Nil(t, c) + + _, err = strict(r, "sub-a") + require.ErrorContains(t, err, "date-only expiryDate") + + r = sharedFixture(nil) + r.Properties.ExpiryDate = &now + _, err = strict(r, "sub-a") + require.ErrorContains(t, err, "date-only expiryDate", "an unattributable row expiring today may be live, so the lifecycle check still runs") + + r = sharedFixture(nil) + r.Properties.ProvisioningState = &canceled + c, err = strict(r, "sub-a") + require.NoError(t, err) + require.NotNil(t, c) + assert.Equal(t, common.CommitmentStateCanceled, c.State) +}