Skip to content

feat: [M3-7343] – Display conditional Notices depending on config scenarios in Linode Add/Edit Config dialog #9916

Merged
dwiley-akamai merged 7 commits intolinode:developfrom
dwiley-akamai:M3-7343-linode-config-dialog-unrecommended-notices
Nov 28, 2023
Merged

feat: [M3-7343] – Display conditional Notices depending on config scenarios in Linode Add/Edit Config dialog #9916
dwiley-akamai merged 7 commits intolinode:developfrom
dwiley-akamai:M3-7343-linode-config-dialog-unrecommended-notices

Conversation

@dwiley-akamai
Copy link
Contributor

@dwiley-akamai dwiley-akamai commented Nov 17, 2023

Description 📝

If a user is about to select an unrecommended configuration in the Add/Edit Config dialog, we want to display conditional warning notices informing them of this and what we recommend to avoid the network connectivity issues that would come with the unrecommended configuration.

Changes 🔄

  • Added unrecommendedConfigNoticeSelector function
  • Added several constants for copy
  • Display conditional notices depending on scenario in Add/Edit Config dialog

Preview 📷

Scenario 1
  • Have a VPC interface and another non-VPC interface (public is fine)
  • the index of the primary interface !== the index of the VPC interface (i.e., the non-VPC interface should be primary)
  • nattedIPv4Address (i.e., "Assign a public IPv4 address for this Linode" checked)

Screenshot 2023-11-20 at 10 51 31 AM

Scenario 2
  • all of Scenario 1, except: !nattedIPv4Address (i.e., "Assign a public IPv4 address for this Linode" unchecked)

Screenshot 2023-11-20 at 10 50 43 AM

Scenario 3
  • only eth0 populated, and it is a VPC interface where !nattedIPv4Address (i.e., "Assign a public IPv4 address for this Linode" unchecked)

Screenshot 2023-11-20 at 10 52 23 AM

How to test 🧪

Prerequisites

  • An account with the proper VPC tags

Verification steps

In the Add/Edit Config dialog, ensure that the specific scenarios outlined in the Preview section above are all handled as expected, and that none of the notices display outside of the outlined scenarios.

As an Author I have 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

@dwiley-akamai dwiley-akamai added the VPC Relating to VPC project label Nov 17, 2023
@dwiley-akamai dwiley-akamai self-assigned this Nov 17, 2023
@dwiley-akamai dwiley-akamai marked this pull request as ready for review November 21, 2023 19:25
@dwiley-akamai dwiley-akamai requested a review from a team as a code owner November 21, 2023 19:25
@dwiley-akamai dwiley-akamai requested review from carrillo-erik, coliu-akamai and mjac0bs and removed request for a team November 21, 2023 19:25
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.

✅ Tested all three scenarios and saw the different error messages as pictured, and confirmed that making the appropriate selections caused the notice to disappear.
✅ Checked that the notice did not appear for scenarios other than the one specified and that multiple notices appeared if there were multiple VPCs.
❓ Checked that I could still submit the form without errors and found one thing mentioned below.

Question: in messing around with different settings, I created a config that had Public Internet in eth0, VPC in eth1, and VLAN in eth2. When I attempted to save my changes, the form did not submit and no feedback was given in the UI. In the console, I saw the error below. Do we want to either surface this or provide some validation in the form?

Screenshot 2023-11-21 at 11 59 19 AM
Screenshot 2023-11-21 at 12 00 35 PM

I don't know enough about configs to know whether this is related to recent VPC changes or a separate issue related to VLANs (I suspect that we just never surfaced this error) -- or whether a user would ever reasonably even try this, but wanted to raise it. Disregard in this PR if it should be addressed in a follow up.

"This network configuration is not recommended. The VPC interface with 1:1 NAT will use the Public IP even if it's not on the default network interface. The Linode will lose access to public network connectivity since the default route isn't able to route through the public IPv4 address. We recommend selecting VPC as the primary interface.";

export const NOT_NATTED_HELPER_TEXT =
'The Linode will not be able to access the internet. If this Linode needs access to the internet we recommend checking the “Assign a public IPv4 address for this Linode” checkbox to enable 1:1 NAT on the VPC interface.';
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
'The Linode will not be able to access the internet. If this Linode needs access to the internet we recommend checking the “Assign a public IPv4 address for this Linode” checkbox to enable 1:1 NAT on the VPC interface.';
'The Linode will not be able to access the internet. If this Linode needs access to the internet, we recommend checking the “Assign a public IPv4 address for this Linode” checkbox to enable 1:1 NAT on the VPC interface.';

@dwiley-akamai
Copy link
Contributor Author

Question: in messing around with different settings, I created a config that had Public Internet in eth0, VPC in eth1, and VLAN in eth2. When I attempted to save my changes, the form did not submit and no feedback was given in the UI. In the console, I saw the error below. Do we want to either surface this or provide some validation in the form?

I don't know enough about configs to know whether this is related to recent VPC changes or a separate issue related to VLANs (I suspect that we just never surfaced this error) -- or whether a user would ever reasonably even try this, but wanted to raise it. Disregard in this PR if it should be addressed in a follow up.

Good find -- I believe from a networking perspective this would be a counterproductive configuration. The API explicitly forbids VLAN interfaces from being marked as primary, and that field is a newer one tied to the VPC project. We should definitely surface the error, but I find the current wording of it to be ambiguous when it's in the context of Cloud. Going to ask API if the reason can be adjusted and I'll create a follow-up ticket to address this issue.

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.

Going to ask API if the reason can be adjusted and I'll create a follow-up ticket to address this issue.

Sounds good, thanks! Approving this now.

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label Nov 27, 2023
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.

✅ Confirmed for the Add and the Edit config dialogs:

  • scenario 1
  • scenario 2
  • scenario 3

Also confirmed could still submit for these scenarios (excluding the case Mariah pointed out) ✅
✅ code review

awesome job! 🚀

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Nov 28, 2023
@dwiley-akamai
Copy link
Contributor Author

The follow-up ticket mentioned earlier has been created (M3-7480)

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.

4 participants