aws-dotnet-extensions-configuration icon indicating copy to clipboard operation
aws-dotnet-extensions-configuration copied to clipboard

Published 20 hours ago • verified publisher icon aws

FIX ISSUE : IF Duplicate SSM parameter with different Case cause the whole SSM parameter fail to load

Open malah-code opened this issue 1 year ago • 4 comments

Fix issue #146 - SSM parameter case senstive but dotnet config is not, so if we have duplicate SSM parameters with different case, it will not load the ssm params at all.

Description

Fix issue (edge case), if we have duplicate SSM parameters with different case (like /param/one and /Param/ONE) like below parameter, the whole SSM parameters not added to the config at all, and it's completely ignored with no error message or log.

  • /Connection/OracleConnection
  • /Connection/ORACLECONNECTION
  • /connection/oracleconnection

Motivation and Context

this is fix an open issue with name "Duplicate SSM parameter with different Case cause the whole SSM parameter fail to load"

Testing

Before fix, create below SSM parameters

  • /Connection/OracleConnection
  • /Connection/ORACLECONNECTION
  • /Connection/anyOtherParam

Run the application to read from /Connection prefix, you will notice no ssm parameter loaded in the config even anyOtherParam. After apply fix, you will notice OracleConnection and anyOtherParam will appear in the dotnet config.

Types of changes

Bug fix (non-breaking change which fixes an issue)

Checklist

  • [ Y] My code follows the code style of this project
  • [ N] My change requires a change to the documentation
  • [ N] I have updated the documentation accordingly
  • [ Y] I have read the README document
  • [ N] I have added tests to cover my changes
  • [Y ] All new and existing tests passed

License

  • [ Y] I confirm that this pull request can be released under the Apache 2 license

malah-code avatar May 04 '23 04:05 malah-code

@malah-code Thanks for the contribution. As mentioned in the https://github.com/aws/aws-dotnet-extensions-configuration/issues/146#issuecomment-1540525422, it might not be recommended 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. This would need review with the team if throwing an error would make more sense.

ashishdhingra avatar May 09 '23 21:05 ashishdhingra

Thanks for the PR. I commented on the parent issue that we should throw an exception if we are in this error condition of duplicate keys. Silently continuing picking the last value will cause unpredictable behavior for users of the library.

normj avatar May 12 '23 18:05 normj

@ashishdhingra / @normj , thanks for review, I understand your idea, and totally agree that silently continuing picking the last value will cause unpredictable behaviour for users of the library, but do you agree that we at least we need to log warning with enough information whenever error happened in the DefaultParameterProcessor ?. I'm saying that because it took me hours to detect this issue in my SSM parameters, and I had to take the library source code to debug to know the root cause. let me know if that make sense.

malah-code avatar May 12 '23 19:05 malah-code

@ashishdhingra / @normj , thanks for review, I understand your idea, and totally agree that silently continuing picking the last value will cause unpredictable behaviour for users of the library, but do you agree that we at least we need to log warning with enough information whenever error happened in the DefaultParameterProcessor ?. I'm saying that because it took me hours to detect this issue in my SSM parameters, and I had to take the library source code to debug to know the root cause. let me know if that make sense.

@malah-code Looking at the design of the library, the exception could be logged by the consumer of this library as would be the case with any other exception (like failing to load the parameter). Please advise if you would be amending the PR or contribute a new PR (closing this one) for

  • Throwing an exception in case of duplicate parameters for this use case.
  • For optional parameters, if the parameter is not found or fail to load, it should use the existing logic to silently ignore the error.

Hence for fixing this, the simple fix could be to catch specific exception raised for duplicate keys while loading parameters (DefaultParameterProcessor or JsonParameterProcessor), perhaps in SystemsManagerConfigurationProvider.LoadAsync().

Thanks, Ashish

ashishdhingra avatar May 16 '23 17:05 ashishdhingra

Closing this PR in favor of https://github.com/aws/aws-dotnet-extensions-configuration/pull/171

ashishdhingra avatar Jun 11 '24 17:06 ashishdhingra