-
Notifications
You must be signed in to change notification settings - Fork 5.1k
[Extensions] Fixing managed identity error text #53415
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
base: main
Are you sure you want to change the base?
Conversation
The focus of these changes is to improve the error messaging when configuration provided for one of the managed identity credentials is incorrect.
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.
Pull Request Overview
This PR improves error messaging for managed identity credential configuration issues. The changes clarify what configuration values must be provided and correct inconsistencies in the error messages.
Key Changes:
- Updated error messages to remove references to environment variables (configuration only)
- Fixed inconsistent identifier names in error messages to match actual configuration keys
- Corrected the order of valid identifiers based on whether federated identity is being used
sdk/extensions/Microsoft.Extensions.Azure/src/Internal/ClientFactory.cs
Outdated
Show resolved
Hide resolved
| ? "'managedIdentityClientId', 'managedIdentityResourceId', or 'managedIdentityObjectId'" | ||
| : "'clientId', 'managedIdentityClientId', 'managedIdentityResourceId', or 'managedIdentityObjectId'"; |
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 logic was correct before. If federated, clientId is required. The underlying ClientAssertionCredential will need it.
| ? "'managedIdentityClientId', 'managedIdentityResourceId', or 'managedIdentityObjectId'" | |
| : "'clientId', 'managedIdentityClientId', 'managedIdentityResourceId', or 'managedIdentityObjectId'"; | |
| ? "'clientId', 'managedIdentityClientId', 'managedIdentityResourceId', or 'managedIdentityObjectId'" | |
| : "'managedIdentityClientId', 'managedIdentityResourceId', or 'managedIdentityObjectId'"; |
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.
You're missing L426. The message ends up being:
Managed Identity (non-fed)
I need a 'clientId', 'managedIdentityClientId', 'managedIdentityResourceId', or 'managedIdentityObjectId' to use.
Federated Managed Identity
I need a clientId AND "'managedIdentityClientId', 'managedIdentityResourceId', or 'managedIdentityObjectId' to use.
Summary
The focus of these changes is to improve the error messaging when configuration provided for one of the managed identity credentials is incorrect.