Fix xFirewall if CIDR notation used for LocalAddress/RemoteAddress - Fixes #169

[PlagueHO]

The primary purpose for this PR is to fix issue #169. In fixing this I also uncovered a bug in the Integration tests for xFirewall that resulted in the firewall rule properties not actually being tested after the rule was set. Fixing this didn't actually identify any test failures.

However, I've also begun work on the resource to bring it up to HQRM (issue #135) and added the CommonResourceHelper.psm1 module (issue #152).

The detailed list of changes are as follows:

• Added CommonResourceHelper.psm1 based on copy from PSDscResources.
• MSFT_xFirewall:
  • Cleaned up ParameterList table layout and moved into a new file (MSFT_xFirewall.data.psd1).
  • Separated Localization strings into strings file.
  • Added standard help blocks to all functions to meet HQRM standards.
  • Added CmdletBinding attribute to all functions to meet HQRM standards.
  • Style changes to meet HQRM standards.
  • Fixed issue using CIDR notation for LocalAddress or RemoteAddress. See GitHub issue.
  • Fixed integration tests so that values being set are correctly tested.
  • Added integration tests for Removal of Firewall rule.
• Resolved final markdown errors (MD*) in Readmd.md.
• Added NetworkingCommon module to contain shared networking functions.

---

This change is 

[PlagueHO]

@tysonjhayes - I'm thinking this one will need some fixing up before it can be merged once your PR #174 goes through. So no need to rush out a review on this till after then.

[tysonjhayes]

Reviewed 3 of 14 files at r1, 1 of 1 files at r2.
Review status: 4 of 13 files reviewed at latest revision, 1 unresolved discussion.

---

