fix(abcip,ablegacy): honour the authored EtherNet/IP port (#534)

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
2026-08-01 02:24:30 -04:00
parent b13bdc154a
commit 01538464b7
6 changed files with 136 additions and 4 deletions
@@ -122,4 +122,19 @@ public sealed record AbCipTagCreateParams(
    bool AllowPacking = true,
    int ConnectionSize = 4002,
    int ElementCount = 1,
-    bool IsArray = false);
+    bool IsArray = false)
+{
+    /// <summary>
+    ///     The value for libplctag's <c>gateway</c> attribute: <c>host:port</c>, or bare <c>host</c> when
+    ///     no usable port was configured (libplctag then applies its own 44818 default).
+    /// </summary>
+    /// <remarks>
+    ///     ⚠️ <b><see cref="Port"/> used to be carried here and never consumed</b> (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.
+    /// </remarks>
+    public string LibplctagGateway => Port > 0 ? $"{Gateway}:{Port}" : Gateway;
+}
@@ -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,
@@ -79,4 +79,19 @@ public sealed record AbLegacyTagCreateParams(
    string LibplctagPlcAttribute,
    string TagName,
    TimeSpan Timeout,
-    int ElementCount = 1);
+    int ElementCount = 1)
+{
+    /// <summary>
+    ///     The value for libplctag's <c>gateway</c> attribute: <c>host:port</c>, or bare <c>host</c> when
+    ///     no usable port was configured (libplctag then applies its own 44818 default).
+    /// </summary>
+    /// <remarks>
+    ///     ⚠️ <b><see cref="Port"/> used to be carried here and never consumed</b> (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.
+    /// </remarks>
+    public string LibplctagGateway => Port > 0 ? $"{Gateway}:{Port}" : Gateway;
+}
@@ -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
@@ -0,0 +1,44 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.AbCip;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.AbCip.Tests;
+
+/// <summary>
+///     Gitea #534 — the authored EtherNet/IP port must reach libplctag's <c>gateway</c> attribute.
+/// </summary>
+/// <remarks>
+///     This driver carried the identical defect to AbLegacy, where it was found:
+///     <see cref="AbCipTagCreateParams.Port"/> 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.
+/// </remarks>
+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");
+    }
+}
@@ -0,0 +1,54 @@
+using Shouldly;
+using Xunit;
+using ZB.MOM.WW.OtOpcUa.Driver.AbLegacy;
+
+namespace ZB.MOM.WW.OtOpcUa.Driver.AbLegacy.Tests;
+
+/// <summary>
+///     Gitea #534 — the authored EtherNet/IP port must reach libplctag's <c>gateway</c> attribute.
+/// </summary>
+/// <remarks>
+///     <para>
+///         <see cref="AbLegacyTagCreateParams.Port"/> was carried on the record and consumed by nobody:
+///         the runtime set <c>Tag.Gateway = p.Gateway</c>, 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.
+///     </para>
+///     <para>
+///         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.
+///     </para>
+/// </remarks>
+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");
+    }
+}