From a2674b724c948e620ca6ac3704ed8893d89e00a8 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 3 Jun 2026 15:31:22 +0200 Subject: [PATCH] fix(ri-exchange): resolve Azure credentials via per-subscription resolver buildAzureExchangeClient was calling azidentity.NewDefaultAzureCredential directly. In Lambda there is no ambient Azure identity so credential construction fails and the handler returns a 500, surfaced on the RI Exchange page as "Failed to load Azure reservations: Internal server error". Replace with credentials.ResolveAzureTokenCredentialWithOpts scoped to the registered CloudAccount whose ExternalID matches the requested subscription_id, mirroring the exact pattern used by scheduler.collectAzureForAccount and purchase/execution.go resolveAzureProvider. The Handler's wired credStore, signer, and issuerURL are forwarded so managed_identity, client_secret, and WIF auth modes all work correctly. Error classification: DB/credential failures are server-side faults (5xx); no matching CloudAccount is a graceful empty state (not an error) so the page shows "no reservations" instead of an opaque 500 banner when Azure is not configured for the tenant. Closes #932 --- internal/api/handler_ri_exchange.go | 62 +++++++-- internal/api/handler_ri_exchange_test.go | 167 +++++++++++++++++++++++ 2 files changed, 218 insertions(+), 11 deletions(-) diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index e7046cbc8..3ca47b05f 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -12,7 +12,6 @@ import ( "strconv" "time" - "github.com/Azure/azure-sdk-for-go/sdk/azidentity" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" awsconfig "github.com/aws/aws-sdk-go-v2/config" @@ -20,6 +19,7 @@ import ( "github.com/LeanerCloud/CUDly/internal/auth" "github.com/LeanerCloud/CUDly/internal/config" + "github.com/LeanerCloud/CUDly/internal/credentials" "github.com/LeanerCloud/CUDly/pkg/exchange" "github.com/LeanerCloud/CUDly/pkg/logging" awsprovider "github.com/LeanerCloud/CUDly/providers/aws" @@ -171,17 +171,50 @@ type azureExchangeClient interface { } // buildAzureExchangeClient returns the injected factory result when one has -// been set (test path), or constructs a real Azure compute client otherwise -// (production path). Returns an error when the real Azure credential cannot -// be obtained. -func (h *Handler) buildAzureExchangeClient(subscriptionID string) (azureExchangeClient, error) { +// been set (test path), or constructs a real Azure compute client by resolving +// per-subscription credentials via the project's credential resolver (production +// path). +// +// Credential resolution mirrors every other Azure call in the project +// (scheduler.collectAzureForAccount, purchase/execution.go resolveAzureProvider): +// look up the registered CloudAccount whose ExternalID matches subscriptionID, +// then call credentials.ResolveAzureTokenCredentialWithOpts with the Handler's +// wired OIDC signer so managed_identity / client_secret / WIF all work. +// +// Graceful empty-state rules (returns nil client, nil error): +// - subscriptionID is empty AND no Azure accounts are registered at all: +// Azure is not configured; the caller returns an empty reservations list. +// - subscriptionID is provided but no matching CloudAccount is found: +// treated as "Azure not configured for this subscription". +// +// A genuine configuration error (missing credentials, auth failure) returns +// a non-nil error that the handler maps to a 500 with a clear message. +func (h *Handler) buildAzureExchangeClient(ctx context.Context, subscriptionID string) (azureExchangeClient, error) { if h.azureExchangeFactory != nil { return h.azureExchangeFactory(subscriptionID), nil } - cred, err := azidentity.NewDefaultAzureCredential(nil) + + // Look up the registered Azure CloudAccount by its subscription ID. + // For Azure, ExternalID stores the subscription ID (see handler_accounts.go: + // buildSelfAccountRequest sets ExternalID = si.ExternalID() = si.SubscriptionID). + account, err := h.config.GetCloudAccountByExternalID(ctx, "azure", subscriptionID) if err != nil { - return nil, fmt.Errorf("azure: obtain default credential: %w", err) + return nil, fmt.Errorf("azure: look up account for subscription %q: %w", subscriptionID, err) + } + if account == nil { + // No Azure account registered for this subscription. + // Return nil client so the caller emits a graceful empty state. + return nil, nil } + + cred, err := credentials.ResolveAzureTokenCredentialWithOpts(ctx, account, h.credStore, credentials.AzureResolveOptions{ + Signer: h.signer, + IssuerURL: h.issuerURL, + }) + if err != nil { + return nil, fmt.Errorf("azure: resolve credentials for subscription %q: %w", subscriptionID, err) + } + // Region is left empty -- ListExchangeableReservations uses the tenant- // wide armreservations API which is not scoped to a region. return azurecompute.NewClient(cred, subscriptionID, ""), nil @@ -191,9 +224,10 @@ func (h *Handler) buildAzureExchangeClient(subscriptionID string) (azureExchange // eligible for the cross-SKU/cross-region exchange flow (InstanceFlexibility // == On, ProvisioningState == Succeeded). Requires "view:purchases" permission. // -// The optional ?subscription_id= query parameter scopes the client to a -// specific subscription for the capacity-provider registration check; the -// listing itself is tenant-wide. +// The optional ?subscription_id= query parameter scopes the credential lookup +// to the matching registered CloudAccount. When no Azure account is configured +// for the requested subscription (or no subscription is specified and none are +// registered), the handler returns an empty reservations list rather than a 500. func (h *Handler) listExchangeableAzureRIs(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { if _, err := h.requirePermission(ctx, req, "view", "purchases"); err != nil { return nil, err @@ -201,10 +235,16 @@ func (h *Handler) listExchangeableAzureRIs(ctx context.Context, req *events.Lamb subscriptionID := req.QueryStringParameters["subscription_id"] - client, err := h.buildAzureExchangeClient(subscriptionID) + client, err := h.buildAzureExchangeClient(ctx, subscriptionID) if err != nil { return nil, fmt.Errorf("failed to build Azure exchange client: %w", err) } + if client == nil { + // No Azure account configured for this subscription (or no Azure accounts + // registered at all). Return an empty list rather than a 500 so the page + // renders a "no reservations" state instead of an opaque error banner. + return &ExchangeableAzureRIsResponse{Reservations: []azurecompute.ExchangeableReservation{}}, nil + } reservations, err := client.ListExchangeableReservations(ctx) if err != nil { diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index 014c9625a..88dd3175d 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -808,6 +808,173 @@ func TestListExchangeableAzureRIs_SubscriptionIDPassedToFactory(t *testing.T) { assert.Equal(t, "sub-abc", capturedSubID) } +// --- Azure credential-resolution path tests (issue #871) --- +// +// These tests exercise the production path of buildAzureExchangeClient, which +// was previously hardcoded to azidentity.NewDefaultAzureCredential. The fix +// routes through the project's per-subscription resolver so Lambda (which has +// no ambient Azure identity) stops 500ing. +// +// No real Azure calls are made: the MockConfigStore.GetCloudAccountByExternalID +// override supplies a fake CloudAccount, and the azureExchangeFactory is NOT +// wired — the test exercises the real buildAzureExchangeClient production path. +// The credential store returns nil from LoadRaw, which causes +// ResolveAzureTokenCredentialWithOpts to fail when the account uses client_secret +// mode (expected: we assert the error rather than a 500). For the graceful-empty +// path (no account registered) we assert a clean empty response. + +// TestListExchangeableAzureRIs_NoAzureAccountRegistered verifies that when no +// Azure CloudAccount is registered for the requested subscription, the handler +// returns an empty reservations list (graceful empty state) rather than a 500. +// This is the fix for issue #871: Lambda has no ambient Azure identity so the +// old DefaultAzureCredential call failed, producing an opaque 500. +func TestListExchangeableAzureRIs_NoAzureAccountRegistered(t *testing.T) { + mockStore := &MockConfigStore{} + // GetCloudAccountByExternalID returns (nil, nil) — no account for this sub. + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, externalID string) (*config.CloudAccount, error) { + require.Equal(t, "azure", provider, "provider must be azure") + require.Equal(t, "sub-not-registered", externalID) + return nil, nil + } + + h := &Handler{ + auth: &mockAuthForExchange{}, + config: mockStore, + // azureExchangeFactory is intentionally NOT set so the production + // buildAzureExchangeClient path runs (verifying it handles nil account). + } + + res, err := h.listExchangeableAzureRIs(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + QueryStringParameters: map[string]string{"subscription_id": "sub-not-registered"}, + }) + require.NoError(t, err, "missing Azure account must produce empty state, not an error") + resp, ok := res.(*ExchangeableAzureRIsResponse) + require.True(t, ok) + assert.Empty(t, resp.Reservations, "no Azure account registered must return empty reservation list") +} + +// TestListExchangeableAzureRIs_EmptySubscriptionNoAccounts verifies the graceful +// empty state when subscription_id is omitted and no Azure accounts are registered. +func TestListExchangeableAzureRIs_EmptySubscriptionNoAccounts(t *testing.T) { + mockStore := &MockConfigStore{} + // GetCloudAccountByExternalID with empty externalID also returns (nil, nil). + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, externalID string) (*config.CloudAccount, error) { + require.Equal(t, "azure", provider) + return nil, nil + } + + h := &Handler{ + auth: &mockAuthForExchange{}, + config: mockStore, + } + + res, err := h.listExchangeableAzureRIs(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + // No subscription_id parameter. + }) + require.NoError(t, err, "no Azure accounts + no subscription_id must return empty state") + resp, ok := res.(*ExchangeableAzureRIsResponse) + require.True(t, ok) + assert.Empty(t, resp.Reservations) +} + +// TestListExchangeableAzureRIs_AccountLookupError verifies that a DB error from +// GetCloudAccountByExternalID surfaces as an error (which maps to 500) rather +// than silently falling through. A DB outage is a server-side fault, not a +// client mistake, so 5xx is the correct classification. +func TestListExchangeableAzureRIs_AccountLookupError(t *testing.T) { + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, _ string) (*config.CloudAccount, error) { + return nil, fmt.Errorf("database connection lost") + } + + h := &Handler{ + auth: &mockAuthForExchange{}, + config: mockStore, + } + + _, err := h.listExchangeableAzureRIs(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + QueryStringParameters: map[string]string{"subscription_id": "sub-db-error"}, + }) + require.Error(t, err, "DB lookup failure must propagate as an error") + // Must NOT be a 4xx ClientError: DB failures are server-side faults. + _, isClientErr := IsClientError(err) + assert.False(t, isClientErr, "DB lookup failure must not be classified as a client (4xx) error") + assert.Contains(t, err.Error(), "database connection lost") +} + +// TestListExchangeableAzureRIs_CredentialResolutionError verifies that when a +// CloudAccount IS registered but credential resolution fails (e.g. missing stored +// secret), the error propagates as a server-side error. This guards against the +// regression where a misconfigured account silently returned an empty list +// instead of surfacing the configuration problem to the operator. +func TestListExchangeableAzureRIs_CredentialResolutionError(t *testing.T) { + const subID = "sub-cred-error" + mockStore := &MockConfigStore{} + // Account exists but uses client_secret mode with no stored secret. + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, externalID string) (*config.CloudAccount, error) { + require.Equal(t, "azure", provider) + require.Equal(t, subID, externalID) + return &config.CloudAccount{ + ID: "acct-uuid-001", + Provider: "azure", + ExternalID: subID, + AzureSubscriptionID: subID, + AzureTenantID: "tenant-001", + AzureClientID: "client-001", + AzureAuthMode: "client_secret", + Enabled: true, + }, nil + } + + // credStore.LoadRaw returns nil (no secret stored) which causes + // ResolveAzureTokenCredentialWithOpts to return an error. + h := &Handler{ + auth: &mockAuthForExchange{}, + config: mockStore, + credStore: &MockCredentialStore{}, // LoadRaw always returns (nil, nil) + } + + _, err := h.listExchangeableAzureRIs(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + QueryStringParameters: map[string]string{"subscription_id": subID}, + }) + require.Error(t, err, "missing credential must produce an error, not a silent empty state") + // The error must be server-side (not a 4xx ClientError). + _, isClientErr := IsClientError(err) + assert.False(t, isClientErr, "credential resolution failure must not be classified as a client (4xx) error") +} + +// TestBuildAzureExchangeClient_UsesResolver verifies that buildAzureExchangeClient +// calls GetCloudAccountByExternalID with the correct (provider, subscriptionID) pair, +// confirming the production path no longer uses DefaultAzureCredential. The subscription +// ID from the request must flow through to the lookup without being altered. +func TestBuildAzureExchangeClient_UsesResolver(t *testing.T) { + const subID = "sub-resolver-test" + var capturedProvider, capturedExtID string + + mockStore := &MockConfigStore{} + mockStore.GetCloudAccountByExternalIDFn = func(_ context.Context, provider, externalID string) (*config.CloudAccount, error) { + capturedProvider = provider + capturedExtID = externalID + // Return nil to short-circuit credential resolution (graceful empty path). + return nil, nil + } + + h := &Handler{ + config: mockStore, + // No azureExchangeFactory: production path runs. + } + + client, err := h.buildAzureExchangeClient(context.Background(), subID) + require.NoError(t, err) + assert.Nil(t, client, "nil account must yield nil client (graceful empty path)") + assert.Equal(t, "azure", capturedProvider, "lookup must use provider=azure") + assert.Equal(t, subID, capturedExtID, "lookup must use the subscription ID from the request") +} + // mockAuthForExchange is a minimal auth mock that returns an admin session. type mockAuthForExchange struct{}