Support custom IConfigurationRefresher implementations in AzureAppConfigurationRefresherProvider

[Nechepso]

Hi!

We're trying to add Azure App Configuration sources with a priority lower than the default appsettings.{Environment}.json file's one, using WebApplicationBuilder.

We noticed that WebApplicationBuilder's behavior is to trigger a reload of all sources in Configuration.Sources every time the collection is modified in any way other than with Add (e.g., with Insert).

Loading the configuration from AAC more than once is problematic as it can take a lot of time, especially when some configuration keys link to Azure Key Vaults. Doing this work several times impacts negatively the startup time of an application that uses WebApplicationBuilder.

We already opened an issue about this behavior in the ASP.NET Core repo, but we thought that in the meantime, we could shield the Azure App Configuration source/provider from those reloads by registering our one decorator implementation of IConfigurationSource/IConfigurationProvider/IConfigurationRefresher.

However, as soon as we try to use AAC's dynamic configuration middleware (with UseAzureAppConfiguration), we get an InvalidOperationException from this code:

if (!refreshers.Any())
{
    throw new InvalidOperationException("Unable to access the Azure App Configuration provider. Please ensure that it has been configured correctly.");
}

Despite the interfaces being public, the method FindRefreshers of AzureAppConfigurationRefresherProvider still expects the implementation of all configurationRoot.Providers to be of the AzureAppConfigurationProvider internal type (probably to be able to set the logger factory via the LoggerFactory property, which is not part of the IConfigurationRefresher interface).

if (provider is AzureAppConfigurationProvider appConfigurationProvider)
{
    appConfigurationProvider.LoggerFactory = loggerFactory;
    refreshers.Add(appConfigurationProvider);
}

This prevents us to use AzureAppConfigurationRefresherProvider with our own implementation of IConfigurationProvider/IConfigurationRefresher.

We could implement our own IConfigurationRefresherProvider (and thus duplicate the logic of AzureAppConfigurationRefresherProvider) in order to support our other custom implementations, but it is not ideal.

Would it be possible to stop locking AzureAppConfigurationRefresherProvider to a specific implementation of IConfigurationRefresher (possibly by promoting the LoggerFactory property to the IConfigurationRefresher interface)?

Thanks a lot for your consideration.

[zhiyuanliang-ms]

Hey, @Nechepso This is a reasonable request. I am taking a look at it.

[zhiyuanliang-ms]

** updated **

I figured out your scenario and I had reproduced the issue.

[zhiyuanliang-ms]

Hey, @Nechepso

Would it be possible to stop locking AzureAppConfigurationRefresherProvider to a specific implementation of IConfigurationRefresher (possibly by promoting the LoggerFactory property to the IConfigurationRefresher interface)?

I am thinking about modifying the ConfigurationRefreshProvider implementation like this:

if (provider is IConfigurationRefresher configurationRefresher)
{
    refreshers.Add(configurationRefresher);

    if (configurationRefresher is AzureAppConfigurationProvider azureAppConfigurationProvider)
    {
        azureAppConfigurationProvider.LoggerFactory = loggerFactory;
    }
}

In this case, we don't need to modify the public interface (which is a breaking change).

Besides, I am also thinking about this

This prevents us to use AzureAppConfigurationRefresherProvider with our own implementation of IConfigurationProvider/IConfigurationRefresher.

It totally make sense that AzureAppConfigurationRefresherProvider only find AzureAppConfigrationProvider. From the design perspective, this is why we expose the IConfigurationRefresherProvider interface.

I need to discuss with my team about this. Another option is to add a toggle for the AzureAppConfigurationRefresherProvider, this toggle decides whether it finds IConfigurationRefresher or AzureAppConfigurationProvider. And when you use app config middleware, you must call AddAzureAppConfiguration on service collection first. We can twist this method to set the toggle.

[Nechepso]

Hey @zhiyuanliang-ms,

Thanks a lot for your time.

In this case, we don't need to modify the public interface (which is a breaking change).

Indeed, I missed that, my bad. Your proposal of adding the IConfigurationRefresher to the list of refreshers first, before testing further its type, looks fine to me.

It totally make sense that AzureAppConfigurationRefresherProvider only find AzureAppConfigrationProvider. From the design perspective, this is why we expose the IConfigurationRefresherProvider interface.

True, but since this provider contains no behavior specific to AzureAppConfigrationProvider other than setting the log factory (if I'm not mistaken), and all of that behavior is in an internal class, implementors of IConfigurationRefresherProvider cannot leverage the existing code.

It would maybe help to split this class into two: one base ConfigurationRefresherProvider, handling everything true for any IConfigurationRefresherProvider and make AzureAppConfigurationRefresherProvider a subclass of it to handle the AzureAppConfigrationProvider specifics.

Assuming that it works well and it is the chosen approach, another thing would be to be able to choose which implementation to use. To build on your idea of allowing more control in AddAzureAppConfiguration, there could be a way to optionally specify the type (and/or the instance?) of IConfigurationRefresherProvider to use? Or maybe using TryAddSingleton for the AzureAppConfigurationRefresherProvider which would do nothing if another IConfigurationRefresherProvider has already been added to the service collection?

EDIT: My bad, AddAzureAppConfiguration is already using TryAddSingleton so injecting a custom IConfigurationRefresherProvider is already possible! 🎉

[jimmyca15]

If I'm following correctly this appears to be resolved based on the existing usage of TryAddSingleton. I will close this. Please re-open if there is still an issue.

[Nechepso]

Not quite.

With the current design/implementation of AzureAppConfigurationRefresherProvider, it is not possible to either:

1. Use this provider to find custom implementations of IConfigurationRefresher (including but not limited to AzureAppConfigrationProvider).

2. Derive from this provider or wrap it (the class is internal) to reuse its search logic to find these implementations.

@zhiyuanliang-ms suggested lifting this limitation by working on 1. (either by changing the pattern-matching from AzureAppConfigurationProvider to IConfigurationRefresher or by doing both).

Another possible approach I suggested is focusing on 2. and extracting the behavior of AzureAppConfigurationRefresherProvider that is not specific AzureAppConfigurationProvider into a base class, to allow implementors to reuse this behavior (and benefit from any future changes to it) rather than them having to duplicate/fork it in their own implementation.

TryAddSingleton is what makes this second approach possible, but it is not enough: until a change is made (working towards either making possible 1., 2. or by finding another approach), the initial issue remains unsolved.

[Nechepso]

For instance, building on @zhiyuanliang-ms' initial modification of AzureAppConfigurationRefresherProvider, splitting the class into two could look like this:

public class ConfigurationRefresherProvider : IConfigurationRefresherProvider
{
    // Would contain most code currently in AzureAppConfigurationRefresherProvider (omitted here for readability).

    // Called everytime an IConfigurationRefresher is found in the Configuration.
    // The refresher is added to the list of discovered refreshers if this method returns true. Otherwise it is ignored.
    protected virtual bool OnRefresherFound(IConfigurationRefresher refresher) => true;
    
    private void FindRefreshers(IConfigurationRoot configurationRoot, ILoggerFactory loggerFactory, List<IConfigurationRefresher> refreshers)
    {
        if (configurationRoot != null)
        {
            foreach (IConfigurationProvider provider in configurationRoot.Providers)
            {
                if (provider is IConfigurationRefresher configurationRefresher)
                {
                    if (OnRefresherFound(configurationRefresher))
                        refreshers.Add(configurationRefresher);
                }
                else if (provider is ChainedConfigurationProvider chainedProvider)
                {
                    if (_propertyInfo != null)
                    {
                        var chainedProviderConfigurationRoot = _propertyInfo.GetValue(chainedProvider) as IConfigurationRoot;
                        FindRefreshers(chainedProviderConfigurationRoot, loggerFactory, refreshers);
                    }
                }
            }
        }
    }
}

internal class AzureAppConfigurationRefresherProvider : ConfigurationRefresherProvider
{
    protected override bool OnRefresherFound(IConfigurationRefresher refresher)
    {
        if (refresher is AzureAppConfigurationProvider azureAppConfigurationProvider)
        {
            azureAppConfigurationProvider.LoggerFactory = loggerFactory;
            return true;
        }
        return false;
    }
}

As far as I can think, there would be no API or behavior change in AzureAppConfigurationRefresherProvider: it would still discover AzureAppConfigurationProvider refreshers (and only those), while being able to set their LoggerFactory.

On the other hand, doing a simple AddSingleton<IConfigurationRefresherProvider, ConfigurationRefresherProvider>() (or another implementation) before calling AddAzureAppConfiguration would allow to switch to a more generic behavior (possibly discovering all types of IConfigurationRefresher).

[zhiyuanliang-ms]

The typical code path:

// TryAddSingleton<IConfigurationRefresherProvider, AzureAppConfigurationRefresherProvider>()
service.AddAzureAppConfiguration(); 

...

// use AzureAppConfigurationRefreshMiddleware and check whether there is IConfigurationRefresherProvider in the DI container. At the same time, AzureAppConfigurationRefresherProvider will check whether there is any AzureAppConfigurationProvider (which is IConfigurationRefresher)
app.UseAzureAppConfiguration(); 

Due to the ASP.NET core configuration unexpected reload behavior. The user wants to shield the Azure App Configuration source/provider from those reloads by wrapping it. So there is no AzureAppConfigurationProvider (but there is IConfigurationRefresher).

However, if no AzureAppConfigurationProvider can be found, the AzureAppConfigurationRefresherProvider will throw exception. So, the user needs to implement IConfigurationRefresherProvider and adds it to DI manually as well.

Our middleware consumes IConfigurationRefresherProvider, so there is no problem at this part. The pain the user wants to avoid is to implement his own IConfigurationRefresherProvider which will share most of the same code as the existing internal AzureAppConfigurationRefresherProvider.

Is my understanding correct? @Nechepso @jimmyca15

[zhiyuanliang-ms]

Hey, @Nechepso

Fixed in #715

[Nechepso]

Hey, @zhiyuanliang-ms

Looks fine to me. Thanks a lot! 👍

[Nechepso]

I'll let you know as soon as I tested your fix.