From 9c9c5beeff318122be9b8e33e4ba9dd7fdf38727 Mon Sep 17 00:00:00 2001 From: Dajahi Wiley Date: Fri, 17 Nov 2023 12:37:12 -0500 Subject: [PATCH 1/5] Display conditional notices in Config dialog depending on configuration scenarios --- .../LinodeConfigs/LinodeConfigDialog.tsx | 128 ++++++++++++++---- .../manager/src/features/VPCs/constants.ts | 10 ++ 2 files changed, 108 insertions(+), 30 deletions(-) diff --git a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx index b14a87507b6..803f0ece195 100644 --- a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx +++ b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx @@ -35,6 +35,11 @@ import { TooltipIcon } from 'src/components/TooltipIcon'; import { Typography } from 'src/components/Typography'; import { DeviceSelection } from 'src/features/Linodes/LinodesDetail/LinodeRescue/DeviceSelection'; import { titlecase } from 'src/features/Linodes/presentation'; +import { + LINODE_UNREACHABLE_HELPER_TEXT, + NATTED_PUBLIC_IP_HELPER_TEXT, + NOT_NATTED_HELPER_TEXT, +} from 'src/features/VPCs/constants'; import { useAccountManagement } from 'src/hooks/useAccountManagement'; import { useFlags } from 'src/hooks/useFlags'; import { @@ -986,36 +991,44 @@ export const LinodeConfigDialog = (props: Props) => { )} {values.interfaces.map((thisInterface, idx) => { return ( - - handleInterfaceChange(idx, newInterface) - } - ipamAddress={thisInterface.ipam_address} - key={`eth${idx}-interface`} - label={thisInterface.label} - nattedIPv4Address={thisInterface.ipv4?.nat_1_1} - purpose={thisInterface.purpose} - readOnly={isReadOnly} - region={linode?.region} - regionHasVLANs={regionHasVLANS} - regionHasVPCs={regionHasVPCs} - slotNumber={idx} - subnetId={thisInterface.subnet_id} - vpcIPv4={thisInterface.ipv4?.vpc} - vpcId={thisInterface.vpc_id} - /> + <> + {unrecommendedConfigNoticeSelector({ + _interface: thisInterface, + primaryInterfaceIndex, + thisIndex: idx, + values, + })} + + handleInterfaceChange(idx, newInterface) + } + ipamAddress={thisInterface.ipam_address} + key={`eth${idx}-interface`} + label={thisInterface.label} + nattedIPv4Address={thisInterface.ipv4?.nat_1_1} + purpose={thisInterface.purpose} + readOnly={isReadOnly} + region={linode?.region} + regionHasVLANs={regionHasVLANS} + regionHasVPCs={regionHasVPCs} + slotNumber={idx} + subnetId={thisInterface.subnet_id} + vpcIPv4={thisInterface.ipv4?.vpc} + vpcId={thisInterface.vpc_id} + /> + ); })} @@ -1181,3 +1194,58 @@ const isUsingCustomRoot = (value: string) => '/dev/sdg', '/dev/sdh', ].includes(value) === false; + +const noticeForScenario = (scenarioText: string) => ( + +); + +const unrecommendedConfigNoticeSelector = ({ + _interface, + primaryInterfaceIndex, + thisIndex, + values, +}: { + _interface: ExtendedInterface; + primaryInterfaceIndex: number | undefined; + thisIndex: number; + values: EditableFields; +}): JSX.Element | null => { + const publicInterfaceInEth0 = values.interfaces[0].purpose === 'public'; + const vpcInterface = _interface.purpose === 'vpc'; + const nattedIPv4Address = Boolean(_interface.ipv4?.nat_1_1); + + const filteredInterfaces = values.interfaces.filter( + (_interface) => _interface.purpose !== 'none' + ); + + /* + Scenario 1: + - Public interface in eth0 + - the interface passed in to this function is a VPC interface + - the index of the primary interface !== the index of the interface passed in to this function + - nattedIPv4Address (i.e., "Assign a public IPv4 address for this Linode" checked) + + Scenario 2: + - all of Scenario 1, except: !nattedIPv4Address (i.e., "Assign a public IPv4 address for this Linode" unchecked) + + Scenario 3: + - only eth0 populated, and it is a VPC interface + + If not one of the above scenarios, do not display a warning notice re: configuration + */ + if ( + publicInterfaceInEth0 && + vpcInterface && + primaryInterfaceIndex !== thisIndex + ) { + return nattedIPv4Address + ? noticeForScenario(NATTED_PUBLIC_IP_HELPER_TEXT) + : noticeForScenario(LINODE_UNREACHABLE_HELPER_TEXT); + } + + if (filteredInterfaces.length === 1 && vpcInterface && !nattedIPv4Address) { + return noticeForScenario(NOT_NATTED_HELPER_TEXT); + } + + return null; +}; diff --git a/packages/manager/src/features/VPCs/constants.ts b/packages/manager/src/features/VPCs/constants.ts index 0bbc03f0cc4..9ec24503a32 100644 --- a/packages/manager/src/features/VPCs/constants.ts +++ b/packages/manager/src/features/VPCs/constants.ts @@ -27,3 +27,13 @@ export const VPC_CREATE_FORM_VPC_HELPER_TEXT = export const VPC_FEEDBACK_FORM_URL = 'https://docs.google.com/forms/d/e/1FAIpQLScvWbTupCNsBF5cz5YEsv5oErHM4ONBZodDYi8KuOgC8fyfag/viewform'; + +// Linode Config dialog helper text for unrecommended configurations +export const LINODE_UNREACHABLE_HELPER_TEXT = + 'This network configuration is not recommended. The Linode will not be reachable or be able to reach Linodes in the other subnets of the VPC. We recommend selecting VPC as the primary interface and checking the “Assign a public IPv4 address for this Linode” checkbox.'; + +export const NATTED_PUBLIC_IP_HELPER_TEXT = + "This network configuration is not recommended. The VPC interface with 1:1 NAT will use the Public IP even if it's not on the default network interface. The Linode will lose access to public network connectivity since the default route isn't able to route through the public IPv4 address. We recommend selecting VPC as the primary interface."; + +export const NOT_NATTED_HELPER_TEXT = + 'The Linode will not be able to access the internet. If this Linode needs access to the internet we recommend checking the “Assign a public IPv4 address for this Linode” checkbox to enable 1:1 NAT on the VPC interface.'; From 37357d9d92c23f6deac3d196c5c1625fc75516cf Mon Sep 17 00:00:00 2001 From: Dajahi Wiley Date: Mon, 20 Nov 2023 12:02:08 -0500 Subject: [PATCH 2/5] Added changeset --- .../.changeset/pr-9916-upcoming-features-1700499528271.md | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 packages/manager/.changeset/pr-9916-upcoming-features-1700499528271.md diff --git a/packages/manager/.changeset/pr-9916-upcoming-features-1700499528271.md b/packages/manager/.changeset/pr-9916-upcoming-features-1700499528271.md new file mode 100644 index 00000000000..1ffb3f1cb5f --- /dev/null +++ b/packages/manager/.changeset/pr-9916-upcoming-features-1700499528271.md @@ -0,0 +1,5 @@ +--- +"@linode/manager": Upcoming Features +--- + +Display warning notices for unrecommended configurations in Linode Add/Edit Config dialog ([#9916](https://github.com/linode/manager/pull/9916)) From 06bdda1e8a3887c81e88f714f89c36991f50ca80 Mon Sep 17 00:00:00 2001 From: Dajahi Wiley Date: Mon, 20 Nov 2023 15:31:15 -0500 Subject: [PATCH 3/5] Add unit tests and some documentation within code --- .../LinodeConfigs/LinodeConfigDialog.test.tsx | 124 +++++++++++++++++- .../LinodeConfigs/LinodeConfigDialog.tsx | 18 ++- 2 files changed, 136 insertions(+), 6 deletions(-) diff --git a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.test.tsx b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.test.tsx index 0ae8fb3e5cf..abb52f90b47 100644 --- a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.test.tsx +++ b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.test.tsx @@ -1,14 +1,22 @@ import { waitFor } from '@testing-library/react'; import * as React from 'react'; -import { configFactory } from 'src/factories'; +import { LinodeConfigInterfaceFactory, configFactory } from 'src/factories'; import { accountFactory } from 'src/factories/account'; +import { + LINODE_UNREACHABLE_HELPER_TEXT, + NATTED_PUBLIC_IP_HELPER_TEXT, + NOT_NATTED_HELPER_TEXT, +} from 'src/features/VPCs/constants'; import { rest, server } from 'src/mocks/testServer'; import { queryClientFactory } from 'src/queries/base'; import { mockMatchMedia, renderWithTheme } from 'src/utilities/testHelpers'; -import { padList } from './LinodeConfigDialog'; -import { LinodeConfigDialog } from './LinodeConfigDialog'; +import { + LinodeConfigDialog, + unrecommendedConfigNoticeSelector, +} from './LinodeConfigDialog'; +import { MemoryLimit, padList } from './LinodeConfigDialog'; const queryClient = queryClientFactory(); @@ -68,4 +76,114 @@ describe('LinodeConfigDialog', () => { }); }); }); + + const publicInterface = LinodeConfigInterfaceFactory.build({ + primary: true, + purpose: 'public', + }); + + const vpcInterface = LinodeConfigInterfaceFactory.build({ + ipv4: { + nat_1_1: '10.0.0.0', + }, + primary: false, + purpose: 'vpc', + }); + + const vpcInterfaceWithoutNAT = LinodeConfigInterfaceFactory.build({ + primary: false, + purpose: 'vpc', + }); + + const editableFields = { + devices: {}, + helpers: { + devtmpfs_automount: true, + distro: true, + modules_dep: true, + network: true, + updatedb_disabled: true, + }, + initrd: null, + interfaces: [publicInterface, vpcInterface], + label: 'Test', + root_device: '/dev/sda', + setMemoryLimit: 'no_limit' as MemoryLimit, + useCustomRoot: false, + }; + + describe('unrecommendedConfigNoticeSelector function', () => { + it('should return a with NATTED_PUBLIC_IP_HELPER_TEXT under the appropriate conditions', () => { + const valueReturned = unrecommendedConfigNoticeSelector({ + _interface: vpcInterface, + primaryInterfaceIndex: editableFields.interfaces.findIndex( + (element) => element.primary === true + ), + thisIndex: editableFields.interfaces.findIndex( + (element) => element.purpose === 'vpc' + ), + values: editableFields, + }); + + expect(valueReturned?.props.text).toEqual(NATTED_PUBLIC_IP_HELPER_TEXT); + }); + + it('should return a with LINODE_UNREACHABLE_HELPER_TEXT under the appropriate conditions', () => { + const editableFieldsWithVPCInterfaceNotNatted = { + ...editableFields, + interfaces: [publicInterface, vpcInterfaceWithoutNAT], + }; + + const valueReturned = unrecommendedConfigNoticeSelector({ + _interface: vpcInterfaceWithoutNAT, + primaryInterfaceIndex: editableFields.interfaces.findIndex( + (element) => element.primary === true + ), + thisIndex: editableFields.interfaces.findIndex( + (element) => element.purpose === 'vpc' + ), + values: editableFieldsWithVPCInterfaceNotNatted, + }); + + expect(valueReturned?.props.text).toEqual(LINODE_UNREACHABLE_HELPER_TEXT); + }); + + it('should return a with NOT_NATTED_HELPER_TEXT under the appropriate conditions', () => { + const editableFieldsWithSingleInterface = { + ...editableFields, + interfaces: [vpcInterfaceWithoutNAT], + }; + + const valueReturned = unrecommendedConfigNoticeSelector({ + _interface: vpcInterfaceWithoutNAT, + primaryInterfaceIndex: editableFieldsWithSingleInterface.interfaces.findIndex( + (element) => element.primary === true + ), + thisIndex: editableFieldsWithSingleInterface.interfaces.findIndex( + (element) => element.purpose === 'vpc' + ), + values: editableFieldsWithSingleInterface, + }); + + expect(valueReturned?.props.text).toEqual(NOT_NATTED_HELPER_TEXT); + }); + + it('should not return a outside of the prescribed conditions', () => { + const editableFieldsWithoutVPCInterface = { + ...editableFields, + interfaces: [publicInterface], + }; + + const valueReturned = unrecommendedConfigNoticeSelector({ + _interface: publicInterface, + primaryInterfaceIndex: editableFieldsWithoutVPCInterface.interfaces.findIndex( + (element) => element.primary === true + ), + thisIndex: 0, + values: editableFieldsWithoutVPCInterface, + }); + + expect(valueReturned?.props.text).toBe(undefined); + }); + }); }); diff --git a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx index 803f0ece195..96983694cf9 100644 --- a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx +++ b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx @@ -84,7 +84,7 @@ interface Helpers { type RunLevel = 'binbash' | 'default' | 'single'; type VirtMode = 'fullvirt' | 'paravirt'; -type MemoryLimit = 'no_limit' | 'set_limit'; +export type MemoryLimit = 'no_limit' | 'set_limit'; interface EditableFields { comments?: string; @@ -1196,10 +1196,22 @@ const isUsingCustomRoot = (value: string) => ].includes(value) === false; const noticeForScenario = (scenarioText: string) => ( - + ); -const unrecommendedConfigNoticeSelector = ({ +/** + * + * @param _interface the current config interface being passed in + * @param primaryInterfaceIndex the index of the primary interface + * @param thisIndex the index of the current config interface within the `interfaces` array of the `config` object + * @param values the values held in Formik state, having a type of `EditableFields` + * @returns JSX.Element | null + */ +export const unrecommendedConfigNoticeSelector = ({ _interface, primaryInterfaceIndex, thisIndex, From c69547ee2d02f205d10c7f142b26dc4f7721c8b5 Mon Sep 17 00:00:00 2001 From: Dajahi Wiley Date: Tue, 21 Nov 2023 14:05:56 -0500 Subject: [PATCH 4/5] Update unrecommendedConfigNoticeSelector() logic and revise unit tests --- .../LinodeConfigs/LinodeConfigDialog.test.tsx | 9 +++++++-- .../LinodeConfigs/LinodeConfigDialog.tsx | 12 ++++++++---- 2 files changed, 15 insertions(+), 6 deletions(-) diff --git a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.test.tsx b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.test.tsx index abb52f90b47..d139fa5af59 100644 --- a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.test.tsx +++ b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.test.tsx @@ -149,13 +149,18 @@ describe('LinodeConfigDialog', () => { }); it('should return a with NOT_NATTED_HELPER_TEXT under the appropriate conditions', () => { + const vpcInterfacePrimaryWithoutNAT = { + ...vpcInterfaceWithoutNAT, + primary: true, + }; + const editableFieldsWithSingleInterface = { ...editableFields, - interfaces: [vpcInterfaceWithoutNAT], + interfaces: [vpcInterfacePrimaryWithoutNAT], }; const valueReturned = unrecommendedConfigNoticeSelector({ - _interface: vpcInterfaceWithoutNAT, + _interface: vpcInterfacePrimaryWithoutNAT, primaryInterfaceIndex: editableFieldsWithSingleInterface.interfaces.findIndex( (element) => element.primary === true ), diff --git a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx index 96983694cf9..e8186009ba7 100644 --- a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx +++ b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx @@ -1222,7 +1222,6 @@ export const unrecommendedConfigNoticeSelector = ({ thisIndex: number; values: EditableFields; }): JSX.Element | null => { - const publicInterfaceInEth0 = values.interfaces[0].purpose === 'public'; const vpcInterface = _interface.purpose === 'vpc'; const nattedIPv4Address = Boolean(_interface.ipv4?.nat_1_1); @@ -1230,9 +1229,14 @@ export const unrecommendedConfigNoticeSelector = ({ (_interface) => _interface.purpose !== 'none' ); + // Edge case: users w/ ability to have multiple VPC interfaces. Scenario 1 & 2 notices not helpful if that's done + const primaryInterfaceIsVPC = + primaryInterfaceIndex !== undefined + ? values.interfaces[primaryInterfaceIndex].purpose === 'vpc' + : false; + /* Scenario 1: - - Public interface in eth0 - the interface passed in to this function is a VPC interface - the index of the primary interface !== the index of the interface passed in to this function - nattedIPv4Address (i.e., "Assign a public IPv4 address for this Linode" checked) @@ -1246,9 +1250,9 @@ export const unrecommendedConfigNoticeSelector = ({ If not one of the above scenarios, do not display a warning notice re: configuration */ if ( - publicInterfaceInEth0 && vpcInterface && - primaryInterfaceIndex !== thisIndex + primaryInterfaceIndex !== thisIndex && + !primaryInterfaceIsVPC ) { return nattedIPv4Address ? noticeForScenario(NATTED_PUBLIC_IP_HELPER_TEXT) From a6508792f1b25da6772063fe29924be943b0f9bf Mon Sep 17 00:00:00 2001 From: Dajahi Wiley Date: Wed, 22 Nov 2023 14:51:08 -0500 Subject: [PATCH 5/5] Minor cleanup, changes to lay foundation for addressing interfaces[idx].primary issue --- .../LinodeConfigs/LinodeConfigDialog.tsx | 2 ++ .../LinodeSettings/InterfaceSelect.tsx | 19 +++++++++++-------- 2 files changed, 13 insertions(+), 8 deletions(-) diff --git a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx index e8186009ba7..5b353e408ea 100644 --- a/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx +++ b/packages/manager/src/features/Linodes/LinodesDetail/LinodeConfigs/LinodeConfigDialog.tsx @@ -1003,6 +1003,8 @@ export const LinodeConfigDialog = (props: Props) => { ipamError: formik.errors[`interfaces[${idx}].ipam_address`], labelError: formik.errors[`interfaces[${idx}].label`], + primaryError: + formik.errors[`interfaces[${idx}].primary`], publicIPv4Error: formik.errors[`interfaces[${idx}].ipv4.nat_1_1`], subnetError: diff --git a/packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/InterfaceSelect.tsx b/packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/InterfaceSelect.tsx index 0ca6633a984..6a299c39682 100644 --- a/packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/InterfaceSelect.tsx +++ b/packages/manager/src/features/Linodes/LinodesDetail/LinodeSettings/InterfaceSelect.tsx @@ -11,15 +11,16 @@ import { Divider } from 'src/components/Divider'; import Select, { Item } from 'src/components/EnhancedSelect/Select'; import { Stack } from 'src/components/Stack'; import { TextField } from 'src/components/TextField'; +import { Typography } from 'src/components/Typography'; import { VPCPanel } from 'src/features/Linodes/LinodesCreate/VPCPanel'; import { useFlags } from 'src/hooks/useFlags'; import { useAccount } from 'src/queries/account'; import { useVlansQuery } from 'src/queries/vlans'; import { isFeatureEnabled } from 'src/utilities/accountCapabilities'; import { sendLinodeCreateDocsEvent } from 'src/utilities/analytics'; -import { Typography } from 'src/components/Typography'; interface Props { + errors: VPCInterfaceErrors & OtherInterfaceErrors; fromAddonsPanel?: boolean; handleChange: (updatedInterface: ExtendedInterface) => void; ipamAddress?: null | string; @@ -27,13 +28,11 @@ interface Props { purpose: ExtendedPurpose; readOnly: boolean; region?: string; - slotNumber: number; regionHasVLANs?: boolean; regionHasVPCs?: boolean; + slotNumber: number; } - -interface VPCStateErrors { - ipamError?: string; +interface VPCInterfaceErrors { labelError?: string; publicIPv4Error?: string; subnetError?: string; @@ -41,8 +40,12 @@ interface VPCStateErrors { vpcIPv4Error?: string; } +interface OtherInterfaceErrors { + ipamError?: string; + primaryError?: string; +} + interface VPCState { - errors: VPCStateErrors; nattedIPv4Address?: string; subnetId?: null | number; vpcIPv4?: string; @@ -69,12 +72,12 @@ export const InterfaceSelect = (props: CombinedProps) => { purpose, readOnly, region, + regionHasVLANs, + regionHasVPCs, slotNumber, subnetId, vpcIPv4, vpcId, - regionHasVLANs, - regionHasVPCs, } = props; const theme = useTheme();