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

feat: [M3-7121 & M3-7122] - Support VPC in Linode Add/Edit Config dialog #9709

Merged

Conversation

hana-akamai
Copy link
Contributor

@hana-akamai hana-akamai commented Sep 21, 2023

Description 📝

Add support for VPCs in the Linode Add Config and Linode Edit Config dialogs, as well as within the config row in the Linode Configurations table.

Major Changes 🔄

  • VPCPanel component used in the Add/Edit Linode Configuration dialogs
    • Tweaks in LinodeConfigDialog.tsx, InterfaceSelect.tsx, and VPCPanel.tsx
  • Network Interfaces column in Linode Config table now supports VPCs
    • new InterfaceListItem component

How to test 🧪

With a properly-tagged account or with the VPC feature flag on:

  • For both the "Add Configuration" and "Edit Configuration" dialogs:

    • The section title should be "Networking" and there should be updated helper text that reflects the VPC option
    • There should be a "Primary Interface (Default Route)" dropdown
    • You should be able to successfully create a config with the designated interfaces
    • Field errors should be surfaced (example: uncheck the "Auto-assign a VPC IPv4 [...]" checkbox, input an invalid VPC IPv4, and observe the field error)
    • The dialog should reset when closed and re-opened
  • For the "Edit Configuration" dialog:

    • Ensure the "Primary Interface" field displays the correct primary interface (the interface designated as "primary" based on the API payload should be in the eth position indicated by the "Primary Interface" field)
    • Changing the primary interface gets reflected in the payload submitted to the API, and after a successful submission, opening the dialog again for that config should reflect the changes you just made

None of the above should be viewable/possible without the proper account tags or the VPC feature flag being on.

Because the <VPCPanel /> component was changed to accommodate the Add/Edit Config use case, ensure there were no regressions to its behavior in the Linode Create flow.

@hana-akamai hana-akamai added the VPC Relating to VPC project label Sep 21, 2023
@hana-akamai hana-akamai self-assigned this Sep 21, 2023
@hana-akamai hana-akamai force-pushed the M3-7121-vpc-add-config-dialog-and-table branch from 2241b8e to e325b51 Compare September 21, 2023 19:09
@dwiley-akamai dwiley-akamai changed the title feat: [M3-7121] - Add VPC to Linode Add Config dialog feat: [M3-7121 & M3-7122] - Support VPC in Linode Add/Edit Config dialog Sep 29, 2023
@dwiley-akamai
Copy link
Contributor

Observed some instances of errors not surfacing, so the error mapping still needs tweaking

@dwiley-akamai dwiley-akamai self-assigned this Oct 2, 2023
@dwiley-akamai
Copy link
Contributor

If I uncheck the "Auto-assign a VPC IPv4 [...]" checkbox but do not input any IPv4, what's the expected behavior? Currently it creates (or edits) the config:

Oh is this one of the errors that don't get surfaced correctly?

Screen.Recording.2023-10-05.at.11.21.28.AM.mov
followup: when creating a Linode and leaving that blank, an error surfaces for a blank IPv4: image

The latter case is because we're able to easily see if the checkbox is unchecked and if there's a blank VPC IPv4 and set the error accordingly (L758-L773 in LinodeCreateContainter.tsx). There's only one pair of those fields in the Linode Create flow, whereas here we can have multiple pairs of those fields in the form (one pair per interface)

packages/api-v4/src/linodes/types.ts Outdated Show resolved Hide resolved
packages/api-v4/src/linodes/types.ts Outdated Show resolved Hide resolved
@dwiley-akamai
Copy link
Contributor

For endpoints that take a list of interfaces, if one of the interfaces has an API error, that should be indicated in the field property -- e.g., interfaces[0].ipv4.vpc instead of ipv4.vpc. API has a bug ticket to address this and it should be done in the near future. In the meantime, I pushed up some code to surface those errors in a non-ideal capacity, just so they don't stay submerged until that fix is in. I'll make a follow-up ticket now for cleaning that up when the API fix is in.

With regards to the error when you uncheck the VPC IPv4 autoassign checkbox but don't input a VPC IPv4: I am going to make a separate ticket to handle that situation and will work on that today.

hana-akamai and others added 3 commits October 6, 2023 14:55
…d weird spacing in Add/Edit Linode Config dialog; make noWrap on VPC IPv4 checkbox text conditional so tooltip isn't obscured
@abailly-akamai abailly-akamai self-requested a review October 9, 2023 15:14
@coliu-akamai coliu-akamai self-requested a review October 9, 2023 18:19
@coliu-akamai
Copy link
Contributor

coliu-akamai commented Oct 10, 2023

Functionality from above that I checked off still looks good + the error on the Subnet Assign Linodes Drawer is fixed and I saw the tickets for the other error surfacing issues 🚀

