opcua: make read-only a property of the build, not a setting - #657
Draft
bburda wants to merge 12 commits into
Draft
opcua: make read-only a property of the build, not a setting#657bburda wants to merge 12 commits into
bburda wants to merge 12 commits into
Conversation
Four of the five protocol plugins cannot write to their device at all. OPC UA can, and config-less discovery marks a point writable straight from the server's CurrentWrite bit. A plant asking whether a box can change a controller cannot be answered by a configuration value, so the answer is carried by the binary instead. MEDKIT_OPCUA_READ_ONLY is a CMake cache option, default ON. With it on, OpcuaClient::write_value, the open62541pp Write service templates it instantiates, the vendor route handler and the value-coercion helper are never compiled; the node map's writable: true is ignored with one startup warning; the address-space walk does not consult CurrentWrite whatever infer_writable says; no entity registers x-plc-operations and no set_<name> operation is listed; and write_data plus the value-write half of execute_operation refuse with 403 before any node lookup, naming the build property in the message. Alarm acknowledge/confirm are Part 9 condition method calls rather than value writes and are unchanged. The acceptance is an inspection of the built object: test_opcua_build_variant runs nm over the plugin .so in both variants with opposite expectations, so a symbol list that stopped matching anything fails in the write-capable build instead of passing everywhere. Its markers are chosen to discriminate in an optimized build, which rules out the open62541pp service layer underneath the write templates - it is inlined away at -O2 and would prove nothing in the build CI produces. test_opcua_read_only drives a live gateway against the in-tree fixture server over HTTP, sweeping writable true/false/absent in the map and infer_writable true/false/absent on the walk, and re-checks after a reconnect re-walks the address space.
The package README gains a section on MEDKIT_OPCUA_READ_ONLY with both build commands and what each variant does, and the design doc gains the reasoning and the enforcement points. Two existing claims were wrong and are corrected: the auto_browse section said every auto-discovered point loads read-only, which stopped being true when infer_writable arrived defaulting to true, and infer_writable was undocumented despite being a live knob accepted only in the ROS-param form.
Nothing else in CI configures MEDKIT_OPCUA_READ_ONLY=OFF, so the write path and its tests would rot behind an #if until someone needed them. The job is also the control for the build inspection: test_opcua_build_variant asserts the write symbols are present here, which keeps its symbol list from decaying into one that matches nothing and passes everywhere. The flag is scoped to the package that defines it. Passed to the whole --packages-up-to chain, CMake reports it as a manually-specified variable nobody used in each of the other ten packages, and that stderr would hide a real one.
-fvisibility=hidden covers the sources compiled into the module but not the static archives it links, so the read-only object still carried - and exported - open62541's own __UA_Client_writeAttribute and the rest of the UA_Client_write* family. No route reached them, but anyone holding the .so could dlsym one and drive a controller with it, which makes the package's "no symbol able to put a value on the wire" claim untrue as written. -Wl,--exclude-libs,ALL takes the archives out of the export table and -ffunction-sections -fdata-sections plus -Wl,--gc-sections let the linker drop what nothing references once the C++ write path is gone. Measured on the Release objects: the write family goes from 9 exported and present to 0 present in the read-only build, and the module's dynamic exports fall from 689 to 115 - the six plugin entry points plus C++ vague-linkage symbols, with nothing from the OPC-UA stack in either variant. The inspection test gains two markers that survive optimization (opcua::services::write and services::writeAttribute<Client>), an assertion that neither variant exports OPC-UA machinery, and an assertion that the six entry points the gateway resolves are still exported - a link-time change that hid those would otherwise build and pass every symbol check while failing to load. The UA_Client_write* family is deliberately not a marker: it is now absent from both objects and so discriminates nothing, and the script says so where the exclusion is written. The README claim is narrowed to what is enforced: no code able to issue a Write and no export to reach the library through, while open62541's generic service dispatcher and the type descriptors its UA_TYPES table pins do remain inside the object, unexported and reachable from no route.
…ng writes The OpenPLC docker suite POSTed set_pump_speed and set_valve_position and asserted success, while Dockerfile.gateway built the plugin with defaults - now read-only, where that route is not registered at all. The job went red on this branch, and skipping the write section would have left the shipped image unexercised on the one question the branch is about. The image takes a MEDKIT_OPCUA_READ_ONLY build arg and the suite takes a matching MEDKIT_OPCUA_VARIANT, both defaulting to read-only, and the OpenPLC job runs a leg for each. Against the default image the suite now proves the contract on a real PLC: no x-plc-operations capability and no set_* operation advertised, the vendor route answering 404, the SOVD write refused with the vendor code and a message naming the build property, and both tags reading back unchanged afterwards. The write-success expectations are unchanged and move to the write-capable leg, which also re-reads pump_speed off the PLC. Every existing read, status and error-handling case stays as it was, in both legs. start.sh and test_all.sh take the same variable, so a local session can bring up either image without editing anything.
The export rule matched the prefix "UA_", so open62541's internal entry points - spelled __UA_Client_writeAttribute, __UA_Client_Service - walked straight past it: a module exporting one of those alongside the six plugin entry points was reported as exporting nothing from the OPC-UA stack. The rule is now a regex that makes the leading underscores optional, and a self-check ctest links exactly such a module and requires the export rule to reject it, next to name-level cases for both patterns. Against the old prefix rule that self-check fails on four counts, so it discriminates. The section flags reached no compilation. Upstream compiles its C sources in the open62541-object and open62541-plugins OBJECT libraries and assembles open62541 from $<TARGET_OBJECTS:...>, so options set on open62541 apply to nothing; the flags are now set on the object libraries, and flags.make shows them there. This mattered only outside Release, because upstream adds the same two flags itself under Release and MinSizeRel: measured on the read-only object, the _*UA_(Client|Server)_write* family was 12 in a default-type build and 0 in Release, and is 0 in both now. The inspection asserts that family is absent from a read-only object outright, as an invariant rather than a variant marker - it is absent from the write-capable object too, so it separates nothing between builds, but its presence means the archive member was pulled back in.
A method call that changes alarm state on the server is a write to the controller, and the rule is that a read-only build forbids every write path, not only value writes. Acknowledge and Confirm were left in on the reading that Part 9 condition interactions are not value writes; that reading is overruled. The Part 9 Acknowledge (i=9111) and Confirm (i=9113) calls now go through OpcuaClient::call_condition_method, which is compiled only in a write-capable build, so the read-only object carries no entry point that can issue one - measured 0 against 2 in Release and 0 against 1 in the default build type, and it joins the write markers. list_operations offers neither operation on an event-alarm entity, and execute_operation refuses both by name, before any condition lookup, with the vendor code and a message naming the build property. ConditionRefresh keeps riding on the generic call_method, which stays in both variants: Part 9 5.5.7 makes it a request for the server to replay conditions it already holds, it changes nothing on the controller, and without it a restart loses the active fault set. That is why the guard sits on the condition-method entry point rather than on call_method, and why neither call_method nor opcua::services::call is a write marker - measured, both are in either variant. The integration test grows a leg per variant against a condition the in-tree fixture really raises: read-only offers neither operation and refuses the POST with the vendor code; write-capable offers both and the acknowledge reaches a live ConditionId on the server.
…ite one The AlarmConditionType job POSTs acknowledge_fault and confirm_fault through SOVD, and its image is built from Dockerfile.gateway defaults - now read-only, where both are refused. It went red on this branch for the same reason the OpenPLC job did: that job became a matrix and this one did not. It takes the same MEDKIT_OPCUA_VARIANT the OpenPLC suite takes and derives the image build arg from it, so image and expectations cannot drift. The write-capable leg is the original sequence unchanged. The read-only leg asserts neither operation is advertised, that both POSTs come back 403 with x-medkit-plugin-error and a message naming the build property, and that the condition was not acknowledged - then drives the same ack, latch, confirm, clear lifecycle from the server's own CLI so every downstream expectation still runs on that image. Proving the acknowledge did not land needs a consequence, not a STATE line: the fixture prints those only from its stdin CLI, so a condition acknowledged over OPC-UA is indistinguishable from one that was not. The leg therefore latches the condition and asserts the fault is still raised - had the calls landed, the latch would have cleared it - and then clears it through the CLI, which is what makes the negative meaningful rather than a symptom of a stalled pipeline. Also replaces the loop variables shellcheck reports as unused in this file (SC2034), which the pre-commit hook reaches now that the file is touched.
Three comments narrated the history of the check rather than the property it enforces, and two of them named a process that has no place in a source file. Rewritten as present-tense statements: what the marker list covers and why the open62541 primitives are an absence invariant rather than a marker, and what the self-check's linked object proves about the export rule.
Two things the read-only build got wrong about what it says. The refusal answered 403. SOVD defines 403 once, in auth.rst, as a valid token with insufficient permissions - the one reading that is actively wrong here, because no credential and no role reaches a write path that is not in the binary. The catalogue's idiom for an operation the entity does not support is 501: faults.rst uses it for fault deletion, and data.rst, subscriptions.rst and triggers.rst all spell "not supported" that way. The three read-only refusals now return 501 through one named constant that carries the reason. A write-capable build keeps its codes, including the 403 it returns when the server itself denies access - which is what 403 is for. The infer_writable warning fired on the default. The setting defaults to true, so every read-only deployment with auto_browse enabled was told a key it never wrote was being ignored, and the node-map spelling reached the generic unknown-key warning instead. The config now records where an explicit value came from, the node-map block parses the key like the parameter does, and the warning fires once, only for a value someone set to true, naming the source so it can be found. Four unit tests pin both spellings, present and absent, and the integration sweep asserts the log line appears for an explicit true and does not appear when the key is left alone.
…lly absent The image is what ships, and Dockerfile.gateway builds the plugin with BUILD_TESTING=OFF, so the inspection ctest does not exist inside either container and no docker script ran nm. Both docker jobs now pull the object out of the image they just built and run the inspection on the runner with the expectation their leg declares - the read-only legs are the proof, the write-capable legs the control. Without it a build-arg or Dockerfile drift that shipped the wrong variant would have been caught by behaviour, not by the object. The docs claimed the object contains no code that can issue an OPC UA Write. That is false as written: open62541 is one static library, so the read path leaves behind the generic __UA_Client_Service dispatcher, 23 binary encoders, and the UA_TYPES descriptors the table pins as a whole - measured on the read-only object, WriteRequest, WriteValue, WriteResponse, AddNodes, DeleteNodes, AddReferences, SetMonitoringMode, SetPublishingMode and TransferSubscriptions are all still there, HistoryUpdate is not. Both documents now enumerate what is absent, each name asserted by nm, and what remains with the reason no route reaches it: descriptors are data, nothing composes those requests, nothing is exported, and a read-only build registers three GET routes. Subscription creation and ConditionRefresh are named as deliberately kept, since they change no controller data and the read path needs them.
Every image the OPC-UA workflow builds begins with an unauthenticated network read that nothing of ours has yet touched: apt-get update against the distribution mirrors, and in the gateway image rosdep update against raw.githubusercontent.com. A reset connection there fails the whole build, and the failure names a URL rather than anything about the change being tested. Both commands now run through a three-attempt wrapper, five seconds apart, in Dockerfile.gateway, docker/openplc/Dockerfile and docker/test_alarm_server/Dockerfile. Each attempt's own stderr passes through, so a genuine failure still ends the build with its own error text after the third try rather than being swallowed.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Pull Request
Summary
Read-only was a convention the OPC UA plugin followed, not a property of what shipped.
OpcuaClient::write_valuewas in every build,DataProvider::write_dataand thex-plc-operationsroute reached it, config-less discovery marked a point writable from the server'sCurrentWritebit, and the alarm acknowledge and confirm operations changed condition state on the controller. A configuration value cannot answer the question a plant security review asks, which is whether this gateway can change a controller. Only the build can, and only an inspection of the built object can prove the answer.MEDKIT_OPCUA_READ_ONLY, a CMake option, default ON. In the read-only build the value-write path, the alarm acknowledge/confirm path and thex-plc-operationsroute are compiled out.writableis never inferred from the server and never honoured from a node map; a map that asks for it gets one startup warning naming the build property. The provider methods still exist, because the gateway calls them through its provider interfaces, and they refuse withx-medkit-plugin-errorand a message naming the property.ConditionRefreshstays: it asks the server to replay the conditions it already holds and changes nothing on the controller, and without it a restart loses the active fault set. A write-capable build is selected explicitly with-DMEDKIT_OPCUA_READ_ONLY=OFFand exists for development.The object carries no write path, and CI checks the object, not the flag.
test_opcua_build_variantrunsnmon the built plugin and asserts, for the variant the build declared: the write markers absent (OpcuaClient::write_value,handle_plc_operations,call_condition_method, the open62541pp write templates and services), the read markers present as a positive control so an empty or wrong object fails, noUA_oropcua::symbol in the dynamic export table, the six plugin entry points exported, and none of open62541'sUA_*_write*primitives anywhere in the object. The last two hold because the plugin now links with--exclude-libs,ALLand--gc-sections, with the section flags applied to open62541's object libraries: upstream sets them only under Release, and a default-type build previously kept twelve write primitives as local code. A companion self-check test links a module that exports__UA_Client_writeAttributealongside the six entry points and requires the export rule to reject it.Both variants are exercised against a real server.
test_opcua_read_onlydrives a gateway with the plugin against the in-tree test server in whichever variant was built: the read-only build reports every pointwritable: falsewhatever the server and the map say, advertises nox-plc-operationsand no acknowledge/confirm, refuses writes and condition operations with the vendor code, and keeps doing so after the server restarts and the tree is re-walked; the write-capable build writes, the value reaches the server, and an acknowledge acts on a live condition the fixture raised. The OpenPLC docker job became a two-leg matrix: the default image proves the read-only contract on the PLC (refusals, and both tags unchanged on re-read), the write-capable image proves writes. A Jazzy job builds and tests the write-capable variant so that path cannot rot.Issue
Type
Breaking for a deployment that wrote to a controller through this plugin: the default build refuses those writes. Rebuild with
-DMEDKIT_OPCUA_READ_ONLY=OFFto keep them.Testing
nm -DCon the read-only object: 115 dynamic exports, none from open62541 or open62541pp, the six entry points present.Checklist