[AddsOrgUnitsAndGroups] Fix quadratic DependsOn producing a MOF larger than 50 MB#249
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
💤 Files with no reviewable changes (1)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (2)
WalkthroughThis PR changes DomainLocal group dependency assignment to use a single managed parent OU when possible, adds a regression fixture and test for that behavior, removes defaults from DscPullServerSql mandatory parameters, and updates the changelog. ChangesDomainLocal DependsOn fix
DscPullServerSql parameter defaults
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Warning Review ran into problems🔥 ProblemsGit: Failed to clone repository. Please run the Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/Unit/DSCResources/DscResources.Tests.ps1 (1)
234-273: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider splitting scenarios into separate
ContextandItblocks.The test mixes two distinct scenarios — verifying each DomainLocal group has exactly one OU dependency, and validating a specific group's dependency ID — in a single
Itblock. As per coding guidelines, each scenario should be a separateContextblock, andContextdescriptions should start with 'When'.♻️ Suggested restructure
Describe 'AddsOrgUnitsAndGroups DependsOn' -Tags FunctionalQuality { + Context 'When verifying DomainLocal group dependencies' { + It 'Should give each DomainLocal group exactly one parent-OU dependency' { - It 'Should give each DomainLocal group a single parent-OU dependency (no quadratic blow-up)' { - # Regression guard for the O(groups x OUs) DependsOn explosion: assigning the - # full OU dependency list to every DomainLocal group inflated the compiled MOF - # so that a large DC exceeded DSC's hard 50 MB ValidateInstanceText limit. Each - # DomainLocal group must depend on exactly one [ADOrganizationalUnit] - its own - # parent OU - not on every OU created by the composite. + # Regression guard for the O(groups x OUs) DependsOn explosion: assigning the + # full OU dependency list to every DomainLocal group inflated the compiled MOF + # so that a large DC exceeded DSC's hard 50 MB ValidateInstanceText limit. Each + # DomainLocal group must depend on exactly one [ADOrganizationalUnit] - its own + # parent OU - not on every OU created by the composite. $mofPath = Join-Path -Path $OutputDirectory -ChildPath 'localhost_AddsOrgUnitsAndGroups.mof' Test-Path -Path $mofPath | Should -BeTrue -Because 'the AddsOrgUnitsAndGroups composite must have compiled' @@ -264,8 +267,12 @@ $ouRefCount | Should -Be 1 -Because "DomainLocal group '$groupName' must depend only on its own parent OU, not the whole OU list" } + } + } - # The single dependency must be the group's own parent OU, using the same - # -replace '\W' normalisation the composite applies to OU resource IDs. + Context 'When verifying specific group dependency ID' { + It 'Should match the expected parent OU for App_123_Read' { + # The single dependency must be the group's own parent OU, using the same + # -replace '\W' normalisation the composite applies to OU resource IDs. $readGroupBlock = $domainLocalBlocks | Where-Object { $_ -match 'GroupName\s*=\s*"App_123_Read"' } $readGroupBlock | Should -Match '\[ADOrganizationalUnit\]OUGroupsOUAdminDCcontosoDCcom' + } + } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/Unit/DSCResources/DscResources.Tests.ps1` around lines 234 - 273, Split the current AddsOrgUnitsAndGroups DependsOn test in DscResources.Tests.ps1 into two separate scenarios, using distinct Context blocks whose descriptions start with “When”. Keep the existing DomainLocal dependency-count check in one It block, and move the specific App_123_Read dependency-ID assertion into its own It block under a separate Context. Use the existing test identifiers like AddsOrgUnitsAndGroups DependsOn, domainLocalBlocks, and readGroupBlock to locate the split cleanly.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/Unit/DSCResources/DscResources.Tests.ps1`:
- Line 236: The Pester test case description in the `It` block should follow the
naming guideline and start with “Should”. Update the `It` description in
`DscResources.Tests.ps1` for the `gives each DomainLocal group a single
parent-OU dependency (no quadratic blow-up)` test so it begins with “Should”,
keeping the rest of the intent unchanged.
---
Nitpick comments:
In `@tests/Unit/DSCResources/DscResources.Tests.ps1`:
- Around line 234-273: Split the current AddsOrgUnitsAndGroups DependsOn test in
DscResources.Tests.ps1 into two separate scenarios, using distinct Context
blocks whose descriptions start with “When”. Keep the existing DomainLocal
dependency-count check in one It block, and move the specific App_123_Read
dependency-ID assertion into its own It block under a separate Context. Use the
existing test identifiers like AddsOrgUnitsAndGroups DependsOn,
domainLocalBlocks, and readGroupBlock to locate the split cleanly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 60d04859-7f86-448a-b7d4-db21771c43ca
📒 Files selected for processing (4)
CHANGELOG.mdsource/DSCResources/AddsOrgUnitsAndGroups/AddsOrgUnitsAndGroups.schema.psm1tests/Unit/DSCResources/Assets/Config/AddsOrgUnitsAndGroups.ymltests/Unit/DSCResources/DscResources.Tests.ps1
dan-hughes
left a comment
dan-hughes
left a comment
There was a problem hiding this comment.
@dan-hughes reviewed 4 files and all commit messages, and made 3 comments.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on raandree).
tests/Unit/DSCResources/DscResources.Tests.ps1 line 236 at r2 (raw file):
Describe 'AddsOrgUnitsAndGroups DependsOn' -Tags FunctionalQuality { It 'Should give each DomainLocal group a single parent-OU dependency (no quadratic blow-up)' {
This whole test doesn't really separate setup code that should be in a BeforeDiscovery, BeforeAll/BeforeEach and the code that should be in It.
The foreach should be in it's own It block and use the Pester code generation features.
tests/Unit/DSCResources/DscResources.Tests.ps1 line 252 at r2 (raw file):
# nested '};' that terminates a DependsOn list. $adGroupBlocks = ($mofContent -split 'instance of ') | Where-Object { $_ -match '^MSFT_ADGroup\b' }
If the ADGroup resource prefix is ever changed/updated to match new conventions, this will break.
source/DSCResources/AddsOrgUnitsAndGroups/AddsOrgUnitsAndGroups.schema.psm1 line 96 at r2 (raw file):
} $dependencies = @()
Not changed in this PR but assigning $dependencies from the foreach will give a performance increase.
Suggestion:
$dependencies = foreach ($group in $Groups)
{
# remove case sensitivity from hashtables
$group = @{} + $group
if ($group.GroupScope -eq 'DomainLocal')
{
"[ADGroup]'$($group.GroupName)'"… comply with PS Script Analyzer rules. This fixes the build.
dan-hughes
left a comment
dan-hughes
left a comment
There was a problem hiding this comment.
@dan-hughes reviewed 4 files and all commit messages, made 1 comment, and resolved 3 discussions.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on raandree).
|
Thanks, @dan-hughes |
2 participants
Pull Request (PR) description
Fixes #248 .
AddsOrgUnitsAndGroupsassigned the entire OU dependency list ($ouDependencies)as the
DependsOnof everyDomainLocalgroup:Root cause. That produces O(groups x OUs) dependency edges. When the composite
is expanded into a root configuration, DSC fully-qualifies every
ResourceID/DependsOnreference (e.g.[ADOrganizationalUnit]OUGroupsOUAdminDCcontosoDCcom::[AddsOrgUnitsAndGroups]AddsOrgUnitsAndGroups),multiplying each edge's serialized size. On a real DC (~204 OUs x ~167
DomainLocalgroups, ~34,000 edges) the node MOF reached 52.5 MB and failed to compile with the hard
50 MB (
52428800-byte)ValidateInstanceTextlimit, which is not tunable viaMaxEnvelopeSizeKb.Fix. A group only needs its own parent OU to exist first — OUs already form a correct
creation chain via their own
DependsOn. The parent OU's resource ID is reconstructedfrom the group's
Pathusing the same-replace '\W'normalisation already applied toOU IDs, so a managed parent matches exactly; it falls back to the full list only when the
parent OU is not managed by the composite:
Measured (real DC). Composite MOF 8.91 MB -> 0.49 MB; node MOF 52.5 MB -> 17.8 MB;
build green.
A regression test compiles the composite with two
DomainLocalgroups in different OUsand asserts each gets exactly one
[ADOrganizationalUnit]dependency (its own parent),so the quadratic blow-up cannot silently return.
Fixed
DomainLocalgroup now depends only on its own parent OUinstead of the entire OU list. Assigning every OU as
DependsOnto everyDomainLocalgroup produced an O(groups x OUs) dependency explosion that, once the composite is
expanded into a root configuration, could push a large domain controller's MOF past
DSC's hard 50 MB
ValidateInstanceTextlimit (serializedBufferLength must be in the range of 4 and 52428800). The parent OU is sufficient for correct apply ordering;falls back to the full list when the parent OU is not managed by the composite.
Task list
build.ps1 -ResolveDependency).This change is