Feature/58 aws options from appsettings #61

devklick · 2021-09-17T12:26:27Z

The changes in this PR are to address issues #58 and #57 - Adding support to take pieces of configuration from an appsettings file, rather than all configuration having to be specified in code.

Reading from config

Can be enabled by calling the SecretsManagerConfigurationProviderOptions.ReadFromConfigSection method and optionally specifying the name of the config section to be read from. See example app for usage.

Supported options

Several options can be specified in config - see the example config section in README for those that are supported.

Falling back on AWS config

The default AWS config section will be used as a fallback source for the AWS client region & credentials profile if this information is not specified in the custom config section. See README for more.

Mixing appsettings and hardcoded config

Options can be taken from a combination of appsettings & hardcoded, but some rules apply - see README for more.

Still to be done

Unit tests

…region from AWSOptions if required

Kralizek

Thank you for this PR. Added few comments, waiting for the unit tests.

Kralizek · 2021-09-20T09:26:05Z

README.md

+and both will be used. The same applies for the `ListSecretsFilters` collection. For all other properties, the code-based configuration 
+will take precedence.


Is it really "code-based precedence will take precedence" or simply "last called wins"?

Kralizek · 2021-09-20T09:26:21Z

README.md

-If you would like to build this project locally, just execute the `build.cake` script.
+If you would like to build this project locally, just exe7ute the `build.cake` script.


I guess this is a typo

Kralizek · 2021-09-20T09:27:22Z

...sions.Configuration.AWSSecretsManager/Internal/SecretsManagerConfigurationProviderOptions.cs

+        /// <summary>
+        /// Determines whether or not the options for the configuration provider should be read from configuration.
+        /// </summary>
+        public bool ReadFromConfig { get; set; }


I would avoid shortenings.

Suggested change

public bool ReadFromConfig { get; set; }

public bool ReadFromConfiguration { get; set; }

Kralizek · 2021-09-20T09:28:02Z

...sions.Configuration.AWSSecretsManager/Internal/SecretsManagerConfigurationProviderOptions.cs

+        /// <summary>
+        /// The name of the configuration section containing the options.
+        /// </summary>
+        public string ConfigSectionName { get; set; } = SecretsManagerConfigurationSection.DefaultConfigSectionName;


Same as above.

Suggested change

public string ConfigSectionName { get; set; } = SecretsManagerConfigurationSection.DefaultConfigSectionName;

public string ConfigurationSectionName { get; set; } = SecretsManagerConfigurationSection.DefaultConfigSectionName;

Kralizek · 2021-09-20T09:28:29Z

...sions.Configuration.AWSSecretsManager/Internal/SecretsManagerConfigurationProviderOptions.cs

+        /// Enables the extension to read options from an IConfigurationSection.
+        /// </summary>
+        /// <param name="configSectionName">The name of the configuration section containing the options.</param>
+        public void ReadFromConfigSection(string configSectionName = SecretsManagerConfigurationSection.DefaultConfigSectionName)


I'd go for ReadFromConfiguration instead.

Suggested change

public void ReadFromConfigSection(string configSectionName = SecretsManagerConfigurationSection.DefaultConfigSectionName)

public void ReadFromConfiguration(string configSectionName = SecretsManagerConfigurationSection.DefaultConfigSectionName)

Another option could be UseConfiguration. What do you think?

Kralizek · 2021-09-20T09:31:54Z

