TODO list

[opoplawski]

@f-bor - We have a bit of an issue with the aggregate module's docs and argument specs:

13:46 ERROR: lib/ansible/modules/network/pfsense/pfsense_aggregate.py:0:0: doc-required-mismatch: Argument 'action' in argument_spec found in aggregated_rules is not required, but is documented as being required (75%)
13:46 ERROR: lib/ansible/modules/network/pfsense/pfsense_aggregate.py:0:0: doc-required-mismatch: Argument 'destination' in argument_spec found in aggregated_rules is not required, but is documented as being required (75%)
13:46 ERROR: lib/ansible/modules/network/pfsense/pfsense_aggregate.py:0:0: doc-required-mismatch: Argument 'enable' in argument_spec found in aggregated_interfaces is not required, but is documented as being required (75%)
13:46 ERROR: lib/ansible/modules/network/pfsense/pfsense_aggregate.py:0:0: doc-required-mismatch: Argument 'interface' in argument_spec found in aggregated_interfaces is not required, but is documented as being required (75%)
13:46 ERROR: lib/ansible/modules/network/pfsense/pfsense_aggregate.py:0:0: doc-required-mismatch: Argument 'interface' in argument_spec found in aggregated_rule_separators is not required, but is documented as being required (75%)
13:46 ERROR: lib/ansible/modules/network/pfsense/pfsense_aggregate.py:0:0: doc-required-mismatch: Argument 'source' in argument_spec found in aggregated_rules is not required, but is documented as being required (75%)
13:46 ERROR: lib/ansible/modules/network/pfsense/pfsense_aggregate.py:0:0: doc-required-mismatch: Argument 'state' in argument_spec found in aggregated_aliases is not required, but is documented as being required (75%)
13:46 ERROR: lib/ansible/modules/network/pfsense/pfsense_aggregate.py:0:0: doc-required-mismatch: Argument 'state' in argument_spec found in aggregated_interfaces is not required, but is documented as being required (75%)
13:46 ERROR: lib/ansible/modules/network/pfsense/pfsense_aggregate.py:0:0: doc-required-mismatch: Argument 'state' in argument_spec found in aggregated_rule_separators is not required, but is documented as being required (75%)
13:46 ERROR: lib/ansible/modules/network/pfsense/pfsense_aggregate.py:0:0: doc-required-mismatch: Argument 'state' in argument_spec found in aggregated_vlans is not required, but is documented as being required (75%)

This issue is that the individual modules allow for a "removal" (state: absent) mode - and so many arguments are optional, while in the aggregate module all items are in the "add" (state: present) mode. I'm not sure what the best way to handle this as it is nice to be able to re-use as much of the argument specs as possible.

[f-bor]

it's fixed. Every parameters with choices and a default value is considered as always present. So in that case required=True is invalid (the linter complains about it), and required=False is tautological.

I have also removed description and details from required parameters on alias when state is equal to present (they are not).

We should etablish a todo list before submitting our code for real.

[f-bor]

done:

• implement icmp types in pfsense_rule
• complete rules testing (check the CLI output)
• a few bugs in pfsense_interface needs to be fix
• clarify interface terminology (in interfaces xml, it can be named by tag, by descr or by if)

todo:

• full code review
• rewrite user, group, ca and ldap modules with module_base inheritance
• write unit tests for these modules
• check ip type consistency between ipv4 & ipv6 everywhere there is an ip protocol parameter (pfsense_rule is not doing this check for example)
• implement ipv6 and dhcp in interfaces
• for each delete (absent), check if the object is in use (missing at least for aliases)

maybe:

• i dont like much what we've done in modules_util/pfsense.py regarding init (searching all kind of nodes for later). A when needed search & cache would be more clean imo.
• split into multiple files module_utils/pfsense.py which is starting to be too big (I have wrote last new functions in __impl folder)
• in rules, the protocol default to any. In the web gui, it's tcp. Maybe it would be better (more intuitive) to stay on pfsense gui default values ?
• in rules, we're using a parameters 'name' when it's a 'descr'. It's confusing since there is some modules that use real name fields (like alias or gateway).

[f-bor]

we have an issue with ipv6 on rules. Since we've used ":" as a separator, we can't correctly parse something like 2001::2001:22. It can either be the address 2001::2001:22 or the address 2001::2001 on port 22. I suggest to split the source and destination fields and to create two new parameters for the ports (like source_port and destination_port)

[f-bor]

I have added the two parameters. The old syntax is still working but a warning is emitted about the deprecation.

