Code review: Is this the best approach for retrieving network info of a host?

This topic contains 5 replies, has 2 voices, and was last updated by Profile photo of Max Kozlov Max Kozlov 4 months, 1 week ago.

  • Author
    Posts
  • #66496
    Profile photo of Ron Bergin
    Ron Bergin
    Participant

    Let me start by saying that this is the very first time I've worked with Powershell. I've been asked to take over the maintenance of a startup script that is used to join the station to the (samba4) domain.

    After reviewing the script I found enough bugs to want to do a major rewrite, but for the purpose of this thread, I'm focusing on the section that retrieves the workstation's IP address.

    Here's the original code as it is now.

    function Wait-Network ($tries) {
            while (1) {
        # Get a list of DHCP-enabled interfaces that have a 
        # non-$null DefaultIPGateway property.
                    $x = gwmi -class Win32_NetworkAdapterConfiguration `
                            -filter IPEnabled=TRUE 
    
        # If there is (at least) one available, exit the loop.
                    if ( ($x | measure).count -gt 0 ) {
                            break
                    }
    
        # If $tries > 0 and we have tried $tries times without
        # success, throw an exception.
                    if ( $tries -gt 0 -and $try++ -ge $tries ) {
                            Write-TheLog("Network unavailable after $try tries.")
    						New-UserMessage "No Network Access" "This station was unable to find an active network connection."
    						Exit
                    }
    
        # Wait one second.
                    start-sleep -s 1
            }
    }
    
    $NICs = Get-WMIObject Win32_NetworkAdapterConfiguration ` 
     where{$_.IPEnabled -eq "TRUE"} 
    $NICS | Foreach { 
    $_.EnableDHCP()
    }
    start-sleep -s 15
    Wait-Network(30)
    $IPAddrs = (Get-WMIObject Win32_NetworkAdapterConfiguration -Filter "IPEnabled = True").IPAddress
    
    forEach($i in $IPAddrs)
    {
    	if(-not $i.Contains("::"))
    	{
    		$stationIP = $i
    	}
    } 
    

    The main (but not only problem) is that it loops over all interfaces and uses the IP of the last interface that doesn't have an IP6 address, which may or may not be the correct interface.

    I need to fix that bug and extend functionality to retrieve additional network info. Here is the function I worked up based on examples I found while researching powershell syntax.

    function Get-HostInfo
    {
    Get-HostInfo -ComputerName TESTPC
    
            IPaddress   : 10.100.0.119
            FQDN        : TESTPC.company.com
            DHCPEnabled : True
            Hostname    : TESTPC
            GW          : 10.100.0.1
            MACaddress  : 00:1C:C0:A3:6E:0A
    
    
            .TODO
            - If DHCPEnabled is FALSE on the desired interface, enable it
              and make a recursive call to the function to make sure
              it is retrieving the allocated DHCP reservation IP address.
    
            - Extend the function to also be able to get remote host info
    
            .Author
            Ron Bergin
            me@company.com
    #>
    
        [CmdletBinding()]
    
        Param
        (
            [Parameter(
                       Mandatory         = $true,
                       ValueFromPipeline = $true
                       )
            ]
            [string]$ComputerName
        )
    
        Begin { }
    
        Process {
    
            $FQDN       = (Get-WmiObject win32_computersystem).DNSHostName + '.' + (Get-WmiObject win32_computersystem).Domain
            #$FQDN       = $ComputerName + '.' + (Get-WmiObject win32_computersystem).Domain
            $interfaces = Get-WmiObject -class 'Win32_NetworkAdapterConfiguration' -computername $ComputerName | Where{ $_.IpEnabled -Match 'True' }
    
            Foreach ($interface in $interfaces) {
    
                if ($interface.IPAddress -match '^10.') {
                    $ip   = $interface.IPAddress[0]
                    $GW   = $interface.DefaultIPGateway[0]
                    $DHCP = $interface.DHCPEnabled
                    $MAC  = Get-CimInstance win32_networkadapterconfiguration | Where {$_.Description -eq $interface.Description} | select MACaddress 
                    break
                }
            }
    
            $hostinfo = @{
                'Hostname'    = $ComputerName
                'FQDN'        = $FQDN
                'IPaddress'   = $ip
                'GW'          = $GW
                'MACaddress'  = $MAC.MACaddress
                'DHCPEnabled' = $DHCP
            }
            
            $host_info = New-Object -TypeName PSObject -Property $hostinfo
            
        }
    
        End { $host_info }
    }
    
    $host_info = Get-HostInfo -ComputerName $env:computername
    Write-Output $host_info
    

    Questions:
    1) Is this a good (best practice) approach or is there a better approach?

    2) What adjustments do I need to make to have it be more generic so that I can retrieve the same info from a remote host? My goal is to move the function into a module so that it can be used in other scripts.

  • #66582
    Profile photo of Max Kozlov
    Max Kozlov
    Participant

    for remote computers you need to add -computername parameter to each wmi call (and may be switch to CIM)

    .IPEnabled -match 'true' — working but not the best option
    you shoukd read about -match operator. it's for strings, not booleans as .IPEnabled
    you can use where{ $_.IpEnabled -eq $True } or (because it's already boolean where{ $_.IpEnabled }
    or return to -Filter "IPEnabled = True"

    and...serious bug... you code will return only the last info.
    'comp1','comp2','comp3' | Get-HostInfo will return info only for comp3, becuase you iutput data to pipeline in the end{} block
    you should clean end{} block and generate output to pipeline, not variable
    New-Object -TypeName PSObject -Property $hostinfo

  • #66634
    Profile photo of Ron Bergin
    Ron Bergin
    Participant

    Thank you Max, your input was helpful and fixed most of the important issues.

    I removed the End block and made the other adjustments you suggested. This is what I have now.

        Process {
    
            $FQDN       = (Get-WmiObject win32_computersystem -ComputerName $ComputerName).DNSHostName + '.' + (Get-WmiObject win32_computersystem -ComputerName $ComputerName).Domain
            $interfaces = Get-WmiObject -class 'Win32_NetworkAdapterConfiguration' -ComputerName $ComputerName | Where{ $_.IpEnabled }
    
            Foreach ($interface in $interfaces) {
    
                if ($interface.IPAddress -match '^10.') {
                    $ip   = $interface.IPAddress[0]
                    $GW   = $interface.DefaultIPGateway[0]
                    $DHCP = $interface.DHCPEnabled
                    $MAC  = Get-WmiObject -class 'win32_networkadapterconfiguration' -ComputerName $ComputerName | Where {$_.Description -eq $interface.Description} | select MACaddress 
                    break
                }
            }
    
            $hostinfo = @{
                'Hostname'    = $ComputerName
                'FQDN'        = $FQDN
                'IPaddress'   = $ip
                'GW'          = $GW
                'MACaddress'  = $MAC.MACaddress
                'DHCPEnabled' = $DHCP
            }
            
            New-Object -TypeName PSObject -Property $hostinfo
        }
    
    }
    
    $host_info = 'Remote-Hostname', $env:computername | Get-HostInfo
    Write-Output $host_info
    

    It does return all the proper info for both hosts provided they are on the same subnet. If the remote host is on a different subnet, instead of receiving the desired data I receive the following error output for that host.

    PS C:\test> .\ip.ps1
    Get-WmiObject : The RPC server is unavailable. (Exception from HRESULT: 0x800706BA)
    At C:\test\ip.ps1:49 char:24
    +         $FQDN       = (Get-WmiObject win32_computersystem -ComputerName $Compute ...
    +                        ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        + CategoryInfo          : InvalidOperation: (:) [Get-WmiObject], COMException
        + FullyQualifiedErrorId : GetWMICOMException,Microsoft.PowerShell.Commands.GetWmiObjectCommand
    
    Get-WmiObject : The RPC server is unavailable. (Exception from HRESULT: 0x800706BA)
    At C:\test\ip.ps1:49 char:109
    + ... tName + '.' + (Get-WmiObject win32_computersystem -ComputerName $ComputerName).D ...
    +                    ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        + CategoryInfo          : InvalidOperation: (:) [Get-WmiObject], COMException
        + FullyQualifiedErrorId : GetWMICOMException,Microsoft.PowerShell.Commands.GetWmiObjectCommand
    
    Get-WmiObject : The RPC server is unavailable. (Exception from HRESULT: 0x800706BA)
    At C:\test\ip.ps1:50 char:23
    +         $interfaces = Get-WmiObject -class 'Win32_NetworkAdapterConfiguration' - ...
    + ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
        + CategoryInfo          : InvalidOperation: (:) [Get-WmiObject], COMException
        + FullyQualifiedErrorId : GetWMICOMException,Microsoft.PowerShell.Commands.GetWmiObjectCommand
    

    Would that be due to firewall rules blocking something, or is this an expected limitation when connecting to another subnet?

  • #66658
    Profile photo of Max Kozlov
    Max Kozlov
    Participant

    afaik there is no subnet limitations. I think this is firewall that block wmi calls
    btw, I overlooked one more thing – how you get macaddress
    you make another wmi caLL (the same call) and search the same object...
    instead of

     $MAC  = Get-WmiObject -class 'win32_networkadapterconfiguration' -ComputerName $ComputerName | Where {$_.Description -eq $interface.Description} | select MACaddress 
    #[...]
    'MACaddress'  = $MAC.MACaddress
    

    you can just

    $MAC  = $interface.MACaddress
    #[...]
    'MACaddress'  = $MAC
    

    Further optimizations can include moving hostinfo object creation right inside foreach { if { block and elimination all of internal variables like $ip, $gw, $dhcp and $mac 😉

  • #66672
    Profile photo of Ron Bergin
    Ron Bergin
    Participant

    Thanks Max,

    I figured out that fix for the mac address assignment shortly after applying the prior adjustments.

    When I wrote the function, I thought about putting the $hostinfo assignment inside the if { block but didn't because if the conditional never evaluated to true, then I wouldn't have the hash keys in the object, which I need to have available for later use in the script.

  • #66771
    Profile photo of Max Kozlov
    Max Kozlov
    Participant

    there is two options:
    1. you can create hostinfo hash before foreach loop and fill it with defaults
    2. you can create resulting object inside if { block (if your function ends here and all other object usage outside of a function (this is preferrable)

You must be logged in to reply to this topic.