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-7645] – Support VPC IPv4 Ranges in Add/Edit Linode Config dialog #10170

Conversation

dwiley-akamai
Copy link
Contributor

@dwiley-akamai dwiley-akamai commented Feb 9, 2024

Description πŸ“

Add support for VPC IPv4 ranges in Add/Edit Linode Config dialog

Changes πŸ”„

  • Remove some vpcEnabled/isFeatureEnabled logic in LinodeConfigDialog.tsx
  • Logic to include ip_ranges in config payload, display individual IP range errors

Preview πŸ“·

Starting state, before any ranges have been assigned

Screenshot 2024-02-09 at 1 41 29 PM

How to test πŸ§ͺ

Verification steps

For a Linode already assigned to a VPC, go to Linode Details > Configurations > Edit and confirm that the info displayed for the VPC interface is correct (particularly the IP ranges -- existing IP ranges should be displayed, but if there are none, you should just see the "Assign additional IPv4 ranges" section with no input fields to start).

Try adding and deleting IP ranges and ensure the payload (PUT request to linode/instances/{linode_id}/configs/{config_id}) and UI matches what you'd expect.

Note: The CIDR notation (e.g., /24) of the range needs to be narrower (i.e., a larger number) than the subnet IP range. For example, if the subnet IP range has /16, the CIDR notation for the ranges could be /24.

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 Feb 9, 2024
@dwiley-akamai dwiley-akamai self-assigned this Feb 9, 2024
Comment on lines +983 to +997
const thisInterfaceIPRanges: ExtendedIP[] = (
thisInterface.ip_ranges ?? []
).map((ip_range, index) => {
// Display a more user-friendly error to the user as opposed to, for example, "interfaces[1].ip_ranges[1] is invalid"
const errorString: string = formik.errors[
`interfaces[${idx}].ip_ranges[${index}]`
]?.includes('is invalid')
? 'Invalid IP range'
: formik.errors[`interfaces[${idx}].ip_ranges[${index}]`];

return {
address: ip_range,
error: errorString,
};
});
Copy link
Contributor Author

@dwiley-akamai dwiley-akamai Feb 9, 2024

Choose a reason for hiding this comment

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

Because these are deeply nested errors that we're trying to surface, it required a bit of less-than-ideal logic and mapping.

I am aware of one unfortunate shortcoming–if you have multiple IP ranges but one of them (not the last one in the sequence) gets an error when you click "Save Changes", clicking the "X" to remove that IP range will result in the error moving onto the range that moves up to occupy that index. Sadly, I could not see a way around this, as I tried having something like ipRangeIndicesWithErrors in component state or as a ref to track the indices so that the errors could be cleared from inside handleInterfaceChange or handleChange, but that was not fruitful.

@dwiley-akamai dwiley-akamai marked this pull request as ready for review February 9, 2024 19:28
@dwiley-akamai dwiley-akamai requested a review from a team as a code owner February 9, 2024 19:28
@dwiley-akamai dwiley-akamai requested review from hana-akamai, abailly-akamai and jaalah-akamai and removed request for a team February 9, 2024 19:28
Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

The Linode Config dialog is working as I'd expect! Error handling looks good too πŸŽ‰

Screenshot 2024-02-12 at 11 46 21 AM

There is some weirdness with this specific case, but I don't think we need to address this right now.

Screen.Recording.2024-02-12.at.11.47.30.AM.mov

Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

LGTM! confirming on "Assign additional IPv4 ranges"

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.

UI and code looks good, behavior works as expected βœ…

The layout with a large right col is a bit odd. Maybe it can be improved in the future

Screenshot 2024-02-12 at 11 56 01

Copy link
Contributor

@hana-akamai hana-akamai left a comment

Choose a reason for hiding this comment

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

VPC IPv4 Configs LGTM other than the mentioned error cases

) . Each Linode can have up to three Network Interfaces. For more
information, see our{' '}
Configure the network that a selected interface will connect to
"Public Internet", VLAN, or VPC. Each Linode can have up to
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need the "?

Suggested change
"Public Internet", VLAN, or VPC. Each Linode can have up to
"Public Internet", VLAN, or VPC. Each Linode can have up to

Copy link
Contributor Author

Choose a reason for hiding this comment

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

ESLint recommended escaping them

Screenshot 2024-02-12 at 12 34 48 PM

Copy link

github-actions bot commented Feb 12, 2024

Coverage Report: βœ…
Base Coverage: 81.28%
Current Coverage: 81.28%

@dwiley-akamai dwiley-akamai merged commit 9178a45 into linode:staging-off-release Feb 12, 2024
17 of 18 checks passed
@dwiley-akamai dwiley-akamai deleted the M3-7645-clean-ip-ranges-config-dialog branch February 12, 2024 18:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
VPC Relating to VPC project
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants