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-8560] - NodeBalancer Configurations - Support Linodes with Multiple Private IPs #11069

Conversation

bnussman-akamai
Copy link
Member

@bnussman-akamai bnussman-akamai commented Oct 9, 2024

Description 📝

  • Updates the NodeBalancer Configuration Node IP Address Select component to show every private IP for each Linode
    • Currently, the select component only shows the first private IPv4 for each Linode
    • This change also fixes a bug where the select component could display the wrong selected value if the Linode has many private IPv4
  • This change helps support the internal DevCloud ☁️ effort
  • Adds some unit testing around this component and other private IP utils 🧪

Preview 📷

Before After
Screenshot 2024-10-08 at 10 02 41 PM Screenshot 2024-10-08 at 10 01 45 PM
Only one private IPv4 is showing for Linode test-1 Both private IPv4s are showing for Linode test-1

How to test 🧪

Prerequisites

Verification steps

  • Spin up a Linode in DevCloud ☁️ with a Private IPv4
  • Go to NodeBalancer Create
  • Verify you can select either of the Linode's IPs because they are both technically private IPs
  • Create the NodeBalancer
  • Verify you can select either of the Linode's IPs on the NodeBalancer's details page

Note

We surface all Private IPs for each Linode but you may run into an API validation error when choosing the 172. address. This should be fine because the DevCloud team requested we show both options (even though the API may reject one of then)

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

@bnussman-akamai bnussman-akamai self-assigned this Oct 9, 2024
@bnussman-akamai bnussman-akamai requested a review from a team as a code owner October 9, 2024 02:32
@bnussman-akamai bnussman-akamai requested review from hkhalil-akamai and jaalah-akamai and removed request for a team October 9, 2024 02:32
Copy link
Member Author

Choose a reason for hiding this comment

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

The primary changes are here 🚨

@bnussman-akamai bnussman-akamai added the NodeBalancers Relating to NodeBalancers label Oct 9, 2024
Copy link

github-actions bot commented Oct 9, 2024

Coverage Report:
Base Coverage: 86.97%
Current Coverage: 86.98%

Copy link
Contributor

@hkhalil-akamai hkhalil-akamai left a comment

Choose a reason for hiding this comment

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

Tested and verified that all Private IP options are shown and can be selected. Verified that ConfigNodeIPSelect continues to work as expected.

Verifying this API error is expected when attempting to submit a 172.* IP address:
Screenshot 2024-10-09 at 3 53 26 PM

@bnussman-akamai
Copy link
Member Author

Verifying this API error is expected when attempting to submit a 172.* IP address:

Yes, this is expected!

Copy link
Contributor

@jaalah-akamai jaalah-akamai left a comment

Choose a reason for hiding this comment

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

✅ Verified linodes with multiple ips can be shown and configured

@@ -3,6 +3,8 @@ import { array, boolean, mixed, number, object, string } from 'yup';
const PORT_WARNING = 'Port must be between 1 and 65535.';
const LABEL_WARNING = 'Label must be between 3 and 32 characters.';

export const PRIVATE_IPv4_REGEX = /^10\.|^172\.1[6-9]\.|^172\.2[0-9]\.|^172\.3[0-1]\.|^192\.168\.|^fd/;

Copy link
Contributor

Choose a reason for hiding this comment

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

Much better place for this 🔥

value={options.find((o) => o.label === nodeAddress) ?? null}
/>
);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

✨ Nice refactor

@jaalah-akamai jaalah-akamai added the Approved Multiple approvals and ready to merge! label Oct 10, 2024
@bnussman-akamai bnussman-akamai merged commit 60a1925 into linode:develop Oct 11, 2024
23 checks passed
Copy link

cypress bot commented Oct 11, 2024

Cloud Manager E2E    Run #6661

Run Properties:  status check passed Passed #6661  •  git commit 60a19258fe: feat: [M3-8560] - NodeBalancer Configurations - Support Linodes with Multiple Pr...
Project Cloud Manager E2E
Run status status check passed Passed #6661
Run duration 28m 36s
Commit git commit 60a19258fe: feat: [M3-8560] - NodeBalancer Configurations - Support Linodes with Multiple Pr...
Committer Banks Nussman
View all properties for this run ↗︎

Test results
Tests that failed  Failures 0
Tests that were flaky  Flaky 4
Tests that did not run due to a developer annotating a test with .skip  Pending 2
Tests that did not run due to a failure in a mocha hook  Skipped 0
Tests that passed  Passing 435

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! NodeBalancers Relating to NodeBalancers
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants