From 626d5759b57839b72f89852cff8f4e0879dd862f Mon Sep 17 00:00:00 2001 From: Ruben Herold Date: Thu, 13 Aug 2026 09:17:02 +0200 Subject: firewall: T9205: fix ruff findings in conf_mode/op_mode firewall.py Bare except:, extraneous f-string prefixes on ConfigError()/text calls without placeholders, and 3 unused test locals in test_firewall.py. No behavior change. Split out of PR #5374 (T9160) at sever-sever's request: that PR bundled this cleanup with the apply-path feature because it happened to touch the same files, but it isn't related to apply-path itself. --- src/conf_mode/firewall.py | 14 ++++++++++---- src/op_mode/firewall.py | 8 ++++---- 2 files changed, 14 insertions(+), 8 deletions(-) (limited to 'src') diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py index 3a01d2225..d00d7d214 100755 --- a/src/conf_mode/firewall.py +++ b/src/conf_mode/firewall.py @@ -610,7 +610,9 @@ def verify(firewall): if 'jump' not in priority_conf['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.') + raise ConfigError( + 'Loop detected on default-jump-target.' + ) if target not in dict_search_args(firewall[family], 'name'): raise ConfigError(f'Invalid jump-target. Firewall name {target} does not exist on the system') if 'rule' in priority_conf: @@ -646,7 +648,9 @@ def verify(firewall): for iface in zone_conf['member']['interface']: if iface in zone_interfaces: - raise ConfigError(f'Interfaces cannot be assigned to multiple zones') + raise ConfigError( + 'Interfaces cannot be assigned to multiple zones' + ) iface_vrf = get_interface_vrf(iface) if iface_vrf != 'default': @@ -656,7 +660,9 @@ def verify(firewall): if 'vrf' in zone_conf['member']: for vrf in zone_conf['member']['vrf']: if vrf in zone_vrf: - raise ConfigError(f'VRF cannot be assigned to multiple zones') + raise ConfigError( + 'VRF cannot be assigned to multiple zones' + ) zone_vrf.append(vrf) if 'vrf_interfaces' in zone_conf: @@ -791,7 +797,7 @@ def apply(firewall): ## DOMAIN RESOLVER domain_action = 'restart' if dict_search_args(firewall, 'group', 'remote_group') or dict_search_args(firewall, 'group', 'domain_group') or firewall['ip_fqdn'].items() or firewall['ip6_fqdn'].items(): - text = f'# Automatically generated by firewall.py\nThis file indicates that vyos-domain-resolver service is used by the firewall.\n' + text = '# Automatically generated by firewall.py\nThis file indicates that vyos-domain-resolver service is used by the firewall.\n' write_file(domain_resolver_usage, text) else: if os.path.exists(domain_resolver_usage): diff --git a/src/op_mode/firewall.py b/src/op_mode/firewall.py index 962eff639..8ca5bd2ea 100755 --- a/src/op_mode/firewall.py +++ b/src/op_mode/firewall.py @@ -74,7 +74,7 @@ def get_nftables_details(family, hook, priority): try: results = cmdl(command) - except: + except Exception: return {} out = {} @@ -112,7 +112,7 @@ def get_nftables_state_details(family): command = ['nft', 'list', 'chain', suffix, 'vyos_filter', f'VYOS_STATE_{name_suffix}'] try: results = cmdl(command) - except: + except Exception: return {} out = {} @@ -135,7 +135,7 @@ def get_nftables_group_members(family, table, name): try: results_str = cmdl(['nft', '-j', 'list', 'set', prefix, table, name]) results = json.loads(results_str) - except: + except Exception: return out if 'nftables' not in results: @@ -163,7 +163,7 @@ def get_nftables_remote_group_members(family, table, name): try: results_str = cmdl(['nft', '-j', 'list', 'set', prefix, table, name]) results = json.loads(results_str) - except: + except Exception: return out if 'nftables' not in results: -- cgit v1.2.3 From 5b6752c064c0313ae36a02bb36be1c8970958989 Mon Sep 17 00:00:00 2001 From: Ruben Herold Date: Thu, 13 Aug 2026 14:22:02 +0200 Subject: firewall: T9205: narrow except Exception to expected error types Address copilot-pull-request-reviewer feedback on PR #5398: cmdl() only raises OSError and json.loads() only raises json.JSONDecodeError here, so catching bare Exception hid unrelated bugs. Narrowed all four call sites accordingly (three flagged, one identical pattern found by inspection). --- src/op_mode/firewall.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/op_mode/firewall.py b/src/op_mode/firewall.py index 8ca5bd2ea..449d74d95 100755 --- a/src/op_mode/firewall.py +++ b/src/op_mode/firewall.py @@ -74,7 +74,7 @@ def get_nftables_details(family, hook, priority): try: results = cmdl(command) - except Exception: + except OSError: return {} out = {} @@ -112,7 +112,7 @@ def get_nftables_state_details(family): command = ['nft', 'list', 'chain', suffix, 'vyos_filter', f'VYOS_STATE_{name_suffix}'] try: results = cmdl(command) - except Exception: + except OSError: return {} out = {} @@ -135,7 +135,7 @@ def get_nftables_group_members(family, table, name): try: results_str = cmdl(['nft', '-j', 'list', 'set', prefix, table, name]) results = json.loads(results_str) - except Exception: + except (OSError, json.JSONDecodeError): return out if 'nftables' not in results: @@ -163,7 +163,7 @@ def get_nftables_remote_group_members(family, table, name): try: results_str = cmdl(['nft', '-j', 'list', 'set', prefix, table, name]) results = json.loads(results_str) - except Exception: + except (OSError, json.JSONDecodeError): return out if 'nftables' not in results: -- cgit v1.2.3 From de934a295f177428969b95474aff717d95f02401 Mon Sep 17 00:00:00 2001 From: Ruben Herold Date: Thu, 13 Aug 2026 14:57:03 +0200 Subject: firewall: T9205: also catch UnicodeDecodeError from cmdl() output cmdl() decodes subprocess output as utf-8 and can raise UnicodeDecodeError (not an OSError subclass) if that fails, in addition to OSError on command failure. Catch both, closing a gap left by the exact-exception narrowing in the previous commit. --- src/op_mode/firewall.py | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) (limited to 'src') diff --git a/src/op_mode/firewall.py b/src/op_mode/firewall.py index 449d74d95..0d58ba3c7 100755 --- a/src/op_mode/firewall.py +++ b/src/op_mode/firewall.py @@ -74,7 +74,7 @@ def get_nftables_details(family, hook, priority): try: results = cmdl(command) - except OSError: + except (OSError, UnicodeDecodeError): return {} out = {} @@ -112,7 +112,7 @@ def get_nftables_state_details(family): command = ['nft', 'list', 'chain', suffix, 'vyos_filter', f'VYOS_STATE_{name_suffix}'] try: results = cmdl(command) - except OSError: + except (OSError, UnicodeDecodeError): return {} out = {} @@ -135,7 +135,7 @@ def get_nftables_group_members(family, table, name): try: results_str = cmdl(['nft', '-j', 'list', 'set', prefix, table, name]) results = json.loads(results_str) - except (OSError, json.JSONDecodeError): + except (OSError, UnicodeDecodeError, json.JSONDecodeError): return out if 'nftables' not in results: @@ -163,7 +163,7 @@ def get_nftables_remote_group_members(family, table, name): try: results_str = cmdl(['nft', '-j', 'list', 'set', prefix, table, name]) results = json.loads(results_str) - except (OSError, json.JSONDecodeError): + except (OSError, UnicodeDecodeError, json.JSONDecodeError): return out if 'nftables' not in results: -- cgit v1.2.3 From b931a4cce2f20e447269fa82f5aac155f694b1b7 Mon Sep 17 00:00:00 2001 From: Ruben Herold Date: Fri, 11 Sep 2026 08:33:04 +0200 Subject: firewall: T9205: keep 3 pre-existing over-length ConfigError lines untouched The f-string-without-placeholder cleanup on these 3 raise ConfigError() calls forced darker/black to wrap them onto 3 lines each, since the lines were already over ruff.toml's 88-char limit before this PR. Per review feedback, that reformatting adds more backport friction than it resolves, so leave these 3 pre-existing long lines exactly as they were (f-string prefix included). They're untouched, not part of this diff, so CI's diff-scoped ruff/darker checks don't flag them either. --- src/conf_mode/firewall.py | 12 +++--------- 1 file changed, 3 insertions(+), 9 deletions(-) (limited to 'src') diff --git a/src/conf_mode/firewall.py b/src/conf_mode/firewall.py index d00d7d214..304276b6c 100755 --- a/src/conf_mode/firewall.py +++ b/src/conf_mode/firewall.py @@ -610,9 +610,7 @@ def verify(firewall): if 'jump' not in priority_conf['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( - 'Loop detected on default-jump-target.' - ) + raise ConfigError(f'Loop detected on default-jump-target.') if target not in dict_search_args(firewall[family], 'name'): raise ConfigError(f'Invalid jump-target. Firewall name {target} does not exist on the system') if 'rule' in priority_conf: @@ -648,9 +646,7 @@ def verify(firewall): for iface in zone_conf['member']['interface']: if iface in zone_interfaces: - raise ConfigError( - 'Interfaces cannot be assigned to multiple zones' - ) + raise ConfigError(f'Interfaces cannot be assigned to multiple zones') iface_vrf = get_interface_vrf(iface) if iface_vrf != 'default': @@ -660,9 +656,7 @@ def verify(firewall): if 'vrf' in zone_conf['member']: for vrf in zone_conf['member']['vrf']: if vrf in zone_vrf: - raise ConfigError( - 'VRF cannot be assigned to multiple zones' - ) + raise ConfigError(f'VRF cannot be assigned to multiple zones') zone_vrf.append(vrf) if 'vrf_interfaces' in zone_conf: -- cgit v1.2.3