Support diff mode when using vyos_firewall_rules
#222
Conversation
This change adds support for [diff mode](https://docs.ansible.com/ansible/latest/user_guide/playbooks_checkmode.html#using-diff-mode) to `vyos_firewall_rules`. This can aide someone into understanding what will check when using [check mode](https://docs.ansible.com/ansible/latest/user_guide/playbooks_checkmode.html#using-check-mode), and can help someone know what has changed when running their play (using just `--diff` without `--check`).
| result["diff"] = { | ||
| "after": "\n".join( | ||
| sorted(self.set_state(self._module.params["config"], {})) | ||
| ), | ||
| "before": "\n".join( | ||
| sorted(self.set_state(existing_firewall_rules_facts, {})) | ||
| ), | ||
| } |
There was a problem hiding this comment.
This is only the right thing for overridden, so this PR needs more work. However, this got me thinking that there are two ways to accomplish this.
Commands [to be] Run
You don't get to see the commands this module (returned via results["commands"]) unless you run your play with -vvv as far as I can tell. That has a lot of other output, which makes it not useful in a lot of situations. However, we could just include those as the "after" lines in a diff, and this change would then be quite simple. It would amount to something like this:
if self.state in self.ACTION_STATES and self._module._diff:
result["diff"] = {
"after": "\n".join(result["commands"]),
"before": "",
}Resulting Config
This would basically generate what show configuration commands for the current state (easy) as well as the new state (hard). This would require new helper methods for deleted, merged, and replaced states that will compute what the config object would look like once all the changes we want to apply would look like. Ideally, we'd be able to share code between _state_deleted, _state_merged, and _state_replaced, but I don't actually see a reasonable way to do that.
Comparison
| Option | Pros | Cons |
|---|---|---|
| Commands Run |
|
|
| Resulting Config |
|
|
Despite the table outcome, I actually think the resulting config approach is the better one long-term. However, I don't want to do a bunch of work to implement that if it's not wanted. Therefore, I'm going to wait for some input from y'all before I push this further.
I'd also like to add some tests for this, but unfortunately ansible-test fails a bunch of tests for me, and there aren't really any docs that tell me how to run them locally for this module (at least that I've found). Uploading and waiting for zuul to find out if it works or not isn't a way I want to develop this.
There was a problem hiding this comment.
Code re-use is actually possible here, so that's not a con of the resulting config approach after all. If we go that route, I'll do a refractor in a seperate PR.
There was a problem hiding this comment.
@ashwini-mhatre, any thoughts here? I don't want to do a bunch of work that the project doesn't ultimately want, but I think getting check mode added will be a valuable feature.
There was a problem hiding this comment.
@sdwilsh This seems to be a good feature in my mind as of now. Let me take 2 days to process this and discuss the same with the team and get back to you. Thank you.
There was a problem hiding this comment.
@GomathiselviS, any thoughts on direction to go here?
There was a problem hiding this comment.
@GomathiselviS, bump for any thoughts on direction to go here. I'm still interested in solving this because I think it'll help track down an issue where I see my firewall rules always being listed as changed when I am pretty sure they are not.
|
I'm still interested in getting this change in, for what it's worth. |
|
Hi Shawn. We're in the process of getting things restarted here (I have some PRs that I'm going to be putting in soon as well), having only gotten control of the repository early this week. I'd recommend starting by rebasing on to the current source and checking tests to make sure you've got the baseline working. I was able to successfully run tests with the final version before the transfer, so I expect it should be working now. |
|
This PR was waiting some design feedback from the previous repo owners because I saw two different paths forward, so I guess I'm looking for your design feedback now. |
SUMMARY
This change adds support for diff mode to
vyos_firewall_rules. This can aide someone into understanding what will check when using check mode, and can help someone know what has changed when running their play (using just--diffwithout--check).ISSUE TYPE
COMPONENT NAME
vyos_firewall_rules