DuplicateParameterException can break a production application

[tysonstewart]

Describe the bug

The extension throws a DuplicateParameterException when a "duplicate" parameter is detected because the keys are case-insensitive in code but case-sensitive in Parameter Store. This means that an application that uses this extension on startup as describe in the documentation can be prevented from starting simply by adding a key to Parameter Store--no code changes.

Expected Behavior

The extension should gracefully handle a duplicate key, probably by letting the last value win since .NET configuration works that way anyway.

Current Behavior

The extension throws an exception. Handling the exception isn't really an option since the rest of the configuration won't be available.

Reproduction Steps

1. Add a value to Parameter Store, e.g. /myApp/myKey
2. Add the prefix to the configuration using the application extension method:

   builder.Configuration.AddSystemsManager("/myApp");

3. Optionally start the app to show that it works
4. Add a new value to Parameter Store having the same key as step 1 but with different casing, e.g. /myApp/mykey
5. Restart the app and note that it throws a DuplicateParameterException on startup

Possible Solution

I don't consider the addition of a configuration parameter to be an action that could cause an application to fail to start. While the conflict in keys is something I would definitely want to know about, I don't want the application to not start at least in production. My suggested behavior is allowing the last value to win, though perhaps the exact behavior could be configurable itself.

Additional Information/Context

No response

AWS .NET SDK and/or Package version used

Amazon.Extensions.Configuration.SystemsManager 6.2.0

Targeted .NET Platform

.NET Core 7.0

Operating System and version

N/A

[ashishdhingra]

@tysonstewart It's the expected behavior since .NET Framework would not allows duplicate keys (it is case insensitive). We should not be picking up last value since it could lead to unpredictable results (please refer #146 (comment) for more details).

CC @normj for any additional inputs.

[tysonstewart]

I understand that argument, but there's an important difference between loading configuration from a JSON file and loading configuration from Parameter Store: the file is almost certainly static and deployed with the application while Parameter Store is a live system subject to change during the application's lifetime. Furthermore, because #145 was closed, as developers, we don't have any way to mitigate this scenario. I can catch the exception but none of the configuration from Parameter Store is available now.

Perhaps I'm not using a recommended pattern, but my implementation uses the environment as part of the path, e.g. /MyApp/Staging/Key and /MyApp/Production/Key which means that it's possible to not catch a misconfiguration in a lower environment. I don't believe "be careful with what keys you add to Parameter Store" is a good answer here. I need a way to make it safe to make a mistake.

[normj]

@tysonstewart If you add the configuration as separate calls for staging and production are you seeing the duplicate exception? Something like this:

builder.Configuration.AddSystemsManager("/myApp/Staging");
builder.Configuration.AddSystemsManager("/myApp/Production");

Within a individual configuration source I feel pretty strongly that we should throw the duplicate exception because the order of what is last is not deterministic and applications could get surprising behavior that changes over time. As separate configurations sources I agree we should not throw a duplicate exception because in that case the order is deterministic based on the order of calls to add configuration sources. I think .NET itself handles later configuration sources overriding the previous sources so I'm not sure there is anything the library has to do to make that happen.

[tysonstewart]

We don't load it like that, but rather like this:

builder.Configuration.AddSystemsManager($"/myAppGlobal/{builder.Environment.EnvironmentName/");
builder.Configuration.AddSystemsManager($"/myApp/{builder.Environment.EnvironmentName/");

So only an environment's configuration is loaded. The trouble we ran into is that a dev was testing something in Staging and added duplicate parameters in /myApp/Stage, both brand new, without noticing that there was a single character with different casing. And even though the values weren't being used, when the stage application next started, it threw an exception and failed to start. This was a mystery to the responders because no code had changed.

Fortunately, this happened to us in the stage environment but it's terrifying that making a simple mistake like this could hit us in production and it wouldn't show up until a deployment and for a reason completely unrelated to changes being deployed.

[normj]

@tysonstewart In your use case if we didn't have the duplicate exception and when the dev inadvertently added the duplicate there would be no control which duplicate would get picked. So instead of a crashing behavior that brought the issue to your attention right away you could have a runtime bug that was possibly using the wrong database connection string and corrupting data or sending production data to staging. There are times when picking the last would win but the there would be terrible runtime times when the wrong duplicate was picked.

[tysonstewart]

Right, and I understand that, but again, in this specific case it wasn't even being used--no code was consuming the values that were created. Nonetheless it prevented the application from starting.

I'm not arguing why you added this safeguard. I'm saying that in my case, there is no danger of corrupting data or sending it to the wrong place. Maybe an API key is incorrect and that particular service is down or uses the wrong account, but that's preferable to the entire application being offline while Ops scrambles to figure out what just happened.

[normj]

In your case the duplicate config value wasn't used so it wouldn't be problem but this library has no idea what values are going to be used or not. We could add an opt-in flag to ignore duplicate and use one of the values at random but who would use that flag?

I'm assuming some of the config values being pulled are being used and it would have been bad if the wrong value was used. So if the dev caused a duplicate for a critical config setting and you turn that flag on your system could get corrupted. Would you really leave yourself open for that type of corruption by turning this opt-in flag on?