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

Improve Handling for TLS #5203

Merged
merged 2 commits into from Mar 15, 2019

Conversation

Projects
None yet
2 participants
@sheldonhull
Copy link
Contributor

sheldonhull commented Mar 14, 2019

Type of Change

  • Bug fix

Purpose

AWS instances with restricted internet settings fail to process webrequest without basic parsing. A better practice for rest based requests is to use Invoke-RestMethod.

Approach

  • Timeout parameter added for allowing override of AWS metadata
  • New internal function cloned from Invoke-TlsWebRequest that is using Invoke-RestMethod
  • Proper handling of failure to connect with try{} catch{} should ensure timeout exception handled properly and warned, bypassing attempt to connect to AWS metadata further.
  • new added 3 properties related to CPU metrics to improve the results for Get-DbaComputerSystem in reviewing basics of OS performance specs

Commands to test

  1. Get-DbaComputerSystem
  2. Invoke-TlsRestMethod new internal helper

Note

  1. Without AWS infrastructure can't verify this with pester test, and didn't see any Invoke-TLS* tests built. The new error handling worked, and confirmed on my own AWS instance that results pulled correctly, but you'll want to verify as well.
  2. Not sure of any other location to add detail to.
Improve Handling for TLS
- Timeout parameter added for allowing override of AWS metadata
- New internal function cloned from `Invoke-TlsWebRequest` that is using `Invoke-RestMethod`
- Proper handling of failure to connect with `try{} catch{}` should ensure timeout exception handled properly and warned, bypassing attempt to connect to AWS metadata further.
# Why
AWS instances with restricted internet settings fail to process webrequest without basic parsing. A better practice for rest based requests is to use `Invoke-RestMethod`. 


# Note
Without AWS infrastructure can't verify this with pester test, and didn't see any `Invoke-TLS*` tests built. The new error handling worked, and confirmed on my own AWS instance that results pulled correctly, but you'll want to verify as well.
@@ -55,6 +58,7 @@ function Get-DbaComputerSystem {
[DbaInstanceParameter[]]$ComputerName = $env:COMPUTERNAME,
[PSCredential]$Credential,
[switch]$IncludeAws,
[int]$TimeoutSec = 5,

This comment has been minimized.

@wsmelton

wsmelton Mar 14, 2019

Member

Why does this need to be a parameter? As well, we had it set to 15 previously, if the parameter is to stay in place it needs to be set to 15 seconds.

This comment has been minimized.

@wsmelton

wsmelton Mar 14, 2019

Member

Talking with Chrissy this needs to be removed and implemented as a module configuration.

However from my point of view I see no reason for it. The site you are hitting is a local site in AWS servers only. I believe we can simply set it to a value and leave it.

This comment has been minimized.

@sheldonhull

sheldonhull Mar 15, 2019

Author Contributor

Sounds good to me. thanks!

@wsmelton

This comment has been minimized.

Copy link
Member

wsmelton commented Mar 15, 2019

When the timeout parameter is removed I'm good for this to be merged.

Update Get-DbaComputerSystem.ps1
removed timeout parameter
@wsmelton

This comment has been minimized.

Copy link
Member

wsmelton commented Mar 15, 2019

I have not had time but Azure finally added an REST API on Azure VMs for similar information if you happen to want to tackle that one 😁

@wsmelton wsmelton merged commit 705253c into sqlcollaborative:development Mar 15, 2019

1 check failed

continuous-integration/appveyor/pr AppVeyor build failed
Details
@wsmelton

This comment has been minimized.

Copy link
Member

wsmelton commented Mar 15, 2019

Failure was unrelated on Appveyor

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.
You signed in with another tab or window. Reload to refresh your session. You signed out in another tab or window. Reload to refresh your session.