Adding DefaultAzureCredentialOptions to ManagedIdentityTokenSource()

[bachuv]

These changes resolve #1279 by adding an optional ManagedIdentityOptions parameter to ManagedIdentityTokenSource(). ManagedIdentityOptions is a new type that is introduced with these changes.

The issue mentioned wanted the ability to configure azureAdInstance and tenantId. ManagedIdentityOptions.AuthorityHost is equivalent to azureAdInstance and ManagedIdentityOptions.TenantId is equivalent to tenantId.

[cgillum]

I love this idea! I think we should add some unit tests to make sure the various configuration options work correctly, though. I've added a few other comments as well.

[cgillum]

We'll want to make sure that our other language implementations can also specify the "options" parameter for managed identities. Can you sync with @davidmrdavid to see where and whether we need to make corresponding changes for JS and Python?

[cgillum]

Sorry, one more thing: can you update the the CallHttpActionOrchestrationWithManagedIdentity unit test to cover the "options" setting?

[bachuv]

DurableAzureCredentialOptions.Transport is the type of HttpPipelineTransport and defaults to the instance of HttpClientTransport. Users could configure DefaultAzureCredentialOptions and introduce a possible HttpClient like in the code below which sets up DefaultAzureCredentialOptions:

HttpClientHandler handler = new HttpClientHandler();
handler.Credentials = new NetworkCredential(username, password);

DefaultAzureCredentialOptions options = new DefaultAzureCredentialOptions();
options.Transport = new HttpClientTransport(new HttpClient(handler));

Supporting DefaultAzureCredentialOptions.Transport introduces nested serialization and if it's created in an orchestrator method then it introduces this warning: System.Net.Http.HttpClientHandler violates the orchestrator deterministic code constraint.

Is it possible to not support DefaultAzureCredentialOptions.Transport? Another option could be to create a wrapper class that doesn’t support DefaultAzureCredentialOptions.Transport. This would be like DurableHttpRequest and DurableHttpResponse for the Durable HTTP feature.

@cgillum @ConnorMcMahon Any thoughts on how to handle DefaultAzureCredentialOptions.Transport?

[cgillum]

Hmm...this is problematic. There is no way we can safely serialize all these fields (especially not Transport). I considered using a custom serializer to serialize only the things that are known to be serializable, but then that could lead to customer confusion because we're not preserving all the properties they set.

The safest approach might be to create our own type instead of relying on DefaultAzureCredentialOptions. We could call it ManagedIdentityOptions or something similar, and expose our own subset of DefaultAzureCredentialOption's properties. I think we would start by supporting in the properties required to solve #1279, like AuthorityHost and ManagedIdentityClientId (I would prefer to shorten it to just ClientId) to support user-assigned identities. If we get customers asking for more fields, we can add them later. On the HTTP activity side, we could use this data and initialize own own private DefaultAzureCredentialsOptions object from the values we get from our deserialized ManagedIdentityOptions field.

Having our own type could also simplify documentation and cross-language support.

@bachuv @ConnorMcMahon thoughts?

[bachuv]

@cgillum I agree with that approach. It would definitely make documentation and serialization easier. I can create the new type and add the 2 fields that were mentioned in #1279.

[ConnorMcMahon]

A few more things to address before merging.

[ConnorMcMahon]

Looks good to me, just a few small suggestions around the tests.

[cgillum]

I sign off as well!