fix(ri-exchange): resolve Azure credentials via per-subscription resolver

internal/api/handler_ri_exchange.go

@@ -12,14 +12,14 @@ 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"
 	"github.com/aws/smithy-go"
 
 	"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,20 +224,27 @@ 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
 	}
 
 	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 {

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{}