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

AADAdministrativeUnit - suggestions for solution of various issues #2786

Closed
salbeck-sit opened this issue Jan 16, 2023 · 8 comments · Fixed by #2794
Closed

AADAdministrativeUnit - suggestions for solution of various issues #2786

salbeck-sit opened this issue Jan 16, 2023 · 8 comments · Fixed by #2794
Labels
Bug Something isn't working Entra ID

Comments

@salbeck-sit
Copy link
Contributor

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

Various issues have been reported from users trying to use the AADAdministrativeUnit resource.
I have myself seen issues with export and updates when testing against a tenant that has an AU with ScopedRoleMembers. Sorry to say, when I built the resource I never tested it 'live' prior to submitting it.
The issue is a nested CimInstance in the schema-class MSFT_MicrosoftGraphScopedRoleMembership
The majority of the code for this resource is provided by DRG but DRG doesn't include sufficient code to fully address nested CimInstances with regards to converting between formats in Test- and Export-TargetResource.

Verbose logs showing the problem

Suggested solution to the issue

There are two solutions - a) A fix could be implemented in DRG to handle nested CimInstances allowing the resource to be 'rebuilt' or b) the Cim Class specifying ScopedRoleMembership could be changed to include the fields that are provided by the nested CimInstance (ie 'flatten the object') and thereby remove the cause for issues.
Solution b) will be a breaking change for anyone that are using the resource. However, I doubt that many do, given the issues it has.
If solution b) is acceptable, I have a fix ready to PR

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

# insert configuration here
Export resource AADAdministrativeUnit from a tenant that has one or more AUs with scoped role membership(s)


#### The operating system the target node is running
OsName               : Microsoft Windows Server 2016 Datacenter
OsOperatingSystemSKU : DatacenterServerEdition
OsArchitecture       : 64-bit
WindowsBuildLabEx    : 14393.5582.amd64fre.rs1_release.221130-1719
OsLanguage           : en-US
OsMuiLanguages       : {en-US}

#### Version of the DSC module that was used ('dev' if using current dev branch)
dev
@Borgquite
Copy link
Contributor

I think I'm the only one who has been reporting the bugs - I'd be quite OK with a breaking change here if it fixes the issue better :)

@andikrueger andikrueger added Bug Something isn't working Entra ID labels Jan 16, 2023
@andikrueger
Copy link
Collaborator

According to our BR change policy we could introduce the BR change in the first version of April 23.

If there is a fix available, that could introduced now, I would go ahead with the fix now, that would not bring any BR.

Could you outline the issue a bit more granular?

@salbeck-sit
Copy link
Contributor Author

The issue is that the parameter ScopedRoleMember contains a nested CimInstance. However, the code that DRG has built doesn't address that, so exporting a resource will fail or return useless info. Same with updates as the Test-TargetResource is also unable to compare parameters in this scenario.
I'm currently testing the proposed solution 'live' after ensuring that tests pass. Some bits are not yet in place. In the process I'll look into updating the tests to be more 'life-like'

@ykuijs
Copy link
Member

ykuijs commented Jan 16, 2023

I agree that Breaking Changed should only be released according to the release schedule. However in this case it looks like the resource never worked anyways, so the breaking change would be to fix something that currently doesn't work. So I would be fine with that.

@NikCharlebois, what do you think?

@salbeck-sit
Copy link
Contributor Author

To be clear, I don't have a solution that is not a BR.
A question is if I (or someone else) should work on that. It seems to me that the problem is with DRG as the code is happy to build a resource that uses nested CimInstances (classes defined in the schema.mof file) but it sidesteps trying to format parameters adequately for the Test- and Export-TargetResource functions. I can certainly understand why, as it has dawned on me that it's both difficult to code the resource and difficult to create a configuration that is easily readable.
I've also tried to build a resource for AADAccessReviews but it has only highlighted the need for carefully reviewing and reconstructing whatever DRG will build for complex resources. So - a lazy suggestion would be to assert that the problem is with the DRG and then sit back and wait. However, I actually want to build live configurations with Admin Units and be able to use the latest M365DSC modules. And I think - assuming usage-stats show 'little enough' usage - that a tiny BR could be in order.

@Borgquite
Copy link
Contributor

Borgquite commented Jan 17, 2023

Can confirm that both Get-TargetResource (#2775) and Set-TargetResource (#2776) both appear to be broken at present - agree with @ykuijs that perhaps it needn't be considered a 'breaking change' in the same way (since it is already broken!)

I am also keen on building live configs with AUs. Could the resource be fixed for now, and the issue with the DRG raised as a separate issue for further consideration?

@andikrueger
Copy link
Collaborator

If it’s already broken, we should go ahead with a fix.

@salbeck-sit
Copy link
Contributor Author

ok, I have tested a solution that both passes tests and seems to actually work. See PR #2794

@ykuijs ykuijs linked a pull request Feb 22, 2023 that will close this issue
ykuijs added a commit that referenced this issue Mar 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Something isn't working Entra ID
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants