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

@azure/identity throw environmental credetials exception on constructor #33305

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AndreaBeggiato
Copy link

@AndreaBeggiato AndreaBeggiato commented Mar 7, 2025

Packages impacted by this PR

@azure/identity

Issues associated with this PR

Describe the problem that is addressed by this PR

EnviromentalCredential never throws an exception in constructor, but always throw exception in getToken inside a tracingClient.withSpan (https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity/src/credentials/environmentCredential.ts#L151-L168)

When using DefaultCredentials the first try to getToken is EnvironmentalCredential and (e.g in a Managed identity environment) this will throw an exception that will be recorded in the tracing. Our trace is full of errors because of that (see attachment)

What are the possible designs available to address the problem? If there are more than one possible design, why was the one in this PR chosen?

Suggested approach will fix the issue, but a better approach IMHO should be to add an isAvailable method to TokenCredential interface and in DefaultAzureCredentials constructor (https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/identity/identity/src/credentials/defaultAzureCredential.ts#L235-L262) use this new method to conditionally add the credentials to ChainedTokenCredential constructor.

The issue exists also in the other ChainedTokenCredential subclasses

Are there test cases added in this PR? (If not, why?)

  • Yes

Command used to generate this PR:**(Applicable only to SDK release request PRs)

Checklists

  • [ X] Added impacted package name to the issue description
  • Does this PR needs any fixes in the SDK Generator?** (If so, create an Issue in the Autorest/typescript repository and link it here)
  • Added a changelog (if necessary)
image

Copy link

github-actions bot commented Mar 7, 2025

Thank you for your contribution @AndreaBeggiato! We will review the pull request and get back to you soon.

@github-actions github-actions bot added Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization. labels Mar 7, 2025
@AndreaBeggiato AndreaBeggiato changed the title fix: throw environmental credetials exception on constructor @azure/identity throw environmental credetials exception on constructor Mar 7, 2025
@AndreaBeggiato
Copy link
Author

AndreaBeggiato commented Mar 7, 2025 via email

@azure-sdk
Copy link
Collaborator

API change check

API changes are not detected in this pull request.

}

throw new CredentialUnavailableError(
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
throw new CredentialUnavailableError(
if(!this._credential)
throw new CredentialUnavailableError(

@@ -138,7 +138,12 @@ export class EnvironmentCredential implements TokenCredential {
password,
newOptions,
);
return;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return;

* A no-op credential that logs the reason it was skipped if getToken is called.
* @internal
*/
export class UnavailableAzureApplicationCredential implements TokenCredential {
Copy link
Member

Choose a reason for hiding this comment

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

We are discussing this design internally, so we'll hold off making this change for now.

@AndreaBeggiato
Copy link
Author

AndreaBeggiato commented Mar 11, 2025

There are a lot of other place where the same pattern is followed (expecting an exception inside a tracer.withSpan on purpose)

e.g. https://github.com/Azure/azure-sdk-for-js/blob/main/sdk/storage/storage-blob/src/ContainerClient.ts#L768-L795

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Azure.Identity Community Contribution Community members are working on the issue customer-reported Issues that are reported by GitHub users external to the Azure organization.
Projects
Status: Untriaged
Development

Successfully merging this pull request may close these issues.

3 participants