Skip to content

firewall: T7781: Firewall chains are created when unneeded #4757

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

Open
wants to merge 1 commit into
base: current
Choose a base branch
from
Open

firewall: T7781: Firewall chains are created when unneeded #4757

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 10 additions & 10 deletions data/templates/firewall/nftables.j2
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,13 @@ table ip6 raw {
delete table ip vyos_filter
{% endif %}
table ip vyos_filter {
{% if ipv4 is vyos_defined %}
{% if flowtable is vyos_defined %}
{% for name, flowtable_conf in flowtable.items() %}
{% if flowtable is vyos_defined %}
{% for name, flowtable_conf in flowtable.items() %}
{{ offload_tmpl.flowtable(name, flowtable_conf) }}
{% endfor %}
{% endif %}
{% endfor %}
{% endif %}

{% if ipv4 is vyos_defined %}
{% set ns = namespace(sets=[]) %}
{% if ipv4.forward is vyos_defined %}
{% for prior, conf in ipv4.forward.items() %}
Expand Down Expand Up @@ -222,13 +222,13 @@ table ip vyos_filter {
delete table ip6 vyos_filter
{% endif %}
table ip6 vyos_filter {
{% if ipv6 is vyos_defined %}
{% if flowtable is vyos_defined %}
{% for name, flowtable_conf in flowtable.items() %}
{% if flowtable is vyos_defined %}
{% for name, flowtable_conf in flowtable.items() %}
{{ offload_tmpl.flowtable(name, flowtable_conf) }}
{% endfor %}
{% endif %}
{% endfor %}
{% endif %}

{% if ipv6 is vyos_defined %}
{% set ns = namespace(sets=[]) %}
{% if ipv6.forward is vyos_defined %}
{% for prior, conf in ipv6.forward.items() %}
Expand Down
1 change: 0 additions & 1 deletion interface-definitions/include/firewall/default-action-base-chains.xml.i
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,5 @@
<regex>(drop|accept)</regex>
</constraint>
</properties>
<defaultValue>accept</defaultValue>
Copy link
Member

Choose a reason for hiding this comment

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

The defaultValue not only serves internally as the default for the code, but also for our documentation. defaultValues will be automatically attached to the <help> message during CLI completion.

The is only one "better/ugly/you name it" solution to the problem you identified:

Mangling the dict and removing defaults if required.

vyos-1x/python/vyos/frrender.py

Lines 78 to 83 in 5845c4b

def dict_helper_ospf_defaults(ospf, path):
# We have gathered the dict representation of the CLI, but there are default
# options which we need to update into the dictionary retrived.
default_values = conf.get_config_defaults(path, key_mangling=('-', '_'),
get_first_key=True, recursive=True)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Generating unnecessary config, just to turn around and delete it so it doesn't cause unintended behavior seems a little backwards to me. It's treating a symptom rather than solving the problem.

Is the only additional concern the help string? I can just manually update it to:

Default-action for rule-set (default: accept)

The defaultValue has a lot of value within the code, (like for the firewall global settings), but to enable a default value of accept in python, all that is required was this change (along with deleting the defaultValue):

    default_action = fw_conf.get('default_action', 'accept')

All of the other changes (smoketests, verify(), Jinja2, etc...) would be required regardless of the solution. Does keeping the default value in the XML really buy anything at that point?

Copy link
Member

Choose a reason for hiding this comment

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

Is the only additional concern the help string? I can just manually update it to:

Currently this is true - but it will (unfortunately until we find a generic solution to this issue) decouple the CLI help string from the internal implementation. I am a fan of a generic solution but yet have no proper idea how to handle this in the firewall context. Thus I tend to agree we go with your implementation.

Please add a <!-- XML COMMENT --> to the XML help string indicating that the default you define here in the help string is implemented in a different file, so we have a future reference by comment in case anyone starts wondering.

</leafNode>
<!-- include end -->
2 changes: 1 addition & 1 deletion python/vyos/template.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -600,7 +600,7 @@ def nft_rule(rule_conf, fw_hook, fw_name, rule_id, ip_name='ip'):
@register_filter('nft_default_rule')
def nft_default_rule(fw_conf, fw_name, family):
output = ['counter']
default_action = fw_conf['default_action']
default_action = fw_conf.get('default_action', 'accept')
#family = 'ipv6' if ipv6 else 'ipv4'

if 'default_log' in fw_conf:
Expand Down
9 changes: 9 additions & 0 deletions smoketest/scripts/cli/test_firewall.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -650,6 +650,14 @@ def test_ipv4_global_state(self):
self.cli_set(['firewall', 'global-options', 'state-policy', 'related', 'action', 'accept'])
self.cli_set(['firewall', 'global-options', 'state-policy', 'invalid', 'action', 'drop'])

self.cli_set(['firewall', 'ipv4', 'input', 'filter', 'rule', '1', 'action', 'accept'])
self.cli_set(['firewall', 'ipv4', 'input', 'filter', 'rule', '1', 'protocol', 'tcp'])
self.cli_set(['firewall', 'ipv4', 'input', 'filter', 'rule', '1', 'destination', 'port', '22'])

self.cli_set(['firewall', 'ipv4', 'forward', 'filter', 'rule', '1', 'action', 'accept'])
self.cli_set(['firewall', 'ipv4', 'forward', 'filter', 'rule', '1', 'protocol', 'tcp'])
self.cli_set(['firewall', 'ipv4', 'forward', 'filter', 'rule', '1', 'destination', 'port', '22'])

self.cli_commit()

nftables_search = [
Expand Down Expand Up @@ -766,6 +774,7 @@ def test_bridge_firewall(self):
self.cli_set(['firewall', 'bridge', 'prerouting', 'filter', 'rule', '2', 'ethernet-type', 'arp'])
self.cli_set(['firewall', 'bridge', 'prerouting', 'filter', 'rule', '2', 'action', 'accept'])

self.cli_set(['firewall', 'bridge', 'output', 'filter', 'rule', '1', 'action', 'accept'])

self.cli_commit()

Expand Down
4 changes: 2 additions & 2 deletions src/conf_mode/firewall.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -539,11 +539,11 @@ def verify(firewall):
for chain in ['name','forward','input','output', 'prerouting']:
if chain in firewall[family]:
for priority, priority_conf in firewall[family][chain].items():
if 'jump' in priority_conf['default_action'] and 'default_jump_target' not in priority_conf:
if 'jump' in priority_conf.get('default_action', []) and 'default_jump_target' not in priority_conf:
raise ConfigError('default-action set to jump, but no default-jump-target specified')
if 'default_jump_target' in priority_conf:
target = priority_conf['default_jump_target']
if 'jump' not in priority_conf['default_action']:
if 'jump' not in priority_conf.get('default_action', []):
raise ConfigError('default-jump-target defined, but default-action jump needed and it is not defined')
if priority_conf['default_jump_target'] == priority:
raise ConfigError(f'Loop detected on default-jump-target.')
Expand Down
Loading