Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
65 changes: 9 additions & 56 deletions app/forms/ip-pool-range-add.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*
* Copyright Oxide Computer Company
*/
import { useForm, type FieldErrors } from 'react-hook-form'
import { useForm } from 'react-hook-form'
import { useNavigate } from 'react-router'

import {
Expand Down Expand Up @@ -34,57 +34,11 @@ const defaultValues: IpRange = {
last: '',
}

// Using a resolver overrides all field-level validation (required, min, max,
// etc.), so this function must cover everything. Field-level `required` props
// still affect UI display (hiding the "optional" label) but are inert for
// validation.

/**
* Validates IP range addresses against the pool's IP version.
* Ensures both addresses are valid IPs and match the pool's version.
*/
function createResolver(poolVersion: IpVersion) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this was originally a resolver because pools were not locked to a single IP version, so we had to make sure the start and end were of the same version. Now we can just compare each one individually to the version of the pool. So this can all just be a field-level validation. This change does not rely on form-level validation, this was just discovered while looking for candidates for form-level validation.

return (values: IpRange) => {
const first = parseIp(values.first)
const last = parseIp(values.last)

const errors: FieldErrors<IpRange> = {}

// Validate first address matches pool version
if (first.type === 'error') {
errors.first = { type: 'pattern', message: first.message }
} else if (first.type === 'v4' && poolVersion === 'v6') {
errors.first = {
type: 'pattern',
message: 'IPv4 address not allowed in IPv6 pool',
}
} else if (first.type === 'v6' && poolVersion === 'v4') {
errors.first = {
type: 'pattern',
message: 'IPv6 address not allowed in IPv4 pool',
}
}

// Validate last address matches pool version
if (last.type === 'error') {
errors.last = { type: 'pattern', message: last.message }
} else if (last.type === 'v4' && poolVersion === 'v6') {
errors.last = {
type: 'pattern',
message: 'IPv4 address not allowed in IPv6 pool',
}
} else if (last.type === 'v6' && poolVersion === 'v4') {
errors.last = {
type: 'pattern',
message: 'IPv6 address not allowed in IPv4 pool',
}
}

// TODO: if we were really cool we could check first <= last but it would add
// 6k gzipped to the bundle with ip-num

// no errors
return Object.keys(errors).length > 0 ? { values: {}, errors } : { values, errors: {} }
const validateAddress = (value: string, poolVersion: IpVersion) => {
const parsed = parseIp(value)
if (parsed.type === 'error') return parsed.message
if (parsed.type !== poolVersion) {
return `IP${parsed.type} address not allowed in IP${poolVersion} pool`
}
}

Expand All @@ -107,10 +61,7 @@ export default function IpPoolAddRange() {
},
})

const form = useForm({
defaultValues,
resolver: createResolver(poolData.ipVersion),
})
const form = useForm({ defaultValues })

return (
<SideModalForm
Expand All @@ -132,12 +83,14 @@ export default function IpPoolAddRange() {
description="First address in the range"
control={form.control}
required
validate={(value) => validateAddress(value, poolData.ipVersion)}
/>
<TextField
name="last"
description="Last address in the range"
control={form.control}
required
validate={(value) => validateAddress(value, poolData.ipVersion)}
/>
<SideModalFormDocs docs={[docLinks.systemIpPools]} />
</SideModalForm>
Expand Down
44 changes: 22 additions & 22 deletions app/forms/subnet-pool-member-add.spec.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,91 +7,91 @@
*/
import { describe, expect, it } from 'vitest'

import { createResolver } from './subnet-pool-member-add'
import { validateForm } from './subnet-pool-member-add'

const resolve = createResolver('v4')
const resolve6 = createResolver('v6')
const validate = (values: Parameters<typeof validateForm>[1]) => validateForm('v4', values)
const validate6 = (values: Parameters<typeof validateForm>[1]) => validateForm('v6', values)

const valid = { subnet: '10.0.0.0/16', minPrefixLength: 20, maxPrefixLength: 28 }

type Result = ReturnType<typeof resolve>
type Field = 'subnet' | 'minPrefixLength' | 'maxPrefixLength'

function errMsg(result: Result, field: keyof Result['errors']) {
return result.errors[field]?.message
function errMsg(result: ReturnType<typeof validate>, field: Field) {
return result === true ? undefined : result[field]?.message
}

describe('createResolver', () => {
describe('validateForm', () => {
it('accepts valid v4 input', () => {
expect(Object.keys(resolve(valid).errors)).toEqual([])
expect(validate(valid)).toBe(true)
})

it('accepts valid v6 input', () => {
const result = resolve6({
const result = validate6({
subnet: 'fd00:1000::/32',
minPrefixLength: 48,
maxPrefixLength: 64,
})
expect(Object.keys(result.errors)).toEqual([])
expect(result).toBe(true)
})

it('accepts omitted prefix lengths', () => {
const result = resolve({
const result = validate({
subnet: '10.0.0.0/16',
minPrefixLength: NaN,
maxPrefixLength: NaN,
})
expect(Object.keys(result.errors)).toEqual([])
expect(result).toBe(true)
})

it('rejects invalid CIDR', () => {
const result = resolve({ ...valid, subnet: 'not-a-cidr' })
const result = validate({ ...valid, subnet: 'not-a-cidr' })
expect(errMsg(result, 'subnet')).toMatch(/IP address/)
})

it('rejects v6 subnet in v4 pool', () => {
const result = resolve({ ...valid, subnet: 'fd00::/32' })
const result = validate({ ...valid, subnet: 'fd00::/32' })
expect(errMsg(result, 'subnet')).toBe('IPv6 subnet not allowed in IPv4 pool')
})

it('rejects v4 subnet in v6 pool', () => {
const result = resolve6({ ...valid, subnet: '10.0.0.0/16' })
const result = validate6({ ...valid, subnet: '10.0.0.0/16' })
expect(errMsg(result, 'subnet')).toBe('IPv4 subnet not allowed in IPv6 pool')
})

it('rejects min > max prefix length', () => {
const result = resolve({ ...valid, minPrefixLength: 28, maxPrefixLength: 20 })
const result = validate({ ...valid, minPrefixLength: 28, maxPrefixLength: 20 })
expect(errMsg(result, 'minPrefixLength')).toMatch(/≤/)
})

it('rejects min prefix length < subnet width', () => {
const result = resolve({ ...valid, minPrefixLength: 8 })
const result = validate({ ...valid, minPrefixLength: 8 })
expect(errMsg(result, 'minPrefixLength')).toMatch(/≥ subnet prefix length \(16\)/)
})

it('rejects max prefix length < subnet width', () => {
const result = resolve({ ...valid, maxPrefixLength: 8 })
const result = validate({ ...valid, maxPrefixLength: 8 })
expect(errMsg(result, 'maxPrefixLength')).toMatch(/≥ subnet prefix length \(16\)/)
})

it('rejects prefix length above max bound (v4: 32)', () => {
const result = resolve({ ...valid, minPrefixLength: 33 })
const result = validate({ ...valid, minPrefixLength: 33 })
expect(errMsg(result, 'minPrefixLength')).toBe('Must be between 0 and 32')
})

it('rejects prefix length below 0', () => {
const result = resolve({ ...valid, maxPrefixLength: -1 })
const result = validate({ ...valid, maxPrefixLength: -1 })
expect(errMsg(result, 'maxPrefixLength')).toBe('Must be between 0 and 32')
})

it('shows min-≤-max error even when min is also below subnet width', () => {
// min(12) > max(10) AND min(12) < subnetWidth(16): the min-≤-max error
// should take priority over the subnet-width error
const result = resolve({ ...valid, minPrefixLength: 12, maxPrefixLength: 10 })
const result = validate({ ...valid, minPrefixLength: 12, maxPrefixLength: 10 })
expect(errMsg(result, 'minPrefixLength')).toMatch(/≤/)
})

it('rejects prefix length above max bound (v6: 128)', () => {
const result = resolve6({
const result = validate6({
subnet: 'fd00::/32',
minPrefixLength: 48,
maxPrefixLength: 200,
Expand Down
108 changes: 52 additions & 56 deletions app/forms/subnet-pool-member-add.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -5,7 +5,7 @@
*
* Copyright Oxide Computer Company
*/
import { useForm, type FieldErrors } from 'react-hook-form'
import { useForm } from 'react-hook-form'
import { useNavigate } from 'react-router'

import {
Expand Down Expand Up @@ -41,65 +41,62 @@ const defaultValues: MemberAddForm = {
maxPrefixLength: NaN,
}

// Using a resolver overrides all field-level validation (required, min, max,
// etc.), so this function must cover everything. Field-level props like
// `required` on subnet and `min`/`max` on NumberFields still affect UI display
// and stepper behavior, but their RHF validation rules are inert.
export function createResolver(poolVersion: IpVersion) {
return (values: MemberAddForm) => {
const errors: FieldErrors<MemberAddForm> = {}
const maxBound = poolVersion === 'v4' ? 32 : 128

const parsed = parseIpNet(values.subnet)
if (parsed.type === 'error') {
errors.subnet = { type: 'pattern', message: parsed.message }
} else if (parsed.type !== poolVersion) {
errors.subnet = {
type: 'pattern',
message: `IP${parsed.type} subnet not allowed in IP${poolVersion} pool`,
}
// Uses form-level validate (RHF ≥7.72.0) so we can look at all three fields
// together. Unlike `resolver`, this runs alongside field-level validation, so
// `required` / `min` / `max` on the fields still apply.
export function validateForm(poolVersion: IpVersion, values: MemberAddForm) {
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Look at this with whitespace changes hidden to see what a small change it is. Basically it's just the return type changing.

https://github.com/oxidecomputer/console/pull/3188/changes?w=1

const maxBound = poolVersion === 'v4' ? 32 : 128
const parsed = parseIpNet(values.subnet)
const { minPrefixLength: minPL, maxPrefixLength: maxPL } = values
const subnetWidth = parsed.type !== 'error' ? parsed.width : undefined
const inRange = (v: number) => !Number.isNaN(v) && v >= 0 && v <= maxBound

const errors: Partial<Record<keyof MemberAddForm, { type: string; message: string }>> = {}
Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't love this, will revisit.


if (parsed.type === 'error') {
errors.subnet = { type: 'pattern', message: parsed.message }
} else if (parsed.type !== poolVersion) {
errors.subnet = {
type: 'pattern',
message: `IP${parsed.type} subnet not allowed in IP${poolVersion} pool`,
}
}

const { minPrefixLength: minPL, maxPrefixLength: maxPL } = values
const subnetWidth = parsed.type !== 'error' ? parsed.width : undefined
const inRange = (v: number) => !Number.isNaN(v) && v >= 0 && v <= maxBound

// min and max prefix length are optional, and NaN is the value they have
// when they're unset (matching NumberField)

// min prefix: bounds → ordering → subnet width
if (!Number.isNaN(minPL) && !inRange(minPL)) {
errors.minPrefixLength = {
type: 'validate',
message: `Must be between 0 and ${maxBound}`,
}
} else if (inRange(minPL) && inRange(maxPL) && minPL > maxPL) {
errors.minPrefixLength = {
type: 'validate',
message: 'Min prefix length must be ≤ max prefix length',
}
} else if (inRange(minPL) && subnetWidth !== undefined && minPL < subnetWidth) {
errors.minPrefixLength = {
type: 'validate',
message: `Must be ≥ subnet prefix length (${subnetWidth})`,
}
}
// min and max prefix length are optional, and NaN is the value they have
// when they're unset (matching NumberField)

// max prefix: bounds → subnet width
if (!Number.isNaN(maxPL) && !inRange(maxPL)) {
errors.maxPrefixLength = {
type: 'validate',
message: `Must be between 0 and ${maxBound}`,
}
} else if (inRange(maxPL) && subnetWidth !== undefined && maxPL < subnetWidth) {
errors.maxPrefixLength = {
type: 'validate',
message: `Must be ≥ subnet prefix length (${subnetWidth})`,
}
// min prefix: bounds → ordering → subnet width
if (!Number.isNaN(minPL) && !inRange(minPL)) {
errors.minPrefixLength = {
type: 'validate',
message: `Must be between 0 and ${maxBound}`,
}
} else if (inRange(minPL) && inRange(maxPL) && minPL > maxPL) {
errors.minPrefixLength = {
type: 'validate',
message: 'Min prefix length must be ≤ max prefix length',
}
} else if (inRange(minPL) && subnetWidth !== undefined && minPL < subnetWidth) {
errors.minPrefixLength = {
type: 'validate',
message: `Must be ≥ subnet prefix length (${subnetWidth})`,
}
}

return { values: Object.keys(errors).length > 0 ? {} : values, errors }
// max prefix: bounds → subnet width
if (!Number.isNaN(maxPL) && !inRange(maxPL)) {
errors.maxPrefixLength = {
type: 'validate',
message: `Must be between 0 and ${maxBound}`,
}
} else if (inRange(maxPL) && subnetWidth !== undefined && maxPL < subnetWidth) {
errors.maxPrefixLength = {
type: 'validate',
message: `Must be ≥ subnet prefix length (${subnetWidth})`,
}
}

return Object.keys(errors).length > 0 ? errors : true
}

export const handle = titleCrumb('Add Member')
Expand All @@ -125,8 +122,7 @@ export default function SubnetPoolMemberAdd() {

const form = useForm<MemberAddForm>({
defaultValues,
// doesn't need to be memoized, doesn't trigger renders
resolver: createResolver(poolData.ipVersion),
validate: ({ formValues }) => validateForm(poolData.ipVersion, formValues),
})

const maxBound = poolData.ipVersion === 'v4' ? 32 : 128
Expand Down
8 changes: 4 additions & 4 deletions package-lock.json

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

2 changes: 1 addition & 1 deletion package.json
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,7 @@
"react-aria": "^3.44.0",
"react-dom": "^19.2.0",
"react-error-boundary": "^4.0.13",
"react-hook-form": "^7.53.0",
"react-hook-form": "^7.72.1",
"react-is": "^19.2.0",
"react-merge-refs": "^2.1.1",
"react-router": "^7.13.0",
Expand Down
Loading