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..4bd1cf0e 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,31 @@ 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" + +// 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 %s has an unrecognized SKU (none reported); extend the SKU mapping in convertSavingsPlan", *sp.ID) + } + 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 +} + // 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 +265,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 +290,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..a9a53118 --- /dev/null +++ b/providers/azure/services/savingsplans/resource_type_test.go @@ -0,0 +1,74 @@ +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"}`) + 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) + 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"}`, + } { + 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, "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) +} 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 {