Also, the source, source_port, destination & destination_port are a bit long. How about just src, src_port, dst and dst_port ?

[opoplawski]

They are long, but that it what the iptables module uses so i would like to be consistent with that I think.

[f-bor]

Ok. I took a look to iptables module, they used ':' as a separator for port range. Do you want us to do the same ? (in the new fields, with a proper warning)

[f-bor]

Regarding the interface names:

• the xml descr field is the display name, which is named "interface" in modules parameters
• the xml if field is the os name (igb0, igb0.100, etc.), which is also named "interface" in pfsense_vlan parameters or 'port' in some parts of pfSense code
• the xml tag is the internal pfSense name, or id, which should never be exposed to users, and is used all along config.xml

Therefore, to clarify, variables or parameters should use:

• interface or displayname (interface in parameters, get_interface_by_displayname)
• interface_port (interface_port in parameters, get_interface_by_port)
• interface_id (get_interface_by_id)

I will do some changes to reflect that unless you disagree.

[f-bor]

Hi,

It's on my mind (as general & advanced setup, packages, frr, ...). Which vip type(s) are you using ?

[aded]

Hi @f-bor , glad to hear that :-) I'm using "ip alias" vip.

[piethonkoop]

Awesome job, thanks!

+1 for VIP IP Alias :)

[f-bor]

@piethonkoop Thanks.

I don't have free time for now but my roadmap/needs for the next new modules is:

• pfsense_log
• pfsense_notification
• pfsense_package
• pfsense_patch
• pfsense_shellcmd
• pfsense_cron
• pfsense_frr
• pfsense_frr_acl
• pfsense_frr_ospf
• pfsense_frr_ospf_interface

Since it's not much work, I will probably start with the vip ip alias.

[piethonkoop]

Wonderful!

Thanks,

Piet

On 2020-02-20 06:56, Frederic Bor wrote:

@piethonkoop https://github.com/piethonkoop Thanks.

I don't have free time for now but my roadmap/needs for the next new modules is:

• pfsense_log
• pfsense_notification
• pfsense_package
• pfsense_patch
• pfsense_shellcmd
• pfsense_cron
• pfsense_frr
• pfsense_frr_acl
• pfsense_frr_ospf
• pfsense_frr_ospf_interface

Since it's not much work, I will probably start with the vip ip alias.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/opoplawski/ansible-pfsense/issues/40?email_source=notifications&email_token=AHYYCAZYP53KTXDCRUR73GTRDYLSBA5CNFSM4KHNZPH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMK27KQ#issuecomment-588623786, or unsubscribe https://github.com/notifications/unsubscribe-auth/AHYYCA2XJHCMNJ5G3YJHPRTRDYLSBANCNFSM4KHNZPHQ.Web Bug from https://github.com/notifications/beacon/AHYYCAZMFUH4LSF65DF74GLRDYLSBA5CNFSM4KHNZPH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMK27KQ.gif

[ { "@context": "http://schema.org", "@type": "EmailMessage", "potentialAction": { "@type": "ViewAction", "target": "https://github.com/opoplawski/ansible-pfsense/issues/40?email_source=notifications\u0026email_token=AHYYCAZYP53KTXDCRUR73GTRDYLSBA5CNFSM4KHNZPH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMK27KQ#issuecomment-588623786","url": "https://github.com/opoplawski/ansible-pfsense/issues/40?email_source=notifications\u0026email_token=AHYYCAZYP53KTXDCRUR73GTRDYLSBA5CNFSM4KHNZPH2YY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGOEMK27KQ#issuecomment-588623786", "name": "View Issue" }, "description": "View this Issue on GitHub", "publisher": { "@type": "Organization", "name": "GitHub", "url": "https://github.com" } } ]

Dit bericht is gescanned op virussen en andere gevaarlijke inhoud door MailScanner en lijkt schoon te zijn.

[lhanson]

If I'm reading things correctly, it's not currently possible to use this collection to manage services like DHCP or DNS, is that correct? Is that functionality planned, or should I approach this a different way?

[opoplawski]

@lhanson - it certainly could be added if anyone has the time/inclination. Feel free to file an RFE request so that is stays on the radar.

[opoplawski]

So, I've ported user, group, ca, and authserver_ldap to PFSenseModuleBase. Need to finally start writing unit tests myself...

[f-bor]

good news ! Writing unit tests is boring but it saved us from a lot of bugs :)

[opoplawski]

Travis CI is now running ansible-test sanity and units tests.

[opoplawski]

Closing this repo down. Please file new requests at https://github.com/pfsensible/core