microsoft / Microsoft365DSC

Manages, configures, extracts and monitors Microsoft 365 tenant configurations
https://aka.ms/M365DSC
MIT License
1.47k stars 446 forks source link

AADAdministrativeUnit: Cannot create since Id is a required property #3175

Open Borgquite opened 1 year ago

Borgquite commented 1 year ago

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

The 'breaking change' for AADAdministrativeUnit in version 1.23.405.1 really does break things. The 'Id' parameter was made a Key parameter. This means it is no longer possible to create an AADAdministrativeUnit in a configuration.

Verbose logs showing the problem

When creating a resource VSCode says 'Resource 'AADAdministrativeUnit' requires that a value of type 'String' be provided for property 'Id'. This parameter cannot be supplied when creating a resource - it is a GUID read-only parameter auto-generated by Graph on creation. See the Graph API documentation (below).

Suggested solution to the issue

Revert to previous code. The Id parameter should not be a Key. Was this breaking change tested in production?

https://learn.microsoft.com/en-us/graph/api/resources/administrativeunit?view=graph-rest-1.0#properties

The DSC configuration that is used to reproduce the issue (as detailed as possible)

The code in the example file will fail:

Configuration Example
{
    param
    (
        [Parameter(Mandatory = $true)]
        [PSCredential]
        $credsGlobalAdmin
    )

    Import-DscResource -ModuleName Microsoft365DSC

    node localhost
    {
        AADAdministrativeUnit 'TestUnit'
        {
            Id                            = '49a843c7-e80c-4bae-8819-825656a108f2'
            DisplayName                   = 'Test-Unit'
            MembershipRule                = "(user.country -eq `"Canada`")"
            MembershipRuleProcessingState = 'On'
            MembershipType                = 'Dynamic'
            Ensure                        = 'Present'
            Credential                    = $credsGlobalAdmin
        }
    }
}

The operating system the target node is running

OsName : Microsoft Windows 11 Enterprise OsOperatingSystemSKU : EnterpriseEdition OsArchitecture : 64-bit WindowsVersion : 2009 WindowsBuildLabEx : 22621.1.amd64fre.ni_release.220506-1250 OsLanguage : en-GB OsMuiLanguages : {en-GB, en-US}

Version of the DSC module that was used ('dev' if using current dev branch)

1.23.412.1

andikrueger commented 1 year ago

Please have a look at this article: https://microsoft365dsc.com/concepts/key-parameters/

You can provide whatever you want in this case. The Resource should handle the non-existing ID by creating a new object in AAD. After the first run, you need to change the id to the new value.

Borgquite commented 1 year ago

Umm... have you really thought this through?

One of the key things that we do with DSC (and therefore Microsoft365DSC) is to allow for a single configuration to be used with dev/test/live environments. So we want to have one configuration that applies to multiple tenants. If we have to hard-code the ID into every single configuration then you completely break that.

I am also trying to create/manage 20+ administrative units and other objects based on a PowerShell data file - a simplified version:

@{
    Regions = 
    @(
        @{
            RegionName = "Africa"
            Offices = 
            @(
                @{
                    OfficeName = "Chad"
                    OfficeCode = "TCD"
                    Bases = 
                    @(
                        @{
                            BaseName = "N'Djamena"
                            BaseCode = "NDJ"
                        }
                    )
                }
                @{
                    OfficeName = "Guinea"
                    OfficeCode = "GIN"
                    Bases = 
                    @(
                        @{
                            BaseName = "Remote Workers"
                            BaseCode = "VPN"
                        }
                    )
                }
            }
        }
    }
}

This is so that I can loop through regions/offices/bases and create a security group, administrative unit and Exchange Online management role assignment for each and then assign permissions based on this. The power of DSC is that I can do this automatically. I can't afford to be trying to store unique identifiers for each object in each base.

Please remember this tool isn't just being used for 'templating' and 'comparing' existing tenants - it should also support creating infrastructure from scratch (this is what DSC was originally for). Please, please, please reconsider this horrific change. It completely rules out using this module for configuring and subsequently monitoring multiple tenants using a single configuration, as well as using foreach loops to generate resources.

andikrueger commented 1 year ago

Thank you for the good feedback and the use case description.

We discussed the pros and cons of this change for any situation. We even thought about a custom M365DSC key which could be used to uniquely identify the objects and how we could save the later created immutable ids within an object in the tenant. (See #2006). Multi Tenant management is one of the biggest challenges as the ids would change across all tenants.

There are many M365DSC users that “just” export and validate a tenant or create a new one for testing purposes. These users will profit the most from the current change. Exports that were not able to be compiled due to duplicate keys are now working fine…

Just thinking out loud: maybe there is a good reason to implement a global setting for M365DSC. This setting would be a Boolean to enable or disable the new logic. This Boolean would disable the new logic and basically turn it back to the previous state. You could assign the id key with what ever you want and the logic would go for the display names or any other “old” identifier. We would still have better compatibility for those who export and import. The more advanced users could switch back to the heuristic approach and need to make sure to have unique values within the configuration.

Would this work in your situation?

ricmestre commented 1 year ago

@andikrueger Hi, I still didn't test the breaking release but this will definitely be a problem for us. We are creating a solution where we have a blueprint with MS's best practices and that multiple customers may, or may not adopt depending on their requirements and then you need to multiply that by their own several tenants. We need to have some method of being able to go back to old behaviour even if not the default.

Borgquite commented 1 year ago

@andikrueger I could see the Boolean global setting working (so long as the old code base isn't neglected) - although my gut is that it would be better for this to be resource-specific instead of a global setting, so that the configuration remains the single source of truth for how Microsoft365DSC's behaviour (do global settings get stored per-machine? Or somewhere else?)

How about this idea: The Id parameter can take two types of string:

GUIDs without curly braces are used to export / validate tenants and allow for the scenario where configurations need compiling and testing with the same display name

GUIDs with curly braces trigger the 'old' behaviour - where resources can be created without specifying a GUID, and values like 'DisplayName' are used to identify existing resources if necessary. The supplied GUID is randomly generated for each compilation and never actually used. (the 'gotcha' being that it is understood that the person creating the configuration is now solely responsible for making sure that there are and will be no duplicate DisplayNames in the tenant for the resources they want to create/test)

Could also use a different way of telling the GUIDs apart if you want (e.g. round brackets with (New-Guid).ToString('P')) but the main thing is it must be unique, it's clear from a configuration whether it's from Graph or randomly generated, and it triggers the 'alternate key' behaviour whenever the module finds it.

(Interesting to read this StackOverflow user struggling with a similar issue? - 'Craig-MSFT' ends up resorted to using a 'Fake Identifier' in the schema which seems similar to what we're doing here)

I've just tried setting Id = (New-Guid).ToString('B') for an AADAdministrativeUnit resource running inside a foreach() loop and it seems to compile on my machine - any reason why this wouldn't work?

Borgquite commented 1 year ago

I did at first wonder if you could use the Null Guid (00000000-0000-0000-0000-000000000000) or [guid]::Empty to revert to the old behaviour, but I think that would require that DisplayName is made a Key again for those of us who are creating multiple resources with the same type, which you may not wish to do? This might be cleaner though as you wouldn't have to fiddle with braces vs no braces.

andikrueger commented 1 year ago

The change introduced the ID as a key parameter. This key and the type of the resource need to be unique within a configuration. The (display)name of an object got demoted to be a required parameter but will not be considered in the validation of the configuration. Now, we can have unique configurations, but need to modify the ID to match the system generated one in AAD/M365 to be able to further manage the object. In any other case, we would create the resource again and again. To prevent this from happening, all resource have implemented a try-catch-block to first get the object by ID and if this would fail, the get-method should return the first object with the display name or throw an exception of not being able to retrieve a unique object.

This screenshot is taken from AADCOnditionalAccessPolicy.

image

In a well-managed tenant, it should not happen to have objects with the same display name and different settings.

Using a null-GUID could work to revert back to the old behavior without any other settings: BUT, it would only allow one object of each resource. Basically any GUID could be used to manage the objects - if the implementation is consistent across all resources.

ykuijs commented 1 year ago

@Borgquite As Andi mentioned before, we discussed this issue extensively: Evaluating several possible scenarios. The main issue was that somehow various resource were created without a Key parameter or mandatory parameters. This caused some issues we needed to solve. That is why we now added a QA test that checks if each resource has at least one Key parameter.

For the "faulty" resources we needed a solution: Configuring a parameter as key (parameter becomes mandatory) is a breaking change. According to our Breaking Changes policy we can only release breaking changes twice a year and the April deadline was coming up quickly. That is why we had to come up with a first implementation while discussing a more robust and solution.

This first deployment changed all resources that did not have a Key parameter: The Id parameter was configured as Key and the DisplayName as Required. The idea behind this is that we have seen instances where DisplayName wasn't unique and also wanted to cater for the situation where a rename of a component didn't break DSC immediately. It wasn't ideal, but the best we could come up with now.

In the code, each resource first checks if a component exist using the ID and when that is not the case, try to use the DisplayName to retrieve the component. When that also fails, we assume the component doesn't exist and try to create it.

I am sorry to hear that this first implementation did not cater for you scenario. That was not our intention at all.

The Id parameter is a string parameter and you can feed in whatever value you like. I just tested "Dummy", which works as expected. So you could even do something like this:

Configuration Example
{
    param
    (
        [Parameter(Mandatory = $true)]
        [PSCredential]
        $credsGlobalAdmin
    )

    Import-DscResource -ModuleName Microsoft365DSC

    node localhost
    {
        AADAdministrativeUnit 'TestUnit'
        {
            Id                            = $ConfigurationData.NonNodeData.AdminUnit.DisplayName
            DisplayName                   = $ConfigurationData.NonNodeData.AdminUnit.DisplayName
            MembershipRule                = $ConfigurationData.NonNodeData.AdminUnit.MembershipRule
            MembershipRuleProcessingState = $ConfigurationData.NonNodeData.AdminUnit.MembershipRuleProcessingState
            MembershipType                = $ConfigurationData.NonNodeData.AdminUnit.MembershipType
            Ensure                        = $ConfigurationData.NonNodeData.AdminUnit.Ensure
            Credential                    = $credsGlobalAdmin
        }
    }
}

$cd = @{
    AllNodes = @(
        @{
            NodeName = "localhost"
            PsDscAllowPlainTextPassword = $true
            PSDscAllowDomainUser = $true
        }
    )
    NonNodeData = @{
        AdminUnit = @{
            DisplayName                   = 'Test-Unit3'
            MembershipRule                = "(user.country -eq `"Canada`")"
            MembershipRuleProcessingState = 'On'
            MembershipType                = 'Dynamic'
            Ensure                        = 'Present'
        }
    }
}

Might this be a solution for you?

Borgquite commented 1 year ago

@andikrueger Thanks for that explanation.

Borgquite commented 1 year ago

@ykuijs If you're making a promise that you'll retain the 'does the ID fail? If so fall back to Display Name' behaviour for updating existing resources as well as creating new ones, I think that might be OK.

I'd still suggest as mentioned to @andikrueger just now that having two keys (Id and DisplayName) and making the Null GUID the formally accepted way of doing this might be preferable - since it would ensure that the DisplayName was always unique within the configuration when using the Null GUID. But I'll try your suggestion for now :)

Borgquite commented 1 year ago

Yes. https://learn.microsoft.com/en-us/powershell/dsc/concepts/class-based-resources?view=dsc-2.0#class-based-dsc-resource-properties

[DscProperty(Key)] - Indicates that this property uniquely identifies an instance of this DSC Resource. Every DSC Resource must have at least one Key property. If a DSC Resource has more than one Key property, those properties are used together to uniquely identify an instance of the DSC Resource. If any Key properties of a DSC Resource aren't specified when using Invoke-DscResource, the cmdlet raises an error. If any Key properties aren't specified when authoring a DSC Configuration, compiling the configuration raises an error.

So if you make Id and DisplayName 'key' properties, then:

I don't think you'd need to modify any code either (although it would be nice if you caught the Null GUID as a base case before checking for a non-existent resource?

The best of both worlds?

andikrueger commented 1 year ago

It would be possible to have more than one key for any resource.

The current implementation is as close as possible to having two keys. Both values are required, but only one is considered during compilation to make the resource unique. As @ykuijs pointed out, it would be possible to use the display name as ID as well.

At the time this change was introduced, there were several Intune resources that had the ID as key and the display name as required. Having this combination has some advantages over having two keys:

with ID as key and display name required, there is only one unique resource possible. This is needed for real management scenarios. As soon as the id matches the object in M365 the resource is unique. If the display name would be a key as well, there would be minimum of two/three possible combinations:

ID = abc DisplayName=def

ID=abc DisplayName=xyz

Combinations one and two would cause a constant drift. The first resource would configure object with ID=abc to named def, the second one would like to set DisplayName xyz.

Borgquite commented 1 year ago

@andikrueger Understood. Would it be possible to document within the Key Parameters in DSC that it is supported to use DisplayName for the Id value when creating 'infrastructure as code' (i.e. using DSC to create tenants from scratch?), so that people creating dev/test/live or applying a configuration from scratch for multiple tenants know what to do?

(The idea of creating a resource using IaC, then applying it, then getting the IDs, putting those IDs into your code, then applying again undermines most of the expected benefits of IaC, at least to me.)

Also would it be possible to test the Id parameter using a regex before the Get- commands run and also failing over to DisplayName at that point if it's not a valid GUID? This would presumably allow faster performance by removing the unnecessary call, as well as reducing warnings, and I presume it would reduce the load on Microsoft Graph infrastructure as well (otherwise us IaC types using Azure Automation Desired State Configuration who can't afford to use Ids will be running multiple unnecessary Get- cmdlets against Graph every 15 minutes before we fail over).

andikrueger commented 1 year ago

I would go for a new documentation page within the user guide. There we are able to put it into context and add more information.

I like the idea about a 'validation' of the Id. We can easily evaluate a GUID and switch over to the display name instead.

ykuijs commented 1 year ago

@ykuijs If you're making a promise that you'll retain the 'does the ID fail? If so fall back to Display Name' behaviour for updating existing resources as well as creating new ones, I think that might be OK.

I'd still suggest as mentioned to @andikrueger just now that having two keys (Id and DisplayName) and making the Null GUID the formally accepted way of doing this might be preferable - since it would ensure that the DisplayName was always unique within the configuration when using the Null GUID. But I'll try your suggestion for now :)

That is what we are aiming for and what has been implemented in the DRG (Dynamic Resource Generator). But, it might be that we missed some resources that do not adhere to this principle yet.

Note on your comment about two key parameters: Each parameter that is specified as Key is also a mandatory parameter. That means you cannot omit it in favour of the DisplayName parameter. As @AndiKreuger mentions, DisplayName is already required and making it Key introduces some other issues 😉