fix(options): validate Host Node/Database/Logging options at startup — empty NodeName fails fast instead of NULLing SourceNode (plan R2-08 T7, arch-review 08r2 NF4)

This commit is contained in:
Joseph Doherty
2026-07-13 10:49:42 -04:00
parent f85f036b09
commit 4a0462e4d0
11 changed files with 224 additions and 3 deletions
@@ -0,0 +1,28 @@
using ZB.MOM.WW.Configuration;
namespace ZB.MOM.WW.ScadaBridge.Host;
/// <summary>
/// Validates <see cref="DatabaseOptions"/> at host startup (arch-review 08
/// round 2 NF4). All three connection settings are nullable and role-dependent
/// (central needs the SQL Server strings, a site needs only the SQLite path),
/// so a <c>null</c> value is valid. A present-but-whitespace value is not — it
/// signals a mis-set config key that would fail opaquely at first DB use.
/// </summary>
public sealed class DatabaseOptionsValidator : OptionsValidatorBase<DatabaseOptions>
{
/// <inheritdoc />
protected override void Validate(ValidationBuilder builder, DatabaseOptions options)
{
RequireNotWhitespace(builder, options.ConfigurationDb, nameof(DatabaseOptions.ConfigurationDb));
RequireNotWhitespace(builder, options.MachineDataDb, nameof(DatabaseOptions.MachineDataDb));
RequireNotWhitespace(builder, options.SiteDbPath, nameof(DatabaseOptions.SiteDbPath));
}
// null is valid (role-dependent); reject only a present-but-blank value.
private static void RequireNotWhitespace(ValidationBuilder builder, string? value, string field)
{
builder.RequireThat(value is null || !string.IsNullOrWhiteSpace(value),
$"ScadaBridge:Database:{field} is set but blank — either remove it or give it a real value.");
}
}
@@ -0,0 +1,27 @@
using Serilog.Events;
using ZB.MOM.WW.Configuration;
namespace ZB.MOM.WW.ScadaBridge.Host;
/// <summary>
/// Validates <see cref="LoggingOptions"/> at host startup (arch-review 08 round
/// 2 NF4). <see cref="LoggingOptions.MinimumLevel"/> is parsed into a Serilog
/// <see cref="LogEventLevel"/> by <c>LoggerConfigurationFactory.ParseLevel</c>,
/// which silently falls back to <c>Information</c> for an unrecognised value.
/// This validator surfaces that misconfiguration at boot instead: a present,
/// non-blank value must name a real level (Verbose/Debug/Information/Warning/
/// Error/Fatal, case-insensitive). A null/blank value is treated as "unset"
/// (defaults to Information), mirroring the parser.
/// </summary>
public sealed class LoggingOptionsValidator : OptionsValidatorBase<LoggingOptions>
{
/// <inheritdoc />
protected override void Validate(ValidationBuilder builder, LoggingOptions options)
{
builder.RequireThat(
string.IsNullOrWhiteSpace(options.MinimumLevel)
|| Enum.TryParse<LogEventLevel>(options.MinimumLevel, ignoreCase: true, out _),
$"ScadaBridge:Logging:{nameof(LoggingOptions.MinimumLevel)} ('{options.MinimumLevel}') " +
$"is not a recognised Serilog level. Valid: {string.Join(", ", Enum.GetNames<LogEventLevel>())}.");
}
}
@@ -0,0 +1,34 @@
using ZB.MOM.WW.Configuration;
namespace ZB.MOM.WW.ScadaBridge.Host;
/// <summary>
/// Validates <see cref="NodeOptions"/> at host startup (arch-review 08 round 2
/// NF4). The headline invariant: <see cref="NodeOptions.NodeName"/> must be
/// non-empty — an empty value normalises to a NULL <c>SourceNode</c> audit
/// column, silently degrading the audit trail (the wonder-app-vd03 failure
/// mode), so the host must fail fast at boot instead. Ports are bounded to the
/// TCP range; <c>0</c> stays valid because it requests a dynamically-assigned
/// port (CompositionRootTests rely on this).
/// </summary>
public sealed class NodeOptionsValidator : OptionsValidatorBase<NodeOptions>
{
/// <inheritdoc />
protected override void Validate(ValidationBuilder builder, NodeOptions options)
{
builder.RequireThat(!string.IsNullOrWhiteSpace(options.NodeName),
$"ScadaBridge:Node:{nameof(NodeOptions.NodeName)} must be a non-empty label " +
"— an empty node name stamps the SourceNode audit column NULL.");
RequirePort(builder, options.RemotingPort, nameof(NodeOptions.RemotingPort));
RequirePort(builder, options.GrpcPort, nameof(NodeOptions.GrpcPort));
RequirePort(builder, options.MetricsPort, nameof(NodeOptions.MetricsPort));
}
// 0 stays valid (dynamic-port request); reject only out-of-TCP-range values.
private static void RequirePort(ValidationBuilder builder, int port, string field)
{
builder.RequireThat(port is >= 0 and <= 65_535,
$"ScadaBridge:Node:{field} ({port}) must be in [0, 65535].");
}
}
@@ -157,19 +157,33 @@ public static class SiteServiceRegistration
/// <param name="config">Application configuration supplying the option values.</param> /// <param name="config">Application configuration supplying the option values.</param>
public static void BindSharedOptions(IServiceCollection services, IConfiguration config) public static void BindSharedOptions(IServiceCollection services, IConfiguration config)
{ {
services.Configure<NodeOptions>(config.GetSection("ScadaBridge:Node")); // Bind + eagerly validate: an empty NodeName would stamp the SourceNode audit
// column NULL, so fail the host at boot with a key-naming message instead of
// silently degrading the audit trail (arch-review 08r2 NF4).
services.AddOptions<NodeOptions>().Bind(config.GetSection("ScadaBridge:Node")).ValidateOnStart();
services.TryAddEnumerable(
ServiceDescriptor.Singleton<IValidateOptions<NodeOptions>, NodeOptionsValidator>());
// Bind + eagerly validate: ClusterOptionsValidator is registered (TryAddEnumerable) // Bind + eagerly validate: ClusterOptionsValidator is registered (TryAddEnumerable)
// by the ClusterInfrastructure module, so chaining ValidateOnStart() here makes a bad // by the ClusterInfrastructure module, so chaining ValidateOnStart() here makes a bad
// ScadaBridge:Cluster section fail fast at host build instead of lazily on first resolve. // ScadaBridge:Cluster section fail fast at host build instead of lazily on first resolve.
services.AddOptions<ClusterOptions>().Bind(config.GetSection("ScadaBridge:Cluster")).ValidateOnStart(); services.AddOptions<ClusterOptions>().Bind(config.GetSection("ScadaBridge:Cluster")).ValidateOnStart();
services.Configure<DatabaseOptions>(config.GetSection("ScadaBridge:Database")); // Bind + eagerly validate: a present-but-blank connection setting fails fast
// here rather than opaquely at first DB use (arch-review 08r2 NF4).
services.AddOptions<DatabaseOptions>().Bind(config.GetSection("ScadaBridge:Database")).ValidateOnStart();
services.TryAddEnumerable(
ServiceDescriptor.Singleton<IValidateOptions<DatabaseOptions>, DatabaseOptionsValidator>());
services.Configure<CommunicationOptions>(config.GetSection("ScadaBridge:Communication")); services.Configure<CommunicationOptions>(config.GetSection("ScadaBridge:Communication"));
// Bind + eagerly validate: HealthMonitoringOptionsValidator is registered (TryAddEnumerable) // Bind + eagerly validate: HealthMonitoringOptionsValidator is registered (TryAddEnumerable)
// by the HealthMonitoring module, so chaining ValidateOnStart() here makes a bad // by the HealthMonitoring module, so chaining ValidateOnStart() here makes a bad
// ScadaBridge:HealthMonitoring section fail fast at host build instead of lazily on first resolve. // ScadaBridge:HealthMonitoring section fail fast at host build instead of lazily on first resolve.
services.AddOptions<HealthMonitoringOptions>().Bind(config.GetSection("ScadaBridge:HealthMonitoring")).ValidateOnStart(); services.AddOptions<HealthMonitoringOptions>().Bind(config.GetSection("ScadaBridge:HealthMonitoring")).ValidateOnStart();
services.Configure<NotificationOptions>(config.GetSection("ScadaBridge:Notification")); services.Configure<NotificationOptions>(config.GetSection("ScadaBridge:Notification"));
services.Configure<LoggingOptions>(config.GetSection("ScadaBridge:Logging")); // Bind + eagerly validate: an unrecognised MinimumLevel would silently fall back
// to Information; fail fast at boot so the operator's intended floor is honoured
// (arch-review 08r2 NF4).
services.AddOptions<LoggingOptions>().Bind(config.GetSection("ScadaBridge:Logging")).ValidateOnStart();
services.TryAddEnumerable(
ServiceDescriptor.Singleton<IValidateOptions<LoggingOptions>, LoggingOptionsValidator>());
// Audit Log — exposes ScadaBridge:Node:NodeName to downstream audit // Audit Log — exposes ScadaBridge:Node:NodeName to downstream audit
// writers so they can stamp the SourceNode column. Registered here in // writers so they can stamp the SourceNode column. Registered here in
@@ -48,6 +48,7 @@ public class CentralActorPathTests : IAsyncLifetime
{ {
config.AddInMemoryCollection(new Dictionary<string, string?> config.AddInMemoryCollection(new Dictionary<string, string?>
{ {
["ScadaBridge:Node:NodeName"] = "central-a",
["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:NodeHostname"] = "localhost",
["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:RemotingPort"] = "0",
["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:25510", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:25510",
@@ -171,6 +172,7 @@ public class SiteActorPathTests : IAsyncLifetime
config.AddInMemoryCollection(new Dictionary<string, string?> config.AddInMemoryCollection(new Dictionary<string, string?>
{ {
["ScadaBridge:Node:Role"] = "Site", ["ScadaBridge:Node:Role"] = "Site",
["ScadaBridge:Node:NodeName"] = "node-a",
["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:NodeHostname"] = "localhost",
["ScadaBridge:Node:SiteId"] = "TestSite", ["ScadaBridge:Node:SiteId"] = "TestSite",
["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:RemotingPort"] = "0",
@@ -114,6 +114,7 @@ public class CentralAuditWiringTests : IDisposable
{ {
config.AddInMemoryCollection(new Dictionary<string, string?> config.AddInMemoryCollection(new Dictionary<string, string?>
{ {
["ScadaBridge:Node:NodeName"] = "central-a",
["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:NodeHostname"] = "localhost",
["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:RemotingPort"] = "0",
["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551",
@@ -294,6 +295,7 @@ public class SiteAuditWiringTests : IDisposable
builder.Configuration.AddInMemoryCollection(new Dictionary<string, string?> builder.Configuration.AddInMemoryCollection(new Dictionary<string, string?>
{ {
["ScadaBridge:Node:Role"] = "Site", ["ScadaBridge:Node:Role"] = "Site",
["ScadaBridge:Node:NodeName"] = "node-a",
["ScadaBridge:Node:NodeHostname"] = "test-site", ["ScadaBridge:Node:NodeHostname"] = "test-site",
["ScadaBridge:Node:SiteId"] = "TestSite", ["ScadaBridge:Node:SiteId"] = "TestSite",
["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:RemotingPort"] = "0",
@@ -108,6 +108,7 @@ public class CentralCompositionRootTests : IDisposable
{ {
config.AddInMemoryCollection(new Dictionary<string, string?> config.AddInMemoryCollection(new Dictionary<string, string?>
{ {
["ScadaBridge:Node:NodeName"] = "central-a",
["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:NodeHostname"] = "localhost",
["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:RemotingPort"] = "0",
["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551",
@@ -348,6 +349,7 @@ public class SiteCompositionRootTests : IDisposable
builder.Configuration.AddInMemoryCollection(new Dictionary<string, string?> builder.Configuration.AddInMemoryCollection(new Dictionary<string, string?>
{ {
["ScadaBridge:Node:Role"] = "Site", ["ScadaBridge:Node:Role"] = "Site",
["ScadaBridge:Node:NodeName"] = "node-a",
["ScadaBridge:Node:NodeHostname"] = "test-site", ["ScadaBridge:Node:NodeHostname"] = "test-site",
["ScadaBridge:Node:SiteId"] = "TestSite", ["ScadaBridge:Node:SiteId"] = "TestSite",
["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:RemotingPort"] = "0",
@@ -44,6 +44,7 @@ public class HealthCheckTests : IDisposable
{ {
config.AddInMemoryCollection(new Dictionary<string, string?> config.AddInMemoryCollection(new Dictionary<string, string?>
{ {
["ScadaBridge:Node:NodeName"] = "central-a",
["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:NodeHostname"] = "localhost",
["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:RemotingPort"] = "0",
["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551",
@@ -41,6 +41,7 @@ public class HostStartupTests : IDisposable
{ {
config.AddInMemoryCollection(new Dictionary<string, string?> config.AddInMemoryCollection(new Dictionary<string, string?>
{ {
["ScadaBridge:Node:NodeName"] = "central-a",
["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:NodeHostname"] = "localhost",
["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:RemotingPort"] = "0",
["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551",
@@ -74,6 +75,7 @@ public class HostStartupTests : IDisposable
builder.Configuration.AddInMemoryCollection(new Dictionary<string, string?> builder.Configuration.AddInMemoryCollection(new Dictionary<string, string?>
{ {
["ScadaBridge:Node:Role"] = "Site", ["ScadaBridge:Node:Role"] = "Site",
["ScadaBridge:Node:NodeName"] = "node-a",
["ScadaBridge:Node:NodeHostname"] = "test-site", ["ScadaBridge:Node:NodeHostname"] = "test-site",
["ScadaBridge:Node:SiteId"] = "TestSite", ["ScadaBridge:Node:SiteId"] = "TestSite",
["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:RemotingPort"] = "0",
@@ -103,6 +105,7 @@ public class HostStartupTests : IDisposable
builder.Configuration.AddInMemoryCollection(new Dictionary<string, string?> builder.Configuration.AddInMemoryCollection(new Dictionary<string, string?>
{ {
["ScadaBridge:Node:Role"] = "Site", ["ScadaBridge:Node:Role"] = "Site",
["ScadaBridge:Node:NodeName"] = "node-a",
["ScadaBridge:Node:NodeHostname"] = "test-site", ["ScadaBridge:Node:NodeHostname"] = "test-site",
["ScadaBridge:Node:SiteId"] = "TestSite", ["ScadaBridge:Node:SiteId"] = "TestSite",
["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:RemotingPort"] = "0",
@@ -40,6 +40,7 @@ public class MetricsEndpointTests : IDisposable
{ {
config.AddInMemoryCollection(new Dictionary<string, string?> config.AddInMemoryCollection(new Dictionary<string, string?>
{ {
["ScadaBridge:Node:NodeName"] = "central-a",
["ScadaBridge:Node:NodeHostname"] = "localhost", ["ScadaBridge:Node:NodeHostname"] = "localhost",
["ScadaBridge:Node:RemotingPort"] = "0", ["ScadaBridge:Node:RemotingPort"] = "0",
["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551", ["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@localhost:2551",
@@ -0,0 +1,107 @@
using ZB.MOM.WW.ScadaBridge.Host;
namespace ZB.MOM.WW.ScadaBridge.Host.Tests;
/// <summary>
/// Eager startup validation for the shared Host options (arch-review 08 round 2
/// NF4). Covers <see cref="NodeOptionsValidator"/>,
/// <see cref="DatabaseOptionsValidator"/>, and
/// <see cref="LoggingOptionsValidator"/> in one file. The headline case: an
/// empty <see cref="NodeOptions.NodeName"/> normalises to a NULL SourceNode
/// audit column, so it must fail the host at boot rather than silently degrade
/// the audit trail.
/// </summary>
public class NodeOptionsValidatorTests
{
// ---- NodeOptions ----
[Fact]
public void Node_DefaultsWithNodeName_AreValid()
{
var validator = new NodeOptionsValidator();
Assert.True(validator.Validate(null, new NodeOptions { NodeName = "node-a" }).Succeeded);
}
[Theory]
[InlineData("")]
[InlineData(" ")]
public void Node_EmptyOrWhitespaceNodeName_Fails(string nodeName)
{
var validator = new NodeOptionsValidator();
var result = validator.Validate(null, new NodeOptions { NodeName = nodeName });
Assert.False(result.Succeeded);
Assert.Contains(result.Failures!,
f => f.Contains(nameof(NodeOptions.NodeName), StringComparison.Ordinal));
}
[Fact]
public void Node_ZeroRemotingPort_IsValid()
{
// CompositionRootTests uses RemotingPort = 0 to request a dynamic port.
var validator = new NodeOptionsValidator();
Assert.True(validator.Validate(null,
new NodeOptions { NodeName = "node-a", RemotingPort = 0 }).Succeeded);
}
[Theory]
[InlineData(-1)]
[InlineData(65_536)]
public void Node_PortOutOfRange_Fails(int port)
{
var validator = new NodeOptionsValidator();
var result = validator.Validate(null,
new NodeOptions { NodeName = "node-a", GrpcPort = port });
Assert.False(result.Succeeded);
Assert.Contains(result.Failures!,
f => f.Contains(nameof(NodeOptions.GrpcPort), StringComparison.Ordinal));
}
// ---- DatabaseOptions ----
[Fact]
public void Database_AllNull_IsValid()
{
// All three fields are nullable and role-dependent; null stays valid.
var validator = new DatabaseOptionsValidator();
Assert.True(validator.Validate(null, new DatabaseOptions()).Succeeded);
}
[Fact]
public void Database_WhitespaceValue_Fails()
{
var validator = new DatabaseOptionsValidator();
var result = validator.Validate(null, new DatabaseOptions { ConfigurationDb = " " });
Assert.False(result.Succeeded);
Assert.Contains(result.Failures!,
f => f.Contains(nameof(DatabaseOptions.ConfigurationDb), StringComparison.Ordinal));
}
// ---- LoggingOptions ----
[Fact]
public void Logging_DefaultInformation_IsValid()
{
var validator = new LoggingOptionsValidator();
Assert.True(validator.Validate(null, new LoggingOptions()).Succeeded);
}
[Theory]
[InlineData("Verbose")]
[InlineData("debug")]
[InlineData("FATAL")]
public void Logging_RecognisedLevel_IsValid(string level)
{
var validator = new LoggingOptionsValidator();
Assert.True(validator.Validate(null, new LoggingOptions { MinimumLevel = level }).Succeeded);
}
[Fact]
public void Logging_UnknownLevel_Fails()
{
var validator = new LoggingOptionsValidator();
var result = validator.Validate(null, new LoggingOptions { MinimumLevel = "Chatty" });
Assert.False(result.Succeeded);
Assert.Contains(result.Failures!,
f => f.Contains(nameof(LoggingOptions.MinimumLevel), StringComparison.Ordinal));
}
}