Unable to inject reusable ConfigurationClient per recommendation of SDK guide.
Opened this issue · 18 comments
The SDK Guide, recommends reusing your ConfigurationClient, however the APIs of this library prevent this by disallowing injection of a custom ConfigurationClient instance. The library creates a new instance of ConfigurationClient for each call to ConfigurationBuilder.AddAzureAppConfiguration, which is unduly restrictive.
When ConfigurationBuilder.AddAzureAppConfiguration is called more than once, do you connect to the same App Configuration store or different stores?
I'm working on a multi-tenanted project where a distinct IConfiguration (and thus ConfigurationBuilder) is created and cached for each tenant. Depending on the environment and tenant, ConfigurationBuilder.AddAzureAppConfiguration is sometimes called passing the same ConfigurationClient instance but using different label filters.
With all due respect, I believe the response to your question is irrelevant, as a user should be able to specify whatever ClientConfiguration instance they please. Forcing a very specific mechanism for constructing the underlying ConfigurationClient (as apposed to allowing it to be injected) violates software design best practices and principles.
@zhenlan Had time to look at this? My PR went stale. Is it going to be ignored again if I update it?
Hello?! @amerjusupovic Can you give this some love?
Hi @goldsam, I'm sorry about the delayed response here. I think this looks like a valid point from the blog you linked and related issues I found. Right now, I'm just getting more context on this from the team to see if there was a reason for omitting that capability. If not, then I'll get back to you about the PR, but I'm working on it!
@amerjusupovic Thank you. I had actually submitted a PR which now has I conflicts. I would be happed to update it if you are willing to give it some attention afterwards. If you would review my PR and provide feedback, I would be happy to do the work to get this ready. Just let me know. Thank you very much.
@amerjusupovic Did you see that I had created a PR previously?
@goldsam I saw your PR but from what I gathered so far from others, it's unfortunately not the way we want to implement this. We're keeping this in mind as a future improvement, but for now it's not supported.
@amerjusupovic As an alternative to @goldsam 's PR, could the options class be modified to make the client setter public so that we can inject the client ourselves? It's a smaller change and allows us users to decide what to do.
@zhenlan @amerjusupovic @zhiyuanliang-ms You guys are killing me. Please add support to customize the creation and lifetime of the client. I think this is a fairly basic capability.
I created a working PR which you rejected without any justification beyond" that's not what we want to do". @carlin-q-scott also offered a simple, straightforward solution which you completely ignored. It's been well over a year.
Time to up your game and be better open source denizens.
Because of geo-replication support, the provider creates ConfigurationClient instances as needed by discovering any replicas in App Configuration. I think it could be confusing to be able to pass in a ConfigurationClient while the provider is still creating clients behind the scenes, since they may be used in case of failover, like if an App Configuration endpoint can't be reached and the provider tries to connect to a replica instead.
Here's another option that might fit the current provider usage:
.AddAzureAppConfiguration(o =>
{
o.Connect(new Uri("https://foo.azconfig.io"), new MyConfigurationClientFactory());
});class MyConfigurationClientFactory : IConfigurationClientFactory
{
public ConfigurationClient CreateClient(string endpoint)
{
...
}
}interface IConfigurationClientFactory
{
ConfigurationClient CreateClient(string endpoint);
}The IConfigurationClientFactory interface has a CreateClient method that the provider would call whenever it needs to get a ConfigurationClient for a specific endpoint. By default, the provider would have an implementation that behaves the same as it does today where it simply creates a new client. However, a user could pass in their own implemented factory instead where the CreateClient method returns an existing ConfigurationClient.
Of course, it's only a suggestion but just want to throw this out there.
Azure SDK client DI flow may be able to help here.
@drago-draganov I don't think I understand what you're referencing. This library doesn't support inject of the client like most other Azure libraries do. That is another aspect of this issue.
To be clear, this is not respected by this library:
builder.Services.AddAzureClients(clientBuilder =>
{
clientBuilder.AddConfigurationClient(...);
});@goldsam
Sorry, I wasn't specific. My reference was that AppConfigProvider should add support for Azure SDK client DI, instead of custom factory or else (I saw a few proposals mentioned). That, I thought, will enable the scenario you asked for.
Perhaps naming the clients needs level of conformance (ex. endpoint/connection_string as client name), but that's details.
Appreciate your feedback!
@drago-draganov I think we are in agreement😉 Supporting Azure SDK client DI would suffice for my needs just fine. Its unclear to me how this would work while maintaining backwards API compatibility. I think regardless of the solution, additions be necessary to class AzureAppConfigurationOptions.
I am not sure that we need changes in AzureAppConfigurationOptions. Perhaps, if we try IAzureClientFactory, it may be OK.
Just an update: we're looking into using the SDK DI pattern as mentioned, but as far as I know we don't have access to DI from the provider. I'm reaching out to some people who are involved with the SDK to see if they have any suggestions or alternative ways to get access to clients added this way.
After discussion with the SDK team, it sounds like it would be difficult to unify the approach to add ConfigurationClient instances to the provider with the SDK's AddAzureClients API. Our next step is likely allowing users to pass in a client factory to the AddAzureAppConfiguration options once we finalize that design.