One other thing I'm noticing is that when we edit a Linode's configuration details to include or delete a VPC related interface, the configuration will update, but other information won't until we refresh - something to do with query invalidation/the query cache, most likely?

https://github.com/linode/manager/assets/139280159/9150f54e-7507-4afc-a172-7eddb6d055c7
This video here is showing how although the config has a VPC interface, this VPC isn't appearing in the linode's details header. Also, the VPC landing page isn't correctly updated (but an individual VPC's detail page is.

(Actually this reminds me about the issue for freshly deleted linodes still appearing in VPCs -- let me make a ticket for that so it doesn't get lost 😅)

Copy link
Contributor

@coliu-akamai coliu-akamai left a comment

Choose a reason for hiding this comment

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

awesome work Hana and Dajahi!

approving pending query caching issue is resolved (moved to a new ticket or handled in this PR)!

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Nice work! things are working

I tested both flows (edit and create config) as we will as the linode creation flow and the form behaviors work well. Left a couple comments to clarify and clean up a couple things.

Also, there is no test coverage added here although we are modifying core flows and adding/modifying many components. I know there must be upcoming e2e tests covering some of those cases but we should also have unit tests testing the rendering and some of the utilities added.

@@ -268,6 +306,15 @@ export const VPCPanel = (props: VPCPanelProps) => {
</Box>
}
/>
{assignPublicIPv4Address && nat_1_1Error && (
Copy link
Contributor

Choose a reason for hiding this comment

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

how can we test this error?

also, let's update this variable name so it's human readable, it's pretty unclear from reading the code

Copy link
Contributor

Choose a reason for hiding this comment

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

If the linode doesn't have an unused public IP available and the "Assign a public IPv4 address for this Linode" checkbox is checked, the following error gets returned:

Screenshot 2023-10-03 at 2 24 44 PM

One way to get it to be returned is to have an interface with 2 VPC slots and checking off that checkbox for both of them. You need an extra tag to be able to get to that state.

I'll rename the variable to publicIPv4Error or something to that effect.

@jaalah-akamai
Copy link
Contributor

✅ For both the "Add Configuration" and "Edit Configuration" dialogs:

  • The section title should be "Networking" and there should be updated helper text that reflects the VPC option
  • There should be a "Primary Interface (Default Route)" dropdown
  • You should be able to successfully create a config with the designated interfaces
  • Field errors should be surfaced (example: uncheck the "Auto-assign a VPC IPv4 [...]" checkbox, input an invalid VPC IPv4, and observe the field error)
  • The dialog should reset when closed and re-opened

✅ For the "Edit Configuration" dialog:

  • Ensure the "Primary Interface" field displays the correct primary interface (the interface designated as "primary" based on the API payload should be in the eth position indicated by the "Primary Interface" field)
  • Changing the primary interface gets reflected in the payload submitted to the API, and after a successful submission, opening the dialog again for that config should reflect the changes you just made

Copy link
Contributor

@abailly-akamai abailly-akamai left a comment

Choose a reason for hiding this comment

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

Thanks for addressing the feedback and adding coverage - looks great!

Functionality and UI are good as far as my testing went. ✅
E2E failure seems unrelated

@dwiley-akamai dwiley-akamai merged commit 472cfa2 into linode:develop Oct 16, 2023
12 of 13 checks passed
hana-akamai added a commit that referenced this pull request Oct 17, 2023
## Description 📝
Bug from #9709

On the prod env, we were unable to edit a config due to the config interface being deleted. This was due to the `regionHasVPCs` variable being coupled with `regionHasVLANS` in the if statement to delete config interfaces. The fix was to handle VPCs and VLANs separately.

## How to test 🧪

### Prerequisites
- Point to the prod environment

### Reproduction steps
- Go to a Linode's details page and click on the `Configurations` tab
- Click the edit button and scroll down to the Networking section
- Select a VLAN and save changes

### Verification steps 
- The VLAN changes should be reflected in the Configurations table
abailly-akamai pushed a commit to abailly-akamai/manager that referenced this pull request Oct 17, 2023
abailly-akamai pushed a commit to abailly-akamai/manager that referenced this pull request Oct 17, 2023
## Description 📝
Bug from linode#9709

On the prod env, we were unable to edit a config due to the config interface being deleted. This was due to the `regionHasVPCs` variable being coupled with `regionHasVLANS` in the if statement to delete config interfaces. The fix was to handle VPCs and VLANs separately.

## How to test 🧪

### Prerequisites
- Point to the prod environment

### Reproduction steps
- Go to a Linode's details page and click on the `Configurations` tab
- Click the edit button and scroll down to the Networking section
- Select a VLAN and save changes

### Verification steps 
- The VLAN changes should be reflected in the Configurations table
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready for Review VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants