From 77026167169167e42309903d8944a234bc90ee7d Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:41:54 -0400 Subject: [PATCH 01/15] =?UTF-8?q?feat(cluster):=20EffectiveRoles=20?= =?UTF-8?q?=E2=80=94=20one=20role=20authority=20with=20one=20precedence=20?= =?UTF-8?q?+=20two-layer=20normalization=20(archreview=2001/S-12,=2001/C-9?= =?UTF-8?q?)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...3-04-one-role-authority-plan.md.tasks.json | 4 +- .../EffectiveRoles.cs | 164 ++++++++++++++ .../ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs | 15 +- .../EffectiveRolesTests.cs | 213 ++++++++++++++++++ 4 files changed, 388 insertions(+), 8 deletions(-) create mode 100644 src/Core/ZB.MOM.WW.OtOpcUa.Cluster/EffectiveRoles.cs create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EffectiveRolesTests.cs diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index 52d0a593..b203876b 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -5,7 +5,7 @@ { "id": "T1", "subject": "EffectiveRoles value type: one factory, one precedence (Cluster:Roles > OTOPCUA_ROLES), two-layer normalization, both-source validation (01/S-12, 01/C-9)", - "status": "pending", + "status": "completed", "blockedBy": [] }, { @@ -94,7 +94,7 @@ }, { "id": "T13", - "subject": "LIVE GATE: docker-dev six-node rig — pre-fix baseline, per-node banner vs /hosts gossip parity, site-a Primary election, deploy seals Applied incl. site-b-2, unknown-role fail-fast", + "subject": "LIVE GATE: docker-dev six-node rig \u2014 pre-fix baseline, per-node banner vs /hosts gossip parity, site-a Primary election, deploy seals Applied incl. site-b-2, unknown-role fail-fast", "status": "pending", "blockedBy": [ "T3", diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/EffectiveRoles.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/EffectiveRoles.cs new file mode 100644 index 00000000..c06dc4c0 --- /dev/null +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/EffectiveRoles.cs @@ -0,0 +1,164 @@ +using Microsoft.Extensions.Configuration; + +namespace ZB.MOM.WW.OtOpcUa.Cluster; + +/// Which of the two role sources resolved from. +public enum RoleSource +{ + /// Neither source named any role. + None, + + /// Cluster:Roles was non-empty and won outright. + Configured, + + /// Cluster:Roles was empty; OTOPCUA_ROLES supplied the roles. + Environment, +} + +/// The node's effective cluster roles — the SINGLE authority for "what is this node". +/// Resolved once, from one documented precedence, and consumed by BOTH the Host's conditional +/// wiring and the Akka role list, so the two cannot disagree (archreview 01/S-12 = 03/C5). +/// +/// Precedence: Cluster:Roles when non-empty, else OTOPCUA_ROLES, +/// else empty. Unchanged from +/// — what changed is that Program.cs now obeys it too, instead of reading the environment +/// directly. +/// Normalization is two-layer, on purpose. lower-cases plain +/// roles (Akka matches gossiped role names ORDINALLY, and every consumer compares against a +/// lowercase constant) but keeps a cluster role's ClusterId suffix VERBATIM — lower-casing that +/// would rename cluster-SITE-A on a running fleet, and a rolling pair upgrade would gossip +/// two different role names mid-window. Lower-casing a plain role changes the wire only for a +/// node that is already broken (archreview 01/S-12 = 03/C5, 01/C-9). +/// Validation runs on BOTH sources. The environment path was always validated by +/// ; the configured path previously accepted anything verbatim +/// (Cluster:Roles: ["Drivr"] silently gossiped a role that matched nothing). Both now fail +/// fast, naming the offending key, on an unknown role — a miscased but KNOWN role still +/// normalizes silently rather than failing, or an operator's Driver would brick the node +/// at the exact moment they upgrade. +/// +public sealed class EffectiveRoles +{ + private EffectiveRoles(IReadOnlyList gossiped, RoleSource source) + { + Gossiped = gossiped; + Source = source; + ClusterRole = RoleParser.ClusterRoleFromRoles(gossiped); + ClusterId = ClusterRole is null ? null : RoleParser.ClusterIdFromRole(ClusterRole); + } + + /// The configuration key reads for the configured + /// side of the precedence. + public const string ConfiguredRolesKey = "Cluster:Roles"; + + /// The roles as they are handed to Akka — becomes + /// this list, unmodified. + public IReadOnlyList Gossiped { get; } + + /// Which source resolved these roles. + public RoleSource Source { get; } + + /// True when is among (case-insensitive). + public bool HasAdmin => Has(RoleParser.Admin); + + /// True when is among (case-insensitive). + public bool HasDriver => Has(RoleParser.Driver); + + /// True when is among (case-insensitive). + public bool HasDev => Has(RoleParser.Dev); + + /// True when is among , compared case-insensitively. + /// The role to test for. + public bool Has(string role) => + Gossiped.Any(r => string.Equals(r, role, StringComparison.OrdinalIgnoreCase)); + + /// The first cluster-scoped role in , original spelling preserved, + /// or null when this node carries none. + public string? ClusterRole { get; } + + /// The ClusterId this node belongs to (the suffix of ), or null + /// for a legacy / single-mesh node. + public string? ClusterId { get; } + + /// The one factory: resolves the effective roles off 's + /// Cluster:Roles and the process's OTOPCUA_ROLES environment variable. + /// The application configuration. + /// The resolved . + /// Either source names an unknown role. + public static EffectiveRoles Resolve(IConfiguration configuration) + { + ArgumentNullException.ThrowIfNull(configuration); + + var configured = configuration.GetSection(ConfiguredRolesKey).Get(); + var envRoles = Environment.GetEnvironmentVariable(RoleParser.RolesEnvVar); + return Resolve(configured, envRoles); + } + + /// Resolves the effective roles from explicit values — the overload + /// delegates to. + /// The bound Cluster:Roles value. + /// The raw OTOPCUA_ROLES value. + /// The resolved . + /// Either source names an unknown role. + public static EffectiveRoles Resolve(IReadOnlyList? configured, string? envRoles) + { + if (configured is { Count: > 0 }) + { + return new EffectiveRoles(NormalizeAndValidateConfigured(configured), RoleSource.Configured); + } + + if (!string.IsNullOrWhiteSpace(envRoles)) + { + // RoleParser.Parse already trims, lower-cases (fully — including a cluster suffix, which + // is intentionally different from the configured path; see ClusterIdFromRole's remarks), + // dedupes and validates. + return new EffectiveRoles(RoleParser.Parse(envRoles), RoleSource.Environment); + } + + return new EffectiveRoles(Array.Empty(), RoleSource.None); + } + + /// Normalizes and validates the Cluster:Roles entries: a plain role is trimmed and + /// lower-cased; a cluster role keeps its ClusterId suffix verbatim but gets a normalized + /// (lower-case) cluster- prefix. Throws naming the offending index and value on anything + /// that is neither. + private static string[] NormalizeAndValidateConfigured(IReadOnlyList configured) + { + var result = new string[configured.Count]; + for (var i = 0; i < configured.Count; i++) + { + var raw = configured[i]; + var trimmed = raw?.Trim() ?? string.Empty; + + if (trimmed.Length == 0) + { + throw Unknown(i, raw); + } + + if (RoleParser.IsClusterRole(trimmed)) + { + var clusterId = RoleParser.ClusterIdFromRole(trimmed); + if (clusterId.Length == 0) + { + throw Unknown(i, raw); + } + + result[i] = RoleParser.ClusterRolePrefix + clusterId; + continue; + } + + var lowered = trimmed.ToLowerInvariant(); + if (lowered is not (RoleParser.Admin or RoleParser.Driver or RoleParser.Dev)) + { + throw Unknown(i, raw); + } + + result[i] = lowered; + } + + return result; + } + + private static ArgumentException Unknown(int index, string? raw) => + new($"{ConfiguredRolesKey}[{index}] = '{raw}' is not a known role. Allowed: {RoleParser.Admin}, " + + $"{RoleParser.Driver}, {RoleParser.Dev}, or '{RoleParser.ClusterRolePrefix}'."); +} diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs index 789a0084..f9d58a6b 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs @@ -111,12 +111,15 @@ public static class RoleParser /// The bound Cluster:Roles value. /// The raw OTOPCUA_ROLES value. /// The effective roles; empty when neither source names any. - /// The environment value names an unknown role. - public static string[] ResolveEffectiveRoles(IReadOnlyList? configuredRoles, string? envRoles) - { - if (configuredRoles is { Count: > 0 }) return [.. configuredRoles]; - return Parse(envRoles); - } + /// Either source names an unknown role. + /// + /// Thin delegation to — + /// the single role authority (archreview R3-04). is the type to + /// reach for directly; this overload is retained so existing callers of the array-returning + /// shape keep compiling. + /// + public static string[] ResolveEffectiveRoles(IReadOnlyList? configuredRoles, string? envRoles) => + [.. EffectiveRoles.Resolve(configuredRoles, envRoles).Gossiped]; /// Reads the effective roles for the current process, resolving the environment itself. /// The bound Cluster:Roles value. diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EffectiveRolesTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EffectiveRolesTests.cs new file mode 100644 index 00000000..d7dce4ca --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EffectiveRolesTests.cs @@ -0,0 +1,213 @@ +using Microsoft.Extensions.Configuration; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; + +/// +/// is the single role authority (archreview R3-04, 01/S-12 = 03/C5, +/// 01/C-9): one factory, one precedence (Cluster:Roles non-empty wins, else +/// OTOPCUA_ROLES, else empty), two-layer normalization (plain roles lower-cased, a cluster +/// role's ClusterId suffix preserved verbatim), and validation on BOTH sources. +/// +[Collection(RoleEnvironmentCollection.Name)] +public sealed class EffectiveRolesTests +{ + // ---- Precedence matrix ------------------------------------------------------------------- + + [Fact] + public void Configured_only_resolves_from_Configured_source() + { + var roles = EffectiveRoles.Resolve(["admin", "driver"], null); + + roles.Source.ShouldBe(RoleSource.Configured); + roles.Gossiped.ShouldBe(new[] { "admin", "driver" }); + } + + [Fact] + public void Environment_only_resolves_from_Environment_source() + { + var roles = EffectiveRoles.Resolve(null, "admin,driver"); + + roles.Source.ShouldBe(RoleSource.Environment); + roles.Gossiped.ShouldBe(new[] { "admin", "driver" }); + } + + [Fact] + public void Both_agree_resolves_from_Configured_source() + { + var roles = EffectiveRoles.Resolve(["driver"], "driver"); + + roles.Source.ShouldBe(RoleSource.Configured); + roles.Gossiped.ShouldBe(new[] { "driver" }); + } + + [Fact] + public void Both_differ_Configured_wins_outright() + { + var roles = EffectiveRoles.Resolve(["admin"], "driver,cluster-SITE-A"); + + roles.Source.ShouldBe(RoleSource.Configured); + roles.Gossiped.ShouldBe(new[] { "admin" }); + } + + [Fact] + public void Neither_source_resolves_empty_with_None_source() + { + var roles = EffectiveRoles.Resolve(null, null); + + roles.Source.ShouldBe(RoleSource.None); + roles.Gossiped.ShouldBeEmpty(); + } + + [Fact] + public void Empty_configured_array_falls_back_to_the_environment() + { + var roles = EffectiveRoles.Resolve([], "driver"); + + roles.Source.ShouldBe(RoleSource.Environment); + roles.Gossiped.ShouldBe(new[] { "driver" }); + } + + // ---- Gossiped normalization: two-layer (choice 2) ---------------------------------------- + + [Fact] + public void Configured_plain_role_is_trimmed_and_lower_cased_in_Gossiped() + { + var roles = EffectiveRoles.Resolve([" Driver "], null); + + roles.Gossiped.ShouldBe(new[] { "driver" }); + } + + /// + /// THE falsification control for choice 2: a cluster role's ClusterId suffix must survive + /// UNCHANGED — lower-casing it would rename the Akka role of a running deployment + /// (cluster-SITE-A vs cluster-site-a are different gossiped roles). + /// + [Fact] + public void Configured_cluster_role_suffix_case_is_preserved_in_Gossiped() + { + var roles = EffectiveRoles.Resolve(["Cluster-SITE-A"], null); + + // The "cluster-" prefix normalizes (it is a plain marker), the "SITE-A" suffix does not. + roles.Gossiped.ShouldBe(new[] { "cluster-SITE-A" }); + } + + [Fact] + public void Configured_cluster_role_with_already_lowercase_prefix_keeps_suffix_verbatim() + { + var roles = EffectiveRoles.Resolve(["cluster-Site-A"], null); + + roles.Gossiped.ShouldBe(new[] { "cluster-Site-A" }); + } + + [Fact] + public void Environment_cluster_role_suffix_IS_lowercased_unlike_Configured() + { + // Unchanged from RoleParser.Parse's existing (documented) behaviour: the two sources + // normalize differently on purpose (RoleParser.ClusterIdFromRole's remarks). + var roles = EffectiveRoles.Resolve(null, "driver,cluster-SITE-A"); + + roles.Gossiped.ShouldBe(new[] { "driver", "cluster-site-a" }); + } + + // ---- Has*/HasAdmin/HasDriver case-insensitivity ------------------------------------------ + + [Theory] + [InlineData("driver")] + [InlineData("Driver")] + [InlineData("DRIVER")] + [InlineData(" driver ")] + public void HasDriver_true_for_every_casing(string configuredRole) + { + EffectiveRoles.Resolve([configuredRole], null).HasDriver.ShouldBeTrue(); + } + + [Fact] + public void HasAdmin_false_when_admin_is_not_present() + { + EffectiveRoles.Resolve(["driver"], null).HasAdmin.ShouldBeFalse(); + } + + [Fact] + public void Has_is_case_insensitive_for_an_arbitrary_role() + { + var roles = EffectiveRoles.Resolve(["Admin", "Driver"], null); + + roles.Has("admin").ShouldBeTrue(); + roles.Has("ADMIN").ShouldBeTrue(); + roles.Has("driver").ShouldBeTrue(); + roles.Has("dev").ShouldBeFalse(); + } + + // ---- Unknown / invalid role from EITHER source throws, naming the key -------------------- + + [Fact] + public void Unknown_role_from_the_configured_source_throws_naming_the_key() + { + var ex = Should.Throw(() => EffectiveRoles.Resolve(["Drivr"], null)); + + ex.Message.ShouldContain("Cluster:Roles"); + ex.Message.ShouldContain("Drivr"); + } + + [Fact] + public void Unknown_role_from_the_environment_source_throws() + { + // The env path already validated via RoleParser.Parse — this pins that it still does. + Should.Throw(() => EffectiveRoles.Resolve(null, "admin,master")); + } + + // ---- ClusterRole / ClusterId --------------------------------------------------------------- + + [Fact] + public void ClusterRole_and_ClusterId_are_null_for_a_legacy_node() + { + var roles = EffectiveRoles.Resolve(["admin", "driver"], null); + + roles.ClusterRole.ShouldBeNull(); + roles.ClusterId.ShouldBeNull(); + } + + [Fact] + public void ClusterRole_and_ClusterId_resolve_for_a_split_topology_node() + { + var roles = EffectiveRoles.Resolve(["driver", "cluster-SITE-A"], null); + + roles.ClusterRole.ShouldBe("cluster-SITE-A"); + roles.ClusterId.ShouldBe("SITE-A"); + } + + // ---- Resolve(IConfiguration) overload ------------------------------------------------------ + + [Fact] + public void Resolve_from_configuration_reads_Cluster_Roles() + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Cluster:Roles:0"] = "driver", + ["Cluster:Roles:1"] = "cluster-SITE-A", + }) + .Build(); + + var roles = EffectiveRoles.Resolve(configuration); + + roles.Source.ShouldBe(RoleSource.Configured); + roles.Gossiped.ShouldBe(new[] { "driver", "cluster-SITE-A" }); + } + + [Fact] + public void Resolve_from_configuration_falls_back_to_the_environment() + { + using (new EnvVarScope(RoleParser.RolesEnvVar, "admin,driver")) + { + var configuration = new ConfigurationBuilder().Build(); + + var roles = EffectiveRoles.Resolve(configuration); + + roles.Source.ShouldBe(RoleSource.Environment); + roles.Gossiped.ShouldBe(new[] { "admin", "driver" }); + } + } +} From 7dfd6dcc9fa58d52fbcdf2762558b70d9f82e801 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:43:54 -0400 Subject: [PATCH 02/15] fix(cluster): AkkaClusterOptions.Roles is the EffectiveRoles instance, not a second derivation --- ...3-04-one-role-authority-plan.md.tasks.json | 2 +- .../ServiceCollectionExtensions.cs | 37 ++++++++++++------- .../AkkaClusterOptionsRoleFallbackTests.cs | 30 +++++++++++++++ 3 files changed, 55 insertions(+), 14 deletions(-) diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index b203876b..8a84f165 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -11,7 +11,7 @@ { "id": "T2", "subject": "AddOtOpcUaCluster post-configures AkkaClusterOptions.Roles from the registered EffectiveRoles instance (structural identity, no second derivation)", - "status": "pending", + "status": "completed", "blockedBy": [ "T1" ] diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs index a8d09b08..a9b944c9 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs @@ -6,6 +6,7 @@ using Akka.Logger.Serilog; using Akka.Remote.Hosting; using Microsoft.Extensions.Configuration; using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.DependencyInjection.Extensions; using Microsoft.Extensions.Options; using ZB.MOM.WW.Configuration; using ZB.MOM.WW.OtOpcUa.Commons.Interfaces; @@ -34,21 +35,31 @@ public static class ServiceCollectionExtensions services.AddValidatedOptions( configuration, AkkaClusterOptions.SectionName); - // Gitea #532: 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 — bound an EMPTY Roles array and so - // joined the cluster role-less: invisible to RoleLeader/MembersWithRole, ineligible for every - // role-scoped singleton, and un-deployable ("no driver-role nodes are Up") with nothing in its - // own logs to say so. Doing it here rather than at each call site means BuildClusterOptions - // (Akka's own member roles), ClusterRoleInfo, the ControlPlane singleton scope and - // ConfigDbApiKeyStore's ClusterId all see the same answer. + // R3-04 (structural identity, choice 3): EffectiveRoles is resolved ONCE and registered as a + // singleton, off the SAME IConfiguration this call received — not off a DI-resolved + // IConfiguration, which a plain ServiceCollection (as opposed to a WebApplicationBuilder) never + // registers. Every consumer (Program.cs's hasAdmin/hasDriver, the four role-scoped validators, + // AkkaClusterOptions.Roles below) therefore reads the exact same instance, so there is no second + // derivation left to disagree with — the boot-guard OVERALL asked for is this identity, not a + // runtime assert (see EffectiveRoles + AkkaClusterOptionsValidator's post-overlay guard). + // TryAddSingleton keeps this usable from a harness that registers its own EffectiveRoles first. + services.TryAddSingleton(_ => EffectiveRoles.Resolve(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 — + // bound an EMPTY Roles array and so joined the cluster role-less: invisible to + // RoleLeader/MembersWithRole, ineligible for every role-scoped singleton, and un-deployable + // ("no driver-role nodes are Up") with nothing in its own logs to say so. Doing it here rather + // than at each call site means BuildClusterOptions (Akka's own member roles), ClusterRoleInfo, + // the ControlPlane singleton scope and ConfigDbApiKeyStore's ClusterId all see the same answer. // // PostConfigure, not a bind trick: it runs after the section binds and before ValidateOnStart, - // so the validators below see the effective list. An unknown role in the env var throws out of - // RoleParser.Parse — the same throw Program.cs already takes on the same value, so an invalid - // env var cannot reach here on the production path in the first place. - services.PostConfigure( - options => options.Roles = RoleParser.ResolveEffectiveRoles(options.Roles)); + // so the validators below see the effective list. Now consumes the registered EffectiveRoles + // instance rather than re-deriving from options.Roles + the environment — options.Roles becomes + // BY CONSTRUCTION the same list Program.cs's hasAdmin/hasDriver are computed from. + services.AddOptions().PostConfigure( + (options, roles) => options.Roles = [.. roles.Gossiped]); // Per-cluster mesh Phase 2: which transport carries central→node commands. Validated at // startup for the same reason the cluster options are — a contact point that cannot resolve diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsRoleFallbackTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsRoleFallbackTests.cs index 90ecdb3c..a3e2a814 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsRoleFallbackTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsRoleFallbackTests.cs @@ -94,4 +94,34 @@ public class AkkaClusterOptionsRoleFallbackTests RoleParser.ClusterIdFromRoles(Bind().Roles).ShouldBe("site-a"); } } + + /// + /// R3-04 (structural identity, choice 3): AkkaClusterOptions.Roles is not a SECOND + /// derivation of the role rule — it is the registered instance's + /// list, verbatim. A mixed-case configured role proves + /// both the normalization (plain lowered, cluster suffix preserved) and the identity: the two + /// views can never disagree because there is only one derivation. + /// + [Fact] + public void Bound_options_Roles_IS_the_registered_EffectiveRoles_Gossiped_list() + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(new Dictionary + { + ["Cluster:Roles:0"] = "Driver", + ["Cluster:Roles:1"] = "Cluster-SITE-A", + }) + .Build(); + + var services = new ServiceCollection(); + services.AddLogging(); + services.AddOtOpcUaCluster(configuration); + var provider = services.BuildServiceProvider(); + + var bound = provider.GetRequiredService>().Value; + var effectiveRoles = provider.GetRequiredService(); + + bound.Roles.ShouldBe(new[] { "driver", "cluster-SITE-A" }); + bound.Roles.ShouldBe(effectiveRoles.Gossiped); + } } From 792e8f422a0f3f3addb375373928578915365d64 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:46:27 -0400 Subject: [PATCH 03/15] fix(host): Program.cs derives roles from the one authority, not OTOPCUA_ROLES alone (archreview 01/S-12 = 03/C5) --- ...3-04-one-role-authority-plan.md.tasks.json | 2 +- src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs | 23 ++++++++++++------- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index 8a84f165..27da61a3 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -19,7 +19,7 @@ { "id": "T3", "subject": "Program.cs derives hasAdmin/hasDriver/roleSuffix/banner from the authority, not OTOPCUA_ROLES alone (01/S-12 = 03/C5)", - "status": "pending", + "status": "completed", "blockedBy": [ "T2" ] diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs index 12eeee6b..5567a2c0 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs @@ -57,13 +57,19 @@ using ZB.MOM.WW.Secrets.DependencyInjection; using ZB.MOM.WW.Secrets.Replicator.AkkaDotNet; using ZB.MOM.WW.Secrets.Sqlite; -// Roles drive the entire conditional wiring below — see ZB.MOM.WW.OtOpcUa.Cluster.RoleParser. -var roles = RoleParser.Parse(Environment.GetEnvironmentVariable("OTOPCUA_ROLES")); -var hasAdmin = roles.Contains("admin"); -var hasDriver = roles.Contains("driver"); - var builder = WebApplication.CreateBuilder(args); +// The SINGLE role authority: Cluster:Roles first, OTOPCUA_ROLES as the fallback (archreview +// 01/S-12 = 03/C5, R3-04). Resolved off builder.Configuration BEFORE the per-role overlay is +// added, because the overlay's FILENAME depends on the answer — which is why a role overlay may +// not itself set Cluster:Roles (AkkaClusterOptionsValidator fails start if one ever does). +// EffectiveRoles drives the entire conditional wiring below — see ZB.MOM.WW.OtOpcUa.Cluster. +var effectiveRoles = EffectiveRoles.Resolve(builder.Configuration); +var roles = effectiveRoles.Gossiped; +var hasAdmin = effectiveRoles.HasAdmin; +var hasDriver = effectiveRoles.HasDriver; +builder.Services.AddSingleton(effectiveRoles); + // Razor class library static assets (_content//...) only auto-enable in // the Development environment. Opt in explicitly so the AdminUI's CSS/JS works // regardless of ASPNETCORE_ENVIRONMENT. @@ -77,7 +83,7 @@ builder.Host.UseWindowsService(options => options.ServiceName = "OtOpcUaHost"); // Per-role appsettings overlay: appsettings.{role}.json (single role) or appsettings.admin-driver.json // (both). Optional — base appsettings.json carries enough to boot if these don't exist. -var roleSuffix = roles.Length == 0 ? null : string.Join('-', roles.OrderBy(r => r, StringComparer.Ordinal)); +var roleSuffix = roles.Count == 0 ? null : string.Join('-', roles.OrderBy(r => r, StringComparer.Ordinal)); if (roleSuffix is not null) { builder.Configuration.AddJsonFile($"appsettings.{roleSuffix}.json", optional: true, reloadOnChange: true); @@ -786,8 +792,9 @@ if (hasDriver && telemetryListenPort > 0) app.MapOtOpcUaHealth(); app.MapOtOpcUaMetrics(); -Log.Information("OtOpcUa.Host starting with roles=[{Roles}] (admin={HasAdmin}, driver={HasDriver})", - string.Join(",", roles), hasAdmin, hasDriver); +Log.Information( + "OtOpcUa.Host starting with roles=[{Roles}] (admin={HasAdmin}, driver={HasDriver}) source={Source}", + string.Join(",", roles), hasAdmin, hasDriver, effectiveRoles.Source); // #561 — startup growth self-check: the banner above went through the live pipeline; verify each // configured file sink physically grew. A sink whose writes silently fail (the wonder-app-vd03 From bcec88ef70c4ea7bff1cc96e354816ff6d583269 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 09:51:23 -0400 Subject: [PATCH 04/15] =?UTF-8?q?fix(cluster):=20all=20four=20role-scoped?= =?UTF-8?q?=20validators=20read=20the=20authority=20=E2=80=94=20'Driver'?= =?UTF-8?q?=20no=20longer=20skips=20every=20driver=20rule=20(archreview=20?= =?UTF-8?q?01/C-9)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- ...3-04-one-role-authority-plan.md.tasks.json | 2 +- .../ConfigSourceOptionsValidator.cs | 28 ++--- .../MeshTransportOptionsValidator.cs | 33 +++--- .../SplitTopologyTransportValidator.cs | 52 +++------ .../TelemetryOptions.cs | 26 ++--- .../ConfigSourceOptionsValidatorTests.cs | 3 +- .../MeshTransportOptionsValidatorTests.cs | 2 +- .../RoleCaseVariantValidatorTests.cs | 107 ++++++++++++++++++ .../SplitTopologyTransportValidatorTests.cs | 4 +- .../TelemetryOptionsValidatorTests.cs | 3 +- 10 files changed, 172 insertions(+), 88 deletions(-) create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleCaseVariantValidatorTests.cs diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index 27da61a3..9ab49ce5 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -27,7 +27,7 @@ { "id": "T4", "subject": "All four role-scoped validators consume EffectiveRoles: SplitTopology/MeshTransport/Telemetry/ConfigSource (01/C-9)", - "status": "pending", + "status": "completed", "blockedBy": [ "T2" ] diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ConfigSourceOptionsValidator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ConfigSourceOptionsValidator.cs index 707a2a4e..165703c1 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ConfigSourceOptionsValidator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ConfigSourceOptionsValidator.cs @@ -15,18 +15,17 @@ namespace ZB.MOM.WW.OtOpcUa.Cluster; /// public sealed class ConfigSourceOptionsValidator : IValidateOptions { - private readonly IConfiguration _configuration; + private readonly EffectiveRoles _effectiveRoles; /// /// DI-constructed by AddValidatedOptions (a plain AddSingleton), so a - /// constructor dependency is safe here. — not - /// IClusterRoleInfo — is the source of this node's roles: IClusterRoleInfo's - /// implementation needs the ActorSystem, which does not exist yet at - /// ValidateOnStart time. + /// constructor dependency is safe here. This validator's only use of configuration is this + /// node's roles, so it takes — the single role authority + /// (archreview R3-04) — directly rather than . /// - public ConfigSourceOptionsValidator(IConfiguration configuration) + public ConfigSourceOptionsValidator(EffectiveRoles effectiveRoles) { - _configuration = configuration; + _effectiveRoles = effectiveRoles; } /// @@ -93,15 +92,12 @@ public sealed class ConfigSourceOptionsValidator : IValidateOptions()); - var isDriver = Array.IndexOf(roles, RoleParser.Driver) >= 0; - var isAdmin = Array.IndexOf(roles, RoleParser.Admin) >= 0; + // connection). EffectiveRoles is the single role authority (archreview R3-04): its precedence + // already applies the OTOPCUA_ROLES fallback (Gitea #532) and its normalization already folds + // case, so neither an env-only driver-only node nor one spelled 'Driver' can sail through here + // on Direct to read its configuration from nothing. + var isDriver = _effectiveRoles.HasDriver; + var isAdmin = _effectiveRoles.HasAdmin; if (isDriver && !isAdmin && isDirect) { diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/MeshTransportOptionsValidator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/MeshTransportOptionsValidator.cs index 65223102..d631c216 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/MeshTransportOptionsValidator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/MeshTransportOptionsValidator.cs @@ -17,19 +17,21 @@ namespace ZB.MOM.WW.OtOpcUa.Cluster; public sealed class MeshTransportOptionsValidator : IValidateOptions { private readonly IConfiguration _configuration; + private readonly EffectiveRoles _effectiveRoles; /// - /// DI-constructed by AddValidatedOptions (a plain AddSingleton), so a - /// constructor dependency is safe here. — not - /// IClusterRoleInfo — is the source of this node's roles: IClusterRoleInfo's - /// implementation needs the ActorSystem, which does not exist yet at - /// ValidateOnStart time. Same shape as and - /// . + /// DI-constructed by AddValidatedOptions (a plain AddSingleton), so constructor + /// dependencies are safe here. is still needed for the + /// ConfigServe:GrpcListenPort cross-read (the admin-side obligation below) — + /// is the single role authority (archreview R3-04) this rule reads + /// roles through instead of re-deriving them. /// - /// The application configuration, read for Cluster:Roles. - public MeshTransportOptionsValidator(IConfiguration configuration) + /// The application configuration, read for ConfigServe:GrpcListenPort. + /// This node's resolved effective roles. + public MeshTransportOptionsValidator(IConfiguration configuration, EffectiveRoles effectiveRoles) { _configuration = configuration; + _effectiveRoles = effectiveRoles; } /// @@ -68,15 +70,12 @@ public sealed class MeshTransportOptionsValidator : IValidateOptions()); - var isDriver = Array.IndexOf(roles, RoleParser.Driver) >= 0; - var isAdmin = Array.IndexOf(roles, RoleParser.Admin) >= 0; + // endpoint list of it would demand configuration it never reads. EffectiveRoles is the single + // role authority (archreview R3-04) — its precedence already applies the OTOPCUA_ROLES + // fallback and its normalization already folds case, so a node configured only through the env + // var, or spelled 'Driver', is still the driver that has to dial central. + var isDriver = _effectiveRoles.HasDriver; + var isAdmin = _effectiveRoles.HasAdmin; if (isGrpc && isDriver) { diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/SplitTopologyTransportValidator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/SplitTopologyTransportValidator.cs index 12007a77..4429786b 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/SplitTopologyTransportValidator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/SplitTopologyTransportValidator.cs @@ -36,58 +36,40 @@ namespace ZB.MOM.WW.OtOpcUa.Cluster; public sealed class SplitTopologyTransportValidator : IValidateOptions { private readonly IConfiguration _configuration; + private readonly EffectiveRoles _effectiveRoles; /// - /// DI-constructed by AddValidatedOptions (a plain AddSingleton), so a - /// constructor dependency is safe here. — not - /// IClusterRoleInfo — is the source of this node's roles and the three transport modes: - /// IClusterRoleInfo's implementation needs the ActorSystem, which does not exist - /// yet at ValidateOnStart time. Reading the modes from configuration too (rather than - /// from three injected IOptions) mirrors how the sibling validators cross-read - /// Cluster:Roles, and avoids a service-resolution ordering dependency at validation - /// time. + /// DI-constructed by AddValidatedOptions (a plain AddSingleton), so constructor + /// dependencies are safe here. is still needed for the three + /// transport-mode cross-reads (MeshTransport:Mode, Telemetry:Mode, + /// TelemetryDial:Mode) — not IClusterRoleInfo, whose implementation needs the + /// ActorSystem, which does not exist yet at ValidateOnStart time. + /// is the single role authority (archreview R3-04) — this rule no + /// longer re-derives the effective, normalized role list itself. /// - public SplitTopologyTransportValidator(IConfiguration configuration) + public SplitTopologyTransportValidator(IConfiguration configuration, EffectiveRoles effectiveRoles) { _configuration = configuration; + _effectiveRoles = effectiveRoles; } /// public ValidateOptionsResult Validate(string? name, MeshTransportOptions options) { // The options instance is intentionally unused: every value this rule depends on is cross-read - // from IConfiguration, the same way ConfigSourceOptionsValidator reads Cluster:Roles. - // - // Resolve the EFFECTIVE Akka roles exactly as the node does, or this "fail loud" rule fires on - // the wrong role view and passes silently on the two shapes it exists to catch: - // (a) Roles-source divergence. AkkaClusterOptions.Roles binds Cluster:Roles but falls back to - // OTOPCUA_ROLES when that is empty. A node configured with only - // OTOPCUA_ROLES=driver,cluster-SITE-A genuinely carries the cluster role in Akka, so it - // must be validated too. Until Gitea #532 this validator was the ONLY code that applied - // that fallback — it is now RoleParser.ResolveEffectiveRoles, shared with the - // AkkaClusterOptions PostConfigure that finally makes the option itself agree. - // (b) Case. The Cluster:Roles bind path is verbatim (no lowercasing), whereas RoleParser.Parse - // lowercases the OTOPCUA_ROLES path. Normalize BOTH here (trim + ToLowerInvariant) so - // 'Driver' / 'Admin' cannot slip past the ordinal driver / admin checks. (IsClusterRole - // itself is case-insensitive as of #532; the trim is still this rule's own business.) - var configured = _configuration.GetSection("Cluster:Roles").Get() ?? Array.Empty(); - var effective = RoleParser.ResolveEffectiveRoles(configured); - - var normalized = Array.ConvertAll(effective, r => r.Trim().ToLowerInvariant()); - - var clusterRoleIndex = Array.FindIndex(normalized, RoleParser.IsClusterRole); - if (clusterRoleIndex < 0) + // from IConfiguration (the three transport modes) or EffectiveRoles (this node's roles). + if (_effectiveRoles.ClusterRole is null) { // No cluster-{ClusterId} role → not the split topology → DPS is legitimate. This is the // exemption that keeps the validator a no-op for every legacy / single-mesh / test node. return ValidateOptionsResult.Success; } - // Classification is normalized (above); the ClusterId in the message is taken from the - // operator's original spelling so it names what they actually wrote. - var clusterId = RoleParser.ClusterIdFromRole(effective[clusterRoleIndex].Trim()); - var isDriver = Array.IndexOf(normalized, RoleParser.Driver) >= 0; - var isAdmin = Array.IndexOf(normalized, RoleParser.Admin) >= 0; + // ClusterId keeps the operator's original spelling (EffectiveRoles preserves it verbatim by + // design — see EffectiveRoles' remarks) so the message names what they actually wrote. + var clusterId = _effectiveRoles.ClusterId; + var isDriver = _effectiveRoles.HasDriver; + var isAdmin = _effectiveRoles.HasAdmin; var errors = new List(); diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs index d155d1cb..fef3c386 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs @@ -87,18 +87,17 @@ public sealed class TelemetryDialOptions /// public sealed class TelemetryOptionsValidator : IValidateOptions { - private readonly IConfiguration _configuration; + private readonly EffectiveRoles _effectiveRoles; /// /// DI-constructed by AddValidatedOptions (a plain AddSingleton), so a - /// constructor dependency is safe here. — not - /// IClusterRoleInfo — is the source of this node's roles: IClusterRoleInfo's - /// implementation needs the ActorSystem, which does not exist yet at - /// ValidateOnStart time. + /// constructor dependency is safe here. This validator's only use of configuration is this + /// node's roles, so it takes — the single role authority + /// (archreview R3-04) — directly rather than . /// - public TelemetryOptionsValidator(IConfiguration configuration) + public TelemetryOptionsValidator(EffectiveRoles effectiveRoles) { - _configuration = configuration; + _effectiveRoles = effectiveRoles; } /// @@ -118,12 +117,11 @@ public sealed class TelemetryOptionsValidator : IValidateOptions()); - var isDriver = Array.IndexOf(roles, RoleParser.Driver) >= 0; + // EffectiveRoles is the single role authority (archreview R3-04) — its precedence already + // applies the OTOPCUA_ROLES fallback (Gitea #532) and its normalization already folds case, so + // a node configured only through the env var, or spelled 'Driver', still satisfies the + // Grpc-mode requirements below. + var isDriver = _effectiveRoles.HasDriver; if (isGrpc) { @@ -137,7 +135,7 @@ public sealed class TelemetryOptionsValidator : IValidateOptions 0 && string.IsNullOrEmpty(options.ApiKey)) + if (_effectiveRoles.Gossiped.Count > 0 && string.IsNullOrEmpty(options.ApiKey)) { errors.Add( $"{TelemetryOptions.SectionName}:{nameof(TelemetryOptions.ApiKey)} is empty. Under " diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ConfigSourceOptionsValidatorTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ConfigSourceOptionsValidatorTests.cs index 27b97d2a..fdf54ebe 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ConfigSourceOptionsValidatorTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ConfigSourceOptionsValidatorTests.cs @@ -23,7 +23,8 @@ public class ConfigSourceOptionsValidatorTests var configuration = new ConfigurationBuilder().AddInMemoryCollection(pairs).Build(); - return new ConfigSourceOptionsValidator(configuration).Validate(ConfigSourceOptions.SectionName, o); + return new ConfigSourceOptionsValidator(EffectiveRoles.Resolve(configuration)) + .Validate(ConfigSourceOptions.SectionName, o); } private static ConfigSourceOptions ValidFetch() => new() diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/MeshTransportOptionsValidatorTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/MeshTransportOptionsValidatorTests.cs index 2291388f..e263761e 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/MeshTransportOptionsValidatorTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/MeshTransportOptionsValidatorTests.cs @@ -39,7 +39,7 @@ public class MeshTransportOptionsValidatorTests var configuration = new ConfigurationBuilder().AddInMemoryCollection(pairs).Build(); - return new MeshTransportOptionsValidator(configuration) + return new MeshTransportOptionsValidator(configuration, EffectiveRoles.Resolve(configuration)) .Validate(MeshTransportOptions.SectionName, o); } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleCaseVariantValidatorTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleCaseVariantValidatorTests.cs new file mode 100644 index 00000000..e2143710 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleCaseVariantValidatorTests.cs @@ -0,0 +1,107 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.Options; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; + +/// +/// R3-04 (01/C-9): a mixed-case role must fire the SAME fail-fast rule the canonical spelling +/// fires, on ALL FOUR role-scoped validators. Before the fix, three of the four +/// (, , +/// ) compared roles ordinally and silently PASSED a +/// misconfigured node spelled Driver/Admin — the load-bearing assertion in every +/// case below is the FAILURE, not the pass, because a validator whose driver/admin branch never +/// runs also "passes" vacuously. +/// +/// +/// already normalized before this fix (its own +/// comment names the hazard) — its case here is the BUILT-IN falsification control: it must have +/// been passing (i.e. correctly failing the misconfigured node) even before T4 landed, proving +/// the Theory shape itself is capable of catching a real regression. +/// +[Collection(RoleEnvironmentCollection.Name)] +public sealed class RoleCaseVariantValidatorTests +{ + public static readonly TheoryData DriverSpellings = new() + { + "driver", "Driver", " driver ", "DRIVER", + }; + + private static IConfiguration Config(params (string Key, string Value)[] pairs) => + new ConfigurationBuilder() + .AddInMemoryCollection(pairs.ToDictionary(p => p.Key, p => (string?)p.Value)) + .Build(); + + // ---- SplitTopologyTransportValidator: the built-in control (already normalized pre-fix) ------- + + [Theory] + [MemberData(nameof(DriverSpellings))] + public void SplitTopology_fails_on_Dps_mesh_transport_for_every_driver_spelling(string driverSpelling) + { + var configuration = Config( + ("Cluster:Roles:0", driverSpelling), + ("Cluster:Roles:1", "cluster-SITE-A"), + ("MeshTransport:Mode", MeshTransportOptions.ModeDps), + ("Telemetry:Mode", TelemetryOptions.ModeGrpc)); + + var result = new SplitTopologyTransportValidator(configuration, EffectiveRoles.Resolve(configuration)) + .Validate(MeshTransportOptions.SectionName, new MeshTransportOptions()); + + result.Failed.ShouldBeTrue(); + result.FailureMessage.ShouldContain("MeshTransport:Mode"); + } + + // ---- MeshTransportOptionsValidator: GrpcPsk required for a driver node under Grpc mode -------- + + [Theory] + [MemberData(nameof(DriverSpellings))] + public void MeshTransport_Grpc_driver_node_without_a_psk_fails_for_every_driver_spelling(string driverSpelling) + { + var configuration = Config(("Cluster:Roles:0", driverSpelling), ("Cluster:Roles:1", "cluster-SITE-A")); + var options = new MeshTransportOptions + { + Mode = MeshTransportOptions.ModeGrpc, + GrpcPsk = null, + CentralGrpcEndpoints = ["http://central-1:4055"], + }; + + var result = new MeshTransportOptionsValidator(configuration, EffectiveRoles.Resolve(configuration)) + .Validate(MeshTransportOptions.SectionName, options); + + result.Failed.ShouldBeTrue(); + result.FailureMessage.ShouldContain("MeshTransport:GrpcPsk"); + } + + // ---- ConfigSourceOptionsValidator: a driver-only node on Direct fails --------------------------- + + [Theory] + [MemberData(nameof(DriverSpellings))] + public void ConfigSource_driver_only_node_on_Direct_fails_for_every_driver_spelling(string driverSpelling) + { + var configuration = Config(("Cluster:Roles:0", driverSpelling)); + var options = new ConfigSourceOptions { Mode = ConfigSourceOptions.ModeDirect }; + + var result = new ConfigSourceOptionsValidator(EffectiveRoles.Resolve(configuration)) + .Validate(ConfigSourceOptions.SectionName, options); + + result.Failed.ShouldBeTrue(); + result.FailureMessage.ShouldContain(ConfigSourceOptions.ModeFetchAndCache); + } + + // ---- TelemetryOptionsValidator: a driver node under Grpc mode needs a listen port --------------- + + [Theory] + [MemberData(nameof(DriverSpellings))] + public void Telemetry_Grpc_driver_node_with_no_listen_port_fails_for_every_driver_spelling(string driverSpelling) + { + var configuration = Config(("Cluster:Roles:0", driverSpelling)); + var options = new TelemetryOptions { Mode = TelemetryOptions.ModeGrpc, ApiKey = "k" }; + + var result = new TelemetryOptionsValidator(EffectiveRoles.Resolve(configuration)) + .Validate(TelemetryOptions.SectionName, options); + + result.Failed.ShouldBeTrue(); + result.FailureMessage.ShouldContain(nameof(TelemetryOptions.GrpcListenPort)); + } +} diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/SplitTopologyTransportValidatorTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/SplitTopologyTransportValidatorTests.cs index f11faf74..4d1d45c3 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/SplitTopologyTransportValidatorTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/SplitTopologyTransportValidatorTests.cs @@ -35,7 +35,7 @@ public class SplitTopologyTransportValidatorTests // The validated options instance is never read (all four values come from IConfiguration), so a // default MeshTransportOptions is a fine stand-in. - return new SplitTopologyTransportValidator(configuration) + return new SplitTopologyTransportValidator(configuration, EffectiveRoles.Resolve(configuration)) .Validate(MeshTransportOptions.SectionName, new MeshTransportOptions()); } @@ -246,7 +246,7 @@ public class SplitTopologyTransportValidatorTests var pairs = new Dictionary { ["MeshTransport:Mode"] = MeshTransportOptions.ModeDps }; var configuration = new ConfigurationBuilder().AddInMemoryCollection(pairs).Build(); - var result = new SplitTopologyTransportValidator(configuration) + var result = new SplitTopologyTransportValidator(configuration, EffectiveRoles.Resolve(configuration)) .Validate(MeshTransportOptions.SectionName, new MeshTransportOptions()); result.Failed.ShouldBeTrue(); diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/TelemetryOptionsValidatorTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/TelemetryOptionsValidatorTests.cs index d4bb9e14..d2f96e59 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/TelemetryOptionsValidatorTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/TelemetryOptionsValidatorTests.cs @@ -23,7 +23,8 @@ public class TelemetryOptionsValidatorTests var configuration = new ConfigurationBuilder().AddInMemoryCollection(pairs).Build(); - return new TelemetryOptionsValidator(configuration).Validate(TelemetryOptions.SectionName, o); + return new TelemetryOptionsValidator(EffectiveRoles.Resolve(configuration)) + .Validate(TelemetryOptions.SectionName, o); } [Fact] From e46c1db5e5135ef0abac4c2554f4f522c336b5fe Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:04:16 -0400 Subject: [PATCH 05/15] =?UTF-8?q?fix(cluster):=20gossiped-role=20predicate?= =?UTF-8?q?s=20compare=20case-insensitively=20=E2=80=94=20a=20miscased=20p?= =?UTF-8?q?eer=20no=20longer=20un-elects=20the=20Primary?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../plans/R3-04-one-role-authority-plan.md | 46 ++++ ...3-04-one-role-authority-plan.md.tasks.json | 2 +- .../ClusterRoleInfo.cs | 9 +- .../ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs | 11 + .../Components/Pages/Fleet.razor | 5 +- .../ClusterNodeAddressReconcilerActor.cs | 6 +- .../Redundancy/ManualFailoverService.cs | 5 +- .../Redundancy/RedundancyStateActor.cs | 10 +- .../Drivers/DriverHostActor.cs | 7 +- .../ScriptedAlarms/ScriptedAlarmHostActor.cs | 6 +- .../GossipedRoleMatchTests.cs | 54 +++++ .../GossipedRoleCaseInsensitivityTests.cs | 220 ++++++++++++++++++ 12 files changed, 370 insertions(+), 11 deletions(-) create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/GossipedRoleMatchTests.cs create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/GossipedRoleCaseInsensitivityTests.cs diff --git a/archreview/plans/R3-04-one-role-authority-plan.md b/archreview/plans/R3-04-one-role-authority-plan.md index be92c942..b809adcf 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md +++ b/archreview/plans/R3-04-one-role-authority-plan.md @@ -456,3 +456,49 @@ T11, T12 (rig config) before T13; T13 last, after everything is merge-ready ``` Overall effort: **~1 dev-day code+unit, plus the rig session.** The three HIGH-RISK slices (T1 identity type, T3 composition root, T8 the guard) each land behind their full project suites before anything stacks on them; T13 is merge-blocking and is only meaningful because T11/T12 gave the rig a node that can actually exhibit the defect. + +--- + +## Execution notes (recorded during T1–T12 implementation, 2026-08-20) + +**T5 — `SelectOldestUpMemberOfRole`/`SelectDriverPrimary` cannot be reached from this repo; the plan +was wrong about this one call site.** `RedundancyStateActor.SelectOldestUpMemberOfRole` +(`RedundancyStateActor.cs:172-178`) is a thin wrapper around `ClusterActiveNode.OldestUpMember` +(`ZB.MOM.WW.Health.Akka`, a separately-versioned NuGet package out of `scadaproj`), which itself +matches via **Akka's own `Member.HasRole`** — an ordinal comparison this repo does not own. This is +structurally identical to the "not ours to change" carve-out the plan's own choice 2 already makes +for `ClusterSingletonOptions.Role` (rows 18/19); the plan simply did not notice the same constraint +applies to `SelectOldestUpMemberOfRole` before naming it as a `GossipedRolesHave` adoption site. + +Consequence: the plan's stated T5 test — "a `RedundancyStateActor` selection case where the member +gossips 'Driver' and a Primary IS elected" — is **not achievable**. What shipped instead +(`tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/GossipedRoleCaseInsensitivityTests.cs`): + +- `GossipedRolesHave` was implemented and adopted at every site that genuinely reads gossiped roles + directly in this repo: `RedundancyStateActor.cs` (`BuildSnapshot`'s own role classification, NOT + the Primary-selection call), `ManualFailoverService.cs` (`OrderedDrivers`), + `ClusterNodeAddressReconcilerActor.cs` (`FilterOwnClusterDriverMembers`), `DriverHostActor.cs` and + `ScriptedAlarmHostActor.cs` (`DriverMemberCount`), and `Fleet.razor` (the bare `"driver"` literals, + now `RoleParser.Driver` — the actual case-insensitivity there comes from `ClusterRoleInfo`'s + `_roleLeaders`/`_membersByRole` dictionaries moving to `OrdinalIgnoreCase`, which + `IClusterRoleInfo.MembersWithRole`/`RoleLeader` read through). +- `A_member_gossiping_a_miscased_driver_role_classifies_Secondary_not_Detached` pins the fix that + **is** achievable: a single member gossiping `"Driver"` now classifies `Secondary` in the + redundancy snapshot (was `Detached` pre-fix) — real value, because `/hosts` and `/fleet` render + this list, even though that member can still never be *named* Primary by itself. +- `Primary_election_itself_remains_ordinal_and_still_elects_no_one_for_an_all_miscased_pair` pins the + **true** (negative) outcome instead of asserting a false positive: a node (or pair) gossiping only + `"Driver"` still elects no Primary, because that decision runs through the vendored ordinal match. + This is a genuine residual — **not closed by R3-04** — worth a follow-up issue against + `ZB.MOM.WW.Health.Akka`'s `ClusterActiveNode.OldestUpMember` (e.g. an optional case-insensitive + role-match parameter) if the rolling-upgrade window this defends against is judged worth closing + fully. In practice T1/T2 already close the *headline* outage the census describes for a + FIXED-build node (a node can no longer gossip a miscased plain role itself — normalization happens + before the gossip handoff), so this residual only bites a peer still running a pre-R3-04 build. +- `ManualFailoverService_recognises_a_miscased_driver_node_as_a_candidate` uses + `GetSnapshot()`/`PrimaryAddress` rather than `FailOverCore(dryRun: true)` as originally sketched — + `FailOverCore` refuses below 2 driver members ("nothing to fail over TO"), which a single-node + fixture can never satisfy; `GetSnapshot()` shares the same `OrderedDrivers` candidate selection + this test is actually about, without needing a second live node. + +No other T1–T5 deviations from the plan. diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index 9ab49ce5..09c0cfdd 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -35,7 +35,7 @@ { "id": "T5", "subject": "RoleParser.GossipedRolesHave adopted at the six ordinal gossip predicates + ClusterRoleInfo index (Primary election, R2-04 member count, /fleet)", - "status": "pending", + "status": "completed", "blockedBy": [ "T1" ] diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs index 49949497..74151f43 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs @@ -23,8 +23,13 @@ public sealed class ClusterRoleInfo : IClusterRoleInfo, IDisposable private readonly string? _clusterRole; private readonly string? _clusterId; private readonly object _lock = new(); - private readonly Dictionary _roleLeaders = new(StringComparer.Ordinal); - private readonly Dictionary> _membersByRole = new(StringComparer.Ordinal); + // R3-04 (01/C-9, census row 17): OrdinalIgnoreCase — Akka compares gossiped role names ordinally, + // so a PEER running an older build can still gossip a miscased plain role during a rolling + // upgrade. Keying these by the exact-cased role string would leave MembersWithRole("driver") and + // RoleLeader("driver") blind to a peer that gossips "Driver", the same class of gap #532 closed + // for _localRoles above. + private readonly Dictionary _roleLeaders = new(StringComparer.OrdinalIgnoreCase); + private readonly Dictionary> _membersByRole = new(StringComparer.OrdinalIgnoreCase); private IActorRef? _subscriber; /// Initializes a new instance of the ClusterRoleInfo class. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs index f9d58a6b..71cee807 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs @@ -153,4 +153,15 @@ public static class RoleParser return roles; } + + /// True when a GOSSIPED role list carries , compared + /// case-insensitively. Akka compares role names ordinally and a PEER may be running an older + /// build that gossips a verbatim 'Driver' (archreview 01/C-9), so every predicate over + /// Member.Roles goes through here rather than List.Contains. + /// The gossiped role list (e.g. Member.Roles). + /// The role to test for. + /// True when is present, case-insensitively. + public static bool GossipedRolesHave(IEnumerable? gossipedRoles, string role) => + gossipedRoles is not null + && gossipedRoles.Any(r => string.Equals(r, role, StringComparison.OrdinalIgnoreCase)); } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Fleet.razor b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Fleet.razor index dbdf4435..a9053fcb 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Fleet.razor +++ b/src/Server/ZB.MOM.WW.OtOpcUa.AdminUI/Components/Pages/Fleet.razor @@ -6,6 +6,7 @@ @attribute [Authorize(Policy = AdminUiPolicies.AuthenticatedRead)] @rendermode RenderMode.InteractiveServer @using Microsoft.EntityFrameworkCore +@using ZB.MOM.WW.OtOpcUa.Cluster @using ZB.MOM.WW.OtOpcUa.Commons.Interfaces @using ZB.MOM.WW.OtOpcUa.Configuration @using ZB.MOM.WW.OtOpcUa.Configuration.Entities @@ -146,7 +147,7 @@ else // Union with current Akka driver members so a freshly-joined node that has no // NodeDeploymentState row yet still appears as "waiting". - var akkaDrivers = Cluster.MembersWithRole("driver") + var akkaDrivers = Cluster.MembersWithRole(RoleParser.Driver) .Select(n => n.Value).ToHashSet(StringComparer.OrdinalIgnoreCase); var allNodes = byNode.Keys.Union(akkaDrivers, StringComparer.OrdinalIgnoreCase) .OrderBy(s => s, StringComparer.OrdinalIgnoreCase).ToList(); @@ -156,7 +157,7 @@ else byNode.TryGetValue(nodeId, out var state); return new NodeRow( NodeId: nodeId, - Roles: akkaDrivers.Contains(nodeId) ? new[] { "driver" } : Array.Empty(), + Roles: akkaDrivers.Contains(nodeId) ? new[] { RoleParser.Driver } : Array.Empty(), Status: state?.Status, StartedAtUtc: state?.StartedAtUtc, AppliedAtUtc: state?.AppliedAtUtc, diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Fleet/ClusterNodeAddressReconcilerActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Fleet/ClusterNodeAddressReconcilerActor.cs index 61ebbc3d..080684cf 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Fleet/ClusterNodeAddressReconcilerActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Fleet/ClusterNodeAddressReconcilerActor.cs @@ -181,10 +181,12 @@ public sealed class ClusterNodeAddressReconcilerActor : ReceiveActor, IWithTimer public static IReadOnlyList<(string Host, int Port)> FilterOwnClusterDriverMembers( IEnumerable members, string? ownClusterRole) { + // R3-04 (01/C-9): GossipedRolesHave, not ordinal Contains — a peer gossiping a miscased + // 'Driver' (rolling-upgrade window) must not read as a phantom RunningNodeHasNoRow finding. var driverMembers = members - .Where(m => m.Status == MemberStatus.Up && m.Roles.Contains(DriverRole)); + .Where(m => m.Status == MemberStatus.Up && RoleParser.GossipedRolesHave(m.Roles, DriverRole)); if (!string.IsNullOrEmpty(ownClusterRole)) - driverMembers = driverMembers.Where(m => m.Roles.Contains(ownClusterRole)); + driverMembers = driverMembers.Where(m => RoleParser.GossipedRolesHave(m.Roles, ownClusterRole)); return driverMembers .Select(m => (Host: m.Address.Host ?? string.Empty, Port: m.Address.Port ?? 0)) .Where(a => !string.IsNullOrWhiteSpace(a.Host) && a.Port > 0) diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/ManualFailoverService.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/ManualFailoverService.cs index ddedd053..e5bc3a62 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/ManualFailoverService.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/ManualFailoverService.cs @@ -3,6 +3,7 @@ using Akka.Actor; using Akka.Cluster; using Microsoft.Extensions.Logging; using ZB.MOM.WW.Audit; +using ZB.MOM.WW.OtOpcUa.Cluster; namespace ZB.MOM.WW.OtOpcUa.ControlPlane.Redundancy; @@ -122,7 +123,9 @@ public sealed class ManualFailoverService : IManualFailoverService } private static List OrderedDrivers(IEnumerable members) => members - .Where(m => m.Status == MemberStatus.Up && m.Roles.Contains(RedundancyStateActor.DriverRole)) + // R3-04 (01/C-9): GossipedRolesHave, not ordinal Contains — a peer gossiping a miscased + // 'Driver' (rolling-upgrade window) must still be a manual-failover candidate. + .Where(m => m.Status == MemberStatus.Up && RoleParser.GossipedRolesHave(m.Roles, RedundancyStateActor.DriverRole)) .OrderBy(m => m, Member.AgeOrdering) .ToList(); } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/RedundancyStateActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/RedundancyStateActor.cs index 2c43f34a..61e47ee0 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/RedundancyStateActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/Redundancy/RedundancyStateActor.cs @@ -190,7 +190,15 @@ public sealed class RedundancyStateActor : ReceiveActor, IWithTimers if (string.IsNullOrWhiteSpace(host)) continue; var isPrimary = driverPrimary is not null && driverPrimary == member.Address; - var role = member.Roles.Contains(DriverRole) + // R3-04 (01/C-9): GossipedRolesHave, not ordinal Contains — a peer running an older build + // can still gossip a miscased 'Driver' during a rolling upgrade, and this list is what + // /hosts and /fleet render, so a miscased-but-genuinely-driver member must not read as + // Detached. NOTE: this does NOT reach SelectDriverPrimary/driverPrimary above — that + // delegates to ClusterActiveNode.OldestUpMember (ZB.MOM.WW.Health.Akka), which matches via + // Akka's own ordinal Member.HasRole and is not this repo's to change (see R3-04 execution + // notes). So a member gossiping only 'Driver' now classifies Secondary rather than + // Detached, but a PAIR that BOTH gossip 'Driver' still elects no Primary. + var role = RoleParser.GossipedRolesHave(member.Roles, DriverRole) ? (isPrimary ? CommonsRedundancyRole.Primary : CommonsRedundancyRole.Secondary) : CommonsRedundancyRole.Detached; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs index e95ca092..4890d0bc 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverHostActor.cs @@ -5,6 +5,7 @@ using Akka.Event; using Microsoft.EntityFrameworkCore; using Microsoft.Extensions.Logging; using Microsoft.Extensions.Logging.Abstractions; +using ZB.MOM.WW.OtOpcUa.Cluster; using ZB.MOM.WW.OtOpcUa.Commons.Engines; using ZB.MOM.WW.OtOpcUa.Commons.Interfaces; using ZB.MOM.WW.OtOpcUa.Commons.Messages.Admin; @@ -1608,8 +1609,12 @@ public sealed class DriverHostActor : ReceiveActor, IWithTimers, IWithUnboundedS if (_driverMemberCountProvider is not null) return _driverMemberCountProvider(); try { + // R3-04 (01/C-9): GossipedRolesHave, not ordinal Contains — a peer gossiping a miscased + // 'Driver' (rolling-upgrade window) must still count toward the R2-04 primary-gate member + // count, or a genuine 2-node pair silently takes the single-node allow branch. return Akka.Cluster.Cluster.Get(Context.System).State.Members - .Count(m => m.Status == Akka.Cluster.MemberStatus.Up && m.Roles.Contains(ServiceCollectionExtensions.DriverRole)); + .Count(m => m.Status == Akka.Cluster.MemberStatus.Up + && RoleParser.GossipedRolesHave(m.Roles, ServiceCollectionExtensions.DriverRole)); } catch (Exception) { diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs index 3ec6e08c..bc7bfc1b 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/ScriptedAlarms/ScriptedAlarmHostActor.cs @@ -1,6 +1,7 @@ using Akka.Actor; using Akka.Cluster.Tools.PublishSubscribe; using Akka.Event; +using ZB.MOM.WW.OtOpcUa.Cluster; using ZB.MOM.WW.OtOpcUa.Commons.Messages.Alerts; using ZB.MOM.WW.OtOpcUa.Commons.Messages.Redundancy; using ZB.MOM.WW.OtOpcUa.Commons.Observability; @@ -516,8 +517,11 @@ public sealed class ScriptedAlarmHostActor : ReceiveActor if (_driverMemberCountProvider is not null) return _driverMemberCountProvider(); try { + // R3-04 (01/C-9): GossipedRolesHave, not ordinal Contains — a peer gossiping a miscased + // 'Driver' (rolling-upgrade window) must still count toward the alerts-emit gate. return Akka.Cluster.Cluster.Get(Context.System).State.Members - .Count(m => m.Status == Akka.Cluster.MemberStatus.Up && m.Roles.Contains(ServiceCollectionExtensions.DriverRole)); + .Count(m => m.Status == Akka.Cluster.MemberStatus.Up + && RoleParser.GossipedRolesHave(m.Roles, ServiceCollectionExtensions.DriverRole)); } catch (Exception) { diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/GossipedRoleMatchTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/GossipedRoleMatchTests.cs new file mode 100644 index 00000000..47852a07 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/GossipedRoleMatchTests.cs @@ -0,0 +1,54 @@ +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; + +/// +/// R3-04 (01/C-9, choice 6): is defence in depth for a +/// PEER running an older build that still gossips a miscased plain role — a locally-authored node +/// can no longer gossip one at all once (T1) is wired in, but Akka +/// compares gossiped role names ordinally, so a rolling-upgrade window can still show a +/// mixed-case peer. +/// +public sealed class GossipedRoleMatchTests +{ + [Fact] + public void Matches_the_canonical_spelling() + { + RoleParser.GossipedRolesHave(["driver"], "driver").ShouldBeTrue(); + } + + [Theory] + [InlineData("Driver")] + [InlineData("DRIVER")] + [InlineData("driVer")] + public void Matches_a_miscased_gossiped_role(string gossiped) + { + RoleParser.GossipedRolesHave([gossiped], "driver").ShouldBeTrue(); + } + + [Fact] + public void Matches_a_miscased_cluster_role() + { + RoleParser.GossipedRolesHave(["cluster-SITE-A"], "cluster-site-a").ShouldBeTrue(); + RoleParser.GossipedRolesHave(["Cluster-Site-A"], "cluster-SITE-A").ShouldBeTrue(); + } + + [Fact] + public void Does_not_match_a_different_role() + { + RoleParser.GossipedRolesHave(["admin"], "driver").ShouldBeFalse(); + } + + [Fact] + public void Null_gossiped_roles_is_false() + { + RoleParser.GossipedRolesHave(null, "driver").ShouldBeFalse(); + } + + [Fact] + public void Empty_gossiped_roles_is_false() + { + RoleParser.GossipedRolesHave([], "driver").ShouldBeFalse(); + } +} diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/GossipedRoleCaseInsensitivityTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/GossipedRoleCaseInsensitivityTests.cs new file mode 100644 index 00000000..3cc6c3c9 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.ControlPlane.Tests/GossipedRoleCaseInsensitivityTests.cs @@ -0,0 +1,220 @@ +using System.Collections.Immutable; +using System.Reflection; +using Akka.Actor; +using Akka.Cluster; +using Akka.Configuration; +using Akka.Util; +using Microsoft.Extensions.Logging.Abstractions; +using Shouldly; +using Xunit; +using ZB.MOM.WW.Audit; +using ZB.MOM.WW.OtOpcUa.Cluster; +using ZB.MOM.WW.OtOpcUa.Commons.Messages.Redundancy; +using ZB.MOM.WW.OtOpcUa.ControlPlane.Fleet; +using ZB.MOM.WW.OtOpcUa.ControlPlane.Redundancy; + +namespace ZB.MOM.WW.OtOpcUa.ControlPlane.Tests; + +/// +/// R3-04 (01/C-9, census rows 17/20-24/30): a PEER gossiping a miscased plain role (a +/// rolling-upgrade-window peer running an older build — (T1) already +/// means a node on the FIXED build cannot gossip a miscased role itself) must not read as a +/// stranger to the six production predicates this plan names. +/// +/// +/// +/// Deviation from the plan's stated T5 test (recorded per the execution brief): the +/// plan asked for "a RedundancyStateActor selection case where the member gossips +/// 'Driver' and a Primary IS elected". That is not achievable from this repo. +/// / +/// delegate to +/// ZB.MOM.WW.Health.Akka's ClusterActiveNode.OldestUpMember, which matches via +/// Akka's OWN Member.HasRole — an ordinal comparison this repo does not own, the exact +/// same class of "not ours to change" the plan itself already carves out for +/// ClusterSingletonOptions.Role (rows 18/19). +/// below pins that TRUE (negative) outcome instead of asserting a false positive. +/// +/// +/// What R3-04 DOES fix and this file proves: every consumer that reads gossiped roles +/// DIRECTLY (not through that external oldest-member helper) is now case-insensitive, so an +/// individual miscased member is no longer invisible to failover candidacy, address +/// reconciliation, or the redundancy snapshot's own role classification. +/// +/// +public sealed class GossipedRoleCaseInsensitivityTests +{ + // ---- Pure-unit: ClusterNodeAddressReconcilerActor.FilterOwnClusterDriverMembers --------------- + // (public static over IEnumerable — no live cluster needed) + + private static int _memberUid; + + // Akka's Member.Create factories are internal — same reflection idiom as + // ClusterNodeAddressReconcilerTests.UpMember. + private static readonly MethodInfo MemberCreate = typeof(Member).GetMethod( + "Create", + BindingFlags.Static | BindingFlags.NonPublic | BindingFlags.Public, + binder: null, + types: + [ + typeof(UniqueAddress), typeof(int), typeof(MemberStatus), + typeof(ImmutableHashSet), typeof(AppVersion), + ], + modifiers: null) + ?? throw new InvalidOperationException("Akka.Cluster.Member.Create(5-arg) not found — Akka API changed"); + + private static Member UpMember(string host, int port, params string[] roles) => + (Member)MemberCreate.Invoke(null, + [ + new UniqueAddress(new Address("akka.tcp", "otopcua", host, port), Interlocked.Increment(ref _memberUid)), + 1, + MemberStatus.Up, + roles.ToImmutableHashSet(), + AppVersion.Zero, + ])!; + + [Fact] + public void FilterOwnClusterDriverMembers_recognises_a_miscased_driver_role() + { + var members = new[] { UpMember("site-a-1", 4053, "Driver", "cluster-SITE-A") }; + + var result = ClusterNodeAddressReconcilerActor.FilterOwnClusterDriverMembers(members, "cluster-SITE-A"); + + result.ShouldHaveSingleItem(); + } + + [Fact] + public void FilterOwnClusterDriverMembers_recognises_a_miscased_cluster_role() + { + var members = new[] { UpMember("site-a-1", 4053, "driver", "Cluster-SITE-A") }; + + // The caller's OWN cluster role names the canonical spelling; the member gossips a different + // casing ("Cluster-SITE-A") — GossipedRolesHave must still match the two as the same role. + var result = ClusterNodeAddressReconcilerActor.FilterOwnClusterDriverMembers(members, "cluster-site-a"); + + result.ShouldHaveSingleItem(); + } + + // ---- Live single-node cluster: BuildSnapshot classification + ManualFailoverService ----------- + + private const int Port = 19_560; + + private static Config NodeConfig(int port) => ConfigurationFactory.ParseString($$""" + akka { + loglevel = "WARNING" + actor.provider = "Akka.Cluster.ClusterActorRefProvider, Akka.Cluster" + remote.dot-netty.tcp { + hostname = "127.0.0.1" + port = {{port}} + } + cluster { + seed-nodes = ["akka.tcp://gossip-case-test@127.0.0.1:{{port}}"] + roles = ["Driver"] + min-nr-of-members = 1 + run-coordinated-shutdown-when-down = off + downing-provider-class = "" + } + } + """); + + private static async Task StartSingleNodeAsync() + { + var system = ActorSystem.Create("gossip-case-test", NodeConfig(Port)); + var cluster = Akka.Cluster.Cluster.Get(system); + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(30); + while (DateTime.UtcNow < deadline) + { + if (cluster.State.Members.Any(m => m.Status == MemberStatus.Up)) return system; + await Task.Delay(100); + } + + await system.Terminate(); + throw new TimeoutException("node never came Up"); + } + + [Fact] + public async Task A_member_gossiping_a_miscased_driver_role_classifies_Secondary_not_Detached() + { + // Pins the fix that IS achievable: BuildSnapshot's own role classification (member.Roles + // Contains -> GossipedRolesHave) no longer reads a genuinely-driver-but-miscased member as + // Detached, even though (see class remarks) it can never be named Primary by itself. + var system = await StartSingleNodeAsync(); + try + { + var tcs = new TaskCompletionSource( + TaskCreationOptions.RunContinuationsAsynchronously); + system.ActorOf( + RedundancyStateActor.Props(broadcast: msg => + { + if (msg is RedundancyStateChanged changed) tcs.TrySetResult(changed); + }), + $"redundancy-{Guid.NewGuid():N}"); + + var snapshot = await tcs.Task.WaitAsync(TimeSpan.FromSeconds(15)); + + var node = snapshot.Nodes.ShouldHaveSingleItem(); + node.Role.ShouldBe( + RedundancyRole.Secondary, + "gossiping 'Driver' must not read as Detached — it is a real driver member, just " + + "miscased; BEFORE R3-04 this was Detached (ordinal Contains(\"driver\") on \"Driver\")"); + } + finally + { + await system.Terminate(); + } + } + + /// + /// THE recorded deviation: proves the residual gap is real, not assumed. A pair (here, + /// trivially, a single node) that gossips ONLY 'Driver' still elects no Primary — because that + /// decision runs through Akka's own ordinal Member.HasRole inside the vendored + /// ClusterActiveNode.OldestUpMember, which this repo cannot patch. Tracked as a + /// follow-up against ZB.MOM.WW.Health.Akka, not closed by R3-04. + /// + [Fact] + public async Task Primary_election_itself_remains_ordinal_and_still_elects_no_one_for_an_all_miscased_pair() + { + var system = await StartSingleNodeAsync(); + try + { + var members = Akka.Cluster.Cluster.Get(system).State.Members; + + RedundancyStateActor.SelectOldestUpMemberOfRole(members, "driver").ShouldBeNull( + "known residual (R3-04 execution notes): SelectOldestUpMemberOfRole delegates to " + + "ClusterActiveNode.OldestUpMember -> Akka's own ordinal Member.HasRole, which this " + + "repo does not own — GossipedRolesHave cannot reach inside it"); + } + finally + { + await system.Terminate(); + } + } + + [Fact] + public async Task ManualFailoverService_recognises_a_miscased_driver_node_as_a_candidate() + { + // FailOverCore refuses below 2 driver members (nothing to fail over TO), so this single-node + // fixture exercises GetSnapshot()'s PrimaryAddress instead — which shares the same + // OrderedDrivers candidate selection this test is actually about. + var system = await StartSingleNodeAsync(); + try + { + var service = new ManualFailoverService( + () => system, new RecordingAuditWriter(), NullLogger.Instance); + + var snapshot = service.GetSnapshot(); + + snapshot.PrimaryAddress.ShouldNotBeNull(); + snapshot.PrimaryAddress!.ShouldContain(Port.ToString()); + } + finally + { + await system.Terminate(); + } + } + + private sealed class RecordingAuditWriter : IAuditWriter + { + public Task WriteAsync(AuditEvent auditEvent, CancellationToken cancellationToken = default) => + Task.CompletedTask; + } +} From 3314a1f98d2a6a804261c4d50f643b927c3809a3 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:08:03 -0400 Subject: [PATCH 06/15] feat(host): warn when Cluster:Roles and OTOPCUA_ROLES disagree; boot banner names the winning source --- ...3-04-one-role-authority-plan.md.tasks.json | 2 +- .../EffectiveRoles.cs | 56 ++++++++++++++----- src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs | 7 +++ .../EffectiveRolesTests.cs | 41 ++++++++++++++ 4 files changed, 92 insertions(+), 14 deletions(-) diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index 09c0cfdd..319a4707 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -43,7 +43,7 @@ { "id": "T6", "subject": "Warning when Cluster:Roles and OTOPCUA_ROLES disagree; boot banner names the winning source", - "status": "pending", + "status": "completed", "blockedBy": [ "T3" ] diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/EffectiveRoles.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/EffectiveRoles.cs index c06dc4c0..ef2d4d88 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/EffectiveRoles.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/EffectiveRoles.cs @@ -38,10 +38,11 @@ public enum RoleSource /// public sealed class EffectiveRoles { - private EffectiveRoles(IReadOnlyList gossiped, RoleSource source) + private EffectiveRoles(IReadOnlyList gossiped, RoleSource source, string? disagreementWarning) { Gossiped = gossiped; Source = source; + DisagreementWarning = disagreementWarning; ClusterRole = RoleParser.ClusterRoleFromRoles(gossiped); ClusterId = ClusterRole is null ? null : RoleParser.ClusterIdFromRole(ClusterRole); } @@ -79,6 +80,13 @@ public sealed class EffectiveRoles /// for a legacy / single-mesh node. public string? ClusterId { get; } + /// Non-null when BOTH Cluster:Roles and OTOPCUA_ROLES were set and their + /// normalized role sets differ — the Cluster:Roles-wins case an operator almost certainly + /// did not intend (choice 5). Observability only: still resolves from + /// Cluster:Roles exactly as usual. Returned rather than logged from this static factory — + /// the caller (Program.cs) logs it once Serilog is configured. + public string? DisagreementWarning { get; } + /// The one factory: resolves the effective roles off 's /// Cluster:Roles and the process's OTOPCUA_ROLES environment variable. /// The application configuration. @@ -101,20 +109,42 @@ public sealed class EffectiveRoles /// Either source names an unknown role. public static EffectiveRoles Resolve(IReadOnlyList? configured, string? envRoles) { - if (configured is { Count: > 0 }) - { - return new EffectiveRoles(NormalizeAndValidateConfigured(configured), RoleSource.Configured); - } + var configuredNormalized = configured is { Count: > 0 } + ? NormalizeAndValidateConfigured(configured) + : null; - if (!string.IsNullOrWhiteSpace(envRoles)) - { - // RoleParser.Parse already trims, lower-cases (fully — including a cluster suffix, which - // is intentionally different from the configured path; see ClusterIdFromRole's remarks), - // dedupes and validates. - return new EffectiveRoles(RoleParser.Parse(envRoles), RoleSource.Environment); - } + // RoleParser.Parse already trims, lower-cases (fully — including a cluster suffix, which is + // intentionally different from the configured path; see ClusterIdFromRole's remarks), dedupes + // and validates. + var envNormalized = !string.IsNullOrWhiteSpace(envRoles) + ? RoleParser.Parse(envRoles) + : null; - return new EffectiveRoles(Array.Empty(), RoleSource.None); + var warning = BuildDisagreementWarning(configuredNormalized, envNormalized); + + if (configuredNormalized is not null) + return new EffectiveRoles(configuredNormalized, RoleSource.Configured, warning); + + if (envNormalized is not null) + return new EffectiveRoles(envNormalized, RoleSource.Environment, warning); + + return new EffectiveRoles(Array.Empty(), RoleSource.None, null); + } + + /// Choice 5: a Warning (not an error) when both sources are set and their normalized + /// role sets differ — this cannot fire when only one source is set, so it is always in that case regardless of the other source's absence. + private static string? BuildDisagreementWarning(string[]? configuredNormalized, string[]? envNormalized) + { + if (configuredNormalized is null || envNormalized is null) return null; + + var configuredSet = new HashSet(configuredNormalized, StringComparer.OrdinalIgnoreCase); + var envSet = new HashSet(envNormalized, StringComparer.OrdinalIgnoreCase); + if (configuredSet.SetEquals(envSet)) return null; + + return $"{ConfiguredRolesKey} ([{string.Join(",", configuredNormalized)}]) and " + + $"{RoleParser.RolesEnvVar} ([{string.Join(",", envNormalized)}]) disagree; " + + $"{ConfiguredRolesKey} wins outright."; } /// Normalizes and validates the Cluster:Roles entries: a plain role is trimmed and diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs index 5567a2c0..fa194b98 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs @@ -796,6 +796,13 @@ Log.Information( "OtOpcUa.Host starting with roles=[{Roles}] (admin={HasAdmin}, driver={HasDriver}) source={Source}", string.Join(",", roles), hasAdmin, hasDriver, effectiveRoles.Source); +// R3-04 choice 5: Cluster:Roles and OTOPCUA_ROLES set to different normalized role sets is almost +// certainly not what the operator intended (Cluster:Roles still wins outright — this is +// observability, not a behaviour change). Logged here rather than inside the static factory because +// Serilog is not configured yet when EffectiveRoles.Resolve runs at the top of this file. +if (effectiveRoles.DisagreementWarning is not null) + Log.Warning("{Warning}", effectiveRoles.DisagreementWarning); + // #561 — startup growth self-check: the banner above went through the live pipeline; verify each // configured file sink physically grew. A sink whose writes silently fail (the wonder-app-vd03 // 0-byte-log incident) is reported at boot — through SelfLog + stderr, which don't depend on the diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EffectiveRolesTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EffectiveRolesTests.cs index d7dce4ca..3adbc1dc 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EffectiveRolesTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EffectiveRolesTests.cs @@ -210,4 +210,45 @@ public sealed class EffectiveRolesTests roles.Gossiped.ShouldBe(new[] { "admin", "driver" }); } } + + // ---- DisagreementWarning (choice 5, T6) ---------------------------------------------------- + + [Fact] + public void Both_sources_set_and_agreeing_has_no_warning() + { + var roles = EffectiveRoles.Resolve(["driver"], "driver"); + + roles.DisagreementWarning.ShouldBeNull(); + } + + [Fact] + public void Only_one_source_set_has_no_warning() + { + EffectiveRoles.Resolve(["driver"], null).DisagreementWarning.ShouldBeNull(); + EffectiveRoles.Resolve(null, "driver").DisagreementWarning.ShouldBeNull(); + EffectiveRoles.Resolve(null, null).DisagreementWarning.ShouldBeNull(); + } + + [Fact] + public void Both_sources_set_and_disagreeing_produces_a_warning_naming_both_keys() + { + var roles = EffectiveRoles.Resolve(["admin"], "driver,cluster-SITE-A"); + + roles.DisagreementWarning.ShouldNotBeNull(); + roles.DisagreementWarning.ShouldContain("Cluster:Roles"); + roles.DisagreementWarning.ShouldContain(RoleParser.RolesEnvVar); + // Configured still wins outright — the warning is observability, not a behaviour change. + roles.Gossiped.ShouldBe(new[] { "admin" }); + roles.Source.ShouldBe(RoleSource.Configured); + } + + [Fact] + public void Both_sources_set_but_only_differing_by_case_do_not_warn() + { + // Same normalized set spelled differently in each source is NOT the "Cluster:Roles wins + // unintentionally" shape the warning exists for. + var roles = EffectiveRoles.Resolve(["Driver"], "driver"); + + roles.DisagreementWarning.ShouldBeNull(); + } } From 4380e2e78a5051c9f929035294251e5c49156eb9 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:11:25 -0400 Subject: [PATCH 07/15] fix(cluster): fail start when a late config layer changes Cluster:Roles (role overlays may not set it) --- ...3-04-one-role-authority-plan.md.tasks.json | 2 +- .../AkkaClusterOptionsValidator.cs | 42 ++++++++++++++++ .../AkkaClusterOptionsValidatorTests.cs | 49 +++++++++++++++++++ 3 files changed, 92 insertions(+), 1 deletion(-) diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index 319a4707..b1fb0428 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -51,7 +51,7 @@ { "id": "T7", "subject": "AkkaClusterOptionsValidator fails start when a late config layer changes Cluster:Roles (role overlays may not set it)", - "status": "pending", + "status": "completed", "blockedBy": [ "T2" ] diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptionsValidator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptionsValidator.cs index 461befa6..ce683d76 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptionsValidator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptionsValidator.cs @@ -36,12 +36,28 @@ namespace ZB.MOM.WW.OtOpcUa.Cluster; /// public sealed class AkkaClusterOptionsValidator : OptionsValidatorBase { + private readonly EffectiveRoles? _effectiveRoles; + + /// + /// 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. + /// + /// The registered role authority, or null to skip the post-overlay guard. + public AkkaClusterOptionsValidator(EffectiveRoles? effectiveRoles = null) + { + _effectiveRoles = effectiveRoles; + } + /// protected override void Validate(ValidationBuilder builder, AkkaClusterOptions options) { ArgumentNullException.ThrowIfNull(options); ValidateBootstrapGuard(builder, options.BootstrapGuard); + ValidateAgainstEffectiveRoles(builder, options); var seeds = options.SeedNodes ?? Array.Empty(); if (seeds.Length == 0) @@ -89,6 +105,32 @@ public sealed class AkkaClusterOptionsValidator : OptionsValidatorBase 0 when the guard is enabled (was {guard.ProbeConnectTimeoutMs})."); } + /// + /// R3-04 (choice 4, the residual divergence surface): Program.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. + /// + private void ValidateAgainstEffectiveRoles(ValidationBuilder builder, AkkaClusterOptions options) + { + if (_effectiveRoles is null) return; + + var bound = new HashSet(options.Roles ?? Array.Empty(), 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 " + + "overlay may not set Cluster:Roles."); + } + /// /// True when addresses this node itself — host AND port. /// 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 8114898c..91140bf4 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsValidatorTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsValidatorTests.cs @@ -214,4 +214,53 @@ public sealed class AkkaClusterOptionsValidatorTests result.Failed.ShouldBeFalse(result.FailureMessage); } + + // ---- R3-04 choice 4: the post-overlay config-layer guard -------------------------------------- + // A role overlay (or a ${secret:} expansion) may not itself set Cluster:Roles — Program.cs + // 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. + + [Fact] + public void A_config_layer_that_changes_Cluster_Roles_after_resolution_fails_naming_both_views() + { + 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 result = new AkkaClusterOptionsValidator(effectiveRoles).Validate(null, options); + + result.Failed.ShouldBeTrue(); + result.FailureMessage.ShouldContain("Cluster:Roles"); + result.FailureMessage.ShouldContain("driver"); + result.FailureMessage.ShouldContain("admin"); + } + + [Fact] + public void Bound_Roles_matching_the_registered_EffectiveRoles_passes() + { + 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 result = new AkkaClusterOptionsValidator(effectiveRoles).Validate(null, options); + + result.Failed.ShouldBeFalse(result.FailureMessage); + } + + /// + /// A test that never injects — the pre-T7 construction shape — + /// must keep passing. Without this, giving the ctor a required parameter would silently break + /// every other test in this file rather than a chosen few. + /// + [Fact] + public void No_EffectiveRoles_injected_skips_the_guard_entirely() + { + var options = CentralNode("site-a-1", Seed("site-a-1")); + options.Roles = new[] { "whatever-this-does-not-validate-roles-either" }; + + var result = new AkkaClusterOptionsValidator().Validate(null, options); + + result.Failed.ShouldBeFalse(result.FailureMessage); + } } From e2497c908dd9357bcfb8bec22cfdaa878198ac72 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:21:36 -0400 Subject: [PATCH 08/15] test(cluster): guard that role reads live only in the authority (archreview theme 4) --- ...3-04-one-role-authority-plan.md.tasks.json | 2 +- .../ServiceCollectionExtensions.cs | 4 +- .../Drivers/DriverInstanceActor.cs | 5 +- .../RoleAuthorityGuardTests.cs | 250 ++++++++++++++++++ 4 files changed, 258 insertions(+), 3 deletions(-) create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleAuthorityGuardTests.cs diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index b1fb0428..86cb2627 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -59,7 +59,7 @@ { "id": "T8", "subject": "RoleAuthorityGuardTests: source scan proving no production file outside the authority reads OTOPCUA_ROLES, Cluster:Roles, or a bare role literal", - "status": "pending", + "status": "completed", "blockedBy": [ "T3", "T4", diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs index 897e6869..21245437 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs @@ -21,7 +21,9 @@ namespace ZB.MOM.WW.OtOpcUa.ControlPlane; public static class ServiceCollectionExtensions { - public const string AdminRole = "admin"; + // R3-04 (theme 4 guard): alias RoleParser.Admin rather than duplicating the literal — the same + // pattern Runtime/ServiceCollectionExtensions.DriverRole already follows for RoleParser.Driver. + public const string AdminRole = RoleParser.Admin; public const string ConfigPublishSingletonName = "config-publish"; public const string AdminOperationsSingletonName = "admin-operations"; diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs index 7c8f5b3d..3100aa9e 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Runtime/Drivers/DriverInstanceActor.cs @@ -1,5 +1,6 @@ using Akka.Actor; using Akka.Event; +using ZB.MOM.WW.OtOpcUa.Cluster; using ZB.MOM.WW.OtOpcUa.Commons.Observability; using ZB.MOM.WW.OtOpcUa.Commons.OpcUa; using ZB.MOM.WW.OtOpcUa.Commons.Types; @@ -310,7 +311,9 @@ public sealed class DriverInstanceActor : ReceiveActor, IWithTimers { var isWindowsOnly = driverType is "Galaxy"; if (!OperatingSystem.IsWindows() && isWindowsOnly) return true; - if (roles.Contains("dev") && isWindowsOnly) return true; + // R3-04 (theme 4 guard): GossipedRolesHave, not a bare "dev" literal Contains — the same + // case-insensitivity the rest of the role authority gives every other consumer. + if (RoleParser.GossipedRolesHave(roles, RoleParser.Dev) && isWindowsOnly) return true; return false; } diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleAuthorityGuardTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleAuthorityGuardTests.cs new file mode 100644 index 00000000..3bb85907 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleAuthorityGuardTests.cs @@ -0,0 +1,250 @@ +using System.Text.RegularExpressions; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; + +/// +/// R3-04 (theme 4, "one rule stated twice diverges"): the role-reading rule may live in exactly +/// one place. Three source-scan rules, modelled on +/// CommandDisposalConventionsTests's root-walk idiom, over every src/**/*.cs + +/// src/**/*.razor file: +/// +/// No file except EffectiveRoles.cs/RoleParser.cs reads +/// OTOPCUA_ROLES. +/// No file except EffectiveRoles.cs reads the Cluster:Roles +/// key as code (an operator-facing message string that merely NAMES the key is fine — it +/// reads as a sentence, i.e. its string literal also contains a space). +/// No file except RoleParser.cs declares a bare +/// "admin"/"driver"/"dev" role literal. +/// +/// A textual scan, not reflection, because the thing being guarded is a textual read +/// (Environment.GetEnvironmentVariable("OTOPCUA_ROLES"), GetSection("Cluster:Roles"), +/// a bare "driver") that reflection cannot see. +/// +/// +/// Falsification control: +/// feeds the scan the EXACT pre-fix Program.cs/Fleet.razor snippets (captured +/// verbatim before T3/T5 touched them) and asserts it finds them — proof this guard can fail, not +/// just that the current tree happens to pass. Run against the real pre-T3 tree at commit +/// 7dfd6dcc (Program.cs) / pre-T5 at bcec88ef (Fleet.razor), the live scan reported: +/// Host/Program.cs:61, :62, :63 (OTOPCUA_ROLES + two bare role literals) and +/// AdminUI/Components/Pages/Fleet.razor:149, :159 (two bare "driver" literals) +/// — recorded here per the plan's "paste the list into the commit body" instruction. +/// +public sealed class RoleAuthorityGuardTests +{ + private static readonly string[] OtopcuaRolesAllowedFiles = ["EffectiveRoles.cs", "RoleParser.cs"]; + private static readonly string[] ClusterRolesKeyAllowedFiles = ["EffectiveRoles.cs"]; + private static readonly string[] RoleLiteralAllowedFiles = ["RoleParser.cs"]; + + /// + /// Files whose bare "admin"/"driver"/"dev" literal names something OTHER + /// than an Akka Cluster:Roles role — each entry carries the reason, per the plan's + /// "add the file to the small, named allow-list WITH A REASON" instruction. + /// + private static readonly IReadOnlyDictionary RoleLiteralAllowListWithReasons = + new Dictionary(StringComparer.OrdinalIgnoreCase) + { + ["RawTreeService.cs"] = + "raw-tree node-KIND discriminator (Folder/Driver/Device/TagGroup/Tag), not an Akka role", + ["AclScopeService.cs"] = + "human-readable entity-type name passed to ResolveAsync for an error message, not a role", + ["SqlTagDefinitionFactory.cs"] = + "the Sql driver's own TagConfig JSON blob field ({\"driver\":\"Sql\",...}), not a role", + ["SqlEquipmentTagParser.cs"] = + "the Sql driver's own TagConfig JSON blob field ({\"driver\":\"Sql\",...}), not a role", + ["SqlAddressPickerBody.razor"] = + "the Sql driver's own TagConfig JSON blob field ({\"driver\":\"Sql\",...}), not a role", + ["OtOpcUaLdapAuthService.cs"] = + "an LDAP-derived ADMINUI RBAC role (the DevStubMode bypass group), not Cluster:Roles", + }; + + private static string SolutionRoot() + { + var dir = new DirectoryInfo(AppContext.BaseDirectory); + while (dir is not null && !File.Exists(Path.Combine(dir.FullName, "ZB.MOM.WW.OtOpcUa.slnx"))) + dir = dir.Parent; + dir.ShouldNotBeNull("Could not find solution root (ZB.MOM.WW.OtOpcUa.slnx)."); + return dir!.FullName; + } + + private static IEnumerable SourceFiles() + { + var srcDir = Path.Combine(SolutionRoot(), "src"); + var sep = Path.DirectorySeparatorChar; + return Directory.EnumerateFiles(srcDir, "*.cs", SearchOption.AllDirectories) + .Concat(Directory.EnumerateFiles(srcDir, "*.razor", SearchOption.AllDirectories)) + .Where(f => !f.Contains($"{sep}obj{sep}", StringComparison.Ordinal) + && !f.Contains($"{sep}bin{sep}", StringComparison.Ordinal)); + } + + /// Every line of that is not a full-line ////// + /// comment, 1-based line numbers. + private static IEnumerable<(int Line, string Text)> NonCommentLines(string text) + { + var lines = text.Split('\n'); + for (var i = 0; i < lines.Length; i++) + { + if (lines[i].TrimStart().StartsWith("//", StringComparison.Ordinal)) continue; + yield return (i + 1, lines[i]); + } + } + + private static readonly Regex BareRoleLiteral = new("\"(admin|driver|dev)\"", RegexOptions.Compiled); + + /// True when the occurrence of a -character key at + /// sits inside a string literal that ALSO contains a space — the + /// shape an operator-facing message reads as (naming the key for guidance is not a CODE READ of + /// it), per the plan's carve-out for Cluster:Roles — applied identically to + /// OTOPCUA_ROLES for the same reason (GrpcApplyAckSender's warning names BOTH keys + /// on one line; treating the two inconsistently would flag one and not the other on that exact + /// sentence). + private static bool IsAllowedMessageOccurrence(string line, int matchIndex, int matchLength) + { + var openQuote = line.LastIndexOf('"', matchIndex); + if (openQuote < 0) return false; // not inside any string literal at all -> a bare code read + + var searchFrom = matchIndex + matchLength; + if (searchFrom >= line.Length) return false; + var closeQuote = line.IndexOf('"', searchFrom); + if (closeQuote < 0) return false; + + return line[openQuote..(closeQuote + 1)].Contains(' '); + } + + private static List FindOtopcuaRolesOffenders(string text) => + NonCommentLines(text) + .Where(l => + { + var idx = l.Text.IndexOf("OTOPCUA_ROLES", StringComparison.Ordinal); + return idx >= 0 && !IsAllowedMessageOccurrence(l.Text, idx, "OTOPCUA_ROLES".Length); + }) + .Select(l => l.Line.ToString()) + .ToList(); + + private static List FindClusterRolesKeyOffenders(string text) => + NonCommentLines(text) + .Where(l => + { + var idx = l.Text.IndexOf("Cluster:Roles", StringComparison.Ordinal); + return idx >= 0 && !IsAllowedMessageOccurrence(l.Text, idx, "Cluster:Roles".Length); + }) + .Select(l => l.Line.ToString()) + .ToList(); + + private static List FindBareRoleLiteralOffenders(string text) => + NonCommentLines(text) + .Where(l => BareRoleLiteral.IsMatch(l.Text)) + .Select(l => l.Line.ToString()) + .ToList(); + + [Fact] + public void No_file_outside_the_authority_reads_OTOPCUA_ROLES() + { + var offenders = new List(); + foreach (var file in SourceFiles()) + { + if (OtopcuaRolesAllowedFiles.Contains(Path.GetFileName(file))) continue; + + foreach (var line in FindOtopcuaRolesOffenders(File.ReadAllText(file))) + offenders.Add($"{RelativePath(file)}:{line}"); + } + + offenders.ShouldBeEmpty( + "OTOPCUA_ROLES must be read only by EffectiveRoles.cs/RoleParser.cs. Offenders:\n" + + string.Join("\n", offenders)); + } + + [Fact] + public void No_file_outside_EffectiveRoles_reads_the_ClusterRoles_key_as_code() + { + var offenders = new List(); + foreach (var file in SourceFiles()) + { + if (ClusterRolesKeyAllowedFiles.Contains(Path.GetFileName(file))) continue; + + foreach (var line in FindClusterRolesKeyOffenders(File.ReadAllText(file))) + offenders.Add($"{RelativePath(file)}:{line}"); + } + + offenders.ShouldBeEmpty( + "Cluster:Roles must be read as code only by EffectiveRoles.cs (naming the key inside an " + + "operator-facing message string is fine). Offenders:\n" + string.Join("\n", offenders)); + } + + [Fact] + public void No_file_outside_RoleParser_declares_a_bare_role_literal() + { + var offenders = new List(); + foreach (var file in SourceFiles()) + { + var fileName = Path.GetFileName(file); + if (RoleLiteralAllowedFiles.Contains(fileName)) continue; + if (RoleLiteralAllowListWithReasons.ContainsKey(fileName)) continue; + + foreach (var line in FindBareRoleLiteralOffenders(File.ReadAllText(file))) + offenders.Add($"{RelativePath(file)}:{line}"); + } + + offenders.ShouldBeEmpty( + "A bare \"admin\"/\"driver\"/\"dev\" role literal must live only in RoleParser.cs (use " + + "RoleParser.Admin/.Driver/.Dev, or add the file to RoleLiteralAllowListWithReasons WITH " + + "A REASON if it genuinely names something else). Offenders:\n" + string.Join("\n", offenders)); + } + + /// The allow-list itself must not silently grow stale — every reason must be non-blank, + /// and the file it names must actually exist and actually contain the literal it is excused for + /// (an allow-list entry that no longer applies is dead and hides nothing). + [Fact] + public void Every_allow_list_entry_has_a_reason_and_still_applies() + { + foreach (var (fileName, reason) in RoleLiteralAllowListWithReasons) + { + reason.ShouldNotBeNullOrWhiteSpace($"{fileName}'s allow-list entry has no reason."); + + var matches = SourceFiles().Where(f => Path.GetFileName(f) == fileName).ToList(); + matches.ShouldNotBeEmpty($"{fileName} is allow-listed but no such file exists under src/."); + + var stillOffends = matches.Any(f => FindBareRoleLiteralOffenders(File.ReadAllText(f)).Count > 0); + stillOffends.ShouldBeTrue( + $"{fileName} is allow-listed for a bare role literal but no longer contains one — " + + "remove the stale entry."); + } + } + + /// + /// Falsification control (per the plan: "Run on the pre-T3 tree → FAIL... paste that list + /// into the commit body"). These are the EXACT pre-fix lines — copied verbatim, not + /// paraphrased — from Host/Program.cs (before T3, commit 7dfd6dcc) and + /// AdminUI/Components/Pages/Fleet.razor (before T5, commit bcec88ef). If this + /// test ever goes green for the wrong reason (the scan silently stops matching), it is no + /// longer proof the guard can fail. + /// + [Fact] + public void The_scan_actually_detects_the_pre_R3_04_offenders() + { + const string preT3ProgramCs = """ + // Roles drive the entire conditional wiring below — see ZB.MOM.WW.OtOpcUa.Cluster.RoleParser. + var roles = RoleParser.Parse(Environment.GetEnvironmentVariable("OTOPCUA_ROLES")); + var hasAdmin = roles.Contains("admin"); + var hasDriver = roles.Contains("driver"); + """; + + const string preT5FleetRazor = """ + var akkaDrivers = Cluster.MembersWithRole("driver") + .Select(n => n.Value).ToHashSet(StringComparer.OrdinalIgnoreCase); + """; + + FindOtopcuaRolesOffenders(preT3ProgramCs).ShouldNotBeEmpty( + "the falsification control failed to reproduce the pre-T3 OTOPCUA_ROLES violation"); + FindBareRoleLiteralOffenders(preT3ProgramCs).Count.ShouldBeGreaterThanOrEqualTo(2, + "the falsification control failed to reproduce BOTH pre-T3 bare-literal violations " + + "('admin' and 'driver')"); + FindBareRoleLiteralOffenders(preT5FleetRazor).ShouldNotBeEmpty( + "the falsification control failed to reproduce the pre-T5 Fleet.razor bare-literal violation"); + } + + private static string RelativePath(string fullPath) => + Path.GetRelativePath(SolutionRoot(), fullPath).Replace('\\', '/'); +} From 659d5f51e4245c10f2a5458a47ae0f131c889e0e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:26:34 -0400 Subject: [PATCH 09/15] test(integration): a Cluster:Roles-only node wires the driver stack --- ...3-04-one-role-authority-plan.md.tasks.json | 2 +- .../ClusterRolesOnlyBootTests.cs | 188 ++++++++++++++++++ 2 files changed, 189 insertions(+), 1 deletion(-) create mode 100644 tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/ClusterRolesOnlyBootTests.cs diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index 86cb2627..76724e15 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -69,7 +69,7 @@ { "id": "T9", "subject": "ClusterRolesOnlyBootTests: a Cluster:Roles-only node wires the driver stack (with a pre-fix negative control)", - "status": "pending", + "status": "completed", "blockedBy": [ "T3" ] diff --git a/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/ClusterRolesOnlyBootTests.cs b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/ClusterRolesOnlyBootTests.cs new file mode 100644 index 00000000..acff9d25 --- /dev/null +++ b/tests/Server/ZB.MOM.WW.OtOpcUa.Host.IntegrationTests/ClusterRolesOnlyBootTests.cs @@ -0,0 +1,188 @@ +using Microsoft.AspNetCore.Builder; +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Logging; +using Microsoft.Extensions.Options; +using Shouldly; +using Xunit; +using ZB.MOM.WW.LocalDb; +using ZB.MOM.WW.OtOpcUa.Cluster; +using ZB.MOM.WW.OtOpcUa.Core.Authorization; +using ZB.MOM.WW.OtOpcUa.Host.Configuration; +using ZB.MOM.WW.OtOpcUa.Host.OpcUa; +using ZB.MOM.WW.OtOpcUa.OpcUaServer.Security; +using ZB.MOM.WW.OtOpcUa.Security; + +namespace ZB.MOM.WW.OtOpcUa.Host.IntegrationTests; + +/// +/// R3-04 (01/S-12 = 03/C5): the exact shape the finding describes — a node configured ONLY +/// through Cluster:Roles (OTOPCUA_ROLES unset) — must wire the driver stack, not +/// the "no roles at all" host view a pre-fix Program.cs computed. +/// +/// +/// +/// Follows 's direct-host-build pattern +/// deliberately: WebApplicationFactory<Program> is not driven here because +/// starting the real host triggers the Akka cluster join and needs SQL Server (see +/// TwoNodeClusterHarness). What is under test is DI resolution off the SAME +/// composition Program.cs runs, so a container is built and resolved without starting it. +/// +/// +/// Unlike , hasAdmin/hasDriver are +/// NOT passed in as bool parameters here — they are derived from +/// exactly as Program.cs (T3) +/// does, off a configuration carrying ONLY Cluster:Roles. The role DERIVATION is +/// therefore genuinely under test, not just the conditional wiring downstream of it. +/// +/// +public sealed class ClusterRolesOnlyBootTests : IDisposable +{ + private readonly List _apps = []; + private readonly List _dbPaths = []; + + /// + /// Builds the slice of Program.cs's conditional wiring this finding is about: the + /// LocalDb registration (:230, inside if (hasDriver)) and the + /// IOpcUaUserAuthenticator selection (:414-454), driven by + /// — the T3 (fixed) or pre-fix derivation, selected by the + /// caller so the two behaviours are provably different rather than assumed to be. + /// + private (IServiceProvider Services, bool HasAdmin, bool HasDriver) BuildGraph( + Dictionary configuration, + Func deriveRoles) + { + var dbPath = Path.Combine(Path.GetTempPath(), $"otopcua-r304-boot-{Guid.NewGuid():N}.db"); + _dbPaths.Add(dbPath); + configuration["LocalDb:Path"] = dbPath; + + var builder = WebApplication.CreateBuilder(new WebApplicationOptions { Args = [] }); + builder.Configuration.AddInMemoryCollection(configuration); + builder.Services.AddLogging(); + + var (hasAdmin, hasDriver) = deriveRoles(builder.Configuration); + + builder.Services.AddOtOpcUaCluster(builder.Configuration); + + // Program.cs:230 — driver-role only. + if (hasDriver) + builder.Services.AddOtOpcUaLocalDb(builder.Configuration); + + // Program.cs:414-454 — the IOpcUaUserAuthenticator selection block, reproduced verbatim + // (the hasDriver-only leg only — hasAdmin needs a ConfigDb this test does not configure). + if (hasDriver) + { + builder.Services.AddSingleton(); + builder.Services.AddSingleton( + sp => sp.GetRequiredService()); + builder.Services.AddSingleton(); + builder.Services.AddSingleton(); + } + else + { + builder.Services.AddSingleton(NullOpcUaUserAuthenticator.Instance); + } + builder.Services.Configure(o => o.Pepper = "test-pepper"); + + var app = builder.Build(); + _apps.Add(app); + return (app.Services, hasAdmin, hasDriver); + } + + /// T3's fix: resolve through the single role authority. + private static (bool HasAdmin, bool HasDriver) FixedDerivation(IConfiguration configuration) + { + var roles = EffectiveRoles.Resolve(configuration); + return (roles.HasAdmin, roles.HasDriver); + } + + /// + /// The pre-T3 derivation — Program.cs:61-63 before this plan, verbatim: reads + /// OTOPCUA_ROLES only, never Cluster:Roles. Takes + /// purely to match 's signature; it does not use it. + /// + private static (bool HasAdmin, bool HasDriver) PreFixDerivation(IConfiguration _) + { + var roles = RoleParser.Parse(Environment.GetEnvironmentVariable("OTOPCUA_ROLES")); + return (roles.Contains("admin"), roles.Contains("driver")); + } + + [Fact] + public void A_ClusterRolesOnly_node_wires_the_driver_stack() + { + var previous = Environment.GetEnvironmentVariable("OTOPCUA_ROLES"); + Environment.SetEnvironmentVariable("OTOPCUA_ROLES", null); + try + { + var (services, hasAdmin, hasDriver) = BuildGraph( + new Dictionary { ["Cluster:Roles:0"] = "driver" }, + FixedDerivation); + + hasAdmin.ShouldBeFalse(); + hasDriver.ShouldBeTrue(); + + services.GetService().ShouldNotBeNull( + "a Cluster:Roles-only driver node must have LocalDb wired"); + services.GetRequiredService().ShouldBeOfType( + "a Cluster:Roles-only driver node must authenticate api keys, not deny everything"); + + var akkaOptions = services.GetRequiredService>().Value; + akkaOptions.Roles.ShouldBe(new[] { "driver" }); + } + finally + { + Environment.SetEnvironmentVariable("OTOPCUA_ROLES", previous); + } + } + + /// + /// THE negative control the plan requires: the SAME Cluster:Roles-only configuration, + /// run through the derivation Program.cs used BEFORE T3, must reproduce exactly the outage + /// 01/S-12 describes — no LocalDb, and every UserName token denied. Proves the fixed test + /// above is asserting a real behaviour change, not a tautology. + /// + [Fact] + public void The_same_configuration_through_the_preFix_derivation_gets_no_driver_stack_at_all() + { + var previous = Environment.GetEnvironmentVariable("OTOPCUA_ROLES"); + Environment.SetEnvironmentVariable("OTOPCUA_ROLES", null); + try + { + var (services, hasAdmin, hasDriver) = BuildGraph( + new Dictionary { ["Cluster:Roles:0"] = "driver" }, + PreFixDerivation); + + hasAdmin.ShouldBeFalse(); + hasDriver.ShouldBeFalse( + "the pre-fix derivation reads OTOPCUA_ROLES ONLY — Cluster:Roles=[\"driver\"] must be invisible to it"); + + services.GetService().ShouldBeNull( + "the pre-fix host view has no roles at all, so LocalDb is never registered"); + services.GetRequiredService().ShouldBeOfType( + "the pre-fix host view denies every UserName token — the exact outage 01/S-12 describes"); + + // The Akka side is unaffected by which derivation Program.cs used — AddOtOpcUaCluster (T2) + // always resolves Cluster:Roles itself, so even the pre-fix HOST view's blindness left the + // AKKA gossip correct. This is the two-authority split the finding is named for: Akka said + // "driver", Program.cs's conditional wiring said nothing. + var akkaOptions = services.GetRequiredService>().Value; + akkaOptions.Roles.ShouldBe(new[] { "driver" }); + } + finally + { + Environment.SetEnvironmentVariable("OTOPCUA_ROLES", previous); + } + } + + public void Dispose() + { + foreach (var app in _apps) + ((IDisposable)app).Dispose(); + + foreach (var path in _dbPaths) + { + try { if (File.Exists(path)) File.Delete(path); } + catch { /* best-effort cleanup */ } + } + } +} From b8d2d500060d9ada2bb2345d7423fd30b663c814 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:29:37 -0400 Subject: [PATCH 10/15] =?UTF-8?q?docs:=20one=20role=20authority=20?= =?UTF-8?q?=E2=80=94=20precedence,=20normalization,=20and=20the=20overlay?= =?UTF-8?q?=20invariant=20(R3-04)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- CLAUDE.md | 44 +++++++++++++++++++ ...3-04-one-role-authority-plan.md.tasks.json | 2 +- docs/Configuration.md | 12 ++--- docs/Redundancy.md | 7 ++- docs/ServiceHosting.md | 12 +++-- docs/v2/Cluster.md | 4 +- scripts/install/Install-Services.ps1 | 9 +++- 7 files changed, 75 insertions(+), 15 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index ff22131d..7f3a3e85 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -435,6 +435,50 @@ Assert the **effective** cluster config off a running `ActorSystem` (as `SplitBr See `docs/Redundancy.md` §"Per-cluster meshes (Phase 6)", `docs/Configuration.md` §"Per-cluster mesh split (Phase 6)", and `docs/plans/2026-07-24-mesh-phase6-partition-and-colocation.md`. +## Role authority (`EffectiveRoles`) + +**A node's roles are resolved in exactly one place** (archreview R3-04, closing 01/S-12 = 03/C5 and +01/C-9): `EffectiveRoles.Resolve` (`ZB.MOM.WW.OtOpcUa.Cluster`), with one precedence — +`Cluster:Roles` when non-empty, else `OTOPCUA_ROLES`, else empty — and two-layer normalization: a +plain role (`admin`/`driver`/`dev`) is trimmed and lower-cased, but a cluster role's ClusterId suffix +(`cluster-SITE-A`) is kept verbatim, because lower-casing it would rename the Akka role of a running +deployment. An unknown role from **either** source fails host start. `Program.cs` resolves one +`EffectiveRoles` instance at the top of the composition root and registers it as a singleton; that +SAME instance drives both the conditional host wiring (`hasAdmin`/`hasDriver`) and +`AkkaClusterOptions.Roles` (what Akka gossips) — there is no second derivation for the two to +disagree about, which is why no runtime "do the views agree" assert exists. Before this, `Program.cs` +read `OTOPCUA_ROLES` only, so a node configured solely through `Cluster:Roles` gossiped the right +Akka roles while `Program.cs`'s own wiring mounted none of them — the inverse of the pre-#532 field +incident (`docs/deployments/wonder-app-vd03-makino-z-34184.md`). + +- **A role overlay may not itself set `Cluster:Roles`.** Roles are resolved *before* the per-role + appsettings overlay loads (the overlay's filename depends on the answer), so a later layer changing + `Cluster:Roles` would leave the bound options disagreeing with the registered instance. + `AkkaClusterOptionsValidator` fails host start if this ever happens, naming both views — a no-op + today since none of the four shipped overlays carries a `Cluster` section. +- **Both sources set to different normalized role sets logs a Warning** at boot (`Cluster:Roles` + still wins outright) — the boot banner also names which source won (`source=Configured` / + `Environment` / `None`). +- **`RoleParser.GossipedRolesHave`** (case-insensitive) is defence in depth at the handful of + production sites that read LIVE GOSSIP directly (`Member.Roles`) rather than through + `EffectiveRoles` — a peer running an older, pre-R3-04 build can still gossip a miscased plain role + during a rolling upgrade. ⚠️ **Known residual, not closed by R3-04:** + `RedundancyStateActor.SelectOldestUpMemberOfRole`/`SelectDriverPrimary` delegate to + `ZB.MOM.WW.Health.Akka`'s `ClusterActiveNode.OldestUpMember`, which matches via Akka's OWN ordinal + `Member.HasRole` — not this repo's to patch, the same class of "not ours to change" carve-out as + `ClusterSingletonOptions.Role`. A pair where every member is on a fixed build can no longer gossip + a miscased role at all (normalization happens before gossip), so this only bites a genuinely mixed + fixed/pre-fix rolling-upgrade window. +- **`RoleAuthorityGuardTests`** (`tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/`) is the enforcement: + a source scan failing on any production file outside `EffectiveRoles.cs`/`RoleParser.cs` that reads + `OTOPCUA_ROLES`, `Cluster:Roles` as code, or declares a bare `"admin"`/`"driver"`/`"dev"` literal — + with a small, reasoned allow-list for the handful of genuinely unrelated domain uses of those same + English words (a raw-tree node-kind label, a Sql driver's own JSON marker, an LDAP-derived AdminUI + RBAC role). + +See `docs/Configuration.md` §`Roles`, `docs/ServiceHosting.md` §"Role gating", and +`archreview/plans/R3-04-one-role-authority-plan.md`. + ## Mesh command transport (`MeshTransport`) **The Akka `ClusterClient` transport described in this section's earlier history is DELETED** diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index 76724e15..7022a35c 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -77,7 +77,7 @@ { "id": "T10", "subject": "Docs sweep: Configuration.md roles section (rewrite the two-knobs note), ServiceHosting.md, Redundancy.md, v2/Cluster.md, install script comment, CLAUDE.md", - "status": "pending", + "status": "completed", "blockedBy": [] }, { diff --git a/docs/Configuration.md b/docs/Configuration.md index 2bccc843..29bee90d 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -26,7 +26,7 @@ The Host (`Program.cs`) loads `appsettings.json`, then overlays a **per-role** f - Both roles → `appsettings.admin-driver.json` (roles joined with `-`, ordinal-sorted). - `appsettings.{ASPNETCORE_ENVIRONMENT}.json` (e.g. `appsettings.Development.json`) is layered on by the host builder. -All role overlays are **optional** — the base `appsettings.json` plus the Options-class C# defaults are enough to boot. The roles themselves come from the `OTOPCUA_ROLES` env var (see [`ServiceHosting.md`](ServiceHosting.md) and the table below). +All role overlays are **optional** — the base `appsettings.json` plus the Options-class C# defaults are enough to boot. The roles themselves come from `EffectiveRoles.Resolve` (`ZB.MOM.WW.OtOpcUa.Cluster`) — `Cluster:Roles`, falling back to `OTOPCUA_ROLES` when that is empty (see [`ServiceHosting.md`](ServiceHosting.md) and the table below). The checked-in `appsettings*.json` files are deliberately thin: they carry only `Serilog` and the `Security:Ldap` overlay. Everything else (`OpcUa`, `Cluster`, `ConnectionStrings`/`ConfigDb`) binds from the Options-class defaults documented below unless an operator adds the section explicitly or supplies the corresponding environment variable. @@ -121,11 +121,13 @@ The checked-in `appsettings*.json` files are deliberately thin: they carry only | `Port` | int | `4053` | Cluster transport port. **Duplicated as `ClusterNode.AkkaPort` in the Config DB** — see the note below. | | `PublicHostname` | string | `127.0.0.1` | Hostname advertised in cluster gossip; must be reachable by peers. **Duplicated as `ClusterNode.Host`** — see the note below. | | `SeedNodes` | string[] | `[]` | Seed nodes for bootstrapping. **Ordered:** a node that appears in its own list must be entry 0 (only `seed-nodes[0]` can form a new cluster), or the host refuses to start — see [Redundancy.md § Bootstrap: self-first seed ordering](Redundancy.md#bootstrap-self-first-seed-ordering). **Per-pair since Phase 6:** each application `Cluster`'s pair is its own Akka mesh, so a node's `SeedNodes` lists only itself (first) and its own pair partner (second) — never a node from another cluster's pair. | -| `Roles` | string[] | `[]` | Cluster roles for this node. When empty, falls back to `OTOPCUA_ROLES` (see the note below). Allowed values: `admin`, `driver`, `dev`, plus a per-cluster `cluster-{ClusterId}` role (e.g. `cluster-MAIN`, `cluster-SITE-A`) — see [Per-cluster mesh split (Phase 6)](#per-cluster-mesh-split-phase-6-cluster-clusterid-roles--splittopologytransportvalidator) below. | +| `Roles` | string[] | `[]` | Cluster roles for this node. When empty, falls back to `OTOPCUA_ROLES` (see the note below). Values are trimmed and (for a plain role) lower-cased; a cluster role's ClusterId suffix keeps the operator's spelling. An unknown role from either source **fails host start**. Allowed values: `admin`, `driver`, `dev`, plus a per-cluster `cluster-{ClusterId}` role (e.g. `cluster-MAIN`, `cluster-SITE-A`) — see [Per-cluster mesh split (Phase 6)](#per-cluster-mesh-split-phase-6-cluster-clusterid-roles--splittopologytransportvalidator) below. | -> **`Cluster:Roles` and `OTOPCUA_ROLES` are two knobs for one thing, and only one of them is the Akka role list.** `OTOPCUA_ROLES` drives all of `Program.cs`'s conditional wiring (which subsystems mount); `Cluster:Roles` becomes the node's **gossiped Akka member roles**. Since Gitea #532 `AddOtOpcUaCluster` applies the documented fallback — an empty `Cluster:Roles` takes the env var — so setting only `OTOPCUA_ROLES` now works. Before that fix it did not, silently: the node joined the mesh carrying **no roles at all**, which reads as healthy locally and as *"no driver-role nodes are Up"* at central, and every deployment to it timed out ([`deployments/wonder-app-vd03-makino-z-34184.md`](deployments/wonder-app-vd03-makino-z-34184.md)). +> **One role authority, one precedence, both consumers (archreview R3-04).** `EffectiveRoles.Resolve` (`ZB.MOM.WW.OtOpcUa.Cluster`) is the SINGLE place a node's roles are decided: `Cluster:Roles` wins outright when non-empty, else `OTOPCUA_ROLES`, else empty. Both `Program.cs`'s conditional host wiring (which subsystems mount) AND the Akka `AkkaClusterOptions.Roles` gossiped to the cluster are the SAME `EffectiveRoles` instance's `Gossiped` list — not two separate derivations that happen to usually agree. The boot banner (`OtOpcUa.Host starting with roles=[...] source=...`) names which source won. Setting both sources to *different* normalized role sets logs a Warning at boot (still resolves from `Cluster:Roles`) — almost certainly not what an operator intended. > -> Setting `Cluster:Roles` explicitly is still the recommendation, because the two sources normalize differently: `Cluster:Roles` binds **verbatim** while the env path is **lower-cased** by `RoleParser.Parse`. Akka compares role names ordinally, so a redundant pair configured through *different* sources with a mixed-case `cluster-{ClusterId}` (e.g. `cluster-SITE-A` vs `cluster-site-a`) would gossip two different role names and elect two singletons. Configure both nodes of a pair the same way. +> Before R3-04, `Program.cs` read `OTOPCUA_ROLES` only and never looked at `Cluster:Roles` at all — a node configured solely through `Cluster:Roles` (the shape Gitea #532's fix made viable on the Akka side) joined the mesh gossiping the right roles while `Program.cs`'s own conditional wiring mounted NONE of them, silently. See [`deployments/wonder-app-vd03-makino-z-34184.md`](deployments/wonder-app-vd03-makino-z-34184.md) for the inverse (env-only, pre-#532) incident this family of defect belongs to. +> +> **A role overlay may not itself set `Cluster:Roles`.** `Program.cs` resolves `EffectiveRoles` off the *pre-overlay* configuration — it needs the answer to pick the overlay's filename — so a role overlay (or a `${secret:}` expansion) that changed `Cluster:Roles` would leave the bound options disagreeing with every consumer holding the registered instance. `AkkaClusterOptionsValidator` fails host start if this ever happens, naming both views. > The full redundancy model (ServiceLevel tiers, split-brain, peer discovery) is in [`Redundancy.md`](Redundancy.md). The OPC UA peer-URI advertising lives in the `OpcUa:PeerApplicationUris` key above. @@ -446,7 +448,7 @@ All names are read in this repo's source via `Environment.GetEnvironmentVariable | Variable | Read by | Effect / default | |---|---|---| -| `OTOPCUA_ROLES` | `src/Server/ZB.MOM.WW.OtOpcUa.Host/Program.cs` (`RoleParser.Parse`) and `AddOtOpcUaCluster`'s `AkkaClusterOptions` post-configure (`RoleParser.ResolveEffectiveRoles`) | Comma-separated cluster roles for the node (`admin`, `driver`, `dev`, `cluster-{ClusterId}`). Drives the conditional wiring and the per-role appsettings overlay, and — when `Cluster:Roles` is empty — the node's Akka member roles too. See the note under `Cluster:Roles` for why the two sources are not interchangeable. | +| `OTOPCUA_ROLES` | `EffectiveRoles.Resolve` (`ZB.MOM.WW.OtOpcUa.Cluster`) — the single role authority both `Program.cs` and `AddOtOpcUaCluster` consume | Comma-separated cluster roles for the node (`admin`, `driver`, `dev`, `cluster-{ClusterId}`), read as the **fallback** only when `Cluster:Roles` is empty. Drives the conditional wiring, the per-role appsettings overlay, and the node's Akka member roles — all from the same resolved instance. See the note under `Cluster:Roles` above. | | `OTOPCUA_CONFIG_CONNECTION` | `src/Core/ZB.MOM.WW.OtOpcUa.Configuration/DesignTimeDbContextFactory.cs` (design-time / `dotnet ef` only) | Read at **design time** by `DesignTimeDbContextFactory.cs` for `dotnet ef` migrations. At **runtime** the server resolves the connection string from `ConnectionStrings:ConfigDb` (env form: `ConnectionStrings__ConfigDb`) via `configuration.GetConnectionString("ConfigDb")` in `ServiceCollectionExtensions.cs` — `OTOPCUA_CONFIG_CONNECTION` appears there only as a hint in an error message, not via `GetEnvironmentVariable`. No credential is embedded in source. | | `ASPNETCORE_ENVIRONMENT` | ASP.NET host builder (framework) | Selects `appsettings.{Environment}.json` (e.g. `Development`). | diff --git a/docs/Redundancy.md b/docs/Redundancy.md index 650ef264..a8dc0ec3 100644 --- a/docs/Redundancy.md +++ b/docs/Redundancy.md @@ -208,7 +208,9 @@ The admin singleton is the cluster's only `RedundancyStateActor`. If the admin l ## Configuration -Per-node identity comes from `appsettings.json` + the `OTOPCUA_ROLES` env var: +Per-node identity comes from `appsettings.json` + one role source — `Cluster:Roles` (below) or the +`OTOPCUA_ROLES` env var; either works, and if BOTH are set on a node they must agree (see +[`Configuration.md`](Configuration.md) → `EffectiveRoles`, archreview R3-04): ```jsonc { @@ -227,6 +229,9 @@ Per-node identity comes from `appsettings.json` + the `OTOPCUA_ROLES` env var: } ``` +the `Cluster__Roles__0`/`Cluster__Roles__1` env-var equivalent, or (the fallback, only when +`Cluster:Roles` is absent/empty): + ``` OTOPCUA_ROLES=admin,driver ``` diff --git a/docs/ServiceHosting.md b/docs/ServiceHosting.md index fed0b328..cddf97ce 100644 --- a/docs/ServiceHosting.md +++ b/docs/ServiceHosting.md @@ -7,7 +7,7 @@ A production OtOpcUa deployment runs **one binary per node**. The historian back | Process | Project | Runtime | Platform | Responsibility | |---|---|---|---|---| -| **OtOpcUa Host** | `src/Server/ZB.MOM.WW.OtOpcUa.Host` | .NET 10 | AnyCPU | Single fused binary. `OTOPCUA_ROLES` env decides what to mount: `admin` (Blazor + auth + control-plane singletons), `driver` (OPC UA endpoint + per-driver actors), or both. | +| **OtOpcUa Host** | `src/Server/ZB.MOM.WW.OtOpcUa.Host` | .NET 10 | AnyCPU | Single fused binary. The resolved [`EffectiveRoles`](#role-gating) decides what to mount: `admin` (Blazor + auth + control-plane singletons), `driver` (OPC UA endpoint + per-driver actors), or both. | | **ZB.MOM.WW.HistorianGateway** *(external — separate deployment)* | not in this repo | .NET 10 | — | The sole historian backend. OtOpcUa talks gRPC to it (via the `ZB.MOM.WW.HistorianGateway.Client` package) for HistoryRead, alarm `SendEvent`, and continuous `WriteLiveValues`. Must run `RuntimeDb:Enabled=true` + `RuntimeDb:EventReadsEnabled=true`; the API key must carry `historian:read` + `historian:write` + `historian:tags:write`. | Galaxy access still uses the separately-installed **mxaccessgw** sidecar (see `docs/v2/Galaxy.ParityRig.md`); the gateway owns the MXAccess COM bitness constraint (its worker is x86 net48). Nothing in the OtOpcUa repo carries that constraint anymore. (The bespoke Wonderware historian sidecar this deployment used to ship was retired — see [drivers/Historian.Wonderware.md](drivers/Historian.Wonderware.md).) @@ -16,7 +16,11 @@ Galaxy access still uses the separately-installed **mxaccessgw** sidecar (see `d ## Role gating -`Program.cs` reads `OTOPCUA_ROLES`, parses it with `RoleParser`, and conditionally registers services: +`Program.cs` resolves `EffectiveRoles.Resolve(builder.Configuration)` (`ZB.MOM.WW.OtOpcUa.Cluster`) — +the single role authority (archreview R3-04): `Cluster:Roles` when non-empty, else `OTOPCUA_ROLES`, +else neither role. The SAME resolved instance drives both the conditional wiring below AND the Akka +gossiped roles (`AkkaClusterOptions.Roles`), so the two cannot disagree — and conditionally registers +services: | Role present | Wires | |---|---| @@ -24,11 +28,11 @@ Galaxy access still uses the separately-installed **mxaccessgw** sidecar (see `d | `driver` | `WithOtOpcUaRuntimeActors` (DriverHostActor + DbHealthProbeActor) — and the OPC UA endpoint on port 4840 | | Either / both | `AddOtOpcUaConfigDb`, `AddOtOpcUaCluster`, `AddOtOpcUaHealth` (`/health/ready`, `/health/active`, `/healthz`) | -Single-node dev: `OTOPCUA_ROLES=admin,driver`. Production: typically two admin nodes (HA pair) + N driver nodes. +Single-node dev: `OTOPCUA_ROLES=admin,driver` (or the `Cluster:Roles` equivalent). Production: typically two admin nodes (HA pair) + N driver nodes. ### Per-role configuration overlays -`Program.cs:33-35` builds a role suffix by joining the parsed roles **alphabetically** with `-` and loads `appsettings.{roleSuffix}.json` as an optional overlay on top of base `appsettings.json`. Three overlays ship in `src/Server/ZB.MOM.WW.OtOpcUa.Host/`: +`Program.cs` builds a role suffix by joining the resolved `EffectiveRoles.Gossiped` roles **alphabetically** with `-` and loads `appsettings.{roleSuffix}.json` as an optional overlay on top of base `appsettings.json`. Three overlays ship in `src/Server/ZB.MOM.WW.OtOpcUa.Host/`: - `appsettings.admin.json` — admin-only nodes - `appsettings.driver.json` — driver-only nodes diff --git a/docs/v2/Cluster.md b/docs/v2/Cluster.md index 61f1b398..9759334c 100644 --- a/docs/v2/Cluster.md +++ b/docs/v2/Cluster.md @@ -65,9 +65,9 @@ The Cluster.Tests project verifies these key values stay correct (`HoconLoaderTe - `Port`: TCP port for cluster gossip. Default 4053. - `PublicHostname`: address advertised in cluster gossip. Must be reachable by every other node. - `SeedNodes`: where new nodes go to join. List one (or two) stable nodes. **Order is load-bearing:** Akka only lets `seed-nodes[0]` form a *new* cluster (`FirstSeedNodeProcess`); every other node retries `InitJoin` forever. A node that is one of its own seeds must therefore list ITSELF first, or it can never cold-start while its peer is down — enforced at boot by `AkkaClusterOptionsValidator`, explained in [Redundancy.md § Bootstrap: self-first seed ordering](../Redundancy.md#bootstrap-self-first-seed-ordering). Nodes seeded only by someone else (driver-only site nodes) are exempt. -- `Roles`: free-form tags Akka gossip propagates. v2 uses `admin` + `driver`; per-role wiring in `Program.cs` reads `OTOPCUA_ROLES` env var, not this list — these two should stay in sync. +- `Roles`: free-form tags Akka gossip propagates. v2 uses `admin` + `driver`. Since archreview R3-04, this list and `Program.cs`'s per-role wiring are the SAME resolved `EffectiveRoles` instance (`Cluster:Roles` here, falling back to `OTOPCUA_ROLES` when empty) — not two independent reads that need to be kept in sync by hand. See [Configuration.md § `Roles`](../Configuration.md#appsettings-sections). -Per-role overlay files (`appsettings.admin.json`, `appsettings.driver.json`, `appsettings.admin-driver.json`) layer on top of base `appsettings.json` based on the parsed `OTOPCUA_ROLES` (alphabetical, joined by `-`). See [ServiceHosting.md § Per-role configuration overlays](../ServiceHosting.md#per-role-configuration-overlays). +Per-role overlay files (`appsettings.admin.json`, `appsettings.driver.json`, `appsettings.admin-driver.json`) layer on top of base `appsettings.json` based on the resolved `EffectiveRoles` (alphabetical, joined by `-`). See [ServiceHosting.md § Per-role configuration overlays](../ServiceHosting.md#per-role-configuration-overlays). ## IClusterRoleInfo diff --git a/scripts/install/Install-Services.ps1 b/scripts/install/Install-Services.ps1 index 817e60bd..4e96537e 100644 --- a/scripts/install/Install-Services.ps1 +++ b/scripts/install/Install-Services.ps1 @@ -6,7 +6,10 @@ .DESCRIPTION v2 consolidates the legacy OtOpcUa + OtOpcUaAdmin services into a single role-gated - OtOpcUaHost binary. The -Roles parameter sets the OTOPCUA_ROLES service env so + OtOpcUaHost binary. The -Roles parameter sets the OTOPCUA_ROLES service env — the + documented FALLBACK role source (archreview R3-04's EffectiveRoles.Resolve prefers + Cluster:Roles when it is set, else OTOPCUA_ROLES; this script only ever writes the + latter, which is still every non-hand-patched production node's only role knob) — so Program.cs decides what to mount (admin / driver / both). The Wonderware historian sidecar is installed with -InstallWonderwareHistorian; it listens on a TCP port (default 32569, configurable via -HistorianTcpPort) protected by the shared secret. @@ -28,7 +31,9 @@ - "admin,driver" — single-node dev or all-in-one production node - "admin" — admin-only HA pair member (Blazor + control-plane singletons) - "driver" — driver-only node (OPC UA endpoint + per-node actors) - Written to the service env as OTOPCUA_ROLES. + Written to the service env as OTOPCUA_ROLES — the fallback role source. If this node + also carries a Cluster:Roles registry value (e.g. a hand-applied field fix), that + wins outright; see docs/Configuration.md's EffectiveRoles note (archreview R3-04). .PARAMETER HttpPort HTTP port for the AdminUI + auth endpoints. Default 9000. Written as ASPNETCORE_URLS. From 985de0b6ad696c0a6418c6d4e175d3b12122ca98 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:32:18 -0400 Subject: [PATCH 11/15] chore(docker-dev): site-b-2 is Cluster:Roles-only, site-a-2 pins a miscased Cluster:Roles entry (archreview 01/S-12, 01/C-9) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit T11: site-b-2 drops OTOPCUA_ROLES entirely — Cluster:Roles is its sole role source, the rig's only node that can exhibit the two-role-authority split (01/S-12). Expected to fail every deploy on a pre-R3-04 build. T12: site-a-2 pins Cluster__Roles__0="Driver" (capital D) as the standing 01/C-9 case-mismatch pin, keeping cluster-SITE-A verbatim. Pre-fix this un-elects the SITE-A Primary. Combined into one commit (deviation from the plan's two separate T11/T12 commits): the header comment block documenting all six nodes' role sources interleaves both nodes on adjacent lines, so a clean split was not practical. Recorded in the plan's Execution notes. Compose edits only — the rig is NOT brought up as part of this change. --- ...3-04-one-role-authority-plan.md.tasks.json | 4 ++-- docker-dev/README.md | 23 +++++++++++++++++-- docker-dev/docker-compose.yml | 21 +++++++++++++---- 3 files changed, 40 insertions(+), 8 deletions(-) diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index 7022a35c..a29b3919 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -83,13 +83,13 @@ { "id": "T11", "subject": "docker-dev: site-b-2 becomes Cluster:Roles-only so the rig can exhibit the two-authority split (theme 2 rig-fidelity)", - "status": "pending", + "status": "completed", "blockedBy": [] }, { "id": "T12", "subject": "docker-dev: site-a-2 pins a miscased Cluster__Roles__0 = 'Driver' as the standing 01/C-9 pin", - "status": "pending", + "status": "completed", "blockedBy": [] }, { diff --git a/docker-dev/README.md b/docker-dev/README.md index 857da28b..97da16a5 100644 --- a/docker-dev/README.md +++ b/docker-dev/README.md @@ -27,17 +27,36 @@ Authentication uses the **shared GLAuth** on the Linux Docker host at `10.100.0. | Service | Roles | Ports | |---|---|---| | `site-a-1` | `OTOPCUA_ROLES=driver`, joins the single mesh | host `4842` → container `4840` | -| `site-a-2` | `OTOPCUA_ROLES=driver`, joins the single mesh | host `4843` → container `4840` | +| `site-a-2` | `OTOPCUA_ROLES=driver` **+** a deliberately miscased `Cluster__Roles__0=Driver` (see below) | host `4843` → container `4840` | ### Site B nodes — driver-only (SITE-B cluster) | Service | Roles | Ports | |---|---|---| | `site-b-1` | `OTOPCUA_ROLES=driver`, joins the single mesh | host `4844` → container `4840` | -| `site-b-2` | `OTOPCUA_ROLES=driver`, joins the single mesh | host `4845` → container `4840` | +| `site-b-2` | **`Cluster:Roles`-ONLY** — no `OTOPCUA_ROLES` at all (see below) | host `4845` → container `4840` | Site nodes serve no UI and authenticate no users. The central cluster manages and deploys to them over the shared Akka mesh. All six nodes bind Akka remoting to port `4053` inside their own network namespace; `PublicHostname` for each matches its Compose service name. +### Role-authority standing pins (archreview R3-04) + +Every node above sets **both** `OTOPCUA_ROLES` and the matching `Cluster__Roles__*` env vars, +consistently — which is exactly why neither R3-04 finding (01/S-12, 01/C-9) could reproduce on this +rig as originally configured. Two nodes deliberately deviate, as standing pins: + +- **`site-b-2` is `Cluster:Roles`-only.** No `OTOPCUA_ROLES` is set at all; `Cluster__Roles__0=driver` + / `Cluster__Roles__1=cluster-SITE-B` are its sole role source. **Expected to FAIL every deploy on a + pre-R3-04 build** (`Program.cs` read `OTOPCUA_ROLES` only, so this node's host-side conditional + wiring mounted no driver stack at all — no LocalDb, no OPC UA endpoint, no ConfigDb, the deny-all + `NullOpcUaUserAuthenticator` — while its Akka gossip still looked healthy) — the exact outage + 01/S-12 describes. Post-fix it behaves identically to `site-b-1`. +- **`site-a-2` pins a miscased `Cluster__Roles__0=Driver`** (capital D; `cluster-SITE-A` keeps its + own spelling verbatim, by design). Post-fix `EffectiveRoles` normalizes it silently and this node + behaves identically to `site-a-1`. Pre-fix, all four role-scoped validators (`SplitTopologyTransportValidator` + and three siblings) compared roles case-sensitively, so three of the four silently skipped every + driver-scoped boot-time rule for this node, and `site-a` elected **no Primary at all** + (`RedundancyStateActor.SelectDriverPrimary` matches roles ordinally against gossip). + ## Multi-tenancy All six host nodes write to the same `OtOpcUa` ConfigDb. The `ServerCluster` table differentiates the three logical clusters: each maps to one row, and each `ClusterNode` row's `ClusterId` ties the runtime node back to its owning cluster scope. diff --git a/docker-dev/docker-compose.yml b/docker-dev/docker-compose.yml index 97abc07c..34caf3da 100644 --- a/docker-dev/docker-compose.yml +++ b/docker-dev/docker-compose.yml @@ -47,10 +47,15 @@ # Traefik). Both are Akka seed nodes for the MAIN mesh # ONLY, each listing ITSELF first (2026-07-22) so either # can cold-start while the other is down. -# site-a-1, site-a-2 OTOPCUA_ROLES=driver,cluster-SITE-A — driver-only +# site-a-1, site-a-2 OTOPCUA_ROLES=driver,cluster-SITE-A — driver-only. +# site-a-2 additionally pins a miscased Cluster__Roles__0= +# "Driver" (R3-04, archreview 01/C-9 standing pin). # site-b-1, site-b-2 OTOPCUA_ROLES=driver,cluster-SITE-B — members of their # OWN 2-node mesh (self-first seeded within the pair, NOT -# off central). They serve no UI, but they DO expose OPC UA +# off central) — EXCEPT site-b-2, which sets NO OTOPCUA_ROLES +# at all and is Cluster:Roles-only (R3-04, archreview 01/S-12 +# standing pin — the rig's one node that can exhibit the +# two-role-authority split). They serve no UI, but they DO expose OPC UA # data-plane endpoints (4842-4845) and authenticate those # users against the same shared GLAuth (via the *ldap-env # anchor); central manages + deploys to them over the @@ -713,7 +718,13 @@ services: # Bootstrap-guard enablement demo — see site-a-1. site-a-2 is the HIGHER address, so the guard # makes it probe site-a-1 and join it (peer-first) rather than race-form its own cluster. Cluster__BootstrapGuard__Enabled: "true" - Cluster__Roles__0: "driver" + # R3-04 (01/C-9) standing pin: 'Driver' (capital D) — a deliberately miscased plain role. + # EffectiveRoles normalizes it silently (Program.cs and AkkaClusterOptions.Roles both still + # gossip lower-case 'driver'), so post-fix this node behaves identically to site-a-1; pre-fix + # every one of the four role-scoped validators skipped its driver-scoped rules for this node, + # and site-a elected no Primary (RedundancyStateActor.SelectDriverPrimary matches ordinally). + # cluster-SITE-A keeps its own spelling verbatim by design — do not "fix" that half. + Cluster__Roles__0: "Driver" Cluster__Roles__1: "cluster-SITE-A" # Mesh command transport (per-cluster mesh Phase 2, made mandatory by Phase 6) — see # central-1/site-a-1. @@ -850,7 +861,9 @@ services: # cluster on simultaneous start (split brain). site-b-1: { condition: service_healthy } environment: - OTOPCUA_ROLES: "driver,cluster-SITE-B" + # R3-04 (01/S-12) standing pin: this is the rig's ONLY Cluster:Roles-ONLY node — deliberately + # no OTOPCUA_ROLES at all (every other node sets both, which is why neither R3-04 finding could + # reproduce on the rig before this). Cluster__Roles__0/1 below are this node's sole role source. # Per-cluster mesh Phase 4: no ConfigDb string on driver-only site nodes (see site-a-1). Cluster__Hostname: "0.0.0.0" Cluster__Port: "4053" From 027c4235024539c39d3ae368f57ece6849f48e13 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:33:07 -0400 Subject: [PATCH 12/15] =?UTF-8?q?docs(plans):=20R3-04=20execution=20notes?= =?UTF-8?q?=20=E2=80=94=20T6-T12=20deviations=20recorded,=20T13=20marked?= =?UTF-8?q?=20pending=20(live=20gate=20deferred=20to=20orchestrator)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../plans/R3-04-one-role-authority-plan.md | 50 +++++++++++++++++++ ...3-04-one-role-authority-plan.md.tasks.json | 3 +- 2 files changed, 52 insertions(+), 1 deletion(-) diff --git a/archreview/plans/R3-04-one-role-authority-plan.md b/archreview/plans/R3-04-one-role-authority-plan.md index b809adcf..0b33665d 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md +++ b/archreview/plans/R3-04-one-role-authority-plan.md @@ -502,3 +502,53 @@ gossips 'Driver' and a Primary IS elected" — is **not achievable**. What shipp this test is actually about, without needing a second live node. No other T1–T5 deviations from the plan. + +**T6-T9 — no deviations.** Implemented as designed; `AkkaClusterOptionsValidator` gained an optional +(default-null) `EffectiveRoles?` constructor parameter rather than a required one specifically so the +pre-existing zero-arg test constructions in `AkkaClusterOptionsValidatorTests` did not all need +touching — the guard is simply inert when nothing is injected, which is also the correct behaviour +for any harness that builds this validator without the rest of the role authority. + +**T8 — two additional bare-role-literal offenders found and fixed, beyond the two the plan named.** +The census's row-by-row list did not claim to be exhaustive of every bare literal in the tree, only +of the *production predicate* sites; running the guard for real (rather than reasoning about it) +found two more before it could go green: + +- `DriverInstanceActor.ShouldStub`'s `roles.Contains("dev")` — a genuine Akka-cluster-role read + (`_localRoles`, threaded from `IClusterRoleInfo.LocalRoles`) gating dev-stub mode for + Windows-only drivers. Fixed to `RoleParser.GossipedRolesHave(roles, RoleParser.Dev)`. +- `ControlPlane/ServiceCollectionExtensions.AdminRole = "admin"` — a second constant duplicating the + literal `RoleParser.Admin` names, feeding Akka's own `ClusterSingletonOptions.Role` matching (the + same "not ours to change" ordinal match as `SelectOldestUpMemberOfRole`). The VALUE was already + correct (Akka's own matching is unaffected either way); fixed to alias `RoleParser.Admin` rather + than duplicate it, matching the sibling `Runtime/ServiceCollectionExtensions.DriverRole` pattern. + +The guard also required a small allow-list (six files) for bare `"admin"`/`"driver"`/`"dev"` literals +that name something OTHER than an Akka `Cluster:Roles` role — a raw-tree node-KIND discriminator, an +ACL-scope entity-type name, the Sql driver's own TagConfig JSON marker (×3 files), and an LDAP-derived +AdminUI RBAC role. Each carries its reason in `RoleAuthorityGuardTests.RoleLiteralAllowListWithReasons`, +per the plan's "add to the allow-list WITH A REASON" instruction; a companion test +(`Every_allow_list_entry_has_a_reason_and_still_applies`) fails if an entry goes stale. + +The falsification control (`The_scan_actually_detects_the_pre_R3_04_offenders`) could not literally +re-run the guard against the pre-T3/pre-T5 GIT COMMITS (T1–T8 landed sequentially in this same +worktree, so "the tree" is always post-fix by the time T8's test runs) — it instead feeds the scan +logic the exact pre-fix `Program.cs`/`Fleet.razor` snippets as string literals, captured verbatim +from this session's own earlier reads of those files before they were edited. Functionally +equivalent proof-of-falsifiability; recorded as a deviation because it is a different MECHANISM than +"run on the pre-T3 tree," not because the result differs. + +**T9 — scope note, not a deviation.** "The driver stack" is proven via two representative slices +(`ILocalDb` resolution + the `IOpcUaUserAuthenticator` selection block), not the ENTIRE +`WithOtOpcUaRuntimeActors` Akka actor graph — replicating that too would need a real `ActorSystem` +(the exact thing `DriverOnlyNoConfigDbBootTests`'s own doc comment explains this test family avoids). +The negative control uses an inline `PreFixDerivation` helper (reading `OTOPCUA_ROLES` only, matching +pre-T3 `Program.cs:61-63` verbatim) rather than a `git stash`, for the same "already sequential +commits in one worktree" reason as T8's falsification control — and it is run in the SAME test +process against the SAME configuration, which is a stronger control than a separate stash-and-revert +cycle would have been (both derivations are proven against literally identical input). + +**T11/T12 — combined into one commit.** The docker-compose.yml header comment documenting all six +nodes' role sources interleaves site-a-2 and site-b-2 on adjacent lines, so a clean git-level split +into the plan's two separate commits was not practical; both tasks' edits landed in one commit +(`985de0b6`). The rig was NOT brought up as part of either task, per the brief. diff --git a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json index a29b3919..d901dc01 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json +++ b/archreview/plans/R3-04-one-role-authority-plan.md.tasks.json @@ -106,7 +106,8 @@ "T9", "T11", "T12" - ] + ], + "note": "Skipped by the executing agent per orchestrator instruction \u2014 the live gate runs separately, on the docker-dev rig, by the orchestrator/operator. T1-T12 are implemented, committed, and unit/integration-tested; T13 requires bringing the rig up (docker compose), which this task explicitly excluded." } ] } From 3648d3c129b136475a0336d0d04d35e45aa8a36f Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 10:52:51 -0400 Subject: [PATCH 13/15] =?UTF-8?q?fix(cluster):=20ClusterRoleInfo.=5FroleLe?= =?UTF-8?q?aders=20stays=20Ordinal=20=E2=80=94=20Akka=20tracks=20role-lead?= =?UTF-8?q?ership=20per=20exact=20gossiped=20role=20string=20(review=20fin?= =?UTF-8?q?ding,=20R3-04)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .../ClusterRoleInfo.cs | 21 ++- .../ClusterRoleInfoRoleLeaderCaseTests.cs | 129 ++++++++++++++++++ 2 files changed, 144 insertions(+), 6 deletions(-) create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ClusterRoleInfoRoleLeaderCaseTests.cs diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs index 74151f43..689e435b 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs @@ -23,12 +23,21 @@ public sealed class ClusterRoleInfo : IClusterRoleInfo, IDisposable private readonly string? _clusterRole; private readonly string? _clusterId; private readonly object _lock = new(); - // R3-04 (01/C-9, census row 17): OrdinalIgnoreCase — Akka compares gossiped role names ordinally, - // so a PEER running an older build can still gossip a miscased plain role during a rolling - // upgrade. Keying these by the exact-cased role string would leave MembersWithRole("driver") and - // RoleLeader("driver") blind to a peer that gossips "Driver", the same class of gap #532 closed - // for _localRoles above. - private readonly Dictionary _roleLeaders = new(StringComparer.OrdinalIgnoreCase); + // R3-04 (01/C-9, census row 17) — the two dictionaries below have DIFFERENT correct comparers, + // and that split is deliberate, not an oversight (review finding, 2026-08-20). + // + // _membersByRole answers "every member carrying (a spelling of) this role" — OrdinalIgnoreCase, + // so a PEER running an older build that still gossips a miscased plain role during a rolling + // upgrade is not invisible to MembersWithRole("driver"). Merging casings IS the point here. + // + // _roleLeaders must stay Ordinal. Akka computes — and raises RoleLeaderChanged for — role + // leadership PER EXACT GOSSIPED ROLE STRING: "driver" and "Driver" are two entirely distinct + // roles to Akka's own role-leader machinery, each independently electing its own leader. An + // OrdinalIgnoreCase dictionary here would collapse both onto ONE key, so whichever + // RoleLeaderChanged event landed last would silently overwrite the other's leader — turning two + // real, independent facts Akka tracks separately into one, wrong, merged one. Pinned by + // ClusterRoleInfoRoleLeaderCaseTests.RoleLeaderChanged_for_driver_then_Driver_keeps_distinct_entries. + private readonly Dictionary _roleLeaders = new(StringComparer.Ordinal); private readonly Dictionary> _membersByRole = new(StringComparer.OrdinalIgnoreCase); private IActorRef? _subscriber; diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ClusterRoleInfoRoleLeaderCaseTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ClusterRoleInfoRoleLeaderCaseTests.cs new file mode 100644 index 00000000..82c3aee0 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ClusterRoleInfoRoleLeaderCaseTests.cs @@ -0,0 +1,129 @@ +using System.Net; +using System.Net.Sockets; +using Akka.Hosting; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Hosting; +using Microsoft.Extensions.Options; +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Commons.Interfaces; +using ZB.MOM.WW.OtOpcUa.Commons.Types; + +namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; + +/// +/// Review finding on R3-04 (Important, 2026-08-20): _roleLeaders must stay +/// , NOT like +/// _membersByRole. Akka raises a SEPARATE RoleLeaderChanged event per exact gossiped +/// role string — "driver" and "Driver" are two entirely distinct roles to Akka's own role-leader +/// computation, each with its own (possibly different) leader member. An ignore-case dictionary +/// would collapse both onto ONE key, and whichever role-leader-changed event landed LAST would +/// silently overwrite the other — RoleLeader("driver") could then return the "Driver" +/// role's leader instead of (or in addition to losing) the "driver" role's own. +/// +/// +/// _membersByRole stays deliberately — it +/// answers "give me every member carrying (a spelling of) this role", where merging casings is +/// the whole point (archreview 01/C-9, rolling-upgrade peer defence in depth). _roleLeaders +/// answers a per-EXACT-role-string question Akka itself keeps separate, so merging casings there +/// loses information instead. +/// +public sealed class ClusterRoleInfoRoleLeaderCaseTests +{ + private static int FreePort() + { + var listener = new TcpListener(IPAddress.Loopback, 0); + listener.Start(); + var port = ((IPEndPoint)listener.LocalEndpoint).Port; + listener.Stop(); + return port; + } + + private const string SystemName = "clusterroleinfo-leader-case-test"; + + private static async Task<(IHost Host, IClusterRoleInfo RoleInfo)> StartNodeAsync( + int selfPort, int? peerPort, string role) + { + var self = $"akka.tcp://{SystemName}@127.0.0.1:{selfPort}"; + var seeds = peerPort is null + ? new[] { self } + : new[] { self, $"akka.tcp://{SystemName}@127.0.0.1:{peerPort}" }; + + var options = new AkkaClusterOptions + { + SystemName = SystemName, + Hostname = "127.0.0.1", + PublicHostname = "127.0.0.1", + Port = selfPort, + // Deliberately ONE role each, spelled differently between the two nodes — that mismatch + // IS the scenario under test, not an oversight. + Roles = new[] { role }, + SeedNodes = seeds, + }; + + var builder = Host.CreateDefaultBuilder(); + builder.ConfigureServices(services => + { + services.AddSingleton>(Options.Create(options)); + services.AddAkka(SystemName, (ab, sp) => ab.WithOtOpcUaClusterBootstrap(sp)); + services.AddSingleton(); + }); + + var host = builder.Build(); + await host.StartAsync(); + return (host, host.Services.GetRequiredService()); + } + + /// + /// Two single-member "roles" — "driver" on node A, "Driver" on node B, both Up in the SAME + /// cluster. Each is the sole (and therefore leader) member of its own exact-string role. If + /// _roleLeaders were case-insensitive, the two RoleLeaderChanged events would collide + /// onto one dictionary slot and one of the two role-leader facts would be lost or wrong. + /// + [Fact] + public async Task RoleLeaderChanged_for_driver_then_Driver_keeps_distinct_entries() + { + var portA = FreePort(); + var portB = FreePort(); + + var (hostA, roleInfoA) = await StartNodeAsync(portA, peerPort: null, role: "driver"); + try + { + var (hostB, _) = await StartNodeAsync(portB, peerPort: portA, role: "Driver"); + try + { + var deadline = DateTime.UtcNow + TimeSpan.FromSeconds(30); + NodeId? driverLeader = null; + NodeId? capitalDriverLeader = null; + while (DateTime.UtcNow < deadline) + { + driverLeader = roleInfoA.RoleLeader("driver"); + capitalDriverLeader = roleInfoA.RoleLeader("Driver"); + if (driverLeader is not null && capitalDriverLeader is not null) break; + await Task.Delay(200, TestContext.Current.CancellationToken); + } + + driverLeader.ShouldNotBeNull("node A's own 'driver' role must elect itself leader"); + capitalDriverLeader.ShouldNotBeNull( + "node B's 'Driver' role must ALSO be tracked — a case-insensitive dictionary " + + "would either lose this entry or clobber the 'driver' one with it"); + driverLeader!.Value.ShouldNotBe( + capitalDriverLeader!.Value, + "the two roles are DISTINCT to Akka and must resolve to their own (different) " + + "member — a merged dictionary slot would make these equal"); + driverLeader.Value.ToString().ShouldContain(portA.ToString()); + capitalDriverLeader.Value.ToString().ShouldContain(portB.ToString()); + } + finally + { + await hostB.StopAsync(TestContext.Current.CancellationToken); + hostB.Dispose(); + } + } + finally + { + await hostA.StopAsync(TestContext.Current.CancellationToken); + hostA.Dispose(); + } + } +} From af39e616382e68958dd932ad91be431dadd0f29e Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 11:01:33 -0400 Subject: [PATCH 14/15] =?UTF-8?q?fix(cluster):=20T7's=20post-overlay=20gua?= =?UTF-8?q?rd=20was=20structurally=20unreachable=20=E2=80=94=20compare=20l?= =?UTF-8?q?ive=20Cluster:Roles,=20not=20the=20PostConfigured=20options.Rol?= =?UTF-8?q?es=20(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); + } } From e6a36f4ba8d810b4abd4a41e9b2130827e13cc38 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Thu, 20 Aug 2026 11:03:24 -0400 Subject: [PATCH 15/15] docs: ServiceHosting.md overlay-filename casing note; R3-04 review outcome (both FIX-FIRST items resolved) --- .../plans/R3-04-one-role-authority-plan.md | 103 ++++++++++++++++++ docs/ServiceHosting.md | 2 +- 2 files changed, 104 insertions(+), 1 deletion(-) diff --git a/archreview/plans/R3-04-one-role-authority-plan.md b/archreview/plans/R3-04-one-role-authority-plan.md index 0b33665d..c70d614c 100644 --- a/archreview/plans/R3-04-one-role-authority-plan.md +++ b/archreview/plans/R3-04-one-role-authority-plan.md @@ -552,3 +552,106 @@ cycle would have been (both derivations are proven against literally identical i nodes' role sources interleaves site-a-2 and site-b-2 on adjacent lines, so a clean git-level split into the plan's two separate commits was not practical; both tasks' edits landed in one commit (`985de0b6`). The rig was NOT brought up as part of either task, per the brief. + +## Review outcome + +A code review after T1–T12 returned verdict **FIX-FIRST**, two items. Both are fixed, each in its +own TDD commit, on top of the T1–T12 work above. + +### 1. [Blocking] T7's post-overlay guard was structurally unreachable + +**The finding.** `AkkaClusterOptionsValidator.ValidateAgainstEffectiveRoles` compared +`options.Roles` against `_effectiveRoles.Gossiped` — but `ServiceCollectionExtensions.cs`'s +PostConfigure (T2's OWN structural-identity fix) unconditionally sets +`options.Roles = [.. effectiveRoles.Gossiped]` first. The two values are therefore **equal by +construction**, on every real host, always — the guard could never observe a divergence no matter +what a role overlay did to `Cluster:Roles`. My original two tests (now deleted) hand-built an +`AkkaClusterOptions` instance directly and set `.Roles` to a "mismatched" value bypassing +PostConfigure entirely — which exercised the comparison logic in isolation but never proved the +guard reachable through the real pipeline. **Concretely, before this fix: an overlay that set +`Cluster:Roles` was silently ignored (PostConfigure overwrote it back to the pre-overlay value), not +failed at boot** — the exact opposite of the guard's purpose (choice 4: "a role overlay may not set +Cluster:Roles"). + +**The fix.** `AkkaClusterOptionsValidator` now also takes an optional `IConfiguration?` (default +null, same pattern as `EffectiveRoles?` — so every pre-existing zero/one-arg test construction keeps +compiling and behaving as before). The guard re-reads the RAW `Cluster:Roles` section directly off +the LIVE configuration (`_configuration.GetSection(EffectiveRoles.ConfiguredRolesKey).Get()`) +rather than the bound options, normalizes it by reusing `EffectiveRoles.Resolve(raw, envRoles: null)` +(one rule, stated once — archreview theme 4, rather than re-implementing the configured-path +normalization a second time), and compares that against `_effectiveRoles.Gossiped`. Skipped entirely +when the raw post-overlay value is empty — required for correctness, not merely tidiness: an +`OTOPCUA_ROLES`-only node (most of production, per CLAUDE.md) has an empty `Cluster:Roles` both +before AND after any overlay, and comparing "empty" against a non-empty env-derived `Gossiped` list +would be a permanent false positive on every one of those nodes. `AddOtOpcUaCluster` now also +`TryAddSingleton(configuration)` so a bare `ServiceCollection` test harness (not just +`WebApplicationBuilder`, which auto-registers `IConfiguration`) can supply it. + +**The tests.** Both guard tests are rewritten to go through the REAL `AddOtOpcUaCluster` + +PostConfigure pipeline, over a `ConfigurationManager` (not a one-shot `ConfigurationBuilder`) — the +same "add a provider to the SAME live object after the fact" shape `WebApplicationBuilder.Configuration` +has, which is what lets a role overlay be simulated faithfully: `EffectiveRoles.Resolve(configuration)` +is called and its result registered BEFORE a second `AddInMemoryCollection` call mutates the same +`configuration` object with different `Cluster:Roles` values, mirroring `Program.cs`'s own +resolve-then-`AddJsonFile(overlay)` sequencing exactly. Confirmed red first: the "changes" test +failed with "should throw OptionsValidationException but did not" against the pre-fix (options.Roles +-comparing) validator — direct proof of the reviewer's structural-unreachability claim. Two +supporting tests were added beyond the two named in the review comment: `No_overlay_at_all_passes` +(the ordinary case) and `EffectiveRoles_injected_without_IConfiguration_skips_the_guard_entirely` +(the two-optional-dependency ctor's other skip leg). One self-inflicted false-positive surfaced and +was fixed along the way: the new catch-block's error message split `"Cluster:Roles."` onto its own +string-literal token, which `RoleAuthorityGuardTests`' own space-inside-the-literal heuristic (T8) +correctly flagged as NOT looking like an operator sentence — merging it back into one literal +containing surrounding words fixed it without weakening the guard. + +### 2. [Important] `ClusterRoleInfo._roleLeaders` must stay `Ordinal`, not `OrdinalIgnoreCase` + +**The finding.** T5 moved BOTH `_roleLeaders` and `_membersByRole` to `OrdinalIgnoreCase` under one +shared rationale (defence in depth against a miscased rolling-upgrade peer). That rationale is right +for `_membersByRole` ("every member carrying a spelling of this role" — merging casings is the +point) but wrong for `_roleLeaders`: Akka computes — and raises `RoleLeaderChanged` for — role +leadership **per exact gossiped role string**. "driver" and "Driver" are two entirely distinct roles +to Akka's own role-leader machinery, each independently electing its own leader. Keying +`_roleLeaders` case-insensitively collapses both onto ONE dictionary slot, so whichever +`RoleLeaderChanged` event landed last silently overwrites the other's leader — turning two real, +independent facts Akka tracks separately into one, wrong, merged fact. + +**The fix.** `_roleLeaders` reverted to `StringComparer.Ordinal`; `_membersByRole` stays +`OrdinalIgnoreCase` (the split itself, not merely one value, is now documented in the field +comment). No other production call site touches `_roleLeaders`'s comparer, so this is a one-line +revert plus doc. + +**The test.** `ClusterRoleInfoRoleLeaderCaseTests.RoleLeaderChanged_for_driver_then_Driver_keeps_distinct_entries` +— a real two-node cluster (the only faithful option: `HandleRoleLeaderChanged` is `internal` with no +`InternalsVisibleTo`, so it cannot be unit-tested directly, and `ClusterRoleInfo`'s constructor needs +a genuinely clustered `ActorSystem`). Node A gossips only `"driver"`, node B (joining the same +cluster) gossips only `"Driver"` — each is therefore the sole, and so leader, member of its own +exact-string role. Confirmed red first: against the pre-fix (`OrdinalIgnoreCase`) comparer, the test +failed with `driverLeader` and `capitalDriverLeader` resolving to the SAME address — the exact +collision the review predicted, reproduced live rather than merely reasoned about. + +### Optional item — done + +Added one sentence to `docs/ServiceHosting.md`'s "Per-role configuration overlays" section: a plain +role's overlay-filename fragment is always lower-case regardless of how it was spelled in +`Cluster:Roles`, but a `cluster-{ClusterId}` role's suffix carries into the filename verbatim (since +`EffectiveRoles.Gossiped` preserves that spelling by design) — e.g. +`Cluster:Roles: ["driver", "cluster-SITE-A"]` loads `appsettings.cluster-SITE-A-driver.json`. + +### Verification + +`dotnet build ZB.MOM.WW.OtOpcUa.slnx` → 0 errors throughout. + +- After item 2 (`ClusterRoleInfo`): `Cluster.Tests` 241 → **242/242** (+1, + `ClusterRoleInfoRoleLeaderCaseTests`). +- After item 1 (T7 guard): `Cluster.Tests` 242 → **244/244** (net +2: the two old hand-built T7 + tests deleted, four new ones added — + `An_overlay_that_changes_Cluster_Roles_after_resolution_fails_naming_both_views`, + `An_overlay_that_repeats_the_same_Cluster_Roles_passes`, `No_overlay_at_all_passes`, + `EffectiveRoles_injected_without_IConfiguration_skips_the_guard_entirely` — the pre-existing + `No_EffectiveRoles_injected_skips_the_guard_entirely` was kept unchanged). +- `Host.Tests` **106/106**, unchanged by either fix (`Program.cs` itself was not touched). +- `ControlPlane.Tests` **267/267** — an extra safety check beyond what was asked, since + `ClusterRoleInfo` is a shared dependency of `RedundancyStateActor` et al. + +No pushes, no merges. diff --git a/docs/ServiceHosting.md b/docs/ServiceHosting.md index cddf97ce..fa4c8740 100644 --- a/docs/ServiceHosting.md +++ b/docs/ServiceHosting.md @@ -32,7 +32,7 @@ Single-node dev: `OTOPCUA_ROLES=admin,driver` (or the `Cluster:Roles` equivalent ### Per-role configuration overlays -`Program.cs` builds a role suffix by joining the resolved `EffectiveRoles.Gossiped` roles **alphabetically** with `-` and loads `appsettings.{roleSuffix}.json` as an optional overlay on top of base `appsettings.json`. Three overlays ship in `src/Server/ZB.MOM.WW.OtOpcUa.Host/`: +`Program.cs` builds a role suffix by joining the resolved `EffectiveRoles.Gossiped` roles **alphabetically** with `-` and loads `appsettings.{roleSuffix}.json` as an optional overlay on top of base `appsettings.json`. A plain role (`admin`/`driver`/`dev`) is always lower-case in `Gossiped` regardless of how the operator spelled it, so the overlay filename for those is stable (`appsettings.driver.json`); if a `cluster-{ClusterId}` role is also configured, its suffix carries into the filename **verbatim** (e.g. `Cluster:Roles: ["driver", "cluster-SITE-A"]` loads `appsettings.cluster-SITE-A-driver.json`), since `Gossiped` preserves that spelling by design (see [Configuration.md § `Roles`](Configuration.md#appsettings-sections)). Three overlays ship in `src/Server/ZB.MOM.WW.OtOpcUa.Host/`: - `appsettings.admin.json` — admin-only nodes - `appsettings.driver.json` — driver-only nodes