feat(options): eager startup validation for edge/management options (InboundAPI, ESG, Notification, ManagementService) (arch-review 08 §1.5)

Claude-Session: https://claude.ai/code/session_01MtdgwpEeCUn6cUA5f1LMPj
2026-07-10 06:52:59 -04:00
parent 8b775f4ae1
commit fc918d4679
16 changed files with 404 additions and 4 deletions
@@ -0,0 +1,33 @@
+using ZB.MOM.WW.Configuration;
+
+namespace ZB.MOM.WW.ScadaBridge.ExternalSystemGateway;
+
+/// <summary>
+/// Validates <see cref="ExternalSystemGatewayOptions"/> at startup.
+/// <see cref="ExternalSystemGatewayOptions.DefaultHttpTimeout"/> bounds each outbound call,
+/// <see cref="ExternalSystemGatewayOptions.MaxConcurrentConnectionsPerSystem"/> is fed into
+/// <c>SocketsHttpHandler.MaxConnectionsPerServer</c> (a non-positive value throws in the handler
+/// constructor), and <see cref="ExternalSystemGatewayOptions.MaxResponseBodyBytes"/> caps buffered
+/// response bodies. Registered with <c>ValidateOnStart()</c> so a bad
+/// <c>ScadaBridge:ExternalSystemGateway</c> section fails fast at boot with a clear, key-naming
+/// message rather than crashing the HTTP client factory or silently disabling the gateway.
+/// </summary>
+public sealed class ExternalSystemGatewayOptionsValidator : OptionsValidatorBase<ExternalSystemGatewayOptions>
+{
+    /// <inheritdoc />
+    protected override void Validate(ValidationBuilder builder, ExternalSystemGatewayOptions options)
+    {
+        builder.RequireThat(options.DefaultHttpTimeout > TimeSpan.Zero,
+            $"ScadaBridge:ExternalSystemGateway:DefaultHttpTimeout must be a positive duration " +
+            $"(was {options.DefaultHttpTimeout}); it bounds each outbound external-system HTTP call.");
+
+        builder.RequireThat(options.MaxConcurrentConnectionsPerSystem > 0,
+            $"ScadaBridge:ExternalSystemGateway:MaxConcurrentConnectionsPerSystem must be greater than zero " +
+            $"(was {options.MaxConcurrentConnectionsPerSystem}); it is fed into " +
+            "SocketsHttpHandler.MaxConnectionsPerServer, which rejects non-positive values.");
+
+        builder.RequireThat(options.MaxResponseBodyBytes > 0,
+            $"ScadaBridge:ExternalSystemGateway:MaxResponseBodyBytes must be greater than zero " +
+            $"(was {options.MaxResponseBodyBytes}); it caps the outbound response body buffered into memory.");
+    }
+}
@@ -1,4 +1,5 @@
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.DependencyInjection.Extensions;
 using Microsoft.Extensions.Http;
 using Microsoft.Extensions.Options;
 using ZB.MOM.WW.ScadaBridge.Commons.Interfaces.Services;