...zek.Extensions.Configuration.AWSSecretsManager/Internal/SecretsManagerConfigurationSource.cs


        public RegionEndpoint? Region { get; set; }

        public IConfigurationProvider Build(IConfigurationBuilder builder)
        {
-            var client = CreateClient();
+            var client = CreateClient(); 


Extra space.

Kralizek · 2021-09-20T09:32:38Z

src/Kralizek.Extensions.Configuration.AWSSecretsManager/SecretsManagerExtensions.cs

+                options.AcceptedSecretArns?.AddRange(smConfig.AcceptedSecretArns);
+
+            options.PollingInterval ??= smConfig?.PollingIntervalInSeconds != null
+                ? TimeSpan.FromSeconds(smConfig.PollingIntervalInSeconds.Value)


I think we should support TimeSpan.Parse as well here.

Kralizek · 2021-09-20T09:32:56Z

src/Kralizek.Extensions.Configuration.AWSSecretsManager/SecretsManagerExtensions.cs

+            if (profile == null)
+                return FallbackCredentialsFactory.GetCredentials();


Suggested change

if (profile == null)

return FallbackCredentialsFactory.GetCredentials();

if (profile == null)

{

return FallbackCredentialsFactory.GetCredentials();

}

Kralizek · 2021-09-20T09:34:52Z

README.md

+## Configuration via appsettings.json
+Many options can be specified in a JSON file rather than hardcoded within the app. To do so, 
+the `ReadFromConfigSection` method should be called, like so:


I would rather specify that many options can be specified using the Microsoft Configuration system without limiting to the JSON file. After all, the configuration values can be provided using other providers.

Kralizek · 2021-09-20T09:35:24Z

README.md

+```csharp
+builder.AddSecretsManager(configurator: options => 
+{
+    options.ReadFromConfigSection();


Align to new name.

Co-authored-by: Renato Golia <[email protected]>

savornicesei · 2022-11-07T12:06:03Z

Hi @Kralizek, @devklick
What needs to be done to get this PR merged?

Thank you.

savornicesei · 2022-11-07T17:53:58Z

src/Kralizek.Extensions.Configuration.AWSSecretsManager/Internal/AwsConfigSection.cs

+        /// The location of the file containing the credentials profiles.
+        /// </summary>
+        public string ProfilesLocation { get; set; }
+    }


I've been thinking if it's better for the extension to handle everything or if it is worth taking a dependency on AWSSDK.Extensions.NETCore.Setup .

Unfortunately AWSOptions? options = settings.GetAWSOptions("AWS:SecretsManager"); from the mentioned AWS package does not initialize AWSCredentials for some reason.
To overcome this I added a new method to IConfiguration : GetAWSOptionsWithCredentials that also sets the options.Credentials.

Using AWSSDK.Extensions.NETCore.Setup will allow smooth integration with localstack through localstack-dotnet-client

I'd like to keep this library as supple and lean as possible.

Yeah, makes sense

Kralizek · 2022-11-08T11:13:17Z

I'm considering closing this PR because I am doubtful about importing the whole authentication and request signing into this library. There are still good bits that should be taken like support for configuration file. I will open a new PR with that part and make sure that @devklick will get recognition for their work on this PR that was instrumental to understand the problem space and how to go forward.

devklick added 6 commits September 7, 2021 21:35

Updated SecretsManagerConfigurationSource to try and credentials and …

85fc387

…region from AWSOptions if required

Updated to read AWS options and secrets manager options from config

67aa42e

Removed AWSSDK.Extensions.NETCore.Setup dependency

b0a6ae0

Fixed bug where binding to the AWS Filter model was silently failing

8089de2

Added sample application to demonstrate usage of ReadFromConfigSection

fd9c4f5

Updated README

1e3f4b8

Kralizek requested changes Sep 20, 2021

View reviewed changes

jcanady20 and others added 3 commits September 4, 2022 19:15

Replace Newtonsoft with System.Text.Json (Kralizek#71)

32c71d1

Co-authored-by: Renato Golia <[email protected]>

Update AWS SDK package to 3.7.2.0

66bd753

Merge branch 'master' into feature/58-aws-options-from-appsettings

6dd88a5

savornicesei reviewed Nov 7, 2022

View reviewed changes

This was linked to issues Nov 8, 2022

Take AWS options from appsettings #58

Open

Is it possible to provide AcceptedArns through config file? #57

Open

Kralizek force-pushed the master branch 3 times, most recently from 433e32f to 2b70308 Compare November 8, 2022 14:37

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Feature/58 aws options from appsettings #61

Feature/58 aws options from appsettings #61

devklick commented Sep 17, 2021

Kralizek left a comment •

edited

Loading

Kralizek Sep 20, 2021

Kralizek Sep 20, 2021

Kralizek Sep 20, 2021

Kralizek Sep 20, 2021

Kralizek Sep 20, 2021

Kralizek Sep 20, 2021

Kralizek Sep 20, 2021

Kralizek Sep 20, 2021

Kralizek Sep 20, 2021

Kralizek Sep 20, 2021

savornicesei commented Nov 7, 2022

savornicesei Nov 7, 2022 •

edited

Loading

Kralizek Nov 7, 2022

savornicesei Nov 8, 2022

Kralizek commented Nov 8, 2022 •

edited

Loading

		and both will be used. The same applies for the `ListSecretsFilters` collection. For all other properties, the code-based configuration
		will take precedence.

		If you would like to build this project locally, just execute the `build.cake` script.
		If you would like to build this project locally, just exe7ute the `build.cake` script.

	public bool ReadFromConfig { get; set; }
	public bool ReadFromConfiguration { get; set; }

	public string ConfigSectionName { get; set; } = SecretsManagerConfigurationSection.DefaultConfigSectionName;
	public string ConfigurationSectionName { get; set; } = SecretsManagerConfigurationSection.DefaultConfigSectionName;

	public void ReadFromConfigSection(string configSectionName = SecretsManagerConfigurationSection.DefaultConfigSectionName)
	public void ReadFromConfiguration(string configSectionName = SecretsManagerConfigurationSection.DefaultConfigSectionName)

		if (profile == null)
		return FallbackCredentialsFactory.GetCredentials();

Feature/58 aws options from appsettings #61

Are you sure you want to change the base?

Feature/58 aws options from appsettings #61

Conversation

devklick commented Sep 17, 2021

Reading from config

Supported options

Falling back on AWS config

Mixing appsettings and hardcoded config

Still to be done

Kralizek left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

savornicesei commented Nov 7, 2022

savornicesei Nov 7, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Kralizek commented Nov 8, 2022 • edited Loading

Kralizek left a comment •

edited

Loading

savornicesei Nov 7, 2022 •

edited

Loading

Kralizek commented Nov 8, 2022 •

edited

Loading