Skip to content

feat: [M3-9984] Add support for entities to have multiple firewalls on CM #12241

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

Open
wants to merge 9 commits into
base: develop
Choose a base branch
from
5 changes: 5 additions & 0 deletions packages/manager/.changeset/pr-12241-added-1747755312168.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
"@linode/manager": Added
---

Support for Linodes and NodeBalancers to have multiple firewalls ([#12241](https://github.com/linode/manager/pull/12241))
Original file line number Diff line number Diff line change
Expand Up @@ -28,9 +28,14 @@ interface Props<DisableClearable extends boolean>
label?: string;
/**
* Optionally pass your own array of Firewalls.
* All Firewall will show if this is omitted.
* All Firewalls will show if this is omitted.
*/
options?: Firewall[];
/**
* Determine which firewalls should be available as options.
* If the options prop is used, this is filter ignored.
*/
optionsFilter?: (firewall: Firewall) => boolean;
/**
* The ID of the selected Firewall
*/
Expand All @@ -47,9 +52,20 @@ interface Props<DisableClearable extends boolean>
export const FirewallSelect = <DisableClearable extends boolean>(
props: Props<DisableClearable>
) => {
const { errorText, hideDefaultChips, loading, value, ...rest } = props;
const {
errorText,
hideDefaultChips,
loading,
value,
options,
optionsFilter,
...rest
} = props;

const { data: firewalls, error, isLoading } = useAllFirewallsQuery(!options);

const { data: firewalls, error, isLoading } = useAllFirewallsQuery();
const firewallOptions =
options || (optionsFilter ? firewalls?.filter(optionsFilter) : firewalls);
Copy link
Member

Choose a reason for hiding this comment

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

Not really sure if it matters, but ?? might be better here

Suggested change
options || (optionsFilter ? firewalls?.filter(optionsFilter) : firewalls);
options ?? (optionsFilter ? firewalls?.filter(optionsFilter) : firewalls);


const { defaultNumEntities, isDefault, tooltipText } =
useDefaultFirewallChipInformation(value, hideDefaultChips);
Expand All @@ -65,7 +81,7 @@ export const FirewallSelect = <DisableClearable extends boolean>(
label="Firewall"
loading={isLoading || loading}
noMarginTop
options={firewalls ?? []}
options={firewallOptions ?? []}
placeholder="None"
renderOption={({ key, ...props }, option, state) => (
<FirewallSelectOption
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ import { Link } from 'src/components/Link';
import { FirewallSelect } from 'src/features/Firewalls/components/FirewallSelect';
import { formattedTypes } from 'src/features/Firewalls/FirewallDetail/Devices/constants';

import type { FirewallDeviceEntityType } from '@linode/api-v4';
import type { Firewall, FirewallDeviceEntityType } from '@linode/api-v4';

interface Values {
firewallId: number;
Expand All @@ -22,13 +22,18 @@ const schema = object({
});

interface Props {
attachedFirewalls: Firewall[];
entityId: number;
entityType: FirewallDeviceEntityType;
onCancel: () => void;
}

export const AddFirewallForm = (props: Props) => {
const { entityId, entityType, onCancel } = props;
const { attachedFirewalls, entityId, entityType, onCancel } = props;
const hasEnabledFirewall = attachedFirewalls?.some(
(firewall) => firewall.status === 'enabled'
);

const { enqueueSnackbar } = useSnackbar();

const entityLabel = formattedTypes[entityType] ?? entityType;
Expand All @@ -51,6 +56,13 @@ export const AddFirewallForm = (props: Props) => {
}
};

const optionsFilter = (firewall: Firewall) => {
return (
!(hasEnabledFirewall && firewall.status === 'enabled') &&
!attachedFirewalls?.some((fw) => fw.id === firewall.id)
);
};
Comment on lines +59 to +64
Copy link
Contributor Author

Choose a reason for hiding this comment

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

We cannot assign a firewall to our entity if

  • if our entity already has this firewall
  • our entity has an enabled firewall and this firewall is enabled

rather than filtering out, should I just let the API return an error if the user can't assign the firewall? (I think I prefer filtering out ineligible firewalls though)

Copy link
Member

Choose a reason for hiding this comment

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

I like API errors personally because they help the user understand why something isn't allowed, but UX tends to like not letting users run into them

If we hide some options, maybe we should add some copy or helper text or tooltip or noOptionsMessage to help inform them about the "1 active firewall" limitation

We can let UX decide, but my vote will always be let the user see the API error πŸ˜„

Copy link
Contributor

Choose a reason for hiding this comment

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

How can I assign multiple firewalls to a linode/nodebalancer if the "Add Firewall" option only allows assigning one firewall at a time, and only when no firewall is currently assigned? Am I missing something?

This is what I'm seeing (same in the case of nodebalancer):

Screen.Recording.2025-05-21.at.4.11.28.PM.mov


return (
<form onSubmit={form.handleSubmit(onSubmit)}>
<Stack spacing={2}>
Expand All @@ -66,6 +78,7 @@ export const AddFirewallForm = (props: Props) => {
errorText={fieldState.error?.message}
label="Firewall"
onChange={(e, value) => field.onChange(value?.id)}
optionsFilter={optionsFilter}
placeholder="Select a Firewall"
textFieldProps={{
inputRef: field.ref,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -86,9 +86,7 @@ export const LinodeFirewalls = (props: LinodeFirewallsProps) => {
</Typography>
<Button
buttonType="primary"
disabled={attachedFirewallData && attachedFirewallData.results >= 1}
onClick={() => setIsAddFirewalDrawerOpen(true)}
tooltipText="Linodes can only have one Firewall assigned."
>
Add Firewall
</Button>
Expand Down Expand Up @@ -118,6 +116,7 @@ export const LinodeFirewalls = (props: LinodeFirewallsProps) => {
title="Add Firewall"
>
<AddFirewallForm
attachedFirewalls={attachedFirewalls ?? []}
entityId={linodeID}
entityType="linode"
onCancel={() => setIsAddFirewalDrawerOpen(false)}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -112,14 +112,12 @@ export const NodeBalancerFirewalls = (props: Props) => {
</Typography>
<Button
buttonType="primary"
disabled={attachedFirewallData && attachedFirewallData.results >= 1}
onClick={() =>
navigate({
params: { id: String(nodeBalancerId) },
to: '/nodebalancers/$id/settings/add-firewall',
})
}
tooltipText="NodeBalanacers can only have one Firewall assigned."
>
Add Firewall
</Button>
Expand Down Expand Up @@ -168,6 +166,7 @@ export const NodeBalancerFirewalls = (props: Props) => {
title="Add Firewall"
>
<AddFirewallForm
attachedFirewalls={attachedFirewalls ?? []}
entityId={nodeBalancerId}
entityType="nodebalancer"
onCancel={() =>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,11 @@ export const SummaryPanel = () => {
const { data: attachedFirewallData } = useNodeBalancersFirewallsQuery(
Number(id)
);
const linkText = attachedFirewallData?.data[0]?.label;
const linkID = attachedFirewallData?.data[0]?.id;
const firewalls = attachedFirewallData?.data ?? [];
const region = regions?.find((r) => r.id === nodebalancer?.region);
const { mutateAsync: updateNodeBalancer } = useNodebalancerUpdateMutation(
Number(id)
);
const displayFirewallLink = !!attachedFirewallData?.data?.length;

const isNodeBalancerReadOnly = useIsResourceRestricted({
grantLevel: 'read_only',
Expand Down Expand Up @@ -182,20 +180,24 @@ export const SummaryPanel = () => {
</StyledSection>
</StyledSummarySection>
</StyledSummarySectionWrapper>
{displayFirewallLink && (
{firewalls.length > 0 && (
<StyledSummarySection>
<StyledTitle data-qa-title variant="h3">
Firewall
{firewalls.length > 1 ? 'Firewalls' : 'Firewall'}
</StyledTitle>
<Typography data-qa-firewall variant="body1">
<Link
accessibleAriaLabel={`Firewall ${linkText}`}
className="secondaryLink"
to={`/firewalls/${linkID}`}
>
{linkText}
</Link>
</Typography>
{firewalls.map((firewall) => {
return (
<Typography data-qa-firewall key={firewall.id} variant="body1">
<Link
accessibleAriaLabel={`Firewall ${firewall.label}`}
className="secondaryLink"
to={`/firewalls/${firewall.id}`}
>
{firewall.label}
</Link>
</Typography>
);
})}
</StyledSummarySection>
)}
<StyledSummarySection>
Expand Down