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-7168] - Increment Subnet IPv4 value when recommending new subnet range #10010

Merged
merged 8 commits into from
Jan 18, 2024

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Dec 19, 2023

Description 📝

When creating new subnets via both the Create VPC flow and the Create Subnet drawer, we will now be incrementing the subnet range so that the default value is not a static 10.0.4.0/24 value.

Talked with Daniel about this behavior 👍 -

  • For both the VPC Create flows and Subnet Create Drawer, the recommended IP will start at 10.0.0.0/24 or 10.0.1.0/24 and go up to 10.0.1.0/24, 10.0.2.0/24, etc. 10.0.x.0/24 ips will be the only ones recommended.
    • (There was a previous version that depended more on the user's already existing IPs for the Subnet Create Drawer >> checkout and run this commit - this led to pretty frequent recommendation of "bad IPs" (ips that are technically valid but have overlapping ranges with existing ip addresses, leading to the backend throwing an error), so I switched it to current version 😢)
    • In terms of enough subnets being recommended: since a person can normally have at most 10 subnets to a VPC (special exceptions can occur), for the most part, we should not run out of subnets to recommend
    • note that due to user input, there's still a chance of a 'bad ip' being recommended (ex. if I have an IP of 10.0.0.0/8, then any 10.0.x.0/24 recommendation is already covered by 10.0.0.0/8...)
  • If we want a version where there's more robust ip-recommending, it's going to be a lot more complicated. Since these are just ip recommendations, not what people actually have to choose, this version is a bit better than the previous behavior of just recommending 10.0.4.0/24 though!

Changes 🔄

List any change relevant to the reviewer.

  • Created new helper function to recommend an IPv4 for a subnet (ty to @bnussman for the inspiration in AGLB create flow!) + add relevant tests
  • update SubnetCreateDrawer and VPCCreate flows
  • added a getAllSubnets request to get around pagination

Preview 📷

VPC Create Flow Subnet Create Drawer
Screen.Recording.2023-12-19.at.8.11.40.PM.mov
Screen.Recording.2023-12-19.at.8.07.57.PM.mov

How to test 🧪

Prerequisites

NOTE - there is a separate bug regarding SubnetCreateDrawer erroring when we try to create more than 10 subnets - see internal ticket for details

Verification steps

(How to verify changes)

  • yarn test subnets.test.ts
  • Verify that when creating subnets on VPC Create flows (drawer and page) and Subnet Create Drawer, a new subnet IP that is not in the already existing list of IPs is recommended (note - if you go past 256 subnets though, there will be duplicates)

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

@coliu-akamai coliu-akamai added the VPC Relating to VPC project label Dec 19, 2023
@coliu-akamai coliu-akamai self-assigned this Dec 19, 2023
@coliu-akamai coliu-akamai marked this pull request as ready for review December 20, 2023 16:00
@coliu-akamai coliu-akamai requested a review from a team as a code owner December 20, 2023 16:00
@coliu-akamai coliu-akamai requested review from jdamore-linode and hana-akamai and removed request for a team December 20, 2023 16:00
@@ -26,9 +27,15 @@ export const SubnetCreateDrawer = (props: Props) => {

const { data: profile } = useProfile();
const { data: grants } = useGrants();
const { data: allSubnets } = useAllSubnetsQuery(vpcId);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

rather than a getAll subnets of a VPC, we could directly get the VPC since subnets are part of the VPC's return object if that's a better approach! (we just wouldn't need any of the vpc's other information besides its subnets)

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think using useVPCQuery to get all of the subnets would be much better because it would be one less things to fetch and work about invalidating. I pushed this change in 644f882

Copy link

github-actions bot commented Jan 2, 2024

Coverage Report:
Base Coverage: 79.25%
Current Coverage: 79.29%

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.

Unit test passed and confirmed that the subnet range is increased when adding a new subnet in the Create VPC and Create Subnet flow

@hana-akamai hana-akamai added Add'tl Approval Needed Waiting on another approval! and removed Ready for Review labels Jan 8, 2024
@bnussman-akamai bnussman-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Jan 16, 2024
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.

Incrementation in VPC Create flow and Create Subnet drawer ✅
Code review ✅

@jaalah-akamai jaalah-akamai merged commit ba45d60 into linode:develop Jan 18, 2024
18 checks passed
@coliu-akamai coliu-akamai deleted the m3-7168 branch August 27, 2024 16:40
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.

6 participants