-
Notifications
You must be signed in to change notification settings - Fork 365
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-6731] β Add VPC and Firewall sections in Linode Create flow #9635
feat: [M3-6731] β Add VPC and Firewall sections in Linode Create flow #9635
Conversation
packages/manager/src/features/Linodes/LinodesCreate/FirewallPanel.tsx
Outdated
Show resolved
Hide resolved
β¦b and the VPC dropdown list in the Linode Create flow refreshes upon successful VPC creation (given the newly-created VPC is in the same region as the to-be created Linode)
β¦oad, add account.containter.ts
β¦ --> src/components/SelectFirewallPanel and added helperText prop
β¦ fix refetchOnWindowFocus so that it works (important for having a refreshed list once a user returns from creating a VPC in a new tab)
options={firewallsDropdownOptions} | ||
placeholder={''} | ||
/> | ||
<StyledCreateLink to={`${APP_ROOT}/firewalls/create`}> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
APP_ROOT
is used to ensure the link opens up in a new tab (can't be done with a relative path without adding the external
prop which would be misleading)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I could've sworn we had this file already, but perhaps it got deleted sometime in the past year. This allows us to pull in account data in class components.
WithAccountProps & | ||
WithDisplayData & | ||
WithLinodesProps & | ||
WithTypesProps; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just alphabetically sorted and added WithAccountProps
const payload = { | ||
authorized_users: this.props.authorized_users, | ||
backup_id: this.props.selectedBackupID, | ||
backups_enabled: this.props.backupsEnabled, | ||
booted: true, | ||
firewall_id: | ||
this.props.firewallId !== -1 ? this.props.firewallId : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This check prevents the firewall_id
from being shown in the "Create Using Command Line" modal if a firewall isn't selected.
const vpcInterfaceData: InterfacePayload = { | ||
ipam_address: null, | ||
ipv4: { | ||
nat_1_1: this.props.assignPublicIPv4Address ? 'any' : undefined, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the "Assign a public IPv4 address for this Linode" checkbox is checked, send any
in the payload (otherwise, send nothing).
nat_1_1: this.props.assignPublicIPv4Address ? 'any' : undefined, | ||
vpc: this.props.autoassignIPv4WithinVPC | ||
? undefined | ||
: this.props.vpcIPv4AddressOfLinode, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If the "Auto-assign a VPC IPv4 address for this Linode in the VPC" checkbox is checked, send nothing β otherwise, send the contents of the "VPC IPv4" input field.
|
||
interfaces.push(vpcInterfaceData); | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The VLAN logic below had to be reworked to account for VPC's impact on the interfaces
property of the payload.
Essentially:
- Define
interfaces
earlier in the file as any empty array - If there's VPC data, push it to
interfaces
- If there's VLAN data, push it to
interfaces
- If there's VLAN data but no VPC data, also add a default public interface in
interfaces[0]
(maintains parity with previous logic)
attachedVLANLabel: '', | ||
authorized_users: [], | ||
autoassignIPv4WithinVPCEnabled: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
By default, the "Auto-assign a VPC IPv4 address for this Linode in the VPC" checkbox should be checked.
{ | ||
enabled, | ||
keepPreviousData: true, | ||
refetchOnWindowFocus: alwaysRefetch ? 'always' : false, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This allows for users to click the "Create VPC" link, complete that flow in the newly-opened tab, and come back to the Linode Create flow and have a refreshed VPC dropdown that includes the newly-created VPC.
purpose: mixed().oneOf( | ||
['public', 'vlan', 'vpc'], | ||
'Purpose must be public, vlan, or vpc.' | ||
export const linodeInterfaceSchema = object().shape({ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously the interfaces
property would be either a public interface + a VLAN, or omitted from the payload. Now an interface can be a VPC as well, so I've separated out individual interface validation and defined linodeInterfacesSchema
as an array of these.
Going ahead and opening this up for review but I will still be working on adding unit tests between now and tomorrow |
I do have the |
Will also be checking these off as I get to them
Additional checks (ty Tyler & Hana!)
|
Will check this off as well:
Additional checks:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Firewall assignment doesn't seem to be working π€
Screen.Recording.2023-09-26.at.1.32.51.PM.mov
In the summary paper, should we also include if we assigned any firewalls like how we do it for VPCs and VLANs? however, I can see the paper easily getting crowded and hard to quickly read. |
packages/manager/src/features/Linodes/LinodesCreate/LinodeCreateContainer.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodesCreate/VPCPanel.tsx
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodesCreate/LinodeCreateContainer.tsx
Outdated
Show resolved
Hide resolved
packages/validation/.changeset/pr-9635-changed-1695321529834.md
Outdated
Show resolved
Hide resolved
It's feature flagged separately (apart from VPC) on the back-end now. It's not working from the command line either so I've reached out to API about the issue edit: the flag is off on the back-end presently |
β¦y on validation for subnet error, update example IP address
β¦inodeCreate.tsx to retain test coverage of that aspect
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Confirm the presence of the VPC & Firewall panels in the Linode Create flow
- For the VPC panel:
- Nothing below the "Create VPC" link shows when the VPC dropdown is set to "None"
- the VPC dropdown options are only those from the same region as the region selected for the Linode
- the Subnet dropdown options are only those subnets belonging to the selected VPC
- the "Auto-assign a VPC IPv4 address for this Linode in the VPC" checkbox is checked by default
- unchecking the "Auto-assign a VPC IPv4 address for this Linode in the VPC" checkbox reveals a "VPC IPv4" text field which must be filled out (attempting to create the linode without this should yield a field error)
- expectations for the payload based on the checkboxes: a checked "Auto-assign a VPC IPv4 address for this Linode in the VPC" checkbox should result in ipv4.vpc not being present in the payload, while an unchecked one should result in the contents of the "VPC IPv4" text field populating ipv4.vpc in the payload; a checked "Assign a public IPv4 address for this Linode" should result in ipv4.nat_1_1 being set to any, while an unchecked one should result in ipv4.nat_1_1 not being present in the payload the original tab: the VPC dropdown should have your newly-created VPC included
- Clicking the "Create VPC" link and completing that flow in the new tab, then returning to the Linode Create flow in
- For the Firewall panel:
- Your firewalls should be listed as options
- The Summary paper at the bottom should say "VPC Assigned" if you've assigned a VPC
- Creating a non-VPC, non-firewalled linode with and without a VLAN
- Creating a linode with both a VPC and a VLAN
- With a non-tagged account & the VPC feature flag off, confirm (1) above is not present, and confirm normal Linode creation functionality works as expected.
Additional checks:
- Create firewall link brings you to the create firewall drawer
- Confirmed that after creating a Linode with an assigned VPC, the VPC details page lists the linode as one of the assigned Linodes
- The firewall gets assigned to the Linode
- With a non-tagged account & the VPC feature flag off, requests to the vpc endpoint should not be made
It would be nice if the errors scrolled
β¦rors; ensure VPC details in LinodeEntityDetail show upupon Linode creation for linodes assigned to a VPC
will be going through the functionality again (using my old comment to check things off) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice work! π
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Functionality looks good, awesome work!! π
Description π
Major Changes π
VPCs
added to region capabilitieslinodeInterfaceSchema
for individual interfaces,linodeInterfacesSchema
for the actualinterfaces
property in Linode Create payloadrefetchOnWindowFocus
added touseVPCsQuery
VPCPanel
&SelectFirewallPanel
components that are present in Linode Create flowlinodeCreateWithFirewall
feature flag that determines whetherSelectFirewallPanel
is displayed or not in Linode Create flowPreview π·
VPC and Firewall panels
"Private IP" conditional copy
"VPC Assigned" in summary paper
How to test π§ͺ
With a properly-tagged account or with the VPC feature flag on,
a) Nothing below the "Create VPC" link shows when the VPC dropdown is set to "None"
b) the VPC dropdown options are only those from the same region as the region selected for the Linode
c) the Subnet dropdown options are only those subnets belonging to the selected VPC
d) the "Auto-assign a VPC IPv4 address for this Linode in the VPC" checkbox is checked by default
e) unchecking the "Auto-assign a VPC IPv4 address for this Linode in the VPC" checkbox reveals a "VPC IPv4" text field which must be filled out (attempting to create the linode without this should yield a field error)
f) expectations for the payload based on the checkboxes: a checked "Auto-assign a VPC IPv4 address for this Linode in the VPC" checkbox should result in
ipv4.vpc
not being present in the payload, while an unchecked one should result in the contents of the "VPC IPv4" text field populatingipv4.vpc
in the payload; a checked "Assign a public IPv4 address for this Linode" should result inipv4.nat_1_1
being set toany
, while an unchecked one should result inipv4.nat_1_1
not being present in the payload the original tab: the VPC dropdown should have your newly-created VPC includedg) Clicking the "Create VPC" link and completing that flow in the new tab, then returning to the Linode Create flow in
a) Your firewalls should be listed as options
b) You can add
linodeCreateWithFirewall: false
somewhere between L30-L33 inwithFeatureFlagConsumer.container.ts
to confirm the Firewall panel is not visible without the feature flag being onWith a non-tagged account & the VPC feature flag off, confirm (1) above is not present, and confirm normal Linode creation functionality works as expected.