diff --git a/pkg/recfilter/dedupe.go b/pkg/recfilter/dedupe.go index 0bd528ec..4d8e106d 100644 --- a/pkg/recfilter/dedupe.go +++ b/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 { 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 diff --git a/pkg/recfilter/dedupe_test.go b/pkg/recfilter/dedupe_test.go index 69bbcc8d..c5ec1a5c 100644 --- a/pkg/recfilter/dedupe_test.go +++ b/pkg/recfilter/dedupe_test.go @@ -633,3 +633,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) +} diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index c1aaead6..b862c103 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -143,6 +143,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 { @@ -169,7 +176,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) @@ -183,7 +190,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) @@ -192,13 +199,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 diff --git a/providers/azure/services/savingsplans/client_completeness_sdk_test.go b/providers/azure/services/savingsplans/client_completeness_sdk_test.go index 95a8c1c0..06c2b32b 100644 --- a/providers/azure/services/savingsplans/client_completeness_sdk_test.go +++ b/providers/azure/services/savingsplans/client_completeness_sdk_test.go @@ -52,7 +52,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) diff --git a/providers/azure/services/savingsplans/scope.go b/providers/azure/services/savingsplans/scope.go index da0a7ea9..af945158 100644 --- a/providers/azure/services/savingsplans/scope.go +++ b/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") diff --git a/providers/azure/services/savingsplans/scope_test.go b/providers/azure/services/savingsplans/scope_test.go index 6409ec67..df648a91 100644 --- a/providers/azure/services/savingsplans/scope_test.go +++ b/providers/azure/services/savingsplans/scope_test.go @@ -185,3 +185,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 +}