Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow access to child providers in ChainedConfigurationProvider #44486

Closed
Tracked by #64015
avanigupta opened this issue Nov 10, 2020 · 10 comments · Fixed by #67610
Closed
Tracked by #64015

Allow access to child providers in ChainedConfigurationProvider #44486

avanigupta opened this issue Nov 10, 2020 · 10 comments · Fixed by #67610
Assignees
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Configuration
Milestone

Comments

@avanigupta
Copy link
Contributor

avanigupta commented Nov 10, 2020

API Proposal

This API proposal provides the ability to enumerate a ChainedConfigurationProvider's child providers:

namespace Microsoft.Extensions.Configuration
{
    public class ChainedConfigurationProvider : IConfigurationProvider, IDisposable
    {
+        /// <summary>
+        /// The chained <see cref="IConfigurationProvider"/> instances.
+        /// </summary>
+        IEnumerable<IConfigurationProvider>? Providers { get; }

Motivation

Developers tend to add their configuration source via a ChainedConfigurationSource/Provider and they would want to access it from the outer ConfigurationRoot.

ConfigurationRoot exposed Providers. With this new API now, configuration providers that are nested under a ChainedConfigurationProvider if the user calls IConfigurationBulder.AddConfiguration(...) would now also be able to be accessed.

Usage

Sample usage can be found in AzureAppConfigurationRefresherProvider under the Azure/AppConfiguration-DotnetProvider repo.

Original Description

We have an issue in Azure App Configuration where AzureAppConfigurationProvider could be nested under a ChainedConfigurationProvider if the user calls IConfigurationBuilder.AddConfiguration(...) to add configuration built from the App Configuration provider.

Sample code snippet of this use case:

public static IWebHost BuildWebHost(string[] args)
{
    var configBuilder = new ConfigurationBuilder();
    IConfiguration configuration = configBuilder.AddAzureAppConfiguration(options =>
    {
        options.Connect(connectionString)
        .ConfigureRefresh(refreshOptions =>
        {
            refreshOptions.Register("Settings:BackgroundColor", refreshAll: true)
                          .SetCacheExpiration(TimeSpan.FromSeconds(10));
        });
    })
    .Build();

    return WebHost.CreateDefaultBuilder(args)
                  .ConfigureAppConfiguration((hostingContext, config) =>
                  {
                      config.AddJsonFile("appsettings.json").Build();
                      config.AddConfiguration(configuration);
                  })
                  .UseStartup<Startup>()
                  .Build();
}

Unfortunately, ChainedConfigurationProvider does not provide a way for others to access its child providers. This caused the issue that the App Configuration middleware could not find the instance of AzureAppConfigurationProvider (because in this case its under ChainedConfigurationProvider) and thus cannot obtain the instance of IConfigurationRefresher, which is responsible for refreshing the application configuration (our relevant middleware code here).

Would it be possible to expose the underlying providers in ChainedConfigurationProvider so that we can extract AzureAppConfigurationProvider and enable users to leverage our dynamic refresh functionality?

@ghost
Copy link

ghost commented Nov 10, 2020

Tagging subscribers to this area: @maryamariyan
See info in area-owners.md if you want to be subscribed.


Issue meta data
Issue content: We have an [issue](https://github.com/Azure/AppConfiguration-DotnetProvider/issues/168) in Azure App Configuration where `AzureAppConfigurationProvider` could be nested under a `ChainedConfigurationProvider` if the user calls `IConfigurationBuilder.AddConfiguration(...)` to add configuration built from the App Configuration provider.

Sample code snippet of this use case:

public static IWebHost BuildWebHost(string[] args)
{
    var configBuilder = new ConfigurationBuilder();
    IConfiguration configuration = configBuilder.AddAzureAppConfiguration(options =>
    {
        options.Connect(connectionString)
        .ConfigureRefresh(refreshOptions =>
        {
            refreshOptions.Register("Settings:BackgroundColor", refreshAll: true)
                          .SetCacheExpiration(TimeSpan.FromSeconds(10));
        });
    })
    .Build();

    return WebHost.CreateDefaultBuilder(args)
                  .ConfigureAppConfiguration((hostingContext, config) =>
                  {
                      config.AddJsonFile("appsettings.json").Build();
                      config.AddConfiguration(configuration);
                  })
                  .UseStartup<Startup>()
                  .Build();
}

Unfortunately, ChainedConfigurationProvider does not provide a way for others to access its child providers. This caused the issue that the App Configuration middleware could not find the instance of AzureAppConfigurationProvider (because in this case its under ChainedConfigurationProvider) and thus cannot obtain the instance of IConfigurationRefresher, which is responsible for refreshing the application configuration (our relevant middleware code here).

Would it be possible to expose the underlying providers in ChainedConfigurationProvider so that we can extract AzureAppConfigurationProvider and enable users to leverage our dynamic refresh functionality?

Issue author: avanigupta
Assignees: -
Milestone: -

@maryamariyan maryamariyan added untriaged New issue has not been triaged by the area owner and removed untriaged New issue has not been triaged by the area owner labels Nov 11, 2020
@maryamariyan maryamariyan added this to the Future milestone Nov 12, 2020
@avanigupta
Copy link
Contributor Author

With the upcoming .NET 6 release, when WebApplicationBuilder.Build() is called, all configuration providers will be consolidated under ChainedConfigurationProvider by default.
As a result, .NET 6 applications will not be able to use AzureAppConfigurationProvider for dynamically refreshing their config. We need to be able to access child providers under ChainedConfigurationProvider to address this issue. Is there any update on when this will be possible?

@maryamariyan
Copy link
Member

@avanigupta are you still blocked by this or you already have the workaround?

/cc @halter73

@avanigupta
Copy link
Contributor Author

@maryamariyan , we are still blocked since there's no clean workaround for this.

@halter73
Copy link
Member

halter73 commented Dec 2, 2021

With the upcoming .NET 6 release, when WebApplicationBuilder.Build() is called, all configuration providers will be consolidated under ChainedConfigurationProvider by default.

I investigated this with @avanigupta before the 6.0 release and we confirmed that this is no longer the case in the final release. However, I realize the original issue wasn't about a regression in 6.0, but instead was asking for the ability to enumerate a ChainedConfigurationProvider's child providers which likely would have avoided the regression we saw in preview releases.

I think the proposal is basically:

namespace Microsoft.Extensions.Configuration
{
    public class ChainedConfigurationProvider : IConfigurationProvider, IDisposable
    {
+        /// <summary>
+        /// The chained <see cref="IConfigurationProvider"/> instances.
+        /// </summary>
+        IEnumerable<IConfigurationProvider> Providers { get; }

And then the usage in AzureAppConfigurationRefresherProvider would be updated like this:

// This code is from the ctor of a singleton service that injects IConfiguration configuration.

var configurationRoot = configuration as IConfigurationRoot;
var refreshers = new List<IConfigurationRefresher>();

if (configurationRoot != null)
{
+   void AddRefreshers(IEnumerable<IConfigurationProvider> providers)
+   {
-   foreach (IConfigurationProvider provider in configurationRoot.Providers)
+   foreach (IConfigurationProvider provider in providers)
    {
        if (provider is IConfigurationRefresher refresher)
        {
            // Use _loggerFactory only if LoggerFactory hasn't been set in AzureAppConfigurationOptions
            if (refresher.LoggerFactory == null)
            {
                refresher.LoggerFactory = _loggerFactory;
            }

            refreshers.Add(refresher);
        }
+       else if (provider is ChainedConfigurationProvider chainedProvider)
+       {
+           AddRefreshers(chainedProvider.Providers);
+       }
    }
+   }
+
+   AddRefreshers(configurationRoot.Providers);
}

I didn't bother reindenting the code now in the local void AddRefreshers(IEnumerable<IConfigurationProvider> providers) function to make the diff more readable.

This is being done so a custom service can reference the configuration providers added by the call to AddAzureAppConfiguration. At the point configuration is added, no services exist yet to register the providers with and the IConfigurationBuilder does not give any sort of access to the IServiceCollection.

In an ideal world, there would be a single API to both add the AzureAppConfigurationSource to the IConfigurationBuilder and add service descriptors to the IServiceCollection that reference the added AzureAppConfigurationSource and any AzureAppConfigurationProviders it creates. This would work even if someone created their own version of ChainedConfigurationProvider unlike the API proposed above.

@avanigupta What happens if someone tries to call AddAzureAppConfiguration on a ConfigurationBuilder they've newed up themselves that never chain it to the IConfigurationBuilder provided by Host or WebHost? Would config never get refreshed because there are no services?

@Edgaras91
Copy link

Google took me here, I have a similar issue.

in Program.cs I build my IConfiguration, then I pass it into WebHostBuilder()UseConfiguration(config) that uses "UseStartup().
I inject the IConfiguration into the startup constructor, and it too loses all the providers, and is replaced by a single ChainedConfigurationProvider.

I am on Core 3.1, and It fails to register the Azure Configurations too, at IApplicationBuilder.UseAzureAppConfiguration()

Is there a workaround for my scenario?

@halter73
Copy link
Member

The easiest workaround probably would be to build your configuration using the IConfigurationBuilder in IWebHostBuilder.ConfigureAppConfiguration(Action<WebHostBuilderContext, IConfigurationBuilder> configureDelegate).

Or, if you can upgrade to .NET 6, you could use WebApplicationBuilder.Configuration ConfigurationManager.

@Edgaras91
Copy link

Thanks, @halter73 for that. Both are good solutions for us (not perfect), as a short term solution I applied your first suggestion and refactored my own code to this:

          await new WebHostBuilder()
                 .ConfigureAppConfiguration((context, configBuilder) => { configBuilder.AddConfigs(); })

.AddConfigs() is the custom method into which I moved all of our IConfigurationBuilder additions and ended up calling this AddConfigs() twice.
Hopefully, the Azure Configurations integration (IConfigurationBuilder.AddAzureAppConfiguration) doesn't pull configs twice - yet to be tested.

@Edgaras91
Copy link

I was able to resolve it without duplicating the builder code, by configuring the DI using ConfigureServices and passing the instance in:

new WebHostBuilder()
     .ConfigureServices(services => { services.AddSingleton<IConfiguration>(config); })
     .UseStartup<Startup>()

That resulted in getting the ConfigurationRoot with its providers correctly.

@maryamariyan maryamariyan added api-ready-for-review API is ready for review, it is NOT ready for implementation blocking Marks issues that we want to fast track in order to unblock other important work and removed api-suggestion Early API idea and discussion, it is NOT ready for implementation labels Mar 22, 2022
@bartonjs
Copy link
Member

bartonjs commented Apr 5, 2022

Video

During discussion it was believed that exposing the IConfiguration directly instead of inventing the IEnumerable for it was a better approach.

namespace Microsoft.Extensions.Configuration
{
    public partial class ChainedConfigurationProvider : IConfigurationProvider, IDisposable
    {
        public IConfiguration Configuration { get; }
    }
}

@bartonjs bartonjs added api-approved API was approved in API review, it can be implemented and removed blocking Marks issues that we want to fast track in order to unblock other important work api-ready-for-review API is ready for review, it is NOT ready for implementation labels Apr 5, 2022
@ghost ghost added the in-pr There is an active PR which will close this issue when it is merged label Apr 5, 2022
maryamariyan added a commit to maryamariyan/runtime that referenced this issue Apr 5, 2022
@ghost ghost removed the in-pr There is an active PR which will close this issue when it is merged label Apr 6, 2022
@ghost ghost locked as resolved and limited conversation to collaborators May 7, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
api-approved API was approved in API review, it can be implemented area-Extensions-Configuration
Projects
None yet
Development

Successfully merging a pull request may close this issue.

8 participants