Allow auditing to be configured and enabled via environment variables #7535
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.
|
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? |
| var auditEnabledValue = Environment.GetEnvironmentVariable(Features.Audit.IsEnabledEnvironmentVariableKey); | ||
| Settings.SetDefault("Audit.Enabled", auditEnabledValue is null || !bool.TryParse(auditEnabledValue, out var isEnabled) || isEnabled); | ||
|
|
||
| var defaultAuditQueue = Environment.GetEnvironmentVariable(Features.Audit.AddressEnvironmentVariableKey); | ||
| if (defaultAuditQueue is not null) | ||
| { | ||
| Settings.Set("Audit.Address", defaultAuditQueue); | ||
| } |
There was a problem hiding this comment.
Conform to extensions.configuration behavior:
- Please ensure this is case-insensitive, so also support
NSERVICEBUS__AUDIT__ISENABLED - Please support both variations so with
:and__to behave similarly to extensions.configuration.
This isn't really following POSIX "standards":
- Usually just the existence of a key often is sufficient. Consider complying to the robustness principle and also accept
1as valid value, support uppercase (POSIX convention) as envvar keys are case-sensitive. IsEnabledis not very common for envvars, likely better to just useEnable. This would result inNSERVICEBUS__AUDIT__ENABLEwhich IMHO is better.
There was a problem hiding this comment.
Synced with @abparticular , preferring full uppercase as is common for envvars.
Not support 1 as that is not compatible with Extentions.Configuration
My comment on : was incorrect, that is invalid for envvar keys, but is valid for other configuration providers.
I asked why the Enable flag is needed and he mentioned that there is also a need to disable auditing where removing __ADDRESS isn't sufficient as it could be configured via the code api and specifying an empty address value does not feel right which I agree. If the intent it disabling, why not use that in the envvar key, as the default without it is that auditing is enabled.
| public sealed class Audit : Feature | ||
| { | ||
| internal const string AddressEnvironmentVariableKey = "NServiceBus__Audit__Address"; | ||
| internal const string IsEnabledEnvironmentVariableKey = "NServiceBus__Audit__IsEnabled"; |
There was a problem hiding this comment.
If disabling the feature is the intent of the flag, why not name it NSERVICEBUS__AUDIT__DISABLED. Not against NSERVICEBUS_AUDIT_ENABLED, but it seems to better describe intent.
| internal const string IsEnabledEnvironmentVariableKey = "NServiceBus__Audit__IsEnabled"; | |
| internal const string IsDisabledEnvironmentVariableKey = "NSERVICEBUS_AUDIT_DISABLE"; |
| /// </summary> | ||
| public sealed class Audit : Feature | ||
| { | ||
| internal const string AddressEnvironmentVariableKey = "NServiceBus__Audit__Address"; |
There was a problem hiding this comment.
| internal const string AddressEnvironmentVariableKey = "NServiceBus__Audit__Address"; | |
| internal const string AddressEnvironmentVariableKey = "NSERVICEBUS__AUDIT__ADDRESS"; |
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. |
No description provided.