diff --git a/packages/api-v4/.changeset/pr-11677-changed-1739995208483.md b/packages/api-v4/.changeset/pr-11677-changed-1739995208483.md new file mode 100644 index 00000000000..add94420352 --- /dev/null +++ b/packages/api-v4/.changeset/pr-11677-changed-1739995208483.md @@ -0,0 +1,5 @@ +--- +"@linode/api-v4": Changed +--- + +Make `label` field in `CreateFirewallPayload` required ([#11677](https://github.com/linode/manager/pull/11677)) diff --git a/packages/api-v4/src/firewalls/types.ts b/packages/api-v4/src/firewalls/types.ts index 859b3ad9402..4f5eca2dbce 100644 --- a/packages/api-v4/src/firewalls/types.ts +++ b/packages/api-v4/src/firewalls/types.ts @@ -67,14 +67,14 @@ export interface FirewallTemplate { } export interface CreateFirewallPayload { - label?: string; + label: string; tags?: string[]; rules: UpdateFirewallRules; devices?: { linodes?: number[]; nodebalancers?: number[]; interfaces?: number[]; - }; + } | null; } export interface UpdateFirewallPayload { diff --git a/packages/manager/.changeset/pr-11677-tech-stories-1739897652667.md b/packages/manager/.changeset/pr-11677-tech-stories-1739897652667.md new file mode 100644 index 00000000000..a2a30278ccc --- /dev/null +++ b/packages/manager/.changeset/pr-11677-tech-stories-1739897652667.md @@ -0,0 +1,5 @@ +--- +"@linode/manager": Tech Stories +--- + +Refactor CreateFirewallDrawer to use `react-hook-form` ([#11677](https://github.com/linode/manager/pull/11677)) diff --git a/packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx b/packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx index 1465f91002b..2f25514fadd 100644 --- a/packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx +++ b/packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx @@ -1,4 +1,5 @@ /* eslint-disable jsx-a11y/anchor-is-valid */ +import { yupResolver } from '@hookform/resolvers/yup'; import { Box, FormControlLabel, @@ -9,9 +10,9 @@ import { Typography, } from '@linode/ui'; import { CreateFirewallSchema } from '@linode/validation/lib/firewalls.schema'; -import { useFormik } from 'formik'; import { useSnackbar } from 'notistack'; import * as React from 'react'; +import { Controller, useForm } from 'react-hook-form'; import { useLocation } from 'react-router-dom'; import { ActionsPanel } from 'src/components/ActionsPanel/ActionsPanel'; @@ -28,17 +29,16 @@ import { sendLinodeCreateFormInputEvent, sendLinodeCreateFormStepEvent, } from 'src/utilities/analytics/formEventAnalytics'; -import { getErrorMap } from 'src/utilities/errorUtils'; -import { - handleFieldErrors, - handleGeneralErrors, -} from 'src/utilities/formikErrorUtils'; import { getEntityIdsByPermission } from 'src/utilities/grants'; import { getQueryParamsFromQueryString } from 'src/utilities/queryParams'; import { + FIREWALL_HELPER_TEXT, + FIREWALL_LABEL_TEXT, LINODE_CREATE_FLOW_TEXT, NODEBALANCER_CREATE_FLOW_TEXT, + NODEBALANCER_HELPER_TEXT, + READ_ONLY_DEVICES_HIDDEN_MESSAGE, } from './constants'; import type { @@ -51,10 +51,6 @@ import type { import type { LinodeCreateQueryParams } from 'src/features/Linodes/types'; import type { LinodeCreateFormEventOptions } from 'src/utilities/analytics/types'; -export const READ_ONLY_DEVICES_HIDDEN_MESSAGE = - 'Only services you have permission to modify are shown.'; -const NODEBALANCER_HELPER_TEXT = `Only the firewall's inbound rules apply to NodeBalancers.`; - export interface CreateFirewallDrawerProps { createFlow: FirewallDeviceEntityType | undefined; onClose: () => void; @@ -80,7 +76,7 @@ export const CreateFirewallDrawer = React.memo( const { createFlow, onClose, onFirewallCreated, open } = props; const { _hasGrant, _isRestrictedUser } = useAccountManagement(); const { data: grants } = useGrants(); - const { mutateAsync } = useCreateFirewall(); + const { mutateAsync: createFirewall } = useCreateFirewall(); const { data } = useAllFirewallsQuery(open); const { enqueueSnackbar } = useSnackbar(); @@ -99,104 +95,47 @@ export const CreateFirewallDrawer = React.memo( }; const { - errors, - handleBlur, - handleChange, + control, + formState: { errors, isSubmitting }, handleSubmit, - isSubmitting, - resetForm, - setFieldValue, - status, - values, - } = useFormik({ - initialValues, - onSubmit( - values: CreateFirewallPayload, - { setErrors, setStatus, setSubmitting } - ) { - // Clear drawer error state - setStatus(undefined); - setErrors({}); - const payload = { ...values }; + reset, + setError, + } = useForm({ + defaultValues: initialValues, + mode: 'onBlur', + resolver: yupResolver(CreateFirewallSchema), + values: initialValues, + }); - if (payload.label === '') { - payload.label = undefined; - } + const handleClose = () => { + onClose(); + reset(); + }; - if ( - Array.isArray(payload.rules.inbound) && - payload.rules.inbound.length === 0 - ) { - payload.rules.inbound = undefined; - } + const createCustomFirewall = async (values: CreateFirewallPayload) => { + try { + const firewall = await createFirewall(values); + enqueueSnackbar(`Firewall ${values.label} successfully created`, { + variant: 'success', + }); - if ( - Array.isArray(payload.rules.outbound) && - payload.rules.outbound.length === 0 - ) { - payload.rules.outbound = undefined; + if (onFirewallCreated) { + onFirewallCreated(firewall); } - - mutateAsync(payload) - .then((response) => { - setSubmitting(false); - enqueueSnackbar(`Firewall ${payload.label} successfully created`, { - variant: 'success', - }); - - if (onFirewallCreated) { - onFirewallCreated(response); - } - onClose(); - - // Fire analytics form submit upon successful firewall creation from Linode Create flow. - if (isFromLinodeCreate) { - sendLinodeCreateFormStepEvent({ - ...firewallFormEventOptions, - label: 'Create Firewall', - }); - } - }) - .catch((err) => { - const mapErrorToStatus = () => - setStatus({ generalError: getErrorMap([], err).none }); - - setSubmitting(false); - handleFieldErrors(setErrors, err); - handleGeneralErrors( - mapErrorToStatus, - err, - 'Error creating Firewall.' - ); + handleClose(); + // Fire analytics form submit upon successful firewall creation from Linode Create flow. + if (isFromLinodeCreate) { + sendLinodeCreateFormStepEvent({ + ...firewallFormEventOptions, + label: 'Create Firewall', }); - }, - validateOnBlur: false, - validateOnChange: false, - validationSchema: CreateFirewallSchema, - }); - - const FirewallLabelText = `Assign services to the Firewall`; - const FirewallHelperText = `Assign one or more services to this firewall. You can add services later if you want to customize your rules first.`; - - React.useEffect(() => { - if (open) { - resetForm(); + } + } catch (errors) { + for (const error of errors) { + setError(error?.field ?? 'root', { message: error.reason }); + } } - }, [open, resetForm]); - - const handleInboundPolicyChange = React.useCallback( - (e: React.ChangeEvent, value: 'ACCEPT' | 'DROP') => { - setFieldValue('rules.inbound_policy', value); - }, - [setFieldValue] - ); - - const handleOutboundPolicyChange = React.useCallback( - (e: React.ChangeEvent, value: 'ACCEPT' | 'DROP') => { - setFieldValue('rules.outbound_policy', value); - }, - [setFieldValue] - ); + }; const userCannotAddFirewall = _isRestrictedUser && !_hasGrant('add_firewalls'); @@ -257,82 +196,97 @@ export const CreateFirewallDrawer = React.memo( ); - const generalError = - status?.generalError || - // @ts-expect-error this form intentionally breaks Formik's error type - errors['rules.inbound'] || - // @ts-expect-error this form intentionally breaks Formik's error type - errors['rules.outbound'] || - errors.rules; - return ( - -
+ + {userCannotAddFirewall ? ( ) : null} - {generalError && ( - + {errors.root?.message && ( + )} - ( + + )} + control={control} name="label" - onBlur={handleBlur} - onChange={handleChange} - required - value={values.label} /> - Default Inbound Policy - - } - label="Accept" - value="ACCEPT" - /> - } label="Drop" value="DROP" /> - - + ( + + } + label="Accept" + value="ACCEPT" + /> + } + label="Drop" + value="DROP" + /> + + )} + control={control} + name="rules.inbound_policy" + /> Default Outbound Policy - - } - label="Accept" - value="ACCEPT" - /> - } label="Drop" value="DROP" /> - - + ( + + } + label="Accept" + value="ACCEPT" + /> + } + label="Drop" + value="DROP" + /> + + )} + control={control} + name="rules.outbound_policy" + /> ({ @@ -340,11 +294,11 @@ export const CreateFirewallDrawer = React.memo( })} variant="h3" > - {FirewallLabelText} + {FIREWALL_LABEL_TEXT} - {FirewallHelperText} - {deviceSelectGuidance ? ` ${deviceSelectGuidance}` : null} + {FIREWALL_HELPER_TEXT} + {deviceSelectGuidance && ` ${deviceSelectGuidance}`} ({ @@ -356,41 +310,47 @@ export const CreateFirewallDrawer = React.memo( {learnMoreLink}. - { - setFieldValue( - 'devices.linodes', - linodes.map((linode) => linode.id) - ); - }} - // @ts-expect-error this form intentionally breaks Formik's error type - errorText={errors['devices.linodes']} - helperText={deviceSelectGuidance} - multiple - optionsFilter={linodeOptionsFilter} - value={values.devices?.linodes ?? null} + ( + { + field.onChange(linodes.map((linode) => linode.id)); + }} + errorText={fieldState.error?.message} + helperText={deviceSelectGuidance} + multiple + optionsFilter={linodeOptionsFilter} + value={field.value ?? null} + /> + )} + control={control} + name="devices.linodes" /> - { - setFieldValue( - 'devices.nodebalancers', - nodebalancers.map((nodebalancer) => nodebalancer.id) - ); - }} - // @ts-expect-error this form intentionally breaks Formik's error type - errorText={errors['devices.nodebalancers']} - helperText={deviceSelectGuidance} - multiple - optionsFilter={nodebalancerOptionsFilter} - value={values.devices?.nodebalancers ?? null} + ( + { + field.onChange( + nodebalancers.map((nodebalancer) => nodebalancer.id) + ); + }} + errorText={fieldState.error?.message} + helperText={deviceSelectGuidance} + multiple + optionsFilter={nodebalancerOptionsFilter} + value={field.value ?? null} + /> + )} + control={control} + name="devices.nodebalancers" /> diff --git a/packages/manager/src/features/Firewalls/FirewallLanding/constants.ts b/packages/manager/src/features/Firewalls/FirewallLanding/constants.ts index 6e1825920b7..fd1bf8479b4 100644 --- a/packages/manager/src/features/Firewalls/FirewallLanding/constants.ts +++ b/packages/manager/src/features/Firewalls/FirewallLanding/constants.ts @@ -1,2 +1,11 @@ export const LINODE_CREATE_FLOW_TEXT = 'Additional Linodes'; export const NODEBALANCER_CREATE_FLOW_TEXT = 'Additional NodeBalancers'; + +export const FIREWALL_LABEL_TEXT = 'Assign services to the Firewall'; +export const FIREWALL_HELPER_TEXT = + 'Assign one or more services to this firewall. You can add services later if you want to customize your rules first.'; + +export const READ_ONLY_DEVICES_HIDDEN_MESSAGE = + 'Only services you have permission to modify are shown.'; +export const NODEBALANCER_HELPER_TEXT = + "Only the firewall's inbound rules apply to NodeBalancers."; diff --git a/packages/validation/.changeset/pr-11677-changed-1739897615658.md b/packages/validation/.changeset/pr-11677-changed-1739897615658.md new file mode 100644 index 00000000000..e3978aca978 --- /dev/null +++ b/packages/validation/.changeset/pr-11677-changed-1739897615658.md @@ -0,0 +1,5 @@ +--- +"@linode/validation": Changed +--- + +Update CreateFirewallSchema to match API types ([#11677](https://github.com/linode/manager/pull/11677)) diff --git a/packages/validation/src/firewalls.schema.ts b/packages/validation/src/firewalls.schema.ts index d3c5a423586..dbda3097726 100644 --- a/packages/validation/src/firewalls.schema.ts +++ b/packages/validation/src/firewalls.schema.ts @@ -1,7 +1,7 @@ // We must use a default export for ipaddr.js so our packages node compatability // Refer to https://github.com/linode/manager/issues/8675 import ipaddr from 'ipaddr.js'; -import { array, mixed, number, object, string } from 'yup'; +import { array, number, object, string } from 'yup'; export const IP_ERROR_MESSAGE = 'Must be a valid IPv4 or IPv6 address or range.'; @@ -34,7 +34,7 @@ export const CreateFirewallDeviceSchema = object({ nodebalancers: array().of(number()), }); -export const ipAddress = string().test({ +export const ipAddress = string().defined().test({ name: 'validateIP', message: IP_ERROR_MESSAGE, test: validateIP, @@ -139,11 +139,12 @@ const validateFirewallPorts = string().test({ }, }); -const validFirewallRuleProtocol = ['ALL', 'TCP', 'UDP', 'ICMP', 'IPENCAP']; export const FirewallRuleTypeSchema = object().shape({ - action: mixed().oneOf(['ACCEPT', 'DROP']).required('Action is required'), - protocol: mixed() - .oneOf(validFirewallRuleProtocol) + action: string().oneOf(['ACCEPT', 'DROP']).required('Action is required'), + description: string().nullable(), + label: string().nullable(), + protocol: string() + .oneOf(['ALL', 'TCP', 'UDP', 'ICMP', 'IPENCAP']) .required('Protocol is required.'), ports: string().when('protocol', { is: (val: any) => val !== 'ICMP' && val !== 'IPENCAP', @@ -162,28 +163,38 @@ export const FirewallRuleTypeSchema = object().shape({ ipv6: array().of(ipAddress).nullable(), }) .strict(true) + .notRequired() .nullable(), }); export const FirewallRuleSchema = object().shape({ inbound: array(FirewallRuleTypeSchema).nullable(), outbound: array(FirewallRuleTypeSchema).nullable(), - inbound_policy: mixed() + inbound_policy: string() .oneOf(['ACCEPT', 'DROP']) .required('Inbound policy is required.'), - outbound_policy: mixed() + outbound_policy: string() .oneOf(['ACCEPT', 'DROP']) .required('Outbound policy is required.'), }); +const CreateFirewallDevicesSchema = object() + .shape({ + linodes: array().of(number().defined()), + nodebalancers: array().of(number().defined()), + interfaces: array().of(number().defined()), + }) + .notRequired(); + export const CreateFirewallSchema = object().shape({ label: string() .required('Label is required.') .min(3, 'Label must be between 3 and 32 characters.') .max(32, 'Label must be between 3 and 32 characters.'), // Label validation on the back end is more complicated, we only do basic checks here. - tags: array().of(string()), + tags: array().of(string().defined()), rules: FirewallRuleSchema, + devices: CreateFirewallDevicesSchema, }); export const UpdateFirewallSchema = object().shape({