Fix check mode in iptables_state for incomplete iptables-save files along with integration tests #8029

Maxopoly · 2024-02-25T14:20:23Z

SUMMARY

ISSUE TYPE

Bugfix Pull Request
Test Pull Request

COMPONENT NAME

iptables_state

ADDITIONAL INFORMATION

This PR first attempted to reproduce the behavior observed in #7463 through integration tests. iptables-save doesn't work in docker, so this was tested against the azure pipeline through this PR, please excuse the commit spam.

a58bc16 was able reproduce the issue in #7463 based on incomplete iptables-save files. Initially, all tables are empty and uninitialized, which will make iptables-save return nothing for them.

For example when creating a nat rule and then deleting it, an output of iptables-save may look like this:

        "*filter",
        ":INPUT ACCEPT [0:0]",
        ":FORWARD DROP [0:0]",
        ":OUTPUT ACCEPT [0:0]",
        "COMMIT",
        "*nat",
        ":PREROUTING ACCEPT [0:0]",
        ":INPUT ACCEPT [0:0]",
        ":OUTPUT ACCEPT [0:0]",
        ":POSTROUTING ACCEPT [0:0]",
        "COMMIT",

while one on a new container/machine which has never had a nat rule will look like this:

        "*filter",
        ":INPUT ACCEPT [0:0]",
        ":FORWARD DROP [0:0]",
        ":OUTPUT ACCEPT [0:0]",
        "COMMIT",

This broke the existing check_mode checks, which did simple string comparison. Most likely there were also edge cases outside of check mode in regards to uninitialized tables of existing iptables-save dumps, which would report changes when restoring them, despite nothing changing. This is only the case for iptables-save files created through means other than this module.

To circument this I implemented parsing of iptables-save dumps into a simple data structure based on preexisting code. If a table currently has some kind of state (including rules) but is not referenced in the iptables-save dump, it will not be touched by iptables-restore and now handled properly by this module.

I also added the tables tables_after after restoring as a data structure, similar to the already existing state tables before running the module

Maxopoly · 2024-02-25T14:58:27Z

Ok, a58bc16 can reproduce the issue described. Incomplete iptables save files are not handled properly in check mode.

ansibullbot · 2024-02-25T15:17:13Z

cc @quidame
click here for bot help

felixfontein

Thanks for your contribution!

changelogs/fragments/8029-iptables-state-restore-check-mode.yml

plugins/modules/iptables_state.py

Maxopoly · 2024-03-07T20:24:00Z

Fixed as suggested and removed the additional return value introduced. I'll make an additional PR against master to readd that once this PR is merged, as it depends on the changes made here.

felixfontein

If nobody objects, I'll merge this in ~a week.

patchback · 2024-03-24T17:02:52Z

Backport to stable-7: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-7/23396e62dc75bb4a9a16f0d1b29d85fb66d93725/pr-8029

Backported as #8136

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

felixfontein · 2024-03-24T17:02:57Z

@Maxopoly thanks for your contribution!

