ports, hosts, and targets subforms actually work

Author: david-crespo

app/pages/project/networking/VpcPage/modals/firewall-rules.tsx

@@ -18,24 +18,45 @@ 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 = {
   error: ErrorResponse | null
   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<VpcFirewallRule['filters']['protocols']>
+
+  // port subform
+  ports: NonNullable<VpcFirewallRule['filters']['ports']>
+  portRange: string
+
+  // host subform
+  hosts: NonNullable<VpcFirewallRule['filters']['hosts']>
+  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<Values>()
   return (
     <Form id={id}>
       <SideModal.Section>
@@ -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)
           }}
         />
         <div className="space-y-0.5">
-          <FieldTitle htmlFor="targetName">Name</FieldTitle>
-          <TextField id="targetName" name="targetName" />
+          <FieldTitle htmlFor="targetValue">Name</FieldTitle>
+          <TextField id="targetValue" name="targetValue" />
         </div>
 
         <div className="flex justify-end">
+          {/* TODO does this clear out the form or the existing targets? */}
           <Button variant="ghost" className="mr-2.5">
             Clear
           </Button>
           <Button
             variant="dim"
             onClick={() => {
-              if (!targets.some((t) => t.name === targetName)) {
+              // TODO: show error instead of ignoring click
+              if (
+                values.targetType &&
+                values.targetValue && // TODO: validate
+                !values.targets.some(
+                  (t) =>
+                    t.value === values.targetValue &&
+                    t.type === values.targetType
+                )
+              ) {
                 setFieldValue('targets', [
-                  ...targets,
-                  { type: targetType, name: targetName },
+                  ...values.targets,
+                  { type: values.targetType, value: values.targetValue },
                 ])
+                setFieldValue('targetValue', '')
+                // TODO: clear dropdown too?
               }
             }}
           >
@@ -113,17 +145,20 @@ const CommonForm = ({ id, error }: FormProps) => {
             </Table.HeaderRow>
           </Table.Header>
           <Table.Body>
-            {targets.map((t) => (
-              <Table.Row key={t.name}>
+            {values.targets.map((t) => (
+              <Table.Row key={`${t.type}|${t.value}`}>
+                {/* TODO: should be the pretty type label, not the type key */}
                 <Table.Cell>{t.type}</Table.Cell>
-                <Table.Cell>{t.name}</Table.Cell>
+                <Table.Cell>{t.value}</Table.Cell>
                 <Table.Cell>
                   <Delete10Icon
                     className="cursor-pointer"
                     onClick={() => {
                       setFieldValue(
                         'targets',
-                        targets.filter((t1) => t1.name !== t.name)
+                        values.targets.filter(
+                          (t1) => t1.value !== t.value || t1.type !== t.type
+                        )
                       )
                     }}
                   />
@@ -144,28 +179,53 @@ const CommonForm = ({ id, error }: FormProps) => {
             { value: 'ip', label: 'IP' },
             { value: 'internet_gateway', label: 'Internet Gateway' },
           ]}
+          onChange={(item) => {
+            setFieldValue('hostType', item?.value)
+          }}
         />
         <div className="space-y-0.5">
           {/* 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. */}
-          <FieldTitle htmlFor="host-filter-value">Value</FieldTitle>
-          <TextFieldHint id="host-filter-value-hint">
+          <FieldTitle htmlFor="hostValue">Value</FieldTitle>
+          <TextFieldHint id="hostValue-hint">
             For IP, an address. For the rest, a name. [TODO: copy]
           </TextFieldHint>
           <TextField
-            id="host-filter-value"
-            name="host-filter-value"
-            aria-describedby="host-filter-value-hint"
+            id="hostValue"
+            name="hostValue"
+            aria-describedby="hostValue-hint"
           />
         </div>
 
         <div className="flex justify-end">
           <Button variant="ghost" className="mr-2.5">
             Clear
           </Button>
-          <Button variant="dim">Add host filter</Button>
+          <Button
+            variant="dim"
+            onClick={() => {
+              // TODO: show error instead of ignoring click
+              if (
+                values.hostType &&
+                values.hostValue && // TODO: validate
+                !values.hosts.some(
+                  (t) =>
+                    t.value === values.hostValue || t.type === values.hostType
+                )
+              ) {
+                setFieldValue('hosts', [
+                  ...values.hosts,
+                  { type: values.hostType, value: values.hostValue },
+                ])
+                setFieldValue('hostValue', '')
+                // TODO: clear dropdown too?
+              }
+            }}
+          >
+            Add host filter
+          </Button>
         </div>
 
         <Table className="w-full">
@@ -177,13 +237,26 @@ const CommonForm = ({ id, error }: FormProps) => {
             </Table.HeaderRow>
           </Table.Header>
           <Table.Body>
-            <Table.Row>
-              <Table.Cell>VPC</Table.Cell>
-              <Table.Cell>my-vpc</Table.Cell>
-              <Table.Cell>
-                <Delete10Icon className="cursor-pointer" />
-              </Table.Cell>
-            </Table.Row>
+            {values.hosts.map((h) => (
+              <Table.Row key={`${h.type}|${h.value}`}>
+                {/* TODO: should be the pretty type label, not the type key */}
+                <Table.Cell>{h.type}</Table.Cell>
+                <Table.Cell>{h.value}</Table.Cell>
+                <Table.Cell>
+                  <Delete10Icon
+                    className="cursor-pointer"
+                    onClick={() => {
+                      setFieldValue(
+                        'hosts',
+                        values.hosts.filter(
+                          (h1) => h1.value !== h.value && h1.type !== h.type
+                        )
+                      )
+                    }}
+                  />
+                </Table.Cell>
+              </Table.Row>
+            ))}
           </Table.Body>
         </Table>
       </SideModal.Section>
@@ -204,21 +277,34 @@ const CommonForm = ({ id, error }: FormProps) => {
             <Button variant="ghost" className="mr-2.5">
               Clear
             </Button>
-            <Button variant="dim">Add port filter</Button>
+            <Button
+              variant="dim"
+              onClick={() => {
+                const portRange = values.portRange.trim()
+                // TODO: show error instead of ignoring the click
+                if (!parsePortRange(portRange)) return
+                setFieldValue('ports', [...values.ports, portRange])
+                setFieldValue('portRange', '')
+              }}
+            >
+              Add port filter
+            </Button>
           </div>
           <ul>
-            <li>
-              1234
-              <Delete10Icon className="cursor-pointer ml-2" />
-            </li>
-            <li>
-              456-567
-              <Delete10Icon className="cursor-pointer ml-2" />
-            </li>
-            <li>
-              8080-8086
-              <Delete10Icon className="cursor-pointer ml-2" />
-            </li>
+            {values.ports.map((p) => (
+              <li key={p}>
+                {p}
+                <Delete10Icon
+                  className="cursor-pointer ml-2"
+                  onClick={() => {
+                    setFieldValue(
+                      'ports',
+                      values.ports.filter((p1) => p1 !== p)
+                    )
+                  }}
+                />
+              </li>
+            ))}
           </ul>
         </div>
       </SideModal.Section>
@@ -308,25 +394,34 @@ export function CreateFirewallRuleModal({
       onDismiss={dismiss}
     >
       <Formik
-        initialValues={{
-          enabled: false,
-          priority: '',
-          name: '',
-          description: '',
-          action: 'allow',
-          direction: 'inbound',
-          // TODO: in the request body, these go in a `filters` object. we probably don't
-          // need such nesting here though. not even sure how to do it
-          // filters
-          protocols: [],
-          ports: [],
-          hosts: [],
+        initialValues={
+          {
+            enabled: false,
+            priority: '',
+            name: '',
+            description: '',
+            action: 'allow',
+            direction: 'inbound',
 
-          // target subform
-          targets: [],
-          targetType: '',
-          targetName: '',
-        }}
+            // in the request body, these go in a `filters` object. we probably don't
+            // need such nesting here though. not even sure how to do it
+            protocols: [],
+
+            // port subform
+            ports: [],
+            portRange: '',
+
+            // host subform
+            hosts: [],
+            hostType: '',
+            hostValue: '',
+
+            // target subform
+            targets: [],
+            targetType: '',
+            targetValue: '',
+          } as Values // best way to tell formik this type
+        }
         validationSchema={Yup.object({
           priority: Yup.number()
             .integer()

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(false)
+  const [createModalOpen, setCreateModalOpen] = useState(true)
   const [editing, setEditing] = useState<VpcFirewallRule | null>(null)
 
   const actions = (rule: VpcFirewallRule): MenuAction[] => [

libs/api/index.ts

@@ -26,6 +26,7 @@ export const useApiQuery = getUseApiQuery(api.methods)
 export const useApiMutation = getUseApiMutation(api.methods)
 export const useApiQueryClient = getUseApiQueryClient<A>()
 
+export * from './parse'
 export * from './__generated__/Api'
 
 // for convenience so we can do `import type { ApiTypes } from '@oxide/api'`

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)
+    })
+  })
+})

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]
+}