cs_firewall: add dest cidrs

[resmo]

closes #76

/cc @nathanmcgarvey may you have a look and test it?

[codecov[bot]]

Codecov Report

Merging #84 (f228096) into master (d16c15f) will decrease coverage by 0.02%. The diff coverage is 66.66%.

@@            Coverage Diff             @@
##           master      #84      +/-   ##
==========================================
- Coverage   84.25%   84.23%   -0.03%     
==========================================
  Files          56       56              
  Lines        5597     5602       +5     
  Branches     1255     1256       +1     
==========================================
+ Hits         4716     4719       +3     
- Misses        445      446       +1     
- Partials      436      437       +1     

Impacted Files	Coverage Δ
plugins/modules/cs_firewall.py	87.60% <66.66%> (-1.19%)	:arrow_down:

---

Continue to review full report at Codecov.

Legend - Click here to learn more Δ = absolute <relative> (impact), ø = not affected, ? = missing data Powered by Codecov. Last update d16c15f...f228096. Read the comment docs.

[rvalle]

hi @resmo, this is implemented but not released, right? Is it testing that is missing? I would also benefit from this enhancement, I can help here.

[resmo]

hey @rvalle, yes, only testing is missing.

[rvalle]

ok @resmo will test it and let you know.

[rvalle]

@resmo, setting dest_cidr works, however, it should be called "dest_cidrs", as it takes an array of cidrs for destination, and for source, the parameter it is already called cidrs, in plural.

I tested with one, and several values and they get set properly on ACS (using 4.16.x) What I noticed, is that once set, if the module is called with another set of dest_cidr the rule is not executed. dest_cidr seems not to be part of the data used to workout if the module needs to be applied or if the state is already OK.

[rvalle]

ok, I can see that it is dest_cidrs, dont know where I picked dest_cidr from, which I see it works because it is an alias only.

[resmo]

ok, I can see that it is dest_cidrs, dont know where I picked dest_cidr from, which I see it works because it is an alias only.

yeah, it was a "design decision" I made in the early development days to have an alias for lists in the "singular" form because ansible allows to use the list as "single" value:

dest_cidrs: example

when you would expect it should have been:

dest_cidrs: [ example ]

that is why a singluar alias makes sense:

dest_cidr: example

But the downside of it, is, you can not see anymore from the name that it the value is actually a list.

[rvalle]

@resmo I dont see any obvious bug with not detecting the change of dest_cidrs... any idea?

[rvalle]

@resmo I would suggest we release this feature in its current state. it is certainly an improvement.

It does not always detect changes, but it is certainly not the only module with that issue. Perhaps we should open a different issue to deal with functionalities for which change is not detected.