Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Mock API: error if parent selectors present when resource is fetched by ID #2396

Merged
merged 2 commits into from
Aug 29, 2024
+106 −32
Merged

Mock API: error if parent selectors present when resource is fetched by ID #2396

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5292988
error if parent selectors given when resources are fetched by ID
david-crespo Aug 24, 2024
f0fdcdb
more detailed comments about API logic we're imitating
david-crespo Aug 26, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
104 changes: 84 additions & 20 deletions mock-api/msw/db.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 = <T extends { id: string }>(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<string, string | undefined>
) {
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)
Expand All @@ -63,7 +79,10 @@ export const lookup = {
instance({ instance: id, ...projectSelector }: PP.Instance): Json<Api.Instance> {
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)
Expand All @@ -77,7 +96,10 @@ export const lookup = {
}: PP.NetworkInterface): Json<Api.InstanceNetworkInterface> {
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)

Expand All @@ -91,7 +113,10 @@ export const lookup = {
disk({ disk: id, ...projectSelector }: PP.Disk): Json<Api.Disk> {
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)

Expand All @@ -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)
}

Expand All @@ -119,7 +144,10 @@ export const lookup = {
snapshot({ snapshot: id, ...projectSelector }: PP.Snapshot): Json<Api.Snapshot> {
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)
Expand All @@ -130,7 +158,10 @@ export const lookup = {
vpc({ vpc: id, ...projectSelector }: PP.Vpc): Json<Api.Vpc> {
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)
Expand All @@ -141,7 +172,10 @@ export const lookup = {
vpcRouter({ router: id, ...vpcSelector }: PP.VpcRouter): Json<Api.VpcRouter> {
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)
Expand All @@ -155,7 +189,10 @@ export const lookup = {
}: PP.VpcRouterRoute): Json<Api.RouterRoute> {
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(
Expand All @@ -168,7 +205,10 @@ export const lookup = {
vpcSubnet({ subnet: id, ...vpcSelector }: PP.VpcSubnet): Json<Api.VpcSubnet> {
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)
}
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

There's a small downside to this implementation for double-nested resources like subnet: if you were to try to select a subnet by (subnet name, VPC ID, project name), the thing that would blow up would be the VPC lookup, and the error would be "when VPC is specified by ID, project should not be specified." When I put it that way, it's not that confusing. But if you only look at the error message and not what endpoint the error is coming from, you can get thrown off.


const vpc = lookup.vpc(vpcSelector)
const subnet = db.vpcSubnets.find((s) => s.vpc_id === vpc.id && s.name === id)
Expand All @@ -179,16 +219,33 @@ export const lookup = {
image({ image: id, project: projectId }: PP.Image): Json<Api.Image> {
if (!id) throw notFoundErr('no image specified')

if (isUuid(id)) return lookupById(db.images, id)
// 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)
}

let image: Json<Api.Image> | 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}'`)
Expand Down Expand Up @@ -264,8 +321,15 @@ export const lookup = {
samlIdp({ provider: id, silo }: PP.IdentityProvider): Json<Api.SamlIdentityProvider> {
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
Expand Down
34 changes: 22 additions & 12 deletions mock-api/msw/handlers.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -288,15 +288,22 @@ 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
// 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 floatingIp
return dbFloatingIp
},
floatingIpDetach({ path, query }) {
const floatingIp = lookup.floatingIp({ ...path, ...query })
Expand Down Expand Up @@ -359,13 +366,16 @@ 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 } }) {
// 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 })

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)
Expand Down
Loading