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

[malah-code]

Describe the bug

If we have duplicate SSM parameters with different case like below, the whole SSM parameters not added to the config at all.

• /dotnet-aws-samples/systems-manager-sample/OracleConnectionString
• /dotnet-aws-samples/systems-manager-sample/ORACLECONNECTIONSTRING
• /dotnet-aws-samples/systems-manager-sample/oracleconnectionstring

note: this is applicable, because AWS SSM parameter names are case sensitive.

this is because of the code in DefaultParameterProcessor, that doesn't take into consideration is edge case.

Expected Behavior

Just ignore the duplicate SSM parameter and continue, or load the latest duplicate SSM parameters , or even throw error, that will be acceptable too (when configureSource.Optional = true).

Current Behavior

silently ignores ALL the SSM parameters and not add any of them to the config.

Reproduction Steps

to reproduce, setup the sample project, and add below parameters to the AWS SSM , and use configureSource.Optional = true

configurationBuilder.AddSystemsManager(configureSource =>
                        {
               configureSource.Path = "/your/path/here"; 
               configureSource.Optional = true;
})

• /dotnet-aws-samples/systems-manager-sample/sampleone
• /dotnet-aws-samples/systems-manager-sample/SAMPLEONE
• /dotnet-aws-samples/systems-manager-sample/SampleOne

reload the project, you will not find any config value coming from AWS SSM at all.

Possible Solution

this pull request should have the fix #145
summary
we just add 2 lines of code in DefaultParameterProcessor.cs file, to prevent duplicated keys and take first one only instead of throw error

                .GroupBy(parameter => parameter.Key, StringComparer.OrdinalIgnoreCase)
                .ToDictionary(group => group.Key, group => group.First().Value, StringComparer.OrdinalIgnoreCase);

So the final method will be like that

        public virtual IDictionary<string, string> ProcessParameters(IEnumerable<Parameter> parameters, string path)
        {
            return parameters
                .Where(parameter => IncludeParameter(parameter, path))
                .Select(parameter => new
                {
                    Key = GetKey(parameter, path),
                    Value = GetValue(parameter, path)
                })                
                .GroupBy(parameter => parameter.Key, StringComparer.OrdinalIgnoreCase)
                .ToDictionary(group => group.Key, group => group.First().Value, StringComparer.OrdinalIgnoreCase);
        }

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

"AWSSDK.AppConfigData" Version="3.7.0.23"
"AWSSDK.Extensions.NETCore.Setup" Version="3.7.1"
"AWSSDK.SimpleSystemsManagement" Version="3.7.12.9"
"Microsoft.Extensions.Configuration" Version="2.0.*"

Targeted .NET Platform

net6.0

Operating System and version

AmazonLinux, and MacOs 13.3.1

[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 means 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.

[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.

[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>();

[ashishdhingra]

Created PR #171

[ashishdhingra]

Amazon.Extensions.Configuration.SystemsManager 6.2.0 released containing the change. Kindly note that this is a breaking change and exception would be thrown if duplicate Systems Manager parameter keys (case insensitive) are detected irrespective of whether the parameter is optional or not.

[github-actions]

⚠️COMMENT VISIBILITY WARNING⚠️

Comments on closed issues are hard for our team to see.
If you need more assistance, please either tag a team member or open a new issue that references this one.
If you wish to keep having a conversation with other community members under this issue feel free to do so.