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.

Sign up for GitHub

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

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

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
a560d16
Issue #61 - Fix issues with the retry logic in the OktaApiClient and …
TriggerAu Nov 1, 2024
66d2c05
Issue #61 - Move the fixes to the mustache and openapi3 generation
TriggerAu Nov 1, 2024
327d7a0
Issue #61 - Correct the UnitTests to match the public API
TriggerAu Nov 1, 2024
7e8c8ff
Suppress output from AddRetryHeaders to prevent pipeline interference
TriggerAu Jan 7, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
18 changes: 13 additions & 5 deletions openapi3/codegen-templates/api_client.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,7 +163,8 @@ function Invoke-{{{apiNamePrefix}}}ApiClient {
-ErrorAction Stop `
-UseBasicParsing `
-SkipCertificateCheck `
-UserAgent $OktaUserAgent
-UserAgent $OktaUserAgent `
-SkipHttpErrorCheck
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@TriggerAu TriggerAu Nov 5, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

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

  1. 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
  2. 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
  3. 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)

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

} else {
# skip certification check, use proxy
$RawResponse = Invoke-WebRequest -Uri $UriBuilder.Uri `
Expand All @@ -175,7 +176,8 @@ function Invoke-{{{apiNamePrefix}}}ApiClient {
-SkipCertificateCheck `
-Proxy $Configuration["Proxy"].GetProxy($UriBuilder.Uri) `
-ProxyUseDefaultCredentials `
-UserAgent $OktaUserAgent
-UserAgent $OktaUserAgent `
-SkipHttpErrorCheck
}
} else {
if ($null -eq $Configuration["Proxy"]) {
Expand All @@ -186,7 +188,8 @@ function Invoke-{{{apiNamePrefix}}}ApiClient {
-Body $RequestBody `
-ErrorAction Stop `
-UseBasicParsing `
-UserAgent $OktaUserAgent
-UserAgent $OktaUserAgent `
-SkipHttpErrorCheck
} else {
# perform certification check, use proxy
$RawResponse = Invoke-WebRequest -Uri $UriBuilder.Uri `
Expand All @@ -197,7 +200,8 @@ function Invoke-{{{apiNamePrefix}}}ApiClient {
-UseBasicParsing `
-Proxy $Configuration["Proxy"].GetProxy($UriBuilder.Uri) `
-ProxyUseDefaultCredentials `
-UserAgent $OktaUserAgent
-UserAgent $OktaUserAgent `
-SkipHttpErrorCheck
}

$Response = $null
Expand All @@ -216,7 +220,11 @@ function Invoke-{{{apiNamePrefix}}}ApiClient {
if ($WaitInMilliseconds -gt 0) {
$RetryCount = $RetryCount + 1
$RequestId = $Headers['X-Okta-Request-Id'][0]
AddRetryHeaders -Headers $HeaderParameters -RequestId $RequestId -RetryCount $RetryCount
# This has to be piped to null as the AddRetryHeaders function has a return that adds to the pipeline
# and messes up the callers that assume they are getting a response object and not an array of headers
# and a response if there is a retry
AddRetryHeaders -Headers $HeaderParameters -RequestId $RequestId -RetryCount $RetryCount | out-null
Write-Verbose "Hit Rate limit: Retrying request after $WaitInMilliseconds milliseconds"
Start-Sleep -Milliseconds $WaitInMilliseconds
}
else {
Expand Down
11 changes: 7 additions & 4 deletions openapi3/codegen-templates/okta_retryApi.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,15 @@ function CalculateDelay {

$Configuration = Get-OktaConfiguration

if ($null -eq $Headers['X-Rate-Limit-Reset']) {
throw "Error! The required header `X-Rate-Limit-Reset` missing when calling CalculateDelay."
if ($null -eq $Headers['x-rate-limit-reset']) {
throw "Error! The required header `x-rate-limit-reset` missing when calling CalculateDelay."
}

$RateLimitReset = Get-Date -Date $Headers["X-Rate-Limit-Reset"][0]
$RetryAtUtcTime = $RateLimitReset.ToUniversalTime()
$RateLimitResetEpoch = $Headers["x-rate-limit-reset"][0]
# this is a unich seconds since epoch time, so we convert to date
$RateLimitResetUTC = New-Object DateTime(1970, 1, 1, 0, 0, 0, 0)
$RateLimitResetUTC = $RateLimitResetUTC.addSeconds($RateLimitResetEpoch)
$RetryAtUtcTime = $RateLimitResetUTC


if ($null -eq $Headers["Date"]) {
Expand Down
11 changes: 7 additions & 4 deletions src/Okta.PowerShell/Client/OktaRetryApi.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -86,12 +86,15 @@ function CalculateDelay {

$Configuration = Get-OktaConfiguration

if ($null -eq $Headers['X-Rate-Limit-Reset']) {
throw "Error! The required header `X-Rate-Limit-Reset` missing when calling CalculateDelay."
if ($null -eq $Headers['x-rate-limit-reset']) {
throw "Error! The required header `x-rate-limit-reset` missing when calling CalculateDelay."
}

$RateLimitReset = Get-Date -Date $Headers["X-Rate-Limit-Reset"][0]
$RetryAtUtcTime = $RateLimitReset.ToUniversalTime()
$RateLimitResetEpoch = $Headers["x-rate-limit-reset"][0]
# this is a unich seconds since epoch time, so we convert to date
$RateLimitResetUTC = New-Object DateTime(1970, 1, 1, 0, 0, 0, 0)
$RateLimitResetUTC = $RateLimitResetUTC.addSeconds($RateLimitResetEpoch)
$RetryAtUtcTime = $RateLimitResetUTC


if ($null -eq $Headers["Date"]) {
Expand Down
18 changes: 13 additions & 5 deletions src/Okta.PowerShell/Private/OktaApiClient.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,8 @@ function Invoke-OktaApiClient {
-ErrorAction Stop `
-UseBasicParsing `
-SkipCertificateCheck `
-UserAgent $OktaUserAgent
-UserAgent $OktaUserAgent `
-SkipHttpErrorCheck
} else {
# skip certification check, use proxy
$RawResponse = Invoke-WebRequest -Uri $UriBuilder.Uri `
Expand All @@ -163,7 +164,8 @@ function Invoke-OktaApiClient {
-SkipCertificateCheck `
-Proxy $Configuration["Proxy"].GetProxy($UriBuilder.Uri) `
-ProxyUseDefaultCredentials `
-UserAgent $OktaUserAgent
-UserAgent $OktaUserAgent `
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

Makes sense, thanks for clarifying!

-SkipHttpErrorCheck
}
} else {
if ($null -eq $Configuration["Proxy"]) {
Expand All @@ -174,7 +176,8 @@ function Invoke-OktaApiClient {
-Body $RequestBody `
-ErrorAction Stop `
-UseBasicParsing `
-UserAgent $OktaUserAgent
-UserAgent $OktaUserAgent `
-SkipHttpErrorCheck
} else {
# perform certification check, use proxy
$RawResponse = Invoke-WebRequest -Uri $UriBuilder.Uri `
Expand All @@ -185,7 +188,8 @@ function Invoke-OktaApiClient {
-UseBasicParsing `
-Proxy $Configuration["Proxy"].GetProxy($UriBuilder.Uri) `
-ProxyUseDefaultCredentials `
-UserAgent $OktaUserAgent
-UserAgent $OktaUserAgent `
-SkipHttpErrorCheck
}

$Response = $null
Expand All @@ -204,7 +208,11 @@ function Invoke-OktaApiClient {
if ($WaitInMilliseconds -gt 0) {
$RetryCount = $RetryCount + 1
$RequestId = $Headers['X-Okta-Request-Id'][0]
AddRetryHeaders -Headers $HeaderParameters -RequestId $RequestId -RetryCount $RetryCount
# This has to be piped to null as the AddRetryHeaders function has a return that adds to the pipeline
# and messes up the callers that assume they are getting a response object and not an array of headers
# and a response if there is a retry
AddRetryHeaders -Headers $HeaderParameters -RequestId $RequestId -RetryCount $RetryCount | out-null
Write-Verbose "Hit Rate limit: Retrying request after $WaitInMilliseconds milliseconds"
Start-Sleep -Milliseconds $WaitInMilliseconds
}
else {
Expand Down
14 changes: 9 additions & 5 deletions tests/Internal/OktaApiClient.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,16 @@ Context 'Invoke-OktaApiClient - 429 Responses' {
$Config.RequestTimeout = $null
$Now = Get-Date # Used as a reference for the test. Indicates when the request was executed
$ResetDate = $Now.AddSeconds(3) # Indicates when one should retry

$ResetDateEpoch = $ResetDate.ToUniversalTime().Subtract((New-Object DateTime(1970, 1, 1, 0, 0, 0, 0))).TotalSeconds

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)
Copy link
Collaborator

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

image

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

"X-Okta-Request-Id" = @("foo")
}
StatusCode = 429
Expand All @@ -33,6 +34,7 @@ Context 'Invoke-OktaApiClient - 429 Responses' {
$Config.RequestTimeout = 100000
$Now = Get-Date # Used as a reference for the test. Indicates when the request was executed
$ResetDate = $Now.AddSeconds(3) # Indicates when one should retry
$ResetDateEpoch = $ResetDate.ToUniversalTime().Subtract((New-Object DateTime(1970, 1, 1, 0, 0, 0, 0))).TotalSeconds

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

Expand All @@ -41,7 +43,7 @@ Context 'Invoke-OktaApiClient - 429 Responses' {
Headers = @{
"Date" = @($Now)
"Content-Type" = @('application/json; charset=utf-8')
"X-Rate-Limit-Reset" = @($ResetDate)
"x-rate-limit-reset" = @($ResetDateEpoch)
"X-Okta-Request-Id" = @("foo")
}
StatusCode = 429
Expand All @@ -61,6 +63,7 @@ Context 'Invoke-OktaApiClient - 429 Responses' {
$Config.RequestTimeout = 1000
$Now = Get-Date # Used as a reference for the test. Indicates when the request was executed
$ResetDate = $Now.AddSeconds(3) # Indicates when one should retry
$ResetDateEpoch = $ResetDate.ToUniversalTime().Subtract((New-Object DateTime(1970, 1, 1, 0, 0, 0, 0))).TotalSeconds

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

Expand All @@ -71,7 +74,7 @@ Context 'Invoke-OktaApiClient - 429 Responses' {
Headers = @{
"Date" = @($Now)
"Content-Type" = @('application/json; charset=utf-8')
"X-Rate-Limit-Reset" = @($ResetDate)
"x-rate-limit-reset" = @($ResetDateEpoch)
"X-Okta-Request-Id" = @("foo")
}
StatusCode = 429
Expand All @@ -92,6 +95,7 @@ Context 'Invoke-OktaApiClient - 429 Responses' {
$Config.RequestTimeout = 1000
$Now = Get-Date # Used as a reference for the test. Indicates when the request was executed
$ResetDate = $Now.AddSeconds(3) # Indicates when one should retry
$ResetDateEpoch = $ResetDate.ToUniversalTime().Subtract((New-Object DateTime(1970, 1, 1, 0, 0, 0, 0))).TotalSeconds

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

Expand All @@ -102,7 +106,7 @@ Context 'Invoke-OktaApiClient - 429 Responses' {
Headers = @{
"Date" = @($Now)
"Content-Type" = @('application/json; charset=utf-8')
"X-Rate-Limit-Reset" = @($ResetDate)
"x-rate-limit-reset" = @($ResetDateEpoch)
"X-Okta-Request-Id" = @("foo")
}
StatusCode = 400
Expand Down
17 changes: 10 additions & 7 deletions tests/Internal/OktaRetryApi.Tests.ps1
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -129,17 +129,19 @@ Describe -tag 'Okta.PowerShell' -name 'OktaRetryApi' {

$Now = Get-Date # Used as a reference for the test. Indicates when the request was executed
$ResetDate = $Now.AddSeconds(3) # Indicates when one should retry

$ResetDateEpoch = $ResetDate.ToUniversalTime().Subtract((New-Object DateTime(1970, 1, 1, 0, 0, 0, 0))).TotalSeconds

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

$Headers = @{"X-Rate-Limit-Reset" = @($ResetDate); "Date" = @($Now)}
$Headers = @{"x-rate-limit-reset" = @($ResetDateEpoch); "Date" = @($Now)}

# $BackoffInMilliseconds = (New-TimeSpan -Start $RequestUtcDate -End $RetryAtUtcTime).Milliseconds + 1000 #delta
$Result = CalculateDelay -Headers $Headers

Assert-MockCalled -ModuleName Okta.PowerShell Get-OktaConfiguration -Times 1

$Result | Should -Be 4000
[int]$intResult = $result
$intResult | Should -Be 4000
}

It 'Should fail if mandatory headers are not provided' {
Expand All @@ -151,17 +153,18 @@ Describe -tag 'Okta.PowerShell' -name 'OktaRetryApi' {

$Now = Get-Date # Used as a reference for the test. Indicates when the request was executed
$ResetDate = $Now.AddSeconds(3) # Indicates when one should retry

$ResetDateEpoch = $ResetDate.ToUniversalTime().Subtract((New-Object DateTime(1970, 1, 1, 0, 0, 0, 0))).TotalSeconds

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

$Headers = @{"Date" = @($Now)} # "X-Rate-Limit-Reset" = $ResetDate; not included
$Headers = @{"Date" = @($Now)} # "x-rate-limit-reset" = $ResetDate; not included

# $BackoffInMilliseconds = (New-TimeSpan -Start $RequestUtcDate -End $RetryAtUtcTime).Milliseconds + 1000 #delta
{ CalculateDelay -Headers $Headers } | Should -Throw -ExpectedMessage "Error! The required header `X-Rate-Limit-Reset` missing when calling CalculateDelay."
{ CalculateDelay -Headers $Headers } | Should -Throw -ExpectedMessage "Error! The required header `x-rate-limit-reset` missing when calling CalculateDelay."

Assert-MockCalled -ModuleName Okta.PowerShell Get-OktaConfiguration -Times 1

$Headers = @{ "X-Rate-Limit-Reset" = @($ResetDate)} # "Date" = $Now; not included
$Headers = @{ "x-rate-limit-reset" = @($ResetDateEpoch)} # "Date" = $Now; not included

# $BackoffInMilliseconds = (New-TimeSpan -Start $RequestUtcDate -End $RetryAtUtcTime).Milliseconds + 1000 #delta
{ CalculateDelay -Headers $Headers } | Should -Throw -ExpectedMessage "Error! The required header `Date` missing when calling CalculateDelay."
Expand Down