Azure / azure-webjobs-sdk

Azure WebJobs SDK
MIT License
738 stars 358 forks source link

WebJobsHostBuilderExtensions.ConfigureWebJobs should not add configuration sources #1931

Closed Skovvart closed 8 months ago

Skovvart commented 6 years ago

When configuring the HostBuilder the order of ConfigureWebJobs and ConfigureAppConfiguration is significant, as ConfigureWebJobs adds "appsettings.json" and environment variables as configuration sources. This can lead to invalid configuration values and unexpected behavior, as the order of configuration sources defined in ConfigureAppConfiguration is not the definitive list of configuration sources.

Repro steps

  1. Create a new HostBuilder, call ConfigureAppConfiguration with one or more configuration sources that has values that should take precedence over appsettings.json and/or environment variables

  2. Call ConfigureWebJobs on the host builder

  3. Access a configuration value (example: in hostBuilder.ConfigureServices((context, services) => { var configValue = context.Configuration["ExampleValue"]; })

  4. Build and run the Host.

Expected behavior

ConfigureWebJobs should not manipulate configuration sources at all, or at the very least have an option to disable the behavior. Configuration values should appear as defined in ConfigureAppConfiguration.

Actual behavior

The rogue configuration sources are added to the configuration sources and may provide invalid configuration values.

Known workarounds

Call ConfigureWebJobs before ConfigureAppConfiguration. The rogue application sources are still added, but the desired sources takes precedence.

Related information

acds commented 6 years ago

To tack on here related but slightly different (given minimal docs) how do you determine the environment such that you can load an environment specific appsettings.json ?

Skovvart commented 6 years ago

I'd say the general-purpose answer would be var environment = System.Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT") ?? "Development" (or another appropriate settings and default), but I wouldn't say it's really related.

thomaslevesque commented 5 years ago

What's the official stance on this? This behavior is clearly incorrect, as it silently overrides the user defined configuration

brettsam commented 5 years ago

@fabiocav should weigh in on this as well. If I recall, the intention was for this to be similar to WebHost.CreateDefaultBuilder() (https://github.com/aspnet/AspNetCore/blob/master/src/DefaultBuilder/src/WebHost.cs#L162-L184), where a bunch of "standard" wire-up is done for you and then you can overwrite those services/sources in subsequent calls. The idea was that you call this first, then go from there.

I agree that's not entirely clear, though. Perhaps there should be a CreateDefaultWebJobsBuilder() that makes it clear we're doing some initial wire-up -- and ConfigureWebJobs() should just do this?: https://github.com/Azure/azure-webjobs-sdk/blob/85bff2b622f172f5792685cc9eca5c14cf284554/src/Microsoft.Azure.WebJobs.Host/Hosting/WebJobsHostBuilderExtensions.cs#L34-L40

thomaslevesque commented 5 years ago

Perhaps there should be a CreateDefaultWebJobsBuilder() that makes it clear we're doing some initial wire-up -- and ConfigureWebJobs() should just do this?:

Sounds good to me!

fhurta commented 5 years ago

I just spent a day tracking down why my configuration does not work. I was adding Key Vault configuration and environment specific settings which seemed to be quite easy task but after deployment to Azure it didn't work. I really didn't expect that configuring webjobs add appsettings.json on top of my configuration effectively reverting all the specific changes

This is quite old issue, still not fixed, causing unnecessary waste of time and workarounds.. 👎

nathansoz commented 5 years ago

I spent an evening looking at this in our service and filed what I guess is a duplicate bug here #2247

Is there a plan to fix this?

fabiocav commented 5 years ago

I agree that the current behavior is not ideal and unexpected. We need to be careful though, as changing this behavior would be a breaking change (customers may be relying on the current behavior), so we need to introduce an API that is clear while preserving the current functionality.

@brettsam

If I recall, the intention was for this to be similar to WebHost.CreateDefaultBuilder()

Yes, that was the original design.

Zenuka commented 5 years ago

I agree that it would be a breaking change but I can't really think of a scenario which you want this behavior so I would go for changing the current behavior, but that's just me ;-)

andriysavin commented 4 years ago

@fabiocav You could introduce a new set of ConfigureWebJobs with a flag parameter, or add such parameter to the existing methods with default value (though this would be a breaking change). However, there is high risk that the flag will get unnoticed and everybody will face the issue at least initially. I had to re-implement this set of extension methods under ConfigureWebJobsWithoutConfig name as a workaround. The name is not ideal, and I would prefer the original ConfigureWebJobs had more cryptic name, and this one to be just ConfigureWebJobs.

aorlenko83 commented 4 years ago

Just ran into this, after spending couple of hours troubleshooting the issues with configuration of my webjobs. Fixed just by placing ConfigureWebJobs method closer to the starting of my host configuration. The fact that any app configuration is taking place within ConfigureWebJobs method is really not intuitive. Any progress on this one? I definately support the idea of creating new CreateDefaultWebJobsBuilder() method, likewise the asp.net core. This is familiar in general, and also quite intuitive. And it guarantees that it is called first and so we have default app configuration go first, which can be overriden if needed. And on the other hand, since ConfigureWebJobs is extension method, so there's high probability that it is called after ConfigureAppConfiguration method, and this is breaking things.

robertmclaws commented 4 years ago

I just ran into this issue as well. The offending lines in ConfigreWebJobs should just be removed. Best practices should be to call Host.CreateDefaultBuilder() before calling ConfigureWebJobs(). I'm using the WebJobs in a unique way for SimpleMessageBus, and because of this, I have to manually remove the registered ConfigurationProviders from the collection after the fact with code like this:

var configProviders = ((hostContext.Configuration as ConfigurationRoot).Providers as List<IConfigurationProvider>);

if (configProviders.Count(c => c.GetType() == typeof(JsonConfigurationProvider) && (c as JsonConfigurationProvider).Source.Path == "appsettings.json") > 1)
{
    configProviders.Remove(configProviders.Last(c => c.GetType() == typeof(JsonConfigurationProvider) && (c as JsonConfigurationProvider).Source.Path == "appsettings.json"));
}

if (configProviders.Count(c => c.GetType() == typeof(EnvironmentVariablesConfigurationProvider)) > 1)
{
    configProviders.Remove(configProviders.Last(c => c.GetType() == typeof(EnvironmentVariablesConfigurationProvider)));
}

Couldn't figure out why my code was breaking... it's entirely not obvious that this behavior is happening.

If you're concerned about a breaking change @fabiocav, then just do the opposite of my code above, and check to see if those providers are registered before adding them yourself. That way no one is broken, and the correct behavior just works.

bassem-mf commented 4 years ago

Here is how I remove the configuration sources added silently by ConfigureWebJobs() and add the configuration sources explicitly.

var hostBuilder = new HostBuilder();

hostBuilder.ConfigureWebJobs(b =>
    {
        b.AddAzureStorageCoreServices();
        b.AddAzureStorage();
    });

hostBuilder.ConfigureAppConfiguration(configurationBuilder =>
{
    // Replace the configuration sources added by ConfigureWebJobs()
    configurationBuilder.Sources.Clear();

    configurationBuilder.AddJsonFile("appsettings.json", false, true);
    configurationBuilder.AddJsonFile($"appsettings.{environment}.json", true, true);
    if (environment == ProgramEnvironment.Development)
        configurationBuilder.AddUserSecrets<Program>();
});
fabiocav commented 4 years ago

@robertmclaws @bassem-mf , this hasn't been assigned, but I wanted to comment to make make you aware that your feedback is not being ignored :)

We have a plan to update this with the proper behavior (again, as mentioned above, the current behavior was not really intentional), but haven't picked this item up due to competing priorities. We'll have this assigned soon.

taixes commented 4 years ago

Overwriting appsettings only with AddJsonFile("appsettings.json) is a bad idea indeed. It should not override anything or use both AddJsonFile("appsettings.json) and .AddJsonFile($"appsettings.{env.EnvironmentName}.json", like ConfigureWebJobs. Actually, I think having a CreateDefaultWebjobBuilder, analogous to CreateDefaultBuilder (with AddEnvironmentVariables, all the logging functionality, etc), it would be great.

ScottyMac52 commented 4 years ago

I am REALLY struggling with this same issue in Azure Functions for dot net core 3.x! When I run locally my local.settings.json is loaded and all is well. After I publish the Azure Function to Azure all of the sudden my custom settings are null? Here is the code I am currently using to load my configuration:

internal static IHost Configure(Action<IServiceCollection, IConfiguration> addtionalServices = null)
{
    return Host.CreateDefaultBuilder()
        .ConfigureAppConfiguration((context, builder) =>
        {
            builder.AddJsonFile("appsettings.json", optional: false);
            builder.AddJsonFile($"appsettings-{context.HostingEnvironment.EnvironmentName}.json", optional: true);
            builder.AddEnvironmentVariables();
        })
        .ConfigureServices((context, services) =>
        {
            //  Configure additional services and configurations
            addtionalServices?.Invoke(services, context.Configuration);
        })
        .Build();
}

I am using VS 2019 and dot net core 3.1 to author Azure Functions in C#. I have really struggled for most of 2 days with this issue! I ran extensive testing locally and then it just started losing my configuration settings after it was published. Thanks!

ScottyMac52 commented 4 years ago

I FINALLY found a way to make this work. Many many many thanks to @bassem-mf for his solution created a light at the end of the tunnel!

internal static IHost Configure2(Action<IServiceCollection, IConfiguration> addtionalServices = null, ILogger logger = null, ExecutionContext context = null)
{
    var hostBuilder = new HostBuilder();
        hostBuilder.ConfigureWebJobs(b =>
    {
        b.AddAzureStorageCoreServices();
        b.AddAzureStorage((configureQueues) =>
        {
            configureQueues.MaxDequeueCount = 1;
        });
    });
    hostBuilder.ConfigureAppConfiguration(configurationBuilder =>
    {
        // Replace the configuration sources added by ConfigureWebJobs()
        configurationBuilder.Sources.Clear();
        var fileLocation = Path.Combine(context?.FunctionAppDirectory ?? Environment.CurrentDirectory, "appsettings.json");
        logger?.LogInformation($"Should load config from {fileLocation}");
        configurationBuilder.AddJsonFile(fileLocation, false, true);
    });
    hostBuilder.ConfigureServices((context, services) =>
    {
        //  Configure additional services and configurations
        addtionalServices?.Invoke(services, context.Configuration);
    });
    return hostBuilder.Build();
}

Now I KNOW that the appsettings.json file is loaded for my function app!

Agendum commented 4 years ago

I use Host.CreateDefaultBuilder() to create my host, but a setting in my appSettings.json was overriding a setting in my appSettings.Production.json. Eventually I figured out that ConfigureWebJobs() also adds appSettings.json. This is pretty dumb -- ConfigureWebJobs() has no business touching my app configuration this way and the breaking change just needs to happen to fix this.

As a work-around, what I did was create a wrapping class around IHostBuilder which just forwards all work to the default implementation, except calls to ConfigureAppConfiguration() are just dropped. This allows me to allow ConfigureWebJobs() to do all it's work but filter out app configuration changes.

public static class WebJobsHostBuilderExtensions
{
    public static IHostBuilder ConfigureWebJobsWithoutAppConfiguration(
        this IHostBuilder hostBuilder,
        Action<IWebJobsBuilder> configure,
        Action<JobHostOptions> configureOptions = null)
    {
        if (hostBuilder is null)
        {
            throw new ArgumentNullException(nameof(hostBuilder));
        }

        IHostBuilder hostBuilderWithoutAppConfiguration = new HostBuilderWithoutAppConfiguration(hostBuilder);

        hostBuilderWithoutAppConfiguration.ConfigureWebJobs(
            configure ?? (webJobsBuilder => { }),
            configureOptions ?? (configureOptions => { }));

        return hostBuilder;
    }

    public static IHostBuilder ConfigureWebJobsWithoutAppConfiguration(
        this IHostBuilder hostBuilder,
        Action<HostBuilderContext, IWebJobsBuilder> configure = null,
        Action<JobHostOptions> configureOptions = null)
    {
        if (hostBuilder is null)
        {
            throw new ArgumentNullException(nameof(hostBuilder));
        }

        IHostBuilder hostBuilderWithoutAppConfiguration = new HostBuilderWithoutAppConfiguration(hostBuilder);

        hostBuilderWithoutAppConfiguration.ConfigureWebJobs(
            configure ?? ((hostBuilderContext, webJobsBuilder) => { }),
            configureOptions ?? (configureOptions => { }));

        return hostBuilder;
    }

    private class HostBuilderWithoutAppConfiguration : IHostBuilder
    {
        private readonly IHostBuilder _hostBuilder;

        public HostBuilderWithoutAppConfiguration(IHostBuilder hostBuilder)
        {
            _hostBuilder = hostBuilder ?? throw new ArgumentNullException(nameof(hostBuilder));
        }

        public IDictionary<object, object> Properties => _hostBuilder.Properties;

        public IHost Build() => _hostBuilder.Build();

        public IHostBuilder ConfigureAppConfiguration(Action<HostBuilderContext, IConfigurationBuilder> configureDelegate)
        {
            return this;
        }

        public IHostBuilder ConfigureContainer<TContainerBuilder>(Action<HostBuilderContext, TContainerBuilder> configureDelegate)
        {
            _hostBuilder.ConfigureContainer(configureDelegate);
            return this;
        }

        public IHostBuilder ConfigureHostConfiguration(Action<IConfigurationBuilder> configureDelegate)
        {
            _hostBuilder.ConfigureHostConfiguration(configureDelegate);
            return this;
        }

        public IHostBuilder ConfigureServices(Action<HostBuilderContext, IServiceCollection> configureDelegate)
        {
            _hostBuilder.ConfigureServices(configureDelegate);
            return this;
        }

        public IHostBuilder UseServiceProviderFactory<TContainerBuilder>(IServiceProviderFactory<TContainerBuilder> factory)
        {
            _hostBuilder.UseServiceProviderFactory(factory);
            return this;
        }

        public IHostBuilder UseServiceProviderFactory<TContainerBuilder>(Func<HostBuilderContext, IServiceProviderFactory<TContainerBuilder>> factory)
        {
            _hostBuilder.UseServiceProviderFactory(factory);
            return this;
        }
    }
}
david-campbell commented 4 years ago

The way I got around this (VERY ANNOYING BUG ) was to remove the EnvironmentVariables. So that my appsettings.{environment}.json wouldn't override them. Once it is added. I can then call configApp.AddEnvironmentVariables(); to get them in the right order.

var builder = new HostBuilder()
            .ConfigureWebJobs(webJobConfiguration =>
            {
                webJobConfiguration.AddTimers();
                webJobConfiguration.AddAzureStorageCoreServices();
            }).ConfigureAppConfiguration((hostContext, configApp) =>
            {
                // Remove the environment configuration source added by ConfigureWebJobs()
                if (configApp.Sources != null && configApp.Sources.Count > 0) {
                    List<IConfigurationSource> RemoveSources = configApp.Sources.Where(cs => cs is Microsoft.Extensions.Configuration.EnvironmentVariables.EnvironmentVariablesConfigurationSource).ToList();
                    if (RemoveSources.Any())
                    {
                        RemoveSources.ForEach(r => configApp.Sources.Remove(r));
                    }
                 }
                configApp.SetBasePath(Directory.GetCurrentDirectory());
                        configApp.AddJsonFile($"appsettings.{Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT")}.json", optional: true, reloadOnChange: true);
                        configApp.AddEnvironmentVariables();
             })
            .ConfigureServices((context, services) =>
            {
                services.AddSingleton<IConfiguration>(context.Configuration);
                services.AddSingleton<TimmerJob>();
                services.AddTransient<IUpdateService, UpdateService>();
            })
            .Build();
            builder.Run();
robertmclaws commented 4 years ago

Out of curiosity to those that came up with other solutions, did my 7 LoC solution not work?

david-campbell commented 4 years ago

Out of curiosity to those that came up with other solutions, did my 7 LoC solution not work?

I already implemented my fix before I found this thread. I just didn't quite understand why it was happening until I found this thread.

Agendum commented 4 years ago

Out of curiosity to those that came up with other solutions, did my 7 LoC solution not work?

I really appreciate your contribution, and I am sure it works great in your application. The reason I chose to go my route was because I don't want to make any assumptions of how the underlying configuration providers are configured, their order, or what they are pointing to. Your solution assumes no other code adds configuration settings, especially environment ones (i.e. a new prefix). I don't like those hidden side-effects. In fact all solutions in this thread are just mucking with the configuration providers, but all of them could have side-effects and will probably cause future bugs if WebJobs does decide to truly fix this with a breaking change.

Instead I wanted to fix the problem at its root and just disable WebJobs ability to cause the problem in the first place, without affecting any other component. So not only does my solution have zero potential side-effects on any other component, it likely is future-proof on WebJobs too since it just disables its ability to run ConfigureAppConfiguration.

thomaslevesque commented 4 years ago

I'm not a fan of manually removing the config sources... I just use a modified version of ConfigureWebJobs, which does what ConfigureWebJobs should be doing.

robertmclaws commented 4 years ago

So, the way the Configuration API works is, the registration order matters, because the data is merged all the way down. If the same Provider with the same FileName is registered more than one later down the chain, that file will override any previous merges.

So, regardless of what the WebJobs SDK is doing, You should never add a configuration provider more than once with the same file path, or you will negate any previous merges.

The code I posted was heavily tested and sould not break in any situation because it checks to see if the provider has been registered more than once for the same path, and removes any subsequent registrations.

So in my application, I put those calls in an extension method (like .FixWebJobsHost()) that gets called immediately after .AddWebJobs(), which means if you add your dependencies after that, the way you're supposed to, your DI container should be configured properly, whether the WebJobs SDK fixes the problem at some point in the future, or not.

OTOH, the other solutions introduce the potential for breakage if the WebJobs SDK changes their API surface at any point in the future... at least, if I am reading them correctly.

Just food for thought. HTH!

david-campbell commented 4 years ago

I don't love my solution.. I just posted it as an alternative.. it doesn't remove all as suggested. it removes all with a type of EnvironmentVariablesConfigurationSource. My issue was my environment fille was being registered after the environment variables. so..wrong order. it fixed my issue.. but I actually went back and switched this use an extension implementation. and I'll likely review your implementation again. as usual..my code will be reviewed and improved many times ... ;)

BC89 commented 4 years ago

I've noticed this too and believe it plays into my current issue with ApplicationInsights. No matter what i do in my config settings, the built in webjobs logger nuget Microsoft.Azure.WebJobs.Logging.ApplicationInsights 3.0.18 wants to default to LogLevel == Information which is a lot of chatter/noise. In code I can override this by setting the log level in the configuration builder.

builder.ConfigureLogging((context, b) =>
            {
                b.AddConsole();

                string instrumentationKey = context.Configuration["APPINSIGHTS_INSTRUMENTATIONKEY"];
                if (!string.IsNullOrEmpty(instrumentationKey))
                {
                    b.AddApplicationInsightsWebJobs(o => o.InstrumentationKey = instrumentationKey);
                    b.SetMinimumLevel(LogLevel.Warning);
                }
            });

`
Annoyingly, doing this workaround then kills the DI telemetryclient from logging TrackEvents.

public Sync(TelemetryClient telemetryclient)
        {
            _telemetryclient = telemetryclient;
           _telemetryclient.TrackEvent("22a");
        }

image

-BC

brettsam commented 4 years ago

@BC89 -- that seems like a different issue -- can you spin this off into a new issue and tag me in it?

bryancrosby commented 2 years ago

It's been over 3 years since this issue was opened -- is there any chance this might get a fix? This still appears to be broken in Microsoft.Azure.WebJobs 3.0.32 (target net60)

fartwhif commented 2 years ago

It's been over 3 years since this issue was opened -- is there any chance this might get a fix?

v3.0.31 here, this please

ilmax commented 2 years ago

This is causing so much confusion because the ConfigureWebJobs does half of the job that Host.CreateDefaultBuilder does (see here) for example it's not configuring logging filters from appSettings and so on. Using Host.CreateDefaultBuilder results in issues reading configurations because the appSettings.Json is added after the eventual call to ConfigureAppConfiguration.

It has always been tricky to get azure functions and web job reading configuration properly, not sure why not stick to the widely used and understood asp.net core model.

We have a plan to update this with the proper behavior (again, as mentioned above, the current behavior was not really intentional), but haven't picked this item up due to competing priorities. We'll have this assigned soon.

@fabiocav is it possible to get this done? We've been waiting for quite a while now...

fartwhif commented 2 years ago

The longer it goes unaddressed the wider and deeper the hole people have to dig to work around it. Because of that this would have been an extreme priority if it were up to me. Putting it in the "who cares just work around it for now" bin is self-destructive in this case and now you've given yourselves a dilemma, or it will soon grow into a dilemma. Even after this internal technical debt is corrected then anyone who has given special attention to this because of the original deviation (and those who haven't) will potentially be in for a nasty surprise when the correction is published - the combined impact is potentially massive for a bug like this and it's proportional to usage count which should be increasing over time. A lot of things I think will break after you correct this. If you can correct it without breaking dependent projects that would be ideal, but I don't know if that's possible - it would have been far easier to prioritize this properly from the onset. The continuing decision to delay is probably, if not already, going to deter some from using your tool and having an undue deleterious effect on the image of associated products.

robertmclaws commented 2 years ago

@liliankasem There are a bunch of PRs in this repo that have not been accepted. If I create PR for this, could it please be prioritized for inclusion in the next release?

jeroenbongers commented 2 years ago

@brettsam @fabiocav I just spent 2 hours figuring out why on earth my appsetting.{environment}.json was not overwriting my appsetting.json. It turns out it was because the ConfigureWebJobs was adding an additional appsettings.json which kept getting priority.

Can something please be done about this, it's extremely confusing when developing.

robertmclaws commented 2 years ago

@jeroenbongers Here's a method that can help:

        /// <summary>
        /// The WebJobs SDK (for some reason) calls the Host.AddAppConfiguration for you. The holders for those methods are internal, and the last entry in the pipe overrides previous merging.
        /// So we have to delete the unnecessary ConfigurationProviders until the WebJobs team fixes their mistake.
        /// </summary>
        /// <param name="hostContext"></param>
        private static void FixWebJobsRegistration(HostBuilderContext hostContext)
        {
            var configProviders = ((hostContext.Configuration as ConfigurationRoot).Providers as List<IConfigurationProvider>);

            if (configProviders.Count(c => c.GetType() == typeof(JsonConfigurationProvider) && (c as JsonConfigurationProvider).Source.Path == "appsettings.json") > 1)
            {
                configProviders.Remove(configProviders.Last(c => c.GetType() == typeof(JsonConfigurationProvider) && (c as JsonConfigurationProvider).Source.Path == "appsettings.json"));
            }

            if (configProviders.Count(c => c.GetType() == typeof(EnvironmentVariablesConfigurationProvider)) > 1)
            {
                configProviders.Remove(configProviders.Last(c => c.GetType() == typeof(EnvironmentVariablesConfigurationProvider)));
            }
        }

Call this as the very first line of code inside ConfigureServies (ie HostBuilder.ConfigureServices((context, config) => { FixWebJobsRegistration(context); }); and you should be good to go.

I'm trying to get Microsoft to let me more actively contribute to this project so I can fix this bug.

liliankasem commented 2 years ago

@robertmclaws let me follow up on this internally. It sounds like @fabiocav and @brettsam worry about breaking changes here and we may need a design in place before addressing it.

robertmclaws commented 2 years ago

@liliankasem Thanks for responding! Really appreciate you.

No disrespect to @fabiocav and @brettsam, but if they believe there is a problem, they should be having that discussion on here so that the community can triage it together. It has been an issue for 3 years now, and the fix is (and has been) relatively simple. It can be done without breaking changes (something that can easily be verified with the right unit tests). I'm happy to commit a fix, but I need to be reasonably certain that my PR is not going to sit open waiting to be reviewed for years.

For reference, I'm a member of the .NET Foundation, I manage the OData Restier project for Microsoft, and I have a product called SimpleMessageBus that turns the WebJobs SDK into an out-of-band message processor that powers major systems for the State of Florida, a nationwide restaurant chain, a startup called BurnRate, and public crypto projects.

My team heavily depends on this project, and I'd like to be able to have my company work with your team to contribute fixes. Thanks for your time! :)

Note: this comment has been edited to remove content that violated this project’s code of conduct

fabiocav commented 2 years ago

We’ve discussed this issue in the review/triage (as it was flagged by @liliankasem) and before the end of the current sprint (Wednesday 8/31), will be sharing details on how we plan to address this and gathering feedback.

Thank you all for the feedback and patience.

fabiocav commented 2 years ago

As discussed, we wanted to provide an update to share the plans on how this issue will be addressed:

1. The behavior of the ConfigureWebJobs API will change to conditionally add the configuration providers

The ConfigureWebJobs API will continue to register the following configuration providers:

Moving forward, similar to one of the workarounds provided above, the API will perform a check against the existing/registered providers and skip registration if a provider with the same settings is already present.

2. API documentation updates

The XML documentation on the ConfigureWebJobs API will be updated to clearly describe the behavior of the API and what is registered with that call.

Additional steps (optional)

These are additional options we've discussed and wanted to get feedback on (not in scope for the initial set of changes). This would be done in addition to the above:

1. A new API for WebJobs service registration will be added

The ConfigureWebJobs API will be refactored, with the logic responsible for configuration and service registrations split. The service registration will be made available as an IServiceCollection extension method, enabling users to register the core WebJobs services only.

2. A new API to create a default builder will be introduced

An new API to create the builder will be introduced. This uses a pattern familiar to many developers (similar to WebApplication.CreateBuilder, which will have the same behavior as the updated ConfigureWebJobs API and guarantee the registration order.

We expect PRs for those items to be landing soon, but welcome feedback on the approach.

Thank you again for the help and patience!

robertmclaws commented 2 years ago

First off, I'm glad the team is finally giving this the attention it deserves. Having digested this a bit, here is my feedback: I would humbly suggest that this plan take the opposite approach.

Step outside of the fact that you folks built this framework years ago, and ask yourselves how you would build it NOW if you started over? The whole point of the Host.CreateDefaultBuilder() mechanism is that the developer should be deciding how the host system is configured, not you.

Why should the SDK even be presuming to know anything about how the Configuration system is set up? That's not it's job! It's job is to register services in DI, simply USE the configuration that is available, and start watching for events to be processed. That's it. If configuration is not registered, then the Host should fail to finish loading. NOT override and break people who were already doing the right thing.

So the priority should be to bring the API up to date with modern IHost development practices, so that using it on .NET 6 or 7 doesn't feel as out of place as it does today. I would recommend the following pattern:

This would be the pattern of record moving forward. I would do this first, get alignment on how the pattern works, write unit tests to back up the separation of concerns, and heavily XML Documentation comment both public AND private methods.

Then in the same release, to fix the people that are currently broken, I would:

This way, you maintain backwards compatibility, direct people to get off the old system, and have a new way for developers to register WebJobs that maintains Separation of Responsibility and doesn't try to outsmart anyone involved.

As someone who spent months hacking the WebJobs platform to be an out-of-band queued event processor that can work on-prem or in the cloud with zero code changes, this is now I would introduce these changes inside my own codebase for my own customers. I hope this helps, and am happy to participate in the code changes to make this happen.

ilmax commented 2 years ago

I'm also here to second what @robertmclaws said. I'd like to just use Host.CreateDefaultBuilder() which configure a bunch of things the way developers expects it (e.g. logging filter from appsettings just to name one). I'm happy this is finally being taken into account, I just wish that the webjob experience was as close as possible to the asp.net core one, without some of the annoyances we have today.

fabiocav commented 2 years ago

Thank you for the feedback!

@robertmclaws about the the first point:

AddWebJobs(this IServiceCollection services) that would register the services with the container (similar to what we did here, but without the configuration hacks we had to use to work around the current limitations),

Is exactly what is described in the first item I've described in the additional steps above. So I think we're aligned there.

@ilmax with the proposed changes, you'll be able to use Host.CreateDefaultBuilder() and have the ability to add WebJobs without hitting the problem initially described in this issue. With what is being proposed, considering the additional steps, can you share more about where you believe the experience would diverge from what you're doing with ASP.NET today? Can you detail the issues impacting you might not have captured and addressed here?

robertmclaws commented 2 years ago

@fabiocav Yep, I think the work is pretty closely aligned, and I hope I conveyed that properly. My concern was more in line with stated priorities + execution order, and directing people off the current codepath long-term. It feels to me like setting up for the right future can't be less of a priority than fixing a past that people are already having to work around. If we get this done in one atomic unit, then people can make the decision to maintain binary compatibility or set themselves up for a solid future in one motion.

ilmax commented 2 years ago

@fabiocav I am mostly concerned about keep registering configuration sources in the ConfigureWebJobs method, but thinking about this a bit more, I think if we make sure no provider will be re-registered if they're present already, that should be fine. The issues I'd like to solve once and for all are the difficulty to reason about configuration that's order dependent, that what's bothers me the most with the current implementation, so having that resolved and being able to use Host.CreateDefaultBuilder() will solve it for me.

mathewc commented 2 years ago

I've got a PR out https://github.com/Azure/azure-webjobs-sdk/pull/2911 for a couple of the initial improvements we discussed above. For anyone interested, please provide feedback on that PR, thanks :)

ilmax commented 2 years ago

@mathewc Added few nits, looks good to me, Thanks!

jameswoodley commented 1 year ago

Any idea when this may be released? PR was merged over 5 months ago, but I'm still seeing the erroneous behavior

liliankasem commented 1 year ago

The change was released as part of v3.0.36 of the WebJobs SDK and should be in the latest version of the host (v4.15+)

fabiocav commented 8 months ago

Closing stale/resolved issue.