From c41cb41c7bfe91cc178109bd6c9d29ca01e2e225 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Mon, 1 Jun 2026 16:46:59 -0400 Subject: [PATCH] fix(scadabridge): default MetricsPort to 8084 (avoid site RemotingPort collision) + validate port distinctness --- docker/site-a-node-a/appsettings.Site.json | 3 +- docker/site-a-node-b/appsettings.Site.json | 3 +- docker/site-b-node-a/appsettings.Site.json | 3 +- docker/site-b-node-b/appsettings.Site.json | 3 +- docker/site-c-node-a/appsettings.Site.json | 3 +- docker/site-c-node-b/appsettings.Site.json | 3 +- src/ZB.MOM.WW.ScadaBridge.Host/NodeOptions.cs | 9 +- src/ZB.MOM.WW.ScadaBridge.Host/Program.cs | 8 +- .../StartupValidator.cs | 15 +++ .../appsettings.Site.json | 1 + .../StartupValidatorTests.cs | 99 +++++++++++++++++++ 11 files changed, 139 insertions(+), 11 deletions(-) diff --git a/docker/site-a-node-a/appsettings.Site.json b/docker/site-a-node-a/appsettings.Site.json index 841b93e1..44f588fe 100644 --- a/docker/site-a-node-a/appsettings.Site.json +++ b/docker/site-a-node-a/appsettings.Site.json @@ -6,7 +6,8 @@ "NodeHostname": "scadabridge-site-a-a", "SiteId": "site-a", "RemotingPort": 8082, - "GrpcPort": 8083 + "GrpcPort": 8083, + "MetricsPort": 8084 }, "Cluster": { "SeedNodes": [ diff --git a/docker/site-a-node-b/appsettings.Site.json b/docker/site-a-node-b/appsettings.Site.json index 1ab9fbb8..7a3a9dec 100644 --- a/docker/site-a-node-b/appsettings.Site.json +++ b/docker/site-a-node-b/appsettings.Site.json @@ -6,7 +6,8 @@ "NodeHostname": "scadabridge-site-a-b", "SiteId": "site-a", "RemotingPort": 8082, - "GrpcPort": 8083 + "GrpcPort": 8083, + "MetricsPort": 8084 }, "Cluster": { "SeedNodes": [ diff --git a/docker/site-b-node-a/appsettings.Site.json b/docker/site-b-node-a/appsettings.Site.json index fda43640..88a7c8af 100644 --- a/docker/site-b-node-a/appsettings.Site.json +++ b/docker/site-b-node-a/appsettings.Site.json @@ -6,7 +6,8 @@ "NodeHostname": "scadabridge-site-b-a", "SiteId": "site-b", "RemotingPort": 8082, - "GrpcPort": 8083 + "GrpcPort": 8083, + "MetricsPort": 8084 }, "Cluster": { "SeedNodes": [ diff --git a/docker/site-b-node-b/appsettings.Site.json b/docker/site-b-node-b/appsettings.Site.json index 451f9650..9334191d 100644 --- a/docker/site-b-node-b/appsettings.Site.json +++ b/docker/site-b-node-b/appsettings.Site.json @@ -6,7 +6,8 @@ "NodeHostname": "scadabridge-site-b-b", "SiteId": "site-b", "RemotingPort": 8082, - "GrpcPort": 8083 + "GrpcPort": 8083, + "MetricsPort": 8084 }, "Cluster": { "SeedNodes": [ diff --git a/docker/site-c-node-a/appsettings.Site.json b/docker/site-c-node-a/appsettings.Site.json index 3af46da7..33b17691 100644 --- a/docker/site-c-node-a/appsettings.Site.json +++ b/docker/site-c-node-a/appsettings.Site.json @@ -6,7 +6,8 @@ "NodeHostname": "scadabridge-site-c-a", "SiteId": "site-c", "RemotingPort": 8082, - "GrpcPort": 8083 + "GrpcPort": 8083, + "MetricsPort": 8084 }, "Cluster": { "SeedNodes": [ diff --git a/docker/site-c-node-b/appsettings.Site.json b/docker/site-c-node-b/appsettings.Site.json index eec984d1..5a69e034 100644 --- a/docker/site-c-node-b/appsettings.Site.json +++ b/docker/site-c-node-b/appsettings.Site.json @@ -6,7 +6,8 @@ "NodeHostname": "scadabridge-site-c-b", "SiteId": "site-c", "RemotingPort": 8082, - "GrpcPort": 8083 + "GrpcPort": 8083, + "MetricsPort": 8084 }, "Cluster": { "SeedNodes": [ diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/NodeOptions.cs b/src/ZB.MOM.WW.ScadaBridge.Host/NodeOptions.cs index bc01cf0c..a4dd0c39 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/NodeOptions.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/NodeOptions.cs @@ -20,6 +20,11 @@ public class NodeOptions public int RemotingPort { get; set; } = 8081; /// Gets or sets the gRPC port for the site stream server. public int GrpcPort { get; set; } = 8083; - /// HTTP/1.1 port serving the Prometheus /metrics scrape endpoint on site nodes. - public int MetricsPort { get; set; } = 8082; + /// + /// HTTP/1.1 port serving the Prometheus /metrics scrape endpoint on site nodes. + /// Defaults to 8084 — deliberately distinct from (8082) + /// and (8083) so the Kestrel metrics listener never contends + /// with the Akka remoting port a site node binds. + /// + public int MetricsPort { get; set; } = 8084; } diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs index c7930a1d..43d32967 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/Program.cs @@ -293,10 +293,12 @@ try // Read GrpcPort from config (NodeOptions already has default 8083) var grpcPort = configuration.GetValue("ScadaBridge:Node:GrpcPort", 8083); - // Read MetricsPort from config (NodeOptions already has default 8082). + // Read MetricsPort from config (NodeOptions already has default 8084). // Separate HTTP/1.1 listener so a standard HTTP/1.1 Prometheus scraper can - // reach /metrics; the gRPC port stays HTTP/2-only below. - var metricsPort = configuration.GetValue("ScadaBridge:Node:MetricsPort", 8082); + // reach /metrics; the gRPC port stays HTTP/2-only below. The default is + // 8084 — distinct from RemotingPort (8082, Akka) and GrpcPort (8083) so the + // metrics listener never collides with the Akka remoting port on site nodes. + var metricsPort = configuration.GetValue("ScadaBridge:Node:MetricsPort", 8084); // Configure Kestrel for HTTP/2 only on the gRPC port builder.WebHost.ConfigureKestrel(options => diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs index 90cda76a..7c0a9121 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs +++ b/src/ZB.MOM.WW.ScadaBridge.Host/StartupValidator.cs @@ -58,6 +58,21 @@ public static class StartupValidator if (port == grpcPort) errors.Add("ScadaBridge:Node:GrpcPort must differ from RemotingPort"); + var metricsPortStr = nodeSection["MetricsPort"]; + int metricsPort = 8084; // NodeOptions default when the key is absent + if (metricsPortStr != null && (!int.TryParse(metricsPortStr, out metricsPort) || metricsPort < 1 || metricsPort > 65535)) + errors.Add("ScadaBridge:Node:MetricsPort must be 1-65535"); + + // Host-007 / REQ-HOST-4: the Kestrel metrics (HTTP/1.1) listener port + // must differ from BOTH the Akka remoting port and the gRPC port. + // A collision makes the metrics listener contend with Akka.Remote or + // the gRPC listener for the same TCP port and fail opaquely at runtime. + // Uses the resolved MetricsPort, including the 8084 default. + if (metricsPort == port) + errors.Add("ScadaBridge:Node:MetricsPort must differ from RemotingPort"); + if (metricsPort == grpcPort) + errors.Add("ScadaBridge:Node:MetricsPort must differ from GrpcPort"); + var dbSection = configuration.GetSection("ScadaBridge:Database"); if (string.IsNullOrEmpty(dbSection["SiteDbPath"])) errors.Add("ScadaBridge:Database:SiteDbPath required for Site nodes"); diff --git a/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Site.json b/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Site.json index b1faee82..ee0fa91b 100644 --- a/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Site.json +++ b/src/ZB.MOM.WW.ScadaBridge.Host/appsettings.Site.json @@ -7,6 +7,7 @@ "SiteId": "site-a", "RemotingPort": 8082, "GrpcPort": 8083, + "MetricsPort": 8084, "NodeName": "node-a" }, "Cluster": { diff --git a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs index f6e77d16..95b0af2f 100644 --- a/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs +++ b/tests/ZB.MOM.WW.ScadaBridge.Host.Tests/StartupValidatorTests.cs @@ -352,6 +352,105 @@ public class StartupValidatorTests Assert.Null(ex); } + [Theory] + [InlineData("0")] + [InlineData("-1")] + [InlineData("65536")] + [InlineData("abc")] + public void Site_InvalidMetricsPort_FailsValidation(string metricsPort) + { + var values = ValidSiteConfig(); + values["ScadaBridge:Node:MetricsPort"] = metricsPort; + var config = BuildConfig(values); + + var ex = Assert.Throws(() => StartupValidator.Validate(config)); + Assert.Contains("MetricsPort must be 1-65535", ex.Message); + } + + [Fact] + public void Site_ValidMetricsPort_PassesValidation() + { + var values = ValidSiteConfig(); + values["ScadaBridge:Node:MetricsPort"] = "8084"; + var config = BuildConfig(values); + + var ex = Record.Exception(() => StartupValidator.Validate(config)); + Assert.Null(ex); + } + + [Fact] + public void Site_MetricsPortEqualsRemotingPort_FailsValidation() + { + // Host-007 regression: the Kestrel metrics (HTTP/1.1) listener port must + // differ from RemotingPort. Identical values cause the metrics listener + // and Akka.Remote to contend for the same port at runtime. + var values = ValidSiteConfig(); + values["ScadaBridge:Node:RemotingPort"] = "8082"; + values["ScadaBridge:Node:MetricsPort"] = "8082"; + var config = BuildConfig(values); + + var ex = Assert.Throws(() => StartupValidator.Validate(config)); + Assert.Contains("MetricsPort must differ from RemotingPort", ex.Message); + } + + [Fact] + public void Site_MetricsPortEqualsGrpcPort_FailsValidation() + { + // Host-007 regression: the metrics listener port must differ from GrpcPort. + var values = ValidSiteConfig(); + values["ScadaBridge:Node:GrpcPort"] = "8083"; + values["ScadaBridge:Node:MetricsPort"] = "8083"; + var config = BuildConfig(values); + + var ex = Assert.Throws(() => StartupValidator.Validate(config)); + Assert.Contains("MetricsPort must differ from GrpcPort", ex.Message); + } + + [Fact] + public void Site_DefaultMetricsPortEqualsRemotingPort_FailsValidation() + { + // MetricsPort absent => NodeOptions default 8084. A site whose RemotingPort + // is also 8084 must still be rejected. + var values = ValidSiteConfig(); + values["ScadaBridge:Node:RemotingPort"] = "8084"; + // Keep GrpcPort distinct so only the metrics-vs-remoting rule fires. + values["ScadaBridge:Node:GrpcPort"] = "8083"; + // Seed nodes default to the remoting port (8082) in ValidSiteConfig; realign + // them to 8084 so the seed-vs-grpc rule is not what trips here. + values["ScadaBridge:Cluster:SeedNodes:0"] = "akka.tcp://scadabridge@site-a-node1:8084"; + values["ScadaBridge:Cluster:SeedNodes:1"] = "akka.tcp://scadabridge@site-a-node2:8084"; + var config = BuildConfig(values); + + var ex = Assert.Throws(() => StartupValidator.Validate(config)); + Assert.Contains("MetricsPort must differ from RemotingPort", ex.Message); + } + + [Fact] + public void Site_MetricsPortDiffersFromRemotingAndGrpc_PassesValidation() + { + var values = ValidSiteConfig(); + values["ScadaBridge:Node:RemotingPort"] = "8082"; + values["ScadaBridge:Node:GrpcPort"] = "8083"; + values["ScadaBridge:Node:MetricsPort"] = "8084"; + var config = BuildConfig(values); + + var ex = Record.Exception(() => StartupValidator.Validate(config)); + Assert.Null(ex); + } + + [Fact] + public void Central_InvalidMetricsPort_NotValidated() + { + // The metrics-port rules apply to Site nodes only; a Central node runs no + // metrics listener, so an out-of-range MetricsPort must not fail startup. + var values = ValidCentralConfig(); + values["ScadaBridge:Node:MetricsPort"] = "0"; + var config = BuildConfig(values); + + var ex = Record.Exception(() => StartupValidator.Validate(config)); + Assert.Null(ex); + } + [Fact] public void MultipleErrors_AllReported() {