feat: [M3-7641] – Support IPv4 ranges in VPC "Assign Linodes" drawer #10089

dwiley-akamai · 2024-01-20T00:32:38Z

Description 📝

Rework the "Assign Linodes" drawer to support the assigning and display of IPv4 ranges.

This PR will not be merged into develop but rather into a to-be created branch to accommodate the off-cycle release next week to release this for VPC.

Changes 🔄

Rely on VPC feature flag to determine whether the "BETA Feedback" link is displayed or not on the VPC landing & detail pages
Update LinodeInterfaceSchema to remove max length of 1 for ip_ranges field
New AssignIPRanges component (will be used in the Linode Create flow & Add/Edit Linode Config dialog as well in subsequent tickets)
Added RemovableSelectionsListTable component instead of muddying RemovableSelectionsList (the latter will be getting used in other projects also, not just VPC anymore)
Minor adjustments in MultipleIPInput.tsx so it can be used for this use case

Preview 📷

Before	After
Screen.Recording.2024-01-24.at.5.03.21.PM.mov	Screen.Recording.2024-01-24.at.5.04.23.PM.mov

How to test 🧪

Verification steps

For the "BETA Feedback" link, toggle the VPC feature flag in our DevTool and confirm the link on the VPC landing and detail pages doesn't show when the flag is off.

Have the vpc-extra-beta tag on your account and a VPC containing at least one subnet. Navigate to the "Assign Linodes to subnet" drawer (VPC Details > three-dot menu on subnet line item > "Assign Linodes")...

the "Auto-assign a VPC IPv4 address for this Linode" checkbox and "Assign additional IPv4 ranges" section should be hidden until you select a linode
the "Auto-assign a VPC IPv4 address for this Linode" checkbox functionality should not have been adversely impacted (i.e., the payload with it checked and unchecked should look as expected. If it's unchecked, it should have the input of the "VPC IPv4" field)
Add 3 or more IPv4 ranges and assign the linode. Observe: the table under "Linodes Assigned to Subnet" should be populated with the details for the linode you just added. The "+X" chip in the "VPC IPv4 Ranges" column should (after an initial click, and then any time after that upon hover) show the other ranges beyond the first one displayed in the table.
Clicking the "X" for the linode in the "Linodes Assigned to Subnet" table should unassign it from the subnet.

There were slight changes to MultipleIPInput.tsx so it could be used satisfactorily here. Please confirm there have been no adverse impacts to its use elsewhere in the app (Database "Add Access Control" drawer, "Edit Domain" drawer for secondary domains, etc.)

As an Author I have considered 🤔

dwiley-akamai · 2024-01-24T19:04:23Z

packages/manager/src/components/RemovableSelectionsList/RemovableSelectionsListTable.tsx

Opted to separate this out into its own component as opposed to having a bunch of conditionals in RemovableSelectionsList.tsx in order to display the data in table form. RemovableSelectionsList.tsx was initially created for VPC but is being used moving forward in other projects.

This is the only current use case for this component. VPC-specific data is hard-coded in here. There might be an elegant way around that but I think it's basically the same dilemma we faced with tables/table rows in the past: we used to have something called EntityTable IIRC and that was flexible enough to consume different headers and data for the rows. We got rid of that and have been doing entity-specific rows, so perhaps this file should be renamed to be specific for VPC.

dwiley-akamai · 2024-01-24T21:26:19Z

packages/manager/src/features/Linodes/LinodeSelect/LinodeSelect.tsx

@@ -161,6 +161,7 @@ export const LinodeSelect = (
      helperText={helperText}
      id={id}
      inputValue={inputValue}
+      isOptionEqualToValue={(option, value) => option.id === value.id}


Without this, I was seeing a warning error getting aggressively logged to the console every time I was in the drawer. I added this line based on this StackOverflow comment. I didn't observe any adverse impacts of this change where this component is used elsewhere, e.g. the Volume Create flow.

I wonder if we should be safe and only pass this to your instance via optional prop. No harm in only targeting the smallest audience as we're unaware of potential side effects

That's fair -- I made an adjustment in 60f99db so it doesn't get passed unconditionally as an prop

packages/manager/src/components/RegionSelect/RegionMultiSelect.stories.tsx

dwiley-akamai · 2024-01-24T21:31:41Z

packages/manager/src/features/VPCs/VPCDetail/SubnetAssignLinodesDrawer.test.tsx

+  it.skip('should show the IPv4 textbox when the checkmark is clicked', async () => {
+    const { findByText, getByLabelText } = renderWithTheme(
+      <SubnetAssignLinodesDrawer {...props} />,
+      {
+        queryClient,
+      }
    );

-    const checkbox = getByText(
+    const selectField = getByLabelText('Linode');
+    fireEvent.change(selectField, { target: { value: 'this-linode' } });
+
+    const checkbox = await findByText(
      'Auto-assign a VPC IPv4 address for this Linode'
    );
-    expect(checkbox).toBeVisible();
+
+    await waitFor(() => expect(checkbox).toBeVisible());
    fireEvent.click(checkbox);

-    const ipv4Textbox = getByText('VPC IPv4');
-    expect(ipv4Textbox).toBeVisible();
+    const ipv4Textbox = await findByText('VPC IPv4');
+    await waitFor(() => expect(ipv4Textbox).toBeVisible());


One of the changes to this UI is that the "Auto-assign a VPC IPv4 address for this Linode" checkbox is no longer exposed by default, but rather only when a user selects a linode. I couldn't quite get the test working as I had hoped (console.log'ing the value of linodes in SubnetAssignLinodesDrawer.tsx showed it as undefined despite L40 up above).

This is fine - we've done this in other places too

dwiley-akamai · 2024-01-24T22:22:52Z

packages/manager/src/features/VPCs/VPCDetail/SubnetAssignLinodesDrawer.tsx

@@ -57,7 +61,7 @@ interface SubnetAssignLinodesDrawerProps {

 type LinodeAndConfigData = Linode & {
  configId: number;
-  interfaceId: number;
+  interfaceData: Interface | undefined;


This change was made so that the needed VPC IPv4 & VPC IPv4 Ranges data could be passed into the RemovableSelectionsListTable for display.

Suggested change

interfaceData: Interface | undefined;

interfaceData?: Interface;

Strictly speaking, it's not an optional property. The undefined piece is there because it gets stored in the newInterface ref which gets initialized as undefined.

github-actions · 2024-01-24T22:54:16Z

Coverage Report: ✅
Base Coverage: 79.94%
Current Coverage: 80.07%

abailly-akamai

UI looks great, and functionality worked as expected. Added comments to ad tests and cleanup some things, also have a few questions:

Do we want to show the array index here?

I am confused about the difference between the error returned by the API and what is displayed to the user. Can you clarify if this is expected?

abailly-akamai · 2024-01-25T15:21:29Z

packages/manager/src/components/MultipleIPInput/MultipleIPInput.tsx

  className?: string;
  error?: string;
  forDatabaseAccessControls?: boolean;
+  forVPCIPv4Ranges?: boolean;


Am not crazy about making a feature specific prop for a reusable component. Any way we can make this more generic? This prop could be more than a boolean (could be a custom object serving your purpose)

What about passing in a custom button JSX element?

I like the custom button JSX element option, but it presents some difficulties since the onClick handler (addNewInput) gets defined in this file.

Due to time constraints we'll revisit making this more generic in a follow-up ticket

Sounds good. Let's add a todo reminder comment

abailly-akamai · 2024-01-25T15:24:06Z

packages/manager/src/components/RemovableSelectionsList/RemovableSelectionsListTable.tsx

+      </Table>
+    </>
+  );
+};


can we get a test for this new component and add it to the existing story?

abailly-akamai · 2024-01-25T15:28:15Z

packages/manager/src/features/Linodes/LinodeSelect/LinodeSelect.tsx

@@ -161,6 +161,7 @@ export const LinodeSelect = (
      helperText={helperText}
      id={id}
      inputValue={inputValue}
+      isOptionEqualToValue={(option, value) => option.id === value.id}


I wonder if we should be safe and only pass this to your instance via optional prop. No harm in only targeting the smallest audience as we're unaware of potential side effects

abailly-akamai · 2024-01-25T15:28:55Z

packages/manager/src/features/VPCs/VPCDetail/AssignIPRanges.tsx

+      />
+    </>
+  );
+};


unless it is covered somewhere else, can we also add unit test for this?

hana-linode

Functionality LGTM

packages/manager/src/features/VPCs/VPCDetail/AssignIPRanges.tsx

hana-linode · 2024-01-25T20:46:03Z

packages/manager/src/features/VPCs/VPCDetail/SubnetAssignLinodesDrawer.tsx

@@ -57,7 +61,7 @@ interface SubnetAssignLinodesDrawerProps {

 type LinodeAndConfigData = Linode & {
  configId: number;
-  interfaceId: number;
+  interfaceData: Interface | undefined;


Suggested change

interfaceData: Interface | undefined;

interfaceData?: Interface;

hana-linode · 2024-01-25T20:52:41Z

packages/manager/src/components/MultipleIPInput/MultipleIPInput.tsx

  className?: string;
  error?: string;
  forDatabaseAccessControls?: boolean;
+  forVPCIPv4Ranges?: boolean;


What about passing in a custom button JSX element?

hana-linode · 2024-01-25T22:34:31Z

packages/manager/src/features/VPCs/VPCDetail/AssignIPRanges.tsx

+        {ASSIGN_IPV4_RANGES_DESCRIPTION}{' '}
+        <Link to="https://www.linode.com/docs/guides/how-to-understand-ip-addresses/">
+          Learn more
+        </Link>
+        .


Should this be in a tooltip? In the Add/Edit Linode Config Mocks, it's in a tooltip

In the Add/Edit Linode Config context it should be in a tooltip because of how long and dense that form is, but elsewhere the copy is exposed

…re flag so the link disappears once we turn off the feature flag for GA

… field tied to it only if a linode has been selected

…ved schema limit on how many IP ranges can be provided; added prop to <Autocomplete /> in LinodeSelect.tsx to prevent 'The value provided to Autocomplete is invalid' console error

…put placeholder and button text per updated mockups

…signIPRanges />; temp skip in SubnetAssignLinodesDrawer.test.tsx and remove some assertions given changed behavior

…splayed in PrimaryNav (linode#10090)

…Id; unit test adjustment

…imited to 1 element

…ete />

jaalah-akamai

Added unit test AssignIPRanges.test.tsx.
Second time reviewing this PR and things are working as intended. I also reviewed Domains & Firewalls since those use similar components.

hana-linode

"BETA Feedback" link is hidden when the vpc feature flag is off
"Auto-assign a VPC IPv4 address for this Linode" checkbox and "Assign additional IPv4 ranges" section is hidden until a Linode is selected
Able to assign IPv4 ranges to the Linode
- "+X" chip is present when assigning more than one range
- Chip tooltip displays the additional ranges
Clicking "X" unassigns the Linode from the subnet
No regressions found for MultipleIPInput

… Table (#10108) ## Description 📝 Add support for displaying VPC IPv4 addresses and ranges in the Linode Details -> Network tab -> "IP Addresses" table ## Changes 🔄 List any change relevant to the reviewer. - Add Support for VPC IPv4 addresses and ranges to the Linode IP Address table - If there is a VPC interface with 1:1 NAT, hide the Public IPv4 IP address row - Improve mobile text display of the Type column ## How to test 🧪 ### Prerequisites (How to setup test environment) - Have the `vpc-extra-beta` tag on your account and a VPC with at least one Subnet - Assign a Linode to the Subnet and check both the checkboxes (Auto-assign a VPC IPv4 address & Assign a public IPv4 address) - To add `ip_ranges`, check out #10089 ### Verification steps (How to verify changes) - Go to the Linode Details -> Network tab of the assigned Linode and check the IP Addresses table - You should see IP Address rows for `IPv4 – VPC`, `VPC IPv4 – NAT`, and `IPv4 – VPC – Range` for each range - You should _not_ see an IP address row for the type `IPv4 - Public` ``` yarn test LinodeIPAddressRow ```

dwiley-akamai added Work in Progress @linode/validation Changes are made to the @linode/validation package VPC Relating to VPC project labels Jan 20, 2024

dwiley-akamai self-assigned this Jan 20, 2024

dwiley-akamai commented Jan 24, 2024

View reviewed changes

packages/manager/src/components/RegionSelect/RegionMultiSelect.stories.tsx Outdated Show resolved Hide resolved

dwiley-akamai commented Jan 24, 2024

View reviewed changes

dwiley-akamai marked this pull request as ready for review January 24, 2024 22:23

dwiley-akamai requested a review from a team as a code owner January 24, 2024 22:23

dwiley-akamai requested review from bnussman-akamai, abailly-akamai, hana-linode and jaalah-akamai and removed request for a team January 24, 2024 22:23

abailly-akamai reviewed Jan 25, 2024

View reviewed changes

dwiley-akamai removed the Work in Progress label Jan 25, 2024

dwiley-akamai mentioned this pull request Jan 25, 2024

feat: [M3-7642] – Support IPv4 Ranges data in "Unassign Linodes" drawer #10111

Closed

12 tasks

hana-linode reviewed Jan 25, 2024

View reviewed changes

hana-linode mentioned this pull request Jan 25, 2024

feat: [M3-7644] - Add VPC IPv4 address and range to Linode IP Address Table #10108

Merged

12 tasks

hana-linode reviewed Jan 25, 2024

View reviewed changes

DevDW added 7 commits January 25, 2024 23:45

Make display of 'BETA Feedback' link conditoned based solely on featu…

ba03023

…re flag so the link disappears once we turn off the feature flag for GA

Display 'Auto-assign a VPC IPv4 address for this Linode' checkbox and…

3576a6b

… field tied to it only if a linode has been selected

Basic structure & functionality in place for IPv4 range support; remo…

9af0bfb

…ved schema limit on how many IP ranges can be provided; added prop to <Autocomplete /> in LinodeSelect.tsx to prevent 'The value provided to Autocomplete is invalid' console error

Filter empty strings out from IP Ranges payload; update IPv4 Range in…

02d4044

…put placeholder and button text per updated mockups

Use <Chip /> for '+X' piece

088a1e4

Use <Chip /> for '+X' piece pt 2

0b0e5bb

Utility file and tests for chip function; spacing adjustments for <As…

78ca148

…signIPRanges />; temp skip in SubnetAssignLinodesDrawer.test.tsx and remove some assertions given changed behavior

DevDW and others added 7 commits January 25, 2024 23:46

Use <LinkButton /> to get desired styling for 'Add IPv4 Range' button

c777aa5

desired RemovableSelectionsList UI (refactor forthcoming)

ff18de7

RemovableSelectionsListTable.tsx

493b4f4

refactor: [M3-7672] – Use flags.vpc to determine if beta chip gets di…

8019eec

…splayed in PrimaryNav (linode#10090)

Minor refactor in SubnetAssignLinodesDrawer.tsx surrounding interface…

1c06c09

…Id; unit test adjustment

Changeset added for Cloud changes

814a487

Added changeset: ip_ranges field in LinodeInterfaceSchema no longer l…

6dd87a1

…imited to 1 element

dwiley-akamai force-pushed the M3-7641-range-support-assign-drawer branch from f7146be to 6dd87a1 Compare January 26, 2024 15:26

dwiley-akamai changed the base branch from develop to staging-off-release January 26, 2024 15:35

Undo changes to RemovableSelectionsList.tsx

b97c083

dwiley-akamai force-pushed the M3-7641-range-support-assign-drawer branch from 5363c28 to b97c083 Compare January 26, 2024 15:50

This was referenced Jan 26, 2024

feat: [M3-7642] – Support IPv4 Ranges data in "Unassign Linodes" drawer #10114

Merged

feat: [M3-7643] – Support VPC IPv4 Ranges in Linode Create flow #10116

Merged

DevDW and others added 3 commits January 26, 2024 14:42

More narrow in the passing of isOptionEqualToValue prop to <Autocompl…

60f99db

…ete />

Remove 'important' prop from <Notice /> for general IP Ranges error

eb4f867

Add AssignIPRanges test

fe85edb

jaalah-akamai approved these changes Jan 26, 2024

View reviewed changes

hana-linode approved these changes Jan 26, 2024

View reviewed changes

hana-linode added the Approved Multiple approvals and ready to merge! label Jan 26, 2024

dwiley-akamai merged commit f35b249 into linode:staging-off-release Jan 26, 2024
16 of 18 checks passed

dwiley-akamai deleted the M3-7641-range-support-assign-drawer branch January 26, 2024 21:58

jaalah-akamai mentioned this pull request Jan 29, 2024

Release v1.111.0 - release → staging #10120

Closed

bnussman-akamai mentioned this pull request Feb 12, 2024

Release v1.112.0 - release → staging #10178

Merged

cpathipa mentioned this pull request Feb 12, 2024

Release v1.112.0 - staging → master #10180

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: [M3-7641] – Support IPv4 ranges in VPC "Assign Linodes" drawer #10089

feat: [M3-7641] – Support IPv4 ranges in VPC "Assign Linodes" drawer #10089

dwiley-akamai commented Jan 20, 2024 •

edited

dwiley-akamai Jan 24, 2024

dwiley-akamai Jan 24, 2024

abailly-akamai Jan 25, 2024

dwiley-akamai Jan 26, 2024

dwiley-akamai Jan 24, 2024

jaalah-akamai Jan 26, 2024

dwiley-akamai Jan 24, 2024

hana-linode Jan 25, 2024

dwiley-akamai Jan 26, 2024

github-actions bot commented Jan 24, 2024 •

edited

abailly-akamai left a comment

abailly-akamai Jan 25, 2024

hana-linode Jan 25, 2024

dwiley-akamai Jan 26, 2024

jaalah-akamai Jan 26, 2024

hana-linode Jan 26, 2024

abailly-akamai Jan 25, 2024

abailly-akamai Jan 25, 2024

abailly-akamai Jan 25, 2024

hana-linode left a comment

hana-linode Jan 25, 2024

hana-linode Jan 25, 2024

hana-linode Jan 25, 2024

dwiley-akamai Jan 26, 2024

jaalah-akamai left a comment

hana-linode left a comment

	interfaceData: Interface \| undefined;
	interfaceData?: Interface;

feat: [M3-7641] – Support IPv4 ranges in VPC "Assign Linodes" drawer #10089

feat: [M3-7641] – Support IPv4 ranges in VPC "Assign Linodes" drawer #10089

Conversation

dwiley-akamai commented Jan 20, 2024 • edited

Description 📝

Changes 🔄

Preview 📷

How to test 🧪

Verification steps

As an Author I have considered 🤔

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

github-actions bot commented Jan 24, 2024 • edited

abailly-akamai left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hana-linode left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jaalah-akamai left a comment

Choose a reason for hiding this comment

hana-linode left a comment

Choose a reason for hiding this comment

dwiley-akamai commented Jan 20, 2024 •

edited

github-actions bot commented Jan 24, 2024 •

edited