Skip to content

Firewall rules create/edit modal #630

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 27 commits into from
Feb 9, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
62e6e65
stub out firewall rule form based on API (needs design pass)
david-crespo Feb 1, 2022
a8a9d49
msw endpoint for get rules
david-crespo Feb 2, 2022
9882b5b
hack in vpcId on firewall rule in anticipation of omicron#663
david-crespo Feb 2, 2022
36dbae3
working but ugly targets subform
david-crespo Feb 2, 2022
cfb5eb2
ports, hosts, and targets subforms actually work
david-crespo Feb 2, 2022
6a71251
we can PUT rules!
david-crespo Feb 2, 2022
271bb26
get rid of vestigial subnet copy, move name/description up
david-crespo Feb 2, 2022
03fe20c
include all existing rules when creating one
david-crespo Feb 3, 2022
87a404a
be super sure we're not getting any extra nonsense in the update object
david-crespo Feb 3, 2022
e9cc6f8
fucking epic test
david-crespo Feb 4, 2022
3b3a4ca
update API to pull in vpc_id change from omicron main
david-crespo Feb 4, 2022
cf440ca
automatically update packer-id
github-actions[bot] Feb 4, 2022
9c755b7
switch back to fireEvent, everything passes, get rid of yarn test:run
david-crespo Feb 7, 2022
f98a86e
Merge branch 'main' into firewall-rule-crud
david-crespo Feb 7, 2022
8b74db4
update clear button colors
david-crespo Feb 7, 2022
dc7f5f2
lengthen timeout for CI. appalling
david-crespo Feb 7, 2022
9ed9e7c
faster testing through science
david-crespo Feb 7, 2022
3523771
explicit dep on @testing-lib/dom was actually messing me up
david-crespo Feb 7, 2022
d078b7f
wait longer for the modal to close because CI
david-crespo Feb 7, 2022
286a1fc
memoize tableData more goodly
david-crespo Feb 8, 2022
e1121d6
update API for rules so we get list of rules instead of map
david-crespo Feb 8, 2022
438de04
automatically update packer-id
github-actions[bot] Feb 8, 2022
470d3b0
sort firewall rules by name in msw, fix get to put transformer
david-crespo Feb 8, 2022
ab165da
Merge main into firewall-rule-crud
david-crespo Feb 8, 2022
2c3607a
prettier format
david-crespo Feb 8, 2022
89e1792
make editing work
david-crespo Feb 8, 2022
055f2a0
test for edit modal
david-crespo Feb 8, 2022
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
2 changes: 1 addition & 1 deletion .github/workflows/lintBuildTest.yml
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ jobs:
run: yarn lint

- name: Test
run: yarn test:run
run: yarn test run

- name: Build
run: yarn build
Expand Down
2 changes: 1 addition & 1 deletion .github/workflows/packer.yaml
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ jobs:
CLOUDFLARE_TOKEN: ${{secrets.CLOUDFLARE_TOKEN}}
SSL_CERT: ${{secrets.SSL_CERT}}
SSL_KEY: ${{secrets.SSL_KEY}}
API_VERSION: c4e76cb01fa791c4dc9722072800c8969397aa03
API_VERSION: f615ee43803902fc8ed37a7e66a39677f885dbdb

# get the image information from gcloud
- name: Get image information
Expand Down
2 changes: 1 addition & 1 deletion README.md
Original file line number Diff line number Diff line change
Expand Up @@ -118,7 +118,7 @@ Using the script is strongly recommended, but if you really don't want to, make

| Command | Description |
| --------------- | ---------------------------------------------------------------------------------- |
| `yarn test:run` | Vitest tests |
| `yarn test run` | Vitest tests |
| `yarn test` | Vitest tests in watch mode |
| `yarn lint` | ESLint |
| `yarn tsc` | Check types |
Expand Down
12 changes: 5 additions & 7 deletions app/pages/__tests__/InstanceCreatePage.spec.tsx
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ import {
override,
renderAppAt,
screen,
userEvent,
typeByRole,
waitFor,
} from 'app/test-utils'
import { org, project, instance } from '@oxide/api-mocks'
Expand All @@ -30,8 +30,7 @@ describe('InstanceCreatePage', () => {

it('shows specific message for known server error code', async () => {
renderAppAt(formUrl)
const name = screen.getByRole('textbox', { name: 'Choose a name' })
userEvent.type(name, instance.name) // already exists in db
typeByRole('textbox', 'Choose a name', instance.name) // already exists in db

fireEvent.click(submitButton())

Expand Down Expand Up @@ -59,10 +58,9 @@ describe('InstanceCreatePage', () => {
const instancesPage = `/orgs/${org.name}/projects/${project.name}/instances`
expect(window.location.pathname).not.toEqual(instancesPage)

const name = screen.getByRole('textbox', { name: 'Choose a name' })
userEvent.type(name, 'new-instance')
userEvent.click(screen.getByLabelText(/6 CPUs/))
userEvent.click(submitButton())
typeByRole('textbox', 'Choose a name', 'new-instance')
fireEvent.click(screen.getByLabelText(/6 CPUs/))
fireEvent.click(submitButton())

// nav to instances list
await waitFor(() => expect(window.location.pathname).toEqual(instancesPage))
Expand Down
8 changes: 2 additions & 6 deletions app/pages/project/instances/actions.tsx
Original file line number Diff line number Diff line change
@@ -1,8 +1,4 @@
import type {
ApiListMethods,
Instance,
ProjectInstancesGetParams,
} from '@oxide/api'
import type { Instance, ProjectInstancesGetParams } from '@oxide/api'
import { useApiMutation, useApiQueryClient } from '@oxide/api'
import type { MakeActions } from '@oxide/table'
import { Success16Icon } from '@oxide/ui'
Expand All @@ -28,7 +24,7 @@ const instanceCan: Record<string, (i: Instance) => boolean> = {

export const useInstanceActions = (
params: ProjectInstancesGetParams
): MakeActions<ApiListMethods, 'projectInstancesGet'> => {
): MakeActions<Instance> => {
const addToast = useToast()
const queryClient = useApiQueryClient()
const refetch = () =>
Expand Down
193 changes: 176 additions & 17 deletions app/pages/project/networking/VpcPage/VpcPage.spec.ts
Original file line number Diff line number Diff line change
@@ -1,50 +1,209 @@
import {
fireEvent,
clickByRole,
renderAppAt,
screen,
userEvent,
typeByRole,
waitForElementToBeRemoved,
userEvent,
getByRole,
} from 'app/test-utils'
import { defaultFirewallRules } from '@oxide/api-mocks'

describe('VpcPage', () => {
describe('subnets tab', () => {
it('creating a subnet works', async () => {
describe('subnet', () => {
it('create works', async () => {
renderAppAt('/orgs/maze-war/projects/mock-project/vpcs/mock-vpc')
screen.getByText('Subnets')

// wait for subnet to show up in the table
await screen.findByRole('cell', { name: 'mock-subnet' })
// second one is not there though
await screen.findByText('mock-subnet')
// the one we'll be adding is not there
expect(screen.queryByRole('cell', { name: 'mock-subnet-2' })).toBeNull()

// modal is not already open
expect(screen.queryByRole('dialog', { name: 'Create subnet' })).toBeNull()
expect(screen.queryByTestId('create-vpc-subnet-modal')).toBeNull()

// click button to open modal
fireEvent.click(screen.getByRole('button', { name: 'New subnet' }))
await clickByRole('button', 'New subnet')

// modal is open
screen.getByRole('dialog', { name: 'Create subnet' })

const ipv4 = screen.getByRole('textbox', { name: 'IPv4 block' })
userEvent.type(ipv4, '1.1.1.2/24')
typeByRole('textbox', 'IPv4 block', '1.1.1.2/24')

typeByRole('textbox', 'Name', 'mock-subnet-2')

// submit the form
await clickByRole('button', 'Create subnet')

// wait for modal to close
await waitForElementToBeRemoved(() =>
screen.queryByTestId('create-vpc-subnet-modal')
)

// table refetches and now includes second subnet
await screen.findByText('mock-subnet')
await screen.findByText('mock-subnet-2')
}, 10000) // otherwise it flakes in CI
})

describe('firewall rule', () => {
it('create works', async () => {
renderAppAt('/orgs/maze-war/projects/mock-project/vpcs/mock-vpc')
await clickByRole('tab', 'Firewall Rules')

// default rules show up in the table
for (const { name } of defaultFirewallRules) {
await screen.findByText(name)
}
// the one we'll be adding is not there
expect(screen.queryByRole('cell', { name: 'my-new-rule' })).toBeNull()

// modal is not already open
expect(
screen.queryByRole('dialog', { name: 'Create firewall rule' })
).toBeNull()

// click button to open modal
await clickByRole('button', 'New rule')

// modal is open
screen.getByRole('dialog', { name: 'Create firewall rule' })

typeByRole('textbox', 'Name', 'my-new-rule')

await clickByRole('radio', 'Outgoing')

// input type="number" becomes spinbutton for some reason
typeByRole('spinbutton', 'Priority', '5')

await clickByRole('button', 'Target type')
await clickByRole('option', 'VPC')
typeByRole('textbox', 'Target name', 'my-target-vpc')
await clickByRole('button', 'Add target')

// target is added to targets table
screen.getByRole('cell', { name: 'my-target-vpc' })

await clickByRole('button', 'Host type')
await clickByRole('option', 'Instance')
typeByRole('textbox', 'Value', 'host-filter-instance')
await clickByRole('button', 'Add host filter')

// host is added to hosts table
screen.getByRole('cell', { name: 'host-filter-instance' })

// TODO: test invalid port range once I put an error message in there
typeByRole('textbox', 'Port filter', '123-456')
await clickByRole('button', 'Add port filter')

// port range is added to port ranges table
screen.getByRole('cell', { name: '123-456' })

await clickByRole('checkbox', 'UDP')

// submit the form
await clickByRole('button', 'Create rule')

// wait for modal to close
await waitForElementToBeRemoved(
() => screen.queryByText('Create firewall rule'),
// fails in CI without a longer timeout (default 1000). boo
{ timeout: 2000 }
)

// table refetches and now includes the new rule as well as the originals
await screen.findByText('my-new-rule')
screen.getByRole('cell', {
name: 'instance host-filter-instance UDP 123-456',
})

for (const { name } of defaultFirewallRules) {
screen.getByText(name)
}
}, 15000)

it('edit works', async () => {
renderAppAt('/orgs/maze-war/projects/mock-project/vpcs/mock-vpc')
await clickByRole('tab', 'Firewall Rules')

// default rules show up in the table
for (const { name } of defaultFirewallRules) {
await screen.findByText(name)
}
expect(screen.getAllByRole('row').length).toEqual(5) // 4 + header

// the one we'll be adding is not there
expect(screen.queryByRole('cell', { name: 'new-rule-name' })).toBeNull()

// modal is not already open
expect(
screen.queryByRole('dialog', { name: 'Edit firewall rule' })
).toBeNull()

// click more button on allow-icmp row to get menu, then click Edit
const allowIcmpRow = screen.getByRole('row', { name: /allow-icmp/ })
const more = getByRole(allowIcmpRow, 'button', { name: 'More' })
await userEvent.click(more)
await clickByRole('menuitem', 'Edit')

// now the modal is open
screen.getByRole('dialog', { name: 'Edit firewall rule' })

// name is populated
const name = screen.getByRole('textbox', { name: 'Name' })
userEvent.type(name, 'mock-subnet-2')
expect(name).toHaveValue('allow-icmp')

// priority is populated
const priority = screen.getByRole('spinbutton', { name: 'Priority' })
expect(priority).toHaveValue(65534)

// protocol is populated
expect(screen.getByRole('checkbox', { name: /ICMP/ })).toBeChecked()
expect(screen.getByRole('checkbox', { name: /TCP/ })).not.toBeChecked()
expect(screen.getByRole('checkbox', { name: /UDP/ })).not.toBeChecked()

// targets default vpc
screen.getByRole('cell', { name: 'vpc' })
screen.getByRole('cell', { name: 'default' })

// update name
await userEvent.clear(name)
await userEvent.type(name, 'new-rule-name')

// add host filter
await clickByRole('button', 'Host type')
await clickByRole('option', 'Instance')
typeByRole('textbox', 'Value', 'edit-filter-instance')
await clickByRole('button', 'Add host filter')

// host is added to hosts table
screen.getByRole('cell', { name: 'edit-filter-instance' })

// submit the form
fireEvent.click(screen.getByRole('button', { name: 'Create subnet' }))
await clickByRole('button', 'Update rule')

// wait for modal to close
await waitForElementToBeRemoved(
() => screen.queryByRole('dialog', { name: 'Create subnet' }),
() => screen.queryByText('Edit firewall rule'),
// fails in CI without a longer timeout (default 1000). boo
{ timeout: 2000 }
)

// table refetches and now includes second subnet
screen.getByRole('cell', { name: 'mock-subnet' })
await screen.findByRole('cell', { name: 'mock-subnet-2' })
}, 10000) // otherwise it flakes in CI
// table refetches and now includes the updated rule name, not the old name
await screen.findByText('new-rule-name')
expect(screen.queryByRole('cell', { name: 'allow-icmp' })).toBeNull()
expect(screen.getAllByRole('row').length).toEqual(5) // 4 + header

screen.getByRole('cell', {
name: 'instance edit-filter-instance ICMP',
})

// other 3 rules are still there
const rest = defaultFirewallRules.filter((r) => r.name !== 'allow-icmp')
for (const { name } of rest) {
screen.getByRole('cell', { name })
}
}, 20000)
})
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

These used to be beautiful role-scoped queries:

screen.findByRole('dialog', { name: 'Create firewall rule' })
screen.findByRole('cell', { name: 'my-new-rule' })

But it turns out these were causing all our performance problems. Changing these to *byText cuts the time from from the onSuccess on the rules PUT firing to finding the new cell in the table from 2 seconds down to like 150ms. *byRole is very slow — it's fine for getByRole because that runs once and is done, but for findByRole, which polls every 50ms until it finds the thing, it messes everything up.

There's a PR out on dom-testing-lib that allegedly improves the performance significantly, but when I applied it as a patch using patch-package, I was not able to find the improvement. Maybe a slight improvement, but the above 2s delay remained.

Copy link
Contributor

Choose a reason for hiding this comment

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

Worth noting that that PR commented out one of the biggest parts of the slow down which is trying to derive the implicit role of an element. Evidently if you pass { hidden: true } as one of the arguments to findByRole that increases the speed by ignoring hidden element checks... which may not be what we want. You can read more about that in this issue. Potential performance improvements would likely be required in the downstream in the dom-accessibility-api which is leveraged by @testing-library/jest-dom

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I mean... it does seem bad to treat hidden elements as present from a user's POV, but if actually did eliminate the bulk of the slowdown I'd consider it

})
Loading