diff --git a/CHANGELOG.md b/CHANGELOG.md index 086a05a8..d479ac09 100644 --- a/CHANGELOG.md +++ b/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 diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index 2e0c38b9..23b9bc70 100644 --- a/providers/azure/services/compute/client.go +++ b/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. diff --git a/providers/azure/services/compute/exchange_billing_plan_sdk_test.go b/providers/azure/services/compute/exchange_billing_plan_sdk_test.go new file mode 100644 index 00000000..564d2e62 --- /dev/null +++ b/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") + }) + } +} diff --git a/providers/azure/services/compute/exchange_operations.go b/providers/azure/services/compute/exchange_operations.go index 4fa2713e..298b8a36 100644 --- a/providers/azure/services/compute/exchange_operations.go +++ b/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), diff --git a/providers/azure/services/compute/export_test.go b/providers/azure/services/compute/export_test.go new file mode 100644 index 00000000..6a4e9664 --- /dev/null +++ b/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 +}