From 62e6e6599fa3a7856d8eabde923064c72b6f15c6 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Feb 2022 01:09:21 -0500 Subject: [PATCH 01/25] stub out firewall rule form based on API (needs design pass) --- .../VpcPage/modals/firewall-rules.tsx | 404 ++++++++++++++++++ .../VpcPage/tabs/VpcFirewallRulesTab.tsx | 56 ++- libs/ui/lib/checkbox/Checkbox.tsx | 13 +- libs/ui/lib/radio-group/RadioGroup.tsx | 8 +- libs/ui/lib/side-modal/SideModal.tsx | 2 +- libs/ui/lib/text-field/TextField.tsx | 29 +- package.json | 3 +- tailwind.config.js | 5 + yarn.lock | 40 ++ 9 files changed, 538 insertions(+), 22 deletions(-) create mode 100644 app/pages/project/networking/VpcPage/modals/firewall-rules.tsx diff --git a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx new file mode 100644 index 0000000000..9c416100b8 --- /dev/null +++ b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx @@ -0,0 +1,404 @@ +import React from 'react' +import { Form, Formik } from 'formik' +import * as Yup from 'yup' + +import { + Button, + CheckboxField, + Delete10Icon, + Dropdown, + FieldTitle, + NumberTextField, + Radio, + RadioGroup, + SideModal, + Table, + TextField, + TextFieldError, + TextFieldHint, +} from '@oxide/ui' +import type { VpcFirewallRule, ErrorResponse } from '@oxide/api' +import { useApiMutation, useApiQueryClient } from '@oxide/api' +import { getServerError } from 'app/util/errors' + +type FormProps = { + error: ErrorResponse | null + id: string +} + +// the moment the two forms diverge, inline them rather than introducing BS +// props here +const CommonForm = ({ id, error }: FormProps) => ( +
+ +
+ Priority + Must be 0–35535 + + +
+
+ Action + + Allow + Deny + +
+
+ Direction of traffic + + Incoming + Outgoing + +
+ + +

Targets

+ +
+ Name + +
+ +
+ + +
+ + + + + Type + Name + + + + + + VPC + target-vpc + + + + + + VPC Subnet + target-subnet + + + + + +
+ + +

Host filters

+ +
+ {/* For everything but IP this is a name, but for IP it's an IP. + So we should probably have the label on this field change when the + host type changes. Also need to confirm that it's just an IP and + not a block. */} + Value + + For IP, an address. For the rest, a name. [TODO: copy] + + +
+ +
+ + +
+ + + + + Type + Value + + + + + + VPC + my-vpc + + + + + +
+ + +
+ Port filter + + A single port (1234) or a range (1234-2345) + + + +
+ {/* TODO: ghost variant is wrong, we actually need a new one to match the design */} + + +
+
    +
  • + 1234 + +
    • +
  • + 456-567 + +
    • +
  • + 8080-8086 + +
    • +
+
+ + +
+ Protocols +
+ + TCP + +
+
+ + UDP + +
+
+ + ICMP + +
+
+ + + {/* omitting value prop makes it a boolean value. beautiful */} + Enabled +
+ + Name + + +
+
+ + Description {/* TODO: indicate optional */} + + +
+ + +
{getServerError(error)}
+ +
+) + +type CreateProps = { + isOpen: boolean + onDismiss: () => void + orgName: string + projectName: string + vpcName: string +} + +export function CreateFirewallRuleModal({ + isOpen, + onDismiss, + orgName, + projectName, + vpcName, +}: CreateProps) { + const parentIds = { orgName, projectName, vpcName } + const queryClient = useApiQueryClient() + + function dismiss() { + createRule.reset() + onDismiss() + } + + const createRule = useApiMutation('vpcFirewallRulesPut', { + onSuccess() { + queryClient.invalidateQueries('vpcFirewallRulesGet', parentIds) + dismiss() + }, + }) + + const formId = 'create-vpc-subnet-form' + + return ( + + { + console.log({ name, enabled, ...rest }) + // const status = values.enabled ? 'enabled' : 'disabled' + // createRule.mutate({ + // ...parentIds, + // body: { + // [name]: { status, ...rest}, + // }, + // }) + }} + > + + + + + + + + ) +} + +// TODO: right now edit works like the other resources in that it only knows +// about the rule being modified and expects to send that straight to the PUT +// endpoint. In reality (at least for now) the PUT endpoint expects the full +// list of rules, which means the edit modal should take the full list plus an +// ID for the one being edited. Then it can apply the user's changes to that one +// and PUT the full list. I am putting off implementing this aspect of it +// because we will probably get a normal PUT endpoint that relieves us of having +// to do all that. See https://github.com/oxidecomputer/omicron/issues/623 + +type EditProps = { + onDismiss: () => void + orgName: string + projectName: string + vpcName: string + originalRule: VpcFirewallRule | null +} + +export function EditFirewallRuleModal({ + onDismiss, + orgName, + projectName, + vpcName, + originalRule, +}: EditProps) { + const parentIds = { orgName, projectName, vpcName } + const queryClient = useApiQueryClient() + + function dismiss() { + updateRule.reset() + onDismiss() + } + + const updateRule = useApiMutation('vpcFirewallRulesPut', { + onSuccess() { + queryClient.invalidateQueries('vpcSubnetsGet', parentIds) + dismiss() + }, + }) + + if (!originalRule) return null + + const formId = 'edit-vpc-subnet-form' + return ( + + { + // updateRule.mutate({ + // ...parentIds, + // body: { + // name, + // description, + // }, + // }) + }} + > + + + + + + + + ) +} diff --git a/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx b/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx index 0f799bfcec..d27af96cef 100644 --- a/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx +++ b/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx @@ -1,3 +1,4 @@ +import type { MenuAction } from '@oxide/table' import { DateCell, EnabledCell, @@ -5,22 +6,59 @@ import { TypeValueListCell, useQueryTable, } from '@oxide/table' -import React from 'react' +import React, { useState } from 'react' import { useParams } from 'app/hooks' +import type { VpcFirewallRule } from '@oxide/api' +import { Button } from '@oxide/ui' +import { + CreateFirewallRuleModal, + EditFirewallRuleModal, +} from '../modals/firewall-rules' export const VpcFirewallRulesTab = () => { const vpcParams = useParams('orgName', 'projectName', 'vpcName') const { Table, Column } = useQueryTable('vpcFirewallRulesGet', vpcParams) + const [createModalOpen, setCreateModalOpen] = useState(false) + const [editing, setEditing] = useState(null) + + const actions = (rule: VpcFirewallRule): MenuAction[] => [ + { + label: 'Edit', + onActivate: () => setEditing(rule), + }, + ] + return ( - - - - - - - -
+ <> +
+ + setCreateModalOpen(false)} + /> + setEditing(null)} + /> +
+ + + + + + + +
+ ) } diff --git a/libs/ui/lib/checkbox/Checkbox.tsx b/libs/ui/lib/checkbox/Checkbox.tsx index df492f2f16..2dad72d7b3 100644 --- a/libs/ui/lib/checkbox/Checkbox.tsx +++ b/libs/ui/lib/checkbox/Checkbox.tsx @@ -1,5 +1,7 @@ import { Checkmark12Icon } from '@oxide/ui' import React from 'react' +import type { FieldAttributes } from 'formik' +import { Field } from 'formik' import { classed } from '@oxide/util' @@ -21,7 +23,7 @@ const inputStyle = ` export type CheckboxProps = { indeterminate?: boolean children?: React.ReactNode -} & React.ComponentProps<'input'> +} & Omit, 'type'> // ref function is from: https://davidwalsh.name/react-indeterminate. this makes // the native input work with indeterminate. you can't pass indeterminate as a @@ -29,6 +31,7 @@ export type CheckboxProps = { // examples using forwardRef to allow passing ref from outside: // https://github.com/tannerlinsley/react-table/discussions/1989 +/** Checkbox component that handles label, styling, and indeterminate state */ export const Checkbox = ({ indeterminate, children, @@ -53,3 +56,11 @@ export const Checkbox = ({ )} ) + +// eslint-disable-next-line @typescript-eslint/no-explicit-any +type CheckboxFieldProps = CheckboxProps & Omit, 'type'> + +/** Formik Field version of Checkbox */ +export const CheckboxField = (props: CheckboxFieldProps) => ( + +) diff --git a/libs/ui/lib/radio-group/RadioGroup.tsx b/libs/ui/lib/radio-group/RadioGroup.tsx index 53273e6df2..960fb13728 100644 --- a/libs/ui/lib/radio-group/RadioGroup.tsx +++ b/libs/ui/lib/radio-group/RadioGroup.tsx @@ -6,8 +6,8 @@ *
* Pick a foot * - * Left - * Right + * Left + * Right * *
* @@ -17,8 +17,8 @@ * Pick a foot *

Don't think about it too hard

* - * Left - * Right + * Left + * Right * * * diff --git a/libs/ui/lib/side-modal/SideModal.tsx b/libs/ui/lib/side-modal/SideModal.tsx index b571c93dbb..88d10e706d 100644 --- a/libs/ui/lib/side-modal/SideModal.tsx +++ b/libs/ui/lib/side-modal/SideModal.tsx @@ -31,7 +31,7 @@ export function SideModal({ aria-labelledby={titleId} >
{/* Title */} diff --git a/libs/ui/lib/text-field/TextField.tsx b/libs/ui/lib/text-field/TextField.tsx index afeccaadd6..c3ba9ae2f6 100644 --- a/libs/ui/lib/text-field/TextField.tsx +++ b/libs/ui/lib/text-field/TextField.tsx @@ -8,18 +8,20 @@ import { ErrorMessage, Field } from 'formik' // through, but couldn't get it to work. FieldAttributes is closest but // it makes a bunch of props required that should be optional. Instead we simply // take the props of an input field (which are part of the Field props) and -// manually tack on validate. Omit `type` because this is always a text field. -export type TextFieldProps = Omit, 'type'> & { +// manually tack on validate. +export type TextFieldProps = React.ComponentProps<'input'> & { validate?: FieldValidator // error is used to style the wrapper, also to put aria-invalid on the input error?: boolean disabled?: boolean className?: string + fieldClassName?: string } export const TextField = ({ error, className, + fieldClassName, ...fieldProps }: TextFieldProps) => (
) +// TODO: do this properly: extract a NumberField that styles the up and down +// buttons for when we do want them *and* add a flag to hide them using +// appearance-textfield +export const NumberTextField = ({ + fieldClassName, + ...props +}: Omit) => ( + +) + type HintProps = { // ID required as a reminder to pass aria-describedby on TextField id: string diff --git a/package.json b/package.json index 26cf5b543e..01fec66def 100644 --- a/package.json +++ b/package.json @@ -52,7 +52,8 @@ "react-transition-group": "^4.4.1", "recharts": "^2.1.6", "tslib": "^2.0.0", - "uuid": "^8.3.2" + "uuid": "^8.3.2", + "yup": "^0.32.11" }, "devDependencies": { "@babel/core": "^7.13.10", diff --git a/tailwind.config.js b/tailwind.config.js index e1b489ae8e..a33cb1ebbf 100644 --- a/tailwind.config.js +++ b/tailwind.config.js @@ -104,6 +104,11 @@ module.exports = { ) addUtilities(textUtilities) addUtilities(colorUtilities) + addUtilities({ + '.appearance-textfield': { + appearance: 'textfield', + }, + }) }), ], } diff --git a/yarn.lock b/yarn.lock index 7897e05706..6b9dbea274 100644 --- a/yarn.lock +++ b/yarn.lock @@ -1295,6 +1295,13 @@ dependencies: regenerator-runtime "^0.13.4" +"@babel/runtime@^7.15.4": + version "7.16.7" + resolved "https://registry.yarnpkg.com/@babel/runtime/-/runtime-7.16.7.tgz#03ff99f64106588c9c403c6ecb8c3bafbbdff1fa" + integrity sha512-9E9FJowqAsytyOY6LG+1KuueckRL+aQW+mKvXRXnuFGyRAyepJPmEo9vgMfXUA6O9u3IeEdv9MAkppFcaQwogQ== + dependencies: + regenerator-runtime "^0.13.4" + "@babel/template@^7.12.13", "@babel/template@^7.12.7", "@babel/template@^7.15.4": version "7.15.4" resolved "https://registry.npmjs.org/@babel/template/-/template-7.15.4.tgz" @@ -3337,6 +3344,11 @@ "@types/interpret" "*" "@types/node" "*" +"@types/lodash@^4.14.175": + version "4.14.178" + resolved "https://registry.yarnpkg.com/@types/lodash/-/lodash-4.14.178.tgz#341f6d2247db528d4a13ddbb374bcdc80406f4f8" + integrity sha512-0d5Wd09ItQWH1qFbEyQ7oTQ3GZrMfth5JkbN3EvTKLXcHLRDSXeLnlvlOn0wvxVIwK5o2M8JzP/OWz7T3NRsbw== + "@types/mdast@^3.0.0": version "3.0.3" resolved "https://registry.npmjs.org/@types/mdast/-/mdast-3.0.3.tgz" @@ -10020,6 +10032,11 @@ nano-time@1.0.0: dependencies: big-integer "^1.6.16" +nanoclone@^0.2.1: + version "0.2.1" + resolved "https://registry.yarnpkg.com/nanoclone/-/nanoclone-0.2.1.tgz#dd4090f8f1a110d26bb32c49ed2f5b9235209ed4" + integrity sha512-wynEP02LmIbLpcYw8uBKpcfF6dmg2vcpKqxeH5UcoKEYdExslsdUA4ugFauuaeYdTB76ez6gJW8XAZ6CgkXYxA== + nanoid@^3.1.23, nanoid@^3.1.28, nanoid@^3.1.30: version "3.2.0" resolved "https://registry.yarnpkg.com/nanoid/-/nanoid-3.2.0.tgz#62667522da6673971cca916a6d3eff3f415ff80c" @@ -11218,6 +11235,11 @@ prop-types@^15.0.0, prop-types@^15.6.0, prop-types@^15.6.2, prop-types@^15.7.2: object-assign "^4.1.1" react-is "^16.8.1" +property-expr@^2.0.4: + version "2.0.5" + resolved "https://registry.yarnpkg.com/property-expr/-/property-expr-2.0.5.tgz#278bdb15308ae16af3e3b9640024524f4dc02cb4" + integrity sha512-IJUkICM5dP5znhCckHSv30Q4b5/JA5enCtkRHYaOVOAocnH/1BQEYTC5NMfT3AVl/iXKdr3aqQbQn9DxyWknwA== + property-information@^5.0.0, property-information@^5.3.0: version "5.6.0" resolved "https://registry.npmjs.org/property-information/-/property-information-5.6.0.tgz" @@ -13306,6 +13328,11 @@ token-transformer@^0.0.17: dependencies: yargs "^17.3.1" +toposort@^2.0.2: + version "2.0.2" + resolved "https://registry.yarnpkg.com/toposort/-/toposort-2.0.2.tgz#ae21768175d1559d48bef35420b2f4962f09c330" + integrity sha1-riF2gXXRVZ1IvvNUILL0li8JwzA= + tough-cookie@^4.0.0: version "4.0.0" resolved "https://registry.npmjs.org/tough-cookie/-/tough-cookie-4.0.0.tgz" @@ -14375,6 +14402,19 @@ yocto-queue@^0.1.0: resolved "https://registry.npmjs.org/yocto-queue/-/yocto-queue-0.1.0.tgz" integrity sha512-rVksvsnNCdJ/ohGc6xgPwyN8eheCxsiLM8mxuE/t/mOVqJewPuO1miLpTHQiRgTKCLexL4MeAFVagts7HmNZ2Q== +yup@^0.32.11: + version "0.32.11" + resolved "https://registry.yarnpkg.com/yup/-/yup-0.32.11.tgz#d67fb83eefa4698607982e63f7ca4c5ed3cf18c5" + integrity sha512-Z2Fe1bn+eLstG8DRR6FTavGD+MeAwyfmouhHsIUgaADz8jvFKbO/fXc2trJKZg+5EBjh4gGm3iU/t3onKlXHIg== + dependencies: + "@babel/runtime" "^7.15.4" + "@types/lodash" "^4.14.175" + lodash "^4.17.21" + lodash-es "^4.17.21" + nanoclone "^0.2.1" + property-expr "^2.0.4" + toposort "^2.0.2" + zwitch@^1.0.0: version "1.0.5" resolved "https://registry.npmjs.org/zwitch/-/zwitch-1.0.5.tgz" From a8a9d49ecdcaca87d584167709f3bbe551e75a7c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Feb 2022 23:03:37 -0500 Subject: [PATCH 02/25] msw endpoint for get rules --- .../VpcPage/modals/firewall-rules.tsx | 4 +- libs/api-mocks/msw/db.ts | 1 + libs/api-mocks/msw/handlers.ts | 11 +++ libs/api-mocks/vpc.ts | 79 ++++++++++++++++++- 4 files changed, 91 insertions(+), 4 deletions(-) diff --git a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx index 9c416100b8..c131bf92d8 100644 --- a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx +++ b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx @@ -33,7 +33,7 @@ const CommonForm = ({ id, error }: FormProps) => (
Priority - Must be 0–35535 + Must be 0–65535 JSON.parse(JSON.stringify(o)) diff --git a/libs/api-mocks/msw/handlers.ts b/libs/api-mocks/msw/handlers.ts index 31ad942091..a0dd5c9206 100644 --- a/libs/api-mocks/msw/handlers.ts +++ b/libs/api-mocks/msw/handlers.ts @@ -311,4 +311,15 @@ export const handlers = [ return res(ctx.status(204)) } ), + + rest.get | GetErr>( + '/api/organizations/:orgName/projects/:projectName/vpcs/:vpcName/firewall/rules', + (req, res, ctx) => { + const vpc = lookupVpc(req, res, ctx) + if (vpc.err) return vpc.err + // TODO: uncomment once omicron PR lands + const items = db.vpcFirewallRules //.filter((r) => r.vpc_id === vpc.ok.id) + return res(json({ items })) + } + ), ] diff --git a/libs/api-mocks/vpc.ts b/libs/api-mocks/vpc.ts index 02a4ec079d..ed2ad1e6f5 100644 --- a/libs/api-mocks/vpc.ts +++ b/libs/api-mocks/vpc.ts @@ -2,20 +2,24 @@ import type { Json } from './json-type' import { project } from './project' import type { Vpc, + VpcFirewallRule, VpcResultsPage, VpcSubnet, VpcSubnetResultsPage, } from '@oxide/api' +const time_created = new Date(2021, 0, 1).toISOString() +const time_modified = new Date(2021, 0, 2).toISOString() + export const vpc: Json = { id: 'vpc-id', name: 'mock-vpc', description: 'a fake vpc', dns_name: 'mock-vpc', - time_created: new Date(2021, 0, 1).toISOString(), - time_modified: new Date(2021, 0, 2).toISOString(), project_id: project.id, system_router_id: 'router-id', // ??? + time_created, + time_modified, } export const vpcs: Json = { items: [vpc] } @@ -43,3 +47,74 @@ export const vpcSubnet2: Json = { export const vpcSubnets: Json = { items: [vpcSubnet], } + +// TODO: uncomment vpc_ids once omicron PR lands +export const defaultFirewallRules: Json[] = [ + { + id: 'firewall-rule-id-1', + name: 'allow-internal-inbound', + status: 'enabled', + direction: 'inbound', + targets: [{ type: 'vpc', value: 'default' }], + action: 'allow', + description: + 'allow inbound traffic to all instances within the VPC if originated within the VPC', + filters: { + hosts: [{ type: 'vpc', value: 'default' }], + }, + priority: 65534, + time_created, + time_modified, + // vpc_id: vpc.id, + }, + { + id: 'firewall-rule-id-2', + name: 'allow-ssh', + status: 'enabled', + direction: 'inbound', + targets: [{ type: 'vpc', value: 'default' }], + description: 'allow inbound TCP connections on port 22 from anywhere', + filters: { + ports: ['22'], + protocols: ['TCP'], + }, + action: 'allow', + priority: 65534, + time_created, + time_modified, + // vpc_id: vpc.id, + }, + { + id: 'firewall-rule-id-3', + name: 'allow-icmp', + status: 'enabled', + direction: 'inbound', + targets: [{ type: 'vpc', value: 'default' }], + description: 'allow inbound ICMP traffic from anywhere', + filters: { + protocols: ['ICMP'], + }, + action: 'allow', + priority: 65534, + time_created, + time_modified, + // vpc_id: vpc.id, + }, + { + id: 'firewall-rule-id-4', + name: 'allow-rdp', + status: 'enabled', + direction: 'inbound', + targets: [{ type: 'vpc', value: 'default' }], + description: 'allow inbound TCP connections on port 3389 from anywhere', + filters: { + ports: ['3389'], + protocols: ['TCP'], + }, + action: 'allow', + priority: 65534, + time_created, + time_modified, + // vpc_id: vpc.id, + }, +] From 9882b5ba3f1e84a1d922a9cd3e159d9931d2abf8 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Feb 2022 23:55:37 -0500 Subject: [PATCH 03/25] hack in vpcId on firewall rule in anticipation of omicron#663 --- libs/api-mocks/msw/handlers.ts | 3 +-- libs/api-mocks/vpc.ts | 9 ++++----- libs/api/__generated__/Api.ts | 4 ++++ 3 files changed, 9 insertions(+), 7 deletions(-) diff --git a/libs/api-mocks/msw/handlers.ts b/libs/api-mocks/msw/handlers.ts index a0dd5c9206..2ed10269a7 100644 --- a/libs/api-mocks/msw/handlers.ts +++ b/libs/api-mocks/msw/handlers.ts @@ -317,8 +317,7 @@ export const handlers = [ (req, res, ctx) => { const vpc = lookupVpc(req, res, ctx) if (vpc.err) return vpc.err - // TODO: uncomment once omicron PR lands - const items = db.vpcFirewallRules //.filter((r) => r.vpc_id === vpc.ok.id) + const items = db.vpcFirewallRules.filter((r) => r.vpc_id === vpc.ok.id) return res(json({ items })) } ), diff --git a/libs/api-mocks/vpc.ts b/libs/api-mocks/vpc.ts index ed2ad1e6f5..144fbd8ba0 100644 --- a/libs/api-mocks/vpc.ts +++ b/libs/api-mocks/vpc.ts @@ -48,7 +48,6 @@ export const vpcSubnets: Json = { items: [vpcSubnet], } -// TODO: uncomment vpc_ids once omicron PR lands export const defaultFirewallRules: Json[] = [ { id: 'firewall-rule-id-1', @@ -65,7 +64,7 @@ export const defaultFirewallRules: Json[] = [ priority: 65534, time_created, time_modified, - // vpc_id: vpc.id, + vpc_id: vpc.id, }, { id: 'firewall-rule-id-2', @@ -82,7 +81,7 @@ export const defaultFirewallRules: Json[] = [ priority: 65534, time_created, time_modified, - // vpc_id: vpc.id, + vpc_id: vpc.id, }, { id: 'firewall-rule-id-3', @@ -98,7 +97,7 @@ export const defaultFirewallRules: Json[] = [ priority: 65534, time_created, time_modified, - // vpc_id: vpc.id, + vpc_id: vpc.id, }, { id: 'firewall-rule-id-4', @@ -115,6 +114,6 @@ export const defaultFirewallRules: Json[] = [ priority: 65534, time_created, time_modified, - // vpc_id: vpc.id, + vpc_id: vpc.id, }, ] diff --git a/libs/api/__generated__/Api.ts b/libs/api/__generated__/Api.ts index cb9491ac41..c015e994e9 100644 --- a/libs/api/__generated__/Api.ts +++ b/libs/api/__generated__/Api.ts @@ -831,6 +831,10 @@ export type VpcFirewallRule = { * timestamp when this resource was last modified */ timeModified: Date + /** + * the VPC to which this rule belongs + */ + vpcId: string } export type VpcFirewallRuleAction = 'allow' | 'deny' From 36dbae37083ebde2c32c6433163cdb122bf46721 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 2 Feb 2022 01:34:48 -0500 Subject: [PATCH 04/25] working but ugly targets subform --- .../VpcPage/modals/firewall-rules.tsx | 424 ++++++++++-------- libs/ui/lib/dropdown/Dropdown.tsx | 5 + 2 files changed, 234 insertions(+), 195 deletions(-) diff --git a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx index c131bf92d8..fcd1ea14f7 100644 --- a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx +++ b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx @@ -1,5 +1,5 @@ import React from 'react' -import { Form, Formik } from 'formik' +import { Form, Formik, useFormikContext } from 'formik' import * as Yup from 'yup' import { @@ -26,217 +26,247 @@ type FormProps = { id: string } +// TODO (can pass to useFormikContext to get it to behave) +// type FormState = {} + // the moment the two forms diverge, inline them rather than introducing BS // props here -const CommonForm = ({ id, error }: FormProps) => ( -
- -
- Priority - Must be 0–65535 - { + const { + setFieldValue, + values: { targetName, targetType, targets }, + } = useFormikContext() + return ( + + +
+ Priority + + Must be 0–65535 + + + +
+
+ Action + + Allow + Deny + +
+
+ Direction of traffic + + Incoming + Outgoing + +
+ + +

Targets

+ { + setFieldValue('targetType', item?.value) + }} /> - -
-
- Action - - Allow - Deny - -
-
- Direction of traffic - - Incoming - Outgoing - -
- - -

Targets

- -
- Name - -
+
+ Name + +
-
- - -
+
+ + +
- - - - Type - Name - - - - - - VPC - target-vpc - - - - - - VPC Subnet - target-subnet - - - - - -
- - -

Host filters

- -
- {/* For everything but IP this is a name, but for IP it's an IP. + + + + Type + Name + + + + + {targets.map((t) => ( + + {t.type} + {t.name} + + { + setFieldValue( + 'targets', + targets.filter((t1) => t1.name !== t.name) + ) + }} + /> + + + ))} + +
+ + +

Host filters

+ +
+ {/* For everything but IP this is a name, but for IP it's an IP. So we should probably have the label on this field change when the host type changes. Also need to confirm that it's just an IP and not a block. */} - Value - - For IP, an address. For the rest, a name. [TODO: copy] - - -
- -
- - -
+ Value + + For IP, an address. For the rest, a name. [TODO: copy] + + +
- - - - Type - Value - - - - - - VPC - my-vpc - - - - - -
- - -
- Port filter - - A single port (1234) or a range (1234-2345) - - -
- {/* TODO: ghost variant is wrong, we actually need a new one to match the design */} - +
-
    -
  • - 1234 - -
    • -
  • - 456-567 - -
    • -
  • - 8080-8086 - -
    • -
-
- - -
- Protocols -
- - TCP - + + + + + Type + Value + + + + + + VPC + my-vpc + + + + + +
+ + +
+ Port filter + + A single port (1234) or a range (1234-2345) + + + +
+ {/* TODO: ghost variant is wrong, we actually need a new one to match the design */} + + +
+
    +
  • + 1234 + +
    • +
  • + 456-567 + +
    • +
  • + 8080-8086 + +
    • +
-
- - UDP - + + +
+ Protocols +
+ + TCP + +
+
+ + UDP + +
+
+ + ICMP + +
+
+ + + {/* omitting value prop makes it a boolean value. beautiful */} + Enabled +
+ + Name + +
-
- - ICMP - +
+ + Description {/* TODO: indicate optional */} + +
-
- - - {/* omitting value prop makes it a boolean value. beautiful */} - Enabled -
- - Name - - -
-
- - Description {/* TODO: indicate optional */} - - -
- - -
{getServerError(error)}
- -
-) + + +
{getServerError(error)}
+ + + ) +} type CreateProps = { isOpen: boolean @@ -291,7 +321,11 @@ export function CreateFirewallRuleModal({ protocols: [], ports: [], hosts: [], + + // target subform targets: [], + targetType: '', + targetName: '', }} validationSchema={Yup.object({ priority: Yup.number() diff --git a/libs/ui/lib/dropdown/Dropdown.tsx b/libs/ui/lib/dropdown/Dropdown.tsx index 709d9cb040..330b6049e6 100644 --- a/libs/ui/lib/dropdown/Dropdown.tsx +++ b/libs/ui/lib/dropdown/Dropdown.tsx @@ -22,6 +22,7 @@ export interface DropdownProps { */ showLabel?: boolean className?: string + onChange?: (value: Item | null | undefined) => void } export const Dropdown: FC = ({ @@ -32,12 +33,16 @@ export const Dropdown: FC = ({ placeholder, showLabel = true, className, + onChange, }) => { const itemToString = (item: Item | null) => (item ? item.label : '') const select = useSelect({ initialSelectedItem: items.find((i) => i.value === defaultValue) || null, items, itemToString, + onSelectedItemChange(changes) { + onChange?.(changes.selectedItem) + }, }) const hintId = hint ? `${select.getLabelProps().id}-hint` : null From cfb5eb267bbc6e930e15291edf5e0bc18b943345 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 2 Feb 2022 12:40:40 -0500 Subject: [PATCH 05/25] ports, hosts, and targets subforms actually work --- .../VpcPage/modals/firewall-rules.tsx | 217 +++++++++++++----- .../VpcPage/tabs/VpcFirewallRulesTab.tsx | 2 +- libs/api/index.ts | 1 + libs/api/parse.spec.ts | 33 +++ libs/api/parse.ts | 18 ++ 5 files changed, 209 insertions(+), 62 deletions(-) create mode 100644 libs/api/parse.spec.ts create mode 100644 libs/api/parse.ts diff --git a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx index fcd1ea14f7..fbf41164a6 100644 --- a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx +++ b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx @@ -18,7 +18,7 @@ import { TextFieldHint, } from '@oxide/ui' import type { VpcFirewallRule, ErrorResponse } from '@oxide/api' -import { useApiMutation, useApiQueryClient } from '@oxide/api' +import { parsePortRange, useApiMutation, useApiQueryClient } from '@oxide/api' import { getServerError } from 'app/util/errors' type FormProps = { @@ -26,16 +26,37 @@ type FormProps = { id: string } -// TODO (can pass to useFormikContext to get it to behave) -// type FormState = {} +type Values = { + enabled: boolean + priority: string + name: string + description: string + action: VpcFirewallRule['action'] + direction: VpcFirewallRule['direction'] + + protocols: NonNullable + + // port subform + ports: NonNullable + portRange: string + + // host subform + hosts: NonNullable + hostType: string + hostValue: string + + // target subform + targets: VpcFirewallRule['targets'] + targetType: string + targetValue: string +} + +// TODO: pressing enter in ports, hosts, and targets value field should "submit" subform // the moment the two forms diverge, inline them rather than introducing BS // props here const CommonForm = ({ id, error }: FormProps) => { - const { - setFieldValue, - values: { targetName, targetType, targets }, - } = useFormikContext() + const { setFieldValue, values } = useFormikContext() return (
@@ -75,28 +96,39 @@ const CommonForm = ({ id, error }: FormProps) => { { value: 'subnet', label: 'VPC Subnet' }, { value: 'instance', label: 'Instance' }, ]} - // TODO: this is kind of a hack? move this inside Dropdown somehow onChange={(item) => { setFieldValue('targetType', item?.value) }} />
- Name - + Name +
+ {/* TODO does this clear out the form or the existing targets? */} - +
@@ -177,13 +237,26 @@ const CommonForm = ({ id, error }: FormProps) => { - - VPC - my-vpc - - - - + {values.hosts.map((h) => ( + + {/* TODO: should be the pretty type label, not the type key */} + {h.type} + {h.value} + + { + setFieldValue( + 'hosts', + values.hosts.filter( + (h1) => h1.value !== h.value && h1.type !== h.type + ) + ) + }} + /> + + + ))}
@@ -204,21 +277,34 @@ const CommonForm = ({ id, error }: FormProps) => { - +
    -
  • - 1234 - -
    • -
  • - 456-567 - -
    • -
  • - 8080-8086 - -
    • + {values.ports.map((p) => ( +
  • + {p} + { + setFieldValue( + 'ports', + values.ports.filter((p1) => p1 !== p) + ) + }} + /> +
    • + ))}
@@ -308,25 +394,34 @@ export function CreateFirewallRuleModal({ onDismiss={dismiss} > { const { Table, Column } = useQueryTable('vpcFirewallRulesGet', vpcParams) - const [createModalOpen, setCreateModalOpen] = useState(false) + const [createModalOpen, setCreateModalOpen] = useState(true) const [editing, setEditing] = useState(null) const actions = (rule: VpcFirewallRule): MenuAction[] => [ diff --git a/libs/api/index.ts b/libs/api/index.ts index b56ac09a69..7815a4395a 100644 --- a/libs/api/index.ts +++ b/libs/api/index.ts @@ -26,6 +26,7 @@ export const useApiQuery = getUseApiQuery(api.methods) export const useApiMutation = getUseApiMutation(api.methods) export const useApiQueryClient = getUseApiQueryClient() +export * from './parse' export * from './__generated__/Api' // for convenience so we can do `import type { ApiTypes } from '@oxide/api'` diff --git a/libs/api/parse.spec.ts b/libs/api/parse.spec.ts new file mode 100644 index 0000000000..fac9a05856 --- /dev/null +++ b/libs/api/parse.spec.ts @@ -0,0 +1,33 @@ +import { parsePortRange } from './parse' + +describe('parsePortRange', () => { + describe('parses', () => { + it('parses single ports up to 5 digits', () => { + expect(parsePortRange('0')).toEqual([0, 0]) + expect(parsePortRange('1')).toEqual([1, 1]) + expect(parsePortRange('123')).toEqual([123, 123]) + expect(parsePortRange('12356')).toEqual([12356, 12356]) + }) + + it('parses ranges', () => { + expect(parsePortRange('123-456')).toEqual([123, 456]) + expect(parsePortRange('1-45690')).toEqual([1, 45690]) + expect(parsePortRange('5-5')).toEqual([5, 5]) + }) + }) + + describe('rejects', () => { + it('nonsense', () => { + expect(parsePortRange('12a5')).toEqual(null) + expect(parsePortRange('lkajsdfha')).toEqual(null) + }) + + it('p2 < p1', () => { + expect(parsePortRange('123-45')).toEqual(null) + }) + + it('too many digits', () => { + expect(parsePortRange('239032')).toEqual(null) + }) + }) +}) diff --git a/libs/api/parse.ts b/libs/api/parse.ts new file mode 100644 index 0000000000..329f11aa46 --- /dev/null +++ b/libs/api/parse.ts @@ -0,0 +1,18 @@ +type PortRange = [number, number] + +/** Parse '1234' into [1234, 1234] and '80-100' into [80, 100] */ +// TODO: parsing should probably throw errors rather than returning +// null so we can annotate the failure with a reason +export function parsePortRange(portRange: string): PortRange | null { + // TODO: pull pattern from openapi spec (requires generator work) + const match = /^([0-9]{1,5})((?:-)[0-9]{1,5})?$/.exec(portRange) + if (!match) return null + + const p1 = parseInt(match[1], 10) + // API treats a single port as a range with the same start and end + const p2 = match[2] ? parseInt(match[2].slice(1), 10) : p1 + + if (p2 < p1) return null + + return [p1, p2] +} From 6a71251cdba873ff925a5ae7b5cdcf3c3ac8c287 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 2 Feb 2022 17:20:12 -0500 Subject: [PATCH 06/25] we can PUT rules! --- .../VpcPage/modals/firewall-rules.tsx | 30 +++++++++++++------ 1 file changed, 21 insertions(+), 9 deletions(-) diff --git a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx index fbf41164a6..5c179575b5 100644 --- a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx +++ b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx @@ -430,15 +430,27 @@ export function CreateFirewallRuleModal({ .required('Required'), })} validateOnBlur - onSubmit={({ name, enabled, ...rest }) => { - console.log({ name, enabled, ...rest }) - // const status = values.enabled ? 'enabled' : 'disabled' - // createRule.mutate({ - // ...parentIds, - // body: { - // [name]: { status, ...rest}, - // }, - // }) + onSubmit={({ name, ...values }) => { + console.log({ name, ...values }) + createRule.mutate({ + ...parentIds, + body: { + [name]: { + status: values.enabled ? 'enabled' : 'disabled', + action: values.action, + description: values.description, + direction: values.direction, + filters: { + hosts: values.hosts, + ports: values.ports, + protocols: values.protocols, + }, + priority: parseInt(values.priority, 10), + targets: values.targets, + }, + // TODO: need to spread all existing rules here + }, + }) }} > From 271bb26dbcc2b925ab63a53e4099ea01891f1b8e Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 2 Feb 2022 18:15:22 -0500 Subject: [PATCH 07/25] get rid of vestigial subnet copy, move name/description up --- .../VpcPage/modals/firewall-rules.tsx | 51 +++++++++---------- .../VpcPage/tabs/VpcFirewallRulesTab.tsx | 2 +- 2 files changed, 25 insertions(+), 28 deletions(-) diff --git a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx index 5c179575b5..1eced82c81 100644 --- a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx +++ b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx @@ -59,6 +59,21 @@ const CommonForm = ({ id, error }: FormProps) => { const { setFieldValue, values } = useFormikContext() return ( + + {/* omitting value prop makes it a boolean value. beautiful */} + {/* TODO: better text or heading or tip or something on this checkbox */} + Enabled +
+ Name + +
+
+ + Description {/* TODO: indicate optional */} + + +
+
Priority @@ -328,25 +343,6 @@ const CommonForm = ({ id, error }: FormProps) => {
- - {/* omitting value prop makes it a boolean value. beautiful */} - Enabled -
- - Name - - -
-
- - Description {/* TODO: indicate optional */} - - -
-
{getServerError(error)}
 @@ -384,7 +380,7 @@ export function CreateFirewallRuleModal({ }, }) - const formId = 'create-vpc-subnet-form' + const formId = 'create-firewall-rule-form' return ( diff --git a/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx b/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx index 7a26c072c0..d27af96cef 100644 --- a/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx +++ b/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx @@ -20,7 +20,7 @@ export const VpcFirewallRulesTab = () => { const { Table, Column } = useQueryTable('vpcFirewallRulesGet', vpcParams) - const [createModalOpen, setCreateModalOpen] = useState(true) + const [createModalOpen, setCreateModalOpen] = useState(false) const [editing, setEditing] = useState(null) const actions = (rule: VpcFirewallRule): MenuAction[] => [ From 03fe20c95a661576f8840de766b67f00e507fefa Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 3 Feb 2022 01:20:55 -0500 Subject: [PATCH 08/25] include all existing rules when creating one --- .../VpcPage/modals/firewall-rules.tsx | 20 +++++++++++++++++-- .../VpcPage/tabs/VpcFirewallRulesTab.tsx | 3 +++ package.json | 1 + yarn.lock | 2 +- 4 files changed, 23 insertions(+), 3 deletions(-) diff --git a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx index 1eced82c81..7ae9faabcf 100644 --- a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx +++ b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx @@ -1,6 +1,7 @@ import React from 'react' import { Form, Formik, useFormikContext } from 'formik' import * as Yup from 'yup' +import omit from 'lodash/omit' import { Button, @@ -17,7 +18,11 @@ import { TextFieldError, TextFieldHint, } from '@oxide/ui' -import type { VpcFirewallRule, ErrorResponse } from '@oxide/api' +import type { + VpcFirewallRule, + ErrorResponse, + VpcFirewallRuleUpdateParams, +} from '@oxide/api' import { parsePortRange, useApiMutation, useApiQueryClient } from '@oxide/api' import { getServerError } from 'app/util/errors' @@ -356,6 +361,16 @@ type CreateProps = { orgName: string projectName: string vpcName: string + existingRules: VpcFirewallRule[] +} + +function rulesArrToObj(rules: VpcFirewallRule[]): VpcFirewallRuleUpdateParams { + const obj: VpcFirewallRuleUpdateParams = {} + for (const rule of rules) { + const { name } = rule + obj[name] = omit(rule, 'id', 'name', 'timeCreated', 'timeModified', 'vpcId') + } + return obj } export function CreateFirewallRuleModal({ @@ -364,6 +379,7 @@ export function CreateFirewallRuleModal({ orgName, projectName, vpcName, + existingRules, }: CreateProps) { const parentIds = { orgName, projectName, vpcName } const queryClient = useApiQueryClient() @@ -432,6 +448,7 @@ export function CreateFirewallRuleModal({ createRule.mutate({ ...parentIds, body: { + ...rulesArrToObj(existingRules), [name]: { status: values.enabled ? 'enabled' : 'disabled', action: values.action, @@ -445,7 +462,6 @@ export function CreateFirewallRuleModal({ priority: parseInt(values.priority, 10), targets: values.targets, }, - // TODO: need to spread all existing rules here }, }) }} diff --git a/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx b/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx index d27af96cef..5334b83b9f 100644 --- a/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx +++ b/app/pages/project/networking/VpcPage/tabs/VpcFirewallRulesTab.tsx @@ -9,6 +9,7 @@ import { import React, { useState } from 'react' import { useParams } from 'app/hooks' import type { VpcFirewallRule } from '@oxide/api' +import { useApiQuery } from '@oxide/api' import { Button } from '@oxide/ui' import { CreateFirewallRuleModal, @@ -18,6 +19,7 @@ import { export const VpcFirewallRulesTab = () => { const vpcParams = useParams('orgName', 'projectName', 'vpcName') + const { data: rules } = useApiQuery('vpcFirewallRulesGet', vpcParams) const { Table, Column } = useQueryTable('vpcFirewallRulesGet', vpcParams) const [createModalOpen, setCreateModalOpen] = useState(false) @@ -44,6 +46,7 @@ export const VpcFirewallRulesTab = () => { {...vpcParams} isOpen={createModalOpen} onDismiss={() => setCreateModalOpen(false)} + existingRules={rules?.items || []} /> Date: Thu, 3 Feb 2022 17:55:32 -0500 Subject: [PATCH 09/25] be super sure we're not getting any extra nonsense in the update object --- .../VpcPage/modals/firewall-rules.tsx | 25 ++++++++++----- package.json | 1 - types/util.d.ts | 31 +++++++++++++++++++ 3 files changed, 49 insertions(+), 8 deletions(-) diff --git a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx index 7ae9faabcf..b9f30d9be3 100644 --- a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx +++ b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx @@ -1,7 +1,7 @@ import React from 'react' import { Form, Formik, useFormikContext } from 'formik' import * as Yup from 'yup' -import omit from 'lodash/omit' +import { pick } from '@oxide/util' import { Button, @@ -21,6 +21,7 @@ import { import type { VpcFirewallRule, ErrorResponse, + VpcFirewallRuleUpdate, VpcFirewallRuleUpdateParams, } from '@oxide/api' import { parsePortRange, useApiMutation, useApiQueryClient } from '@oxide/api' @@ -365,12 +366,22 @@ type CreateProps = { } function rulesArrToObj(rules: VpcFirewallRule[]): VpcFirewallRuleUpdateParams { - const obj: VpcFirewallRuleUpdateParams = {} - for (const rule of rules) { - const { name } = rule - obj[name] = omit(rule, 'id', 'name', 'timeCreated', 'timeModified', 'vpcId') - } - return obj + return Object.fromEntries( + rules.map((rule) => { + const ruleUpdate: NoExtraKeys = + pick( + rule, + 'action', + 'description', + 'direction', + 'filters', + 'priority', + 'status', + 'targets' + ) + return [rule.name, ruleUpdate] + }) + ) } export function CreateFirewallRuleModal({ diff --git a/package.json b/package.json index 36cf460699..01fec66def 100644 --- a/package.json +++ b/package.json @@ -37,7 +37,6 @@ "filesize": "^6.3.0", "formik": "^2.2.9", "history": "^5.0.0", - "lodash-es": "^4.17.21", "match-sorter": "^6.3.0", "mousetrap": "^1.6.5", "prop-types": "^15.7.2", diff --git a/types/util.d.ts b/types/util.d.ts index 9ab07c2da5..a33d66d9f9 100644 --- a/types/util.d.ts +++ b/types/util.d.ts @@ -13,3 +13,34 @@ type Optional = Pick, K> & Omit /** Join types for `P1` and `P2` where `P2` takes precedence in conflicts */ type Assign = Omit & P2 + +/** + * Produce a version of `FewerKeys` that assigns a type of `never` to all keys + * of `MoreKeys` that aren't in `FewerKeys`. Why? Good question: + * + * In most cases TS will error if you try to assign an object to a variable if + * there are keys that are not in the type of that variable. But there are + * situations where it will not: + * + * type X = { a: number } + * const x: X = { a: 1, b: 2 } // error: b is not in X + * const arr: X[] = [{ a: 1, b: 2 }].map((x) => x) // no error + * + * To avoid this, we can use NoExtraKeys; + * + * type X = { a: number } + * type Y = { a: number; b: number } + * type StrictX = NoExtraKeys + * const arr: StrictX[] = [{ a: 1, b: 2 }].map((x) => x) // error + * + * Which produces the following error: + * + * Type '{ a: number; b: number; }' is not comparable to type '{ b?: undefined; }'. + * Types of property 'b' are incompatible. + * Type 'number' is not comparable to type 'undefined'.ts(2352) + * + * The `?` is necessary, otherwise the resulting type is impossible to instantiate. + **/ +type NoExtraKeys = FewerKeys & { + [K in Exclude]?: never +} From e9cc6f8c76f369a9c27cf1c8619b9c929cf79221 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 4 Feb 2022 00:38:14 -0500 Subject: [PATCH 10/25] fucking epic test --- .../__tests__/InstanceCreatePage.spec.tsx | 10 +- .../networking/VpcPage/VpcPage.spec.ts | 92 +++++++++++++++++-- .../VpcPage/modals/firewall-rules.tsx | 79 +++++++--------- app/test-utils.tsx | 39 +++++++- libs/api-mocks/msw/handlers.ts | 31 +++++++ libs/api/index.ts | 2 +- libs/api/{parse.spec.ts => util.spec.ts} | 2 +- libs/api/{parse.ts => util.ts} | 29 ++++++ package.json | 4 +- yarn.lock | 49 +++++----- 10 files changed, 248 insertions(+), 89 deletions(-) rename libs/api/{parse.spec.ts => util.spec.ts} (95%) rename libs/api/{parse.ts => util.ts} (50%) diff --git a/app/pages/__tests__/InstanceCreatePage.spec.tsx b/app/pages/__tests__/InstanceCreatePage.spec.tsx index 3212a33d35..607bde3bda 100644 --- a/app/pages/__tests__/InstanceCreatePage.spec.tsx +++ b/app/pages/__tests__/InstanceCreatePage.spec.tsx @@ -31,9 +31,9 @@ describe('InstanceCreatePage', () => { it('shows specific message for known server error code', async () => { renderAppAt(formUrl) const name = screen.getByRole('textbox', { name: 'Choose a name' }) - userEvent.type(name, instance.name) // already exists in db + await userEvent.type(name, instance.name) // already exists in db - fireEvent.click(submitButton()) + await userEvent.click(submitButton()) await screen.findByText( 'An instance with that name already exists in this project' @@ -60,9 +60,9 @@ describe('InstanceCreatePage', () => { expect(window.location.pathname).not.toEqual(instancesPage) const name = screen.getByRole('textbox', { name: 'Choose a name' }) - userEvent.type(name, 'new-instance') - userEvent.click(screen.getByLabelText(/6 CPUs/)) - userEvent.click(submitButton()) + await userEvent.type(name, 'new-instance') + await userEvent.click(screen.getByLabelText(/6 CPUs/)) + await userEvent.click(submitButton()) // nav to instances list await waitFor(() => expect(window.location.pathname).toEqual(instancesPage)) diff --git a/app/pages/project/networking/VpcPage/VpcPage.spec.ts b/app/pages/project/networking/VpcPage/VpcPage.spec.ts index 012eb6d9bc..eb8568df0c 100644 --- a/app/pages/project/networking/VpcPage/VpcPage.spec.ts +++ b/app/pages/project/networking/VpcPage/VpcPage.spec.ts @@ -1,39 +1,43 @@ import { - fireEvent, renderAppAt, screen, userEvent, waitForElementToBeRemoved, + clickByRole, + typeByRole, } from 'app/test-utils' +import { defaultFirewallRules } from '@oxide/api-mocks' describe('VpcPage', () => { - describe('subnets tab', () => { - it('creating a subnet works', async () => { + describe('subnet', () => { + it('create works', async () => { renderAppAt('/orgs/maze-war/projects/mock-project/vpcs/mock-vpc') screen.getByText('Subnets') // wait for subnet to show up in the table await screen.findByRole('cell', { name: 'mock-subnet' }) - // second one is not there though + // the one we'll be adding is not there expect(screen.queryByRole('cell', { name: 'mock-subnet-2' })).toBeNull() // modal is not already open expect(screen.queryByRole('dialog', { name: 'Create subnet' })).toBeNull() // click button to open modal - fireEvent.click(screen.getByRole('button', { name: 'New subnet' })) + await userEvent.click(screen.getByRole('button', { name: 'New subnet' })) // modal is open screen.getByRole('dialog', { name: 'Create subnet' }) const ipv4 = screen.getByRole('textbox', { name: 'IPv4 block' }) - userEvent.type(ipv4, '1.1.1.2/24') + await userEvent.type(ipv4, '1.1.1.2/24') const name = screen.getByRole('textbox', { name: 'Name' }) - userEvent.type(name, 'mock-subnet-2') + await userEvent.type(name, 'mock-subnet-2') // submit the form - fireEvent.click(screen.getByRole('button', { name: 'Create subnet' })) + await userEvent.click( + screen.getByRole('button', { name: 'Create subnet' }) + ) // wait for modal to close await waitForElementToBeRemoved( @@ -43,8 +47,78 @@ describe('VpcPage', () => { ) // table refetches and now includes second subnet - screen.getByRole('cell', { name: 'mock-subnet' }) + await screen.findByRole('cell', { name: 'mock-subnet' }) await screen.findByRole('cell', { name: 'mock-subnet-2' }) }, 10000) // otherwise it flakes in CI }) + + describe('firewall rule', () => { + it('create works', async () => { + renderAppAt('/orgs/maze-war/projects/mock-project/vpcs/mock-vpc') + await clickByRole('tab', 'Firewall Rules') + + // default rules show up in the table + for (const { name } of defaultFirewallRules) { + await screen.findByRole('cell', { name }) + } + // the one we'll be adding is not there + expect(screen.queryByRole('cell', { name: 'my-new-rule' })).toBeNull() + + // modal is not already open + expect( + screen.queryByRole('dialog', { name: 'Create firewall rule' }) + ).toBeNull() + + // click button to open modal + await clickByRole('button', 'New rule') + + // modal is open + await screen.findByRole('dialog', { name: 'Create firewall rule' }) + + await typeByRole('textbox', 'Name', 'my-new-rule') + + await clickByRole('radio', 'Outgoing') + + // input type="number" becomes spinbutton for some reason + await typeByRole('spinbutton', 'Priority', '5') + + await clickByRole('button', 'Target type') + await clickByRole('option', 'VPC') + await typeByRole('textbox', 'Target name', 'my-target-vpc') + await clickByRole('button', 'Add target') + + // target is added to targets table + screen.getByRole('cell', { name: 'my-target-vpc' }) + + await clickByRole('button', 'Host type') + await clickByRole('option', 'Instance') + await typeByRole('textbox', 'Value', 'my-target-instance') + await clickByRole('button', 'Add host filter') + + // host is added to hosts table + screen.getByRole('cell', { name: 'my-target-instance' }) + + // TODO: test invalid port range once I put an error message in there + await typeByRole('textbox', 'Port filter', '123-456') + await clickByRole('button', 'Add port filter') + + // port range is added to port ranges table + screen.getByRole('cell', { name: '123-456' }) + + await clickByRole('checkbox', 'UDP') + + // submit the form + await clickByRole('button', 'Create rule') + + // wait for modal to close + await waitForElementToBeRemoved( + () => screen.queryByRole('dialog', { name: 'Create firewall rule' }), + // fails in CI without a longer timeout (default 1000). boo + { timeout: 2000 } + ) + + // table refetches and now includes the new rule + await screen.findByRole('cell', { name: 'my-new-rule' }) + }, 15000) + }) }) diff --git a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx index b9f30d9be3..f06668eae9 100644 --- a/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx +++ b/app/pages/project/networking/VpcPage/modals/firewall-rules.tsx @@ -1,7 +1,6 @@ import React from 'react' import { Form, Formik, useFormikContext } from 'formik' import * as Yup from 'yup' -import { pick } from '@oxide/util' import { Button, @@ -18,13 +17,13 @@ import { TextFieldError, TextFieldHint, } from '@oxide/ui' -import type { - VpcFirewallRule, - ErrorResponse, - VpcFirewallRuleUpdate, - VpcFirewallRuleUpdateParams, +import type { VpcFirewallRule, ErrorResponse } from '@oxide/api' +import { + firewallRulesArrToObj, + parsePortRange, + useApiMutation, + useApiQueryClient, } from '@oxide/api' -import { parsePortRange, useApiMutation, useApiQueryClient } from '@oxide/api' import { getServerError } from 'app/util/errors' type FormProps = { @@ -122,7 +121,7 @@ const CommonForm = ({ id, error }: FormProps) => { }} />
- Name + Target name
@@ -311,23 +310,34 @@ const CommonForm = ({ id, error }: FormProps) => { Add port filter
-