diff --git a/frontend/src/__tests__/riexchange-account-1692.test.ts b/frontend/src/__tests__/riexchange-account-1692.test.ts new file mode 100644 index 000000000..1e3891d42 --- /dev/null +++ b/frontend/src/__tests__/riexchange-account-1692.test.ts @@ -0,0 +1,146 @@ +/** + * Issue #1692: the RI Exchange table and the Exchange dialog name the cloud + * account the RIs belong to, so an operator can confirm the target before + * quoting or executing an exchange. + */ + +jest.mock('../api', () => ({ + listConvertibleRIs: jest.fn(), + listAccountsMinimal: jest.fn(), + listExchangeableAzureRIs: jest.fn(), + getRIUtilization: jest.fn(), + getReshapeRecommendations: jest.fn(), + getRIExchangeHistory: jest.fn(), + listTargetOfferings: jest.fn(), +})); + +jest.mock('../navigation', () => ({ + switchTab: jest.fn(), + switchSettingsSubTab: jest.fn(), +})); + +jest.mock('../state', () => ({ + subscribeProvider: jest.fn(() => () => undefined), + subscribeAccount: jest.fn(() => () => undefined), + getCurrentProvider: jest.fn(() => 'aws'), + getCurrentAccountIDs: jest.fn(() => []), + getCurrentUser: jest.fn(() => ({ id: 'u', email: 'u@example.com', groups: ['00000000-0000-5000-8000-000000000001'] })), + getRiExchangeColumnFilters: jest.fn(() => ({})), + setRiExchangeColumnFilter: jest.fn(), + clearAllRiExchangeColumnFilters: jest.fn(), + getActiveRiColumnFilters: jest.fn(() => ({})), + setActiveRiColumnFilter: jest.fn(), + clearAllActiveRiColumnFilters: jest.fn(), +})); + +jest.mock('../permissions', () => ({ + canAccess: jest.fn(() => true), +})); + +import { loadRIExchange, openExchangeModal } from '../riexchange'; +import * as api from '../api'; +import { flushPromises } from './flush-promises'; + +const ri = (id: string, accountID?: string) => ({ + reserved_instance_id: id, + instance_type: 't4g.nano', + availability_zone: '', + instance_count: 1, + start: '2026-05-22T00:00:00Z', + end: '2027-05-22T00:00:00Z', + offering_type: 'No Upfront', + fixed_price: 0, + usage_price: 0, + state: 'active', + normalization_factor: 0.25, + ...(accountID ? { account_id: accountID } : {}), +}); + +const hostAccount = { id: 'uuid-host', name: 'CUDly host (123456789012)', external_id: '123456789012', provider: 'aws' }; +const plainAccount = { id: 'uuid-plain', name: 'Payer', external_id: '210987654321', provider: 'aws' }; + +describe('RI Exchange account label (issue #1692)', () => { + let instancesEl: HTMLDivElement; + + beforeEach(() => { + document.body.replaceChildren(); + instancesEl = document.createElement('div'); + instancesEl.id = 'ri-exchange-instances-list'; + const recsEl = document.createElement('div'); + recsEl.id = 'ri-exchange-recommendations-list'; + const historyEl = document.createElement('div'); + historyEl.id = 'ri-exchange-history-list'; + const modal = document.createElement('div'); + modal.id = 'ri-exchange-modal'; + const content = document.createElement('div'); + content.className = 'modal-content'; + modal.appendChild(content); + document.body.append(instancesEl, recsEl, historyEl, modal); + + (api.getRIUtilization as jest.Mock).mockResolvedValue([]); + (api.getRIExchangeHistory as jest.Mock).mockResolvedValue([]); + (api.getReshapeRecommendations as jest.Mock).mockResolvedValue({ recommendations: [], recs_staleness: '', recs_collected_at: null }); + (api.listTargetOfferings as jest.Mock).mockResolvedValue([]); + (api.listAccountsMinimal as jest.Mock).mockResolvedValue([hostAccount, plainAccount]); + }); + + const accountLine = (): string => + instancesEl.querySelector('#ri-exchange-account-label')?.textContent?.trim() ?? ''; + + it('names the account once, without repeating an ID already in its name', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([ri('ri-1', '123456789012'), ri('ri-2', '123456789012')]); + + await loadRIExchange(); + await flushPromises(); + + expect(accountLine()).toBe('Account: CUDly host (123456789012)'); + }); + + it('adds the account ID when the name does not contain it', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([ri('ri-1', '210987654321')]); + + await loadRIExchange(); + await flushPromises(); + + expect(accountLine()).toBe('Account: Payer (210987654321)'); + }); + + it('falls back to the account ID when the account is not registered', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([ri('ri-1', '555555555555')]); + + await loadRIExchange(); + await flushPromises(); + + expect(accountLine()).toBe('Account: AWS account 555555555555'); + }); + + it('keeps the ID label when the account names cannot be loaded', async () => { + (api.listAccountsMinimal as jest.Mock).mockRejectedValue(new Error('forbidden')); + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([ri('ri-1', '123456789012')]); + + await loadRIExchange(); + await flushPromises(); + + expect(accountLine()).toBe('Account: AWS account 123456789012'); + }); + + it('says when the account could not be determined', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([ri('ri-1')]); + + await loadRIExchange(); + await flushPromises(); + + expect(accountLine()).toBe('Account: could not be determined'); + }); + + it('shows the account in the Exchange dialog', async () => { + (api.listConvertibleRIs as jest.Mock).mockResolvedValue([ri('ri-1', '123456789012')]); + await loadRIExchange(); + await flushPromises(); + + openExchangeModal('ri-1', 1); + + const dialogAccount = document.querySelector('#ri-exchange-modal #modal-exchange-account')?.textContent?.trim(); + expect(dialogAccount).toBe('Account: CUDly host (123456789012)'); + }); +}); diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index 17f4b7e3e..9b8c05ec0 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -1396,7 +1396,10 @@ describe('RI Exchange global filter scoping (issue #871)', () => { it('forwards undefined and skips the account lookup when no account is selected', async () => { await loadRIExchange(); - expect(api.listAccountsMinimal).not.toHaveBeenCalled(); + // No scope lookup (an unfiltered listAccountsMinimal()); the only call is + // the AWS account-name lookup for the account label (issue #1692). + expect(api.listAccountsMinimal).not.toHaveBeenCalledWith(); + expect(api.listAccountsMinimal).toHaveBeenCalledWith({ provider: 'aws' }); expect(api.listConvertibleRIs).toHaveBeenCalledWith(undefined); }); diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 83cc3fc31..e45544d75 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -741,6 +741,9 @@ export interface ConvertibleRI { usage_price: number; state: string; normalization_factor: number; + // The AWS account the RI belongs to; absent when the API could not + // resolve it (issue #1692). + account_id?: string; } // ExchangeableAzureRI is one Azure VM reservation eligible for the diff --git a/frontend/src/riexchange.ts b/frontend/src/riexchange.ts index a2395ea44..a45b244d2 100644 --- a/frontend/src/riexchange.ts +++ b/frontend/src/riexchange.ts @@ -49,6 +49,8 @@ let currentRIs: ConvertibleRI[] = []; let currentRIsLoaded = false; let currentUtilization: Map = new Map(); let currentRecommendations: ReshapeRecommendation[] = []; +// Registered account names by AWS account ID, for the account label (#1692). +let riAccountNames: Map = new Map(); const exchangePaymentPattern = /^\d+(?:\.\d+)?$/; // Generation counter to prevent stale utilization data from overwriting fresh data @@ -320,7 +322,7 @@ async function loadConvertibleRIs(accountID?: string, generation: number = scope try { // issue #871: scope to the selected account so the page honours the // Main Header global filter. - const ris = await api.listConvertibleRIs(accountID); + const [ris] = await Promise.all([api.listConvertibleRIs(accountID), loadRIAccountNames()]); if (generation !== scopeGeneration) return; currentRIs = ris; currentRIsLoaded = true; @@ -371,6 +373,25 @@ async function loadExchangeableAzureRIs(accountID: string | undefined, generatio } } +// Best effort: without names the label falls back to the account ID. +async function loadRIAccountNames(): Promise { + try { + const accounts = (await api.listAccountsMinimal({ provider: 'aws' })) ?? []; + riAccountNames = new Map(accounts.map(a => [a.external_id, a.name])); + } catch (error) { + console.warn('RI Exchange: could not load account names:', error); + riAccountNames = new Map(); + } +} + +// "CUDly host (123456789012)": the ID is added only when the name lacks it. +function riAccountLabel(accountID: string | undefined): string { + if (!accountID) return 'could not be determined'; + const name = riAccountNames.get(accountID); + if (!name) return `AWS account ${accountID}`; + return name.includes(accountID) ? name : `${name} (${accountID})`; +} + async function loadUtilization(generation: number): Promise { try { const utilization = await api.getRIUtilization(); @@ -416,7 +437,9 @@ function renderRIsTable(container: HTMLElement, accountID?: string): void { return ``; }; + const accountLabels = [...new Set(currentRIs.map(r => riAccountLabel(r.account_id)))]; container.innerHTML = ` +

Account: ${escapeHtml(accountLabels.join(', '))}

@@ -1206,6 +1229,16 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy riRow.appendChild(riLabel); content.appendChild(riRow); + // #1692: name the account before anything is quoted or executed. + const sourceRI = currentRIs.find(r => r.reserved_instance_id === riId); + if (sourceRI) { + const accountRow = document.createElement('div'); + accountRow.className = 'setting-row'; + accountRow.id = 'modal-exchange-account'; + accountRow.textContent = `Account: ${riAccountLabel(sourceRI.account_id)}`; + content.appendChild(accountRow); + } + // Targets container: one or more rows, each with offering picker + // count. Users click "+ Add target" to split a source RI across // multiple target shapes in a single atomic AWS exchange. diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index 7c7a0110d..36eadac33 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -1485,25 +1485,28 @@ func (h *Handler) resolveReshapeCloudAccountID(ctx context.Context) (string, err return h.resolveAWSCloudAccountID(ctx) } -// checkListRIsAccountIDParam enforces the ?account_id= chip filter for -// listConvertibleRIs. Returns (true, nil) when the running AWS account matches -// the requested account (or when no account_id param is given), (false, nil) -// when it does not match (caller should return an empty list), and -// (false, err) on STS failure. -func (h *Handler) checkListRIsAccountIDParam(ctx context.Context, params map[string]string) (bool, error) { - accountID := params["account_id"] - if accountID == "" { - return true, nil - } +// listRIsAccount resolves the AWS account the convertible RIs are read from +// (the deployment's ambient credentials, via STS) and applies the ?account_id= +// chip filter (issue #871). With a filter, an STS failure fails closed and a +// different account is out of scope. Without one the account only labels the +// rows (#1692), so a failure is logged and the label is left empty. +func (h *Handler) listRIsAccount(ctx context.Context, requested string) (accountID string, inScope bool, err error) { resolve := h.resolveAWSAccountID if h.riInstancesAccountResolver != nil { resolve = h.riInstancesAccountResolver } - runningAccountID, err := resolve(ctx) - if err != nil { - return false, fmt.Errorf("failed to resolve running AWS account for RI scope: %w", err) + running, resolveErr := resolve(ctx) + if requested == "" { + if resolveErr != nil { + logging.Warnf("listConvertibleRIs: could not resolve the running AWS account: %v", resolveErr) + return "", true, nil + } + return running, true, nil } - return runningAccountID == accountID, nil + if resolveErr != nil { + return "", false, fmt.Errorf("failed to resolve running AWS account for RI scope: %w", resolveErr) + } + return running, running == requested, nil } // listConvertibleRIs returns all active convertible Reserved Instances for @@ -1528,13 +1531,14 @@ func (h *Handler) listConvertibleRIs(ctx context.Context, req *events.LambdaFunc if inScope, sErr := h.reshapeCloudAccountInScope(ctx, session); sErr != nil { return nil, sErr } else if !inScope { - return &ConvertibleRIsResponse{Instances: []ec2ConvertibleRI{}}, nil + return &ConvertibleRIsResponse{Instances: []convertibleRIWithAccount{}}, nil } - if inScope, sErr := h.checkListRIsAccountIDParam(ctx, req.QueryStringParameters); sErr != nil { - return nil, sErr + runningAccountID, inScope, err := h.listRIsAccount(ctx, req.QueryStringParameters["account_id"]) + if err != nil { + return nil, err } else if !inScope { - return &ConvertibleRIsResponse{Instances: []ec2ConvertibleRI{}}, nil + return &ConvertibleRIsResponse{Instances: []convertibleRIWithAccount{}}, nil } region := req.QueryStringParameters["region"] @@ -1543,13 +1547,17 @@ func (h *Handler) listConvertibleRIs(ctx context.Context, req *events.LambdaFunc return nil, fmt.Errorf("failed to load AWS config: %w", err) } - ec2Client := newReshapeEC2Client(cfg) + ec2Client := h.buildReshapeEC2Client(cfg) instances, err := ec2Client.ListConvertibleReservedInstances(ctx) if err != nil { return nil, fmt.Errorf("failed to list convertible RIs: %w", err) } - return &ConvertibleRIsResponse{Instances: instances}, nil + out := make([]convertibleRIWithAccount, len(instances)) + for i := range instances { + out[i] = convertibleRIWithAccount{ec2ConvertibleRI: instances[i], AccountID: runningAccountID} + } + return &ConvertibleRIsResponse{Instances: out}, nil } // getRIUtilization returns per-RI utilization from Cost Explorer. @@ -2108,7 +2116,15 @@ func mapAWSExchangeError(opMsg string, err error) error { // ConvertibleRIsResponse holds the list of convertible RIs. type ConvertibleRIsResponse struct { - Instances []ec2ConvertibleRI `json:"instances"` + Instances []convertibleRIWithAccount `json:"instances"` +} + +// convertibleRIWithAccount names the AWS account an RI belongs to, so the UI +// can show it before an exchange (#1692). AccountID is empty when the running +// account could not be resolved. +type convertibleRIWithAccount struct { + ec2ConvertibleRI + AccountID string `json:"account_id,omitempty"` } // ExchangeableAzureRIsResponse holds the list of Azure VM reservations that diff --git a/internal/api/handler_ri_exchange_account_test.go b/internal/api/handler_ri_exchange_account_test.go new file mode 100644 index 000000000..712cb9d31 --- /dev/null +++ b/internal/api/handler_ri_exchange_account_test.go @@ -0,0 +1,94 @@ +package api + +import ( + "context" + "encoding/json" + "errors" + "testing" + + ec2svc "github.com/LeanerCloud/cloud-commitments-go/providers/aws/services/ec2" + "github.com/aws/aws-lambda-go/events" + "github.com/aws/aws-sdk-go-v2/aws" + "github.com/stretchr/testify/assert" + "github.com/stretchr/testify/require" +) + +// testRunningAccount is the placeholder STS account the stub resolver returns. +const testRunningAccount = "123456789012" + +// Issue #1692: the convertible-RI list names the AWS account each RI belongs +// to, so the RI Exchange page can show it before an exchange. + +func listRIsHandler(resolve func(context.Context) (string, error), ec2 *countingUtilEC2) *Handler { + h := &Handler{ + auth: &mockAuthForExchange{}, + riInstancesAccountResolver: resolve, + reshapeEC2Factory: func(_ aws.Config) reshapeEC2Client { return ec2 }, + } + // Pre-seal awsCfgOnce so the handler never runs LoadDefaultConfig. + h.awsCfgOnce.Do(func() { h.awsCfg = aws.Config{Region: "us-east-1"} }) + return h +} + +func listRIs(t *testing.T, h *Handler, params map[string]string) *ConvertibleRIsResponse { + t.Helper() + resp, err := h.listConvertibleRIs(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + QueryStringParameters: params, + }) + require.NoError(t, err) + typed, ok := resp.(*ConvertibleRIsResponse) + require.True(t, ok, "expected *ConvertibleRIsResponse, got %T", resp) + return typed +} + +func TestListConvertibleRIs_LabelsEachRIWithTheRunningAccount(t *testing.T) { + ec2 := &countingUtilEC2{instances: []ec2svc.ConvertibleRI{ + {ReservedInstanceID: "ri-a", InstanceType: "t4g.nano"}, + {ReservedInstanceID: "ri-b", InstanceType: "m5.large"}, + }} + h := listRIsHandler(func(context.Context) (string, error) { return testRunningAccount, nil }, ec2) + + resp := listRIs(t, h, nil) + + require.Len(t, resp.Instances, 2) + for _, ri := range resp.Instances { + assert.Equal(t, testRunningAccount, ri.AccountID) + } + // The embedded RI fields stay at the top level of each JSON object. + raw, err := json.Marshal(resp) + require.NoError(t, err) + var decoded struct { + Instances []map[string]any `json:"instances"` + } + require.NoError(t, json.Unmarshal(raw, &decoded)) + assert.Equal(t, "ri-a", decoded.Instances[0]["reserved_instance_id"]) + assert.Equal(t, "t4g.nano", decoded.Instances[0]["instance_type"]) + assert.Equal(t, testRunningAccount, decoded.Instances[0]["account_id"]) +} + +func TestListConvertibleRIs_MatchingAccountFilterKeepsTheLabel(t *testing.T) { + ec2 := &countingUtilEC2{instances: []ec2svc.ConvertibleRI{{ReservedInstanceID: "ri-a"}}} + h := listRIsHandler(func(context.Context) (string, error) { return testRunningAccount, nil }, ec2) + + resp := listRIs(t, h, map[string]string{"account_id": testRunningAccount}) + + require.Len(t, resp.Instances, 1) + assert.Equal(t, testRunningAccount, resp.Instances[0].AccountID) +} + +// Without an account filter the account only labels the rows, so an STS +// failure must not hide the RIs; the label is left out instead. +func TestListConvertibleRIs_UnresolvedAccountWithoutFilterStillLists(t *testing.T) { + ec2 := &countingUtilEC2{instances: []ec2svc.ConvertibleRI{{ReservedInstanceID: "ri-a"}}} + h := listRIsHandler(func(context.Context) (string, error) { return "", errors.New("sts denied") }, ec2) + + resp := listRIs(t, h, nil) + + require.Len(t, resp.Instances, 1) + assert.Empty(t, resp.Instances[0].AccountID) + raw, err := json.Marshal(resp.Instances[0]) + require.NoError(t, err) + assert.NotContains(t, string(raw), "account_id") + assert.Equal(t, 1, ec2.calls) +} diff --git a/internal/api/openapi.yaml b/internal/api/openapi.yaml index cb971c99e..0c27cabd8 100644 --- a/internal/api/openapi.yaml +++ b/internal/api/openapi.yaml @@ -2033,7 +2033,9 @@ paths: '200': description: > Convertible RI list. Empty when the deployment's AWS account is - outside the session's allowed accounts. + outside the session's allowed accounts. Each entry in `instances` + carries `account_id`, the AWS account the RI belongs to; it is + omitted when that account could not be resolved. content: application/json: schema: