-
Notifications
You must be signed in to change notification settings - Fork 156
Refactor retry logic and add special behavior for IMDS #960
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
Conversation
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultRetryPolicy.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IMDSRetryPolicy.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultRetryPolicy.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IMDSRetryPolicy.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityRetryPolicy.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ManagedIdentityTests.java
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IMDSRetryPolicy.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IMDSRetryPolicy.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/IMDSRetryPolicy.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/test/java/com/microsoft/aad/msal4j/ManagedIdentityTests.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultRetryPolicy.java
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/HttpHelper.java
Outdated
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/ManagedIdentityRetryPolicy.java
Show resolved
Hide resolved
msal4j-sdk/src/main/java/com/microsoft/aad/msal4j/DefaultRetryPolicy.java
Show resolved
Hide resolved
Consistently use constants for HTTP status codes
| @@ -0,0 +1,25 @@ | |||
| package com.microsoft.aad.msal4j; | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
suggestion: All this HTTP and Retry files should go into a folder of their own?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree that it would be good to group similar files together, but currently all of the source files are just in one big folder. I don't know why it was originally designed that way (the people who would did it are no longer here), and I've always felt it was a bad practice.
I've created an issue to track a refactor of the directory structure, at the very least there's quick value in properly organizing the non-public classes: #965
Add API to disable internal retry behavior
This PR refactors HTTP retry logic by splitting it into classes that implement a new interface, similar to what was done in MSAL Node and .NET:
AzureAD/microsoft-authentication-library-for-js#7614
AzureAD/microsoft-authentication-library-for-dotnet#5231
The following changes were made:
IRetryPolicyinterface, defining methods to determine if a certain HTTP response should be retried, the number of retries to perform, and how long to wait between retriesIRetryPolicy:IMDSRetryPolicy, for behavior specific to IMDSManagedIdentityRetryPolicy, for all other managed identity scenariosDefaultRetryPolicy, for all other scenariosFor testing:
RequestThrottlingTestandManagedIdentityTests.managedIdentityTest_Retry()IMDSRetryPolicybased on status codes, which is now covered by the new unit test inManagedIdentityTestsMost of the "files changed" list in this PR are due to changes in two other PRs:
#960
#964