upcoming: [M3-8465] – Add "Volume Encryption" section to "Create and Attach Volume" drawer by dwiley-akamai · Pull Request #10837 · linode/manager

dwiley-akamai · 2024-08-26T20:30:05Z

Description 📝

Add "Volume Encryption" section to "Create and Attach Volume" drawer

Changes 🔄

Add useVolumeQuery
- (tangential refactor for useDetachVolumeMutation to accommodate invalidation of useVolumeQuery)
VolumeSelect refactor to use our custom Autocomplete component rather than MUI's directly
Conditionally-displayed reboot notice in LinodeVolumeAddDrawer
- E2E test coverage for this
"Volume Encryption" section added in LinodeVolumeCreateForm
- Unit test coverage for this

Target release date 🗓️

9/16/24

Preview 📷

Before	After

How to test 🧪

Prerequisites

Point at the dev environment with the blockstorage-encryption tag on your account. The linode you use should have a capabilities array that does not contain blockstorage_encryption. Without one or both of those, you should not observe any of the below.

Verification steps

"Create and Attach Volume" view
- check the "Encrypt Volume" checkbox --> observe: reboot notice below radio buttons & caveats in notice above checkbox, disabled "Create Volume" button
"Attach Existing Volume" view
- select an encrypted volume --> observe: reboot notice below radio buttons, disabled "Attach Volume" button
Test the volume queries that were changed in this PR

As an Author I have considered 🤔

…TORAGE_CLIENT_LIBRARY_UPDATE_REQUIRED_COPY; add Volume Encryption section to LinodeVolumeCreateForm

…isable Create Volume when user needs to reboot linode for client library update

…awer

…l display of client library copy; minor docs update

…or to use custom Autocomplete instead of MUI Autocomplete directly

…date

dwiley-akamai · 2024-08-29T20:48:49Z

packages/manager/src/components/Autocomplete/Autocomplete.tsx

Just linter changes

dwiley-akamai · 2024-08-29T20:50:02Z

packages/api-v4/src/linodes/types.ts

  alerts: LinodeAlerts;
  backups: LinodeBackups;
-  bs_encryption_supported?: boolean; // @TODO BSE: Remove optionality once BSE is fully rolled out
+  capabilities?: string[]; // @TODO BSE: Remove optionality once BSE is fully rolled out


May not be reflected in the API spec (yet), but this is how it was actually implemented. Reach out for Slack convo if needed.

dwiley-akamai · 2024-08-29T20:57:48Z

packages/manager/cypress/e2e/core/volumes/create-volume.spec.ts

+        ui.autocompletePopper
+          .findByTitle(volume.label)
+          .should('be.visible')
+          .click();


@jdamore-linode It seems that everything is good up until this point, and then the assertion on L228 causes the failure. Did some console.log'ing with the Cypress test running and clientLibraryCopyVisible in LinodeVolumeAddDrawer.tsx was never getting set to true for the "Attach Existing Volume" flow. I'm not sure why that is; it definitely works using the actual UI 🤔

I see! When Cypress selects the Volume, Cloud fires off a GET request to /volumes/:id for the selected Volume. Since we're using a mock Volume, the response is 404ing so the attach form doesn't know that the Volume is encrypted.

Mocking that request fixes the issue, but surprisingly we don't have a mock util for that endpoint. You can add one to cypress/support/intercepts/volumes.ts:

/** * Intercepts GET request to fetch a Volume and mocks response. * * @param volume - Volume with which to mock response. * * @returns Cypress chainable. */ export const mockGetVolume = (volume: Volume): Cypress.Chainable<null> => { return cy.intercept('GET', apiMatcher(`volumes/${volume.id}`), makeResponse(volume)); };

Then you can set it up right around where you mock the other Volumes request at line ~198:

mockGetVolumes([volume]).as('getVolumes'); mockGetVolume(volume);

That caused the library update message to display for me and allowed the test to pass!

Awesome, thanks for helping get to the bottom of that!

…er.test.tsx type error

dwiley-akamai · 2024-08-30T16:52:23Z

packages/manager/cypress/support/util/linodes.ts

Just linter changes aside from L187-190

dwiley-akamai · 2024-08-30T16:52:46Z

packages/manager/src/components/Autocomplete/Autocomplete.tsx

Just linter changes

…object to 'capabilities'

… client library update notices in Create/Attach Volume drawer

…Create/Attach Volume drawer

github-actions · 2024-08-30T17:31:08Z

Coverage Report: ✅
Base Coverage: 86.4%
Current Coverage: 86.4%

packages/manager/src/queries/volumes/volumes.ts

…umeMutation

bnussman-akamai

We can also add invalidations in packages/manager/src/queries/volumes/events.ts for the new query key

packages/manager/src/queries/volumes/volumes.ts

bnussman-akamai · 2024-09-05T20:23:52Z

packages/manager/src/queries/volumes/volumes.ts

+      // Invalidate the volume
+      queryClient.invalidateQueries({
+        queryKey: volumeQueries.volume(volumeId).queryKey,
+      });
+    },


Because volume detaches take time to happen on the backend, I don't think we get any value from invalidating the Volume immediately. We can probably remove this.

The invalidation should happen when the volume detaches finishes in packages/manager/src/queries/volumes/events.ts

@bnussman-akamai From my testing, I've only seen volume_detach events come through with a status of notification.

To clarify, do you think adding an invalidation for something like the event.action === 'volume_detach' && event.status === 'notification' case is sufficient in packages/manager/src/queries/volumes/events.ts, or are you proposing changes beyond that?

