dataplat
Update-DbaBuildReference - Add Parameter Proxy
Add optional parameters 'Proxy' and 'ProxyCredential'
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.
- [x] 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 # )
- [x] New feature (non-breaking change, adds functionality, fixes #9172)
- [ ] 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
Allows supplying a proxy as parameter for Update-DbaBuildReference
Approach
Parameters "Proxy" and "ProxyCredential" are passed to function Get-DbaBuildReferenceIndexOnline, which calls Invoke-TlsWebRequest that in turn forwards the parameters to Invoke-WebRequest.
Commands to test
Update-DbaBuildReference -Proxy $ProxyURI
Update-DbaBuildReference -Proxy $ProxyURI -ProxyCredential $ProxyCred
Learning
- Discussion regarding this topic #9172
@fm-knopfler doubtful it'll work till also https://github.com/dataplat/dbatools/blob/development/private/functions/Invoke-TlsWebRequest.ps1 will get the addition. I don't understand why the parameterset though .... you may have a proxy and not wanting to pass a credential. I'd design them the same way they are on Invoke-RestMethod or Invoke-WebRequest (and, there's a nifty -ProxyUseDefaultCredentials, too)
Also, you may need to update tests (but lets get to them after the "code" part of the PR is ironed out)
@fm-knopfler doubtful it'll work till also https://github.com/dataplat/dbatools/blob/development/private/functions/Invoke-TlsWebRequest.ps1 will get the addition. I don't understand why the parameterset though .... you may have a proxy and not wanting to pass a credential. I'd design them the same way they are on Invoke-RestMethod or Invoke-WebRequest (and, there's a nifty -ProxyUseDefaultCredentials, too)
Also, you may need to update tests (but lets get to them after the "code" part of the PR is ironed out)
I don't belive any changes to Invoke-TlsWebRequest are necessary, as it passes all arguments to Invoke-WebRequest.
Invoke-WebRequest @Args
The parameterset is, as you assumed, designed to be able to accept a proxy URI without a credential, but not a credential without a proxy URI. Would you design the paramblock like this?
[CmdletBinding(DefaultParameterSetName = 'Build')]
param (
[string]$LocalFile,
[switch]$EnableException,
[Parameter(ParameterSetName = 'Build')]
[Parameter(Mandatory, ParameterSetName = 'Proxy')]
[Parameter(ParameterSetName = 'ProxyDefaultCredential')]
[URI]$Proxy,
[Parameter(ParameterSetName = 'Proxy')]
[pscredential]$ProxyCredential,
[Parameter(ParameterSetName = 'ProxyDefaultCredential')]
[switch]$ProxyUseDefaultCredentials
)
right, I forgot about the splat. I'd add -ProxyUseDefaultCredentials nonetheless then.
right, I forgot about the splat. I'd add -ProxyUseDefaultCredentials nonetheless then.
For that, I assume, a change to Invoke-TlsWebRequest would be necessary, as the parameters provided to Invoke-WebRequest are positional, which doesn't work when calling it the following way.
Invoke-TlsWebRequest $url -Proxy:$Proxy -ProxyCredential:$ProxyCredential -ProxyUseDefaultCredentials:$ProxyUseDefaultCredentials -UseBasicParsing -ErrorAction Stop
I'm trying to anticipate what will happen for sure: once we give out the possibility to use proxies, someone will come up asking for -ProxyUseDefaultCredentials ;-) . That being said, isn't @Args a proper hash (hence not positional) ?
No, $args is automatically created as an array, I'll try to figure out a solution using $PSCmdlet.MyInvocation.BoundParameters.
mmmhhh, it's @Args, not $args . if it doesn't work imho a clean solution is creating a splat as an hash within the caller and use it instead of positional params
i.e. instead of
$webContent = Invoke-TlsWebRequest $url -Proxy:$Proxy -ProxyCredential:$ProxyCredential -UseBasicParsing -ErrorAction Stop
doing this
$splat = @{
Proxy=$Proxy ,
...
}
$webContent = Invoke-TlsWebRequest @splat
Invoke-TlsWebRequest doesn't accept named parameters, because it doesn't have a param block. Calling the function using splatting will not make a difference to the $args or @args variable, as this is just an array with supplied unnamed (positional) parameters. To use named parameters, a param block within Invoke-TlsWebRequest is required. I'm not sure that would be desirable.
I have implemented -ProxyUseDefaultCredentials, but the logic is handled in Update-DbaBuildReference to not conflict with the use of unnamed parameters in Invoke-TlsWebRequest. Maybe this is an option to consider.
so splatting has issues with switches. As long as Invoke-TlsWebRequest doesn't end up being an advanced function (cmdletbinding) am I missing something here ?
function Invoke-TlsWebRequest {
Param ([switch]$UseBasicParsing)
<#
Internal utility that mimics invoke-webrequest
but enables all tls available version
rather than the default, which on a lot
of standard installations is just TLS 1.0
#>
$currentVersionTls = [Net.ServicePointManager]::SecurityProtocol
$currentSupportableTls = [Math]::Max($currentVersionTls.value__, [Net.SecurityProtocolType]::Tls.value__)
$availableTls = [enum]::GetValues('Net.SecurityProtocolType') | Where-Object { $_ -gt $currentSupportableTls }
$availableTls | ForEach-Object {
[Net.ServicePointManager]::SecurityProtocol = [Net.ServicePointManager]::SecurityProtocol -bor $_
}
if ($PSBoundParameters.UseBasicParsing)
{
Invoke-WebRequest @Args -UseBasicParsing
}
else {
Invoke-WebRequest @Args
}
[Net.ServicePointManager]::SecurityProtocol = $currentVersionTls
}
$headers = @{
'User-Agent'= 'dbatools'
}
$paramHash = @{
Uri = "https://httpbin.org/anything"
Headers = $headers
UseBasicParsing = $true
}
$req = Invoke-TlsWebRequest @paramHash
$req.Content
this seems to work, and UseBasicParsing should be the same "type" of -ProxyUseDefaultCredentials ... so switching the check on $PSBoundParameters should end up fine ?
@fm-knopfler : do you need some help polishing this PR ?
Hi @niphlod , that would be very helpful, thanks a lot. Epecially the tests I am not familiar with.
okay @fm-knopfler , I'll be available in some hours. First things first, though .... I added the example for UseBasicParsing on Invoke-TlsWebRequest just to show how to deal with switches, I'd expected the same kind of deal with ProxyUseDefaultCredentials so it doesn't have to be treated in each function like the way you did on Update-DbaBuildReference and it can be passed down with the splat. Did you try that ? Unfortunately I don't have an env with proxies so I must rely on you to know if it works or not ;-)
I can fix the tests without your help later.
I'm not sure I undestand this correctly. Do you intend to add every parameter of Invoke-WebRequest to Invoke-TlsWebRequest or just the switches UseBasicParsing and ProxyUseDefaultCredentials?
just the needed ones, and I guess those are the only needed at the moment.... unless I'm missing something.
After some investigation, I found using positional parameters via $args to be unreliable at best. I think we'll need to have all available or at least all required parameters of Invoke-WebRequest as named parameters.
function Invoke-TlsWebRequest {
Param (
[switch]$UseBasicParsing,
[switch]$ProxyUseDefaultCredentials
)
<#
Internal utility that mimics invoke-webrequest
but enables all tls available version
rather than the default, which on a lot
of standard installations is just TLS 1.0
#>
$currentVersionTls = [Net.ServicePointManager]::SecurityProtocol
$currentSupportableTls = [Math]::Max($currentVersionTls.value__, [Net.SecurityProtocolType]::Tls.value__)
$availableTls = [enum]::GetValues('Net.SecurityProtocolType') | Where-Object { $_ -gt $currentSupportableTls }
$availableTls | ForEach-Object {
[Net.ServicePointManager]::SecurityProtocol = [Net.ServicePointManager]::SecurityProtocol -bor $_
}
Invoke-WebRequest @Args -UseBasicParsing:$UseBasicParsing -ProxyUseDefaultCredentials:$ProxyUseDefaultCredentials
[Net.ServicePointManager]::SecurityProtocol = $currentVersionTls
}
# Function Call
$param = @{
"Uri" = "https://dataplat.github.io/assets/dbatools-buildref-index.json"
"Proxy" = "http://yourproxy:9000"
"UseBasicParsing" = $true
}
Invoke-TlsWebRequest @param
if that works I'm not against it. it now needs to be wired correctly with update-dbabuildreference and then the "code" part should be okay.... and you should be able to use your proxy. For the tests, as anticipated, I can chime in once the code works.
soooo, few things to account for here ... sorry @fm-knopfler , it seems you triggered a long-standing bug in how we check for help parameters in dbatools. Be a little patient while we figure it out.
Let's split "jobs"...
1. Code related
@fm-knopfler , can you try the latest version I pushed which I refactored a bit to see if the code works with proxies ?
2. Tests related
I posted the same in dbatools-dev, but I need a hand from @potatoqualitee , maybe @wsmelton too (but anyone that can chime in is welcome). For good measure, and since we'll soon discover New-DbaDbUser is probably in the need of an adjustment, @andreasjordan ^_^ Sooooo. Within InModule.Help.Tests we check if the outputted help matches with the parameter definition in various ways. One of those that this modification to Update-DbaBuildReference introduced is that "Proxy" is a Required parameter (but not on the default parameterset) AND the help doesn't show the parameter as required. This is (I guessed) because Get-Help builds params from the default parameterset, and we do a list without looking if the parameterset is the default or not. June, which we originally based a lot of the code upon, has a solution already https://github.com/juneb/PesterTDD/blob/main/InModule.Help.Tests.ps1 which I introduced here, too. It lists parameters looking at parametersets too, and this seems to fix the "issue" on this new Update-DbaBuildReference. Issue now is that a problem is now found within another command
Describe : Test help for New-DbaDbUser
Context : Test parameter help for New-DbaDbUser
Name : It help for Username parameter in New-DbaDbUser has correct Mandatory value
Result : Failed
Message : Expected strings to be the same, but they were different.
Expected length: 4
Actual length: 5
Strings differ at index 0.
Expected: 'True'
But was: 'false'
-----------^
Frankly, I think June's code is good, but I fail to see the error on New-DbaDbUser's param declaration. So ... it's either one of those:
- it's late and I need to pause the brain
- looking at params the way June's code does is wrong (which, in turn, deems that the way Update-DbaBuildReference params are declared wrongly)
- looking at params the way June's code does is good and New-DbaDbUser params are declared wrongly
Of course we can always use InModule.Help.Exceptions.ps1 but I'd like to cross it out and figure who's wrong ^_^
I'm very busy the next days, but I can try to have a look at the weekend.
In this commit, I declared the parameters in Invoke-TlsWebRequest to avoid passing any positional parameters to Invoke-WebRequest.
Yes, you can reproduce it by calling the function like this.
$params = @{
"UseBasiParsing" = $true
"Proxy" = "http://yourproxy:9000"
"Uri" = "https://dataplat.github.io/assets/dbatools-buildref-index.json"
}
Invoke-TlsWebRequest @params
By using @args, an array, not a hashtable, is used for parameter splatting. All unbound (those not specified in the function) parameters are available in that array. Powershell will in most cases fail to correctly assign these positional parameters.
Invoke-TlsWebRequest:
Line |
7 | Invoke-TlsWebRequest @params
| ~~~~~~~
| Cannot process argument transformation on parameter 'ProxyUseDefaultCredentials'. Cannot convert value "System.String" to type "System.Management.Automation.SwitchParameter". Boolean parameters
accept only Boolean values and numbers, such as $True, $False, 1 or 0.
this works without issues ....
function Invoke-TlsWebRequest {
Param (
[switch]$UseBasicParsing,
[switch]$ProxyUseDefaultCredentials
)
<#
Internal utility that mimics invoke-webrequest
but enables all tls available version
rather than the default, which on a lot
of standard installations is just TLS 1.0
#>
$currentVersionTls = [Net.ServicePointManager]::SecurityProtocol
$currentSupportableTls = [Math]::Max($currentVersionTls.value__, [Net.SecurityProtocolType]::Tls.value__)
$availableTls = [enum]::GetValues('Net.SecurityProtocolType') | Where-Object { $_ -gt $currentSupportableTls }
$availableTls | ForEach-Object {
[Net.ServicePointManager]::SecurityProtocol = [Net.ServicePointManager]::SecurityProtocol -bor $_
}
Invoke-WebRequest @Args -UseBasicParsing:$UseBasicParsing -ProxyUseDefaultCredentials:$ProxyUseDefaultCredentials
[Net.ServicePointManager]::SecurityProtocol = $currentVersionTls
}
$params = @{
"UseBasicParsing" = $true
"Uri" = "https://dataplat.github.io/assets/dbatools-buildref-index.json"
}
Invoke-TlsWebRequest @params
what am I not getting ?
You haven't declared the proxy parameter, this way, it works for me too. To test, use some random URI as proxy, as Invoke-WebRequest will not successfully execute anyways. Copy this and run it, I expect you to get the same result.
$params = @{
"UseBasicParsing" = $true
"Uri" = "https://dataplat.github.io/assets/dbatools-buildref-index.json"
"Proxy" = "http://proxyurl:9000"
}
Invoke-TlsWebRequest @params
Maybe someone knows a more concise way to make this work, but I wasn't able to find one.
hey everyone, sorry for being an absent maintainer. I'll be back next year.
I think we could use $env: vars or $PSDefaultParameterValues. That's how I do it when I want to access internal commands.
My $0.02USD here...
So, I've only skimmed over the extremely long conversation on this PR; would be better served to convert that discussion to an issue for better tracking of what is going on...just an FYI.
I believe it would be better served by having the proxy for web access stored inside of a configuration item. Then simply have the wrapper function pass that value to Invoke-WebRequest. Be aware that this cmdlet changes behavior between Windows PowerShell and PowerShell 7.4+ (latest LTS release by the way).
@wsmelton, @potatoqualitee : I'm up for rewriting a summary of what's happening . Do you want me to "split" this into the two/three main arguments in separate issues ? Code, though, is going to need merging into development (if we wanna go through with the current implementation) as one .
Issue 1:
Users in the need of using a proxy can either:
- pass that proxy (and credentials) to any supporting cmdlet (e.g. done here as first attempt, Update-DbaBuildReference)
- we introduce another configuration item for the proxy (and, optionally, credentials to use) so any interaction with invoke-webrequest/invoke-restmethod will use it (through the wrappers). Problem here may be that a global config will force every cmdlet to use the proxy, opposed to a somewhat more flexible parameter passed on each cmdlet invocation.
Going with 2. means simplifying this PR, potentially just adding onto Update-DbaBuildReference (and later on to all invoking cmdlets) a warning in case of failures that remembers what config "key" to use to supply proxy and credentials. Wrappers then may remain simpler and use the config to know if a proxy (and credentials) are needed .
Issue 2:
If we don't go for a global config, problem here is that we had a simple wrapper to enable TLS1.2 for older PS versions that hit a limit. Either we remove the wrapper or we "complicate it" doing what @fm-knopfler did to make it work. I'm unaware of simpler ways to make wrappers (simpler meaning a way where declaring all parameters isn't needed). Also, I'm unaware of behaviour changes between windows powershell and 7.4 ... care to elaborate ? If it's something like additional params that aren't cross-plat or compatible, shouldn't be a problem... our cmdlets use a very restricted number of parameters: here @fm-knopfler did "the homework" and straight up reimplemented every possible parameter, but technically we just need to define in the wrapper what we specifically use in the callers.
Issue 3 (somewhat unrelated, but still valid):
Solution 1 (having proxy and credential parameters on the cmdlet itself) create a 100% good parameterset, that is wrongly "marked as failed" by our unittests. This is due to what I consider a bug, and I found a fix, although fixing the logic shows that New-DbaDbUser has an incorrect definition of parameters.