diff --git a/app/api/util.spec.ts b/app/api/util.spec.ts index 1459ea45ca..1c3f97fb23 100644 --- a/app/api/util.spec.ts +++ b/app/api/util.spec.ts @@ -23,6 +23,12 @@ describe('parsePortRange', () => { expect(parsePortRange('1-45690')).toEqual([1, 45690]) expect(parsePortRange('5-5')).toEqual([5, 5]) }) + + it('with surrounding whitespace', () => { + expect(parsePortRange('123-456 ')).toEqual([123, 456]) + expect(parsePortRange(' 1-45690')).toEqual([1, 45690]) + expect(parsePortRange(' 5-5 \n')).toEqual([5, 5]) + }) }) describe('rejects', () => { diff --git a/app/api/util.ts b/app/api/util.ts index 5d263c60d8..352455c161 100644 --- a/app/api/util.ts +++ b/app/api/util.ts @@ -45,7 +45,7 @@ type PortRange = [number, number] // 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) + const match = /^([0-9]{1,5})((?:-)[0-9]{1,5})?$/.exec(portRange.trim()) if (!match) return null const p1 = parseInt(match[1], 10) diff --git a/app/forms/firewall-rules-create.tsx b/app/forms/firewall-rules-create.tsx index 43afafdf1e..c31464c19c 100644 --- a/app/forms/firewall-rules-create.tsx +++ b/app/forms/firewall-rules-create.tsx @@ -26,13 +26,15 @@ import { ListboxField } from '~/components/form/fields/ListboxField' import { NameField } from '~/components/form/fields/NameField' import { NumberField } from '~/components/form/fields/NumberField' import { RadioField } from '~/components/form/fields/RadioField' -import { TextField } from '~/components/form/fields/TextField' +import { TextField, TextFieldInner } from '~/components/form/fields/TextField' import { SideModalForm } from '~/components/form/SideModalForm' import { useForm, useVpcSelector } from '~/hooks' import { Badge } from '~/ui/lib/Badge' import { Button } from '~/ui/lib/Button' import { FormDivider } from '~/ui/lib/Divider' +import { Message } from '~/ui/lib/Message' import * as MiniTable from '~/ui/lib/MiniTable' +import { TextInputHint } from '~/ui/lib/TextInput' import { KEYS } from '~/ui/util/keys' export type FirewallRuleValues = { @@ -134,15 +136,44 @@ function getFilterValueProps(hostType: VpcFirewallRuleHostFilter['type']) { } } +const DocsLinkMessage = () => ( + + Read the{' '} + + guest networking guide + {' '} + and{' '} + + API docs + {' '} + to learn more about firewall rules. + + } + /> +) + export const CommonFields = ({ error, control }: CommonFieldsProps) => { const portRangeForm = useForm({ defaultValues: portRangeDefaultValues }) const ports = useController({ name: 'ports', control }).field const submitPortRange = portRangeForm.handleSubmit(({ portRange }) => { const portRangeValue = portRange.trim() - // ignore click if invalid or already in the list - // TODO: show error instead of ignoring the click - if (!parsePortRange(portRangeValue)) return - if (ports.value.includes(portRangeValue)) return + // at this point we've already validated in validate() that it parses and + // that it is not already in the list ports.onChange([...ports.value, portRangeValue]) portRangeForm.reset() }) @@ -174,6 +205,7 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { return ( <> + {/* omitting value prop makes it a boolean value. beautiful */} {/* TODO: better text or heading or tip or something on this checkbox */} @@ -182,14 +214,6 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { - - - { { value: 'outbound', label: 'Outbound' }, ]} /> + {/* Really this should be its own
, but you can't have a form inside a form, so we just stick the submit handler in a button onClick */} -

Target

+

Targets

+ {/* TODO: make ListboxField smarter with the values like RadioField is */} { submitTarget(e) } }} + // TODO: validate here, but it's complicated because it's conditional + // on which type is selected />
@@ -301,110 +337,40 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { -

Host filters

- 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. */} - { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitHost(e) - } - }} - /> - -
- - + {/* We have to blow this up instead of using TextField to get better + text styling on the label */} +
+ + + A single port (1234) or a range (1234–2345) + + { + if (e.key === KEYS.enter) { + e.preventDefault() // prevent full form submission + submitPortRange(e) + } + }} + validate={(value) => { + if (!parsePortRange(value)) return 'Not a valid port range' + if (ports.value.includes(value.trim())) return 'Port range already added' + }} + />
-
- - {!!hosts.value.length && ( - - - Type - Value - {/* For remove button */} - - - - {hosts.value.map((h, index) => ( - - - {h.type} - - {h.value} - - - - - ))} - - - )} - - - -
- { - if (e.key === KEYS.enter) { - e.preventDefault() // prevent full form submission - submitPortRange(e) - } - }} - />
{!!ports.value.length && ( - + - Range + Port ranges {/* For remove button */} @@ -445,10 +411,8 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => { )} - -
- Protocols + Protocol filters
TCP @@ -466,6 +430,96 @@ export const CommonFields = ({ error, control }: CommonFieldsProps) => {
+
+

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. */} + { + if (e.key === KEYS.enter) { + e.preventDefault() // prevent full form submission + submitHost(e) + } + }} + // TODO: validate here, but it's complicated because it's conditional + // on which type is selected + /> + +
+ + +
+ + {!!hosts.value.length && ( + + + Type + Value + {/* For remove button */} + + + + {hosts.value.map((h, index) => ( + + + {h.type} + + {h.value} + + + + + ))} + + + )} +
+ {error && ( <> diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index ae376b7465..70cc557f88 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -31,7 +31,7 @@ test('can create firewall rule', async ({ page }) => { await expect(modal).toBeVisible() await page.fill('input[name=name]', 'my-new-rule') - await page.locator('text=Outbound').click() + await page.getByRole('radio', { name: 'Outbound' }).click() await page.fill('role=textbox[name="Priority"]', '5') @@ -54,19 +54,33 @@ test('can create firewall rule', async ({ page }) => { const hosts = page.getByRole('table', { name: 'Host filters' }) await expectRowVisible(hosts, { Type: 'instance', Value: 'host-filter-instance' }) - // TODO: test invalid port range once I put an error message in there - await page.fill('role=textbox[name="Port filter"]', '123-456') - await page.getByRole('button', { name: 'Add port filter' }).click() + const portRangeField = page.getByRole('textbox', { name: 'Port filters' }) + const invalidPort = page.getByRole('dialog').getByText('Not a valid port range') + const addPortButton = page.getByRole('button', { name: 'Add port filter' }) + await portRangeField.fill('abc') + await expect(invalidPort).toBeHidden() + await addPortButton.click() + await expect(invalidPort).toBeVisible() + + await portRangeField.fill('123-456') + await addPortButton.click() + await expect(invalidPort).toBeHidden() // port range is added to port ranges table - const ports = page.getByRole('table', { name: 'Ports' }) - await expectRowVisible(ports, { Range: '123-456' }) + const ports = page.getByRole('table', { name: 'Port filters' }) + await expectRowVisible(ports, { 'Port ranges': '123-456' }) + + const dupePort = page.getByRole('dialog').getByText('Port range already added') + await expect(dupePort).toBeHidden() + await portRangeField.fill('123-456') + // don't need to click because we're already validating onChange + await expect(dupePort).toBeVisible() // check the UDP box await page.locator('text=UDP').click() // submit the form - await page.locator('text="Add rule"').click() + await page.getByRole('button', { name: 'Add rule' }).click() // modal closes again await expect(modal).toBeHidden()