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

Change the default value for the authority host option to be read from the environment variable first. #4980

Merged
merged 4 commits into from
Oct 5, 2023

Conversation

ahsonkhan
Copy link
Member

Fixes #4875

Ultimately, this isn't a breaking change, but rather a bug fix to match user intention:

  1. If the user set the option before to a particular value, it continues to get honored (since that is the highest precedence)
  2. If the user doesn't set the option, and was running in an environment where the environment variable wasn't set, we previously fell back to the default authority (which is the public cloud), and we continue to do so, since the env var value will be empty.
  3. If the user doesn't set the option, and was running in an environment where the environment variable was set, we now correctly use that value over the fallback default authority. That's the bug fix here.

If, for some reason, the user really wanted the previous behavior, and ignore the environment variable set on the environment, they can explicitly set the AuthorityHost option to the desired value. This way the intent is explicit and clear, rather than implicit/accidental.

@joshfree
Copy link
Member

Added Discuss in Office Hours

Copy link
Member

@antkmsft antkmsft left a comment

Choose a reason for hiding this comment

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

Can you please cover the "read-from-the-environment" behavior with tests?
See CredentialTestHelper::EnvironmentOverride.

And if you permanently keep "" = public cloud, then you probably also need some tests via CredentialTestHelper::SimulateTokenRequest, that the request URL is the authority URL.
Otherwise, I think, just creating the options and making sure the AuthorityHost matches the env variable, is sufficient (each credential / common credential impl already has tests verifying that the URL of the request being made does match the authority URL supplied via options).

Copy link
Member

@antkmsft antkmsft left a comment

Choose a reason for hiding this comment

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

I am approving with an assumption that #5002 will be addressed before the release and the "get option value from the environment" tests will be written. I don't mind if that comes in subsequent PRs. If that is not what you're planning to do, I'd like to talk more.

Copy link
Member

@antkmsft antkmsft left a comment

Choose a reason for hiding this comment

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

I think you're unintentionally introducing a regression in WorkloadIdentityCredential (see comment for workload_identity_credential.cpp below)

@ahsonkhan ahsonkhan merged commit 81d95c9 into Azure:main Oct 5, 2023
71 checks passed
@ahsonkhan ahsonkhan deleted the AuthorityHostEnvVar branch October 5, 2023 18:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
4 participants