dohertj2/lmxopcua
1
0
Fork 0
Code Issues 14 Pull Requests Actions Packages Projects Releases Wiki Activity

refactor(mqtt): .Browser references .Driver — extract the client-options builder so the layering exception can go #512

New Issue
Closed
opened 2026-07-27 13:50:30 -04:00 by dohertj2 · 1 comment
dohertj2 commented 2026-07-27 13:50:30 -04:00
Owner
Copy Link
Copy Source

ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser.csproj carries a ProjectReference to
Driver.Mqtt (line 44) in addition to .Contracts. Every other driver's .Browser depends on
.Contracts only.

Why it was accepted. The browser needs to open its own broker connection, which means building MQTTnet
client options with the same TLS / CA-pinning / auth semantics the driver uses. Reusing
MqttConnection.BuildClientOptions was judged better than duplicating security-relevant TLS and CA-pin logic
in a second place, where the two copies would drift and one of them would drift open. The exception is
documented rather than silent.

Clean fix. Extract the pure options-building function to .Contracts (it is transport-free — it maps
MqttDriverOptions to client options; it does not connect), leaving MqttConnection as the only thing that
owns a live client. Then drop the .Driver reference from .Browser.

Guard against the wrong fix: do not resolve this by copy-pasting the TLS setup into the browser. The
duplication is the actual hazard the exception was taken to avoid.

Acceptance. .Browser references .Contracts only; one implementation of the TLS/CA-pin options builder
exists in the tree; the live browse suite still connects over TLS with CA pinning (both the pinned-good and
pinned-wrong cases).

`ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Browser.csproj` carries a `ProjectReference` to **`Driver.Mqtt`** (line 44) in addition to `.Contracts`. Every other driver's `.Browser` depends on `.Contracts` only. **Why it was accepted.** The browser needs to open its own broker connection, which means building MQTTnet client options with the same TLS / CA-pinning / auth semantics the driver uses. Reusing `MqttConnection.BuildClientOptions` was judged better than duplicating security-relevant TLS and CA-pin logic in a second place, where the two copies would drift and one of them would drift *open*. The exception is documented rather than silent. **Clean fix.** Extract the pure options-building function to `.Contracts` (it is transport-free — it maps `MqttDriverOptions` to client options; it does not connect), leaving `MqttConnection` as the only thing that owns a live client. Then drop the `.Driver` reference from `.Browser`. **Guard against the wrong fix:** do not resolve this by copy-pasting the TLS setup into the browser. The duplication is the actual hazard the exception was taken to avoid. **Acceptance.** `.Browser` references `.Contracts` only; one implementation of the TLS/CA-pin options builder exists in the tree; the live browse suite still connects over TLS with CA pinning (both the pinned-good and pinned-wrong cases).
dohertj2 added the driver/mqtt label 2026-07-27 13:50:30 -04:00
dohertj2 commented 2026-07-31 06:42:58 -04:00
Author
Owner
Copy Link
Copy Source

Fixed in master 2a7d1167. .Browser now references .Contracts + a new .Transport leaf, and not the runtime .Driver.

Two corrections to the issue.

1. It costed this as one extraction; it needed two. The browse session decodes births itself, so it
also used SparkplugTopic/SparkplugMessageType, SparkplugCodec and RebirthRequester — all in
.Driver/Sparkplug/. Moving only the client-options builder would have left the .Driver reference
exactly where it was. Those three moved to .Contracts, beside the Tahu wire schema they encode.

2. The suggested destination for the options builder does not work, and the .Browser csproj
already said so in a comment this issue did not account for: .Contracts is deliberately transport-free
and BuildClientOptions returns MqttClientOptions, an MQTTnet type. Putting it there would hand
MQTTnet to every .Contracts consumer — including ControlPlane, which has no business with a broker
client. So it went to the leaf project that comment had already prescribed:
ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Transport (refs .Contracts + MQTTnet), referenced by both .Driver
and .Browser. Still exactly one copy of the TLS/CA-pin logic — the guard the issue asked for.

Measured, not assumed. After the change Driver.Mqtt.dll, Polly.Core and Serilog are all absent
from the AdminUI's output. Core.dll is not — because Driver.Sql.Browser has the identical
violation, undocumented. The old comment blamed this browser for Core reaching the AdminUI and was
wrong. Filed as #526 rather than fixed in passing.

MqttBrowserLayeringTests guards the graph in both directions, including that .Transport stays a leaf
and .Contracts stays MQTTnet-free.

Fixed in master `2a7d1167`. `.Browser` now references `.Contracts` + a new `.Transport` leaf, and **not** the runtime `.Driver`. **Two corrections to the issue.** **1. It costed this as one extraction; it needed two.** The browse session decodes births itself, so it also used `SparkplugTopic`/`SparkplugMessageType`, `SparkplugCodec` and `RebirthRequester` — all in `.Driver/Sparkplug/`. Moving only the client-options builder would have left the `.Driver` reference exactly where it was. Those three moved to `.Contracts`, beside the Tahu wire schema they encode. **2. The suggested destination for the options builder does not work,** and the `.Browser` csproj already said so in a comment this issue did not account for: `.Contracts` is deliberately transport-free and `BuildClientOptions` returns `MqttClientOptions`, an **MQTTnet type**. Putting it there would hand MQTTnet to every `.Contracts` consumer — including **ControlPlane**, which has no business with a broker client. So it went to the leaf project that comment had already prescribed: `ZB.MOM.WW.OtOpcUa.Driver.Mqtt.Transport` (refs `.Contracts` + MQTTnet), referenced by both `.Driver` and `.Browser`. Still exactly one copy of the TLS/CA-pin logic — the guard the issue asked for. **Measured, not assumed.** After the change `Driver.Mqtt.dll`, `Polly.Core` and `Serilog` are all absent from the AdminUI's output. **`Core.dll` is not** — because **`Driver.Sql.Browser` has the identical violation**, undocumented. The old comment blamed this browser for Core reaching the AdminUI and was wrong. Filed as #526 rather than fixed in passing. `MqttBrowserLayeringTests` guards the graph in both directions, including that `.Transport` stays a leaf and `.Contracts` stays MQTTnet-free.
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
auto-managed
queue management
 cross-driver
queue management
 driver/abcip
queue management
 driver/ablegacy
queue management
 driver/focas
queue management
 driver/mqtt
MQTT / Sparkplug B broker-subscribe driver
 driver/opcuaclient
queue management
 driver/s7
queue management
 driver/twincat
queue management
 phase/1
queue management
 phase/2
queue management
 phase/3
queue management
 phase/4
queue management
 phase/5
queue management
 phase/6
queue management
 queue/blocked
queue management
 queue/done
queue management
 queue/failed
queue management
 queue/in-progress
queue management
 queue/queued
queue management
No Label driver/mqtt
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
dohertj2 (dohertj2)
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: dohertj2/lmxopcua#512
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?