win_firewall_rule: Implement idempotency, check-mode and diff support #23162

dagwieers · 2017-03-30T23:47:29Z

SUMMARY

This PR includes the following changes:

An idempotency fix when profile: any
Documentation fixes (remove required: false)
Parameter handling fixes
RDP example that matches default RDP rule
Renamed parameter 'enable' to 'enabled' (kept alias for compatibility)
Renamed parameter 'profile' to 'profiles' (kept alias for compatibility)
Implemented check-mode support
Implemented diff support
Adapted integration tests
Returns proper command output on failure.

This fixes #23455 and #24976

ISSUE TYPE

Bugfix Pull Request

COMPONENT NAME

win_firewall_rule

ANSIBLE VERSION

v2.3

ansibot · 2017-03-31T00:04:11Z

The test ansible-test sanity --test validate-modules failed with the following error:

lib/ansible/modules/windows/win_firewall_rule.py:0:0: E311 EXAMPLES is not valid YAML. Line 112 column 3

click here for bot help

ansibot · 2017-03-31T00:04:12Z

cc @timothyvandenbrande @ar7z1
click here for bot help

mattclay · 2017-04-01T00:03:19Z

Integration test failure: https://app.shippable.com/github/ansible/ansible/runs/17791/9/tests

Here's one example from test/integration/targets/win_firewall_rule/tasks/main.yml:39:

{
    "assertion": "add_firewall_rule_again.changed == false", 
    "changed": false, 
    "evaluated_to": false, 
    "failed": true
}

ar7z1

shipit

ar7z1 · 2017-04-02T16:35:40Z

lib/ansible/modules/windows/win_firewall_rule.ps1

+If ($profiles -eq "any") {
+    $fwsettings.Add("Profiles", "Domain,Private,Public")
+} Else {
+    $fwsettings.Add("Profiles", (Get-Culture).textinfo.totitlecase($profiles))


It seems it can be removed. Comparison operator -eq are case-insensitive by default:

By default, all comparison operators are case-insensitive. To make a comparison operator case-sensitive, precede the operator name with a c. For example, the case-sensitive version of -eq is -ceq. To make the case-insensitivity explicit, precede the operator with an i. For example, the explicitly case-insensitive version of -eq is -ieq.

Aha, good to know. Thanks !

ar7z1 · 2017-04-02T16:43:31Z

@dagwieers Thank you for excellent work!
I can't understand what's wrong with the tests. I will have access to my dev machine in a few days. I'll try to debug the failed tests.

jborean93 · 2017-04-02T22:27:55Z

shipit

ar7z1 · 2017-04-10T17:03:28Z

@dagwieers I created a PR to your PR. ;-)

dagwieers · 2017-04-11T09:44:33Z

@ar7z1 I have retained the fix for the integration tests from your PR. So that this PR becomes mergeable again.

However I noticed my changes to "differences" do not make sense (useful for debugging, but output is not very clear), and do not use the normal provisions for showing diff output (-D/--diff). So we definitely need to revise this too. So I am going to fix my PR for that too.

dagwieers · 2017-04-11T12:01:15Z

Alright, after changes what needed to be changed I ended up rewriting most of the module.
Although the logic is still the same.

skasai · 2017-05-29T18:20:55Z

Question - Would this be the reason I can't set/update firewall rules with current win_firewall_rule? I am currently dipping my feet into Ansible and noticing when I did tests on machines, I wasn't creating the rule. Using the force option did delete pre-defined rule.

dagwieers · 2017-05-29T18:50:08Z

@skasai Yup, as I added to the notes in this PR:

Modifying existing firewall rules is not possible, the module does allow replacing complete rules based on name, but that works by removing the existing rule completely, and recreating it with provided information (when using C(force)).

dagwieers · 2017-05-29T18:52:20Z

@nitzmahone Can you please merge this PR ? It is almost 2 months old, and people are still having issues with things that this PR fixes.

skasai · 2017-05-29T19:28:37Z

Yup, as I added to the notes in this PR:

Modifying existing firewall rules is not possible, the module does allow replacing complete rules based on name, but that works by removing the existing rule completely, and recreating it with provided information (when using C(force)).

Ok... I was playing around with this last night and today and scratching my head as to why it said changed and looking on the test machines, I see no added rule and wondering why it deleted the existing rule when I did force.

Even running with -vvvvvv, wasn't really indicative of what was going on.

I just found this this morning and was wondering why the module that is listed wasn't working as intended.

dagwieers · 2017-05-29T19:31:09Z

@skasai Was this with PR #23162 or with the version in v2.3 or devel ?

skasai · 2017-05-30T03:00:29Z

At the moment, not sure, since I know the version of Ansible is v2.3.0.0 right now and what I see near the end is this:

changed: ['machine'] => {
"changed": true,
"difference": [],
"failed": false,
"fwsettings": {
"Action": "allow",
"Direction": "in",
"Enabled": "yes",
"LocalIP": "any",
"LocalPort": "any",
"Profiles": "Private,Public",
"Protocol": "ICMPv4",
"RemoteIP": "'iprange'/24",
"RemotePort": "any",
"Rule Name": "ATest",
"Service": "any"
},
"msg": [
"No rule could be found",
"Created firewall rule ATest"
]
}
META: ran handlers
META: ran handlers

PLAY RECAP **********************************************************************
'machine' : ok=2 changed=1 unreachable=0 failed=0

This is on CentOS 7 doing a yum install so not devel. Not sure how to do PR or what not.

dagwieers · 2017-05-30T06:08:20Z

@skasai If you would have been running this code, it would have told you that you cannot provide a localport or remoteport when using a protocol other than TCP or UDP.

skasai · 2017-05-30T07:16:13Z

Figured as much. How would one pull down a PR then for testing? I saw exerpt code for files, but didn't seem to line up so I avoided trying to hand modify.

dagwieers · 2017-05-30T07:25:59Z

@skasai The easiest option is to take the file and put it in library/, however in this case we also modify the vmware library, so that's not going to work. If you don't know git very well, maybe it's best to wait for this to be released. At least you now know how to fix this :-)

skasai · 2017-05-30T21:55:06Z

Took me a while and a few mistakes but hand modified it based off the PR mods and it looks like this works. Was able to create the test firewall rule when earlier behavior showed no changes.

Do note that it treated 'existing rule' with 'force' as not counting as 'changed', but I suppose this is expected behavior?

dagwieers · 2017-05-30T22:15:27Z

Indeed, it compares the before and after, and only reports a change if there is a difference. Thanks for testing !

skasai · 2017-05-30T22:20:53Z

@dagwieers Ok. Although technically if you are forcing a change in values on an existing rule, technically, that is a change. :)

What I see is this:

"msg": [
    "The rule 'File and Printer Sharing (Echo Request - ICMPv4-In)' exists.", 
    "The rule 'File and Printer Sharing (Echo Request - ICMPv4-In)' exists but has different values", 
    "Removed the rule 'File and Printer Sharing (Echo Request - ICMPv4-In)'", 
    "Created firewall rule 'File and Printer Sharing (Echo Request - ICMPv4-In)'"
]

Run again:
"msg": [
"The rule 'File and Printer Sharing (Echo Request - ICMPv4-In)' exists.",
"An identical rule exists",
"Firewall rule 'File and Printer Sharing (Echo Request - ICMPv4-In)' was already created"
]

In both cases, Ansible did show 'changed': false.
ok: ['machine'] => {
"changed": false,
"diff": null,
"fwsettings": {
"Action": "Allow",
"Direction": "In",
"Edge traversal": "No",
"Enabled": "Yes",
"InterfaceTypes": "Any",
"LocalIP": "any",
"Profiles": "Private,Public",
"Protocol": "ICMPV4",
"RemoteIP": "'iprange'/24",
"Rule Name": "File and Printer Sharing (Echo Request - ICMPv4-In)",
"Security": "NotRequired"
},

dagwieers · 2017-05-30T22:36:33Z

@skasai That's a bug. Although looking at the code, it seems not possible to have changed=False in this case.

I did improve the output so it doesn't state twice that the rule exists.

dagwieers force-pushed the wfr_idempotency branch from b1851a0 to 475247a Compare March 30, 2017 23:55

dagwieers force-pushed the wfr_idempotency branch 4 times, most recently from 53d5fd9 to f9266e3 Compare March 31, 2017 01:07

s-hertel removed the needs_triage Needs a first human triage before being processed. label Mar 31, 2017

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Apr 1, 2017

ar7z1 approved these changes Apr 2, 2017

View reviewed changes

dagwieers force-pushed the wfr_idempotency branch from f9266e3 to a592ded Compare April 3, 2017 08:47

ansibot added test_pull_requests and removed ci_verified Changes made in this PR are causing tests to fail. labels Apr 3, 2017

mattclay added the ci_verified Changes made in this PR are causing tests to fail. label Apr 3, 2017

ar7z1 added a commit to ar7z1/ansible that referenced this pull request Apr 10, 2017

Fix PR ansible#23162

afbde71

dagwieers force-pushed the wfr_idempotency branch from a592ded to 8bb4322 Compare April 10, 2017 23:14

ansibot removed the ci_verified Changes made in this PR are causing tests to fail. label Apr 10, 2017

dagwieers changed the title ~~win_firewall_rule: Small idempotency fix~~ win_firewall_rule: Implement idempotency, check-mode and diff support Apr 11, 2017

dagwieers mentioned this pull request May 30, 2017

win_firewall_rule: If adding fails, rule is removed. #25157

Closed

Improve the logic wrt. diff

4359099

dagwieers force-pushed the wfr_idempotency branch from d115b02 to 4359099 Compare May 30, 2017 22:42

ansibot added community_review In order to be merged, this PR must follow the community review workflow. and removed shipit This PR is ready to be merged by Core stale_ci This PR has been tested by CI more than one week ago. Close and re-open this PR to get it retested. labels May 30, 2017

nitzmahone approved these changes May 30, 2017

View reviewed changes

nitzmahone merged commit d958440 into ansible:devel May 30, 2017

jhawkesworth mentioned this pull request Jul 27, 2017

win_firewall_rule has no effect on Windows 8.1 embedded machine #27326

Closed

ar7z1 mentioned this pull request Aug 25, 2017

win_firewall_rule: Corrupted rules on Win7 and Win10 #28555

Closed

ansibot added bug This issue/PR relates to a bug. and removed bugfix_pull_request labels Mar 6, 2018

ansible locked and limited conversation to collaborators Apr 26, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

win_firewall_rule: Implement idempotency, check-mode and diff support #23162

win_firewall_rule: Implement idempotency, check-mode and diff support #23162

dagwieers commented Mar 30, 2017 •

edited

Loading

ansibot commented Mar 31, 2017

ansibot commented Mar 31, 2017

mattclay commented Apr 1, 2017

ar7z1 left a comment

ar7z1 Apr 2, 2017

dagwieers Apr 3, 2017

ar7z1 commented Apr 2, 2017

jborean93 commented Apr 2, 2017

ar7z1 commented Apr 10, 2017

dagwieers commented Apr 11, 2017

dagwieers commented Apr 11, 2017

skasai commented May 29, 2017

dagwieers commented May 29, 2017

dagwieers commented May 29, 2017

skasai commented May 29, 2017 •

edited

Loading

dagwieers commented May 29, 2017

skasai commented May 30, 2017 •

edited

Loading

dagwieers commented May 30, 2017

skasai commented May 30, 2017

dagwieers commented May 30, 2017

skasai commented May 30, 2017

dagwieers commented May 30, 2017

skasai commented May 30, 2017

dagwieers commented May 30, 2017 •

edited

Loading

win_firewall_rule: Implement idempotency, check-mode and diff support #23162

win_firewall_rule: Implement idempotency, check-mode and diff support #23162

Conversation

dagwieers commented Mar 30, 2017 • edited Loading

SUMMARY

ISSUE TYPE

COMPONENT NAME

ANSIBLE VERSION

ansibot commented Mar 31, 2017

ansibot commented Mar 31, 2017

mattclay commented Apr 1, 2017

ar7z1 left a comment

Choose a reason for hiding this comment

ar7z1 Apr 2, 2017

Choose a reason for hiding this comment

dagwieers Apr 3, 2017

Choose a reason for hiding this comment

ar7z1 commented Apr 2, 2017

jborean93 commented Apr 2, 2017

ar7z1 commented Apr 10, 2017

dagwieers commented Apr 11, 2017

dagwieers commented Apr 11, 2017

skasai commented May 29, 2017

dagwieers commented May 29, 2017

dagwieers commented May 29, 2017

skasai commented May 29, 2017 • edited Loading

dagwieers commented May 29, 2017

skasai commented May 30, 2017 • edited Loading

dagwieers commented May 30, 2017

skasai commented May 30, 2017

dagwieers commented May 30, 2017

skasai commented May 30, 2017

dagwieers commented May 30, 2017

skasai commented May 30, 2017

dagwieers commented May 30, 2017 • edited Loading

dagwieers commented Mar 30, 2017 •

edited

Loading

skasai commented May 29, 2017 •

edited

Loading

skasai commented May 30, 2017 •

edited

Loading

dagwieers commented May 30, 2017 •

edited

Loading