If preload is disabled, KeyVaultConfigBuilder throws an ArgumentNullException

aspnet / MicrosoftConfigurationBuilders

Microsoft.Configuration.Builders

MIT License

122 stars 61 forks source link

If preload is disabled, KeyVaultConfigBuilder throws an ArgumentNullException #161

Closed heaths closed 2 years ago

heaths commented 3 years ago

https://github.com/aspnet/MicrosoftConfigurationBuilders/blob/22371104020c6540446f86f5102990ac7ed8f02c/src/Azure/AzureKeyVaultConfigBuilder.cs#L102-L105

If _preload is false, then _allKeys is null and https://github.com/aspnet/MicrosoftConfigurationBuilders/blob/22371104020c6540446f86f5102990ac7ed8f02c/src/Azure/AzureKeyVaultConfigBuilder.cs#L132 ends up throwing an ArgumentNullException citing the source parameter for Enumerable.Contains.

heaths commented 3 years ago

FWIW, preloading secrets from Key Vault - especially when your Key Vault has many secrets (1 per app is recommended) - can lead to rate throttling, especially in clustered apps starting up all at once.

Instead, the Key Vault team recommends using AppConfig with a link to Key Vault for general configuration needs, where AppConfig can have links to secrets that Microsoft.Extensions.Configuration.AzureAppConfiguration can resolve by default, though it's not hard to achieve from any configuration builder.

karshinlin commented 3 years ago

+1 on this issue. More details from customer reported incidents:

the preload setting is default True for the AzureKeyVaultConfigurationBuilder. We've seen this ArgumentNullException from _allKeys being null when this builder is used for the section (not sure if this is a red herring).
possible theory is a race condition in the initialization logic, where the _lazyInitialized flag is set before the secrets are actually preloaded from Key Vault: https://github.com/aspnet/MicrosoftConfigurationBuilders/blob/22371104020c6540446f86f5102990ac7ed8f02c/src/Base/KeyValueConfigBuilder.cs#L283-L295 Initialized here: https://github.com/aspnet/MicrosoftConfigurationBuilders/blob/22371104020c6540446f86f5102990ac7ed8f02c/src/Base/KeyValueConfigBuilder.cs#L138 But preload logic is farther down in the initialization logic: https://github.com/aspnet/MicrosoftConfigurationBuilders/blob/22371104020c6540446f86f5102990ac7ed8f02c/src/Azure/AzureKeyVaultConfigBuilder.cs#L102-L105

peterox commented 3 years ago

Is there a fix or workaround for this issue?

peterox commented 3 years ago

@heaths I think @karshinlin was on to the real cause of this issue. The keyvault config builder as well as the json config builder both suffer from a problem where the class is not fully initialised and can cause various exceptions. I've just created a pull request that fixes the race condition.

heaths commented 3 years ago

Yes, but making sure it's not null was more of a defense-in-depth resolution. With no tests available, if someone were to later change any initialization code, the problem could happen again. Perhaps the two PRs should be merged. If allKeys is null it wasn't initialized for whatever reason, so no reason to check it. This also obviates a state variable that was effectively trying to mimic that simple null check.

karshinlin commented 3 years ago

Thanks for tackling this @peterox @heaths! I agree with Heath in that both PRs may need to be merged. Peter's work ensures we avoid the race condition if we fail to finish initializing before another thread tries to request secrets. If there's an exception thrown (and silently swallowed if Optional=true), _allKeys may still be null which can be problematic in the next secret fetch (thus Heath's PR).

StephenMolloy commented 2 years ago

Fixed by #163. Well, and also by #134 which hopefully avoids any race in the first place. But I think a Lazy approach in #163 is also an upgrade in this scenario.