Validate IPs and IP networks #2461

david-crespo · 2024-09-20T17:05:07Z

Basic parsing-based validation with ip-num
~~Confirm Rust code has the same validation results (https://github.com/oxidecomputer/test-ip-validation)~~
Abandon ip-num because it does not have the same validation results
Decide on return type of parser function
Figure out how this should be integrated into forms
Validate transit IP nets
Validate IPs and IP nets in route form
Validate IPs and IP nets in firewall rule form
Polish up weird behavior on host and target subform on type change, submit success, and clear
Write some e2e tests

vercel · 2024-09-20T17:05:12Z

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name	Status	Preview	Updated (UTC)
console	✅ Ready (Inspect)	Visit Preview	Oct 1, 2024 7:44pm

david-crespo · 2024-09-20T19:48:41Z

Ok, here's something interesting. I tried swapping in the ip-num things in our existing validateIp function and a bunch of tests fail, indicating it disagrees significantly with the Rust parsing code.

 export function validateIp(ip: string) {
-  const isv4 = IPV4_REGEX.test(ip)
-  const isv6 = !isv4 && IPV6_REGEX.test(ip)
+  let isv4 = false
+  try {
+    IPv4.fromString(ip)
+    isv4 = true
+  } catch (e) {}
+  let isv6 = false
+  if (!isv4) {
+    try {
+      IPv6.fromString(ip)
+      isv6 = true
+    } catch (e) {}
+  }
   return { isv4, isv6, valid: isv4 || isv6 }
 }

I didn't notice these differences before because my list of examples for IP networks was much less thorough than my kickass list for plain addresses. When I add the offending address to the list of IP nets, ip-num considers it valid, as expected.

Rust oxnet's Ipv4Net considers it invalid, which is expected because it splits the thing on / and uses the underlying Ipv4Addr to parse the inner IP address (source).

So, after all that excitement, I am going to take ip-num back out and use our existing IP address thing to parse an IP network. Not sure why I didn't start with that.

david-crespo · 2024-09-24T20:19:38Z

truly, we are jamming.

2024-09-24-validate-ips-and-nets.mp4

david-crespo · 2024-09-24T21:32:53Z

app/forms/network-interface-edit.tsx

    form.setValue('transitIps', [...transitIps, transitIp])
    transitIpsForm.reset()
-  }
+  })


Without wrapping this in handleSubmit, it doesn't run the validate function on submit. This way the inner function never gets called unless validation passes.

app/forms/network-interface-edit.tsx

…fields

david-crespo · 2024-09-30T20:04:38Z

app/forms/firewall-rules-common.tsx

+          // back to validating on submit instead of change
+          onTypeChange={() =>
+            hostForm.reset({ type: hostForm.getValues('type'), value: '' })
+          }


I kind of hate this, but it does pretty elegantly express what I want to happen here, namely that we are basically starting over from scratch whenever we change the type. Very weird combination of feeling both wrong and exactly right.

#2480

david-crespo · 2024-10-01T15:01:32Z

Still need to add another test or two and fix the clear button bug in the firewall rules inputs, but this is mostly ready to review. It's crazy long but most of the length is tests and fresh additions.

david-crespo

Added some explanations.

david-crespo · 2024-10-01T17:24:56Z

app/components/form/fields/ComboboxField.tsx

  name: TName
  control: Control<TFieldValues>
  onChange?: (value: string | null | undefined) => void
+  validate?: Validate<FieldPathValue<TFieldValues, TName>, TFieldValues>


Adding this lets us validate names in arbitrary-value comboboxes.

david-crespo · 2024-10-01T17:26:24Z

app/util/ip.spec.ts

+
+test.each(invalid)('parseIp catches invalid IP: %s', (s) => {
+  expect(parseIp(s)).toStrictEqual({ type: 'error', message: 'Not a valid IP address' })
+})


From here up, the tests are basically unchanged, I just had to extract the lists of IPs so I could reuse them in the IP net tests.

david-crespo · 2024-10-01T17:27:02Z

app/util/ip.ts

+  /^(?:(?:[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


regexes are unchanged, the point was to distinguish between parse and validate and to add the IP net things

david-crespo · 2024-10-01T17:46:28Z

app/forms/firewall-rules-common.tsx

+          validate={(value) =>
+            // TODO: is required false correct here? should this function even have that argument?
+            validateName(value, `${capitalize(sectionType)} name`, false)
+          }


I think required: false is correct because we don't want this to show a required error when it's left empty after a bad input was attempted.

david-crespo · 2024-10-01T17:46:55Z

app/forms/firewall-rules-common.tsx

    if (targets.value.some((t) => t.value === value && t.type === type)) return
    targets.onChange([...targets.value, { type, value }])
-    targetForm.reset()
+    targetForm.reset(targetAndHostDefaultValues)


Fixed by useEffect below.

david-crespo · 2024-10-01T17:47:22Z

app/forms/firewall-rules-common.tsx

        />
        <MiniTable.ClearAndAddButtons
          addButtonCopy="Add host filter"
-          disableClear={!!hostValue}


david-crespo · 2024-10-01T17:47:49Z

app/forms/ip-pool-range-add.tsx

-  } else if (last.isv6) {
+  if (last.type === 'error') {
+    errors.last = { type: 'pattern', message: last.message }
+  } else if (last.type === 'v6') {


Nothing actually changes in this file, so I didn't change the tests.

david-crespo · 2024-10-01T17:48:31Z

app/forms/network-interface-edit.tsx

+              if (error) return error
+
+              if (transitIps.includes(value)) return 'Transit IP already in list'
+            }}


Doing this here instead of in the handleSubmit means we get a beautiful validation error inline on attempts to add a dupe.

charliepark · 2024-10-01T20:45:52Z

app/forms/firewall-rules-common.tsx

  const targets = useController({ name: 'targets', control }).field
-  const targetType = targetForm.watch('type')
-  const targetValue = targetForm.watch('value')
+  const [targetType, targetValue] = targetForm.watch(['type', 'value'])


charliepark · 2024-10-01T20:48:23Z

app/forms/firewall-rules-common.tsx

+  const { isSubmitSuccessful: targetSubmitSuccessful } = targetForm.formState
+  useEffect(() => {
+    if (targetSubmitSuccessful) targetForm.reset(targetAndHostDefaultValues)
+  }, [targetSubmitSuccessful, targetForm])


That's a clever workaround. A shame that we need to distinguish between the two flows, but premature validation errors are even worse. Nevertheless, this is clever.

I'm thinking there is probably a way to wrap this up into some kind of subform helper hook.

charliepark

Looks great!

vercel bot deployed to Preview September 20, 2024 19:33 View deployment

david-crespo force-pushed the validate-ip-net branch from 2603284 to 1ca36d7 Compare September 20, 2024 20:26

vercel bot deployed to Preview September 20, 2024 20:26 View deployment

david-crespo added 2 commits September 24, 2024 14:23

move ip utils to their own file

ed1f486

validateIpNet util based one existing validateIp

72b506f

david-crespo force-pushed the validate-ip-net branch from 1ca36d7 to 72b506f Compare September 24, 2024 19:23

vercel bot deployed to Preview September 24, 2024 19:23 View deployment

david-crespo added 2 commits September 24, 2024 14:33

let's try a more interesting result type on the validator

7edb0f3

validate IPs and IP nets on the route form

02a20d1

vercel bot deployed to Preview September 24, 2024 20:18 View deployment

redo validateIp to look more like validateIpNet, esp. including message

ee03705

vercel bot deployed to Preview September 24, 2024 20:50 View deployment

vercel bot deployed to Preview September 24, 2024 21:08 View deployment

tweak validation logic to get more helpful messages, test it

1cce7b4

david-crespo force-pushed the validate-ip-net branch from 9e44d64 to 1cce7b4 Compare September 24, 2024 21:13

vercel bot deployed to Preview September 24, 2024 21:13 View deployment

validate transit IPs on network interface edit

22747cb

vercel bot deployed to Preview September 24, 2024 21:32 View deployment

david-crespo commented Sep 24, 2024

View reviewed changes

app/forms/network-interface-edit.tsx Show resolved Hide resolved

split parse from validate, it's clean!

56d872b

vercel bot deployed to Preview September 24, 2024 21:50 View deployment

fix e2e failure but it raises existential questions

da65a40

vercel bot deployed to Preview September 24, 2024 22:13 View deployment

david-crespo added 2 commits September 25, 2024 00:00

test leading zeroes in width

feeedd0

pass noop validate functions to comboboxes that get swapped with text…

48f2985

…fields

vercel bot deployed to Preview September 25, 2024 18:36 View deployment

vercel bot deployed to Preview September 27, 2024 21:34 View deployment

charliepark mentioned this pull request Sep 27, 2024

Update id for combobox and listbox #2470

Closed

really fix subform revalidation and test it like crazy

7fd1d1c

vercel bot deployed to Preview September 30, 2024 20:03 View deployment

david-crespo commented Sep 30, 2024

View reviewed changes

add test for transit IPs form, incorporate PR #2480

a048e32

#2480

vercel bot deployed to Preview October 1, 2024 04:42 View deployment

david-crespo mentioned this pull request Oct 1, 2024

Fix disableClear bug in Transit IP form; don't add duplicate IPs to list #2480

Closed

david-crespo marked this pull request as ready for review October 1, 2024 15:00

david-crespo added 2 commits October 1, 2024 11:22

validate transit ip dupes in validate function

c554f31

tests that catch clear button bug, fix bug

26ee9a9

vercel bot deployed to Preview October 1, 2024 16:31 View deployment

have to do the reset on success effect for transit IPs too

92c0d8d

vercel bot deployed to Preview October 1, 2024 16:42 View deployment

test IP validation in route form

b6bb51a

vercel bot deployed to Preview October 1, 2024 17:23 View deployment

david-crespo requested a review from charliepark October 1, 2024 17:24

david-crespo changed the title ~~Validation utilities for IP networks~~ Validate IPs and IP networks Oct 1, 2024

david-crespo commented Oct 1, 2024

View reviewed changes

polish up last two todos to do two twos too

fb16091

vercel bot deployed to Preview October 1, 2024 19:44 View deployment

charliepark reviewed Oct 1, 2024

View reviewed changes

charliepark approved these changes Oct 1, 2024

View reviewed changes

david-crespo merged commit dec4849 into main Oct 1, 2024

david-crespo deleted the validate-ip-net branch October 1, 2024 21:13

david-crespo mentioned this pull request Oct 3, 2024

Bump web console (boot disk, transit IPs, validate IPs) oxidecomputer/omicron#6765

Merged

Validate IPs and IP networks #2461

Validate IPs and IP networks #2461

Uh oh!

Conversation

david-crespo commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

vercel bot commented Sep 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

david-crespo commented Sep 20, 2024

Uh oh!

david-crespo commented Sep 24, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

david-crespo commented Oct 1, 2024

Uh oh!

david-crespo left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

charliepark left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

david-crespo commented Sep 20, 2024 •

edited

Loading

vercel bot commented Sep 20, 2024 •

edited

Loading

david-crespo commented Sep 24, 2024 •

edited

Loading