Should xIPAddress disable DHCP?

[dsccommunity]

Should xIPAddress disable DHCP?

[lorengordon]

Ahh, so, I just reviewed the test code and have other concerns, too... What if the IP is correct, but the subnet mask is not? Or if the default gateway is incorrect? This code will skip those issues, won't it? Apologies if I'm not reading the code correctly. This is my first time diving into DSC code.

[tysonjhayes]

I think I remember having concerns about that when I was last looking at this module. You are correct that if the default gateway, subnet mask, etc will not be validated correctly if the IP address is correct. If you wanted to submit a fix for this I'd be happy to review it and offer suggestions.

[lorengordon]

Not sure I'll have time to test this beyond my local workstation, but to address the second issue, maybe something like the below for that try/catch block.

This code doesn't address the DHCP question, but I was thinking that if this module gets the ability to disable DHCP, then it should probably be able to enable it as well, but enabling DHCP doesn't seem relevant for an 'IPAddress' module. Perhaps it would make more sense if this was an 'IPInterface' module, bent on managing all the configuration settings of the interface? Is there such a module yet?

    try
    {
        Write-Verbose -Message "Checking the IPAddress ..."
        #Get the current IP Address based on the parameters given.
        $currentIP = Get-NetIPAddress -InterfaceAlias $InterfaceAlias -AddressFamily $AddressFamily -ErrorAction Stop

        $requiresChanges=$false
        #Test if the IP Address passed is equal to the current ip address
        if(!$currentIP.IPAddress.Equals($IPAddress))
        {
            Write-Verbose -Message "IPAddress not correct. Expected $IPAddress, actual $($currentIP.IPAddress)"
            $requiresChanges = $true
        }
        else
        {
            Write-Verbose -Message "IPAddress is correct."
        }

        #Test if the Subnet Mask passed is equal to the current subnet mask
        if(!$currentIP.PrefixLength.Equals([byte]$SubnetMask))
        {
            Write-Verbose -Message "Subnet mask not correct. Expected $SubnetMask, actual $($currentIP.PrefixLength)"
            $requiresChanges = $true
        }
        else
        {
            Write-Verbose -Message "Subnet mask is correct."
        }

        #Test if the Default Gateway passed is equal to the current default gateway
        if($DefaultGateway)
        {
            $netIpConf = Get-NetIPConfiguration -InterfaceAlias $InterfaceAlias -ErrorAction Stop
            $currentGateway = ($netIpConf."$($AddressFamily)DefaultGateway").NextHop

            if(!$currentGateway.Equals($DefaultGateway))
            {
                Write-Verbose -Message "Default gateway not correct. Expected $DefaultGateway, actual $($currentIP.PrefixLength)"
                $requiresChanges = $true
            }
            else
            {
                Write-Verbose -Message "Default gateway is correct."
            }
        }

        if($requiresChanges)
        {
            #Apply is true in the case of set - target resource - in which case, it will apply the required IP configuration
            $Parameters = @{}

            if($Apply)
            {
                Write-Verbose -Message "At least one setting differs from the passed parameters. Applying configuration ..."
                $Parameters["IPAddress"] = $IPAddress
                $Parameters["PrefixLength"] = $SubnetMask
                $Parameters["InterfaceAlias"] = $currentIP[0].InterfaceAlias

                if($DefaultGateway){ $Parameters["DefaultGateWay"] = $DefaultGateway }
                $null = New-NetIPAddress @Parameters -ErrorAction Stop

                # Make the connection profile private
                Get-NetConnectionProfile -InterfaceAlias $InterfaceAlias | Set-NetConnectionProfile -NetworkCategory Private -ErrorAction SilentlyContinue
                Write-Verbose -Message "IPAddress is set to $IPAddress."
            }
            else {return $false}
        }
        else
        {
            Write-Verbose -Message "No changes were required."
            return $true
        }
    }
    catch
    {
       Write-Verbose -Message $_
       throw "Can not set or find valid IPAddress using InterfaceAlias $InterfaceAlias and AddressFamily $AddressFamily"
    }

[lorengordon]

I suppose in the $DefaultGateway test I would probably also check the parameter's format using [System.Net.Ipaddress]::TryParse()...

[tysonjhayes]

So looking over this code again I really disagree with the way its written. Most DSC code is written where Test just validates the config where Set actually does the work. With the combined methods it's logically hard to parse, but that's probably a different issue for a different pull request.

Speaking of which could you please submit this as a pull request? We'll want to run it through the existing tests.

To your point, I do not believe there is an IPInterface module. I'm not sure a seperate one is needed, but we would want to design something that would check if DHCP is being used or not.

[lorengordon]

Agree on all points. I'll work up a pull request sometime this week (got a few short term deadlines with work that I need to focus on first). Is the test suite public? I like to execute tests locally before pushing, if I can. Also, is there a DSC module you'd recommend as an example for The Right Way to structure the functions? I'll first get it working just with the modified if structure, but then I can look at refactoring the functions.

While I slept on it, I realized with the proposed if structure, I can easily test whether DHCP is enabled to trigger the Set function. May have to modify the $apply code somewhat to make sure it disables DHCP when nothing else changes, but I should be able to work that in.

Really not sure how we might enable DHCP with the current parameter-set, though, so I'll leave that for a future request.

[lorengordon]

Found the Contributing document and the Tests directory. I'll refrain from further stupid questions until I read it and study the Tests.

[tysonjhayes]

@lorengordon no stupid questions only stupid, possibly sarcastic answers. 😆

As for a "good" module, I did just finish fixing up MSFT_xWebVirtualDirectory so it looks more like I expect a good resource to look like.

Which is to say.

• Get/Set/Test-TargetResources each do their own chunk of work.
• Set shouldn't call test again in order to see if work needs to be done, if Test fails Set should just configure the resource.
• Verb-Noun functions
• Good test coverage and tests call xDscResourceDesigner to validate the schema and resource.

I'm of course a bit biased because I wrote the thing, so keep that in mind. 😀

Also don't worry about how long it takes you, most open source is written in people's free time so there isn't much expectation of things being done in a specific amount of time around here.

[lorengordon]

@tysonjhayes thanks for the pointers. I just submitted PR #11. I didn't refactor Set and Test yet. Not really used to the syntax of the pester tests yet, so I did just enough to fix this issue and get the existing tests passing.

I'm rather confused by the function of Get-TargetResource. It doesn't seem to be doing much all that useful -- other than the IP Address, it just returns the parameters passed to it? I would think that it should also be returning the configured SubnetMask and DefaultGateway values. Also, would it be appropriate to call Get-TargetResource from the Test/Set functions, rather than duplicate the functionality?

[tysonjhayes]

Get-TargetResource is garbage in my mind. I totally vote we change it to something better.

Also yes I think it makes sense to reference Get-TargetResource as appropriate.

I'll have some comments on the PR. 👍 Thanks for doing this.