change: [UIE-9888] - Make firewall selection mandatory while creating linode and its interfaces#13410
Conversation
c096c03 to
4844424
Compare
|
|
||
| export const CreateLinodeInterfaceSchema = object({ | ||
| firewall_id: number().nullable(), | ||
| firewall_id: number() |
There was a problem hiding this comment.
API accepts both -1 and null(will be deprecated soon and will be utilized only to assign default firewall if any) to not to assign a firewall to a linode and hence firewall field has been made mandatory in UI.
3926c9e to
262df83
Compare
Cloud Manager UI test results🔺 3 failing tests on test run #5 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/images/search-images.spec.ts,cypress/e2e/core/parentChild/account-switching.spec.ts,cypress/e2e/core/stackscripts/smoke-community-stackscripts.spec.ts" |
|||||||||||||||||||||||
| label="Linode Interfaces - Public Interface Firewall" | ||
| onChange={(e, firewall) => field.onChange(firewall.id)} | ||
| placeholder={DEFAULT_FIREWALL_PLACEHOLDER} | ||
| showNoFirewallOption={false} |
There was a problem hiding this comment.
In future, default firewalls will also support "No Firewalls" option.
Failure in |
packages/manager/src/features/Linodes/LinodeCreate/Networking/utilities.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreate/Networking/utilities.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreate/Networking/utilities.ts
Outdated
Show resolved
Hide resolved
packages/manager/src/features/Linodes/LinodeCreate/Networking/utilities.ts
Outdated
Show resolved
Hide resolved
5724cc4 to
b700773
Compare
b700773 to
b5ffc6d
Compare
There was a problem hiding this comment.
Thanks @grevanak-akamai! Had a couple of questions/comments below.
Also, this PR is missing a changeset - do we still need changesets for the release process? looked at some other open PRs and they had changesets - could we add one to this PR?
| firewall_id: number().required( | ||
| 'Select an option or create a new Firewall.' | ||
| ), |
There was a problem hiding this comment.
Agree but user can create a firewall from firewalls page if no option is available. So suggesting this message. I can check with our techwriter too.
...inodesDetail/LinodeNetworking/LinodeInterfaces/EditInterfaceDrawer/EditInterfaceFirewall.tsx
Show resolved
Hide resolved
| values.linodeInterfaces = values.linodeInterfaces.map( | ||
| getCleanedLinodeInterfaceValues | ||
| ); | ||
| if (tab === 'Clone Linode' && !values.firewall_id) { |
There was a problem hiding this comment.
lines 41-43 look the same as lines 72-75 - do we need this check here underneath if (context?.isLinodeInterfacesEnabled) if we're running it anyway at lines 72-75?
Also, regarding this comment:
The Clone Linode flow does not have the firewall_id field under interfaces object, so we set firewall_id to -1 to bypass the firewall requirement in the validation schema.
just to double check - since we're keeping the original firewall schema which lets us have undefined values, do we still need this condition & the ones below?
There was a problem hiding this comment.
I agree lines 41-43 is redundant so removed it. However we still need conditions to override firewall_id as we are making it mandatory in CreateLinodeInterfaceSchema. Let me know if I'm missing something here.
There was a problem hiding this comment.
Pull request overview
This PR enforces an explicit firewall choice during Linode creation and when adding Linode interfaces by introducing a “No firewall” selectable option (represented by firewall_id = -1) and showing a warning notice when that option is chosen.
Changes:
- Make firewall selection required in Linode Create (legacy config) and Add Interface flows, while still allowing an explicit “No firewall” selection.
- Extend
FirewallSelectto include a “No firewall (not recommended)” option and an optional warning notice message. - Update unit tests and a broad set of Cypress E2E flows to select a firewall (or “No firewall”) to satisfy the new requirement.
Reviewed changes
Copilot reviewed 45 out of 45 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/validation/src/linodes.schema.ts | Adjusts Create Linode schema firewall field definition (nullable). |
| packages/manager/src/features/VPCs/VPCDetail/SubnetAssignLinodesDrawer.tsx | Passes “No firewall” warning message into firewall selection for VPC interface assignment. |
| packages/manager/src/features/Linodes/constants.ts | Adds shared warning message string for “No firewall” selections. |
| packages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/LinodeInterfaces/EditInterfaceDrawer/EditInterfaceFirewall.tsx | Disables “No firewall” option in edit-interface firewall selection. |
| packages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/LinodeInterfaces/AddInterfaceDrawer/utilities.ts | Makes firewall_id required in Add Interface form schema. |
| packages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/LinodeInterfaces/AddInterfaceDrawer/InterfaceType.tsx | Sets firewall_id to -1 for VLAN interfaces (no firewall support). |
| packages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/LinodeInterfaces/AddInterfaceDrawer/InterfaceFirewall.tsx | Updates placeholder and passes warning message prop to FirewallSelect. |
| packages/manager/src/features/Linodes/LinodesDetail/LinodeNetworking/LinodeInterfaces/AddInterfaceDrawer/AddInterfaceForm.tsx | Changes default firewall value handling and adjusts validation resolver inputs. |
| packages/manager/src/features/Linodes/LinodeCreate/utilities.test.tsx | Updates tests to include firewall_id in interface fixtures. |
| packages/manager/src/features/Linodes/LinodeCreate/schemas.ts | Adds conditional firewall requirement for legacy config create flow. |
| packages/manager/src/features/Linodes/LinodeCreate/resolvers.ts | Adds -1 sentinel assignments to satisfy validation (incl. Clone flow handling). |
| packages/manager/src/features/Linodes/LinodeCreate/Summary/Summary.tsx | Treats firewall_id = -1 as “no firewall” when computing “Firewall Assigned”. |
| packages/manager/src/features/Linodes/LinodeCreate/Networking/InterfaceType.tsx | Sets per-interface firewall_id to -1 for VLAN interface type. |
| packages/manager/src/features/Linodes/LinodeCreate/Networking/InterfaceFirewall.tsx | Updates placeholder and passes warning message prop to FirewallSelect. |
| packages/manager/src/features/Linodes/LinodeCreate/Networking/Firewall.tsx | Updates placeholder and passes warning message prop to FirewallSelect. |
| packages/manager/src/features/Linodes/LinodeCreate/Firewall.tsx | Updates placeholder and passes warning message prop to FirewallSelect. |
| packages/manager/src/features/Kubernetes/NodePoolFirewallSelect.tsx | Hides “No firewall” option in Kubernetes node pool firewall selection. |
| packages/manager/src/features/Firewalls/components/FirewallSelect.tsx | Adds “No firewall” option, warning notice rendering, and related props. |
| packages/manager/src/features/Firewalls/components/FirewallSelect.test.tsx | Adds unit tests for “No firewall” option and warning notice behavior. |
| packages/manager/src/features/Account/DefaultFirewalls.tsx | Hides “No firewall” option in account default firewall selection fields. |
| packages/manager/cypress/support/util/linodes.ts | Updates test Linode creation payload to use firewall_id = -1. |
| packages/manager/cypress/support/ui/pages/linode-create-page.ts | Adds a reusable Cypress helper to select a firewall from the dropdown. |
| packages/manager/cypress/e2e/core/stackscripts/smoke-community-stackscripts.spec.ts | Mocks firewalls and selects one during Linode creation flow. |
| packages/manager/cypress/e2e/core/stackscripts/create-stackscripts.spec.ts | Mocks firewalls and selects firewall / “No firewall” in create flows. |
| packages/manager/cypress/e2e/core/placementGroups/create-linode-with-placement-groups.spec.ts | Mocks and selects firewall as part of placement group creation scenarios. |
| packages/manager/cypress/e2e/core/oneClickApps/one-click-apps.spec.ts | Mocks and selects firewall in Marketplace/one-click app create flow. |
| packages/manager/cypress/e2e/core/linodes/create-linode.spec.ts | Mocks and selects firewall across multiple Linode create scenarios. |
| packages/manager/cypress/e2e/core/linodes/create-linode-with-vpc.spec.ts | Mocks and selects firewall in VPC-related Linode create flows. |
| packages/manager/cypress/e2e/core/linodes/create-linode-with-vlan.spec.ts | Mocks and selects firewall in VLAN-related Linode create flows. |
| packages/manager/cypress/e2e/core/linodes/create-linode-with-user-data.spec.ts | Mocks and selects firewall for user-data Linode creation. |
| packages/manager/cypress/e2e/core/linodes/create-linode-with-ssh-key.spec.ts | Mocks and selects firewall for SSH-key Linode creation scenarios. |
| packages/manager/cypress/e2e/core/linodes/create-linode-with-firewall.spec.ts | Refactors firewall selection steps to use the shared page helper. |
| packages/manager/cypress/e2e/core/linodes/create-linode-with-disk-encryption.spec.ts | Mocks and selects firewall for disk encryption Linode creation. |
| packages/manager/cypress/e2e/core/linodes/create-linode-with-add-ons.spec.ts | Mocks and selects firewall in add-ons creation flows (incl. legacy selection). |
| packages/manager/cypress/e2e/core/linodes/create-linode-vm-host-maintenance.spec.ts | Mocks and selects firewall in VM host maintenance create flow. |
| packages/manager/cypress/e2e/core/linodes/create-linode-view-code-snippet.spec.ts | Mocks and selects firewall before opening code snippet modal. |
| packages/manager/cypress/e2e/core/linodes/create-linode-mobile.spec.ts | Mocks and selects firewall in mobile viewport create flow. |
| packages/manager/cypress/e2e/core/linodes/create-linode-in-distributed-region.spec.ts | Mocks and selects firewall in distributed region create flow. |
| packages/manager/cypress/e2e/core/linodes/create-linode-in-core-region.spec.ts | Mocks and selects firewall in core region create flow. |
| packages/manager/cypress/e2e/core/linodes/create-linode-blackwell.spec.ts | Mocks and selects firewall in Blackwell GPU create smoke test. |
| packages/manager/cypress/e2e/core/linodes/clone-linode.spec.ts | Mocks and selects firewall during clone-related test flow. |
| packages/manager/cypress/e2e/core/linodes/alerts-create.spec.ts | Mocks and selects firewall in alerts-create related flows. |
| packages/manager/cypress/e2e/core/images/create-linode-from-image.spec.ts | Mocks and selects firewall in “create from image” flow. |
| packages/manager/cypress/e2e/core/helpAndSupport/open-support-ticket.spec.ts | Mocks and selects firewall to satisfy create form in support-ticket scenario. |
| packages/manager/cypress/e2e/core/general/gdpr-agreement.spec.ts | Mocks and selects firewall in GDPR agreement gating flow. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
packages/manager/src/features/Linodes/LinodeCreate/resolvers.ts
Outdated
Show resolved
Hide resolved
...ures/Linodes/LinodesDetail/LinodeNetworking/LinodeInterfaces/AddInterfaceDrawer/utilities.ts
Show resolved
Hide resolved
...odes/LinodesDetail/LinodeNetworking/LinodeInterfaces/AddInterfaceDrawer/AddInterfaceForm.tsx
Show resolved
Hide resolved
b5ffc6d to
261781e
Compare
coliu-akamai
left a comment
There was a problem hiding this comment.
thanks @grevanak-akamai, have a few more comments
regarding the e2e tests - not sure why later commits haven't generated an overview of the failing tests. I looked into the smoke-stackscripts one since you mentioned earlier it's related to your changes tho
| const image = 'AlmaLinux 9'; | ||
| const region = chooseRegion({ capabilities: ['Linodes', 'Vlans'] }); | ||
| const linodeLabel = randomLabel(); | ||
| const mockFirewall = firewallFactory.build({ |
|
|
||
| // Select a firewall | ||
| linodeCreatePage.selectFirewall( | ||
| mockFirewall.label, |
There was a problem hiding this comment.
re: fixing this test, I tried selecting the no firewall option, and it seems to work, at least on my end
| mockFirewall.label, | |
| 'No firewall - traffic is unprotected (not recommended)', |
| const options = useMemo( | ||
| () => [ | ||
| ...(firewalls ?? []), | ||
| ...(showNoFirewallOption ? [noFirewallOption] : []), | ||
| ], | ||
| [firewalls, showNoFirewallOption] | ||
| ); |
There was a problem hiding this comment.
regarding the github copilot comment for this file - since we already have a prop named options, we should probably rename this to something else (firewallOptions?)
Seeing we never used the options prop in the original code though - could we fix that here? maybe something like ...(options ? options : (firewalls ?? [])), for the first spread (or something more readable)


Description 📝
Make firewall selection mandatory while creating linode and its interfaces. If there are no firewalls or user doesn't wants to assign a firewall to a linode, "No firewall" option can be selected but the firewall field cannot be left blank.
Changes 🔄
List any change(s) relevant to the reviewer.
Scope 🚢
Upon production release, changes in this PR will be visible to:
Target release date 🗓️
Mar 2026
Preview 📷
How to test 🧪
Prerequisites
Verification steps
Author Checklists
As an Author, to speed up the review process, I considered 🤔
👀 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
As an Author, before moving this PR from Draft to Open, I confirmed ✅