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

All Resources: Handling error in Get-TargetResource #4317

Open
William-Francillette opened this issue Feb 13, 2024 · 7 comments
Open

All Resources: Handling error in Get-TargetResource #4317

William-Francillette opened this issue Feb 13, 2024 · 7 comments
Labels
Enhancement New feature or request

Comments

@William-Francillette
Copy link
Contributor

Description of the issue

Since we removed identity as a key, it is now possible to create a resource without the key id.
When we retrieve a resource using the displayname path, the engine could retrieve multiple policies with the same name.
We thought raising an error using throw when this case occur would resolve the issue however it doesn't: the current implementation will trigger the catch statement which returns $nullResult with effect to create a new policy from Set-TargetResource every time the dsc engine runs.

I tried to exit from the catch however it process continue to Test-TargetResource and throw an error because it doesn't return true of false
image
I thought of using a $errorResult but this would potentially imply to modify all resources....

What would be the best way to handle this case and ensure the dsc engine stop processing this resource when it contains a fatal error? @NikCharlebois @ykuijs @andikrueger

Microsoft 365 DSC Version

1.24.131.2

Which workloads are affected

other

The DSC configuration

No response

Verbose logs showing the problem

No response

Environment Information + PowerShell Version

No response

@William-Francillette
Copy link
Contributor Author

William-Francillette commented Feb 13, 2024

Quick search and found this
https://learn.microsoft.com/en-us/powershell/scripting/learn/deep-dives/everything-about-exceptions?view=powershell-7.4#re-throwing-an-exception

instead of returning $nullResult in the catch we should re-throw the exception this way

    catch
    {
        if ($_.Exception -like '*401*' -or $_.ErrorDetails.Message -like "*`"ErrorCode`":`"Forbidden`"*" -or `
            $_.Exception -like "*Unable to perform redirect as Location Header is not set in response*")
        {
            if (Assert-M365DSCIsNonInteractiveShell)
            {
                Write-Host "`r`n    $($Global:M365DSCEmojiYellowCircle) The current tenant is not registered for Intune."
            }
        }
        else
        {
            New-M365DSCLogEntry -Message 'Error retrieving data:' `
                -Exception $_ `
                -Source $($MyInvocation.MyCommand.Source) `
                -TenantId $TenantId `
                -Credential $Credential
        }

        throw
    }

image

but this will need to apply to all resources

@ykuijs
Copy link
Member

ykuijs commented Feb 16, 2024

I have looked into this issue. It indeed doesn't work as it is supposed to work. Just like we check for null, we should also check for more than one result. The issue with the catch block is an interesting one. The code now simply continues, fails and then hits the catch block.

One of the issues we ran into in the past is that exceptions were thrown in the Get method, which resulted in exports failing because of those exceptions. That is why we now have these nullResults build in.

Would it be an idea to:

  • Update the Get method to return the nullResult when more than one policy is detected.
  • Update the Set method to throw an exception when two policies are detected
  • Maybe we can change the DisplayName property into something like "Multiple" to make sure the Set method can detect that there are multiple policies found.

What do you think of this idea?

@William-Francillette
Copy link
Contributor Author

William-Francillette commented Feb 19, 2024

I have implemented a different solution using $nullResult but clearing all authentication parameter ie assigning $null to each of them

    catch
    {
        New-M365DSCLogEntry -Message 'Error retrieving data:' `
            -Exception $_ `
            -Source $($MyInvocation.MyCommand.Source) `
            -TenantId $TenantId `
            -Credential $Credential

        $nullResult = Clear-M365DSCAuthenticationParameter -BoundParameters $nullResult
        return $nullResult
    }

During Test-TargetResource if an error, such as duplicated policy, is found the resource will be skipped raising an error

    $CurrentValues = Get-TargetResource @PSBoundParameters
    if (-not (Test-M365DSCAuthenticationParameter -BoundParameters $CurrentValues))
    {
        Write-Verbose "An error occured in Get-TargetResource, the policy {$displayName} will not be processed"
        throw "An error occured in Get-TargetResource, the policy {$displayName} will not be processed. Refer to the event viewer logs for more information."
    }

This method does not stop the dsc engine to process the next resource from the mof file

Are you ok with skipping faulty resource from a mof file or should we raise a fatal error and stop the processing because it contains an error?

for your last point @ykuijs I'm not really fan of modifying displayname to an array as it would modify the parameter definition. I think using $nullResult will have a lesser impact

@William-Francillette
Copy link
Contributor Author

Implemented the solution in IntuneappConfigurationPolicy in #4323
Let me know if that works for you
will update the rest of this PR resources if you validate that change

@andikrueger andikrueger added the Enhancement New feature or request label Feb 23, 2024
@ykuijs
Copy link
Member

ykuijs commented Feb 26, 2024

Will have a look at the mentioned PR.

About the modification of the displayname: I didn't mean returning an array with all of the names, but the actual string "Multiple" (or something similar, to prevent issues if someone named their policy Multiple 😉).

@ykuijs
Copy link
Member

ykuijs commented Feb 26, 2024

Implemented the solution in IntuneappConfigurationPolicy in #4323 Let me know if that works for you will update the rest of this PR resources if you validate that change

This solution will work, but we also need to update the Export method to make sure the export will also work correctly.

@William-Francillette
Copy link
Contributor Author

Updated the export with raising an exception when Get-TargetResource returns a policy with no authentication parameter indicating an error

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

No branches or pull requests

3 participants