xADDomainController: Add RODC Creation Support #390
Conversation
Codecov Report
@@ Coverage Diff @@
## dev #390 +/- ##
===================================
- Coverage 92% 92% -1%
===================================
Files 20 20
Lines 2538 2626 +88
Branches 10 10
===================================
+ Hits 2345 2418 +73
- Misses 183 198 +15
Partials 10 10 |
|
@SSvilen Thank you for this PR! Really looking forward getting this through. Before I review this, could you please resolve the tabs -> spaces, and the indentation has been changed too. See the screenshot here from reviewable. Also see the failing test here (failing because of tabs):
|
There was a problem hiding this comment.
Reviewed 1 of 6 files at r1.
Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @SSvilen)
a discussion (no related file):
There are failing tests here, could you please resolve them before I review. https://ci.appveyor.com/project/PowerShell/xactivedirectory/builds/25479763?fullLog=true#L2454
|
@SSvilen Let me know if I can be of any assistance. 🙂 |
johlju
added
waiting for code fix
There was a problem hiding this comment.
Reviewable status: 1 of 6 files reviewed, 1 unresolved discussion (waiting on @johlju)
a discussion (no related file):
Previously, johlju (Johan Ljunggren) wrote…
There are failing tests here, could you please resolve them before I review. https://ci.appveyor.com/project/PowerShell/xactivedirectory/builds/25479763?fullLog=true#L2454
I don't understand why are those test failing. By me locally they are not failing.
That with the psscriptanalyzer I don't understand either.
If you could take a look, would be gread.
|
For the [PSScriptAnalyzer failing tests[(https://ci.appveyor.com/project/PowerShell/xactivedirectory/builds/25481046?fullLog=true#L861) if the parameter need to contain 'Password' then we need to override that specific rule and a justification, see example. https://github.com/PowerShell/xActiveDirectory/blob/727474acdee1f9930b083991d602cb3f5054a769/DSCResources/MSFT_xADDomainTrust/MSFT_xADDomainTrust.psm1#L111 This PR still suggest (wrongly) to change the indentation of a lot of code, can you please go through the code an fix the indentation back? See example of this PR suggest to change the indentation here. I can help take a look at the other failing tests, but it can take a while, I will be traveling for a week. So in the meantime please see if you can resolve them. 🙂 |
|
Hi @SSvilen, to help with the PSScriptAnalyzer |
|
I think we should not use abbreviations in parameter/property names. I rather we override it instead. Or use another word instead of 'Password'. |
There was a problem hiding this comment.
OK that's fixed now.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @johlju)
a discussion (no related file):
Previously, SSvilen (Svilen) wrote…
I don't understand why are those test failing. By me locally they are not failing.
That with the psscriptanalyzer I don't understand either.
If you could take a look, would be gread.
I think I see where the problem is. I need to investigate why Pester behaves differently on my machine.
There was a problem hiding this comment.
Reviewable status: 0 of 6 files reviewed, 1 unresolved discussion (waiting on @johlju)
There was a problem hiding this comment.
I gave this a quick review, but skipped the tests until they are sorted. Just minor things.
Reviewed 1 of 6 files at r1, 2 of 5 files at r2, 1 of 3 files at r3.
Reviewable status: 4 of 6 files reviewed, 25 unresolved discussions (waiting on @johlju and @SSvilen)
a discussion (no related file):
Can we add an example too, that shows how to add a read-only domain controller using the new properties?
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 108 at r3 (raw file):
ForEach-Object sAMAccountName
We should use named parameters | ForEach-Object -MemberName sAMAccountName
We could also have used | Select-Object -ExpandProperty sAMAccountName since we are only returning a property?
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 109 at r3 (raw file):
| ForEach-Object sAMAccountName
Same as previous.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 265 at r3 (raw file):
if(
We should have a space between the if-statement and the parentheses
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 265 at r3 (raw file):
if($PSBoundParameters.ContainsKey('ReadOnlyReplica') -and $ReadOnlyReplica -eq $true)
Please add a new row before this one.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 267 at r3 (raw file):
if(
We should have a space between the if-statement and the parentheses
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 267 at r3 (raw file):
if($PSBoundParameters.ContainsKey('SiteName') -eq $false)
Non-blocking. Informational only. In other parts of the module, this is normally written as if (-not $PSBoundParameters.ContainsKey('SiteName'))
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 271 at r3 (raw file):
$installADDSDomainControllerParameters.Add('ReadOnlyReplica', $true)
Please add a new row before this one.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 274 at r3 (raw file):
if(
We should have a space between the if-statement and the parentheses
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 279 at r3 (raw file):
if(
We should have a space between the if-statement and the parentheses
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 353 at r3 (raw file):
if ($domainControllerObject)
Since the ´Get-DomainControllerObjectwas moved, I think this evaluation and error message in theelse` block should be moved too. 🤔 If the domain controller object is not found we should not do anything here?
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 377 at r3 (raw file):
if(
We should have a space between the if-statement and the parentheses
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 379 at r3 (raw file):
$testMembersParams
Can we avoid abbreviation? $testMembersParameters. Throughout.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 381 at r3 (raw file):
$AllowPasswordReplicationAccountName;
Please remove the semi-colon at the end.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 406 at r3 (raw file):
Remove-ADDomainControllerPasswordReplicationPolicy @removeADPasswordPolicy Add-ADDomainControllerPasswordReplicationPolicy @addADPasswordPolicy
This will remove all existing members even if one or more already is in the desired state, could we instead add a helper function that either return the correct members to remove and add (it can be reused for the next block too), or have the helper function add and remove only the correct members?
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 411 at r3 (raw file):
if(
We should have a space between the if-statement and the parentheses
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 440 at r3 (raw file):
Remove-ADDomainControllerPasswordReplicationPolicy @removeADPasswordPolicy Add-ADDomainControllerPasswordReplicationPolicy @addADPasswordPolicy
See comment in previous block.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 599 at r3 (raw file):
if(
We should have a space between the if-statement and the parentheses
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 600 at r3 (raw file):
$null -ne $existingResource.AllowPasswordReplicationAccountName)
I think we should move this evaluation up to the previous row together with the first evaluation.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 606 at r3 (raw file):
if(-not (Test-Members @testMembersParams))
Please add a new row before this one.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 606 at r3 (raw file):
if(
We should have a space between the if-statement and the parentheses
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 613 at r3 (raw file):
$testTargetResourceReturnValue = $false
Please add a new row before this one.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 617 at r3 (raw file):
} if($PSBoundParameters.ContainsKey('DenyPasswordReplicationAccountName') -and
Same comments for this block of code as the previous block of code.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.schema.mof, line 13 at r3 (raw file):
[Write, Description("The path of the media you want to use install the Domain Controller.")] String InstallationMediaPath; [Write, Description("Specifies if the domain controller will be a Global Catalog (GC).")] Boolean IsGlobalCatalog; [Write, Description("Indicates that the cmdlet installs the domain controller as an RODC for an existing domain.")] Boolean ReadOnlyReplica;
Please update the README.md with these new properties.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.schema.mof, line 13 at r3 (raw file):
RODC
Can we write out these as 'Read-Only Domain Controller (RODC)'? Throughout where only the abbreviation 'RODC' is used.
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 25 unresolved discussions (waiting on @johlju and @SSvilen)
a discussion (no related file):
Previously, johlju (Johan Ljunggren) wrote…
Can we add an example too, that shows how to add a read-only domain controller using the new properties?
I saw that you have added examples to the wiki, the wiki documentation is generated automatically from the schema.mof and from the folder /Examples/Resources/*. Please copy the examples from the wiki to examples files in this PR so they are not removed when I update the documentation for the next release in a day or so. 🙂
|
@SSvilen I saved the example below before the wiki content was regenerated. I saved it in case you needed it to create the example file (see review comment) This configuration will add a read-only domain controller to the domain contoso.com and specify a list of account, whose passwords are allowed/denied for synchronisation. Configuration RODC_Config
{
param
(
[Parameter(Mandatory = $true)]
[ValidateNotNullOrEmpty()]
[System.Management.Automation.PSCredential]
$DomainAdministratorCredential
)
Import-DscResource -ModuleName PSDscResources
Import-DscResource -ModuleName xActiveDirectory
node localhost
{
WindowsFeature 'InstallADDomainServicesFeature' {
Ensure = 'Present'
Name = 'AD-Domain-Services'
}
WindowsFeature 'RSATADPowerShell' {
Ensure = 'Present'
Name = 'RSAT-AD-PowerShell'
DependsOn = '[WindowsFeature]InstallADDomainServicesFeature'
}
xWaitForADDomain 'WaitForestAvailability' {
DomainName = 'contoso.com'
DomainUserCredential = $DomainAdministratorCredential
RetryCount = 10
RetryIntervalSec = 120
DependsOn = '[WindowsFeature]RSATADPowerShell'
}
xADDomainController 'RODC' {
DomainName = 'contoso.com'
DomainAdministratorCredential = $DomainAdministratorCredential
SafemodeAdministratorPassword = $DomainAdministratorCredential
ReadOnlyReplica = $true
SiteName = 'Default-First-Site-Name'
AllowPasswordReplicationAccountName = 'pvdi.test1','pvdi.test'
DenyPasswordReplicationAccountName = 'SVC_PVS','TA2SCVMM'
DependsOn = '[xWaitForADDomain]WaitForestAvailability'
}
}
} |
There was a problem hiding this comment.
Reviewable status: 4 of 6 files reviewed, 25 unresolved discussions (waiting on @johlju)
a discussion (no related file):
Previously, johlju (Johan Ljunggren) wrote…
I saw that you have added examples to the wiki, the wiki documentation is generated automatically from the schema.mof and from the folder
/Examples/Resources/*. Please copy the examples from the wiki to examples files in this PR so they are not removed when I update the documentation for the next release in a day or so. 🙂
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 108 at r3 (raw file):
MemberName
I think foreach is better since I connect -expandproperty more with properties, which are arrays or so.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 109 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
| ForEach-Object sAMAccountNameSame as previous.
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 265 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
if(We should have a space between the if-statement and the parentheses
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 265 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
if($PSBoundParameters.ContainsKey('ReadOnlyReplica') -and $ReadOnlyReplica -eq $true)Please add a new row before this one.
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 267 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
if(We should have a space between the if-statement and the parentheses
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 271 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
$installADDSDomainControllerParameters.Add('ReadOnlyReplica', $true)Please add a new row before this one.
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 274 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
if(We should have a space between the if-statement and the parentheses
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 279 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
if(We should have a space between the if-statement and the parentheses
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 353 at r3 (raw file):
if ($domainControllerObject)
I believe this check is redundant. I can just remove it?
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 377 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
if(We should have a space between the if-statement and the parentheses
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 379 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
$testMembersParamsCan we avoid abbreviation?
$testMembersParameters. Throughout.
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 381 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
$AllowPasswordReplicationAccountName;Please remove the semi-colon at the end.
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 406 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Remove-ADDomainControllerPasswordReplicationPolicy @removeADPasswordPolicy Add-ADDomainControllerPasswordReplicationPolicy @addADPasswordPolicyThis will remove all existing members even if one or more already is in the desired state, could we instead add a helper function that either return the correct members to remove and add (it can be reused for the next block too), or have the helper function add and remove only the correct members?
I modified the code a bit instead of creating a new function. Is it ok so?
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 411 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
if(We should have a space between the if-statement and the parentheses
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 440 at r3 (raw file):
I modified the code a bit instead of creating a new function. Is it ok so?
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 599 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
if(We should have a space between the if-statement and the parentheses
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 600 at r3 (raw file):
evaluation
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 606 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
if(-not (Test-Members @testMembersParams))Please add a new row before this one.
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 606 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
if(We should have a space between the if-statement and the parentheses
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 613 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
$testTargetResourceReturnValue = $falsePlease add a new row before this one.
Done.
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 617 at r3 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Same comments for this block of code as the previous block of code.
I modified the code a bit instead of creating a new function. Is it ok so?
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.schema.mof, line 13 at r3 (raw file):
Read-Only Domain Controller (RODC)
Which README.md should I update? The one in the root folder has links to the wiki, that`s why I updated the wiki page
DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.schema.mof, line 13 at r3 (raw file):
Read-Only Domain Controller (RODC)
Done.
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 29 unresolved discussions (waiting on @johlju)
Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 709 at r3 (raw file):
{ Set-TargetResource @testDefaultParams -DomainName $correctDomainName -AllowPasswordReplicationAccountName $allowedAccount } | Should -Not -Throw Assert-MockCalled -CommandName Remove-ADDomainControllerPasswordReplicationPolicy -ParameterFilter { $AllowedList.SamAccountName -contains 'allowedAccount2' } -Exactly -Times 1 -Scope It
I removed the ParameterFilter because it seems that ADPrincipal objects are not so easy to compare(samaccountname is not getting populated after creation - it's just an 'empty' object). I've got it working using Compare-Objects, but it does not look so nice. I have to go thru all objects in AllowedList array and do a compare for each object. Like that:
$AllowedList.ForEach({if(Compare-Object $PSItem $AllowedAccount -ExcludeDifferent -IncludeEqual) {$true}})
Basically I wanted to prove in the test, that only unneeded/needed accounts are getting removed/added.
What is your suggestion here?
Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 710 at r3 (raw file):
Assert-MockCalled -CommandName Remove-ADDomainControllerPasswordReplicationPolicy -ParameterFilter { $AllowedList.SamAccountName -contains 'allowedAccount2' } -Exactly -Times 1 -Scope It Assert-MockCalled -CommandName Add-ADDomainControllerPasswordReplicationPolicy -ParameterFilter { $AllowedList.SamAccountName -eq $allowedAccount } -Exactly -Times 1 -Scope It
Same as above.
Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 728 at r3 (raw file):
{ Set-TargetResource @testDefaultParams -DomainName $correctDomainName -DenyPasswordReplicationAccountName $deniedAccount } | Should -Not -Throw Assert-MockCalled -CommandName Remove-ADDomainControllerPasswordReplicationPolicy -ParameterFilter { $DeniedList.SamAccountName -contains 'deniedAccount2' } -Exactly -Times 1 -Scope It
Same as above.
Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 729 at r3 (raw file):
Assert-MockCalled -CommandName Remove-ADDomainControllerPasswordReplicationPolicy -ParameterFilter { $DeniedList.SamAccountName -contains 'deniedAccount2' } -Exactly -Times 1 -Scope It Assert-MockCalled -CommandName Add-ADDomainControllerPasswordReplicationPolicy -ParameterFilter { $DeniedList.SamAccountName -eq $deniedAccount } -Exactly -Times 1 -Scope It
Same as above
There was a problem hiding this comment.
Reviewable status: 1 of 7 files reviewed, 29 unresolved discussions (waiting on @johlju)
a discussion (no related file):
Previously, SSvilen (Svilen) wrote…
I think I see where the problem is. I need to investigate why Pester behaves differently on my machine.
I don't understand why 'Returns "False" when "IsGlobalCatalog" does not match' is failing. How do I troubleshoot the AppVeyor behavior? Do I put more verbose logging in my code and do a push?
|
Easiest is to connect your own AppVeyor account (it is free for open source) and add your fork of xActiveDirectory as project in you AppVeyor account. Then you can branch out to another branch (e.g Either way, you can connect to the RDP session by enabling it in the appveyor.yml. Then you can run tests manually in the AppVeyor build worker. See here https://www.appveyor.com/docs/how-to/rdp-to-build-worker/ I wonder if this problem is unrelated to your PR (bug in the tests), but it started to show now when the code changed. Either way, I can't merge until all tests pass. 😕 But I will help you debug this after Tuesday unless you solved it by then (off on a mini-vaction now, just catching up on stuff on the train ride). 😄 |
There was a problem hiding this comment.
I found the problem and it should be fixed now. Should open a new PR with cleaner commit history?
Thanks for the help with AppVeyor - I'll do all builds on my repo in the future, before opening a PR.
I don't know if I caused that, but the my build are currently in status 'queued'.
Reviewable status: 1 of 7 files reviewed, 29 unresolved discussions (waiting on @johlju)
|
I found another problem, that I have to fix, so I'll just close that PR and open a new one with cleaner commit history. This branch history turned into a mess. |
johlju
removed
the
waiting for code fix
Pull Request (PR) description
Adding support for Read-only Domain Controllers creation.
This Pull Request (PR) fixes the following issues
Task list
Entry should say what was changed, and how that affects users (if applicable).
and comment-based help.
This change is