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(); + } }