Skip to content

Commit 239e34b

Browse files
Disable primary NIC delete when multiple NICs present (#2806)
* Disable delete on primary NIC when other NICs present * Move nics.length evaluation to const * cannot * More precise locator text * That wasn't it * Add a couple of checks to slow down the process and reduce flakiness * one more check * fix e2e test failure by closing menu * slight copy tweak * Update test copy --------- Co-authored-by: David Crespo <david.crespo@oxidecomputer.com>
1 parent 376f172 commit 239e34b

File tree

4 files changed

+58
-21
lines changed

4 files changed

+58
-21
lines changed

app/pages/project/instances/NetworkingTab.tsx

Lines changed: 23 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -222,9 +222,30 @@ export default function NetworkingTab() {
222222
query: { project },
223223
})
224224

225+
const nics = usePrefetchedApiQuery('instanceNetworkInterfaceList', {
226+
query: { ...instanceSelector, limit: ALL_ISH },
227+
}).data.items
228+
229+
const multipleNics = nics.length > 1
230+
225231
const makeActions = useCallback(
226232
(nic: NicRow): MenuAction[] => {
227233
const canUpdateNic = instanceCan.updateNic({ runState: nic.instanceState })
234+
235+
const deleteDisabledReason = () => {
236+
if (!canUpdateNic) {
237+
return <>The instance must be {updateNicStates} to delete a network interface</>
238+
}
239+
// If the NIC is primary, we can't delete it if there are other NICs. Per Ben N:
240+
// > There is always zero or one primary NIC. There may zero or more secondary NICs (up to 7 today), but only if there is already a primary.
241+
// > The primary NIC is where we attach all the external networking state, like external addresses, and the VPC information like routes, subnet information, internet gateways, etc.
242+
// > You may delete any secondary NIC. You may delete the primary NIC only if it's the only NIC (there are no secondary NICs).
243+
if (nic.primary && multipleNics) {
244+
return 'The primary interface can’t be deleted while other interfaces are attached. To delete it, make another interface primary.'
245+
}
246+
return undefined
247+
}
248+
228249
return [
229250
{
230251
label: 'Make primary',
@@ -266,21 +287,15 @@ export default function NetworkingTab() {
266287
}),
267288
label: nic.name,
268289
}),
269-
disabled: !canUpdateNic && (
270-
<>The instance must be {updateNicStates} to delete a network interface</>
271-
),
290+
disabled: deleteDisabledReason(),
272291
},
273292
]
274293
},
275-
[deleteNic, editNic, instanceSelector]
294+
[deleteNic, editNic, instanceSelector, multipleNics]
276295
)
277296

278297
const columns = useColsWithActions(staticCols, makeActions)
279298

280-
const nics = usePrefetchedApiQuery('instanceNetworkInterfaceList', {
281-
query: { ...instanceSelector, limit: ALL_ISH },
282-
}).data.items
283-
284299
const nicRows = useMemo(
285300
() => nics.map((nic) => ({ ...nic, instanceState: instance.runState })),
286301
[nics, instance]

test/e2e/instance-networking.e2e.ts

Lines changed: 24 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,13 @@
77
*/
88
import { expect, test } from '@playwright/test'
99

10-
import { clickRowAction, expectRowVisible, expectVisible, stopInstance } from './utils'
10+
import {
11+
clickRowAction,
12+
clickRowActions,
13+
expectRowVisible,
14+
expectVisible,
15+
stopInstance,
16+
} from './utils'
1117

1218
test('Instance networking tab — NIC table', async ({ page }) => {
1319
await page.goto('/projects/mock-project/instances/db1')
@@ -72,7 +78,23 @@ test('Instance networking tab — NIC table', async ({ page }) => {
7278
const nic3 = page.getByRole('cell', { name: 'nic-3' })
7379
await expect(nic3).toBeVisible()
7480

75-
// Delete just-added network interface
81+
// See that the primary NIC cannot be deleted when other NICs exist
82+
await clickRowActions(page, 'nic-3')
83+
const deleteButton = page.getByRole('menuitem', { name: 'Delete' })
84+
await expect(deleteButton).toBeDisabled()
85+
await deleteButton.hover()
86+
await expect(page.getByText('The primary interface can’t')).toBeVisible()
87+
88+
// close the menu for nic-3, without the next line fails in FF and Safari (but not Chrome)
89+
await clickRowActions(page, 'nic-3')
90+
91+
// Delete the non-primary NIC
92+
await clickRowAction(page, 'my-nic', 'Delete')
93+
await expect(page.getByText('Are you sure you want to delete my-nic?')).toBeVisible()
94+
await page.getByRole('button', { name: 'Confirm' }).click()
95+
await expect(page.getByRole('cell', { name: 'my-nic' })).toBeHidden()
96+
97+
// Now the primary NIC is deletable
7698
await clickRowAction(page, 'nic-3', 'Delete')
7799
await page.getByRole('button', { name: 'Confirm' }).click()
78100
await expect(nic3).toBeHidden()

test/e2e/instance.e2e.ts

Lines changed: 9 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -7,10 +7,10 @@
77
*/
88
import {
99
clickRowAction,
10+
clickRowActions,
1011
closeToast,
1112
expect,
1213
expectRowVisible,
13-
openRowActions,
1414
test,
1515
type Page,
1616
} from './utils'
@@ -41,7 +41,7 @@ test('can start a failed instance', async ({ page }) => {
4141
await page.goto('/projects/mock-project/instances')
4242

4343
// check the start button disabled message on a running instance
44-
await openRowActions(page, 'db1')
44+
await clickRowActions(page, 'db1')
4545
await page.getByRole('menuitem', { name: 'Start' }).hover()
4646
await expect(
4747
page.getByText('Only stopped or failed instances can be started')
@@ -118,14 +118,14 @@ test('can reboot a running instance', async ({ page }) => {
118118
test('cannot reboot a failed instance', async ({ page }) => {
119119
await page.goto('/projects/mock-project/instances')
120120
await expectInstanceState(page, 'you-fail', 'failed')
121-
await openRowActions(page, 'you-fail')
121+
await clickRowActions(page, 'you-fail')
122122
await expect(page.getByRole('menuitem', { name: 'Reboot' })).toBeDisabled()
123123
})
124124

125125
test('cannot reboot a starting instance, or a stopped instance', async ({ page }) => {
126126
await page.goto('/projects/mock-project/instances')
127127
await expectInstanceState(page, 'not-there-yet', 'starting')
128-
await openRowActions(page, 'not-there-yet')
128+
await clickRowActions(page, 'not-there-yet')
129129
await expect(page.getByRole('menuitem', { name: 'Reboot' })).toBeDisabled()
130130
// hit escape to close the menu so clickRowAction succeeds
131131
await page.keyboard.press('Escape')
@@ -136,21 +136,21 @@ test('cannot reboot a starting instance, or a stopped instance', async ({ page }
136136
await expectInstanceState(page, 'not-there-yet', 'stopping')
137137
await expectInstanceState(page, 'not-there-yet', 'stopped')
138138
// reboot is still disabled for a stopped instance
139-
await openRowActions(page, 'not-there-yet')
139+
await clickRowActions(page, 'not-there-yet')
140140
await expect(page.getByRole('menuitem', { name: 'Reboot' })).toBeDisabled()
141141
})
142142

143143
test('cannot resize a running or starting instance', async ({ page }) => {
144144
await page.goto('/projects/mock-project/instances')
145145

146146
await expectInstanceState(page, 'db1', 'running')
147-
await openRowActions(page, 'db1')
147+
await clickRowActions(page, 'db1')
148148
await expect(page.getByRole('menuitem', { name: 'Resize' })).toBeDisabled()
149149

150150
await page.keyboard.press('Escape') // get out of the menu
151151

152152
await expectInstanceState(page, 'not-there-yet', 'starting')
153-
await openRowActions(page, 'not-there-yet')
153+
await clickRowActions(page, 'not-there-yet')
154154
await expect(page.getByRole('menuitem', { name: 'Resize' })).toBeDisabled()
155155
})
156156

@@ -254,7 +254,7 @@ async function expectRowMenuStaysOpen(page: Page, rowSelector: string) {
254254
await expect(menu).toBeHidden()
255255
await expect(stopped).toBeHidden()
256256

257-
await openRowActions(page, rowSelector)
257+
await clickRowActions(page, rowSelector)
258258
await expect(stopped).toBeHidden() // still not stopped yet
259259
await expect(menu).toBeVisible()
260260

@@ -300,7 +300,7 @@ test("polling doesn't close row actions: instances", async ({ page }) => {
300300
await expect(menu).toBeHidden()
301301
await expect(stopped).toBeHidden()
302302

303-
await openRowActions(page, 'db1')
303+
await clickRowActions(page, 'db1')
304304
await expect(stopped).toBeHidden() // still not stopped yet
305305
await expect(menu).toBeVisible()
306306

test/e2e/utils.ts

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -154,7 +154,7 @@ export async function closeToast(page: Page) {
154154
export const clipboardText = async (page: Page) =>
155155
page.evaluate(() => navigator.clipboard.readText())
156156

157-
export const openRowActions = async (page: Page, name: string) => {
157+
export const clickRowActions = async (page: Page, name: string) => {
158158
await page
159159
.getByRole('row', { name, exact: false })
160160
.getByRole('button', { name: 'Row actions' })
@@ -163,7 +163,7 @@ export const openRowActions = async (page: Page, name: string) => {
163163

164164
/** Select row by `rowName`, click the row actions button, and click `actionName` */
165165
export async function clickRowAction(page: Page, rowName: string, actionName: string) {
166-
await openRowActions(page, rowName)
166+
await clickRowActions(page, rowName)
167167
await page.getByRole('menuitem', { name: actionName }).click()
168168
}
169169

0 commit comments

Comments
 (0)