fix(azure/savingsplans): take the inventory resource type from the SKU, not the ARM name

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,

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}$`)

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":

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")},
 	}
 }

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),

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)
+}

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 {