Tests/Unit/MSFT_xFirewall.Tests.ps1, line 622 at r2 (raw file):

            foreach ($parameter in $ParameterList)
            {
                $parameterSource = (Invoke-Expression -Command "`$($($parameter.source))")

This will need an override at the top of the file as we're not supposed to use invoke-expression, however, I'm not really sure how else to get around that rule.

---

Comments from Reviewable

[tysonjhayes]

Review status: 4 of 13 files reviewed at latest revision, 2 unresolved discussions.

---

Tests/Integration/MSFT_xFirewall_add.config.ps1, line 45 at r2 (raw file):

    node localhost {
       xFirewall Integration_Test {
            Name                  = $Node.RuleName

I might be missing something but where is the node variable defined?

---

Comments from Reviewable

[tysonjhayes]

Review status: 4 of 13 files reviewed at latest revision, 4 unresolved discussions.

---

DSCResources/MSFT_xFirewall/MSFT_xFirewall.psm1, line 6 at r2 (raw file):

    -Parent
$script:commonResourceHelperFilePath = Join-Path `
    -Path $script:dscResourcesFolderFilePath `

I know we're holding off on final paths until I merge in my thing, but I noted SharePoint has a \SharepointDsc\Modules directory that I think this would be a better fit under, so it kind of cleanly encapsulates everything.

---

DSCResources/MSFT_xFirewall/MSFT_xFirewall.psm1, line 16 at r2 (raw file):

Import-Module -Name ( Join-Path `
    -Path (Split-Path -Path $PSScriptRoot -Parent) `
    -ChildPath '\NetworkingCommon\NetworkingCommon.psm1' )

See above on path.

---

Comments from Reviewable

[PlagueHO]

Review status: 0 of 15 files reviewed at latest revision, 4 unresolved discussions.

---

Tests/Integration/MSFT_xFirewall_add.config.ps1, line 45 at r2 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

I might be missing something but where is the node variable defined?

$Node is a variable that automatically gets exposed by the node block if a ConfigurationData parameter was passed to the DSC Configuration when it is compiled. You'll see that the a $ConfigData object is defined and passed in the integration test. This is a little more elegant than the older solution so it's used more often now (but I haven't updated everything to use it yet).

---

Comments from Reviewable

[PlagueHO]

Review status: 0 of 15 files reviewed at latest revision, 4 unresolved discussions.

---

Tests/Unit/MSFT_xFirewall.Tests.ps1, line 622 at r2 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

This will need an override at the top of the file as we're not supposed to use invoke-expression, however, I'm not really sure how else to get around that rule.

I gave this a try, but unfortunately, suppressing the rule actually violates some of the standard tests. We can only suppress certain rules and this isn't one of them 😢

So, we might have to leave the warning here and figure out a way to address rewrite the code as part of issue #166.

---

DSCResources/MSFT_xFirewall/MSFT_xFirewall.psm1, line 6 at r2 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

I know we're holding off on final paths until I merge in my thing, but I noted SharePoint has a \SharepointDsc\Modules directory that I think this would be a better fit under, so it kind of cleanly encapsulates everything.

Done.

---

DSCResources/MSFT_xFirewall/MSFT_xFirewall.psm1, line 16 at r2 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

See above on path.

Done.

---

Comments from Reviewable

[PlagueHO]

@tysonjhayes - this is what happens when I try and suppress the rule:

[tysonjhayes]

I get the intent of that test but I'm not sure I agree with it. I guess we just take the warning for now...

---

Reviewed 3 of 23 files at r1, 11 of 22 files at r3, 12 of 12 files at r4.
Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

DSCResource.Tests, line 1 at r4 (raw file):

Subproject commit e7c0dc5a29a3cc23c2ce84ad23ff02353a799280

What's the deal with this?

---

Modules/xNetworking/DSCResources/MSFT_xFirewall/MSFT_xFirewall.psm1, line 120 at r4 (raw file):

<#
    .SYNOPSIS

So because of the merge we took with my changes I believe the help is auto generated from the mof files, do we still need this here?

---

Modules/xNetworking/DSCResources/MSFT_xFirewall/en-US/MSFT_xFirewall.strings.psd1, line 4 at r4 (raw file):

ConvertFrom-StringData @'
GettingFirewallRuleMessage = Getting firewall rule with Name '{0}'.

Should all of these be tabed over?

---

Modules/xNetworking/Examples/Resources/xFirewall/2-xFirewall_AddFirewallRule_AllParameters.ps1, line 20 at r4 (raw file):

        xFirewall Firewall
        {
            Name                  = "NotePadFirewallRule"

Any reason we're going back to double quotes?

---

Tests/Integration/MSFT_xFirewall_remove.config.ps1, line 2 at r4 (raw file):

<#
  This file exists so we can load the test file without necessarily having xNetworking in

Just curious, are we creating .ps1 for every config rule? I always figured one file with all of the rules made sense, and allowed us to load the file once instead of before every rule.

---

Comments from Reviewable

[PlagueHO]

I know what you mean. I guess there are going to have to be exceptions to this test though - unless we can find a different way of writing the code.

@kwirkykat - does your team have any thoughts on this?

The issue is that we're using Invoke-Expression to allow the large number of parameters in the Firewall to be stored in a PSD1 data structure and processed by various loops instead of writing a whole lot of code. We haven't got an alternative to using Invoke-Expression for this code. But we can't suppress the message. Perhaps you guys have got some ideas on what we might replace it with?

---

Review status: all files reviewed at latest revision, 5 unresolved discussions.

---

DSCResource.Tests, line 1 at r4 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

What's the deal with this?

Er um. I'm not sure how that got there? Possibly turned up when I was working on fixing the merge conflicts and figuring out how to test the repo with the new layout. It shouldn't be there though - good catch. Removed.

---

Modules/xNetworking/DSCResources/MSFT_xFirewall/MSFT_xFirewall.psm1, line 120 at r4 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

So because of the merge we took with my changes I believe the help is auto generated from the mof files, do we still need this here?

This is one of the many requirements of HQRM 😁

I think it does help though because it does enable a bit of extra detail in intellisense when writing DSC Configs.

---

Modules/xNetworking/DSCResources/MSFT_xFirewall/en-US/MSFT_xFirewall.strings.psd1, line 4 at r4 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

Should all of these be tabed over?

Yes - it should. Done.

---

Modules/xNetworking/Examples/Resources/xFirewall/2-xFirewall_AddFirewallRule_AllParameters.ps1, line 20 at r4 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

Any reason we're going back to double quotes?

Nope. I fixed it. Sorry about that!

---

Tests/Integration/MSFT_xFirewall_remove.config.ps1, line 2 at r4 (raw file):

Previously, tysonjhayes (Tyson J. Hayes) wrote…

Just curious, are we creating .ps1 for every config rule? I always figured one file with all of the rules made sense, and allowed us to load the file once instead of before every rule.

I've tweaked this a bit by moving the $Rule into the $ConfigData object which gets passed to the Configs. I probably could have just used a single config and just passed a $ConfigData with different node properties. I just felt it was cleaner separating them (and that was the design pattern I'd used elsewhere - see xPSDesiredStateConfiguration). But happy to change it if you think it's better that way?

---

Comments from Reviewable

[TravisEz13]

LGTM

[tysonjhayes]

Reviewed 1 of 23 files at r1, 1 of 22 files at r3, 6 of 6 files at r5.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

Modules/xNetworking/DSCResources/MSFT_xFirewall/MSFT_xFirewall.psm1, line 120 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

This is one of the many requirements of HQRM 😁

I think it does help though because it does enable a bit of extra detail in intellisense when writing DSC Configs.

I'm more thinking I'm not seeing that on SharePointDsc for example, and it would mean we have documentation for the functions in two different places (mof file and the ps1) which could lead to discrepancies on the documentation. I'm not sure IN THE FILE is necessary, as long as the help is present (which we do on build).

---

Tests/Integration/MSFT_xFirewall_remove.config.ps1, line 2 at r4 (raw file):

Previously, PlagueHO (Daniel Scott-Raynsford) wrote…

I've tweaked this a bit by moving the $Rule into the $ConfigData object which gets passed to the Configs. I probably could have just used a single config and just passed a $ConfigData with different node properties. I just felt it was cleaner separating them (and that was the design pattern I'd used elsewhere - see xPSDesiredStateConfiguration). But happy to change it if you think it's better that way?

Honestly I'm not sure which I would prefer, let's leave it this way for now.

---

Comments from Reviewable