Skip to content
This repository has been archived by the owner on Jan 6, 2023. It is now read-only.

TODO list #40

Closed
opoplawski opened this issue Jan 16, 2020 · 18 comments
Closed

TODO list #40

opoplawski opened this issue Jan 16, 2020 · 18 comments

Comments

@opoplawski
Copy link
Owner

@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
Copy link
Collaborator

f-bor commented Jan 16, 2020 •
edited
Loading

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
Copy link
Collaborator

f-bor commented Jan 16, 2020 •
edited
Loading

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).

@opoplawski opoplawski changed the title aggregate modules documentation / spec mismatches TODO list Jan 17, 2020
@f-bor
Copy link
Collaborator

f-bor commented Jan 18, 2020

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
Copy link
Collaborator

f-bor commented Jan 19, 2020 •
edited
Loading

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
Copy link
Owner Author

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

@f-bor
Copy link
Collaborator

f-bor commented Jan 20, 2020

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
Copy link
Collaborator

f-bor commented Jan 20, 2020 •
edited
Loading

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
Copy link
Collaborator

f-bor commented Jan 30, 2020

Hi,

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

@aded
Copy link

aded commented Jan 30, 2020

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

@piethonkoop
Copy link

Awesome job, thanks!

+1 for VIP IP Alias :)

@f-bor
Copy link
Collaborator

f-bor commented Feb 20, 2020

@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
Copy link

piethonkoop commented Feb 20, 2020 via email

@lhanson
Copy link

lhanson commented Mar 23, 2020

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
Copy link
Owner Author

@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
Copy link
Owner Author

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

@f-bor
Copy link
Collaborator

f-bor commented Jun 7, 2020

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

@opoplawski
Copy link
Owner Author

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

@opoplawski
Copy link
Owner Author

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

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
5 participants
@lhanson @opoplawski @aded @piethonkoop @f-bor