From 06c56d77195dae727aa0557cf46297d7c9d79944 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 1 Aug 2026 01:51:22 -0400 Subject: [PATCH] fix(cluster,acl): OTOPCUA_ROLES fallback + one string-identity rule (#532, #533) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two independent defects surfaced in review of #520 stage 3. #532 — Cluster:Roles had no OTOPCUA_ROLES fallback, despite two doc sites promising one. A node configured only through the env var — which drives ALL of Program.cs's conditional wiring, so it reads as the authoritative knob — bound an empty Roles array and 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. Not hypothetical: the field fix on wonder-app-vd03 was to hand-add Cluster__Roles__0/1 to a Windows service's registry environment. Anyone reading the code to diagnose that box would have concluded from the XML doc that the fallback was already applied. AddOtOpcUaCluster now post-configures AkkaClusterOptions through the shared RoleParser.ResolveEffectiveRoles, so BuildClusterOptions (Akka's own member roles), ClusterRoleInfo, the ControlPlane singleton scope and ConfigDbApiKeyStore's ClusterId all see one answer. Two sibling validators (ConfigSource, Telemetry) read Cluster:Roles directly and were exempting the exact node they exist for — an env-only driver-only node has no ConfigDb and could sit on ConfigSource:Mode=Direct unchallenged. They resolve the same way now. Also collapsed the four open-coded `Roles.FirstOrDefault(IsClusterRole)` + ClusterIdFromRole copies into RoleParser.ClusterIdFromRoles. They had drifted: three classified the cluster role ordinally, so a verbatim-bound `Cluster-SITE-A` yielded a null ClusterId — and a null ClusterId applies NO cluster filter, which made #528's cross-cluster over-grant guard fail OPEN. IsClusterRole is now case-insensitive; the ClusterId suffix keeps its authored spelling, because normalizing it would silently rename the Akka role of a running deployment. #533 — three string-identity rules in one authorization path. AclScopeRef used default (ordinal) equality while the trie builder filtered ClusterId and hashed principals OrdinalIgnoreCase; the deploy gate's cross-cluster check was ordinal too, which was the worse half — it could reject a whole deploy over a ClusterId case difference the runtime would have accepted. It only worked because resolver and builder were handed the same NodeAcl instances. The rule is now stated once, on AclScopeRef, and applied throughout: identifiers (ScopeId, ClusterId, Principal) compare OrdinalIgnoreCase; path segments compare Ordinal. AclScopePathResolver re-keys its own maps rather than documenting a comparer contract for callers, so the two independent producers — EF at the gate, artifact JSON on the node — cannot diverge from the outside. PermissionTrieNode.Children stays Ordinal: those are browse names, and Speed / speed are two nodes that can coexist under one device. Split PermissionTrieBuildDiagnosticReason.RetiredScopeKind out of UnresolvedScope. A legacy FolderSegment row and a typo'd id had the same message and different remedies. Falsified: five mutations, each reddening only its own guard — remove the PostConfigure (3 tests), revert IsClusterRole to ordinal (3), revert AclScopeRef equality (1), revert the resolver's map comparer (6), revert the gate's comparisons (2), remove the FolderSegment short-circuit (1). Test isolation: OTOPCUA_ROLES is process-global and xUnit runs classes in parallel, so the three validator suites that now resolve effective roles share a non-parallel collection. Without it, cases passing no roles would intermittently observe another class's env var. Full solution build clean; full suite green apart from two pre-existing failures confirmed identical on 3a6b2a1d (PrimaryGateFailoverTests, and DriverProbeRegistrationTests expecting 10 probe types where 12 now register) plus the AbLegacy integration suite, which needs a docker fixture. Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo --- docs/Configuration.md | 8 +- .../wonder-app-vd03-makino-z-34184.md | 13 +++ docs/security.md | 39 +++++-- .../AkkaClusterOptions.cs | 22 +++- .../ClusterRoleInfo.cs | 16 ++- .../ConfigSourceOptionsValidator.cs | 14 ++- .../ZB.MOM.WW.OtOpcUa.Cluster/RoleParser.cs | 97 +++++++++++++++- .../ServiceCollectionExtensions.cs | 16 +++ .../SplitTopologyTransportValidator.cs | 16 +-- .../TelemetryOptions.cs | 8 +- .../Validation/DraftValidator.cs | 16 ++- .../Authorization/AclScopePathResolver.cs | 105 ++++++++++++++---- .../Authorization/PermissionTrieBuilder.cs | 24 +++- .../ServiceCollectionExtensions.cs | 2 +- .../OpcUa/ConfigDbApiKeyStore.cs | 11 +- .../AkkaClusterOptionsRoleFallbackTests.cs | 97 ++++++++++++++++ .../ConfigSourceOptionsValidatorTests.cs | 1 + .../EnvVarScope.cs | 44 ++++++++ .../RoleParserTests.cs | 65 +++++++++++ .../SplitTopologyTransportValidatorTests.cs | 17 +-- .../TelemetryOptionsValidatorTests.cs | 2 + .../DraftValidatorNodeAclScopeTests.cs | 40 +++++++ .../AclScopePathResolverTests.cs | 60 ++++++++++ .../PermissionTrieBuilderTests.cs | 60 ++++++++++ 24 files changed, 708 insertions(+), 85 deletions(-) create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/AkkaClusterOptionsRoleFallbackTests.cs create mode 100644 tests/Core/ZB.MOM.WW.OtOpcUa.Cluster.Tests/EnvVarScope.cs 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"); + } }