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

pfsense_aggregate module #50390

Open
wants to merge 14 commits into
base: devel
from

Conversation

Projects
None yet
5 participants
@opoplawski
Copy link
Contributor

opoplawski commented Dec 30, 2018

SUMMARY

This is initial work on a pfsense module with various tasks to allow configuring of pfSense boxes. This is in the very initial stages and we are looking for feedback on the design and implementation.

ISSUE TYPE
  • New Module Pull Request
COMPONENT NAME

pfsense

ADDITIONAL INFORMATION

pfsense stores its configuration in an XML config file: /cf/conf/config.xml. Normally configuration is done via a PHP web interface. This module manipulates the config.xml file directly, then runs some php commands to get the pfsense code to reload the XML config file and apply any needed changes.

We are also developing this module here: https://github.com/opoplawski/ansible-pfsense

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 30, 2018

@opoplawski this PR contains more than one new module.

Please submit only one new module per pull request. For a detailed explanation, please read the grouped modules documentation

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 30, 2018

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

lib/ansible/module_utils/pfsense/pfsense.py:180:33: undefined-variable Undefined variable 'unicode'
lib/ansible/module_utils/pfsense/pfsense.py:187:33: undefined-variable Undefined variable 'unicode'
test/units/modules/pfsense/pfsense_module.py:31:4: bare-except No exception type(s) specified

The test ansible-test sanity --test import --python 2.6 [explain] failed with 2 errors:

lib/ansible/module_utils/pfsense/pfsense.py:6:0: ImportError: No module named ipaddress
test/runner/.tox/import/lib/ansible/module_utils/pfsense/pfsense.py:6:0: ImportError: No module named ipaddress

The test ansible-test sanity --test import --python 2.7 [explain] failed with 8 errors:

lib/ansible/module_utils/pfsense/pfsense.py:6:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_aggregate.py:56:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_alias.py:64:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_authserver_ldap.py:98:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_ca.py:53:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_group.py:63:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_rule.py:92:0: ImportError: No module named ipaddress
test/runner/.tox/import/lib/ansible/module_utils/pfsense/pfsense.py:6:0: ImportError: No module named ipaddress

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

test/units/modules/pfsense/pfsense_module.py:31:5: E722 do not use bare 'except'
test/units/modules/pfsense/pfsense_module.py:59:13: E265 block comment should start with '# '
test/units/modules/pfsense/pfsense_module.py:60:13: E265 block comment should start with '# '

The test ansible-test sanity --test shebang [explain] failed with 2 errors:

lib/ansible/module_utils/pfsense/pfsense_alias.py:0:0: should not have a shebang
lib/ansible/module_utils/pfsense/pfsense_rule.py:0:0: should not have a shebang

The test ansible-test sanity --test validate-modules [explain] failed with 9 errors:

lib/ansible/modules/pfsense/pfsense_aggregate.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_alias.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_authserver_ldap.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_authserver_ldap.py:227:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/pfsense/pfsense_ca.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_ca.py:188:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/pfsense/pfsense_group.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_group.py:155:0: E107 Imports should be directly below DOCUMENTATION/EXAMPLES/RETURN/ANSIBLE_METADATA.
lib/ansible/modules/pfsense/pfsense_rule.py:0:0: E312 No RETURN provided

click here for bot help

@opoplawski

This comment has been minimized.

Copy link
Contributor

opoplawski commented Dec 30, 2018

I'm using the ipaddress module to validate ip addresses. Is this okay or should something else be used to validate those?

@opoplawski

This comment has been minimized.

Copy link
Contributor

opoplawski commented Dec 30, 2018

I will rework this as a single initial module request, but perhaps a couple of questions could be decided here first (in addition to the ipaddress question above):

  • Does this belong in the network module namespace?
  • Should the multiple module_utils files be combined into a singe file or is it okay as is?
  • Is the approach taken with the pfsense_aggregate module reasonable? Because aliases and rules are so connected (e.g. changing an alias name will require changing the alias name reference in the rules as well) it seems necessary - but the arguments currently seem unconventional.
@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 30, 2018

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

lib/ansible/module_utils/pfsense/pfsense.py:180:33: undefined-variable Undefined variable 'unicode'
lib/ansible/module_utils/pfsense/pfsense.py:187:33: undefined-variable Undefined variable 'unicode'
test/units/modules/pfsense/pfsense_module.py:31:4: bare-except No exception type(s) specified

The test ansible-test sanity --test import --python 2.6 [explain] failed with 2 errors:

lib/ansible/module_utils/pfsense/pfsense.py:6:0: ImportError: No module named ipaddress
test/runner/.tox/import/lib/ansible/module_utils/pfsense/pfsense.py:6:0: ImportError: No module named ipaddress

The test ansible-test sanity --test import --python 2.7 [explain] failed with 8 errors:

lib/ansible/module_utils/pfsense/pfsense.py:6:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_aggregate.py:56:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_alias.py:64:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_authserver_ldap.py:99:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_ca.py:54:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_group.py:64:0: ImportError: No module named ipaddress
lib/ansible/modules/pfsense/pfsense_rule.py:92:0: ImportError: No module named ipaddress
test/runner/.tox/import/lib/ansible/module_utils/pfsense/pfsense.py:6:0: ImportError: No module named ipaddress

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

test/units/modules/pfsense/pfsense_module.py:31:5: E722 do not use bare 'except'
test/units/modules/pfsense/pfsense_module.py:59:13: E265 block comment should start with '# '
test/units/modules/pfsense/pfsense_module.py:60:13: E265 block comment should start with '# '

The test ansible-test sanity --test validate-modules [explain] failed with 6 errors:

lib/ansible/modules/pfsense/pfsense_aggregate.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_alias.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_authserver_ldap.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_ca.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_group.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_rule.py:0:0: E312 No RETURN provided

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 30, 2018

The test ansible-test sanity --test pylint [explain] failed with 1 error:

test/units/modules/pfsense/pfsense_module.py:31:4: bare-except No exception type(s) specified

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

test/units/modules/pfsense/pfsense_module.py:31:5: E722 do not use bare 'except'
test/units/modules/pfsense/pfsense_module.py:59:13: E265 block comment should start with '# '
test/units/modules/pfsense/pfsense_module.py:60:13: E265 block comment should start with '# '

The test ansible-test sanity --test validate-modules [explain] failed with 6 errors:

lib/ansible/modules/pfsense/pfsense_aggregate.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_alias.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_authserver_ldap.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_ca.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_group.py:0:0: E312 No RETURN provided
lib/ansible/modules/pfsense/pfsense_rule.py:0:0: E312 No RETURN provided

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 30, 2018

The test ansible-test sanity --test pylint [explain] failed with 1 error:

test/units/modules/pfsense/pfsense_module.py:31:4: bare-except No exception type(s) specified

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

test/units/modules/pfsense/pfsense_module.py:31:5: E722 do not use bare 'except'
test/units/modules/pfsense/pfsense_module.py:59:13: E265 block comment should start with '# '
test/units/modules/pfsense/pfsense_module.py:60:13: E265 block comment should start with '# '

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 31, 2018

The test ansible-test sanity --test pylint [explain] failed with 3 errors:

lib/ansible/module_utils/pfsense/pfsense_rule.py:337:39: bad-whitespace Exactly one space required after comma         rule['name'] = rule.pop('descr','UNKNOWN')                                        ^
lib/ansible/module_utils/pfsense/pfsense_rule.py:349:40: bad-whitespace Exactly one space required after comma         rule['action'] = rule.pop('type','UNKNOWN')                                         ^
test/units/modules/pfsense/pfsense_module.py:31:4: bare-except No exception type(s) specified

The test ansible-test sanity --test pep8 [explain] failed with 5 errors:

lib/ansible/module_utils/pfsense/pfsense_rule.py:337:40: E231 missing whitespace after ','
lib/ansible/module_utils/pfsense/pfsense_rule.py:349:41: E231 missing whitespace after ','
test/units/modules/pfsense/pfsense_module.py:31:5: E722 do not use bare 'except'
test/units/modules/pfsense/pfsense_module.py:59:13: E265 block comment should start with '# '
test/units/modules/pfsense/pfsense_module.py:60:13: E265 block comment should start with '# '

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Dec 31, 2018

The test ansible-test sanity --test pylint [explain] failed with 1 error:

test/units/modules/pfsense/pfsense_module.py:31:4: bare-except No exception type(s) specified

The test ansible-test sanity --test pep8 [explain] failed with 3 errors:

test/units/modules/pfsense/pfsense_module.py:31:5: E722 do not use bare 'except'
test/units/modules/pfsense/pfsense_module.py:59:13: E265 block comment should start with '# '
test/units/modules/pfsense/pfsense_module.py:60:13: E265 block comment should start with '# '

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 1, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

test/units/modules/pfsense/pfsense_module.py:33:4: bare-except No exception type(s) specified

The test ansible-test sanity --test pep8 [explain] failed with 2 errors:

lib/ansible/module_utils/pfsense/pfsense.py:14:1: E302 expected 2 blank lines, found 1
test/units/modules/pfsense/pfsense_module.py:33:5: E722 do not use bare 'except'

The test ansible-test sanity --test validate-modules [explain] failed with 1 error:

lib/ansible/modules/pfsense/pfsense_rule.py:0:0: E325 argument_spec for "disabled" defines type="bool" but documentation does not

click here for bot help

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 2, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

test/units/modules/pfsense/pfsense_module.py:33:4: bare-except No exception type(s) specified

The test ansible-test sanity --test pep8 [explain] failed with 1 error:

test/units/modules/pfsense/pfsense_module.py:33:5: E722 do not use bare 'except'

click here for bot help

@opoplawski

This comment has been minimized.

Copy link
Contributor

opoplawski commented Jan 2, 2019

The test ansible-test sanity --test pylint [explain] failed with 1 error:

test/units/modules/pfsense/pfsense_module.py:33:4: bare-except No exception type(s) specified

@f-bor This seems to be the last sanity test failure - can you please make a fix for that?

Also, the python 2.6 tests are failing for two reasons:

  • @unittest.expectedFailure -> AttributeError: 'module' object has no attribute 'expectedFailure'
    

and indeed, no other ansible tests appear to use that.

  • TypeError: write() got an unexpected keyword argument 'xml_declaration'

I did find one other use of xml_declaration in ansible - in the xml module. So I'm not really sure how to address this as it seems ansible is already violating python 2.6 compatibility here.

@f-bor f-bor force-pushed the opoplawski:pfsense branch from 530c92a to 0c445f7 Jan 2, 2019

@f-bor f-bor force-pushed the opoplawski:pfsense branch from 0c445f7 to 1671148 Jan 2, 2019

@Qalthos

This comment has been minimized.

Copy link
Contributor

Qalthos commented Jan 9, 2019

This should be moved into modules/networking/pfsense instead of modules/pfsense

@ansibot

This comment has been minimized.

@ansibot ansibot added test and removed needs_triage labels Jan 9, 2019

@ansibot

This comment has been minimized.

Copy link
Contributor

ansibot commented Jan 9, 2019

The test ansible-test sanity --test no-underscore-variable [explain] failed with 9 errors:

lib/ansible/module_utils/networking/pfsense/pfsense_alias.py:107:14: use `dummy` instead of `_` for a variable name
lib/ansible/module_utils/networking/pfsense/pfsense_rule.py:407:19: use `dummy` instead of `_` for a variable name
lib/ansible/module_utils/networking/pfsense/pfsense_rule.py:446:19: use `dummy` instead of `_` for a variable name
lib/ansible/module_utils/networking/pfsense/pfsense_rule.py:504:14: use `dummy` instead of `_` for a variable name
lib/ansible/modules/networking/pfsense/pfsense_aggregate.py:184:14: use `dummy` instead of `_` for a variable name
lib/ansible/modules/networking/pfsense/pfsense_authserver_ldap.py:151:25: use `dummy` instead of `_` for a variable name
lib/ansible/modules/networking/pfsense/pfsense_ca.py:138:17: use `dummy` instead of `_` for a variable name
lib/ansible/modules/networking/pfsense/pfsense_ca.py:145:22: use `dummy` instead of `_` for a variable name
lib/ansible/modules/networking/pfsense/pfsense_group.py:109:20: use `dummy` instead of `_` for a variable name

click here for bot help

f-bor and others added some commits Jan 9, 2019

@ansibot ansibot added core_review and removed needs_revision labels Jan 15, 2019

@opoplawski opoplawski changed the title pfsense module pfsense_aggregate module Jan 15, 2019

@opoplawski

This comment has been minimized.

Copy link
Contributor

opoplawski commented Jan 15, 2019

Okay, we are now down to one (rather large) module - pfsense_aggregate. But that is the one I'm most interested in feedback on.

@f-bor

This comment has been minimized.

Copy link
Contributor

f-bor commented Jan 15, 2019

@Qalthos In a few words, the idea behind this module is to use other pfsense modules code to push all aliases and rules at once. It also handles purge parameters to delete everything that is not explicitly defined anymore. Since pfsense is not providing a CLI, we can't just push a big configuration file to a device to achieve quite the same job.

We have also started to write a lookup plugin (which is not include in this PR), designed to work with pfsense_aggregate. You have to supply a file describing your network topology and security. The plugin determines and generates accordingly what is required on each firewall. It allows to easily manage a fleet of firewalls sharing same network elements.

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