Duplicate SSM parameter with different Case cause the whole SSM parameter fail to load

[malah-code]

Duplicate SSM parameter with different Case cause the whole SSM parameter fail to load

[normj]

@malah-code I debated the same thing about Optional but if you look at the docs for the AddJson method here it says the Optional property means "Whether the file is optional.". I'm interpreting that as it is optional the file exists not that it should ignore errors with the file. Which seems to prove true because when I make my appsettings.Development.json file have unparseable JSON I get the error Failed to load configuration file <file>. So following that pattern I think it is safe for us to throw exceptions if the data coming back from SSM is invalid like duplicate keys.

[ashishdhingra]

When running the sample with multiple parameters with same name but differing in case, the error An item with the same key has already been added. Key: OracleConnectionString is returned. Used package Amazon.Extensions.Configuration.SystemsManager version 5.0.2. Reproduction code is below:
.csproj

<Project Sdk="Microsoft.NET.Sdk.Web">

  <PropertyGroup>
    <TargetFramework>net6.0</TargetFramework>
    <Nullable>enable</Nullable>
    <ImplicitUsings>enable</ImplicitUsings>
  </PropertyGroup>

  <ItemGroup>
    <PackageReference Include="Amazon.Extensions.Configuration.SystemsManager" Version="5.0.2" />
  </ItemGroup>

</Project>

Program.cs

using Microsoft.Extensions.Configuration;

var builder = WebApplication.CreateBuilder(args);
builder.Services.AddRazorPages();

builder.Configuration.AddSystemsManager("/dotnet-aws-samples/systems-manager-sample");
var app = builder.Build();

if (!app.Environment.IsDevelopment())
{
    app.UseExceptionHandler("/Error");
    app.UseHsts();
}

app.UseHttpsRedirection();
app.UseStaticFiles();

app.UseRouting();

app.UseAuthorization();

app.MapRazorPages();

app.Run();

Pages\Index.cshtml.cs

using Microsoft.AspNetCore.Mvc;
using Microsoft.AspNetCore.Mvc.RazorPages;

namespace TestParameterStoreAspNet.Pages
{
    public class IndexModel : PageModel
    {
        private readonly ILogger<IndexModel> _logger;
        private readonly IConfiguration Configuration;

        public IndexModel(ILogger<IndexModel> logger, IConfiguration configuration)
        {
            _logger = logger;
            Configuration = configuration;
        }

        public ContentResult OnGet()
        {
            var defaultConnectionString = Configuration["OracleConnectionString"];
            return Content($"{defaultConnectionString}");
        }
    }
}

@malah-code You mentioned expected behavior to throw an error. Are you not getting error in your case?

Thanks,
Ashish

[malah-code]

@ashishdhingra , Note that I use configureSource.Optional = true, because SSM is not the only source of my settings, so, I don't get any error, and when I debug it, I found that all errors are ignored in this line
https://github.com/aws/aws-dotnet-extensions-configuration/blob/master/src/Amazon.Extensions.Configuration.SystemsManager/SystemsManagerConfigurationProvider.cs#L125

catch (Exception ex)
            {
                if (Source.Optional) return;
               // Rest of code ignored and error not throw because the source is Source.Optional=true
           }

Also note that I use optional=true because we need to take default values from other settings, but the SSM parameter (if exists) should overwrite at the end , so we use the code like that in program.cs

                    configurationBuilder
                        .SetBasePath(Directory.GetCurrentDirectory())
                        .AddJsonFile("appsettings.json", true, false)
                        .AddJsonFile($"appsettings.{Environment.GetEnvironmentVariable("ASPNETCORE_ENVIRONMENT")}.json", true, false)
                        .AddEnvironmentVariables()
                        .AddSystemsManager(configureSource =>
                          {
                                   configureSource.Path = "/your/path/here"; 
                                   configureSource.Optional = true;
                                   double ReloadAfterInMinutes = 5;
                         });

[ashishdhingra]

@ashishdhingra , Note that I use configureSource.Optional = true, because SSM is not the only source of my settings, so, I don't get any error, and when I debug it, I found that all errors are ignored in this line
https://github.com/aws/aws-dotnet-extensions-configuration/blob/master/src/Amazon.Extensions.Configuration.SystemsManager/SystemsManagerConfigurationProvider.cs#L125

@malah-code The probable reason is that when optional is true, the error should not be thrown while fetching such parameters. I would not recommend changing the logic to load the latest duplicate SSM parameter, since that would not be ideal and could result in undesired behavior for some scenarios. Throwing an error makes more sense in case duplication is observed. This needs review with the team.

[normj]

What we should be doing if we get this error is throw an exception even if Optional is set to true. The Optional flag imeans the existence of the settings which I view also including the existence of AWS credentials. If there is an error handling the data that we get back we should be throwing an error.

[malah-code]

I can't find clear documentation of the Optional parameter, but I think as far as I understand from this thread, The Optional property determines whether the configuration source is optional. If set to true, the application will not throw an exception if the configuration source is not found or cannot be loaded. If set to false, an exception will be thrown if the configuration source is not found or cannot be loaded.
So, If that correct, so I agree the code should not throw an exception in case of bad SSM parameter, but I suggest to do below logic

• First we need to have some kind of logs, to make it easy for develop to understand what's going on, imagine I did mistake in SSM like adding same key twice by different case, or chosing the wrong KMS for the secret key and app not have permission to that KMS, so in this case ALL data coming from SSM parameters will be ignored without anything show error in logs.
• I suggest in case of Optional, and in case of one SSM key has errors, we could bypass this key only, and log warning in the default logger.

[malah-code]

Just a proposed solution to provide Tolerant Implementation in case of duplicate parameters that may located in SSM with different CASE (like \Path1\Param1 and \path1\param1 , only first one will be taken).
.
this is like additional option for user to use instead of the DefaultParameterProcessor by using

        public static IWebHostBuilder CreateWebHostBuilder(string[] args) =>
            WebHost.CreateDefaultBuilder(args)
                .ConfigureAppConfiguration(builder =>
                {
                    builder.AddSystemsManager(config => {
                        config.Path = "/my-application/";
                        config.ParameterProcessor = new TolerantDefaultParameterProcessor();
                    });
                })
                .UseStartup<Startup>();