Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

XDnsServerAdZone: Removing or Fixing Call to New-CimSession #53

Closed
TraGicCode opened this issue Sep 11, 2017 · 27 comments · Fixed by #62
Closed

XDnsServerAdZone: Removing or Fixing Call to New-CimSession #53

TraGicCode opened this issue Sep 11, 2017 · 27 comments · Fixed by #62

Comments

@TraGicCode
Copy link
Contributor

Currently we only have winrm configured to listen on loopback ONLY with the following below settings

image

Unfortunately the XDnsServerAdZone DSC resource creates a new-cimsession in the Get-TargetResource function. This ends up resolving to the ip of the nic of the machine. Therefore the cimsession to connect will fail since it's not connecting VIA loopback address configured above for the winrm listener. Below is the error i'm getting

Could not evaluate: Cannot validate argument on parameter 'CimSession'. The argument is null or empty. Provide an argument that is not null or empty, and then try the command again

1.) Why is New-CimSession even needed here? can it be removed?
2.) If New-CimSession cannot be removed can we allow passing in of the ip to create the cimsession for?

@greggoindenver
Copy link

I can confirm the same error message even with no winrm constraints in place.
2017-09-11_13-42-01

@TraGicCode
Copy link
Contributor Author

@greggoindenver Do you have any ideas as to why a new-cimsession is being used in this dsc resource?

@TraGicCode
Copy link
Contributor Author

@corydwood Maybe you could shed some light on this

@corydwood
Copy link
Contributor

New-CimSession is being used to allow creating zones on remote DNS servers and/or using specific credentials for creating the zone on either the local server or a remote server.

The code could be changed to only use New-CimSession if the ComputerName or Credential parameters are used. I can look into doing that, but may not be able to get to it for a few days.

@corydwood
Copy link
Contributor

Also, it looks like #49 broke this resource as the Set-TargetResource function relies on the CimSession property being returned by the call to Get-TargetResource.

@TraGicCode
Copy link
Contributor Author

@corydwood I might be able to get around to doing this as well.. Cannot promise anything depend on how my schedule goes.

@Sawal72
Copy link

Sawal72 commented Oct 17, 2017

We have the same problem with version 1.8.0.0. Version 1.7.0.0 works fine.

@greggoindenver
Copy link

Has there been any further status on this issue?

@jpuskar
Copy link

jpuskar commented Nov 3, 2017

+1, same repro

@greggoindenver
Copy link

@corydwood, @TraGicCode : Have either of you been able to reproduce this error? I tried it again today on a different machine. I can create forward zones, but not reverse zones.

@ejleroy
Copy link

ejleroy commented Nov 22, 2017

I had the same issue "Cannot validate argument on parameter 'CimSession'"
Workaround was to comment out the CimSession (line 240)
C:\Program Files\WindowsPowerShell\Modules\xDnsServer\1.9.0.0\DSCResources\MSFT_xDnsServerADZone\MSFT_xDnsServerADZone.psm1

image

Should only add the CimSession parameter if the computername attribute was set on the resource.

@ejleroy
Copy link

ejleroy commented Jan 5, 2018

Update: This problem still exists.

I've updated the following lines as a proposed solution:
Removed line 240 (see previous comment screenshot).
Added (before line 242):

if($targetResource.CimSession) 
{
	$addParams += @{
                    CimSession = $targetResource.CimSession
        }
}

Also, under ($targetResource.Ensure -eq 'Present'):

## Update the existing zone
            $updateParams = @{
                Name = $targetResource.Name
            }
	    if($targetResource.CimSession) 
	    {
		 $addParams += @{
                    CimSession = $targetResource.CimSession
            }

(Please forgive the formatting)

@regedit32
Copy link
Member

@ejleroy your fix looks good. Are you going to create a pull request with it?

@regedit32
Copy link
Member

Also, the $targetResource returned by Get-TargetResource doesn't actually include CimSession, so we should probably just create the CimSession if needed in the Set-TargetResource. Otherwise, as-is, the CimSessions are being created for each zone, but never being removed.

@ejleroy
Copy link

ejleroy commented Jan 10, 2018

Made some more changes based on your feedback. Going to test this out hopefully tomorrow and create a pull request if it looks okay. Basically, we'll only use CimSession (in Get and Set) if needed (based on if ComputerName is passed in) and remove any CimSessions created.

`function Get-TargetResource
{
[CmdletBinding()]
[OutputType([System.Collections.Hashtable])]
param
(
[Parameter(Mandatory)]
[ValidateNotNullOrEmpty()]
[System.String]
$Name,

    [Parameter()]
    [ValidateSet('None','NonsecureAndSecure','Secure')]
    [System.String]
    $DynamicUpdate = 'Secure',

    [Parameter(Mandatory)]
    [ValidateSet('Custom','Domain','Forest','Legacy')]
    [System.String]
    $ReplicationScope,

    [Parameter()]
    [System.String]
    $DirectoryPartitionName,

    [Parameter()]
    [System.String]
    $ComputerName,

    [Parameter()]
    [pscredential]
    $Credential,

    [Parameter()]
    [ValidateSet('Present','Absent')]
    [System.String]
    $Ensure = 'Present'
)
Assert-Module -Name 'DNSServer'
Write-Verbose ($LocalizedData.CheckingZoneMessage -f $Name, $Ensure)
$getParams = @{
    Name = $Name
    ErrorAction = 'SilentlyContinue'
}
if ($ComputerName)
{
	$cimSessionParams = @{ErrorAction = 'SilentlyContinue'}
    $cimSessionParams += @{ComputerName = $ComputerName}
	if ($Credential)
	{
		$cimSessionParams += @{Credential = $Credential}
	}
	$cimSession = New-CimSession @cimSessionParams
	$getParams += @{CimSession = $cimSession}
}
$dnsServerZone = Get-DnsServerZone @getParams
if($ComputerName) {
	Remove-CimSession $cimSession
}

$targetResource = @{
    Name = $dnsServerZone.ZoneName
    DynamicUpdate = $dnsServerZone.DynamicUpdate
    ReplicationScope = $dnsServerZone.ReplicationScope
    DirectoryPartitionName = $dnsServerZone.DirectoryPartitionName
    Ensure = if ($null -eq $dnsServerZone) { 'Absent' } else { 'Present' }
}
return $targetResource

}`

`function Set-TargetResource
{
[CmdletBinding()]
param
(
[Parameter(Mandatory)]
[ValidateNotNullOrEmpty()]
[System.String]
$Name,

    [Parameter()]
    [ValidateSet('None','NonsecureAndSecure','Secure')]
    [System.String]
    $DynamicUpdate = 'Secure',

    [Parameter(Mandatory)]
    [ValidateSet('Custom','Domain','Forest','Legacy')]
    [System.String]
    $ReplicationScope,

    [Parameter()]
    [System.String]
    $DirectoryPartitionName,

    [Parameter()]
    [System.String]
    $ComputerName,

    [Parameter()]
    [pscredential]
    $Credential,

    [Parameter()]
    [ValidateSet('Present','Absent')]
    [System.String]
    $Ensure = 'Present'
)
Assert-Module -Name 'DNSServer'
$targetResource = Get-TargetResource @PSBoundParameters
if ($Ensure -eq 'Present')
{
    if ($targetResource.Ensure -eq 'Present')
    {
        ## Update the existing zone
        $updateParams = @{
            Name = $targetResource.Name
        }
		if ($ComputerName)
		{
			$cimSessionParams = @{ErrorAction = 'SilentlyContinue'}
			$cimSessionParams += @{ComputerName = $ComputerName}
			if ($Credential)
			{
				$cimSessionParams += @{Credential = $Credential}
			}
			$cimSession = New-CimSession @cimSessionParams
			$updateParams += @{CimSession = $cimSession}
		}
        if ($targetResource.DynamicUpdate -ne $DynamicUpdate)
        {
            $updateParams += @{DynamicUpdate = $DynamicUpdate}
            Write-Verbose ($LocalizedData.SetPropertyMessage -f 'DynamicUpdate')
        }
        if ($targetResource.ReplicationScope -ne $ReplicationScope)
        {
            $updateParams += @{ReplicationScope = $ReplicationScope}
            Write-Verbose ($LocalizedData.SetPropertyMessage -f 'ReplicationScope')
        }
        if ($DirectoryPartitionName -and $targetResource.DirectoryPartitionName -ne $DirectoryPartitionName)
        {
            $updateParams += @{DirectoryPartitionName = $DirectoryPartitionName}
            Write-Verbose ($LocalizedData.SetPropertyMessage -f 'DirectoryPartitionName')
        }
        Set-DnsServerPrimaryZone @updateParams
    }
    elseif ($targetResource.Ensure -eq 'Absent')
    {
        ## Create the zone
        Write-Verbose ($LocalizedData.AddingZoneMessage -f $targetResource.Name)
        $addParams = @{
            Name = $Name
            DynamicUpdate = $DynamicUpdate
            ReplicationScope = $ReplicationScope
        }
		if ($ComputerName)
		{
			$cimSessionParams = @{ErrorAction = 'SilentlyContinue'}
			$cimSessionParams += @{ComputerName = $ComputerName}
			if ($Credential)
			{
				$cimSessionParams += @{Credential = $Credential}
			}
			$cimSession = New-CimSession @cimSessionParams
			$addParams += @{CimSession = $cimSession}
		}
        if ($DirectoryPartitionName)
        {
            $addParams += @{
                DirectoryPartitionName = $DirectoryPartitionName
            }
        }
        Add-DnsServerPrimaryZone @addParams
    }
}
elseif ($Ensure -eq 'Absent')
{
    # Remove the DNS Server zone
    Write-Verbose ($LocalizedData.RemovingZoneMessage -f $targetResource.Name)
	$removeParams = @{
		Name = $targetResource.Name
		Force = $true
	}
	if ($ComputerName)
	{
		$cimSessionParams = @{ErrorAction = 'SilentlyContinue'}
		$cimSessionParams += @{ComputerName = $ComputerName}
		if ($Credential)
		{
			$cimSessionParams += @{Credential = $Credential}
		}
		$cimSession = New-CimSession @cimSessionParams
		$removeParams += @{CimSession = $cimSession}
	}
	Remove-DnsServerZone @removeParams
}
if($ComputerName) {
	Remove-CimSession $cimSession
}

}`

@regedit32
Copy link
Member

@ejleroy Looks good. Looking forward to the PR

@TraGicCode
Copy link
Contributor Author

@ejleroy Were you going to open up a PR for this?

@TraGicCode
Copy link
Contributor Author

@greggoindenver I cannot create a forward or a reverse lookup zone still =[

@greggoindenver
Copy link

I can confirm that the module still will not create AD-enabled zones. I just tested it about two weeks ago.

@TraGicCode
Copy link
Contributor Author

@johlju , I'm cc'ing you on this to hopefully get some feedback on this before i dive in and start working on a PR to fix this issue.

  1. Is there an accepted pattern for having a surrogate node apply dsc configurations on itself that connect to some other node in which the resource should be managed? For example this module allows specifying credentials + computer name in which it will connect to and try and manage a xDnsServerAdZone resource. Id there any example in any known DSC Resources in the wild you know of that has this feature?

  2. If using a CIMSession is considered acceptable then should it be returned from Get-TargetResource? Also what about the schema file as Added the CimSession to the schema #44 is attempting to do?

Thanks.

@JrAsparagus
Copy link

JrAsparagus commented Feb 5, 2018 via email

@johlju
Copy link
Member

johlju commented Feb 6, 2018

@TraGicCode

  1. The resource SqlAG and SqlAGReplica in SqlServerDsc does something similar to find the primary replica. If the current node is not the primary replica and a change is needed, it will connect to the primary replica of the Always On cluster, since the change can only be done on the primary replica. It connects the same way using the SMO object model, which in this case can be compared to New-CIMSession. It connects the same regardless if it is the target node or one of the other nodes in the AlwaysOn cluster.

  2. Each *-TargetResource should start it's own CIM session. I say this because each property returned by Get-TargetResource should have at least a read-only property in the schema.mof. I don't think returning a CIMSession object is good practice (or even possible as a read-only property).

@johlju
Copy link
Member

johlju commented Feb 6, 2018

@TraGicCode to solve this issue, using New-CimSession only when ComputerName is assigned a value seems okay to me. It feels to me like it is a breaking change though, what do you think? Not a problem, just needs to be "tagged" accordingly. Read about breaking changes https://github.com/PowerShell/DscResources/blob/master/CONTRIBUTING.md#breaking-changes

Regarding that the target node connects to a different server. Normally I would expect this to be run on the DNS server itself, and there would be no need for a ComputerName parameter. But since there is a ComputerName parameter there is probably a scenario where it is needed. Curious, does can anyone give an example for such a scenario where one need to run this on a different server than the DNS server?

@johlju
Copy link
Member

johlju commented Feb 7, 2018

If there need to be a specific account that needs to be used to run the configuration then the built-in credential parameter PSDscRunAsCredential could be used to run the resource as another user than SYSTEM. Then there should be be no need to specify the credential parameter? But for using this in WMF 4.0 then Credential parameter need to be specified to use New-CImSession with -Credential parameter.

@TraGicCode
Copy link
Contributor Author

@johlju I don't think this would be a breaking change since it was working previously and as pointed out #49 broke this unknowingly.

The solution you are proposing i'm okay with implementing as it seems like the original author of this @corydwood was proposing the same solution.

I do have a couple of more questions in the regards:

  1. What versions of WMF/Powershell should be supported in DSC modules that exist in the DSC resource kit?
    • Both WMF4 and WMF5?

@johlju
Copy link
Member

johlju commented Feb 9, 2018

I asked the question before as well, the respons to my question was that if it is possible to keep WMF 4.0 support, then that would be a good thing, but it is not a requirement.

In SqlServerDsc we are only supporting WMF 5.x since to support for WMF 4.0 would have been to much work. In SqlServerDsc we actually removed some WMF 4.0 support to remove technical debt and support more WMF 5.x scenarios (like support for built-in PsDscRunAsCredential). Maybe some resource in SqlServerDsc work on WMF 4.0, but that is nothing that is tested.

In this case, try to keep WMF 4.0 support if possible, but if that will result in more technical debt (new know issues that can't be solved) then I would say to remove WMF 4.0. But that would be a breaking change. At the end it's up to the DSC community, should we keep WMF 4.0 or not for this resource module?

@TraGicCode
Copy link
Contributor Author

@greggoindenver @ejleroy @regedit32 Feel free to try my branch at #62 to verify this fixes your issues.

johlju pushed a commit that referenced this issue Apr 20, 2018
- Fixed all PSSA rule warnings.
- Changes to xDnsServerADZone
  -  Fixed bug introduced by issue #49. Previously, CimSessions were always used
     regardless of connecting to a remote machine or the local machine. Now
     CimSessions are only utilized when a ComputerName, or ComputerName and
     Credential are used (issue #53).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

9 participants