Skip to content

upcoming: [M3-10105] - Tie up loose ends for Public IP address tooltip#12408

Merged
coliu-akamai merged 14 commits intolinode:developfrom
coliu-akamai:m3-10105
Jul 1, 2025
Merged

upcoming: [M3-10105] - Tie up loose ends for Public IP address tooltip#12408
coliu-akamai merged 14 commits intolinode:developfrom
coliu-akamai:m3-10105

Conversation

@coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Jun 20, 2025

Description 📝

Improves accuracy for when a public IPv4 or IPv6 should be disabled or not in the AccessTable/IP addresses table. See internal ticket for linked slack threads discussing when public IPv4/IPv6 should be disabled.

note: while working on this I've been seeing some inconsistencies/bugs with the IP Address table, especially for Linode Interfaces (default routing stuff, showing the nat_1_1 twice) - will look into it more in a separate ticket/pr.

I'm thinking this PR moves us closer in terms of correctly showing when IPs should be disabled or not though

Changes 🔄

  • rename useVPCInterface hooks to useDetermineUnreachableIPs, as their scope/purpose has changed, and add test cases for new hook. Updated return values to explicitly determine if an IP is unreachable (this is what useVPCInterface used to do for IPv4 - it returned "isVPCOnlyLinode", which was used to disable the public IPv4)
  • Update disabling of IPs for AccessTable and IP Addresses Table to be more accurate and reflect each other. The AccessTable used to disable both IPv4 and IPv6 even if only IPv4 should be disabled

Target release date 🗓️

Preview 📷

Before After
image image
^ ipv6 disabled when it shouldn't be
image image
^ ipv6 not disabled when it should be

How to test 🧪

Have Linode Interfaces customer tag and feature flag enabled

verification:

Confirm logic/interpretations for when IPv4/IPv6 should be disabled makes sense (see linked slack thread discussions in ticket) - this is how I interpreted the cases while also noting that currently, our IP address table automatically hides the public IPv4 and shows the Nat_1_1 address if a VPC interface has nat_1_1

for Linode Interfaces:

  • disable public IPv6 if no public interface
  • disable public IPv4 if VPC only linode or no interface is the default IPv4 (I'm seeing that the API will automatically assign a default route for IPv4 if possible)

For Config interfaces it's a bit more complicated since if the Linode has no interfaces, the API automatically provides public connectivity - so:

  • disable public IPv6 if Linode has interfaces but no public interface
  • disable IPv4 - taking old logic of isVPCOnlyLinode + doesn't have otherwise given public connectivity

Some test cases I've run through

Linode interfaces

  • no interfaces - disable both IPv4/IPv6
  • only VLAN interfaces - disable both IPv4/IPv6
  • VPC Interface that is default route with no nat_1_1 -> disable both IPv4 and IPv6
  • VPC Interface that is default route and has nat_1_1 -> disable IPv6
  • VPC Interface that is default route with no nat_1_1 but also has a public interface -> disable both IPv4
  • VPC Interface that is default route and has nat_1_1 and also has public interface -> nothing disabled (public IP already hidden)
  • VPC Interface that is NOT default route for IPv4 with no nat_1_1 and has public interface -> nothing idsabled

Legacy Interfaces

  • no interfaces in config profile - nothing disabled
  • only VLAN interfaces - both IPv4/IPv6 disabled
  • primary VPC interface without nat_1_1 - IPv4 and IPv6 disabled
  • primary VPC interface without nat_1_1 and public interface - IPv4 disabled

In general my understanding is that the config legacy interface cases should match Linode Interfaces (although there's the special case with no interfaces)

Author Checklists

As an Author, to speed up the review process, I 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


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

@coliu-akamai
Copy link
Contributor Author

coliu-akamai commented Jun 23, 2025

planning to close this PR in favor of a v2 that I think makes more sense
(or merge that into this one)

<StyledColumnLabelGrid>
{title}{' '}
{isDisabled && (
{showDisabledPublicIPTooltip && (
Copy link
Contributor Author

@coliu-akamai coliu-akamai Jun 24, 2025

Choose a reason for hiding this comment

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

I've aligned the disabling of the public IP addresses' access table with the IP Addresses table, but this leaves some cases where the copy doesn't align with the disabling (implies multiple IPs disabled but only one is). We could potentially:

  • update the copy to have the correct tense (but may need to clarify what "The noted IP Address" means - maybe use disabled instead)?
  • show the tooltip at the individual IP level
    • show tooltip at individual level only if one is IP is disabled, and if both disabled, keep tooltip at the top?

I think the second option is more clear/flexible - thoughts?
Also would we need to get UX approval for either option?

Current options
image image
image

(forgot to mask the IPs - Linode deleted)

Copy link
Member

@bnussman-akamai bnussman-akamai Jun 27, 2025

Choose a reason for hiding this comment

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

I like the idea of putting the tooltip at the individual IP level.

if both disabled, keep tooltip at the top?

We could do this, but to keep the code simple, I honestly think it would be okay to just keep the tooltips at the individual IP level because this UI (hopefully) won't be show often

@coliu-akamai coliu-akamai marked this pull request as ready for review June 24, 2025 15:32
@coliu-akamai coliu-akamai requested a review from a team as a code owner June 24, 2025 15:32
@coliu-akamai coliu-akamai requested review from bnussman-akamai, cpathipa and mjac0bs and removed request for a team June 24, 2025 15:32
@coliu-akamai coliu-akamai requested a review from a team as a code owner June 27, 2025 19:03
@coliu-akamai coliu-akamai requested review from dmcintyr-akamai and removed request for a team June 27, 2025 19:03
@coliu-akamai coliu-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Jun 27, 2025
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.

Nothing like a Friday to do a "bit of light reading" on IP addressing. (This PR gets further into the weeds of VPC and Linode Interfaces than I'm familiar with.) Did anyone from the API side end up checking if the cases in the internal ticket seem current? I read the thread from 2023, but couldn't access the other one.

Code and test coverage for the cases looks good overall, with consistency between the IP Addresses table on the Networking tab vs. the IP addresses in the table in the summary section at the top.

Approving pending what looks like one issue in a unit test issue is fixed.

@github-project-automation github-project-automation bot moved this from Merged to Approved in Cloud Manager Jun 27, 2025
@mjac0bs mjac0bs added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jun 27, 2025
Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
@coliu-akamai
Copy link
Contributor Author

thanks @mjac0bs! added screenshots of the relevant conversation from theother thread to the ticket

@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🎉 670 passing tests on test run #14 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
0 Failing670 Passing4 Skipped126m 2s

@coliu-akamai coliu-akamai merged commit ba35c21 into linode:develop Jul 1, 2025
32 checks passed
@coliu-akamai coliu-akamai deleted the m3-10105 branch July 1, 2025 16:02
@github-project-automation github-project-automation bot moved this from Approved to Merged in Cloud Manager Jul 1, 2025
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! Linode Interfaces

Projects

Archived in project

Development

Successfully merging this pull request may close these issues.

4 participants