Skip to content

Fix iptables state aborting when the OS-default iptables conf file is absent#69763

Open
ggiesen wants to merge 1 commit into
saltstack:3006.xfrom
ggiesen:fix-54459
Open

Fix iptables state aborting when the OS-default iptables conf file is absent#69763
ggiesen wants to merge 1 commit into
saltstack:3006.xfrom
ggiesen:fix-54459

Conversation

@ggiesen

@ggiesen ggiesen commented Jul 10, 2026

Copy link
Copy Markdown
Contributor

What does this PR do?

When iptables.get_saved_rules is called without an explicit conf_file, _parse_conf falls back to the OS-default path returned by _conf() (for example /etc/sysconfig/iptables on RedHat) and opened it unconditionally. On a minion where that file does not exist, salt.utils.files.fopen raised IOError/FileNotFoundError, which propagated up through get_saved_rules into state.iptables.insert and state.iptables.append whenever save=True was set, aborting the state run.

This wraps that read in try/except FileNotFoundError and returns an empty dict instead. Returning {} is safe for every internal caller: the state save-diff sees empty saved rules, no-ops the comparison loop, and correctly triggers the save that writes the missing file (the user's desired outcome); get_saved_policy falls through its existing KeyError guard and returns None. The stale docstring that claimed a False return is corrected to describe the empty-dict behaviour.

What issues does this PR fix or reference?

Fixes #54459

Previous Behavior

A state such as iptables.insert with save: True on a minion that has no /etc/sysconfig/iptables failed with:

IOError: [Errno 2] No such file or directory: u'/etc/sysconfig/iptables'

raised from _parse_conf and surfaced as a state exception.

New Behavior

_parse_conf returns an empty dict when the OS-default conf file is absent. The state no longer aborts; the diff against the in-memory rules triggers the save, which creates the missing file. The happy path (file present), the in_mem path, and the explicit-conf_file path are unchanged.

Merge requirements satisfied?

[NOTICE] Bug fixes or features added to Salt require tests.

  • [Documentation] n/a - no doc-facing behaviour change (internal helper; docstring corrected)
  • Changelog entry added (changelog/54459.fixed.md)
  • Tests written and described - added to tests/pytests/unit/modules/test_iptables.py:
    • test_parse_conf_missing_file_54459 - direct-altitude: calls _parse_conf(conf_file=None, family="ipv4") (the exact shape get_saved_rules passes from the save=True state path) with fopen raising FileNotFoundError; asserts {}.
    • test_get_saved_rules_missing_file_54459 - end-to-end reproducer through the real caller get_saved_rules(family="ipv4").
    • test_parse_conf_present_file_no_regression_54459 - inverse must-not-regress: conf file present via mock_open, parsed dict still yields the filter/INPUT rules; passes with and without the fix.
    • test_get_saved_policy_missing_file_54459 - peripheral: get_saved_policy resolves to None (its KeyError guard) when the conf file is missing.

Commits signed with GPG?

No

When get_saved_rules is called without an explicit conf_file, _parse_conf
falls back to the OS-default path from _conf() and opened it
unconditionally. On a minion where that file does not exist (for example
/etc/sysconfig/iptables on RedHat), fopen raised IOError/FileNotFoundError,
which propagated up through get_saved_rules into state.iptables.insert and
state.iptables.append when save=True, aborting the state run.

Catch FileNotFoundError and return an empty dict instead. Returning {} is
safe for every internal caller: the state save-diff no-ops and correctly
triggers the save that writes the missing file, and get_saved_policy falls
through its existing KeyError guard to return None. The stale docstring that
claimed a False return is corrected to describe the empty-dict behaviour.

Fixes saltstack#54459
@ggiesen ggiesen requested a review from a team as a code owner July 10, 2026 17:55
@charzl charzl self-assigned this Jul 10, 2026
@dwoz dwoz added the test:full Run the full test suite label Jul 10, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

test:full Run the full test suite

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants