Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Provide a reloadOnChange setting to AddAzureKeyVault #1054

Merged
merged 34 commits into from Mar 5, 2019

Conversation

@HobbyProjects
Copy link
Contributor

commented Feb 1, 2019

Summary of the changes (Less than 80 chars)

  • Added a polling mechanism to AzureKeyVaultConfigurationProvider to implement the reloadOnChange behavior
  • Added a unit test to cover the feature

Addresses #771 (in this specific format)

zanawar and others added some commits Jan 28, 2019

Added 'ReloadOnChange' and 'ReloadPollDelay' members to the AzureKeyV…
…aultConfigurationSource model. Added ability to add AzureKeyVaultConfigurationProvider directly through the source model. Exposed optional boolean `ReloadOnChange` parameter on `AddAzureKeyVault` extension methods.
Merge pull request #1 from HobbyProjects/keyvault-reloading
KeyVault reloading configuration and constructors
Nima Poulad Nima Poulad
Updating the constructors based on product group conversation. Also, …
…addressed a PR feedback with delays in test.
Merge pull request #2 from HobbyProjects/keyvault-checks
Added the polling mechanism to provider and finished the unit test.

@HobbyProjects HobbyProjects requested a review from Tratcher as a code owner Feb 1, 2019

@dnfclas

This comment has been minimized.

Copy link

commented Feb 1, 2019

CLA assistant check
All CLA requirements met.

Nima Poulad Nima Poulad

@pakrym pakrym self-requested a review Feb 4, 2019

@pakrym
Copy link
Member

left a comment

Needs work.

@Tratcher

This comment has been minimized.

Copy link
Member

commented Feb 6, 2019

Fixed the CI with #1082, please rebase.

@HobbyProjects

This comment has been minimized.

Copy link
Contributor Author

commented Feb 7, 2019

@pakrym Please have a look. We've addressed the comments and added 4 new tests. Thanks.

@ryancammer

This comment has been minimized.

Copy link

commented Feb 21, 2019

We're looking forward to this change. Is there any reason this hasn't been reviewed and merged yet? Anything I can help with? Thanks @pakrym!

pakrym added some commits Feb 26, 2019

Show resolved Hide resolved ...t/ref/Microsoft.Extensions.Configuration.AzureKeyVault.netstandard2.0.cs Outdated
Show resolved Hide resolved ...iguration/Config.AzureKeyVault/src/AzureKeyVaultConfigurationProvider.cs Outdated
var tasks = new List<Task<SecretBundle>>(secrets.Count());
protected virtual async Task WaitForReload()
{
await Task.Delay(_reloadInterval.Value.Milliseconds, _cancellationToken.Token);

This comment has been minimized.

Copy link
@Tratcher

Tratcher Feb 28, 2019

Member

Does this throw when cancelled?

This comment has been minimized.

Copy link
@pakrym

pakrym Feb 28, 2019

Member

Yes, but it would only surface up to the background task.

Show resolved Hide resolved ...iguration/Config.AzureKeyVault/src/AzureKeyVaultConfigurationProvider.cs Outdated
foreach (var secretItem in secrets)
var tasks = new List<Task<SecretBundle>>();
var newLoadedSecrets = new Dictionary<string, LoadedSecret>();
var oldLoadedSecrets = Interlocked.Exchange(ref _loadedSecrets, null);

This comment has been minimized.

Copy link
@Tratcher

Tratcher Feb 28, 2019

Member

Why Interlocked? It looks like _loadedSecrets is only used in this method and this method shouldn't be called concurrently (no?).

This comment has been minimized.

Copy link
@pakrym

pakrym Feb 28, 2019

Member

In case someone calls Reload on configuration while automatic reload is in progress.

This comment has been minimized.

Copy link
@Tratcher

Tratcher Feb 28, 2019

Member

oldLoadedSecrets being null will give you inconsistent results on the overlapping refreshes. What if we guarded this at a higher level? If someone calls reload while an automatic refresh is in progress, let them await the one that's already happening and use those results. The same arrangement works in reverse as well.

This comment has been minimized.

Copy link
@pakrym

pakrym Feb 28, 2019

Member

oldLoadedSecrets being null will give you inconsistent results on the overlapping refreshes

It is eventually consistent. Both reloads would retrieve the correct set of secrets.

What if we guarded this at a higher level

I'm not sure if this option is simpler.

Show resolved Hide resolved ...iguration/Config.AzureKeyVault/src/AzureKeyVaultConfigurationProvider.cs Outdated

pakrym added some commits Feb 28, 2019

@pakrym pakrym requested a review from Tratcher Feb 28, 2019

@pakrym pakrym dismissed their stale review Feb 28, 2019

...

@@ -16,6 +17,17 @@ public static partial class AzureKeyVaultConfigurationExtensions
}
namespace Microsoft.Extensions.Configuration.AzureKeyVault
{
public partial class AzureKeyVaultConfigurationOptions
{
public AzureKeyVaultConfigurationOptions() { }

This comment has been minimized.

Copy link
@Tratcher

Tratcher Mar 2, 2019

Member

Do you need the no-paramater constructor? I'd think it would cause people to not discover the others.

This comment has been minimized.

Copy link
@pakrym

pakrym Mar 5, 2019

Member

I'd like to leave the ability to set everything manually without going through any custom logic

Show resolved Hide resolved ...uration/Config.AzureKeyVault/src/AzureKeyVaultConfigurationExtensions.cs Outdated
Show resolved Hide resolved ...iguration/Config.AzureKeyVault/src/AzureKeyVaultConfigurationProvider.cs Outdated
Show resolved Hide resolved ...uration/Config.AzureKeyVault/src/AzureKeyVaultConfigurationExtensions.cs
FB

@pakrym pakrym merged commit 3885348 into aspnet:master Mar 5, 2019

9 checks passed

Extensions-ci Build #20190305.7 succeeded
Details
Extensions-ci (Code check) Code check succeeded
Details
Extensions-ci (OSX debug) OSX debug succeeded
Details
Extensions-ci (OSX release) OSX release succeeded
Details
Extensions-ci (Ubuntu 16.04 debug) Ubuntu 16.04 debug succeeded
Details
Extensions-ci (Ubuntu 16.04 release) Ubuntu 16.04 release succeeded
Details
Extensions-ci (Windows debug) Windows debug succeeded
Details
Extensions-ci (Windows release) Windows release succeeded
Details
license/cla All CLA requirements met.
Details
@ryancammer

This comment has been minimized.

Copy link

commented Mar 15, 2019

Question: is the reloadOnChange functionality slated for the .NET Core 3.0 release, or will there be a 2.2.x release in the interim?

@pakrym

This comment has been minimized.

Copy link
Member

commented Mar 15, 2019

It would only go into 3.0 release.

@ryancammer

This comment has been minimized.

Copy link

commented Mar 15, 2019

Gotcha. TY.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.