fix(cluster): T7's post-overlay guard was structurally unreachable — compare live Cluster:Roles, not the PostConfigured options.Roles (review finding, R3-04)
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.
This commit is contained in:
@@ -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<AkkaClusterOptions>
|
||||
{
|
||||
private readonly EffectiveRoles? _effectiveRoles;
|
||||
private readonly IConfiguration? _configuration;
|
||||
|
||||
/// <summary>
|
||||
/// DI-constructed by <c>AddValidatedOptions</c> (a plain <c>AddSingleton</c>), so a
|
||||
/// constructor dependency is safe here. <paramref name="effectiveRoles"/> is optional (default
|
||||
/// <see langword="null"/>) 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 <c>AddValidatedOptions</c> (a plain <c>AddSingleton</c>), so constructor
|
||||
/// dependencies are safe here. Both <paramref name="effectiveRoles"/> and
|
||||
/// <paramref name="configuration"/> are optional (default <see langword="null"/>) 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.
|
||||
/// </summary>
|
||||
/// <param name="effectiveRoles">The registered role authority, or null to skip the post-overlay guard.</param>
|
||||
public AkkaClusterOptionsValidator(EffectiveRoles? effectiveRoles = null)
|
||||
/// <param name="configuration">
|
||||
/// The live application configuration — deliberately NOT read through
|
||||
/// <paramref name="effectiveRoles"/>'s own snapshot (review finding, 2026-08-20): the guard
|
||||
/// needs to see a role overlay applied AFTER <paramref name="effectiveRoles"/> was resolved,
|
||||
/// and only a fresh read off the live <see cref="IConfiguration"/> can observe that.
|
||||
/// </param>
|
||||
public AkkaClusterOptionsValidator(EffectiveRoles? effectiveRoles = null, IConfiguration? configuration = null)
|
||||
{
|
||||
_effectiveRoles = effectiveRoles;
|
||||
_configuration = configuration;
|
||||
}
|
||||
|
||||
/// <inheritdoc />
|
||||
@@ -109,25 +119,78 @@ public sealed class AkkaClusterOptionsValidator : OptionsValidatorBase<AkkaClust
|
||||
/// R3-04 (choice 4, the residual divergence surface): <c>Program.cs</c> resolves
|
||||
/// <see cref="EffectiveRoles"/> BEFORE the per-role appsettings overlay loads (the overlay's
|
||||
/// filename depends on the answer), so if a role overlay — or a <c>${secret:}</c> expansion —
|
||||
/// ever set <c>Cluster:Roles</c> itself, the bound <paramref name="options"/>.Roles (which
|
||||
/// sees the FULL, post-overlay configuration) would silently disagree with the
|
||||
/// <see cref="EffectiveRoles"/> 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 <c>Cluster</c> section.
|
||||
/// ever set <c>Cluster:Roles</c> itself, that value would silently disagree with the
|
||||
/// <see cref="EffectiveRoles"/> instance every other consumer holds.
|
||||
/// </summary>
|
||||
/// <remarks>
|
||||
/// <para>
|
||||
/// ⚠️ <b>Review finding (2026-08-20): deliberately NOT <paramref name="options"/>.Roles.</b>
|
||||
/// <see cref="ServiceCollectionExtensions.AddOtOpcUaCluster"/>'s <c>PostConfigure</c>
|
||||
/// unconditionally sets <c>options.Roles = [.. effectiveRoles.Gossiped]</c> — the very
|
||||
/// structural-identity fix T2 landed — so <paramref name="options"/>.Roles and
|
||||
/// <see cref="_effectiveRoles"/>.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
|
||||
/// <c>An_overlay_that_changes_Cluster_Roles_after_resolution_fails_naming_both_views</c>,
|
||||
/// which failed (no throw) against the options.Roles-comparing version of this method.
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// Instead this re-reads the RAW <c>Cluster:Roles</c> section directly off the live
|
||||
/// <see cref="_configuration"/>, which — being the same mutable object
|
||||
/// <c>Program.cs</c>'s <c>builder.Configuration</c> is — reflects every provider added up
|
||||
/// to the point something first resolves <c>IOptions<AkkaClusterOptions></c>,
|
||||
/// INCLUDING a role overlay applied after <see cref="_effectiveRoles"/> was captured.
|
||||
/// Skipped when that raw value is empty — the ordinary shape for every
|
||||
/// <c>OTOPCUA_ROLES</c>-only node, where nothing meaningful lives under the
|
||||
/// <c>Cluster:Roles</c> key either before OR after an overlay, and comparing an empty
|
||||
/// array against a non-empty <c>Gossiped</c> list would be a permanent false positive for
|
||||
/// every env-only deployment (which CLAUDE.md notes is most of production).
|
||||
/// </para>
|
||||
/// <para>
|
||||
/// 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 <c>Cluster</c> section.
|
||||
/// </para>
|
||||
/// </remarks>
|
||||
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<string>(options.Roles ?? Array.Empty<string>(), StringComparer.OrdinalIgnoreCase);
|
||||
var postOverlayRaw = _configuration.GetSection(EffectiveRoles.ConfiguredRolesKey).Get<string[]>();
|
||||
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<string>(normalized, StringComparer.OrdinalIgnoreCase);
|
||||
var registered = new HashSet<string>(_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<string>())}]). A per-role appsettings "
|
||||
+ $"[{string.Join(",", normalized)}]). A per-role appsettings "
|
||||
+ "overlay may not set Cluster:Roles.");
|
||||
}
|
||||
|
||||
|
||||
@@ -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 —
|
||||
|
||||
@@ -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()
|
||||
/// <summary>
|
||||
/// Builds the REAL DI graph <c>Program.cs</c> builds — a live, mutable
|
||||
/// <see cref="ConfigurationManager"/> (the same "add a provider on the fly" shape
|
||||
/// <c>WebApplicationBuilder.Configuration</c> has, unlike a one-shot
|
||||
/// <see cref="ConfigurationBuilder"/>) — so a role overlay can be simulated as a SECOND
|
||||
/// provider added to the SAME configuration object AFTER <see cref="EffectiveRoles"/> was
|
||||
/// already resolved and registered, exactly as <c>Program.cs</c>'s
|
||||
/// <c>builder.Configuration.AddJsonFile(overlay)</c> mutates <c>builder.Configuration</c>
|
||||
/// after the role read at the top of the file.
|
||||
/// </summary>
|
||||
private static IServiceProvider BuildGraphWithOverlay(
|
||||
Dictionary<string, string?> baseConfig, Dictionary<string, string?>? 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<string, string?> { ["Cluster:Roles:0"] = "driver" },
|
||||
overlayConfig: new Dictionary<string, string?>
|
||||
{
|
||||
["Cluster:Roles:0"] = "admin",
|
||||
["Cluster:Roles:1"] = "driver",
|
||||
});
|
||||
|
||||
var result = new AkkaClusterOptionsValidator(effectiveRoles).Validate(null, options);
|
||||
var ex = Should.Throw<OptionsValidationException>(
|
||||
() => provider.GetRequiredService<IOptions<AkkaClusterOptions>>().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<string, string?>
|
||||
{
|
||||
["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<string, string?> { ["Serilog:MinimumLevel:Default"] = "Debug" });
|
||||
|
||||
Should.NotThrow(() => provider.GetRequiredService<IOptions<AkkaClusterOptions>>().Value);
|
||||
}
|
||||
|
||||
[Fact]
|
||||
public void No_overlay_at_all_passes()
|
||||
{
|
||||
var provider = BuildGraphWithOverlay(
|
||||
baseConfig: new Dictionary<string, string?> { ["Cluster:Roles:0"] = "driver" },
|
||||
overlayConfig: null);
|
||||
|
||||
Should.NotThrow(() => provider.GetRequiredService<IOptions<AkkaClusterOptions>>().Value);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
@@ -263,4 +324,21 @@ public sealed class AkkaClusterOptionsValidatorTests
|
||||
|
||||
result.Failed.ShouldBeFalse(result.FailureMessage);
|
||||
}
|
||||
|
||||
/// <summary>
|
||||
/// <see cref="EffectiveRoles"/> injected but <see cref="IConfiguration"/> not — the guard must
|
||||
/// still skip cleanly rather than null-reference. Mirrors the previous test from the other
|
||||
/// side of the two-dependency ctor.
|
||||
/// </summary>
|
||||
[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);
|
||||
}
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user