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 <[email protected]>

Author: charliepark

app/components/form/fields/DisksTableField.tsx

@@ -30,9 +30,11 @@ export type DiskTableItem =
 export function DisksTableField({
   control,
   disabled,
+  unavailableDiskNames,
 }: {
   control: Control<InstanceCreateInput>
   disabled: boolean
+  unavailableDiskNames: string[]
 }) {
   const [showDiskCreate, setShowDiskCreate] = useState(false)
   const [showDiskAttach, setShowDiskAttach] = useState(false)
@@ -108,6 +110,7 @@ export function DisksTableField({
             onChange([...items, { type: 'create', ...values }])
             setShowDiskCreate(false)
           }}
+          unavailableDiskNames={unavailableDiskNames}
           onDismiss={() => setShowDiskCreate(false)}
         />
       )}

app/forms/disk-create.tsx

@@ -64,12 +64,14 @@ type CreateSideModalFormProps = {
    */
   onDismiss: (navigate: NavigateFunction) => void
   onSuccess?: (disk: Disk) => void
+  unavailableDiskNames?: string[]
 }
 
 export function CreateDiskSideModalForm({
   onSubmit,
   onSuccess,
   onDismiss,
+  unavailableDiskNames = [],
 }: CreateSideModalFormProps) {
   const queryClient = useApiQueryClient()
   const navigate = useNavigate()
@@ -132,7 +134,15 @@ export function CreateDiskSideModalForm({
       loading={createDisk.isPending}
       submitError={createDisk.error}
     >
-      <NameField name="name" control={form.control} />
+      <NameField
+        name="name"
+        control={form.control}
+        validate={(name: string) => {
+          if (unavailableDiskNames.includes(name)) {
+            return 'Name is already in use'
+          }
+        }}
+      />
       <DescriptionField name="description" control={form.control} />
       <FormDivider />
       <DiskSourceField

app/forms/instance-create.tsx

@@ -252,6 +252,12 @@ export function CreateInstanceForm() {
     }
   }, [createInstance.error])
 
+  const otherDisks = useWatch({ control, name: 'otherDisks' })
+  const unavailableDiskNames = [
+    ...allDisks, // existing disks from the API
+    ...otherDisks.filter((disk) => disk.type === 'create'), // disks being created here
+  ].map((d) => d.name)
+
   // additional form elements for projectImage and siloImage tabs
   const bootDiskSizeAndName = (
     <>
@@ -278,10 +284,18 @@ export function CreateInstanceForm() {
         required={false}
         control={control}
         disabled={isSubmitting}
+        validate={(name) => {
+          // don't allow the user to use an existing disk name for the boot disk's name
+          if (unavailableDiskNames.includes(name)) {
+            return 'Name is already in use'
+          }
+        }}
       />
     </>
   )
 
+  const bootDiskName = useWatch({ control, name: 'bootDiskName' })
+
   return (
     <>
       <PageHeader>
@@ -566,7 +580,13 @@ export function CreateInstanceForm() {
         </Tabs.Root>
         <FormDivider />
         <Form.Heading id="additional-disks">Additional disks</Form.Heading>
-        <DisksTableField control={control} disabled={isSubmitting} />
+        <DisksTableField
+          control={control}
+          disabled={isSubmitting}
+          // Don't allow the user to create a new disk with a name that matches other disk names (either the boot disk,
+          // the names of disks that will be created and attached to this instance, or disks that already exist).
+          unavailableDiskNames={[bootDiskName, ...unavailableDiskNames]}
+        />
         <FormDivider />
         <Form.Heading id="authentication">Authentication</Form.Heading>
         <SshKeysField control={control} isSubmitting={isSubmitting} />

test/e2e/instance-create.e2e.ts

@@ -243,7 +243,41 @@ test('with disk name already taken', async ({ page }) => {
   await page.fill('input[name=bootDiskName]', 'disk-1')
 
   await page.getByRole('button', { name: 'Create instance' }).click()
-  await expectVisible(page, ['text=Disk name already exists'])
+  await expect(page.getByText('Name is already in use').first()).toBeVisible()
+})
+
+test('can’t create a disk with a name that collides with the boot disk name', async ({
+  page,
+}) => {
+  // Set up the instance and name the boot disk "disk-11"
+  await page.goto('/projects/mock-project/instances-new')
+  await page.fill('input[name=name]', 'another-instance')
+  await selectAProjectImage(page, 'image-1')
+  await page.fill('input[name=bootDiskName]', 'disk-11')
+
+  // Attempt to create a disk with the same name
+  await page.getByRole('button', { name: 'Create new disk' }).click()
+  const dialog = page.getByRole('dialog')
+  await dialog.getByRole('textbox', { name: 'name' }).fill('disk-11')
+  await dialog.getByRole('button', { name: 'Create disk' }).click()
+  // Expect to see an error message
+  await expect(dialog.getByText('Name is already in use')).toBeVisible()
+  // Change the disk name to something else
+  await dialog.getByRole('textbox', { name: 'name' }).fill('disk-12')
+  await dialog.getByRole('button', { name: 'Create disk' }).click()
+  // The disk has been "created" (is in the list of Additional Disks)
+  await expectVisible(page, ['text=disk-12'])
+  // Create the instance
+  await page.getByRole('button', { name: 'Create instance' }).click()
+  await expect(page).toHaveURL('/projects/mock-project/instances/another-instance/storage')
+
+  // Find the Boot Disk table and verify that disk-11 is there
+  const bootDiskTable = page.getByRole('table', { name: 'Boot disk' })
+  await expect(bootDiskTable.getByRole('cell', { name: 'disk-11' })).toBeVisible()
+
+  // Find the Other Disks table and verify that disk-12 is there
+  const otherDisksTable = page.getByRole('table', { name: 'Other disks' })
+  await expect(otherDisksTable.getByRole('cell', { name: 'disk-12' })).toBeVisible()
 })
 
 test('add ssh key from instance create form', async ({ page }) => {
@@ -510,13 +544,28 @@ test('create instance with additional disks', async ({ page }) => {
   await page.getByRole('button', { name: 'Create new disk' }).click()
 
   const createForm = page.getByRole('dialog', { name: 'Create disk' })
-  await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1')
+
+  // verify that an existing name can't be used
+  await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('disk-6')
   await createForm.getByRole('textbox', { name: 'Size (GiB)' }).fill('5')
   await createForm.getByRole('button', { name: 'Create disk' }).click()
+  await expect(createForm.getByText('Name is already in use')).toBeVisible()
+
+  // rename the disk to one that's allowed
+  await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1')
+  await createForm.getByRole('button', { name: 'Create disk' }).click()
 
   const disksTable = page.getByRole('table', { name: 'Disks' })
+  await expect(disksTable.getByText('disk-6')).toBeHidden()
   await expectRowVisible(disksTable, { Name: 'new-disk-1', Type: 'create', Size: '5GiB' })
 
+  // now that name is taken too, so disk create disallows it
+  await page.getByRole('button', { name: 'Create new disk' }).click()
+  await createForm.getByRole('textbox', { name: 'Name', exact: true }).fill('new-disk-1')
+  await createForm.getByRole('button', { name: 'Create disk' }).click()
+  await expect(createForm.getByText('Name is already in use')).toBeVisible()
+  await createForm.getByRole('button', { name: 'Cancel' }).click()
+
   // Attach an existing disk
   await page.getByRole('button', { name: 'Attach existing disk' }).click()
   await selectOption(page, 'Disk name', 'disk-3')