SqlAGDatabase: Fix impersonate permission test #1295
Conversation
… cody-konior/matchdatabaseowner
Codecov Report
@@ Coverage Diff @@
## dev #1295 +/- ##
=====================================
+ Coverage 97% 97% +<1%
=====================================
Files 35 35
Lines 4376 4432 +56
=====================================
+ Hits 4271 4327 +56
Misses 105 105 |
and increment Invoke-Query mock call count instead.
|
Can you please look at the failing tests |
johlju
added
the
waiting for code fix
|
Any idea how I can investigate this further? I can't reproduce the failure locally. |
|
You can debug in the appveyor build worker if you can’t reproduce it locally. See the AppVeyor documentation |
|
I found the cause of the test failures. The Pester mock parameter filters I modified are multi-line and still matched on Windows but would fail on the Linux build machine because of line endings. It was never an issue before because each line of the existing code ended in a wildcard which swallowed the carriage returns. By me adding a line without a wildcard the mock would be skipped and try to connect to an actual server and cause spurious errors. Anyway this is ready for review once the latest build passes. |
johlju
added
needs review
There was a problem hiding this comment.
This is awesome work, especially the work you put in to manually test this change!
Just a few comments.
Reviewed 5 of 9 files at r1, 4 of 4 files at r3, 1 of 1 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @codykonior)
README.md, line 290 at r4 (raw file):
PSDscRunAsCredential
This should actually be written with 'Ps'. https://docs.microsoft.com/en-us/powershell/dsc/configurations/runasuser
We could do a search/replace if there are other places where this is written wrongly.
SqlServerDscHelper.psm1, line 1062 at r4 (raw file):
function Test-LoginEffectivePermissions
Non-blocking: Would be really nice if we could add comment-based help to this function. But since there were none to begin this, this can be added in another PR in a larger cleanup job.
SqlServerDscHelper.psm1, line 1084 at r4 (raw file):
# Other types are not used here and so not implemented:
Can we change this to a comment-block? Although this would be more suitable in a comment-based help (see previous comment)
<#
Other types are not used here and so not implemented:
...
#>
SqlServerDscHelper.psm1, line 1279 at r4 (raw file):
$impersonatePermissionsPresent = $false
I thin k this row is unnecessary since it is set two calls down to either false or true?
SqlServerDscHelper.psm1, line 1292 at r4 (raw file):
return $impersonatePermissionsPresent
Maybe we should write a Verbose message saying what type of impersonate permission was returned for informational (debug) purposes? Before each return statement below.
Tests/Unit/SqlServerDSCHelper.Tests.ps1, line 1277 at r4 (raw file):
Assert-MockCalled -CommandName Invoke-Query -Exactly -Times 1 -Scope It
I think we instead should mock Test-LoginEffectivePermissions in the tests for Test-ImpersonatePermissions, and check with a ParameterFilter on the Assert-MockCalled that the correct properties of Test-LoginEffectivePermissions was called.
It would be easier than rely on a call in another helper function (the Invoke-Query).
The separate unit tests for Test-LoginEffectivePermissions should assert that Invoke-Query was called correctly.
We should preferably not test two helper functions in the same unit test, especially now when one become more complex.
johlju
added
waiting for code fix
Test-LoginEffectivePermissions is called.
There was a problem hiding this comment.
Reviewable status: 4 of 10 files reviewed, 5 unresolved discussions (waiting on @johlju)
README.md, line 290 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
PSDscRunAsCredentialThis should actually be written with 'Ps'. https://docs.microsoft.com/en-us/powershell/dsc/configurations/runasuser
We could do a search/replace if there are other places where this is written wrongly.
Done.
SqlServerDscHelper.psm1, line 1084 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
# Other types are not used here and so not implemented:Can we change this to a comment-block? Although this would be more suitable in a comment-based help (see previous comment)
<# Other types are not used here and so not implemented: ... #>
Done.
SqlServerDscHelper.psm1, line 1279 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
$impersonatePermissionsPresent = $falseI thin k this row is unnecessary since it is set two calls down to either false or true?
Done.
SqlServerDscHelper.psm1, line 1292 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
return $impersonatePermissionsPresentMaybe we should write a Verbose message saying what type of impersonate permission was returned for informational (debug) purposes? Before each return statement below.
Done.
Tests/Unit/SqlServerDSCHelper.Tests.ps1, line 1277 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
Assert-MockCalled -CommandName Invoke-Query -Exactly -Times 1 -Scope ItI think we instead should mock
Test-LoginEffectivePermissionsin the tests forTest-ImpersonatePermissions, and check with aParameterFilteron theAssert-MockCalledthat the correct properties ofTest-LoginEffectivePermissionswas called.
It would be easier than rely on a call in another helper function (theInvoke-Query).The separate unit tests for
Test-LoginEffectivePermissionsshould assert thatInvoke-Querywas called correctly.We should preferably not test two helper functions in the same unit test, especially now when one become more complex.
I've reverted this test to as it was before but I'm unsure where you'd like the replacement test put.
There was a problem hiding this comment.
Reviewed 5 of 6 files at r5.
Reviewable status: 3 of 10 files reviewed, 1 unresolved discussion (waiting on @codykonior and @johlju)
Tests/Unit/SqlServerDSCHelper.Tests.ps1, line 1277 at r4 (raw file):
Previously, codykonior (Cody Konior) wrote…
I've reverted this test to as it was before but I'm unsure where you'd like the replacement test put.
I think the changes to Test-ImpersonatePermissions should be tested here, but mock and assert the call to Test-LoginEffectivePermissions ( and assert using ParameterFilter to make sure it is called with the expected parameters).
I think the changes to Test-LoginEffectivePermissions should go in the tests in the following link, and will mock and assert Invoke-Query.
https://github.com/PowerShell/SqlServerDsc/blob/6a87852967c15f22f8b1c3809c2d531682a88873/Tests/Unit/SqlServerDSCHelper.Tests.ps1#L682
Would that work?
There was a problem hiding this comment.
Reviewed 7 of 7 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @codykonior)
These cover the alternate code path inside where a different Invoke-Query is issued for a LOGIN object (the new tests) as opposed to a SERVER object (the old tests).
There was a problem hiding this comment.
Reviewable status: 9 of 10 files reviewed, 1 unresolved discussion (waiting on @johlju)
Tests/Unit/SqlServerDSCHelper.Tests.ps1, line 1277 at r4 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
I think the changes to
Test-ImpersonatePermissionsshould be tested here, but mock and assert the call toTest-LoginEffectivePermissions( and assert usingParameterFilterto make sure it is called with the expected parameters).I think the changes to
Test-LoginEffectivePermissionsshould go in the tests in the following link, and will mock and assertInvoke-Query.
https://github.com/PowerShell/SqlServerDsc/blob/6a87852967c15f22f8b1c3809c2d531682a88873/Tests/Unit/SqlServerDSCHelper.Tests.ps1#L682Would that work?
Done.
There was a problem hiding this comment.
Reviewed 1 of 1 files at r8.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @codykonior)
Tests/Unit/SqlServerDSCHelper.Tests.ps1, line 766 at r8 (raw file):
It 'Should return $false when the specified login has no permissions assigned'
Is this test the same as the one two tests down?
Tests/Unit/SqlServerDSCHelper.Tests.ps1, line 775 at r8 (raw file):
It 'Should return $false when the desired login permissions are not present' {
Is this test the same as the previous comment?
|
@codykonior Can you double check the tests I commented on? It looks like the are the same? Otherwise it is all good now! 😄 |
There was a problem hiding this comment.
Reviewable status: 9 of 10 files reviewed, 2 unresolved discussions (waiting on @johlju)
Tests/Unit/SqlServerDSCHelper.Tests.ps1, line 766 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
It 'Should return $false when the specified login has no permissions assigned'Is this test the same as the one two tests down?
Done.
Tests/Unit/SqlServerDSCHelper.Tests.ps1, line 775 at r8 (raw file):
Previously, johlju (Johan Ljunggren) wrote…
It 'Should return $false when the desired login permissions are not present' {Is this test the same as the previous comment?
It was but it wasn't meant to be. I've updated the test.
There was a problem hiding this comment.
Reviewed 3 of 3 files at r9.
Reviewable status:complete! all files reviewed, all discussions resolved
|
@codykonior Awesome work on this! This one is merged now. 😄 |
johlju
removed
the
waiting for code fix
Pull Request (PR) description
When adding a database to an AG using SqlAGDatabase and the MatchDatabaseOwner switch, it checks for an IMPERSONATE ANY LOGIN permission.
That permission doesn't exist until SQL 2014. There are two other alternatives that exist on SQL Server 2012 (and also exist in higher versions):
This PR adds checks for the two additional permissions if the IMPERSONATE ANY LOGIN permission doesn't exist. It also fixes code coverage for New-TerminatingError as I noticed that wasn't right.
I raised this previously but the PR became too out of date to maintain. To address some of the comments of the time:
Re: SqlServerDscHelper.psm1 about $testLoginEffectPermissionsParams
The query that tests the permissions works fine even on lower versions so no change is necessary.
It's copy-paste of what was there previously.
ServiceName is not the service name. For default instances it's MSSQLSERVER where InstanceName is blank. For non-default instances both are the same.
Invoke-Query depends on InstanceName being MSSQLSERVER instead of blank so it has to stay as-is.
It would work in limited circumstances such as SQL 2014 and above, with IMPERSONATE ANY LOGIN permission (and possibly even more).
The reason I say "and possibly more" is that the RESTORE FULL is done under the impersonated account but then RESTORE LOG wasn't. This causes the process to fail even with the IMPERSONATE ANY LOGIN unless it has yet another another high level permission. This has all been fixed.
Re: Tests/Unit/SqlServerDSCHelper.Tests.ps1 about mocking
Test-LoginEffectivePermissions only gets called once, it then executes Invoke-Query one or more times to determine if the permissions are present; which is why that is mocked instead.
If I didn't do this and just mocked Test-LoginEffectivePermissions then nothing would change and my code wouldn't be covered by the test, and so PR would have reduced code coverage.
Re: Other comments about style and updating documentation.
I believe these are all done.
Further testing
I built a separate test rig to confirm this all works correctly on an actual instance. I ran it on my 5-node lab which can be built with my OftenOn PowerShell module and the script executed in a PowerShell admin console on CHWK01 to reproduce.
This separate test is pretty huge and in this gist so as not to clutter up the PR but the final bit which is captured in the gif is easy enough to understand..
It's checking the return bool from Invoke-DscResource and also who the owner is on the databases.
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