diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index b1005c83..a4235d0b 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -4,6 +4,7 @@ package savingsplans import ( "context" "fmt" + "log" "net/http" "net/url" "regexp" @@ -135,7 +136,10 @@ func (c *Client) GetRecommendations(ctx context.Context, params *common.Recommen // never terminates fails loud instead of looping forever. const maxSavingsPlanPages = 100 -// GetExistingCommitments retrieves the Azure Savings Plans billed to this client's subscription. +// GetExistingCommitments retrieves the Azure Savings Plans billed to this +// client's subscription, plus Single-scope plans billed to a billing account +// 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) { var pager SavingsPlanListAllPager @@ -150,6 +154,7 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen } commitments := make([]common.Commitment, 0) + skippedSingle := 0 for pageIndex := 1; pager.More(); pageIndex++ { if err := ctx.Err(); err != nil { return nil, fmt.Errorf("failed to list savings plans: context canceled during pagination: %w", err) @@ -161,28 +166,36 @@ func (c *Client) GetExistingCommitments(ctx context.Context) ([]common.Commitmen if err != nil { return nil, fmt.Errorf("failed to list savings plans: %w", err) } - commitments, err = c.appendOwnedPlans(commitments, page.Value) + var skipped int + commitments, skipped, err = c.appendOwnedPlans(commitments, page.Value) + skippedSingle += skipped if err != nil { return nil, fmt.Errorf("incomplete savings plan inventory for subscription %s, page %d: %w", c.subscriptionID, pageIndex, err) } } + if skippedSingle > 0 { + log.Printf("WARNING: skipped %d single-scope savings plans applied to other subscriptions", skippedSingle) + } return commitments, nil } const subscriptionScopePrefix = "/subscriptions/" -func (c *Client) appendOwnedPlans(dst []common.Commitment, plans []*armbillingbenefits.SavingsPlanModel) ([]common.Commitment, error) { - ownScope := c.billingScopeID() +func (c *Client) appendOwnedPlans(dst []common.Commitment, plans []*armbillingbenefits.SavingsPlanModel) ([]common.Commitment, int, error) { + skipped := 0 for i, sp := range plans { - owned, err := planOwnedBy(sp, ownScope) + disposition, err := planOwnedBy(sp, c.subscriptionID) if err != nil { - return nil, fmt.Errorf("row %d: %w", i+1, err) + return nil, 0, fmt.Errorf("row %d: %w", i+1, err) } - if owned { + switch disposition { + case planOwned: dst = append(dst, *convertSavingsPlan(sp, c.subscriptionID)) + case planSkippedSingle: + skipped++ } } - return dst, nil + return dst, skipped, nil } func hasPrefixFold(s, prefix string) bool { diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index dbacf2d7..c825c5c5 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -214,8 +214,8 @@ func TestGetExistingCommitments_SubscriptionOwnership(t *testing.T) { {"foreign-empty-id", plan("", "/subscriptions/sub-b", "", ""), false, false}, {"prefix-lookalike", plan("foreign", "/subscriptions/sub-a-2", "", ""), false, false}, {"account-shared", plan("shared", accountScope, armbillingbenefits.AppliedScopeTypeShared, ""), false, true}, - {"account-single-ours", plan("single", accountScope, armbillingbenefits.AppliedScopeTypeSingle, "/subscriptions/sub-a"), false, true}, - {"account-single-other", plan("single", accountScope, armbillingbenefits.AppliedScopeTypeSingle, "/subscriptions/sub-b"), false, true}, + {"account-single-ours", plan("single", accountScope, armbillingbenefits.AppliedScopeTypeSingle, "/subscriptions/sub-a"), true, false}, + {"account-single-other", plan("single", accountScope, armbillingbenefits.AppliedScopeTypeSingle, "/subscriptions/sub-b"), false, false}, {"account-management-group", plan("group", accountScope, armbillingbenefits.AppliedScopeTypeManagementGroup, ""), false, true}, {"nil-row", nil, false, true}, {"missing-properties", &armbillingbenefits.SavingsPlanModel{ID: toPtr("missing")}, false, true}, diff --git a/providers/azure/services/savingsplans/scope.go b/providers/azure/services/savingsplans/scope.go index e8ea534f..da0a7ea9 100644 --- a/providers/azure/services/savingsplans/scope.go +++ b/providers/azure/services/savingsplans/scope.go @@ -3,29 +3,178 @@ package savingsplans import ( "fmt" "strings" + "unicode" "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits" ) -func planOwnedBy(sp *armbillingbenefits.SavingsPlanModel, ownScope string) (bool, error) { +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 +) + +const billingAccountPrefix = "/providers/Microsoft.Billing/billingAccounts/" + +// planOwnedBy decides whether a plan belongs to ownSub's inventory. Plans billed +// to a subscription are owned by that subscription. Plans billed to a billing +// account are attributed only when AppliedScopeType is Single and names one +// subscription; Shared, management-group and unknown scopes fail closed. +func planOwnedBy(sp *armbillingbenefits.SavingsPlanModel, ownSub string) (planDisposition, error) { if sp == nil || sp.Properties == nil || sp.Properties.BillingScopeID == nil { - return false, fmt.Errorf("missing billing scope") + return planNotOurs, fmt.Errorf("missing billing scope") } billingScope := *sp.Properties.BillingScopeID - if !hasPrefixFold(billingScope, subscriptionScopePrefix) { - return false, fmt.Errorf("billing scope does not identify a subscription") + switch { + case hasPrefixFold(billingScope, subscriptionScopePrefix): + return subscriptionBilledPlan(sp, billingScope, ownSub) + case isBillingAccountScope(billingScope): + return accountBilledPlan(sp, ownSub) } - subscription := billingScope[len(subscriptionScopePrefix):] - if subscription == "" || strings.IndexFunc(subscription, func(r rune) bool { - return !strings.ContainsRune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-", r) - }) >= 0 { - return false, fmt.Errorf("malformed billing subscription scope") + return planNotOurs, fmt.Errorf("billing scope does not identify a subscription") +} + +func subscriptionBilledPlan(sp *armbillingbenefits.SavingsPlanModel, billingScope, ownSub string) (planDisposition, error) { + if !isSubscriptionSegment(billingScope[len(subscriptionScopePrefix):]) { + return planNotOurs, fmt.Errorf("malformed billing subscription scope") } - if !strings.EqualFold(billingScope, ownScope) { - return false, nil + if !strings.EqualFold(billingScope, subscriptionScopePrefix+ownSub) { + return planNotOurs, nil } + return ownedWithID(sp) +} + +func ownedWithID(sp *armbillingbenefits.SavingsPlanModel) (planDisposition, error) { if sp.ID == nil || strings.TrimSpace(*sp.ID) == "" { - return false, fmt.Errorf("owned savings plan has no ID") + return planNotOurs, fmt.Errorf("owned savings plan has no ID") + } + return planOwned, nil +} + +// 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. +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") + } + applied, err := singleScopeSubscription(sp.Properties.AppliedScopeProperties) + if err != nil { + return planNotOurs, err + } + if !strings.EqualFold(applied, ownSub) { + return planSkippedSingle, nil + } + return ownedWithID(sp) +} + +// singleScopeSubscription resolves the one subscription a Single scope names, +// from SubscriptionID or ResourceGroupID. Errors never echo the scope strings. +func singleScopeSubscription(props *armbillingbenefits.AppliedScopeProperties) (string, error) { + if props == nil { + return "", fmt.Errorf("single-scope plan has no applied scope properties") + } + if props.ManagementGroupID != nil && strings.TrimSpace(*props.ManagementGroupID) != "" { + return "", fmt.Errorf("single-scope plan names a management group") + } + fromSub, err := appliedSubscription(props.SubscriptionID) + if err != nil { + return "", err + } + fromRG, err := appliedResourceGroupSubscription(props.ResourceGroupID) + if err != nil { + return "", err + } + return agreeingSubscription(fromSub, fromRG) +} + +// agreeingSubscription picks the one subscription named by the SubscriptionID +// and ResourceGroupID fields ("" means absent); two names must agree. +func agreeingSubscription(fromSub, fromRG string) (string, error) { + switch { + case fromSub == "" && fromRG == "": + return "", fmt.Errorf("single-scope plan names no subscription") + case fromSub != "" && fromRG != "" && !strings.EqualFold(fromSub, fromRG): + return "", fmt.Errorf("applied subscription and resource group scopes disagree") + case fromSub == "": + return fromRG, nil + } + return fromSub, nil +} + +func appliedResourceGroupSubscription(id *string) (string, error) { + if id == nil { + return "", nil + } + sub, ok := resourceGroupSubscription(*id) + if !ok { + return "", fmt.Errorf("malformed applied resource group scope") + } + return sub, nil +} + +// appliedSubscription parses exactly /subscriptions/. A nil or blank value +// counts as absent and yields "". +func appliedSubscription(id *string) (string, error) { + if id == nil || strings.TrimSpace(*id) == "" { + return "", nil + } + rest, ok := cutPrefixFold(*id, subscriptionScopePrefix) + if !ok || !isSubscriptionSegment(rest) { + return "", fmt.Errorf("malformed applied subscription scope") + } + return rest, nil +} + +// resourceGroupSubscription parses exactly /subscriptions//resourceGroups/. +func resourceGroupSubscription(id string) (string, bool) { + rest, ok := cutPrefixFold(id, subscriptionScopePrefix) + if !ok { + return "", false + } + parts := strings.Split(rest, "/") + if len(parts) != 3 || !strings.EqualFold(parts[1], "resourceGroups") || + !isSubscriptionSegment(parts[0]) || !isOpaqueID(parts[2]) { + return "", false + } + return parts[0], true +} + +// isBillingAccountScope accepts exactly the EA shape (billingAccounts/{id}) and +// the MCA shape (billingAccounts/{id}/billingProfiles/{id}). MCA ids contain +// ':' and '_', so ids are opaque rather than alphanumeric-dash. +func isBillingAccountScope(scope string) bool { + rest, ok := cutPrefixFold(scope, billingAccountPrefix) + if !ok { + return false + } + parts := strings.Split(rest, "/") + switch len(parts) { + case 1: + return isOpaqueID(parts[0]) + case 3: + return isOpaqueID(parts[0]) && strings.EqualFold(parts[1], "billingProfiles") && isOpaqueID(parts[2]) + } + return false +} + +func isOpaqueID(s string) bool { + return s != "" && strings.IndexFunc(s, func(r rune) bool { + return r == '/' || unicode.IsSpace(r) || unicode.IsControl(r) + }) < 0 +} + +func isSubscriptionSegment(s string) bool { + return s != "" && strings.IndexFunc(s, func(r rune) bool { + return !strings.ContainsRune("abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ0123456789-", r) + }) < 0 +} + +func cutPrefixFold(s, prefix string) (string, bool) { + if !hasPrefixFold(s, prefix) { + return "", false } - return true, nil + return s[len(prefix):], true } diff --git a/providers/azure/services/savingsplans/scope_test.go b/providers/azure/services/savingsplans/scope_test.go new file mode 100644 index 00000000..dbbe35b1 --- /dev/null +++ b/providers/azure/services/savingsplans/scope_test.go @@ -0,0 +1,187 @@ +package savingsplans + +import ( + "bytes" + "context" + "fmt" + "log" + "net/http" + "net/http/httptest" + "os" + "testing" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/billingbenefits/armbillingbenefits" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/pkg/recfilter" +) + +const ( + eaScope = "/providers/Microsoft.Billing/billingAccounts/ea-SECRET" + mcaScope = "/providers/Microsoft.Billing/billingAccounts/mca-SECRET:1_x/billingProfiles/BP:1_x" +) + +func accountPlan(scope string, typ *armbillingbenefits.AppliedScopeType, props *armbillingbenefits.AppliedScopeProperties) *armbillingbenefits.SavingsPlanModel { + return &armbillingbenefits.SavingsPlanModel{ID: toPtr("plan-id"), Properties: &armbillingbenefits.SavingsPlanModelProperties{BillingScopeID: toPtr(scope), AppliedScopeType: typ, AppliedScopeProperties: props}} +} + +// scoped builds applied-scope properties; "" leaves the field nil. +func scoped(subscription, resourceGroup string) *armbillingbenefits.AppliedScopeProperties { + p := &armbillingbenefits.AppliedScopeProperties{} + if subscription != "" { + p.SubscriptionID = toPtr(subscription) + } + if resourceGroup != "" { + p.ResourceGroupID = toPtr(resourceGroup) + } + return p +} + +func TestPlanOwnedBy_BillingAccountScopes(t *testing.T) { + single := toPtr(armbillingbenefits.AppliedScopeTypeSingle) + sub := func(s string) *armbillingbenefits.AppliedScopeProperties { return scoped(s, "") } + rg := func(s string) *armbillingbenefits.AppliedScopeProperties { return scoped("", s) } + both := scoped + cases := []struct { + name string + scope string + typ *armbillingbenefits.AppliedScopeType + props *armbillingbenefits.AppliedScopeProperties + want planDisposition // planNotOurs means an error is expected + }{ + {"ea-sub-ours", eaScope, single, sub("/subscriptions/sub-a"), planOwned}, + {"mca-sub-ours", mcaScope, single, sub("/Subscriptions/SUB-A"), planOwned}, + {"mca-sub-other", mcaScope, single, sub("/subscriptions/sub-b"), planSkippedSingle}, + {"mixed-case-profile-scope", "/providers/MICROSOFT.billing/BILLINGACCOUNTS/x/BillingProfiles/y", single, sub("/subscriptions/sub-a"), planOwned}, + {"mixed-case-rg-literal", eaScope, single, rg("/Subscriptions/sub-a/RESOURCEGROUPS/rg"), planOwned}, + {"rg-ours", eaScope, single, rg("/subscriptions/sub-a/resourceGroups/rg1"), planOwned}, + {"rg-other", eaScope, single, rg("/subscriptions/sub-b/resourceGroups/rg1"), planSkippedSingle}, + {"sub-rg-agree", eaScope, single, both("/subscriptions/sub-a", "/subscriptions/sub-a/resourceGroups/rg1"), planOwned}, + {"sub-rg-disagree", eaScope, single, both("/subscriptions/sub-a", "/subscriptions/sub-b/resourceGroups/rg1"), planNotOurs}, + {"blank-sub-valid-rg", eaScope, single, both(" ", "/subscriptions/sub-a/resourceGroups/rg1"), planOwned}, + {"blank-sub-no-rg", eaScope, single, sub(" "), planNotOurs}, + {"nil-properties", eaScope, single, nil, planNotOurs}, + {"nil-type", eaScope, nil, sub("/subscriptions/sub-a"), planNotOurs}, + {"unknown-type", eaScope, toPtr(armbillingbenefits.AppliedScopeType("Bogus")), sub("/subscriptions/sub-a"), planNotOurs}, + {"shared-with-sub", eaScope, toPtr(armbillingbenefits.AppliedScopeTypeShared), sub("/subscriptions/sub-a"), planNotOurs}, + {"mg-id-set", eaScope, single, &armbillingbenefits.AppliedScopeProperties{SubscriptionID: toPtr("/subscriptions/sub-a"), ManagementGroupID: toPtr("/providers/Microsoft.Management/managementGroups/mg")}, planNotOurs}, + {"mg-id-blank-ignored", eaScope, single, &armbillingbenefits.AppliedScopeProperties{SubscriptionID: toPtr("/subscriptions/sub-a"), ManagementGroupID: toPtr(" ")}, planOwned}, + {"tenant-and-name-ignored", eaScope, single, &armbillingbenefits.AppliedScopeProperties{SubscriptionID: toPtr("/subscriptions/sub-a"), TenantID: toPtr("t"), DisplayName: toPtr("n")}, planOwned}, + {"sub-with-suffix", eaScope, single, sub("/subscriptions/sub-a/resourceGroups/rg"), planNotOurs}, + {"sub-trailing-slash", eaScope, single, sub("/subscriptions/sub-a/"), planNotOurs}, + {"rg-providers-tail", eaScope, single, rg("/subscriptions/sub-a/resourceGroups/rg/providers/x/y"), planNotOurs}, + {"rg-trailing-slash", eaScope, single, rg("/subscriptions/sub-a/resourceGroups/rg/"), planNotOurs}, + {"rg-empty-name", eaScope, single, rg("/subscriptions/sub-a/resourceGroups/"), planNotOurs}, + {"rg-whitespace", eaScope, single, rg("/subscriptions/sub-a/resourceGroups/r g"), planNotOurs}, + {"rg-control", eaScope, single, rg("/subscriptions/sub-a/resourceGroups/r\x00g"), planNotOurs}, + {"rg-blank-non-nil", eaScope, single, rg(" "), planNotOurs}, + {"account-foo-suffix", "/providers/Microsoft.Billing/billingAccounts/x/foo", single, sub("/subscriptions/sub-a"), planNotOurs}, + {"account-trailing-slash", eaScope + "/", single, sub("/subscriptions/sub-a"), planNotOurs}, + {"account-empty-id", "/providers/Microsoft.Billing/billingAccounts/", single, sub("/subscriptions/sub-a"), planNotOurs}, + {"account-empty-profile", "/providers/Microsoft.Billing/billingAccounts/x/billingProfiles/", single, sub("/subscriptions/sub-a"), planNotOurs}, + {"account-whitespace", "/providers/Microsoft.Billing/billingAccounts/x y", single, sub("/subscriptions/sub-a"), planNotOurs}, + } + for _, tt := range cases { + t.Run(tt.name, func(t *testing.T) { + got, err := planOwnedBy(accountPlan(tt.scope, tt.typ, tt.props), "sub-a") + if tt.want == planNotOurs { + require.Error(t, err) + for _, leak := range []string{"SECRET", "billingAccounts", "sub-b"} { + assert.NotContains(t, err.Error(), leak) + } + return + } + require.NoError(t, err) + assert.Equal(t, tt.want, got) + }) + } + + for name, want := range map[string]planDisposition{"ours": planNotOurs, "other": planSkippedSingle} { + t.Run(name+"-without-id", func(t *testing.T) { + s := "/subscriptions/sub-a" + if name == "other" { + s = "/subscriptions/sub-b" + } + sp := accountPlan(eaScope, single, sub(s)) + sp.ID = nil + got, err := planOwnedBy(sp, "sub-a") + assert.Equal(t, want, got) + assert.Equal(t, want == planNotOurs, err != nil) + }) + } +} + +// Not parallel: it replaces the global log writer. +func TestGetExistingCommitments_AggregateSkipLog(t *testing.T) { + var buf bytes.Buffer + log.SetOutput(&buf) + defer log.SetOutput(os.Stderr) + + single := toPtr(armbillingbenefits.AppliedScopeTypeSingle) + other, ours := scoped("/subscriptions/sub-b", ""), scoped("/subscriptions/sub-a", "") + run := func(rows ...*armbillingbenefits.SavingsPlanModel) { + buf.Reset() + c := NewClient(nil, "sub-a", "eastus") + c.SetListAllPager(&mockListAllPager{results: rows}) + _, err := c.GetExistingCommitments(context.Background()) + require.NoError(t, err) + } + + run(accountPlan(eaScope, single, other), accountPlan(mcaScope, single, other), accountPlan(eaScope, single, ours)) + assert.Equal(t, 1, bytes.Count(buf.Bytes(), []byte("skipped 2 single-scope savings plans applied to other subscriptions"))) + assert.NotContains(t, buf.String(), "SECRET") + assert.NotContains(t, buf.String(), "plan-id") + + run(accountPlan(eaScope, single, ours)) + assert.NotContains(t, buf.String(), "skipped") +} + +func TestGetExistingCommitments_SDKBillingAccountSingleScope(t *testing.T) { + row := func(id, scope, applied, props string) string { + return fmt.Sprintf(`{"id":%q,"name":"Compute","properties":{"billingScopeId":%q,"appliedScopeType":%q,"appliedScopeProperties":%s,"provisioningState":"Succeeded","effectiveDateTime":%q}}`, + id, scope, applied, props, time.Now().UTC().Format(time.RFC3339)) + } + cases := []struct { + name string + row string + wantOwned bool + wantErr bool + }{ + {"ea-ours", row("p1", eaScope, "Single", `{"subscriptionId":"/subscriptions/sub-a","tenantId":"t","displayName":"d"}`), true, false}, + {"mca-rg-ours", row("p1", mcaScope, "Single", `{"resourceGroupId":"/subscriptions/sub-a/resourceGroups/rg"}`), true, false}, + {"ea-other", row("p1", eaScope, "Single", `{"subscriptionId":"/subscriptions/sub-b"}`), false, false}, + {"single-no-subscription", row("p1", eaScope, "Single", `{}`), false, true}, + {"shared", row("p1", eaScope, "Shared", `{}`), false, true}, + } + 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) + client := NewClient(nil, "sub-a", "eastus") + client.SetListAllPager(newFixtureSavingsPlanClient(t, server).NewListAllPager(nil)) + recs := []common.Recommendation{{Provider: common.ProviderAzure, Service: common.ServiceSavingsPlansAll, ResourceType: "Compute", Count: 1}} + passed, filtered, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, client) + if tt.wantErr { + require.ErrorContains(t, err, "incomplete savings plan inventory") + assert.Equal(t, recs, passed) + assert.Empty(t, filtered) + return + } + require.NoError(t, err) + if tt.wantOwned { + assert.Empty(t, passed) + assert.Len(t, filtered, 1) + } else { + assert.Equal(t, recs, passed) + assert.Empty(t, filtered) + } + }) + } +}