From 88fc5c1c811b559a4609637ac33d4a9d7f4c3f0a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 02:33:26 +0200 Subject: [PATCH 1/2] fix(azure/savingsplans): take the inventory resource type from the SKU, not the ARM name convertSavingsPlan set ResourceType from the ARM resource name, which is GUID-shaped, while recommendations use "Compute". recfilter.DuplicateChecker keys on ResourceType, so an owned plan never suppressed a repeat recommendation and the duplicate guard failed open. ResourceType now comes from sku.name: Compute_Savings_Plan maps to the shared computeResourceType constant used on both sides. A missing, empty or unknown SKU returns an error, so GetExistingCommitments fails closed through the existing "incomplete savings plan inventory" path. There is no fallback to Name. Not verified against a live response: the SKU value comes from the armbillingbenefits SDK examples. Closes #492 Co-Authored-By: Claude Opus 5.5 --- .../savingsplans/benefit_recommendations.go | 2 +- .../azure/services/savingsplans/client.go | 41 ++++++++++--- .../client_completeness_sdk_test.go | 2 +- .../savingsplans/client_pager_cap_test.go | 1 + .../services/savingsplans/client_test.go | 6 +- .../savingsplans/resource_type_test.go | 59 +++++++++++++++++++ .../azure/services/savingsplans/scope_test.go | 4 +- 7 files changed, 101 insertions(+), 14 deletions(-) create mode 100644 providers/azure/services/savingsplans/resource_type_test.go diff --git a/providers/azure/services/savingsplans/benefit_recommendations.go b/providers/azure/services/savingsplans/benefit_recommendations.go index 3cdb7046..41d9c662 100644 --- a/providers/azure/services/savingsplans/benefit_recommendations.go +++ b/providers/azure/services/savingsplans/benefit_recommendations.go @@ -236,7 +236,7 @@ func convertBenefitRecommendation(r *benefitRecommendation, subscriptionID strin Provider: common.ProviderAzure, Account: subscriptionID, Service: common.ServiceSavingsPlansAll, - ResourceType: "Compute", + ResourceType: computeResourceType, Count: 1, CommitmentType: common.CommitmentSavingsPlan, Term: term, diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index a4235d0b..870892fd 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -190,7 +190,11 @@ func (c *Client) appendOwnedPlans(dst []common.Commitment, plans []*armbillingbe } switch disposition { case planOwned: - dst = append(dst, *convertSavingsPlan(sp, c.subscriptionID)) + commitment, err := convertSavingsPlan(sp, c.subscriptionID) + if err != nil { + return nil, 0, fmt.Errorf("row %d: %w", i+1, err) + } + dst = append(dst, *commitment) case planSkippedSingle: skipped++ } @@ -228,10 +232,34 @@ func azureSavingsPlanState(ps armbillingbenefits.ProvisioningState) common.Commi } } +// computeResourceType is the ResourceType of a Compute savings plan on both the +// recommendation and the inventory side; recfilter.DuplicateChecker matches on it. +const computeResourceType = "Compute" + +// computeSavingsPlanSKU is the SKU name the billing benefits API reports for a +// Compute savings plan (from the SDK examples, not a live response). +const computeSavingsPlanSKU = "Compute_Savings_Plan" + +// savingsPlanResourceType maps the plan SKU to the shared resource type. A +// missing or unknown SKU is an error so the duplicate guard fails closed. +func savingsPlanResourceType(sp *armbillingbenefits.SavingsPlanModel) (string, error) { + if sp.SKU == nil || sp.SKU.Name == nil || *sp.SKU.Name == "" { + return "", fmt.Errorf("owned savings plan has no SKU name") + } + if *sp.SKU.Name != computeSavingsPlanSKU { + return "", fmt.Errorf("owned savings plan has unsupported SKU %q", *sp.SKU.Name) + } + return computeResourceType, nil +} + // convertSavingsPlan maps an armbillingbenefits.SavingsPlanModel to a common.Commitment. -func convertSavingsPlan(sp *armbillingbenefits.SavingsPlanModel, subscriptionID string) *common.Commitment { +func convertSavingsPlan(sp *armbillingbenefits.SavingsPlanModel, subscriptionID string) (*common.Commitment, error) { if sp == nil || sp.ID == nil { - return nil + return nil, fmt.Errorf("savings plan has no ID") + } + resourceType, err := savingsPlanResourceType(sp) + if err != nil { + return nil, err } commitment := common.Commitment{ @@ -240,14 +268,11 @@ func convertSavingsPlan(sp *armbillingbenefits.SavingsPlanModel, subscriptionID CommitmentID: *sp.ID, CommitmentType: common.CommitmentSavingsPlan, Service: common.ServiceSavingsPlansAll, + ResourceType: resourceType, Count: 1, State: common.CommitmentStateActive, } - if sp.Name != nil { - commitment.ResourceType = *sp.Name - } - if sp.Properties != nil { props := sp.Properties @@ -268,7 +293,7 @@ func convertSavingsPlan(sp *armbillingbenefits.SavingsPlanModel, subscriptionID } } - return &commitment + return &commitment, nil } var currencyCodePattern = regexp.MustCompile(`^[A-Za-z]{3}$`) diff --git a/providers/azure/services/savingsplans/client_completeness_sdk_test.go b/providers/azure/services/savingsplans/client_completeness_sdk_test.go index c070445d..6cd32095 100644 --- a/providers/azure/services/savingsplans/client_completeness_sdk_test.go +++ b/providers/azure/services/savingsplans/client_completeness_sdk_test.go @@ -44,7 +44,7 @@ func TestGetExistingCommitments_SDKPaginationCompleteness(t *testing.T) { requests.Add(1) switch r.URL.Query().Get("page") { case "": - _, err := fmt.Fprintf(w, `{"value":[{"id":"owned","name":"Compute","properties":{"billingScopeId":"/subscriptions/sub-a","provisioningState":"Succeeded","effectiveDateTime":%q}}],"nextLink":%q}`, + _, err := fmt.Fprintf(w, `{"value":[{"id":"owned","name":"Compute","sku":{"name":"Compute_Savings_Plan"},"properties":{"billingScopeId":"/subscriptions/sub-a","provisioningState":"Succeeded","effectiveDateTime":%q}}],"nextLink":%q}`, time.Now().UTC().Format(time.RFC3339), endpoint+"/providers/Microsoft.BillingBenefits/savingsPlans?api-version=2022-11-01&page=2") assert.NoError(t, err) case "2": diff --git a/providers/azure/services/savingsplans/client_pager_cap_test.go b/providers/azure/services/savingsplans/client_pager_cap_test.go index 468c5e41..4f6e1990 100644 --- a/providers/azure/services/savingsplans/client_pager_cap_test.go +++ b/providers/azure/services/savingsplans/client_pager_cap_test.go @@ -33,6 +33,7 @@ func (p *pagedListAllPager) NextPage(_ context.Context) (armbillingbenefits.Savi func ownedPlan(id string) *armbillingbenefits.SavingsPlanModel { return &armbillingbenefits.SavingsPlanModel{ ID: &id, + SKU: &armbillingbenefits.SKU{Name: toPtr("Compute_Savings_Plan")}, Properties: &armbillingbenefits.SavingsPlanModelProperties{BillingScopeID: toPtr("/subscriptions/sub-abc")}, } } diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index c825c5c5..838a4f97 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -153,6 +153,7 @@ func TestGetExistingCommitments_Happy(t *testing.T) { sp := &armbillingbenefits.SavingsPlanModel{ ID: &spID, Name: &spName, + SKU: &armbillingbenefits.SKU{Name: toPtr("Compute_Savings_Plan")}, Properties: &armbillingbenefits.SavingsPlanModelProperties{ BillingScopeID: toPtr("/subscriptions/sub-abc"), ProvisioningState: &prov, @@ -178,7 +179,7 @@ func TestGetExistingCommitments_Happy(t *testing.T) { assert.Equal(t, common.CommitmentSavingsPlan, got.CommitmentType) assert.Equal(t, common.ServiceSavingsPlansAll, got.Service) assert.Equal(t, spID, got.CommitmentID) - assert.Equal(t, spName, got.ResourceType) + assert.Equal(t, "Compute", got.ResourceType) assert.Equal(t, common.CommitmentStateActive, got.State) assert.Equal(t, amount, got.Cost) assert.False(t, got.StartDateIsDay, "EffectiveDateTime is a timestamp, not a calendar date") @@ -187,7 +188,8 @@ func TestGetExistingCommitments_Happy(t *testing.T) { func TestGetExistingCommitments_SubscriptionOwnership(t *testing.T) { plan := func(id, scope string, applied armbillingbenefits.AppliedScopeType, subscription string) *armbillingbenefits.SavingsPlanModel { return &armbillingbenefits.SavingsPlanModel{ - ID: toPtr(id), + ID: toPtr(id), + SKU: &armbillingbenefits.SKU{Name: toPtr("Compute_Savings_Plan")}, Properties: &armbillingbenefits.SavingsPlanModelProperties{ BillingScopeID: toPtr(scope), AppliedScopeType: toPtr(applied), diff --git a/providers/azure/services/savingsplans/resource_type_test.go b/providers/azure/services/savingsplans/resource_type_test.go new file mode 100644 index 00000000..a3d2e634 --- /dev/null +++ b/providers/azure/services/savingsplans/resource_type_test.go @@ -0,0 +1,59 @@ +package savingsplans + +import ( + "context" + "fmt" + "net/http" + "net/http/httptest" + "testing" + "time" + + "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" +) + +// Fixtures follow the shape of the armbillingbenefits SDK examples (GUID name, +// Compute_Savings_Plan SKU); they are not a captured live response. +func resourceTypeFixture(t *testing.T, sku string) *Client { + t.Helper() + row := fmt.Sprintf(`{"id":"/subscriptions/sub-a/providers/Microsoft.BillingBenefits/savingsPlanOrders/o1/savingsPlans/6f1f3c1a-1b2c-4d5e-8f90-123456789abc","name":"6f1f3c1a-1b2c-4d5e-8f90-123456789abc"%s,"properties":{"billingScopeId":"/subscriptions/sub-a","provisioningState":"Succeeded","effectiveDateTime":%q}}`, + sku, time.Now().UTC().Format(time.RFC3339)) + server := httptest.NewTLSServer(http.HandlerFunc(func(w http.ResponseWriter, _ *http.Request) { + w.Header().Set("Content-Type", "application/json") + _, err := fmt.Fprintf(w, `{"value":[%s]}`, row) + assert.NoError(t, err) + })) + t.Cleanup(server.Close) + client := NewClient(nil, "sub-a", "eastus") + client.SetListAllPager(newFixtureSavingsPlanClient(t, server).NewListAllPager(nil)) + return client +} + +func TestOwnedPlanSuppressesComputeRecommendation(t *testing.T) { + client := resourceTypeFixture(t, `,"sku":{"name":"Compute_Savings_Plan"}`) + recs := []common.Recommendation{{Provider: common.ProviderAzure, Service: common.ServiceSavingsPlansAll, ResourceType: computeResourceType, Count: 1}} + passed, filtered, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, client) + require.NoError(t, err) + assert.Empty(t, passed) + assert.Len(t, filtered, 1) +} + +func TestGetExistingCommitments_FailsClosedOnBadSKU(t *testing.T) { + for name, sku := range map[string]string{ + "missing": ``, + "empty": `,"sku":{"name":""}`, + "no-name": `,"sku":{}`, + "unknown": `,"sku":{"name":"Mystery_Plan"}`, + "lowercase": `,"sku":{"name":"compute_savings_plan"}`, + } { + t.Run(name, func(t *testing.T) { + got, err := resourceTypeFixture(t, sku).GetExistingCommitments(context.Background()) + require.ErrorContains(t, err, "incomplete savings plan inventory") + require.ErrorContains(t, err, "SKU") + assert.Nil(t, got) + }) + } +} diff --git a/providers/azure/services/savingsplans/scope_test.go b/providers/azure/services/savingsplans/scope_test.go index dbbe35b1..d0b0c016 100644 --- a/providers/azure/services/savingsplans/scope_test.go +++ b/providers/azure/services/savingsplans/scope_test.go @@ -25,7 +25,7 @@ const ( ) 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}} + return &armbillingbenefits.SavingsPlanModel{ID: toPtr("plan-id"), SKU: &armbillingbenefits.SKU{Name: toPtr("Compute_Savings_Plan")}, Properties: &armbillingbenefits.SavingsPlanModelProperties{BillingScopeID: toPtr(scope), AppliedScopeType: typ, AppliedScopeProperties: props}} } // scoped builds applied-scope properties; "" leaves the field nil. @@ -141,7 +141,7 @@ func TestGetExistingCommitments_AggregateSkipLog(t *testing.T) { 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}}`, + return fmt.Sprintf(`{"id":%q,"name":"Compute","sku":{"name":"Compute_Savings_Plan"},"properties":{"billingScopeId":%q,"appliedScopeType":%q,"appliedScopeProperties":%s,"provisioningState":"Succeeded","effectiveDateTime":%q}}`, id, scope, applied, props, time.Now().UTC().Format(time.RFC3339)) } cases := []struct { From bbe16c7a5aeb52888b37cc45d02a62ffddfac54a Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Sat, 17 Oct 2026 02:35:53 +0200 Subject: [PATCH 2/2] fix(azure/savingsplans): name the plan in SKU errors and match the SKU case-insensitively SKU errors now include the plan ID and say the SKU mapping needs extending. The SKU is matched with strings.EqualFold against the existing armSkuComputeSavingsPlan constant, so one plan with different casing does not block the whole subscription. The dedupe test now builds its recommendation through convertBenefitRecommendation, so a wrong constant on either side fails it. Refs #492 Co-Authored-By: Claude Opus 5.5 --- .../azure/services/savingsplans/client.go | 15 ++++------ .../savingsplans/resource_type_test.go | 29 ++++++++++++++----- 2 files changed, 28 insertions(+), 16 deletions(-) diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index 870892fd..4bd1cf0e 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -236,18 +236,15 @@ func azureSavingsPlanState(ps armbillingbenefits.ProvisioningState) common.Commi // recommendation and the inventory side; recfilter.DuplicateChecker matches on it. const computeResourceType = "Compute" -// computeSavingsPlanSKU is the SKU name the billing benefits API reports for a -// Compute savings plan (from the SDK examples, not a live response). -const computeSavingsPlanSKU = "Compute_Savings_Plan" - -// savingsPlanResourceType maps the plan SKU to the shared resource type. A -// missing or unknown SKU is an error so the duplicate guard fails closed. +// savingsPlanResourceType maps the plan SKU (armSkuComputeSavingsPlan, matched +// case-insensitively) to the shared resource type. A missing or unknown SKU is +// an error so the duplicate guard fails closed. func savingsPlanResourceType(sp *armbillingbenefits.SavingsPlanModel) (string, error) { if sp.SKU == nil || sp.SKU.Name == nil || *sp.SKU.Name == "" { - return "", fmt.Errorf("owned savings plan has no SKU name") + return "", fmt.Errorf("owned savings plan %s has an unrecognized SKU (none reported); extend the SKU mapping in convertSavingsPlan", *sp.ID) } - if *sp.SKU.Name != computeSavingsPlanSKU { - return "", fmt.Errorf("owned savings plan has unsupported SKU %q", *sp.SKU.Name) + if !strings.EqualFold(*sp.SKU.Name, armSkuComputeSavingsPlan) { + return "", fmt.Errorf("owned savings plan %s has an unrecognized SKU %q; extend the SKU mapping in convertSavingsPlan", *sp.ID, *sp.SKU.Name) } return computeResourceType, nil } diff --git a/providers/azure/services/savingsplans/resource_type_test.go b/providers/azure/services/savingsplans/resource_type_test.go index a3d2e634..a9a53118 100644 --- a/providers/azure/services/savingsplans/resource_type_test.go +++ b/providers/azure/services/savingsplans/resource_type_test.go @@ -34,7 +34,15 @@ func resourceTypeFixture(t *testing.T, sku string) *Client { func TestOwnedPlanSuppressesComputeRecommendation(t *testing.T) { client := resourceTypeFixture(t, `,"sku":{"name":"Compute_Savings_Plan"}`) - recs := []common.Recommendation{{Provider: common.ProviderAzure, Service: common.ServiceSavingsPlansAll, ResourceType: computeResourceType, Count: 1}} + hours, cost, saved, commit := int32(168), 100.0, 20.0, 1.5 + row := &benefitRecommendation{ID: "rec-1", Kind: kindSavingsPlan, Properties: &benefitRecommendationProps{ + ArmSkuName: armSkuComputeSavingsPlan, Term: "P1Y", Scope: scopeShared, CommitmentGranularity: grainHourly, + CurrencyCode: "USD", TotalHours: &hours, CostWithoutBenefit: &cost, + RecommendationDetails: &benefitRecommendDetls{CommitmentAmount: &commit, SavingsAmount: &saved}, + }} + rec, err := convertBenefitRecommendation(row, "sub-a") + require.NoError(t, err) + recs := []common.Recommendation{*rec} passed, filtered, err := recfilter.NewDuplicateChecker(24).AdjustRecommendationsForExisting(context.Background(), recs, client) require.NoError(t, err) assert.Empty(t, passed) @@ -43,17 +51,24 @@ func TestOwnedPlanSuppressesComputeRecommendation(t *testing.T) { func TestGetExistingCommitments_FailsClosedOnBadSKU(t *testing.T) { for name, sku := range map[string]string{ - "missing": ``, - "empty": `,"sku":{"name":""}`, - "no-name": `,"sku":{}`, - "unknown": `,"sku":{"name":"Mystery_Plan"}`, - "lowercase": `,"sku":{"name":"compute_savings_plan"}`, + "missing": ``, + "empty": `,"sku":{"name":""}`, + "no-name": `,"sku":{}`, + "unknown": `,"sku":{"name":"Mystery_Plan"}`, } { t.Run(name, func(t *testing.T) { got, err := resourceTypeFixture(t, sku).GetExistingCommitments(context.Background()) require.ErrorContains(t, err, "incomplete savings plan inventory") - require.ErrorContains(t, err, "SKU") + require.ErrorContains(t, err, "6f1f3c1a-1b2c-4d5e-8f90-123456789abc") + require.ErrorContains(t, err, "extend the SKU mapping in convertSavingsPlan") assert.Nil(t, got) }) } } + +func TestGetExistingCommitments_SKUMatchIsCaseInsensitive(t *testing.T) { + got, err := resourceTypeFixture(t, `,"sku":{"name":"compute_savings_plan"}`).GetExistingCommitments(context.Background()) + require.NoError(t, err) + require.Len(t, got, 1) + assert.Equal(t, "Compute", got[0].ResourceType) +}