From 05ff804d3af86582ac932329c45aa4e9f52a357f Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 20:49:46 +0200 Subject: [PATCH 1/2] feat(azure/savingsplans): source Compute savings plan recommendations from benefitRecommendations Replaces the empty stub with Shared-scope recommendations at subscription scope (Cost Management benefitRecommendations, api-version 2024-08-01). Honours params.Term and params.LookbackPeriod, follows nextLink to the ARM host only, and fails loud on unknown values. Co-Authored-By: Claude Opus 5.5 --- .../savingsplans/benefit_recommendations.go | 301 ++++++++++++++++++ .../benefit_recommendations_test.go | 186 +++++++++++ .../azure/services/savingsplans/client.go | 27 +- .../services/savingsplans/client_test.go | 7 - 4 files changed, 494 insertions(+), 27 deletions(-) create mode 100644 providers/azure/services/savingsplans/benefit_recommendations.go create mode 100644 providers/azure/services/savingsplans/benefit_recommendations_test.go diff --git a/providers/azure/services/savingsplans/benefit_recommendations.go b/providers/azure/services/savingsplans/benefit_recommendations.go new file mode 100644 index 0000000..781aec8 --- /dev/null +++ b/providers/azure/services/savingsplans/benefit_recommendations.go @@ -0,0 +1,301 @@ +package savingsplans + +import ( + "context" + "encoding/json" + "fmt" + "io" + "net/http" + "net/url" + "strings" + "time" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +// Cost Management Benefit Recommendations - List, api-version 2024-08-01: +// https://learn.microsoft.com/rest/api/cost-management/benefit-recommendations/list?view=rest-cost-management-2024-08-01 +// +// Scope: this client requests at subscription scope with +// properties/scope eq 'Shared', the same applied scope PurchaseCommitment +// uses. Billing-account and billing-profile scopes need an ID this client +// does not have, and Single-scope rows would need a per-subscription purchase +// path that does not exist. +const ( + benefitRecsAPIVersion = "2024-08-01" + armHost = "management.azure.com" + armEndpoint = "https://" + armHost + benefitRecsMaxPages = 50 + benefitRecsPageTO = 30 * time.Second + + armSkuComputeSavingsPlan = "Compute_Savings_Plan" + kindSavingsPlan = "SavingsPlan" + scopeShared = "Shared" + grainHourly = "Hourly" + + // hoursPerMonth matches the 730 h/month used by the other Azure + // recommendation converters for monthly figures. + hoursPerMonth = 730.0 +) + +type benefitRecsPage struct { + Value []benefitRecommendation `json:"value"` + NextLink string `json:"nextLink"` +} + +type benefitRecommendation struct { + ID string `json:"id"` + Kind string `json:"kind"` + Properties *benefitRecommendationProps `json:"properties"` +} + +type benefitRecommendationProps struct { + ArmSkuName string `json:"armSkuName"` + Term string `json:"term"` + Scope string `json:"scope"` + CommitmentGranularity string `json:"commitmentGranularity"` + CurrencyCode string `json:"currencyCode"` + TotalHours *int32 `json:"totalHours"` + CostWithoutBenefit *float64 `json:"costWithoutBenefit"` + RecommendationDetails *benefitRecommendDetls `json:"recommendationDetails"` +} + +type benefitRecommendDetls struct { + CommitmentAmount *float64 `json:"commitmentAmount"` + SavingsAmount *float64 `json:"savingsAmount"` + SavingsPct *float64 `json:"savingsPercentage"` +} + +// benefitLookback maps RecommendationParams.LookbackPeriod to the API enum. +// An empty value is omitted from the filter (API default Last60Days). +func benefitLookback(p string) (string, error) { + switch strings.ToLower(strings.TrimSpace(p)) { + case "": + return "", nil + case "7d": + return "Last7Days", nil + case "30d": + return "Last30Days", nil + case "60d": + return "Last60Days", nil + default: + return "", fmt.Errorf("unsupported lookback period %q for Azure savings plan recommendations (want 7d, 30d or 60d)", p) + } +} + +// benefitTerms returns the API terms to request. An empty term requests both +// P1Y and P3Y because the API default (P3Y) would silently drop 1-year rows. +func benefitTerms(t string) ([]string, error) { + switch strings.ToLower(strings.TrimSpace(t)) { + case "": + return []string{"P1Y", "P3Y"}, nil + case "1", "1yr", "1y", "p1y": + return []string{"P1Y"}, nil + case "3", "3yr", "3y", "p3y": + return []string{"P3Y"}, nil + default: + return nil, fmt.Errorf("unsupported term %q for Azure savings plan recommendations (the API supports 1yr and 3yr)", t) + } +} + +func (c *Client) benefitRecsURL(term, lookback string) string { + filter := "properties/scope eq 'Shared' AND properties/term eq '" + term + "'" + if lookback != "" { + filter += " AND properties/lookBackPeriod eq '" + lookback + "'" + } + q := url.Values{} + q.Set("$filter", filter) + q.Set("api-version", benefitRecsAPIVersion) + return fmt.Sprintf("%s/subscriptions/%s/providers/Microsoft.CostManagement/benefitRecommendations?%s", + armEndpoint, url.PathEscape(c.subscriptionID), q.Encode()) +} + +// getBenefitRecommendations fetches Shared-scope Compute Savings Plan +// recommendations for the client's subscription. +func (c *Client) getBenefitRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { + if params == nil { + params = &common.RecommendationParams{} + } + if c.cred == nil { + return nil, fmt.Errorf("azure savings plan recommendations require a credential") + } + lookback, err := benefitLookback(params.LookbackPeriod) + if err != nil { + return nil, err + } + terms, err := benefitTerms(params.Term) + if err != nil { + return nil, err + } + token, err := c.cred.GetToken(ctx, policy.TokenRequestOptions{Scopes: []string{armEndpoint + "/.default"}}) + if err != nil { + return nil, fmt.Errorf("failed to get access token: %w", err) + } + + recs := make([]common.Recommendation, 0) + for _, term := range terms { + rows, err := c.fetchBenefitPages(ctx, token.Token, c.benefitRecsURL(term, lookback)) + if err != nil { + return nil, fmt.Errorf("azure savings plan recommendations (term %s): %w", term, err) + } + for i := range rows { + rec, err := convertBenefitRecommendation(&rows[i], c.subscriptionID) + if err != nil { + return nil, fmt.Errorf("azure savings plan recommendation %q (term %s): %w", rows[i].ID, term, err) + } + recs = append(recs, *rec) + } + } + return recs, nil +} + +func (c *Client) fetchBenefitPages(ctx context.Context, bearer, firstURL string) ([]benefitRecommendation, error) { + var all []benefitRecommendation + seen := map[string]struct{}{} + next := firstURL + for pageIdx := 0; next != ""; pageIdx++ { + if pageIdx >= benefitRecsMaxPages { + return nil, fmt.Errorf("result exceeds the %d-page cap with nextLink still set", benefitRecsMaxPages) + } + if _, dup := seen[next]; dup { + return nil, fmt.Errorf("self-referential nextLink (page %d)", pageIdx) + } + seen[next] = struct{}{} + if err := requireARMURL(next); err != nil { + return nil, fmt.Errorf("page %d: %w", pageIdx, err) + } + page, err := c.fetchBenefitPage(ctx, bearer, next, pageIdx) + if err != nil { + return nil, err + } + all = append(all, page.Value...) + next = page.NextLink + } + return all, nil +} + +// requireARMURL stops the bearer token being sent to a host the response +// named, since nextLink comes from the server. +func requireARMURL(raw string) error { + u, err := url.Parse(raw) + if err != nil || u.Scheme != "https" || !strings.EqualFold(u.Host, armHost) { + return fmt.Errorf("refusing to follow link outside https://%s", armHost) + } + return nil +} + +func (c *Client) fetchBenefitPage(ctx context.Context, bearer, pageURL string, pageIdx int) (*benefitRecsPage, error) { + pageCtx, cancel := context.WithTimeout(ctx, benefitRecsPageTO) + defer cancel() + req, err := http.NewRequestWithContext(pageCtx, http.MethodGet, pageURL, http.NoBody) + if err != nil { + return nil, fmt.Errorf("page %d: build request: %w", pageIdx, err) + } + req.Header.Set("Authorization", "Bearer "+bearer) + resp, err := c.httpClient.Do(req) + if err != nil { + return nil, fmt.Errorf("page %d: %w", pageIdx, err) + } + defer resp.Body.Close() + body, err := io.ReadAll(resp.Body) + if err != nil { + return nil, fmt.Errorf("page %d: read body: %w", pageIdx, err) + } + if resp.StatusCode != http.StatusOK { + return nil, fmt.Errorf("page %d: status %d: %s", pageIdx, resp.StatusCode, string(body)) + } + var page benefitRecsPage + if err := json.Unmarshal(body, &page); err != nil { + return nil, fmt.Errorf("page %d: decode: %w", pageIdx, err) + } + return &page, nil +} + +func termFromAPI(t string) (string, error) { + switch t { + case "P1Y": + return "1yr", nil + case "P3Y": + return "3yr", nil + default: + return "", fmt.Errorf("unknown term %q", t) + } +} + +// convertBenefitRecommendation maps one API row to a Recommendation. The API +// reports cost and savings over totalHours of look-back usage; they are +// scaled to a 730 h month. Savings plans bill monthly with no upfront payment +// (CommitmentCost 0; RecurringMonthlyCost = hourly commitment x 730). +func convertBenefitRecommendation(r *benefitRecommendation, subscriptionID string) (*common.Recommendation, error) { + term, err := validateBenefitRow(r) + if err != nil { + return nil, err + } + p, d := r.Properties, r.Properties.RecommendationDetails + monthly := hoursPerMonth / float64(*p.TotalHours) + recurring := *d.CommitmentAmount * hoursPerMonth + rec := &common.Recommendation{ + Provider: common.ProviderAzure, + Account: subscriptionID, + Service: common.ServiceSavingsPlansAll, + ResourceType: "Compute", + Count: 1, + CommitmentType: common.CommitmentSavingsPlan, + Term: term, + PaymentOption: "monthly", + OnDemandCost: *p.CostWithoutBenefit * monthly, + CommitmentCost: 0, + EstimatedSavings: *d.SavingsAmount * monthly, + RecurringMonthlyCost: &recurring, + SourceRecommendation: r.ID, + Timestamp: time.Now(), + Details: &common.SavingsPlanDetails{ + PlanType: "Compute", + HourlyCommitment: *d.CommitmentAmount, + Currency: strings.ToUpper(p.CurrencyCode), + }, + } + if d.SavingsPct != nil { + rec.SavingsPercentage = *d.SavingsPct + } + return rec, nil +} + +// validateBenefitRow rejects any row whose identity or numbers cannot be +// mapped faithfully, and returns the CUDly term. +func validateBenefitRow(r *benefitRecommendation) (string, error) { + p := r.Properties + switch { + case r.Kind != kindSavingsPlan: + return "", fmt.Errorf("unexpected kind %q", r.Kind) + case p == nil || p.RecommendationDetails == nil: + return "", fmt.Errorf("missing properties.recommendationDetails") + case p.ArmSkuName != armSkuComputeSavingsPlan: + return "", fmt.Errorf("unknown armSkuName %q", p.ArmSkuName) + case p.Scope != scopeShared: + return "", fmt.Errorf("unexpected scope %q (only Shared is requested)", p.Scope) + case p.CommitmentGranularity != grainHourly: + return "", fmt.Errorf("unsupported commitmentGranularity %q", p.CommitmentGranularity) + case !currencyCodePattern.MatchString(p.CurrencyCode): + return "", fmt.Errorf("currencyCode %q is not a 3-letter code", p.CurrencyCode) + } + if err := validateBenefitNumbers(p); err != nil { + return "", err + } + return termFromAPI(p.Term) +} + +func validateBenefitNumbers(p *benefitRecommendationProps) error { + d := p.RecommendationDetails + switch { + case d.CommitmentAmount == nil || *d.CommitmentAmount <= 0: + return fmt.Errorf("missing or non-positive commitmentAmount") + case d.SavingsAmount == nil || p.CostWithoutBenefit == nil: + return fmt.Errorf("missing savingsAmount or costWithoutBenefit") + case p.TotalHours == nil || *p.TotalHours <= 0: + return fmt.Errorf("missing or non-positive totalHours") + } + return nil +} diff --git a/providers/azure/services/savingsplans/benefit_recommendations_test.go b/providers/azure/services/savingsplans/benefit_recommendations_test.go new file mode 100644 index 0000000..efecb4a --- /dev/null +++ b/providers/azure/services/savingsplans/benefit_recommendations_test.go @@ -0,0 +1,186 @@ +package savingsplans + +import ( + "context" + "errors" + "io" + "net/http" + "net/url" + "strings" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/azcore" + "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" + + "github.com/LeanerCloud/cloud-commitments-go/pkg/common" +) + +type fakeCred struct{ err error } + +func (f fakeCred) GetToken(context.Context, policy.TokenRequestOptions) (azcore.AccessToken, error) { + return azcore.AccessToken{Token: "tok"}, f.err +} + +type routeHTTP struct { + reqs []*http.Request + reply func(r *http.Request) (int, string) +} + +func (h *routeHTTP) Do(r *http.Request) (*http.Response, error) { + h.reqs = append(h.reqs, r) + code, body := h.reply(r) + return &http.Response{StatusCode: code, Body: io.NopCloser(strings.NewReader(body))}, nil +} + +// Fixture-based: shape follows the documented 2024-08-01 response +// (benefitRecommendationModel, sharedScopeBenefitRecommendationProperties). +// The live probe of the test subscription returned {"value": []}. +func benefitRow(term string, mutate func(string) string) string { + row := `{"id":"/subscriptions/sub-1/providers/Microsoft.CostManagement/benefitRecommendations/r-` + term + `", +"name":"r","type":"Microsoft.CostManagement/benefitRecommendations","kind":"SavingsPlan", +"properties":{"firstConsumptionDate":"2026-08-01T00:00:00Z","lastConsumptionDate":"2026-10-01T00:00:00Z", +"lookBackPeriod":"Last60Days","totalHours":1460,"armSkuName":"Compute_Savings_Plan","term":"` + term + `", +"commitmentGranularity":"Hourly","currencyCode":"USD","costWithoutBenefit":2920,"scope":"Shared", +"recommendationDetails":{"commitmentAmount":1.5,"savingsAmount":292,"savingsPercentage":10,"totalCost":2628}}}` + if mutate != nil { + row = mutate(row) + } + return row +} + +func newTestClient(h HTTPClient) *Client { return NewClientWithHTTP(fakeCred{}, "sub-1", "", h) } + +func TestGetRecommendations_MapsRow(t *testing.T) { + h := &routeHTTP{reply: func(*http.Request) (int, string) { + return 200, `{"value":[` + benefitRow("P1Y", nil) + `]}` + }} + recs, err := newTestClient(h).GetRecommendations(context.Background(), &common.RecommendationParams{Term: "1yr", LookbackPeriod: "30d"}) + require.NoError(t, err) + require.Len(t, recs, 1) + r := recs[0] + assert.Equal(t, "1yr", r.Term) + assert.Equal(t, "monthly", r.PaymentOption) + assert.Equal(t, 0.0, r.CommitmentCost) + require.NotNil(t, r.RecurringMonthlyCost) + assert.InDelta(t, 1.5*730, *r.RecurringMonthlyCost, 1e-9) + assert.InDelta(t, 292*730.0/1460, r.EstimatedSavings, 1e-9) + assert.InDelta(t, 2920*730.0/1460, r.OnDemandCost, 1e-9) + d := r.Details.(*common.SavingsPlanDetails) + assert.Equal(t, 1.5, d.HourlyCommitment) + assert.Equal(t, "USD", d.Currency) + assert.Equal(t, common.ServiceSavingsPlansAll, r.Service) + + require.Len(t, h.reqs, 1) + q := h.reqs[0].URL.Query() + assert.Equal(t, "2024-08-01", q.Get("api-version")) + assert.Equal(t, "properties/scope eq 'Shared' AND properties/term eq 'P1Y' AND properties/lookBackPeriod eq 'Last30Days'", q.Get("$filter")) + assert.Equal(t, "Bearer tok", h.reqs[0].Header.Get("Authorization")) +} + +func TestGetRecommendations_EmptyTermRequestsBoth(t *testing.T) { + h := &routeHTTP{reply: func(r *http.Request) (int, string) { + term := "P1Y" + if strings.Contains(r.URL.RawQuery, "P3Y") || strings.Contains(r.URL.Query().Get("$filter"), "P3Y") { + term = "P3Y" + } + return 200, `{"value":[` + benefitRow(term, nil) + `]}` + }} + recs, err := newTestClient(h).GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.NoError(t, err) + require.Len(t, recs, 2) + assert.Equal(t, "1yr", recs[0].Term) + assert.Equal(t, "3yr", recs[1].Term) + assert.NotContains(t, h.reqs[0].URL.Query().Get("$filter"), "lookBackPeriod") +} + +func TestGetRecommendations_EmptyValue(t *testing.T) { + h := &routeHTTP{reply: func(*http.Request) (int, string) { return 200, `{"value":[]}` }} + recs, err := newTestClient(h).GetRecommendations(context.Background(), &common.RecommendationParams{Term: "3yr"}) + require.NoError(t, err) + assert.Empty(t, recs) +} + +func TestGetRecommendations_Pagination(t *testing.T) { + next := "https://management.azure.com/next?page=2" + h := &routeHTTP{reply: func(r *http.Request) (int, string) { + if r.URL.Query().Get("page") == "2" { + return 200, `{"value":[` + benefitRow("P3Y", nil) + `]}` + } + return 200, `{"value":[` + benefitRow("P3Y", nil) + `],"nextLink":"` + next + `"}` + }} + recs, err := newTestClient(h).GetRecommendations(context.Background(), &common.RecommendationParams{Term: "3yr"}) + require.NoError(t, err) + assert.Len(t, recs, 2) +} + +func TestGetRecommendations_RejectsForeignNextLink(t *testing.T) { + h := &routeHTTP{reply: func(*http.Request) (int, string) { + return 200, `{"value":[],"nextLink":"https://evil.example/steal"}` + }} + _, err := newTestClient(h).GetRecommendations(context.Background(), &common.RecommendationParams{Term: "3yr"}) + require.Error(t, err) + assert.Len(t, h.reqs, 1, "bearer token must not be sent to the foreign host") +} + +func TestGetRecommendations_SelfReferentialNextLink(t *testing.T) { + h := &routeHTTP{reply: func(*http.Request) (int, string) { + return 200, `{"value":[],"nextLink":"https://management.azure.com/loop"}` + }} + _, err := newTestClient(h).GetRecommendations(context.Background(), &common.RecommendationParams{Term: "3yr"}) + require.Error(t, err) +} + +func TestGetRecommendations_Errors(t *testing.T) { + ok := func(*http.Request) (int, string) { return 200, `{"value":[` + benefitRow("P1Y", nil) + `]}` } + cases := []struct { + name string + params common.RecommendationParams + reply func(*http.Request) (int, string) + cred error + }{ + {"http 403", common.RecommendationParams{Term: "1yr"}, func(*http.Request) (int, string) { return 403, `{"error":{"code":"AuthorizationFailed"}}` }, nil}, + {"bad json", common.RecommendationParams{Term: "1yr"}, func(*http.Request) (int, string) { return 200, `{` }, nil}, + {"bad term", common.RecommendationParams{Term: "5yr"}, ok, nil}, + {"bad lookback", common.RecommendationParams{Term: "1yr", LookbackPeriod: "90d"}, ok, nil}, + {"token error", common.RecommendationParams{Term: "1yr"}, ok, errors.New("no token")}, + {"unknown term", common.RecommendationParams{Term: "1yr"}, func(*http.Request) (int, string) { + return 200, `{"value":[` + benefitRow("P5Y", nil) + `]}` + }, nil}, + {"unknown sku", common.RecommendationParams{Term: "1yr"}, func(*http.Request) (int, string) { + return 200, `{"value":[` + benefitRow("P1Y", func(s string) string { return strings.Replace(s, "Compute_Savings_Plan", "Other", 1) }) + `]}` + }, nil}, + {"single scope", common.RecommendationParams{Term: "1yr"}, func(*http.Request) (int, string) { + return 200, `{"value":[` + benefitRow("P1Y", func(s string) string { return strings.Replace(s, `"Shared"`, `"Single"`, 1) }) + `]}` + }, nil}, + {"missing commitment", common.RecommendationParams{Term: "1yr"}, func(*http.Request) (int, string) { + return 200, `{"value":[` + benefitRow("P1Y", func(s string) string { return strings.Replace(s, `"commitmentAmount":1.5,`, "", 1) }) + `]}` + }, nil}, + {"zero hours", common.RecommendationParams{Term: "1yr"}, func(*http.Request) (int, string) { + return 200, `{"value":[` + benefitRow("P1Y", func(s string) string { return strings.Replace(s, `"totalHours":1460`, `"totalHours":0`, 1) }) + `]}` + }, nil}, + {"bad currency", common.RecommendationParams{Term: "1yr"}, func(*http.Request) (int, string) { + return 200, `{"value":[` + benefitRow("P1Y", func(s string) string { return strings.Replace(s, `"USD"`, `""`, 1) }) + `]}` + }, nil}, + } + for _, tc := range cases { + t.Run(tc.name, func(t *testing.T) { + c := NewClientWithHTTP(fakeCred{err: tc.cred}, "sub-1", "", &routeHTTP{reply: tc.reply}) + recs, err := c.GetRecommendations(context.Background(), &tc.params) + require.Error(t, err) + assert.Nil(t, recs) + }) + } +} + +func TestGetRecommendations_NoCredential(t *testing.T) { + _, err := NewClient(nil, "sub", "").GetRecommendations(context.Background(), &common.RecommendationParams{}) + require.Error(t, err) +} + +func TestBenefitRecsURL_PathAndQuery(t *testing.T) { + u, err := url.Parse(newTestClient(nil).benefitRecsURL("P3Y", "")) + require.NoError(t, err) + assert.Equal(t, "/subscriptions/sub-1/providers/Microsoft.CostManagement/benefitRecommendations", u.Path) +} diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index 87c6a6e..8562dc6 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -122,26 +122,13 @@ func (c *Client) GetRegion() string { return c.region } -// GetRecommendations returns an empty slice. -// -// Contract for the future implementation: when this is backed by the Cost -// Management benefitRecommendations API, it must set -// SavingsPlanDetails.Currency from that response's properties.currencyCode. -// There is no billing-account lookup, and an empty currency is never treated -// as USD (PurchaseCommitment, ValidateOffering and GetOfferingDetails reject it). -// -// Azure does not have a stable public API for Savings Plan purchase -// recommendations (the Benefits Recommendations API is still in preview). -// This mirrors the AWS Savings Plans client which also returns empty here and -// delegates to Cost Explorer centrally. -// -// NOTE: because this stub succeeds unconditionally, it is excluded from the -// all-attempted-failed guard in the parent RecommendationsClientAdapter -// (providers/azure/recommendations.go, COR-03). When this method starts -// making real API calls, flip that call site's attempted flag back to the -// params-filter value so its failures count toward the guard. -func (c *Client) GetRecommendations(_ context.Context, _ *common.RecommendationParams) ([]common.Recommendation, error) { - return []common.Recommendation{}, nil +// GetRecommendations returns Shared-scope Compute Savings Plan purchase +// recommendations from the Cost Management Benefit Recommendations API +// (api-version 2024-08-01) at this client's subscription scope. See +// benefit_recommendations.go. Term and LookbackPeriod from params are honoured; +// an unsupported value is an error. +func (c *Client) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { + return c.getBenefitRecommendations(ctx, params) } // GetExistingCommitments retrieves the Azure Savings Plans billed to this client's subscription. diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index 2f425b4..cff8757 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -120,13 +120,6 @@ func TestGetRegion(t *testing.T) { // --- GetRecommendations --- -func TestGetRecommendations_AlwaysEmpty(t *testing.T) { - c := NewClient(nil, "sub", "eastus") - recs, err := c.GetRecommendations(context.Background(), &common.RecommendationParams{}) - require.NoError(t, err) - assert.Empty(t, recs) -} - // --- GetValidResourceTypes --- func TestGetValidResourceTypes(t *testing.T) { From 1ccf62000abbe7694f20dd2f34cb8604ac99c690 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Tue, 13 Oct 2026 21:03:12 +0200 Subject: [PATCH 2/2] fix(azure/savingsplans): send lookback explicitly, share month length, address review Send Last7Days when no lookback is given (never rely on the API default), use recommendations.DaysPerMonth for the monthly scale, reuse ParseTermYears, drop the redundant loop guard (page cap covers it, now tested), test the https check, and fix the misspell lint finding. Co-Authored-By: Claude Opus 5.5 --- .../savingsplans/benefit_recommendations.go | 46 +++++++++---------- .../benefit_recommendations_test.go | 22 ++++++--- .../azure/services/savingsplans/client.go | 2 +- 3 files changed, 38 insertions(+), 32 deletions(-) diff --git a/providers/azure/services/savingsplans/benefit_recommendations.go b/providers/azure/services/savingsplans/benefit_recommendations.go index 781aec8..3cdb704 100644 --- a/providers/azure/services/savingsplans/benefit_recommendations.go +++ b/providers/azure/services/savingsplans/benefit_recommendations.go @@ -13,6 +13,8 @@ import ( "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/LeanerCloud/cloud-commitments-go/pkg/common" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/internal/recommendations" + "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/internal/reservations" ) // Cost Management Benefit Recommendations - List, api-version 2024-08-01: @@ -35,9 +37,14 @@ const ( scopeShared = "Shared" grainHourly = "Hourly" - // hoursPerMonth matches the 730 h/month used by the other Azure - // recommendation converters for monthly figures. - hoursPerMonth = 730.0 + // defaultBenefitLookback is sent when params carry no lookback, so the + // scaling never depends on an API-side default. It matches the Azure + // reservation path (internal/recommendations consumptionLookBack). + defaultBenefitLookback = "Last7Days" + + // hoursPerMonth uses the same average month as the other Azure + // recommendation converters (recommendations.DaysPerMonth = 30.4375 days). + hoursPerMonth = recommendations.DaysPerMonth * 24 ) type benefitRecsPage struct { @@ -69,13 +76,11 @@ type benefitRecommendDetls struct { } // benefitLookback maps RecommendationParams.LookbackPeriod to the API enum. -// An empty value is omitted from the filter (API default Last60Days). +// An empty value selects defaultBenefitLookback. func benefitLookback(p string) (string, error) { switch strings.ToLower(strings.TrimSpace(p)) { - case "": - return "", nil - case "7d": - return "Last7Days", nil + case "", "7d": + return defaultBenefitLookback, nil case "30d": return "Last30Days", nil case "60d": @@ -88,23 +93,19 @@ func benefitLookback(p string) (string, error) { // benefitTerms returns the API terms to request. An empty term requests both // P1Y and P3Y because the API default (P3Y) would silently drop 1-year rows. func benefitTerms(t string) ([]string, error) { - switch strings.ToLower(strings.TrimSpace(t)) { - case "": + if strings.TrimSpace(t) == "" { return []string{"P1Y", "P3Y"}, nil - case "1", "1yr", "1y", "p1y": - return []string{"P1Y"}, nil - case "3", "3yr", "3y", "p3y": - return []string{"P3Y"}, nil - default: - return nil, fmt.Errorf("unsupported term %q for Azure savings plan recommendations (the API supports 1yr and 3yr)", t) } + years, err := reservations.ParseTermYears(t) + if err != nil { + return nil, fmt.Errorf("unsupported term %q for Azure savings plan recommendations (the API supports 1yr and 3yr): %w", t, err) + } + return []string{fmt.Sprintf("P%dY", years)}, nil } func (c *Client) benefitRecsURL(term, lookback string) string { - filter := "properties/scope eq 'Shared' AND properties/term eq '" + term + "'" - if lookback != "" { - filter += " AND properties/lookBackPeriod eq '" + lookback + "'" - } + filter := "properties/scope eq 'Shared' AND properties/term eq '" + term + + "' AND properties/lookBackPeriod eq '" + lookback + "'" q := url.Values{} q.Set("$filter", filter) q.Set("api-version", benefitRecsAPIVersion) @@ -153,16 +154,11 @@ func (c *Client) getBenefitRecommendations(ctx context.Context, params *common.R func (c *Client) fetchBenefitPages(ctx context.Context, bearer, firstURL string) ([]benefitRecommendation, error) { var all []benefitRecommendation - seen := map[string]struct{}{} next := firstURL for pageIdx := 0; next != ""; pageIdx++ { if pageIdx >= benefitRecsMaxPages { return nil, fmt.Errorf("result exceeds the %d-page cap with nextLink still set", benefitRecsMaxPages) } - if _, dup := seen[next]; dup { - return nil, fmt.Errorf("self-referential nextLink (page %d)", pageIdx) - } - seen[next] = struct{}{} if err := requireARMURL(next); err != nil { return nil, fmt.Errorf("page %d: %w", pageIdx, err) } diff --git a/providers/azure/services/savingsplans/benefit_recommendations_test.go b/providers/azure/services/savingsplans/benefit_recommendations_test.go index efecb4a..3e6372d 100644 --- a/providers/azure/services/savingsplans/benefit_recommendations_test.go +++ b/providers/azure/services/savingsplans/benefit_recommendations_test.go @@ -64,9 +64,9 @@ func TestGetRecommendations_MapsRow(t *testing.T) { assert.Equal(t, "monthly", r.PaymentOption) assert.Equal(t, 0.0, r.CommitmentCost) require.NotNil(t, r.RecurringMonthlyCost) - assert.InDelta(t, 1.5*730, *r.RecurringMonthlyCost, 1e-9) - assert.InDelta(t, 292*730.0/1460, r.EstimatedSavings, 1e-9) - assert.InDelta(t, 2920*730.0/1460, r.OnDemandCost, 1e-9) + assert.InDelta(t, 1.5*hoursPerMonth, *r.RecurringMonthlyCost, 1e-9) + assert.InDelta(t, 292*hoursPerMonth/1460, r.EstimatedSavings, 1e-9) + assert.InDelta(t, 2920*hoursPerMonth/1460, r.OnDemandCost, 1e-9) d := r.Details.(*common.SavingsPlanDetails) assert.Equal(t, 1.5, d.HourlyCommitment) assert.Equal(t, "USD", d.Currency) @@ -92,7 +92,7 @@ func TestGetRecommendations_EmptyTermRequestsBoth(t *testing.T) { require.Len(t, recs, 2) assert.Equal(t, "1yr", recs[0].Term) assert.Equal(t, "3yr", recs[1].Term) - assert.NotContains(t, h.reqs[0].URL.Query().Get("$filter"), "lookBackPeriod") + assert.Contains(t, h.reqs[0].URL.Query().Get("$filter"), "properties/lookBackPeriod eq 'Last7Days'") } func TestGetRecommendations_EmptyValue(t *testing.T) { @@ -124,12 +124,13 @@ func TestGetRecommendations_RejectsForeignNextLink(t *testing.T) { assert.Len(t, h.reqs, 1, "bearer token must not be sent to the foreign host") } -func TestGetRecommendations_SelfReferentialNextLink(t *testing.T) { +func TestGetRecommendations_RejectsHTTPNextLink(t *testing.T) { h := &routeHTTP{reply: func(*http.Request) (int, string) { - return 200, `{"value":[],"nextLink":"https://management.azure.com/loop"}` + return 200, `{"value":[],"nextLink":"http://management.azure.com/next"}` }} _, err := newTestClient(h).GetRecommendations(context.Background(), &common.RecommendationParams{Term: "3yr"}) require.Error(t, err) + assert.Len(t, h.reqs, 1) } func TestGetRecommendations_Errors(t *testing.T) { @@ -184,3 +185,12 @@ func TestBenefitRecsURL_PathAndQuery(t *testing.T) { require.NoError(t, err) assert.Equal(t, "/subscriptions/sub-1/providers/Microsoft.CostManagement/benefitRecommendations", u.Path) } + +func TestGetRecommendations_PageCapIsAnError(t *testing.T) { + h := &routeHTTP{reply: func(*http.Request) (int, string) { + return 200, `{"value":[],"nextLink":"https://management.azure.com/loop"}` + }} + _, err := newTestClient(h).GetRecommendations(context.Background(), &common.RecommendationParams{Term: "3yr"}) + require.Error(t, err) + assert.Len(t, h.reqs, benefitRecsMaxPages) +} diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index 8562dc6..c8d3d9f 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -125,7 +125,7 @@ func (c *Client) GetRegion() string { // GetRecommendations returns Shared-scope Compute Savings Plan purchase // recommendations from the Cost Management Benefit Recommendations API // (api-version 2024-08-01) at this client's subscription scope. See -// benefit_recommendations.go. Term and LookbackPeriod from params are honoured; +// benefit_recommendations.go. Term and LookbackPeriod from params are honored; // an unsupported value is an error. func (c *Client) GetRecommendations(ctx context.Context, params *common.RecommendationParams) ([]common.Recommendation, error) { return c.getBenefitRecommendations(ctx, params)