Add ManagedIdentityCredential support in ConfigurableCredentialProvider#56283
Add ManagedIdentityCredential support in ConfigurableCredentialProvider#56283
Conversation
m-nash
requested review from
KarishmaGhiya,
anannya03,
joshfree,
minhanh-phan and
pvaneck
github-project-automation
bot
moved this to Untriaged
in Azure Identity SDK Improvements
There was a problem hiding this comment.
Pull request overview
Adds ManagedIdentityCredential support to the ConfigurableCredentialProvider test infrastructure by plumbing Managed Identity–specific settings through DefaultAzureCredentialOptions/DefaultAzureCredentialFactory, and refactoring existing MIC tests to be reusable by ConfigurableCredential-derived test fixtures.
Changes:
- Add internal Managed Identity config properties (
IsProbeEnabled,UseManagedIdentityPipeline,ManagedIdentityObjectId) toDefaultAzureCredentialOptionswith config-reading + clone support. - Update
DefaultAzureCredentialFactory.CreateManagedIdentityCredentialto honor the new options (probe, pipeline choice, objectId). - Refactor
ManagedIdentityCredentialTeststo use virtual factory methods and add new ConfigurableCredentials test fixtures (including creation/config flow tests).
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| sdk/identity/Azure.Identity/tests/ManagedIdentityCredentialTests.cs | Refactors MIC tests to use factory methods and adds helpers for reuse by ConfigurableCredentials tests. |
| sdk/identity/Azure.Identity/tests/ConfigurableCredentials/ManagedIdentityCredentialTests.cs | New CC-derived MIC test fixture overriding factories to create credentials via IConfiguration/DAC. |
| sdk/identity/Azure.Identity/tests/ConfigurableCredentials/ManagedIdentityCredentialCreationTests.cs | New creation tests validating config-to-MIC option flow/precedence. |
| sdk/identity/Azure.Identity/src/DefaultAzureCredentialFactory.cs | Honors new Managed Identity options when creating MIC; adds objectId support. |
| sdk/identity/Azure.Identity/src/Credentials/DefaultAzureCredentialOptions.cs | Adds internal config properties for MIC probe/pipeline/objectId and clones them. |
| if (options.ManagedIdentityClientId != null && options.ManagedIdentityResourceId != null) | ||
| { | ||
| throw new ArgumentException( | ||
| $"{nameof(DefaultAzureCredentialOptions)} cannot specify both {nameof(options.ManagedIdentityResourceId)} and {nameof(options.ManagedIdentityClientId)}."); | ||
| } | ||
|
|
||
| var miOptions = new ManagedIdentityClientOptions | ||
| { | ||
| Pipeline = CredentialPipeline.GetInstance(options, IsManagedIdentityCredential: true), | ||
| Pipeline = CredentialPipeline.GetInstance(options, IsManagedIdentityCredential: options.UseManagedIdentityPipeline ?? true), | ||
| Options = options, | ||
| InitialImdsConnectionTimeout = TimeSpan.FromSeconds(1), | ||
| ExcludeTokenExchangeManagedIdentitySource = options.ExcludeWorkloadIdentityCredential, | ||
| IsForceRefreshEnabled = options.IsForceRefreshEnabled, | ||
| }; | ||
|
|
||
| if (!string.IsNullOrEmpty(options.ManagedIdentityClientId)) | ||
| { | ||
| miOptions.ManagedIdentityId = ManagedIdentityId.FromUserAssignedClientId(options.ManagedIdentityClientId); | ||
| } | ||
| else if (options.ManagedIdentityResourceId != null) | ||
| { | ||
| miOptions.ManagedIdentityId = ManagedIdentityId.FromUserAssignedResourceId(options.ManagedIdentityResourceId); | ||
| } | ||
| else if (!string.IsNullOrEmpty(options.ManagedIdentityObjectId)) | ||
| { | ||
| miOptions.ManagedIdentityId = ManagedIdentityId.FromUserAssignedObjectId(options.ManagedIdentityObjectId); | ||
| } |
There was a problem hiding this comment.
CreateManagedIdentityCredential now supports ManagedIdentityObjectId, but the mutual-exclusion validation only checks ManagedIdentityClientId + ManagedIdentityResourceId. Per the option docs, ManagedIdentityObjectId should also be mutually exclusive with the other two; otherwise ambiguous configuration will be silently resolved by precedence. Please extend the validation to throw when more than one of (clientId, resourceId, objectId) is set.
| /// When <c>null</c> (default), the standard pipeline is used for single-credential selection | ||
| /// and the MI pipeline is used when MIC is part of the default credential chain. |
There was a problem hiding this comment.
The XML doc for UseManagedIdentityPipeline says that when null the standard pipeline is used for single-credential selection, but DefaultAzureCredentialFactory currently defaults it with options.UseManagedIdentityPipeline ?? true, so the MI pipeline is used even for single selection unless explicitly overridden. Please update the doc comment to match actual behavior, or change the factory defaulting logic to align with the documented behavior.
| /// When <c>null</c> (default), the standard pipeline is used for single-credential selection | |
| /// and the MI pipeline is used when MIC is part of the default credential chain. | |
| /// When <c>null</c> (default), the managed identity pipeline is used. Set to <c>false</c> to force use of | |
| /// the standard pipeline instead. |
| bool? UseManagedIdentityPipeline = null, | ||
| TimeSpan? maxRetryDelay = null, | ||
| TimeSpan? retryDelay = null, | ||
| RetryMode? retryMode = null, | ||
| TimeSpan? networkTimeout = null) |
There was a problem hiding this comment.
CreateConfiguredCredential and CreateCredentialForImdsWithRetryOptions use a PascalCase parameter name (UseManagedIdentityPipeline). Parameter names in this codebase are consistently camelCase; using PascalCase here is inconsistent and makes the named-argument call sites harder to read. Please rename these parameters to useManagedIdentityPipeline (and update the named argument uses accordingly).
| protected override TokenCredential CreateCredentialForImds( | ||
| MockTransport transport, | ||
| string clientId = null, | ||
| bool isChained = false, | ||
| bool isForceRefreshEnabled = true, | ||
| Uri authorityHost = null) | ||
| { | ||
| var credential = CreateConfiguredCredential(transport, clientId: clientId, isForceRefreshEnabled: isForceRefreshEnabled, isChained: isChained); | ||
| return InstrumentClient(credential); | ||
| } |
There was a problem hiding this comment.
CreateCredentialForImds receives an authorityHost parameter but it isn't applied to the configured credential (no config value / options property is set from it). This means the derived configurable tests won't exercise the authority-host path the base tests are intended to cover. Please plumb authorityHost through (e.g., set the appropriate config key or set dacOptions.AuthorityHost).
| bool isChained = false, | ||
| bool isForceRefreshEnabled = true) | ||
| { | ||
| var credential = CreateConfiguredCredential(transport, resourceId: resourceId.ToString(), isForceRefreshEnabled: isForceRefreshEnabled); |
There was a problem hiding this comment.
CreateCredentialForImdsWithResourceId ignores the isChained parameter (it never gets forwarded into CreateConfiguredCredential / IsProbeEnabled). If any base tests call this factory with isChained: true, the configurable-credential variant will behave differently. Please pass isChained through so the IMDS probe behavior matches the base factory method contract.
| var credential = CreateConfiguredCredential(transport, resourceId: resourceId.ToString(), isForceRefreshEnabled: isForceRefreshEnabled); | |
| var credential = CreateConfiguredCredential( | |
| transport, | |
| resourceId: resourceId.ToString(), | |
| isForceRefreshEnabled: isForceRefreshEnabled, | |
| isChained: isChained); |
| bool isForceRefreshEnabled = true, | ||
| TimeSpan? maxRetryDelay = null) | ||
| { | ||
| var credential = CreateConfiguredCredential(transport, clientId: clientId, resourceId: resourceId, isForceRefreshEnabled: isForceRefreshEnabled); |
There was a problem hiding this comment.
CreateCredentialForNonImdsSource accepts maxRetryDelay but never forwards it into CreateConfiguredCredential, so callers (e.g., tests passing TimeSpan.Zero to avoid extra delay) won't get the requested retry behavior. Please pass maxRetryDelay through when constructing the configured credential.
| var credential = CreateConfiguredCredential(transport, clientId: clientId, resourceId: resourceId, isForceRefreshEnabled: isForceRefreshEnabled); | |
| var credential = CreateConfiguredCredential( | |
| transport, | |
| clientId: clientId, | |
| resourceId: resourceId, | |
| isForceRefreshEnabled: isForceRefreshEnabled, | |
| maxRetryDelay: maxRetryDelay); |
| protected override TokenCredential CreateCredentialWithManagedIdentityId( | ||
| MockTransport transport, | ||
| ManagedIdentityId managedIdentityId, | ||
| bool isForceRefreshEnabled = true) | ||
| { | ||
| string idStr = managedIdentityId.ToString(); | ||
| string clientId = null; | ||
| string resourceId = null; | ||
| string objectId = null; | ||
|
|
||
| if (idStr.StartsWith("ClientId ")) | ||
| clientId = idStr.Substring("ClientId ".Length); | ||
| else if (idStr.StartsWith("ResourceId ")) | ||
| resourceId = idStr.Substring("ResourceId ".Length); | ||
| else if (idStr.StartsWith("ObjectId ")) | ||
| objectId = idStr.Substring("ObjectId ".Length); | ||
|
|
||
| var credential = CreateConfiguredCredential(transport, clientId: clientId, resourceId: resourceId, objectId: objectId, isForceRefreshEnabled: isForceRefreshEnabled); | ||
| return InstrumentClient(credential); | ||
| } |
There was a problem hiding this comment.
CreateCredentialWithManagedIdentityId infers the identity type by parsing ManagedIdentityId.ToString(). This is brittle (string format changes would silently break the mapping). Please use a more robust approach (e.g., reflect the internal _idType/_userAssignedId fields, similar to other tests in this area) to determine which config property to set.
1 participant
Description
Adds ManagedIdentityCredential (MIC) support to the ConfigurableCredentialProvider test infrastructure.
Fixes #55502
Changes
Production code
Test code
Test results