fix(mesh): gate follows the endpoint not the mode + review fixes

The known-cluster gate published only under Mode=Grpc, but the interceptor it feeds is registered on every admin node and 1B maps CentralCommandService by LISTENER PORT — neither consults the mode. A dark-switch rollback to ClusterClient or Dps would therefore have left that endpoint serving with the gate permanently inactive, silently restoring both the timing oracle and the pre-auth secret-store/audit flood, as a side effect of a rollback nobody would connect to authentication. The gate is central's cluster inventory, not transport state, so it is now decoupled from the mode entirely: - SetKnownClusters + InvalidateAll run on EVERY successful contact load in EVERY mode, and the contact-refresh timer starts in all three. Under Dps this is a ConfigDb query every ContactRefreshSeconds that did not run before; central always has a ConfigDb, and one indexed read a minute is the cheaper half of the trade. - The GATE set is built from ALL ClusterNode rows. The eligibility filter moved out of the query's WHERE and into the loop, so the FAN set is byte-for-byte what it was (enabled && !maintenance) while the gate widens. This also dissolves the maintenance coupling flagged in 1A.2: a cluster whose nodes are all disabled or in maintenance can still ack, and refusing it would make maintenance mode an authentication switch. Central still refuses clusters it has never heard of. - HandleContactsLoaded keeps a documented order — targets adopted, gate published, cache dropped — now pinned by test. Guards: PreStart mirrors the null-dialer error for a null PSK provider (silent absence is the worst outcome there — nothing malfunctions, so nothing is investigated, while the gate never publishes and keys never rotate); a failed contact load now says out loud that the gate is stale and, before any load has succeeded, inactive; MeshGrpcCommandDialer clamps a non-positive DispatchTimeoutSeconds to 10 with a construction Warning, because the validator for that key is driver-role-scoped and a pure-admin central reads it unvalidated; the dialer's local PSK refusal was shortened so its remediation survives the actor's 200-char truncation of remote-authored status detail; the portless-node warn-once set is rebuilt each refresh, so a node that regains then loses a port warns again — the regression is the occurrence worth seeing. Docs/pins: dialer disposal comment no longer claims one-way; the two interceptor-test comments claiming Matches has a default implementation corrected (it deliberately has none); new pins for per-refresh invalidation, the SetKnownClusters-before-InvalidateAll order, and one channel per endpoint via a new CachedChannelCount. The_known_cluster_set_is_not_published_by_the_other_two_modes is replaced — its premise is obsolete — by three facts: every mode publishes, gate set is a strict superset of the fan set, and the handler ordering. ControlPlane.Tests 230 -> 236, Host.Tests 92, Cluster.Tests 184, 0 warnings.
2026-08-06 06:44:20 -04:00
parent 6a86201105
commit 48dc2120c6
5 changed files with 368 additions and 84 deletions
@@ -461,6 +461,28 @@ public class CentralCommunicationActorTests : ControlPlaneActorTestBase
        rig.Dialer.Calls[0].Endpoint.ShouldBe("http://host-b:4056");
    }

+    [Fact]
+    public void A_node_that_regains_then_loses_its_GrpcPort_warns_again()
+    {
+        // Warn-once is per continuous skip run, not per process. The suppression exists so a
+        // permanently portless row does not repeat every minute; carried across a repair it would
+        // silence the SECOND time a node loses its endpoint — the occurrence an operator most needs
+        // to see, because it is a regression rather than a known gap.
+        var factory = NewInMemoryDbFactory();
+        SeedGrpcCluster(factory, "C1", ("host-a:4053", "host-a", null, true, false));
+        var rig = SpawnGrpcRig(factory);
+        AwaitContactsLoaded(rig);
+
+        SetGrpcPort(factory, "host-a:4053", 4056);
+        rig.Actor.Tell(new CentralCommunicationActor.RefreshContacts());
+        AwaitCondition(
+            () => rig.Psk.KnownClusterPublications.Count >= 2, TimeSpan.FromSeconds(5));
+
+        SetGrpcPort(factory, "host-a:4053", null);
+        EventFilter.Warning(contains: "has no GrpcPort").ExpectOne(TimeSpan.FromSeconds(5), () =>
+            rig.Actor.Tell(new CentralCommunicationActor.RefreshContacts()));
+    }
+
    [Fact]
    public void One_failing_node_does_not_stop_the_fan_and_is_logged_by_node_id()
    {
@@ -690,29 +712,89 @@ public class CentralCommunicationActorTests : ControlPlaneActorTestBase
        AwaitCondition(() => rig.Dialer.Calls.Count == 2, TimeSpan.FromSeconds(5));
        rig.Dialer.Calls.Select(c => c.Endpoint).ShouldBe(
            new[] { "http://host-a:4056", "http://host-b:4056" }, ignoreOrder: true);
+
+        // Once PER REFRESH, not once at startup. A key can rotate without any membership change, so
+        // an invalidation that only fired when the cluster set moved would serve the pre-rotation
+        // key forever and a rotation would need a central restart.
+        rig.Psk.InvalidateAllCount.ShouldBeGreaterThanOrEqualTo(2);
    }

-    [Fact]
-    public void The_known_cluster_set_is_not_published_by_the_other_two_modes()
+    [Theory]
+    [InlineData(MeshTransportOptions.ModeDps)]
+    [InlineData(MeshTransportOptions.ModeClusterClient)]
+    [InlineData(MeshTransportOptions.ModeGrpc)]
+    public void The_known_cluster_gate_is_published_in_every_transport_mode(string mode)
    {
-        // The gate is a gRPC-command-plane control. Publishing it under Dps or ClusterClient would
-        // start refusing acks on a transport whose ack path never consults the provider at all —
-        // and, worse, would do so from a code path those modes' tests do not cover.
+        // Replaces an earlier fact asserting the OPPOSITE, whose premise did not survive 1B: the
+        // gate feeds CentralCommandAuthInterceptor, which is registered on every admin node, and the
+        // ack service it protects is mapped by LISTENER PORT — neither consults MeshTransport:Mode.
+        // Gating the publication on Grpc meant a dark-switch rollback to ClusterClient or Dps left
+        // that endpoint serving with a permanently inactive gate, silently restoring both the timing
+        // oracle and the pre-authentication store/audit flood. The set is central's cluster
+        // inventory, not transport state.
        var factory = NewInMemoryDbFactory();
-        SeedCluster(factory, "C1", ("host-a:4053", "host-a", 4053, true, false));
+        SeedGrpcCluster(factory, "C1", ("host-a:4053", "host-a", 4056, true, false));
        var psk = new RecordingPskProvider();

        Sys.ActorOf(CentralCommunicationActor.Props(
            factory,
-            ClusterClientMode(),
+            new MeshTransportOptions { Mode = mode, ContactRefreshSeconds = 60 },
            new RecordingClientFactory(() => CreateTestProbe()),
            () => null,
-            dialer: null,
-            pskProvider: psk));
+            new RecordingDialer(),
+            psk));

-        ExpectNoMsg(TimeSpan.FromMilliseconds(500));
-        psk.KnownClusterPublications.ShouldBeEmpty();
-        psk.InvalidateAllCount.ShouldBe(0);
+        AwaitCondition(
+            () => psk.KnownClusterPublications.Count >= 1, TimeSpan.FromSeconds(5));
+        psk.KnownClusterPublications[0].ShouldBe(new[] { "C1" });
+        psk.InvalidateAllCount.ShouldBeGreaterThanOrEqualTo(1);
+    }
+
+    [Fact]
+    public void The_gate_set_includes_a_cluster_the_fan_set_excludes()
+    {
+        // The two sets answer different questions. The FAN asks "who does central talk to" — enabled,
+        // non-maintenance, ported. The GATE asks "is this a cluster central has ever heard of", and
+        // must say yes for a cluster whose every node is disabled or in maintenance: such a cluster
+        // can still send a deployment ack, and refusing it would quietly turn maintenance mode into
+        // an authentication switch.
+        var factory = NewInMemoryDbFactory();
+        SeedGrpcCluster(factory, "C1", ("host-a:4053", "host-a", 4056, true, false));
+        SeedGrpcCluster(factory, "C-DISABLED", ("host-b:4053", "host-b", 4056, false, false));
+        SeedGrpcCluster(factory, "C-MAINT", ("host-c:4053", "host-c", 4056, true, true));
+        var rig = SpawnGrpcRig(factory);
+        AwaitContactsLoaded(rig);
+
+        rig.Psk.KnownClusterPublications[0].ShouldBe(
+            new[] { "C1", "C-DISABLED", "C-MAINT" }, ignoreOrder: true);
+
+        rig.Actor.Tell(new MeshCommand(DeploymentsTopic, NewDispatch()));
+
+        AwaitCondition(() => rig.Dialer.Calls.Count == 1, TimeSpan.FromSeconds(5));
+        ExpectNoMsg(TimeSpan.FromMilliseconds(300));
+        rig.Dialer.Calls.Count.ShouldBe(1);
+        rig.Dialer.Calls[0].ClusterId.ShouldBe("C1");
+    }
+
+    [Fact]
+    public void The_gate_is_published_after_the_dispatch_set_is_adopted_and_before_the_cache_is_dropped()
+    {
+        // Two orderings in one handler, both load-bearing. SetKnownClusters must precede
+        // InvalidateAll, or the first lookup after a refresh re-reads the store under the PREVIOUS
+        // membership — briefly resolving a cluster central has just retired. And the dispatch set is
+        // adopted first so a command processed immediately after the publication already fans over
+        // the current targets.
+        var factory = NewInMemoryDbFactory();
+        SeedGrpcCluster(factory, "C1", ("host-a:4053", "host-a", 4056, true, false));
+        var rig = SpawnGrpcRig(factory);
+        AwaitContactsLoaded(rig);
+
+        rig.Psk.Sequence.ShouldBe(new[] { "SetKnownClusters", "InvalidateAll" });
+
+        // Targets-before-gate, observed the only way it can be from outside the actor: the very next
+        // message finds the dispatch set already populated.
+        rig.Actor.Tell(new MeshCommand(DeploymentsTopic, NewDispatch()));
+        AwaitCondition(() => rig.Dialer.Calls.Count == 1, TimeSpan.FromSeconds(5));
    }

    [Fact]
@@ -738,6 +820,15 @@ public class CentralCommunicationActorTests : ControlPlaneActorTestBase
            () => actor.Tell(new MeshCommand(DeploymentsTopic, NewDispatch())));
    }

+    private static void SetGrpcPort(
+        IDbContextFactory<OtOpcUaConfigDbContext> factory, string nodeId, int? grpcPort)
+    {
+        using var db = factory.CreateDbContext();
+        var node = db.ClusterNodes.Single(n => n.NodeId == nodeId);
+        node.GrpcPort = grpcPort;
+        db.SaveChanges();
+    }
+
    private static DispatchDeployment NewDispatch() => new(
        new DeploymentId(Guid.NewGuid()), new RevisionHash("abc"), new CorrelationId(Guid.NewGuid()));

@@ -839,8 +930,15 @@ public class CentralCommunicationActorTests : ControlPlaneActorTestBase
    {
        private readonly object _gate = new();
        private readonly List<IReadOnlyList<string>> _publications = [];
+        private readonly List<string> _sequence = [];
        private int _invalidateAllCount;

+        /// <summary>Names of the mutating calls made, in the order they arrived.</summary>
+        public IReadOnlyList<string> Sequence
+        {
+            get { lock (_gate) return _sequence.ToList(); }
+        }
+
        /// <summary>Every known-cluster set published, in order.</summary>
        public IReadOnlyList<IReadOnlyList<string>> KnownClusterPublications
        {
@@ -860,11 +958,19 @@ public class CentralCommunicationActorTests : ControlPlaneActorTestBase

        public void SetKnownClusters(IReadOnlyCollection<string> clusterIds)
        {
-            lock (_gate) _publications.Add(clusterIds.ToList());
+            lock (_gate)
+            {
+                _publications.Add(clusterIds.ToList());
+                _sequence.Add(nameof(SetKnownClusters));
+            }
        }

        public void Invalidate(string clusterId) { }

-        public void InvalidateAll() => Interlocked.Increment(ref _invalidateAllCount);
+        public void InvalidateAll()
+        {
+            Interlocked.Increment(ref _invalidateAllCount);
+            lock (_gate) _sequence.Add(nameof(InvalidateAll));
+        }
    }
 }
@@ -1,6 +1,8 @@
 using System.Diagnostics.CodeAnalysis;
 using Grpc.Core;
 using Microsoft.Extensions.DependencyInjection;
+using Microsoft.Extensions.Logging;
+using Microsoft.Extensions.Logging.Abstractions;
 using Microsoft.Extensions.Options;
 using Shouldly;
 using Xunit;
@@ -41,6 +43,7 @@ public sealed class MeshGrpcCommandDialerTests
        => new(
            psk,
            Options.Create(new MeshTransportOptions { DispatchTimeoutSeconds = dispatchTimeoutSeconds }),
+            NullLogger<MeshGrpcCommandDialer>.Instance,
            _ => client);

    [Fact]
@@ -136,17 +139,25 @@ public sealed class MeshGrpcCommandDialerTests
    }

    [Fact]
-    public async Task A_zero_dispatch_timeout_leaves_the_dial_bounded_only_by_the_callers_token()
+    public async Task A_non_positive_dispatch_timeout_is_clamped_rather_than_honoured()
    {
-        // The validator requires > 0 under Mode=Grpc, so this is the misconfigured/other-mode shape.
-        // It must not be read as "expire immediately" — that would drop every command instantly.
-        var client = new RecordingClient();
-        var dialer = CreateDialer(
-            new FakePskProvider(("SITE-A", "psk-site-a")), client, dispatchTimeoutSeconds: 0);
+        // Zero would mean an UNBOUNDED call — the exact failure the deadline exists to prevent — and
+        // MeshTransportOptionsValidator's rule for this key is scoped to driver-role nodes, so a
+        // pure-admin central reaches this code with the value unvalidated. Clamp loudly.
+        var client = new RecordingClient { HangUntilCancelled = true };
+        var logger = new RecordingLogger();
+        var dialer = new MeshGrpcCommandDialer(
+            new FakePskProvider(("SITE-A", "psk-site-a")),
+            Options.Create(new MeshTransportOptions { DispatchTimeoutSeconds = 0 }),
+            logger,
+            _ => client);

-        await dialer.DispatchAsync(Endpoint, Envelope(), "SITE-A", CancellationToken.None);
+        logger.Warnings.ShouldContain(w => w.Contains("DispatchTimeoutSeconds"));

-        client.Calls.ShouldBe(1);
+        // And the clamp is real, not just logged: the dial must still expire on its own.
+        await Should.ThrowAsync<OperationCanceledException>(
+            () => dialer.DispatchAsync(Endpoint, Envelope(), "SITE-A", CancellationToken.None)
+                .WaitAsync(TimeSpan.FromSeconds(20)));
    }

    [Fact]
@@ -169,7 +180,8 @@ public sealed class MeshGrpcCommandDialerTests
        // and Dispose must then clean up the channel the dial created.
        var dialer = new MeshGrpcCommandDialer(
            new FakePskProvider(("SITE-A", "psk-site-a")),
-            Options.Create(new MeshTransportOptions { DispatchTimeoutSeconds = 5 }));
+            Options.Create(new MeshTransportOptions { DispatchTimeoutSeconds = 5 }),
+            NullLogger<MeshGrpcCommandDialer>.Instance);

        await Should.ThrowAsync<RpcException>(
            () => dialer.DispatchAsync(
@@ -178,6 +190,38 @@ public sealed class MeshGrpcCommandDialerTests
        Should.NotThrow(dialer.Dispose);
    }

+    [Fact]
+    public async Task Two_dials_to_one_endpoint_share_a_single_channel()
+    {
+        // The cache is the whole defence against a socket per call: a deploy dispatch fans across
+        // every node in the fleet, and it repeats on every redeploy. A channel per dial exhausts
+        // ports on central long before anything else notices.
+        var dialer = new MeshGrpcCommandDialer(
+            new FakePskProvider(("SITE-A", "psk-site-a")),
+            Options.Create(new MeshTransportOptions { DispatchTimeoutSeconds = 5 }),
+            NullLogger<MeshGrpcCommandDialer>.Instance);
+
+        // Nothing is listening, so both dials fail — irrelevant here: the channel is constructed
+        // before the call is placed, which is exactly what is being counted.
+        for (var i = 0; i < 2; i++)
+        {
+            await Should.ThrowAsync<RpcException>(
+                () => dialer.DispatchAsync(
+                    "http://127.0.0.1:1", Envelope(), "SITE-A", CancellationToken.None));
+        }
+
+        dialer.CachedChannelCount.ShouldBe(1);
+
+        // A second endpoint is a second channel — the cache keys on the endpoint, it does not
+        // collapse every node onto one connection.
+        await Should.ThrowAsync<RpcException>(
+            () => dialer.DispatchAsync(
+                "http://127.0.0.1:2", Envelope(), "SITE-A", CancellationToken.None));
+
+        dialer.CachedChannelCount.ShouldBe(2);
+        Should.NotThrow(dialer.Dispose);
+    }
+
    [Fact]
    public void It_activates_from_the_container_despite_the_optional_client_factory_parameter()
    {
@@ -186,6 +230,7 @@ public sealed class MeshGrpcCommandDialerTests
        // constructor — and the failure is a startup crash on the admin node, invisible to every
        // other test here because they all construct the dialer by hand.
        var services = new ServiceCollection();
+        services.AddLogging();
        services.AddSingleton<IMeshPskProvider>(new FakePskProvider(("SITE-A", "psk-site-a")));
        services.AddSingleton<IOptions<MeshTransportOptions>>(
            Options.Create(new MeshTransportOptions { DispatchTimeoutSeconds = 10 }));
@@ -249,6 +294,28 @@ public sealed class MeshGrpcCommandDialerTests
        }
    }

+    /// <summary>Captures Warning-level messages so a construction-time check is assertable.</summary>
+    private sealed class RecordingLogger : ILogger<MeshGrpcCommandDialer>
+    {
+        /// <summary>Every warning logged, formatted.</summary>
+        public List<string> Warnings { get; } = [];
+
+        public IDisposable? BeginScope<TState>(TState state) where TState : notnull => null;
+
+        public bool IsEnabled(LogLevel logLevel) => true;
+
+        public void Log<TState>(
+            LogLevel logLevel,
+            EventId eventId,
+            TState state,
+            Exception? exception,
+            Func<TState, Exception?, string> formatter)
+        {
+            if (logLevel == LogLevel.Warning)
+                Warnings.Add(formatter(state, exception));
+        }
+    }
+
    /// <summary>An <see cref="IMeshPskProvider"/> over a fixed key set; unknown clusters fail closed.</summary>
    private sealed class FakePskProvider(params (string Cluster, string Psk)[] keys) : IMeshPskProvider
    {
@@ -248,8 +248,8 @@ public sealed class CentralCommandAuthInterceptorTests
        // Phase 1A security register item 2. The interceptor must not fetch the raw key and compare
        // it — that is how a constant-time comparison eventually becomes '=='. Matches owns the
        // comparison; TryGetPsk survives only for the DIAL side, which has to send the key.
-        // (The provider's own default Matches calls TryGetPsk internally; this fake overrides it, so
-        // a zero TryGetPsk count here means the INTERCEPTOR never asked for the key.)
+        // (The production provider's Matches calls TryGetPsk internally; this fake implements the two
+        // independently, so a zero TryGetPsk count here means the INTERCEPTOR never asked for the key.)
        var provider = new FakeMeshPskProvider(("SITE-A", "psk-site-a"));
        var interceptor = new CentralCommandAuthInterceptor(
            provider, NullLogger<CentralCommandAuthInterceptor>.Instance);
@@ -279,8 +279,10 @@ public sealed class CentralCommandAuthInterceptorTests
        }

        /// <summary>
-        ///     Overrides the interface's default so the two calls are separately countable. The
-        ///     semantics are the default's: no key, or a key that is not exactly the presented token.
+        ///     Implemented independently of <see cref="TryGetPsk"/> so the two calls are separately
+        ///     countable — the interface declares this with no default implementation, deliberately.
+        ///     Semantics match the production provider's: no key, or a key that is not exactly the
+        ///     presented token.
        /// </summary>
        public bool Matches(string clusterId, string presented)
        {