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]