Page MenuHomeVyOS Platform
Create Task
Maniphest T9210

firewall: fix ruff findings in python/vyos/firewall.py
In progress, NormalPublicBUG
Actions

Description

Component: python/vyos/firewall.py

Description:
Follow-up cleanup split out of PR #5372 (T9157, fib-type match) at sarthurdev's request — that PR bundled unrelated ruff findings in python/vyos/firewall.py with the fib
feature because it happened to touch the same file. Fixes, purely mechanical, no behavior change:

  • fqdn_resolve(): bare except: → except gaierror: (E722)
  • != None → is not None in the geoip inverse-match check (E711)
  • 3× extraneous f prefix on string literals without placeholders (F541): hook_name = f'name', output.append(f'pkttype ' + ...), output.append(f'last')

Test plan: ruff check python/vyos/firewall.py passes clean; python3 -m py_compile.

Details

Version
2026.08.15-rolling
Is it a breaking change?
Unspecified (possibly destroys the router)
Issue type
Cosmetic issue (typos etc.)

Related Objects

Event Timeline

rherold created this task.Aug 13 2026, 4:57 PM

PR: https://github.com/vyos/vyos-1x/pull/5401

Viacheslav closed this task as Resolved.Aug 14 2026, 6:30 AM
Viacheslav assigned this task to rherold.
Viacheslav moved this task from Need Triage to Completed on the VyOS Rolling board.
a.kudientsov reopened this task as In progress.Aug 15 2026, 2:23 PM
a.kudientsov subscribed.

Subject fix causes firewall fqdn/domain-group regression. It narrowed fqdn_resolve() exception handling in from a bare except: statement to except gaierror:, claiming "no behavior change." That let non-gaierror OSErrors (e.g. transient network errors right after commit) escape uncaught, crashing the unguarded main loop of the vyos-domain-resolver daemon, the process that fills in D_* (domain-group) nftables sets.

DEBUG - ======================================================================
DEBUG - FAIL: test_groups (__main__.TestFirewall.test_groups)
DEBUG - ----------------------------------------------------------------------
DEBUG - Traceback (most recent call last):
DEBUG -   File "/usr/libexec/vyos/tests/smoke/cli/test_firewall.py", line 165, in test_groups
DEBUG -     self.verify_nftables(nftables_search, 'ip vyos_filter')
DEBUG -   File "/usr/libexec/vyos/tests/smoke/cli/base_vyostest_shim.py", line 291, in verify_nftables
DEBUG -     self.assertTrue(not matched if inverse else matched, msg=search)
DEBUG - AssertionError: False is not true : ['elements = { 192.0.2.5, 192.0.2.8,']




DEBUG - ======================================================================
DEBUG - FAIL: test_pbr_domain_group (__main__.TestPolicyRoute.test_pbr_domain_group)
DEBUG - ----------------------------------------------------------------------
DEBUG - Traceback (most recent call last):
DEBUG -   File "/usr/libexec/vyos/tests/smoke/cli/test_policy_route.py", line 128, in test_pbr_domain_group
DEBUG -     self.assertTrue(
DEBUG - AssertionError: False is not true : Expected 192.0.2.5 in D_smoketest_domain, last nft exit code 1
a.kudientsov triaged this task as Normal priority.Aug 15 2026, 2:23 PM

https://github.com/vyos/vyos-1x/pull/5406

a.kudientsov changed Version from 1.5.1 to 2026.08.15-rolling.Aug 15 2026, 2:25 PM
rherold added a comment.Aug 15 2026, 3:23 PM

Thanks for catching this, a.kudientsov — and sorry for the regression. The mistake was narrowing the bare except: in fqdn_resolve() to except gaierror: based on the assumption that getaddrinfo() only ever raises gaierror, without checking that other OSErrors (e.g. transient network failures) can hit the same call and need to keep being
swallowed here too. c-po's fix in #5406 (back to except OSError:, since gaierror is a subclass) looks correct to me.

For future ruff-driven "bare except" cleanups, two things that would have caught this earlier and might help avoid it elsewhere in the codebase:

  1. Default to narrowing to the broadest exception class that's still meaningfully "not swallow everything" (e.g. OSError) rather than the most specific one seen in current tests, unless it's confirmed only that specific subclass is reachable at the call site.
  2. Where a bare-except fix touches a daemon's main loop or similar unguarded path, add a test that actually exercises the previously-swallowed error path, not just confirm the existing smoketest suite stays green — a clean smoketest run doesn't mean the removed exception path was ever covered.

    Happy to write this up as a short note in CONTRIBUTING.md if that seems useful for future contributors.
pasik subscribed.Aug 16 2026, 8:01 AM