From ed1f486791d4081a13f01be682e51ecce717f980 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 20 Sep 2024 00:04:39 -0500 Subject: [PATCH 01/20] move ip utils to their own file --- app/forms/ip-pool-range-add.tsx | 2 +- app/util/ip.spec.ts | 79 +++++++++++++++++++++++++++++++++ app/util/ip.ts | 23 ++++++++++ app/util/str.spec.ts | 68 +--------------------------- app/util/str.ts | 16 ------- mock-api/msw/handlers.ts | 3 +- mock-api/msw/util.ts | 2 +- 7 files changed, 108 insertions(+), 85 deletions(-) create mode 100644 app/util/ip.spec.ts create mode 100644 app/util/ip.ts diff --git a/app/forms/ip-pool-range-add.tsx b/app/forms/ip-pool-range-add.tsx index 96fa8a1036..ada846eb85 100644 --- a/app/forms/ip-pool-range-add.tsx +++ b/app/forms/ip-pool-range-add.tsx @@ -15,8 +15,8 @@ import { SideModalForm } from '~/components/form/SideModalForm' import { useIpPoolSelector } from '~/hooks/use-params' import { addToast } from '~/stores/toast' import { Message } from '~/ui/lib/Message' +import { validateIp } from '~/util/ip' import { pb } from '~/util/path-builder' -import { validateIp } from '~/util/str' const defaultValues: IpRange = { first: '', diff --git a/app/util/ip.spec.ts b/app/util/ip.spec.ts new file mode 100644 index 0000000000..cd4ec440dd --- /dev/null +++ b/app/util/ip.spec.ts @@ -0,0 +1,79 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * Copyright Oxide Computer Company + */ + +import { expect, test } from 'vitest' + +import { validateIp } from './ip' + +// Small Rust project where we validate that the built-in Ipv4Addr and Ipv6Addr +// and oxnet's Ipv4Net and Ipv6Net have the same validation behavior as our code. +// https://github.com/oxidecomputer/test-ip-validation + +const v4 = ['123.4.56.7', '1.2.3.4'] + +test.each(v4)('validateIp catches valid IPV4 / invalid IPV6: %s', (s) => { + expect(validateIp(s)).toStrictEqual({ isv4: true, isv6: false, valid: true }) +}) + +const v6 = [ + '2001:db8:3333:4444:5555:6666:7777:8888', + '2001:db8:3333:4444:CCCC:DDDD:EEEE:FFFF', + '::', + '2001:db8::', + '::1234:5678', + '2001:db8::1234:5678', + '2001:0db8:85a3:0000:0000:8a2e:0370:7334', + '::ffff:192.0.2.128', + '1:2:3:4:5:6:7:8', + '::ffff:10.0.0.1', + '::ffff:1.2.3.4', + '::ffff:0.0.0.0', + '1:2:3:4:5:6:77:88', + '::ffff:255.255.255.255', + 'fe08::7:8', + 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff', +] + +test.each(v6)('validateIp catches invalid IPV4 / valid IPV6: %s', (s) => { + expect(validateIp(s)).toStrictEqual({ isv4: false, isv6: true, valid: true }) +}) + +const invalid = [ + '', + '1', + 'abc', + 'a.b.c.d', + // some implementations (I think incorrectly) allow leading zeros but nexus does not + '01.102.103.104', + '127.0.0', + '127.0.0.1.', + '127.0.0.1 ', + ' 127.0.0.1', + '10002.3.4', + '1.2.3.4.5', + '256.0.0.0', + '260.0.0.0', + '256.1.1.1', + '2001:0db8:85a3:0000:0000:8a2e:0370:7334 ', + ' 2001:db8::', + '1:2:3:4:5:6:7:8:9', + '1:2:3:4:5:6::7:8', + ':1:2:3:4:5:6:7:8', + '1:2:3:4:5:6:7:8:', + '::1:2:3:4:5:6:7:8', + '1:2:3:4:5:6:7:8::', + '1:2:3:4:5:6:7:88888', + '2001:db8:3:4:5::192.0.2.33', // std::new::Ipv6Net allows this one + 'fe08::7:8%', + 'fe08::7:8i', + 'fe08::7:8interface', +] + +test.each(invalid)('validateIp catches invalid IP: %s', (s) => { + expect(validateIp(s)).toStrictEqual({ isv4: false, isv6: false, valid: false }) +}) diff --git a/app/util/ip.ts b/app/util/ip.ts new file mode 100644 index 0000000000..497822b730 --- /dev/null +++ b/app/util/ip.ts @@ -0,0 +1,23 @@ +/* + * This Source Code Form is subject to the terms of the Mozilla Public + * License, v. 2.0. If a copy of the MPL was not distributed with this + * file, you can obtain one at https://mozilla.org/MPL/2.0/. + * + * Copyright Oxide Computer Company + */ + +// Borrowed from Valibot. I tried some from Zod and an O'Reilly regex cookbook +// but they didn't match results with std::net on simple test cases +// https://github.com/fabian-hiller/valibot/blob/2554aea5/library/src/regex.ts#L43-L54 + +const IPV4_REGEX = + /^(?:(?:[1-9]|1\d|2[0-4])?\d|25[0-5])(?:\.(?:(?:[1-9]|1\d|2[0-4])?\d|25[0-5])){3}$/u + +const IPV6_REGEX = + /^(?:(?:[\da-f]{1,4}:){7}[\da-f]{1,4}|(?:[\da-f]{1,4}:){1,7}:|(?:[\da-f]{1,4}:){1,6}:[\da-f]{1,4}|(?:[\da-f]{1,4}:){1,5}(?::[\da-f]{1,4}){1,2}|(?:[\da-f]{1,4}:){1,4}(?::[\da-f]{1,4}){1,3}|(?:[\da-f]{1,4}:){1,3}(?::[\da-f]{1,4}){1,4}|(?:[\da-f]{1,4}:){1,2}(?::[\da-f]{1,4}){1,5}|[\da-f]{1,4}:(?::[\da-f]{1,4}){1,6}|:(?:(?::[\da-f]{1,4}){1,7}|:)|fe80:(?::[\da-f]{0,4}){0,4}%[\da-z]+|::(?:f{4}(?::0{1,4})?:)?(?:(?:25[0-5]|(?:2[0-4]|1?\d)?\d)\.){3}(?:25[0-5]|(?:2[0-4]|1?\d)?\d)|(?:[\da-f]{1,4}:){1,4}:(?:(?:25[0-5]|(?:2[0-4]|1?\d)?\d)\.){3}(?:25[0-5]|(?:2[0-4]|1?\d)?\d))$/iu + +export function validateIp(ip: string) { + const isv4 = IPV4_REGEX.test(ip) + const isv6 = !isv4 && IPV6_REGEX.test(ip) + return { isv4, isv6, valid: isv4 || isv6 } +} diff --git a/app/util/str.spec.ts b/app/util/str.spec.ts index 37115bd02e..3a4a8a28e2 100644 --- a/app/util/str.spec.ts +++ b/app/util/str.spec.ts @@ -5,9 +5,9 @@ * * Copyright Oxide Computer Company */ -import { describe, expect, it, test } from 'vitest' +import { describe, expect, it } from 'vitest' -import { camelCase, capitalize, commaSeries, kebabCase, titleCase, validateIp } from './str' +import { camelCase, capitalize, commaSeries, kebabCase, titleCase } from './str' describe('capitalize', () => { it('capitalizes the first letter', () => { @@ -76,67 +76,3 @@ describe('titleCase', () => { expect(titleCase('123 abc')).toBe('123 Abc') }) }) - -// Rust playground comparing results with std::net::{Ipv4Addr, Ipv6Addr} -// https://play.rust-lang.org/?version=stable&mode=debug&edition=2021&gist=493b3345b9f6c0b1c8ee91834e99ef7b - -test.each(['123.4.56.7', '1.2.3.4'])( - 'validateIp catches valid IPV4 / invalid IPV6: %s', - (s) => { - expect(validateIp(s)).toStrictEqual({ isv4: true, isv6: false, valid: true }) - } -) - -test.each([ - '2001:db8:3333:4444:5555:6666:7777:8888', - '2001:db8:3333:4444:CCCC:DDDD:EEEE:FFFF', - '::', - '2001:db8::', - '::1234:5678', - '2001:db8::1234:5678', - '2001:0db8:85a3:0000:0000:8a2e:0370:7334', - '::ffff:192.0.2.128', - '1:2:3:4:5:6:7:8', - '::ffff:10.0.0.1', - '::ffff:1.2.3.4', - '::ffff:0.0.0.0', - '1:2:3:4:5:6:77:88', - '::ffff:255.255.255.255', - 'fe08::7:8', - 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff', -])('validateIp catches invalid IPV4 / valid IPV6: %s', (s) => { - expect(validateIp(s)).toStrictEqual({ isv4: false, isv6: true, valid: true }) -}) - -test.each([ - '', - '1', - 'abc', - 'a.b.c.d', - // some implementations (I think incorrectly) allow leading zeros but nexus does not - '01.102.103.104', - '127.0.0', - '127.0.0.1.', - '127.0.0.1 ', - ' 127.0.0.1', - '10002.3.4', - '1.2.3.4.5', - '256.0.0.0', - '260.0.0.0', - '256.1.1.1', - '2001:0db8:85a3:0000:0000:8a2e:0370:7334 ', - ' 2001:db8::', - '1:2:3:4:5:6:7:8:9', - '1:2:3:4:5:6::7:8', - ':1:2:3:4:5:6:7:8', - '1:2:3:4:5:6:7:8:', - '::1:2:3:4:5:6:7:8', - '1:2:3:4:5:6:7:8::', - '1:2:3:4:5:6:7:88888', - '2001:db8:3:4:5::192.0.2.33', // std::new::Ipv6Net allows this one - 'fe08::7:8%', - 'fe08::7:8i', - 'fe08::7:8interface', -])('validateIp catches invalid IP: %s', (s) => { - expect(validateIp(s)).toStrictEqual({ isv4: false, isv6: false, valid: false }) -}) diff --git a/app/util/str.ts b/app/util/str.ts index 069f8508ee..934530917c 100644 --- a/app/util/str.ts +++ b/app/util/str.ts @@ -50,22 +50,6 @@ export const titleCase = (text: string): string => { ) } -// Borrowed from Valibot. I tried some from Zod and an O'Reilly regex cookbook -// but they didn't match results with std::new on simple test cases -// https://github.com/fabian-hiller/valibot/blob/2554aea5/library/src/regex.ts#L43-L54 - -const IPV4_REGEX = - /^(?:(?:[1-9]|1\d|2[0-4])?\d|25[0-5])(?:\.(?:(?:[1-9]|1\d|2[0-4])?\d|25[0-5])){3}$/u - -const IPV6_REGEX = - /^(?:(?:[\da-f]{1,4}:){7}[\da-f]{1,4}|(?:[\da-f]{1,4}:){1,7}:|(?:[\da-f]{1,4}:){1,6}:[\da-f]{1,4}|(?:[\da-f]{1,4}:){1,5}(?::[\da-f]{1,4}){1,2}|(?:[\da-f]{1,4}:){1,4}(?::[\da-f]{1,4}){1,3}|(?:[\da-f]{1,4}:){1,3}(?::[\da-f]{1,4}){1,4}|(?:[\da-f]{1,4}:){1,2}(?::[\da-f]{1,4}){1,5}|[\da-f]{1,4}:(?::[\da-f]{1,4}){1,6}|:(?:(?::[\da-f]{1,4}){1,7}|:)|fe80:(?::[\da-f]{0,4}){0,4}%[\da-z]+|::(?:f{4}(?::0{1,4})?:)?(?:(?:25[0-5]|(?:2[0-4]|1?\d)?\d)\.){3}(?:25[0-5]|(?:2[0-4]|1?\d)?\d)|(?:[\da-f]{1,4}:){1,4}:(?:(?:25[0-5]|(?:2[0-4]|1?\d)?\d)\.){3}(?:25[0-5]|(?:2[0-4]|1?\d)?\d))$/iu - -export const validateIp = (ip: string) => { - const isv4 = IPV4_REGEX.test(ip) - const isv6 = !isv4 && IPV6_REGEX.test(ip) - return { isv4, isv6, valid: isv4 || isv6 } -} - /** * Does a base64 string represent underlying data that's all zeros, i.e., does * it look like `AAAAAAAAAAAAAAAA==`? diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index d1d346615e..a98a13637c 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -22,7 +22,8 @@ import { import { json, makeHandlers, type Json } from '~/api/__generated__/msw-handlers' import { instanceCan } from '~/api/util' -import { commaSeries, validateIp } from '~/util/str' +import { validateIp } from '~/util/ip' +import { commaSeries } from '~/util/str' import { GiB } from '~/util/units' import { genCumulativeI64Data } from '../metrics' diff --git a/mock-api/msw/util.ts b/mock-api/msw/util.ts index bcc8647127..8829e8836d 100644 --- a/mock-api/msw/util.ts +++ b/mock-api/msw/util.ts @@ -25,7 +25,7 @@ import { } from '@oxide/api' import { json, type Json } from '~/api/__generated__/msw-handlers' -import { validateIp } from '~/util/str' +import { validateIp } from '~/util/ip' import { GiB, TiB } from '~/util/units' import type { DbRoleAssignmentResourceType } from '..' From 72b506f515219c89b995b321ec1ff8cd6093af60 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 20 Sep 2024 15:26:09 -0500 Subject: [PATCH 02/20] validateIpNet util based one existing validateIp --- app/util/ip.spec.ts | 22 +++++++++++++++++++++- app/util/ip.ts | 35 +++++++++++++++++++++++++++++++++++ 2 files changed, 56 insertions(+), 1 deletion(-) diff --git a/app/util/ip.spec.ts b/app/util/ip.spec.ts index cd4ec440dd..45b0f77267 100644 --- a/app/util/ip.spec.ts +++ b/app/util/ip.spec.ts @@ -8,7 +8,7 @@ import { expect, test } from 'vitest' -import { validateIp } from './ip' +import { validateIp, validateIpNet } from './ip' // Small Rust project where we validate that the built-in Ipv4Addr and Ipv6Addr // and oxnet's Ipv4Net and Ipv6Net have the same validation behavior as our code. @@ -77,3 +77,23 @@ const invalid = [ test.each(invalid)('validateIp catches invalid IP: %s', (s) => { expect(validateIp(s)).toStrictEqual({ isv4: false, isv6: false, valid: false }) }) + +test.each([...v4.concat(v6).map((ip) => ip + '/10'), '2001:db8::/128'])('%s', (s) => { + expect(validateIpNet(s).valid).toBe(true) +}) + +test.each([ + ...invalid.map((ip) => ip + '/10'), + 'abc', + '', + '1.1.1.1', + '1.1.1.1/180', + '256.0.0.0/24', + '192.168.0.0/33', + '192.168.0.0/-1', + '192.168.0.0.0/24', + '192.168.0/24', + '2001:db8::/129', +])('validateIpNet catches invalid value: %s', (s) => { + expect(validateIpNet(s).valid).toBe(false) +}) diff --git a/app/util/ip.ts b/app/util/ip.ts index 497822b730..f04a143436 100644 --- a/app/util/ip.ts +++ b/app/util/ip.ts @@ -21,3 +21,38 @@ export function validateIp(ip: string) { const isv6 = !isv4 && IPV6_REGEX.test(ip) return { isv4, isv6, valid: isv4 || isv6 } } + +// Following oxnet: +// https://github.com/oxidecomputer/oxnet/blob/7dacd265f1bcd0f8b47bd4805250c4f0812da206/src/ipnet.rs#L373-L385 +// https://github.com/oxidecomputer/oxnet/blob/7dacd265f1bcd0f8b47bd4805250c4f0812da206/src/ipnet.rs#L217-L223 + +// TODO: change this terrible result type to include more info, including a message +const invalidResult = { isv4: false, isv6: false, valid: false } + +export function validateIpNet(ipNet: string) { + // don't trim -- the form field should disallow whitespace + const splits = ipNet.split('/') + if (splits.length !== 2) { + return { isv4: false, isv6: false, valid: false } + } + + const [addrStr, widthStr] = splits + + const { isv4, isv6, valid } = validateIp(addrStr) + + if (!/^\d+$/.test(widthStr)) { + // TODO: return message about bad width + return { isv4: false, isv6: false, valid: false } + } + const width = parseInt(widthStr, 10) + + // validate width is a number and for IPv4 is under <= 32 + // https://github.com/oxidecomputer/oxnet/blob/7dacd265f1bcd0f8b47bd4805250c4f0812da206/src/ipnet.rs#L206 + // and for IPv6 is <= 128 + // https://github.com/oxidecomputer/oxnet/blob/7dacd265f1bcd0f8b47bd4805250c4f0812da206/src/ipnet.rs#L443 + // TODO: return message about bad width + if (isv4 && width > 32) return invalidResult + if (isv6 && width > 128) return invalidResult + + return { isv4, isv6, valid } +} From 7edb0f3ebae590375110c4bac01983def49f8146 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 24 Sep 2024 14:33:46 -0500 Subject: [PATCH 03/20] let's try a more interesting result type on the validator --- app/util/ip.spec.ts | 10 +++++++--- app/util/ip.ts | 37 ++++++++++++++++++++++--------------- 2 files changed, 29 insertions(+), 18 deletions(-) diff --git a/app/util/ip.spec.ts b/app/util/ip.spec.ts index 45b0f77267..187ef3c146 100644 --- a/app/util/ip.spec.ts +++ b/app/util/ip.spec.ts @@ -78,8 +78,12 @@ test.each(invalid)('validateIp catches invalid IP: %s', (s) => { expect(validateIp(s)).toStrictEqual({ isv4: false, isv6: false, valid: false }) }) -test.each([...v4.concat(v6).map((ip) => ip + '/10'), '2001:db8::/128'])('%s', (s) => { - expect(validateIpNet(s).valid).toBe(true) +test.each(v4.map((ip) => ip + '/10'))('%s', (s) => { + expect(validateIpNet(s).type).toBe('v4') +}) + +test.each([...v6.map((ip) => ip + '/10'), '2001:db8::/128'])('%s', (s) => { + expect(validateIpNet(s).type).toBe('v6') }) test.each([ @@ -95,5 +99,5 @@ test.each([ '192.168.0/24', '2001:db8::/129', ])('validateIpNet catches invalid value: %s', (s) => { - expect(validateIpNet(s).valid).toBe(false) + expect(validateIpNet(s).type).toBe('error') }) diff --git a/app/util/ip.ts b/app/util/ip.ts index f04a143436..718fa858f3 100644 --- a/app/util/ip.ts +++ b/app/util/ip.ts @@ -26,33 +26,40 @@ export function validateIp(ip: string) { // https://github.com/oxidecomputer/oxnet/blob/7dacd265f1bcd0f8b47bd4805250c4f0812da206/src/ipnet.rs#L373-L385 // https://github.com/oxidecomputer/oxnet/blob/7dacd265f1bcd0f8b47bd4805250c4f0812da206/src/ipnet.rs#L217-L223 -// TODO: change this terrible result type to include more info, including a message -const invalidResult = { isv4: false, isv6: false, valid: false } +type IpNetValidation = + | { type: 'v4' | 'v6'; address: string; width: number } + | { type: 'error'; message: string } -export function validateIpNet(ipNet: string) { - // don't trim -- the form field should disallow whitespace +export function validateIpNet(ipNet: string): IpNetValidation { const splits = ipNet.split('/') if (splits.length !== 2) { - return { isv4: false, isv6: false, valid: false } + return { + type: 'error', + message: 'Must contain an address and a width, separated by a /', + } } const [addrStr, widthStr] = splits const { isv4, isv6, valid } = validateIp(addrStr) + if (!valid) return { type: 'error', message: 'Invalid IP address' } + if (!/^\d+$/.test(widthStr)) { - // TODO: return message about bad width - return { isv4: false, isv6: false, valid: false } + return { type: 'error', message: 'Width must be an integer' } } const width = parseInt(widthStr, 10) - // validate width is a number and for IPv4 is under <= 32 - // https://github.com/oxidecomputer/oxnet/blob/7dacd265f1bcd0f8b47bd4805250c4f0812da206/src/ipnet.rs#L206 - // and for IPv6 is <= 128 - // https://github.com/oxidecomputer/oxnet/blob/7dacd265f1bcd0f8b47bd4805250c4f0812da206/src/ipnet.rs#L443 - // TODO: return message about bad width - if (isv4 && width > 32) return invalidResult - if (isv6 && width > 128) return invalidResult + if (isv4 && width > 32) { + return { type: 'error', message: 'Max width for IPv4 is 32' } + } + if (isv6 && width > 128) { + return { type: 'error', message: 'Max width for IPv6 is 128' } + } - return { isv4, isv6, valid } + return { + type: isv4 ? 'v4' : 'v6', + address: addrStr, + width: width, + } } From 02a20d1c3c1110e62c45ee4fd96d759f65121810 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 24 Sep 2024 15:17:46 -0500 Subject: [PATCH 04/20] validate IPs and IP nets on the route form --- app/forms/vpc-router-route-common.tsx | 27 +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 deletions(-) diff --git a/app/forms/vpc-router-route-common.tsx b/app/forms/vpc-router-route-common.tsx index 6207d8b305..bd1b461600 100644 --- a/app/forms/vpc-router-route-common.tsx +++ b/app/forms/vpc-router-route-common.tsx @@ -24,6 +24,7 @@ import { NameField } from '~/components/form/fields/NameField' import { TextField } from '~/components/form/fields/TextField' import { useVpcRouterSelector } from '~/hooks/use-params' import { Message } from '~/ui/lib/Message' +import { validateIp, validateIpNet } from '~/util/ip' export type RouteFormValues = RouterRouteCreate | Required @@ -149,13 +150,27 @@ export const RouteFormFields = ({ form, disabled }: RouteFormFieldsProps) => { required onChange={() => { form.setValue('destination.value', '') + form.clearErrors('destination.value') }} disabled={disabled} /> {destinationType === 'subnet' ? ( ) : ( - + { + const destType = formValues.destination.type + if (destType === 'ip_net') { + const result = validateIpNet(value) + if (result.type === 'error') return result.message + } else if (destType === 'ip') { + if (!validateIp(value).valid) { + return 'Not a valid IP address' + } + } + }} + /> )} { required onChange={(value) => { form.setValue('target.value', value === 'internet_gateway' ? 'outbound' : '') + form.clearErrors('target.value') }} disabled={disabled} /> {targetType === 'drop' ? null : targetType === 'instance' ? ( ) : ( - + { + if (formValues.target.type === 'ip' && !validateIp(value).valid) { + return 'Not a valid IP address' + } + }} + /> )} ) From ee03705e3bf49531b62231c92f6d00c8ffec5d40 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 24 Sep 2024 15:44:38 -0500 Subject: [PATCH 05/20] redo validateIp to look more like validateIpNet, esp. including message --- app/forms/ip-pool-range-add.tsx | 14 ++++++-------- app/forms/vpc-router-route-common.tsx | 10 +++++----- app/util/ip.spec.ts | 6 +++--- app/util/ip.ts | 22 +++++++++++++--------- mock-api/msw/handlers.ts | 5 ++++- mock-api/msw/util.ts | 6 +++--- 6 files changed, 34 insertions(+), 29 deletions(-) diff --git a/app/forms/ip-pool-range-add.tsx b/app/forms/ip-pool-range-add.tsx index ada846eb85..8dbb2d9003 100644 --- a/app/forms/ip-pool-range-add.tsx +++ b/app/forms/ip-pool-range-add.tsx @@ -23,8 +23,6 @@ const defaultValues: IpRange = { last: '', } -const invalidAddressError = { type: 'pattern', message: 'Not a valid IP address' } - const ipv6Error = { type: 'pattern', message: 'IPv6 ranges are not yet supported' } /** @@ -40,15 +38,15 @@ function resolver(values: IpRange) { const errors: FieldErrors = {} - if (!first.valid) { - errors.first = invalidAddressError - } else if (first.isv6) { + if (first.type === 'error') { + errors.first = { type: 'pattern', message: first.message } + } else if (first.type === 'v6') { errors.first = ipv6Error } - if (!last.valid) { - errors.last = invalidAddressError - } else if (last.isv6) { + if (last.type === 'error') { + errors.last = { type: 'pattern', message: last.message } + } else if (last.type === 'v6') { errors.last = ipv6Error } diff --git a/app/forms/vpc-router-route-common.tsx b/app/forms/vpc-router-route-common.tsx index bd1b461600..fcce5e1216 100644 --- a/app/forms/vpc-router-route-common.tsx +++ b/app/forms/vpc-router-route-common.tsx @@ -165,9 +165,8 @@ export const RouteFormFields = ({ form, disabled }: RouteFormFieldsProps) => { const result = validateIpNet(value) if (result.type === 'error') return result.message } else if (destType === 'ip') { - if (!validateIp(value).valid) { - return 'Not a valid IP address' - } + const result = validateIp(value) + if (result.type === 'error') return result.message } }} /> @@ -191,8 +190,9 @@ export const RouteFormFields = ({ form, disabled }: RouteFormFieldsProps) => { { - if (formValues.target.type === 'ip' && !validateIp(value).valid) { - return 'Not a valid IP address' + if (formValues.target.type === 'ip') { + const result = validateIp(value) + if (result.type === 'error') return result.message } }} /> diff --git a/app/util/ip.spec.ts b/app/util/ip.spec.ts index 187ef3c146..270b058508 100644 --- a/app/util/ip.spec.ts +++ b/app/util/ip.spec.ts @@ -17,7 +17,7 @@ import { validateIp, validateIpNet } from './ip' const v4 = ['123.4.56.7', '1.2.3.4'] test.each(v4)('validateIp catches valid IPV4 / invalid IPV6: %s', (s) => { - expect(validateIp(s)).toStrictEqual({ isv4: true, isv6: false, valid: true }) + expect(validateIp(s)).toStrictEqual({ type: 'v4', address: s }) }) const v6 = [ @@ -40,7 +40,7 @@ const v6 = [ ] test.each(v6)('validateIp catches invalid IPV4 / valid IPV6: %s', (s) => { - expect(validateIp(s)).toStrictEqual({ isv4: false, isv6: true, valid: true }) + expect(validateIp(s).type).toEqual('v6') }) const invalid = [ @@ -75,7 +75,7 @@ const invalid = [ ] test.each(invalid)('validateIp catches invalid IP: %s', (s) => { - expect(validateIp(s)).toStrictEqual({ isv4: false, isv6: false, valid: false }) + expect(validateIp(s)).toStrictEqual({ type: 'error', message: 'Not a valid IP address' }) }) test.each(v4.map((ip) => ip + '/10'))('%s', (s) => { diff --git a/app/util/ip.ts b/app/util/ip.ts index 718fa858f3..1b17d207b9 100644 --- a/app/util/ip.ts +++ b/app/util/ip.ts @@ -16,10 +16,14 @@ const IPV4_REGEX = const IPV6_REGEX = /^(?:(?:[\da-f]{1,4}:){7}[\da-f]{1,4}|(?:[\da-f]{1,4}:){1,7}:|(?:[\da-f]{1,4}:){1,6}:[\da-f]{1,4}|(?:[\da-f]{1,4}:){1,5}(?::[\da-f]{1,4}){1,2}|(?:[\da-f]{1,4}:){1,4}(?::[\da-f]{1,4}){1,3}|(?:[\da-f]{1,4}:){1,3}(?::[\da-f]{1,4}){1,4}|(?:[\da-f]{1,4}:){1,2}(?::[\da-f]{1,4}){1,5}|[\da-f]{1,4}:(?::[\da-f]{1,4}){1,6}|:(?:(?::[\da-f]{1,4}){1,7}|:)|fe80:(?::[\da-f]{0,4}){0,4}%[\da-z]+|::(?:f{4}(?::0{1,4})?:)?(?:(?:25[0-5]|(?:2[0-4]|1?\d)?\d)\.){3}(?:25[0-5]|(?:2[0-4]|1?\d)?\d)|(?:[\da-f]{1,4}:){1,4}:(?:(?:25[0-5]|(?:2[0-4]|1?\d)?\d)\.){3}(?:25[0-5]|(?:2[0-4]|1?\d)?\d))$/iu -export function validateIp(ip: string) { - const isv4 = IPV4_REGEX.test(ip) - const isv6 = !isv4 && IPV6_REGEX.test(ip) - return { isv4, isv6, valid: isv4 || isv6 } +type IpValidation = + | { type: 'v4' | 'v6'; address: string } + | { type: 'error'; message: string } + +export function validateIp(ip: string): IpValidation { + if (IPV4_REGEX.test(ip)) return { type: 'v4', address: ip } + if (IPV6_REGEX.test(ip)) return { type: 'v6', address: ip } + return { type: 'error', message: 'Not a valid IP address' } } // Following oxnet: @@ -41,24 +45,24 @@ export function validateIpNet(ipNet: string): IpNetValidation { const [addrStr, widthStr] = splits - const { isv4, isv6, valid } = validateIp(addrStr) + const { type: ipType } = validateIp(addrStr) - if (!valid) return { type: 'error', message: 'Invalid IP address' } + if (ipType === 'error') return { type: 'error', message: 'Invalid IP address' } if (!/^\d+$/.test(widthStr)) { return { type: 'error', message: 'Width must be an integer' } } const width = parseInt(widthStr, 10) - if (isv4 && width > 32) { + if (ipType === 'v4' && width > 32) { return { type: 'error', message: 'Max width for IPv4 is 32' } } - if (isv6 && width > 128) { + if (ipType === 'v6' && width > 128) { return { type: 'error', message: 'Max width for IPv6 is 128' } } return { - type: isv4 ? 'v4' : 'v6', + type: ipType, address: addrStr, width: width, } diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index a98a13637c..af3078725d 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -729,7 +729,10 @@ export const handlers = makeHandlers({ const ranges = db.ipPoolRanges .filter((r) => r.ip_pool_id === pool.id) .map((r) => r.range) - const [ipv4Ranges, ipv6Ranges] = R.partition(ranges, (r) => validateIp(r.first).isv4) + const [ipv4Ranges, ipv6Ranges] = R.partition( + ranges, + (r) => validateIp(r.first).type === 'v4' + ) // in the real backend there are also SNAT IPs, but we don't currently // represent those because they are not exposed through the API (except diff --git a/mock-api/msw/util.ts b/mock-api/msw/util.ts index 8829e8836d..3a9f267457 100644 --- a/mock-api/msw/util.ts +++ b/mock-api/msw/util.ts @@ -384,14 +384,14 @@ export function requireRole( } const ipToBigInt = (ip: string): bigint => - validateIp(ip).isv4 ? new IPv4(ip).value : new IPv6(ip).value + validateIp(ip).type === 'v4' ? new IPv4(ip).value : new IPv6(ip).value export const ipRangeLen = ({ first, last }: IpRange) => ipToBigInt(last) - ipToBigInt(first) + 1n function ipInRange(ip: string, { first, last }: IpRange): boolean { - const ipIsV4 = validateIp(ip).isv4 - const rangeIsV4 = validateIp(first).isv4 + const ipIsV4 = validateIp(ip).type === 'v4' + const rangeIsV4 = validateIp(first).type === 'v4' // if they're not the same version then definitely false if (ipIsV4 !== rangeIsV4) return false From 1cce7b4411f876fb073954e4b367b4b40bd54934 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 24 Sep 2024 16:07:29 -0500 Subject: [PATCH 06/20] tweak validation logic to get more helpful messages, test it --- app/util/ip.spec.ts | 41 ++++++++++++++++++++++++++++------------- app/util/ip.ts | 15 ++++++++------- 2 files changed, 36 insertions(+), 20 deletions(-) diff --git a/app/util/ip.spec.ts b/app/util/ip.spec.ts index 270b058508..5afec9cce9 100644 --- a/app/util/ip.spec.ts +++ b/app/util/ip.spec.ts @@ -59,6 +59,7 @@ const invalid = [ '256.0.0.0', '260.0.0.0', '256.1.1.1', + '192.168.0.0.0', '2001:0db8:85a3:0000:0000:8a2e:0370:7334 ', ' 2001:db8::', '1:2:3:4:5:6:7:8:9', @@ -86,18 +87,32 @@ test.each([...v6.map((ip) => ip + '/10'), '2001:db8::/128'])('%s', (s) => { expect(validateIpNet(s).type).toBe('v6') }) +test.each(invalid.map((ip) => ip + '/10'))( + 'validateIpNet catches invalid value: %s', + (s) => { + expect(validateIpNet(s).type).toBe('error') + } +) + +const nonsense = 'Must contain an IP address and a width, separated by a /' +const badWidth = 'Width must be an integer' +const ipv4Width = 'Max width for IPv4 is 32' +const ipv6Width = 'Max width for IPv6 is 128' + test.each([ - ...invalid.map((ip) => ip + '/10'), - 'abc', - '', - '1.1.1.1', - '1.1.1.1/180', - '256.0.0.0/24', - '192.168.0.0/33', - '192.168.0.0/-1', - '192.168.0.0.0/24', - '192.168.0/24', - '2001:db8::/129', -])('validateIpNet catches invalid value: %s', (s) => { - expect(validateIpNet(s).type).toBe('error') + ['', nonsense], + ['abc', nonsense], + ['/32', nonsense], + ['1.1.1.1', nonsense], + ['abc/2', nonsense], + ['1.1.1.1/', nonsense], + ['192.168.0.0.0/24', nonsense], + ['fd::/', nonsense], + ['1.1.1.1/a', badWidth], + ['192.168.0.0/-1', badWidth], + ['fd::/a', badWidth], + ['1.1.1.1/33', ipv4Width], + ['fd::/129', ipv6Width], +])('validateIpNet message: %s', (input, message) => { + expect(validateIpNet(input)).toEqual({ type: 'error', message }) }) diff --git a/app/util/ip.ts b/app/util/ip.ts index 1b17d207b9..e07367746b 100644 --- a/app/util/ip.ts +++ b/app/util/ip.ts @@ -34,20 +34,21 @@ type IpNetValidation = | { type: 'v4' | 'v6'; address: string; width: number } | { type: 'error'; message: string } +const nonsenseError = { + type: 'error' as const, + message: 'Must contain an IP address and a width, separated by a /', +} + export function validateIpNet(ipNet: string): IpNetValidation { const splits = ipNet.split('/') - if (splits.length !== 2) { - return { - type: 'error', - message: 'Must contain an address and a width, separated by a /', - } - } + if (splits.length !== 2) return nonsenseError const [addrStr, widthStr] = splits const { type: ipType } = validateIp(addrStr) - if (ipType === 'error') return { type: 'error', message: 'Invalid IP address' } + if (ipType === 'error') return nonsenseError + if (widthStr.trim().length === 0) return nonsenseError if (!/^\d+$/.test(widthStr)) { return { type: 'error', message: 'Width must be an integer' } From 22747cbdb737960c7f89434f10f468e410857a41 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 24 Sep 2024 16:31:25 -0500 Subject: [PATCH 07/20] validate transit IPs on network interface edit --- app/forms/network-interface-edit.tsx | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/app/forms/network-interface-edit.tsx b/app/forms/network-interface-edit.tsx index bb9cdf558e..6f2f9786b6 100644 --- a/app/forms/network-interface-edit.tsx +++ b/app/forms/network-interface-edit.tsx @@ -25,6 +25,7 @@ import { FieldLabel } from '~/ui/lib/FieldLabel' import * as MiniTable from '~/ui/lib/MiniTable' import { TextInputHint } from '~/ui/lib/TextInput' import { KEYS } from '~/ui/util/keys' +import { validateIpNet } from '~/util/ip' import { links } from '~/util/links' type EditNetworkInterfaceFormProps = { @@ -57,12 +58,11 @@ export function EditNetworkInterfaceForm({ const transitIpsForm = useForm({ defaultValues: { transitIp: '' } }) - const submitTransitIp = () => { - const transitIp = transitIpsForm.getValues('transitIp') + const submitTransitIp = transitIpsForm.handleSubmit(({ transitIp }) => { if (!transitIp) return form.setValue('transitIps', [...transitIps, transitIp]) transitIpsForm.reset() - } + }) return ( - Enter an IPv4 or IPv6 address.{' '} + An IP network, like 192.168.0.0/16.{' '} Learn more about transit IPs. @@ -107,10 +107,16 @@ export function EditNetworkInterfaceForm({ submitTransitIp() } }} + validate={(value) => { + const result = validateIpNet(value) + if (result.type === 'error') return result.message + }} + placeholder="Enter an IP network" /> Date: Tue, 24 Sep 2024 16:38:19 -0500 Subject: [PATCH 08/20] split parse from validate, it's clean! --- app/forms/ip-pool-range-add.tsx | 6 +++--- app/forms/network-interface-edit.tsx | 5 +---- app/forms/vpc-router-route-common.tsx | 21 +++++------------- app/util/ip.spec.ts | 31 ++++++++++++--------------- app/util/ip.ts | 30 ++++++++++++++++++++------ mock-api/msw/handlers.ts | 4 ++-- mock-api/msw/util.ts | 8 +++---- 7 files changed, 52 insertions(+), 53 deletions(-) diff --git a/app/forms/ip-pool-range-add.tsx b/app/forms/ip-pool-range-add.tsx index 8dbb2d9003..b5cb0488e0 100644 --- a/app/forms/ip-pool-range-add.tsx +++ b/app/forms/ip-pool-range-add.tsx @@ -15,7 +15,7 @@ import { SideModalForm } from '~/components/form/SideModalForm' import { useIpPoolSelector } from '~/hooks/use-params' import { addToast } from '~/stores/toast' import { Message } from '~/ui/lib/Message' -import { validateIp } from '~/util/ip' +import { parseIp } from '~/util/ip' import { pb } from '~/util/path-builder' const defaultValues: IpRange = { @@ -33,8 +33,8 @@ const ipv6Error = { type: 'pattern', message: 'IPv6 ranges are not yet supported * regex twice, though. */ function resolver(values: IpRange) { - const first = validateIp(values.first) - const last = validateIp(values.last) + const first = parseIp(values.first) + const last = parseIp(values.last) const errors: FieldErrors = {} diff --git a/app/forms/network-interface-edit.tsx b/app/forms/network-interface-edit.tsx index 6f2f9786b6..d7f1ee62f6 100644 --- a/app/forms/network-interface-edit.tsx +++ b/app/forms/network-interface-edit.tsx @@ -107,10 +107,7 @@ export function EditNetworkInterfaceForm({ submitTransitIp() } }} - validate={(value) => { - const result = validateIpNet(value) - if (result.type === 'error') return result.message - }} + validate={validateIpNet} placeholder="Enter an IP network" /> diff --git a/app/forms/vpc-router-route-common.tsx b/app/forms/vpc-router-route-common.tsx index fcce5e1216..05f9cc1018 100644 --- a/app/forms/vpc-router-route-common.tsx +++ b/app/forms/vpc-router-route-common.tsx @@ -159,16 +159,10 @@ export const RouteFormFields = ({ form, disabled }: RouteFormFieldsProps) => { ) : ( { - const destType = formValues.destination.type - if (destType === 'ip_net') { - const result = validateIpNet(value) - if (result.type === 'error') return result.message - } else if (destType === 'ip') { - const result = validateIp(value) - if (result.type === 'error') return result.message - } - }} + validate={(value, { destination }) => + (destination.type === 'ip_net' && validateIpNet(value)) || + (destination.type === 'ip' && validateIp(value)) + } /> )} { ) : ( { - if (formValues.target.type === 'ip') { - const result = validateIp(value) - if (result.type === 'error') return result.message - } - }} + validate={(value, { target }) => target.type === 'ip' && validateIp(value)} /> )} diff --git a/app/util/ip.spec.ts b/app/util/ip.spec.ts index 5afec9cce9..6951bdbc27 100644 --- a/app/util/ip.spec.ts +++ b/app/util/ip.spec.ts @@ -8,7 +8,7 @@ import { expect, test } from 'vitest' -import { validateIp, validateIpNet } from './ip' +import { parseIp, parseIpNet } from './ip' // Small Rust project where we validate that the built-in Ipv4Addr and Ipv6Addr // and oxnet's Ipv4Net and Ipv6Net have the same validation behavior as our code. @@ -16,8 +16,8 @@ import { validateIp, validateIpNet } from './ip' const v4 = ['123.4.56.7', '1.2.3.4'] -test.each(v4)('validateIp catches valid IPV4 / invalid IPV6: %s', (s) => { - expect(validateIp(s)).toStrictEqual({ type: 'v4', address: s }) +test.each(v4)('parseIp catches valid IPV4 / invalid IPV6: %s', (s) => { + expect(parseIp(s)).toStrictEqual({ type: 'v4', address: s }) }) const v6 = [ @@ -39,8 +39,8 @@ const v6 = [ 'ffff:ffff:ffff:ffff:ffff:ffff:ffff:ffff', ] -test.each(v6)('validateIp catches invalid IPV4 / valid IPV6: %s', (s) => { - expect(validateIp(s).type).toEqual('v6') +test.each(v6)('parseIp catches invalid IPV4 / valid IPV6: %s', (s) => { + expect(parseIp(s).type).toEqual('v6') }) const invalid = [ @@ -75,24 +75,21 @@ const invalid = [ 'fe08::7:8interface', ] -test.each(invalid)('validateIp catches invalid IP: %s', (s) => { - expect(validateIp(s)).toStrictEqual({ type: 'error', message: 'Not a valid IP address' }) +test.each(invalid)('parseIp catches invalid IP: %s', (s) => { + expect(parseIp(s)).toStrictEqual({ type: 'error', message: 'Not a valid IP address' }) }) test.each(v4.map((ip) => ip + '/10'))('%s', (s) => { - expect(validateIpNet(s).type).toBe('v4') + expect(parseIpNet(s).type).toBe('v4') }) test.each([...v6.map((ip) => ip + '/10'), '2001:db8::/128'])('%s', (s) => { - expect(validateIpNet(s).type).toBe('v6') + expect(parseIpNet(s).type).toBe('v6') }) -test.each(invalid.map((ip) => ip + '/10'))( - 'validateIpNet catches invalid value: %s', - (s) => { - expect(validateIpNet(s).type).toBe('error') - } -) +test.each(invalid.map((ip) => ip + '/10'))('parseIpNet catches invalid value: %s', (s) => { + expect(parseIpNet(s).type).toBe('error') +}) const nonsense = 'Must contain an IP address and a width, separated by a /' const badWidth = 'Width must be an integer' @@ -113,6 +110,6 @@ test.each([ ['fd::/a', badWidth], ['1.1.1.1/33', ipv4Width], ['fd::/129', ipv6Width], -])('validateIpNet message: %s', (input, message) => { - expect(validateIpNet(input)).toEqual({ type: 'error', message }) +])('parseIpNet message: %s', (input, message) => { + expect(parseIpNet(input)).toEqual({ type: 'error', message }) }) diff --git a/app/util/ip.ts b/app/util/ip.ts index e07367746b..9d060aab5b 100644 --- a/app/util/ip.ts +++ b/app/util/ip.ts @@ -16,21 +16,28 @@ const IPV4_REGEX = const IPV6_REGEX = /^(?:(?:[\da-f]{1,4}:){7}[\da-f]{1,4}|(?:[\da-f]{1,4}:){1,7}:|(?:[\da-f]{1,4}:){1,6}:[\da-f]{1,4}|(?:[\da-f]{1,4}:){1,5}(?::[\da-f]{1,4}){1,2}|(?:[\da-f]{1,4}:){1,4}(?::[\da-f]{1,4}){1,3}|(?:[\da-f]{1,4}:){1,3}(?::[\da-f]{1,4}){1,4}|(?:[\da-f]{1,4}:){1,2}(?::[\da-f]{1,4}){1,5}|[\da-f]{1,4}:(?::[\da-f]{1,4}){1,6}|:(?:(?::[\da-f]{1,4}){1,7}|:)|fe80:(?::[\da-f]{0,4}){0,4}%[\da-z]+|::(?:f{4}(?::0{1,4})?:)?(?:(?:25[0-5]|(?:2[0-4]|1?\d)?\d)\.){3}(?:25[0-5]|(?:2[0-4]|1?\d)?\d)|(?:[\da-f]{1,4}:){1,4}:(?:(?:25[0-5]|(?:2[0-4]|1?\d)?\d)\.){3}(?:25[0-5]|(?:2[0-4]|1?\d)?\d))$/iu -type IpValidation = - | { type: 'v4' | 'v6'; address: string } - | { type: 'error'; message: string } +type ParsedIp = { type: 'v4' | 'v6'; address: string } | { type: 'error'; message: string } -export function validateIp(ip: string): IpValidation { +export function parseIp(ip: string): ParsedIp { if (IPV4_REGEX.test(ip)) return { type: 'v4', address: ip } if (IPV6_REGEX.test(ip)) return { type: 'v6', address: ip } return { type: 'error', message: 'Not a valid IP address' } } +/** + * Convenience helper that can be plugged into RHF simply as + * `validate={validateIp}` + */ +export function validateIp(ip: string): string | undefined { + const result = parseIp(ip) + if (result.type === 'error') return result.message +} + // Following oxnet: // https://github.com/oxidecomputer/oxnet/blob/7dacd265f1bcd0f8b47bd4805250c4f0812da206/src/ipnet.rs#L373-L385 // https://github.com/oxidecomputer/oxnet/blob/7dacd265f1bcd0f8b47bd4805250c4f0812da206/src/ipnet.rs#L217-L223 -type IpNetValidation = +type ParsedIpNet = | { type: 'v4' | 'v6'; address: string; width: number } | { type: 'error'; message: string } @@ -39,13 +46,13 @@ const nonsenseError = { message: 'Must contain an IP address and a width, separated by a /', } -export function validateIpNet(ipNet: string): IpNetValidation { +export function parseIpNet(ipNet: string): ParsedIpNet { const splits = ipNet.split('/') if (splits.length !== 2) return nonsenseError const [addrStr, widthStr] = splits - const { type: ipType } = validateIp(addrStr) + const { type: ipType } = parseIp(addrStr) if (ipType === 'error') return nonsenseError if (widthStr.trim().length === 0) return nonsenseError @@ -68,3 +75,12 @@ export function validateIpNet(ipNet: string): IpNetValidation { width: width, } } + +/** + * Convenience helper that can be plugged into RHF simply as + * `validate={validateIpNet}` + */ +export function validateIpNet(ipNet: string): string | undefined { + const result = parseIpNet(ipNet) + if (result.type === 'error') return result.message +} diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index af3078725d..7527181c2e 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -22,7 +22,7 @@ import { import { json, makeHandlers, type Json } from '~/api/__generated__/msw-handlers' import { instanceCan } from '~/api/util' -import { validateIp } from '~/util/ip' +import { parseIp } from '~/util/ip' import { commaSeries } from '~/util/str' import { GiB } from '~/util/units' @@ -731,7 +731,7 @@ export const handlers = makeHandlers({ .map((r) => r.range) const [ipv4Ranges, ipv6Ranges] = R.partition( ranges, - (r) => validateIp(r.first).type === 'v4' + (r) => parseIp(r.first).type === 'v4' ) // in the real backend there are also SNAT IPs, but we don't currently diff --git a/mock-api/msw/util.ts b/mock-api/msw/util.ts index 3a9f267457..8b309eb66d 100644 --- a/mock-api/msw/util.ts +++ b/mock-api/msw/util.ts @@ -25,7 +25,7 @@ import { } from '@oxide/api' import { json, type Json } from '~/api/__generated__/msw-handlers' -import { validateIp } from '~/util/ip' +import { parseIp } from '~/util/ip' import { GiB, TiB } from '~/util/units' import type { DbRoleAssignmentResourceType } from '..' @@ -384,14 +384,14 @@ export function requireRole( } const ipToBigInt = (ip: string): bigint => - validateIp(ip).type === 'v4' ? new IPv4(ip).value : new IPv6(ip).value + parseIp(ip).type === 'v4' ? new IPv4(ip).value : new IPv6(ip).value export const ipRangeLen = ({ first, last }: IpRange) => ipToBigInt(last) - ipToBigInt(first) + 1n function ipInRange(ip: string, { first, last }: IpRange): boolean { - const ipIsV4 = validateIp(ip).type === 'v4' - const rangeIsV4 = validateIp(first).type === 'v4' + const ipIsV4 = parseIp(ip).type === 'v4' + const rangeIsV4 = parseIp(first).type === 'v4' // if they're not the same version then definitely false if (ipIsV4 !== rangeIsV4) return false From da65a4020d37f86e491141a287515d3cdc7ad2c7 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 24 Sep 2024 17:08:46 -0500 Subject: [PATCH 09/20] fix e2e failure but it raises existential questions --- app/forms/vpc-router-route-common.tsx | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-) diff --git a/app/forms/vpc-router-route-common.tsx b/app/forms/vpc-router-route-common.tsx index 05f9cc1018..d989c7e494 100644 --- a/app/forms/vpc-router-route-common.tsx +++ b/app/forms/vpc-router-route-common.tsx @@ -159,9 +159,13 @@ export const RouteFormFields = ({ form, disabled }: RouteFormFieldsProps) => { ) : ( - (destination.type === 'ip_net' && validateIpNet(value)) || - (destination.type === 'ip' && validateIp(value)) + // it seems this validation function stays registered on the combobox + // even when the type is switched to subnet!!!?!?!?!?! + validate={ + (value, { destination }) => + (destination.type === 'ip_net' && validateIpNet(value)) || + (destination.type === 'ip' && validateIp(value)) || + undefined // false is invalid but true and undefined are valid????? } /> )} @@ -183,7 +187,9 @@ export const RouteFormFields = ({ form, disabled }: RouteFormFieldsProps) => { ) : ( target.type === 'ip' && validateIp(value)} + validate={(value, { target }) => + (target.type === 'ip' && validateIp(value)) || undefined + } /> )} From feeedd0b8543a7bf487f548df3222612b76310ac Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 25 Sep 2024 00:00:27 -0500 Subject: [PATCH 10/20] test leading zeroes in width --- app/util/ip.spec.ts | 11 +++++++---- 1 file changed, 7 insertions(+), 4 deletions(-) diff --git a/app/util/ip.spec.ts b/app/util/ip.spec.ts index 6951bdbc27..1b41d8ea0a 100644 --- a/app/util/ip.spec.ts +++ b/app/util/ip.spec.ts @@ -79,13 +79,16 @@ test.each(invalid)('parseIp catches invalid IP: %s', (s) => { expect(parseIp(s)).toStrictEqual({ type: 'error', message: 'Not a valid IP address' }) }) -test.each(v4.map((ip) => ip + '/10'))('%s', (s) => { +test.each([...v4.map((ip) => ip + '/10'), '1.1.1.1/04', '1.1.1.1/000004'])('%s', (s) => { expect(parseIpNet(s).type).toBe('v4') }) -test.each([...v6.map((ip) => ip + '/10'), '2001:db8::/128'])('%s', (s) => { - expect(parseIpNet(s).type).toBe('v6') -}) +test.each([...v6.map((ip) => ip + '/10'), '2001:db8::/128', '2001:db8::/00004'])( + '%s', + (s) => { + expect(parseIpNet(s).type).toBe('v6') + } +) test.each(invalid.map((ip) => ip + '/10'))('parseIpNet catches invalid value: %s', (s) => { expect(parseIpNet(s).type).toBe('error') From 48f29856250207ffac0c869292a2c9f1512f1182 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Wed, 25 Sep 2024 13:35:21 -0500 Subject: [PATCH 11/20] pass noop validate functions to comboboxes that get swapped with textfields --- app/components/form/fields/ComboboxField.tsx | 10 +++++++++- app/forms/vpc-router-route-common.tsx | 18 +++++++++++------- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/app/components/form/fields/ComboboxField.tsx b/app/components/form/fields/ComboboxField.tsx index 3b9ee63f51..ce79af7e4d 100644 --- a/app/components/form/fields/ComboboxField.tsx +++ b/app/components/form/fields/ComboboxField.tsx @@ -10,7 +10,9 @@ import { useController, type Control, type FieldPath, + type FieldPathValue, type FieldValues, + type Validate, } from 'react-hook-form' import { Combobox, type ComboboxBaseProps } from '~/ui/lib/Combobox' @@ -25,6 +27,7 @@ export type ComboboxFieldProps< name: TName control: Control onChange?: (value: string | null | undefined) => void + validate?: Validate, TFieldValues> } & ComboboxBaseProps export function ComboboxField< @@ -51,9 +54,14 @@ export function ComboboxField< : allowArbitraryValues ? 'Select an option or enter a custom value' : 'Select an option', + validate, ...props }: ComboboxFieldProps) { - const { field, fieldState } = useController({ name, control, rules: { required } }) + const { field, fieldState } = useController({ + name, + control, + rules: { required, validate }, + }) return (
{ required: true, disabled, description: destinationValueDescription[destinationType], + // need a default to prevent the text field validation function from + // sticking around when we switch to the combobox + validate: () => undefined, } const targetValueProps = { name: 'target.value' as const, @@ -133,6 +136,9 @@ export const RouteFormFields = ({ form, disabled }: RouteFormFieldsProps) => { // 'internet_gateway' targetTypes can only have the value 'outbound', so we disable the field disabled: disabled || targetType === 'internet_gateway', description: targetValueDescription[targetType], + // need a default to prevent the text field validation function from + // sticking around when we switch to the combobox + validate: () => undefined, } return ( <> @@ -159,13 +165,11 @@ export const RouteFormFields = ({ form, disabled }: RouteFormFieldsProps) => { ) : ( - (destination.type === 'ip_net' && validateIpNet(value)) || - (destination.type === 'ip' && validateIp(value)) || - undefined // false is invalid but true and undefined are valid????? + validate={(value, { destination }) => + (destination.type === 'ip_net' && validateIpNet(value)) || + (destination.type === 'ip' && validateIp(value)) || + // false is invalid but true and undefined are valid so we need this + undefined } /> )} From 1efa7ef378c04af55b7be280c8f1b7ae9c023cde Mon Sep 17 00:00:00 2001 From: David Crespo Date: Thu, 26 Sep 2024 17:29:48 -0500 Subject: [PATCH 12/20] validate IPs, IP nets and names on firewall rules form --- app/forms/firewall-rules-common.tsx | 26 +++++++++++++++++--------- 1 file changed, 17 insertions(+), 9 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 5adac48cbe..5c1d39cac0 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -27,7 +27,7 @@ import { CheckboxField } from '~/components/form/fields/CheckboxField' import { ComboboxField } from '~/components/form/fields/ComboboxField' import { DescriptionField } from '~/components/form/fields/DescriptionField' import { ListboxField } from '~/components/form/fields/ListboxField' -import { NameField } from '~/components/form/fields/NameField' +import { NameField, validateName } from '~/components/form/fields/NameField' import { NumberField } from '~/components/form/fields/NumberField' import { RadioField } from '~/components/form/fields/RadioField' import { TextField, TextFieldInner } from '~/components/form/fields/TextField' @@ -39,6 +39,7 @@ import * as MiniTable from '~/ui/lib/MiniTable' import { TextInputHint } from '~/ui/lib/TextInput' import { KEYS } from '~/ui/util/keys' import { ALL_ISH } from '~/util/consts' +import { validateIp, validateIpNet } from '~/util/ip' import { links } from '~/util/links' import { capitalize } from '~/util/str' @@ -132,8 +133,10 @@ const DynamicTypeAndValueFields = ({ items={items} allowArbitraryValues hideOptionalTag - // TODO: validate here, but it's complicated because it's conditional - // on which type is selected + validate={(value) => + // TODO: is required false correct here? should this function even have that argument? + validateName(value, `${capitalize(sectionType)} name`, false) + } /> ) : ( + (valueType === 'ip' && validateIp(value)) || + (valueType === 'ip_net' && validateIpNet(value)) || + undefined + } /> )} @@ -272,7 +278,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = if (!type || !value) return if (targets.value.some((t) => t.value === value && t.type === type)) return targets.onChange([...targets.value, { type, value }]) - targetForm.reset() + targetForm.reset(targetAndHostDefaultValues) }) // Ports @@ -306,7 +312,9 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = if (!type || !value) return if (hosts.value.some((t) => t.value === value && t.type === type)) return hosts.onChange([...hosts.value, { type, value }]) - hostForm.reset() + // TODO: something is not resetting right -- if the IP net field is set to + // validate on change due to having an error, it stays that way after submit + hostForm.reset(targetAndHostDefaultValues) }) return ( @@ -411,7 +419,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = control={targetForm.control} valueType={targetType} items={targetItems[targetType]} - onTypeChange={() => targetForm.setValue('value', '')} + onTypeChange={() => targetForm.resetField('value')} onInputChange={(value) => targetForm.setValue('value', value)} onSubmitTextField={submitTarget} /> @@ -518,7 +526,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = control={hostForm.control} valueType={hostType} items={hostFilterItems[hostType]} - onTypeChange={() => hostForm.setValue('value', '')} + onTypeChange={() => targetForm.resetField('value')} onInputChange={(value) => hostForm.setValue('value', value)} onSubmitTextField={submitHost} /> From e54fc3810492cf2ae468c6f87d7809cb6bfd0c6c Mon Sep 17 00:00:00 2001 From: David Crespo Date: Fri, 27 Sep 2024 16:33:18 -0500 Subject: [PATCH 13/20] fix subform revalidation issue --- app/forms/firewall-rules-common.tsx | 33 +++++++++++++++++++++-------- 1 file changed, 24 insertions(+), 9 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 5c1d39cac0..72ed6998d0 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -6,6 +6,7 @@ * Copyright Oxide Computer Company */ +import { useEffect } from 'react' import { useController, useForm, @@ -63,7 +64,6 @@ type TargetAndHostFilterType = type TargetAndHostFormValues = { type: TargetAndHostFilterType value: string - subnetVpc?: string } // these are part of the target and host filter form; @@ -239,14 +239,14 @@ type CommonFieldsProps = { error: ApiError | null } +const targetAndHostDefaultValues: TargetAndHostFormValues = { + type: 'vpc', + value: '', +} + export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) => { const { project, vpc } = useVpcSelector() - const targetAndHostDefaultValues: TargetAndHostFormValues = { - type: 'vpc', - value: '', - // only becomes relevant when the type is 'VPC subnet'; ignored otherwise - subnetVpc: vpc, - } + // prefetchedApiQueries below are prefetched in firewall-rules-create and -edit const { data: { items: instances }, @@ -280,6 +280,18 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = targets.onChange([...targets.value, { type, value }]) targetForm.reset(targetAndHostDefaultValues) }) + // HACK: we need to reset the target form completely after a successful submit, + // including especially `isSubmitted`, because that governs whether we're in + // the validate regime (which doesn't validate until submit) or the reValidate + // regime (which validate on every keypress). The reset inside `handleSubmit` + // doesn't do that for us because `handleSubmit` set `isSubmitted: true` after + // running the callback. So we have to watch for a successful submit and call + // the reset again here. + // https://github.com/react-hook-form/react-hook-form/blob/9a497a70a/src/logic/createFormControl.ts#L1194-L1203 + const { isSubmitSuccessful: targetSubmitSuccessful } = targetForm.formState + useEffect(() => { + if (targetSubmitSuccessful) targetForm.reset(targetAndHostDefaultValues) + }, [targetSubmitSuccessful, targetForm]) // Ports const portRangeForm = useForm({ defaultValues: { portRange: '' } }) @@ -312,10 +324,13 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = if (!type || !value) return if (hosts.value.some((t) => t.value === value && t.type === type)) return hosts.onChange([...hosts.value, { type, value }]) - // TODO: something is not resetting right -- if the IP net field is set to - // validate on change due to having an error, it stays that way after submit hostForm.reset(targetAndHostDefaultValues) }) + // HACK: see comment above about doing the same for target form + const { isSubmitSuccessful: hostSubmitSuccessful } = hostForm.formState + useEffect(() => { + if (hostSubmitSuccessful) hostForm.reset(targetAndHostDefaultValues) + }, [hostSubmitSuccessful, hostForm]) return ( <> From 7fd1d1ccdadbbd55f02085be7da264c69cec4bc5 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 30 Sep 2024 14:40:25 -0500 Subject: [PATCH 14/20] really fix subform revalidation and test it like crazy --- app/forms/firewall-rules-common.tsx | 14 +++- test/e2e/firewall-rules.e2e.ts | 122 +++++++++++++++++++++++++++- 2 files changed, 133 insertions(+), 3 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 72ed6998d0..19ad51a7d2 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -434,7 +434,12 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = control={targetForm.control} valueType={targetType} items={targetItems[targetType]} - onTypeChange={() => targetForm.resetField('value')} + // HACK: reset the whole subform, keeping type (because we just set + // it). most importantly, this resets isSubmitted so the form can go + // back to validating on submit instead of change + onTypeChange={() => + targetForm.reset({ type: targetForm.getValues('type'), value: '' }) + } onInputChange={(value) => targetForm.setValue('value', value)} onSubmitTextField={submitTarget} /> @@ -541,7 +546,12 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = control={hostForm.control} valueType={hostType} items={hostFilterItems[hostType]} - onTypeChange={() => targetForm.resetField('value')} + // HACK: reset the whole subform, keeping type (because we just set + // it). most importantly, this resets isSubmitted so the form can go + // back to validating on submit instead of change + onTypeChange={() => + hostForm.reset({ type: hostForm.getValues('type'), value: '' }) + } onInputChange={(value) => hostForm.setValue('value', value)} onSubmitTextField={submitHost} /> diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 7046fedbd8..9c27be663b 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -156,7 +156,8 @@ test('firewall rule form targets table', async ({ page }) => { await page.getByRole('link', { name: 'New rule' }).click() const targets = page.getByRole('table', { name: 'Targets' }) - const targetVpcNameField = page.getByRole('combobox', { name: 'VPC name' }).nth(0) + const targetVpcNameField = page.getByRole('combobox', { name: 'VPC name' }).first() + const addButton = page.getByRole('button', { name: 'Add target' }) // add targets with overlapping names and types to test delete @@ -204,6 +205,125 @@ test('firewall rule form targets table', async ({ page }) => { await expectRowVisible(targets, { Type: 'ip', Value: '192.168.0.1' }) }) +test('firewall rule form target validation', async ({ page }) => { + await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new') + + const addButton = page.getByRole('button', { name: 'Add target' }) + const targets = page.getByRole('table', { name: 'Targets' }) + + const formModal = page.getByRole('dialog', { name: 'Add firewall rule' }) + const nameError = formModal.getByText('Must end with a letter or number') + + // Enter invalid VPC name + const vpcNameField = page.getByRole('combobox', { name: 'VPC name' }).first() + await vpcNameField.fill('ab-') + await addButton.click() + await expect(nameError).toBeVisible() + + // Change to IP, error should disappear and value field should be empty + await selectOption(page, 'Target type', 'IP') + await expect(nameError).toBeHidden() + const ipField = page.getByRole('textbox', { name: 'IP address' }) + await expect(ipField).toHaveValue('') + + // Type '1', error should not appear immediately (back to validating onSubmit) + await ipField.fill('1') + const ipError = formModal.getByText('Not a valid IP address') + await expect(ipError).toBeHidden() + await addButton.click() + await expect(ipError).toBeVisible() + + // Change back to VPC, enter valid value + await selectOption(page, 'Target type', 'VPC') + await expect(ipError).toBeHidden() + await expect(nameError).toBeHidden() + await vpcNameField.fill('abc') + await addButton.click() + await expectRowVisible(targets, { Type: 'vpc', Value: 'abc' }) + + // Switch to IP again + await selectOption(page, 'Target type', 'IP') + await ipField.fill('1') + + // No error means we're back to validating on submit + await expect(ipError).toBeHidden() + + // Hit submit to get error + await addButton.click() + await expect(ipError).toBeVisible() + + // Fill out valid IP and submit + await ipField.fill('1.1.1.1') + await addButton.click() + await expect(ipError).toBeHidden() + await expectRowVisible(targets, { Type: 'ip', Value: '1.1.1.1' }) +}) + +// This test may appear redundant because host and target share their logic, but +// testing them separately is still valuable because we want to make sure we're +// hooking up the shared code correctly and we don't break them if we refactor +// that code. + +test('firewall rule form host validation', async ({ page }) => { + await page.goto('/projects/mock-project/vpcs/mock-vpc/firewall-rules-new') + + const addButton = page.getByRole('button', { name: 'Add host filter' }) + const hosts = page.getByRole('table', { name: 'Host filters' }) + + const formModal = page.getByRole('dialog', { name: 'Add firewall rule' }) + const nameError = formModal.getByText('Must end with a letter or number') + + // Enter invalid VPC name + const vpcNameField = page.getByRole('combobox', { name: 'VPC name' }).nth(1) + await vpcNameField.fill('ab-') + await addButton.click() + await expect(nameError).toBeVisible() + + // Change to IP, error should disappear and value field should be empty + await selectOption(page, 'Host type', 'IP') + await expect(nameError).toBeHidden() + const ipField = page.getByRole('textbox', { name: 'IP address' }) + await expect(ipField).toHaveValue('') + + // Type '1', error should not appear immediately (back to validating onSubmit) + await ipField.fill('1') + const ipError = formModal.getByText('Not a valid IP address') + await expect(ipError).toBeHidden() + await addButton.click() + await expect(ipError).toBeVisible() + + // Change back to VPC, enter valid value + await selectOption(page, 'Host type', 'VPC') + await expect(ipError).toBeHidden() + await expect(nameError).toBeHidden() + await vpcNameField.fill('abc') + await addButton.click() + await expectRowVisible(hosts, { Type: 'vpc', Value: 'abc' }) + + // use subnet to be slightly different from the target validation test + + // Switch to IP subnet + await selectOption(page, 'Host type', 'IP subnet') + const ipSubnetField = page.getByRole('textbox', { name: 'IP network' }) + await ipSubnetField.fill('1') + + // No error means we're back to validating on submit + const ipNetError = formModal.getByText( + 'Must contain an IP address and a width, separated by a /' + ) + await expect(ipNetError).toBeHidden() + + // Hit submit to get error + await addButton.click() + await expect(ipNetError).toBeVisible() + + // Fill out valid IP and submit + await ipSubnetField.fill('1.1.1.1/1') + await addButton.click() + await expect(ipNetError).toBeHidden() + await expectRowVisible(hosts, { Type: 'ip_net', Value: '1.1.1.1/1' }) +}) + test('firewall rule form hosts table', async ({ page }) => { await page.goto('/projects/mock-project/vpcs/mock-vpc') await page.getByRole('tab', { name: 'Firewall Rules' }).click() From a048e32dc53bbd198381af0260b631c4476c2c2e Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 30 Sep 2024 23:42:07 -0500 Subject: [PATCH 15/20] add test for transit IPs form, incorporate PR #2480 https://github.com/oxidecomputer/console/pull/2480 --- app/forms/network-interface-edit.tsx | 12 +++--- test/e2e/instance-networking.e2e.ts | 57 ++++++++++++++++++++++++++++ 2 files changed, 64 insertions(+), 5 deletions(-) diff --git a/app/forms/network-interface-edit.tsx b/app/forms/network-interface-edit.tsx index d7f1ee62f6..be9623230f 100644 --- a/app/forms/network-interface-edit.tsx +++ b/app/forms/network-interface-edit.tsx @@ -57,10 +57,13 @@ export function EditNetworkInterfaceForm({ const transitIps = form.watch('transitIps') || [] const transitIpsForm = useForm({ defaultValues: { transitIp: '' } }) + const transitIpValue = transitIpsForm.watch('transitIp').trim() const submitTransitIp = transitIpsForm.handleSubmit(({ transitIp }) => { - if (!transitIp) return - form.setValue('transitIps', [...transitIps, transitIp]) + // only add the IP if it isn't already in the list of transit IPs + if (!transitIps.includes(transitIp)) { + form.setValue('transitIps', [...transitIps, transitIp]) + } transitIpsForm.reset() }) @@ -113,9 +116,8 @@ export function EditNetworkInterfaceForm({
transitIpsForm.reset()} onSubmit={submitTransitIp} /> diff --git a/test/e2e/instance-networking.e2e.ts b/test/e2e/instance-networking.e2e.ts index 2ae4fb4ef3..d89a74fb10 100644 --- a/test/e2e/instance-networking.e2e.ts +++ b/test/e2e/instance-networking.e2e.ts @@ -153,3 +153,60 @@ test('Instance networking tab — floating IPs', async ({ page }) => { // And that button should be enabled again await expect(attachFloatingIpButton).toBeEnabled() }) + +test('Edit network interface - Transit IPs', async ({ page }) => { + await page.goto('/projects/mock-project/instances/db1/networking') + + // Stop the instance to enable editing + await stopInstance(page) + + await clickRowAction(page, 'my-nic', 'Edit') + + const modal = page.getByRole('dialog', { name: 'Edit network interface' }) + const transitIpField = modal.getByRole('textbox', { name: 'Transit IPs' }) + const addTransitIpButton = modal.getByRole('button', { name: 'Add Transit IP' }) + const clearButton = modal.getByRole('button', { name: 'Clear' }) + const errorMessage = modal.getByText( + 'Must contain an IP address and a width, separated by a /' + ) + const transitIpsTable = modal.getByRole('table', { name: 'Transit IPs' }) + + // Type invalid value + await transitIpField.fill('invalid-ip') + await expect(errorMessage).toBeHidden() + + // Hit add transit IP button + await addTransitIpButton.click() + await expect(errorMessage).toBeVisible() + + // Clear the value and error + await clearButton.click() + await expect(errorMessage).toBeHidden() + await expect(transitIpField).toHaveValue('') + + // Type bad value again + await transitIpField.fill('invalid-ip') + await expect(errorMessage).toBeHidden() + + // Hit add again to get the error + await addTransitIpButton.click() + await expect(errorMessage).toBeVisible() + + // Change to valid IP network + await transitIpField.fill('192.168.0.0/16') + await expect(errorMessage).toBeHidden() + + // Submit and assert it's in the table + await addTransitIpButton.click() + await expect(transitIpField).toHaveValue('') + await expectRowVisible(transitIpsTable, { 'Transit IPs': '192.168.0.0/16' }) + + // Submit the form + await modal.getByRole('button', { name: 'Update network interface' }).click() + + // Assert the transit IP is in the NICs table + const nicTable = page.getByRole('table', { name: 'Network interfaces' }) + await expectRowVisible(nicTable, { 'Transit IPs': '172.30.0.0/22+1' }) + // TODO: now hover over the +1 to see the one we just added + // await page.getByText('Other IPs 192.168.0.0/16') +}) From c554f31dad29feef0d8a57e80c8cd08f89c0c0f4 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Oct 2024 11:20:56 -0500 Subject: [PATCH 16/20] validate transit ip dupes in validate function --- app/forms/network-interface-edit.tsx | 13 ++++++++----- test/e2e/instance-networking.e2e.ts | 8 ++++++++ 2 files changed, 16 insertions(+), 5 deletions(-) diff --git a/app/forms/network-interface-edit.tsx b/app/forms/network-interface-edit.tsx index be9623230f..1db347bbc4 100644 --- a/app/forms/network-interface-edit.tsx +++ b/app/forms/network-interface-edit.tsx @@ -60,10 +60,8 @@ export function EditNetworkInterfaceForm({ const transitIpValue = transitIpsForm.watch('transitIp').trim() const submitTransitIp = transitIpsForm.handleSubmit(({ transitIp }) => { - // only add the IP if it isn't already in the list of transit IPs - if (!transitIps.includes(transitIp)) { - form.setValue('transitIps', [...transitIps, transitIp]) - } + // validate has already checked to make sure it's not in the list + form.setValue('transitIps', [...transitIps, transitIp]) transitIpsForm.reset() }) @@ -110,7 +108,12 @@ export function EditNetworkInterfaceForm({ submitTransitIp() } }} - validate={validateIpNet} + validate={(value) => { + const error = validateIpNet(value) + if (error) return error + + if (transitIps.includes(value)) return 'Transit IP already in list' + }} placeholder="Enter an IP network" /> diff --git a/test/e2e/instance-networking.e2e.ts b/test/e2e/instance-networking.e2e.ts index d89a74fb10..115fffeb27 100644 --- a/test/e2e/instance-networking.e2e.ts +++ b/test/e2e/instance-networking.e2e.ts @@ -201,6 +201,14 @@ test('Edit network interface - Transit IPs', async ({ page }) => { await expect(transitIpField).toHaveValue('') await expectRowVisible(transitIpsTable, { 'Transit IPs': '192.168.0.0/16' }) + const dupeError = modal.getByText('Transit IP already in list') + + // try to add the same one again to see the dupe message + await transitIpField.fill('192.168.0.0/16') + await expect(dupeError).toBeHidden() + await addTransitIpButton.click() + await expect(dupeError).toBeVisible() + // Submit the form await modal.getByRole('button', { name: 'Update network interface' }).click() From 26ee9a97532c299554dd6e902a3274e03832c2a2 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Oct 2024 11:30:31 -0500 Subject: [PATCH 17/20] tests that catch clear button bug, fix bug --- app/forms/firewall-rules-common.tsx | 14 ++++++-------- test/e2e/firewall-rules.e2e.ts | 12 ++++++++++++ 2 files changed, 18 insertions(+), 8 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 19ad51a7d2..586a55d21a 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -261,8 +261,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = // Targets const targetForm = useForm({ defaultValues: targetAndHostDefaultValues }) const targets = useController({ name: 'targets', control }).field - const targetType = targetForm.watch('type') - const targetValue = targetForm.watch('value') + const [targetType, targetValue] = targetForm.watch(['type', 'value']) // get the list of items that are not already in the list of targets const targetItems = { vpc: availableItems(targets.value, 'vpc', vpcs), @@ -308,8 +307,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = // Host Filters const hostForm = useForm({ defaultValues: targetAndHostDefaultValues }) const hosts = useController({ name: 'hosts', control }).field - const hostType = hostForm.watch('type') - const hostValue = hostForm.watch('value') + const [hostType, hostValue] = hostForm.watch(['type', 'value']) // get the list of items that are not already in the list of host filters const hostFilterItems = { vpc: availableItems(hosts.value, 'vpc', vpcs), @@ -445,7 +443,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = /> targetForm.reset()} onSubmit={submitTarget} /> @@ -496,8 +494,8 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = portRangeForm.reset()} onSubmit={submitPortRange} /> @@ -557,7 +555,7 @@ export const CommonFields = ({ control, nameTaken, error }: CommonFieldsProps) = /> hostForm.reset()} onSubmit={submitHost} /> diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 9c27be663b..44f3ba7731 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -67,6 +67,10 @@ test('can create firewall rule', async ({ page }) => { await addPortButton.click() await expect(invalidPort).toBeVisible() + // test clear button + await page.getByRole('button', { name: 'Clear' }).nth(1).click() + await expect(portRangeField).toHaveValue('') + await portRangeField.fill('123-456') await addPortButton.click() await expect(invalidPort).toBeHidden() @@ -233,6 +237,10 @@ test('firewall rule form target validation', async ({ page }) => { await addButton.click() await expect(ipError).toBeVisible() + // test clear button + await page.getByRole('button', { name: 'Clear' }).first().click() + await expect(ipField).toHaveValue('') + // Change back to VPC, enter valid value await selectOption(page, 'Target type', 'VPC') await expect(ipError).toBeHidden() @@ -292,6 +300,10 @@ test('firewall rule form host validation', async ({ page }) => { await addButton.click() await expect(ipError).toBeVisible() + // test clear button + await page.getByRole('button', { name: 'Clear' }).nth(2).click() + await expect(ipField).toHaveValue('') + // Change back to VPC, enter valid value await selectOption(page, 'Host type', 'VPC') await expect(ipError).toBeHidden() From 92c0d8d1b2d35791d7b891854ebad31fb9b5a74f Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Oct 2024 11:42:10 -0500 Subject: [PATCH 18/20] have to do the reset on success effect for transit IPs too --- app/forms/network-interface-edit.tsx | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/forms/network-interface-edit.tsx b/app/forms/network-interface-edit.tsx index 1db347bbc4..c57bde7899 100644 --- a/app/forms/network-interface-edit.tsx +++ b/app/forms/network-interface-edit.tsx @@ -5,6 +5,7 @@ * * Copyright Oxide Computer Company */ +import { useEffect } from 'react' import { useForm } from 'react-hook-form' import * as R from 'remeda' @@ -57,7 +58,8 @@ export function EditNetworkInterfaceForm({ const transitIps = form.watch('transitIps') || [] const transitIpsForm = useForm({ defaultValues: { transitIp: '' } }) - const transitIpValue = transitIpsForm.watch('transitIp').trim() + const transitIpValue = transitIpsForm.watch('transitIp') + const { isSubmitSuccessful: transitIpSubmitSuccessful } = transitIpsForm.formState const submitTransitIp = transitIpsForm.handleSubmit(({ transitIp }) => { // validate has already checked to make sure it's not in the list @@ -65,6 +67,10 @@ export function EditNetworkInterfaceForm({ transitIpsForm.reset() }) + useEffect(() => { + if (transitIpSubmitSuccessful) transitIpsForm.reset() + }, [transitIpSubmitSuccessful, transitIpsForm]) + return ( Date: Tue, 1 Oct 2024 12:22:49 -0500 Subject: [PATCH 19/20] test IP validation in route form --- test/e2e/vpcs.e2e.ts | 118 +++++++++++++++++++++++++++++++------------ 1 file changed, 85 insertions(+), 33 deletions(-) diff --git a/test/e2e/vpcs.e2e.ts b/test/e2e/vpcs.e2e.ts index 331337132e..2e6ea3b466 100644 --- a/test/e2e/vpcs.e2e.ts +++ b/test/e2e/vpcs.e2e.ts @@ -218,51 +218,103 @@ test('can’t create or delete Routes on system routers', async ({ page }) => { await expect(page.getByRole('menuitem', { name: 'Delete' })).toBeDisabled() }) -test('can create, update, and delete Route', async ({ page }) => { - // go to the custom-router-2 page +test('create router route', async ({ page }) => { await page.goto('/projects/mock-project/vpcs/mock-vpc/routers/mock-custom-router') - const table = page.getByRole('table') - const routeRows = table.locator('tbody >> tr') - await expectRowVisible(table, { Name: 'drop-local' }) + + // Selectors + const form = page.getByRole('dialog', { name: 'Create route' }) + const nameInput = page.getByRole('textbox', { name: 'Name' }) + const destinationValueInput = page.getByRole('textbox', { name: 'Destination value' }) + const targetValueInput = page.getByRole('textbox', { name: 'Target value' }) + const submitButton = page.getByRole('button', { name: 'Create route' }) + + const invalidIpError = form.getByText('Not a valid IP address') + const invalidIpNetError = form.getByText( + 'Must contain an IP address and a width, separated by a /' + ) // create a new route - await page.click('text=New route') - await page.getByRole('textbox', { name: 'name' }).fill('new-route') - await page.getByRole('textbox', { name: 'Destination value' }).fill('0.0.0.0') - - // we'll set the target in a second, but first verify that selecting internet gateway disables the value - await page.getByRole('button', { name: 'Target type' }).click() - await page.getByRole('option', { name: 'Internet gateway' }).click() - await expect(page.getByRole('textbox', { name: 'Target value' })).toBeDisabled() - await expect(page.getByRole('textbox', { name: 'Target value' })).toHaveValue('outbound') - await page.getByRole('button', { name: 'Target type' }).click() - await page.getByRole('option', { name: 'IP' }).click() - await page.getByRole('textbox', { name: 'Target value' }).fill('1.1.1.1') - await page.getByRole('button', { name: 'Create route' }).click() - await expect(routeRows).toHaveCount(2) - await expectRowVisible(table, { + await page.getByRole('link', { name: 'New route' }).click() + await nameInput.fill('new-route') + + // Test IP validation for destination + await selectOption(page, 'Destination type', 'IP') + await destinationValueInput.fill('invalid-ip') + await expect(invalidIpError).toBeHidden() + await submitButton.click() + // other field was left empty, so is also invalid + await expect(invalidIpError).toHaveCount(2) + + // change target to valid, error disappear + await targetValueInput.fill('192.168.0.2') + await expect(invalidIpError).toHaveCount(1) + + // Test IP net validation for destination + await selectOption(page, 'Destination type', 'IP network') + // error on dest value clears on type change + await expect(invalidIpError).toBeHidden() + await destinationValueInput.fill('invalid-ip-net') + await expect(invalidIpError).toBeHidden() + await submitButton.click() + await expect(invalidIpNetError).toBeVisible() + + // revalidates on change + await destinationValueInput.fill('192.168.0.0/24') + await expect(invalidIpNetError).toBeHidden() + + // Set target + await selectOption(page, 'Target type', 'IP') + await targetValueInput.fill('10.0.0.1') + + await submitButton.click() + await expect(form).toBeHidden() + await expectRowVisible(page.getByRole('table'), { Name: 'new-route', - Destination: 'IP0.0.0.0', - Target: 'IP1.1.1.1', + Destination: 'IP network192.168.0.0/24', + Target: 'IP10.0.0.1', }) +}) + +test('edit and delete router route', async ({ page }) => { + await page.goto('/projects/mock-project/vpcs/mock-vpc/routers/mock-custom-router') - // update the route by clicking the edit button - await clickRowAction(page, 'new-route', 'Edit') - // change the destination type to VPC subnet: `mock-subnet` + const form = page.getByRole('dialog', { name: 'Edit route' }) + await expect(form).toBeHidden() + + await clickRowAction(page, 'drop-local', 'Edit') + await expect(form).toBeVisible() + + const targetValueInput = page.getByRole('textbox', { name: 'Target value' }) + const submitButton = page.getByRole('button', { name: 'Update route' }) + + await form.getByRole('textbox', { name: 'Name' }).fill('new-name') + + // Test IP validation for target + await selectOption(page, 'Target type', 'IP') + await targetValueInput.fill('invalid-ip') + await submitButton.click() + await expect(form.getByText('Not a valid IP address')).toBeVisible() + + await targetValueInput.fill('10.0.0.2') + await expect(form.getByText('Not a valid IP address')).toBeHidden() + + // Change destination to subnet await selectOption(page, 'Destination type', 'Subnet') await selectOption(page, 'Destination value', 'mock-subnet') - await page.getByRole('textbox', { name: 'Target value' }).fill('0.0.0.1') - await page.getByRole('button', { name: 'Update route' }).click() - await expect(routeRows).toHaveCount(2) + + await submitButton.click() + await expect(form).toBeHidden() + + const table = page.getByRole('table') await expectRowVisible(table, { - Name: 'new-route', + Name: 'new-name', Destination: 'VPC subnetmock-subnet', - Target: 'IP0.0.0.1', + Target: 'IP10.0.0.2', }) // delete the route - await clickRowAction(page, 'new-route', 'Delete') + await clickRowAction(page, 'new-name', 'Delete') await page.getByRole('button', { name: 'Confirm' }).click() - await expect(routeRows).toHaveCount(1) - await expect(page.getByRole('row', { name: 'new-route' })).toBeHidden() + await expect(table).toBeHidden() + await expect(page.getByText('No routes')).toBeVisible() }) From fb160916988b9e53e393d0e77bded0f085492f28 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 1 Oct 2024 14:43:16 -0500 Subject: [PATCH 20/20] polish up last two todos to do two twos too --- app/forms/firewall-rules-common.tsx | 5 ++++- test/e2e/instance-networking.e2e.ts | 7 +++++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/app/forms/firewall-rules-common.tsx b/app/forms/firewall-rules-common.tsx index 586a55d21a..abb0992d34 100644 --- a/app/forms/firewall-rules-common.tsx +++ b/app/forms/firewall-rules-common.tsx @@ -134,7 +134,10 @@ const DynamicTypeAndValueFields = ({ allowArbitraryValues hideOptionalTag validate={(value) => - // TODO: is required false correct here? should this function even have that argument? + // required: false arg is desirable because otherwise if you put in + // a bad name and submit, causing it to revalidate onChange, then + // clear the field you're left with a BS "Target name is required" + // error message validateName(value, `${capitalize(sectionType)} name`, false) } /> diff --git a/test/e2e/instance-networking.e2e.ts b/test/e2e/instance-networking.e2e.ts index 115fffeb27..26b868a751 100644 --- a/test/e2e/instance-networking.e2e.ts +++ b/test/e2e/instance-networking.e2e.ts @@ -215,6 +215,9 @@ test('Edit network interface - Transit IPs', async ({ page }) => { // Assert the transit IP is in the NICs table const nicTable = page.getByRole('table', { name: 'Network interfaces' }) await expectRowVisible(nicTable, { 'Transit IPs': '172.30.0.0/22+1' }) - // TODO: now hover over the +1 to see the one we just added - // await page.getByText('Other IPs 192.168.0.0/16') + + await page.getByText('+1').hover() + await expect( + page.getByRole('tooltip', { name: 'Other transit IPs 192.168.0.0/16' }) + ).toBeVisible() })