diff --git a/docs/Configuration.md b/docs/Configuration.md index f9208c28..eff4ab94 100644 --- a/docs/Configuration.md +++ b/docs/Configuration.md @@ -104,7 +104,11 @@ 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`. 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). 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)). +> +> 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. > 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. @@ -329,7 +333,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`) | Comma-separated cluster roles for the node (`admin`, `driver`, `dev`). Drives the conditional wiring and the per-role appsettings overlay. Used when `Cluster:Roles` is empty. | +| `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_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/deployments/wonder-app-vd03-makino-z-34184.md b/docs/deployments/wonder-app-vd03-makino-z-34184.md index 51cf687b..00580cf8 100644 --- a/docs/deployments/wonder-app-vd03-makino-z-34184.md +++ b/docs/deployments/wonder-app-vd03-makino-z-34184.md @@ -56,6 +56,19 @@ now joins **UP with roles ADMIN+DRIVER** and deployments seal. Prior env backed > Product follow-up: either wire `OTOPCUA_ROLES` → `AkkaClusterOptions.Roles`, or bake `Cluster:Roles` > into the deploy template, so a node redeploy doesn't regress to role-less. +> **RESOLVED 2026-08-01 (Gitea #532).** The first option shipped: `AddOtOpcUaCluster` post-configures +> `AkkaClusterOptions`, so an empty `Cluster:Roles` now falls back to `OTOPCUA_ROLES` through +> `RoleParser.ResolveEffectiveRoles`. A node in this box's original configuration would join **UP with +> roles ADMIN+DRIVER** without the registry edit above, so a redeploy that drops the hand-added +> `Cluster__Roles__0/1` no longer regresses to role-less. The `Cluster__Roles` entries are still worth +> keeping — the two sources normalize case differently (see `docs/Configuration.md` → `Cluster:Roles`), +> and an explicit list is the one that survives someone changing the env var for a different reason. +> +> Worth recording for the next incident of this shape: this was **documented behaviour that did not +> exist**. `AkkaClusterOptions.Roles`' XML doc and `SplitTopologyTransportValidator` both described the +> fallback, so anyone reading the code to diagnose this box would have concluded the roles were already +> being applied and looked elsewhere. + ## Live verification (OtOpcUa CLI client → server) `connect` + `browse` confirm the equipment + both tags are served at the node IDs above. diff --git a/docs/security.md b/docs/security.md index c9b26f35..8f308e01 100644 --- a/docs/security.md +++ b/docs/security.md @@ -337,16 +337,21 @@ until the tracking issue below is closed. **The four things blocking a wire-up** (all verified, none of them merely "call the evaluator"): -1. **Raw-realm nodes have no representable scope.** `NodeHierarchyKind` has exactly one member, - `Equipment`. Raw NodeIds are `Folder→Driver→Device→TagGroup→Tag` RawPaths, which `NodeScope` cannot - express — yet the write gate is attached to raw tags. Roughly half the writable address space has - no scope to evaluate. This is a design decision, not wiring. +1. ~~**Raw-realm nodes have no representable scope.**~~ **Closed by #520 stage 3.** `NodeScope` is now + `(ClusterId, Kind, Segments)` with a variable-length path, and `NodeHierarchyKind` carries both + `Uns` and `Raw`. The previous fixed-arity shape could not address a real raw tree at all — folder + chains and tag-group chains both nest without bound — so it silently capped at five levels and + mislabelled every one of them. Four `Raw*` scope kinds were appended for authoring. 2. ~~**The identity carries roles, not groups.**~~ **Dissolved by #520 stage 2.** The column is now `NodeAcl.Principal` and the principal comes from the presented API key (or the reserved `anonymous`), not from a directory — so there is no group list to lose. -3. **`PermissionTrieBuilder`'s `scopePaths` argument has no producer.** Without it every sub-cluster - grant lands in the builder's fallback bucket — the hazard its own comments warn about. It would have - to be derived from the artifact's UNS relations. +3. ~~**`PermissionTrieBuilder`'s `scopePaths` argument has no producer.**~~ **Closed by #520 stage 3.** + `AclScopePathResolver` is that producer, built on both sides of the split — EF entities at the deploy + gate, artifact JSON on the node — and `scopePaths` is now a required argument with no fallback: an + unresolvable scope is **dropped with a diagnostic** rather than planted at a trie position the walk + can never reach. A scope resolves to *several* paths when one entity is reachable at several NodeIds + (a raw tag projected into N equipments has one raw path plus N UNS paths), because a grant that + covered only some of them would be bypassable through the one it missed. 4. **No session lifecycle exists.** `UserAuthorizationState` has the freshness/staleness predicates but nothing in production constructs one per session. **Half-closed by stage 4:** the *generation* now exists and advances — `DataPlaneAuthorityState` stamps a monotonic `GenerationId` each time an @@ -354,9 +359,23 @@ until the tracking issue below is closed. the stamping is stage 5's job, because until an enforcement seam reads it, a per-session state object would be constructed and never consulted. -Two smaller inconsistencies inside the subsystem itself: `NodeAclScopeKind.FolderSegment` is offered in -the authoring dropdown but the trie walker has no level to match it at, and `NodePermissions.AlarmRead` -has no `OpcUaOperation` mapping, so it is grantable but unreachable. +`NodeAclScopeKind.FolderSegment` is the retired v2 kind with no v3 entity behind it. It is still offered +in the authoring dropdown (labelled retired), but it can never resolve, and both ends now say so +specifically rather than reporting it as a missing id: the deploy gate rejects it with +`NodeAclScopeKindRetired`, and the trie builder drops it with a `RetiredScopeKind` diagnostic (Gitea +#533 — it used to arrive as `UnresolvedScope`, which sent an operator holding a legacy row hunting for a +typo in an id that was never the problem). One inconsistency remains: `NodePermissions.AlarmRead` has no +`OpcUaOperation` mapping, so it is grantable but unreachable. + +**String identity in this subsystem** (Gitea #533): **identifiers** — `ScopeId`, `ClusterId`, +`Principal` — compare `OrdinalIgnoreCase`, because they are opaque handles whose spelling genuinely +varies by source (a `ClusterId` arrives lower-cased through `OTOPCUA_ROLES` and verbatim through +`Cluster:Roles`; a `ScopeId` is operator free-text until the stage-7 picker lands). **Path segments** — +every level of a scope path, and therefore `PermissionTrieNode.Children` — compare `Ordinal`, because +they are browse names and `Speed` / `speed` are two different nodes that can coexist under one device. +The rule is stated in full on `AclScopeRef`. It matters most where the deploy gate and the node are fed +from *different* snapshots, which is exactly the split `AclScopePathResolver` exists to bridge: case +drift there would silently drop every grant on the affected scope. ### Hierarchy (design) diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptions.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptions.cs index 99aad37c..d1dc6cb5 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptions.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/AkkaClusterOptions.cs @@ -48,10 +48,26 @@ public sealed class AkkaClusterOptions public string[] SeedNodes { get; set; } = Array.Empty(); /// - /// Cluster roles for this node. When empty the role list comes from - /// OTOPCUA_ROLES via . Allowed values: - /// admin, driver, dev. + /// Cluster roles for this node. Allowed values: admin, driver, dev, plus one + /// per-cluster cluster-{ClusterId} role (per-cluster mesh, Phase 6). /// + /// + /// + /// When this is empty the list falls back to the OTOPCUA_ROLES environment variable, + /// applied by AddOtOpcUaCluster through + /// . That fallback was + /// described here — and relied upon in the field — for a year before it was implemented; a + /// node configured only through the env var joined the cluster carrying no roles at all. + /// See Gitea #532 and docs/deployments/wonder-app-vd03-makino-z-34184.md. + /// + /// + /// The two sources normalize differently: values bound from Cluster:Roles are taken + /// verbatim, while the environment path is lower-cased by . + /// Akka compares gossiped role names ordinally, so configure both nodes of a redundant + /// pair through the same source — a pair split across the two with a mixed-case + /// cluster-{ClusterId} would gossip two different role names and elect two singletons. + /// + /// public string[] Roles { get; set; } = Array.Empty(); /// diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs index d1f660b6..49949497 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ClusterRoleInfo.cs @@ -39,22 +39,26 @@ public sealed class ClusterRoleInfo : IClusterRoleInfo, IDisposable // sharing a bind IP) stay distinct. Production hosts have unique DNS names so the port // suffix is harmless redundancy. _localNode = CommonsNodeId.Parse($"{options.Value.PublicHostname}:{options.Value.Port}"); - _localRoles = new HashSet(options.Value.Roles, StringComparer.Ordinal); + // OrdinalIgnoreCase (Gitea #532): Cluster:Roles binds verbatim, so a node authored with 'Driver' + // used to answer HasRole("driver") false while gossiping a role Akka would still match against + // its own ordinal comparison — the node was a driver to the cluster and not to itself. + _localRoles = new HashSet(options.Value.Roles, StringComparer.OrdinalIgnoreCase); // Derived from the node's OWN configured roles (not live Cluster.State) so this identity is // available at host-build time, before the cluster forms. Iterate the ORIGINAL array — not // _localRoles — because first-wins requires configuration order, which HashSet does not - // guarantee. - var clusterRoles = options.Value.Roles.Where(RoleParser.IsClusterRole).ToArray(); - if (clusterRoles.Length > 0) + // guarantee. The OTOPCUA_ROLES fallback is already applied to the bound options by + // AddOtOpcUaCluster's PostConfigure, so nothing is re-resolved here. + var clusterRoles = RoleParser.ClusterRolesIn(options.Value.Roles); + if (clusterRoles.Count > 0) { _clusterRole = clusterRoles[0]; _clusterId = RoleParser.ClusterIdFromRole(_clusterRole); - if (clusterRoles.Length > 1) + if (clusterRoles.Count > 1) { _logger.LogWarning( "Node configured with {Count} cluster-scoped roles ({Roles}); using the first ({ClusterRole})", - clusterRoles.Length, string.Join(", ", clusterRoles), _clusterRole); + clusterRoles.Count, string.Join(", ", clusterRoles), _clusterRole); } } diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ConfigSourceOptionsValidator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ConfigSourceOptionsValidator.cs index 520044ef..707a2a4e 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ConfigSourceOptionsValidator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ConfigSourceOptionsValidator.cs @@ -93,11 +93,15 @@ public sealed class ConfigSourceOptionsValidator : IValidateOptions() ?? Array.Empty(); - var isDriver = Array.IndexOf(roles, "driver") >= 0; - var isAdmin = Array.IndexOf(roles, "admin") >= 0; + // connection). Roles come from configuration, not IClusterRoleInfo — see the ctor remark — and + // through RoleParser.ResolveEffectiveRoles so the OTOPCUA_ROLES fallback applies (Gitea #532). + // Reading Cluster:Roles alone exempted the exact node this rule exists for: an env-only + // driver-only node has no ConfigDb (Program.cs gates it on the same env var) yet saw an empty + // role list here and sailed through on Direct, to read its configuration from nothing. + var roles = RoleParser.ResolveEffectiveRoles( + _configuration.GetSection("Cluster:Roles").Get()); + var isDriver = Array.IndexOf(roles, RoleParser.Driver) >= 0; + var isAdmin = Array.IndexOf(roles, RoleParser.Admin) >= 0; if (isDriver && !isAdmin && isDirect) { diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs index 1421543a..789a0084 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs @@ -23,15 +23,110 @@ public static class RoleParser /// Returns true when is a cluster-scoped role — starts with /// and carries a non-empty ClusterId suffix. + /// + /// The prefix test is case-insensitive (Gitea #532). It was ordinal, which meant a + /// Cluster:Roles entry spelled Cluster-SITE-A — that path binds verbatim, unlike + /// the path, which lower-cases — classified as an ordinary role. The node + /// then resolved a ClusterId while still gossiping a cluster-scoped role, + /// so it looked split-topology to its peers and un-scoped to itself. + /// pre-normalized for exactly this reason; + /// ClusterRoleInfo did not, and got the null. + /// /// The role name to test. public static bool IsClusterRole(string role) => - role.StartsWith(ClusterRolePrefix, StringComparison.Ordinal) && role.Length > ClusterRolePrefix.Length; + role is not null + && role.StartsWith(ClusterRolePrefix, StringComparison.OrdinalIgnoreCase) + && role.Length > ClusterRolePrefix.Length; /// Extracts the ClusterId from a cluster-scoped role, e.g. cluster-SITE-A → SITE-A. /// Assumes is true for . + /// + /// The suffix keeps the operator's own spelling. It is NOT case-normalized here because the two + /// role sources disagree by construction — Cluster:Roles binds verbatim + /// (cluster-SITE-A ⇒ SITE-A) while lower-cases the + /// OTOPCUA_ROLES path (⇒ site-a) — and normalizing one side would silently rename + /// the Akka role of a running deployment (docker-dev's rig configures cluster-MAIN / + /// cluster-SITE-A verbatim today). Every consumer of a ClusterId therefore compares it + /// case-insensitively; see the identity rule on AclScopeRef (Gitea #533). + /// /// The cluster-scoped role name. public static string ClusterIdFromRole(string role) => role[ClusterRolePrefix.Length..]; + /// + /// The cluster-scoped roles in , in the order given. More than one is a + /// misconfiguration the caller is expected to report — a node belongs to exactly one mesh. + /// + /// The node's roles. + /// The cluster-scoped roles, original spelling preserved. + public static IReadOnlyList ClusterRolesIn(IEnumerable? roles) => + roles is null ? Array.Empty() : roles.Where(IsClusterRole).ToArray(); + + /// The first cluster-scoped role in , or null when there is none. + /// The node's roles. + /// The cluster role, e.g. cluster-SITE-A. + public static string? ClusterRoleFromRoles(IEnumerable? roles) => + roles?.FirstOrDefault(IsClusterRole); + + /// + /// The ClusterId this node belongs to, derived from its own roles; null when it carries no + /// cluster-scoped role (a legacy / single-mesh node). + /// + /// + /// The single home for a derivation that was open-coded at four call sites before Gitea #532 + /// (ClusterRoleInfo, the ControlPlane singleton scope, SplitTopologyTransportValidator + /// and ConfigDbApiKeyStore) — each with its own view of whether the classification was + /// case-sensitive, which is how the mixed-case gap above survived in three of the four. + /// + /// The node's roles. + /// The ClusterId, or . + public static string? ClusterIdFromRoles(IEnumerable? roles) + { + var role = ClusterRoleFromRoles(roles); + return role is null ? null : ClusterIdFromRole(role); + } + + /// + /// Resolves a node's effective Akka roles: Cluster:Roles when it is non-empty, + /// otherwise the OTOPCUA_ROLES environment variable through . + /// + /// + /// + /// ⚠️ This fallback was documented for a year before it existed (Gitea #532). + /// AkkaClusterOptions.Roles' own XML doc and + /// both described it; only the validator implemented it, privately. A node configured solely + /// through OTOPCUA_ROLES — which drives every bit of Program.cs's conditional + /// wiring, so it reads as the authoritative knob — joined the cluster carrying no Akka + /// roles at all. It ran, served, and logged nothing wrong, while central reported "no + /// driver-role nodes are Up" and every deployment to it timed out. That is not hypothetical: + /// see docs/deployments/wonder-app-vd03-makino-z-34184.md, where the field fix was to + /// hand-add Cluster__Roles__0/1 to a Windows service's registry environment. + /// + /// + /// Configured wins outright — a stray OTOPCUA_ROLES can never widen an explicit + /// Cluster:Roles. The two sources normalize differently on purpose (see + /// ), so a redundant pair should be configured through the + /// same one. + /// + /// + /// 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); + } + + /// Reads the effective roles for the current process, resolving the environment itself. + /// The bound Cluster:Roles value. + /// The effective roles. + public static string[] ResolveEffectiveRoles(IReadOnlyList? configuredRoles) => + ResolveEffectiveRoles(configuredRoles, Environment.GetEnvironmentVariable(RolesEnvVar)); + + /// The environment variable the role list falls back to when Cluster:Roles is empty. + public const string RolesEnvVar = "OTOPCUA_ROLES"; + /// Parses a comma-separated string of role names into a validated array. /// The raw role string to parse. /// The distinct, lower-cased, validated role names. diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs index 1dad8d14..a8d09b08 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/ServiceCollectionExtensions.cs @@ -34,6 +34,22 @@ 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. + // + // 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)); + // 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 // produces silence, not an error, and silence is indistinguishable from "nothing to do". diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/SplitTopologyTransportValidator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/SplitTopologyTransportValidator.cs index 8d400ef8..e9a80a95 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/SplitTopologyTransportValidator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/SplitTopologyTransportValidator.cs @@ -61,17 +61,17 @@ public sealed class SplitTopologyTransportValidator : IValidateOptions() ?? Array.Empty(); - var effective = configured.Length > 0 - ? configured - : RoleParser.Parse(Environment.GetEnvironmentVariable("OTOPCUA_ROLES")); + var effective = RoleParser.ResolveEffectiveRoles(configured); var normalized = Array.ConvertAll(effective, r => r.Trim().ToLowerInvariant()); diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs index b9fc6d9a..d155d1cb 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Cluster/TelemetryOptions.cs @@ -118,8 +118,12 @@ public sealed class TelemetryOptionsValidator : IValidateOptions() ?? Array.Empty(); - var isDriver = Array.IndexOf(roles, "driver") >= 0; + // Through ResolveEffectiveRoles so the OTOPCUA_ROLES fallback applies (Gitea #532) — a node + // configured only through the env var is still a driver node and must satisfy the Grpc-mode + // requirements below. + var roles = RoleParser.ResolveEffectiveRoles( + _configuration.GetSection("Cluster:Roles").Get()); + var isDriver = Array.IndexOf(roles, RoleParser.Driver) >= 0; if (isGrpc) { diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs index 3f44056f..20e6b671 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Configuration/Validation/DraftValidator.cs @@ -71,10 +71,16 @@ public static class DraftValidator /// both places — the accepted cost of not inverting the project graph for one gate. /// /// - /// Existence is checked with the same ordinal, untrimmed comparison + /// Existence is checked with the same case-insensitive, untrimmed comparison /// AclScopePathResolver's dictionary lookups use, so a row this rule passes is a row that /// resolver resolves. The realm check mirrors NodeAclRealms.TryCanonicalize instead - /// (trimmed, case-insensitive) for the same reason. + /// (trimmed, case-insensitive) for the same reason. Both this rule and the resolver were + /// ordinal until Gitea #533; the cross-cluster comparison below was ordinal too, which was the + /// worse half — it could reject a deploy as cross-cluster over a ClusterId the runtime + /// would have matched, and the two spellings genuinely occur (see + /// RoleParser.ClusterIdFromRole). The rule now is: identifiers compare + /// OrdinalIgnoreCase, path segments compare Ordinal; it is stated in full on + /// AclScopeRef. /// /// /// Containment is checked as well as existence. A grant in cluster A naming an entity that @@ -115,8 +121,8 @@ public static class DraftValidator static Dictionary Link( IReadOnlyList rows, Func key, Func value) => - rows.GroupBy(key, StringComparer.Ordinal) - .ToDictionary(g => g.Key, g => value(g.First()), StringComparer.Ordinal); + rows.GroupBy(key, StringComparer.OrdinalIgnoreCase) + .ToDictionary(g => g.Key, g => value(g.First()), StringComparer.OrdinalIgnoreCase); // Owning cluster, or null when a link in the chain is dangling. Direct-ClusterId kinds bottom out // immediately; the rest hop through their parent. @@ -240,7 +246,7 @@ public static class DraftValidator continue; } - if (!string.Equals(owner, acl.ClusterId, StringComparison.Ordinal)) + if (!string.Equals(owner, acl.ClusterId, StringComparison.OrdinalIgnoreCase)) errors.Add(new("NodeAclScopeCrossCluster", $"NodeAcl '{id}' is a grant in cluster '{acl.ClusterId}' but is scoped at {acl.ScopeKind} " + $"'{scopeId}', which belongs to cluster '{owner}'. The id is real, so the grant would resolve " + diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclScopePathResolver.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclScopePathResolver.cs index 9b0d04f2..0c8914fc 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclScopePathResolver.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/AclScopePathResolver.cs @@ -8,15 +8,57 @@ namespace ZB.MOM.WW.OtOpcUa.Core.Authorization; /// Identifies a scope for path lookup. Keyed on the kind as well as the id. /// /// -/// The kind is part of the key because NodeAcl.ScopeId is a bare string with no foreign key -/// and no type discriminator of its own — ids are prefixed by convention (RF…, EQ…) -/// but nothing enforces it. Keying on the id alone would let a DeviceId that happened to -/// equal a UnsAreaId hand a grant the other entity's paths, which is a silent -/// wrong-node authorization rather than a visible error. +/// +/// The kind is part of the key because NodeAcl.ScopeId is a bare string with no foreign key +/// and no type discriminator of its own — ids are prefixed by convention (RF…, EQ…) +/// but nothing enforces it. Keying on the id alone would let a DeviceId that happened to +/// equal a UnsAreaId hand a grant the other entity's paths, which is a silent +/// wrong-node authorization rather than a visible error. +/// +/// +/// The string-identity rule for the whole authorization path (Gitea #533), stated here because +/// this type is where it was first violated: +/// +/// +/// +/// Identifiers compare OrdinalIgnoreCase — ScopeId, ClusterId, +/// Principal. They are opaque handles, not addresses; two spellings of one id always +/// mean the same entity, and the sources genuinely differ in case (a ClusterId arrives +/// lower-cased through OTOPCUA_ROLES and verbatim through Cluster:Roles — see +/// RoleParser.ClusterIdFromRole — and a ScopeId is operator free-text in the ACL +/// editor until the stage-7 picker lands). +/// +/// +/// Path segments compare Ordinal — every level of , +/// and therefore PermissionTrieNode.Children. They are OPC UA browse names, where +/// Speed and speed are two different nodes that can coexist under one device +/// (RawPaths.Comparer is ordinal, so raw sibling-name uniqueness is too). +/// +/// +/// +/// Before #533 this record struct used the compiler's default equality, i.e. ordinal, while the +/// trie builder filtered ClusterId and hashed principals OrdinalIgnoreCase — three +/// rules in one path. It worked only because the resolver and the builder were handed the same +/// NodeAcl instances, so the ScopeId strings were literally the same object. Feed +/// the two sides from different snapshots — which is the entire reason this resolver is +/// input-shape agnostic, EF on the gate and artifact JSON on the node — and any case drift drops +/// every grant on that scope. Fail-closed and diagnosed, but a silent loss of authority. +/// /// /// The authored scope kind. /// The authored scope id. -public readonly record struct AclScopeRef(NodeAclScopeKind Kind, string ScopeId); +public readonly record struct AclScopeRef(NodeAclScopeKind Kind, string ScopeId) +{ + /// Equality over the kind plus a case-insensitive id — see the rule on the type. + /// The other reference. + /// when both name the same scope. + public bool Equals(AclScopeRef other) => + Kind == other.Kind && string.Equals(ScopeId, other.ScopeId, StringComparison.OrdinalIgnoreCase); + + /// + public override int GetHashCode() => + HashCode.Combine(Kind, StringComparer.OrdinalIgnoreCase.GetHashCode(ScopeId ?? string.Empty)); +} /// /// Produces the scopePaths lookup consumes: for each @@ -88,21 +130,29 @@ public sealed class AclScopePathResolver ArgumentNullException.ThrowIfNull(equipment); ArgumentNullException.ThrowIfNull(references); - _folders = folders; - _drivers = drivers; - _devices = devices; - _groups = groups; - _tags = tags; - _areas = areas; - _lines = lines; - _equipment = equipment; + // Every map is re-keyed OrdinalIgnoreCase rather than used as handed in. Ids are identifiers, so + // they follow the identifier half of the rule on AclScopeRef (Gitea #533) — and doing it HERE, + // rather than documenting a comparer contract for callers, is what makes the rule impossible to + // get wrong from the outside: the two producers (EF entities at the deploy gate, artifact JSON on + // the node) build these dictionaries independently and neither can now diverge from the other. + _folders = ById(folders); + _drivers = ById(drivers); + _devices = ById(devices); + _groups = ById(groups); + _tags = ById(tags); + _areas = ById(areas); + _lines = ById(lines); + _equipment = ById(equipment); _references = references; + // The projected group map goes in with the same comparer, so a raw ACL path and the RawPath that + // forms the node's NodeId still agree segment-for-segment — RawPathResolver emits each entity's + // OWN name, so a case-insensitive id lookup widens which chains resolve and never what they spell. _rawPaths = new RawPathResolver( - folders, - drivers, - devices, - groups.ToDictionary(kv => kv.Key, kv => (kv.Value.ParentId, kv.Value.Name), StringComparer.Ordinal)); + _folders, + _drivers, + _devices, + ById(groups.ToDictionary(kv => kv.Key, kv => (kv.Value.ParentId, kv.Value.Name)))); } /// @@ -172,8 +222,10 @@ public sealed class AclScopePathResolver return TryGroupSegments(scopeId) is { } groupSegs ? [RawPath(groupSegs)] : []; // Cluster never reaches here (the caller short-circuits it), and FolderSegment is the retired - // v2 kind with no v3 entity behind it — deliberately unresolvable, so an operator who still - // has such a row gets a diagnostic naming it rather than a grant that quietly does nothing. + // v2 kind with no v3 entity behind it — deliberately unresolvable. The trie builder now + // short-circuits FolderSegment too, so it reports RetiredScopeKind rather than the + // indistinguishable-from-a-typo UnresolvedScope (Gitea #533); this arm stays as the answer + // for a direct ResolveOne caller. case NodeAclScopeKind.Cluster: case NodeAclScopeKind.FolderSegment: default: @@ -197,7 +249,7 @@ public sealed class AclScopePathResolver // grant has to cover all four or it is bypassable through whichever one it missed. foreach (var reference in _references) { - if (!string.Equals(reference.TagId, tagId, StringComparison.Ordinal)) continue; + if (!string.Equals(reference.TagId, tagId, StringComparison.OrdinalIgnoreCase)) continue; if (TryEquipmentSegments(reference.EquipmentId) is not { } equipSegs) continue; paths.Add(UnsPath([.. equipSegs, reference.EffectiveName])); } @@ -276,6 +328,17 @@ public sealed class AclScopePathResolver return [.. deviceSegs, .. chain]; } + /// Re-keys an id map under the identifier comparer — see the rule on . + private static Dictionary ById(IReadOnlyDictionary source) + { + var copy = new Dictionary(source.Count, StringComparer.OrdinalIgnoreCase); + // Last-wins on a case-collision, matching the GroupBy/First tolerance the deploy gate applies to + // duplicate ids: a duplicate is another rule's business, and throwing here would fault the whole + // decode over one malformed row. + foreach (var (key, value) in source) copy[key] = value; + return copy; + } + // No params-array overloads here: string[] binds to both params and IEnumerable, and the resulting // ambiguity would be resolved silently by overload rules rather than by intent. private static NodeAclPath RawPath(IEnumerable segments) => diff --git a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrieBuilder.cs b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrieBuilder.cs index 003974c4..77b6726e 100644 --- a/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrieBuilder.cs +++ b/src/Core/ZB.MOM.WW.OtOpcUa.Core/Authorization/PermissionTrieBuilder.cs @@ -69,6 +69,18 @@ public static class PermissionTrieBuilder continue; } + // Ahead of the ScopeId checks, mirroring the deploy gate's ordering (DraftValidator's + // NodeAclScopeKindRetired): the retirement is the actionable fact, and a row carrying this + // kind can never resolve whatever its ScopeId, so reporting it as "unresolved" would send an + // operator hunting for a typo in an id that was never the problem. + if (row.ScopeKind == NodeAclScopeKind.FolderSegment) + { + diagnostic?.Invoke(new PermissionTrieBuildDiagnostic( + row.NodeAclId, row.ScopeKind, row.ScopeId, + PermissionTrieBuildDiagnosticReason.RetiredScopeKind)); + continue; + } + // A sub-cluster row with no ScopeId names nothing. It used to vanish here with no signal at // all — unlike an unresolvable ScopeId, which at least emitted a diagnostic. Both are // authoring mistakes and both now say so. @@ -151,13 +163,23 @@ public enum PermissionTrieBuildDiagnosticReason { /// /// The row's ScopeId did not resolve to any node in the deployed address space — a dangling - /// id, a deleted entity, or a scope kind with no v3 meaning (). + /// id, or an entity someone deleted after the grant was authored. The remedy is to correct the id + /// or delete the grant. /// UnresolvedScope, /// The row is narrower than but carries no ScopeId. MissingScopeId, + /// + /// The row's is retired and has no v3 entity behind it + /// (), so it can never resolve whatever its + /// ScopeId. Split out of by Gitea #533: the two have + /// different remedies — re-scope the grant to versus + /// fix an id — and conflating them told an operator with a legacy row to go looking for a typo. + /// + RetiredScopeKind, + /// /// A resolved path was empty or did not begin with a realm segment, so it addresses a trie node /// the walk cannot reach. This is a bug in the scope-path producer, not an authoring mistake. diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs index 81a2f973..08cf21e9 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.ControlPlane/ServiceCollectionExtensions.cs @@ -231,7 +231,7 @@ public static class ServiceCollectionExtensions // on the fused central node). AkkaClusterOptions is pure configuration, available before the // cluster forms — the same source ClusterRoleInfo.ClusterRole itself derives from — so the // scope is identical. First cluster role wins, in configuration order (matching ClusterRoleInfo). - var clusterRole = clusterOptions.Roles.FirstOrDefault(RoleParser.IsClusterRole); + var clusterRole = RoleParser.ClusterRoleFromRoles(clusterOptions.Roles); return new ClusterSingletonOptions { Role = clusterRole ?? RoleParser.Driver }; } } diff --git a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs index 0b3456ad..519f8525 100644 --- a/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs +++ b/src/Server/ZB.MOM.WW.OtOpcUa.Host/OpcUa/ConfigDbApiKeyStore.cs @@ -61,8 +61,12 @@ public sealed class ConfigDbApiKeyStore : IApiKeyStore /// /// /// This is the same derivation performs for - /// IClusterRoleInfo.ClusterId, reusing the same primitives — - /// first cluster-scoped role wins, as it does there. + /// IClusterRoleInfo.ClusterId — literally so as of Gitea #532, which collapsed four + /// open-coded copies into . The copies had drifted: + /// three classified the cluster role case-sensitively and one did not, and only one applied + /// the OTOPCUA_ROLES fallback. On a node that hit either gap this method returned + /// , and a null ClusterId applies no filter at all — so the + /// cross-cluster over-grant this store exists to prevent failed open. /// /// /// ⚠️ Takes (pure configuration), NOT @@ -79,8 +83,7 @@ public sealed class ConfigDbApiKeyStore : IApiKeyStore { ArgumentNullException.ThrowIfNull(options); - var clusterRole = options.Roles.FirstOrDefault(RoleParser.IsClusterRole); - return clusterRole is null ? null : RoleParser.ClusterIdFromRole(clusterRole); + return RoleParser.ClusterIdFromRoles(options.Roles); } /// diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsRoleFallbackTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsRoleFallbackTests.cs new file mode 100644 index 00000000..90ecdb3c --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsRoleFallbackTests.cs @@ -0,0 +1,97 @@ +using Microsoft.Extensions.Configuration; +using Microsoft.Extensions.DependencyInjection; +using Microsoft.Extensions.Options; +using Shouldly; +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; + +/// +/// Gitea #532 — AkkaClusterOptions.Roles must actually apply the OTOPCUA_ROLES fallback +/// its own XML doc has always described. +/// +/// +/// +/// These go through AddOtOpcUaCluster + a real service provider rather than calling +/// RoleParser.ResolveEffectiveRoles directly, because the defect was never in the parsing — +/// it was that nothing called it on the way to the bound options. A unit test of the +/// helper alone would have passed against the broken tree. +/// +/// +/// The consequence being pinned: BuildClusterOptions feeds Roles straight to Akka as +/// the node's gossiped member roles. Empty there means a node that joins, comes Up, serves OPC UA +/// — and is invisible to RoleLeader, to MembersWithRole("driver"), to every +/// role-scoped singleton, and to ConfigPublishCoordinator's expected-ack set. The field +/// symptom was "no driver-role nodes are Up" with nothing wrong in the node's own logs +/// (docs/deployments/wonder-app-vd03-makino-z-34184.md). +/// +/// +[Collection(RoleEnvironmentCollection.Name)] +public class AkkaClusterOptionsRoleFallbackTests +{ + private static AkkaClusterOptions Bind(params (string Key, string Value)[] settings) + { + var configuration = new ConfigurationBuilder() + .AddInMemoryCollection(settings.ToDictionary(s => s.Key, s => (string?)s.Value)) + .Build(); + + var services = new ServiceCollection(); + services.AddLogging(); + services.AddOtOpcUaCluster(configuration); + + return services.BuildServiceProvider().GetRequiredService>().Value; + } + + [Fact] + public void Roles_fall_back_to_the_environment_when_Cluster_Roles_is_empty() + { + using (new EnvVarScope(RoleParser.RolesEnvVar, "admin,driver")) + { + Bind().Roles.ShouldBe(new[] { "admin", "driver" }); + } + } + + [Fact] + public void Akka_member_roles_carry_the_fallback() + { + // The end that matters. This is the value handed to Akka.Cluster.Hosting, so an empty array here + // is a role-less member — the exact production shape #532 describes. + using (new EnvVarScope(RoleParser.RolesEnvVar, "driver")) + { + ServiceCollectionExtensions.BuildClusterOptions(Bind()).Roles.ShouldBe(new[] { "driver" }); + } + } + + [Fact] + public void Configured_roles_win_over_the_environment() + { + // Negative control for the fallback: without this, a PostConfigure that unconditionally + // overwrote Roles from the environment would pass the test above and silently ignore every + // deployment's explicit Cluster:Roles. + using (new EnvVarScope(RoleParser.RolesEnvVar, "admin,driver,cluster-SITE-B")) + { + Bind(("Cluster:Roles:0", "driver"), ("Cluster:Roles:1", "cluster-SITE-A")).Roles + .ShouldBe(new[] { "driver", "cluster-SITE-A" }); + } + } + + [Fact] + public void No_roles_from_either_source_stays_empty() + { + using (new EnvVarScope(RoleParser.RolesEnvVar, null)) + { + Bind().Roles.ShouldBeEmpty(); + } + } + + [Fact] + public void The_cluster_role_from_the_environment_reaches_the_ClusterId_derivation() + { + // The over-grant fix in ConfigDbApiKeyStore reads its ClusterId off these options, and a null + // ClusterId applies no cluster filter at all — so an env-only node failed OPEN before #532. + using (new EnvVarScope(RoleParser.RolesEnvVar, "driver,cluster-SITE-A")) + { + RoleParser.ClusterIdFromRoles(Bind().Roles).ShouldBe("site-a"); + } + } +} 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 419f522d..27b97d2a 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ConfigSourceOptionsValidatorTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/ConfigSourceOptionsValidatorTests.cs @@ -10,6 +10,7 @@ namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; /// applies a deployment, and the deploy times out naming a node that is otherwise healthy. These /// tests make that misconfiguration a loud host-start failure instead. /// +[Collection(RoleEnvironmentCollection.Name)] public class ConfigSourceOptionsValidatorTests { private static ValidateOptionsResult Validate(ConfigSourceOptions o, params string[] roles) diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EnvVarScope.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EnvVarScope.cs new file mode 100644 index 00000000..2c626175 --- /dev/null +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EnvVarScope.cs @@ -0,0 +1,44 @@ +using Xunit; + +namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; + +/// Sets an environment variable for the scope's lifetime and restores its prior value on dispose. +public sealed class EnvVarScope : IDisposable +{ + private readonly string _name; + private readonly string? _previous; + + /// Sets to until disposed. + /// The environment variable name. + /// The value to set, or null to unset. + public EnvVarScope(string name, string? value) + { + _name = name; + _previous = Environment.GetEnvironmentVariable(name); + Environment.SetEnvironmentVariable(name, value); + } + + /// + public void Dispose() => Environment.SetEnvironmentVariable(_name, _previous); +} + +/// +/// Serializes every test class that reads or writes the OTOPCUA_ROLES process environment. +/// +/// +/// ⚠️ Load-bearing since Gitea #532. Environment variables are process-global while xUnit runs +/// test classes in an assembly concurrently, so a class that sets OTOPCUA_ROLES is +/// visible to every class running beside it. That was harmless while +/// SplitTopologyTransportValidator was the only code applying the fallback — its own tests both +/// set and read the variable. It stopped being harmless when ConfigSourceOptionsValidator and +/// TelemetryOptionsValidator started resolving effective roles the same way: their cases that +/// pass NO roles would intermittently observe another class's driver,cluster-SITE-A and change +/// verdict. Any test touching that variable, directly or through +/// RoleParser.ResolveEffectiveRoles, belongs in this collection. +/// +[CollectionDefinition(Name, DisableParallelization = true)] +public sealed class RoleEnvironmentCollection +{ + /// The collection name to put on [Collection]. + public const string Name = "role-environment"; +} diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleParserTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleParserTests.cs index ceac246b..1c7804f7 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleParserTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/RoleParserTests.cs @@ -89,6 +89,12 @@ public sealed class RoleParserTests [InlineData("driver", false)] [InlineData("admin", false)] [InlineData("clusterx", false)] + // Gitea #532: the prefix test is case-insensitive. Cluster:Roles binds verbatim, so an operator's + // 'Cluster-SITE-A' used to classify as an ordinary role — the node then gossiped a cluster-scoped + // role while resolving a null ClusterId for itself. + [InlineData("Cluster-SITE-A", true)] + [InlineData("CLUSTER-main", true)] + [InlineData("Cluster-", false)] public void IsClusterRole_classifies_correctly(string role, bool expected) { RoleParser.IsClusterRole(role).ShouldBe(expected); @@ -101,6 +107,65 @@ public sealed class RoleParserTests RoleParser.ClusterIdFromRole("cluster-SITE-A").ShouldBe("SITE-A"); } + /// The suffix keeps the operator's spelling — consumers compare ClusterIds case-insensitively. + [Fact] + public void ClusterIdFromRole_preserves_the_authored_case() + { + // Normalizing here would silently rename the Akka role of a running deployment (the docker-dev + // rig configures cluster-MAIN / cluster-SITE-A verbatim), so the case is carried, not folded. + RoleParser.ClusterIdFromRole("Cluster-SITE-A").ShouldBe("SITE-A"); + } + + /// Verifies the ClusterId derivation picks the first cluster role, in configuration order. + [Fact] + public void ClusterIdFromRoles_takes_the_first_cluster_role() + { + RoleParser.ClusterIdFromRoles(["driver", "cluster-SITE-A", "cluster-SITE-B"]).ShouldBe("SITE-A"); + RoleParser.ClusterRoleFromRoles(["driver", "cluster-SITE-A"]).ShouldBe("cluster-SITE-A"); + RoleParser.ClusterRolesIn(["driver", "cluster-SITE-A", "cluster-SITE-B"]).Count.ShouldBe(2); + } + + /// Verifies a node with no cluster-scoped role derives a null ClusterId rather than throwing. + [Fact] + public void ClusterIdFromRoles_is_null_for_a_legacy_node() + { + RoleParser.ClusterIdFromRoles(["admin", "driver"]).ShouldBeNull(); + RoleParser.ClusterIdFromRoles([]).ShouldBeNull(); + RoleParser.ClusterIdFromRoles(null).ShouldBeNull(); + } + + /// Verifies the mixed-case derivation gap #532 named: classification folds case, the id does not. + [Fact] + public void ClusterIdFromRoles_resolves_a_mixed_case_cluster_role() + { + // Before #532 this returned null, and a null ClusterId applies NO cluster filter — so + // ConfigDbApiKeyStore's cross-cluster guard failed open on exactly this configuration. + RoleParser.ClusterIdFromRoles(["driver", "Cluster-SITE-A"]).ShouldBe("SITE-A"); + } + + /// Verifies Cluster:Roles wins outright over the environment fallback. + [Fact] + public void ResolveEffectiveRoles_prefers_the_configured_list() + { + RoleParser.ResolveEffectiveRoles(["admin"], "driver,cluster-SITE-A").ShouldBe(new[] { "admin" }); + } + + /// Verifies an empty or absent Cluster:Roles falls back to OTOPCUA_ROLES. + [Fact] + public void ResolveEffectiveRoles_falls_back_to_the_environment() + { + RoleParser.ResolveEffectiveRoles([], "driver,cluster-SITE-A") + .ShouldBe(new[] { "driver", "cluster-site-a" }); + RoleParser.ResolveEffectiveRoles(null, "admin").ShouldBe(new[] { "admin" }); + } + + /// Verifies neither source naming a role yields an empty list rather than a null or a throw. + [Fact] + public void ResolveEffectiveRoles_with_neither_source_is_empty() + { + RoleParser.ResolveEffectiveRoles(null, null).ShouldBeEmpty(); + } + /// Verifies the three well-known role constants match the historical literal values. [Fact] public void Well_known_role_constants() 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 b873db2e..6d5b018c 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/SplitTopologyTransportValidatorTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/SplitTopologyTransportValidatorTests.cs @@ -12,6 +12,7 @@ namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; /// that misconfiguration a loud host-start failure instead. A node with no cluster-role is exempt /// (legacy / single-mesh / test), and the validator is a no-op for it. /// +[Collection(RoleEnvironmentCollection.Name)] public class SplitTopologyTransportValidatorTests { private static ValidateOptionsResult Validate( @@ -248,20 +249,4 @@ public class SplitTopologyTransportValidatorTests telemetryDialMode: null, "cluster-SITE-A").Succeeded.ShouldBeTrue(); } - - /// Sets an environment variable for the scope's lifetime and restores its prior value on dispose. - private sealed class EnvVarScope : IDisposable - { - private readonly string _name; - private readonly string? _previous; - - public EnvVarScope(string name, string? value) - { - _name = name; - _previous = Environment.GetEnvironmentVariable(name); - Environment.SetEnvironmentVariable(name, value); - } - - public void Dispose() => Environment.SetEnvironmentVariable(_name, _previous); - } } 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 c55bd59a..d4bb9e14 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/TelemetryOptionsValidatorTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/TelemetryOptionsValidatorTests.cs @@ -10,6 +10,7 @@ namespace ZB.MOM.WW.OtOpcUa.Cluster.Tests; /// telemetry, and whoever is watching sees nothing with no stack trace to point at. These tests /// make that misconfiguration a loud host-start failure instead. /// +[Collection(RoleEnvironmentCollection.Name)] public class TelemetryOptionsValidatorTests { private static ValidateOptionsResult Validate(TelemetryOptions o, params string[] roles) @@ -112,6 +113,7 @@ public class TelemetryOptionsValidatorTests /// /// Mirrors for the central dial-side options. /// +[Collection(RoleEnvironmentCollection.Name)] public class TelemetryDialOptionsValidatorTests { private static ValidateOptionsResult Validate(TelemetryDialOptions o) => diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorNodeAclScopeTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorNodeAclScopeTests.cs index 5d6e3b66..6934a57d 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorNodeAclScopeTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Configuration.Tests/DraftValidatorNodeAclScopeTests.cs @@ -245,6 +245,46 @@ public sealed class DraftValidatorNodeAclScopeTests errors.Where(e => e.Code.StartsWith("NodeAcl", StringComparison.Ordinal)).ShouldHaveSingleItem(); } + /// + /// Gitea #533 — the gate's own identifier comparisons follow the same rule as the resolver's: + /// identifiers compare OrdinalIgnoreCase. Both were ordinal, which put the gate and the + /// runtime into disagreement in the direction that costs the most: the deploy gate would reject a + /// whole configuration as cross-cluster over a ClusterId case difference that + /// PermissionTrieBuilder — which has always matched case-insensitively — would have accepted. + /// + [Fact] + public void A_case_drifted_ClusterId_is_not_reported_as_cross_cluster() + { + var acl = Acl(NodeAclScopeKind.RawDevice, "dev-1"); + acl.ClusterId = Own.ToUpperInvariant(); + + var errors = DraftValidator.Validate(Draft(acl)); + + errors.ShouldNotContain(e => e.Code == "NodeAclScopeCrossCluster"); + errors.ShouldNotContain(e => e.Code == "NodeAclScopeUnresolved"); + } + + /// + /// The same rule applied to the ScopeId lookups, so a row this gate passes is a row + /// AclScopePathResolver resolves — the property the gate's whole value rests on. A gate + /// stricter than the resolver blocks deploys that would have worked; a gate looser than it lets + /// through the silently-dropped grants it exists to catch. + /// + [Fact] + public void A_case_drifted_ScopeId_resolves_at_the_gate_too() + { + DraftValidator.Validate(Draft(Acl(NodeAclScopeKind.RawDevice, "DEV-1"))) + .ShouldNotContain(e => e.Code.StartsWith("NodeAcl", StringComparison.Ordinal)); + } + + /// Negative control: relaxing case must not stop a genuinely foreign cluster from being caught. + [Fact] + public void A_genuinely_different_ClusterId_is_still_cross_cluster() + { + DraftValidator.Validate(Draft(Acl(NodeAclScopeKind.RawDevice, "dev-2"))) + .ShouldContain(e => e.Code == "NodeAclScopeCrossCluster"); + } + /// The error must name both clusters — which one the grant is in and which one owns the /// entity — because "re-author under the other cluster" and "repoint at the local entity" are different /// fixes and the operator picks between them from this message alone. diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/AclScopePathResolverTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/AclScopePathResolverTests.cs index ac6d5b46..e3bdcb82 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/AclScopePathResolverTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/AclScopePathResolverTests.cs @@ -258,6 +258,66 @@ public sealed class AclScopePathResolverTests resolved.ShouldContainKey(new AclScopeRef(NodeAclScopeKind.Equipment, "EQ-mixer")); } + [Theory] + [InlineData(NodeAclScopeKind.RawFolder, "rf-line1")] + [InlineData(NodeAclScopeKind.RawDevice, "dv-1")] + [InlineData(NodeAclScopeKind.Equipment, "eq-MIXER")] + [InlineData(NodeAclScopeKind.Tag, "TAG-SPEED")] + public void A_case_drifted_scope_id_still_resolves(NodeAclScopeKind kind, string scopeId) + { + // Gitea #533 — the identifier half of the rule. ScopeId is an opaque handle, and it is operator + // free-text in the ACL editor until the stage-7 picker lands, so two spellings always mean one + // entity. Ordinal lookups made a case slip drop EVERY grant on that scope: fail-closed and + // diagnosed, but indistinguishable from a genuine typo and invisible to the author. + Fixture().ResolveOne(kind, scopeId).ShouldNotBeEmpty(); + } + + [Fact] + public void A_case_drifted_scope_id_resolves_to_the_SAME_paths_as_the_authored_one() + { + // Stronger than "resolves to something": the paths must be identical, because the segments are + // entity NAMES and must keep their own spelling. A lookup that folded case into the emitted path + // would produce an ACL path that no longer matches the RawPath forming the node's NodeId. + var drifted = Fixture().ResolveOne(NodeAclScopeKind.RawDevice, "dv-1").ShouldHaveSingleItem(); + var authored = Fixture().ResolveOne(NodeAclScopeKind.RawDevice, "DV-1").ShouldHaveSingleItem(); + + drifted.Segments.ShouldBe(authored.Segments); + drifted.Segments.ShouldBe(["raw", "Plant", "Line1", "ModbusDrv", "dev1"]); + } + + [Fact] + public void A_case_drifted_scope_ref_hits_the_same_dictionary_entry() + { + // The key type's own equality, which is what the trie builder looks the resolver's output up by. + // Resolver and builder can be fed from different snapshots (EF at the deploy gate, artifact JSON + // on the node) — the whole reason this resolver is input-shape agnostic — so their agreement + // cannot rest on the two sides holding the same string instances. + var resolved = Fixture().Resolve([Acl("acl-1", NodeAclScopeKind.Equipment, "EQ-mixer")]); + + resolved.ShouldContainKey(new AclScopeRef(NodeAclScopeKind.Equipment, "eq-MIXER")); + new AclScopeRef(NodeAclScopeKind.Equipment, "EQ-mixer") + .ShouldBe(new AclScopeRef(NodeAclScopeKind.Equipment, "eq-mixer")); + } + + [Fact] + public void Case_insensitive_ids_do_NOT_merge_different_scope_kinds() + { + // Negative control for the widening: relaxing the id comparison must not relax the kind, or the + // wrong-node authorization the composite key exists to prevent comes back through the case door. + new AclScopeRef(NodeAclScopeKind.RawFolder, "dup") + .ShouldNotBe(new AclScopeRef(NodeAclScopeKind.UnsArea, "DUP")); + } + + [Fact] + public void A_case_drifted_tag_id_still_reaches_every_UNS_projection() + { + // The reference scan is a linear compare rather than a dictionary lookup, so it needed the rule + // applied separately — and this is the path where missing it is worst: a Tag grant that resolved + // its raw path but none of its UNS paths would leave the tag writable through the UNS NodeId it + // silently skipped. + Fixture().ResolveOne(NodeAclScopeKind.Tag, "tag-speed").Count.ShouldBe(3); + } + private static NodeAcl Acl(string id, NodeAclScopeKind kind, string? scopeId) => new() { NodeAclId = id, diff --git a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/PermissionTrieBuilderTests.cs b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/PermissionTrieBuilderTests.cs index e78d555e..cd41c6be 100644 --- a/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/PermissionTrieBuilderTests.cs +++ b/tests/Core/ZB.MOM.WW.OtOpcUa.Core.Tests/Authorization/PermissionTrieBuilderTests.cs @@ -349,4 +349,64 @@ public sealed class PermissionTrieBuilderTests diagnostics[0].Reason.ShouldBe(PermissionTrieBuildDiagnosticReason.UnresolvedScope); trie.Root.Grants.ShouldBeEmpty("an empty path list must not become a cluster-wide grant"); } + + /// + /// Gitea #533 — the retired gets its own reason. It + /// used to arrive as , which told + /// an operator holding a legacy v2 row to go hunting for a typo in a ScopeId that was never + /// the problem — the kind itself has no v3 entity behind it and can never resolve. The two have + /// different remedies, so they need different reasons. + /// + [Fact] + public void Build_Retired_FolderSegment_Row_Emits_RetiredScopeKind() + { + var row = Row("cn=ops", NodeAclScopeKind.FolderSegment, "seg-1", NodePermissions.Read); + var diagnostics = new List(); + + var trie = PermissionTrieBuilder.Build( + "c1", 1, [row], new Dictionary>(), diagnostics.Add); + + diagnostics.Count.ShouldBe(1); + diagnostics[0].Reason.ShouldBe(PermissionTrieBuildDiagnosticReason.RetiredScopeKind); + diagnostics[0].ScopeId.ShouldBe("seg-1", "the id is still reported, so the operator can find the row"); + trie.Root.Grants.ShouldBeEmpty("a retired-kind row must not fall through to the cluster root"); + } + + /// + /// A genuinely dangling id must keep reporting . + /// Without this, moving FolderSegment to its own reason could be "achieved" by relabelling + /// every drop, and the split would carry no information. + /// + [Fact] + public void Build_A_Live_Kind_With_A_Dangling_Id_Still_Emits_UnresolvedScope() + { + var row = Row("cn=ops", NodeAclScopeKind.RawFolder, "RF-gone", NodePermissions.Read); + var diagnostics = new List(); + + PermissionTrieBuilder.Build( + "c1", 1, [row], new Dictionary>(), diagnostics.Add); + + diagnostics.ShouldHaveSingleItem().Reason + .ShouldBe(PermissionTrieBuildDiagnosticReason.UnresolvedScope); + } + + /// + /// Gitea #533 — the identifier rule reaches the builder's own ClusterId filter, which has + /// always been OrdinalIgnoreCase. Pinned because the two spellings genuinely occur: a + /// ClusterId arrives lower-cased through OTOPCUA_ROLES and verbatim through + /// Cluster:Roles. + /// + [Fact] + public void Build_Matches_The_Cluster_Case_Insensitively() + { + var rows = new[] { Row("cn=ops", NodeAclScopeKind.Cluster, null, NodePermissions.Read) }; + + PermissionTrieBuilder.Build( + "C1", 1, rows, new Dictionary>()) + .Root.Grants.ShouldHaveSingleItem(); + + PermissionTrieBuilder.Build( + "c2", 1, rows, new Dictionary>()) + .Root.Grants.ShouldBeEmpty("a DIFFERENT cluster must still be excluded"); + } }