diff --git a/frontend/src/__tests__/riexchange.test.ts b/frontend/src/__tests__/riexchange.test.ts index d3839af17..af3a25844 100644 --- a/frontend/src/__tests__/riexchange.test.ts +++ b/frontend/src/__tests__/riexchange.test.ts @@ -12,6 +12,10 @@ jest.mock('../api', () => ({ getRIExchangeHistory: jest.fn(), getRIExchangeConfig: jest.fn(), updateRIExchangeConfig: jest.fn(), + // listTargetOfferings is called by populateAwsOfferings() in openExchangeModal. + // Default to returning an empty list so tests that don't care about the picker + // content remain unaffected. Override per-test for picker-content assertions. + listTargetOfferings: jest.fn().mockResolvedValue([]), })); // Mock navigation to avoid loading dashboard/plans/... transitively. @@ -70,18 +74,29 @@ describe('openExchangeModal', () => { expect(countInput?.value).toBe('5'); }); - it('pre-fills target input with suggestedTargetType when provided', () => { - openExchangeModal('ri-abc123', 2, 'm5.large'); + it('pre-fills hidden target input with offering_id when suggestedTargetType matches an alternativeTarget', () => { + // suggestedTargetType is resolved to an offering_id via alternativeTargets lookup. + openExchangeModal('ri-abc123', 2, 'm5.large', [ + { instance_type: 'm5.large', offering_id: '4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91', effective_monthly_cost: 42.5 }, + ]); const targetInput = modal.querySelector('.modal-exchange-target'); - expect(targetInput?.value).toBe('m5.large'); + // Hidden input must contain the UUID, not the instance type string. + expect(targetInput?.value).toBe('4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91'); }); - it('leaves target input empty when suggestedTargetType is not provided', () => { + it('leaves hidden target input empty when suggestedTargetType is not provided', () => { openExchangeModal('ri-abc123', 2); const targetInput = modal.querySelector('.modal-exchange-target'); expect(targetInput?.value).toBe(''); }); + it('leaves hidden target input empty when suggestedTargetType has no matching alternativeTarget', () => { + openExchangeModal('ri-abc123', 2, 'm5.large'); + const targetInput = modal.querySelector('.modal-exchange-target'); + // No alternativeTargets provided -- cannot resolve instance type to UUID. + expect(targetInput?.value).toBe(''); + }); + it('starts with exactly one target row', () => { openExchangeModal('ri-abc123', 2); const rows = modal.querySelectorAll('.exchange-target-row'); @@ -111,7 +126,11 @@ describe('openExchangeModal', () => { TargetRemainingUpfrontRaw: '', TargetRemainingTotalRaw: '', }); - openExchangeModal('ri-abc', 3, 'm5.large'); + // Pre-seed with a CE alternative so suggestedTargetType resolves to a UUID. + const offeringUUID = '4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91'; + openExchangeModal('ri-abc', 3, 'm5.large', [ + { instance_type: 'm5.large', offering_id: offeringUUID, effective_monthly_cost: 42.5 }, + ]); const quoteBtn = Array.from(modal.querySelectorAll('button')).find((b) => b.textContent === 'Get Quote'); quoteBtn?.click(); // Wait for the async submit handler to settle. @@ -120,7 +139,8 @@ describe('openExchangeModal', () => { expect(mockGetQuote).toHaveBeenCalledTimes(1); const req = mockGetQuote.mock.calls[0][0]; expect(req.ri_ids).toEqual(['ri-abc']); - expect(req.target_offering_id).toBe('m5.large'); + // Singleton shape: target_offering_id must be the UUID, not the instance type. + expect(req.target_offering_id).toBe(offeringUUID); expect(req.target_count).toBe(3); expect(req.targets).toBeUndefined(); }); @@ -139,14 +159,20 @@ describe('openExchangeModal', () => { TargetRemainingUpfrontRaw: '', TargetRemainingTotalRaw: '', }); - openExchangeModal('ri-multi', 1, 'm5.large'); + const uuid1 = '4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91'; + const uuid2 = '7e123456-0000-4567-abcd-ef0123456789'; + // Pre-seed with CE alternatives so the first row can resolve a UUID. + openExchangeModal('ri-multi', 1, 'm5.large', [ + { instance_type: 'm5.large', offering_id: uuid1, effective_monthly_cost: 40.0 }, + ]); modal.querySelector('#modal-exchange-add-target')?.click(); - // Populate the second row. + // Inject a UUID into the second row's hidden input directly (simulates + // the user picking from the dropdown for the second target). const rows = modal.querySelectorAll('.exchange-target-row'); const secondOffering = rows[1]?.querySelector('.modal-exchange-target'); const secondCount = rows[1]?.querySelector('.modal-exchange-count'); - if (secondOffering) secondOffering.value = 'm6i.large'; + if (secondOffering) secondOffering.value = uuid2; if (secondCount) secondCount.value = '2'; const quoteBtn = Array.from(modal.querySelectorAll('button')).find((b) => b.textContent === 'Get Quote'); @@ -157,8 +183,8 @@ describe('openExchangeModal', () => { const req = mockGetQuote.mock.calls[0][0]; expect(req.ri_ids).toEqual(['ri-multi']); expect(req.targets).toEqual([ - { offering_id: 'm5.large', count: 1 }, - { offering_id: 'm6i.large', count: 2 }, + { offering_id: uuid1, count: 1 }, + { offering_id: uuid2, count: 2 }, ]); expect(req.target_offering_id).toBeUndefined(); expect(req.target_count).toBeUndefined(); @@ -191,7 +217,9 @@ describe('openExchangeModal', () => { expect(modal.classList.contains('hidden')).toBe(true); }); - it('shows a cost chip when the typed instance type matches an alternative', () => { + it('shows a cost chip when the selected offering_id matches an alternative', () => { + // suggestedTargetType='m5.large' resolves to offering_id 'off-m5' via alternativeTargets. + // The chip should show the cost for that offering. openExchangeModal('ri-abc', 2, 'm5.large', [ { instance_type: 'm5.large', offering_id: 'off-m5', effective_monthly_cost: 42.5 }, { instance_type: 'm6i.large', offering_id: 'off-m6i', effective_monthly_cost: 35.0 }, @@ -201,7 +229,9 @@ describe('openExchangeModal', () => { expect(chip?.textContent).toBe('$42.50/mo each'); }); - it('shows an em-dash in the cost chip when the typed instance type has no alternative match', () => { + it('shows an em-dash in the cost chip when the selected offering_id has no CE pricing match', () => { + // 'unknown.shape' cannot be resolved to an offering_id from alternativeTargets, + // so the hidden input stays empty and the chip shows "—". openExchangeModal('ri-abc', 2, 'unknown.shape', [ { instance_type: 'm5.large', offering_id: 'off-m5', effective_monthly_cost: 42.5 }, ]); @@ -224,7 +254,9 @@ describe('openExchangeModal', () => { const secondOffering = rows[1]?.querySelector('.modal-exchange-target'); const secondCount = rows[1]?.querySelector('.modal-exchange-count'); if (secondOffering) { - secondOffering.value = 'm6i.large'; + // Inject the offering_id UUID directly into the hidden input and + // trigger an input event so updateRunningTotal fires. + secondOffering.value = 'off-m6i'; secondOffering.dispatchEvent(new Event('input')); } if (secondCount) { @@ -244,7 +276,8 @@ describe('openExchangeModal', () => { const rows = modal.querySelectorAll('.exchange-target-row'); const secondOffering = rows[1]?.querySelector('.modal-exchange-target'); if (secondOffering) { - secondOffering.value = 'unknown.shape'; + // 'unknown-offering' does not match any CE alternative offering_id. + secondOffering.value = 'unknown-offering'; secondOffering.dispatchEvent(new Event('input')); } const total = modal.querySelector('#modal-exchange-running-total'); @@ -262,6 +295,88 @@ describe('openExchangeModal', () => { document.body.innerHTML = ''; expect(() => openExchangeModal('ri-abc123', 2)).not.toThrow(); }); + + // Defect 1 -- picker tests + it('renders a select picker (not a free-text input) for the target offering', () => { + openExchangeModal('ri-abc123', 2); + // There must be a that holds the resolved +// AWS ReservedInstancesOfferingId UUID. The visible picker +// (modal-exchange-target-select) drives this field on change so the +// submission path always sees a UUID and never a free-text instance type. interface TargetRow { - offeringInput: HTMLInputElement; + offeringInput: HTMLInputElement; // hidden; holds the UUID + pickerSelect: HTMLSelectElement; // visible; drives offeringInput countInput: HTMLInputElement; chipEl: HTMLSpanElement; // cost chip; shows "$X.XX/mo each" or "—". rowEl: HTMLDivElement; @@ -334,7 +341,7 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy riRow.appendChild(riLabel); content.appendChild(riRow); - // Targets container: one or more rows, each with offering ID + + // 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. const targetsContainer = document.createElement('div'); @@ -344,21 +351,110 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy const targetRows: TargetRow[] = []; - const addTargetRow = (initialOffering?: string, initialCount?: number): void => { + // awsOfferings holds the list loaded from the target-offerings endpoint. + // Starts empty; populateAwsOfferings() fills it once after the modal opens. + let awsOfferings: TargetOffering[] = []; + // offeringsLoaded tracks whether the async load has completed so new + // rows added after completion are seeded with the already-loaded list. + let offeringsLoaded = false; + let offeringsError = false; + + // buildPickerOptions rebuilds the with two optgroups (AWS + CE recs). + const pickerLabel = document.createElement('label'); + pickerLabel.textContent = 'Target offering: '; + const pickerSelect = document.createElement('select'); + pickerSelect.className = 'modal-exchange-target-select'; + buildPickerOptions(pickerSelect, initialOfferingId); + pickerLabel.appendChild(pickerSelect); + rowEl.appendChild(pickerLabel); const countLabel = document.createElement('label'); countLabel.textContent = 'Count: '; @@ -386,45 +482,50 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy }); rowEl.appendChild(removeBtn); - // Cost chip: last child of the row. Shows per-instance rate when - // the typed offering ID exact-matches an entry in - // alternativeTargets; otherwise shows "—". Updated live on input. + // Cost chip: shows per-instance monthly cost for the selected offering + // when it appears in alternativeTargets; otherwise shows "—". const chipEl = document.createElement('span'); chipEl.className = 'cost-chip'; chipEl.textContent = '—'; rowEl.appendChild(chipEl); - offeringInput.addEventListener('input', () => { - updateRowChip(offeringInput, chipEl); + // pickerSelect drives the hidden offeringInput and refreshes the chip. + pickerSelect.addEventListener('change', () => { + offeringInput.value = pickerSelect.value; + updateRowChip(pickerSelect.value, chipEl); updateRunningTotal(); }); countInput.addEventListener('input', updateRunningTotal); targetsContainer.appendChild(rowEl); - targetRows.push({ offeringInput, countInput, chipEl, rowEl }); + targetRows.push({ offeringInput, pickerSelect, countInput, chipEl, rowEl }); // Initial chip population for pre-filled rows. - updateRowChip(offeringInput, chipEl); + updateRowChip(offeringInput.value, chipEl); }; - // lookupAlternativeCost returns the per-instance monthly cost for an - // exact instance_type match in alternativeTargets, or undefined when - // no match exists (or when the caller didn't pass alternatives — - // e.g. the Convertible-RIs-table "Exchange" button path). - function lookupAlternativeCost(instanceType: string): number | undefined { - if (!alternativeTargets || !instanceType) return undefined; - const trimmed = instanceType.trim(); - const hit = alternativeTargets.find((a) => a.instance_type === trimmed); + // lookupCECost returns the per-instance monthly cost for an offering_id + // that appears in alternativeTargets, or undefined when absent. + function lookupCECost(offeringId: string): number | undefined { + if (!alternativeTargets || !offeringId) return undefined; + const hit = alternativeTargets.find((a) => a.offering_id === offeringId); return hit?.effective_monthly_cost; } - function updateRowChip(input: HTMLInputElement, chip: HTMLSpanElement): void { - const cost = lookupAlternativeCost(input.value); + function updateRowChip(offeringId: string, chip: HTMLSpanElement): void { + const cost = lookupCECost(offeringId); chip.textContent = cost !== undefined ? `${formatCurrency(cost, '$', 2)}/mo each` : '—'; } - // Seed the modal with the current behaviour: one row, pre-filled - // from the suggested target type + count passed in by the caller. - addTargetRow(suggestedTargetType, count); + // Seed the modal with one row, optionally pre-selecting by offering_id. + // suggestedTargetType is an instance type (from reshape recs); we + // match it against CE alternatives to find the offering_id to pre-select. + const suggestedOfferingId = suggestedTargetType && alternativeTargets + ? (alternativeTargets.find((a) => a.instance_type === suggestedTargetType)?.offering_id) + : undefined; + addTargetRow(suggestedOfferingId, count); + + // Kick off the async AWS offerings load after the first row exists. + void populateAwsOfferings(); const addTargetBtnRow = document.createElement('div'); addTargetBtnRow.className = 'setting-row'; @@ -458,7 +559,7 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy let total = 0; let anyMissing = false; for (const row of targetRows) { - const cost = lookupAlternativeCost(row.offeringInput.value); + const cost = lookupCECost(row.offeringInput.value); const rawCount = parseInt(row.countInput.value, 10); const cnt = isNaN(rawCount) || rawCount < 1 ? 1 : rawCount; if (cost === undefined) { @@ -467,7 +568,7 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy } total += cost * cnt; } - const suffix = anyMissing ? ' (incomplete — some targets have no pricing data)' : ''; + const suffix = anyMissing ? ' (incomplete -- some targets have no pricing data)' : ''; runningTotalEl.textContent = `Estimated monthly cost for the quoted target set: ${formatCurrency(total, '$', 2)}/mo${suffix}`; } updateRunningTotal(); @@ -514,11 +615,13 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy void submitModalExecute(); }); + // offeringUUIDPattern mirrors the backend regex for AWS offering UUIDs. + const offeringUUIDPattern = /^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/; + // collectTargets reads each row into a validated target. Rows with - // empty offering IDs are treated as an error; the first offending - // row's message is surfaced to the user. Count defaults to 1 when - // the input is empty or non-numeric (matches the pre-multi-target - // behaviour). + // empty or non-UUID offering IDs are treated as an error; the first + // offending row's message is surfaced to the user. Count defaults to 1 + // when the input is empty or non-numeric. function collectTargets(): { targets: Array<{ offering_id: string; count: number }>; error?: string } { const targets: Array<{ offering_id: string; count: number }> = []; for (let i = 0; i < targetRows.length; i++) { @@ -526,7 +629,13 @@ export function openExchangeModal(riId: string, count: number, suggestedTargetTy if (!row) continue; const offeringId = row.offeringInput.value.trim(); if (!offeringId) { - return { targets: [], error: `Please enter a target offering ID for target ${i + 1}.` }; + return { targets: [], error: `Please select a target offering for target ${i + 1}.` }; + } + if (!offeringUUIDPattern.test(offeringId)) { + return { + targets: [], + error: `Target ${i + 1}: "${offeringId}" is not a valid offering UUID. Please select an offering from the dropdown.`, + }; } const rawCount = parseInt(row.countInput.value, 10); const targetCount = isNaN(rawCount) || rawCount < 1 ? 1 : rawCount; diff --git a/go.mod b/go.mod index e3735f4df..a1b3d4c07 100644 --- a/go.mod +++ b/go.mod @@ -46,7 +46,7 @@ require ( github.com/aws/aws-sdk-go-v2/service/savingsplans v1.31.0 // indirect github.com/aws/aws-sdk-go-v2/service/sso v1.18.5 // indirect github.com/aws/aws-sdk-go-v2/service/ssooidc v1.21.5 // indirect - github.com/aws/smithy-go v1.24.2 // indirect + github.com/aws/smithy-go v1.24.2 github.com/davecgh/go-spew v1.1.2-0.20180830191138-d8f796af33cc // indirect github.com/felixge/httpsnoop v1.0.4 // indirect github.com/go-logr/logr v1.4.3 // indirect diff --git a/internal/api/handler.go b/internal/api/handler.go index 5e2f0bb15..65b378695 100644 --- a/internal/api/handler.go +++ b/internal/api/handler.go @@ -71,6 +71,10 @@ type Handler struct { reshapeEC2Factory func(aws.Config) reshapeEC2Client reshapeRecsFactory func(aws.Config) reshapeRecsClient + // Optional target-offerings EC2 client factory injected by tests. When nil + // (the production default), listTargetOfferings uses awsprovider.NewEC2ClientDirect. + targetOfferingsEC2Factory func(aws.Config) targetOfferingsEC2Client + // Optional Azure exchange client factory injected by tests. When nil // (the production default), buildAzureExchangeClient uses // azidentity.NewDefaultAzureCredential to construct a real diff --git a/internal/api/handler_ri_exchange.go b/internal/api/handler_ri_exchange.go index f9b9264b5..f21b2457f 100644 --- a/internal/api/handler_ri_exchange.go +++ b/internal/api/handler_ri_exchange.go @@ -4,9 +4,11 @@ import ( "context" "crypto/subtle" "encoding/json" + "errors" "fmt" "math" "math/big" + "regexp" "strconv" "time" @@ -14,6 +16,7 @@ import ( "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" @@ -66,6 +69,99 @@ func (h *Handler) buildReshapeRecsClient(cfg aws.Config) reshapeRecsClient { return awsprovider.NewRecommendationsClientDirect(cfg) } +// targetOfferingsEC2Client is the narrow EC2 interface that +// listTargetOfferings needs. Scoped here so tests can inject a tiny +// stub without implementing the full ec2svc.Client surface. +type targetOfferingsEC2Client interface { + ListConvertibleReservedInstances(ctx context.Context) ([]ec2svc.ConvertibleRI, error) + ListTargetOfferings(ctx context.Context, params ec2svc.ListTargetOfferingsParams) ([]ec2svc.TargetOffering, error) +} + +// buildTargetOfferingsEC2Client honours the injected factory when set, +// falling back to the direct AWS SDK constructor otherwise. +func (h *Handler) buildTargetOfferingsEC2Client(cfg aws.Config) targetOfferingsEC2Client { + if h.targetOfferingsEC2Factory != nil { + return h.targetOfferingsEC2Factory(cfg) + } + return awsprovider.NewEC2ClientDirect(cfg) +} + +// TargetOfferingsResponse is the response for +// GET /api/ri-exchange/target-offerings. +type TargetOfferingsResponse struct { + Offerings []ec2svc.TargetOffering `json:"offerings"` +} + +// offeringIDPattern matches a standard AWS offering UUID used for +// ReservedInstancesOfferingId values. Used both as a server-side guard +// (Defect 2) and to reject any stray free-text before it reaches AWS. +var offeringIDPattern = regexp.MustCompile( + `^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$`, +) + +// listTargetOfferings returns valid convertible RI exchange target +// offerings for the source RI identified by ?source_ri_id=. +// +// The handler looks up the source RI from DescribeReservedInstances, +// extracts its ProductDescription / Tenancy / Scope / Duration / +// OfferingType, and passes those to ec2svc.ListTargetOfferings which +// calls DescribeReservedInstancesOfferings with the same typed-field +// shape used by PR #690. Instance type is intentionally omitted from +// the query so AWS returns all valid target instance types -- the full +// menu of what the user can exchange into. +// +// GET /api/ri-exchange/target-offerings?source_ri_id=®ion= +func (h *Handler) listTargetOfferings(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { + if _, err := h.requirePermission(ctx, req, "view", "purchases"); err != nil { + return nil, err + } + + sourceRIID := req.QueryStringParameters["source_ri_id"] + if sourceRIID == "" { + return nil, NewClientError(400, "source_ri_id is required") + } + + region := req.QueryStringParameters["region"] + cfg, err := h.loadAWSConfigWithRegion(ctx, region) + if err != nil { + return nil, fmt.Errorf("failed to load AWS config: %w", err) + } + + ec2Client := h.buildTargetOfferingsEC2Client(cfg) + + // Fetch all convertible RIs to locate the source RI's attributes. + // DescribeReservedInstances does not support a single-ID filter + // without the full ARN, so we enumerate and filter by ID. + ris, err := ec2Client.ListConvertibleReservedInstances(ctx) + if err != nil { + return nil, fmt.Errorf("failed to list convertible RIs: %w", err) + } + + var sourceRI *ec2svc.ConvertibleRI + for i := range ris { + if ris[i].ReservedInstanceID == sourceRIID { + sourceRI = &ris[i] + break + } + } + if sourceRI == nil { + return nil, NewClientError(404, fmt.Sprintf("source RI %q not found in region %s", sourceRIID, cfg.Region)) + } + + offerings, err := ec2Client.ListTargetOfferings(ctx, ec2svc.ListTargetOfferingsParams{ + ProductDescription: sourceRI.ProductDescription, + Tenancy: sourceRI.InstanceTenancy, + Scope: sourceRI.Scope, + Duration: sourceRI.Duration, + OfferingType: sourceRI.OfferingType, + }) + if err != nil { + return nil, fmt.Errorf("failed to list target offerings: %w", err) + } + + return &TargetOfferingsResponse{Offerings: offerings}, nil +} + // azureExchangeClient is the narrow interface that listExchangeableAzureRIs // needs from the Azure compute client. Satisfied by // *azurecompute.ComputeClient; a stub can be injected via @@ -353,6 +449,25 @@ func firstNonEmptyCurrency(instances []ec2svc.ConvertibleRI) string { return "USD" } +// validateTargets checks each entry in targets for a non-empty, UUID-shaped +// offering_id. Extracted so both getExchangeQuote and validateExecuteExchangeBody +// share the same check without exceeding the gocyclo threshold. +func validateTargets(targets []ExchangeTargetBody) error { + for i, t := range targets { + if t.OfferingID == "" { + return NewClientError(400, fmt.Sprintf("targets[%d].offering_id is required", i)) + } + if !offeringIDPattern.MatchString(t.OfferingID) { + return NewClientError(400, fmt.Sprintf( + "targets[%d].offering_id %q does not look like an AWS offering UUID; "+ + "expected something like 4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91 -- "+ + "did you paste an instance type by mistake?", + i, t.OfferingID)) + } + } + return nil +} + // getExchangeQuote gets a quote for an RI exchange. func (h *Handler) getExchangeQuote(ctx context.Context, req *events.LambdaFunctionURLRequest) (any, error) { if _, err := h.requirePermission(ctx, req, "view", "purchases"); err != nil { @@ -369,10 +484,8 @@ func (h *Handler) getExchangeQuote(ctx context.Context, req *events.LambdaFuncti if len(body.Targets) == 0 && body.TargetOfferingID == "" { return nil, NewClientError(400, "either targets[] or target_offering_id is required") } - for i, t := range body.Targets { - if t.OfferingID == "" { - return nil, NewClientError(400, fmt.Sprintf("targets[%d].offering_id is required", i)) - } + if err := validateTargets(body.Targets); err != nil { + return nil, err } region := body.Region @@ -389,7 +502,7 @@ func (h *Handler) getExchangeQuote(ctx context.Context, req *events.LambdaFuncti }) if err != nil { logging.Errorf("exchange quote failed: %v", err) - return nil, NewClientError(500, "exchange quote failed") + return nil, mapAWSExchangeError("exchange quote failed", err) } return quote, nil @@ -407,10 +520,8 @@ func validateExecuteExchangeBody(body ExchangeExecuteRequestBody) error { if len(body.Targets) == 0 && body.TargetOfferingID == "" { return NewClientError(400, "either targets[] or target_offering_id is required") } - for i, t := range body.Targets { - if t.OfferingID == "" { - return NewClientError(400, fmt.Sprintf("targets[%d].offering_id is required", i)) - } + if err := validateTargets(body.Targets); err != nil { + return err } if body.MaxPaymentDueUSD == "" { return NewClientError(400, "max_payment_due_usd is required as a safety guardrail") @@ -452,7 +563,7 @@ func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctio }) if err != nil { logging.Errorf("exchange execution failed: %v", err) - return nil, NewClientError(500, "exchange execution failed") + return nil, mapAWSExchangeError("exchange execution failed", err) } return &ExchangeExecuteResponse{ @@ -461,6 +572,32 @@ func (h *Handler) executeExchange(ctx context.Context, req *events.LambdaFunctio }, nil } +// awsExchangeClientFaultCodes is the set of AWS error codes that are +// documented client faults for RI exchange operations. These map to +// 4xx responses so the caller receives the original AWS error message +// and understands it was their input that was wrong. All other AWS +// errors remain 5xx (transient / server-side). +var awsExchangeClientFaultCodes = map[string]bool{ + "InvalidOfferingId": true, + "InvalidParameter": true, + "ValidationError": true, + "InvalidReservedInstancesId.NotFound": true, + "InvalidInstanceID.NotFound": true, +} + +// mapAWSExchangeError converts an AWS SDK error from an RI exchange +// operation to a ClientError with the appropriate HTTP status code. +// AWS 4xx client-fault errors produce a 400 with the original AWS +// message preserved. Any other error produces a 500 (generic server +// failure) using the provided opMsg fallback. +func mapAWSExchangeError(opMsg string, err error) error { + var apiErr smithy.APIError + if errors.As(err, &apiErr) && awsExchangeClientFaultCodes[apiErr.ErrorCode()] { + return NewClientError(400, apiErr.ErrorMessage()) + } + return NewClientError(500, opMsg) +} + // Response types // ConvertibleRIsResponse holds the list of convertible RIs. diff --git a/internal/api/handler_ri_exchange_test.go b/internal/api/handler_ri_exchange_test.go index ac136e8eb..4eb0709f8 100644 --- a/internal/api/handler_ri_exchange_test.go +++ b/internal/api/handler_ri_exchange_test.go @@ -12,6 +12,7 @@ import ( azurecompute "github.com/LeanerCloud/CUDly/providers/azure/services/compute" "github.com/aws/aws-lambda-go/events" "github.com/aws/aws-sdk-go-v2/aws" + "github.com/aws/smithy-go" "github.com/stretchr/testify/assert" "github.com/stretchr/testify/mock" "github.com/stretchr/testify/require" @@ -670,3 +671,227 @@ func (m *mockAuthForExchange) MFADisableAPI(_ context.Context, _, _, _ string) e func (m *mockAuthForExchange) MFARegenerateRecoveryCodesAPI(_ context.Context, _, _ string) ([]string, error) { return nil, nil } + +// --------------------------------------------------------------------------- +// Defect 1 backend: GET /api/ri-exchange/target-offerings +// --------------------------------------------------------------------------- + +// stubTargetOfferingsEC2 is a test stub for targetOfferingsEC2Client. +// It returns a fixed list of ConvertibleRIs for the lookup step and a +// fixed list of TargetOfferings for the DescribeReservedInstancesOfferings +// step. Both are configurable per-test so we can exercise the 404 and +// happy paths without live AWS. +type stubTargetOfferingsEC2 struct { + instances []ec2svc.ConvertibleRI + offerings []ec2svc.TargetOffering + err error +} + +func (s *stubTargetOfferingsEC2) ListConvertibleReservedInstances(_ context.Context) ([]ec2svc.ConvertibleRI, error) { + return s.instances, s.err +} + +func (s *stubTargetOfferingsEC2) ListTargetOfferings(_ context.Context, _ ec2svc.ListTargetOfferingsParams) ([]ec2svc.TargetOffering, error) { + if s.err != nil { + return nil, s.err + } + return s.offerings, nil +} + +func TestListTargetOfferings_RequiresPermission(t *testing.T) { + h := &Handler{} + _, err := h.listTargetOfferings(context.Background(), &events.LambdaFunctionURLRequest{}) + require.Error(t, err) + assert.Contains(t, err.Error(), "authentication") +} + +func TestListTargetOfferings_MissingSourceRIID(t *testing.T) { + h := &Handler{auth: &mockAuthForExchange{}} + _, err := h.listTargetOfferings(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "source_ri_id is required") +} + +func TestListTargetOfferings_SourceRINotFound(t *testing.T) { + stub := &stubTargetOfferingsEC2{ + instances: []ec2svc.ConvertibleRI{ + {ReservedInstanceID: "ri-known", InstanceType: "m5.large"}, + }, + } + h := &Handler{ + auth: &mockAuthForExchange{}, + targetOfferingsEC2Factory: func(_ aws.Config) targetOfferingsEC2Client { return stub }, + } + h.awsCfgOnce.Do(func() { h.awsCfg = aws.Config{Region: "us-east-1"} }) + + _, err := h.listTargetOfferings(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + QueryStringParameters: map[string]string{"source_ri_id": "ri-unknown"}, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 404, ce.code) + assert.Contains(t, err.Error(), "ri-unknown") +} + +func TestListTargetOfferings_HappyPath(t *testing.T) { + sourceID := "296818b6-73f8-4cd2-94bc-dbb95f794812" + stub := &stubTargetOfferingsEC2{ + instances: []ec2svc.ConvertibleRI{ + { + ReservedInstanceID: sourceID, + InstanceType: "t3.medium", + ProductDescription: "Linux/UNIX", + InstanceTenancy: "default", + Scope: "Region", + Duration: 31536000, + OfferingType: "No Upfront", + }, + }, + offerings: []ec2svc.TargetOffering{ + {OfferingID: "4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91", InstanceType: "m5.large", OfferingType: "No Upfront"}, + {OfferingID: "7e1234aa-0000-4567-abcd-ef0123456789", InstanceType: "m6i.large", OfferingType: "No Upfront"}, + }, + } + h := &Handler{ + auth: &mockAuthForExchange{}, + targetOfferingsEC2Factory: func(_ aws.Config) targetOfferingsEC2Client { return stub }, + } + h.awsCfgOnce.Do(func() { h.awsCfg = aws.Config{Region: "us-east-1"} }) + + res, err := h.listTargetOfferings(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + QueryStringParameters: map[string]string{"source_ri_id": sourceID}, + }) + require.NoError(t, err) + resp, ok := res.(*TargetOfferingsResponse) + require.True(t, ok) + require.Len(t, resp.Offerings, 2) + assert.Equal(t, "4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91", resp.Offerings[0].OfferingID) + assert.Equal(t, "m5.large", resp.Offerings[0].InstanceType) +} + +// --------------------------------------------------------------------------- +// Defect 2: offering-id UUID format validation +// --------------------------------------------------------------------------- + +func TestGetExchangeQuote_InvalidOfferingIDFormat(t *testing.T) { + h := &Handler{auth: &mockAuthForExchange{}} + + // "t3.medium" looks like an instance type, not an offering UUID + _, err := h.getExchangeQuote(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + Body: `{"ri_ids":["ri-123"],"targets":[{"offering_id":"t3.medium","count":1}]}`, + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "t3.medium") + assert.Contains(t, err.Error(), "offering UUID") +} + +func TestGetExchangeQuote_ValidOfferingIDPassesFormat(t *testing.T) { + // A well-formed UUID should pass the format check. The AWS call will + // fail because there's no real EC2 client wired -- but the test + // exercises that the regex guard does NOT fire on a valid UUID. + // We stub the handler to short-circuit before the AWS call. + called := false + stub := &stubTargetOfferingsEC2{ + instances: []ec2svc.ConvertibleRI{}, + } + _ = stub + _ = called + + h := &Handler{auth: &mockAuthForExchange{}} + + // The handler calls exchange.GetExchangeQuote next; that will fail + // with a config error in test (no real AWS cred). We only care that + // the regex guard doesn't block a valid UUID -- so any error after + // passing the UUID check is acceptable. + _, err := h.getExchangeQuote(context.Background(), &events.LambdaFunctionURLRequest{ + Headers: map[string]string{"authorization": "Bearer test-token"}, + Body: `{"ri_ids":["ri-123"],"targets":[{"offering_id":"4b2293b4-5fbc-4017-9c75-d5a9d3aa8c91","count":1}]}`, + }) + // Must NOT be the UUID format error + if err != nil { + assert.NotContains(t, err.Error(), "offering UUID", + "a valid UUID must not be rejected by the format check") + } +} + +func TestValidateExecuteExchangeBody_InvalidOfferingIDFormat(t *testing.T) { + err := validateExecuteExchangeBody(ExchangeExecuteRequestBody{ + RIIDs: []string{"ri-123"}, + Targets: []ExchangeTargetBody{{OfferingID: "t3.medium", Count: 1}}, + MaxPaymentDueUSD: "50.00", + }) + require.Error(t, err) + ce, ok := IsClientError(err) + require.True(t, ok) + assert.Equal(t, 400, ce.code) + assert.Contains(t, err.Error(), "t3.medium") + assert.Contains(t, err.Error(), "offering UUID") +} + +// --------------------------------------------------------------------------- +// Defect 3: AWS 4xx -> client 4xx, error message preserved +// --------------------------------------------------------------------------- + +// fakeAPIError is a minimal smithy.APIError implementation for tests. +type fakeAPIError struct { + code string + message string +} + +func (e *fakeAPIError) Error() string { return fmt.Sprintf("%s: %s", e.code, e.message) } +func (e *fakeAPIError) ErrorCode() string { return e.code } +func (e *fakeAPIError) ErrorMessage() string { return e.message } +func (e *fakeAPIError) ErrorFault() smithy.ErrorFault { return smithy.FaultClient } + +func TestMapAWSExchangeError_ClientFault4xx(t *testing.T) { + codes := []string{ + "InvalidOfferingId", + "InvalidParameter", + "ValidationError", + "InvalidReservedInstancesId.NotFound", + "InvalidInstanceID.NotFound", + } + for _, code := range codes { + t.Run(code, func(t *testing.T) { + apiErr := &fakeAPIError{code: code, message: "AWS says: bad input"} + mapped := mapAWSExchangeError("fallback msg", apiErr) + ce, ok := IsClientError(mapped) + require.True(t, ok, "must be ClientError") + assert.Equal(t, 400, ce.code, "client-fault codes must map to 400") + assert.Contains(t, mapped.Error(), "AWS says: bad input", + "AWS error message must be preserved") + }) + } +} + +func TestMapAWSExchangeError_ServerFault5xx(t *testing.T) { + // An AWS error with an unrecognised code must stay 500 + apiErr := &fakeAPIError{code: "InternalError", message: "AWS is having a bad day"} + mapped := mapAWSExchangeError("exchange quote failed", apiErr) + ce, ok := IsClientError(mapped) + require.True(t, ok) + assert.Equal(t, 500, ce.code) + assert.Contains(t, mapped.Error(), "exchange quote failed") + assert.NotContains(t, mapped.Error(), "AWS is having a bad day", + "non-client-fault AWS message must NOT leak through") +} + +func TestMapAWSExchangeError_NonAWSError(t *testing.T) { + // A plain Go error must also stay 500 + mapped := mapAWSExchangeError("exchange quote failed", fmt.Errorf("network timeout")) + ce, ok := IsClientError(mapped) + require.True(t, ok) + assert.Equal(t, 500, ce.code) +} diff --git a/internal/api/router.go b/internal/api/router.go index 5072b5377..8381663f6 100644 --- a/internal/api/router.go +++ b/internal/api/router.go @@ -259,6 +259,7 @@ func (r *Router) registerRoutes() { // quote / execute / config writes stay AuthAdmin. {ExactPath: "/api/ri-exchange/azure-instances", Method: "GET", Handler: r.listExchangeableAzureRIsHandler, Auth: AuthUser}, {ExactPath: "/api/ri-exchange/instances", Method: "GET", Handler: r.listConvertibleRIsHandler, Auth: AuthUser}, + {ExactPath: "/api/ri-exchange/target-offerings", Method: "GET", Handler: r.listTargetOfferingsHandler, Auth: AuthUser}, {ExactPath: "/api/ri-exchange/utilization", Method: "GET", Handler: r.getRIUtilizationHandler, Auth: AuthUser}, {ExactPath: "/api/ri-exchange/reshape-recommendations", Method: "GET", Handler: r.getReshapeRecommendationsHandler, Auth: AuthUser}, {ExactPath: "/api/ri-exchange/quote", Method: "POST", Handler: r.getExchangeQuoteHandler, Auth: AuthAdmin}, @@ -680,6 +681,10 @@ func (r *Router) listConvertibleRIsHandler(ctx context.Context, req *events.Lamb return r.h.listConvertibleRIs(ctx, req) } +func (r *Router) listTargetOfferingsHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { + return r.h.listTargetOfferings(ctx, req) +} + func (r *Router) getRIUtilizationHandler(ctx context.Context, req *events.LambdaFunctionURLRequest, params map[string]string) (any, error) { return r.h.getRIUtilization(ctx, req) } diff --git a/providers/aws/services/ec2/client.go b/providers/aws/services/ec2/client.go index 61768544a..ca4836ccc 100644 --- a/providers/aws/services/ec2/client.go +++ b/providers/aws/services/ec2/client.go @@ -705,8 +705,150 @@ func (c *Client) FindConvertibleOffering(ctx context.Context, params FindConvert return aws.ToString(result.ReservedInstancesOfferings[0].ReservedInstancesOfferingId), nil } +// TargetOffering is one valid target offering for a convertible RI exchange. +type TargetOffering struct { + OfferingID string `json:"offering_id"` + InstanceType string `json:"instance_type"` + OfferingType string `json:"offering_type"` + ProductDescription string `json:"product_description"` + Duration int64 `json:"duration"` + FixedPrice float64 `json:"fixed_price"` + UsagePrice float64 `json:"usage_price"` + CurrencyCode string `json:"currency_code"` + Scope string `json:"scope"` + NormalizationFactor float64 `json:"normalization_factor"` +} + +// ListTargetOfferingsParams holds the source RI attributes used to narrow the +// DescribeReservedInstancesOfferings query to valid exchange targets. +type ListTargetOfferingsParams struct { + ProductDescription string + Tenancy string + Scope string + Duration int64 + OfferingType string +} + +// maxTargetOfferingPages caps the pagination walk for ListTargetOfferings. +// At 100 results per page this allows up to 1000 offerings -- more than +// enough given the small number of convertible instance types AWS offers. +const maxTargetOfferingPages = 10 + +// normalizeTargetOfferingsParams fills defaults for any zero-valued fields in +// p and converts the OfferingType string to the AWS SDK enum. Extracted from +// ListTargetOfferings to keep the main function's cyclomatic complexity under +// the project threshold of 10. +func normalizeTargetOfferingsParams(p ListTargetOfferingsParams) (tenancy, scope, productDesc string, duration int64, offeringType types.OfferingTypeValues) { + tenancy = canonicalizeEC2Tenancy(p.Tenancy) + if tenancy == "" { + tenancy = string(types.TenancyDefault) + } + scope = canonicalizeEC2Scope(p.Scope) + if scope == "" { + scope = string(types.ScopeRegional) + } + duration = p.Duration + if duration == 0 { + duration = OneYearSeconds + } + productDesc = p.ProductDescription + if productDesc == "" { + productDesc = "Linux/UNIX" + } + // OfferingType: typed field when non-empty; empty string means "all + // payment options" (the caller didn't specify). Leave unset so AWS + // returns all payment variants, giving the user maximum choice. + if p.OfferingType != "" { + if ot, err := convertEC2PaymentOption(p.OfferingType); err == nil { + offeringType = ot + } + } + return +} + +// appendTargetOfferings maps the SDK result page into TargetOffering values +// and appends them to out. Extracted from ListTargetOfferings to keep it +// under the gocyclo threshold. +func appendTargetOfferings(out []TargetOffering, offerings []types.ReservedInstancesOffering, scope string) []TargetOffering { + for _, o := range offerings { + id := aws.ToString(o.ReservedInstancesOfferingId) + if id == "" { + continue + } + instanceType := string(o.InstanceType) + var size string + if parts := strings.SplitN(instanceType, ".", 2); len(parts) == 2 { + size = parts[1] + } + out = append(out, TargetOffering{ + OfferingID: id, + InstanceType: instanceType, + OfferingType: string(o.OfferingType), + ProductDescription: string(o.ProductDescription), + Duration: aws.ToInt64(o.Duration), + FixedPrice: float64(aws.ToFloat32(o.FixedPrice)), + UsagePrice: float64(aws.ToFloat32(o.UsagePrice)), + CurrencyCode: string(o.CurrencyCode), + Scope: scope, + NormalizationFactor: exchange.NormalizationFactorForSize(size), + }) + } + return out +} + +// ListTargetOfferings returns convertible RI offerings that are valid exchange +// targets for a source RI described by params. The query uses the same +// typed-field approach as findOfferingID / describeInputFromQuery (PR #690): +// typed primary fields (OfferingClass, OfferingType, Duration, ProductDescription, +// InstanceTenancy) instead of Filters[]-heavy style to avoid the empty-page +// pagination bug documented in issue #688. Scope has no typed field and stays +// in Filters[] as before. +// +// InstanceType is intentionally left unset so AWS returns all instance types +// matching the other constraints -- that is exactly the "valid targets" set for +// a convertible RI exchange. +func (c *Client) ListTargetOfferings(ctx context.Context, params ListTargetOfferingsParams) ([]TargetOffering, error) { + tenancy, scope, productDesc, duration, offeringType := normalizeTargetOfferingsParams(params) + + var out []TargetOffering + var nextToken *string + for page := 1; page <= maxTargetOfferingPages; page++ { + if err := ctx.Err(); err != nil { + return nil, err + } + input := &ec2.DescribeReservedInstancesOfferingsInput{ + ProductDescription: types.RIProductDescription(productDesc), + InstanceTenancy: types.Tenancy(tenancy), + MinDuration: aws.Int64(duration), + MaxDuration: aws.Int64(duration), + OfferingClass: types.OfferingClassTypeConvertible, + OfferingType: offeringType, + IncludeMarketplace: aws.Bool(false), + MaxResults: aws.Int32(100), + NextToken: nextToken, + Filters: []types.Filter{{Name: aws.String("scope"), Values: []string{scope}}}, + } + result, err := c.client.DescribeReservedInstancesOfferings(ctx, input) + if err != nil { + return nil, fmt.Errorf("describe target offerings: %w", err) + } + out = appendTargetOfferings(out, result.ReservedInstancesOfferings, scope) + if isLastEC2Page(result.NextToken) { + break + } + nextToken = result.NextToken + if page == maxTargetOfferingPages { + // Return what we collected: the picker is best-effort and + // partial results are still useful to the user. + log.Printf("ListTargetOfferings: pagination cap (%d pages) reached, returning %d offerings", + maxTargetOfferingPages, len(out)) + } + } + return out, nil +} + // normalizationFactorForInstanceType extracts the size from an instance type -// (e.g., "m5.xlarge" → "xlarge") and returns the AWS normalization factor. +// (e.g., "m5.xlarge" -> "xlarge") and returns the AWS normalization factor. func normalizationFactorForInstanceType(instanceType string) float64 { parts := strings.SplitN(instanceType, ".", 2) if len(parts) != 2 {