T5160: Firewall refactor #2016
T5160: Firewall refactor #2016
Conversation
vyosbot
requested review from
a team,
dmbaturin,
sarthurdev,
zdc,
jestabro,
sever-sever and
c-po
and removed request for
a team
|
Should we use ipv4 over ip? |
I prefer just using 'ip'. But if 'ipv4' is sounds better, I can change it |
Moved from ip to ipv4 as requeted! |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
| <list>enable disable</list> | ||
| </completionHelp> | ||
| <valueHelp> | ||
| <format>enable</format> | ||
| <description>Enable log</description> | ||
| </valueHelp> | ||
| <valueHelp> | ||
| <format>disable</format> | ||
| <description>Disable log</description> | ||
| </valueHelp> | ||
| <constraint> | ||
| <regex>(enable|disable)</regex> |
There was a problem hiding this comment.
Why not use only log without enable/disable?
There was a problem hiding this comment.
- First of all, raw is not in used. Just configured in some parts to see how it would be to add nodes in firewall.
- Main idea es refactor, is just to move from one structure to a new one. Fixing or changing this type of configuration will lead to a bigger change, more complex migration, and more failure points. So from my point of view, this is out of scope of this PR. Once we get new cli, I can start fixing or adding things as requested.
interface-definitions/include/firewall/common-rule-ipv4-raw.xml.i
Outdated
Show resolved
Hide resolved
| <help>Protocol to match (protocol name, number, or "all")</help> | ||
| <completionHelp> | ||
| <script>${vyos_completion_dir}/list_protocols.sh</script> | ||
| <list>all tcp_udp</list> |
There was a problem hiding this comment.
What about replacing tcp_udp to tcp-udp?
There was a problem hiding this comment.
Main idea es refactor, is just to move from one structure to a new one. Fixing or changing this type of configuration will lead to a bigger change, more complex migration, and more failure points. So from my point of view, this is out of scope of this PR. Once we get new cli, I can start fixing or adding things as requested.
There was a problem hiding this comment.
tcp_udp looks ugly
We should change it anyway before or after this migration
| </leafNode> | ||
| <leafNode name="weekdays"> | ||
| <properties> | ||
| <help>Comma separated weekdays to match rule on</help> |
There was a problem hiding this comment.
Can we replace comma-separated weekdays with <multi/>?
| <properties> | ||
| <help>Policy for handling of all IPv4 ICMP echo requests</help> | ||
| <completionHelp> | ||
| <list>enable disable</list> |
There was a problem hiding this comment.
Can we use it without enable/disable as all-ping defined explicitly
The same for the following nodes
|
I wonder if should we accept by default other interfaces My firewall rule So I expect reject messages, but I can't ping anything including nft: |
|
If I delete firewall I expect empty dictionary |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
Excuse my ignorance, but is the template-file |
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
| from vyos.configtree import ConfigTree | ||
| from vyos.ifconfig import Section | ||
|
|
||
| if (len(argv) < 1): |
There was a problem hiding this comment.
Should be len 2 if len(argv) < 2 T5427
|
This pull request has conflicts, please resolve those before we can evaluate the pull request. |
…python scripts and src firewall
… update smoketest
…s accidentaly removed. Update smokestest: remove zone test and fix test_sysfs test
…mand show_firewall_group.
… fix in firewall smoketest
…ing that contains fqnd and/or geo-ip in base chains. Fix mig script
…set firewall ipv6 name ...> . Also fix some unexpected behaviour with geoip.
…rom <drop> to <accept> if default-action is not specified in base chains
…ew file with common matcher for ipv4 and ipv6, and use include on all chains for all this comman matchers
|
Conflicts have been resolved. A maintainer will review the pull request shortly. |
|
Merged as discussed in last maintainers meeting |
Change Summary
Firewall refactor
Types of changes
Related Task(s)
Component(s) name
firewall
Proposed changes
How to test
Some config migration were done.
Example on how new cli looks:
Some config example:
Smoketest
Also smoketest for other features, such as policy_route and nat and were tested
Checklist:
I will start working on docs soon