GFlisch / Arc4u.Guidance.Doc

Other
5 stars 1 forks source link

More robust configuration initialization #13

Open vvdb-architecture opened 2 years ago

vvdb-architecture commented 2 years ago

The guidance currently generates this:

        .ConfigureAppConfiguration((hostingContext, config) =>
        {
            foreach (var source in config.Sources.Where(s => s.GetType().Name.Contains("Json")).ToList())
            {
                config.Sources.Remove(source);
            }

            var env = GetEnvironmentName();

            config.AddJsonFile("configs/appsettings.json", true, true);
            config.AddJsonFile($"configs/appsettings.{env}.json", true, true);
            config.AddJsonFile("configs/reverseproxy.json", true, true);
            config.AddJsonFile($"configs/reverseproxy.{env}.json", true, true);
        })

This is fragile because it is based on two undocumented assumptions:

The code only works because these undocumented assumptions happen to be correct.

We can debate if the removal of json-sources is really necessary, since they refer to optional appsettings.json/appsettings.{env}.json, which do not exist anyway in the new deployment. Leaving them in might even be useful to keep them to allow for some "emergency reconfiguration" by simply adding an appsettings.json in the base directory and restarting the project. But there may be security implications as well.

If we must remove the sources, a more robust way of writing this can get rid of the first assumption, but not the second:

        .ConfigureAppConfiguration((hostingContext, config) =>
        {
            for (int i = config.Sources.Count - 1; i >= 0; --i)
                if (config.Sources[i] is Microsoft.Extensions.Configuration.Json.JsonConfigurationSource)
                    config.Sources.RemoveAt(i);
            var executionPath = Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)!, "Configs");
            config.SetBasePath(executionPath);
            var env = GetEnvironmentName();
            config.AddJsonFile("appsettings.json", optional: true, reloadOnChange: true);
            config.AddJsonFile($"appsettings.{env}.json", optional: true, reloadOnChange: true);
            config.AddJsonFile("reverseproxy.json", optional: true, reloadOnChange: true);
            config.AddJsonFile($"reverseproxy.{env}.json", optional: true, reloadOnChange: true);
        })

Note that:

We can go a little further and communicate to the host what the environment name is. If we do that, we can just re-add any existing json file to reflect the current base path, thereby getting rid of the second blind assumption:

        .UseEnvironment(GetEnvironmentName())
        .ConfigureAppConfiguration((hostingContext, config) =>
        {
            var jsonSources = new List<string>();
            for (int i = config.Sources.Count - 1; i >= 0; --i)
                if (config.Sources[i] is Microsoft.Extensions.Configuration.Json.JsonConfigurationSource jsonConfigurationSource)
                {
                    jsonSources.Add(jsonConfigurationSource.Path);
                    config.Sources.RemoveAt(i);
                }
            var executionPath = Path.Combine(Path.GetDirectoryName(Assembly.GetExecutingAssembly().Location)!, "Configs");
            config.SetBasePath(executionPath);
            foreach (var jsonSource in jsonSources)
                config.AddJsonFile(jsonSource, optional: true, reloadOnChange: true);
            var env = hostingContext.HostingEnvironment.EnvironmentName;
            config.AddJsonFile("reverseproxy.json", optional: true, reloadOnChange: true);
            config.AddJsonFile($"reverseproxy.{env}.json", optional: true, reloadOnChange: true);
        })

The .UseEnvironment(GetEnvironmentName())) correctly propagates the environment to the host. After this, the hostingContext.HostingEnvironment.EnvironmentName can be used to check the environment.

On a related matter, the call to GetEnvironmentName() is currently defined as follows:

static string GetEnvironmentName()
{
    var env = System.Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT");
    if (string.IsNullOrWhiteSpace(env))
    {
        env = "Development";
    }
    return env;

There are predefined strings related to environment settings, which replace the magic string:

static string GetEnvironmentName()
{
    var env = System.Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT");
    if (string.IsNullOrWhiteSpace(env))
    {
        env = Environments.Development;
    }
    return env;
}

Finally there is code that checks if we are in a development scenario:

    if (app.Environment.IsDevelopment())
    {
        app.UseDeveloperExceptionPage();
    }

You expect this code to be execute in development contexts, including your local machine, but it won't: since the EnvironmentName is "Localhost" in that case and app.Environment.IsDevelopment() will return false.

We should provide in Arc4u some methods that apply specifically to our environment names:

 /// <summary>
    /// Elia-specific environments (in addition to the standard environments specified in <see cref="Microsoft.Extensions.Hosting.Environments"/>
    /// </summary>
    public static class EliaEnvironments
    {
        /// <summary>
        /// The Localhost environment is used on the user's developement machine. It is similar to "Debug" but differs in that it is not deployed on a separate debug server
        /// but runs locally.
        /// </summary>
        public static readonly string Localhost = "Localhost";

        /// <summary>
        /// The acceptance environment, which is the phase after Test but before Production.
        /// </summary>
        public static readonly string Acceptance = "Acceptance";
    }

 public static class HostEnvironmentExtensions
    {
        /// <summary>
        /// Checks if the current host environment name is <see cref="EliaEnvironments.Localhost"/>.
        /// </summary>
        /// <param name="hostEnvironment">An instance of <see cref="IHostEnvironment"/>.</param>
        /// <returns>True if the environment name is <see cref="EliaEnvironments.Localhost"/>, otherwise false.</returns>
        public static bool IsLocalhost(this IHostEnvironment hostEnvironment)
        {
            if (hostEnvironment == null)
                throw new ArgumentNullException(nameof(hostEnvironment));
            return hostEnvironment.IsEnvironment(EliaEnvironments.Localhost);
        }

        /// <summary>
        /// Checks if the current host environment name is <see cref="EliaEnvironments.Acceptance"/>.
        /// </summary>
        /// <param name="hostEnvironment">An instance of <see cref="IHostEnvironment"/>.</param>
        /// <returns>True if the environment name is <see cref="EliaEnvironments.Acceptance"/>, otherwise false.</returns>
        public static bool IsAcceptance(this IHostEnvironment hostEnvironment)
        {
            if (hostEnvironment == null)
                throw new ArgumentNullException(nameof(hostEnvironment));
            return hostEnvironment.IsEnvironment(EliaEnvironments.Acceptance);
        }
    }

... so that we can check for them as well. Ideally, we should align our terminology with dotnet and talk about "Staging" instead of "Acceptance", but there might be political reasons why this is not a good suggestion.

janschmidmaier50 commented 1 year ago

Quick question: Why do we need a special implementation at all? So what is the default .net implementation missing ?

vvdb-architecture commented 1 year ago

We would not need it if we would align with .NET's way of labelling and managing environments.

GFlisch commented 1 year ago

For .Net Development is the local machine and Dev(elopment) is used by dev team to deploy and test the application on a server. This is why I have introduced the localhost concept which is more accurate for me.

vvdb-architecture commented 1 year ago

Indeed, Localhost is a new environment specific for Elia, but the others could have been named like the predefined .NET constants. But, as mentioned in the original submission, they're might be political reasons why this is not a good suggestion.