fix(azure): count savings plans billed to another subscription in the duplicate check

pkg/recfilter/dedupe.go

@@ -17,6 +17,15 @@ type recentCommitmentFilter interface {
 	FilterRecommendationsForRecentCommitments(recs []common.Recommendation, existing []common.Commitment) (passed, filtered []common.Recommendation, err error)
 }
 
+// coveringCommitmentLister is implemented by clients whose inventory
+// (GetExistingCommitments, where Account is the payer) leaves out commitments
+// billed elsewhere that still apply to the client's scope. The duplicate check
+// uses this wider list instead, so a recent covering commitment suppresses a
+// re-buy without being counted in a second owner's inventory.
+type coveringCommitmentLister interface {
+	GetCoveringCommitments(ctx context.Context) ([]common.Commitment, error)
+}
+
 // DuplicateChecker checks for existing commitments to avoid duplicates.
 type DuplicateChecker struct { //nolint:govet // fieldalignment: file owned by open PR #491
 	LookbackHours int // How many hours to look back for recent purchases
@@ -45,7 +54,7 @@ func NewDuplicateChecker(hours int) *DuplicateChecker {
 // Note: This is designed to prevent re-purchasing something you just bought, not to prevent
 // purchasing RIs in other accounts that happen to have the same characteristics.
 func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context, recs []common.Recommendation, client provider.ServiceClient) (passed, filtered []common.Recommendation, err error) {
-	existing, err := client.GetExistingCommitments(ctx)
+	existing, err := commitmentsForDedupe(ctx, client)
 	if err != nil {
 		return recs, nil, err
 	}
@@ -80,6 +89,15 @@ func (d *DuplicateChecker) AdjustRecommendationsForExisting(ctx context.Context,
 	return passed, filtered, nil
 }
 
+// commitmentsForDedupe returns the commitments a recommendation is checked
+// against: the covering list when the client provides one, else its inventory.
+func commitmentsForDedupe(ctx context.Context, client provider.ServiceClient) ([]common.Commitment, error) {
+	if lister, ok := client.(coveringCommitmentLister); ok {
+		return lister.GetCoveringCommitments(ctx)
+	}
+	return client.GetExistingCommitments(ctx)
+}
+
 // filterRecentCommitments filters commitments to only recent purchases within the lookback window.
 func (d *DuplicateChecker) filterRecentCommitments(existing []common.Commitment) []common.Commitment {
 	now := time.Now

pkg/recfilter/dedupe_test.go

@@ -634,3 +634,34 @@ func TestAdjustRecommendationsForExisting_PartialCoverageScalesMoney(t *testing.
 	assert.InDelta(t, 40.0, *passed[0].RecurringMonthlyCost, 1e-9)
 	assert.Equal(t, 100.0, monthly, "caller's pointer is not mutated")
 }
+
+type coveringClient struct {
+	fakeServiceClient
+	covering []common.Commitment
+	err      error
+}
+
+func (c *coveringClient) GetCoveringCommitments(context.Context) ([]common.Commitment, error) {
+	return c.covering, c.err
+}
+
+// A commitment billed elsewhere is absent from the inventory but still covers
+// the client; the check must suppress against the covering list (#466).
+func TestAdjustRecommendationsForExisting_UsesCoveringCommitments(t *testing.T) {
+	t.Parallel()
+	recent := common.Commitment{ResourceType: "Compute", Count: 1, State: common.CommitmentStateActive, StartDate: time.Now().Add(-time.Hour)}
+	recs := []common.Recommendation{{ResourceType: "Compute", Count: 1}}
+
+	client := &coveringClient{covering: []common.Commitment{recent}}
+	passed, filtered, err := NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, client)
+	require.NoError(t, err)
+	assert.Empty(t, passed)
+	assert.Equal(t, recs, filtered)
+
+	wantErr := errors.New("covering list unavailable")
+	client = &coveringClient{fakeServiceClient: fakeServiceClient{commitments: []common.Commitment{recent}}, err: wantErr}
+	passed, filtered, err = NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, client)
+	require.ErrorIs(t, err, wantErr)
+	assert.Equal(t, recs, passed)
+	assert.Nil(t, filtered)
+}

providers/azure/services/savingsplans/client.go

@@ -144,6 +144,13 @@ const maxSavingsPlanPages = 100
 // that name this subscription (see planOwnedBy). Shared and management-group
 // plans billed to a billing account fail the call closed.
 func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) {
+	return c.listCommitments(ctx, false)
+}
+
+// listCommitments walks the savings plan inventory. covering also keeps plans
+// billed to another subscription that may apply to this one (see
+// GetCoveringCommitments).
+func (c *Client) listCommitments(ctx context.Context, covering bool) ([]common.Commitment, error) {
 	var pager SavingsPlanListAllPager
 
 	if c.listAllPager != nil {
@@ -170,7 +177,7 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen
 			return nil, fmt.Errorf("failed to list savings plans: %w", err)
 		}
 		var skipped int
-		commitments, skipped, err = c.appendOwnedPlans(commitments, page.Value)
+		commitments, skipped, err = c.appendOwnedPlans(commitments, page.Value, covering)
 		skippedSingle += skipped
 		if err != nil {
 			return nil, fmt.Errorf("incomplete savings plan inventory for subscription %s, page %d: %w", c.subscriptionID, pageIndex, err)
@@ -184,7 +191,7 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen
 
 const subscriptionScopePrefix = "/subscriptions/"
 
-func (c *Client) appendOwnedPlans(dst []common.Commitment, plans []*armbillingbenefits.SavingsPlanModel) ([]common.Commitment, int, error) {
+func (c *Client) appendOwnedPlans(dst []common.Commitment, plans []*armbillingbenefits.SavingsPlanModel, covering bool) ([]common.Commitment, int, error) {
 	skipped := 0
 	for i, sp := range plans {
 		disposition, err := planOwnedBy(sp, c.subscriptionID)
@@ -193,13 +200,19 @@ func (c *Client) appendOwnedPlans(dst []common.Commitment, plans []*armbillingbe
 		}
 		switch disposition {
 		case planOwned:
-			commitment, err := convertSavingsPlan(sp, c.subscriptionID)
-			if err != nil {
-				return nil, 0, fmt.Errorf("row %d: %w", i+1, err)
+			commitment, convErr := convertSavingsPlan(sp, c.subscriptionID)
+			if convErr != nil {
+				return nil, 0, fmt.Errorf("row %d: %w", i+1, convErr)
 			}
 			dst = append(dst, *commitment)
 		case planSkippedSingle:
 			skipped++
+		case planOtherSubscription:
+			if covering {
+				if dst, err = appendCoveringForeignPlan(dst, sp, c.subscriptionID); err != nil {
+					return nil, 0, fmt.Errorf("row %d: %w", i+1, err)
+				}
+			}
 		}
 	}
 	return dst, skipped, nil

providers/azure/services/savingsplans/client_completeness_sdk_test.go

@@ -53,7 +53,8 @@ func TestGetExistingCommitments_SDKPaginationCompleteness(t *testing.T) {
 						if ambiguous {
 							scope = "/providers/Microsoft.Billing/billingAccounts/ba-1"
 						}
-						_, err := fmt.Fprintf(w, `{"value":[{"id":"second","properties":{"billingScopeId":%q,"appliedScopeType":"Shared"}}]}`, scope)
+						_, err := fmt.Fprintf(w, `{"value":[{"id":"second","sku":{"name":"Compute_Savings_Plan"},"properties":{"billingScopeId":%q,"appliedScopeType":"Shared","effectiveDateTime":%q}}]}`,
+							scope, time.Now().UTC().Format(time.RFC3339))
 						assert.NoError(t, err)
 					default:
 						t.Errorf("unexpected page: %s", r.URL.RawQuery)

providers/azure/services/savingsplans/scope.go

@@ -1,19 +1,23 @@
 package savingsplans
 
 import (
+	"context"
 	"fmt"
 	"strings"
 	"unicode"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits"
+
+	"github.com/LeanerCloud/cloud-commitments-go/pkg/common"
 )
 
 type planDisposition int
 
 const (
-	planNotOurs       planDisposition = iota // billed to another subscription
-	planOwned                                // belongs in this subscription's inventory
-	planSkippedSingle                        // account-billed Single plan applied to another subscription
+	planNotOurs           planDisposition = iota // unresolved; returned only with an error
+	planOwned                                    // belongs in this subscription's inventory
+	planSkippedSingle                            // account-billed Single plan applied to another subscription
+	planOtherSubscription                        // billed to another subscription; may still apply to this one
 )
 
 const billingAccountPrefix = "/providers/Microsoft.Billing/billingAccounts/"
@@ -41,7 +45,7 @@ func subscriptionBilledPlan(sp *armbillingbenefits.SavingsPlanModel, billingScop
 		return planNotOurs, fmt.Errorf("malformed billing subscription scope")
 	}
 	if !strings.EqualFold(billingScope, subscriptionScopePrefix+ownSub) {
-		return planNotOurs, nil
+		return planOtherSubscription, nil
 	}
 	return ownedWithID(sp)
 }
@@ -53,9 +57,48 @@ func ownedWithID(sp *armbillingbenefits.SavingsPlanModel) (planDisposition, erro
 	return planOwned, nil
 }
 
+// GetCoveringCommitments returns GetExistingCommitments plus the plans billed
+// to another subscription that may apply to this one, for the duplicate check
+// only (#466). Account stays the payer, so such a plan is still owned only by
+// the subscription that pays for it.
+func (c *Client) GetCoveringCommitments(ctx context.Context) ([]common.Commitment, error) {
+	return c.listCommitments(ctx, true)
+}
+
+// appendCoveringForeignPlan appends a plan billed to another subscription
+// unless it provably cannot cover ownSub.
+func appendCoveringForeignPlan(dst []common.Commitment, sp *armbillingbenefits.SavingsPlanModel, ownSub string) ([]common.Commitment, error) {
+	if !foreignPlanMayCover(sp.Properties, ownSub) {
+		return dst, nil
+	}
+	if _, err := ownedWithID(sp); err != nil {
+		return nil, fmt.Errorf("covering savings plan has no ID")
+	}
+	payer := (*sp.Properties.BillingScopeID)[len(subscriptionScopePrefix):]
+	commitment, err := convertSavingsPlan(sp, payer)
+	if err != nil {
+		return nil, err
+	}
+	return append(dst, *commitment), nil
+}
+
+// foreignPlanMayCover reports whether a plan billed to another subscription
+// may apply to ownSub. Only a Single scope that resolves to a different
+// subscription rules it out. Shared and management-group scopes cover
+// subscriptions this client cannot enumerate, so like unknown or unresolvable
+// scopes they count as covering: skipping a purchase is recoverable, a
+// duplicate commitment is not.
+func foreignPlanMayCover(props *armbillingbenefits.SavingsPlanModelProperties, ownSub string) bool {
+	if props.AppliedScopeType == nil || *props.AppliedScopeType != armbillingbenefits.AppliedScopeTypeSingle {
+		return true
+	}
+	applied, err := singleScopeSubscription(props.AppliedScopeProperties)
+	return err != nil || strings.EqualFold(applied, ownSub)
+}
+
 // accountBilledPlan attributes a Single plan to the subscription it names,
 // because no subscription pays for it. Plans billed to another subscription
-// but applied to this one are tracked in #466 and are not handled here.
+// but applied to this one are handled by GetCoveringCommitments.
 func accountBilledPlan(sp *armbillingbenefits.SavingsPlanModel, ownSub string) (planDisposition, error) {
 	if sp.Properties.AppliedScopeType == nil || *sp.Properties.AppliedScopeType != armbillingbenefits.AppliedScopeTypeSingle {
 		return planNotOurs, fmt.Errorf("billing-account plan is not single-subscription scoped")

providers/azure/services/savingsplans/scope_test.go

@@ -187,3 +187,145 @@ func TestGetExistingCommitments_SDKBillingAccountSingleScope(t *testing.T) {
 		})
 	}
 }
+
+// subPlan is a plan billed to subscription sub-b with the given applied scope.
+func subPlan(id string, typ *armbillingbenefits.AppliedScopeType, props *armbillingbenefits.AppliedScopeProperties) *armbillingbenefits.SavingsPlanModel {
+	sp := accountPlan("/subscriptions/sub-b", typ, props)
+	sp.ID = toPtr(id)
+	return sp
+}
+
+func TestGetCoveringCommitments_ForeignSubscriptionPlans(t *testing.T) {
+	single := toPtr(armbillingbenefits.AppliedScopeTypeSingle)
+	own := accountPlan("/subscriptions/sub-a", toPtr(armbillingbenefits.AppliedScopeTypeShared), nil)
+	own.ID = toPtr("owned")
+	coveringNoID := subPlan("", single, scoped("/subscriptions/sub-a", ""))
+	coveringNoID.ID = nil
+	otherNoID := subPlan("", single, scoped("/subscriptions/sub-c", ""))
+	otherNoID.ID = nil
+	cases := []struct {
+		name      string
+		row       *armbillingbenefits.SavingsPlanModel
+		wantCover bool
+		wantErr   bool
+	}{
+		{"single-ours", subPlan("f", single, scoped("/subscriptions/sub-a", "")), true, false},
+		{"single-ours-mixed-case", subPlan("f", single, scoped("/Subscriptions/SUB-A", "")), true, false},
+		{"single-rg-ours", subPlan("f", single, scoped("", "/subscriptions/sub-a/resourceGroups/rg")), true, false},
+		{"single-other", subPlan("f", single, scoped("/subscriptions/sub-c", "")), false, false},
+		{"single-rg-other", subPlan("f", single, scoped("", "/subscriptions/sub-c/resourceGroups/rg")), false, false},
+		{"single-unresolvable", subPlan("f", single, nil), true, false},
+		{"single-disagreeing", subPlan("f", single, scoped("/subscriptions/sub-c", "/subscriptions/sub-a/resourceGroups/rg")), true, false},
+		{"shared", subPlan("f", toPtr(armbillingbenefits.AppliedScopeTypeShared), nil), true, false},
+		{"management-group", subPlan("f", toPtr(armbillingbenefits.AppliedScopeTypeManagementGroup), nil), true, false},
+		{"nil-type", subPlan("f", nil, nil), true, false},
+		{"unknown-type", subPlan("f", toPtr(armbillingbenefits.AppliedScopeType("Bogus")), scoped("/subscriptions/sub-c", "")), true, false},
+		{"covering-no-id", coveringNoID, false, true},
+		{"covering-blank-id", subPlan(" ", single, scoped("/subscriptions/sub-a", "")), false, true},
+		{"other-no-id", otherNoID, false, false},
+		{"account-shared", accountPlan(eaScope, toPtr(armbillingbenefits.AppliedScopeTypeShared), nil), false, true},
+	}
+	for _, tt := range cases {
+		t.Run(tt.name, func(t *testing.T) {
+			c := NewClient(nil, "sub-a", "eastus")
+			c.SetListAllPager(&mockListAllPager{results: []*armbillingbenefits.SavingsPlanModel{own, tt.row}})
+			got, err := c.GetCoveringCommitments(context.Background())
+			if tt.wantErr {
+				require.Error(t, err)
+				assert.Nil(t, got)
+				return
+			}
+			require.NoError(t, err)
+			want := []common.Commitment{mustConvertSavingsPlan(t, own, "sub-a")}
+			if tt.wantCover {
+				want = append(want, mustConvertSavingsPlan(t, tt.row, "sub-b"))
+			}
+			assert.Equal(t, want, got)
+
+			// Ownership is unchanged: the payer sub-b owns the plan, not sub-a.
+			c.SetListAllPager(&mockListAllPager{results: []*armbillingbenefits.SavingsPlanModel{own, tt.row}})
+			owned, err := c.GetExistingCommitments(context.Background())
+			require.NoError(t, err)
+			assert.Equal(t, want[:1], owned)
+		})
+	}
+}
+
+// coveringLister mirrors the optional interface recfilter.DuplicateChecker
+// consults before GetExistingCommitments (pkg/recfilter/dedupe.go).
+type coveringLister interface {
+	GetCoveringCommitments(ctx context.Context) ([]common.Commitment, error)
+}
+
+var _ coveringLister = (*Client)(nil)
+
+// coveringView hands DuplicateChecker the covering list as its inventory, so
+// this test exercises the recfilter matching against the pinned pkg as well as
+// the workspace one, which picks GetCoveringCommitments up on its own.
+type coveringView struct{ *Client }
+
+func (v coveringView) GetExistingCommitments(ctx context.Context) ([]common.Commitment, error) {
+	return v.GetCoveringCommitments(ctx)
+}
+
+// A plan billed to sub-b and bought an hour ago, decoded by the real SDK pager
+// (#466): only sub-b owns it, but it suppresses sub-a's matching recommendation
+// unless its Single scope names another subscription.
+func TestDuplicateChecker_PlanBilledToAnotherSubscription(t *testing.T) {
+	row := func(applied, props string) string {
+		return fmt.Sprintf(`{"id":"p1","name":"Compute","sku":{"name":"Compute_Savings_Plan"},"properties":{"billingScopeId":"/subscriptions/sub-b","appliedScopeType":%q,"appliedScopeProperties":%s,"provisioningState":"Succeeded","effectiveDateTime":%q}}`,
+			applied, props, time.Now().UTC().Add(-time.Hour).Format(time.RFC3339))
+	}
+	cases := []struct {
+		name         string
+		row          string
+		wantSuppress bool
+	}{
+		{"single-ours", row("Single", `{"subscriptionId":"/subscriptions/sub-a"}`), true},
+		{"single-rg-ours", row("Single", `{"resourceGroupId":"/subscriptions/sub-a/resourceGroups/rg"}`), true},
+		{"shared", row("Shared", `{}`), true},
+		{"management-group", row("ManagementGroup", `{"managementGroupId":"/providers/Microsoft.Management/managementGroups/mg"}`), true},
+		{"single-other", row("Single", `{"subscriptionId":"/subscriptions/sub-c"}`), false},
+	}
+	for _, tt := range cases {
+		t.Run(tt.name, func(t *testing.T) {
+			server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) {
+				w.Header().Set("Content-Type", "application/json")
+				_, err := fmt.Fprintf(w, `{"value":[%s]}`, tt.row)
+				assert.NoError(t, err)
+			}))
+			t.Cleanup(server.Close)
+			sdk := newFixtureSavingsPlanClient(t, server)
+			client := func(sub string) *Client {
+				c := NewClient(nil, sub, "eastus")
+				c.SetListAllPager(sdk.NewListAllPager(nil))
+				return c
+			}
+			ownedA, err := client("sub-a").GetExistingCommitments(context.Background())
+			require.NoError(t, err)
+			assert.Empty(t, ownedA)
+			ownedB, err := client("sub-b").GetExistingCommitments(context.Background())
+			require.NoError(t, err)
+			require.Len(t, ownedB, 1)
+			assert.Equal(t, "sub-b", ownedB[0].Account)
+
+			recs := []common.Recommendation{{Provider: common.ProviderAzure, Service: common.ServiceSavingsPlansAll, ResourceType: "Compute", Count: 1}}
+			passed, filtered, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, coveringView{client("sub-a")})
+			require.NoError(t, err)
+			if tt.wantSuppress {
+				assert.Empty(t, passed)
+				assert.Equal(t, recs, filtered)
+			} else {
+				assert.Equal(t, recs, passed)
+				assert.Empty(t, filtered)
+			}
+		})
+	}
+}
+
+func mustConvertSavingsPlan(t *testing.T, sp *armbillingbenefits.SavingsPlanModel, sub string) common.Commitment {
+	t.Helper()
+	c, err := convertSavingsPlan(sp, sub)
+	require.NoError(t, err)
+	return *c
+}