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

Update-DbaBuildReference - Add Parameter Proxy #9174

Open
wants to merge 14 commits into
base: development
Choose a base branch
from

Conversation

fm-knopfler
Copy link

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.

  • 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 # )
  • New feature (non-breaking change, adds functionality, fixes Update-DbaBuildReference: Supply Proxy via Parameter #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

Add optional parameters 'Proxy' and 'ProxyCredential'
@niphlod
Copy link
Contributor

niphlod commented Nov 29, 2023

@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
Copy link
Author

@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
)

@niphlod
Copy link
Contributor

niphlod commented Nov 29, 2023

right, I forgot about the splat. I'd add -ProxyUseDefaultCredentials nonetheless then.

@fm-knopfler
Copy link
Author

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

@niphlod
Copy link
Contributor

niphlod commented Nov 29, 2023

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) ?

@fm-knopfler
Copy link
Author

No, $args is automatically created as an array, I'll try to figure out a solution using $PSCmdlet.MyInvocation.BoundParameters.

@niphlod
Copy link
Contributor

niphlod commented Nov 29, 2023

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 

@fm-knopfler
Copy link
Author

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.

Implemented Parameter ProxyUseDefaultCredentials
@fm-knopfler
Copy link
Author

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.

@niphlod
Copy link
Contributor

niphlod commented Nov 30, 2023

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 ?

named parameter UseBasicParsing ensures mapping of unnamed parameters
@niphlod
Copy link
Contributor

niphlod commented Dec 11, 2023

@fm-knopfler : do you need some help polishing this PR ?

@fm-knopfler
Copy link
Author

@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.

@niphlod
Copy link
Contributor

niphlod commented Dec 12, 2023

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.

@fm-knopfler
Copy link
Author

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?

@niphlod
Copy link
Contributor

niphlod commented Dec 12, 2023

just the needed ones, and I guess those are the only needed at the moment.... unless I'm missing something.

@fm-knopfler
Copy link
Author

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

@niphlod
Copy link
Contributor

niphlod commented Dec 12, 2023

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.

@niphlod
Copy link
Contributor

niphlod commented Dec 12, 2023

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 ^_^

@andreasjordan
Copy link
Contributor

I'm very busy the next days, but I can try to have a look at the weekend.

using namend parameters and pass these to Invoke-WebRequest
@fm-knopfler
Copy link
Author

In this commit, I declared the parameters in Invoke-TlsWebRequest to avoid passing any positional parameters to Invoke-WebRequest.

@niphlod
Copy link
Contributor

niphlod commented Dec 13, 2023

@fm-knopfler : were there issues with the previous implementation or not ?

@fm-knopfler
Copy link
Author

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.

@niphlod
Copy link
Contributor

niphlod commented Dec 13, 2023

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 ?

@fm-knopfler
Copy link
Author

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

@niphlod
Copy link
Contributor

niphlod commented Dec 13, 2023

owwww shush, I was aiming at keeping it simple but .... okay.

@fm-knopfler
Copy link
Author

Maybe someone knows a more concise way to make this work, but I wasn't able to find one.

@niphlod
Copy link
Contributor

niphlod commented Dec 13, 2023

yeah, no worries and thanks for your patience.

@potatoqualitee
Copy link
Member

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.

@wsmelton
Copy link
Member

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).

@niphlod
Copy link
Contributor

niphlod commented Dec 29, 2023

@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:

  1. pass that proxy (and credentials) to any supporting cmdlet (e.g. done here as first attempt, Update-DbaBuildReference)
  2. 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.

@potatoqualitee
Copy link
Member

Yes, I believe this can be simplified with using dbatools' config in lieu of updating a single command plus internal commands. That way, setting the proxy can help all other commands.

@fm-knopfler
Copy link
Author

You mean adding an option to dbatools global config and referencing it in this (and possibly other) function?

@potatoqualitee
Copy link
Member

yep! then set the proxy within the internal command.

@fm-knopfler
Copy link
Author

Are there already predfined names for proxy and proxycredentials?

Set-dbatoolsconfig -FullName network.proxy -Value $proxyUrl
Set-dbatoolsconfig -FullName network.proxycredential -Value $proxyCred

@potatoqualitee
Copy link
Member

Just looked and this is indeed a new category. Your proposal works! If you have issues with the credential, you can also store the username and SecureString then build up the $Cred in Invoke-TlsWebRequest like:

$username = Get-DbatoolsConfigValue -FullName network.proxy.username
$securePassword = Get-DbatoolsConfigValue -FullName network.proxy.securepassword

$credential = New-Object System.Management.Automation.PSCredential ($username, $securePassword)

To set that securepassword, i usually do like (Get-Credential).Password

@fm-knopfler
Copy link
Author

I got a version of it working, please have a look.

@potatoqualitee
Copy link
Member

Looks great. These tests are..i dunno whats going on with them but in the interest of time, I'd like to skip the param test here if it's cool @niphlod @wsmelton. gonna commit a couple changes

@potatoqualitee
Copy link
Member

okay this is great progress. but i think, and what do you say @niphlod, this should be in Invoke-TlsWebRequest itself. that way, if someone sets a proxy, then it works across the board and not just for one command.

i also added the initialization scripts 😊

@potatoqualitee potatoqualitee marked this pull request as ready for review May 16, 2024 16:09
@@ -0,0 +1,3 @@
Set-DbatoolsConfig -FullName network.proxy.url -Value $null -Initialize -Validation string -Description "The URL of the network proxy."
Set-DbatoolsConfig -FullName network.proxy.username -Value $null -Initialize -Validation string -Description "The username for the network proxy."
Set-DbatoolsConfig -FullName network.proxy.securepassword -Value $null -Initialize -Validation securestring -Description "The secure password for the network proxy."
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not sure I've ever tried but is there any reason we couldn't just squash these two into a config that holds the credential object itself?

Copy link
Member

@wsmelton wsmelton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, moving the content into one command is best starting point for us so we don't have to replicate this code around.

Not sure if it would be possible to actually have it do the secondary attempts as well, i.e. what the try/catch in this command does

if ($proxyUsername) {
$proxyCredential = New-Object System.Management.Automation.PSCredential ($proxyUsername, $proxySecurePassword)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I'm more for this being embedded into Invoke-TlsWebRequest instead of having to copy this everywhere

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 this pull request may close these issues.

None yet

5 participants