diff --git a/go.mod b/go.mod index 9d63b4711..92b3542a9 100644 --- a/go.mod +++ b/go.mod @@ -84,9 +84,9 @@ require ( github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations v1.1.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azkeys v1.4.0 github.com/Azure/azure-sdk-for-go/sdk/security/keyvault/azsecrets v1.4.0 - github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260929105827-b3b4cb5e3d80 + github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261006104817-90e61e668b99 github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695 - github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928214714-ce9513612901 + github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007130138-e24345ce68f8 github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901 github.com/aws/aws-lambda-go v1.47.0 github.com/aws/aws-sdk-go-v2/service/kms v1.50.4 diff --git a/go.sum b/go.sum index 85adcd71b..d9fc83f03 100644 --- a/go.sum +++ b/go.sum @@ -92,12 +92,12 @@ github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0 github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/cloudmock v0.54.0/go.mod h1:vB2GH9GAYYJTO3mEn8oYwzEdhlayZIdQz6zdzgUIRvA= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0 h1:s0WlVbf9qpvkh1c/uDAPElam0WrL7fHRIidgZJ7UqZI= github.com/GoogleCloudPlatform/opentelemetry-operations-go/internal/resourcemapping v0.54.0/go.mod h1:Mf6O40IAyB9zR/1J8nGDDPirZQQPbYJni8Yisy7NTMc= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260929105827-b3b4cb5e3d80 h1:wVKlMokfaME/Lw525Qz3F155R3nY4VmuaIyh+6d4sCU= -github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20260929105827-b3b4cb5e3d80/go.mod h1:ApWBliDXe099f3oDXBz41K/I9v4bHvn1dG/BGoRmHlw= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261006104817-90e61e668b99 h1:inu/nYSAVY6cLsTn4qc64RIbCsymURLlEdgkhha79cw= +github.com/LeanerCloud/cloud-commitments-go/pkg v0.0.0-20261006104817-90e61e668b99/go.mod h1:ApWBliDXe099f3oDXBz41K/I9v4bHvn1dG/BGoRmHlw= github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695 h1:DfNEBzFS7/MaBZljLGRRRUaarfnXXZyBE/6iozzp9/k= github.com/LeanerCloud/cloud-commitments-go/providers/aws v0.0.0-20261003204812-9962786e0695/go.mod h1:d4nsy61/Ptib0SxqMg0ble3yksVm3+QbtUtmGja82PA= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928214714-ce9513612901 h1:iSdHYdmGUjcjtSmgGZxptBzDuLrJ9KR2mh1/x7FNvSY= -github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20260928214714-ce9513612901/go.mod h1:zgCL/ozOkcZUDbEC7a2UwW+6MPcLoBlEU2WUZ0TORUs= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007130138-e24345ce68f8 h1:4qx5bagtW25H5X+whaZ/3tWXP7VGyG/qcypTh8yK7Hc= +github.com/LeanerCloud/cloud-commitments-go/providers/azure v0.0.0-20261007130138-e24345ce68f8/go.mod h1:iIvPuWLWd+SXqLHAQNdodYMUAAcq7PnwfScXWgARt5A= github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901 h1:i6OwXLUheudN3GfwnYXdKuEq8vPdE9qkNJ+r71LRLKA= github.com/LeanerCloud/cloud-commitments-go/providers/gcp v0.0.0-20260928214714-ce9513612901/go.mod h1:UnMqDe2mBUHHF6gnKAgJn9/JeXa7WZqQxfWPs4unGrg= github.com/Microsoft/go-winio v0.6.2 h1:F2VQgta7ecxGYO8k3ZZz3RS8fVIXVxONVUPlNERoyfY= diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index e379bc370..30208fb71 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -638,13 +638,17 @@ func validateAzureExecuteBody(body AzureExecuteExchangeRequestBody) error { return nil } -// toAzureExchangeSources converts the HTTP-shaped sources into the -// provider-layer shape. Pure field mapping; validateAzureExchangeSources -// must be called first. -func toAzureExchangeSources(sources []AzureExchangeSourceBody) []azurecompute.ExchangeableReservation { +// toAzureExchangeSources keeps the server-listed reservation metadata and +// applies the requested quantities after requireAzureSourceOwnership succeeds. +func toAzureExchangeSources(sources []AzureExchangeSourceBody, owned []azurecompute.ExchangeableReservation) []azurecompute.ExchangeableReservation { + byID := make(map[string]azurecompute.ExchangeableReservation, len(owned)) + for i := range owned { + byID[strings.ToLower(owned[i].ReservationID)] = owned[i] + } out := make([]azurecompute.ExchangeableReservation, len(sources)) for i, s := range sources { - out[i] = azurecompute.ExchangeableReservation{ReservationID: s.ReservationID, Quantity: s.Quantity} + out[i] = byID[strings.ToLower(s.ReservationID)] + out[i].Quantity = s.Quantity } return out } @@ -866,12 +870,15 @@ func requireAzureSourceOwnership(owned []azurecompute.ExchangeableReservation, s // applies requireAzureSourceOwnership. Split from the pure check so the // authorization rule itself is testable without a client, and so both the // pricing and execute endpoints share one code path. -func checkAzureSourceOwnership(ctx context.Context, client azureExchangeClient, sources []AzureExchangeSourceBody, subscriptionID string) error { +func checkAzureSourceOwnership(ctx context.Context, client azureExchangeClient, sources []AzureExchangeSourceBody, subscriptionID string) ([]azurecompute.ExchangeableReservation, error) { owned, err := listOwnedAzureReservations(ctx, client) if err != nil { - return err + return nil, err } - return requireAzureSourceOwnership(owned, sources, subscriptionID) + if err := requireAzureSourceOwnership(owned, sources, subscriptionID); err != nil { + return nil, err + } + return toAzureExchangeSources(sources, owned), nil } // listOwnedAzureReservations fetches the tenant-wide reservation listing that @@ -925,8 +932,9 @@ func (h *Handler) getAzureCompatibleOfferings(ctx context.Context, req *events.L return nil, NewClientError(404, fmt.Sprintf("no Azure account registered for subscription %q", body.SubscriptionID)) } - if ownErr := checkAzureSourceOwnership(ctx, client, body.Sources, body.SubscriptionID); ownErr != nil { - return nil, ownErr + sources, err := checkAzureSourceOwnership(ctx, client, body.Sources, body.SubscriptionID) + if err != nil { + return nil, err } targets, err := toAzureExchangeTargets(body.Targets, body.SubscriptionID) @@ -934,7 +942,7 @@ func (h *Handler) getAzureCompatibleOfferings(ctx context.Context, req *events.L return nil, err } - preview, offerings, err := client.CalculateExchange(ctx, toAzureExchangeSources(body.Sources), targets) + preview, offerings, err := client.CalculateExchange(ctx, sources, targets) if err != nil { logging.Errorf("azure compatible offerings failed: %v", err) return nil, mapAzureExchangeError("failed to find compatible offerings", err) @@ -983,7 +991,7 @@ const azureMaxPurchaseAmountCurrency = "USD" // It also sharpens exchangeRegions: every source reaching it is // known-owned, so unknownRegionConstraint means only "owned, but Azure // reported no region" rather than doubling as "not yours" or "not real". -func (h *Handler) authorizeAzureExchangeExecution(ctx context.Context, session *Session, body AzureExecuteExchangeRequestBody, maxRat *big.Rat) (azureExchangeClient, error) { +func (h *Handler) authorizeAzureExchangeExecution(ctx context.Context, session *Session, body AzureExecuteExchangeRequestBody, maxRat *big.Rat) (azureExchangeClient, []azurecompute.ExchangeableReservation, error) { // Scope check MUST precede building the client (mirrors // getAzureCompatibleOfferings): otherwise an unregistered subscription // (distinguishable 404: "no Azure account registered...") and a @@ -992,35 +1000,35 @@ func (h *Handler) authorizeAzureExchangeExecution(ctx context.Context, session * // and credentials for an out-of-scope account could be resolved before // the denial. if scopeErr := h.requireAzureSubscriptionScope(ctx, session, body.SubscriptionID); scopeErr != nil { - return nil, scopeErr + return nil, nil, scopeErr } client, err := h.buildAzureExchangeClient(ctx, body.SubscriptionID) if err != nil { - return nil, fmt.Errorf("failed to build Azure exchange client: %w", err) + return nil, nil, fmt.Errorf("failed to build Azure exchange client: %w", err) } if client == nil { - return nil, NewClientError(404, fmt.Sprintf("no Azure account registered for subscription %q", body.SubscriptionID)) + return nil, nil, NewClientError(404, fmt.Sprintf("no Azure account registered for subscription %q", body.SubscriptionID)) } owned, err := listOwnedAzureReservations(ctx, client) if err != nil { - return nil, err + return nil, nil, err } if ownErr := requireAzureSourceOwnership(owned, body.Sources, body.SubscriptionID); ownErr != nil { - return nil, ownErr + return nil, nil, ownErr } accountID, err := h.resolveAzureExchangeAccountID(ctx, body.SubscriptionID) if err != nil { - return nil, err + return nil, nil, err } if err := h.checkAzureExecuteConstraints(ctx, session, body, accountID, maxRat, exchangeRegions(body.Targets, body.Sources, owned)); err != nil { - return nil, err + return nil, nil, err } - return client, nil + return client, toAzureExchangeSources(body.Sources, owned), nil } // resolveAzureExchangeAccountID looks up the CloudAccount registered for @@ -1189,7 +1197,7 @@ func (h *Handler) executeAzureExchange(ctx context.Context, req *events.LambdaFu // authorizeAzureExchangeExecution applies every gate: allowed_accounts // scope, source ownership (issue #1527) and the execute:ri-exchange // Constraints, all before the pricing call below. - client, err := h.authorizeAzureExchangeExecution(ctx, session, body, maxRat) + client, sources, err := h.authorizeAzureExchangeExecution(ctx, session, body, maxRat) if err != nil { return nil, err } @@ -1199,7 +1207,7 @@ func (h *Handler) executeAzureExchange(ctx context.Context, req *events.LambdaFu return nil, err } - preview, _, err := client.CalculateExchange(ctx, toAzureExchangeSources(body.Sources), targets) + preview, _, err := client.CalculateExchange(ctx, sources, targets) if err != nil { logging.Errorf("azure exchange re-quote failed: %v", err) return nil, mapAzureExchangeError("failed to price the exchange before execution", err) diff --git a/internal/api/handler_ri_exchange_azure_scope_test.go b/internal/api/handler_ri_exchange_azure_scope_test.go new file mode 100644 index 000000000..87718c5c4 --- /dev/null +++ b/internal/api/handler_ri_exchange_azure_scope_test.go @@ -0,0 +1,187 @@ +package api + +import ( + "context" + "encoding/json" + "strings" + "testing" + + "github.com/Azure/azure-sdk-for-go/sdk/resourcemanager/reservations/armreservations" + azurecompute "github.com/LeanerCloud/cloud-commitments-go/providers/azure/services/compute" + "github.com/aws/aws-lambda-go/events" + "github.com/stretchr/testify/require" +) + +type azureScopeExchangeClient struct { + *azurecompute.Client + owned []azurecompute.ExchangeableReservation + priced []azurecompute.ExchangeableReservation + listCalls int +} + +func (c *azureScopeExchangeClient) ListExchangeableReservations(context.Context) ([]azurecompute.ExchangeableReservation, error) { + c.listCalls++ + return c.owned, nil +} + +func (c *azureScopeExchangeClient) CalculateExchange(ctx context.Context, sources []azurecompute.ExchangeableReservation, targets []azurecompute.ExchangeTarget) (*azurecompute.ExchangePreview, []azurecompute.CompatibleOffering, error) { + c.priced = sources + return c.Client.CalculateExchange(ctx, sources, targets) +} + +func azureScopeRequest(t *testing.T, sources []AzureExchangeSourceBody) *events.LambdaFunctionURLRequest { + t.Helper() + var body AzureExecuteExchangeRequestBody + require.NoError(t, json.Unmarshal([]byte(validAzureExecuteBody), &body)) + body.Sources = sources + encoded, err := json.Marshal(body) + require.NoError(t, err) + return &events.LambdaFunctionURLRequest{Headers: map[string]string{"authorization": "Bearer tok"}, Body: string(encoded)} +} + +func azureScopeHandler(t *testing.T, execute bool, client azureExchangeClient) *Handler { + t.Helper() + if execute { + return newAzureExecuteSourceGateHandler(t, client) + } + return &Handler{ + auth: scopedAzureAuth(t, "view", "purchases", nil), + azureExchangeFactory: func(string) azureExchangeClient { return client }, + } +} + +func TestAzureExchangeScopePreserved(t *testing.T) { + for _, execute := range []bool{false, true} { + for _, scope := range []armreservations.AppliedScopeType{armreservations.AppliedScopeTypeShared, armreservations.AppliedScopeTypeSingle} { + name := "preview/" + string(scope) + if execute { + name = "execute/" + string(scope) + } + t.Run(name, func(t *testing.T) { + owned := azurecompute.ExchangeableReservation{ + ReservationID: "res-1", ReservationOrderID: "order-1", + BillingScopeID: "/subscriptions/sub-1", AppliedScopeType: scope, + SKU: "Standard_D2s_v3", Region: "eastus", Quantity: 4, + } + if scope == armreservations.AppliedScopeTypeSingle { + owned.AppliedScopes = []string{"/subscriptions/discount-sub/resourceGroups/discount-rg"} + } + client := &azureScopeExchangeClient{Client: new(azurecompute.Client), owned: []azurecompute.ExchangeableReservation{owned}} + calculateCalls, executeCalls := 0, 0 + client.SetCalculateExchangeCaller(func(_ context.Context, request armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + calculateCalls++ + props := request.Properties + require.NotNil(t, props) + require.Len(t, props.ReservationsToExchange, 1) + require.Equal(t, owned.ReservationID, *props.ReservationsToExchange[0].ReservationID) + require.Equal(t, int32(2), *props.ReservationsToExchange[0].Quantity) + require.Len(t, props.ReservationsToPurchase, 1) + target := props.ReservationsToPurchase[0].Properties + require.Equal(t, scope, *target.AppliedScopeType) + require.Equal(t, "/subscriptions/sub-1", *target.BillingScopeID) + if scope == armreservations.AppliedScopeTypeSingle { + require.Len(t, target.AppliedScopes, 1) + require.Equal(t, owned.AppliedScopes[0], *target.AppliedScopes[0]) + } else { + require.Nil(t, target.AppliedScopes) + } + return armreservations.CalculateExchangeOperationResultResponse{Properties: &armreservations.CalculateExchangeResponseProperties{ + SessionID: toPtr("scope-session"), NetPayable: &armreservations.Price{Amount: toPtr(10.0), CurrencyCode: toPtr("USD")}, + }}, nil + }) + client.SetDoExchangeCaller(func(_ context.Context, sessionID string) (armreservations.ExchangeOperationResultResponse, error) { + executeCalls++ + require.Equal(t, "scope-session", sessionID) + return armreservations.ExchangeOperationResultResponse{Properties: &armreservations.ExchangeResponseProperties{}, Status: toPtr(armreservations.ExchangeOperationResultStatusSucceeded)}, nil + }) + h := azureScopeHandler(t, execute, client) + req := azureScopeRequest(t, []AzureExchangeSourceBody{{ReservationID: strings.ToUpper(owned.ReservationID), Quantity: 2}}) + var err error + if execute { + _, err = h.executeAzureExchange(context.Background(), req) + } else { + _, err = h.getAzureCompatibleOfferings(context.Background(), req) + } + require.NoError(t, err) + require.Equal(t, 1, calculateCalls) + require.Equal(t, 1, client.listCalls) + want := owned + want.Quantity = 2 + require.Equal(t, []azurecompute.ExchangeableReservation{want}, client.priced) + require.Equal(t, []azurecompute.ExchangeableReservation{owned}, client.owned) + if execute { + require.Equal(t, 1, executeCalls) + } else { + require.Zero(t, executeCalls) + } + }) + } + } +} + +func TestAzureExchangeScopeRefused(t *testing.T) { + base := azurecompute.ExchangeableReservation{ReservationID: "res-1", BillingScopeID: "/subscriptions/sub-1", AppliedScopeType: armreservations.AppliedScopeTypeShared, Region: "eastus", Quantity: 4} + missing := base + missing.AppliedScopeType = "" + unsupported := base + unsupported.AppliedScopeType = armreservations.AppliedScopeType("ManagementGroup") + singleMissing := base + singleMissing.AppliedScopeType = armreservations.AppliedScopeTypeSingle + mixed := base + mixed.ReservationID = "res-2" + mixed.AppliedScopeType = armreservations.AppliedScopeTypeSingle + mixed.AppliedScopes = []string{"/subscriptions/sub-1"} + foreign := base + foreign.BillingScopeID = "/subscriptions/foreign" + for _, execute := range []bool{false, true} { + for _, tc := range []struct { + name string + owned []azurecompute.ExchangeableReservation + foreign bool + }{ + {name: "missing", owned: []azurecompute.ExchangeableReservation{missing}}, + {name: "unsupported", owned: []azurecompute.ExchangeableReservation{unsupported}}, + {name: "single_missing_scopes", owned: []azurecompute.ExchangeableReservation{singleMissing}}, + {name: "mixed", owned: []azurecompute.ExchangeableReservation{base, mixed}}, + {name: "foreign_billing", owned: []azurecompute.ExchangeableReservation{foreign}, foreign: true}, + } { + name := "preview/" + tc.name + if execute { + name = "execute/" + tc.name + } + t.Run(name, func(t *testing.T) { + client := &azureScopeExchangeClient{Client: new(azurecompute.Client), owned: tc.owned} + client.SetCalculateExchangeCaller(func(context.Context, armreservations.CalculateExchangeRequest) (armreservations.CalculateExchangeOperationResultResponse, error) { + t.Fatal("a rejected source must not reach the calculate SDK caller") + return armreservations.CalculateExchangeOperationResultResponse{}, nil + }) + client.SetDoExchangeCaller(func(context.Context, string) (armreservations.ExchangeOperationResultResponse, error) { + t.Fatal("a rejected source must not execute an exchange") + return armreservations.ExchangeOperationResultResponse{}, nil + }) + sources := make([]AzureExchangeSourceBody, len(tc.owned)) + for i := range tc.owned { + sources[i] = AzureExchangeSourceBody{ReservationID: tc.owned[i].ReservationID, Quantity: 1} + } + h := azureScopeHandler(t, execute, client) + req := azureScopeRequest(t, sources) + var err error + if execute { + _, err = h.executeAzureExchange(context.Background(), req) + } else { + _, err = h.getAzureCompatibleOfferings(context.Background(), req) + } + require.Error(t, err) + require.Equal(t, 1, client.listCalls) + if tc.foreign { + ce, ok := IsClientError(err) + require.True(t, ok) + require.Equal(t, 403, ce.code) + require.Nil(t, client.priced) + } else { + require.Len(t, client.priced, len(sources)) + } + }) + } + } +} diff --git a/internal/server/handler_ri_exchange_test.go b/internal/server/handler_ri_exchange_test.go index f072b9066..1b9b0dd86 100644 --- a/internal/server/handler_ri_exchange_test.go +++ b/internal/server/handler_ri_exchange_test.go @@ -551,6 +551,7 @@ func TestExecuteRIExchangeReshape_ManualMode(t *testing.T) { IsValidExchange: true, PaymentDueUSD: new(big.Rat).SetFloat64(5.00), PaymentDueUSDStr: "5.00", + CurrencyCode: "USD", }, nil }, }, @@ -641,6 +642,7 @@ func TestExecuteRIExchangeReshape_AutoMode(t *testing.T) { IsValidExchange: true, PaymentDueUSD: new(big.Rat).SetFloat64(3.50), PaymentDueUSDStr: "3.50", + CurrencyCode: "USD", }, nil }, executeFunc: func(ctx context.Context, req exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { @@ -648,6 +650,7 @@ func TestExecuteRIExchangeReshape_AutoMode(t *testing.T) { IsValidExchange: true, PaymentDueUSD: new(big.Rat).SetFloat64(3.50), PaymentDueUSDStr: "3.50", + CurrencyCode: "USD", }, nil }, }, @@ -745,6 +748,7 @@ func TestExecuteRIExchangeReshape_DailyCapHitMidRun(t *testing.T) { IsValidExchange: true, PaymentDueUSD: new(big.Rat).SetFloat64(8.00), PaymentDueUSDStr: "8.00", + CurrencyCode: "USD", }, nil }, executeFunc: func(ctx context.Context, req exchange.ExchangeExecuteRequest) (string, *exchange.ExchangeQuoteSummary, error) { @@ -752,6 +756,7 @@ func TestExecuteRIExchangeReshape_DailyCapHitMidRun(t *testing.T) { IsValidExchange: true, PaymentDueUSD: new(big.Rat).SetFloat64(8.00), PaymentDueUSDStr: "8.00", + CurrencyCode: "USD", }, nil }, },