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.

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

Merged
merged 13 commits into from
May 18, 2021
Merged

Reginal endpoint support #358

merged 13 commits into from
May 18, 2021

Conversation

rayluo
Copy link
Collaborator

@rayluo rayluo commented May 6, 2021

This resolves #295.

Feature wise this is completed.

TODO: Currently the authority validation is still performed. Would need to decide whether that is desirable. CC: @bgavrilMS Now MSAL will ignore connection error during Authority Validation if app developer opted in to use azure_region.

msal/application.py Outdated Show resolved Hide resolved
``ClientApplication.ATTEMPT_REGION_DISCOVERY`` to auto-detect region.
(Attempting this on a non-VM could hang indefinitely.
Make sure you configure a short timeout,
or provide a custom http_client which has a short timeout.

Choose a reason for hiding this comment

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

http_client

imo, we should have dedicated http client with a shorter timeout for this.

Copy link
Collaborator Author

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.

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.

Copy link

@neha-bhargava neha-bhargava May 10, 2021

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.

Copy link
Collaborator Author

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.

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.

msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
msal/application.py Outdated Show resolved Hide resolved
Copy link
Collaborator Author

@rayluo rayluo left a comment

Choose a reason for hiding this comment

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

Responded to some review comments. A new commit is on the way.

msal/application.py Outdated Show resolved Hide resolved
``ClientApplication.ATTEMPT_REGION_DISCOVERY`` to auto-detect region.
(Attempting this on a non-VM could hang indefinitely.
Make sure you configure a short timeout,
or provide a custom http_client which has a short timeout.
Copy link
Collaborator Author

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.

msal/application.py Show resolved Hide resolved
msal/region.py Show resolved Hide resolved
msal/region.py Show resolved Hide resolved
msal/region.py Outdated Show resolved Hide resolved
@rayluo rayluo force-pushed the reginal-endpoint-experiment branch from 451428d to f7ec1e4 Compare May 10, 2021 23:42
@rayluo rayluo marked this pull request as ready for review May 11, 2021 01:21
@rayluo rayluo force-pushed the reginal-endpoint-experiment branch 3 times, most recently from 991c046 to 35fe256 Compare May 18, 2021 21:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feature: Enable regional support
4 participants