Fix issues with the retry logic in the OktaApiClient and OktaRetryApi classes (#61) #62

TriggerAu · 2024-11-01T02:56:11Z

Fix issues with the retry logic in the OktaApiClient and OktaRetryApi classes

The OktaApiClient class was not correctly handling a non 200 response code
The OktaRetryApi class was searching for a header field using incorrect case
The OktaRetryApi class was trying to parse a Unix Epoch number as a string date
Also added a verbose message so the user knows how long they waiting for the retry

Root cause was missing a switch on API calls and the retyr logic not matching the Okta API names and data types

Issue #61

…and OktaRetryApi classes * The OktaApiClient class was not correctly handling a non 200 response code * The OktaRetryApi class was searching for a header field using incorrect classes * The OktaRetryApi class was trying to parse a Unix Epoch number as a string date * Also added a verbose message so teh user knows how long they waiting for the retry

laura-rodriguez · 2024-11-04T16:28:25Z

Thank you @TriggerAu!

The team will review and prioritize this issue accordingly.

Internal Ref: OKTA-825949

laura-rodriguez · 2024-11-04T21:10:24Z

Hey @TriggerAu, I was looking into the existing rate limit/retry logic tests we already had to understand why they didn't catch the issue before. Can you confirm if they were failing for you before your changes, and if so, what PowerShell version are you using?

I understand that if the rate limit headers casing were consistently wrong everywhere, tests wouldn't catch the error, but I'm curious about your comment in #61 about the retry code being unreachable due to ErrorAction settings. If I understand this correctly, those tests should fail in your local env because the retry logic was never reached due to fatal errors. Can you please provide more details on how to reproduce the issues you faced so I can revisit the tests and docs regarding $ErrorActionPreference accordingly? Thanks!

laura-rodriguez · 2024-11-04T22:01:34Z

tests/Internal/OktaApiClient.Tests.ps1

        Mock -ModuleName Okta.PowerShell Get-OktaConfiguration { return $Config } -Verifiable

        $Response = [PSCustomObject]@{
            Content = "[]"
            Headers =  @{
                "Date" = @($Now)
                "Content-Type" = @('application/json; charset=utf-8')
-                "X-Rate-Limit-Reset" = @($ResetDate)
+                "x-rate-limit-reset" = @($ResetDateEpoch)


Do we also need to fix the casing (or make the appropriate changes to handle headers in a case-insensitive way) of additional headers such as x-okta-request-id? Is x-rate-limit-reset the only problematic one?

Interesting one, I didn't look at that one because it wasn't erroring - now im questioning my case sensitive testing for sure

That is a great question and I didnt do that one because it didn't throw errors on me. Im now questioning myself about the case behavior - Ill retest it on the weekend

Apologies for the delay, I started in on this and it got weird, but I finally found it

Heres a screenshot of the debugging and some annotations,

So whats happening is that inside Invoke-OptaApiClient the $headers that come back from the Invoke are taken from the rawResponse.Headers. These are a Dictionary and the reference to the Dictionary keyvalue pairs is case insensitive as you can see in the debug checks using various casing in the box that points to 1

When this Dictionary is passed in to CalculateDelay the Dictionary is cast to a Hashtable which behaves in a case sensitive behavior - as you can see in the debug results in the box pointing to 2

So the short answer is the casing re x-okta-request-id works as is because the problem is occuring in the pass of the headers into the CalculateDelay method. I really wish I could figure out why this is the case, but the day job is chewing a lot of time

Thanks for sharing your findings, and we appreciate the time you took to look into this!
I'm currently working on other priorities, but I'll get back to this as soon as possible.

laura-rodriguez · 2024-11-04T22:04:30Z

src/Okta.PowerShell/Private/OktaApiClient.ps1

@@ -163,7 +164,8 @@ function Invoke-OktaApiClient {
                                        -SkipCertificateCheck `
                                        -Proxy $Configuration["Proxy"].GetProxy($UriBuilder.Uri) `
                                        -ProxyUseDefaultCredentials `
-                                        -UserAgent $OktaUserAgent
+                                        -UserAgent $OktaUserAgent `


Considering your comment about IWR not reaching the Retry logic due to ErrorAction = Stop, I'm curious how tests still passed. I left a question for you to help me reproduce the issue.

I gave that a look and the tests pass because the calls to IWR are being Mock'ed out. For example in this block

okta-powershell-cli/tests/Internal/OktaApiClient.Tests.ps1

Lines 9 to 27 in f78b2c6

Mock -ModuleName Okta.PowerShell Get-OktaConfiguration { return $Config } -Verifiable

$Response = [PSCustomObject]@{

Content = "[]"

Headers = @{

"Date" = @($Now)

"Content-Type" = @('application/json; charset=utf-8')

"X-Rate-Limit-Reset" = @($ResetDate)

"X-Okta-Request-Id" = @("foo")

}

StatusCode = 429

}

Mock -ModuleName Okta.PowerShell Invoke-WebRequest {

return $Response

} -Verifiable

$Result = Invoke-OktaListApplications

Assert-MockCalled -ModuleName Okta.PowerShell Invoke-WebRequest -Times 2

Calls to Get-OktaConfiguration and Invoke-WebRequest are replaced by the script blocks in the Mock definition. When Invoke-OktaListApplications is called the inner calls to the two mocked functions run the script blocks not the actual commands so Invoke-WebRequest never gets called or a non-OK status response for IWR to throw an error

I cant think of an "easy" way to unit test IWR getting an actual 429 response thats not overly contrived - eg mock an internal part of that method - so powershells IWR method gets the 429, or call an actual weburl tat forcibly returns 429 - both seem more like integration tests than unit tests though

Makes sense, thanks for clarifying!

laura-rodriguez · 2024-11-04T22:09:42Z

openapi3/codegen-templates/api_client.mustache

@@ -163,7 +163,8 @@ function Invoke-{{{apiNamePrefix}}}ApiClient {
                                        -ErrorAction Stop `
                                        -UseBasicParsing `
                                        -SkipCertificateCheck `
-                                        -UserAgent $OktaUserAgent
+                                        -UserAgent $OktaUserAgent `
+                                        -SkipHttpErrorCheck


Since the SkipHttpErrorCheck param is supported by PS 7+, and the Okta PS module supports PS 6.2+, do you have any thoughts on how this should be handled in PS 6+?

Ah, interesting one, I did look at Powershell 5.1, but didn't look at PS 6 vs 7 consideration apologies.

Off top of my head can think of three ways to handle it

rewrite the code so it does try catches non-200 responses and in the catch it needs logic to separate 429, etc from other errors

adjust it with a case for PS6 vs 7 - probably using try catch in PS^ as theres no other way around that IWR behvior and the new param in 7

Consider moving to PS7 - Im unsure of whether you can see the mix of PSVersions in use from PSGallery though - so that may not be an easy one either (MS did end of support PS6 a whiles ago, but where customers are in their version journey is certainly a consideration)

I agree with these three options. I'm inclined towards 1, but I would also consider number 3. We'll have to revisit the status of previous PS versions starting from 6.2.

TriggerAu · 2024-11-05T08:55:15Z

I am 100% questioning my sanity at this point

Hey @TriggerAu, I was looking into the existing rate limit/retry logic tests we already had to understand why they didn't catch the issue before. Can you confirm if they were failing for you before your changes, and if so, what PowerShell version are you using?

I understand that if the rate limit headers casing were consistently wrong everywhere, tests wouldn't catch the error, but I'm curious about your comment in #61 about the retry code being unreachable due to ErrorAction settings. If I understand this correctly, those tests should fail in your local env because the retry logic was never reached due to fatal errors. Can you please provide more details on how to reproduce the issues you faced so I can revisit the tests and docs regarding $ErrorActionPreference accordingly? Thanks!

Hey Laura, Think I covered these in the code comments the version I'm using is PS7 in the issue report

To create the failure I used the following script - I did try parallelising it to be faster, but moved away from that due to parallel requests causing the api to return resource not found. So I just ran this and wait for it to hit the call limit

This is using the fix in #53 for setting the ApiKey value set

zDTTest.ps1.txt

Write-output "Importing Module..."
import-module ".\src\Okta.PowerShell\" -force

Write-output "Setting Config..."
$configuration = $null
$configuration = Get-OktaConfiguration

Set-OktaConfiguration -BaseUrl "https://INSTANCE.oktapreview.com"
Set-OktaConfiguration -MaxRetries 2
Set-OktaConfiguration -RequestTimeout 30000
Set-OktaConfiguration -ApiKey "BLAHBLAHBLAH"

Write-output "Base URL: $($configuration.BaseUrl)"
Write-output "Max Retries: $($configuration.MaxRetries)"
Write-output "Request Timeout: $($configuration.RequestTimeout)"

write-output "Looping for doom..."
1..150 | foreach-object {
    "Attempt: $_"
    $apps = Invoke-OktaListApplications -Limit 1 -verbose  
}

return

laura-rodriguez · 2024-11-05T15:17:58Z

I am 100% questioning my sanity at this point

Hey @TriggerAu, I was looking into the existing rate limit/retry logic tests we already had to understand why they didn't catch the issue before. Can you confirm if they were failing for you before your changes, and if so, what PowerShell version are you using?
I understand that if the rate limit headers casing were consistently wrong everywhere, tests wouldn't catch the error, but I'm curious about your comment in #61 about the retry code being unreachable due to ErrorAction settings. If I understand this correctly, those tests should fail in your local env because the retry logic was never reached due to fatal errors. Can you please provide more details on how to reproduce the issues you faced so I can revisit the tests and docs regarding $ErrorActionPreference accordingly? Thanks!

Hey Laura, Think I covered these in the code comments the version I'm using is PS7 in the issue report

To create the failure I used the following script - I did try parallelising it to be faster, but moved away from that due to parallel requests causing the api to return resource not found. So I just ran this and wait for it to hit the call limit

This is using the fix in #53 for setting the ApiKey value set

zDTTest.ps1.txt
Write-output "Importing Module..."
import-module ".\src\Okta.PowerShell\" -force

Write-output "Setting Config..."
$configuration = $null
$configuration = Get-OktaConfiguration

Set-OktaConfiguration -BaseUrl "https://INSTANCE.oktapreview.com"
Set-OktaConfiguration -MaxRetries 2
Set-OktaConfiguration -RequestTimeout 30000
Set-OktaConfiguration -ApiKey "BLAHBLAHBLAH"

Write-output "Base URL: $($configuration.BaseUrl)"
Write-output "Max Retries: $($configuration.MaxRetries)"
Write-output "Request Timeout: $($configuration.RequestTimeout)"

write-output "Looping for doom..."
1..150 | foreach-object {
    "Attempt: $_"
    $apps = Invoke-OktaListApplications -Limit 1 -verbose  
}

return

Thank you for providing this sample. I'll also investigate if there's a way to mock an internal IWR and get the same behavior.

TriggerAu · 2025-01-07T07:31:50Z

Added another commit that fixes an issue caused by AddRetryHeaders affecting the pipeline with return values

laura-rodriguez · 2025-01-09T20:06:43Z

Hey @TriggerAu, I just wanted to give you a quick update.

To facilitate testing, I've submitted a PR that added an HttpListener, which will allow us to test the IWR behavior better. As you mentioned in this thread, mocking IWR with Pester prevents us from detecting several issues. In that PR, you will see that I added a unit test that allowed us to verify that, as you described, without the flag -SkipHttpErrorCheck IWR throws so the retry logic is omitted. Once we have that PR approved and merged, we'll revisit your PR.

Again, thanks for all the feedback provided!

TriggerAu added 3 commits November 1, 2024 13:38

Issue okta#61 - Move the fixes to the mustache and openapi3 generation

66d2c05

Issue okta#61 - Correct the UnitTests to match the public API

327d7a0

laura-rodriguez reviewed Nov 4, 2024

View reviewed changes

laura-rodriguez mentioned this pull request Dec 19, 2024

Set-OktaConfiguration doesnt set values if the passed in parameter equates to $false #70

Closed

Suppress output from AddRetryHeaders to prevent pipeline interference

7e8c8ff

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with the retry logic in the OktaApiClient and OktaRetryApi classes (#61) #62

Fix issues with the retry logic in the OktaApiClient and OktaRetryApi classes (#61) #62

TriggerAu commented Nov 1, 2024

laura-rodriguez commented Nov 4, 2024

laura-rodriguez commented Nov 4, 2024 •

edited

Loading

laura-rodriguez Nov 4, 2024

TriggerAu Nov 5, 2024

TriggerAu Nov 5, 2024

TriggerAu Nov 21, 2024

laura-rodriguez Nov 25, 2024

laura-rodriguez Nov 4, 2024

TriggerAu Nov 5, 2024

laura-rodriguez Nov 5, 2024

laura-rodriguez Nov 4, 2024

TriggerAu Nov 5, 2024 •

edited

Loading

laura-rodriguez Nov 5, 2024

TriggerAu commented Nov 5, 2024

laura-rodriguez commented Nov 5, 2024

TriggerAu commented Jan 7, 2025 •

edited

Loading

laura-rodriguez commented Jan 9, 2025

	Mock -ModuleName Okta.PowerShell Get-OktaConfiguration { return $Config } -Verifiable

	$Response = [PSCustomObject]@{
	Content = "[]"
	Headers = @{
	"Date" = @($Now)
	"Content-Type" = @('application/json; charset=utf-8')
	"X-Rate-Limit-Reset" = @($ResetDate)
	"X-Okta-Request-Id" = @("foo")
	}
	StatusCode = 429
	}

	Mock -ModuleName Okta.PowerShell Invoke-WebRequest {
	return $Response
	} -Verifiable

	$Result = Invoke-OktaListApplications
	Assert-MockCalled -ModuleName Okta.PowerShell Invoke-WebRequest -Times 2

Fix issues with the retry logic in the OktaApiClient and OktaRetryApi classes (#61) #62

Are you sure you want to change the base?

Fix issues with the retry logic in the OktaApiClient and OktaRetryApi classes (#61) #62

Conversation

TriggerAu commented Nov 1, 2024

laura-rodriguez commented Nov 4, 2024

laura-rodriguez commented Nov 4, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TriggerAu Nov 5, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TriggerAu commented Nov 5, 2024

laura-rodriguez commented Nov 5, 2024

TriggerAu commented Jan 7, 2025 • edited Loading

laura-rodriguez commented Jan 9, 2025

laura-rodriguez commented Nov 4, 2024 •

edited

Loading

TriggerAu Nov 5, 2024 •

edited

Loading

TriggerAu commented Jan 7, 2025 •

edited

Loading