Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Reginal endpoint support #358
Reginal endpoint support #358
Changes from 3 commits
9052ef7
c2e9899
b0f3299
46f7592
84fca2d
8422ea3
ab45659
177bf28
f7ec1e4
6876820
080d995
6053380
babe142
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
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.
imo, we should have dedicated http client with a shorter timeout for this.
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.
Having 2 different http clients (one dedicated for region detection and one for the rest), would complicate not just MSAL's internal implementation, but also the API model, which would then propagate to our app developer's implementation. Yet the gain is debatable.
When an http_client has short timeout (say, 2 seconds x 2 retries = 4 seconds latency), those seemingly short latency might go unnoticed, and become a perpetual behavior for that app. (If that app happens to be a command-line app such as Azure CLI, it would mean each "az ..." would have 4+ seconds extra latency.)
The current approach would have a longer latency by default. But hopefully this "fail early, fail loudly" approach would lead customer to a better solution: to make their "region" setting configurable, so that it could (and should) be completely turned off by an on-site configuration, when their app is deployed outside of Azure VMs.
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.
This is not a http client dedicated for region, but a client dedicated to fetch data from the imds endpoint. Primary reason is the timeout setting from above.
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.
Yes, usually imds call fails fast but with the default retry logic it was taking longer to fail making the overall response time longer. So it was decided to set timeout to 2 sec for the imds call specifically with 1 retry.
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.
My understanding is: (1) if your app is running inside Azure VM (or Azure Function etc., for that matter), the detection is a local http call which would be quick (even quicker than other cross-machine http requests), so customizing timeout would be unnecessary; (2) if your app is running outside of Azure infrastructure, you wouldn't want to waste some latency on a meaningless region detection.
The default longer timeout would cause app developer in scenario No.2 to notice the hanging during their smoke testing, which would then lead them to somehow add a per-deployment flag to opt out of the region behavior when/if their app is not deployed on Azure. After that, their app would be more performant when deployed to production.
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.
@rayluo agree that there is a need to set the timeout specifically for the imds call to 2 seconds.