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

upcoming: [M3-8074] – Add "Disk Encryption" section to Linode Create flow #10462

Conversation

dwiley-akamai
Copy link
Contributor

@dwiley-akamai dwiley-akamai commented May 13, 2024

Description πŸ“

Add "Disk Encryption" section to the Linode Create flow to allow user to choose if their VM will be encrypted or not.

Changes πŸ”„

  • Disk Encryption added as AccountCapability and region Capabilities
  • New props added to AccessPanel component
  • Conditional notice for Backups added in AddonsPanel component
  • useIsDiskEncryptionFeatureEnabled hook added

Target release date πŸ—“οΈ

5/28/24

Preview πŸ“·

Before (LDE not supported) After (LDE supported)
Screenshot 2024-05-16 at 5 42 04β€―PM Screenshot 2024-05-16 at 5 42 14β€―PM
Screenshot 2024-05-16 at 5 44 11β€―PM Screenshot 2024-05-16 at 5 45 04β€―PM
Screenshot 2024-05-16 at 5 46 16β€―PM Screenshot 2024-05-16 at 5 46 42β€―PM

How to test πŸ§ͺ

Prerequisites

  1. Have the linode_disk_encryption tag on your account
  2. Point at the alpha/dev environment
  3. Have the Linode Disk Encryption (LDE) flag in our dev tool toggled on

Verification steps

Without the customer tag and/or with the LDE flag in our dev tool toggled off, you should not see any LDE-related things in the UI (or in the Linode Create payload). Otherwise,

  1. "Disk Encryption" should be below "SSH Keys" in the same paper, and that paper should be labeled "Security"
    a. No region selected or region w/o Disk Encryption support selected --> "Encrypt Disk" checkbox should be unchecked and disabled with an explanatory tooltip; region w/ Disk Encryption support selected (Newark has support in alpha) --> checkbox should be enabled, and default to checked
    b. Toggling the "Encrypt Disk" checkbox should work as expected
    c. The "Summary" section at the bottom should reflect your choice to encrypt or not, i.e., if "Encrypt Disk" is checked, you should see "Encrypted" in the summary
    d. "Encrypt Disk" checked & Backups add-on checked --> observe: notice under Backups stating, "Virtual Machine Backups are not encrypted."
  2. disk_encryption shows with the expected value for cURL and Linode CLI when you click "Create Using Command Line" (and should be absent when the selected region doesn't support LDE)
  3. No adverse or unexpected impacts where <AccessPanel /> is used elsewhere (e.g., UDF fields, Rebuild from Image & Stackscript, Create Disk drawer --> Create from Image, etc.)

As an Author I have considered πŸ€”

  • πŸ‘€ Doing a self review
  • ❔ Our contribution guidelines
  • 🀏 Splitting feature into small PRs
  • βž• Adding a changeset
  • πŸ§ͺ Providing/Improving test coverage
  • πŸ” Removing all sensitive information from the code and PR description
  • 🚩 Using a feature flag to protect the release
  • πŸ‘£ Providing comprehensive reproduction steps
  • πŸ“‘ Providing or updating our documentation
  • πŸ•› Scheduling a pair reviewing session
  • πŸ“± Providing mobile support
  • β™Ώ Providing accessibility support

@dwiley-akamai dwiley-akamai added the Linode Disk Encryption (LDE) Relating to LDE project label May 13, 2024
@dwiley-akamai dwiley-akamai self-assigned this May 13, 2024
…'Encrypted' in Linode Create summary panel, remove temp code in several components
@bnussman-akamai
Copy link
Member

I'm not sure how I feel about putting disk encryption in the Access Panel on the Linode create flow. Disk encryption feels unrelated to configuring credentials to me.

I think the Addons section would be a lot more fitting. (and maybe rename addons to "Options" or "Additional Configuration" if UX is concerned about the "Addons" naming)

@dwiley-akamai
Copy link
Contributor Author

I'm not sure how I feel about putting disk encryption in the Access Panel on the Linode create flow. Disk encryption feels unrelated to configuring credentials to me.

I think the Addons section would be a lot more fitting. (and maybe rename addons to "Options" or "Additional Configuration" if UX is concerned about the "Addons" naming)

I surfaced this feedback to UX and Product, and the consensus was to roll forward with the current UX given encryption's centrality to the VM creation workflow

@bnussman-akamai
Copy link
Member

Got it! Thanks for surfacing πŸ‘πŸ™πŸΌ

@dwiley-akamai dwiley-akamai marked this pull request as ready for review May 16, 2024 22:04
@dwiley-akamai dwiley-akamai requested a review from a team as a code owner May 16, 2024 22:04
@dwiley-akamai dwiley-akamai requested review from carrillo-erik, AzureLatte and jaalah-akamai and removed request for a team May 16, 2024 22:04
@dwiley-akamai dwiley-akamai requested a review from a team as a code owner May 20, 2024 17:44
@dwiley-akamai dwiley-akamai requested review from cliu-akamai and removed request for a team May 20, 2024 17:44
Copy link

github-actions bot commented May 20, 2024

Coverage Report: βœ…
Base Coverage: 81.53%
Current Coverage: 81.6%

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

βœ… Encryption works based on regions
βœ… Encryption shows in summary
βœ… Encryption enabled shows in payload and request
βœ… Add-on notice appears as expected

Screenshot 2024-05-21 at 8 23 01β€―AM

@carrillo-erik
Copy link
Contributor

I wonder if it would be worth it to rename the AccessPanel component to SecurityPanel or maybe change the header of the AccessPanel to reflect the name of the component. It appears this is the only panel whose header does not match the component name in the code.

Aside from comment above, I was able to verify all functionality described in the "Verification Steps" to work as intended. LGTM βœ…

@dwiley-akamai
Copy link
Contributor Author

dwiley-akamai commented May 21, 2024

I wonder if it would be worth it to rename the AccessPanel component to SecurityPanel or maybe change the header of the AccessPanel to reflect the name of the component. It appears this is the only panel whose header does not match the component name in the code.

That file probably won't be around more than a couple of months given the Linode Create v2 effort so I'd say it's not worth it, but I will keep a possible renaming in mind for the equivalent v2 file (src/features/Linodes/LinodeCreatev2/Access.tsx)

edit: it may be around since it does get used in other places, but not all of them will have the "Security" header

DevDW added 2 commits May 21, 2024 11:09
…om:dwiley-akamai/manager into M3-8074-linode-create-flow-disk-encryption
@AzureLatte AzureLatte removed their request for review May 21, 2024 15:53
@dwiley-akamai dwiley-akamai merged commit 2579ad8 into linode:develop May 21, 2024
18 checks passed
@dwiley-akamai dwiley-akamai deleted the M3-8074-linode-create-flow-disk-encryption branch May 21, 2024 18:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Linode Disk Encryption (LDE) Relating to LDE project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants