From 01538464b7ad9c3c13f406dc10fe6c2604b4a473 Mon Sep 17 00:00:00 2001 From: Joseph Doherty Date: Sat, 1 Aug 2026 02:24:30 -0400 Subject: [PATCH] fix(abcip,ablegacy): honour the authored EtherNet/IP port (#534) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both libplctag drivers carried Port on their tag-create params and consumed it nowhere. The runtimes set Tag.Gateway to the bare host, and libplctag does not fail on a portless gateway — it substitutes 44818. So an authored port was not ignored in the harmless sense; the connection was silently REDIRECTED to whatever answers on the default port. On a plant network that is another PLC, and the driver would read its tags and report them Good. This is the root cause of #534's four AbLegacy integration failures, and the explanation for every observation in that issue: with each PCCC family moved to its own port (44819/44820/44821), the driver still dialled 44818 and kept talking to the AB CIP fixture's ControlLogix container. Hence PCCC-against-ControlLogix errors under every server-path / client-cip-path permutation — the permutations were never reaching a PCCC server at all. Established by a direct libplctag 1.5.2 probe outside the repo, which answered #534's open question: PCCC works fine against ab_server for all three families, with path="1,0" AND with path=null (only path="" throws ErrorBadData). Since the library could reach the simulators and the driver could not, the difference had to be in our Tag construction, and Gateway was the only field that differed. Same probe confirmed libplctag accepts host:port in the gateway attribute, which is what the fix emits. AbCip has the identical defect and is fixed alongside. It has never been observable there: every AbCip fixture profile binds 44818, so no test that driver has could distinguish "uses the authored port" from "always uses 44818". A fixture that only ever exercises the default value cannot falsify a rule about non-default values, however many tests point at it. The port is now emitted unconditionally rather than only when non-default, so the value does not depend on libplctag's own default; a port <= 0 still falls back to the bare host rather than producing "host:0". The load-bearing guard is the AbLegacy integration suite, which addresses each family on a non-default port and cannot pass unless the port is honoured — 4/4 green, having never once passed before. Unit tests in both drivers pin the composition for machines with no fixtures. Falsified both ways: reverting the runtime line reddens all 4 integration tests; dropping the port > 0 guard reddens exactly the two fallback cases. AbCip integration 10/13 (3 pre-existing skips), AbCip unit 345, AbLegacy unit 219. Claude-Session: https://claude.ai/code/session_015p7wGqy3YpZNCpDzTpGMKo --- .../IAbCipTagRuntime.cs | 17 +++++- .../LibplctagTagRuntime.cs | 4 +- .../IAbLegacyTagRuntime.cs | 17 +++++- .../LibplctagLegacyTagRuntime.cs | 4 +- .../AbCipGatewayCompositionTests.cs | 44 +++++++++++++++ .../AbLegacyGatewayCompositionTests.cs | 54 +++++++++++++++++++ 6 files changed, 136 insertions(+), 4 deletions(-) create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipGatewayCompositionTests.cs create mode 100644 tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyGatewayCompositionTests.cs diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs index f05b5164..87a630be 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/IAbCipTagRuntime.cs @@ -122,4 +122,19 @@ public sealed record AbCipTagCreateParams( bool AllowPacking = true, int ConnectionSize = 4002, int ElementCount = 1, - bool IsArray = false); + bool IsArray = false) +{ + /// + /// The value for libplctag's gateway attribute: host:port, or bare host when + /// no usable port was configured (libplctag then applies its own 44818 default). + /// + /// + /// ⚠️ used to be carried here and never consumed (Gitea #534, found in + /// the AbLegacy driver, which had the identical defect). The runtime passed the bare host to + /// libplctag, which silently substitutes 44818 — so an authored port did not merely fail to apply, + /// it redirected the connection to whatever answers on the default port, which on a plant network + /// is another PLC. This driver's own tests never caught it because its fixture sits on 44818, where + /// the bug is invisible by construction. + /// + public string LibplctagGateway => Port > 0 ? $"{Gateway}:{Port}" : Gateway; +} diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs index 9584fa06..11ef9b26 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip/LibplctagTagRuntime.cs @@ -19,7 +19,9 @@ internal sealed class LibplctagTagRuntime : IAbCipTagRuntime { _tag = new Tag { - Gateway = p.Gateway, + // host:port — see AbLegacyTagCreateParams/AbCipTagCreateParams.LibplctagGateway. + // Passing p.Gateway alone drops the authored port onto libplctag's 44818 default (#534). + Gateway = p.LibplctagGateway, Path = p.CipPath, PlcType = MapPlcType(p.LibplctagPlcAttribute), Protocol = Protocol.ab_eip, diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/IAbLegacyTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/IAbLegacyTagRuntime.cs index 6bc3dfa7..9f4872d6 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/IAbLegacyTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/IAbLegacyTagRuntime.cs @@ -79,4 +79,19 @@ public sealed record AbLegacyTagCreateParams( string LibplctagPlcAttribute, string TagName, TimeSpan Timeout, - int ElementCount = 1); + int ElementCount = 1) +{ + /// + /// The value for libplctag's gateway attribute: host:port, or bare host when + /// no usable port was configured (libplctag then applies its own 44818 default). + /// + /// + /// ⚠️ used to be carried here and never consumed (Gitea #534). The runtime + /// passed the bare host to libplctag, which silently substitutes 44818 — so an authored port was + /// not merely ignored, it redirected the connection to whatever answers on the default port. On the + /// test rig that was a different simulator of a different PLC family, which is how the AbLegacy + /// integration suite failed for years while looking like a fixture problem. On a plant network it + /// would be another PLC. + /// + public string LibplctagGateway => Port > 0 ? $"{Gateway}:{Port}" : Gateway; +} diff --git a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs index 72ce1c57..fb71ce32 100644 --- a/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs +++ b/src/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy/LibplctagLegacyTagRuntime.cs @@ -18,7 +18,9 @@ internal sealed class LibplctagLegacyTagRuntime : IAbLegacyTagRuntime { _tag = new Tag { - Gateway = p.Gateway, + // host:port — see AbLegacyTagCreateParams/AbCipTagCreateParams.LibplctagGateway. + // Passing p.Gateway alone drops the authored port onto libplctag's 44818 default (#534). + Gateway = p.LibplctagGateway, Path = p.CipPath, PlcType = MapPlcType(p.LibplctagPlcAttribute), Protocol = Protocol.ab_eip, // PCCC-over-EIP; libplctag routes via the PlcType-specific PCCC layer diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipGatewayCompositionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipGatewayCompositionTests.cs new file mode 100644 index 00000000..017e60b0 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests/AbCipGatewayCompositionTests.cs @@ -0,0 +1,44 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbCip; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests; + +/// +/// Gitea #534 — the authored EtherNet/IP port must reach libplctag's gateway attribute. +/// +/// +/// This driver carried the identical defect to AbLegacy, where it was found: +/// was on the record and consumed by nobody, so the runtime +/// handed libplctag the bare host and libplctag substituted 44818. It survived here precisely because +/// it is invisible here — every AbCip fixture profile binds 44818, so no test this driver has could +/// distinguish "uses the authored port" from "always uses 44818". Worth remembering as a shape: a +/// fixture that only ever exercises the default value cannot falsify a rule about non-default values, +/// however many tests point at it. +/// +public sealed class AbCipGatewayCompositionTests +{ + private static AbCipTagCreateParams Params(string host, int port) => + new(Gateway: host, Port: port, CipPath: "1,0", LibplctagPlcAttribute: "controllogix", + TagName: "TestDINT", Timeout: TimeSpan.FromSeconds(5)); + + [Fact] + public void The_authored_port_is_appended_to_the_gateway() + { + Params("10.100.0.35", 44900).LibplctagGateway.ShouldBe("10.100.0.35:44900"); + } + + [Fact] + public void The_default_port_is_still_written_explicitly() + { + Params("plc1.plant.local", 44818).LibplctagGateway.ShouldBe("plc1.plant.local:44818"); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void A_missing_port_falls_back_to_the_bare_host(int port) + { + Params("10.100.0.35", port).LibplctagGateway.ShouldBe("10.100.0.35"); + } +} diff --git a/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyGatewayCompositionTests.cs b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyGatewayCompositionTests.cs new file mode 100644 index 00000000..062f5360 --- /dev/null +++ b/tests/Drivers/ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests/AbLegacyGatewayCompositionTests.cs @@ -0,0 +1,54 @@ +using Shouldly; +using Xunit; +using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy; + +namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests; + +/// +/// Gitea #534 — the authored EtherNet/IP port must reach libplctag's gateway attribute. +/// +/// +/// +/// was carried on the record and consumed by nobody: +/// the runtime set Tag.Gateway = p.Gateway, the bare host. libplctag does not fail on a +/// portless gateway — it substitutes 44818. So the authored port was not ignored in the harmless +/// sense; the connection was silently *redirected* to whatever answers on the default port. On the +/// test rig that was a different simulator running a different PLC family, which is why the +/// AbLegacy integration suite failed in a way that looked like a missing fixture. On a plant +/// network it is another PLC, and the driver would read someone else's tags and report them Good. +/// +/// +/// The real guard is the integration suite, which now addresses each PCCC family on its own +/// non-default port (44819/44820/44821) and therefore cannot pass unless the port is honoured. +/// These unit tests pin the composition itself so the rule survives on a machine with no fixtures. +/// +/// +public sealed class AbLegacyGatewayCompositionTests +{ + private static AbLegacyTagCreateParams Params(string host, int port) => + new(Gateway: host, Port: port, CipPath: "1,0", LibplctagPlcAttribute: "slc500", + TagName: "N7:0", Timeout: TimeSpan.FromSeconds(5)); + + [Fact] + public void The_authored_port_is_appended_to_the_gateway() + { + Params("10.100.0.35", 44819).LibplctagGateway.ShouldBe("10.100.0.35:44819"); + } + + [Fact] + public void The_default_port_is_still_written_explicitly() + { + // Not an optimisation opportunity: emitting the port unconditionally is what makes the value + // independent of libplctag's own default, so a change to that default cannot move our traffic. + Params("plc1.plant.local", 44818).LibplctagGateway.ShouldBe("plc1.plant.local:44818"); + } + + [Theory] + [InlineData(0)] + [InlineData(-1)] + public void A_missing_port_falls_back_to_the_bare_host(int port) + { + // An unset port must not produce "host:0", which libplctag would take literally. + Params("10.100.0.35", port).LibplctagGateway.ShouldBe("10.100.0.35"); + } +}