New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds iptables_raw module #21054

Open
wants to merge 1 commit into
base: devel
from

Conversation

Projects
None yet
@kustodian
Contributor

kustodian commented Feb 6, 2017

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

iptables_raw

ANSIBLE VERSION
ansible 2.5.0.0

But the module also works from 1.9+.

SUMMARY

This module makes it easy to manage iptables. The module has the following features:

  • Use iptables syntax to define rules
  • Keep iptables state
  • Order rules by weight
  • Support different iptables tables (filter, nat, raw,...)
  • Management of unmanaged rules
  • Diff mode
  • ip6tables support
  • Safe flushing of table rules

Full documentation of the module can be read in the module code, or here. I also wrote a blog post explaining the usage of the module and why we decided to build it.

Here are a few examples how to use it:

# Allow all IPv4 traffic coming in on port 80 (http)
- iptables_raw:
    name: allow_tcp_80
    rules: '-A INPUT -p tcp -m tcp --dport 80 -j ACCEPT'

# Allow all IPv6 traffic coming in on port 443 (https) with weight 50
- iptables_raw:
    ipversion: 6
    weight: 50
    name: allow_tcp_443
    rules: '-A INPUT -p tcp -m tcp --dport 443 -j ACCEPT'

# Remove the above rule
- iptables_raw:
    state: absent
    ipversion: 6
    name: allow_tcp_443

# Reset all IPv4 iptables rules in all tables and allow all traffic
- iptables_raw:
    name: '*'
    table: '*'
    state: absent

Module has been heavily tested on CentOS 5, 6 and 7 and has been used in production (on these distributions) for almost a year now. It should work without any problems on Debian like distributions as well (iptables-persistent` package is needed), as well as on other distributions which use iptables.

On the implementation side this module keeps the state in a json file in /etc/ansible-iptables directory, since the file generated by iptables-save doesn't hold enough information to keep everything that is needed (weight of the rules, information which chains are made by this module, etc). The module only uses iptables-save and iptables-restore commands to save and restore rules.

The module saves the generated rules into /etc/sysconfig/iptables and /etc/sysconfig/ip6tables from which RHEL and it's derivatives read the rules on boot. On Debian like distributions rules are saved in /etc/iptables/rules.v4 and /etc/iptables/rules.v6n boot). The module should work without a problem on other Linux distributions as long as they are set to read the iptables rules on boot from /etc/sysconfig/iptables. Out of the box support for other distributions can be added into the module, by detecting the path of this file and saving to it.

When the module adds rules it automatically adds ansible[name] as a comment to a rule (if a rule has a comment ansible[name]: is prepended to that comment), so that the module can distinguish what rules were created with the module, or outside of it. It also allows the user to easily see which task created a specific rule. For example this task:

- iptables_raw:
    name: allow_tcp_80
    rules: '-A INPUT -p tcp -m tcp --dport 80 -j ACCEPT'

generates this rule:

-A INPUT -p tcp -m tcp --dport 80 -m comment --comment ansible[allow_tcp_80] -j ACCEPT

The module also implements a locking mechanism, so if you run the module on the same host at the same time, one instance will wait for the other to finish execution. This was done just in case so that state files don't get overwritten in a wrong order.

We also wrote integration tests for this module, but we will add them a little later.

This PR is mostly a migration from ansible/ansible-modules-extras#2531.

@ansibot

This comment has been minimized.

Show comment
Hide comment
@ansibot

ansibot Feb 6, 2017

Contributor

@AugustusKling @rosmo @mcv21 @DavidWittman @mulby @srvg @abulimov @dsummersl @EvanK @goozbach @nibalizer @groks @LinusU @risaacson @dagwieers @dougluce @tmshn @jasperla @saito-hideki @jhoekx @natefoo @mscherer @matze @maxamillion @davixx @ovcharenko @pyykkis @pmarkham @ @dankeder

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

Contributor

ansibot commented Feb 6, 2017

@AugustusKling @rosmo @mcv21 @DavidWittman @mulby @srvg @abulimov @dsummersl @EvanK @goozbach @nibalizer @groks @LinusU @risaacson @dagwieers @dougluce @tmshn @jasperla @saito-hideki @jhoekx @natefoo @mscherer @matze @maxamillion @davixx @ovcharenko @pyykkis @pmarkham @ @dankeder

As a maintainer of a module in the same namespace this new module has been submitted to, your vote counts for shipits. Please review this module and add shipit if you would like to see it merged.

click here for bot help

@ansibot ansibot removed the needs_revision label Feb 6, 2017

Show outdated Hide outdated lib/ansible/modules/system/iptables_raw.py
# import module snippets
from ansible.module_utils.basic import *

This comment has been minimized.

@alikins

alikins Feb 6, 2017

Contributor

I'd recommend a more explicit import, ie

from ansible.module_utils.basic import AnsibleModule'

And using regular imports for 're', 'shlex', 'os', 'json', 'tempfile'

@alikins

alikins Feb 6, 2017

Contributor

I'd recommend a more explicit import, ie

from ansible.module_utils.basic import AnsibleModule'

And using regular imports for 're', 'shlex', 'os', 'json', 'tempfile'

This comment has been minimized.

@kustodian

kustodian Feb 6, 2017

Contributor

Sure thing.

@kustodian

kustodian Feb 6, 2017

Contributor

Sure thing.

This comment has been minimized.

@kustodian

kustodian Feb 6, 2017

Contributor

One question. Because I need to support Python 2.4, that means import json won't work, and I would have to do the import with try/except which is already done in basic.py. Can I somehow import json from it without basically copy pasting the code used in basic.py?

EDIT: My mistake, this works like a charm:

from ansible.module_utils.basic import json
@kustodian

kustodian Feb 6, 2017

Contributor

One question. Because I need to support Python 2.4, that means import json won't work, and I would have to do the import with try/except which is already done in basic.py. Can I somehow import json from it without basically copy pasting the code used in basic.py?

EDIT: My mistake, this works like a charm:

from ansible.module_utils.basic import json
Show outdated Hide outdated lib/ansible/modules/system/iptables_raw.py
+ '\nGenerated rules:\n#######\n' \
+ dump_contents + '#####'
else:
msg = "Could not load iptables rules:\n\n" + ipt_load_stderr

This comment has been minimized.

@alikins

alikins Feb 6, 2017

Contributor

ipt_load_stderr doesnt appear to be defined anywhere.

@alikins

alikins Feb 6, 2017

Contributor

ipt_load_stderr doesnt appear to be defined anywhere.

This comment has been minimized.

@kustodian

kustodian Feb 6, 2017

Contributor

A leftover as well, should be just stderr. Will fix it.

@kustodian

kustodian Feb 6, 2017

Contributor

A leftover as well, should be just stderr. Will fix it.

Show outdated Hide outdated lib/ansible/modules/system/iptables_raw.py
# Returns lines with default chain policies.
def _filter_default_chain_policies(self, rules, table):
chains = []
default_chains = Iptables.DEFAULT_CHAINS[table]

This comment has been minimized.

@alikins

alikins Feb 6, 2017

Contributor

This 'default_chains' doesn't seem to be used anywhere.

@alikins

alikins Feb 6, 2017

Contributor

This 'default_chains' doesn't seem to be used anywhere.

This comment has been minimized.

@kustodian

kustodian Feb 6, 2017

Contributor

It's a letfover from rewrite. Will remove it.

@kustodian

kustodian Feb 6, 2017

Contributor

It's a letfover from rewrite. Will remove it.

@LinusU

This comment has been minimized.

Show comment
Hide comment
@LinusU

LinusU Feb 7, 2017

Contributor

This is great work! What are your thoughts on merging this with the existing iptables rather than creating a new one? Then that one could also benefit from proper saving, loading, weighting etc.

Contributor

LinusU commented Feb 7, 2017

This is great work! What are your thoughts on merging this with the existing iptables rather than creating a new one? Then that one could also benefit from proper saving, loading, weighting etc.

@kustodian

This comment has been minimized.

Show comment
Hide comment
@kustodian

kustodian Feb 7, 2017

Contributor

@LinusU The problem is that these two modules work a lot different, so merging them together would probably mean a full rewrite of both modules. But the biggest problem is that the syntax of the current iptables module is totally different than this module's syntax, so merging them together (and keeping backward compatibility) would be impossible and would probably make the merged module some kind of Frankenstein.

Contributor

kustodian commented Feb 7, 2017

@LinusU The problem is that these two modules work a lot different, so merging them together would probably mean a full rewrite of both modules. But the biggest problem is that the syntax of the current iptables module is totally different than this module's syntax, so merging them together (and keeping backward compatibility) would be impossible and would probably make the merged module some kind of Frankenstein.

@LinusU

This comment has been minimized.

Show comment
Hide comment
@LinusU

LinusU Feb 7, 2017

Contributor

The other module already contains the code to turn all the options into what is here the rules option. Backwards compatibility could be something like this?

if module.params['rules']:
  rules = module.params['rules']
else:
  rules = generate_rules(module.params)
Contributor

LinusU commented Feb 7, 2017

The other module already contains the code to turn all the options into what is here the rules option. Backwards compatibility could be something like this?

if module.params['rules']:
  rules = module.params['rules']
else:
  rules = generate_rules(module.params)

@ansibot ansibot added the needs_ci label Feb 7, 2017

@kustodian

This comment has been minimized.

Show comment
Hide comment
@kustodian

kustodian Feb 7, 2017

Contributor

We should be able to merge the rules part, but we would still need to break backward compatibility because of the name parameter which is required in iptables_raw because the name is basically the ID of rules, so it would need to be added to the iptables module as the required parameter.

One other reason why I'm against the merge is because I find the iptables module too complicated to use. It has 30+ parameters, while this module has only 8 (including the name, weight and keep_unmanaged which don't even exist in the iptables one). So merging them together would make a relatively easy module to use, very complicated and hard. Of course if you learn how to use it, it would end up being the same, but somebody first needs to start using it :D

Don't get me wrong, the iptables module does what it says it does and it does it great, but we never used it because we didn't want to learn a new syntax just to be able to manage iptables (which we already know), but also because there is no state and weight.

I would rather go a different route and maybe extract the managing of module state and how the rules are applied/tested in iptables_raw and applying it to the current iptables module. That code could be shared between the two modules, since it's all in the class anyway. That way the current iptables module would benefit from keeping state and weight, and iptables_raw would be a lightweight version of the module. name parameter would still be a problem because of backward compatibility though.

Contributor

kustodian commented Feb 7, 2017

We should be able to merge the rules part, but we would still need to break backward compatibility because of the name parameter which is required in iptables_raw because the name is basically the ID of rules, so it would need to be added to the iptables module as the required parameter.

One other reason why I'm against the merge is because I find the iptables module too complicated to use. It has 30+ parameters, while this module has only 8 (including the name, weight and keep_unmanaged which don't even exist in the iptables one). So merging them together would make a relatively easy module to use, very complicated and hard. Of course if you learn how to use it, it would end up being the same, but somebody first needs to start using it :D

Don't get me wrong, the iptables module does what it says it does and it does it great, but we never used it because we didn't want to learn a new syntax just to be able to manage iptables (which we already know), but also because there is no state and weight.

I would rather go a different route and maybe extract the managing of module state and how the rules are applied/tested in iptables_raw and applying it to the current iptables module. That code could be shared between the two modules, since it's all in the class anyway. That way the current iptables module would benefit from keeping state and weight, and iptables_raw would be a lightweight version of the module. name parameter would still be a problem because of backward compatibility though.

@kustodian kustodian referenced this pull request Feb 7, 2017

Closed

Add iptables_raw module #2531

@selivan

This comment has been minimized.

Show comment
Hide comment
@selivan

selivan Feb 7, 2017

Contributor

IMHO this is much more convenient approach than existing iptables module. Ansible guys, you should definitely merge this one.

Contributor

selivan commented Feb 7, 2017

IMHO this is much more convenient approach than existing iptables module. Ansible guys, you should definitely merge this one.

@kustodian

This comment has been minimized.

Show comment
Hide comment
@kustodian

kustodian Feb 7, 2017

Contributor

I would also like to mention that we have integration tests. I just have to see how to integrate them with this repository so that they are run automatically.

Contributor

kustodian commented Feb 7, 2017

I would also like to mention that we have integration tests. I just have to see how to integrate them with this repository so that they are run automatically.

@ansibot ansibot removed the needs_ci label Feb 8, 2017

@palmerit

This comment has been minimized.

Show comment
Hide comment
@palmerit

palmerit Apr 5, 2017

@kustodian nice work. this module looks as though it'd be pretty painless to implement in a previously existing ansible infrastructure. Kudos.

palmerit commented Apr 5, 2017

@kustodian nice work. this module looks as though it'd be pretty painless to implement in a previously existing ansible infrastructure. Kudos.

@ansibot ansibot added ci_verified and removed ci_verified labels Apr 6, 2017

@jchristi

This comment has been minimized.

Show comment
Hide comment
@jchristi

jchristi May 6, 2017

Contributor

While the preferred method is to merge the two modules, is that merely a preference or a requirement? The existing module is clearly lacking an important feature (saving state) and something needs to be done to address this missing need. Ansible folks @LinusU @alikins et al, please provide some direction so progress on this can continue.

Contributor

jchristi commented May 6, 2017

While the preferred method is to merge the two modules, is that merely a preference or a requirement? The existing module is clearly lacking an important feature (saving state) and something needs to be done to address this missing need. Ansible folks @LinusU @alikins et al, please provide some direction so progress on this can continue.

@lioramilbaum

This comment has been minimized.

Show comment
Hide comment
@lioramilbaum

lioramilbaum Sep 23, 2017

The documentation specify this module exist in 2.4, but, it is still not in. Please advice when.

lioramilbaum commented Sep 23, 2017

The documentation specify this module exist in 2.4, but, it is still not in. Please advice when.

@kustodian

This comment has been minimized.

Show comment
Hide comment
@kustodian

kustodian Oct 8, 2017

Contributor

@lioramilbaum the documentation you are reading is not the official one. For this module to become part of Ansible this pull request needs to be merged.

Contributor

kustodian commented Oct 8, 2017

@lioramilbaum the documentation you are reading is not the official one. For this module to become part of Ansible this pull request needs to be merged.

@lconnell

This comment has been minimized.

Show comment
Hide comment
@lconnell

lconnell Oct 27, 2017

What's the hold-up?

lconnell commented Oct 27, 2017

What's the hold-up?

@szhem

This comment has been minimized.

Show comment
Hide comment
@szhem

szhem Nov 9, 2017

Really great ansible plugin.
Looking forward for its merge into master.

One additional question - is there a possibility to prevent creation of the state directory /etc/ansible-iptables? I.e. just compare already loaded rules and rules to be applied without a state file?

szhem commented Nov 9, 2017

Really great ansible plugin.
Looking forward for its merge into master.

One additional question - is there a possibility to prevent creation of the state directory /etc/ansible-iptables? I.e. just compare already loaded rules and rules to be applied without a state file?

@kustodian

This comment has been minimized.

Show comment
Hide comment
@kustodian

kustodian Nov 9, 2017

Contributor

@szhem it's not possible to prevent the creation of /etc/ansible-iptables state directory and please do not delete it, because it can break your iptables.

The reason why the state file is needed is because it's not possible to put all information needed for the module to work correctly into iptables.

Contributor

kustodian commented Nov 9, 2017

@szhem it's not possible to prevent the creation of /etc/ansible-iptables state directory and please do not delete it, because it can break your iptables.

The reason why the state file is needed is because it's not possible to put all information needed for the module to work correctly into iptables.

@nuklea

This comment has been minimized.

Show comment
Hide comment
@nuklea

nuklea Nov 20, 2017

Using module file /home/user/workspace/ansible/src/ansible/lib/ansible/modules/system/iptables_raw.py
<localhost> ESTABLISH LOCAL CONNECTION FOR USER: user
<localhost> EXEC /bin/sh -c 'sudo -H -S -n -u root /bin/sh -c '"'"'echo BECOME-SUCCESS-mxxvldfpjmagxcwhcxwidoifoosvisos; /usr/bin/python'"'"' && sleep 0'
The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_s6a_1owr/ansible_modlib.zip/ansible/module_utils/basic.py", line 2151, in atomic_move
    os.rename(b_src, b_dest)
FileNotFoundError: [Errno 2] No such file or directory: b'/tmp/tmphia7tr0f' -> b'/etc/sysconfig/iptables'

fatal: [localhost]: FAILED! => {
    "changed": false,
    "failed": true,
    "invocation": {
        "module_args": {
            "backup": true,
            "ipversion": "4",
            "keep_unmanaged": true,
            "name": "Private networks chain",
            "rules": "-N PRIVATE",
            "state": "present",
            "table": "filter",
            "weight": 40
        }
    },
    "msg": "Could not replace file: /tmp/tmphia7tr0f to /etc/sysconfig/iptables: [Errno 2] No such file or directory: b'/tmp/tmphia7tr0f' -> b'/etc/sysconfig/iptables'"
}

nuklea commented Nov 20, 2017

Using module file /home/user/workspace/ansible/src/ansible/lib/ansible/modules/system/iptables_raw.py
<localhost> ESTABLISH LOCAL CONNECTION FOR USER: user
<localhost> EXEC /bin/sh -c 'sudo -H -S -n -u root /bin/sh -c '"'"'echo BECOME-SUCCESS-mxxvldfpjmagxcwhcxwidoifoosvisos; /usr/bin/python'"'"' && sleep 0'
The full traceback is:
Traceback (most recent call last):
  File "/tmp/ansible_s6a_1owr/ansible_modlib.zip/ansible/module_utils/basic.py", line 2151, in atomic_move
    os.rename(b_src, b_dest)
FileNotFoundError: [Errno 2] No such file or directory: b'/tmp/tmphia7tr0f' -> b'/etc/sysconfig/iptables'

fatal: [localhost]: FAILED! => {
    "changed": false,
    "failed": true,
    "invocation": {
        "module_args": {
            "backup": true,
            "ipversion": "4",
            "keep_unmanaged": true,
            "name": "Private networks chain",
            "rules": "-N PRIVATE",
            "state": "present",
            "table": "filter",
            "weight": 40
        }
    },
    "msg": "Could not replace file: /tmp/tmphia7tr0f to /etc/sysconfig/iptables: [Errno 2] No such file or directory: b'/tmp/tmphia7tr0f' -> b'/etc/sysconfig/iptables'"
}
@kustodian

This comment has been minimized.

Show comment
Hide comment
@kustodian

kustodian Nov 22, 2017

Contributor

@nuklea I don't understand what is the example you are showing because you wrote an example for the iptables module and not iptables_raw module.

The error on the other hand displays that iptables_raw module is executed. Which distribution are you using? This module currently only supports RHEL/CentOS and Debian/Ubuntu.

Contributor

kustodian commented Nov 22, 2017

@nuklea I don't understand what is the example you are showing because you wrote an example for the iptables module and not iptables_raw module.

The error on the other hand displays that iptables_raw module is executed. Which distribution are you using? This module currently only supports RHEL/CentOS and Debian/Ubuntu.

@nickcmaynard

This comment has been minimized.

Show comment
Hide comment
@nickcmaynard

nickcmaynard Feb 2, 2018

AFAICS @alikins's code review has been addressed - it's still "open" though, blocking this PR's state.
@kustodian Would it be possible to update this to take account of the recent ansibot failures?

nickcmaynard commented Feb 2, 2018

AFAICS @alikins's code review has been addressed - it's still "open" though, blocking this PR's state.
@kustodian Would it be possible to update this to take account of the recent ansibot failures?

@kustodian

This comment has been minimized.

Show comment
Hide comment
@kustodian

kustodian Feb 2, 2018

Contributor

@nickcmaynard I'll fix the ansibot failures, the thing is for some reason I don't get notifications when these failures occure, so I don't know if I need to fix something.

Contributor

kustodian commented Feb 2, 2018

@nickcmaynard I'll fix the ansibot failures, the thing is for some reason I don't get notifications when these failures occure, so I don't know if I need to fix something.

@manics manics referenced this pull request Jun 15, 2018

Merged

Omero firewall (iptables) #78

2 of 2 tasks complete
@jchristi

This comment has been minimized.

Show comment
Hide comment
@jchristi

jchristi Jun 27, 2018

Contributor

Any update on this?

Contributor

jchristi commented Jun 27, 2018

Any update on this?

@wsgzao

This comment has been minimized.

Show comment
Hide comment
@wsgzao

wsgzao Jul 4, 2018

can it be add into ansible stable version?

wsgzao commented Jul 4, 2018

can it be add into ansible stable version?

@RKov

This comment has been minimized.

Show comment
Hide comment
@RKov

RKov Jul 6, 2018

Thumbs up for this module guys, good job!
It makes iptables management via ansible possible and straightforward with large inventories and diverse setup, which is unfortunately not true for the iptables module.
I would definitely welcome this module as a core module.

RKov commented Jul 6, 2018

Thumbs up for this module guys, good job!
It makes iptables management via ansible possible and straightforward with large inventories and diverse setup, which is unfortunately not true for the iptables module.
I would definitely welcome this module as a core module.

@ansible ansible deleted a comment from ansibot Jul 6, 2018

@ansible ansible deleted a comment from ansibot Jul 6, 2018

@ansible ansible deleted a comment from ansibot Jul 6, 2018

@ansible ansible deleted a comment from ansibot Jul 6, 2018

@dagwieers

This comment has been minimized.

Show comment
Hide comment
@dagwieers

dagwieers Jul 6, 2018

Member

Best way forward is to bring this up in one of the core meetings: ansible/community#325

Member

dagwieers commented Jul 6, 2018

Best way forward is to bring this up in one of the core meetings: ansible/community#325

@kustodian

This comment has been minimized.

Show comment
Hide comment
@kustodian

kustodian Jul 6, 2018

Contributor

@dagwieers it was already discussed in one of the core meetings and the majority was against merging thins module because it keeps state in a separate file on a remote host. A few of the core developers are against this approach.

A few weeks ago I found a way to not need this external file to keep state, but it will require a rewrite of the module, so that's what I decided to do. I also talked to a few core devs which were against the current module, and they liked the idea.

The best thing is that this rewrite will make the module code a lot more simple, while keeping all the current functionality and also add more flexibility (for example it will be able to work with dynamic iptables rules which are created by some external software like libvirtd, Docker, etc). I'm still not sure if I will just change this module, or write a completely new one, but I will update this issue anyway.

Contributor

kustodian commented Jul 6, 2018

@dagwieers it was already discussed in one of the core meetings and the majority was against merging thins module because it keeps state in a separate file on a remote host. A few of the core developers are against this approach.

A few weeks ago I found a way to not need this external file to keep state, but it will require a rewrite of the module, so that's what I decided to do. I also talked to a few core devs which were against the current module, and they liked the idea.

The best thing is that this rewrite will make the module code a lot more simple, while keeping all the current functionality and also add more flexibility (for example it will be able to work with dynamic iptables rules which are created by some external software like libvirtd, Docker, etc). I'm still not sure if I will just change this module, or write a completely new one, but I will update this issue anyway.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment