diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index c1aaead6..2e7f56b2 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -328,6 +328,21 @@ func commitmentCurrency(d *common.SavingsPlanDetails) (string, error) { return strings.ToUpper(d.Currency), nil } +// savingsPlanSKUName maps a CUDly PlanType to the ARM SKU name that the +// SavingsPlanOrderAlias and validatePurchase bodies carry in sku.name. Azure +// names the compute savings plan SKU "Compute_Savings_Plan" (the armSkuName the +// Benefit Recommendations API returns), while CUDly's PlanType is "Compute" +// (issue #387). The ARM name itself is accepted as-is. Anything else fails +// closed before any Azure call rather than sending a sku.name Azure rejects. +func savingsPlanSKUName(planType string) (string, error) { + switch planType { + case "Compute", armSkuComputeSavingsPlan: + return armSkuComputeSavingsPlan, nil + default: + return "", fmt.Errorf("azure savings plan type %q has no known Azure savings plan SKU", planType) + } +} + // PurchaseCommitment creates an Azure Savings Plan via the OrderAlias API. // // Idempotency: the SavingsPlanOrderAlias is created with an HTTP PUT on the @@ -354,22 +369,11 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, result.Error } - term, err := toAzureTerm(rec.Term) - if err != nil { - result.Error = err - return result, result.Error - } - - grain := armbillingbenefits.CommitmentGrainHourly - billingScopeID := c.billingScopeID() - appliedScope := armbillingbenefits.AppliedScopeTypeShared - hourlyAmount := spDetails.HourlyCommitment - currency, err := commitmentCurrency(spDetails) + body, err := c.purchaseOrderBody(rec, spDetails) if err != nil { result.Error = err return result, result.Error } - displayName := fmt.Sprintf("cudly-%s-%s", spDetails.PlanType, rec.Term) // Derive a deterministic alias name from the idempotency token when present // so a re-drive PUTs the same alias (idempotent), falling back to the prior // timestamp-based name on the CLI path (empty token). A malformed non-empty @@ -381,31 +385,10 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, result.Error } - body := armbillingbenefits.SavingsPlanOrderAliasModel{ - SKU: &armbillingbenefits.SKU{Name: toPtr(spDetails.PlanType)}, - Properties: &armbillingbenefits.SavingsPlanOrderAliasProperties{ - DisplayName: &displayName, - BillingScopeID: &billingScopeID, - Term: &term, - AppliedScopeType: &appliedScope, - Commitment: &armbillingbenefits.Commitment{ - Amount: &hourlyAmount, - CurrencyCode: ¤cy, - Grain: &grain, - }, - }, - } - - var aliasClient SavingsPlanOrderAliasAPI - if c.orderAliasClient != nil { - aliasClient = c.orderAliasClient - } else { - client, clientErr := armbillingbenefits.NewSavingsPlanOrderAliasClient(c.cred, nil) - if clientErr != nil { - result.Error = fmt.Errorf("failed to create order alias client: %w", clientErr) - return result, result.Error - } - aliasClient = &realOrderAliasClient{client: client} + aliasClient, err := c.orderAlias() + if err != nil { + result.Error = err + return result, result.Error } poller, err := aliasClient.BeginCreate(ctx, aliasName, body, nil) @@ -431,6 +414,56 @@ func (c *Client) PurchaseCommitment(ctx context.Context, rec common.Recommendati return result, nil } +// purchaseOrderBody builds the order-alias request for a savings plan +// purchase. The term, currency and SKU name are validated here, so an invalid +// recommendation fails before any Azure client exists. +func (c *Client) purchaseOrderBody(rec common.Recommendation, spDetails *common.SavingsPlanDetails) (armbillingbenefits.SavingsPlanOrderAliasModel, error) { + term, err := toAzureTerm(rec.Term) + if err != nil { + return armbillingbenefits.SavingsPlanOrderAliasModel{}, err + } + currency, err := commitmentCurrency(spDetails) + if err != nil { + return armbillingbenefits.SavingsPlanOrderAliasModel{}, err + } + skuName, err := savingsPlanSKUName(spDetails.PlanType) + if err != nil { + return armbillingbenefits.SavingsPlanOrderAliasModel{}, err + } + grain := armbillingbenefits.CommitmentGrainHourly + billingScopeID := c.billingScopeID() + appliedScope := armbillingbenefits.AppliedScopeTypeShared + hourlyAmount := spDetails.HourlyCommitment + displayName := fmt.Sprintf("cudly-%s-%s", spDetails.PlanType, rec.Term) + return armbillingbenefits.SavingsPlanOrderAliasModel{ + SKU: &armbillingbenefits.SKU{Name: toPtr(skuName)}, + Properties: &armbillingbenefits.SavingsPlanOrderAliasProperties{ + DisplayName: &displayName, + BillingScopeID: &billingScopeID, + Term: &term, + AppliedScopeType: &appliedScope, + Commitment: &armbillingbenefits.Commitment{ + Amount: &hourlyAmount, + CurrencyCode: ¤cy, + Grain: &grain, + }, + }, + }, nil +} + +// orderAlias returns the injected order-alias client, or a real one built +// from the client's credential. +func (c *Client) orderAlias() (SavingsPlanOrderAliasAPI, error) { + if c.orderAliasClient != nil { + return c.orderAliasClient, nil + } + client, err := armbillingbenefits.NewSavingsPlanOrderAliasClient(c.cred, nil) + if err != nil { + return nil, fmt.Errorf("failed to create order alias client: %w", err) + } + return &realOrderAliasClient{client: client}, nil +} + // ValidateOffering checks whether the Savings Plan configuration is valid. func (c *Client) ValidateOffering(ctx context.Context, rec common.Recommendation) error { validateBody, err := c.buildValidateBody(rec) @@ -471,12 +504,16 @@ func (c *Client) buildValidateBody(rec common.Recommendation) (armbillingbenefit if err != nil { return armbillingbenefits.SavingsPlanPurchaseValidateRequest{}, err } + skuName, err := savingsPlanSKUName(spDetails.PlanType) + if err != nil { + return armbillingbenefits.SavingsPlanPurchaseValidateRequest{}, err + } displayName := "cudly-validate" return armbillingbenefits.SavingsPlanPurchaseValidateRequest{ Benefits: []*armbillingbenefits.SavingsPlanOrderAliasModel{ { - SKU: &armbillingbenefits.SKU{Name: toPtr(spDetails.PlanType)}, + SKU: &armbillingbenefits.SKU{Name: toPtr(skuName)}, Properties: &armbillingbenefits.SavingsPlanOrderAliasProperties{ DisplayName: &displayName, BillingScopeID: &billingScopeID, diff --git a/providers/azure/services/savingsplans/sku_name_test.go b/providers/azure/services/savingsplans/sku_name_test.go new file mode 100644 index 00000000..481eaa21 --- /dev/null +++ b/providers/azure/services/savingsplans/sku_name_test.go @@ -0,0 +1,104 @@ +package savingsplans + +import ( + "context" + "net/http" + "testing" + + "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" +) + +// Issue #387: the SavingsPlanOrderAlias and validatePurchase bodies must carry +// the ARM SKU name "Compute_Savings_Plan" (the armSkuName the Benefit +// Recommendations API returns and the SDK's SavingsPlanOrderAliasCreate example +// sends), not the CUDly PlanType "Compute". Before the fix both bodies sent +// sku.name "Compute". + +func skuRec(planType string) common.Recommendation { + rec := makeRec("1yr") + rec.Details = &common.SavingsPlanDetails{PlanType: planType, HourlyCommitment: 1.5, Currency: "USD"} + return rec +} + +func okAlias() *mockOrderAliasClient { + orderID := "/providers/Microsoft.BillingBenefits/savingsPlanOrders/order1" + return &mockOrderAliasClient{createPoller: &mockPoller{resp: armbillingbenefits.SavingsPlanOrderAliasClientCreateResponse{ + SavingsPlanOrderAliasModel: armbillingbenefits.SavingsPlanOrderAliasModel{ID: &orderID}, + }}} +} + +func capturedSKUs(t *testing.T, rec common.Recommendation) (purchase, validate string) { + t.Helper() + alias := okAlias() + rp := &mockRPValidate{} + c := NewClient(nil, "sub", "eastus") + c.SetOrderAliasClient(alias) + c.SetRPValidateClient(rp) + + _, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + require.NoError(t, err) + require.NoError(t, c.ValidateOffering(context.Background(), rec)) + + require.NotNil(t, alias.capturedBody.SKU) + require.NotNil(t, alias.capturedBody.SKU.Name) + require.Len(t, rp.captured.Benefits, 1) + require.NotNil(t, rp.captured.Benefits[0].SKU) + require.NotNil(t, rp.captured.Benefits[0].SKU.Name) + return *alias.capturedBody.SKU.Name, *rp.captured.Benefits[0].SKU.Name +} + +func TestSKUName_ComputePlanTypeSendsARMSKU(t *testing.T) { + for _, planType := range []string{"Compute", "Compute_Savings_Plan"} { + t.Run(planType, func(t *testing.T) { + purchase, validate := capturedSKUs(t, skuRec(planType)) + assert.Equal(t, "Compute_Savings_Plan", purchase) + assert.Equal(t, "Compute_Savings_Plan", validate) + }) + } +} + +// A recommendation produced by GetRecommendations from a fixture row must +// purchase under the same SKU the row named. +func TestSKUName_RecommendationRoundTrip(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) + + purchase, validate := capturedSKUs(t, recs[0]) + assert.Equal(t, armSkuComputeSavingsPlan, purchase) + assert.Equal(t, armSkuComputeSavingsPlan, validate) +} + +// A PlanType with no known ARM SKU fails before any Azure call instead of +// sending a sku.name the API would reject. +func TestSKUName_UnknownPlanTypeFailsClosed(t *testing.T) { + for _, planType := range []string{"", "MachineLearning", "compute", " Compute", "Compute_Savings_Plan "} { + t.Run(planType, func(t *testing.T) { + alias := okAlias() + rp := &mockRPValidate{} + c := NewClient(nil, "sub", "eastus") + c.SetOrderAliasClient(alias) + c.SetRPValidateClient(rp) + rec := skuRec(planType) + + result, err := c.PurchaseCommitment(context.Background(), rec, common.PurchaseOptions{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "no known Azure savings plan SKU") + assert.False(t, result.Success) + assert.Equal(t, err, result.Error) + assert.Zero(t, alias.calls) + + err = c.ValidateOffering(context.Background(), rec) + require.Error(t, err) + assert.Contains(t, err.Error(), "no known Azure savings plan SKU") + assert.Zero(t, rp.calls) + }) + } +}