Compare-DscParameterState: Reverse check fails in a certain circumstance

[johlju]

Details of the scenario you tried and the problem that is occurring

Details about the problem can be found in the issue dsccommunity/DnsServerDsc#121.

Steps to reproduce the problem

This does not return the expected output.

 $nameServers = [Microsoft.Management.Infrastructure.CimInstance[]] @(
    New-CimInstance -ClassName 'MSFT_KeyValuePair' -Namespace 'root/microsoft/Windows/DesiredStateConfiguration' -Property @{
        Key   = 'B.ROOT-SERVERS.NET.'
        Value = '199.9.14.201'
    } -ClientOnly

    New-CimInstance -ClassName 'MSFT_KeyValuePair' -Namespace 'root/microsoft/Windows/DesiredStateConfiguration' -Property @{
        Key   = 'M.ROOT-SERVERS.NET.'
        Value = '202.12.27.33'
    } -ClientOnly
 )

Compare-DscParameterState -CurrentValues @{
    IsSingleInstance = 'Yes'
    NameServer = @()
} -DesiredValues @{
    NameServers = $nameServers
    IsSingleInstance = 'Yes'
    Verbose = $true
} -TurnOffTypeChecking -ReverseCheck

Expected behavior

Should return an hashtable with the expected metdata,

Current behavior

Returns $null.

Suggested solution to the issue

The problem is the parameter ReverseCheck that assignes $null for the variable $returnValue which then is returned by the command.

After 3 hours of debugging I was no closer to solution.

The operating system the target node is running

Windows 10

Version and build of PowerShell the target node is running

Name                           Value
----                           -----
PSVersion                      7.1.2
PSEdition                      Core
GitCommitId                    7.1.2
OS                             Microsoft Windows 10.0.19042
Platform                       Win32NT
PSCompatibleVersions           {1.0, 2.0, 3.0, 4.0…}
PSRemotingProtocolVersion      2.3
SerializationVersion           1.1.0.1
WSManStackVersion              3.0

Version of the module that was used

0.10.1

[NicolasBn]

I found a clue.

Compare-DscParameterState is based on recursive calls. When you have subobject like $nameservers, Test-DscParameterState is used to test the differences. And Test-DscParameterState call Compare-DscParameterState.

To establish the difference between DesiredValue and CurrentValue, Compare-DscParameterState based the $keyList with the content of $DesiredValue. That seems to be ok, because a configuration can't have an array or an hahtable without value (prehaps, I believe).

But when you use ReverseCheck, the CurrentValues becomes DesireValues, and if DesiredValues doesn't have any value, Compare-DscParameterState dont use the foreach, and return null value.

It's like than you write :

Compare-DscParameterState -CurrentValue $nameServers -DesiredValues [Microsoft.Management.Infrastructure.CimInstance[]]@() -TurnOffTypeChecking

In Test-DscParameterState, the default value is $true. We might add -IncludeInDesiredState parameter, and another test if the result of Compare-DscParameterState is null.

[johlju]

That seems to be ok, because a configuration can't have an array or an hahtable without value (prehaps, I believe).

A resource could pass an empty array, for example for a property Member where the desired state is no members it should be possible to pass Member = @() for the resource in the configuration. In regards to hashtable, I'm not sure in what scenario a hashtable can be passed, so there you are probably correct.

I think a good start is to build tests that should pass but fail with the current implementation so we make sure to support all (known) scenarios.

[johlju]

When testing your example (to remember what actually was not happening) I saw that there seem to be and issue with types (as well?). 🤔

This does not return anything (using just an array without specifying the type):

Compare-DscParameterState -CurrentValues @{
    IsSingleInstance = 'Yes'
    NameServer = @()
} -DesiredValues @{
    NameServers = [Microsoft.Management.Infrastructure.CimInstance[]] @(
        New-CimInstance -ClassName 'MSFT_KeyValuePair' -Namespace 'root/microsoft/Windows/DesiredStateConfiguration' -Property @{
            Key   = 'B.ROOT-SERVERS.NET.'
            Value = '199.9.14.201'
        } -ClientOnly
    )
    IsSingleInstance = 'Yes'
    Verbose = $true
} -TurnOffTypeChecking -ReverseCheck

When specifying the type (same as in DesiredState) it actually returns something, not sure it is correct though, since actual type is returned as {Name}

Compare-DscParameterState -CurrentValues @{
    IsSingleInstance = 'Yes'
    NameServer = [Microsoft.Management.Infrastructure.CimInstance[]]@()
} -DesiredValues @{
    NameServers = [Microsoft.Management.Infrastructure.CimInstance[]] @(
        New-CimInstance -ClassName 'MSFT_KeyValuePair' -Namespace 'root/microsoft/Windows/DesiredStateConfiguration' -Property @{
            Key   = 'B.ROOT-SERVERS.NET.'
            Value = '199.9.14.201'
        } -ClientOnly
    )
    IsSingleInstance = 'Yes'
    Verbose = $true
} -TurnOffTypeChecking -ReverseCheck

# $a contains the result from the second call above.
C:\source\DscResource.Common [main ≡]> $a

Name                           Value
----                           -----
Property                       NameServer
InDesiredState                 False
ExpectedType                   System.Collections.Hashtable
ActualType                     {Name}

C:\source\DscResource.Common [main ≡]> $a.ActualType

Name                           Value
----                           -----
Name                           Unknown

[NicolasBn]

A resource could pass an empty array, for example for a property Member where the desired state is no members it should be possible to pass Member = @() for the resource in the configuration. In regards to hashtable, I'm not sure in what scenario a hashtable can be passed, so there you are probably correct.

Agree ! A member can be empty or null, but not the object passed to -CurrentValues ou DesiredValues

To resolve and test this, I changed the line 510 of Compare-DscParameterState :

            if ($InDesiredStateTable.InDesiredState)
            {
                if ($desiredValue.Keys.Count -eq 0 -and $currentValue.Keys.Count -ne 0)
                {
                    Write-Verbose -Message ($script:localizedData.NoMatchKeyMessage -f $desiredType.FullName, $key, $($currentValue.Keys -join ', '))
                    $InDesiredStateTable.InDesiredState = $false
                }
                else{
                    $InDesiredStateTable.InDesiredState = Test-DscParameterState @param
                }
            }
            else
            {
                $null = Test-DscParameterState @param
            }
            continue

I need to do more tests, but that seems to be good.

I think you have an error in your command. In CurrentState you use NameServer' without 's', and in DesiredValues, you use NameServerS`

When specifying the type (same as in DesiredState) it actually returns something, not sure it is correct though, since actual type is returned as {Name}

Yes because there is a conversion to hashtable in the function. But there is nothing to convert...

[johlju]

I think you have an error in your command. In CurrentState you use NameServer without 's', and in DesiredValues, you use NameServers

Great catch! Correcting that and then running the following does not return the expected resulting hashtable. But the following do return the expected resulting hashtable if adding the your proposed change,

Compare-DscParameterState -CurrentValues @{
    IsSingleInstance = 'Yes'
    NameServers = [Microsoft.Management.Infrastructure.CimInstance[]]@()
} -DesiredValues @{
    NameServers = [Microsoft.Management.Infrastructure.CimInstance[]] @(
        New-CimInstance -ClassName 'MSFT_KeyValuePair' -Namespace 'root/microsoft/Windows/DesiredStateConfiguration' -Property @{
            Key   = 'B.ROOT-SERVERS.NET.'
            Value = '199.9.14.201'
        } -ClientOnly
    )
    IsSingleInstance = 'Yes'
    Verbose = $true
} -TurnOffTypeChecking -ReverseCheck

@NicolasBn Suggest adding the snippet above to a unit test which will fail without your change, then add another test to cover the other lines not covered by the snippet above (the verbose message).

@NicolasBn Awesome work!

[johlju]

Agree ! A member can be empty or null, but not the object passed to -CurrentValues or DesiredValues

Agree. Can't see a scenario where either would ever be null, so think it safe to assume we must always provide a value. This is also already assumed by the current functionality since those two parameters are mandatory (not allowing $null value).

[NicolasBn]

I already did all this, but I found another bugs :

• When you use -ReverseCheck, this value is used in recursive call of Test-DscParameterState and Compare-DscParameterState`, and that called another time the function (not really performant)...
• When you use -Properties and -ReverseCheck, and you have an array in member, that return a wrong value, because the properties are set in recursive calls of -ReverseCheck to test the value of array.
• When you use -ReverseCheck and, in the function Test-DscSompareState/Compare-DscParameterState are recursively called (like to test are compare value of array), ReverseCheck value is removed from $PSBoundParameter. And the ReverseCheckisn't done.

I cill correct all of this in my next PR.

@NicolasBn Awesome work!

Thank you :)

[johlju]

Looking forward to it.

I think Compare-DscParameterState should not recursively call Test-DscParameterState. If it should recursively call something it should recursively call itself (Compare-DscParameterState). Looking at the the snippet you suggested above.

[NicolasBn]

I think Compare-DscParameterState should not recursively call Test-DscParameterState. If it should recursively call something it should recursively call itself (Compare-DscParameterState). Looking at the the snippet you suggested above.

Hum, That depend the need. Here, Test-DscParameterState is used to compare the content of two hashtable member. You just need to know if it the same content (true) or not (false). If you replace by Compare-DscParameterState, you need to rewrite the logic of Test-DscParameterState in Compare-DscParameterState. For me it's not a good way.