fix(azure): let exchange targets request a Monthly billing plan

CHANGELOG.md

@@ -162,6 +162,13 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/).
 
 ### Fixed
 
+- **`azure/compute`:** reservation exchange targets are no longer always
+  billed Upfront. `ExchangeTarget.BillingPlan` (new, optional) is sent as each
+  target's `billingPlan` in the CalculateExchange body, so the session that
+  `ExecuteExchange` commits carries it. Unset keeps the previous Upfront
+  request byte for byte; anything other than `Upfront` or `Monthly` (exact
+  spelling) is a validation error before any request (#89).
+
 - **`azure/savingsplans`:** the commitment currency is no longer hardcoded to
   USD. `SavingsPlanDetails.Currency` (new, additive) is sent in the purchase and
   validate bodies and returned by `GetOfferingDetails`; empty or malformed

providers/azure/services/compute/client.go

@@ -15,6 +15,7 @@ import (
 	"time"
 
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
 	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/compute/armcompute/v5"
 	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/consumption/armconsumption"
@@ -127,6 +128,12 @@ type Client struct {
 	// hermetically and make the LRO synchronous.
 	calculateExchangeCaller CalculateExchangeCallerFunc
 	doExchangeCaller        DoExchangeCallerFunc
+
+	// Optional ARM client options for the real CalculateExchange and
+	// Exchange SDK clients. Nil in production (SDK defaults); tests set a
+	// fake HTTP transport so the production LRO wrappers and the SDK's own
+	// request serialization run without reaching Azure.
+	exchangeClientOptions *arm.ClientOptions
 }
 
 // NewClient creates a new Azure Compute client.

providers/azure/services/compute/exchange_billing_plan_sdk_test.go

@@ -0,0 +1,172 @@
+package compute_test
+
+import (
+	"context"
+	"encoding/json"
+	"io"
+	"net/http"
+	"strings"
+	"sync"
+	"testing"
+
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
+	"github.com/Azure/azure-sdk-for-go/sdk/azcore/policy"
+	"github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations"
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+
+	"github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute"
+)
+
+// exchangeSDKRequest is one HTTP request the real armreservations clients
+// sent through the fake transport.
+type exchangeSDKRequest struct {
+	method, path string
+	body         string
+}
+
+// exchangeSDKTransport answers the CalculateExchange and Exchange POSTs with
+// a synchronous 200 (no polling headers, so the SDK poller finishes at once)
+// and records every request so tests can assert on the exact wire body.
+type exchangeSDKTransport struct {
+	mu       sync.Mutex
+	requests []exchangeSDKRequest
+}
+
+func (t *exchangeSDKTransport) Do(req *http.Request) (*http.Response, error) {
+	var body []byte
+	if req.Body != nil {
+		var err error
+		body, err = io.ReadAll(req.Body)
+		if err != nil {
+			return nil, err
+		}
+	}
+	t.mu.Lock()
+	t.requests = append(t.requests, exchangeSDKRequest{method: req.Method, path: req.URL.Path, body: string(body)})
+	t.mu.Unlock()
+
+	resp := `{"status":"Succeeded","properties":{"sessionId":"session-abc"}}`
+	return &http.Response{
+		StatusCode: http.StatusOK,
+		Header:     http.Header{"Content-Type": []string{"application/json"}},
+		Body:       io.NopCloser(strings.NewReader(resp)),
+		Request:    req,
+	}, nil
+}
+
+func (t *exchangeSDKTransport) recorded() []exchangeSDKRequest {
+	t.mu.Lock()
+	defer t.mu.Unlock()
+	return append([]exchangeSDKRequest(nil), t.requests...)
+}
+
+// sdkExchangeClient returns a client whose CalculateExchange and
+// ExecuteExchange go through the production SDK wrappers and the SDK's own
+// JSON serialization, ending at the fake transport instead of Azure.
+func sdkExchangeClient(t *testing.T) (*compute.Client, *exchangeSDKTransport) {
+	t.Helper()
+	transport := &exchangeSDKTransport{}
+	client := compute.NewClient(inventorySDKCredential{}, "sub-1", "eastus")
+	client.SetExchangeClientOptionsForTest(&arm.ClientOptions{ClientOptions: azcore.ClientOptions{
+		Transport: transport,
+		Retry:     policy.RetryOptions{MaxRetries: -1},
+	}})
+	return client, transport
+}
+
+// purchaseBillingPlans decodes a recorded CalculateExchange body and returns
+// the raw billingPlan value of each reservationsToPurchase entry.
+func purchaseBillingPlans(t *testing.T, body string) []string {
+	t.Helper()
+	var decoded struct {
+		Properties struct {
+			ReservationsToPurchase []struct {
+				Properties map[string]json.RawMessage `json:"properties"`
+			} `json:"reservationsToPurchase"`
+		} `json:"properties"`
+	}
+	require.NoError(t, json.Unmarshal([]byte(body), &decoded))
+	plans := make([]string, 0, len(decoded.Properties.ReservationsToPurchase))
+	for _, p := range decoded.Properties.ReservationsToPurchase {
+		var plan string
+		require.NoError(t, json.Unmarshal(p.Properties["billingPlan"], &plan))
+		plans = append(plans, plan)
+	}
+	return plans
+}
+
+// calculateAndExecute runs the full priced-then-committed flow the platform
+// handler runs, and returns the two recorded requests.
+func calculateAndExecute(t *testing.T, targets []compute.ExchangeTarget) (calculate, exchange exchangeSDKRequest) {
+	t.Helper()
+	client, transport := sdkExchangeClient(t)
+	preview, _, err := client.CalculateExchange(context.Background(), []compute.ExchangeableReservation{validSource()}, targets)
+	require.NoError(t, err)
+	_, err = client.ExecuteExchange(context.Background(), preview.SessionID)
+	require.NoError(t, err)
+
+	reqs := transport.recorded()
+	require.Len(t, reqs, 2)
+	assert.Equal(t, http.MethodPost, reqs[0].method)
+	assert.Equal(t, "/providers/Microsoft.Capacity/calculateExchange", reqs[0].path)
+	assert.Equal(t, http.MethodPost, reqs[1].method)
+	assert.Equal(t, "/providers/Microsoft.Capacity/exchange", reqs[1].path)
+	return reqs[0], reqs[1]
+}
+
+// Issue #89: an unset BillingPlan must keep sending exactly the request
+// every exchange sent before the field existed -- Upfront -- so existing
+// callers are unaffected.
+func TestExchangeSDK_UnsetBillingPlanKeepsUpfrontWireBody(t *testing.T) {
+	calculate, exchange := calculateAndExecute(t, []compute.ExchangeTarget{validTarget()})
+
+	const wantCalculate = `{"properties":{"reservationsToExchange":[` +
+		`{"quantity":2,"reservationId":"/providers/Microsoft.Capacity/reservationOrders/order-1/reservations/res-1"}],` +
+		`"reservationsToPurchase":[{"location":"eastus","properties":{"appliedScopeType":"Shared","billingPlan":"Upfront",` +
+		`"billingScopeId":"/subscriptions/sub-1","quantity":1,"renew":false,` +
+		`"reservedResourceProperties":{"instanceFlexibility":"On"},"reservedResourceType":"VirtualMachines","term":"P1Y"},` +
+		`"sku":{"name":"Standard_D4s_v3"}}]}}`
+	assert.JSONEq(t, wantCalculate, calculate.body)
+	assert.JSONEq(t, `{"properties":{"sessionId":"session-abc"}}`, exchange.body)
+}
+
+// Issue #89: a Monthly target must be priced as Monthly. The exchange
+// request carries only the session ID, so the Monthly plan priced in that
+// session is the one Azure commits; nothing on the execute leg can turn it
+// back into Upfront.
+func TestExchangeSDK_MonthlyBillingPlanReachesWire(t *testing.T) {
+	monthly := validTarget()
+	monthly.BillingPlan = armreservations.ReservationBillingPlanMonthly
+	upfront := validTarget()
+	upfront.BillingPlan = armreservations.ReservationBillingPlanUpfront
+
+	calculate, exchange := calculateAndExecute(t, []compute.ExchangeTarget{monthly, upfront})
+
+	assert.Equal(t, []string{"Monthly", "Upfront"}, purchaseBillingPlans(t, calculate.body),
+		"each target must keep its own billing plan")
+	assert.JSONEq(t, `{"properties":{"sessionId":"session-abc"}}`, exchange.body,
+		"execute must commit the priced session, not re-send purchase terms")
+}
+
+// Issue #89: an unrecognized billing plan is a validation error raised
+// before any HTTP request, never a silent Upfront purchase.
+func TestExchangeSDK_InvalidBillingPlanRejectedBeforeHTTP(t *testing.T) {
+	for _, plan := range []armreservations.ReservationBillingPlan{"monthly", "UPFRONT", "Annual", " Monthly"} {
+		t.Run(string(plan), func(t *testing.T) {
+			client, transport := sdkExchangeClient(t)
+			bad := validTarget()
+			bad.BillingPlan = plan
+
+			preview, offerings, err := client.CalculateExchange(context.Background(),
+				[]compute.ExchangeableReservation{validSource()},
+				[]compute.ExchangeTarget{validTarget(), bad})
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), `targets[1].billing_plan "`+string(plan)+`" is not a supported billing plan`)
+			assert.Nil(t, preview)
+			assert.Nil(t, offerings)
+			assert.Empty(t, transport.recorded(), "no request may reach Azure for an invalid billing plan")
+		})
+	}
+}

providers/azure/services/compute/exchange_operations.go

@@ -129,6 +129,22 @@ type ExchangeTarget struct {
 	// BillingScopeID is the subscription or billing account that will be
 	// charged. Required by the Azure exchange API.
 	BillingScopeID string
+
+	// BillingPlan is how the purchased reservation is paid for. Optional:
+	// unset ("") means armreservations.ReservationBillingPlanUpfront, the
+	// plan every exchange used before this field existed, so existing
+	// callers send the same request as before. Any other value must be one
+	// of armreservations.PossibleReservationBillingPlanValues() ("Upfront"
+	// or "Monthly", exact spelling); anything else is a validation error,
+	// never a silent fallback to Upfront.
+	//
+	// Azure does not require the plan to match the returned reservations':
+	// a monthly-billed reservation can be exchanged for an upfront one and
+	// vice versa (the new reservation's total commitment must still cover
+	// the returned remaining commitment). The plan is fixed by the
+	// CalculateExchange session; ExecuteExchange sends only the session ID,
+	// so the executed exchange is billed with the plan that was priced.
+	BillingPlan armreservations.ReservationBillingPlan
 }
 
 // CalculateExchangeCallerFunc is the narrow LRO-invoker interface that
@@ -167,7 +183,7 @@ func (c *Client) buildCalculateExchangeCaller() (CalculateExchangeCallerFunc, er
 	if c.calculateExchangeCaller != nil {
 		return c.calculateExchangeCaller, nil
 	}
-	client, err := armreservations.NewCalculateExchangeClient(c.cred, nil)
+	client, err := armreservations.NewCalculateExchangeClient(c.cred, c.exchangeClientOptions)
 	if err != nil {
 		return nil, fmt.Errorf("azure: create CalculateExchange client: %w", err)
 	}
@@ -190,7 +206,7 @@ func (c *Client) buildDoExchangeCaller() (DoExchangeCallerFunc, error) {
 	if c.doExchangeCaller != nil {
 		return c.doExchangeCaller, nil
 	}
-	client, err := armreservations.NewExchangeClient(c.cred, nil)
+	client, err := armreservations.NewExchangeClient(c.cred, c.exchangeClientOptions)
 	if err != nil {
 		return nil, fmt.Errorf("azure: create Exchange client: %w", err)
 	}
@@ -219,10 +235,12 @@ func (c *Client) buildDoExchangeCaller() (DoExchangeCallerFunc, error) {
 // same applied scope as the others (Shared, or Single with its scopes); the
 // purchased reservations get that scope. Every
 // target must have a non-empty SKU/Location/BillingScopeID, Quantity >= 1,
-// and a Term from PossibleReservationTermValues(). There is no coercion of
-// invalid values (no clamping quantity to 1, no defaulting an unrecognized
-// term) -- a caller mistake here is a validation error, not a silently
-// different exchange than the one requested.
+// a Term from PossibleReservationTermValues(), and a BillingPlan that is
+// either unset (Upfront) or one of PossibleReservationBillingPlanValues().
+// There is no coercion of invalid values (no clamping quantity to 1, no
+// defaulting an unrecognized term or billing plan) -- a caller mistake here
+// is a validation error, not a silently different exchange than the one
+// requested.
 //
 // Returns an error only when validation or the API call itself fails; a
 // priced-but-policy-rejected combination is a successful call whose
@@ -417,10 +435,36 @@ func validateExchangeTargets(targets []ExchangeTarget) error {
 		if !isValidReservationTerm(t.Term) {
 			return fmt.Errorf("azure: CalculateExchange: targets[%d].term %q is not a supported reservation term", i, t.Term)
 		}
+		if !isValidBillingPlan(billingPlanOrDefault(t.BillingPlan)) {
+			return fmt.Errorf("azure: CalculateExchange: targets[%d].billing_plan %q is not a supported billing plan", i, t.BillingPlan)
+		}
 	}
 	return nil
 }
 
+// billingPlanOrDefault maps an unset billing plan to Upfront, the plan every
+// exchange used before ExchangeTarget.BillingPlan existed. Any set value is
+// returned unchanged, so an unrecognized one still fails isValidBillingPlan
+// instead of being silently priced as Upfront.
+func billingPlanOrDefault(plan armreservations.ReservationBillingPlan) armreservations.ReservationBillingPlan {
+	if plan == "" {
+		return armreservations.ReservationBillingPlanUpfront
+	}
+	return plan
+}
+
+// isValidBillingPlan reports whether plan is one of the SDK's typed enum
+// values (exact spelling), rather than accepting any string the caller
+// happens to pass.
+func isValidBillingPlan(plan armreservations.ReservationBillingPlan) bool {
+	for _, p := range armreservations.PossibleReservationBillingPlanValues() {
+		if p == plan {
+			return true
+		}
+	}
+	return false
+}
+
 // isValidReservationTerm reports whether term is one of the SDK's typed
 // enum values, rather than accepting any string the caller happens to pass.
 func isValidReservationTerm(term armreservations.ReservationTerm) bool {
@@ -436,7 +480,8 @@ func isValidReservationTerm(term armreservations.ReservationTerm) bool {
 // SDK request shape, using typed SDK enum constants throughout
 // (feedback_sdk_enum_string_literals) rather than raw strings. Every target
 // inherits the sources' applied scope, which validateExchangeSources has
-// checked is present and shared by all sources.
+// checked is present and shared by all sources, and carries its own billing
+// plan (Upfront when unset), which validateExchangeTargets has checked.
 func buildCalculateExchangeRequest(sources []ExchangeableReservation, targets []ExchangeTarget) armreservations.CalculateExchangeRequest {
 	scope := sources[0]
 	toReturn := make([]*armreservations.ReservationToReturn, 0, len(sources))
@@ -464,7 +509,7 @@ func buildCalculateExchangeRequest(sources []ExchangeableReservation, targets []
 			Properties: &armreservations.PurchaseRequestProperties{
 				AppliedScopeType:     to.Ptr(scope.AppliedScopeType),
 				AppliedScopes:        appliedScopes,
-				BillingPlan:          to.Ptr(armreservations.ReservationBillingPlanUpfront),
+				BillingPlan:          to.Ptr(billingPlanOrDefault(tgt.BillingPlan)),
 				BillingScopeID:       to.Ptr(tgt.BillingScopeID),
 				Quantity:             to.Ptr(tgt.Quantity),
 				Renew:                to.Ptr(false),

providers/azure/services/compute/export_test.go

@@ -0,0 +1,10 @@
+package compute
+
+import "github.com/Azure/azure-sdk-for-go/sdk/azcore/arm"
+
+// SetExchangeClientOptionsForTest points the real CalculateExchange and
+// Exchange SDK clients at caller-supplied ARM options (a fake transport), so
+// tests exercise the production LRO wrappers without reaching Azure.
+func (c *Client) SetExchangeClientOptionsForTest(opts *arm.ClientOptions) {
+	c.exchangeClientOptions = opts
+}