-
Notifications
You must be signed in to change notification settings - Fork 355
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
T4072: firewall extend bridge firewall #3901
Conversation
👍 |
❌ |
CI integration 👍 passed! Details
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I couldn't find any issues in the scripts, but I think some option names and help strings can be improved.
<children> | ||
<node name="filter"> | ||
<properties> | ||
<help>Bridge firewall prerouting filter</help> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can find better wording for these help strings, although I'm not sure offhand what a better option might be. We can keep this, if nothing else.
interface-definitions/include/firewall/bridge-hook-prerouting.xml.i
Outdated
Show resolved
Hide resolved
@@ -44,6 +44,25 @@ | |||
</properties> | |||
<defaultValue>disable</defaultValue> | |||
</leafNode> | |||
<node name="apply-for-bridge"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like this option name. First, it's ungrammatical: apply is used with for only in its intransitive sense ("apply for a job"), when it's transitive it should be "apply $something to $somethingElse". Second, it's not very obvious. Maybe apply-to-bridged-traffic
— kinda wordy, but it's immediately obvious what the option does.
Alternatively: filter-bridged-traffic
.
interface-definitions/include/firewall/set-packet-modifications.xml.i
Outdated
Show resolved
Hide resolved
</leafNode> | ||
<leafNode name="dscp"> | ||
<properties> | ||
<help>Packet Differentiated Services Codepoint (DSCP)</help> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe "Set DSCP (Differentiated Services Codepoint) bits"?
interface-definitions/include/firewall/set-packet-modifications.xml.i
Outdated
Show resolved
Hide resolved
d03c1c9
to
b71702f
Compare
Conflicts have been resolved. A maintainer will review the pull request shortly. |
89efe8e
to
25f1b6e
Compare
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
25f1b6e
to
a5ee39a
Compare
…lude new chains, priorities, and firewall groups
…s wrong. Use nft -c option to check temporary file, and use output provided by nftables to parse the error if possible, or print it as it is if it's an unknown error
…enabling/disabling sending traffic from bridge layer to ipvX layer
a5ee39a
to
70b5bd1
Compare
Conflicts have been resolved. A maintainer will review the pull request shortly. |
I've removed features for packet modifications, which orignally was implementef fot |
… prerouting chain; re introduce <set vrf> in policy; change global options for passing traffic to IPvX firewall; update smoketest
70b5bd1
to
c33cd61
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have no objections to merging it now. We can improve help strings later if we figure out better wordings, option names and implementation seems good.
There is only one issue with this: we cannot configure an action for ARP
This way, it will drop all ARP requests.
So needs to be carefully with default action:
|
I'll check this, and create a feature request and submit a separate PR |
Change Summary
Extend bridge firewall by:
Types of changes
Related Task(s)
Related PR(s)
vyos/vyos-documentation#1512
Component(s) name
firewall
Proposed changes
How to test
nftables
Smoketest result
test_firewall.py --> OK
test_policy_route.py --> OK
test_nat.py --> OK
Checklist: