feat: [M3-9984] Add support for entities to have multiple firewalls on CM#12241
feat: [M3-9984] Add support for entities to have multiple firewalls on CM#12241coliu-akamai wants to merge 9 commits intolinode:developfrom
Conversation
There was a problem hiding this comment.
Thinking about adding the disable/enable button s ^ in the Firewall Landing page to an entity's Firewall table, but it might cause a more annoying UX experience than it's worth (lmk what you think!):
- if entity 1 and entity 2 share disabled firewall A, but entity 2 also has enabled firewall B, trying to enable Firewall A from entity 1's table will cause an error - feels like it could be annoying
- If a firewall is default, trying to disable will error
another idea: I could just have the Disable firewall button show up in the entity's firewall table (+ grey it out if it is a default firewall). That way, a user can quickly disable that firewall to add an enabled one from their entity's Firewall table if they wish to do so
| const optionsFilter = (firewall: Firewall) => { | ||
| return ( | ||
| !(hasEnabledFirewall && firewall.status === 'enabled') && | ||
| !attachedFirewalls?.some((fw) => fw.id === firewall.id) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
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)
There was a problem hiding this comment.
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 😄
There was a problem hiding this comment.
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
There was a problem hiding this comment.
@pmakode-akamai an entity can only have one enabled firewall at a time. my-firewall and test are both enabled, so once you assign my-firewall you cannot assign test (and vice versa) - I'd filtered out ineligible firewalls from the select
anyway, closed this PR due to PM agreement that we shouldn't support this feature in the UI!
|
going to open this up for review to collect feedback + will address when I'm back! |
coliu-akamai
requested review from
bnussman-akamai and
pmakode-akamai
and removed request for
a team
| const optionsFilter = (firewall: Firewall) => { | ||
| return ( | ||
| !(hasEnabledFirewall && firewall.status === 'enabled') && | ||
| !attachedFirewalls?.some((fw) => fw.id === firewall.id) | ||
| ); | ||
| }; |
There was a problem hiding this comment.
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 😄
|
|
||
| const { data: firewalls, error, isLoading } = useAllFirewallsQuery(); | ||
| const firewallOptions = | ||
| options || (optionsFilter ? firewalls?.filter(optionsFilter) : firewalls); |
There was a problem hiding this comment.
Not really sure if it matters, but ?? might be better here
| options || (optionsFilter ? firewalls?.filter(optionsFilter) : firewalls); | |
| options ?? (optionsFilter ? firewalls?.filter(optionsFilter) : firewalls); |
bnussman-akamai
added
Add'tl Approval Needed
Cloud Manager UI test results🔺 6 failing tests on test run #4 ↗︎
Details
TroubleshootingUse this command to re-run the failing tests: pnpm cy:run -s "cypress/e2e/core/nodebalancers/nodebalancer-settings.spec.ts,cypress/e2e/core/linodes/linode-network.spec.ts,cypress/e2e/core/kubernetes/lke-update.spec.ts" |
||||||||||||||||||||||||||||||||
|
closing this PR - PM has stated to not support this functionality in the UI |
5 participants
Description 📝
See #12220 (assigning multiple firewalls from Firewall Detail page) and #12176 Linode Entity Details firewall changes
Preview 📷
How to test 🧪
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 ✅