From 2e7a76a7de79dd3c0a9c8cffa9ba716d014ba741 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Mon, 29 Jul 2024 21:45:21 -0500 Subject: [PATCH 1/4] remove allow-rdp from default firewall rules --- mock-api/msw/handlers.ts | 2 ++ mock-api/vpc.ts | 17 ----------------- test/e2e/firewall-rules.e2e.ts | 30 +++++++++++++++--------------- test/e2e/networking.e2e.ts | 1 - 4 files changed, 17 insertions(+), 33 deletions(-) diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index d88e250ad6..e3dbc91d8d 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -997,6 +997,8 @@ export const handlers = makeHandlers({ } db.vpcSubnets.push(newSubnet) + // TODO: create default firewall rules! + return json(newVpc, { status: 201 }) }, vpcView: ({ path, query }) => lookup.vpc({ ...path, ...query }), diff --git a/mock-api/vpc.ts b/mock-api/vpc.ts index 4d180daa97..f389f8fbe2 100644 --- a/mock-api/vpc.ts +++ b/mock-api/vpc.ts @@ -114,23 +114,6 @@ export const firewallRules: Json = [ time_modified, vpc_id: vpc.id, }, - { - id: '5ed562d9-2566-496d-b7b3-7976b04a0b80', - name: 'allow-rdp', - status: 'enabled', - direction: 'inbound', - targets: [{ type: 'vpc', value: 'default' }], - description: 'allow inbound TCP connections on port 3389 from anywhere', - filters: { - ports: ['3389'], - protocols: ['TCP'], - }, - action: 'allow', - priority: 65534, - time_created, - time_modified, - vpc_id: vpc.id, - }, // second mock VPC in other project, meant to test display with lots of // targets and filters { diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 3cbe630900..978a890365 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -8,7 +8,7 @@ import { clickRowAction, expect, expectRowVisible, test } from './utils' -const defaultRules = ['allow-internal-inbound', 'allow-ssh', 'allow-icmp', 'allow-rdp'] +const defaultRules = ['allow-internal-inbound', 'allow-ssh', 'allow-icmp'] test('can create firewall rule', async ({ page }) => { await page.goto('/projects/mock-project/vpcs/mock-vpc') @@ -19,7 +19,7 @@ test('can create firewall rule', async ({ page }) => { await expect(page.locator(`text="${name}"`)).toBeVisible() } const rows = page.locator('tbody >> tr') - await expect(rows).toHaveCount(4) + await expect(rows).toHaveCount(3) const modal = page.getByRole('dialog', { name: 'Add firewall rule' }) await expect(modal).toBeHidden() @@ -100,7 +100,7 @@ test('can create firewall rule', async ({ page }) => { const tooltip = page.getByRole('tooltip', { name: 'Other filters UDP Port 123-' }) await expect(tooltip).toBeVisible() - await expect(rows).toHaveCount(5) + await expect(rows).toHaveCount(4) for (const name of defaultRules) { await expect(page.locator(`text="${name}"`)).toBeVisible() } @@ -241,7 +241,7 @@ test('can update firewall rule', async ({ page }) => { await page.getByRole('tab', { name: 'Firewall Rules' }).click() const rows = page.locator('tbody >> tr') - await expect(rows).toHaveCount(4) + await expect(rows).toHaveCount(3) // allow-icmp is the one we're doing to change const oldNameCell = page.locator('td >> text="allow-icmp"') @@ -298,7 +298,7 @@ test('can update firewall rule', async ({ page }) => { await expect(newNameCell).toBeVisible() await expect(oldNameCell).toBeHidden() - await expect(rows).toHaveCount(4) + await expect(rows).toHaveCount(3) // new target shows up in target cell await expect(page.locator('text=subnetedit-filter-subnetICMP')).toBeVisible() @@ -317,23 +317,23 @@ test('create from existing rule', async ({ page }) => { const modal = page.getByRole('dialog', { name: 'Add firewall rule' }) await expect(modal).toBeHidden() - await clickRowAction(page, 'allow-rdp', 'Clone') + await clickRowAction(page, 'allow-icmp', 'Clone') - await expect(page).toHaveURL(url + '-new/allow-rdp') + await expect(page).toHaveURL(url + '-new/allow-icmp') await expect(modal).toBeVisible() await expect(modal.getByRole('textbox', { name: 'Name', exact: true })).toHaveValue( - 'allow-rdp-copy' + 'allow-icmp-copy' ) - await expect(modal.getByRole('checkbox', { name: 'TCP' })).toBeChecked() + await expect(modal.getByRole('checkbox', { name: 'TCP' })).not.toBeChecked() await expect(modal.getByRole('checkbox', { name: 'UDP' })).not.toBeChecked() - await expect(modal.getByRole('checkbox', { name: 'ICMP' })).not.toBeChecked() + await expect(modal.getByRole('checkbox', { name: 'ICMP' })).toBeChecked() - await expect( - modal - .getByRole('table', { name: 'Port filters' }) - .getByRole('cell', { name: '3389', exact: true }) - ).toBeVisible() + // await expect( + // modal + // .getByRole('table', { name: 'Port filters' }) + // .getByRole('cell', { name: '3389', exact: true }) + // ).toBeVisible() await expect( modal .getByRole('table', { name: 'Targets' }) diff --git a/test/e2e/networking.e2e.ts b/test/e2e/networking.e2e.ts index 7b975dea61..717172ae14 100644 --- a/test/e2e/networking.e2e.ts +++ b/test/e2e/networking.e2e.ts @@ -97,7 +97,6 @@ test('Create and edit subnet', async ({ page }) => { await expectVisible(page, [ 'role=cell[name="allow-icmp"]', 'role=cell[name="allow-internal-inbound"]', - 'role=cell[name="allow-rdp"]', 'role=cell[name="allow-ssh"]', ]) }) From 6808c89c2a466d5b720bcbdb0852befa42b46469 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 30 Jul 2024 15:18:48 -0500 Subject: [PATCH 2/4] populate VPC with default rules on creation in mock API, update e2e test --- mock-api/msw/handlers.ts | 6 +- mock-api/msw/util.ts | 5 -- mock-api/util.ts | 15 +++++ mock-api/vpc.ts | 109 ++++++++++++++++++++----------------- test/e2e/networking.e2e.ts | 64 +++++++++++++++------- 5 files changed, 121 insertions(+), 78 deletions(-) create mode 100644 mock-api/util.ts diff --git a/mock-api/msw/handlers.ts b/mock-api/msw/handlers.ts index e3dbc91d8d..813804d69c 100644 --- a/mock-api/msw/handlers.ts +++ b/mock-api/msw/handlers.ts @@ -27,6 +27,8 @@ import { GiB } from '~/util/units' import { genCumulativeI64Data } from '../metrics' import { serial } from '../serial' import { defaultSilo, toIdp } from '../silo' +import { getTimestamps } from '../util' +import { defaultFirewallRules } from '../vpc' import { db, getIpFromPool, @@ -41,7 +43,6 @@ import { errIfInvalidDiskSize, forbiddenErr, getStartAndEndTime, - getTimestamps, handleMetrics, ipInAnyRange, ipRangeLen, @@ -997,7 +998,8 @@ export const handlers = makeHandlers({ } db.vpcSubnets.push(newSubnet) - // TODO: create default firewall rules! + // populate default firewall rules + db.vpcFirewallRules.push(...defaultFirewallRules(newVpc.id)) return json(newVpc, { status: 201 }) }, diff --git a/mock-api/msw/util.ts b/mock-api/msw/util.ts index 0d75236bd1..eba13130b1 100644 --- a/mock-api/msw/util.ts +++ b/mock-api/msw/util.ts @@ -86,11 +86,6 @@ export function getStartAndEndTime(params: { startTime?: Date; endTime?: Date }) return { startTime, endTime } } -export function getTimestamps() { - const now = new Date().toISOString() - return { time_created: now, time_modified: now } -} - export const forbiddenErr = () => json({ error_code: 'Forbidden', request_id: 'fake-id' }, { status: 403 }) diff --git a/mock-api/util.ts b/mock-api/util.ts new file mode 100644 index 0000000000..41463378c8 --- /dev/null +++ b/mock-api/util.ts @@ -0,0 +1,15 @@ +/* + * 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 + */ + +// this is only in its own file so it can be used in both the mock resources and +// the mock handlers without circular import issues + +export function getTimestamps() { + const now = new Date().toISOString() + return { time_created: now, time_modified: now } +} diff --git a/mock-api/vpc.ts b/mock-api/vpc.ts index f389f8fbe2..bc338dae74 100644 --- a/mock-api/vpc.ts +++ b/mock-api/vpc.ts @@ -6,10 +6,13 @@ * Copyright Oxide Computer Company */ +import { v4 as uuid } from 'uuid' + import type { Vpc, VpcFirewallRule, VpcSubnet } from '@oxide/api' import type { Json } from './json-type' import { project, project2 } from './project' +import { getTimestamps } from './util' const time_created = new Date(2021, 0, 1).toISOString() const time_modified = new Date(2021, 0, 2).toISOString() @@ -63,61 +66,67 @@ export const vpcSubnet2: Json = { ipv4_block: '10.1.1.2/24', } -export const firewallRules: Json = [ - { - id: 'b74aeea8-1201-4efd-b6ec-011f10a0b176', - name: 'allow-internal-inbound', - status: 'enabled', - direction: 'inbound', - targets: [{ type: 'vpc', value: 'default' }], - action: 'allow', - description: - 'allow inbound traffic to all instances within the VPC if originated within the VPC', - filters: { - hosts: [{ type: 'vpc', value: 'default' }], +export function defaultFirewallRules(vpcId: string): Json { + return [ + { + id: uuid(), + vpc_id: vpcId, + name: 'allow-internal-inbound', + status: 'enabled', + direction: 'inbound', + targets: [{ type: 'vpc', value: 'default' }], + action: 'allow', + description: + 'allow inbound traffic to all instances within the VPC if originated within the VPC', + filters: { + hosts: [{ type: 'vpc', value: 'default' }], + }, + priority: 65534, + ...getTimestamps(), }, - priority: 65534, - time_created, - time_modified, - vpc_id: vpc.id, - }, - { - id: '9802cd8e-1e59-4fdf-9b40-99c189f7a19b', - name: 'allow-ssh', - status: 'enabled', - direction: 'inbound', - targets: [{ type: 'vpc', value: 'default' }], - description: 'allow inbound TCP connections on port 22 from anywhere', - filters: { - ports: ['22'], - protocols: ['TCP'], + { + id: uuid(), + vpc_id: vpcId, + name: 'allow-ssh', + status: 'enabled', + direction: 'inbound', + targets: [{ type: 'vpc', value: 'default' }], + description: 'allow inbound TCP connections on port 22 from anywhere', + filters: { + ports: ['22'], + protocols: ['TCP'], + }, + action: 'allow', + priority: 65534, + ...getTimestamps(), }, - action: 'allow', - priority: 65534, - time_created, - time_modified, - vpc_id: vpc.id, - }, - { - id: 'cde07d86-b8c0-49ed-8754-55f1bdee20fe', - name: 'allow-icmp', - status: 'enabled', - direction: 'inbound', - targets: [{ type: 'vpc', value: 'default' }], - description: 'allow inbound ICMP traffic from anywhere', - filters: { - protocols: ['ICMP'], + { + id: uuid(), + vpc_id: vpcId, + name: 'allow-icmp', + status: 'enabled', + direction: 'inbound', + targets: [{ type: 'vpc', value: 'default' }], + description: 'allow inbound ICMP traffic from anywhere', + filters: { + protocols: ['ICMP'], + }, + action: 'allow', + priority: 65534, + ...getTimestamps(), }, - action: 'allow', - priority: 65534, - time_created, - time_modified, - vpc_id: vpc.id, - }, + ] +} + +// usually we try to hard-code resource IDs, but in this case +// we don't rely on them anywhere and it's easier to wrap up if they're dynamic + +export const firewallRules: Json = [ + ...defaultFirewallRules(vpc.id), // second mock VPC in other project, meant to test display with lots of // targets and filters { - id: '097c849e-68c8-43f7-9ceb-b1855c51f178', + id: uuid(), name: 'lots-of-filters', status: 'enabled', direction: 'inbound', @@ -139,7 +148,7 @@ export const firewallRules: Json = [ vpc_id: vpc2.id, }, { - id: '097c849e-68c8-43f7-9ceb-b1855c51f178', + id: uuid(), name: 'lots-of-targets', status: 'enabled', direction: 'inbound', diff --git a/test/e2e/networking.e2e.ts b/test/e2e/networking.e2e.ts index 717172ae14..221dac5d67 100644 --- a/test/e2e/networking.e2e.ts +++ b/test/e2e/networking.e2e.ts @@ -7,35 +7,57 @@ */ import { expect, test } from '@playwright/test' -import { closeToast, expectNotVisible, expectVisible } from './utils' +import { + clickRowAction, + closeToast, + expectNotVisible, + expectRowVisible, + expectVisible, +} from './utils' test('Create and edit VPC', async ({ page }) => { await page.goto('/projects/mock-project') - await page.click('role=link[name*="VPCs"]') - await expectVisible(page, [ - 'role=heading[name*="VPCs"]', - 'role=cell[name="mock-vpc"] >> nth=0', - ]) + await page.getByRole('link', { name: 'VPCs' }).click() + await expect(page.getByRole('heading', { name: 'VPCs' })).toBeVisible() + + const table = page.getByRole('table') + await expectRowVisible(table, { + name: 'mock-vpc', + 'DNS name': 'mock-vpc', + description: 'a fake vpc', + 'Firewall Rules': '3', + }) + await expect(table.getByRole('row')).toHaveCount(2) // header plus row // New VPC form - await page.click('role=link[name="New Vpc"]') - await expectVisible(page, [ - 'role=textbox[name="Name"]', - 'role=textbox[name="Description"]', - 'role=textbox[name="DNS name"]', - 'role=textbox[name="IPV6 prefix"]', - 'role=button[name="Create VPC"]', - ]) - await page.goBack() + await page.getByRole('link', { name: 'New Vpc' }).click() + await page.getByRole('textbox', { name: 'Name', exact: true }).fill('another-vpc') + await page.getByRole('textbox', { name: 'DNS name' }).fill('another-vpc') + await page.getByRole('button', { name: 'Create VPC' }).click() + + // now we're on the VPC detail, on the firewall rules tab + await expect(page.getByRole('heading', { name: 'another-vpc' })).toBeVisible() + await expect(page.getByRole('tab', { name: 'Firewall Rules' })).toBeVisible() + + // we have the three default rules + await expect(table.getByRole('row')).toHaveCount(4) // header plus three rows + for (const name of ['allow-icmp', 'allow-internal-inbound', 'allow-ssh']) { + await expect(page.getByRole('cell', { name })).toBeVisible() + } + + // now go back up a level to vpcs table + await page.getByRole('link', { name: 'VPCs' }).click() + await expect(table.getByRole('row')).toHaveCount(3) // header plus two rows + await expectRowVisible(table, { + name: 'another-vpc', + 'DNS name': 'another-vpc', + description: '—', + 'Firewall Rules': '3', + }) // Edit VPC form - await expectVisible(page, ['role=link[name="mock-vpc"]']) - await page - .locator('role=row', { hasText: 'mock-vpc' }) - .locator('role=button[name="Row actions"]') - .click() - await page.click('role=menuitem[name="Edit"]') + await clickRowAction(page, 'mock-vpc', 'Edit') await expectVisible(page, [ 'role=textbox[name="Name"]', 'role=textbox[name="Description"]', From ab3235ff54384141b708f75424c1fe90210e1cd7 Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 30 Jul 2024 15:29:24 -0500 Subject: [PATCH 3/4] do the clone test even better --- test/e2e/firewall-rules.e2e.ts | 36 ++++++++++++++++++++++++---------- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/test/e2e/firewall-rules.e2e.ts b/test/e2e/firewall-rules.e2e.ts index 978a890365..239c7792d3 100644 --- a/test/e2e/firewall-rules.e2e.ts +++ b/test/e2e/firewall-rules.e2e.ts @@ -329,16 +329,32 @@ test('create from existing rule', async ({ page }) => { await expect(modal.getByRole('checkbox', { name: 'UDP' })).not.toBeChecked() await expect(modal.getByRole('checkbox', { name: 'ICMP' })).toBeChecked() - // await expect( - // modal - // .getByRole('table', { name: 'Port filters' }) - // .getByRole('cell', { name: '3389', exact: true }) - // ).toBeVisible() - await expect( - modal - .getByRole('table', { name: 'Targets' }) - .getByRole('row', { name: 'Name: default, Type: vpc' }) - ).toBeVisible() + // no port filters + const portFilters = modal.getByRole('table', { name: 'Port filters' }) + await expect(portFilters).toBeHidden() + + const targets = modal.getByRole('table', { name: 'Targets' }) + await expect(targets.getByRole('row', { name: 'Name: default, Type: vpc' })).toBeVisible() + + // close the modal + await page.keyboard.press('Escape') + await expect(modal).toBeHidden() + + // do it again with a different rule + await clickRowAction(page, 'allow-ssh', 'Clone') + + await expect(modal).toBeVisible() + await expect(modal.getByRole('textbox', { name: 'Name', exact: true })).toHaveValue( + 'allow-ssh-copy' + ) + + await expect(portFilters.getByRole('cell', { name: '22', exact: true })).toBeVisible() + + await expect(modal.getByRole('checkbox', { name: 'TCP' })).toBeChecked() + await expect(modal.getByRole('checkbox', { name: 'UDP' })).not.toBeChecked() + await expect(modal.getByRole('checkbox', { name: 'ICMP' })).not.toBeChecked() + + await expect(targets.getByRole('row', { name: 'Name: default, Type: vpc' })).toBeVisible() }) const rulePath = '/projects/mock-project/vpcs/mock-vpc/firewall-rules/allow-icmp/edit' From a8964f59755f990198bc6810c23a98c8ce7c7d6e Mon Sep 17 00:00:00 2001 From: David Crespo Date: Tue, 30 Jul 2024 15:34:02 -0500 Subject: [PATCH 4/4] missed one --- test/e2e/vpcs.e2e.ts | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/test/e2e/vpcs.e2e.ts b/test/e2e/vpcs.e2e.ts index b8e87fe35f..d34f579ade 100644 --- a/test/e2e/vpcs.e2e.ts +++ b/test/e2e/vpcs.e2e.ts @@ -18,7 +18,7 @@ test('can nav to VpcPage from /', async ({ page }) => { name: 'mock-vpc', 'DNS name': 'mock-vpc', description: 'a fake vpc', - 'Firewall Rules': '4', + 'Firewall Rules': '3', }) // click the vpc name cell to go there @@ -36,7 +36,7 @@ test('can nav to VpcPage from /', async ({ page }) => { await page.goBack() await expect(page.getByRole('heading', { name: 'mock-vpc' })).toBeHidden() await expect(page.getByRole('cell', { name: 'allow-icmp' })).toBeHidden() - await page.getByRole('link', { name: '4' }).click() + await page.getByRole('link', { name: '3' }).click() await expect(page.getByRole('heading', { name: 'mock-vpc' })).toBeVisible() await expect(page.getByRole('cell', { name: 'allow-icmp' })).toBeVisible() })