Support managed identity with normalized event hub #90

wi-y · 2021-02-10T20:32:15Z

No description provided.

rogordon01 · 2021-02-10T21:14:59Z

deploy/templates/default-azuredeploy-webjobs.json

@@ -353,6 +364,48 @@
        ]
      }
    },
+    {
+      "type": "Microsoft.EventHub/namespaces/eventhubs/providers/roleAssignments",
+      "apiVersion": "2018-07-01",


Since this apiVersion is duplicated consider making it a variable.

deploy/templates/default-azuredeploy-webjobs.json

src/console/Program.cs

src/lib/Microsoft.Health.Events/EventHubProcessor/EventProcessorClientFactory.cs

dustinburson · 2021-02-11T16:20:20Z

src/lib/Microsoft.Health.Events/EventHubProcessor/EventProcessorClientFactory.cs

+        {
+            EnsureArg.IsNotNull(options);
+
+            if (options.ServiceManagedIdentityAuth)


ServiceManagedIdentityAuth [](start = 24, length = 26)

Will this be extended to support cert based auth as well for customer facing MI?

For cert based auth I created a second method in EventProcessorClientFactory which takes an IAzureCredentialProvider which is a way to specify a custom way of retrieving a token. I was thinking of passing in a provider that does the cert based auth.

EventProcessorClient CreateProcessorClient(IAzureCredentialProvider provider, BlobContainerClient blobContainerClient, EventProcessorClientFactoryOptions options, EventProcessorClientOptions eventProcessorClientOptions)

dustinburson

src/console/Program.cs

ms-teli · 2021-02-10T23:40:45Z

src/lib/Microsoft.Health.Events/EventHubProcessor/EventProcessorClientFactory.cs

+{
+    public class EventProcessorClientFactory
+    {
+        private BlobContainerClient _blobContainerClient;


Nit: this does not sound a member of factory but a component to compose the EventProcessorClient, this can be a method argument.

ms-teli · 2021-02-10T23:48:54Z

src/lib/Microsoft.Health.Events/EventHubProcessor/EventProcessorClientFactory.cs

+
+            if (options.ServiceManagedIdentityAuth)
+            {
+                var tokenCredential = new DefaultAzureCredential();


NIt: when the developer customer has the creds in the environment, this may not get the expected creds. https://docs.microsoft.com/en-us/dotnet/api/azure.identity.defaultazurecredential?view=azure-dotnet

Do we need to explain this in the documents? Any reason we can't use the ManagedIdentityCredential? https://docs.microsoft.com/en-us/dotnet/api/azure.identity.managedidentitycredential?view=azure-dotnet

I was thinking that we would want to make it easy to deploy and use service managed identity (where ManagedIdentityCredential is most appropriate) but also allow develops to test/develop locally with their own credentials (where VisualStudio/VisualStudioCode/environment variables credentials might be most appropriate) and DefaultAzureCredential seemed to handle that nicely. I felt like that benefit of testing locally using DefaultAzureCredential was worth it over only using ManagedIdentityCredential and not really having an easy way of testing locally.

ms-teli · 2021-02-10T23:51:27Z

src/lib/Microsoft.Health.Events/EventHubProcessor/EventProcessorClientFactory.cs

+                var tokenCredential = new DefaultAzureCredential();
+                return new EventProcessorClient(_blobContainerClient, options.EventHubConsumerGroup, options.EventHubNamespaceFQDN, options.EventHubName, tokenCredential, eventProcessorClientOptions);
+            }
+            else if (!string.IsNullOrEmpty(options.ConnectionString))


We may need a todo to update the document explaining that the managed identity option take over the ConnectionString.

Yeah we have a backlog item - right now none of this console stuff is documented because we were planning on doing a couple more iterations before putting out some documentation just we don't have to keep changing it.

ms-teli · 2021-02-10T23:53:16Z

src/lib/Microsoft.Health.Events/EventProducers/EventHubProducerClientOptions.cs

+
+namespace Microsoft.Health.Events.EventProducers
+{
+    public class EventHubProducerClientOptions


Nit: suggest to rename this EventHubProducerClientConfig, but this may just be a personal preference, feel free to skip.

ms-teli · 2021-02-11T16:24:58Z

Apologies, I forgot to submit the pending review.

namalu · 2021-02-11T17:58:21Z

src/console/Normalize/Processor.cs

+            var eventHubProducerOptions = new EventHubProducerClientOptions();
+            _env.GetSection("OutputEventHub").Bind(eventHubProducerOptions);
+
+            var eventHubProducerFactory = new EventHubProducerFactory();


Why not just add the EventHubProducerFactory to the Service Collection and then "inject" this into the constructor. I think adding most of these factories to the Service Collection and letting the DI framework take care of dependency resolution would reduce a lot of this code.

Thanks - updated to inject most of these. I moved most of the logic to Startup.cs. It did reduce the amount of code.

namalu · 2021-02-11T18:02:00Z

src/console/Program.cs

@@ -54,7 +48,11 @@ public static async Task Main()
            var storageCheckpointClient = GetStorageCheckpointClient(blobContainerClientFactory, checkpointContainerOptions, storageOptions, logger, eventHubName);
            var eventConsumerService = new EventConsumerService(eventConsumers, logger);

-            var incomingEventReader = GetEventProcessorClient(storageCheckpointClient.GetBlobContainerClient(), eventHubOptions);
+            var eventProcessorOptions = GetEventProcessorFactoryOptions(config);
+            var eventProcessorClientFactory = new EventProcessorClientFactory(storageCheckpointClient.GetBlobContainerClient());


I think in all cases where you are using new to create these items could be simplified by adding these options objects and factory objects to the Service Collection. Then result would probably be getting the service provider, getting the EventProcessor from the provider and calling RunAsync().

Updated - moved to the service collection.

namalu · 2021-02-11T18:13:31Z

src/lib/Microsoft.Health.Events/EventHubProcessor/EventProcessorClientFactoryOptions.cs


        public string EventHubName { get; set; }
+
+        public string ConnectionString { get; set; }


Rather than overloading these 2 options objects with both authentication methods, could we create separate implementations, create a common interface (IEventProcessorClientFactory), and just change which one gets added to the Service Collection at startup? We could just default to ConnectionString in the project, and allow consumers to just change it if needed.

e.g. EventProcessorClientConnectionStringFactoryOptions is injected into EventProcessorClientConnectionStringFactory.

There would be an implementation for Managed Identity - EventProcessorClientManagedIdentityFactoryOptions is injected into EventProcessorClientManagedIdentityFactory.

Both implement IEventProcessorClientFactory.

I tried this out and like the idea of separating in principal, but the problem seems to be what methods IEventProcessorClientFactory should have. We want to be able to pass in options into the factories but if the options can either be EventProcessorClientConnectionStringFactoryOptions or EventProcessorClientManagedIdentityFactoryOptions then I think that means that IEventProcessorClientFactory would have to have methods that accept those options, or some sort of generic options (but then we have to check/validate the generic options that are being passed in are connection options or MI options which seems pretty similar to what the code is currently doing).

support managed id with normalized event hub

6d0d3fd

wi-y requested review from dustinburson, namalu, ms-teli and rogordon01 February 10, 2021 20:32

rogordon01 reviewed Feb 10, 2021

View reviewed changes

dustinburson reviewed Feb 11, 2021

View reviewed changes

dustinburson approved these changes Feb 11, 2021

View reviewed changes

ms-teli reviewed Feb 11, 2021

View reviewed changes

namalu requested changes Feb 11, 2021

View reviewed changes

wi-y added 2 commits February 12, 2021 17:05

address pr feedback

9066677

update env variable names

600214a

wi-y requested a review from namalu February 18, 2021 07:36

wi-y merged commit 58ec243 into master Feb 18, 2021

dustinburson deleted the personal/wiyochum/support-mi-with-normalized-event-hub branch November 8, 2022 18:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support managed identity with normalized event hub #90

Support managed identity with normalized event hub #90

wi-y commented Feb 10, 2021

rogordon01 Feb 10, 2021

dustinburson Feb 11, 2021

wi-y Feb 13, 2021

dustinburson left a comment

ms-teli Feb 10, 2021

ms-teli Feb 10, 2021

wi-y Feb 13, 2021

ms-teli Feb 10, 2021

wi-y Feb 13, 2021

ms-teli Feb 10, 2021

ms-teli commented Feb 11, 2021

namalu Feb 11, 2021

wi-y Feb 13, 2021

namalu Feb 11, 2021

wi-y Feb 13, 2021

namalu Feb 11, 2021

wi-y Feb 13, 2021


		public string EventHubName { get; set; }

		public string ConnectionString { get; set; }

Support managed identity with normalized event hub #90

Support managed identity with normalized event hub #90

Conversation

wi-y commented Feb 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dustinburson left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ms-teli commented Feb 11, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment