Allow auditing to be configured and enabled via environment variables#7535
Allow auditing to be configured and enabled via environment variables#7535abparticular merged 5 commits intomasterfrom
Conversation
abparticular
force-pushed
the
configurable-auditing
branch
from
4d31291 to
b4c146b
Compare
|
Looks good but I am wondering (I haven't been working on the core recently) if using environment for config is a new concept in core or something established? If it is a brand new concept then maybe we should explore the .NET configuration APIs to be able to bind the config values from any source, not just environment? |
|
Using environment for config is a new concept in core. The configuration APIs were explored but there didn't seem to be a way to meaningfully use them without a larger integration effort into core. |
| /// </summary> | ||
| public Audit() | ||
| { | ||
| Prerequisite(context => context.Settings.GetOrDefault<bool>("Audit.Enabled"), $"Auditing was disabled via the `{IsEnabledEnvironmentVariableKey}` environment variable setting"); |
There was a problem hiding this comment.
Ideally, there would be no changes required to the feature to implement this. I'd prefer to see the changes just in EndpointConfiguration, such that the environment variables just act as if the APIs were called to configure auditing.
If this is related to precedence of API vs. environment variables, it seems like you should be able to move where you're reading the environment variables to allow them to take precedence if that's what you want.
There was a problem hiding this comment.
If the feature remains untouched, then when auditing was disabled via env var, the start up diagnostics would report that auditing was disabled because no configured audit queue was found, which would be inaccurate.
There was a problem hiding this comment.
I still want to take a deeper look at this, but I'll open a follow-up PR with any changes.
|
Why does this PR exist? Can this be linked to a TF. Second, why only audit and why not only put this support in NServiceBus.Extensions.Hosting and properly use Exceptions.Configuration? |
I didn't mean to use the word ONLY, but ALSO. There was a comment that this would take an additional dependency which is not the case. NServiceBus.Extensions.Hosting has a dependency to Microsoft.Extentions.Hosting, the package has dependencies on pretty much ALL extension packages as it contains the host builder API that use all these. Here a 5 minute spike of what I meant for reference: If the envvar provider is enabled (which it is for both host and web builders) that should work but not only for envvar or appsettings.json but any configured provider like Azure KeyVault. |
Inverted logic of the "IsEnabled" env var to be "Disabled" instead
abparticular
force-pushed
the
configurable-auditing
branch
from
6ad7544 to
f39f9ec
Compare
|
I have raised #7610 as a follow up |
…#7610) * Revert "Merge pull request #7535 from Particular/configurable-auditing" This reverts commit 8c7e371, reversing changes made to 09bae00. * Allow auditing to be configured and enabled via environment variable * Better tests * Comment --------- Co-authored-by: Daniel Marbach <danielmarbach@users.noreply.github.com>
|
Released in NServiceBus 10.1.0 |
7 participants
This PR allows auditing to be configured via environment variables to facilitate better control of the audit capability enabled/disabled state, without requiring compiling and redeploying of endpoint instances.
Refer to the Configuring auditing documentation for details.
A follow-up PR is at #7610