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

global-settings.jsonc desiredState.excludeScopes broken for resource subscription and resourceGroup scopes #835

Open
audhage opened this issue Dec 5, 2024 · 5 comments
Assignees
Labels
bug Something isn't working

Comments

@audhage
Copy link

audhage commented Dec 5, 2024

Describe the bug
In 10.7.4 I am unable to add exclusion scopes starting with /subscriptions in the excludedScopes property in global-settings.jsonc desiredState.

As far as I can see from the new logic in internal/functions/Get-GlobalSettings.ps1, global-settings.json.desiredState.excludeSubscriptions boolean value must be set to true unless the scope starts with /providers/Microsoft.Management/managementGroups/.

I assume the excludeSubscriptions setting is a setting to exclude all subscriptions, which might not always be the desired behaviour.

This worked fine in 10.4.4.

To Reproduce
Scopes that fail:

"/subscriptions/*"
"/subscriptions/*/resourceGroups/rgName"
"/subscriptions/*/resourceGroups/*"

Expected behavior
That all the above examples would work fine, without the need for the desiredState.excludeSubscriptions setting.

EPAC Version
10.7.4

@audhage audhage added the bug Something isn't working label Dec 5, 2024
@anwather
Copy link
Collaborator

anwather commented Dec 5, 2024

I think it is a logic error - can you test by changing the -and in the line

if ($excludedScope.StartsWith("/subscriptions/") -and $desired.excludeSubscriptions -eq $false) {

to

if ($excludedScope.StartsWith("/subscriptions/") -or $desired.excludeSubscriptions -eq $false) {

@anwather anwather self-assigned this Dec 5, 2024
@audhage
Copy link
Author

audhage commented Dec 8, 2024

I think it is a logic error - can you test by changing the -and in the line

if ($excludedScope.StartsWith("/subscriptions/") -and $desired.excludeSubscriptions -eq $false) {

to

if ($excludedScope.StartsWith("/subscriptions/") -or $desired.excludeSubscriptions -eq $false) {

When testing, the suggested change breaks the logic for ManagementGroups.

In addition, it does not handle scopes ending with a '/' correctly (I assume).
Modified the logic as below, and that seems to work as at least I expect it to.
I have changed the Add-ErrorMessage commands to Write-Host for testing purposes:

$desired = @{excludeSubscriptions = $false }
$excludedScopes = @(
    "/subscrips/"
    "/subscriptions/"
    "/subscriptions/*"
    "/subscriptions/*/resourceGroups/rgName"
    "/subscriptions/*/resourceGroups/*"
    "/providers/Microsoft.Management/managementGroups/mgName"
)

$excludedScopesList = [System.Collections.ArrayList]::new()
$globalExcludedScopesResourceGroupsList = [System.Collections.ArrayList]::new()
$globalExcludedScopesSubscriptionsList = [System.Collections.ArrayList]::new()
$globalExcludedScopesManagementGroupsList = [System.Collections.ArrayList]::new()

if ($null -ne $excludedScopes) {
    if ($excludedScopes -isnot [array]) {
        Write-Host "Global settings error: pacEnvironment $pacSelector field desiredState.excludedScopes must be an array of strings."
    }
    foreach ($excludedScope in $excludedScopes) {
        if ($null -ne $excludedScope -and $excludedScope -is [string] -and $excludedScope -ne "") {
            if ($excludedScope.Contains("/resourceGroupPatterns/", [System.StringComparison]::OrdinalIgnoreCase)) {
                Write-Host "Global settings error: pacEnvironment $pacSelector field desiredState.excludedScopes ($excludedScope) must not contain deprecated /resourceGroupPatterns/.`n`r`t`tReplace it with excludedScopes pattern `"/subscriptions/*/resourceGroups/<pattern>`""
            }
            elseif ($excludedScope.EndsWith("/")) {
                Write-Host "Global settings error: pacEnvironment $pacSelector field desiredState.excludedScopes ($excludedScope) must be a valid scope. It must not end with a '/'."
            }
            else {
                $null = $excludedScopesList.Add($excludedScope)
                if ($excludedScope.StartsWith("/subscriptions/")) {
                    if ($desired.excludeSubscriptions -eq $false) {
                        if ($excludedScope.Contains("/resourceGroups/", [System.StringComparison]::OrdinalIgnoreCase)) {
                            $null = $globalExcludedScopesResourceGroupsList.Add($excludedScope)
                        }
                        else {
                            $null = $globalExcludedScopesSubscriptionsList.Add($excludedScope)
                        }
                    }
                }
                elseif ($excludedScope.StartsWith("/providers/Microsoft.Management/managementGroups/")) {
                    $null = $globalExcludedScopesManagementGroupsList.Add($excludedScope)
                }
                else {
                    Write-Host "Global settings error: pacEnvironment $pacSelector field desiredState.excludedScopes ($excludedScope) must be a valid scope."
                }
            }
        }
    }
}

Write-Host "Reporting:"
Write-Host "---"
Write-Host "excludedScopesList:"
$excludedScopesList
Write-Host "---"
Write-Host "globalExcludedScopesResourceGroupsList:"
$globalExcludedScopesResourceGroupsList
Write-Host "---"
Write-Host "globalExcludedScopesSubscriptionsList:"
$globalExcludedScopesSubscriptionsList
Write-Host "---"
Write-Host "globalExcludedScopesManagementGroupsList:"
$globalExcludedScopesManagementGroupsList

@anwather
Copy link
Collaborator

anwather commented Dec 9, 2024

Thanks - I'll do some testing - we don't support /subscriptions/* as a wildcard for all subscriptions - that's why I had to put in the excludeSubscriptions field - but the ones below should have worked

"/subscriptions/*/resourceGroups/rgName"
    "/subscriptions/*/resourceGroups/*"
    "/providers/Microsoft.Management/managementGroups/mgName"

@audhage
Copy link
Author

audhage commented Dec 10, 2024

Thanks - I'll do some testing - we don't support /subscriptions/* as a wildcard for all subscriptions - that's why I had to put in the excludeSubscriptions field - but the ones below should have worked

"/subscriptions/*/resourceGroups/rgName"
    "/subscriptions/*/resourceGroups/*"
    "/providers/Microsoft.Management/managementGroups/mgName"

I see. Then it should probably be reflected in the desired state docs.
I think it's a pretty common scenario (and one that adheres to the Enterprise Scale responsibility model) to have the EPAC maintain all MG level policies, and let subscription owners manage everything on the subscription level.

@anwather
Copy link
Collaborator

Yes so in the case of not wanting to manage subscriptions we have the excludeSubscriptions switch.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

No branches or pull requests

2 participants