…long with integration tests (#8029) * Implement integration test to reproduce #7463 * Make new iptables_state checks async * Add missing commit to iptable_state integration test * Remove async when using checkmode in iptables_state integration tests * Do per table comparison in check mode for iptables_state * Calculate changes of iptables state per table based on result * Output target iptables state in checkmode * Refactor calculation of invidual table states in iptables_state * Add missing return for table calculation * Add missing arg to regex check * Remove leftover debug output for target iptable state * Parse per table state from raw state string * Join restored state for extration of table specific rules * Switch arguments for joining restored iptable state * Output final ip table state * Compare content of tables * Complete iptables partial tables test cases * Correct order of test iptables data * Update docu for iptables tables_after * Add changelog fragment * Appease the linting gods for iptables_state * Adjust spelling and remove tables_after from return values (cherry picked from commit 23396e6)

patchback · 2024-03-24T17:03:00Z

Backport to stable-8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/stable-8/23396e62dc75bb4a9a16f0d1b29d85fb66d93725/pr-8029

Backported as #8137

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

…long with integration tests (#8029) * Implement integration test to reproduce #7463 * Make new iptables_state checks async * Add missing commit to iptable_state integration test * Remove async when using checkmode in iptables_state integration tests * Do per table comparison in check mode for iptables_state * Calculate changes of iptables state per table based on result * Output target iptables state in checkmode * Refactor calculation of invidual table states in iptables_state * Add missing return for table calculation * Add missing arg to regex check * Remove leftover debug output for target iptable state * Parse per table state from raw state string * Join restored state for extration of table specific rules * Switch arguments for joining restored iptable state * Output final ip table state * Compare content of tables * Complete iptables partial tables test cases * Correct order of test iptables data * Update docu for iptables tables_after * Add changelog fragment * Appease the linting gods for iptables_state * Adjust spelling and remove tables_after from return values (cherry picked from commit 23396e6)

…te for incomplete iptables-save files along with integration tests (#8136) Fix check mode in iptables_state for incomplete iptables-save files along with integration tests (#8029) * Implement integration test to reproduce #7463 * Make new iptables_state checks async * Add missing commit to iptable_state integration test * Remove async when using checkmode in iptables_state integration tests * Do per table comparison in check mode for iptables_state * Calculate changes of iptables state per table based on result * Output target iptables state in checkmode * Refactor calculation of invidual table states in iptables_state * Add missing return for table calculation * Add missing arg to regex check * Remove leftover debug output for target iptable state * Parse per table state from raw state string * Join restored state for extration of table specific rules * Switch arguments for joining restored iptable state * Output final ip table state * Compare content of tables * Complete iptables partial tables test cases * Correct order of test iptables data * Update docu for iptables tables_after * Add changelog fragment * Appease the linting gods for iptables_state * Adjust spelling and remove tables_after from return values (cherry picked from commit 23396e6) Co-authored-by: Maxopoly <max@dermax.org>

…te for incomplete iptables-save files along with integration tests (#8137) Fix check mode in iptables_state for incomplete iptables-save files along with integration tests (#8029) * Implement integration test to reproduce #7463 * Make new iptables_state checks async * Add missing commit to iptable_state integration test * Remove async when using checkmode in iptables_state integration tests * Do per table comparison in check mode for iptables_state * Calculate changes of iptables state per table based on result * Output target iptables state in checkmode * Refactor calculation of invidual table states in iptables_state * Add missing return for table calculation * Add missing arg to regex check * Remove leftover debug output for target iptable state * Parse per table state from raw state string * Join restored state for extration of table specific rules * Switch arguments for joining restored iptable state * Output final ip table state * Compare content of tables * Complete iptables partial tables test cases * Correct order of test iptables data * Update docu for iptables tables_after * Add changelog fragment * Appease the linting gods for iptables_state * Adjust spelling and remove tables_after from return values (cherry picked from commit 23396e6) Co-authored-by: Maxopoly <max@dermax.org>

…long with integration tests (ansible-collections#8029) * Implement integration test to reproduce ansible-collections#7463 * Make new iptables_state checks async * Add missing commit to iptable_state integration test * Remove async when using checkmode in iptables_state integration tests * Do per table comparison in check mode for iptables_state * Calculate changes of iptables state per table based on result * Output target iptables state in checkmode * Refactor calculation of invidual table states in iptables_state * Add missing return for table calculation * Add missing arg to regex check * Remove leftover debug output for target iptable state * Parse per table state from raw state string * Join restored state for extration of table specific rules * Switch arguments for joining restored iptable state * Output final ip table state * Compare content of tables * Complete iptables partial tables test cases * Correct order of test iptables data * Update docu for iptables tables_after * Add changelog fragment * Appease the linting gods for iptables_state * Adjust spelling and remove tables_after from return values

Implement integration test to reproduce ansible-collections#7463

73732ba

ansibullbot added bug This issue/PR relates to a bug integration tests/integration tests tests labels Feb 25, 2024

Maxopoly added 2 commits February 25, 2024 15:31

Make new iptables_state checks async

d32c886

Add missing commit to iptable_state integration test

4b33819

ansibullbot added needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 25, 2024

Remove async when using checkmode in iptables_state integration tests

a58bc16

ansibullbot removed the needs_ci This PR requires CI testing to be performed. Please close and re-open this PR to trigger CI label Feb 25, 2024

Do per table comparison in check mode for iptables_state

2dc9fdf

ansibullbot added module module plugins plugin (any type) labels Feb 25, 2024

Calculate changes of iptables state per table based on result

5282dfb

This comment was marked as outdated.

Sign in to view

Maxopoly added 2 commits February 25, 2024 16:32

Output target iptables state in checkmode

33fbdb3

Refactor calculation of invidual table states in iptables_state

2fc994f

This comment was marked as outdated.

Sign in to view

Maxopoly added 10 commits February 25, 2024 17:22

Add missing return for table calculation

cf44125

Add missing arg to regex check

b659789

Remove leftover debug output for target iptable state

661d2b9

Parse per table state from raw state string

89ff41f

Join restored state for extration of table specific rules

d35abae

Switch arguments for joining restored iptable state

7795091

Output final ip table state

2b4f7b5

Compare content of tables

c3d90cd

Complete iptables partial tables test cases

5841c8c

Correct order of test iptables data

c25d357

Maxopoly added 2 commits February 25, 2024 20:30

Add changelog fragment

6b61980

Appease the linting gods for iptables_state

964704e

Maxopoly mentioned this pull request Feb 25, 2024

iptables_state restore not working when restoring rules #7463

Closed

1 task

ansibullbot added has_issue and removed ci_verified Push fixes to PR branch to re-run CI needs_revision This PR fails CI tests or a maintainer has requested a review/revision of the PR labels Feb 25, 2024

felixfontein added check-before-release PR will be looked at again shortly before release and merged if possible. backport-7 backport-8 Automatically create a backport for the stable-8 branch labels Feb 25, 2024

felixfontein reviewed Feb 25, 2024

View reviewed changes

changelogs/fragments/8029-iptables-state-restore-check-mode.yml Outdated Show resolved Hide resolved

plugins/modules/iptables_state.py Outdated Show resolved Hide resolved

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Mar 4, 2024

Adjust spelling and remove tables_after from return values

c4cdba7

Maxopoly requested a review from felixfontein March 7, 2024 20:24

ansibullbot removed the stale_ci CI is older than 7 days, rerun before merging label Mar 7, 2024

felixfontein reviewed Mar 17, 2024

View reviewed changes

felixfontein changed the title ~~Fix checkmod in iptables_state for incomplete iptables-save files along with integration tests~~ Fix check mode in iptables_state for incomplete iptables-save files along with integration tests Mar 17, 2024

ansibullbot added the stale_ci CI is older than 7 days, rerun before merging label Mar 17, 2024

felixfontein removed the check-before-release PR will be looked at again shortly before release and merged if possible. label Mar 24, 2024

felixfontein merged commit 23396e6 into ansible-collections:main Mar 24, 2024
121 checks passed

patchback bot mentioned this pull request Mar 24, 2024

[PR #8029/23396e62 backport][stable-7] Fix check mode in iptables_state for incomplete iptables-save files along with integration tests #8136

Merged

patchback bot mentioned this pull request Mar 24, 2024

[PR #8029/23396e62 backport][stable-8] Fix check mode in iptables_state for incomplete iptables-save files along with integration tests #8137

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix check mode in iptables_state for incomplete iptables-save files along with integration tests #8029

Fix check mode in iptables_state for incomplete iptables-save files along with integration tests #8029

Maxopoly commented Feb 25, 2024 •

edited

Loading

Maxopoly commented Feb 25, 2024

ansibullbot commented Feb 25, 2024

This comment was marked as outdated.

This comment was marked as outdated.

felixfontein left a comment

Maxopoly commented Mar 7, 2024

felixfontein left a comment

patchback bot commented Mar 24, 2024 •

edited

Loading

felixfontein commented Mar 24, 2024

patchback bot commented Mar 24, 2024 •

edited

Loading

Fix check mode in iptables_state for incomplete iptables-save files along with integration tests #8029

Fix check mode in iptables_state for incomplete iptables-save files along with integration tests #8029

Conversation

Maxopoly commented Feb 25, 2024 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ADDITIONAL INFORMATION

Maxopoly commented Feb 25, 2024

ansibullbot commented Feb 25, 2024

This comment was marked as outdated.

This comment was marked as outdated.

felixfontein left a comment

Choose a reason for hiding this comment

Maxopoly commented Mar 7, 2024

felixfontein left a comment

Choose a reason for hiding this comment

patchback bot commented Mar 24, 2024 • edited Loading

Backport to stable-7: 💚 backport PR created

felixfontein commented Mar 24, 2024

patchback bot commented Mar 24, 2024 • edited Loading

Backport to stable-8: 💚 backport PR created

Maxopoly commented Feb 25, 2024 •

edited

Loading

patchback bot commented Mar 24, 2024 •

edited

Loading

patchback bot commented Mar 24, 2024 •

edited

Loading