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.

Sign up for GitHub

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

Jump to bottom

feat: [M3-8104] - Add options for default policies when creating a Firewall #10474

Merged
merged 5 commits into from
May 17, 2024
Merged

feat: [M3-8104] - Add options for default policies when creating a Firewall #10474

merged 5 commits into from
May 17, 2024

Conversation

jcallahan-akamai
Copy link
Contributor

@jcallahan-akamai jcallahan-akamai commented May 15, 2024 •
edited

Description 📝

This adds "Default Inbound Policy" and "Default Outbound Policy" to the Create Firewall Drawer.

The default inbound policy is now "Drop", which is a change from what Firewalls implicitly default to in Production today. Note: Cloud Manager currently specifies this default, not the API.

Changes 🔄

  • Add "Default Inbound Policy" and "Default Outbound Policy" to the Create Firewall Drawer.

Target release date 🗓️

May 27, 2024

Preview 📷

Before After
Screenshot 2024-05-15 at 3 14 07 PM Screenshot 2024-05-15 at 3 12 30 PM

How to test 🧪

  • Run the updated unit tests: yarn test CreateFirewallDrawer
  • Open the Create Firewall Drawer and observe the two additional radio buttons.
  • Create Firewalls with varying default policy combinations and ensure the Firewall is created appropriately.

As an Author I have considered 🤔

Check all that apply

  • 👀 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

jcallahan-akamai
jcallahan-akamai commented May 15, 2024
View reviewed changes
packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx

import { ActionsPanel } from 'src/components/ActionsPanel/ActionsPanel';
import { Box } from 'src/components/Box';
import { Drawer } from 'src/components/Drawer';
import { FormControlLabel } from 'src/components/FormControlLabel';
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This, Radio, and RadioGroup are the new imports. The rest are just reordering to satisfy the linter.

jcallahan-akamai
jcallahan-akamai commented May 15, 2024
View reviewed changes
packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.tsx
Comment on lines +191 to +208
const handleInboundPolicyChange = React.useCallback(
(e: React.ChangeEvent<HTMLInputElement>, value: 'ACCEPT' | 'DROP') => {
setFieldValue('rules.inbound_policy', value);
},
[setFieldValue]
);

const handleOutboundPolicyChange = React.useCallback(
(e: React.ChangeEvent<HTMLInputElement>, value: 'ACCEPT' | 'DROP') => {
setFieldValue('rules.outbound_policy', value);
},
[setFieldValue]
);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I considered DRYing this up but it didn't seem worth it. Open to other perspective.

@jcallahan-akamai jcallahan-akamai marked this pull request as ready for review May 15, 2024 19:37
@jcallahan-akamai jcallahan-akamai requested a review from a team as a code owner May 15, 2024 19:37
@jcallahan-akamai jcallahan-akamai requested review from dwiley-akamai, carrillo-erik and mjac0bs and removed request for a team and carrillo-erik May 15, 2024 19:37
mjac0bs
mjac0bs approved these changes May 16, 2024
View reviewed changes
Copy link
Contributor

@mjac0bs mjac0bs left a comment

Choose a reason for hiding this comment

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

  • Verified the new radio buttons.
  • Verified I can create a new firewall with any combination of default inbound/outbound policies and they will be reflected accurately on the firewall's details page.
  • Verified tests pass; left a small suggestion for slightly more coverage.
  • We now have a merge conflict since I merged in the form analytics PR that will need to be resolved.

Since this looked good overall, approving pending feedback is addressed. 🚢

packages/manager/src/features/Firewalls/FirewallLanding/CreateFirewallDrawer.test.tsx Outdated
Comment on lines 36 to 43
expect(withinInboundPolicy.getByText('Accept')).toBeVisible();
expect(withinInboundPolicy.getByText('Drop')).toBeVisible();

const withinOutboundPolicy = within(
screen.getByTestId('default-inbound-policy')
);
expect(withinOutboundPolicy.getByText('Accept')).toBeVisible();
expect(withinOutboundPolicy.getByText('Drop')).toBeVisible();
Copy link
Contributor

Choose a reason for hiding this comment

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

In addition to the radio buttons being visible, can we also test that the default are what we expect (e.g. Inbound is Drop and Outbound is Accept)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good suggestion, I will add that.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This suggestion led to me finding/fixing a bug in the test: a17087b#diff-0c798b84bbae6cb1eff09a58fffcf5f797803962ecfbc555de867a42f946cc85R41

Thanks again!

@mjac0bs mjac0bs added the Add'tl Approval Needed Waiting on another approval! label May 16, 2024
dwiley-akamai
dwiley-akamai approved these changes May 16, 2024
View reviewed changes
Copy link
Contributor

@dwiley-akamai dwiley-akamai left a comment

Choose a reason for hiding this comment

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

Unit test passes ✅

New radio buttons present in drawer and functionality (incl. different policy combinations) works as expected ✅

Can we get a changeset added before merging?

@jcallahan-akamai
Copy link
Contributor Author

jcallahan-akamai commented May 17, 2024 •
edited

Thanks for the reviews @mjac0bs and @dwiley-akamai. Today I will:

  • Fix merge conflict
  • Add test coverage as per Mariah's suggestion
  • Add changeset

@carrillo-erik
Copy link
Contributor

carrillo-erik commented May 17, 2024 •
edited

LGTM!

  • Tests passing
  • Observed the new radio buttons in the CreateFirewallDrawer.
  • I verified that I was able to create firewalls with a mixture of inbound/outbound default policies.

Pending the suggestions from Mariah and Dajahi plus some merge conflicts to resolve.

@github-actions GitHub Actions
Copy link

github-actions bot commented May 17, 2024 •
edited

Coverage Report: ✅
Base Coverage: 81.51%
Current Coverage: 81.51%

@jcallahan-akamai jcallahan-akamai merged commit 637f782 into linode:develop May 17, 2024
18 checks passed
This was referenced May 23, 2024
Merged
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dwiley-akamai dwiley-akamai dwiley-akamai approved these changes

@mjac0bs mjac0bs mjac0bs approved these changes

@carrillo-erik carrillo-erik carrillo-erik approved these changes

Assignees

@jcallahan-akamai jcallahan-akamai

Labels
Add'tl Approval Needed Waiting on another approval!
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
4 participants
@jcallahan-akamai @carrillo-erik @dwiley-akamai @mjac0bs