Adding the following to line 18ish of events.ts should cover it

if (event.entity) { invalidateQueries({ queryKey: volumeQueries.volume(event.entity.id).queryKey, }); }

carrillo-erik · 2024-09-05T21:35:18Z

I was able to test the user journey as described in the PR description and verified the UI updates.

The following feedback is not related to the code changes in this PR. I feel that the user should be able to reboot the Linode from within the Create Volume drawer. With this current implementation, the user is able to configure the Volume they want to Create and Attach, but after checking the box for encryption, the Create Volume button is disabled and we essentially make the customer close the drawer, reboot the Linode, and configure the Volume all over again.

…Volumes queries

cpathipa

Nice work confirming the UX changes and functionality as mentioned in the PR description.

~~Question - To be consistent should we show the notice "This Linode requires client libraries... " in this flow as well ?~~ Ignore the above one I see this is included in this PR

Update Linode interface to include capabilities property; add BLOCK_S…

1752e01

…TORAGE_CLIENT_LIBRARY_UPDATE_REQUIRED_COPY; add Volume Encryption section to LinodeVolumeCreateForm

dwiley-akamai added the Block Storage Encryption (BSE) Relating to Block Storage (Volumes) encryption label Aug 26, 2024

dwiley-akamai self-assigned this Aug 26, 2024

DevDW added 5 commits August 27, 2024 13:50

Conditional display of client library copy in Create & Attach view, d…

6f04ede

…isable Create Volume when user needs to reboot linode for client library update

Clear client library copy when user toggles view inside Volume Add dr…

22adfad

…awer

Reintroduce useVolumeQuery; logic in LinodeVolumeAttachForm to contro…

c143d1f

…l display of client library copy; minor docs update

Linter, util, factory updates; test coverage; VolumeSelect.tsx refact…

3b87504

…or to use custom Autocomplete instead of MUI Autocomplete directly

Disable 'Attach Volume' button when linode requires client library up…

c828e21

…date

dwiley-akamai commented Aug 29, 2024

View reviewed changes

packages/manager/src/components/Autocomplete/Autocomplete.tsx

Copy link

Contributor Author

dwiley-akamai Aug 29, 2024

Choose a reason for hiding this comment

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

Just linter changes

dwiley-akamai commented Aug 29, 2024

View reviewed changes

DevDW added 2 commits August 30, 2024 12:35

Passing create-volume.spec.ts

32b47b4

Merge in latest develop and resolve conflict, fix LinodeVolumeAddDraw…

2c00ca2

…er.test.tsx type error

dwiley-akamai commented Aug 30, 2024

View reviewed changes

dwiley-akamai marked this pull request as ready for review August 30, 2024 16:54

dwiley-akamai requested review from a team as code owners August 30, 2024 16:54

dwiley-akamai requested review from carrillo-erik, cpathipa and jdamore-linode and removed request for a team August 30, 2024 16:54

DevDW added 3 commits August 30, 2024 12:57

Added changeset: Change 'bs_encryption_supported' property on Linode …

174d7a7

…object to 'capabilities'

Added changeset: Unit test for LinodeVolumeAddDrawer and E2E test for…

77f9232

… client library update notices in Create/Attach Volume drawer

Added changeset: Support Volume Encryption and associated notices in …

1fa0029

…Create/Attach Volume drawer

Update E2E test to reflect 'Create Volume' --> 'Add Volume' change

3d71087

bnussman-akamai reviewed Sep 4, 2024

View reviewed changes

packages/manager/src/queries/volumes/volumes.ts Show resolved Hide resolved

dwiley-akamai mentioned this pull request Sep 5, 2024

upcoming: [M3-8420] – Add conditionally-displayed reboot notice on Volume Create page #10868

Merged

12 tasks

Add invalidations for individual volume query & refactor useDetachVol…

b8dd4c3

…umeMutation

bnussman-akamai reviewed Sep 5, 2024

View reviewed changes

carrillo-erik approved these changes Sep 5, 2024

View reviewed changes

DevDW added 2 commits September 6, 2024 11:29

Revert useDetachVolumeMutation changes; use setQueryData in relevant …

4d02471

…Volumes queries

Remove stray leftover cy.log()

8012b0c

cpathipa approved these changes Sep 6, 2024

View reviewed changes

Specific volume invalidation in volumeEventsHandler

3126161

bnussman-akamai approved these changes Sep 9, 2024

View reviewed changes

bnussman-akamai added the Approved Multiple approvals and ready to merge! label Sep 9, 2024

dwiley-akamai merged commit 48ec392 into linode:develop Sep 9, 2024

This was referenced Sep 13, 2024

Release v1.128.0 - release → staging #10935

Merged

Release v1.128.0 - staging → master #10946

Merged

Conversation

dwiley-akamai commented Aug 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description 📝

Changes 🔄

Target release date 🗓️

Preview 📷

How to test 🧪

Prerequisites

Verification steps

As an Author I have considered 🤔

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

github-actions bot commented Aug 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

bnussman-akamai left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dwiley-akamai Sep 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

carrillo-erik commented Sep 5, 2024

Uh oh!

cpathipa left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dwiley-akamai commented Aug 26, 2024 •

edited

Loading

github-actions bot commented Aug 30, 2024 •

edited

Loading

dwiley-akamai Sep 6, 2024 •

edited

Loading

cpathipa left a comment •

edited

Loading