Add custom auth option for event producer factory #92
Conversation
wi-y
commented
Mar 4, 2021
wi-y
commented
- Allows for a custom auth option when sending events to an event hub
- Can now use the following 3 options to connect: the service managed identity, a connection string, or a custom auth option (if supplied).
- Refactor the event hub processor factory and storage to behave the same way.
wi-y
requested review from
dustinburson,
namalu,
rogordon01 and
ms-teli
and removed request for
dustinburson
| { | ||
| var tokenCredential = provider.GetCredential(); | ||
| return new BlobContainerClient(containerUri, tokenCredential); | ||
| } | ||
| else | ||
| { | ||
| throw new Exception($"Unable to create blob container client for {blobUri}"); |
There was a problem hiding this comment.
I think we should be more explicit as to why this failed so it's easier to debug.
There was a problem hiding this comment.
Yep makes sense. Updated with error messages that explain the invalid config / how we got to this point.
| host = uri.Host; | ||
| } | ||
|
|
||
| if (Uri.CheckHostName(host) != UriHostNameType.Unknown) |
There was a problem hiding this comment.
Nit, I personal prefer put validations first and let it fail as early as possible.
e.g.
if (!Uri.IsWellFormedUriString(host, UriKind.Absolute)) {
throw new Exception("not well formatted.");
}
var uri = new Uri(host);
host = uri.Host;
if(Uri.CheckHostName(host) == UriHostNameType.Unknown)
throw new Exception("host is unknown");
)
return host;
| @@ -23,27 +24,24 @@ public EventProcessorClient CreateProcessorClient(BlobContainerClient blobContai | |||
| if (options.ServiceManagedIdentityAuth) | |||
There was a problem hiding this comment.
Nit, would that make sense to add an enum property in the EventProcessorClientFactoryOptions to explicitly say the type of connection/authentication? The if-else seems able to handle overriding logic flow in the code itself but I'm not sure whether that's a valid use case. Other than that, I think a single property to specify which type of authentication would make code cleaner.
There was a problem hiding this comment.
If we have a known set an enum seems warrented.
There was a problem hiding this comment.
Is the idea to have a "authentication type" property in EventProcessorClientFactoryOptions that is set to the type of connection/authentication and is calculated based on what options (connection string, event hub mi settings, other) are provided? I agree this could make the code cleaner, but just making sure I understand the comment.
Or do we want to user to explicitly set this new "authentication type" property, in addition to supplying the required connection options properties.
There was a problem hiding this comment.
I was thinking the second one in here, as user, I will have to tell this factory explicitly which auth option I want to use by setting the enum field.
There was a problem hiding this comment.
Thanks for making the changes! They look good to me.
| EventProcessorClient CreateProcessorClient(BlobContainerClient blobContainerClient, EventProcessorClientFactoryOptions options, EventProcessorClientOptions eventProcessorClientOptions); | ||
|
|
||
| EventProcessorClient CreateProcessorClient(IAzureCredentialProvider provider, BlobContainerClient blobContainerClient, EventProcessorClientFactoryOptions options, EventProcessorClientOptions eventProcessorClientOptions); | ||
| EventProcessorClient CreateProcessorClient(BlobContainerClient blobContainerClient, EventProcessorClientFactoryOptions options, EventProcessorClientOptions eventProcessorClientOptions, IAzureCredentialProvider provider = null); |
There was a problem hiding this comment.
Should the provider be part of the method signature? I feel like it can be injected into the ctor for the factory and resolved by DI and if it is needed it will be used. I.E. The factory needs to be resolve any configuration we should support so it should have provider available.
Does the provider need to vary based on the context? The concern here is we have an option signature and we allow the options to require the Azure provider but invoke could fail to provide causing an error.
There was a problem hiding this comment.
I think I did this originally as part of the factory instead of the method, but we changed to the method because it might make sense to be able to generate multiple processors or producers with different configs. Right now there is not a requirement to do so but it could add flexibility in the future.
We were thinking it would be better to have 1 factory that could generate pretty much every kind of client by putting IAzureCredentialProvider in the method signature. If IAzureCredentialProvider is part of the factory instead of the method then we may need to generate a new factory for each custom auth credential provider if there is a requirement to do so in the future.
dustinburson
deleted the
personal/wiyochum/add-custom-auth-option-for-event-producer
branch
