From af39e616382e68958dd932ad91be431dadd0f29e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 11:01:33 -0400 Subject: [PATCH] =?UTF-8?q?fix(cluster):=20T7's=20post-overlay=20guard=20w?= =?UTF-8?q?as=20structurally=20unreachable=20=E2=80=94=20compare=20live=20?= =?UTF-8?q?Cluster:Roles,=20not=20the=20PostConfigured=20options.Roles=20(?= =?UTF-8?q?review=20finding,=20R3-04)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit options.Roles is unconditionally overwritten by AddOtOpcUaCluster's PostConfigure to equal EffectiveRoles.Gossiped (T2's own structural-identity fix), so comparing options.Roles against Gossiped could never observe a divergence — an overlay setting Cluster:Roles was silently ignored, not fail-start. The validator now injects IConfiguration and re-reads the raw Cluster:Roles section LIVE (reflecting any overlay applied after EffectiveRoles was first resolved), normalized through EffectiveRoles.Resolve for reuse of its own validation. AddOtOpcUaCluster now TryAddSingleton(configuration) so a bare ServiceCollection test harness (not just WebApplicationBuilder) can supply IConfiguration to the validator. Both guard tests rewritten to go through the real AddOtOpcUaCluster + PostConfigure pipeline over a live ConfigurationManager, simulating an overlay as a second provider added AFTER EffectiveRoles was resolved off the same object — exactly Program.cs's own sequencing. Confirmed red beforehand: the "changes" test failed to throw against the pre-fix validator. --- .../AkkaClusterOptionsValidator.cs | 93 ++++++++++++--- .../ServiceCollectionExtensions.cs | 9 ++ .../AkkaClusterOptionsValidatorTests.cs | 110 +++++++++++++++--- 3 files changed, 181 insertions(+), 31 deletions(-) diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptionsValidator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptionsValidator.cs index ce683d76..f68302eb 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptionsValidator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptionsValidator.cs @@ -1,4 +1,5 @@ using Akka.Actor; +using Microsoft.Extensions.Configuration; using ZB.MOM.WW.Configuration; namespace ZB.MOM.WW.OtOpcUa.Cluster; @@ -37,18 +38,27 @@ namespace ZB.MOM.WW.OtOpcUa.Cluster; public sealed class AkkaClusterOptionsValidator : OptionsValidatorBase { private readonly EffectiveRoles? _effectiveRoles; + private readonly IConfiguration? _configuration; /// - /// DI-constructed by AddValidatedOptions (a plain AddSingleton), so a - /// constructor dependency is safe here. is optional (default - /// ) so a caller with no role-authority instance to hand — including - /// every test in this file that pre-dates R3-04 — keeps constructing this validator - /// parameterlessly; the post-overlay guard below (choice 4) is simply skipped when it is null. + /// DI-constructed by AddValidatedOptions (a plain AddSingleton), so constructor + /// dependencies are safe here. Both and + /// are optional (default ) so a caller + /// with neither to hand — including every test in this file that pre-dates R3-04 — keeps + /// constructing this validator parameterlessly; the post-overlay guard below (choice 4) is + /// simply skipped when either is null. /// /// The registered role authority, or null to skip the post-overlay guard. - public AkkaClusterOptionsValidator(EffectiveRoles? effectiveRoles = null) + /// + /// The live application configuration — deliberately NOT read through + /// 's own snapshot (review finding, 2026-08-20): the guard + /// needs to see a role overlay applied AFTER was resolved, + /// and only a fresh read off the live can observe that. + /// + public AkkaClusterOptionsValidator(EffectiveRoles? effectiveRoles = null, IConfiguration? configuration = null) { _effectiveRoles = effectiveRoles; + _configuration = configuration; } /// @@ -109,25 +119,78 @@ public sealed class AkkaClusterOptionsValidator : OptionsValidatorBaseProgram.cs resolves /// BEFORE the per-role appsettings overlay loads (the overlay's /// filename depends on the answer), so if a role overlay — or a ${secret:} expansion — - /// ever set Cluster:Roles itself, the bound .Roles (which - /// sees the FULL, post-overlay configuration) would silently disagree with the - /// instance every other consumer holds. Compares as SETS - /// (case-insensitively) rather than sequence, because ordering carries no meaning here and a - /// re-ordering is not the hazard this rule exists to catch — a genuinely different role set - /// is. A no-op today: none of the four shipped overlays carries a Cluster section. + /// ever set Cluster:Roles itself, that value would silently disagree with the + /// instance every other consumer holds. /// + /// + /// + /// ⚠️ Review finding (2026-08-20): deliberately NOT .Roles. + /// 's PostConfigure + /// unconditionally sets options.Roles = [.. effectiveRoles.Gossiped] — the very + /// structural-identity fix T2 landed — so .Roles and + /// .Gossiped are EQUAL BY CONSTRUCTION on every real host. + /// Comparing them here would make this guard structurally unreachable: it could never + /// observe a divergence no matter what an overlay did, because the same PostConfigure step + /// that a later overlay's Cluster:Roles bind feeds into is exactly what overwrites the + /// difference away before this validator ever runs. Caught by + /// An_overlay_that_changes_Cluster_Roles_after_resolution_fails_naming_both_views, + /// which failed (no throw) against the options.Roles-comparing version of this method. + /// + /// + /// Instead this re-reads the RAW Cluster:Roles section directly off the live + /// , which — being the same mutable object + /// Program.cs's builder.Configuration is — reflects every provider added up + /// to the point something first resolves IOptions<AkkaClusterOptions>, + /// INCLUDING a role overlay applied after was captured. + /// Skipped when that raw value is empty — the ordinary shape for every + /// OTOPCUA_ROLES-only node, where nothing meaningful lives under the + /// Cluster:Roles key either before OR after an overlay, and comparing an empty + /// array against a non-empty Gossiped list would be a permanent false positive for + /// every env-only deployment (which CLAUDE.md notes is most of production). + /// + /// + /// Compares as SETS (case-insensitively) rather than sequence, because ordering carries no + /// meaning here and a re-ordering is not the hazard this rule exists to catch — a + /// genuinely different role set is. A no-op today for the "nothing changed" case: none of + /// the four shipped overlays carries a Cluster section. + /// + /// private void ValidateAgainstEffectiveRoles(ValidationBuilder builder, AkkaClusterOptions options) { - if (_effectiveRoles is null) return; + _ = options; // intentionally unused — see remarks: comparing against it would be vacuous. + if (_effectiveRoles is null || _configuration is null) return; - var bound = new HashSet(options.Roles ?? Array.Empty(), StringComparer.OrdinalIgnoreCase); + var postOverlayRaw = _configuration.GetSection(EffectiveRoles.ConfiguredRolesKey).Get(); + if (postOverlayRaw is not { Length: > 0 }) + { + // Cluster:Roles is empty post-overlay too -> nothing to compare. Correct for every + // OTOPCUA_ROLES-only node, not merely "no data yet". + return; + } + + string[] normalized; + try + { + // Reuses EffectiveRoles' own configured-path normalization/validation rather than + // re-implementing it (archreview theme 4 — one rule, stated once). + normalized = [.. EffectiveRoles.Resolve(postOverlayRaw, envRoles: null).Gossiped]; + } + catch (ArgumentException ex) + { + builder.Add( + "Cluster:Roles was changed by a configuration layer applied after role resolution to " + + $"an invalid value ({ex.Message}). A per-role appsettings overlay may not set Cluster:Roles."); + return; + } + + var bound = new HashSet(normalized, StringComparer.OrdinalIgnoreCase); var registered = new HashSet(_effectiveRoles.Gossiped, StringComparer.OrdinalIgnoreCase); if (bound.SetEquals(registered)) return; builder.Add( "Cluster:Roles was changed by a configuration layer applied after role resolution " + $"(host view: [{string.Join(",", _effectiveRoles.Gossiped)}], bound: " - + $"[{string.Join(",", options.Roles ?? Array.Empty())}]). A per-role appsettings " + + $"[{string.Join(",", normalized)}]). A per-role appsettings " + "overlay may not set Cluster:Roles."); } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs index a9b944c9..16f9e466 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs @@ -45,6 +45,15 @@ public static class ServiceCollectionExtensions // TryAddSingleton keeps this usable from a harness that registers its own EffectiveRoles first. services.TryAddSingleton(_ => EffectiveRoles.Resolve(configuration)); + // Review finding (2026-08-20): AkkaClusterOptionsValidator's post-overlay guard needs to + // re-read Cluster:Roles LIVE, off the current (possibly overlay-mutated) configuration — not + // off the pre-overlay EffectiveRoles snapshot above, and not off the bound AkkaClusterOptions + // (the PostConfigure two lines below makes that equal to the snapshot BY CONSTRUCTION, so + // comparing the guard against it could never observe a divergence). A bare ServiceCollection + // (unlike WebApplicationBuilder, which auto-registers IConfiguration) never registers this + // otherwise; TryAddSingleton is a no-op on a real host, where it is already registered. + services.TryAddSingleton(configuration); + // Gitea #532 (superseded by R3-04): apply the OTOPCUA_ROLES fallback the Roles property has // always DOCUMENTED, in the one place every consumer reads through. Before this, a node // configured only through the env var — the knob that drives all of Program.cs's role wiring — diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsValidatorTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsValidatorTests.cs index 91140bf4..7a5cb022 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsValidatorTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsValidatorTests.cs @@ -1,3 +1,6 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; using Shouldly; using Xunit; @@ -220,32 +223,90 @@ public sealed class AkkaClusterOptionsValidatorTests // resolves EffectiveRoles BEFORE the overlay loads (it needs the answer to pick the overlay's // filename), so if a later config layer changed Cluster:Roles the bound options would disagree // with every consumer holding the registered EffectiveRoles instance. + // + // Review finding (2026-08-20): these MUST go through the real AddOtOpcUaCluster + PostConfigure + // pipeline, not hand-built AkkaClusterOptions. options.Roles is unconditionally overwritten by + // ServiceCollectionExtensions' PostConfigure to equal EffectiveRoles.Gossiped (T2's structural + // identity) — so a validator that compared against options.Roles could NEVER observe a + // divergence; it would always be equal by construction. The guard has to re-read the raw + // Cluster:Roles section LIVE off IConfiguration instead, which is what actually reflects a role + // overlay applied after EffectiveRoles was first resolved. - [Fact] - public void A_config_layer_that_changes_Cluster_Roles_after_resolution_fails_naming_both_views() + /// + /// Builds the REAL DI graph Program.cs builds — a live, mutable + /// (the same "add a provider on the fly" shape + /// WebApplicationBuilder.Configuration has, unlike a one-shot + /// ) — so a role overlay can be simulated as a SECOND + /// provider added to the SAME configuration object AFTER was + /// already resolved and registered, exactly as Program.cs's + /// builder.Configuration.AddJsonFile(overlay) mutates builder.Configuration + /// after the role read at the top of the file. + /// + private static IServiceProvider BuildGraphWithOverlay( + Dictionary baseConfig, Dictionary? overlayConfig) { - var effectiveRoles = EffectiveRoles.Resolve(["driver"], null); - var options = CentralNode("site-a-1", Seed("site-a-1")); - options.Roles = new[] { "admin", "driver" }; // the post-overlay bound value disagrees + var configuration = new ConfigurationManager(); + configuration.AddInMemoryCollection(baseConfig); - var result = new AkkaClusterOptionsValidator(effectiveRoles).Validate(null, options); + // Program.cs:61-63 (T3) — resolved BEFORE the overlay loads, off the pre-overlay snapshot. + var effectiveRoles = EffectiveRoles.Resolve(configuration); - result.Failed.ShouldBeTrue(); - result.FailureMessage.ShouldContain("Cluster:Roles"); - result.FailureMessage.ShouldContain("driver"); - result.FailureMessage.ShouldContain("admin"); + var services = new ServiceCollection(); + services.AddLogging(); + services.AddSingleton(effectiveRoles); + + // The overlay: a later provider added to the SAME live configuration object. + if (overlayConfig is not null) + configuration.AddInMemoryCollection(overlayConfig); + + services.AddOtOpcUaCluster(configuration); + return services.BuildServiceProvider(); } [Fact] - public void Bound_Roles_matching_the_registered_EffectiveRoles_passes() + public void An_overlay_that_changes_Cluster_Roles_after_resolution_fails_naming_both_views() { - var effectiveRoles = EffectiveRoles.Resolve(["driver", "cluster-SITE-A"], null); - var options = CentralNode("site-a-1", Seed("site-a-1")); - options.Roles = new[] { "driver", "cluster-SITE-A" }; + var provider = BuildGraphWithOverlay( + baseConfig: new Dictionary { ["Cluster:Roles:0"] = "driver" }, + overlayConfig: new Dictionary + { + ["Cluster:Roles:0"] = "admin", + ["Cluster:Roles:1"] = "driver", + }); - var result = new AkkaClusterOptionsValidator(effectiveRoles).Validate(null, options); + var ex = Should.Throw( + () => provider.GetRequiredService>().Value); - result.Failed.ShouldBeFalse(result.FailureMessage); + var message = string.Join(" ", ex.Failures); + message.ShouldContain("Cluster:Roles"); + message.ShouldContain("driver"); + message.ShouldContain("admin"); + } + + [Fact] + public void An_overlay_that_repeats_the_same_Cluster_Roles_passes() + { + var provider = BuildGraphWithOverlay( + baseConfig: new Dictionary + { + ["Cluster:Roles:0"] = "driver", + ["Cluster:Roles:1"] = "cluster-SITE-A", + }, + // A real overlay never touches Cluster:Roles (the documented invariant) — this is the + // "no-op overlay" shape every one of the four shipped overlays actually is today. + overlayConfig: new Dictionary { ["Serilog:MinimumLevel:Default"] = "Debug" }); + + Should.NotThrow(() => provider.GetRequiredService>().Value); + } + + [Fact] + public void No_overlay_at_all_passes() + { + var provider = BuildGraphWithOverlay( + baseConfig: new Dictionary { ["Cluster:Roles:0"] = "driver" }, + overlayConfig: null); + + Should.NotThrow(() => provider.GetRequiredService>().Value); } /// @@ -263,4 +324,21 @@ public sealed class AkkaClusterOptionsValidatorTests result.Failed.ShouldBeFalse(result.FailureMessage); } + + /// + /// injected but not — the guard must + /// still skip cleanly rather than null-reference. Mirrors the previous test from the other + /// side of the two-dependency ctor. + /// + [Fact] + public void EffectiveRoles_injected_without_IConfiguration_skips_the_guard_entirely() + { + var effectiveRoles = EffectiveRoles.Resolve(["driver"], null); + var options = CentralNode("site-a-1", Seed("site-a-1")); + options.Roles = new[] { "something-else-entirely" }; + + var result = new AkkaClusterOptionsValidator(effectiveRoles).Validate(null, options); + + result.Failed.ShouldBeFalse(result.FailureMessage); + } }