diff --git a/app/components/ProtocolBadge.tsx b/app/components/ProtocolBadge.tsx index 6ddbfa726..9667b99d1 100644 --- a/app/components/ProtocolBadge.tsx +++ b/app/components/ProtocolBadge.tsx @@ -9,6 +9,7 @@ import { Badge } from '@oxide/design-system/ui' import type { VpcFirewallRuleProtocol } from '~/api' +import { getIcmpLabel } from '~/util/protocol' type ProtocolBadgeProps = { protocol: VpcFirewallRuleProtocol @@ -19,14 +20,15 @@ export const ProtocolBadge = ({ protocol }: ProtocolBadgeProps) => { return {protocol.type.toUpperCase()} } + const label = getIcmpLabel(protocol.type) + if (protocol.value === null) { - // All ICMP types - return ICMP + return {label} } return (
- ICMP + {label} type {protocol.value.icmpType} diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 56340d04f..f761fc221 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -8,6 +8,7 @@ import { useEffect, type ReactNode } from 'react' import { useController, useForm, type Control } from 'react-hook-form' +import { match } from 'ts-pattern' import { Badge } from '@oxide/design-system/ui' @@ -52,7 +53,13 @@ import { KEYS } from '~/ui/util/keys' import { ALL_ISH } from '~/util/consts' import { validateIp, validateIpNet } from '~/util/ip' import { links } from '~/util/links' -import { getProtocolDisplayName, getProtocolKey, ICMP_TYPES } from '~/util/protocol' +import { + getIcmpLabel, + getProtocolDisplayName, + getProtocolKey, + ICMPV4_TYPES, + ICMPV6_TYPES, +} from '~/util/protocol' import { capitalize, normalizeDashes } from '~/util/str' import { type FirewallRuleValues } from './firewall-rules-util' @@ -293,18 +300,22 @@ const protocolTypeItems: Array<{ value: VpcFirewallRuleProtocol['type']; label: [ { value: 'tcp', label: 'TCP' }, { value: 'udp', label: 'UDP' }, - { value: 'icmp', label: 'ICMP' }, + { value: 'icmp', label: 'ICMPv4' }, + { value: 'icmp6', label: 'ICMPv6' }, ] -const icmpTypeItems = [ +const buildIcmpTypeItems = (types: Record) => [ { value: '', label: 'All types', selectedLabel: 'All types' }, - ...Object.entries(ICMP_TYPES).map(([type, name]) => ({ + ...Object.entries(types).map(([type, name]) => ({ value: type, label: `${type} - ${name}`, selectedLabel: type, })), ] +const icmpV4TypeItems = buildIcmpTypeItems(ICMPV4_TYPES) +const icmpV6TypeItems = buildIcmpTypeItems(ICMPV6_TYPES) + const targetAndHostTableColumns = [ { header: 'Type', @@ -343,13 +354,13 @@ const isDuplicateProtocol = ( return existingProtocols.some((p) => p.type === newProtocol.type) } - if (newProtocol.type === 'icmp') { + if (newProtocol.type === 'icmp' || newProtocol.type === 'icmp6') { if (newProtocol.value === null) { - return existingProtocols.some((p) => p.type === 'icmp' && p.value === null) + return existingProtocols.some((p) => p.type === newProtocol.type && p.value === null) } return existingProtocols.some( (p) => - p.type === 'icmp' && + p.type === newProtocol.type && p.value?.icmpType === newProtocol.value?.icmpType && p.value?.code === newProtocol.value?.code ) @@ -423,7 +434,7 @@ const ProtocolFilters = ({ control }: { control: Control }) const submitProtocol = protocolForm.handleSubmit((values) => { if (values.protocolType === 'tcp' || values.protocolType === 'udp') { addProtocolIfUnique({ type: values.protocolType }) - } else if (values.protocolType === 'icmp') { + } else if (values.protocolType === 'icmp' || values.protocolType === 'icmp6') { // this parse should never fail because we've already validated, but doing // it this way keeps the just-in-case early return logic consistent const parseResult = parseIcmpType(values.icmpType) @@ -432,14 +443,14 @@ const ProtocolFilters = ({ control }: { control: Control }) const icmpType = parseResult.data if (icmpType === undefined) { // All ICMP types - addProtocolIfUnique({ type: 'icmp', value: null }) + addProtocolIfUnique({ type: values.protocolType, value: null }) } else { // Specific ICMP type const icmpValue: VpcFirewallIcmpFilter = { icmpType } if (values.icmpCode) { icmpValue.code = values.icmpCode } - addProtocolIfUnique({ type: 'icmp', value: icmpValue }) + addProtocolIfUnique({ type: values.protocolType, value: icmpValue }) } } protocolForm.reset() @@ -461,19 +472,28 @@ const ProtocolFilters = ({ control }: { control: Control }) control={protocolForm.control} placeholder="" items={protocolTypeItems} + // ICMPv4 and ICMPv6 type numbers mean different things, so clear the + // selected ICMP type/code when switching protocol + onChange={() => { + protocolForm.setValue('icmpType', '') + protocolForm.setValue('icmpCode', '') + }} /> - {selectedProtocolType === 'icmp' && ( + {(selectedProtocolType === 'icmp' || selectedProtocolType === 'icmp6') && ( <> protocolForm.setValue('icmpType', value)} - items={icmpTypeItems} + items={match(selectedProtocolType) + .with('icmp', () => icmpV4TypeItems) + .with('icmp6', () => icmpV6TypeItems) + .exhaustive()} validate={(value) => { const result = parseIcmpType(value) if (!result.success) return result.message @@ -482,7 +502,7 @@ const ProtocolFilters = ({ control }: { control: Control }) {selectedIcmpType !== undefined && selectedIcmpType !== '' && ( + match(protocol.type) + .with('tcp', () => 'TCP') + .with('udp', () => 'UDP') + .with('icmp', () => 'ICMPv4') + .with('icmp6', () => 'ICMPv6') + .exhaustive() + +const isIcmp = ( + protocol: VpcFirewallRuleProtocol +): protocol is Extract => + protocol.type === 'icmp' || protocol.type === 'icmp6' + export const ProtocolCell = ({ protocol }: { protocol: VpcFirewallRuleProtocol }) => ( - {protocol.type.toUpperCase()} + {protocolLabel(protocol)} ) /** Generate tooltip content for empty protocol cells in the mini table */ const protocolEmptyCellTooltipContent = (protocol: VpcFirewallRuleProtocol): string => { if (protocol.type === 'tcp') return 'This firewall rule will match all TCP traffic' if (protocol.type === 'udp') return 'This firewall rule will match all UDP traffic' + const label = protocolLabel(protocol) // in this case, the user could be looking at the type column or the code column, but both get the same tooltip if (protocol.value === null) { - return 'This firewall rule will match all ICMP traffic' + return `This firewall rule will match all ${label} traffic` } // in this case, there's an icmpType but no code, which means the user is looking at the code column - return `This firewall rule will match all ICMP traffic of type ${protocol.value.icmpType}` + return `This firewall rule will match all ${label} traffic of type ${protocol.value.icmpType}` } export const ProtocolEmptyCell = ({ protocol }: { protocol: VpcFirewallRuleProtocol }) => ( @@ -39,7 +55,7 @@ export const ProtocolEmptyCell = ({ protocol }: { protocol: VpcFirewallRuleProto export const ProtocolTypeCell = ({ protocol }: { protocol: VpcFirewallRuleProtocol }) => // icmpType could be zero, so we check for `not undefined` - protocol.type === 'icmp' && protocol.value?.icmpType !== undefined ? ( + isIcmp(protocol) && protocol.value?.icmpType !== undefined ? ( protocol.value.icmpType ) : ( @@ -47,7 +63,7 @@ export const ProtocolTypeCell = ({ protocol }: { protocol: VpcFirewallRuleProtoc export const ProtocolCodeCell = ({ protocol }: { protocol: VpcFirewallRuleProtocol }) => // code could be zero, so we check for `not undefined` - protocol.type === 'icmp' && protocol.value?.code !== undefined ? ( + isIcmp(protocol) && protocol.value?.code !== undefined ? ( protocol.value.code ) : ( diff --git a/app/util/protocol.ts b/app/util/protocol.ts index a41ec3e72..da479b0a1 100644 --- a/app/util/protocol.ts +++ b/app/util/protocol.ts @@ -6,9 +6,13 @@ * Copyright Oxide Computer Company */ +import { match } from 'ts-pattern' + import type { VpcFirewallRuleProtocol } from '~/api' -export const ICMP_TYPES: Record = { +// IANA ICMP Type Numbers registry: +// https://www.iana.org/assignments/icmp-parameters/icmp-parameters.xhtml#icmp-parameters-types +export const ICMPV4_TYPES: Record = { 0: 'Echo Reply', 3: 'Destination Unreachable', 5: 'Redirect Message', @@ -21,26 +25,60 @@ export const ICMP_TYPES: Record = { 14: 'Timestamp Reply', } +// IANA ICMPv6 Type Numbers registry: +// https://www.iana.org/assignments/icmpv6-parameters/icmpv6-parameters.xhtml#icmpv6-parameters-2 +export const ICMPV6_TYPES: Record = { + 1: 'Destination Unreachable', + 2: 'Packet Too Big', + 3: 'Time Exceeded', + 4: 'Parameter Problem', + 128: 'Echo Request', + 129: 'Echo Reply', + 130: 'Multicast Listener Query', + 131: 'Multicast Listener Report', + 132: 'Multicast Listener Done', + 133: 'Router Solicitation', + 134: 'Router Advertisement', + 135: 'Neighbor Solicitation', + 136: 'Neighbor Advertisement', + 137: 'Redirect Message', +} + +export type IcmpVariant = 'icmp' | 'icmp6' + +const typesFor = (variant: IcmpVariant) => + match(variant) + .with('icmp', () => ICMPV4_TYPES) + .with('icmp6', () => ICMPV6_TYPES) + .exhaustive() + +export const getIcmpLabel = (variant: IcmpVariant) => + match(variant) + .with('icmp', () => 'ICMPv4' as const) + .with('icmp6', () => 'ICMPv6' as const) + .exhaustive() + /** * Get the human-readable name for an ICMP type */ -export const getIcmpTypeName = (type: number): string | undefined => ICMP_TYPES[type] +export const getIcmpTypeName = (variant: IcmpVariant, type: number): string | undefined => + typesFor(variant)[type] /** * Get a display name for a protocol, including ICMP types and codes */ export const getProtocolDisplayName = (protocol: VpcFirewallRuleProtocol): string => { - if (protocol.type === 'icmp') { - if (protocol.value === null) { - return 'ICMP (All types)' - } else { - const typeName = - ICMP_TYPES[protocol.value.icmpType] || `Type ${protocol.value.icmpType}` - const codePart = protocol.value.code ? ` | Code ${protocol.value.code}` : '' - return `ICMP ${protocol.value.icmpType} - ${typeName}${codePart}` - } + if (protocol.type === 'tcp' || protocol.type === 'udp') { + return protocol.type.toUpperCase() + } + const label = getIcmpLabel(protocol.type) + if (protocol.value === null) { + return `${label} (All types)` } - return protocol.type.toUpperCase() + const typeName = + typesFor(protocol.type)[protocol.value.icmpType] || `Type ${protocol.value.icmpType}` + const codePart = protocol.value.code ? ` | Code ${protocol.value.code}` : '' + return `${label} ${protocol.value.icmpType} - ${typeName}${codePart}` } /** @@ -52,6 +90,6 @@ export const getProtocolKey = (protocol: VpcFirewallRuleProtocol): string => { return protocol.type } return protocol.value === null - ? 'icmp|all' - : `icmp|${protocol.value.icmpType}|${protocol.value.code || 'all'}` + ? `${protocol.type}|all` + : `${protocol.type}|${protocol.value.icmpType}|${protocol.value.code || 'all'}` } diff --git a/mock-api/vpc.ts b/mock-api/vpc.ts index 80861df50..68a3fc65f 100644 --- a/mock-api/vpc.ts +++ b/mock-api/vpc.ts @@ -321,7 +321,11 @@ export const firewallRules: Json = [ description: 'we just want to test with lots of filters', filters: { ports: ['3389', '45-89'], - protocols: [{ type: 'tcp' }, { type: 'icmp', value: { icmp_type: 5, code: '1-3' } }], + protocols: [ + { type: 'tcp' }, + { type: 'icmp', value: { icmp_type: 5, code: '1-3' } }, + { type: 'icmp6', value: { icmp_type: 128 } }, + ], hosts: [ { type: 'instance', value: 'hello-friend' }, { type: 'subnet', value: 'my-subnet' }, diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index c073940ef..052dbf604 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -141,15 +141,15 @@ test('firewall rule targets and filters overflow', async ({ page }) => { ).toBeVisible() await expect( - page.getByRole('cell', { name: 'instance hello-friend +6', exact: true }) + page.getByRole('cell', { name: 'instance hello-friend +7', exact: true }) ).toBeVisible() // scroll table sideways past the filters cell await page.getByText('Enabled').first().scrollIntoViewIfNeeded() - await page.getByText('+6').hover() + await page.getByText('+7').hover() const tooltip = page.getByRole('tooltip', { - name: 'Other filters subnet my-subnet ip 148.38.89.5 TCP ICMP type 5 code 1-3 Port 3389 Port 45-89', + name: 'Other filters subnet my-subnet ip 148.38.89.5 TCP ICMPv4 type 5 code 1-3 ICMPv6 type 128 Port 3389 Port 45-89', exact: true, }) await expect(tooltip).toBeVisible() @@ -434,16 +434,16 @@ test('can update firewall rule', async ({ page }) => { // protocol is populated in the table const protocolTable = page.getByRole('table', { name: 'Protocol filters' }) - await expect(protocolTable.getByText('ICMP')).toBeVisible() + await expect(protocolTable.getByText('ICMPv4')).toBeVisible() // remove the existing ICMP protocol filter await protocolTable.getByRole('button', { name: 'remove' }).click() // add a new ICMP protocol filter with type 3 and code 0 - await selectOption(page, 'Protocol filters', 'ICMP') - const icmpTypeField = page.getByRole('combobox', { name: 'ICMP Type' }) + await selectOption(page, 'Protocol filters', 'ICMPv4') + const icmpTypeField = page.getByRole('combobox', { name: 'ICMPv4 type' }) await fillAndSelect(icmpTypeField, page, '3', '3 - Destination Unreachable') - await page.getByRole('textbox', { name: 'ICMP Code' }).fill('0') + await page.getByRole('textbox', { name: 'ICMPv4 code' }).fill('0') await page.getByRole('button', { name: 'Add protocol' }).click() // update name @@ -477,7 +477,7 @@ test('can update firewall rule', async ({ page }) => { await expect(rows).toHaveCount(3) // new host filter shows up in filters cell, along with the new ICMP protocol - await expect(page.locator('text=subnetedit-filter-subnetICMP')).toBeVisible() + await expect(page.locator('text=subnetedit-filter-subnetICMPv4')).toBeVisible() // scroll table sideways past the filters cell to see the full content await page.getByText('Enabled').first().scrollIntoViewIfNeeded() @@ -509,7 +509,7 @@ test('create from existing rule', async ({ page }) => { // protocol is populated in the table const protocolTable = modal.getByRole('table', { name: 'Protocol filters' }) - await expect(protocolTable.getByText('ICMP')).toBeVisible() + await expect(protocolTable.getByText('ICMPv4')).toBeVisible() await expect(protocolTable.getByText('TCP')).toBeHidden() await expect(protocolTable.getByText('UDP')).toBeHidden() @@ -537,7 +537,7 @@ test('create from existing rule', async ({ page }) => { // protocol is populated in the table await expect(protocolTable.getByText('TCP')).toBeVisible() await expect(protocolTable.getByText('UDP')).toBeHidden() - await expect(protocolTable.getByText('ICMP')).toBeHidden() + await expect(protocolTable.getByText('ICMPv4')).toBeHidden() await expect(targets.getByRole('row', { name: 'vpc default' })).toBeVisible() }) @@ -657,8 +657,8 @@ test('arbitrary values combobox', async ({ page }) => { // triggering a validation error for bad input. without onInputChange binding // the input value to the form value, this does not trigger an error because // the form thinks the input is empyt. - await selectOption(page, 'Protocol filters', 'ICMP') - await page.getByRole('combobox', { name: 'ICMP type' }).pressSequentially('abc') + await selectOption(page, 'Protocol filters', 'ICMPv4') + await page.getByRole('combobox', { name: 'ICMPv4 type' }).pressSequentially('abc') const error = page .getByRole('dialog') .getByText('ICMP type must be a number between 0 and 255') @@ -667,6 +667,47 @@ test('arbitrary values combobox', async ({ page }) => { await expect(error).toBeVisible() }) +test('can add ICMPv4 and ICMPv6 protocol filters', async ({ page }) => { + await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new') + + const protocolTable = page.getByRole('table', { name: 'Protocol filters' }) + await expect(protocolTable).toBeHidden() + + // add an ICMPv4 filter with a specific type + const protocolListbox = page.getByRole('button', { name: 'Protocol filters' }) + await protocolListbox.click() + await page.getByRole('option', { name: 'ICMPv4', exact: true }).click() + await fillAndSelect( + page.getByRole('combobox', { name: 'ICMPv4 type' }), + page, + '8', + '8 - Echo Request' + ) + await page.getByRole('button', { name: 'Add protocol filter' }).click() + await expectRowVisible(protocolTable, { Protocol: 'ICMPv4', Type: '8' }) + + // add an ICMPv6 filter with a different type number; v4 type 8 is Echo + // Request, v6 type 128 is Echo Request — different numbers, same intent + await protocolListbox.click() + await page.getByRole('option', { name: 'ICMPv6', exact: true }).click() + await fillAndSelect( + page.getByRole('combobox', { name: 'ICMPv6 type' }), + page, + '128', + '128 - Echo Request' + ) + await page.getByRole('button', { name: 'Add protocol filter' }).click() + await expectRowVisible(protocolTable, { Protocol: 'ICMPv6', Type: '128' }) + + // both rows are present + await expect(protocolTable.getByRole('row')).toHaveCount(3) // header + 2 + + // switching protocol type clears the previously selected ICMP type + await protocolListbox.click() + await page.getByRole('option', { name: 'ICMPv4', exact: true }).click() + await expect(page.getByRole('combobox', { name: 'ICMPv4 type' })).toHaveValue('') +}) + // Regression test: when typing a partial match in an arbitrary-values combobox, // arrowing to a dropdown option, and pressing Enter, the selected option's value // should end up in the field — not the raw typed query.