@@ -21,7 +22,11 @@ public static class ServiceCollectionExtensions
    public static IServiceCollection AddExternalSystemGateway(this IServiceCollection services)
    {
        services.AddOptions<ExternalSystemGatewayOptions>()
-            .BindConfiguration("ScadaBridge:ExternalSystemGateway");
+            .BindConfiguration("ScadaBridge:ExternalSystemGateway")
+            .ValidateOnStart();
+        services.TryAddEnumerable(
+            ServiceDescriptor.Singleton<IValidateOptions<ExternalSystemGatewayOptions>,
+                ExternalSystemGatewayOptionsValidator>());

        services.AddHttpClient();

@@ -13,6 +13,7 @@
    <PackageReference Include="Microsoft.Extensions.Http" />
    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
    <PackageReference Include="Microsoft.Extensions.Options" />
+    <PackageReference Include="ZB.MOM.WW.Configuration" />
  </ItemGroup>

  <ItemGroup>
@@ -1,3 +1,5 @@
+using Microsoft.Extensions.DependencyInjection.Extensions;
+using Microsoft.Extensions.Options;
 using ZB.MOM.WW.Auth.ApiKeys.DependencyInjection;
 using ZB.MOM.WW.Auth.AspNetCore;
 using ZB.MOM.WW.Health;
@@ -291,7 +293,11 @@ try
        // Options binding
        SiteServiceRegistration.BindSharedOptions(builder.Services, builder.Configuration);
        builder.Services.Configure<SecurityOptions>(builder.Configuration.GetSection("ScadaBridge:Security"));
-        builder.Services.Configure<InboundApiOptions>(builder.Configuration.GetSection("ScadaBridge:InboundApi"));
+        builder.Services.AddOptions<InboundApiOptions>()
+            .Bind(builder.Configuration.GetSection("ScadaBridge:InboundApi"))
+            .ValidateOnStart();
+        builder.Services.TryAddEnumerable(
+            ServiceDescriptor.Singleton<IValidateOptions<InboundApiOptions>, InboundApiOptionsValidator>());
        builder.Services.Configure<DeploymentManagerOptions>(
            builder.Configuration.GetSection(ZB.MOM.WW.ScadaBridge.DeploymentManager.ServiceCollectionExtensions.OptionsSection));

@@ -0,0 +1,31 @@
+using ZB.MOM.WW.Configuration;
+
+namespace ZB.MOM.WW.ScadaBridge.InboundAPI;
+
+/// <summary>
+/// Validates <see cref="InboundApiOptions"/> at startup. <see cref="InboundApiOptions.DefaultMethodTimeout"/>
+/// bounds each inbound-API method execution and <see cref="InboundApiOptions.MaxRequestBodyBytes"/> caps the
+/// accepted <c>POST /api/{methodName}</c> body before it is buffered — a zero/negative timeout would cancel
+/// every request immediately (or never), and a non-positive body cap would reject every request with HTTP 413.
+/// Registered with <c>ValidateOnStart()</c> so a bad <c>ScadaBridge:InboundApi</c> section fails fast at boot
+/// with a clear, key-naming message rather than degrading the endpoint at runtime.
+/// <para>
+/// <see cref="InboundApiOptions.ApiKeyPepper"/> is intentionally NOT validated here: an empty pepper is a
+/// legitimate dev default, and production pepper strength is owned by the shared auth-key library / startup
+/// validator, not by this options validator.
+/// </para>
+/// </summary>
+public sealed class InboundApiOptionsValidator : OptionsValidatorBase<InboundApiOptions>
+{
+    /// <inheritdoc />
+    protected override void Validate(ValidationBuilder builder, InboundApiOptions options)
+    {
+        builder.RequireThat(options.DefaultMethodTimeout > TimeSpan.Zero,
+            $"ScadaBridge:InboundApi:DefaultMethodTimeout must be a positive duration " +
+            $"(was {options.DefaultMethodTimeout}); it bounds each inbound-API method execution.");
+
+        builder.RequireThat(options.MaxRequestBodyBytes > 0,
+            $"ScadaBridge:InboundApi:MaxRequestBodyBytes must be greater than zero " +
+            $"(was {options.MaxRequestBodyBytes}); it caps the accepted request body before buffering.");
+    }
+}
@@ -32,6 +32,7 @@
         AddZbApiKeyAuth (DI helper) lives in this package and brings the
         Abstractions contracts (IApiKeyVerifier / ApiKeyOptions) transitively. -->
    <PackageReference Include="ZB.MOM.WW.Auth.ApiKeys" />
+    <PackageReference Include="ZB.MOM.WW.Configuration" />
  </ItemGroup>

 </Project>
@@ -0,0 +1,30 @@
+using ZB.MOM.WW.Configuration;
+
+namespace ZB.MOM.WW.ScadaBridge.ManagementService;
+
+/// <summary>
+/// Validates <see cref="ManagementServiceOptions"/> at startup. Both
+/// <see cref="ManagementServiceOptions.CommandTimeout"/> and
+/// <see cref="ManagementServiceOptions.LongRunningCommandTimeout"/> are used as Ask timeouts against
+/// the ManagementActor; a zero/negative value makes every management command fail immediately (or Ask
+/// forever). Registered with <c>ValidateOnStart()</c> so a bad <c>ScadaBridge:ManagementService</c>
+/// section fails fast at boot with a clear, key-naming message.
+/// <para>
+/// <see cref="ManagementServiceOptions.SecuredWritePendingTtl"/> is intentionally NOT required positive:
+/// a non-positive value legitimately disables server-side secured-write expiry.
+/// </para>
+/// </summary>
+public sealed class ManagementServiceOptionsValidator : OptionsValidatorBase<ManagementServiceOptions>
+{
+    /// <inheritdoc />
+    protected override void Validate(ValidationBuilder builder, ManagementServiceOptions options)
+    {
+        builder.RequireThat(options.CommandTimeout > TimeSpan.Zero,
+            $"ScadaBridge:ManagementService:CommandTimeout must be a positive duration " +
+            $"(was {options.CommandTimeout}); it is the Ask timeout for management commands.");
+
+        builder.RequireThat(options.LongRunningCommandTimeout > TimeSpan.Zero,
+            $"ScadaBridge:ManagementService:LongRunningCommandTimeout must be a positive duration " +
+            $"(was {options.LongRunningCommandTimeout}); it is the Ask timeout for transport/deploy commands.");
+    }
+}
@@ -1,4 +1,6 @@
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.DependencyInjection.Extensions;
+using Microsoft.Extensions.Options;

 namespace ZB.MOM.WW.ScadaBridge.ManagementService;

@@ -13,7 +15,11 @@ public static class ServiceCollectionExtensions
    {
        services.AddSingleton<ManagementActorHolder>();
        services.AddOptions<ManagementServiceOptions>()
-            .BindConfiguration("ScadaBridge:ManagementService");
+            .BindConfiguration("ScadaBridge:ManagementService")
+            .ValidateOnStart();
+        services.TryAddEnumerable(
+            ServiceDescriptor.Singleton<IValidateOptions<ManagementServiceOptions>,
+                ManagementServiceOptionsValidator>());
        return services;
    }
 }
