xADDomainController: Add RODC Creation Support #406

SSvilen · 2019-07-01T08:52:32Z

Pull Request (PR) description

Adding support for Read-only Domain Controllers creation.

This Pull Request (PR) fixes the following issues

Fixes Add RODC Creation Support #40
Replaces PR xADDomainController: Add RODC Creation Support #390

Task list

Added an entry under the Unreleased section in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in resource directory README.md.
Resource parameter descriptions added/updated in schema.mof
and comment-based help.
Comment-based help added/updated.
Localization strings added/updated in all localization files as appropriate.
Examples appropriately added/updated.
Unit tests added/updated. See DSC Resource Testing Guidelines.
Integration tests added/updated (where possible). See DSC Resource Testing Guidelines.
New/changed code adheres to DSC Resource Style Guidelines and Best Practices.

This change is

codecov-io · 2019-07-01T09:05:59Z

Codecov Report

Merging #406 into dev will increase coverage by <1%.
The diff coverage is 100%.

@@         Coverage Diff          @@
##            dev   #406    +/-   ##
====================================
+ Coverage    96%    96%   +<1%     
====================================
  Files        20     20            
  Lines      2599   2705   +106     
  Branches     10     10            
====================================
+ Hits       2502   2608   +106     
  Misses       87     87            
  Partials     10     10

johlju · 2019-07-01T10:40:14Z

I will review this soon. I will need to correlate the comments from the closed PR with this one.

johlju

I added comments from the old PR. I have not review this in its entire yet.

A question from you in the old PR:

Basically I wanted to prove in the test, that only unneeded/needed accounts are getting removed/added. What is your suggestion here?

I think we don't have to test that in the unit test. That could be added as a test to a integration test. I will not enforce creating integration test yet though.

Reviewed 4 of 8 files at r1.
Reviewable status: 4 of 8 files reviewed, 6 unresolved discussions (waiting on @SSvilen)

CHANGELOG.md, line 10 at r1 (raw file):

Add RODC Creation Support

Please add a reference to the issue this closes, see other entries for example.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 355 at r1 (raw file):

if ($domainControllerObject)

From the old PR. I agree this is redundant and can be removed since it is checked in Get-TargetResource (which is called at the top).
When removing this we should remove the localized string as well if it is not used anywhere else.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 383 at r1 (raw file):

$AllowPasswordReplicationAccountName;

Please remove the semi-colon at the end of this row.

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 397 at r1 (raw file):

Quoted 7 lines of code…

            $adPrincipalsToRemove = foreach ($accountName in $targetResource.AllowPasswordReplicationAccountName)
            {
                if ($accountName -notin $AllowPasswordReplicationAccountName)
                {
                    New-Object -TypeName Microsoft.ActiveDirectory.Management.ADPrincipal -ArgumentList $accountName
                }
            }

I still think we should make a helper function that will reduce this duplicate code (4 times). Would be much better to have a helper function (can be in this file too) that returns a hash table with two arrays of the account names that should be added and removed. The the same helper function should be able to work for both AllowPasswordReplicationAccountName and DenyPasswordReplicationAccountName.

DSCResources/MSFT_xADDomainController/README.md, line 4 at r1 (raw file):

.Installation

Missin a space before 'Installation'.

DSCResources/MSFT_xADDomainController/README.md, line 4 at r1 (raw file):

Read Only domain controllers

Maybe change to this since we are using that elsewhere: Read-Only Domain Controllers (RODC)

johlju

Reviewed 3 of 8 files at r1.
Reviewable status: 7 of 8 files reviewed, 34 unresolved discussions (waiting on @johlju and @SSvilen)

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 119 at r1 (raw file):

        $getTargetResourceResult.SiteName = $domainControllerObject.Site
        $getTargetResourceResult.IsGlobalCatalog = $domainControllerObject.IsGlobalCatalog
        $getTargetResourceResult.DomainName = $domainControllerObject.Domain

We should also return the property ReadOnlyReplica here. Maybe return the value in the $domainControllerObject.IsReadOnly property?

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 642 at r1 (raw file):

        $testTargetResourceReturnValue = $false
    }

We are not testing if the domain controller is a RODC? If it is a domain controller but not a RODC we should probably throw an error? We can't turn a DC into a RODC, right?

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 650 at r1 (raw file):

if (-not (Test-Members @testMembersParameters))

I think the helper function created for the Set-TargetResource can be reused here too. 🤔

DSCResources/MSFT_xADDomainController/MSFT_xADDomainController.psm1, line 669 at r1 (raw file):

if (-not (Test-Members @testMembersParameters))

I think the helper function created for the Set-TargetResource can be reused here too. 🤔

Examples/Resources/xADDomainController/4-Read-OnlyDomainController_Config.ps1, line 18 at r1 (raw file):

#Requires -module xActiveDirectory

A blank row after this row

Examples/Resources/xADDomainController/4-Read-OnlyDomainController_Config.ps1, line 24 at r1 (raw file):

    and specify a list of account, whose passwords are allowed/denied for synchronisation.
#>

We should remove this row.

Examples/Resources/xADDomainController/4-Read-OnlyDomainController_Config.ps1, line 71 at r1 (raw file):

'pvdi.test1', 'pvdi.test'

Can we use this so it is clear it is an array @('pvdi.test1', 'pvdi.test').

Examples/Resources/xADDomainController/4-Read-OnlyDomainController_Config.ps1, line 72 at r1 (raw file):

'SVC_PVS', 'TA2SCVMM'

Can we use this so it is clear it is an array @('SVC_PVS', 'TA2SCVMM').

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 1 at r1 (raw file):

[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUsingConvertToSecureStringWithPlainText', 'PSAvoidUsingPlainTextForPassword')]

Instead of adding 'PSAvoidUsingPlainTextForPassword' to this row, I think we should add another row instead.

[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUsingConvertToSecureStringWithPlainText', '')]
[System.Diagnostics.CodeAnalysis.SuppressMessageAttribute('PSAvoidUsingPlainTextForPassword', '')]

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 13 at r1 (raw file):

git.exe

We should just use git here, since that is how it is in the unit template.

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 152 at r1 (raw file):

            Context 'Normal Operations' {

We should remove this blank row.

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 229 at r1 (raw file):

        #region Function Test-TargetResource
        Describe 'xActiveDirectory\Test-TargetResource' -Tag 'Test' {

We should remove this blank row.

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 234 at r1 (raw file):

Quoted 15 lines of code…

            $stubDomainController = New-Object -TypeName Microsoft.ActiveDirectory.Management.ADDomainController
            $stubDomainController.Site = $correctSiteName

            Mock -CommandName Get-DomainControllerObject -MockWith { return $stubDomainController }
            Mock -CommandName Get-ADDomainControllerPasswordReplicationPolicy -ParameterFilter { $Allowed.IsPresent } -MockWith {
                return [PSCustomObject]@{
                    SamAccountName = $allowedAccount
                }
            }

            Mock -CommandName Get-ADDomainControllerPasswordReplicationPolicy -ParameterFilter { $Denied.IsPresent } -MockWith {
                return [PSCustomObject]@{
                    SamAccountName = $deniedAccount
                }
            }

All this should be added to a BeforeAll-block.

BeforeAll {
    # Code goes here
}

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 255 at r1 (raw file):

                $stubDomainController.Domain = $correctDomainName

We should remove this blank row. (two blank rows in a row)

Tests/Unit/MSFT_xADDomainController.Tests.ps1, line 257 at r1 (raw file):

Mock -CommandName Get-DomainControllerObject -MockWith { return $stubDomainController }