Skip to content
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

upcoming: [M3-9105] - Add Account Default Firewalls setting #11828

Merged
merged 11 commits into from
Mar 20, 2025

Conversation

coliu-akamai
Copy link
Contributor

@coliu-akamai coliu-akamai commented Mar 11, 2025

Note

The design is not fully finalized, but you can test that the form works (using devenv)
Once the design is updated, I'll implement it here before merging

Description 📝

  • Adds the Default Firewalls paper to Account Settings

Changes 🔄

  • Add Default Firewalls paper
  • add a quick unit test (interactions to be tested in M3-9172)
  • ignore files with only import changes - those were just eslint fixes
    • Files of note: in the manager package - DefaultFirewalls.tsx, DefaultFirewalls.test.tsx, GlobalSettings.tsx; and then in all the other packages

Preview 📷

How to test 🧪

Prerequisites

  • On devenv, with the LinodeInterfaces feature flag on, navigate to account/settings

Verification steps

  • Confirm updating default firewalls saves
  • Confirm payload / response is what you'd expect
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


  • I have read and considered all applicable items listed above.

As an Author, before moving this PR from Draft to Open, I confirmed ✅

  • All unit tests are passing
  • TypeScript compilation succeeded without errors
  • Code passes all linting rules

Comment on lines +92 to +95
public_interface: number | null;
vpc_interface: number | null;
linode: number | null;
nodebalancer: number | null;
Copy link
Contributor Author

Choose a reason for hiding this comment

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

updated types - when an account starts without defaults, these values come in as null

@coliu-akamai
Copy link
Contributor Author

note: from what I'm seeing, omitting a field or setting it to null in the payload doesn't change the field in the DB/it doesn't reset it back to null. The API just keeps whatever's there, so once you update the default firewalls, there's no way to reset them w/o editing the DB directly. Reach out if you want to clear the defaults and are unsure of which db table to update

@coliu-akamai coliu-akamai marked this pull request as ready for review March 12, 2025 21:01
@coliu-akamai coliu-akamai requested a review from a team as a code owner March 12, 2025 21:01
@coliu-akamai coliu-akamai requested review from cpathipa and pmakode-akamai and removed request for a team March 12, 2025 21:01
@bnussman-akamai
Copy link
Member

Interesting. Good catch on that. @coliu-akamai

You can't even delete the firewall to clear out the account default. 😳

Screenshot 2025-03-12 at 5 08 36 PM

In my option, users should be allowed to set the account defaults back to null if they want. We can bring this up with the API

Copy link
Member

@bnussman-akamai bnussman-akamai left a comment

Choose a reason for hiding this comment

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

Looks great!

Code is clean and readable 🧼
Feature works well when testing against our devenv instance ✅

If we convince the API to allow users to clear out the account defaults, we can come back and make necessary changes in the future

@coliu-akamai coliu-akamai added Add'tl Approval Needed Waiting on another approval! Wait to Merge and removed Ready for Review labels Mar 12, 2025
@coliu-akamai
Copy link
Contributor Author

sounds good! will double check with them about clearing defaults

also pushed up a small change to add success toasts for updating default firewalls/network interface type... not sure why but the button becoming disabled (as expected) suddenly started throwing me off and the toasts helped mitigate that. I can remove the toasts if we think it's too much / extra though!

Copy link

github-actions bot commented Mar 12, 2025

Coverage Report:
Base Coverage: 79.82%
Current Coverage: 79.82%

@coliu-akamai
Copy link
Contributor Author

See internal slack / Default Firewall API specs - they don't plan to let you reset once you've already chosen a firewall. This is so you can't go back on protection once you start having defaults - ig the only way to delete a firewall set as the default then is to select a new default

@bnussman-akamai
Copy link
Member

Thanks for checking @coliu-akamai

Makes sense! If I were a customer I might be super mad I'm not allowed to fully "clean up" my account anymore, but I guess it works

Copy link
Contributor

@cpathipa cpathipa left a comment

Choose a reason for hiding this comment

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

Nice work @coliu-akamai!

@coliu-akamai coliu-akamai added Approved Multiple approvals and ready to merge! and removed Add'tl Approval Needed Waiting on another approval! labels Mar 17, 2025
@linode-gh-bot
Copy link
Collaborator

Cloud Manager UI test results

🔺 2 failing tests on test run #10 ↗︎

❌ Failing✅ Passing↪️ Skipped🕐 Duration
2 Failing538 Passing3 Skipped129m 41s

Details

Failing Tests
SpecTest
create-firewall.spec.tscreate firewall » creates a firewall without a linode
delete-firewall.spec.tsdelete firewall » deletes a firewall

Troubleshooting

Use this command to re-run the failing tests:

pnpm cy:run -s "cypress/e2e/core/firewalls/create-firewall.spec.ts,cypress/e2e/core/firewalls/delete-firewall.spec.ts"

@coliu-akamai coliu-akamai merged commit 71a3bb4 into linode:develop Mar 20, 2025
23 of 25 checks passed
@coliu-akamai coliu-akamai deleted the m3-9105 branch March 20, 2025 13:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Approved Multiple approvals and ready to merge! Linode Interfaces
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants