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

change: [M3-7250] – Properly surface VPC interface errors #9839

Conversation

dwiley-akamai
Copy link
Contributor

@dwiley-akamai dwiley-akamai commented Oct 24, 2023

Description πŸ“

There was an API bug for API endpoints that take a list of interfaces where, if there was an error for one of the interfaces, the field of the error didn't indicate which interface index it applied to. This presented difficulties for surfacing the errors in the UI. Temporary code was added in #9709 to surface the errors in a general way just so that they wouldn't stay submerged, but since the API fix has been merged in, that temporary code is being removed so the errors surface and map to the appropriate interfaces.

Changes πŸ”„

  • Remove temporary code in LinodeConfigDialog.tsx
  • Move publicIPv4Error outside of <Box /> in VPCPanel.tsx to avoid odd formatting of error in the Add/Edit Linode Config dialog

Preview πŸ“·

(for the publicIPv4Error error change -- this error is only really possible to get back in the Linode Config flow, not the Linode Create flow)

Before After
Screenshot 2023-10-24 at 4 22 12 PM Screenshot 2023-10-24 at 4 21 57 PM

How to test πŸ§ͺ

Prerequisites

  • Account with proper VPC tags
  • Point at dev environment

Verification steps

Try adding a Linode config or editing an existing config:

  • Choose a VPC interface, uncheck the "Auto-assign a VPC IPv4 address for this Linode in the VPC" checkbox, and input a reserved IP in the "VPC IPv4" field (example: if the subnet's range is 10.0.4.0/24, you can input 10.0.4.0 in that field).
    • Observe: when you submit the form, you should see a field error: "The provided IP is reserved"
  • Try adding two VPC interfaces and check the "Assign a public IPv4 address for this Linode" checkbox for both of them
    • Observe: the second one should get a "Linode has no public IP to use for 1:1 NAT" error

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

@dwiley-akamai dwiley-akamai added the VPC Relating to VPC project label Oct 24, 2023
@dwiley-akamai dwiley-akamai self-assigned this Oct 24, 2023
@dwiley-akamai dwiley-akamai requested a review from a team as a code owner October 24, 2023 20:41
@dwiley-akamai dwiley-akamai requested review from mjac0bs and carrillo-erik and removed request for a team October 24, 2023 20:41
…rrors and fixed formatting of error in Linode Config dialog
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.

I observed both errors following the provided testing instructions. βœ…

One clarification question:

Try adding two VPC interfaces and check the "Assign a public IPv4 address for this Linode" checkbox for both of them

This means two distinct VPC interfaces, correct? A user is allowed to configure the same VPC on multiple network interfaces, and should not expect to see that error?

Screenshot 2023-10-25 at 8 39 04 AM

…86838.md

Co-authored-by: Mariah Jacobs <114685994+mjac0bs@users.noreply.github.com>
@dwiley-akamai
Copy link
Contributor Author

One clarification question:

Try adding two VPC interfaces and check the "Assign a public IPv4 address for this Linode" checkbox for both of them

This means two distinct VPC interfaces, correct? A user is allowed to configure the same VPC on multiple network interfaces, and should not expect to see that error?

I tried assigning the same VPC and subnet for each interface and observed the error πŸ€”

In verifying this, though, I did come across a bug earlier: trying to submit an initial combo that yields the error, and then changing the selected VPC and subnet resulted in a successful submission despite that interface slot's "Assign a public IPv4 address for this Linode" checkbox being checked. That may have been what you were actually running into?

I believe this was happening because handleVPCLabelChange() and handleSubnetChange() weren't accounting for ipv4.nat_1_1 in the object, so it was getting excluded from the payload on the final submission. This should be fixed in the next commit

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.

trying to submit an initial combo that yields the error, and then changing the selected VPC and subnet resulted in a successful submission despite that interface slot's "Assign a public IPv4 address for this Linode" checkbox being checked. That may have been what you were actually running into?

Hmm, must have been? I thought the first attempt that I made was with assinging the same VPC and subnet to eth0 and eth1, and did not see the expected error, at which point I went on to try (and see it successfully) with two different VPCs and subnets. In any case, I retested with the latest changes and am consistently seeing the expected Linode has no public IP to use for 1:1 NAT error when creating/editing an interface with two VPC interfaces and checked "Assign a public IPv4 address for this Linode", regardless of whether the VPCs were the same, different, or changed and then resubmitted.

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

Confirmed both errors surface βœ…

Inputting a reserved IP Two VPC interfaces with assign IPv4
Screenshot 2023-10-30 at 1 36 11 PM Screenshot 2023-10-30 at 1 35 53 PM

@hana-akamai hana-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Oct 30, 2023
@dwiley-akamai dwiley-akamai merged commit ccff45b into linode:develop Oct 31, 2023
11 checks passed
@dwiley-akamai dwiley-akamai deleted the M3-7250-properly-surface-vpc-interface-errors branch October 31, 2023 16:14
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