@@ -14,6 +14,7 @@
  <ItemGroup>
    <PackageReference Include="Akka" />
    <PackageReference Include="Akka.Cluster.Tools" />
+    <PackageReference Include="ZB.MOM.WW.Configuration" />
  </ItemGroup>
  <ItemGroup>
    <ProjectReference Include="../ZB.MOM.WW.ScadaBridge.Commons/ZB.MOM.WW.ScadaBridge.Commons.csproj" />
@@ -0,0 +1,27 @@
+using ZB.MOM.WW.Configuration;
+
+namespace ZB.MOM.WW.ScadaBridge.NotificationService;
+
+/// <summary>
+/// Validates <see cref="NotificationOptions"/> at startup. The values are the SMTP fallback used
+/// by the central Email delivery adapter when the deployed <c>SmtpConfiguration</c> leaves the
+/// corresponding field unset: <see cref="NotificationOptions.ConnectionTimeoutSeconds"/> becomes an
+/// SMTP connect timeout and <see cref="NotificationOptions.MaxConcurrentConnections"/> caps the
+/// connection pool — a non-positive value there is not a usable fallback. Registered with
+/// <c>ValidateOnStart()</c> so a bad <c>ScadaBridge:Notification</c> section fails fast at boot with
+/// a clear, key-naming message.
+/// </summary>
+public sealed class NotificationOptionsValidator : OptionsValidatorBase<NotificationOptions>
+{
+    /// <inheritdoc />
+    protected override void Validate(ValidationBuilder builder, NotificationOptions options)
+    {
+        builder.RequireThat(options.ConnectionTimeoutSeconds > 0,
+            $"ScadaBridge:Notification:ConnectionTimeoutSeconds must be greater than zero " +
+            $"(was {options.ConnectionTimeoutSeconds}); it is the SMTP connect-timeout fallback.");
+
+        builder.RequireThat(options.MaxConcurrentConnections > 0,
+            $"ScadaBridge:Notification:MaxConcurrentConnections must be greater than zero " +
+            $"(was {options.MaxConcurrentConnections}); it caps the SMTP connection pool.");
+    }
+}
@@ -1,4 +1,6 @@
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.DependencyInjection.Extensions;
+using Microsoft.Extensions.Options;

 namespace ZB.MOM.WW.ScadaBridge.NotificationService;

@@ -18,7 +20,11 @@ public static class ServiceCollectionExtensions
    public static IServiceCollection AddNotificationService(this IServiceCollection services)
    {
        services.AddOptions<NotificationOptions>()
-            .BindConfiguration("ScadaBridge:Notification");
+            .BindConfiguration("ScadaBridge:Notification")
+            .ValidateOnStart();
+        services.TryAddEnumerable(
+            ServiceDescriptor.Singleton<IValidateOptions<NotificationOptions>,
+                NotificationOptionsValidator>());

        services.AddHttpClient();
        services.AddSingleton<OAuth2TokenService>();
@@ -13,6 +13,7 @@
    <PackageReference Include="Microsoft.Extensions.Http" />
    <PackageReference Include="Microsoft.Extensions.Logging.Abstractions" />
    <PackageReference Include="Microsoft.Extensions.Options" />
+    <PackageReference Include="ZB.MOM.WW.Configuration" />
  </ItemGroup>

  <ItemGroup>
@@ -0,0 +1,67 @@
+using Microsoft.Extensions.Options;
+
+namespace ZB.MOM.WW.ScadaBridge.ExternalSystemGateway.Tests;
+
+/// <summary>
+/// Arch-review 08 §1.5: <see cref="ExternalSystemGatewayOptions"/> feeds a per-call HTTP
+/// timeout, a per-system connection cap (used as <c>SocketsHttpHandler.MaxConnectionsPerServer</c>),
+/// and a response-body byte cap into the gateway. A zero/negative timeout or a non-positive
+/// count crashes the handler or silently disables the gateway; reject them fast at startup.
+/// </summary>
+public class ExternalSystemGatewayOptionsValidatorTests
+{
+    private static ValidateOptionsResult Validate(ExternalSystemGatewayOptions options) =>
+        new ExternalSystemGatewayOptionsValidator().Validate(Options.DefaultName, options);
+
+    [Fact]
+    public void DefaultOptions_AreValid()
+    {
+        var result = Validate(new ExternalSystemGatewayOptions());
+        Assert.True(result.Succeeded, result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroDefaultHttpTimeout_IsRejected()
+    {
+        var result = Validate(new ExternalSystemGatewayOptions { DefaultHttpTimeout = TimeSpan.Zero });
+
+        Assert.True(result.Failed);
+        Assert.Contains("DefaultHttpTimeout", result.FailureMessage);
+    }
+
+    [Fact]
+    public void NegativeDefaultHttpTimeout_IsRejected()
+    {
+        var result = Validate(new ExternalSystemGatewayOptions { DefaultHttpTimeout = TimeSpan.FromSeconds(-1) });
+
+        Assert.True(result.Failed);
+        Assert.Contains("DefaultHttpTimeout", result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroMaxConcurrentConnectionsPerSystem_IsRejected()
+    {
+        var result = Validate(new ExternalSystemGatewayOptions { MaxConcurrentConnectionsPerSystem = 0 });
+
+        Assert.True(result.Failed);
+        Assert.Contains("MaxConcurrentConnectionsPerSystem", result.FailureMessage);
+    }
+
+    [Fact]
+    public void NegativeMaxConcurrentConnectionsPerSystem_IsRejected()
+    {
+        var result = Validate(new ExternalSystemGatewayOptions { MaxConcurrentConnectionsPerSystem = -1 });
+
+        Assert.True(result.Failed);
+        Assert.Contains("MaxConcurrentConnectionsPerSystem", result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroMaxResponseBodyBytes_IsRejected()
+    {
+        var result = Validate(new ExternalSystemGatewayOptions { MaxResponseBodyBytes = 0 });
+
+        Assert.True(result.Failed);
+        Assert.Contains("MaxResponseBodyBytes", result.FailureMessage);
+    }
+}
@@ -0,0 +1,68 @@
+using Microsoft.Extensions.Options;
+
+namespace ZB.MOM.WW.ScadaBridge.InboundAPI.Tests;
+
+/// <summary>
+/// Arch-review 08 §1.5: <see cref="InboundApiOptions"/> feeds a per-request timeout
+/// (<see cref="InboundApiOptions.DefaultMethodTimeout"/>) and a body-size cap
+/// (<see cref="InboundApiOptions.MaxRequestBodyBytes"/>) into the inbound-API request
+/// pipeline; a zero/negative timeout or a non-positive body cap must be rejected fast at
+/// startup with a clear, key-naming message rather than silently degrading the endpoint.
+/// The <see cref="InboundApiOptions.ApiKeyPepper"/> is deliberately NOT required non-empty
+/// here (empty is a legitimate dev default; production pepper strength is enforced elsewhere).
+/// </summary>
+public class InboundApiOptionsValidatorTests
+{
+    private static ValidateOptionsResult Validate(InboundApiOptions options) =>
+        new InboundApiOptionsValidator().Validate(Options.DefaultName, options);
+
+    [Fact]
+    public void DefaultOptions_AreValid()
+    {
+        var result = Validate(new InboundApiOptions());
+        Assert.True(result.Succeeded, result.FailureMessage);
+    }
+
+    [Fact]
+    public void EmptyApiKeyPepper_IsValid()
+    {
+        var result = Validate(new InboundApiOptions { ApiKeyPepper = string.Empty });
+        Assert.True(result.Succeeded, result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroDefaultMethodTimeout_IsRejected()
+    {
+        var result = Validate(new InboundApiOptions { DefaultMethodTimeout = TimeSpan.Zero });
+
+        Assert.True(result.Failed);
+        Assert.Contains("DefaultMethodTimeout", result.FailureMessage);
+    }
+
+    [Fact]
+    public void NegativeDefaultMethodTimeout_IsRejected()
+    {
+        var result = Validate(new InboundApiOptions { DefaultMethodTimeout = TimeSpan.FromSeconds(-1) });
+
+        Assert.True(result.Failed);
+        Assert.Contains("DefaultMethodTimeout", result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroMaxRequestBodyBytes_IsRejected()
+    {
+        var result = Validate(new InboundApiOptions { MaxRequestBodyBytes = 0 });
+
+        Assert.True(result.Failed);
+        Assert.Contains("MaxRequestBodyBytes", result.FailureMessage);
+    }
+
+    [Fact]
+    public void NegativeMaxRequestBodyBytes_IsRejected()
+    {
+        var result = Validate(new InboundApiOptions { MaxRequestBodyBytes = -1 });
+
+        Assert.True(result.Failed);
+        Assert.Contains("MaxRequestBodyBytes", result.FailureMessage);
+    }
+}
@@ -0,0 +1,59 @@
+using Microsoft.Extensions.Options;
+
+namespace ZB.MOM.WW.ScadaBridge.ManagementService.Tests;
+
+/// <summary>
+/// Arch-review 08 §1.5: <see cref="ManagementServiceOptions"/> supplies the Ask timeouts
+/// the ManagementActor client uses for command and long-running transport/deploy calls. A
+/// zero/negative timeout makes every command fail (or Ask forever); reject it fast at startup
+/// with a key-naming message. <see cref="ManagementServiceOptions.SecuredWritePendingTtl"/> is
+/// deliberately NOT required positive — a non-positive value legitimately disables server-side
+/// secured-write expiry.
+/// </summary>
+public class ManagementServiceOptionsValidatorTests
+{
+    private static ValidateOptionsResult Validate(ManagementServiceOptions options) =>
+        new ManagementServiceOptionsValidator().Validate(Options.DefaultName, options);
+
+    [Fact]
+    public void DefaultOptions_AreValid()
+    {
+        var result = Validate(new ManagementServiceOptions());
+        Assert.True(result.Succeeded, result.FailureMessage);
+    }
+
+    [Fact]
+    public void NonPositiveSecuredWritePendingTtl_IsValid()
+    {
+        // A non-positive TTL disables server-side expiry by design — not a config error.
+        var result = Validate(new ManagementServiceOptions { SecuredWritePendingTtl = TimeSpan.Zero });
+        Assert.True(result.Succeeded, result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroCommandTimeout_IsRejected()
+    {
+        var result = Validate(new ManagementServiceOptions { CommandTimeout = TimeSpan.Zero });
+
+        Assert.True(result.Failed);
+        Assert.Contains("CommandTimeout", result.FailureMessage);
+    }
+
+    [Fact]
+    public void NegativeCommandTimeout_IsRejected()
+    {
+        var result = Validate(new ManagementServiceOptions { CommandTimeout = TimeSpan.FromSeconds(-1) });
+
+        Assert.True(result.Failed);
+        Assert.Contains("CommandTimeout", result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroLongRunningCommandTimeout_IsRejected()
+    {
+        var result = Validate(new ManagementServiceOptions { LongRunningCommandTimeout = TimeSpan.Zero });
+
+        Assert.True(result.Failed);
+        Assert.Contains("LongRunningCommandTimeout", result.FailureMessage);
+    }
+}
@@ -0,0 +1,58 @@
+using Microsoft.Extensions.Options;
+
+namespace ZB.MOM.WW.ScadaBridge.NotificationService.Tests;
+
+/// <summary>
+/// Arch-review 08 §1.5: <see cref="NotificationOptions"/> supplies the SMTP fallback
+/// connection timeout and max-concurrent-connections used by the central Email delivery
+/// adapter when the deployed <c>SmtpConfiguration</c> leaves them unset. A non-positive
+/// value there is not a usable fallback; reject it fast at startup with a key-naming message.
+/// </summary>
+public class NotificationOptionsValidatorTests
+{
+    private static ValidateOptionsResult Validate(NotificationOptions options) =>
+        new NotificationOptionsValidator().Validate(Options.DefaultName, options);
+
+    [Fact]
+    public void DefaultOptions_AreValid()
+    {
+        var result = Validate(new NotificationOptions());
+        Assert.True(result.Succeeded, result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroConnectionTimeoutSeconds_IsRejected()
+    {
+        var result = Validate(new NotificationOptions { ConnectionTimeoutSeconds = 0 });
+
+        Assert.True(result.Failed);
+        Assert.Contains("ConnectionTimeoutSeconds", result.FailureMessage);
+    }
+
+    [Fact]
+    public void NegativeConnectionTimeoutSeconds_IsRejected()
+    {
+        var result = Validate(new NotificationOptions { ConnectionTimeoutSeconds = -1 });
+
+        Assert.True(result.Failed);
+        Assert.Contains("ConnectionTimeoutSeconds", result.FailureMessage);
+    }
+
+    [Fact]
+    public void ZeroMaxConcurrentConnections_IsRejected()
+    {
+        var result = Validate(new NotificationOptions { MaxConcurrentConnections = 0 });
+
+        Assert.True(result.Failed);
+        Assert.Contains("MaxConcurrentConnections", result.FailureMessage);
+    }
+
+    [Fact]
+    public void NegativeMaxConcurrentConnections_IsRejected()
+    {
+        var result = Validate(new NotificationOptions { MaxConcurrentConnections = -1 });
+
+        Assert.True(result.Failed);
+        Assert.Contains("MaxConcurrentConnections", result.FailureMessage);
+    }
+}