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-7272] - Disable Public IP Address for VPC only Linode #9899

Merged

Conversation

hana-akamai
Copy link
Contributor

@hana-akamai hana-akamai commented Nov 13, 2023

Description 📝

Visually disable Public IP addresses for VPC-only Linodes and add tooltip explanation.

A VPC-only Linode is a Linode that has at least one config interface with primary set to true and purpose vpc and no ipv4.nat_1_1 value.

Changes 🔄

  • Disable the Public IP Address column in the Linodes landing table
  • Disable the Public IP Addresses and Access sections in the Linode's details page
  • Disable the Public IPv4 row in the Linode's details -> Network tab -> IP Addresses table
  • Refactored components into their own file and added unit testing
  • Created custom hook useVPCConfigInterface to share logic

Preview 📷

Linode landing before Linode landing after
linode landing before linode landing after
Linode details before Linode details after
linode detail before linode detail after
Linode Network IP Address table before Linode Network IP Address table after
linode network before linode network after

How to test 🧪

Prerequisites

  • Ensure your account has vpc customer tags
  • Have a VPC-only Linode
    • You can either create a Linode and add a VPC or edit a Linode's config so that the primary interface is VPC. (make sure the assign a public IPv4 address checkbox is unchecked)

Verification steps

For the VPC-only Linode

  • The Public IP Address column in the Linodes landing table should be disabled and there should be a tooltip
  • The Public IP Addresses and Access sections in the Linode's details page should be disabled and there should be a tooltip
  • The Public IPv4 row in the Linode's details -> Network tab -> IP Addresses table should be disabled and there should be a tooltip

As an Author I have considered 🤔

Check all that apply

  • 👀 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

@hana-akamai hana-akamai added the VPC Relating to VPC project label Nov 13, 2023
@hana-akamai hana-akamai self-assigned this Nov 13, 2023
@hana-akamai hana-akamai marked this pull request as ready for review November 13, 2023 22:18
@hana-akamai hana-akamai requested a review from a team as a code owner November 13, 2023 22:18
@hana-akamai hana-akamai requested review from mjac0bs, carrillo-erik, dwiley-akamai and coliu-akamai and removed request for a team November 13, 2023 22:18
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

IPs are disabled for VPCs where mentioned in the testing steps and not disabled otherwise and unit test looks good.

One thing: in dark mode, the disabled IP Addresses table row within the Linode Details > Network tab seems like it probably has a theme styling issue. Light mode is fine!

Screen.Recording.2023-11-14.at.11.35.56.AM.mov

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Going to finish combing through the code tomorrow but the functionality looks good:

The Public IP Address column in the Linodes landing table should be disabled and there should be a tooltip ✅

The Public IP Addresses and Access sections in the Linode's details page should be disabled and there should be a tooltip ✅

The Public IPv4 row in the Linode's details -> Network tab -> IP Addresses table should be disabled and there should be a tooltip ✅

Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

LGTM aside from one small comment. There was also a test failure for smoke-linode-landing-table.spec.ts -- probably related to the Copy icon for IPs? Approving pending investigating that

Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

Approving pending the previously mentioned comments are addressed. The e2e fails because it's looking for IP Addresses as an exact match (L396) and we now have Public IP Addresses:
image

Thanks for fixing the dark mode disabled address row. Confirmed disabled states for all three areas look good in both light and dark modes.

@mjac0bs mjac0bs added the Approved Multiple approvals and ready to merge! label Nov 16, 2023
@coliu-akamai
Copy link
Contributor

coliu-akamai commented Nov 17, 2023

Hmm.. I might be missing something? The tooltip/disabled ip address isn't showing up for the debian-us-lax and debian-us-mia linode, even though I edited their configs. Both of these linodes were created before I started reviewing this PR (debian-us-lax-001 created just now) and had other network interfaces that I removed, unsure if that affects anything:

image

These are the configs when I click 'edit' for both debian-us-lax and lax-001:
debian-us-lax
image
image

debian-us-lax-001 (this one is working)
image
image

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.

Code looks good (one nitpicky comment) + unit tests pass! ✅

I'm honestly very confused about the configuration issue -- I changed the configs for both debian-miami and debian-lax (not 001, that one was already good) a bunch, and eventually the tooltips/disabled public IP started working... not sure what was going on 🤔 😖
I haven't been able to repeat the issue, so it could have been a one time fluke??

@hana-akamai
Copy link
Contributor Author

@coliu-akamai Could it have been bc the "Assign a public IPv4 address" was checked? 🤔

image

@coliu-akamai
Copy link
Contributor

@hana-linode I don't think so bc if I'd had a public IPv4 address assigned for a Linode checked from the start, that checkbox should have been prechecked when I open up the edit config panel right? 😖

It honestly feels like a fluke bc I'm not getting the same issue now no matter what I do - maybe my computer just decided to act up?

@hana-akamai hana-akamai merged commit f465b7d into linode:develop Nov 17, 2023
11 checks passed
hana-akamai added a commit that referenced this pull request Dec 1, 2023
## Description 📝
We are grabbing configs for every Linode in the Linodes landing page to check if it is a VPC-only Linode which does not scale well (same problem as the VPC Linodes landing column). This PR reverts the Linodes landing portion of #9899 and adds a warning banner

## Changes  🔄
- Revert disabling of the Public IP Address column in the Linodes landing table for VPC-only Linodes
- Add warning banner to the Linodes landing page

## How to test 🧪

### Prerequisites
- Ensure your account has vpc customer tags

### Reproduction steps
- Go to `/linodes` and filter your Network tab by `configs`. You should see a configs request for each Linode you have

### Verification steps 
- Go to `/linodes` and filter your Network tab by `configs`
  - You should no longer see a configs request for each Linode you have
  - You should see a warning banner
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants