Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Resources using Test-M365DSCParameterState do not support default Ensure value when using -DesiredValues $PSBoundParameters #5085

Closed
Borgquite opened this issue Sep 23, 2024 · 8 comments · Fixed by #5449

Comments

@Borgquite
Copy link
Contributor

Borgquite commented Sep 23, 2024

Description of the issue

When trying to use EXODistributionGroup to create a resource without specifying 'Ensure', Test-TargetResource always returns True even if the resource doesn't exist

It looks like this is because Test-M365DSCParameterState is called with -DesiredValues $PSBoundParameters, which of course does not include parameters which have the 'default value' - therefore the value is not tested:

This seems to affect many resources using this template style (e.g. ADGroup)

Not sure the best way to fix. Perhaps $PSBoundParameters could be modified to always add Ensure = $Ensure?

Microsoft 365 DSC Version

dev

Which workloads are affected

Exchange Online

The DSC configuration

Configuration Example
{
    param(
        [Parameter()]
        [System.String]
        $ApplicationId,

        [Parameter()]
        [System.String]
        $TenantId,

        [Parameter()]
        [System.String]
        $CertificateThumbprint
    )
    Import-DscResource -ModuleName Microsoft365DSC

    node localhost
    {
        EXODistributionGroup 'Distribution Group Name'
        {
            Alias                              = "DistributionGroupName";
            Identity                           = "Distribution Group Name";
            Name                               = "Distribution Group Name";
            PrimarySmtpAddress                 = "[email protected]";
            Type                  = 'Security'
            ApplicationId         = $ApplicationId
            TenantId              = $TenantId
            CertificateThumbprint = $CertificateThumbprint
        }
    }
}

Verbose logs showing the problem

VERBOSE: [HOSTNAME]: LCM:  [ Start  Resource ]  [[EXODistributionGroup]Distribution Group Name::[DomainController]HOSTNAME]
VERBOSE: [HOSTNAME]: LCM:  [ Start  Test     ]  [[EXODistributionGroup]Distribution Group Name::[DomainController]HOSTNAME]
VERBOSE: [HOSTNAME]:                            [[EXODistributionGroup]Distribution Group Name::[DomainController]HOSTNAME] Testing Distribution Group configuration for {Distribution Group Name}
VERBOSE: [HOSTNAME]:                            [[EXODistributionGroup]Distribution Group Name::[DomainController]HOSTNAME] Getting configuration of Distribution Group for Distribution Group Name
VERBOSE: [HOSTNAME]:                            [[EXODistributionGroup]Distribution Group Name::[DomainController]HOSTNAME] Current Values: Alias=DistributionGroupName
ApplicationId=***
CertificateThumbprint=***
Ensure=Absent
Identity=Distribution Group Name
Name=Distribution Group Name
PrimarySmtpAddress=DistributionGroupName@contoso.onmicrosoft.com
TenantId=***
Type=Security
Verbose=True
VERBOSE: [HOSTNAME]:                            [[EXODistributionGroup]Distribution Group Name::[DomainController]HOSTNAME] Target Values: Alias=DistributionGroupName
ApplicationId=***
CertificateThumbprint=***
Identity=Distribution Group Name
Name=Distribution Group Name
PrimarySmtpAddress=DistributionGroupName@contoso.onmicrosoft.com
TenantId=***
Type=Security
Verbose=True
VERBOSE: [HOSTNAME]:                            [[EXODistributionGroup]Distribution Group Name::[DomainController]HOSTNAME] Test-TargetResource returned True

Environment Information + PowerShell Version

OsName               : Microsoft Windows Server 2019 Standard
OsOperatingSystemSKU : StandardServerEdition
OsArchitecture       : 64-bit
WindowsVersion       : 1809
WindowsBuildLabEx    : 17763.1.amd64fre.rs5_release.180914-1434
OsLanguage           : en-US
OsMuiLanguages       : {en-US}

Key   : PSVersion
Value : 5.1.17763.6189
Name  : PSVersion

Key   : PSEdition
Value : Desktop
Name  : PSEdition

Key   : PSCompatibleVersions
Value : {1.0, 2.0, 3.0, 4.0...}
Name  : PSCompatibleVersions

Key   : BuildVersion
Value : 10.0.17763.6189
Name  : BuildVersion

Key   : CLRVersion
Value : 4.0.30319.42000
Name  : CLRVersion

Key   : WSManStackVersion
Value : 3.0
Name  : WSManStackVersion

Key   : PSRemotingProtocolVersion
Value : 2.3
Name  : PSRemotingProtocolVersion

Key   : SerializationVersion
Value : 1.1.0.1
Name  : SerializationVersion
@FabienTschanz
Copy link
Contributor

Updating the $PSBoundParameters value in all resources would be quite the amount of work to put in. In other resources, we have the following check that exits early if the Ensure property is different:

if ($CurrentValues.Ensure -ne $Ensure)
{
    Write-Verbose -Message "Test-TargetResource returned $false"
    return $false
}

Either we add the above check to the resources that don't have it, or we add the Ensure check with its default value Present to the Test-M365DSCParameterState function.

@NikCharlebois What do you think is the better approach? Personally, I would go for the second one, far simpler, much easier to achieve and affects all resources at once.

@Borgquite
Copy link
Contributor Author

@NikCharlebois Any thoughts on this?

@FabienTschanz
Copy link
Contributor

@NikCharlebois Checking in, what do you think of this?

@Borgquite
Copy link
Contributor Author

@FabienTschanz I'd put in a PR for option 2 and see what happens :)

@FabienTschanz
Copy link
Contributor

@Borgquite Agree. Should I add the implementation and open the PR or do you want to do it?

@Borgquite
Copy link
Contributor Author

@FabienTschanz If don't mind doing it, that would be really helpful :)

@FabienTschanz
Copy link
Contributor

Alright, I'll take a shot at it. Will report back once I have the PR ready.

@ykuijs
Copy link
Member

ykuijs commented Nov 22, 2024

@Borgquite Could you please test the solution suggested by @FabienTschanz in PR #5449? If it works, I will merge the PR.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants