search

Venafi / VenafiPS

Powershell module to fully automate your Venafi TLS Protect Datacenter and Cloud platforms!
https://venafips.readthedocs.io/
Apache License 2.0
17 stars 8 forks source link

Allow specifying a Timeout value #240

Closed gdbarron closed 4 months ago

gdbarron commented 7 months ago

closes #237

Saadi6 commented 7 months ago

comments submitted in review on GitHub

gdbarron commented 7 months ago

If someone uses a low timeout without error handling and an incomplete session is created, why doesn't the first call post New-VenafiSession fail? I would think it would just as the calls to get version and CF info did. Also, I would expect there to be errors for failure to retrieve version and CF, those are just not terminating errors so New-VenafiSession completes and does not stop the script.

Trying to understand the suggestion a bit deeper. Is it to update Invoke-VenafiRestMethod to throw a terminating error whenever an api call fails, or just fails on timeout, or not terminating? Or is the suggestion isolated to New-VenafiSession? Or something else?

gdbarron commented 5 months ago

@Saadi6?

Saadi6 commented 5 months ago

Excuse the late response. Without error handling while calling New-VenafiSession with a very short timeout value a terminating error does not occur, even though some of the background HTTP calls made by VenafiPS (such as gathering custom fields) while creating the session object did timeout.

Wrapping New-VenafiSession in try catch statement - as one should - throws an error when any background HTTP request times out and an incomplete, yet semi-useable VenafiSession is not returned.

Yes, if possible, Invoke-VenafiRestMethod can be updated to throw a terminating error only incase of timeout error. There might other implications if terminating error is thrown for all types of error by Invoke-VenafiRestMethod.

However, given that you don't know someone might use VenafiPS in their own code, just a warning/suggestion to always use error handling - when a timeout value is specified - would also be sufficient IMHO.