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-7289] – Payload refinements in "Assign Linodes" flow #9845

Conversation

dwiley-akamai
Copy link
Contributor

Description 📝

Currently, VPC interfaces added via the Assign Linodes flow are not marked as primary and are not 1:1 NAT'ed. This results in a sub-optimal and possibly non-functional configuration.

Changes 🔄

  • Add primary: true and nat_1_1: 'any' to the interface payload
  • If the config being modified only had an implicit public interface previously (i.e., the interfaces array comes back from the API as empty), explicitly add a public interface in eth0

How to test 🧪

Prerequisites

  • An account with the proper VPC tags
  • Pointed at the dev environment

Verification steps

With the Network tab of Dev Tools open...

  • Create a new Linode with no VPC or VLAN assigned
    • Go to a VPC in the same region as the newly-created linode with a subnet and enter the "Assign Linodes" flow. Select the newly-created linode and assign it. Observe: two requests to /interfaces, the first for adding an explicit public interface, and the second for the VPC interface (confirm it has primary: true and nat_1_1: 'any')

For other possible permutations (e.g., a linode with a VLAN already assigned), confirm that going through the "Assign Linodes" flow results in a VPC interface being added with primary: true and nat_1_1: 'any' in the payload. No explicit public interfaces should be in the payload outside of the scenario detailed above.

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

…onfig previously only had an implicit public interface in Assign Linodes flow
@dwiley-akamai dwiley-akamai added the VPC Relating to VPC project label Oct 26, 2023
@dwiley-akamai dwiley-akamai self-assigned this Oct 26, 2023
@dwiley-akamai dwiley-akamai requested a review from a team as a code owner October 26, 2023 19:44
@dwiley-akamai dwiley-akamai requested review from mjac0bs, abailly-akamai and hana-akamai and removed request for a team October 26, 2023 19:44
@@ -943,7 +943,7 @@ export class LinodeCreate extends React.PureComponent<
];
}

const defaultPublicInterface: InterfacePayload = {
export const defaultPublicInterface: InterfacePayload = {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I briefly considered moving this to src/constants.ts, but it felt out of place there and I didn't see a better spot so I just exported it from here for now.

@hana-akamai
Copy link
Contributor

Confirmed that two requests are made to /interfaces, one for adding a public interface and another for the vpc interface.

Should we add the checkbox for the public IPv4 address similar to the flow in Linodes Create?

image

@dwiley-akamai
Copy link
Contributor Author

@hana-linode I think we just take care of that with nat_1_1: 'any' so that we help the user towards a functional configuration when they go through the "Assign Linodes" flow, but we can revisit its inclusion within the flow depending on user feedback

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.

Looks good! Approving after confirmation of a few questions. ✅

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.

Looks good, thanks Dajahi!

  • confirmed two network requests sent in the first case described
  • confirmed one network request sent if linode already had vlan assigned to it

@coliu-akamai coliu-akamai added the Approved Multiple approvals and ready to merge! label Oct 30, 2023
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.

✅ Two requests to /interfaces with expected payload when no VLAN or VPC on initial create:

Screen.Recording.2023-10-30.at.2.12.41.PM.mov

✅ One request to /interfaces with expected payload when there's a VLAN on initial create:

Screen.Recording.2023-10-30.at.2.13.57.PM.mov

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.

✅ Two requests to /interfaces, the first for adding an explicit public interface, and the second for the VPC interface (confirm it has primary: true and nat_1_1: 'any')

✅ One request to /interfaces when there’s already a VLAN

@dwiley-akamai dwiley-akamai merged commit 10f760c into linode:develop Oct 31, 2023
11 checks passed
@dwiley-akamai dwiley-akamai deleted the M3-7289-assign-linodes-tweaked-payload branch October 31, 2023 19:25
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