xExchReceiveConnector: Enhancing the resource #437

SSvilen · 2019-12-06T21:16:25Z

Pull Request (PR) description

The goal of this PR is to enhance the xExchReceiveConnector resource.

This Pull Request (PR) fixes the following issues

#360
#388
#414

Task list

Added an entry under the Unreleased section of the change log in the CHANGELOG.md.
Entry should say what was changed, and how that affects users (if applicable).
Resource documentation added/updated in README.md.
Resource parameter descriptions added/updated in README.md, 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 · 2019-12-06T21:40:04Z

Codecov Report

Merging #437 into master will increase coverage by 5%.
The diff coverage is 80%.

@@          Coverage Diff           @@
##           master   #437    +/-   ##
======================================
+ Coverage      44%    50%    +5%     
======================================
  Files          42     44     +2     
  Lines        5241   5634   +393     
  Branches        0      1     +1     
======================================
+ Hits         2354   2835   +481     
+ Misses       2887   2798    -89     
- Partials        0      1     +1

mhendric · 2019-12-18T20:08:12Z

Hey @SSvilen , my Exchange lab is currently out of commission. Any chance that you have a lab that you can run the corresponding Integration tests (MSFT_xExchReceiveConnector.Integration.Tests) for this on? This has big enough changes where it's probably a good idea for someone to do that manually and confirm it's good.

SSvilen

Hi @mhendric ,

I had to adjust the tests and some helper function to make them work. The last last check though is always failing.

Context 'When Get-ReceiveConnector is called and the connector is absent' {
            It 'Should not cause an error to be logged in the event log' {
                Get-EventLog -LogName 'MSExchange Management' -After $testStartTime -ErrorAction SilentlyContinue | `
                    Where-Object -FilterScript { $_.Message -like '*Cmdlet failed. Cmdlet Get-ReceiveConnector, parameters -Identity*' } |`
                    Should -Be $null
            }
        }

That error message seems to be always logged, when a receive connector was not found. I dont know if that has to do with the fact, that my lab is based on 2019.

Reviewable status: 0 of 8 files reviewed, all discussions resolved

mhendric · 2020-01-02T20:32:49Z

Hey @SSvilen , sorry for the delay, was out for the holidays. And thanks for running this through the tests. The issue you are seeing was supposed to be fixed in #332, and the test that's failing was supposed to verify that. My first guess is that you have some other entries of this type in your log, and the generic filter for this test isn't excluding them. If that's the case, we may need to update the filter to only get stuff since the last Get-ReceiveConnector attempt (possibly record timestamp before, and then get events after that timestamp after running Get-ReceiveConnector). The other possibility is that some of the newly added code in this PR is trying to get a specific receive connector rather than filtering on all found receive connectors. In #332, this was fixed by changing all instances of:
return (Get-ReceiveConnector @PSBoundParameters -ErrorAction SilentlyContinue)

To:
return (Get-ReceiveConnector @getparams | Where-Object -FilterScript {$_.Identity -like $PSBoundParameters['Identity']})

mhendric

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

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

- Added additional parameters to the MSFT_xExchImapSettings resource
- Fixed unit test it statement for MSFT_xExchAutodiscoverVirtualDirectory\Test-TargetResource
- xExchReceiveConnector enhanced

Can you add some additional details on what was changed in this PR?

README.md, line 1584 at r1 (raw file):

Specifies whether the SMTP server name

This description appears to be incorrect.

DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 4 at r1 (raw file):

    .PARAMETER Name
        Identity of the Receive Connector. Needs to be in format SERVERNAME

The Parameter should be Identity here, right? Also, I don't believe the format is correct. Should be <Name>

DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 158 at r1 (raw file):

Sets the ressource

Please fix spelling.

DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 159 at r1 (raw file):

    .PARAMETER Name
        Identity of the Receive Connector. Needs to be in format SERVERNAME

The Parameter should be Identity here, right? Also, I don't believe the format is correct. Should be <Name>

DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 600 at r1 (raw file):

    .SYNOPSIS
        Tests the ressource

Please fix spelling.

DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 601 at r1 (raw file):

    .PARAMETER Name
        Identity of the Receive Connector. Needs to be in format SERVERNAME

The Parameter should be Identity here, right? Also, I don't believe the format is correct. Should be <Name>

SSvilen

Hi @mhendric ,

Yeah - I see what you mean. I have removed the internal get function entirely. When I'm done with the conversion to Azure DevOps, I'll take care of my two PRs.

Reviewable status: 1 of 8 files reviewed, 7 unresolved discussions (waiting on @SSvilen)

mhendric · 2020-01-27T17:50:09Z

@SSvilen , can you update the base branch of this to point to Master per step 'Remove branch dev from upstream repository' in https://dsccommunity.org/blog/convert-a-module-for-continuous-delivery?

mhendric · 2020-01-28T17:06:50Z

@SSvilen , please disregard my request to point this to Master. I was able to do it myself. Thanks.

SSvilen

Hi @mhendric , as soon as you approve the PR for the sendconnector, I can rewrite the part with AD permissions using the internal function in the helper module.

Reviewable status: 0 of 8 files reviewed, 7 unresolved discussions (waiting on @mhendric)

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

Previously, mhendric (Mike Hendrickson) wrote…

Can you add some additional details on what was changed in this PR?

Done.

README.md, line 1584 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

Specifies whether the SMTP server name
This description appears to be incorrect.

Done.

DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 4 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

    .PARAMETER Name
        Identity of the Receive Connector. Needs to be in format SERVERNAME
The Parameter should be Identity here, right? Also, I don't believe the format is correct. Should be <Name>

Done.

DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 158 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

Sets the ressource
Please fix spelling.

Done.

DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 159 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

    .PARAMETER Name
        Identity of the Receive Connector. Needs to be in format SERVERNAME
The Parameter should be Identity here, right? Also, I don't believe the format is correct. Should be <Name>

Done.

DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 600 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

    .SYNOPSIS
        Tests the ressource

Please fix spelling.

Done.

DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 601 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

    .PARAMETER Name
        Identity of the Receive Connector. Needs to be in format SERVERNAME
The Parameter should be Identity here, right? Also, I don't believe the format is correct. Should be <Name>

Done.

SSvilen · 2020-04-12T15:42:38Z

Hi @mhendric ,

I finally found the time to finish this PR. Can review it again?

mhendric

Reviewed 2 of 13 files at r2.
Reviewable status: 1 of 7 files reviewed, 3 unresolved discussions (waiting on @SSvilen)

README.md, line 1584 at r1 (raw file):

Previously, SSvilen (Svilen) wrote…

Done.

This still looks incorrect...

source/DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 570 at r2 (raw file):

Quoted 50 lines of code…

            if (($ExtendedRightAllowEntries -or $ExtendedRightDenyEntries) -and $null -eq $PSBoundParameters['DomainController'])
            {
                $receiveConnectorFound = Get-ADPermission -Identity $Identity.Split('\')[1] -ErrorAction SilentlyContinue
                $itt = 0

                while ($null -eq $receiveConnectorFound -and $itt -le 3)
                {
                    Write-Verbose -Message 'Extended AD permissions were specified and the new connector is still not found in AD. Sleeping for 30 seconds.'
                    Start-Sleep -Seconds 30
                    $itt++
                    $receiveConnectorFound = Get-ADPermission -Identity $Identity.Split('\')[1] -ErrorAction SilentlyContinue
                }

                if ($null -eq $receiveConnectorFound)
                {
                    throw 'The new send connector was not found after 2 minutes of wait time. Please check AD replication!'
                }
            }
            if ($null -ne $PSBoundParameters['DomainController'])
            {
                Write-Verbose -Message 'Setting domain controller as default parameter.'
                $PSDefaultParameterValues = @{
                    'Add-ADPermission:DomainController' = $DomainController
                }
            }
            if ($ExtendedRightAllowEntries)
            {
                Write-Verbose -Message "Setting ExtendedRightAllowEntries for receive connector: $($Identity.Split('\')[1])."

                foreach ($ExtendedRightAllowEntry in $ExtendedRightAllowEntries)
                {
                    foreach ($Value in $($ExtendedRightAllowEntry.Value.Split(',')))
                    {
                        Add-ADPermission -Identity $Identity.Split('\')[1] -User $ExtendedRightAllowEntry.Key -ExtendedRights $Value
                    }
                }
            }
            if ($ExtendedRightDenyEntries)
            {
                Write-Verbose -Message "Setting ExtendedRightDenyEntries for receive connector: $($Identity.Split('\')[1])."

                foreach ($ExtendedRightDenyEntry in $ExtendedRightDenyEntries)
                {
                    foreach ($Value in $($ExtendedRightDenyEntry.Value.Split(',')))
                    {
                        Add-ADPermission -Identity $Identity.Split('\')[1] -User $ExtendedRightDenyEntry.Key -ExtendedRights $Value -Deny -Confirm:$false
                    }
                }
            }
        }

This section seems largely identical to what's in xExchSendConnector, just replace "Send" for "Receive". What do you think about putting some or all of this into a helper function so we don't have mostly duplicate code in separate files?

source/DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 992 at r2 (raw file):

Quoted 20 lines of code…

            # Get AD permissions if necessary
            if (($ExtendedRightAllowEntries) -or ($ExtendedRightDenyEntries))
            {
                if ($PSBoundParameters.ContainsKey('DomainController'))
                {
                    $adPermissions = Get-ADPermission -Identity $Identity.Split('\')[1] -DomainController $DomainController | Where-Object { $_.IsInherited -eq $false }
                }
                else
                {
                    $adPermissions = Get-ADPermission -Identity $Identity.Split('\')[1] | Where-Object { $_.IsInherited -eq $false }
                }

                $splat = @{
                    ExtendedRightAllowEntries = $ExtendedRightAllowEntries
                    ExtendedRightDenyEntries  = $ExtendedRightDenyEntries
                    ADPermissions             = $adPermissions
                }

                $testResults = Test-ExtendedRights @splat
            }

This section seems largely identical to what's in xExchSendConnector. What do you think about putting some or all of this into a helper function so we don't have mostly duplicate code in separate files?

SSvilen

Reviewable status: 1 of 10 files reviewed, 3 unresolved discussions (waiting on @mhendric)

README.md, line 1584 at r1 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

This still looks incorrect...

It should be ok now.

source/DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 570 at r2 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

            if (($ExtendedRightAllowEntries -or $ExtendedRightDenyEntries) -and $null -eq $PSBoundParameters['DomainController'])
            {
                $receiveConnectorFound = Get-ADPermission -Identity $Identity.Split('\')[1] -ErrorAction SilentlyContinue
                $itt = 0

                while ($null -eq $receiveConnectorFound -and $itt -le 3)
                {
                    Write-Verbose -Message 'Extended AD permissions were specified and the new connector is still not found in AD. Sleeping for 30 seconds.'
                    Start-Sleep -Seconds 30
                    $itt++
                    $receiveConnectorFound = Get-ADPermission -Identity $Identity.Split('\')[1] -ErrorAction SilentlyContinue
                }

                if ($null -eq $receiveConnectorFound)
                {
                    throw 'The new send connector was not found after 2 minutes of wait time. Please check AD replication!'
                }
            }
            if ($null -ne $PSBoundParameters['DomainController'])
            {
                Write-Verbose -Message 'Setting domain controller as default parameter.'
                $PSDefaultParameterValues = @{
                    'Add-ADPermission:DomainController' = $DomainController
                }
            }
            if ($ExtendedRightAllowEntries)
            {
                Write-Verbose -Message "Setting ExtendedRightAllowEntries for receive connector: $($Identity.Split('\')[1])."

                foreach ($ExtendedRightAllowEntry in $ExtendedRightAllowEntries)
                {
                    foreach ($Value in $($ExtendedRightAllowEntry.Value.Split(',')))
                    {
                        Add-ADPermission -Identity $Identity.Split('\')[1] -User $ExtendedRightAllowEntry.Key -ExtendedRights $Value
                    }
                }
            }
            if ($ExtendedRightDenyEntries)
            {
                Write-Verbose -Message "Setting ExtendedRightDenyEntries for receive connector: $($Identity.Split('\')[1])."

                foreach ($ExtendedRightDenyEntry in $ExtendedRightDenyEntries)
                {
                    foreach ($Value in $($ExtendedRightDenyEntry.Value.Split(',')))
                    {
                        Add-ADPermission -Identity $Identity.Split('\')[1] -User $ExtendedRightDenyEntry.Key -ExtendedRights $Value -Deny -Confirm:$false
                    }
                }
            }
        }

This section seems largely identical to what's in xExchSendConnector, just replace "Send" for "Receive". What do you think about putting some or all of this into a helper function so we don't have mostly duplicate code in separate files?

I've created a new function in the exchange helper module.

source/DSCResources/MSFT_xExchReceiveConnector/MSFT_xExchReceiveConnector.psm1, line 992 at r2 (raw file):

Previously, mhendric (Mike Hendrickson) wrote…

            # Get AD permissions if necessary
            if (($ExtendedRightAllowEntries) -or ($ExtendedRightDenyEntries))
            {
                if ($PSBoundParameters.ContainsKey('DomainController'))
                {
                    $adPermissions = Get-ADPermission -Identity $Identity.Split('\')[1] -DomainController $DomainController | Where-Object { $_.IsInherited -eq $false }
                }
                else
                {
                    $adPermissions = Get-ADPermission -Identity $Identity.Split('\')[1] | Where-Object { $_.IsInherited -eq $false }
                }

                $splat = @{
                    ExtendedRightAllowEntries = $ExtendedRightAllowEntries
                    ExtendedRightDenyEntries  = $ExtendedRightDenyEntries
                    ADPermissions             = $adPermissions
                }

                $testResults = Test-ExtendedRights @splat
            }

This section seems largely identical to what's in xExchSendConnector. What do you think about putting some or all of this into a helper function so we don't have mostly duplicate code in separate files?

I've created a new function in the exchange helper module.

SSvilen

Hi @mhendric ,

I had to do quite a lot of code changes. because I found some problems with the integration tests.
Some of them relate to my previous PR about saving a DSC Module local under $env:Temp.
I hope it's ok to use this PR here..

Reviewable status: 0 of 19 files reviewed, 3 unresolved discussions (waiting on @mhendric)

mhendric · 2020-04-20T21:09:40Z

@SSvilen , yeah that's fine with me. Thanks for testing out the Integration tests too. I've been meaning to do that myself but don't have my test VM's setup for it right now. Let me know once you think this is ready for review again.

SSvilen

Hi @mhendric - I also have a production env, where I can test :)
BTW, some of the integration tests are failing, but I don't think it has to do with any of my changes. But I'll open a new issue, so we can track this problem.
This PR is now ready for review.
I also hope that in this turbulent time you and your family are ok!

Reviewable status: 0 of 19 files reviewed, 3 unresolved discussions (waiting on @mhendric)

mhendric

Reviewed 3 of 13 files at r2, 1 of 5 files at r3, 1 of 5 files at r4, 14 of 14 files at r5.
Reviewable status: complete! all files reviewed, all discussions resolved

mhendric mentioned this pull request Dec 10, 2019

X exch autodiscover virtual directory #436

Merged

9 tasks

SSvilen force-pushed the receiveconnectorfix branch from 48a4c29 to a53df68 Compare December 26, 2019 21:43

SSvilen commented Dec 26, 2019

View reviewed changes

mhendric requested changes Jan 4, 2020

View reviewed changes

SSvilen commented Jan 5, 2020

View reviewed changes

mhendric force-pushed the dev branch from d0240e6 to 1851316 Compare January 27, 2020 15:53

mhendric changed the base branch from dev to master January 28, 2020 17:06

SSvilen force-pushed the receiveconnectorfix branch from a53df68 to 8cdf8c8 Compare February 3, 2020 10:50

SSvilen commented Feb 3, 2020

View reviewed changes

SSvilen force-pushed the receiveconnectorfix branch from 8cdf8c8 to 61a98b0 Compare April 10, 2020 19:16

mhendric requested changes Apr 15, 2020

View reviewed changes

SSvilen commented Apr 18, 2020

View reviewed changes

SSvilen and others added 12 commits April 18, 2020 22:08

PR Ready

4dadb36

Mof file fix

153a3dc

fixes

9e367af

Extended permissions fix

ed1ff56

Adjusting the integration testing

19a5b4e

Addressing review comments

e276db1

rewrite using the helper functions

8548439

fixing the call to get-adpermissions

fa6a990

fixing the set-targetresource test

b0dea00

moving the setting of the extenden ad permissions to the helper module

7ac7480

fixing some of the pester tests and the helper module.

cbe6efc

fixing the pester tests

8608980

fix readme spelling error

6e51a7b

SSvilen force-pushed the receiveconnectorfix branch from 3366f90 to 6e51a7b Compare April 18, 2020 20:12

SSvilen added 2 commits April 19, 2020 20:55

fixing integrations tests..

d4b05ec

adjusting the unit test

49ee713

SSvilen commented Apr 20, 2020

View reviewed changes

SSvilen commented Apr 21, 2020

View reviewed changes

mhendric approved these changes Apr 27, 2020

View reviewed changes

mhendric merged commit fed8381 into dsccommunity:master Apr 27, 2020

SSvilen mentioned this pull request May 2, 2020

xExchReceiveConnector: ExtendedRightDenyEntries cause removal of Allow not Deny #414

Closed

SSvilen deleted the receiveconnectorfix branch September 9, 2020 14:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

xExchReceiveConnector: Enhancing the resource #437

xExchReceiveConnector: Enhancing the resource #437

SSvilen commented Dec 6, 2019 •

edited

Loading

codecov bot commented Dec 6, 2019 •

edited

Loading

mhendric commented Dec 18, 2019

SSvilen left a comment

mhendric commented Jan 2, 2020

mhendric left a comment

SSvilen left a comment

mhendric commented Jan 27, 2020

mhendric commented Jan 28, 2020

SSvilen left a comment

SSvilen commented Apr 12, 2020

mhendric left a comment

SSvilen left a comment

SSvilen left a comment

mhendric commented Apr 20, 2020

SSvilen left a comment

mhendric left a comment

xExchReceiveConnector: Enhancing the resource #437

xExchReceiveConnector: Enhancing the resource #437

Conversation

SSvilen commented Dec 6, 2019 • edited Loading

Pull Request (PR) description

This Pull Request (PR) fixes the following issues

Task list

codecov bot commented Dec 6, 2019 • edited Loading

Codecov Report

mhendric commented Dec 18, 2019

SSvilen left a comment

Choose a reason for hiding this comment

mhendric commented Jan 2, 2020

mhendric left a comment

Choose a reason for hiding this comment

SSvilen left a comment

Choose a reason for hiding this comment

mhendric commented Jan 27, 2020

mhendric commented Jan 28, 2020

SSvilen left a comment

Choose a reason for hiding this comment

SSvilen commented Apr 12, 2020

mhendric left a comment

Choose a reason for hiding this comment

SSvilen left a comment

Choose a reason for hiding this comment

SSvilen left a comment

Choose a reason for hiding this comment

mhendric commented Apr 20, 2020

SSvilen left a comment

Choose a reason for hiding this comment

mhendric left a comment

Choose a reason for hiding this comment

SSvilen commented Dec 6, 2019 •

edited

Loading

codecov bot commented Dec 6, 2019 •

edited

Loading