Skip to content

Commit 0517a28

Browse files
charlieparkdavid-crespo
charliepark
and
david-crespo
authored
Prevent creating multiple disks with the same name (#2541)
* Disallow duplicate disk names in instance create flow * Query for ALL_ISH disk list in create disk form * typo * Make sure boot disk can't be named the same as a disk being created in the same process; add test * small refactor * Update test * let API handle disk-create where new name overlaps with existing disks * Move const declaration back to where it had been * test that client-side created disks are disallowed * tweak unavailable names list to save a couple lines --------- Co-authored-by: David Crespo <david.crespo@oxidecomputer.com>
1 parent 510c645 commit 0517a28

4 files changed

Lines changed: 86 additions & 4 deletions

File tree

‎app/components/form/fields/DisksTableField.tsx‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -30,9 +30,11 @@ export type DiskTableItem =
3030
export function DisksTableField({
3131
control,
3232
disabled,
33+
unavailableDiskNames,
3334
}: {
3435
control: Control<InstanceCreateInput>
3536
disabled: boolean
37+
unavailableDiskNames: string[]
3638
}) {
3739
const [showDiskCreate, setShowDiskCreate] = useState(false)
3840
const [showDiskAttach, setShowDiskAttach] = useState(false)
@@ -108,6 +110,7 @@ export function DisksTableField({
108110
onChange([...items, { type: 'create', ...values }])
109111
setShowDiskCreate(false)
110112
}}
113+
unavailableDiskNames={unavailableDiskNames}
111114
onDismiss={() => setShowDiskCreate(false)}
112115
/>
113116
)}

‎app/forms/disk-create.tsx‎

Lines changed: 11 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -64,12 +64,14 @@ type CreateSideModalFormProps = {
6464
*/
6565
onDismiss: (navigate: NavigateFunction) => void
6666
onSuccess?: (disk: Disk) => void
67+
unavailableDiskNames?: string[]
6768
}
6869

6970
export function CreateDiskSideModalForm({
7071
onSubmit,
7172
onSuccess,
7273
onDismiss,
74+
unavailableDiskNames = [],
7375
}: CreateSideModalFormProps) {
7476
const queryClient = useApiQueryClient()
7577
const navigate = useNavigate()
@@ -132,7 +134,15 @@ export function CreateDiskSideModalForm({
132134
loading={createDisk.isPending}
133135
submitError={createDisk.error}
134136
>
135-
<NameField name="name" control={form.control} />
137+
<NameField
138+
name="name"
139+
control={form.control}
140+
validate={(name: string) => {
141+
if (unavailableDiskNames.includes(name)) {
142+
return 'Name is already in use'
143+
}
144+
}}
145+
/>
136146
<DescriptionField name="description" control={form.control} />
137147
<FormDivider />
138148
<DiskSourceField

‎app/forms/instance-create.tsx‎

Lines changed: 21 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -252,6 +252,12 @@ export function CreateInstanceForm() {
252252
}
253253
}, [createInstance.error])
254254

255+
const otherDisks = useWatch({ control, name: 'otherDisks' })
256+
const unavailableDiskNames = [
257+
...allDisks, // existing disks from the API
258+
...otherDisks.filter((disk) => disk.type === 'create'), // disks being created here
259+
].map((d) => d.name)
260+
255261
// additional form elements for projectImage and siloImage tabs
256262
const bootDiskSizeAndName = (
257263
<>
@@ -278,10 +284,18 @@ export function CreateInstanceForm() {
278284
required={false}
279285
control={control}
280286
disabled={isSubmitting}
287+
validate={(name) => {
288+
// don't allow the user to use an existing disk name for the boot disk's name
289+
if (unavailableDiskNames.includes(name)) {
290+
return 'Name is already in use'
291+
}
292+
}}
281293
/>
282294
</>
283295
)
284296

297+
const bootDiskName = useWatch({ control, name: 'bootDiskName' })
298+
285299
return (
286300
<>
287301
<PageHeader>
@@ -566,7 +580,13 @@ export function CreateInstanceForm() {
566580
</Tabs.Root>
567581
<FormDivider />
568582
<Form.Heading id="additional-disks">Additional disks</Form.Heading>
569-
<DisksTableField control={control} disabled={isSubmitting} />
583+
<DisksTableField
584+
control={control}
585+
disabled={isSubmitting}
586+
// Don't allow the user to create a new disk with a name that matches other disk names (either the boot disk,
587+
// the names of disks that will be created and attached to this instance, or disks that already exist).
588+
unavailableDiskNames={[bootDiskName, ...unavailableDiskNames]}
589+
/>
570590
<FormDivider />
571591
<Form.Heading id="authentication">Authentication</Form.Heading>
572592
<SshKeysField control={control} isSubmitting={isSubmitting} />

‎test/e2e/instance-create.e2e.ts‎

Lines changed: 51 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -243,7 +243,41 @@ test('with disk name already taken', async ({ page }) => {
243243
await page.fill('input[name=bootDiskName]', 'disk-1')
244244

245245
await page.getByRole('button', { name: 'Create instance' }).click()
246-
await expectVisible(page, ['text=Disk name already exists'])
246+
await expect(page.getByText('Name is already in use').first()).toBeVisible()
247+
})
248+
249+
test('can’t create a disk with a name that collides with the boot disk name', async ({
250+
page,
251+
}) => {
252+
// Set up the instance and name the boot disk "disk-11"
253+
await page.goto('/projects/mock-project/instances-new')
254+
await page.fill('input[name=name]', 'another-instance')
255+
await selectAProjectImage(page, 'image-1')
256+
await page.fill('input[name=bootDiskName]', 'disk-11')
257+
258+
// Attempt to create a disk with the same name
259+
await page.getByRole('button', { name: 'Create new disk' }).click()
260+
const dialog = page.getByRole('dialog')
261+
await dialog.getByRole('textbox', { name: 'name' }).fill('disk-11')
262+
await dialog.getByRole('button', { name: 'Create disk' }).click()
263+
// Expect to see an error message
264+
await expect(dialog.getByText('Name is already in use')).toBeVisible()
265+
// Change the disk name to something else
266+
await dialog.getByRole('textbox', { name: 'name' }).fill('disk-12')
267+
await dialog.getByRole('button', { name: 'Create disk' }).click()
268+
// The disk has been "created" (is in the list of Additional Disks)
269+
await expectVisible(page, ['text=disk-12'])
270+
// Create the instance
271+
await page.getByRole('button', { name: 'Create instance' }).click()
272+
await expect(page).toHaveURL('/projects/mock-project/instances/another-instance/storage')
273+
274+
// Find the Boot Disk table and verify that disk-11 is there
275+
const bootDiskTable = page.getByRole('table', { name: 'Boot disk' })
276+
await expect(bootDiskTable.getByRole('cell', { name: 'disk-11' })).toBeVisible()
277+
278+
// Find the Other Disks table and verify that disk-12 is there
279+
const otherDisksTable = page.getByRole('table', { name: 'Other disks' })
280+
await expect(otherDisksTable.getByRole('cell', { name: 'disk-12' })).toBeVisible()
247281
})
248282

249283
test('add ssh key from instance create form', async ({ page }) => {
@@ -510,13 +544,28 @@ test('create instance with additional disks', async ({ page }) => {
510544
await page.getByRole('button', { name: 'Create new disk' }).click()
511545

512546
const createForm = page.getByRole('dialog', { name: 'Create disk' })
513-
await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1')
547+
548+
// verify that an existing name can't be used
549+
await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('disk-6')
514550
await createForm.getByRole('textbox', { name: 'Size (GiB)' }).fill('5')
515551
await createForm.getByRole('button', { name: 'Create disk' }).click()
552+
await expect(createForm.getByText('Name is already in use')).toBeVisible()
553+
554+
// rename the disk to one that's allowed
555+
await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1')
556+
await createForm.getByRole('button', { name: 'Create disk' }).click()
516557

517558
const disksTable = page.getByRole('table', { name: 'Disks' })
559+
await expect(disksTable.getByText('disk-6')).toBeHidden()
518560
await expectRowVisible(disksTable, { Name: 'new-disk-1', Type: 'create', Size: '5GiB' })
519561

562+
// now that name is taken too, so disk create disallows it
563+
await page.getByRole('button', { name: 'Create new disk' }).click()
564+
await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1')
565+
await createForm.getByRole('button', { name: 'Create disk' }).click()
566+
await expect(createForm.getByText('Name is already in use')).toBeVisible()
567+
await createForm.getByRole('button', { name: 'Cancel' }).click()
568+
520569
// Attach an existing disk
521570
await page.getByRole('button', { name: 'Attach existing disk' }).click()
522571
await selectOption(page, 'Disk name', 'disk-3')

0 commit comments

Comments
 (0)