win_firewall_rule only change arguments passed by user

[mhunsber]

defaults are controlled by com object
integration test for built in rule

SUMMARY

the current win_firewall_rule module will reset an existing firewall rule's options to the defaults for any unspecified parameters. This is especially a problem when trying to enable a built-in windows firewall rule since changing them is protected.
This just keeps unspecified parameters as null so that they can be checked for existence when changing an existing rule.

Fixes #34392

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

win_firewall_rule

ADDITIONAL INFORMATION

This is the result of the change, instead of needing to pass back in the port and protocol, the module only changes action, because it was different from the existing rule.

tasks:
- name: Add firewall rule
  win_firewall_rule:
    name: http
    enabled: yes
    state: present
    localport: 80
    action: allow
    direction: in
    protocol: tcp

- name: Change firewall rule
  win_firewall_rule:
    name: http
    state: present
    action: block
    direction: in

output (diff):

TASK [win_firewall_rule : Add firewall rule] *********************************************************************************************************************
+[Name='http']
+[Description='']
+[Direction='1']
+[Action='1']
+[ApplicationName='']
+[ServiceName='']
+[Enabled='True']
+[Profiles='2147483647']
+[LocalAddresses='*']
+[RemoteAddresses='*']
+[LocalPorts='80']
+[RemotePorts='*']
+[Protocol='6']
+[InterfaceTypes='All']
+[EdgeTraversalOptions='0']
+[SecureFlags='0']
changed: [testhost]

TASK [win_firewall_rule : Change firewall rule] ******************************************************************************************************************
-[Action='1']
+[Action='0']
changed: [testhost]

[ansibot]

cc @timothyvandenbrande @ar7z1 @dagwieers @if-meaton @jborean93 @jhawkesworth @nitzmahone
click here for bot help

[ansibot]

The test ansible-test sanity --test pslint [explain] failed with 1 error:

test/sanity/pslint/ignore.txt:75:1: A102 Remove since "lib/ansible/modules/windows/win_firewall_rule.ps1" passes "PSAvoidUsingCmdletAliases" test

click here for bot help

[ansibot]

cc @mattclay
click here for bot help

[jborean93]

I've only done a brief look through but I have a few things;

• If we wanted to move ahead with this, we need to update the documentation around the default values. You've "removed" the default values but the docs don't reflect that
• We still need to continue the existing behaviour where when we create a brand new rule the existing default still apply.
• I see you've added a test but I don't fully see how that tests this change. I would have liked to see something simple like disabling a rule by setting only name and enabled

Let me know your thoughts on this

[mhunsber]

@jborean93 thanks for the feedback, and I have a few questions about what should be done for the documentation

• I've "removed" the default values, but if you look at the previous code inside the New-FwRule function, it doesn't use the defaults to set any values, but relies on the defaults for the constructor of the HNetCfg.FWRule com object. e.g. if ($protocol -and $protocol -ne "any") { $rule.Protocol = Parse-ProtocolType -protocol $protocol }. The exception is enabled, which has the opposite default from the COM object. Since my changes behave similarly, I didn't know if the defaults should be changed in the documentation. If you think it is better to remove the default flag from the documentation, we would need to add in the description that it defaults to those values for new rules.
• The first point somewhat answers the second point: even though the Get-AnsibleParam call has the default parameter removed, it doesn't change the behavior that if the user does not specify the value, it "defaults" to those values if the firewall rule is created, because they're driven by the defaults for the com object.
• I agree the test is confusing. With the previous module, that call would fail because the built in firewall rules don't allow you to modify any properties except the enabled flag. I'll change it to be more like the example in the issue I referenced, but that will mean changing some of the required parameters.

[jborean93]

@mhunsber thanks for the changes, I've make some slight tweaks to the documentation but everything else seems good to me. As for your questions;

I didn't know if the defaults should be changed in the documentation

The default: key in the documentation is meant to signify the default value for the option as set by the arg spec (or Get-AnsibleParam). If there is a default based on the state of the rule during execution, i.e. creating a new rule, then what you've done is correct by documenting it in the description.

it doesn't change the behavior that if the user does not specify the value, it "defaults" to those values if the firewall rule is created, because they're driven by the defaults for the com object.

Somewhat, the defaults still indicate the default for the module options which was correct before the PR and based on the changes you are still correct. Because of the defaults were set on the options and the property checks done later on I still think the rules would have changed if a user chose a non-default constructor option but agree it is slightly confusing and convoluted how it was done before.

I'll change it to be more like the example in the issue I referenced, but that will mean changing some of the required parameters.

The tests look good now, easy to see how/why it works and you have a good assertion which is great to see.

[mhunsber]

Awesome! Thanks for the feedback and clarifications.