From fd922b026c4a7f118e81eb5c9813529150cdd9c4 Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Wed, 4 Nov 2026 14:31:44 +0100 Subject: [PATCH] feat(ri-exchange): name the RIs' account in the RI table and Exchange dialog Neither the Active Convertible RIs table nor the Exchange dialog said which account owns the RI, so an operator could not confirm the target before an irreversible exchange. The instances API now labels each RI with the STS-resolved running account (the account filter still fails closed); the UI shows the registered account name and ID above the table and in the dialog, or says the account could not be determined. Closes #1692 Co-Authored-By: Claude Opus 5.5 --- .../__tests__/riexchange-account-1692.test.ts | 146 ++++++++++++++++++ frontend/src/__tests__/riexchange.test.ts | 5 +- frontend/src/api/types.ts | 3 + frontend/src/riexchange.ts | 35 ++++- internal/api/handler_ri_exchange.go | 58 ++++--- .../api/handler_ri_exchange_account_test.go | 94 +++++++++++ internal/api/openapi.yaml | 4 +- 7 files changed, 321 insertions(+), 24 deletions(-) create mode 100644 frontend/src/__tests__/riexchange-account-1692.test.ts create mode 100644 internal/api/handler_ri_exchange_account_test.go 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: