Skip to content
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

Instance custom SSH key select #1867

Merged
merged 21 commits into from
Jan 31, 2024
Merged

Instance custom SSH key select #1867

merged 21 commits into from
Jan 31, 2024

Conversation

benjaminleonard
Copy link
Contributor

@benjaminleonard benjaminleonard commented Dec 20, 2023

Starting to block out the UI for this (hopefully with some support from @charliepark on this and omicron).

CleanShot 2023-12-20 at 14 52 08

The text area is within the checkbox which is worth looking into incase of accessibility ramifications. But it does make the alignment easier.

This PR adds the CheckboxGroup component, modelled off the RadioGroup.


Here's the types I am proposing. Names might not be quite right. We might also accept either InstancePublicKeys[] or an alternative value that includes all of a user's SSH keys – or decide that it can be added to the console and/or CLI but a user can grab those SSH keys themselves (as we are doing here) if they're creating an instance with the API.

export type InstancePublicKeys = 
  { key: NameOrId; type: 'user_key'} |
  { key: string; type: 'string' }

/**
 * Create-time parameters for an `Instance`
 */
export type InstanceCreate = {
  description: string
  /** The disks to be created or attached for this instance. */
  disks?: InstanceDiskAttachment[]
  /** The external IP addresses provided to this instance.

By default, all instances have outbound connectivity, but no inbound connectivity. These external addresses can be used to provide a fixed, known IP address for making inbound connections to the instance. */
  externalIps?: ExternalIpCreate[]
  hostname: string
  memory: ByteCount
  name: Name
  ncpus: InstanceCpuCount
  publicKeys: InstancePublicKeys[]
  /** The network interfaces to be created for this instance. */
  networkInterfaces?: InstanceNetworkInterfaceAttachment
  /** Should this instance be started upon creation; true by default. */
  start?: boolean
  /** User data for instance initialization systems (such as cloud-init). Must be a Base64-encoded string, as specified in RFC 4648 § 4 (+ and / characters with padding). Maximum 32 KiB unencoded data. */
  userData?: string
}

  • Add SSH key validation (likely just required)
  • Accessibility checks
  • Omicron API work
  • Documentation updates (especially if the default behaviour is no longer injecting all of a users SSH keys)

Related issues:
#1818
oxidecomputer/omicron#3056

Copy link

vercel bot commented Dec 20, 2023

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
console ✅ Ready (Inspect) Visit Preview Jan 31, 2024 6:37pm

@david-crespo
Copy link
Collaborator

david-crespo commented Dec 20, 2023

Not sold on taking up all that space to allow pasting in a one-off key. Is this aimed at preventing someone from having to leave the flow because they didn’t know to add a key beforehand? What about a button that pops up the SSH key add modal? Maybe a little confusing because unlike the disk modals, which are building up the request body but not hitting the API, that one would actually add a key.

@benjaminleonard
Copy link
Contributor Author

benjaminleonard commented Dec 20, 2023

The text input only appears if the "New SSH Key" checkbox is checked. So 99% of the time it's not visible. I think some kind of button modal flow would imply the SSH key sticks around outside of the instance.

@david-crespo
Copy link
Collaborator

Ok, that makes sense. On the API shape, I think we can avoid having two types of things in the list because there is already a user_data field on the instance create request body, so we could use that for the one-off key, and keep the list of keys only for existing ones with NameOrId.

@david-crespo
Copy link
Collaborator

Sorry, I’m mashing buttons on my phone.

@benjaminleonard
Copy link
Contributor Author

Ok, that makes sense. On the API shape, I think we can avoid having two types of things in the list because there is already a user_data field on the instance create request body, so we could use that for the one-off key, and keep the list of keys only for existing ones with NameOrId.

That would be simpler. Does that mean we can have multiple public_keys in the cloud init config?

@benjaminleonard
Copy link
Contributor Author

Might also be worthwhile adding the user_data free form or file input field to this PR too.

@david-crespo
Copy link
Collaborator

Right, good point. And yes, I think we could have multiple one-off keys in there. It’s free-form.

@zephraph
Copy link
Contributor

zephraph commented Jan 3, 2024

Instead of having New SSH Key as some hybrid select/button logic how about we just allow opening the side modal form (like for regular SSH key creation) via a button and give the user the option to save or not save the SSH key.

When you're first going through the flow (we'll have plenty of first time instance creators) it's easy to get here an not have an SSH key. This leads to a major point of friction: if you want to persist the key you either have to leave and repopulate the field (as is currently the case) or remember to persist it later. Both seem unfortunate.

@benjaminleonard
Copy link
Contributor Author

benjaminleonard commented Jan 3, 2024

Instead of having New SSH Key as some hybrid select/button logic how about we just allow opening the side modal form (like for regular SSH key creation) via a button and give the user the option to save or not save the SSH key.

I think that could work – I'll hold off on the UI side until we settle on the omicron implementation in case the shape of that API informs this.

But in theory we pass a list of name or IDs of SSH keys. Instance create then takes an intersection of that and the users keys and adds that to the cloud init config. For the new key, we can just insert that into the user_data field directly.

Do we add the SSH key as soon as the user is done with the modal, or wait until the form is submitted.

If we do it immediately, we'll probably want to avoid the mini table / side modal pattern since that is reserved for items which are added when the form is submitted. If we wait and do it at instance create, we need to think about how we handle an error – is the main form blocked? Do we continue with instance create request anyway?

@benjaminleonard
Copy link
Contributor Author

benjaminleonard commented Jan 25, 2024

SSH keys can be added from within the form with a notice to say they will stick around and are scoped to the user not the instance – I experimented with one-time keys but that'd need some more complex work merging it into user_data.
CleanShot 2024-01-25 at 14 54 27

Can now select/deselect all with a checkbox (all keys are checked by default). And we validate to ensure the number of keys is less than the max per instance – currently 8.
CleanShot 2024-01-25 at 14 31 46

Ideally the new keys are pre-selected I suppose, though it might not be worth the work.

@benjaminleonard benjaminleonard marked this pull request as ready for review January 25, 2024 15:43
@benjaminleonard benjaminleonard marked this pull request as draft January 25, 2024 15:43
}
},
},
})
Copy link
Collaborator

Choose a reason for hiding this comment

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

It turns out this does work. I had forgotten that we changed all form validation to happen on submit, so this will not validate the number of keys until the user clicks submit. It's not ideal as a UX, but it is consistent with everything else. After that initial validation on submit, I think it validates on change, so that is a nice experience.

2024-01-26-sshkey-checkbox-validation.mp4

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that this limit has increased to 100, I think it'll be a relatively uncommon occurrence.

Copy link
Contributor

Choose a reason for hiding this comment

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

We'll need a nicer UI for longer lists here eventually I think. Looks good for now though!

@benjaminleonard
Copy link
Contributor Author

Updated API based based on Omicron 5780ff6d1baaa9199e2a64a6b91665d636bf2e3e.
Ready for review.

Copy link
Collaborator

@david-crespo david-crespo left a comment

Choose a reason for hiding this comment

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

we got it

@benjaminleonard benjaminleonard merged commit efa3978 into main Jan 31, 2024
8 checks passed
@benjaminleonard benjaminleonard deleted the ssh-keys-select branch January 31, 2024 19:42
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Jan 31, 2024
oxidecomputer/console@b9013a3...1a4f5d8

* [1a4f5d81](oxidecomputer/console@1a4f5d81) oxidecomputer/console#1910
* [34596e33](oxidecomputer/console@34596e33) oxidecomputer/console#1928
* [ff7cdb28](oxidecomputer/console@ff7cdb28) oxidecomputer/console#1926
* [efa39789](oxidecomputer/console@efa39789) oxidecomputer/console#1867
* [4bfadc02](oxidecomputer/console@4bfadc02) oxidecomputer/console#1927
* [695d3671](oxidecomputer/console@695d3671) oxidecomputer/console#1925
* [30070292](oxidecomputer/console@30070292) better path filter for local files in msw handler
* [5cf4339c](oxidecomputer/console@5cf4339c) oxidecomputer/console#1916
* [a26f7c1e](oxidecomputer/console@a26f7c1e) oxidecomputer/console#1922
* [231b93ed](oxidecomputer/console@231b93ed) oxidecomputer/console#1923
* [c8364638](oxidecomputer/console@c8364638) better msw warning filter so we don't get warning noise in console
* [764f7310](oxidecomputer/console@764f7310) oxidecomputer/console#1908
* [945619eb](oxidecomputer/console@945619eb) oxidecomputer/console#1921
* [e2d82a4c](oxidecomputer/console@e2d82a4c) oxidecomputer/console#1887
* [d6a67bd5](oxidecomputer/console@d6a67bd5) oxidecomputer/console#1918
* [1fb746f4](oxidecomputer/console@1fb746f4) oxidecomputer/console#1899
* [ca7c85de](oxidecomputer/console@ca7c85de) oxidecomputer/console#1917
* [28598e1d](oxidecomputer/console@28598e1d) oxidecomputer/console#1914
* [34eb478d](oxidecomputer/console@34eb478d) oxidecomputer/console#1912
* [4d693088](oxidecomputer/console@4d693088) bump vite-plugin-html to stop getting deprecation warning
* [d5c39549](oxidecomputer/console@d5c39549) oxidecomputer/console#1909
* [7c6f53db](oxidecomputer/console@7c6f53db) oxidecomputer/console#1854
david-crespo added a commit to oxidecomputer/omicron that referenced this pull request Jan 31, 2024
oxidecomputer/console@b9013a3...1a4f5d8

* [1a4f5d81](oxidecomputer/console@1a4f5d81)
oxidecomputer/console#1910
* [34596e33](oxidecomputer/console@34596e33)
oxidecomputer/console#1928
* [ff7cdb28](oxidecomputer/console@ff7cdb28)
oxidecomputer/console#1926
* [efa39789](oxidecomputer/console@efa39789)
oxidecomputer/console#1867
* [4bfadc02](oxidecomputer/console@4bfadc02)
oxidecomputer/console#1927
* [695d3671](oxidecomputer/console@695d3671)
oxidecomputer/console#1925
* [30070292](oxidecomputer/console@30070292)
better path filter for local files in msw handler
* [5cf4339c](oxidecomputer/console@5cf4339c)
oxidecomputer/console#1916
* [a26f7c1e](oxidecomputer/console@a26f7c1e)
oxidecomputer/console#1922
* [231b93ed](oxidecomputer/console@231b93ed)
oxidecomputer/console#1923
* [c8364638](oxidecomputer/console@c8364638)
better msw warning filter so we don't get warning noise in console
* [764f7310](oxidecomputer/console@764f7310)
oxidecomputer/console#1908
* [945619eb](oxidecomputer/console@945619eb)
oxidecomputer/console#1921
* [e2d82a4c](oxidecomputer/console@e2d82a4c)
oxidecomputer/console#1887
* [d6a67bd5](oxidecomputer/console@d6a67bd5)
oxidecomputer/console#1918
* [1fb746f4](oxidecomputer/console@1fb746f4)
oxidecomputer/console#1899
* [ca7c85de](oxidecomputer/console@ca7c85de)
oxidecomputer/console#1917
* [28598e1d](oxidecomputer/console@28598e1d)
oxidecomputer/console#1914
* [34eb478d](oxidecomputer/console@34eb478d)
oxidecomputer/console#1912
* [4d693088](oxidecomputer/console@4d693088)
bump vite-plugin-html to stop getting deprecation warning
* [d5c39549](oxidecomputer/console@d5c39549)
oxidecomputer/console#1909
* [7c6f53db](oxidecomputer/console@7c6f53db)
oxidecomputer/console#1854
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants