geoip: T8590: fix initialization failure and set clobbering on boot and commit

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.

2 files changed, 32 insertions, 49 deletions

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