Script to periodically Identify and act on public IP address

This script checks what the current public IP address is. The plan is for this to run every 5 minutes or so, and then change some host file configuration depending on whether we the IP is identified as that of officeone or officetwo.

Each attempt at retrieving the IP address (as well as each success/failure) is logged to a daily log file which is overwritten every week.

To make it easier to manage, I split the code out into functions:

• SendIpQuery - for actually sending a webrequest to the IP service

• Get-PublicIP - calls SendIpQuery, which will call the above function for each of the services in $website until it finds something or times out (15 second timeout) IP is identified as per the defined regular expression

• SetServerConfiguration - this will call the final method depending on whether the IP found relates to officeone or officetwo

• ConfigureServers - configures the server host files based on which office the IP represents (the order of the parameters passed shows which office's IP was available)

The program begins right at the bottom of the script, and will run 5 times before giving up (if no IP is found).

I was hoping for some feedback on the structure of this script as well as methods used for timeout/requests and how appropriate they are.

```
# For logging each server attempt
$logname = (Get-Date).DayOfWeek
Function Log-Line {
param([string]$line)

Add-Content -Path "C:\servertesting\logs\$logname.txt" -Value $line -Force
}

# Region Functions
# Invokes a webrequest for the given website/service and returns the results.
# Timeout is set to 30 seconds - if this is reached the function returns 'timeout'
Function SendIPQuery {
param([string]$webaddress)

Log-Line "Requesting data from $webaddress..."

Try {return Invoke-WebRequest $webaddress -TimeoutSec 15}
Catch {return 'timeout'}
}
# Calls SendIPQuery for each of the websites in the array, and checks it against a regular expression
# Pos

Interesting little script you have here. Pretty straight forward what is going on here. There is the odd spelling or gramatical mistake in the comments but still enough to understand. Admintingly I am a little lost as to what is happening in ConfigureServers but that could just be me.

IP Address Validation and Comparison

The biggest nitpick I have with this code is your IPv4 Address validation. It looks like you are just trying to match an IP address like ##.###.##.## which might be fine right now but that is not a proper matching regex. There are more simple ones and more precise ones. This is all a matter of whether you can trust your source data and can allow errors. I am leaning to using the simple regex. While is has the potential to match things that are not IP addresses (like 888.888.888.888 which you wont get from the public IP sites) it won't miss valid IP's that your regex would (192.168.100.254).

((?:\d{1,3}\.){3}\d{1,3})

That would match 4 groups of up to 3 digits each separated by commas.

In your function Set-ServerConfiguration you have this

$officeoneip = "11.111.11.11"
$officetwoip    = "22.222.22.22"

If ($currentip -match $officeoneip) {

While this would not likely have the issue I am about to describe I need to point out another potential flaw. Period is a control character in regex. Now, I get that you will never get a public ip "11.111.11T11" but know that your comparison would match that since T would satisfy the "any character" meaning of the period. In this case it would be better anyway to use the -eq operator as you are looking for an exact match anyway.

If ($currentip -eq $officeoneip) {

No need to worry about regex characters. As a side note if you ever get more offices I would consider using a switch statement here so you don't need to get carried away with if's. What you have now is fine though.

Beware of your Try Block

You are catching all stopping errors from Invoke-WebRequest with your catch. However you are returning all errors as "timeout". If you were to type in a known bad address your code would spit that out as a timeout when really it is not a valid address or well formed URI.

try{Invoke-WebRequest "http://" -TimeoutSec 15}
catch{"timeout"}

Just something to be mindful of.

Function Naming Conventions

PowerShell suggests that you name your functions in the basic for of Verb-Noun(s) among other convention standards. This is not super important for personal code but consistency should be. Some of your function names have hyphens and others don't. Hyphens are present in all of the MS cmdlets so it would make sense to follow suit. Again, just be consistent.

Function Names and Calls

You are using the param keyword which is good for your simple functions. However it is also a good practice when calling your functions to use the parameter keywords and not rely on positional argument matching. It is not a question of functionality but of readability. Looking at:

ConfigureServers 'officetwo' 'officeone'

Going into this I would expect that we are performing functions on both servers called 'officetwo' and 'officeone'. This is of course not what exactly is happening. Something a little more descriptive might be useful here.

Update-OfficeHostFiles -CurrentOffice 'officetwo' -PastOffice 'officeone'

I refer to what I mentioned earlier that I am not 100% certain what you are doing here with this function but I think I know enough that you should get my point.

String Repetition

If you are using the same text over and over it is in your best interests to store it in a variable so that future changes are only in one place.

$rootPath = "C:\servertesting"

If (Test-Path ([io.path]::Combine($rootPath, "$notoffice.txt")) {
        Copy-Item ([io.path]::Combine($rootPath,"testy\hosts - $office")) $rootPath -Force
        Remove-Item ([io.path]::Combine($rootPath,"$notoffice.txt")) -Force 
        New-Item    ([io.path]::Combine($rootPath,"$office.txt")) -ItemType File -Force
}

While it might seem like I made it worse I have made it more robust and made future changes easier. Know that the use of [io.path]::Combine allow for making file paths easier without have to worry about placing slashes in between. I am more trying to draw attention to the use of the variable $rootPath.

Output from New-Item

The file object created by New-Item is going to be sent to output. Therefore you commonly see people suppressing that output for a cleaner program execution.

New-Item    "C:\servertesting\$office.txt" -ItemType File -Force | Out-Null

StackExchange Code Review Q#126202, answer score: 3