Set-DbaNetworkCertificate - Support localhost and use it as default for SqlInstance

[HCRitter]

Follows other functions in the project and sets the default value to localhost, as promised in the parameter description.

Please read -- recent changes to our repo

On November 10, 2022, we removed some bloat from our repository (for the second and final time). This change requires that all contributors reclone or refork their repo.

PRs from repos that have not been recently reforked or recloned will be closed and @potatoqualitee will cherry-pick your commits and open a new PR with your changes.

• Please confirm you have the smaller repo (85MB .git directory vs 275MB or 110MB or 185MB .git directory)

Type of Change

• Bug fix (non-breaking change, fixes Set-DbaNetworkCertificate parameter SqlInstance is not defaulting to localhost #9057 )
• New feature (non-breaking change, adds functionality, fixes # )
• Breaking change (affects multiple commands or functionality, fixes # )
• Ran manual Pester test and has passed (.\tests\manual.pester.ps1)
• Adding code coverage to existing functionality
• Pester test is included
• If new file reference added for test, has is been added to github.com/dataplat/appveyor-lab ?
• Unit test is included
• Documentation
• Build system

Purpose

fixes issue: #9057

Approach

Follows other functions in the project and sets the default value to localhost, as promised in the parameter description.

[HCRitter]

build error for appveyor was: Build execution time has reached the maximum allowed time for your plan (60 minutes).

how to proceed?

[andreasjordan]

There is nothing you can do...

But there are other changes needed, as after your change, the connection to localhost is still a "remote" connection and fails if WinRM is not set up. We need to test for localhost and then run the code localy and not remote. Will come back to you to help with this later.

[HCRitter]

@andreasjordan, as discussed I've added another commit to this PR.

Refactored 'Set-DBANetworkCertificate' by removing the Resolve-Section, as it now occurs in 'Invoke-ManagedComputerCommand' as well. This ensures the command can be run locally.

Simplified 'Invoke-ManagedComputerCommand' by replacing the nested try/catch blocks with a loop for better readability and maintainability.

[potatoqualitee]

Thank you, @HCRitter ! I'll let Andreas take care of this one, as there are a lot of changes. Happy to merge once approvals and tests pass.

[potatoqualitee]

Is this ready, @andreasjordan ? 🙏🏼

[andreasjordan]

No, not yet. Maybe I can test that later today. Will give feedback as soon as possible.

[potatoqualitee]

thanks, friend! 🍻

[andreasjordan]

This change from $resolved.FQDN to $computerName has to be applied to new lines 232 and 239 as well.

[andreasjordan]

@HCRitter Do you have time to update the pull request?

[andreasjordan]

I tested this with stopped WinRM service and restarting the service with -RestartService failed. But that is a problem with Restart-DbaService which needs WinRM to work even locally. @FriedrichWeinmann can you help us here? Is this by design? I know best practice is to have WinRM enabled and working from a central admin server, but I have some clients with some maschines where I need to use dbatools on the sql server box itself. And I think all of our commands should work locally without WinRM enabled.

[FriedrichWeinmann]

Is this by design?

The underlying CM system was intended to just work locally, even with disabled WinRM (it automatically switches to CIM/DCOM for localhost). There are problems with that though, as I just tested myself.
The *-DbaService commands however have one issue: They internally call Update-ServiceStatus which is hardcoded to use Invoke-CimMethod, which limits the protocols supported.

To make this work, we need to ...

• Fix Get-DbaCMObject working locally without WinRM service.
• Ensure the parallelized service update doesn't fail over it.

can you help us here?

Looking into it right now.
Can't promise a quick fix, given how the whole structure is somewhat complex and large - don't know whether the time needed for this is within my available time budget, but ... I'll see what I can do :)

All the changes needed will be within the CM code and the *-DbaService commands (and their internal helper), so it should not block this PR for that.

[andreasjordan]

Thanks Fred. If I can help, just let me know.

[FriedrichWeinmann]

Just submitted part one: #9107

Pretty sure I know how I'm fixing part two (Invoke-CimMethod):
The way we currently use it, it always uses WinRM (with the associated headaches for localhost). It also happens in a background runspace that does not have dbatools (but the types, since it's the same process).
This has a two-step resolution:

• Extend the CM system to grant connections a way to generate CimSessions (preconfigured by the entire connection handling, as it already does for Get-DbaCmObject). These sessions can be generated in the background runspace, since only the types are needed. The library change only adds a new feature without impacting existing code so it's really low risk.
• Then extend the internal Update-ServiceStatus (or any other command that uses Invoke-CimMethod) to generate the session via the CM system.

To make this work well, we'd need to update & ship the library first, then wait for the next release of the main dbatools to also update the commands afterwards, in order to avoid breaking the tests.
Should be reasonably simple to manage, will do the library PR later tonight.

[FriedrichWeinmann]

Stage 2 is implemented and pending as PR on the library side:

dataplat/dbatools.library#5

Again, not a prerequisite for this PR, but just to keep everybody in the loop. :)

[potatoqualitee]

Thank you, @FriedrichWeinmann ! Will merge your PRs shortly.