From a263b2b62e72b208fd330f9055aefca29640ff2b Mon Sep 17 00:00:00 2001 From: xTITUSMAXIMUSX Date: Thu, 30 Apr 2026 20:43:46 -0500 Subject: geoip: T8590: fix initialization failure and set clobbering on boot and commit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three related bugs prevented GeoIP nftables sets from being populated correctly at boot and when incrementally modifying firewall or policy route rules. 1. geoip_updated() always returned False The previous implementation called node_changed() and then searched the result with dict_search_recursive(changes, 'geoip'). This could never match: node_changed() is annotated `-> list` and returns a flat list of the immediate top-level child names whose subtree changed (e.g. ['ipv4'] for the firewall path, or route names for policy route). The 'geoip' key sits several levels deeper than those names, so dict_search_recursive — which only walks dicts and lists for a matching dict key — never yielded a hit, and geoip_updated() always returned False. As a consequence, geoip_update() was never triggered by an incremental add or change of a GeoIP rule; the only path that ever populated /run/nftables-geoip.conf was the explicit \"update geoip\" command or the fallback when geoip_refresh() failed. Fix: bypass node_changed() and call get_config_diff() / get_child_nodes_diff() directly with expand_nodes=Diff.ADD|Diff.DELETE and recursive=True. In that mode, the 'add' and 'delete' values in the returned dict are full nested subtrees of the config diff, so dict_search_recursive correctly finds 'geoip' wherever it appears in the change set. 2. GeoIP block executed unconditionally, breaking the boot sequence geoip_sets() always returns {'name': [], 'ipv6_name': []}. A non-empty dict is truthy in Python regardless of whether its values are empty lists, so the guards \"if geoip_sets:\" and \"if 'name' in geoip_sets:\" were always True. On boot, when policy_route.py is invoked as a dependent of firewall.py (triggered by group_resync), it entered the GeoIP block even with no policy route GeoIP rules configured. Because /run/nftables-geoip.conf did not yet exist, geoip_refresh() returned False and geoip_update(policy=policy) was called with an empty policy. This created /run/nftables-geoip.conf containing only empty table stubs. When firewall.py subsequently called geoip_refresh(), the file existed and nft loaded it successfully — so the geoip_update(firewall) call was never reached and the firewall GeoIP sets stayed empty for the entire uptime of the router. Fix: check the actual list contents instead of the container dict: if geoip_sets['name'] or geoip_sets['ipv6_name']. 3. geoip_update() clobbered the other caller's sets geoip_update() renders /run/nftables-geoip.conf from both firewall_sets and policy_sets in a single pass. When called with only one argument (as firewall.py and policy_route.py each do), the other argument defaulted to None and that half of the file was rendered empty, erasing whatever the other script had written. The geoip-update helper used by \"update geoip\" and the weekly cron was unaffected because it always passes both arguments, which masked this bug in normal manual operation. Fix: when either argument is absent, read the missing config from the live Config session before building the set tables, so every invocation writes the complete combined firewall + policy file. --- src/conf_mode/firewall.py | 34 ++++++++++++++----------------- src/conf_mode/policy_route.py | 47 ++++++++++++++++--------------------------- 2 files changed, 32 insertions(+), 49 deletions(-) (limited to 'src') diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py index 5208c4368..4a2706a03 100755 --- a/src/conf_mode/firewall.py +++ b/src/conf_mode/firewall.py @@ -22,8 +22,8 @@ from glob import glob from sys import exit from vyos.base import Warning from vyos.config import Config -from vyos.configdict import is_node_changed, node_changed -from vyos.configdiff import Diff +from vyos.configdict import is_node_changed +from vyos.configdiff import Diff, get_config_diff from vyos.configdep import set_dependents, call_dependents from vyos.configverify import verify_interface_exists from vyos.ethtool import Ethtool @@ -92,17 +92,12 @@ def geoip_sets(firewall): return out def geoip_updated(conf): - changes = node_changed(conf, ['firewall'], - key_mangling=('-', '_'), - recursive=True, - expand_nodes=Diff.ADD | Diff.DELETE) - updated = False - - for _, path in dict_search_recursive(changes, 'geoip'): - updated = True - break - - return updated + D = get_config_diff(conf, key_mangling=('-', '_')) + diff = D.get_child_nodes_diff(['firewall'], + expand_nodes=Diff.ADD | Diff.DELETE, + recursive=True) + return any(any(dict_search_recursive(diff.get(section, {}), 'geoip')) + for section in ('add', 'delete')) def get_config(config=None): if config: @@ -124,6 +119,9 @@ def get_config(config=None): firewall['geoip_sets'] = geoip_sets(firewall) firewall['geoip_updated'] = geoip_updated(conf) + firewall['policy'] = conf.get_config_dict( + ['policy'], key_mangling=('-', '_'), + get_first_key=True, no_tag_node_value_mangle=True) fqdn_config_parse(firewall, 'firewall') @@ -739,12 +737,10 @@ def apply(firewall): domain_action = 'stop' call(f'systemctl {domain_action} vyos-domain-resolver.service') - if firewall['geoip_sets']: - # Call helper script to Update set contents - if 'name' in firewall['geoip_sets'] or 'ipv6_name' in firewall['geoip_sets']: - if firewall['geoip_updated'] or not geoip_refresh(): - print('Updating GeoIP. Please wait...') - geoip_update(firewall) + if firewall['geoip_sets']['name'] or firewall['geoip_sets']['ipv6_name']: + if firewall['geoip_updated'] or not geoip_refresh(): + print('Updating GeoIP. Please wait...') + geoip_update(firewall=firewall, policy=firewall['policy']) return None diff --git a/src/conf_mode/policy_route.py b/src/conf_mode/policy_route.py index 480f3ecf2..3cfdad913 100755 --- a/src/conf_mode/policy_route.py +++ b/src/conf_mode/policy_route.py @@ -21,8 +21,7 @@ from sys import exit from vyos.base import Warning from vyos.config import Config -from vyos.configdict import node_changed -from vyos.configdiff import Diff +from vyos.configdiff import Diff, get_config_diff from vyos.template import render from vyos.utils.dict import dict_search_args from vyos.utils.dict import dict_search_recursive @@ -49,28 +48,15 @@ valid_groups = [ ] def geoip_updated(conf): - updated = False - - changes_v4 = node_changed(conf, ['policy', 'route'], - key_mangling=('-', '_'), - recursive=True, - expand_nodes=Diff.ADD | Diff.DELETE) - - for _, path in dict_search_recursive(changes_v4, 'geoip'): - updated = True - break - - if not updated: - changes_v6 = node_changed(conf, ['policy', 'route6'], - key_mangling=('-', '_'), - recursive=True, - expand_nodes=Diff.ADD | Diff.DELETE) - - for _, path in dict_search_recursive(changes_v6, 'geoip'): - updated = True - break - - return updated + D = get_config_diff(conf, key_mangling=('-', '_')) + for path in (['policy', 'route'], ['policy', 'route6']): + diff = D.get_child_nodes_diff(path, + expand_nodes=Diff.ADD | Diff.DELETE, + recursive=True) + if any(any(dict_search_recursive(diff.get(section, {}), 'geoip')) + for section in ('add', 'delete')): + return True + return False def geoip_sets(policy): out = {'name': [], 'ipv6_name': []} @@ -102,6 +88,9 @@ def get_config(config=None): policy['geoip_sets'] = geoip_sets(policy) policy['geoip_updated'] = geoip_updated(conf) + policy['firewall'] = conf.get_config_dict( + ['firewall'], key_mangling=('-', '_'), + no_tag_node_value_mangle=True, get_first_key=True) return policy @@ -251,12 +240,10 @@ def apply(policy): apply_table_marks(policy) - if policy['geoip_sets']: - # Call helper script to Update set contents - if 'name' in policy['geoip_sets'] or 'ipv6_name' in policy['geoip_sets']: - if policy['geoip_updated'] or not geoip_refresh(): - print('Updating GeoIP. Please wait...') - geoip_update(policy=policy) + if policy['geoip_sets']['name'] or policy['geoip_sets']['ipv6_name']: + if policy['geoip_updated'] or not geoip_refresh(): + print('Updating GeoIP. Please wait...') + geoip_update(firewall=policy['firewall'], policy=policy) return None -- cgit v1.2.3