From 5292988e6cd2fb8507e1ffe1a7139a9dee4552ac Mon Sep 17 00:00:00 2001 From: David Crespo Date: Sat, 24 Aug 2024 15:47:32 -0500 Subject: [PATCH 1/2] error if parent selectors given when resources are fetched by ID --- mock-api/msw/db.ts | 84 ++++++++++++++++++++++++++++++++-------- mock-api/msw/handlers.ts | 26 +++++++------ 2 files changed, 83 insertions(+), 27 deletions(-) diff --git a/mock-api/msw/db.ts b/mock-api/msw/db.ts index 4458dc4c1..423df85e9 100644 --- a/mock-api/msw/db.ts +++ b/mock-api/msw/db.ts @@ -26,17 +26,33 @@ export const notFoundErr = (msg: string) => { return json({ error_code: 'ObjectNotFound', message } as const, { status: 404 }) } -export const badSelectorErr = (resource: string, parents: string[]) => { - const message = `when ${resource} is specified by ID, ${commaSeries(parents, 'and')} should not be specified` - return json({ error_code: 'InvalidRequest', message }, { status: 400 }) -} - export const lookupById = (table: T[], id: string) => { const item = table.find((i) => i.id === id) if (!item) throw notFoundErr(`by id ${id}`) return item } +/** + * Given an object representing (potentially) parent selectors for a resource, + * throw an error if any of the keys in that object have truthy values. For + * example, if selecting an instance by ID, we would pass in an object with + * `project` as the key and error out if and only if `parentSelector.project` + * is present. + */ +function ensureNoParentSelectors( + /** Resource name to be used in error message */ + resourceLabel: string, + parentSelector: Record +) { + const keysWithValues = Object.entries(parentSelector) + .filter(([_, v]) => v) + .map(([k]) => k) + if (keysWithValues.length > 0) { + const message = `when ${resourceLabel} is specified by ID, ${commaSeries(keysWithValues, 'and')} should not be specified` + throw json({ error_code: 'InvalidRequest', message }, { status: 400 }) + } +} + export const getIpFromPool = (poolName: string | undefined) => { const pool = lookup.ipPool({ pool: poolName }) const ipPoolRange = db.ipPoolRanges.find((range) => range.ip_pool_id === pool.id) @@ -63,7 +79,10 @@ export const lookup = { instance({ instance: id, ...projectSelector }: PP.Instance): Json { if (!id) throw notFoundErr('no instance specified') - if (isUuid(id)) return lookupById(db.instances, id) + if (isUuid(id)) { + ensureNoParentSelectors('instance', projectSelector) + return lookupById(db.instances, id) + } const project = lookup.project(projectSelector) const instance = db.instances.find((i) => i.project_id === project.id && i.name === id) @@ -77,7 +96,10 @@ export const lookup = { }: PP.NetworkInterface): Json { if (!id) throw notFoundErr('no NIC specified') - if (isUuid(id)) return lookupById(db.networkInterfaces, id) + if (isUuid(id)) { + ensureNoParentSelectors('network interface', instanceSelector) + return lookupById(db.networkInterfaces, id) + } const instance = lookup.instance(instanceSelector) @@ -91,7 +113,10 @@ export const lookup = { disk({ disk: id, ...projectSelector }: PP.Disk): Json { if (!id) throw notFoundErr('no disk specified') - if (isUuid(id)) return lookupById(db.disks, id) + if (isUuid(id)) { + ensureNoParentSelectors('disk', projectSelector) + return lookupById(db.disks, id) + } const project = lookup.project(projectSelector) @@ -104,7 +129,7 @@ export const lookup = { if (!id) throw notFoundErr('no floating IP specified') if (isUuid(id)) { - if (projectSelector.project) throw badSelectorErr('floating IP', ['project']) + ensureNoParentSelectors('floating IP', projectSelector) return lookupById(db.floatingIps, id) } @@ -119,7 +144,10 @@ export const lookup = { snapshot({ snapshot: id, ...projectSelector }: PP.Snapshot): Json { if (!id) throw notFoundErr('no snapshot specified') - if (isUuid(id)) return lookupById(db.snapshots, id) + if (isUuid(id)) { + ensureNoParentSelectors('snapshot', projectSelector) + return lookupById(db.snapshots, id) + } const project = lookup.project(projectSelector) const snapshot = db.snapshots.find((i) => i.project_id === project.id && i.name === id) @@ -130,7 +158,10 @@ export const lookup = { vpc({ vpc: id, ...projectSelector }: PP.Vpc): Json { if (!id) throw notFoundErr('no VPC specified') - if (isUuid(id)) return lookupById(db.vpcs, id) + if (isUuid(id)) { + ensureNoParentSelectors('vpc', projectSelector) + return lookupById(db.vpcs, id) + } const project = lookup.project(projectSelector) const vpc = db.vpcs.find((v) => v.project_id === project.id && v.name === id) @@ -141,7 +172,10 @@ export const lookup = { vpcRouter({ router: id, ...vpcSelector }: PP.VpcRouter): Json { if (!id) throw notFoundErr('no router specified') - if (isUuid(id)) return lookupById(db.vpcRouters, id) + if (isUuid(id)) { + ensureNoParentSelectors('router', vpcSelector) + return lookupById(db.vpcRouters, id) + } const vpc = lookup.vpc(vpcSelector) const router = db.vpcRouters.find((r) => r.vpc_id === vpc.id && r.name === id) @@ -155,7 +189,10 @@ export const lookup = { }: PP.VpcRouterRoute): Json { if (!id) throw notFoundErr('no route specified') - if (isUuid(id)) return lookupById(db.vpcRouterRoutes, id) + if (isUuid(id)) { + ensureNoParentSelectors('route', routerSelector) + return lookupById(db.vpcRouterRoutes, id) + } const router = lookup.vpcRouter(routerSelector) const route = db.vpcRouterRoutes.find( @@ -168,7 +205,10 @@ export const lookup = { vpcSubnet({ subnet: id, ...vpcSelector }: PP.VpcSubnet): Json { if (!id) throw notFoundErr('no subnet specified') - if (isUuid(id)) return lookupById(db.vpcSubnets, id) + if (isUuid(id)) { + ensureNoParentSelectors('subnet', vpcSelector) + return lookupById(db.vpcSubnets, id) + } const vpc = lookup.vpc(vpcSelector) const subnet = db.vpcSubnets.find((s) => s.vpc_id === vpc.id && s.name === id) @@ -179,8 +219,13 @@ export const lookup = { image({ image: id, project: projectId }: PP.Image): Json { if (!id) throw notFoundErr('no image specified') - if (isUuid(id)) return lookupById(db.images, id) + if (isUuid(id)) { + ensureNoParentSelectors('image', { project: projectId }) + return lookupById(db.images, id) + } + // TODO: is this logic right? seems kinda weird. you should be able to look + // up either kind by ID. we should probably have two lookup functions let image: Json | undefined if (projectId === undefined) { // silo image @@ -264,8 +309,15 @@ export const lookup = { samlIdp({ provider: id, silo }: PP.IdentityProvider): Json { if (!id) throw notFoundErr('no IdP specified') - const dbSilo = lookup.silo({ silo }) + if (isUuid(id)) { + ensureNoParentSelectors('identity provider', { silo }) + return lookupById( + db.identityProviders.filter((o) => o.type === 'saml').map((o) => o.provider), + id + ) + } + const dbSilo = lookup.silo({ silo }) const dbIdp = db.identityProviders.find( ({ type, siloId, provider }) => type === 'saml' && siloId === dbSilo.id && provider.name === id diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index 29dff60b8..b58f96982 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -288,15 +288,17 @@ export const handlers = makeHandlers({ return 204 }, - floatingIpAttach({ path, query, body }) { - const floatingIp = lookup.floatingIp({ ...path, ...query }) - if (floatingIp.instance_id) { + floatingIpAttach({ path: { floatingIp }, query: { project }, body }) { + const dbFloatingIp = lookup.floatingIp({ floatingIp, project }) + if (dbFloatingIp.instance_id) { throw 'floating IP cannot be attached to one instance while still attached to another' } - const instance = lookup.instance({ ...path, ...query, instance: body.parent }) - floatingIp.instance_id = instance.id + // TODO: make sure the logic around when project is passed matches + // the API + const dbInstance = lookup.instance({ instance: body.parent }) + dbFloatingIp.instance_id = dbInstance.id - return floatingIp + return dbFloatingIp }, floatingIpDetach({ path, query }) { const floatingIp = lookup.floatingIp({ ...path, ...query }) @@ -359,13 +361,15 @@ export const handlers = makeHandlers({ return json(image, { status: 202 }) }, - imageDemote({ path, query }) { - const image = lookup.image({ ...path, ...query }) - const project = lookup.project({ ...path, ...query }) + imageDemote({ path: { image }, query: { project } }) { + // TODO: change this to lookup.siloImage when I add that. or at least + // check API logic to make sure we match it + const dbImage = lookup.image({ image }) + const dbProject = lookup.project({ project }) - image.project_id = project.id + dbImage.project_id = dbProject.id - return json(image, { status: 202 }) + return json(dbImage, { status: 202 }) }, instanceList({ query }) { const project = lookup.project(query) From f0fdcdb982b0006259525070de4526d5a947d328 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 26 Aug 2024 16:54:28 -0500 Subject: [PATCH 2/2] more detailed comments about API logic we're imitating --- mock-api/msw/db.ts | 24 ++++++++++++++++++------ mock-api/msw/handlers.ts | 18 ++++++++++++------ 2 files changed, 30 insertions(+), 12 deletions(-) diff --git a/mock-api/msw/db.ts b/mock-api/msw/db.ts index 423df85e9..0dcd648c3 100644 --- a/mock-api/msw/db.ts +++ b/mock-api/msw/db.ts @@ -219,21 +219,33 @@ export const lookup = { image({ image: id, project: projectId }: PP.Image): Json { if (!id) throw notFoundErr('no image specified') + // We match the API logic: + // + // match image_selector { + // (image ID, no project) => + // look up image in DB by ID + // if project ID is present, it's a project image, otherwise silo image + // (image Name, project specified) => it's a project image + // (image Name, no project) => it's a silo image + // (image ID, project specified) => error + // } + // + // https://github.com/oxidecomputer/omicron/blob/219121a/nexus/src/app/image.rs#L32-L76 + if (isUuid(id)) { + // this matches the error case above ensureNoParentSelectors('image', { project: projectId }) return lookupById(db.images, id) } - // TODO: is this logic right? seems kinda weird. you should be able to look - // up either kind by ID. we should probably have two lookup functions let image: Json | undefined - if (projectId === undefined) { - // silo image - image = db.images.find((d) => d.project_id === undefined && d.name === id) - } else { + if (projectId) { // project image const project = lookup.project({ project: projectId }) image = db.images.find((d) => d.project_id === project.id && d.name === id) + } else { + // silo image + image = db.images.find((d) => d.project_id === undefined && d.name === id) } if (!image) throw notFoundErr(`image '${id}'`) diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index b58f96982..8089c8351 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -7,7 +7,7 @@ */ import { delay } from 'msw' import * as R from 'remeda' -import { v4 as uuid } from 'uuid' +import { validate as isUuid, v4 as uuid } from 'uuid' import { diskCan, @@ -293,9 +293,14 @@ export const handlers = makeHandlers({ if (dbFloatingIp.instance_id) { throw 'floating IP cannot be attached to one instance while still attached to another' } - // TODO: make sure the logic around when project is passed matches - // the API - const dbInstance = lookup.instance({ instance: body.parent }) + // Following the API logic here, which says that when the instance is passed + // by name, we pull the project ID off the floating IP. + // + // https://github.com/oxidecomputer/omicron/blob/e434307/nexus/src/app/external_ip.rs#L171-L201 + const dbInstance = lookup.instance({ + instance: body.parent, + project: isUuid(body.parent) ? undefined : project, + }) dbFloatingIp.instance_id = dbInstance.id return dbFloatingIp @@ -362,8 +367,9 @@ export const handlers = makeHandlers({ return json(image, { status: 202 }) }, imageDemote({ path: { image }, query: { project } }) { - // TODO: change this to lookup.siloImage when I add that. or at least - // check API logic to make sure we match it + // unusual case because the project is never used to resolve the image. you + // can only demote silo images, and whether we have an image name or ID, if + // there is no project specified, the lookup assumes it's a silo image const dbImage = lookup.image({ image }) const dbProject = lookup.project({ project })