From 27cfcf1b1eeaa568571436df4674861446e1fe22 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 15:55:04 +0200 Subject: [PATCH 01/12] feat(opcua): make read-only a property of the build, not a setting 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_ 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. --- .../ros2_medkit_opcua/CMakeLists.txt | 61 +++ .../ros2_medkit_opcua/opcua_client.hpp | 8 + .../ros2_medkit_opcua/opcua_plugin.hpp | 12 + .../src/address_space_browser.cpp | 8 + .../ros2_medkit_opcua/src/node_map.cpp | 24 + .../ros2_medkit_opcua/src/opcua_client.cpp | 9 + .../ros2_medkit_opcua/src/opcua_plugin.cpp | 50 ++ .../test/inspect_build_variant.py | 157 ++++++ .../integration/test_opcua_read_only.test.py | 502 ++++++++++++++++++ .../test/test_address_space_browser.cpp | 16 + .../ros2_medkit_opcua/test/test_node_map.cpp | 17 + .../test/test_opcua_client.cpp | 16 +- .../test/test_opcua_plugin.cpp | 66 +++ 13 files changed, 941 insertions(+), 5 deletions(-) create mode 100644 src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py create mode 100644 src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt b/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt index 26af9f4a6..b42fcc4f0 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt @@ -94,6 +94,32 @@ foreach(_op62_target open62541pp open62541) endif() endforeach() +# ---- Read-only build ---- +# Whether this box can change a controller is a property of the binary, not of +# a configuration file: ON compiles the OPC-UA value-write path out entirely, +# so no symbol able to put a value on the wire is emitted and no data point can +# be marked writable. A write-capable build exists for development and is +# selected explicitly. +option(MEDKIT_OPCUA_READ_ONLY + "Build the OPC-UA plugin without any controller write path" ON) +if(MEDKIT_OPCUA_READ_ONLY) + set(_medkit_opcua_read_only 1) + set(_medkit_opcua_variant "read-only") +else() + set(_medkit_opcua_read_only 0) + set(_medkit_opcua_variant "write-capable") +endif() +message(STATUS "ros2_medkit_opcua: ${_medkit_opcua_variant} build " + "(MEDKIT_OPCUA_READ_ONLY=${MEDKIT_OPCUA_READ_ONLY})") + +# Directory scope, and placed after the FetchContent block above: the macro +# guards headers shared by the plugin and every test target, so all of them +# must compile against one value or the one-definition rule is broken, while +# open62541pp - added as a subdirectory before this point - must not see it. +# Always defined to 0 or 1 so `#if MEDKIT_OPCUA_READ_ONLY` cannot silently read +# as false because of a misspelt macro name. +add_compile_definitions(MEDKIT_OPCUA_READ_ONLY=${_medkit_opcua_read_only}) + # ---- MODULE target: loaded via dlopen at runtime by PluginManager ---- # Symbols from ros2_medkit_gateway are resolved from the host process at runtime. add_library(ros2_medkit_opcua_plugin MODULE @@ -180,6 +206,23 @@ if(BUILD_TESTING) include(ROS2MedkitTestDomain) + # ---- Build inspection: does the object match the variant it declares? ---- + # The acceptance for a read-only build is the built object, not a setting, so + # this reads the object with nm. It runs in BOTH variants with opposite + # expectations: asserting only "no write symbols" would also pass for a + # symbol list that matches nothing at all, and the write-capable direction is + # what catches that. The .so comes from a generator expression - a hard-coded + # build path would let the check pass against a stale file. + find_package(Python3 REQUIRED COMPONENTS Interpreter) + add_test(NAME test_opcua_build_variant + COMMAND "${Python3_EXECUTABLE}" + "${CMAKE_CURRENT_SOURCE_DIR}/test/inspect_build_variant.py" + "$" + --expect "${_medkit_opcua_variant}") + set_tests_properties(test_opcua_build_variant PROPERTIES TIMEOUT 60) + # Reads a file with nm; starts no ROS node. + medkit_test_needs_no_domain(test_opcua_build_variant) + # Each test connects to a non-existent OPC UA host and waits ~3.8s for the # DNS / TCP failure path; with 13 tests in the suite the run requires ~90s. # The default ament_add_gtest timeout (60s) is too tight, causing the runner @@ -455,7 +498,25 @@ if(BUILD_TESTING) install(PROGRAMS test/integration/gen_test_certs.sh test/integration/test_opcua_secured.test.py + test/integration/test_opcua_read_only.test.py DESTINATION lib/${PROJECT_NAME}) + + # ---- Read-only / write-capable behaviour over real HTTP ---------------- + # The build-inspection test proves the write symbols are gone; this proves + # what the running system then does with a node that the fixture server + # genuinely permits writing. The variant is passed from CMake so the test + # cannot be told at run time which build it is looking at. + medkit_add_wrapped_test(test_opcua_read_only + COMMAND "${Python3_EXECUTABLE}" + "${CMAKE_CURRENT_SOURCE_DIR}/test/integration/test_opcua_read_only.test.py" + "${CMAKE_BINARY_DIR}/test_alarm_server" + "${_medkit_opcua_variant}") + # Four gateways, each waiting on an OPC-UA connect, plus a server restart; + # the timeout has to exceed the sum of the script's own wait deadlines so a + # slow failing run still reaches its own diagnostics. + set_tests_properties(test_opcua_read_only PROPERTIES + LABELS "integration" + TIMEOUT 900) # This one boots a real gateway and fault manager, so it needs a domain, and # it builds its own command line - hence the generic wrapper rather than an # ament test runner. diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_client.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_client.hpp index d617108dc..cffad93ba 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_client.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_client.hpp @@ -174,6 +174,13 @@ class OpcuaClient { /// Read multiple values std::vector read_values(const std::vector & node_ids); +#if !MEDKIT_OPCUA_READ_ONLY + /// The value-write surface, present only in a write-capable build + /// (``-DMEDKIT_OPCUA_READ_ONLY=OFF``). A read-only build has no declaration + /// and no definition, so nothing in the plugin can reach an OPC-UA Write + /// service call - see the package README on why that is a build property and + /// not a setting. + /// OPC-UA write error classification enum class WriteError { NotConnected, TypeMismatch, AccessDenied, NodeNotFound, TransportError }; @@ -190,6 +197,7 @@ class OpcuaClient { /// @return void on success, WriteErrorInfo on failure with specific error code tl::expected write_value(const opcua::NodeId & node_id, const OpcuaValue & value, const std::string & data_type_hint = ""); +#endif /// The AccessLevel / UserAccessLevel bits of a Variable node, read straight /// from the server. ``ok`` is false when not connected or the attribute read diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp index e77a3a519..431bd336b 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp @@ -69,6 +69,14 @@ namespace ros2_medkit_gateway { /// GET /apps/{id}/x-plc-data/{node} - Single node value /// POST /apps/{id}/x-plc-operations/{op} - Write value to PLC /// GET /components/{id}/x-plc-status - Connection state and stats +/// +/// Whether a value can be written at all is a property of the build, not of a +/// setting. ``MEDKIT_OPCUA_READ_ONLY`` defaults to ON: the OPC-UA write path is +/// then absent from the object, no data point is ever marked writable, neither +/// the x-plc-operations capability nor its POST route is registered, and +/// write_data / the value-write half of execute_operation refuse with 403 +/// before reaching the client. ``-DMEDKIT_OPCUA_READ_ONLY=OFF`` builds the +/// write-capable plugin. class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, public ros2_medkit_gateway::IntrospectionProvider, public ros2_medkit_gateway::DataProvider, @@ -146,7 +154,11 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, void handle_plc_data(const ros2_medkit_gateway::PluginRequest & req, ros2_medkit_gateway::PluginResponse & res); void handle_plc_data_single(const ros2_medkit_gateway::PluginRequest & req, ros2_medkit_gateway::PluginResponse & res); +#if !MEDKIT_OPCUA_READ_ONLY + /// Route handler for the vendor write endpoint. Declared and registered only + /// in a write-capable build; a read-only build serves no such route. void handle_plc_operations(const ros2_medkit_gateway::PluginRequest & req, ros2_medkit_gateway::PluginResponse & res); +#endif void handle_plc_status(const ros2_medkit_gateway::PluginRequest & req, ros2_medkit_gateway::PluginResponse & res); // Fault-detection signal (threshold / status-bit / enum) -> Fault bridge diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/address_space_browser.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/address_space_browser.cpp index b523d0025..53967d85b 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/address_space_browser.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/address_space_browser.cpp @@ -229,7 +229,14 @@ void AutoBrowser::visit_object(const opcua::NodeId & node, const std::vector writable = source_.read_writable(child.node_id); if (writable.has_value()) { @@ -238,6 +245,7 @@ void AutoBrowser::visit_object(const opcua::NodeId & node, const std::vector(false)) { + ++writable_ignored; + } + entry.writable = false; +#else entry.writable = n["writable"].as(false); +#endif if (n["min_value"]) { entry.min_value = n["min_value"].as(); } @@ -663,6 +676,17 @@ bool NodeMap::load(const std::string & yaml_path) { entries_.push_back(std::move(entry)); } +#if MEDKIT_OPCUA_READ_ONLY + if (writable_ignored > 0) { + RCLCPP_WARN(rclcpp::get_logger("opcua.node_map"), + "%zu node map entries request writable: true - ignored, every point stays " + "read-only. This plugin was built with MEDKIT_OPCUA_READ_ONLY=ON and carries " + "no write path; rebuild with -DMEDKIT_OPCUA_READ_ONLY=OFF for a write-capable " + "plugin.", + writable_ignored); + } +#endif + // Issue #386: native AlarmConditionType event subscriptions. Loaded from // top-level ``event_alarms:`` (sibling of ``nodes:``). Each entry must // declare its own entity_id; the entity will be merged into entity_defs_ diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_client.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_client.cpp index 95513dad6..ecbe52a78 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_client.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_client.cpp @@ -196,6 +196,9 @@ opcua::ByteString read_file_bytes(const std::string & path) { return opcua::ByteString(std::string_view(data)); } +#if !MEDKIT_OPCUA_READ_ONLY +// Only the value-write path builds an outgoing Variant; a read-only build +// decodes incoming ones and never encodes one. opcua::Variant value_to_variant(const OpcuaValue & val) { return std::visit( [](auto && v) -> opcua::Variant { @@ -218,6 +221,7 @@ opcua::Variant value_to_variant(const OpcuaValue & val) { }, val); } +#endif // !MEDKIT_OPCUA_READ_ONLY } // namespace @@ -812,6 +816,10 @@ std::vector OpcuaClient::read_values(const std::vector OpcuaClient::write_value(const opcua::NodeId & node_id, const OpcuaValue & value, const std::string & data_type_hint) { std::lock_guard lock(impl_->client_mutex); @@ -987,6 +995,7 @@ OpcuaClient::write_value(const opcua::NodeId & node_id, const OpcuaValue & value return tl::make_unexpected(WriteErrorInfo{WriteError::TransportError, e.what()}); } } +#endif // !MEDKIT_OPCUA_READ_ONLY uint32_t OpcuaClient::create_subscription(double publish_interval_ms, DataChangeCallback callback) { std::lock_guard lock(impl_->client_mutex); diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp index aebb164f9..bf978230d 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp @@ -94,6 +94,15 @@ UserAuthMode require_user_auth_mode(const std::string & value) { return parsed; } +#if MEDKIT_OPCUA_READ_ONLY +/// One sentence for every refusal on the value-write path. It names the build +/// property rather than a permission, because that is the difference an +/// operator reading the HTTP body needs: no credential and no configuration +/// change makes this binary write to a controller. +constexpr const char * kReadOnlyBuildRefusal = + "This OPC UA plugin was built read-only (MEDKIT_OPCUA_READ_ONLY=ON) and contains no controller write path; " + "rebuild with -DMEDKIT_OPCUA_READ_ONLY=OFF for a write-capable plugin"; +#else /// Parse a JSON "value" field, coerce to the node's declared data_type, and /// validate against the optional min/max range. Shared by handle_plc_operations, /// DataProvider::write_data, and OperationProvider::execute_operation to keep @@ -133,6 +142,7 @@ tl::expected parse_coerce_validate(const nlohmann::json return val; } +#endif // MEDKIT_OPCUA_READ_ONLY bool is_valid_path_segment(const std::string & s) { if (s.empty() || s.size() > 256) { @@ -582,6 +592,19 @@ void OpcuaPlugin::set_context(PluginContext & context) { log_security_profile(); +#if MEDKIT_OPCUA_READ_ONLY + // One line, once, when the operator asked the address-space walk to take its + // writability from the server's CurrentWrite bit. The inference is not in + // this binary, so the setting has no effect and saying so at startup beats + // leaving someone to wonder why every discovered point reads back read-only. + if (node_map_.auto_browse_config().enabled && node_map_.auto_browse_config().infer_writable) { + log_warn( + "auto_browse infer_writable is ignored: this plugin was built with MEDKIT_OPCUA_READ_ONLY=ON and carries " + "no write path, so every discovered data point stays read-only. Rebuild with " + "-DMEDKIT_OPCUA_READ_ONLY=OFF for a write-capable plugin."); + } +#endif + const bool connected = client_->connect(client_config_); if (connected) { log_info("Connected to OPC-UA server: " + client_config_.endpoint_url); @@ -675,10 +698,12 @@ std::vector OpcuaPlugin::get_routes() { [this](const PluginRequest & req, PluginResponse & res) { handle_plc_data_single(req, res); }}, +#if !MEDKIT_OPCUA_READ_ONLY {"POST", R"(apps/([^/]+)/x-plc-operations/([^/]+))", [this](const PluginRequest & req, PluginResponse & res) { handle_plc_operations(req, res); }}, +#endif {"GET", R"(components/([^/]+)/x-plc-status)", [this](const PluginRequest & req, PluginResponse & res) { handle_plc_status(req, res); @@ -798,9 +823,11 @@ IntrospectionResult OpcuaPlugin::introspect(const IntrospectionInput & /*input*/ if (!def.data_names.empty()) { ctx_->register_entity_capability(def.id, "x-plc-data"); } +#if !MEDKIT_OPCUA_READ_ONLY if (!def.writable_names.empty()) { ctx_->register_entity_capability(def.id, "x-plc-operations"); } +#endif } } @@ -887,6 +914,7 @@ void OpcuaPlugin::handle_plc_data_single(const PluginRequest & req, PluginRespon res.send_json(j); } +#if !MEDKIT_OPCUA_READ_ONLY void OpcuaPlugin::handle_plc_operations(const PluginRequest & req, PluginResponse & res) { if (!ctx_ || !poller_ || shutdown_requested_.load()) { res.send_error(503, ERR_SERVICE_UNAVAILABLE, "OPC-UA plugin not initialized"); @@ -967,6 +995,7 @@ void OpcuaPlugin::handle_plc_operations(const PluginRequest & req, PluginRespons res.send_json(response); } +#endif // !MEDKIT_OPCUA_READ_ONLY void OpcuaPlugin::handle_plc_status(const PluginRequest & req, PluginResponse & res) { if (!ctx_ || !poller_ || shutdown_requested_.load()) { @@ -1678,6 +1707,16 @@ tl::expected OpcuaPlugin::read_data(const tl::expected OpcuaPlugin::write_data(const std::string & entity_id, const std::string & resource_name, const nlohmann::json & value) { +#if MEDKIT_OPCUA_READ_ONLY + // First statement in the function: the refusal precedes every lookup, so no + // request reaches the OPC-UA client, and 403 says the server understood the + // request and will not carry it out. The gateway renders provider errors as + // the x-medkit-plugin-error vendor code with this message verbatim. + (void)entity_id; + (void)resource_name; + (void)value; + return tl::make_unexpected(DataProviderErrorInfo{DataProviderError::ReadOnly, kReadOnlyBuildRefusal, 403}); +#else if (!ctx_ || !poller_) { return tl::make_unexpected(DataProviderErrorInfo{DataProviderError::Internal, "plugin not initialized", 503}); } @@ -1728,6 +1767,7 @@ tl::expected OpcuaPlugin::write_dat }, *parsed); return dto::DataWriteResult{std::move(result)}; +#endif // MEDKIT_OPCUA_READ_ONLY } bool OpcuaPlugin::has_data(const std::string & entity_id) const { @@ -1890,6 +1930,15 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string return dto::OperationExecutionResult{std::move(out)}; } +#if MEDKIT_OPCUA_READ_ONLY + // Everything past the acknowledge / confirm branch above is the value-write + // path, and this build does not contain it. Nothing advertised reaches here - + // a read-only build marks no point writable, so list_operations emits no + // set_* entry - and a request aimed straight at one is refused before any + // node lookup or client call. Acknowledging an alarm is a Part 9 condition + // interaction, not a value write, and stays available. + return tl::make_unexpected(OperationProviderErrorInfo{OperationProviderError::Rejected, kReadOnlyBuildRefusal, 403}); +#else std::string data_name; if (operation_name.substr(0, 4) == "set_") { data_name = operation_name.substr(4); @@ -1945,6 +1994,7 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string }, *parsed); return dto::OperationExecutionResult{std::move(result)}; +#endif // MEDKIT_OPCUA_READ_ONLY } bool OpcuaPlugin::has_operations(const std::string & entity_id) const { diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py new file mode 100644 index 000000000..96fc5c160 --- /dev/null +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py @@ -0,0 +1,157 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +""" +Inspect the built OPC-UA plugin object for the write surface it declares. + +The check reads the object itself rather than any configuration. + +A read-only build is a property of the binary: `MEDKIT_OPCUA_READ_ONLY=ON` +compiles the OPC-UA value-write path out, so no symbol that can put a value on +the wire is emitted. A runtime flag could be flipped; an absent symbol cannot. + +The check runs in BOTH variants with opposite expectations, and that is what +makes it meaningful: + + read-only every WRITE_MARKER absent, every READ_MARKER present + write-capable every WRITE_MARKER present, every READ_MARKER present + +Asserting only the read-only direction would pass for a marker list that +matches nothing at all - a renamed or fully inlined symbol would read as +"the write path is gone". The write-capable direction is the control that keeps +the marker list honest, and READ_MARKER is the control that fails on an empty, +stripped or wrong object. + +Usage: inspect_build_variant.py --expect {read-only,write-capable} +""" + +import argparse +import shutil +import subprocess +import sys + +# Symbols that exist only when the value-write path is compiled in. Each is on +# the road from the provider entry points to the wire: +# - OpcuaClient::write_value is the plugin's own single write entry point; +# - OpcuaPlugin::handle_plc_operations is the vendor route that reaches it; +# - parse_coerce_validate is the value coercion the three write paths share; +# - Node::writeValueScalar / writeValue are the open62541pp templates +# where the value is actually encoded for the Write service. +# +# Every marker is verified to discriminate in an OPTIMIZED build, which is what +# CI and every release produce. That rules out the layer underneath the two +# templates - opcua::services::writeValue and writeAttributeImpl - which are fully inlined at -O2 and emit no symbol in either variant: +# a marker like that reads as "the write path is gone" in both builds and proves +# nothing. The open62541 C entry points (__UA_Client_writeAttribute and friends) +# are excluded for the opposite reason: they sit in the same statically linked +# translation unit as the read entry points, so the linker keeps them either way. +WRITE_MARKERS = ( + 'ros2_medkit_gateway::OpcuaClient::write_value(', + 'ros2_medkit_gateway::OpcuaPlugin::handle_plc_operations(', + 'parse_coerce_validate(', + 'opcua::Node::writeValueScalar<', + 'opcua::Node::writeValue(opcua::Variant const&)', +) + +# The read path the plugin needs in every variant. Present in both builds, so a +# stripped, truncated or simply wrong object fails here instead of silently +# satisfying the "no write symbols" half. These are the plugin's own out-of-line +# definitions rather than open62541pp templates, for the same optimization +# reason as above: services::detail::readAttributeImpl is inlined away at -O2. +READ_MARKERS = ( + 'ros2_medkit_gateway::OpcuaClient::read_value(', + 'ros2_medkit_gateway::OpcuaClient::read_values(', + 'ros2_medkit_gateway::OpcuaClient::read_access_level(', + 'ros2_medkit_gateway::OpcuaClient::browse_detailed(', +) + +# An optimized plugin object still carries a few thousand symbols. Anything near +# zero means nm read something that is not the plugin, or an object whose symbol +# table was stripped, and every "absent" verdict below would then be vacuous. +MIN_SYMBOLS = 1000 + + +def nm(args, path): + """Run nm with the given flags and return its stdout lines.""" + out = subprocess.run( + ['nm', *args, path], capture_output=True, text=True, check=False) + if out.returncode != 0: + print(f'FAIL: nm {" ".join(args)} {path} exited {out.returncode}\n{out.stderr}', + file=sys.stderr) + sys.exit(1) + return out.stdout.splitlines() + + +def count(lines, marker): + """Return the number of symbol lines containing the marker substring.""" + return sum(1 for line in lines if marker in line) + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('plugin', help='path to libros2_medkit_opcua_plugin.so') + parser.add_argument('--expect', required=True, + choices=('read-only', 'write-capable'), + help='the write surface this build declares') + args = parser.parse_args() + + if shutil.which('nm') is None: + print('FAIL: nm (binutils) not on PATH - the build inspection cannot run', + file=sys.stderr) + return 1 + + defined = nm(['-C'], args.plugin) + # Recorded for the operator reading a failure: the imported half of the + # object. open62541 is linked statically, so both the read and the write + # implementations are defined inside the plugin and this list holds only + # libc/OpenSSL/gateway ABI names - it never discriminates the two variants + # on its own, which is why the verdict below is taken from `nm -C`. + undefined = nm(['-DC', '--undefined-only'], args.plugin) + print(f'{args.plugin}: {len(defined)} defined symbols, ' + f'{len(undefined)} undefined dynamic symbols, expecting {args.expect}') + + failures = [] + if len(defined) < MIN_SYMBOLS: + failures.append(f'only {len(defined)} symbols (< {MIN_SYMBOLS}) - ' + 'object is stripped, truncated or not the plugin') + + for marker in READ_MARKERS: + n = count(defined, marker) + print(f' read {n:>4} {marker}') + if n == 0: + failures.append(f'read path symbol missing: {marker}') + + want_writes = args.expect == 'write-capable' + for marker in WRITE_MARKERS: + n = count(defined, marker) + print(f' write {n:>4} {marker}') + if want_writes and n == 0: + failures.append(f'write-capable build is missing: {marker}') + if not want_writes and n != 0: + failures.append(f'read-only build still contains {n} x: {marker}') + + if failures: + print(f'FAIL ({args.expect}):', file=sys.stderr) + for f in failures: + print(f' - {f}', file=sys.stderr) + return 1 + + print(f'PASS: object matches the {args.expect} build variant') + return 0 + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py new file mode 100644 index 000000000..52c916ba5 --- /dev/null +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py @@ -0,0 +1,502 @@ +#!/usr/bin/env python3 +# Copyright 2026 bburda +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +""" +Drive the read-only / write-capable OPC-UA plugin over real HTTP. + +A live gateway loads the plugin against the in-tree OPC-UA fixture server and +every claim is checked at the wire, not in a mock: + + * a node map asking for ``writable: true`` on a node the server really does + let us write; + * the ``infer_writable`` sweep (true / false / absent) over the address-space + walk, against the same node; + * the SOVD write endpoints and the vendor write route; + * what the entity tree advertises; + * and a reconnect, so the answer does not change when the address space is + walked a second time. + +The expected variant is passed in from CMake, never read from a runtime +parameter: the whole point is that a read-only build is a property of the +binary, and a test that could be told otherwise at run time would not be +testing that. + +Usage: test_opcua_read_only.test.py +""" + +import json +import os +from pathlib import Path +import shutil +import signal +import socket +import subprocess +import sys +import tempfile +import time +import urllib.error +import urllib.request + +# The fixture registers these under the Objects folder in namespace 2. The two +# Int32 registers carry AccessLevel READ|WRITE, so the server would let a +# write-capable client change them; Tank.Level is READ only. +WRITABLE_NODE = 'ns=2;s=StatusWord' +READ_ONLY_NODE = 'ns=2;s=Tank.Level' +SECOND_WRITABLE_NODE = 'ns=2;s=FaultCode' + +ENTITY = 'plc_app' +COMPONENT = 'read_only_runtime' + +# The vendor code the gateway puts on the wire for any plugin provider refusal +# (primitives.cpp maps every x-medkit-* code into error_code "vendor-specific" +# plus this vendor_code). The build property is named in the message. +PLUGIN_VENDOR_CODE = 'x-medkit-plugin-error' +BUILD_PROPERTY = 'MEDKIT_OPCUA_READ_ONLY' + +failures = [] + + +def check(condition, message): + """Record a failed expectation and keep going, so one run reports them all.""" + if condition: + print(f' OK {message}') + else: + print(f' FAIL {message}', file=sys.stderr) + failures.append(message) + return bool(condition) + + +def free_port(): + """Grab an OS-assigned free TCP port on the loopback and release it.""" + with socket.socket(socket.AF_INET, socket.SOCK_STREAM) as s: + s.bind(('127.0.0.1', 0)) + return s.getsockname()[1] + + +def find_plugin(): + """Locate libros2_medkit_opcua_plugin.so under AMENT_PREFIX_PATH.""" + for prefix in os.environ.get('AMENT_PREFIX_PATH', '').split(os.pathsep): + if not prefix: + continue + for root, _dirs, files in os.walk(prefix): + if 'libros2_medkit_opcua_plugin.so' in files: + return os.path.join(root, 'libros2_medkit_opcua_plugin.so') + return None + + +def http(url, method='GET', body=None, timeout=5): + """Return (status, parsed_json_or_raw_text) for one request; (0, None) on transport failure.""" + data = json.dumps(body).encode() if body is not None else None + req = urllib.request.Request(url, data=data, method=method) + if data is not None: + req.add_header('Content-Type', 'application/json') + try: + with urllib.request.urlopen(req, timeout=timeout) as resp: + raw = resp.read().decode() + status = resp.status + except urllib.error.HTTPError as e: + raw = e.read().decode() + status = e.code + except (urllib.error.URLError, OSError): + return 0, None + try: + return status, json.loads(raw) + except ValueError: + return status, raw + + +def wait_json(url, predicate, deadline=90, period=2.0): + """Poll until predicate(json) holds; return the final json or None.""" + last = None + for _ in range(int(deadline / period) + 1): + _status, last = http(url) + if last is not None: + try: + if predicate(last): + return last + except (KeyError, TypeError, ValueError): + pass + time.sleep(period) + return last + + +def wait_log(path, needle, deadline=30, period=0.5): + """Poll a log file until appears; return True/False.""" + for _ in range(int(deadline / period) + 1): + try: + if needle in Path(path).read_text(errors='replace'): + return True + except OSError: + pass + time.sleep(period) + return False + + +def terminate(proc): + """SIGTERM then SIGKILL a child process group started with start_new_session.""" + if proc is None: + return + if proc.returncode is not None: + pgid = None + else: + try: + pgid = os.getpgid(proc.pid) + except ProcessLookupError: + pgid = None + + def signal_group(sig): + if pgid is None: + return + try: + os.killpg(pgid, sig) + except ProcessLookupError: + pass + + signal_group(signal.SIGTERM) + proc.terminate() + try: + proc.wait(timeout=8) + except subprocess.TimeoutExpired: + signal_group(signal.SIGKILL) + proc.kill() + proc.wait() + signal_group(signal.SIGKILL) + log = getattr(proc, '_log', None) + if log is not None: + log.close() + + +def start_server(server_bin, port, log_path): + """Start the plain (unsecured) fixture server and wait for its READY line.""" + log = open(log_path, 'w') + proc = subprocess.Popen( + [str(server_bin), '--port', str(port)], + stdin=subprocess.DEVNULL, stdout=log, stderr=subprocess.STDOUT, + text=True, start_new_session=True, + ) + proc._log = log + if not wait_log(log_path, 'READY ', deadline=25): + terminate(proc) + return None + return proc + + +def write_params(path, *, port, plugin, server_port, node_map, manifest, auto_browse): + """Render a gateway params file for one leg of the sweep.""" + lines = [ + 'ros2_medkit_gateway:', + ' ros__parameters:', + ' server:', + ' host: "127.0.0.1"', + f' port: {port}', + ' plugins: ["opcua"]', + f' plugins.opcua.path: "{plugin}"', + f' plugins.opcua.endpoint_url: "opc.tcp://127.0.0.1:{server_port}"', + f' plugins.opcua.node_map_path: "{node_map}"', + ' plugins.opcua.poll_interval_ms: 500', + ' discovery.mode: "hybrid"', + f' discovery.manifest_path: "{manifest}"', + ' discovery.manifest_strict_validation: false', + ] + # `absent` leaves infer_writable to its own default, which is the shape most + # deployments actually run; the other two pin the documented endpoints. + if auto_browse is not None: + lines.append(' plugins.opcua.auto_browse.enabled: true') + if auto_browse != 'absent': + lines.append(f' plugins.opcua.auto_browse.infer_writable: {auto_browse}') + path.write_text('\n'.join(lines) + '\n') + + +def start_gateway(params_file, log_path, env): + """Launch a gateway_node bound to the given params file.""" + log = open(log_path, 'w') + proc = subprocess.Popen( + ['ros2', 'run', 'ros2_medkit_gateway', 'gateway_node', + '--ros-args', '--params-file', str(params_file)], + stdout=log, stderr=subprocess.STDOUT, env=env, start_new_session=True, + ) + proc._log = log + return proc + + +def item_by_name(payload, name): + """Pick one entry out of an x-plc-data / data collection response.""" + for item in (payload or {}).get('items', []): + if item.get('name') == name or item.get('id') == name: + return item + return None + + +def refusal_is_the_build(status, payload, what): + """Assert one HTTP answer is the read-only build's 403 refusal, naming the build.""" + ok = check(status == 403, f'{what}: 403 (got {status})') + if not isinstance(payload, dict): + check(False, f'{what}: JSON error body (got {payload!r})') + return + ok &= check(payload.get('vendor_code') == PLUGIN_VENDOR_CODE, + f'{what}: vendor_code {PLUGIN_VENDOR_CODE} (got {payload.get("vendor_code")!r})') + ok &= check(BUILD_PROPERTY in str(payload.get('message', '')), + f'{what}: message names {BUILD_PROPERTY} (got {payload.get("message")!r})') + return ok + + +def node_map_text(): + """Return a map sweeping writable true / false / absent over three real server nodes.""" + return ( + 'area_id: plc_systems\n' + f'component_id: {COMPONENT}\n' + 'nodes:\n' + f' - node_id: "{WRITABLE_NODE}"\n' + f' entity_id: {ENTITY}\n' + ' data_name: status_word\n' + ' data_type: int\n' + ' writable: true\n' + f' - node_id: "{SECOND_WRITABLE_NODE}"\n' + f' entity_id: {ENTITY}\n' + ' data_name: fault_code\n' + ' data_type: int\n' + ' writable: false\n' + f' - node_id: "{READ_ONLY_NODE}"\n' + f' entity_id: {ENTITY}\n' + ' data_name: tank_level\n' + ' data_type: float\n' + ) + + +def auto_browse_map_text(): + """Return a map that only turns the address-space walk on, with no hand-written nodes.""" + return ( + 'area_id: plc_systems\n' + f'component_id: {COMPONENT}\n' + 'auto_browse: true\n' + ) + + +def run_node_map_leg(workdir, env, plugin, server_port, manifest, read_only): + """Run the hand-written node map leg: what the tree advertises, and what a write does.""" + print('--- node map leg (writable: true / false / absent) ---') + node_map = workdir / 'nodes.yaml' + node_map.write_text(node_map_text()) + port = free_port() + params = workdir / 'gateway_nodemap.yaml' + write_params(params, port=port, plugin=plugin, server_port=server_port, + node_map=node_map, manifest=manifest, auto_browse=None) + log = workdir / 'gateway_nodemap.log' + gw = start_gateway(params, log, env) + try: + base = f'http://127.0.0.1:{port}/api/v1' + data = wait_json(f'{base}/apps/{ENTITY}/x-plc-data', + lambda j: item_by_name(j, 'status_word') is not None) + if not check(item_by_name(data, 'status_word') is not None, + 'x-plc-data serves the mapped points'): + print(log.read_text(errors='replace')[-3000:], file=sys.stderr) + return + + # 1. The writable flag on the wire. + for name, mapped in (('status_word', True), ('fault_code', False), ('tank_level', None)): + item = item_by_name(data, name) + expected = bool(mapped) and not read_only + check(item is not None and item.get('writable') is expected, + f'{name} (map writable={mapped}) reports writable={expected}') + + # 2. What the entity advertises. + _status, detail = http(f'{base}/apps/{ENTITY}') + names = {c.get('name') for c in (detail or {}).get('capabilities', [])} + check(('x-plc-operations' in names) is not read_only, + f'x-plc-operations capability advertised: {not read_only}') + + # 3. The SOVD operations collection. + _status, ops = http(f'{base}/apps/{ENTITY}/operations') + op_ids = {o.get('id') for o in (ops or {}).get('items', [])} + check(('set_status_word' in op_ids) is not read_only, + f'set_status_word offered: {not read_only} (got {sorted(op_ids)})') + + # 4. PUT /data/{id} on the point the map and the server both allow. + status, body = http(f'{base}/apps/{ENTITY}/data/status_word', 'PUT', {'value': 7}) + if read_only: + refusal_is_the_build(status, body, 'PUT data on a map-writable point') + else: + check(status == 200, f'PUT data succeeds (got {status}: {body!r})') + got = wait_json(f'{base}/apps/{ENTITY}/x-plc-data', + lambda j: (item_by_name(j, 'status_word') or {}).get('value') == 7, + deadline=20) + check((item_by_name(got, 'status_word') or {}).get('value') == 7, + 'the written value reached the server') + + # 5. POST an execution on the operation the map asks for. + exec_url = f'{base}/apps/{ENTITY}/operations/set_status_word/executions' + status, body = http(exec_url, 'POST', {'value': 9}) + if read_only: + refusal_is_the_build(status, body, 'POST execution on set_status_word') + else: + check(status in (200, 202), f'POST execution succeeds (got {status}: {body!r})') + got = wait_json(f'{base}/apps/{ENTITY}/x-plc-data', + lambda j: (item_by_name(j, 'status_word') or {}).get('value') == 9, + deadline=20) + check((item_by_name(got, 'status_word') or {}).get('value') == 9, + 'the executed operation reached the server') + + # 6. The vendor route is not registered at all in a read-only build. + status, body = http(f'{base}/apps/{ENTITY}/x-plc-operations/set_status_word', + 'POST', {'value': 11}) + if read_only: + check(status == 404, + f'POST x-plc-operations is not routed (got {status}: {body!r})') + else: + check(status == 200, f'POST x-plc-operations succeeds (got {status}: {body!r})') + + # 7. Points the map did NOT mark writable. + for name in ('fault_code', 'tank_level'): + status, body = http(f'{base}/apps/{ENTITY}/data/{name}', 'PUT', {'value': 3}) + if read_only: + refusal_is_the_build(status, body, f'PUT data on {name}') + else: + check(status == 400, + f'PUT data on the read-only point {name} is 400 (got {status})') + + # 8. The startup warning naming the build property. + if read_only: + check(wait_log(log, BUILD_PROPERTY, deadline=5), + f'the gateway log names {BUILD_PROPERTY} for the ignored writable: true') + finally: + terminate(gw) + + +def auto_browse_writable(base, deadline=90): + """Return the writable flag of the auto-browsed StatusWord point, or None.""" + payload = wait_json(f'{base}/apps', lambda j: j.get('items'), deadline=deadline) + for app in (payload or {}).get('items', []): + _status, data = http(f'{base}/apps/{app.get("id")}/x-plc-data') + item = item_by_name(data, 'statusword') or item_by_name(data, 'StatusWord') + if item is not None: + return item.get('writable') + return None + + +def run_auto_browse_leg(workdir, env, plugin, server_bin, server, server_port, + manifest, read_only, infer_writable, rebrowse): + """One infer_writable setting: the walk must never mark a point writable here.""" + label = f'infer_writable={infer_writable}' + print(f'--- auto_browse leg ({label}) ---') + node_map = workdir / f'auto_{infer_writable}.yaml' + node_map.write_text(auto_browse_map_text()) + port = free_port() + params = workdir / f'gateway_auto_{infer_writable}.yaml' + write_params(params, port=port, plugin=plugin, server_port=server_port, + node_map=node_map, manifest=manifest, auto_browse=infer_writable) + log = workdir / f'gateway_auto_{infer_writable}.log' + gw = start_gateway(params, log, env) + try: + base = f'http://127.0.0.1:{port}/api/v1' + writable = auto_browse_writable(base) + # The server sets CurrentWrite on StatusWord, so a write-capable build + # infers writable unless the setting says otherwise. A read-only build + # never consults the bit. + expected = (not read_only) and infer_writable != 'false' + check(writable is expected, + f'{label}: auto-browsed StatusWord reports writable={expected} (got {writable!r})') + + if rebrowse: + # CHANGE: the plugin re-walks the address space on a fresh session + # (maybe_rebrowse_on_reconnect). Restarting the server is the only + # runtime path that redoes the walk - the node map itself is read + # once at configure() and has no reload. + print(f'--- auto_browse leg ({label}) after a reconnect ---') + terminate(server[0]) + server[0] = start_server(server_bin, server_port, + workdir / 'alarm_server_restarted.log') + if not check(server[0] is not None, 'fixture server restarted on the same port'): + return + reconnected = wait_json(f'{base}/components/{COMPONENT}/x-plc-status', + lambda j: j.get('connected') is True, deadline=120) + if not check(bool(reconnected) and reconnected.get('connected') is True, + 'the plugin reconnected after the server restart'): + print(log.read_text(errors='replace')[-3000:], file=sys.stderr) + return + writable = auto_browse_writable(base, deadline=60) + check(writable is expected, + f'{label}: after the re-walk, writable={expected} (got {writable!r})') + finally: + terminate(gw) + + +def main(): + if len(sys.argv) < 3: + print('usage: test_opcua_read_only.test.py ', + file=sys.stderr) + return 2 + server_bin = Path(sys.argv[1]).resolve() + variant = sys.argv[2] + if variant not in ('read-only', 'write-capable'): + print(f'unknown build variant {variant!r}', file=sys.stderr) + return 2 + read_only = variant == 'read-only' + print(f'build variant under test: {variant}') + + # ROS_DOMAIN_ID comes from the domain wrapper this test is registered + # behind; a missing value is a wiring bug, not something to guess around. + ros_domain_id = os.environ.get('ROS_DOMAIN_ID') + if not ros_domain_id: + print('ROS_DOMAIN_ID is not set: this test must be launched by CTest, which ' + 'runs it behind medkit_run_with_domain.py', file=sys.stderr) + return 1 + + # Every prerequisite below is produced by the same build that produces this + # test, so a missing one is a broken build rather than an environment this + # run should tiptoe around. + for tool in ('ros2', 'nm'): + if shutil.which(tool) is None: + print(f'FAIL: {tool} not on PATH', file=sys.stderr) + return 1 + if not (server_bin.is_file() and os.access(server_bin, os.X_OK)): + print(f'FAIL: fixture server missing: {server_bin}', file=sys.stderr) + return 1 + plugin = find_plugin() + if plugin is None: + print('FAIL: libros2_medkit_opcua_plugin.so not found under AMENT_PREFIX_PATH', + file=sys.stderr) + return 1 + + workdir = Path(tempfile.mkdtemp(prefix='opcua_read_only_')) + env = dict(os.environ, ROS_DOMAIN_ID=ros_domain_id) + manifest = workdir / 'manifest.yaml' + manifest.write_text('manifest_version: "1.0"\n') + server_port = free_port() + server = [start_server(server_bin, server_port, workdir / 'alarm_server.log')] + try: + if server[0] is None: + print('FAIL: fixture server did not become READY', file=sys.stderr) + print((workdir / 'alarm_server.log').read_text(errors='replace'), file=sys.stderr) + return 1 + + run_node_map_leg(workdir, env, plugin, server_port, manifest, read_only) + for infer_writable, rebrowse in (('absent', True), ('true', False), ('false', False)): + run_auto_browse_leg(workdir, env, plugin, server_bin, server, server_port, + manifest, read_only, infer_writable, rebrowse) + + if failures: + print(f'FAIL: {len(failures)} expectation(s) not met:', file=sys.stderr) + for f in failures: + print(f' - {f}', file=sys.stderr) + return 1 + print(f'PASS: the {variant} plugin behaves as its build declares') + return 0 + finally: + terminate(server[0]) + shutil.rmtree(workdir, ignore_errors=True) + + +if __name__ == '__main__': + sys.exit(main()) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_address_space_browser.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_address_space_browser.cpp index b54bb95f9..dbf313e65 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_address_space_browser.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_address_space_browser.cpp @@ -183,7 +183,14 @@ TEST(AutoBrowserTest, MapsObjectHierarchyToEntityAndVariablesToDataPoints) { EXPECT_EQ(running_entry->entity_id, "plc_1_db_test"); EXPECT_EQ(running_entry->data_name, "running"); EXPECT_EQ(running_entry->data_type, "bool"); +#if MEDKIT_OPCUA_READ_ONLY + // A read-only build never consults the server's CurrentWrite bit: the + // inference is not compiled in, so a point the server would let us write + // still comes back read-only. + EXPECT_FALSE(running_entry->writable); +#else EXPECT_TRUE(running_entry->writable); // server: CurrentWrite set +#endif EXPECT_FALSE(result.node_cap_hit); EXPECT_FALSE(result.depth_cap_hit); @@ -212,7 +219,12 @@ TEST(AutoBrowserTest, WritableInferredFromServerAccessLevel) { auto result = browser.browse(); ASSERT_EQ(result.entries.size(), 3u); +#if MEDKIT_OPCUA_READ_ONLY + EXPECT_FALSE(find_entry(result.entries, rw.toString())->writable) + << "infer_writable must not produce a writable point in a read-only build"; +#else EXPECT_TRUE(find_entry(result.entries, rw.toString())->writable); +#endif EXPECT_FALSE(find_entry(result.entries, ro.toString())->writable); // AccessLevel read failed -> safe read-only default, never a false-positive. EXPECT_FALSE(find_entry(result.entries, unk.toString())->writable); @@ -619,7 +631,11 @@ auto_browse: true ASSERT_NE(level, nullptr); EXPECT_EQ(level->entity_id, "hand_authored_entity"); EXPECT_EQ(level->data_name, "hand_authored_name"); +#if MEDKIT_OPCUA_READ_ONLY + EXPECT_FALSE(level->writable); +#else EXPECT_TRUE(level->writable); +#endif const auto * running = node_map.find_by_node_id("ns=2;s=DB_Test.Running"); ASSERT_NE(running, nullptr); diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp index 08ba58402..2d2389844 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_node_map.cpp @@ -115,8 +115,15 @@ TEST_F(NodeMapTest, WritableEntries) { ASSERT_TRUE(map.load(yaml_path_)); auto writable = map.writable_entries_for_entity("fill_pump"); +#if MEDKIT_OPCUA_READ_ONLY + // The fixture map declares pump_speed as writable: true. A read-only build + // has no write path, so the request is ignored and no entity has a writable + // entry, whatever the file says. + EXPECT_TRUE(writable.empty()); +#else EXPECT_EQ(writable.size(), 1u); EXPECT_EQ(writable[0]->data_name, "pump_speed"); +#endif auto tank_writable = map.writable_entries_for_entity("tank_process"); EXPECT_TRUE(tank_writable.empty()); @@ -142,7 +149,11 @@ TEST_F(NodeMapTest, FindByNodeId) { auto * entry = map.find_by_node_id("ns=1;s=PumpSpeed"); ASSERT_NE(entry, nullptr); EXPECT_EQ(entry->entity_id, "fill_pump"); +#if MEDKIT_OPCUA_READ_ONLY + EXPECT_FALSE(entry->writable) << "writable: true in the file must not survive a read-only build"; +#else EXPECT_TRUE(entry->writable); +#endif } TEST_F(NodeMapTest, AlarmEntries) { @@ -199,7 +210,13 @@ TEST_F(NodeMapTest, EntityDefs) { ASSERT_NE(pump_def, nullptr); EXPECT_EQ(pump_def->data_names.size(), 2u); +#if MEDKIT_OPCUA_READ_ONLY + // No writable entry means no set_* operation and no x-plc-operations + // capability: the tree cannot advertise a write this build cannot perform. + EXPECT_TRUE(pump_def->writable_names.empty()); +#else EXPECT_EQ(pump_def->writable_names.size(), 1u); +#endif EXPECT_FALSE(pump_def->has_faults); } diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_client.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_client.cpp index d015014d7..557caebc2 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_client.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_client.cpp @@ -77,11 +77,6 @@ TEST(OpcuaClientTest, ReadSourceConditionsReportsScanFailureWhenDisconnected) { EXPECT_FALSE(scan_ok); } -TEST(OpcuaClientTest, WriteWhenDisconnected) { - OpcuaClient client; - EXPECT_FALSE(client.write_value({1, "SomeNode"}, 42.0)); -} - TEST(OpcuaClientTest, CreateSubscriptionWhenDisconnected) { OpcuaClient client; auto id = client.create_subscription(500.0, [](const std::string &, const OpcuaValue &) {}); @@ -94,6 +89,16 @@ TEST(OpcuaClientTest, RemoveSubscriptionsWhenEmpty) { client.remove_subscriptions(); } +#if !MEDKIT_OPCUA_READ_ONLY +// OpcuaClient::write_value exists only in a write-capable build +// (-DMEDKIT_OPCUA_READ_ONLY=OFF). A read-only build has no declaration for +// these three to call, which is the source-level half of the property +// test_opcua_build_variant asserts on the built object. +TEST(OpcuaClientTest, WriteWhenDisconnected) { + OpcuaClient client; + EXPECT_FALSE(client.write_value({1, "SomeNode"}, 42.0)); +} + TEST(OpcuaClientTest, WriteValueReturnsNotConnected) { OpcuaClient client; // Client never connected - write should return NotConnected error @@ -109,6 +114,7 @@ TEST(OpcuaClientTest, WriteValueWithTypeHintDisconnected) { EXPECT_FALSE(result.has_value()); EXPECT_EQ(result.error().code, OpcuaClient::WriteError::NotConnected); } +#endif // !MEDKIT_OPCUA_READ_ONLY // --------------------------------------------------------------------------- // Issue #389: OPC-UA client security config parsing (pure helpers, no server). diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp index 4bfdf4bc6..e0e150e10 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp @@ -290,7 +290,13 @@ TEST_F(OpcuaPluginTest, ReadDataReturnsValue) { EXPECT_EQ(result->content["id"], "level"); EXPECT_EQ(result->content["unit"], "mm"); EXPECT_EQ(result->content["data_type"], "float"); +#if MEDKIT_OPCUA_READ_ONLY + // The fixture map says writable: true. A read-only build ignores that, and + // the value it reports has to match what it will actually do. + EXPECT_EQ(result->content["writable"], false); +#else EXPECT_EQ(result->content["writable"], true); +#endif } TEST_F(OpcuaPluginTest, ReadDataNotFound) { @@ -299,6 +305,35 @@ TEST_F(OpcuaPluginTest, ReadDataNotFound) { EXPECT_EQ(result.error().code, DataProviderError::ResourceNotFound); } +#if MEDKIT_OPCUA_READ_ONLY +// A read-only build refuses every write before it looks anything up, so the +// point being writable, read-only or absent, and the body being well formed or +// not, all reach the same answer: 403 and a message naming the build property. +TEST_F(OpcuaPluginTest, WriteDataRefusedOnAReadOnlyBuild) { + const std::vector resources{"level", "pressure", "nonexistent"}; + const std::vector bodies{nlohmann::json{{"value", 5.0}}, nlohmann::json{{"not_value", 42}}, + nlohmann::json::object()}; + for (const auto & resource : resources) { + for (const auto & body : bodies) { + auto result = plugin_.write_data("tank", resource, body); + ASSERT_FALSE(result.has_value()) << resource << " / " << body.dump(); + EXPECT_EQ(result.error().code, DataProviderError::ReadOnly); + EXPECT_EQ(result.error().http_status, 403); + EXPECT_NE(result.error().message.find("MEDKIT_OPCUA_READ_ONLY"), std::string::npos) + << "the refusal must name the build property, got: " << result.error().message; + } + } +} + +// The refusal is not entity-scoped either: an entity this plugin does not own +// is refused the same way rather than answering 404, which would suggest some +// other entity could be written. +TEST_F(OpcuaPluginTest, WriteDataRefusedForUnknownEntityOnAReadOnlyBuild) { + auto result = plugin_.write_data("nonexistent", "level", nlohmann::json{{"value", 5.0}}); + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().http_status, 403); +} +#else TEST_F(OpcuaPluginTest, WriteDataReadOnly) { nlohmann::json body = {{"value", 5.0}}; auto result = plugin_.write_data("tank", "pressure", body); @@ -313,6 +348,7 @@ TEST_F(OpcuaPluginTest, WriteDataMissingValue) { EXPECT_FALSE(result.has_value()); EXPECT_EQ(result.error().code, DataProviderError::InvalidValue); } +#endif // has_data gates PluginManager::get_data_provider_for_entity (and, through // it, the gateway's `data` capability advertisement) at entity granularity - @@ -334,6 +370,15 @@ TEST_F(OpcuaPluginTest, HasDataFalseForUnknownEntity) { // -- OperationProvider tests -- +#if MEDKIT_OPCUA_READ_ONLY +// The tree must not advertise what the box cannot do: with no writable point +// there is no set_* operation to offer, even though the map asks for one. +TEST_F(OpcuaPluginTest, ListOperationsOffersNoWriteOnAReadOnlyBuild) { + auto result = plugin_.list_operations("tank"); + ASSERT_TRUE(result.has_value()); + EXPECT_TRUE(result->items.empty()); +} +#else TEST_F(OpcuaPluginTest, ListOperationsOnlyWritable) { auto result = plugin_.list_operations("tank"); ASSERT_TRUE(result.has_value()); @@ -341,6 +386,7 @@ TEST_F(OpcuaPluginTest, ListOperationsOnlyWritable) { EXPECT_EQ(result->items.size(), 1u); EXPECT_EQ(result->items[0].id, "set_level"); } +#endif TEST_F(OpcuaPluginTest, ListOperationsEntityNotFound) { auto result = plugin_.list_operations("nonexistent"); @@ -366,6 +412,25 @@ TEST_F(OpcuaPluginTest, HasOperationsFalseForUnknownEntity) { EXPECT_FALSE(plugin_.has_operations("nonexistent")); } +#if MEDKIT_OPCUA_READ_ONLY +// Nothing routes to the value-write branch on a read-only build, but a client +// that posts the operation id directly still has to be refused, and refused for +// the reason that is true. +TEST_F(OpcuaPluginTest, ExecuteOperationRefusedOnAReadOnlyBuild) { + const std::vector ops{"set_level", "set_pressure", "set_nonexistent"}; + const std::vector params_list{nlohmann::json{{"value", 5.0}}, nlohmann::json{{"not_value", 42}}}; + for (const auto & op : ops) { + for (const auto & params : params_list) { + auto result = plugin_.execute_operation("tank", op, params); + ASSERT_FALSE(result.has_value()) << op << " / " << params.dump(); + EXPECT_EQ(result.error().code, OperationProviderError::Rejected); + EXPECT_EQ(result.error().http_status, 403); + EXPECT_NE(result.error().message.find("MEDKIT_OPCUA_READ_ONLY"), std::string::npos) + << "the refusal must name the build property, got: " << result.error().message; + } + } +} +#else TEST_F(OpcuaPluginTest, ExecuteOperationMissingValue) { nlohmann::json params = {{"not_value", 42}}; auto result = plugin_.execute_operation("tank", "set_level", params); @@ -379,6 +444,7 @@ TEST_F(OpcuaPluginTest, ExecuteOperationReadOnly) { EXPECT_FALSE(result.has_value()); EXPECT_EQ(result.error().code, OperationProviderError::Rejected); } +#endif // -- auto_alarms fallback entity: has data/operations fitness + introspect // -- capability registration -- From 56fb192c8655a3087cedcae2b084a92cdc232f53 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 15:55:05 +0200 Subject: [PATCH 02/12] docs(opcua): document the read-only and write-capable builds 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. --- .../ros2_medkit_opcua/README.md | 81 +++++++++++++++++-- .../ros2_medkit_opcua/design/index.rst | 44 +++++++++- 2 files changed, 114 insertions(+), 11 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md index 0381ef765..0d58457d4 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md @@ -11,7 +11,10 @@ Follows the same plugin pattern as `ros2_medkit_graph_provider`: implements `Gat discovers them automatically by browsing the live address space (`auto_browse`, see below) - zero node-map config required - Exposes PLC values as the `x-plc-data` vendor collection -- Allows writing setpoints via `x-plc-operations` with type-aware coercion and range validation +- Ships read-only by default: the shipped binary contains no controller write + path at all (see [Read-only and write-capable builds](#read-only-and-write-capable-builds)) +- In a write-capable build, allows writing setpoints via `x-plc-operations` with + type-aware coercion and range validation - Reports the connection state and poll metrics via `x-plc-status` - Maps threshold-based PLC alarms to SOVD faults on the owning entity - Optionally publishes numeric PLC values to ROS 2 `std_msgs/Float32` topics @@ -100,6 +103,44 @@ to `false`) like every other discovery resource - it is a device fingerprint, useful for reconnaissance. Enable gateway authentication (`auth.enabled: true`) or front the API with an authenticating proxy to gate access to it. +## Read-only and write-capable builds + +Whether this box can change a controller is a property of the binary, not of a +configuration file. `MEDKIT_OPCUA_READ_ONLY` is a CMake cache option and +defaults to **ON**: + +```bash +# The default: read-only. No OPC-UA write path in the object. +colcon build --packages-up-to ros2_medkit_opcua + +# Write-capable, for development against a PLC you are allowed to drive. +colcon build --packages-select ros2_medkit_opcua \ + --cmake-args -DMEDKIT_OPCUA_READ_ONLY=OFF +``` + +In the default read-only build: + +- `OpcuaClient::write_value` and the open62541pp Write service calls it + instantiates are **not compiled**, so no symbol able to put a value on the + wire exists in `libros2_medkit_opcua_plugin.so`. The `test_opcua_build_variant` + ctest asserts this against the built object with `nm`, in both variants, so + the claim is checked rather than configured. +- No data point is ever `writable`. A node-map entry that says `writable: true` + is ignored with one startup warning naming the build property, and the + address-space walk never consults the server's `CurrentWrite` bit whatever + `infer_writable` says. +- Nothing advertises a write: no `x-plc-operations` capability on any entity, no + `set_` entry in `/operations`, and the `POST .../x-plc-operations/...` + route is not registered (it answers 404). +- `PUT /{type}/{id}/data/{name}` and the value-write half of + `POST /{type}/{id}/operations/{name}/executions` answer **403** before any + node lookup or client call, with vendor code `x-medkit-plugin-error` and a + message naming `MEDKIT_OPCUA_READ_ONLY`. Alarm `acknowledge_fault` / + `confirm_fault` are Part 9 condition interactions, not value writes, and stay + available in both variants. + +A write-capable build restores everything above; nothing else differs. + ## REST API ### Vendor Endpoints @@ -108,7 +149,7 @@ or front the API with an authenticating proxy to gate access to it. |--------|------|-------------| | GET | `/apps/{id}/x-plc-data` | All OPC-UA values for entity (with units, types, timestamps) | | GET | `/apps/{id}/x-plc-data/{name}` | Single data point value | -| POST | `/apps/{id}/x-plc-operations/set_{name}` | Write value to PLC (`{"value": 75.0}`) | +| POST | `/apps/{id}/x-plc-operations/set_{name}` | Write value to PLC (`{"value": 75.0}`) - write-capable build only; not registered otherwise | | GET | `/components/{id}/x-plc-status` | Connection state, poll stats, active alarms | ### Standard SOVD (provided by gateway) @@ -138,6 +179,9 @@ GET /api/v1/apps/tank_process/x-plc-data } ``` +`writable` is `false` on every item in the default read-only build, whatever the +node map or the server says. + **Write to PLC:** ```json POST /api/v1/apps/fill_pump/x-plc-operations/set_pump_speed @@ -210,7 +254,9 @@ nodes: display_name: Tank Level unit: mm data_type: float - writable: true # Allow writes via x-plc-operations + writable: true # Allow writes via x-plc-operations. Ignored in + # the default read-only build (one startup + # warning, the point stays read-only). min_value: 0.0 # Optional: range validation for writes max_value: 100.0 alarm: # Optional: numeric threshold -> SOVD fault @@ -384,10 +430,13 @@ along hierarchical references: `Objects` are walked normally. A depth limit and a total-node budget bound the walk against a pathological or very large address space; hitting either logs an operator warning that the resulting tree may be incomplete. -- **Read-only.** auto_browse never writes to the server, and every - auto-discovered data point loads with `writable: false` - promoting a - specific point to writable is a deliberate, reviewed decision made via an - explicit `nodes:` entry. +- **The walk itself never writes to the server.** Whether a discovered point is + exposed as writable depends on the build and on `infer_writable`: in the + default read-only build every auto-discovered point loads with + `writable: false` and the server's `CurrentWrite` bit is never read. In a + write-capable build `infer_writable` (default `true`) marks a point writable + exactly when the server says this session may write it; set it to `false` to + require an explicit `nodes:` entry instead. - **Explicit config always wins.** An auto-browsed entry for a NodeId that already has a hand-written `nodes:` entry is dropped; auto_browse only fills in what the node map does not already cover (or the whole tree, when there @@ -416,9 +465,15 @@ discovered endpoint go straight to a populated tree with no node-map file: ```yaml plugins.opcua.endpoint_url: "opc.tcp://192.168.1.10:4840" -plugins.opcua.auto_browse: true # or the same map form as above +plugins.opcua.auto_browse.enabled: true +plugins.opcua.auto_browse.infer_writable: true # write-capable builds only ``` +`infer_writable` is accepted only in this ROS-param form - the node-map YAML's +`auto_browse:` block does not parse it. It defaults to `true` and has no effect +in a read-only build, which logs one warning at startup and leaves every +discovered point read-only. + The JSON/ROS-param form takes precedence over whatever the node-map YAML's `auto_browse:` block set, mirroring how environment variables override the rest of the plugin's YAML config. @@ -805,8 +860,17 @@ Any PLC with an OPC-UA server works out of the box: source /opt/ros/jazzy/setup.bash colcon build --packages-select ros2_medkit_opcua colcon test --packages-select ros2_medkit_opcua + +# Write-capable variant (default is read-only) +colcon build --packages-select ros2_medkit_opcua \ + --cmake-args -DMEDKIT_OPCUA_READ_ONLY=OFF +colcon test --packages-select ros2_medkit_opcua ``` +`test_opcua_build_variant` inspects the built `.so` with `nm` and asserts it +matches the variant it was configured for, so the same `colcon test` command +checks the opposite property in each build. + ### Docker Integration Tests The plugin ships a self-contained OpenPLC tank demo in `docker/` that exercises the full stack end-to-end. CI runs this suite on every PR that touches the plugin; it is also runnable locally from any developer laptop. @@ -883,6 +947,7 @@ When the value returns below threshold, the fault is automatically cleared. - **Type-aware writes** - Plugin reads the OPC-UA node's data type before writing to avoid type mismatches (e.g., writing float32 to a REAL node, not float64). - **Node map driven** - All entity mapping is in YAML config, not code. Same plugin binary works with any PLC by changing the config file. - **Env var overrides** - `OPCUA_ENDPOINT_URL` and `OPCUA_NODE_MAP_PATH` override YAML config for Docker deployment flexibility. +- **Read-only is a build property, not a setting** - the shipped binary contains no OPC-UA write path, and CI proves it by inspecting the object rather than by reading a configuration value. A setting can be flipped on a running box; an absent symbol cannot. ## License diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst b/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst index 8ee173e2e..fc80233fd 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst @@ -57,7 +57,8 @@ All mapping lives in YAML (``config/tank_demo_nodes.yaml`` is the reference exam - ``node_id`` - OPC-UA node identifier (e.g. ``"ns=2;i=1"``) - ``entity_id`` - SOVD app the value belongs to - ``data_name`` - short name used in REST URLs - - ``display_name``, ``unit``, ``data_type``, ``writable`` - metadata + - ``display_name``, ``unit``, ``data_type``, ``writable`` - metadata. + ``writable`` is ignored in a read-only build (see below) - ``min_value`` / ``max_value`` - optional write range check - ``alarm`` - optional fault definition (``fault_code``, ``severity``, ``threshold``, ``above_threshold`` direction) @@ -96,8 +97,45 @@ edge-triggered callbacks: The plugin keeps per-fault state only long enough to detect edges; the fault manager owns persistence and fault lifecycle. -Type-aware writes -================= +Read-only is a property of the build +==================================== + +Four of the five protocol plugins cannot write to their device at all. OPC UA can, +and config-less discovery would mark a point writable straight from the server's +``CurrentWrite`` bit. A plant asking "can this box change a controller" cannot be +answered by a configuration value, because a configuration value can be changed +without rebuilding, reviewing or shipping anything. So the answer is carried by the +binary. + +``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 ``OpcuaPlugin::handle_plc_operations`` and + the value-coercion helper are all outside ``#if`` and are never compiled. The + object contains no symbol that can reach an OPC-UA Write service call. +- ``NodeMap::load`` forces ``writable`` to false and warns once when the file asked + otherwise; ``AutoBrowser`` does not compile the ``infer_writable`` inference, so + the server's ``CurrentWrite`` bit is never read. +- No entity registers the ``x-plc-operations`` capability, ``list_operations`` + emits no ``set_`` entry, and ``get_routes`` does not register the write + route. +- ``write_data`` and the value-write half of ``execute_operation`` return 403 as + their first statement, before any node lookup, with a message naming the build + property. They stay declared because the gateway reaches the plugin through the + ``DataProvider`` / ``OperationProvider`` interfaces; the refusal reaches a client + as SOVD vendor code ``x-medkit-plugin-error``, which is the code the gateway + assigns to every plugin provider error. +- ``acknowledge_fault`` / ``confirm_fault`` are unaffected. They are OPC-UA Part 9 + condition method calls, not value writes. + +The acceptance is an inspection of the built object, not a reading of the +configuration: ``test_opcua_build_variant`` runs ``nm`` over the plugin ``.so`` and +asserts the write symbols are absent and the read symbols present. It runs in both +variants with opposite expectations, so a symbol list that stopped matching anything +fails in the write-capable build instead of passing everywhere. + +Type-aware writes (write-capable build) +======================================= ``POST /apps/{id}/x-plc-operations/{op}`` accepts a JSON body ``{"value": ...}``. The handler: From 75e64a60932333d85b90cd6ef23166a069a06737 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 15:55:05 +0200 Subject: [PATCH 03/12] ci(opcua): build and test the write-capable variant on jazzy 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. --- .github/workflows/opcua-plugin.yml | 88 ++++++++++++++++++++++++++++++ 1 file changed, 88 insertions(+) diff --git a/.github/workflows/opcua-plugin.yml b/.github/workflows/opcua-plugin.yml index 43cf4a39d..3c498943f 100644 --- a/.github/workflows/opcua-plugin.yml +++ b/.github/workflows/opcua-plugin.yml @@ -342,3 +342,91 @@ jobs: - name: Show test results if: always() run: colcon test-result --verbose + + write-capable-build: + # The shipped plugin is read-only; MEDKIT_OPCUA_READ_ONLY=OFF is the + # development variant. Nothing else in CI configures it, so without this job + # the write path and its tests would rot unnoticed behind an #if until + # someone needed them. It is also the control for the build-inspection test: + # test_opcua_build_variant asserts the write symbols are PRESENT here, which + # is what keeps its symbol list from decaying into one that matches nothing + # and passes everywhere. + name: Write-capable build (jazzy) + runs-on: ubuntu-latest + container: + image: ubuntu:noble + timeout-minutes: 60 + defaults: + run: + shell: bash + steps: + - name: Install Git + run: | + apt-get update + apt-get install -y git + + - name: Checkout repository + uses: actions/checkout@v4 + + - name: Pre-install ROS 2 apt source + uses: ./.github/actions/ros-apt-source + + - name: Set up ROS 2 Jazzy + uses: ros-tooling/setup-ros@v0.7 + with: + required-ros-distributions: jazzy + + - name: Install ccache + run: apt-get install -y ccache + + - name: Cache ccache + uses: actions/cache@v4 + with: + path: /root/.cache/ccache + key: ccache-opcua-write-capable-${{ github.sha }} + restore-keys: | + ccache-opcua-write-capable- + + - name: Install dependencies + run: | + apt-get update + apt-get install -y ros-jazzy-test-msgs libyaml-cpp-dev libssl-dev + source /opt/ros/jazzy/setup.bash + rosdep update + rosdep install --from-paths src --ignore-src -y \ + --skip-keys='nav2_msgs ament_cmake_clang_format ament_cmake_clang_tidy' + + - name: Build ros2_medkit_opcua write-capable + env: + CCACHE_DIR: /root/.cache/ccache + CCACHE_MAXSIZE: 500M + CCACHE_SLOPPINESS: pch_defines,time_macros + run: | + source /opt/ros/jazzy/setup.bash + # Two passes so MEDKIT_OPCUA_READ_ONLY reaches only the package that + # defines it. Passed to the whole chain instead, CMake reports it as a + # manually-specified variable nobody used in each of the other ten + # packages, and the resulting stderr would hide any real one. + colcon build --symlink-install \ + --packages-up-to ros2_medkit_opcua --packages-skip ros2_medkit_opcua \ + --cmake-args -DCMAKE_BUILD_TYPE=Release \ + --event-handlers console_direct+ + colcon build --symlink-install \ + --packages-select ros2_medkit_opcua \ + --cmake-args -DCMAKE_BUILD_TYPE=Release -DMEDKIT_OPCUA_READ_ONLY=OFF \ + --event-handlers console_direct+ + ccache -s + + - name: Run tests + timeout-minutes: 20 + run: | + source /opt/ros/jazzy/setup.bash + source install/setup.bash + colcon test --return-code-on-test-failure \ + --packages-select ros2_medkit_opcua \ + --ctest-args -LE linter \ + --event-handlers console_direct+ + + - name: Show test results + if: always() + run: colcon test-result --verbose From 1ef80cefa71a20aaa319d1f9e5a0645d5b5501e3 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 16:32:41 +0200 Subject: [PATCH 04/12] fix(opcua): drop open62541's write primitives from the read-only object -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), 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. --- .../ros2_medkit_opcua/CMakeLists.txt | 32 +++++++- .../ros2_medkit_opcua/README.md | 26 +++++-- .../ros2_medkit_opcua/design/index.rst | 31 ++++++-- .../test/inspect_build_variant.py | 77 +++++++++++++++---- 4 files changed, 137 insertions(+), 29 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt b/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt index b42fcc4f0..1f868172d 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt @@ -94,6 +94,19 @@ foreach(_op62_target open62541pp open62541) endif() endforeach() +# One section per function and per object in the vendored library, so +# --gc-sections below can drop the ones nothing in this module reaches. Without +# this the whole of open62541 is linked in as a handful of large sections and +# the garbage collector has nothing to work with: the read-only object would +# still carry - and, before --exclude-libs, still export - the library's own +# Write service primitives, which a caller with dlsym could reach even though no +# route in the plugin can. +foreach(_op62_target open62541pp open62541) + if(TARGET ${_op62_target}) + target_compile_options(${_op62_target} PRIVATE -ffunction-sections -fdata-sections) + endif() +endforeach() + # ---- Read-only build ---- # Whether this box can change a controller is a property of the binary, not of # a configuration file: ON compiles the OPC-UA value-write path out entirely, @@ -148,6 +161,10 @@ target_include_directories(ros2_medkit_opcua_plugin PRIVATE target_compile_options(ros2_medkit_opcua_plugin PRIVATE -fvisibility=hidden -fvisibility-inlines-hidden + # Paired with --gc-sections on the link, so an unreachable function is not + # merely hidden but absent. + -ffunction-sections + -fdata-sections ) medkit_target_dependencies(ros2_medkit_opcua_plugin @@ -158,9 +175,22 @@ medkit_target_dependencies(ros2_medkit_opcua_plugin std_msgs ) -# Allow unresolved symbols - they resolve from the host process at runtime target_link_options(ros2_medkit_opcua_plugin PRIVATE + # Allow unresolved symbols - they resolve from the host process at runtime. -Wl,--unresolved-symbols=ignore-all + # -fvisibility=hidden covers the sources compiled into this module, but not + # the static archives it links. Without this, every global in the vendored + # open62541 lands in the module's dynamic symbol table, so the read-only + # object exported the library's own Write primitives and dlsym reached them + # even though no route did. ALL, not a list: the same holds for every archive + # the module happens to pull in. The plugin's extern "C" entry points come + # from object files, not archives, and keep their GATEWAY_PLUGIN_EXPORT + # visibility. + -Wl,--exclude-libs,ALL + # Then actually drop what nothing reaches. Retention starts from the dynamic + # symbol table, so this only becomes effective once --exclude-libs has emptied + # it of the library's globals - the two belong together. + -Wl,--gc-sections ) target_link_libraries(ros2_medkit_opcua_plugin diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md index 0d58457d4..f1e092491 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md @@ -120,11 +120,27 @@ colcon build --packages-select ros2_medkit_opcua \ In the default read-only build: -- `OpcuaClient::write_value` and the open62541pp Write service calls it - instantiates are **not compiled**, so no symbol able to put a value on the - wire exists in `libros2_medkit_opcua_plugin.so`. The `test_opcua_build_variant` - ctest asserts this against the built object with `nm`, in both variants, so - the claim is checked rather than configured. +- `OpcuaClient::write_value`, the vendor route handler, and the open62541pp + Write service functions and templates they reach are **not compiled**. On top + of that the plugin links with `-Wl,--exclude-libs,ALL` and `-Wl,--gc-sections` + (over `-ffunction-sections -fdata-sections`), which drops open62541's own + `UA_Client_write*` primitives from the object as well - nothing references + them once the C++ write path is gone. So `libros2_medkit_opcua_plugin.so` + contains **no code that can issue an OPC-UA Write**, and its dynamic symbol + table exports the six plugin entry points and nothing from the OPC-UA stack, + so `dlsym` has no handle on any of it either. + + Precisely, because a claim of this kind should be exact rather than sweeping: + what does remain inside the object is open62541's generic service dispatcher, + which the read path needs, and the generated type descriptors for the Write + request and response messages, which the library's `UA_TYPES` table keeps + alive whatever the linker does. Neither is exported, neither is reachable from + any route, and no function in the object composes a Write request out of them. + + `test_opcua_build_variant` asserts all of this against the built object with + `nm`, in both variants: absent write symbols and an OPC-UA-free export table + in the read-only build, present write symbols in the write-capable one, and + the six entry points exported in both. - No data point is ever `writable`. A node-map entry that says `writable: true` is ignored with one startup warning naming the build property, and the address-space walk never consults the server's `CurrentWrite` bit whatever diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst b/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst index fc80233fd..8f96661ff 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst @@ -109,10 +109,26 @@ binary. ``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 ``OpcuaPlugin::handle_plc_operations`` and - the value-coercion helper are all outside ``#if`` and are never compiled. The - object contains no symbol that can reach an OPC-UA Write service call. +- ``OpcuaClient::write_value``, the ``open62541pp`` Write service functions and + templates it reaches, the vendor route handler + ``OpcuaPlugin::handle_plc_operations`` and the value-coercion helper are all + outside ``#if`` and are never compiled. +- The link then removes what the compiler alone could not. ``-fvisibility=hidden`` + covers the sources compiled into the module but not the static archives it + links, so open62541's own ``UA_Client_write*`` primitives used to sit in the + object *and* in its dynamic symbol table: no route reached them, but a caller + holding the ``.so`` could ``dlsym`` one and drive a controller with it. + ``-Wl,--exclude-libs,ALL`` takes the archives out of the export table, and + ``-ffunction-sections -fdata-sections`` plus ``-Wl,--gc-sections`` then let the + linker drop them from the object entirely, because nothing references them once + the C++ write path is gone. The read-only object exports the six plugin entry + points and nothing from the OPC-UA stack. +- What is left of open62541 inside the object is the generic service dispatcher + the read path needs and the generated type descriptors the ``UA_TYPES`` table + pins. They are data and dispatch, not a write path: nothing exports them and no + function in the object composes a Write request from them. The claim the package + makes is therefore the exact one - no code able to issue a Write, and no export + to reach the library through - not a sweeping "no OPC-UA symbols at all". - ``NodeMap::load`` forces ``writable`` to false and warns once when the file asked otherwise; ``AutoBrowser`` does not compile the ``infer_writable`` inference, so the server's ``CurrentWrite`` bit is never read. @@ -130,9 +146,10 @@ binary. The acceptance is an inspection of the built object, not a reading of the configuration: ``test_opcua_build_variant`` runs ``nm`` over the plugin ``.so`` and -asserts the write symbols are absent and the read symbols present. It runs in both -variants with opposite expectations, so a symbol list that stopped matching anything -fails in the write-capable build instead of passing everywhere. +asserts the write symbols are absent, the read symbols present, the dynamic symbol +table free of OPC-UA machinery, and the six plugin entry points still exported. It +runs in both variants with opposite expectations, so a symbol list that stopped +matching anything fails in the write-capable build instead of passing everywhere. Type-aware writes (write-capable build) ======================================= diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py index 96fc5c160..4a6d47859 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py @@ -48,22 +48,37 @@ # - OpcuaPlugin::handle_plc_operations is the vendor route that reaches it; # - parse_coerce_validate is the value coercion the three write paths share; # - Node::writeValueScalar / writeValue are the open62541pp templates -# where the value is actually encoded for the Write service. +# where the value is encoded for the Write service; +# - services::write / services::writeAttribute are the layer beneath +# them, the last C++ frame before open62541's own client machinery. # # Every marker is verified to discriminate in an OPTIMIZED build, which is what -# CI and every release produce. That rules out the layer underneath the two -# templates - opcua::services::writeValue and writeAttributeImpl - which are fully inlined at -O2 and emit no symbol in either variant: -# a marker like that reads as "the write path is gone" in both builds and proves -# nothing. The open62541 C entry points (__UA_Client_writeAttribute and friends) -# are excluded for the opposite reason: they sit in the same statically linked -# translation unit as the read entry points, so the linker keeps them either way. +# CI and every release produce. That rules out opcua::services::writeValue and +# writeAttributeImpl: they are fully inlined at -O2 and +# emit no symbol in either variant, so they would read as "the write path is +# gone" in both builds and prove nothing. +# +# open62541's own primitives - __UA_Client_writeAttribute and the rest of the +# UA_Client_write* family - are absent from BOTH objects and therefore cannot +# discriminate either. They used to be present, and exported, in both: nothing +# in the plugin referenced them (open62541pp reaches the Write service through +# services::write, not through them), but the whole archive member was linked in +# and -fvisibility=hidden does not reach a static archive, so dlsym could call +# one and drive a controller the REST contract never exposed. +# -Wl,--exclude-libs,ALL plus -ffunction-sections/-fdata-sections and +# -Wl,--gc-sections removed them from the object outright. EXPORT_DENY below is +# what keeps that true: what remains of open62541 in a read-only object - the +# generic __UA_Client_Service dispatcher the read path needs, and the generated +# type descriptors the UA_TYPES table pins - is unreachable precisely because +# the module exports none of it. WRITE_MARKERS = ( 'ros2_medkit_gateway::OpcuaClient::write_value(', 'ros2_medkit_gateway::OpcuaPlugin::handle_plc_operations(', 'parse_coerce_validate(', 'opcua::Node::writeValueScalar<', 'opcua::Node::writeValue(opcua::Variant const&)', + 'opcua::services::write(opcua::Client&, opcua::WriteRequest const&)', + 'opcua::services::writeAttribute', ) # The read path the plugin needs in every variant. Present in both builds, so a @@ -78,6 +93,26 @@ 'ros2_medkit_gateway::OpcuaClient::browse_detailed(', ) +# Nothing from the OPC-UA stack may appear in the module's dynamic symbol table, +# in either variant. The gateway dlopens the plugin and needs its extern "C" +# entry points and nothing else; anything else exported is a dlsym handle on +# machinery no route exposes. Checked as a prefix match on the demangled name, +# which covers both the C library (UA_*, __UA_*) and the C++ wrapper (opcua::*). +EXPORT_DENY = ('UA_', 'opcua::') + +# Symbols the gateway resolves out of the plugin. If a link-time change ever +# hides these, the plugin still builds and still passes every symbol check above +# while failing to load at runtime, so they are asserted here rather than left +# to an integration test to discover. +REQUIRED_EXPORTS = ( + 'create_plugin', + 'plugin_api_version', + 'get_introspection_provider', + 'get_data_provider', + 'get_operation_provider', + 'get_fault_provider', +) + # An optimized plugin object still carries a few thousand symbols. Anything near # zero means nm read something that is not the plugin, or an object whose symbol # table was stripped, and every "absent" verdict below would then be vacuous. @@ -114,14 +149,9 @@ def main(): return 1 defined = nm(['-C'], args.plugin) - # Recorded for the operator reading a failure: the imported half of the - # object. open62541 is linked statically, so both the read and the write - # implementations are defined inside the plugin and this list holds only - # libc/OpenSSL/gateway ABI names - it never discriminates the two variants - # on its own, which is why the verdict below is taken from `nm -C`. - undefined = nm(['-DC', '--undefined-only'], args.plugin) - print(f'{args.plugin}: {len(defined)} defined symbols, ' - f'{len(undefined)} undefined dynamic symbols, expecting {args.expect}') + exported = nm(['-DC', '--defined-only'], args.plugin) + print(f'{args.plugin}: {len(defined)} symbols, {len(exported)} dynamic exports, ' + f'expecting {args.expect}') failures = [] if len(defined) < MIN_SYMBOLS: @@ -143,6 +173,21 @@ def main(): if not want_writes and n != 0: failures.append(f'read-only build still contains {n} x: {marker}') + # The export table is an invariant, not a variant property: neither build + # may hand dlsym a way into the OPC-UA stack. + leaked = [line.split(' ', 2)[-1] for line in exported + if any(line.split(' ', 2)[-1].startswith(p) for p in EXPORT_DENY)] + print(f' export {len(leaked):>4} OPC-UA symbols in the dynamic symbol table') + for name in leaked[:10]: + failures.append(f'dynamic symbol table exports OPC-UA machinery: {name}') + + export_names = {line.split(' ', 2)[-1] for line in exported} + missing = [name for name in REQUIRED_EXPORTS if name not in export_names] + print(f' export {len(REQUIRED_EXPORTS) - len(missing):>4}' + f'/{len(REQUIRED_EXPORTS)} plugin entry points') + for name in missing: + failures.append(f'plugin entry point not exported: {name}') + if failures: print(f'FAIL ({args.expect}):', file=sys.stderr) for f in failures: From c6caf5f7e5fd9f113cb66855342eb94319657b52 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 16:32:56 +0200 Subject: [PATCH 05/12] test(opcua): prove the read-only contract against OpenPLC, keep proving 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. --- .github/workflows/opcua-plugin.yml | 17 ++- .../ros2_medkit_opcua/README.md | 31 +++-- .../docker/Dockerfile.gateway | 9 +- .../docker/scripts/run_integration_tests.sh | 114 ++++++++++++++---- .../ros2_medkit_opcua/docker/scripts/start.sh | 18 ++- .../docker/scripts/test_all.sh | 20 ++- 6 files changed, 171 insertions(+), 38 deletions(-) diff --git a/.github/workflows/opcua-plugin.yml b/.github/workflows/opcua-plugin.yml index 3c498943f..44fb3eff3 100644 --- a/.github/workflows/opcua-plugin.yml +++ b/.github/workflows/opcua-plugin.yml @@ -125,9 +125,21 @@ jobs: run: colcon test-result --verbose integration: - name: Integration (OpenPLC) + name: Integration (OpenPLC, ${{ matrix.variant }}) + # Both write surfaces are exercised against the same real PLC. The + # read-only leg is the shipped image: it proves the refusal, its vendor + # code, and that the tag on the PLC did not move. The write-capable leg + # keeps proving that a write reaches OpenPLC and reads back. runs-on: ubuntu-latest timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + include: + - variant: read-only + read_only: 'ON' + - variant: write-capable + read_only: 'OFF' steps: - name: Checkout repository uses: actions/checkout@v4 @@ -141,6 +153,7 @@ jobs: - name: Build gateway + OPC-UA plugin image run: | docker build \ + --build-arg MEDKIT_OPCUA_READ_ONLY=${{ matrix.read_only }} \ -f src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway \ -t gateway-opcua . @@ -212,6 +225,8 @@ jobs: docker logs gateway 2>&1 | tail -10 - name: Run integration tests + env: + MEDKIT_OPCUA_VARIANT: ${{ matrix.variant }} run: bash src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_integration_tests.sh - name: Dump gateway logs on failure diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md index f1e092491..f58dc86d2 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md @@ -891,17 +891,26 @@ checks the opposite property in each build. The plugin ships a self-contained OpenPLC tank demo in `docker/` that exercises the full stack end-to-end. CI runs this suite on every PR that touches the plugin; it is also runnable locally from any developer laptop. +`MEDKIT_OPCUA_VARIANT` selects the write surface of the image under test and +the expectations applied to it, so the two cannot drift. CI runs both legs. + ```bash cd src/ros2_medkit_plugins/ros2_medkit_opcua/docker -# Start OpenPLC + gateway (builds everything) +# Start OpenPLC + gateway (builds everything). Default: the read-only image. bash scripts/start.sh +MEDKIT_OPCUA_VARIANT=write-capable bash scripts/start.sh # Manual testing curl -s http://localhost:8080/api/v1/apps/tank_process/x-plc-data | jq . -# Automated tests (16 assertions) +# Automated tests against a running pair bash scripts/run_integration_tests.sh +MEDKIT_OPCUA_VARIANT=write-capable bash scripts/run_integration_tests.sh + +# Build, start, test and clean up in one go +bash scripts/test_all.sh +MEDKIT_OPCUA_VARIANT=write-capable bash scripts/test_all.sh # Stop bash scripts/stop.sh @@ -909,14 +918,16 @@ bash scripts/stop.sh ### Test Coverage -| Category | Tests | What it validates | -|----------|-------|-------------------| -| Entity discovery | 5 | Areas, components, apps from PLC node map | -| PLC connection | 2 | OPC-UA connected, zero errors | -| Live data | 3 | Tank level, temperature, pressure have values | -| Write control | 2 | Pump speed, valve position written to PLC | -| Error handling | 3 | Unknown entity, unknown operation, invalid JSON | -| **Total** | **16** | | +| Category | read-only | write-capable | What it validates | +|----------|-----------|---------------|-------------------| +| Entity discovery | 5 | 5 | Areas, components, apps from PLC node map | +| PLC connection | 2 | 2 | OPC-UA connected, zero errors | +| Live data | 3 | 3 | Tank level, temperature, pressure have values | +| Advertised write surface | 2 | 2 | x-plc-operations capability and the set_* operation, absent / present | +| Write control | 6 | 3 | read-only: the vendor route 404s, the SOVD write is refused with the vendor code and a message naming the build property, and both tags read back unchanged on the PLC. write-capable: pump speed and valve position written, pump speed read back | +| Error handling | 3 | 3 | Unknown entity, unknown operation, invalid JSON | +| SOVD /data | 2 | 2 | The standard data collection serves the same points | +| **Total** | **23** | **20** | | ## Security diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway index 2887336b1..2cfda3238 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway @@ -38,6 +38,12 @@ RUN bash -c "source /opt/ros/jazzy/setup.bash && \ colcon build --cmake-args -DBUILD_TESTING=OFF \ --packages-skip vda5050_agent ros2_medkit_vda5050_msgs ros2_medkit_opcua" +# Which write surface the image ships. ON is the default the plugin itself +# defaults to: no controller write path in the object. OFF builds the +# write-capable image the OpenPLC suite uses to prove the write side still +# works; it is not a deployment configuration. +ARG MEDKIT_OPCUA_READ_ONLY=ON + # Build the plugin separately so it picks up headers from the installed # ros2_medkit_gateway from the previous step. BUILD_TESTING=OFF avoids # pulling ament linters into the runtime image; unit tests run via a @@ -45,7 +51,8 @@ RUN bash -c "source /opt/ros/jazzy/setup.bash && \ RUN bash -c "source /opt/ros/jazzy/setup.bash && \ source ${COLCON_WS}/install/setup.bash && \ colcon build --packages-select ros2_medkit_opcua \ - --cmake-args -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=OFF" + --cmake-args -DCMAKE_BUILD_TYPE=Release -DBUILD_TESTING=OFF \ + -DMEDKIT_OPCUA_READ_ONLY=${MEDKIT_OPCUA_READ_ONLY}" # Stage 2: Runtime FROM ros:jazzy-ros-base diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_integration_tests.sh b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_integration_tests.sh index f48a5d2e5..c11f7c87d 100755 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_integration_tests.sh +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_integration_tests.sh @@ -1,12 +1,26 @@ #!/usr/bin/env bash # OpenPLC Tank Demo - Integration Tests -# Validates: entity discovery, live data, control writes, error handling +# Validates: entity discovery, live data, the write surface the image was built +# with, and error handling. +# +# MEDKIT_OPCUA_VARIANT selects which write surface is expected, and it has to +# match how the gateway image was built (docker build --build-arg +# MEDKIT_OPCUA_READ_ONLY=ON|OFF). It defaults to read-only because that is what +# the plugin and the Dockerfile default to; against a default image the suite +# proves the read-only contract on a real PLC - the refusal, its vendor code, +# and that the tag did not move - rather than skipping the write section. set -o pipefail API="${GATEWAY_URL:-http://localhost:8080}/api/v1" +VARIANT="${MEDKIT_OPCUA_VARIANT:-read-only}" PASS=0 FAIL=0 +if [ "$VARIANT" != "read-only" ] && [ "$VARIANT" != "write-capable" ]; then + echo "MEDKIT_OPCUA_VARIANT must be read-only or write-capable (got '$VARIANT')" >&2 + exit 2 +fi + RED='\033[0;31m' GREEN='\033[0;32m' YELLOW='\033[1;33m' @@ -23,7 +37,7 @@ assert() { fi } -echo -e "${YELLOW}=== OpenPLC Tank Demo Integration Tests ===${NC}\n" +echo -e "${YELLOW}=== OpenPLC Tank Demo Integration Tests (${VARIANT} image) ===${NC}\n" # 1. Wait for gateway + PLC entities echo -e "${YELLOW}1. Wait for gateway + PLC entities${NC}" @@ -65,23 +79,77 @@ assert "tank_temperature has value" "$([ -n "$TEMP" ] && [ "$TEMP" != "null" ] & assert "tank_pressure has value" "$([ -n "$PRESS" ] && [ "$PRESS" != "null" ] && echo true || echo false)" echo " Level=$LEVEL mm, Temp=$TEMP C, Pressure=$PRESS bar" -# 5. Write - Pump Speed -echo -e "\n${YELLOW}5. Write Pump Speed${NC}" -WRITE=$(curl -s -X POST "$API/apps/fill_pump/x-plc-operations/set_pump_speed" \ - -H "Content-Type: application/json" -d '{"value": 75.0}') -assert "Write pump speed OK" "$(echo "$WRITE" | jq '.status == "ok"' 2>/dev/null)" -sleep 5 -PUMP=$(curl -s "$API/apps/fill_pump/x-plc-data" | jq '.items[] | select(.name == "pump_speed") | .value' 2>/dev/null) -assert "Pump speed ~= 75" "$(echo "$PUMP" | jq '. >= 74 and . <= 76' 2>/dev/null)" - -# 6. Write - Valve Position -echo -e "\n${YELLOW}6. Write Valve Position${NC}" -WRITE=$(curl -s -X POST "$API/apps/drain_valve/x-plc-operations/set_valve_position" \ - -H "Content-Type: application/json" -d '{"value": 50.0}') -assert "Write valve position OK" "$(echo "$WRITE" | jq '.status == "ok"' 2>/dev/null)" - -# 7. Error Handling -echo -e "\n${YELLOW}7. Error Handling${NC}" +# 5. What the tree advertises about writing +echo -e "\n${YELLOW}5. Advertised write surface${NC}" +CAPS=$(curl -s "$API/apps/fill_pump" | jq '[.capabilities[].name]' 2>/dev/null) +OPS=$(curl -s "$API/apps/fill_pump/operations" | jq '[.items[].id]' 2>/dev/null) +if [ "$VARIANT" = "read-only" ]; then + assert "no x-plc-operations capability" \ + "$(echo "$CAPS" | jq 'contains(["x-plc-operations"]) | not' 2>/dev/null)" + assert "no set_pump_speed operation" \ + "$(echo "$OPS" | jq 'contains(["set_pump_speed"]) | not' 2>/dev/null)" +else + assert "x-plc-operations capability" \ + "$(echo "$CAPS" | jq 'contains(["x-plc-operations"])' 2>/dev/null)" + assert "set_pump_speed operation" \ + "$(echo "$OPS" | jq 'contains(["set_pump_speed"])' 2>/dev/null)" +fi + +# 6. Write - Pump Speed +echo -e "\n${YELLOW}6. Write Pump Speed${NC}" +PUMP_BEFORE=$(curl -s "$API/apps/fill_pump/x-plc-data" | jq '.items[] | select(.name == "pump_speed") | .value' 2>/dev/null) +if [ "$VARIANT" = "read-only" ]; then + # The vendor route is not registered at all, so this is the gateway's own + # 404 rather than a plugin refusal. + WRITE=$(curl -s -X POST "$API/apps/fill_pump/x-plc-operations/set_pump_speed" \ + -H "Content-Type: application/json" -d '{"value": 75.0}') + assert "x-plc-operations set_pump_speed not routed" \ + "$(echo "$WRITE" | jq '.error_code == "resource-not-found"' 2>/dev/null)" + # The SOVD write endpoint does reach the plugin, and the plugin refuses. + PUT=$(curl -s -X PUT "$API/apps/fill_pump/data/pump_speed" \ + -H "Content-Type: application/json" -d '{"value": 75.0}') + assert "PUT pump_speed refused with the vendor code" \ + "$(echo "$PUT" | jq '.vendor_code == "x-medkit-plugin-error"' 2>/dev/null)" + assert "refusal names MEDKIT_OPCUA_READ_ONLY" \ + "$(echo "$PUT" | jq '.message | test("MEDKIT_OPCUA_READ_ONLY")' 2>/dev/null)" + sleep 5 + PUMP=$(curl -s "$API/apps/fill_pump/x-plc-data" | jq '.items[] | select(.name == "pump_speed") | .value' 2>/dev/null) + # The tag did not move. Tolerance rather than equality because the value is + # a float coming back through the PLC; a write of 75 from rest is far larger + # than any representation jitter. + assert "pump_speed unchanged on the PLC" \ + "$(jq -n --argjson a "${PUMP_BEFORE:-null}" --argjson b "${PUMP:-null}" \ + '($a != null) and ($b != null) and (($a - $b) | fabs < 0.5)' 2>/dev/null)" +else + WRITE=$(curl -s -X POST "$API/apps/fill_pump/x-plc-operations/set_pump_speed" \ + -H "Content-Type: application/json" -d '{"value": 75.0}') + assert "Write pump speed OK" "$(echo "$WRITE" | jq '.status == "ok"' 2>/dev/null)" + sleep 5 + PUMP=$(curl -s "$API/apps/fill_pump/x-plc-data" | jq '.items[] | select(.name == "pump_speed") | .value' 2>/dev/null) + assert "Pump speed ~= 75" "$(echo "$PUMP" | jq '. >= 74 and . <= 76' 2>/dev/null)" +fi + +# 7. Write - Valve Position +echo -e "\n${YELLOW}7. Write Valve Position${NC}" +VALVE_BEFORE=$(curl -s "$API/apps/drain_valve/x-plc-data" | jq '.items[] | select(.name == "valve_position") | .value' 2>/dev/null) +if [ "$VARIANT" = "read-only" ]; then + PUT=$(curl -s -X PUT "$API/apps/drain_valve/data/valve_position" \ + -H "Content-Type: application/json" -d '{"value": 50.0}') + assert "PUT valve_position refused with the vendor code" \ + "$(echo "$PUT" | jq '.vendor_code == "x-medkit-plugin-error"' 2>/dev/null)" + sleep 5 + VALVE=$(curl -s "$API/apps/drain_valve/x-plc-data" | jq '.items[] | select(.name == "valve_position") | .value' 2>/dev/null) + assert "valve_position unchanged on the PLC" \ + "$(jq -n --argjson a "${VALVE_BEFORE:-null}" --argjson b "${VALVE:-null}" \ + '($a != null) and ($b != null) and (($a - $b) | fabs < 0.5)' 2>/dev/null)" +else + WRITE=$(curl -s -X POST "$API/apps/drain_valve/x-plc-operations/set_valve_position" \ + -H "Content-Type: application/json" -d '{"value": 50.0}') + assert "Write valve position OK" "$(echo "$WRITE" | jq '.status == "ok"' 2>/dev/null)" +fi + +# 8. Error Handling +echo -e "\n${YELLOW}8. Error Handling${NC}" assert "404 unknown entity" "$(curl -s "$API/apps/nonexistent/x-plc-data" | jq 'has("error_code")' 2>/dev/null)" assert "404 unknown operation" "$(curl -s -X POST "$API/apps/tank_process/x-plc-operations/nonexistent" -H "Content-Type: application/json" -d '{"value":1}' | jq 'has("error_code")' 2>/dev/null)" assert "400 invalid JSON" "$(curl -s -X POST "$API/apps/fill_pump/x-plc-operations/set_pump_speed" -H "Content-Type: application/json" -d 'bad' | jq 'has("error_code")' 2>/dev/null)" @@ -98,9 +166,11 @@ SOVD_DATA=$(curl -s "$API/apps/tank_process/data" 2>/dev/null) assert "SOVD /data returns items" "$(echo "$SOVD_DATA" | jq 'has("items")' 2>/dev/null)" assert "SOVD /data has tank_level" "$(echo "$SOVD_DATA" | jq '[.items[].id] | contains(["tank_level"])' 2>/dev/null)" -# Cleanup - stop pump -curl -s -X POST "$API/apps/fill_pump/x-plc-operations/set_pump_speed" \ - -H "Content-Type: application/json" -d '{"value": 0}' >/dev/null 2>&1 +# Cleanup - stop pump. Only the write-capable image can, and only it moved it. +if [ "$VARIANT" = "write-capable" ]; then + curl -s -X POST "$API/apps/fill_pump/x-plc-operations/set_pump_speed" \ + -H "Content-Type: application/json" -d '{"value": 0}' >/dev/null 2>&1 +fi echo -e "\n${YELLOW}===== Test Summary =====${NC}" TOTAL=$((PASS + FAIL)) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/start.sh b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/start.sh index f80d00fbc..1c942a3c5 100755 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/start.sh +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/start.sh @@ -2,8 +2,21 @@ # Start OpenPLC + medkit gateway for manual testing. # Usage: from the ros2_medkit repo root, run # bash src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/start.sh +# +# MEDKIT_OPCUA_VARIANT=write-capable brings up an image with the OPC-UA write +# path compiled in. The default, read-only, is what ships. set -eo pipefail +VARIANT="${MEDKIT_OPCUA_VARIANT:-read-only}" +if [ "$VARIANT" = "read-only" ]; then + READ_ONLY=ON +elif [ "$VARIANT" = "write-capable" ]; then + READ_ONLY=OFF +else + echo "MEDKIT_OPCUA_VARIANT must be read-only or write-capable (got '$VARIANT')" >&2 + exit 2 +fi + SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" DOCKER_DIR="$(dirname "$SCRIPT_DIR")" PLUGIN_DIR="$(dirname "$DOCKER_DIR")" @@ -15,9 +28,10 @@ echo "=== Building OpenPLC ===" docker build -t openplc-tank "$DOCKER_DIR/openplc" 2>&1 | tail -3 echo "" -echo "=== Building gateway + OPC-UA plugin ===" +echo "=== Building gateway + OPC-UA plugin ($VARIANT) ===" cd "$REPO_ROOT" -docker build -f "$DOCKER_DIR/Dockerfile.gateway" -t gateway-opcua . 2>&1 | tail -5 +docker build --build-arg "MEDKIT_OPCUA_READ_ONLY=$READ_ONLY" \ + -f "$DOCKER_DIR/Dockerfile.gateway" -t gateway-opcua . 2>&1 | tail -5 echo "" echo "=== Starting containers ===" diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/test_all.sh b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/test_all.sh index a8520c931..121938630 100755 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/test_all.sh +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/test_all.sh @@ -3,8 +3,23 @@ # assertions against the OpenPLC tank demo, then clean up. # Usage: from the ros2_medkit repo root, run # bash src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/test_all.sh +# +# MEDKIT_OPCUA_VARIANT selects the write surface of the image under test +# (read-only, the default and what ships, or write-capable). It drives both the +# image build and the expectations the suite applies, so the two cannot drift. set -eo pipefail +VARIANT="${MEDKIT_OPCUA_VARIANT:-read-only}" +if [ "$VARIANT" = "read-only" ]; then + READ_ONLY=ON +elif [ "$VARIANT" = "write-capable" ]; then + READ_ONLY=OFF +else + echo "MEDKIT_OPCUA_VARIANT must be read-only or write-capable (got '$VARIANT')" >&2 + exit 2 +fi +export MEDKIT_OPCUA_VARIANT="$VARIANT" + SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" DOCKER_DIR="$(dirname "$SCRIPT_DIR")" PLUGIN_DIR="$(dirname "$DOCKER_DIR")" @@ -27,9 +42,10 @@ echo -e "${YELLOW}Step 1: Build OpenPLC container${NC}" docker build -t openplc-tank "$DOCKER_DIR/openplc" 2>&1 | tail -3 # 2. Build gateway image (includes ros2_medkit_opcua plugin) -echo -e "\n${YELLOW}Step 2: Build gateway + OPC-UA plugin image${NC}" +echo -e "\n${YELLOW}Step 2: Build gateway + OPC-UA plugin image (${VARIANT})${NC}" cd "$REPO_ROOT" -docker build -f "$DOCKER_DIR/Dockerfile.gateway" -t gateway-opcua . 2>&1 | tail -5 +docker build --build-arg "MEDKIT_OPCUA_READ_ONLY=$READ_ONLY" \ + -f "$DOCKER_DIR/Dockerfile.gateway" -t gateway-opcua . 2>&1 | tail -5 # 3. Start containers on isolated network echo -e "\n${YELLOW}Step 3: Start containers${NC}" From 186448bb8c9b7cc97d80c349ff87b7e5ac388c74 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 17:05:21 +0200 Subject: [PATCH 06/12] fix(opcua): close two holes in the build inspection's own premise 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 $, 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. --- .../ros2_medkit_opcua/CMakeLists.txt | 35 ++- .../ros2_medkit_opcua/design/index.rst | 8 + .../test/inspect_build_variant.py | 202 +++++++++++++++--- 3 files changed, 211 insertions(+), 34 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt b/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt index 1f868172d..9a318b38a 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt @@ -95,13 +95,22 @@ foreach(_op62_target open62541pp open62541) endforeach() # One section per function and per object in the vendored library, so -# --gc-sections below can drop the ones nothing in this module reaches. Without -# this the whole of open62541 is linked in as a handful of large sections and -# the garbage collector has nothing to work with: the read-only object would -# still carry - and, before --exclude-libs, still export - the library's own -# Write service primitives, which a caller with dlsym could reach even though no -# route in the plugin can. -foreach(_op62_target open62541pp open62541) +# --gc-sections on the module below can drop the ones nothing here reaches. +# Without it the library arrives as a handful of large sections the collector +# cannot take apart, and the read-only object keeps carrying open62541's own +# Write service primitives - __UA_Client_writeAttribute and the rest - even +# though no route in the plugin reaches them. +# +# The list names the OBJECT libraries, not just `open62541`: upstream compiles +# its C sources in open62541-object and open62541-plugins and assembles +# `open62541` from $, so options set on `open62541` reach no +# compilation at all. Upstream does add the same two flags itself, but only for +# Release and MinSizeRel (its CMakeLists.txt, "Strip release builds"), so a +# default-type build got neither - measurably: twelve UA_*_write* symbols in the +# read-only object there against none in Release. Setting them here makes the +# property independent of CMAKE_BUILD_TYPE; where upstream already set them the +# repetition is harmless. +foreach(_op62_target open62541pp open62541 open62541-object open62541-plugins) if(TARGET ${_op62_target}) target_compile_options(${_op62_target} PRIVATE -ffunction-sections -fdata-sections) endif() @@ -253,6 +262,18 @@ if(BUILD_TESTING) # Reads a file with nm; starts no ROS node. medkit_test_needs_no_domain(test_opcua_build_variant) + # A test of that test: it links an object exporting + # __UA_Client_writeAttribute alongside the plugin entry points - what a + # version script or a visibility slip on the vendored library produces - and + # requires the export rule to reject it. The rule used to be a prefix match on + # "UA_", which that spelling walks straight past. + add_test(NAME test_opcua_build_variant_self_check + COMMAND "${Python3_EXECUTABLE}" + "${CMAKE_CURRENT_SOURCE_DIR}/test/inspect_build_variant.py" + --self-check) + set_tests_properties(test_opcua_build_variant_self_check PROPERTIES TIMEOUT 60) + medkit_test_needs_no_domain(test_opcua_build_variant_self_check) + # Each test connects to a non-existent OPC UA host and waits ~3.8s for the # DNS / TCP failure path; with 13 tests in the suite the run requires ~90s. # The default ament_add_gtest timeout (60s) is too tight, causing the runner diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst b/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst index 8f96661ff..8d64d405e 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst @@ -123,6 +123,14 @@ binary. linker drop them from the object entirely, because nothing references them once the C++ write path is gone. The read-only object exports the six plugin entry points and nothing from the OPC-UA stack. +- The section flags are set on ``open62541-object`` and ``open62541-plugins``, + which is where upstream compiles its C sources - ``open62541`` itself is + assembled from ``$`` and compiles nothing. Upstream adds the + same two flags, but only under ``Release`` and ``MinSizeRel``, so a default-type + build previously kept twelve ``UA_*_write*`` symbols in the read-only object as + local, unexported code. Setting them here makes the property independent of + ``CMAKE_BUILD_TYPE``, and ``test_opcua_build_variant`` asserts the absence of + that whole family so the difference cannot come back unnoticed. - What is left of open62541 inside the object is the generic service dispatcher the read path needs and the generated type descriptors the ``UA_TYPES`` table pins. They are data and dispatch, not a write path: nothing exports them and no diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py index 4a6d47859..6fd223d8d 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py @@ -38,9 +38,12 @@ """ import argparse +from pathlib import Path +import re import shutil import subprocess import sys +import tempfile # Symbols that exist only when the value-write path is compiled in. Each is on # the road from the provider entry points to the wire: @@ -96,9 +99,25 @@ # Nothing from the OPC-UA stack may appear in the module's dynamic symbol table, # in either variant. The gateway dlopens the plugin and needs its extern "C" # entry points and nothing else; anything else exported is a dlsym handle on -# machinery no route exposes. Checked as a prefix match on the demangled name, -# which covers both the C library (UA_*, __UA_*) and the C++ wrapper (opcua::*). -EXPORT_DENY = ('UA_', 'opcua::') +# machinery no route exposes. +# +# A regex, not a prefix match on "UA_": open62541's internal entry points are +# spelled with leading underscores (__UA_Client_writeAttribute, +# __UA_Client_Service), so a startswith check waves through exactly the symbol +# that started this - a version script exporting it alongside the six entry +# points passed a prefix check while handing dlsym a working write primitive. +# Leading underscores are optional in the match for that reason. +EXPORT_DENY = re.compile(r'^_*UA_|^opcua::') + +# open62541's own Write service primitives, which the plugin never calls: the +# C++ wrapper reaches the service through opcua::services::write. They must be +# absent from a read-only object even as LOCAL symbols, not merely unexported - +# a symbol nothing exports is still a gadget for anything running in the same +# process, and their presence means the archive member was pulled in, which is +# the state a link or optimisation change can silently restore. Checked as an +# absence invariant rather than a variant marker because they are absent from +# the write-capable object too, so they discriminate nothing between builds. +WRITE_PRIMITIVES = re.compile(r'_*UA_(Client|Server)_write') # Symbols the gateway resolves out of the plugin. If a link-time change ever # hides these, the plugin still builds and still passes every symbol check above @@ -135,23 +154,17 @@ def count(lines, marker): return sum(1 for line in lines if marker in line) -def main(): - parser = argparse.ArgumentParser(description=__doc__) - parser.add_argument('plugin', help='path to libros2_medkit_opcua_plugin.so') - parser.add_argument('--expect', required=True, - choices=('read-only', 'write-capable'), - help='the write surface this build declares') - args = parser.parse_args() +def symbol_name(line): + """Return the demangled name from one nm line, without the address and type.""" + return line.split(' ', 2)[-1] - if shutil.which('nm') is None: - print('FAIL: nm (binutils) not on PATH - the build inspection cannot run', - file=sys.stderr) - return 1 - defined = nm(['-C'], args.plugin) - exported = nm(['-DC', '--defined-only'], args.plugin) - print(f'{args.plugin}: {len(defined)} symbols, {len(exported)} dynamic exports, ' - f'expecting {args.expect}') +def check_object(plugin, expect, report): + """Check one object against a variant and return the list of failures.""" + defined = nm(['-C'], plugin) + exported = nm(['-DC', '--defined-only'], plugin) + report(f'{plugin}: {len(defined)} symbols, {len(exported)} dynamic exports, ' + f'expecting {expect}') failures = [] if len(defined) < MIN_SYMBOLS: @@ -160,14 +173,14 @@ def main(): for marker in READ_MARKERS: n = count(defined, marker) - print(f' read {n:>4} {marker}') + report(f' read {n:>4} {marker}') if n == 0: failures.append(f'read path symbol missing: {marker}') - want_writes = args.expect == 'write-capable' + want_writes = expect == 'write-capable' for marker in WRITE_MARKERS: n = count(defined, marker) - print(f' write {n:>4} {marker}') + report(f' write {n:>4} {marker}') if want_writes and n == 0: failures.append(f'write-capable build is missing: {marker}') if not want_writes and n != 0: @@ -175,19 +188,154 @@ def main(): # The export table is an invariant, not a variant property: neither build # may hand dlsym a way into the OPC-UA stack. - leaked = [line.split(' ', 2)[-1] for line in exported - if any(line.split(' ', 2)[-1].startswith(p) for p in EXPORT_DENY)] - print(f' export {len(leaked):>4} OPC-UA symbols in the dynamic symbol table') + leaked = [symbol_name(line) for line in exported + if EXPORT_DENY.search(symbol_name(line))] + report(f' export {len(leaked):>4} OPC-UA symbols in the dynamic symbol table') for name in leaked[:10]: failures.append(f'dynamic symbol table exports OPC-UA machinery: {name}') - export_names = {line.split(' ', 2)[-1] for line in exported} + export_names = {symbol_name(line) for line in exported} missing = [name for name in REQUIRED_EXPORTS if name not in export_names] - print(f' export {len(REQUIRED_EXPORTS) - len(missing):>4}' - f'/{len(REQUIRED_EXPORTS)} plugin entry points') + report(f' export {len(REQUIRED_EXPORTS) - len(missing):>4}' + f'/{len(REQUIRED_EXPORTS)} plugin entry points') for name in missing: failures.append(f'plugin entry point not exported: {name}') + # And the library's Write primitives must not be in the read-only object at + # all, exported or not. Independent of build type: upstream only compiles + # with -ffunction-sections for Release and MinSizeRel, so without the flags + # this package sets on the object libraries a default-type build kept twelve + # of them as local symbols while every other check here still passed. + if not want_writes: + primitives = [symbol_name(line) for line in defined + if WRITE_PRIMITIVES.search(symbol_name(line))] + report(f' absent {len(primitives):>4} open62541 UA_*_write* primitives') + for name in primitives[:10]: + failures.append(f'read-only object still carries a write primitive: {name}') + + return failures + + +# The reviewer's reproducer, kept as a test of the test: a module that exports +# __UA_Client_writeAttribute next to the six entry points. It is what a version +# script, or an -fvisibility slip on the vendored library, produces, and the +# prefix match this check used to do waved it through - the leading underscores +# meant it did not start with "UA_". The object is otherwise a plausible plugin +# (the read markers and entry points are there), so a rejection can only come +# from the export rule under test. +SELF_CHECK_SOURCE = r""" +#include +#define EXPORT __attribute__((visibility("default"))) +EXPORT int __UA_Client_writeAttribute(void) { return 0; } +EXPORT void *create_plugin(void) { return 0; } +EXPORT int plugin_api_version(void) { return 1; } +EXPORT void *get_introspection_provider(void *p) { return p; } +EXPORT void *get_data_provider(void *p) { return p; } +EXPORT void *get_operation_provider(void *p) { return p; } +EXPORT void *get_fault_provider(void *p) { return p; } +""" + + +def build_self_check_object(workdir): + """Compile the reproducer module; return its path, or None with a reason.""" + compiler = shutil.which('cc') or shutil.which('gcc') + if compiler is None: + return None, 'no C compiler on PATH' + src = workdir / 'leaky_plugin.c' + obj = workdir / 'leaky_plugin.so' + src.write_text(SELF_CHECK_SOURCE) + out = subprocess.run( + [compiler, '-shared', '-fPIC', '-fvisibility=hidden', '-o', str(obj), str(src)], + capture_output=True, text=True, check=False) + if out.returncode != 0: + return None, f'compile failed: {out.stderr.strip()}' + return obj, '' + + +def self_check(): + """Prove the rules reject what they are meant to reject. Returns an exit code.""" + quiet = (lambda *a, **k: None) + problems = [] + + # The name rules, on symbol lines rather than on a whole object, so the + # spellings that matter are pinned one by one. + must_deny = ('__UA_Client_writeAttribute', 'UA_Client_writeValueAttribute', + '_UA_Server_write', '__UA_Client_Service', + 'opcua::services::write(opcua::Client&, opcua::WriteRequest const&)') + must_allow = ('create_plugin', 'plugin_api_version', 'medkit_UA_helper', + 'std::__cxx11::basic_string::~basic_string()') + for name in must_deny: + if not EXPORT_DENY.search(name): + problems.append(f'EXPORT_DENY fails to match {name}') + for name in must_allow: + if EXPORT_DENY.search(name): + problems.append(f'EXPORT_DENY wrongly matches {name}') + + must_be_primitives = ('__UA_Client_writeAttribute', 'UA_Client_writeArrayDimensionsAttribute', + 'UA_Server_writeValue', '__UA_Server_write') + must_not_be_primitives = ('__UA_Client_readAttribute', 'UA_Client_Service_read', + 'UA_WriteRequest_init') + for name in must_be_primitives: + if not WRITE_PRIMITIVES.search(name): + problems.append(f'WRITE_PRIMITIVES fails to match {name}') + for name in must_not_be_primitives: + if WRITE_PRIMITIVES.search(name): + problems.append(f'WRITE_PRIMITIVES wrongly matches {name}') + + # And the same rule end to end, on a real object built the way the reviewer + # built one. A compiler is present wherever this test runs, because the + # package it inspects was just compiled. + workdir = Path(tempfile.mkdtemp(prefix='opcua_self_check_')) + try: + obj, reason = build_self_check_object(workdir) + if obj is None: + print(f'FAIL: cannot build the reproducer object - {reason}', file=sys.stderr) + return 1 + failures = check_object(str(obj), 'read-only', quiet) + leak = [f for f in failures if '__UA_Client_writeAttribute' in f + and 'exports OPC-UA machinery' in f] + if not leak: + problems.append('an object exporting __UA_Client_writeAttribute was not ' + 'rejected by the export rule') + entry_points = [f for f in failures if 'entry point not exported' in f] + if entry_points: + problems.append('the reproducer object is not shaped like a plugin, so its ' + f'rejection proves nothing: {entry_points}') + finally: + shutil.rmtree(workdir, ignore_errors=True) + + if problems: + print('FAIL (self-check):', file=sys.stderr) + for p in problems: + print(f' - {p}', file=sys.stderr) + return 1 + print(f'PASS: self-check - {len(must_deny) + len(must_allow)} export-rule cases, ' + f'{len(must_be_primitives) + len(must_not_be_primitives)} write-primitive cases, ' + 'and a linked object exporting __UA_Client_writeAttribute is rejected') + return 0 + + +def main(): + parser = argparse.ArgumentParser(description=__doc__) + parser.add_argument('plugin', nargs='?', + help='path to libros2_medkit_opcua_plugin.so') + parser.add_argument('--expect', choices=('read-only', 'write-capable'), + help='the write surface this build declares') + parser.add_argument('--self-check', action='store_true', + help='check the rules against objects and names built to break them') + args = parser.parse_args() + + if args.self_check: + return self_check() + if not args.plugin or not args.expect: + parser.error('a plugin path and --expect are required unless --self-check is given') + + if shutil.which('nm') is None: + print('FAIL: nm (binutils) not on PATH - the build inspection cannot run', + file=sys.stderr) + return 1 + + failures = check_object(args.plugin, args.expect, print) if failures: print(f'FAIL ({args.expect}):', file=sys.stderr) for f in failures: From 43a792bc1ca82e896b79f9aca1b0c5c56442b6d8 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Sun, 6 Sep 2026 19:36:13 +0200 Subject: [PATCH 07/12] fix(opcua): a read-only build carries no alarm acknowledge either 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. --- .../ros2_medkit_opcua/README.md | 23 +++-- .../ros2_medkit_opcua/design/index.rst | 21 ++++- .../ros2_medkit_opcua/opcua_client.hpp | 25 ++++- .../ros2_medkit_opcua/src/opcua_client.cpp | 17 ++++ .../ros2_medkit_opcua/src/opcua_plugin.cpp | 40 +++++--- .../test/inspect_build_variant.py | 12 ++- .../integration/test_opcua_read_only.test.py | 92 ++++++++++++++++++- .../test/test_opcua_client.cpp | 17 ++++ .../test/test_opcua_plugin.cpp | 76 ++++++++++++++- 9 files changed, 296 insertions(+), 27 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md index f58dc86d2..b2294abb5 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md @@ -148,12 +148,18 @@ In the default read-only build: - Nothing advertises a write: no `x-plc-operations` capability on any entity, no `set_` entry in `/operations`, and the `POST .../x-plc-operations/...` route is not registered (it answers 404). -- `PUT /{type}/{id}/data/{name}` and the value-write half of +- **Alarm `acknowledge_fault` and `confirm_fault` are gone too.** A method call + that changes alarm state on the server is a write to the controller, and the + read-only build forbids every write path, not only value writes. The + `OpcuaClient` entry point that issues the Part 9 Acknowledge / Confirm calls + is not compiled, neither operation is listed on an event-alarm entity, and a + client that posts one anyway is refused. `ConditionRefresh` is unaffected and + stays in both variants: it asks the server to replay conditions it already + holds and changes nothing. +- `PUT /{type}/{id}/data/{name}` and `POST /{type}/{id}/operations/{name}/executions` answer **403** before any - node lookup or client call, with vendor code `x-medkit-plugin-error` and a - message naming `MEDKIT_OPCUA_READ_ONLY`. Alarm `acknowledge_fault` / - `confirm_fault` are Part 9 condition interactions, not value writes, and stay - available in both variants. + node lookup, condition lookup or client call, with vendor code + `x-medkit-plugin-error` and a message naming `MEDKIT_OPCUA_READ_ONLY`. A write-capable build restores everything above; nothing else differs. @@ -404,8 +410,11 @@ that catch-all code is never raised for it (see "System messages" under `auto_alarms` below). A mapping-level `severity_override` / `message` overrides the source-level one; otherwise the source-level value is inherited. -The plugin auto-registers `acknowledge_fault` and `confirm_fault` operations -on every entity that has at least one `event_alarms` entry. Invoke them with: +In a write-capable build the plugin auto-registers `acknowledge_fault` and +`confirm_fault` operations on every entity that has at least one `event_alarms` +entry. The default read-only build offers neither and refuses both - see +[Read-only and write-capable builds](#read-only-and-write-capable-builds). +Invoke them with: ```bash curl -X POST http://localhost:8080/api/v1/apps/tank_process/operations/acknowledge_fault/executions \ diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst b/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst index 8d64d405e..a0bcf93ee 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst @@ -149,8 +149,21 @@ binary. ``DataProvider`` / ``OperationProvider`` interfaces; the refusal reaches a client as SOVD vendor code ``x-medkit-plugin-error``, which is the code the gateway assigns to every plugin provider error. -- ``acknowledge_fault`` / ``confirm_fault`` are unaffected. They are OPC-UA Part 9 - condition method calls, not value writes. +- ``acknowledge_fault`` / ``confirm_fault`` go the same way. 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 carries no write path at all, not merely no value writes. + ``OpcuaClient::call_condition_method`` - the entry point that issues the Part 9 + Acknowledge (i=9111) and Confirm (i=9113) calls - is compiled only in a + write-capable build, ``list_operations`` offers neither operation on an + event-alarm entity, and ``execute_operation`` refuses both by name before any + condition lookup. +- ``ConditionRefresh`` stays, and so does the generic ``OpcuaClient::call_method`` + it rides on. Part 9 5.5.7 makes it a request for the server to replay the + conditions it already holds to the calling subscription; 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, they are present in both variants. The acceptance is an inspection of the built object, not a reading of the configuration: ``test_opcua_build_variant`` runs ``nm`` over the plugin ``.so`` and @@ -429,7 +442,9 @@ Acknowledge / Confirm round-trip -------------------------------- Two SOVD operations appear on every entity that has at least one event-mode -alarm declared: +alarm declared, in a write-capable build. Both change condition state on the +controller, so the default read-only build neither lists nor performs them (see +"Read-only is a property of the build" above): - ``POST /apps/{entity}/operations/acknowledge_fault/executions`` - ``POST /apps/{entity}/operations/confirm_fault/executions`` diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_client.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_client.hpp index cffad93ba..9888798ca 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_client.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_client.hpp @@ -303,13 +303,34 @@ class OpcuaClient { }; /// Synchronously call an OPC-UA Method on a target object. - /// Used by ConditionRefresh, Acknowledge, and Confirm operations on - /// AlarmConditionType nodes (issue #386). + /// The read path uses it for ConditionRefresh, which asks the server to + /// replay active conditions to this subscription and changes nothing on the + /// controller (Part 9 5.5.7). /// @return Output arguments on success, MethodErrorInfo on failure. tl::expected, MethodErrorInfo> call_method(const opcua::NodeId & object_id, const opcua::NodeId & method_id, const std::vector & input_args); +#if !MEDKIT_OPCUA_READ_ONLY + /// Acknowledge or Confirm a live condition (Part 9 Acknowledge i=9111, + /// Confirm i=9113), present only in a write-capable build. + /// + /// Separate from ``call_method`` because it is a different kind of call: it + /// changes alarm state on the server, and a read-only build forbids every + /// write path, not only value writes. ``call_method`` itself stays, because + /// ConditionRefresh rides on it and asks the server for a replay rather than + /// changing anything. + /// + /// @param condition_id ConditionId of the live instance to act on. + /// @param method_id Acknowledge or Confirm on AcknowledgeableConditionType. + /// @param event_id EventId of the event being acknowledged. + /// @param comment Operator comment, forwarded as a LocalizedText. + tl::expected, MethodErrorInfo> call_condition_method(const opcua::NodeId & condition_id, + const opcua::NodeId & method_id, + const opcua::ByteString & event_id, + const std::string & comment); +#endif + /// Map an OPC-UA StatusCode (from an attempted method call or a /// per-argument validation result) to a ``MethodError`` category. /// Exposed as a public static helper so the classification table is diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_client.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_client.cpp index ecbe52a78..219b6b071 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_client.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_client.cpp @@ -1827,6 +1827,23 @@ OpcuaClient::classify_call_result(uint32_t overall_status_code, const std::vecto return {}; } +#if !MEDKIT_OPCUA_READ_ONLY +// The Part 9 Acknowledge / Confirm entry point. It builds the two arguments +// those methods take and hands them to the generic Call service. A read-only +// build does not compile it: acknowledging an alarm changes condition state on +// the controller, which is a write, and the object must carry no way to make +// one. ConditionRefresh keeps using call_method below - it asks the server to +// replay what it already holds. +tl::expected, OpcuaClient::MethodErrorInfo> +OpcuaClient::call_condition_method(const opcua::NodeId & condition_id, const opcua::NodeId & method_id, + const opcua::ByteString & event_id, const std::string & comment) { + std::vector args; + args.push_back(opcua::Variant::fromScalar(event_id)); + args.push_back(opcua::Variant::fromScalar(opcua::LocalizedText("", comment))); + return call_method(condition_id, method_id, args); +} +#endif // !MEDKIT_OPCUA_READ_ONLY + tl::expected, OpcuaClient::MethodErrorInfo> OpcuaClient::call_method(const opcua::NodeId & object_id, const opcua::NodeId & method_id, const std::vector & input_args) { diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp index bf978230d..b99d832b9 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp @@ -1800,10 +1800,15 @@ OpcuaPlugin::list_operations(const std::string & entity_id) { collection.items.push_back(std::move(item)); } +#if !MEDKIT_OPCUA_READ_ONLY // Issue #386: emit acknowledge_fault / confirm_fault when the entity // has at least one native AlarmConditionType event subscription. The // fault_code parameter (passed in operation execution body) discriminates // which condition the operator is acting on. + // + // Not offered by a read-only build. Acknowledging or confirming a condition + // changes alarm state on the controller, so it is a write like any other, + // and the tree must not advertise what the binary cannot do. bool has_event_alarms = std::any_of(node_map_.event_alarms().begin(), node_map_.event_alarms().end(), [&entity_id](const AlarmEventConfig & cfg) { return cfg.entity_id == entity_id; @@ -1823,6 +1828,7 @@ OpcuaPlugin::list_operations(const std::string & entity_id) { confirm.asynchronous_execution = false; collection.items.push_back(std::move(confirm)); } +#endif return collection; } @@ -1839,6 +1845,22 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string OperationProviderErrorInfo{OperationProviderError::Internal, "plugin not initialized", 503}); } +#if MEDKIT_OPCUA_READ_ONLY + // Neither branch below reads these: every operation is refused on its name + // alone, before any entity lookup or parameter parsing. + (void)entity_id; + (void)parameters; + + // Acknowledge and Confirm change alarm state on the controller, so they are + // write paths and this build carries neither. Refused before any condition + // lookup or client call, with the same answer a value write gets - nothing + // advertises them either, so this is the backstop for a client that posts the + // operation id directly. + if (operation_name == "acknowledge_fault" || operation_name == "confirm_fault") { + return tl::make_unexpected( + OperationProviderErrorInfo{OperationProviderError::Rejected, kReadOnlyBuildRefusal, 403}); + } +#else // Issue #386: acknowledge_fault and confirm_fault dispatch to OPC-UA // Method calls on the live ConditionId. AcknowledgeableConditionType // declares Acknowledge as method i=9111 and Confirm as method i=9113; @@ -1883,10 +1905,6 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string constexpr uint32_t kConfirmMethodId = 9113; opcua::NodeId method_id(0, operation_name == "acknowledge_fault" ? kAcknowledgeMethodId : kConfirmMethodId); - std::vector args; - args.push_back(opcua::Variant::fromScalar(runtime->latest_event_id)); - args.push_back(opcua::Variant::fromScalar(opcua::LocalizedText("", comment))); - if (plugin_debug_enabled()) { std::ostringstream hex_oss; const auto * bytes = runtime->latest_event_id.data(); @@ -1900,7 +1918,7 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string << " conditionId=" << runtime->condition_id.toString()); } - auto result = client_->call_method(runtime->condition_id, method_id, args); + auto result = client_->call_condition_method(runtime->condition_id, method_id, runtime->latest_event_id, comment); if (!result.has_value()) { auto code = result.error().code; int http = 502; @@ -1929,14 +1947,14 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string out["condition_id"] = runtime->condition_id.toString(); return dto::OperationExecutionResult{std::move(out)}; } +#endif // MEDKIT_OPCUA_READ_ONLY #if MEDKIT_OPCUA_READ_ONLY - // Everything past the acknowledge / confirm branch above is the value-write - // path, and this build does not contain it. Nothing advertised reaches here - - // a read-only build marks no point writable, so list_operations emits no - // set_* entry - and a request aimed straight at one is refused before any - // node lookup or client call. Acknowledging an alarm is a Part 9 condition - // interaction, not a value write, and stays available. + // Everything past the branch above is the value-write path, and this build + // does not contain it either. Nothing advertised reaches here - a read-only + // build marks no point writable, so list_operations emits no set_* entry - + // and a request aimed straight at one is refused before any node lookup or + // client call. return tl::make_unexpected(OperationProviderErrorInfo{OperationProviderError::Rejected, kReadOnlyBuildRefusal, 403}); #else std::string data_name; diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py index 6fd223d8d..a7d505362 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py @@ -53,7 +53,15 @@ # - Node::writeValueScalar / writeValue are the open62541pp templates # where the value is encoded for the Write service; # - services::write / services::writeAttribute are the layer beneath -# them, the last C++ frame before open62541's own client machinery. +# them, the last C++ frame before open62541's own client machinery; +# - call_condition_method issues the Part 9 Acknowledge / Confirm calls, which +# change alarm state on the controller and are a write like any other. +# +# call_method and opcua::services::call are deliberately NOT markers even though +# the acknowledge path runs through them: ConditionRefresh runs through them +# too, asking the server to replay conditions it already holds, and that is a +# read the shipped build keeps. Measured 3/3 and 1/1 across the two variants - +# they discriminate nothing. # # Every marker is verified to discriminate in an OPTIMIZED build, which is what # CI and every release produce. That rules out opcua::services::writeValue and @@ -82,6 +90,7 @@ 'opcua::Node::writeValue(opcua::Variant const&)', 'opcua::services::write(opcua::Client&, opcua::WriteRequest const&)', 'opcua::services::writeAttribute', + 'ros2_medkit_gateway::OpcuaClient::call_condition_method(', ) # The read path the plugin needs in every variant. Present in both builds, so a @@ -94,6 +103,7 @@ 'ros2_medkit_gateway::OpcuaClient::read_values(', 'ros2_medkit_gateway::OpcuaClient::read_access_level(', 'ros2_medkit_gateway::OpcuaClient::browse_detailed(', + 'ros2_medkit_gateway::OpcuaClient::call_method(', ) # Nothing from the OPC-UA stack may appear in the module's dynamic symbol table, diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py index 52c916ba5..a8fad10ed 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py @@ -24,6 +24,9 @@ * the ``infer_writable`` sweep (true / false / absent) over the address-space walk, against the same node; * the SOVD write endpoints and the vendor write route; + * the Part 9 alarm acknowledge, against a condition the fixture really + raises - acknowledging changes condition state on the controller, so it is + a write and a read-only build neither offers nor performs it; * what the entity tree advertises; * and a reconnect, so the answer does not change when the address space is walked a second time. @@ -57,6 +60,7 @@ SECOND_WRITABLE_NODE = 'ns=2;s=FaultCode' ENTITY = 'plc_app' +ALARM_CODE = 'PLC_OVERPRESSURE' COMPONENT = 'read_only_runtime' # The vendor code the gateway puts on the wire for any plugin provider refusal @@ -183,7 +187,7 @@ def start_server(server_bin, port, log_path): log = open(log_path, 'w') proc = subprocess.Popen( [str(server_bin), '--port', str(port)], - stdin=subprocess.DEVNULL, stdout=log, stderr=subprocess.STDOUT, + stdin=subprocess.PIPE, stdout=log, stderr=subprocess.STDOUT, text=True, start_new_session=True, ) proc._log = log @@ -193,6 +197,18 @@ def start_server(server_bin, port, log_path): return proc +def send_cmd(server, cmd): + """Write one alarm-CLI command to the fixture; False when it is already gone.""" + if server.poll() is not None or server.stdin is None: + return False + try: + server.stdin.write(cmd + '\n') + server.stdin.flush() + except (BrokenPipeError, ValueError): + return False + return True + + def write_params(path, *, port, plugin, server_port, node_map, manifest, auto_browse): """Render a gateway params file for one leg of the sweep.""" lines = [ @@ -374,6 +390,79 @@ def run_node_map_leg(workdir, env, plugin, server_port, manifest, read_only): terminate(gw) +def alarm_map_text(): + """Map one of the fixture's AlarmConditionType sources onto an entity.""" + return ( + 'area_id: plc_systems\n' + f'component_id: {COMPONENT}\n' + 'nodes:\n' + f' - node_id: "{READ_ONLY_NODE}"\n' + f' entity_id: {ENTITY}\n' + ' data_name: tank_level\n' + ' data_type: float\n' + 'event_alarms:\n' + ' - alarm_source: "ns=2;s=Alarms.Overpressure"\n' + f' entity_id: {ENTITY}\n' + f' fault_code: {ALARM_CODE}\n' + ) + + +def run_alarm_leg(workdir, env, plugin, server, server_port, manifest, read_only): + """Acknowledge is a controller state change, so it follows the same rule.""" + print('--- alarm acknowledge / confirm leg ---') + node_map = workdir / 'alarm_nodes.yaml' + node_map.write_text(alarm_map_text()) + port = free_port() + params = workdir / 'gateway_alarm.yaml' + write_params(params, port=port, plugin=plugin, server_port=server_port, + node_map=node_map, manifest=manifest, auto_browse=None) + log = workdir / 'gateway_alarm.log' + gw = start_gateway(params, log, env) + try: + base = f'http://127.0.0.1:{port}/api/v1' + status = wait_json(f'{base}/components/{COMPONENT}/x-plc-status', + lambda j: j.get('connected') is True, deadline=90) + if not check(bool(status) and status.get('connected') is True, + 'the gateway connected for the alarm leg'): + print(log.read_text(errors='replace')[-3000:], file=sys.stderr) + return + + # 1. What the entity offers. + _s, ops = http(f'{base}/apps/{ENTITY}/operations') + op_ids = {o.get('id') for o in (ops or {}).get('items', [])} + for op in ('acknowledge_fault', 'confirm_fault'): + check((op in op_ids) is not read_only, + f'{op} offered: {not read_only} (got {sorted(op_ids)})') + + # 2. Raise a real condition, so the write-capable call has something + # live to act on and the read-only refusal is not just "not found". + if not check(send_cmd(server[0], 'fire Overpressure 750'), + 'fired an alarm on the fixture server'): + return + exec_url = f'{base}/apps/{ENTITY}/operations/acknowledge_fault/executions' + body = {'fault_code': ALARM_CODE, 'comment': 'acked by the integration test'} + if read_only: + status, payload = http(exec_url, 'POST', body) + refusal_is_the_build(status, payload, 'POST acknowledge_fault') + else: + # The condition has to reach the poller's registry first; the + # subscription delivers it a moment after the server emits it. + got = None + for _ in range(30): + status, payload = http(exec_url, 'POST', body) + if status in (200, 202): + got = payload + break + time.sleep(2) + check(got is not None, + f'POST acknowledge_fault succeeds (last: {status} {payload!r})') + if got is not None: + check(got.get('status') == 'ok' and got.get('fault_code') == ALARM_CODE, + f'the acknowledge reached the server ({got!r})') + finally: + terminate(gw) + + def auto_browse_writable(base, deadline=90): """Return the writable flag of the auto-browsed StatusWord point, or None.""" payload = wait_json(f'{base}/apps', lambda j: j.get('items'), deadline=deadline) @@ -482,6 +571,7 @@ def main(): return 1 run_node_map_leg(workdir, env, plugin, server_port, manifest, read_only) + run_alarm_leg(workdir, env, plugin, server, server_port, manifest, read_only) for infer_writable, rebrowse in (('absent', True), ('true', False), ('false', False)): run_auto_browse_leg(workdir, env, plugin, server_bin, server, server_port, manifest, read_only, infer_writable, rebrowse) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_client.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_client.cpp index 557caebc2..5b0860ff8 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_client.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_client.cpp @@ -306,6 +306,8 @@ TEST(OpcuaClientTest, RemoveEventMonitoredItemUnknownIdDoesNotBumpGeneration) { EXPECT_EQ(client.current_generation(), before); } +// call_method itself is in both builds: ConditionRefresh rides on it and asks +// the server to replay conditions rather than changing any. TEST(OpcuaClientTest, CallMethodWhenDisconnected) { OpcuaClient client; auto result = client.call_method(opcua::NodeId(0, UA_NS0ID_SERVER), opcua::NodeId(0, 11489), {}); @@ -313,6 +315,21 @@ TEST(OpcuaClientTest, CallMethodWhenDisconnected) { EXPECT_EQ(result.error().code, OpcuaClient::MethodError::NotConnected); } +#if !MEDKIT_OPCUA_READ_ONLY +// Acknowledge / Confirm change condition state on the controller, so the entry +// point that issues them exists only in a write-capable build - the same rule +// write_value follows, and what test_opcua_build_variant checks on the object. +TEST(OpcuaClientTest, CallConditionMethodWhenDisconnected) { + OpcuaClient client; + constexpr uint32_t kAcknowledgeMethodId = 9111; + auto result = + client.call_condition_method(opcua::NodeId(2, "Alarms.Overpressure"), opcua::NodeId(0, kAcknowledgeMethodId), + opcua::ByteString("event-id"), "comment"); + ASSERT_FALSE(result.has_value()); + EXPECT_EQ(result.error().code, OpcuaClient::MethodError::NotConnected); +} +#endif // !MEDKIT_OPCUA_READ_ONLY + TEST(OpcuaClientTest, GenerationBumpsOnDisconnect) { OpcuaClient client; // The disconnect-without-connect path is a no-op - generation should not diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp index e0e150e10..bb7480135 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp @@ -417,8 +417,10 @@ TEST_F(OpcuaPluginTest, HasOperationsFalseForUnknownEntity) { // that posts the operation id directly still has to be refused, and refused for // the reason that is true. TEST_F(OpcuaPluginTest, ExecuteOperationRefusedOnAReadOnlyBuild) { - const std::vector ops{"set_level", "set_pressure", "set_nonexistent"}; - const std::vector params_list{nlohmann::json{{"value", 5.0}}, nlohmann::json{{"not_value", 42}}}; + const std::vector ops{"set_level", "set_pressure", "set_nonexistent", "acknowledge_fault", + "confirm_fault"}; + const std::vector params_list{nlohmann::json{{"value", 5.0}}, nlohmann::json{{"not_value", 42}}, + nlohmann::json{{"fault_code", "PLC_OVERPRESSURE"}}}; for (const auto & op : ops) { for (const auto & params : params_list) { auto result = plugin_.execute_operation("tank", op, params); @@ -446,6 +448,76 @@ TEST_F(OpcuaPluginTest, ExecuteOperationReadOnly) { } #endif +// -- Condition operations (acknowledge / confirm) per build variant -------- +// +// Acknowledging an alarm changes condition state on the controller, so a +// read-only build neither offers nor performs it. An entity with an +// event_alarms source is the only one those operations are ever offered on. + +class OpcuaPluginEventAlarmsTest : public ::testing::Test { + protected: + void SetUp() override { + yaml_path_ = "/tmp/test_opcua_plugin_event_alarms_nodemap.yaml"; + std::ofstream f(yaml_path_); + f << R"( +area_id: test_plc +component_id: test_runtime +nodes: + - node_id: "ns=2;i=1" + entity_id: tank + data_name: level + data_type: float +event_alarms: + - alarm_source: "ns=2;s=Alarms.Overpressure" + entity_id: tank + fault_code: PLC_OVERPRESSURE +)"; + f.close(); + + nlohmann::json config; + config["node_map_path"] = yaml_path_; + config["endpoint_url"] = "opc.tcp://nonexistent:4840"; + plugin_.configure(config); + ctx_.entities["tank"] = {SovdEntityType::APP, "tank", "/test_plc", "/test_plc/test_runtime/tank"}; + plugin_.set_context(ctx_); + } + + std::string yaml_path_; + OpcuaPlugin plugin_; + FakePluginContext ctx_; +}; + +TEST_F(OpcuaPluginEventAlarmsTest, ConditionOperationsOfferedOnlyWhenTheBuildCanPerformThem) { + auto result = plugin_.list_operations("tank"); + ASSERT_TRUE(result.has_value()); + std::vector ids; + for (const auto & item : result->items) { + ids.push_back(item.id); + } + const bool has_ack = std::find(ids.begin(), ids.end(), "acknowledge_fault") != ids.end(); + const bool has_confirm = std::find(ids.begin(), ids.end(), "confirm_fault") != ids.end(); +#if MEDKIT_OPCUA_READ_ONLY + EXPECT_FALSE(has_ack) << "a read-only build must not advertise an alarm acknowledge"; + EXPECT_FALSE(has_confirm) << "a read-only build must not advertise an alarm confirm"; +#else + EXPECT_TRUE(has_ack); + EXPECT_TRUE(has_confirm); +#endif +} + +#if MEDKIT_OPCUA_READ_ONLY +TEST_F(OpcuaPluginEventAlarmsTest, ConditionOperationsRefusedOnAReadOnlyBuild) { + for (const auto & op : {std::string("acknowledge_fault"), std::string("confirm_fault")}) { + auto result = plugin_.execute_operation("tank", op, nlohmann::json{{"fault_code", "PLC_OVERPRESSURE"}}); + ASSERT_FALSE(result.has_value()) << op; + EXPECT_EQ(result.error().code, OperationProviderError::Rejected); + EXPECT_EQ(result.error().http_status, 403); + EXPECT_NE(result.error().message.find("MEDKIT_OPCUA_READ_ONLY"), std::string::npos) + << "the refusal must name the build property, got: " << result.error().message; + } +} +#endif + // -- auto_alarms fallback entity: has data/operations fitness + introspect // -- capability registration -- // From 7a10e5e9f12c01939e15908be76aa2abdac5b296 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 7 Sep 2026 10:26:13 +0200 Subject: [PATCH 08/12] test(opcua): run the alarm suite against both images, not just the write 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. --- .github/workflows/opcua-plugin.yml | 15 +- .../docker/scripts/run_alarm_tests.sh | 187 ++++++++++++++---- 2 files changed, 163 insertions(+), 39 deletions(-) diff --git a/.github/workflows/opcua-plugin.yml b/.github/workflows/opcua-plugin.yml index 44fb3eff3..fbe8d033e 100644 --- a/.github/workflows/opcua-plugin.yml +++ b/.github/workflows/opcua-plugin.yml @@ -244,13 +244,24 @@ jobs: docker network rm plc-demo 2>/dev/null || true integration-alarms: - name: Integration (AlarmConditionType) + name: Integration (AlarmConditionType, ${{ matrix.variant }}) # Issue #386: tests the native OPC-UA AlarmCondition subscription bridge # against the test_alarm_server fixture (open62541 with FULL ns0 + alarms # ON). Independent of the OpenPLC threshold-mode integration above; runs # in parallel. + # + # Both write surfaces, like the OpenPLC job: acknowledging a condition + # changes its state on the controller, so the read-only leg asserts the + # refusal and that the condition stays unacknowledged on the server, and the + # write-capable leg asserts the acknowledge reaches it. The script builds the + # gateway image itself and derives the build arg from the same variable, so + # the image and the expectations cannot drift apart. runs-on: ubuntu-latest timeout-minutes: 30 + strategy: + fail-fast: false + matrix: + variant: [read-only, write-capable] steps: - name: Checkout repository uses: actions/checkout@v4 @@ -262,6 +273,8 @@ jobs: pip3 install --break-system-packages asyncua - name: Run alarm integration suite + env: + MEDKIT_OPCUA_VARIANT: ${{ matrix.variant }} run: bash src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_alarm_tests.sh - name: Dump container logs on failure diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_alarm_tests.sh b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_alarm_tests.sh index e8f09b6d1..9ba42a6f4 100755 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_alarm_tests.sh +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_alarm_tests.sh @@ -8,12 +8,32 @@ # # Acknowledge / Confirm round-trips go through the SOVD HTTP path # (POST /apps/{entity}/operations/{op}/executions) so that the medkit -# implementation - lookup_condition + EventId tracking + call_method on the -# inherited AcknowledgeableConditionType methods - is exercised end-to-end, -# not bypassed via the server stdin shortcuts. +# implementation - lookup_condition + EventId tracking + the condition-method +# call on the inherited AcknowledgeableConditionType methods - is exercised +# end-to-end, not bypassed via the server stdin shortcuts. +# +# MEDKIT_OPCUA_VARIANT selects the write surface of the image under test and +# has to match how it was built (docker build --build-arg +# MEDKIT_OPCUA_READ_ONLY=ON|OFF); this script builds the image itself, so it +# derives the build arg from the variable and the two cannot drift. It defaults +# to read-only, which is what the plugin and the Dockerfile default to. +# Acknowledging a condition changes its state on the controller, so a read-only +# image refuses it: that leg asserts the refusal and that the condition is still +# unacknowledged on the server, then drives the same lifecycle through the +# server's own CLI so every downstream expectation still runs. set -euo pipefail +VARIANT="${MEDKIT_OPCUA_VARIANT:-read-only}" +if [[ "${VARIANT}" == "read-only" ]]; then + READ_ONLY=ON +elif [[ "${VARIANT}" == "write-capable" ]]; then + READ_ONLY=OFF +else + echo "MEDKIT_OPCUA_VARIANT must be read-only or write-capable (got '${VARIANT}')" >&2 + exit 2 +fi + REPO_ROOT="$(cd "$(dirname "$0")/../../../../.." && pwd)" NET_NAME=alarm-test-net SERVER_NAME=alarm-test-server @@ -52,7 +72,7 @@ trap cleanup EXIT # existing run_integration_tests.sh convention; never sleeps blindly. wait_for() { local url="$1" expr="$2" deadline="${3:-60}" - for i in $(seq 1 "${deadline}"); do + for _ in $(seq 1 "${deadline}"); do if curl -sf "${url}" 2>/dev/null | jq -e "${expr}" >/dev/null 2>&1; then return 0 fi @@ -71,7 +91,7 @@ wait_for() { wait_no_fault() { local fault_code="$1" deadline="${2:-30}" local url="http://localhost:${GATEWAY_PORT}/api/v1/faults" - for i in $(seq 1 "${deadline}"); do + for _ in $(seq 1 "${deadline}"); do local status status=$(curl -sf "${url}" 2>/dev/null \ | jq -r --arg code "${fault_code}" \ @@ -101,12 +121,26 @@ assert_status() { echo " OK ${fault_code}: ${actual}" } +assert_fault_present() { + local fault_code="$1" + local status + status=$(curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/faults" \ + | jq -r --arg code "${fault_code}" \ + '.items[] | select(.fault_code == $code) | .status' \ + | head -1) + if [[ -z "${status}" || "${status}" == "CLEARED" ]]; then + echo "ASSERT FAILED: ${fault_code} expected still raised, got '${status:-absent}'" >&2 + return 1 + fi + echo " OK ${fault_code} still raised (${status})" +} + # Poll the global ``/api/v1/faults`` list until the named fault has the # expected status. Mirrors ``wait_for`` but specialized for the fault list # shape so callers do not need to construct jq filters per scenario. wait_until_status() { local fault_code="$1" expected="$2" deadline="${3:-30}" - for i in $(seq 1 "${deadline}"); do + for _ in $(seq 1 "${deadline}"); do local actual actual=$(curl -sf "http://localhost:${GATEWAY_PORT}/api/v1/faults" 2>/dev/null \ | jq -r --arg code "${fault_code}" \ @@ -129,7 +163,7 @@ wait_until_status() { # corresponding state on the OPC-UA server. assert_server_state() { local condition="$1" key="$2" expected="$3" deadline="${4:-30}" - for i in $(seq 1 "${deadline}"); do + for _ in $(seq 1 "${deadline}"); do local line line=$(docker logs "${SERVER_NAME}" 2>&1 | grep -E "^STATE ${condition} " | tail -1 || true) if [[ "${line}" == *"${key}=${expected}"* ]]; then @@ -157,6 +191,56 @@ sovd_post_op() { echo " OK POST ${op} -> ${code}" } +# The read-only counterpart: the same POST has to come back as the plugin's +# refusal, not as a transport error or a 404, and the body has to name the build +# property so an operator reading it knows no credential will help. +sovd_post_op_refused() { + local op="$1" body="$2" + local url="http://localhost:${GATEWAY_PORT}/api/v1/apps/tank_process/operations/${op}/executions" + local code + code=$(curl -s -o /tmp/alarm_test_resp.json -w '%{http_code}' \ + -X POST -H 'Content-Type: application/json' -d "${body}" "${url}") + if [[ "${code}" != "403" ]]; then + echo "ASSERT FAILED: POST ${op} expected HTTP 403, got ${code}" >&2 + cat /tmp/alarm_test_resp.json >&2 || true + return 1 + fi + if ! jq -e '.vendor_code == "x-medkit-plugin-error"' /tmp/alarm_test_resp.json >/dev/null; then + echo "ASSERT FAILED: POST ${op} refusal carries no x-medkit-plugin-error vendor code" >&2 + cat /tmp/alarm_test_resp.json >&2 || true + return 1 + fi + if ! jq -e '.message | test("MEDKIT_OPCUA_READ_ONLY")' /tmp/alarm_test_resp.json >/dev/null; then + echo "ASSERT FAILED: POST ${op} refusal does not name MEDKIT_OPCUA_READ_ONLY" >&2 + cat /tmp/alarm_test_resp.json >&2 || true + return 1 + fi + echo " OK POST ${op} refused 403 x-medkit-plugin-error naming MEDKIT_OPCUA_READ_ONLY" +} + +# What the tree advertises: a read-only image must not offer an operation it +# will refuse, and a write-capable one must offer both. +assert_operations_offered() { + local url="http://localhost:${GATEWAY_PORT}/api/v1/apps/tank_process/operations" + local ids + ids=$(curl -s "${url}" | jq -c '[.items[].id]') + for op in acknowledge_fault confirm_fault; do + if [[ "${VARIANT}" == "read-only" ]]; then + if jq -e --arg op "${op}" 'contains([$op])' <<<"${ids}" >/dev/null; then + echo "ASSERT FAILED: read-only image advertises ${op} (${ids})" >&2 + return 1 + fi + echo " OK ${op} not advertised" + else + if ! jq -e --arg op "${op}" 'contains([$op])' <<<"${ids}" >/dev/null; then + echo "ASSERT FAILED: write-capable image does not advertise ${op} (${ids})" >&2 + return 1 + fi + echo " OK ${op} advertised" + fi + done +} + # Poll the gateway's docker logs until appears. Required because the # AlarmConditionType subscription has a 500 ms server-side publishing interval - # new events from method calls or stdin commands take up to that long to arrive @@ -165,7 +249,7 @@ sovd_post_op() { # rejects stale IDs with BadEventIdUnknown). wait_gateway_log() { local pattern="$1" deadline="${2:-30}" - for i in $(seq 1 "${deadline}"); do + for _ in $(seq 1 "${deadline}"); do if docker logs "${GATEWAY_NAME}" 2>&1 | grep -q -- "${pattern}"; then return 0 fi @@ -190,8 +274,9 @@ docker build --network=host \ -f src/ros2_medkit_plugins/ros2_medkit_opcua/docker/test_alarm_server/Dockerfile \ -t ros2_medkit_alarm_test_server:dev . >/dev/null -echo "[2/5] Build gateway-opcua image (re-uses existing Dockerfile.gateway)" +echo "[2/5] Build gateway-opcua image (${VARIANT}, re-uses existing Dockerfile.gateway)" docker build --network=host \ + --build-arg "MEDKIT_OPCUA_READ_ONLY=${READ_ONLY}" \ -f src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway \ -t gateway-opcua:alarm-test . >/dev/null @@ -215,7 +300,7 @@ docker run --rm --name "${SERVER_NAME}" --network "${NET_NAME}" \ <&3 >/dev/null 2>&1 & SERVER_DOCKER_PID=$! # Wait for server to bind; the binary prints "READY ..." after listen. -for i in $(seq 1 30); do +for _ in $(seq 1 30); do if docker logs "${SERVER_NAME}" 2>&1 | grep -q '^READY '; then break fi @@ -299,7 +384,7 @@ docker run -d --name "${GATEWAY_NAME}" --network "${NET_NAME}" \ # sometimes too short, leaving the gateway to come up before the # service was discoverable. ``ros2 service list`` is the cheapest # ROS-native availability signal. - for i in $(seq 1 30); do + for _ in $(seq 1 30); do if ros2 service list 2>/dev/null | grep -q "/fault_manager/report_fault"; then break fi @@ -320,7 +405,8 @@ wait_for "http://localhost:${GATEWAY_PORT}/api/v1/apps" \ echo "[5/5] Run alarm scenarios" -echo " [scenario] fire / SOVD ack / latch / SOVD confirm / clear lifecycle" +echo " [scenario] fire / ack / latch / confirm / clear lifecycle (${VARIANT})" +assert_operations_offered echo "fire Overpressure 750" >&3 wait_until_status PLC_OVERPRESSURE CONFIRMED 30 @@ -332,31 +418,56 @@ wait_until_status PLC_OVERPRESSURE CONFIRMED 30 # HEALED verb - we deliberately do not flip fault_manager into PASSED-debounce # territory). The lifecycle proof is therefore ``wait_no_fault`` after the # follow-up SOVD confirm + the OPC-UA event with all three states cleared. -sovd_post_op acknowledge_fault \ - '{"fault_code":"PLC_OVERPRESSURE","comment":"e2e ack via SOVD"}' - -# Latch flips ActiveState=false on the server. Combined with the AckedState= -# true set by the SOVD ack above, the next AlarmCondition event payload has -# active=false, acked=true, confirmed=false -> SovdAlarmStatus::Healed -# (state machine internal), action=ReportHealed (no-op for fault_manager). -# /faults still shows CONFIRMED here, by design. -echo "latch Overpressure" >&3 - -# Wait for the gateway to actually receive and process the latch event before -# issuing SOVD confirm. Without this, the gateway still has the EventId from -# the original fire payload and the OPC-UA Confirm method on the server -# returns BadEventIdUnknown (the server's branch->lastEventId has been -# superseded by the Acknowledge auto-emit and the latch trigger). -wait_gateway_log "AlarmCondition HEALED.*PLC_OVERPRESSURE" 20 - -# Real SOVD confirm - exercises call_method(i=9113) + EventId. After this -# ConfirmedState=true on the server; the resulting event has all three of -# Active=false, Acked=true, Confirmed=true and the state machine emits -# ClearFault, removing the entry from /faults. -sovd_post_op confirm_fault \ - '{"fault_code":"PLC_OVERPRESSURE","comment":"e2e confirm via SOVD"}' -wait_no_fault PLC_OVERPRESSURE 30 -echo " OK PLC_OVERPRESSURE cleared after SOVD ack + latch + SOVD confirm" +if [[ "${VARIANT}" == "read-only" ]]; then + # The image carries no way to acknowledge or confirm, so both calls are + # refused. Proving they never reached the server needs a consequence rather + # than a STATE line: the fixture prints those only from its own stdin CLI, so + # a condition acknowledged over OPC-UA looks identical to one that was not. + # The consequence used here is that the fault does not clear - had both calls + # landed, the latch below would have driven it to cleared - and the CLI-driven + # ack + confirm afterwards does clear it, which is what makes the negative + # meaningful instead of a symptom of a stalled pipeline. + sovd_post_op_refused acknowledge_fault \ + '{"fault_code":"PLC_OVERPRESSURE","comment":"e2e ack via SOVD"}' + sovd_post_op_refused confirm_fault \ + '{"fault_code":"PLC_OVERPRESSURE","comment":"e2e confirm via SOVD"}' + + echo "latch Overpressure" >&3 + wait_gateway_log "AlarmCondition HEALED.*PLC_OVERPRESSURE" 20 + assert_fault_present PLC_OVERPRESSURE + echo " OK the refused acknowledge never reached the server" + + echo "ack Overpressure" >&3 + echo "confirm Overpressure" >&3 + wait_no_fault PLC_OVERPRESSURE 30 + echo " OK PLC_OVERPRESSURE cleared once the server itself acked and confirmed" +else + sovd_post_op acknowledge_fault \ + '{"fault_code":"PLC_OVERPRESSURE","comment":"e2e ack via SOVD"}' + + # Latch flips ActiveState=false on the server. Combined with the AckedState= + # true set by the SOVD ack above, the next AlarmCondition event payload has + # active=false, acked=true, confirmed=false -> SovdAlarmStatus::Healed + # (state machine internal), action=ReportHealed (no-op for fault_manager). + # /faults still shows CONFIRMED here, by design. + echo "latch Overpressure" >&3 + + # Wait for the gateway to actually receive and process the latch event before + # issuing SOVD confirm. Without this, the gateway still has the EventId from + # the original fire payload and the OPC-UA Confirm method on the server + # returns BadEventIdUnknown (the server's branch->lastEventId has been + # superseded by the Acknowledge auto-emit and the latch trigger). + wait_gateway_log "AlarmCondition HEALED.*PLC_OVERPRESSURE" 20 + + # Real SOVD confirm - exercises call_method(i=9113) + EventId. After this + # ConfirmedState=true on the server; the resulting event has all three of + # Active=false, Acked=true, Confirmed=true and the state machine emits + # ClearFault, removing the entry from /faults. + sovd_post_op confirm_fault \ + '{"fault_code":"PLC_OVERPRESSURE","comment":"e2e confirm via SOVD"}' + wait_no_fault PLC_OVERPRESSURE 30 + echo " OK PLC_OVERPRESSURE cleared after SOVD ack + latch + SOVD confirm" +fi echo " [scenario] shelving suppression" echo "fire Overheat 600" >&3 @@ -434,7 +545,7 @@ docker run --rm --name "${SERVER_NAME}" --network "${NET_NAME}" \ -i ros2_medkit_alarm_test_server:dev --port "${SERVER_PORT}" \ <&3 >/dev/null 2>&1 & SERVER_DOCKER_PID=$! -for i in $(seq 1 30); do +for _ in $(seq 1 30); do if docker logs "${SERVER_NAME}" 2>&1 | grep -q '^READY '; then break fi From 05f2875f58259adba15a9be5dd6a75d3eab2df37 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 7 Sep 2026 12:00:01 +0200 Subject: [PATCH 09/12] docs(opcua): state what the build inspection is, not how it got there 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. --- .../ros2_medkit_opcua/CMakeLists.txt | 5 ++- .../test/inspect_build_variant.py | 40 +++++++++---------- 2 files changed, 23 insertions(+), 22 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt b/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt index 9a318b38a..698e41733 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/CMakeLists.txt @@ -265,8 +265,9 @@ if(BUILD_TESTING) # A test of that test: it links an object exporting # __UA_Client_writeAttribute alongside the plugin entry points - what a # version script or a visibility slip on the vendored library produces - and - # requires the export rule to reject it. The rule used to be a prefix match on - # "UA_", which that spelling walks straight past. + # requires the export rule to reject it. The leading underscores are what makes + # it worth asserting: a rule matching the prefix "UA_" lets that spelling + # through. add_test(NAME test_opcua_build_variant_self_check COMMAND "${Python3_EXECUTABLE}" "${CMAKE_CURRENT_SOURCE_DIR}/test/inspect_build_variant.py" diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py index a7d505362..442b4cd04 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py @@ -70,18 +70,18 @@ # gone" in both builds and prove nothing. # # open62541's own primitives - __UA_Client_writeAttribute and the rest of the -# UA_Client_write* family - are absent from BOTH objects and therefore cannot -# discriminate either. They used to be present, and exported, in both: nothing -# in the plugin referenced them (open62541pp reaches the Write service through -# services::write, not through them), but the whole archive member was linked in -# and -fvisibility=hidden does not reach a static archive, so dlsym could call -# one and drive a controller the REST contract never exposed. -# -Wl,--exclude-libs,ALL plus -ffunction-sections/-fdata-sections and -# -Wl,--gc-sections removed them from the object outright. EXPORT_DENY below is -# what keeps that true: what remains of open62541 in a read-only object - the -# generic __UA_Client_Service dispatcher the read path needs, and the generated -# type descriptors the UA_TYPES table pins - is unreachable precisely because -# the module exports none of it. +# UA_Client_write* family - are absent from BOTH objects, so they discriminate +# nothing between variants and are not markers. Nothing in the plugin references +# them (open62541pp reaches the Write service through services::write), and the +# link drops them: -Wl,--exclude-libs,ALL keeps a static archive's globals out of +# the module's dynamic symbol table, which -fvisibility=hidden does not reach, +# and -ffunction-sections/-fdata-sections with -Wl,--gc-sections then discard the +# unreferenced sections. WRITE_PRIMITIVES below asserts their absence as an +# invariant, because an object that carries them is one where the archive member +# was pulled back in. EXPORT_DENY covers the rest: what remains of open62541 in a +# read-only object - the generic __UA_Client_Service dispatcher the read path +# needs, and the generated type descriptors the UA_TYPES table pins - is +# unreachable because the module exports none of it. WRITE_MARKERS = ( 'ros2_medkit_gateway::OpcuaClient::write_value(', 'ros2_medkit_gateway::OpcuaPlugin::handle_plc_operations(', @@ -226,11 +226,11 @@ def check_object(plugin, expect, report): return failures -# The reviewer's reproducer, kept as a test of the test: a module that exports -# __UA_Client_writeAttribute next to the six entry points. It is what a version -# script, or an -fvisibility slip on the vendored library, produces, and the -# prefix match this check used to do waved it through - the leading underscores -# meant it did not start with "UA_". The object is otherwise a plausible plugin +# A test of the export rule: a module that exports __UA_Client_writeAttribute +# next to the six entry points, which is what a version script or an -fvisibility +# slip on the vendored library produces. The leading underscores are the point - +# a rule that matched the prefix "UA_" would let this spelling through and hand +# dlsym a working write primitive. The object is otherwise a plausible plugin # (the read markers and entry points are there), so a rejection can only come # from the export rule under test. SELF_CHECK_SOURCE = r""" @@ -292,9 +292,9 @@ def self_check(): if WRITE_PRIMITIVES.search(name): problems.append(f'WRITE_PRIMITIVES wrongly matches {name}') - # And the same rule end to end, on a real object built the way the reviewer - # built one. A compiler is present wherever this test runs, because the - # package it inspects was just compiled. + # And the same rule end to end, on a real linked object rather than on names + # alone. A compiler is present wherever this test runs, because the package + # it inspects was just compiled. workdir = Path(tempfile.mkdtemp(prefix='opcua_self_check_')) try: obj, reason = build_self_check_object(workdir) From 2d85effbbee201203f5029bf279b3afb8f7a5c86 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 7 Sep 2026 12:00:15 +0200 Subject: [PATCH 10/12] fix(opcua): refuse with 501, and warn only about a setting someone wrote 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. --- .../docker/scripts/run_alarm_tests.sh | 9 ++-- .../include/ros2_medkit_opcua/node_map.hpp | 6 +++ .../ros2_medkit_opcua/opcua_plugin.hpp | 9 +++- .../ros2_medkit_opcua/src/node_map.cpp | 7 ++- .../ros2_medkit_opcua/src/opcua_plugin.cpp | 38 ++++++++++------ .../integration/test_opcua_read_only.test.py | 16 ++++++- .../test/test_address_space_browser.cpp | 45 +++++++++++++++++++ .../test/test_opcua_plugin.cpp | 34 +++++++++++--- 8 files changed, 138 insertions(+), 26 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_alarm_tests.sh b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_alarm_tests.sh index 9ba42a6f4..b99a24b13 100755 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_alarm_tests.sh +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_alarm_tests.sh @@ -25,6 +25,9 @@ set -euo pipefail VARIANT="${MEDKIT_OPCUA_VARIANT:-read-only}" +# SOVD spells "the entity does not support this" as 501; 403 is reserved for a +# valid token with insufficient permissions, which no credential can fix here. +REFUSAL_STATUS=501 if [[ "${VARIANT}" == "read-only" ]]; then READ_ONLY=ON elif [[ "${VARIANT}" == "write-capable" ]]; then @@ -200,8 +203,8 @@ sovd_post_op_refused() { local code code=$(curl -s -o /tmp/alarm_test_resp.json -w '%{http_code}' \ -X POST -H 'Content-Type: application/json' -d "${body}" "${url}") - if [[ "${code}" != "403" ]]; then - echo "ASSERT FAILED: POST ${op} expected HTTP 403, got ${code}" >&2 + if [[ "${code}" != "${REFUSAL_STATUS}" ]]; then + echo "ASSERT FAILED: POST ${op} expected HTTP ${REFUSAL_STATUS}, got ${code}" >&2 cat /tmp/alarm_test_resp.json >&2 || true return 1 fi @@ -215,7 +218,7 @@ sovd_post_op_refused() { cat /tmp/alarm_test_resp.json >&2 || true return 1 fi - echo " OK POST ${op} refused 403 x-medkit-plugin-error naming MEDKIT_OPCUA_READ_ONLY" + echo " OK POST ${op} refused ${REFUSAL_STATUS} x-medkit-plugin-error naming MEDKIT_OPCUA_READ_ONLY" } # What the tree advertises: a read-only image must not offer an operation it diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/node_map.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/node_map.hpp index 0686b19fd..6e2ef92ba 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/node_map.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/node_map.hpp @@ -263,6 +263,12 @@ struct AutoBrowseConfig { /// permits it. When false, every auto-browsed point stays read-only and only /// an explicit node_map ``nodes:`` entry can promote it (legacy behaviour). bool infer_writable{true}; + + /// Where an explicit ``infer_writable`` came from, for a message that can + /// point at the setting to change. Empty when nobody wrote the key, which is + /// what separates "the operator asked for this" from "this is the default" - + /// a read-only build says nothing about a key that was never set. + std::string infer_writable_source; }; /// SOVD entity definition derived from the node map diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp index 431bd336b..a547740e8 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/include/ros2_medkit_opcua/opcua_plugin.hpp @@ -74,7 +74,7 @@ namespace ros2_medkit_gateway { /// setting. ``MEDKIT_OPCUA_READ_ONLY`` defaults to ON: the OPC-UA write path is /// then absent from the object, no data point is ever marked writable, neither /// the x-plc-operations capability nor its POST route is registered, and -/// write_data / the value-write half of execute_operation refuse with 403 +/// write_data / the value-write half of execute_operation refuse with 501 /// before reaching the client. ``-DMEDKIT_OPCUA_READ_ONLY=OFF`` builds the /// write-capable plugin. class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, @@ -123,6 +123,13 @@ class OpcuaPlugin : public ros2_medkit_gateway::GatewayPlugin, // false for the Component itself, which has no entity_defs entry. bool has_operations(const std::string & entity_id) const override; + /// The address-space walk configuration after configure() has merged the + /// node map and the ROS parameters. Read by tests that need to see which + /// source supplied a setting, which no REST response exposes. + const AutoBrowseConfig & auto_browse_config_for_test() const { + return node_map_.auto_browse_config(); + } + // -- FaultProvider interface -- tl::expected list_faults(const std::string & entity_id) override; tl::expected get_fault(const std::string & entity_id, diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp index f3034ba10..2f02eb216 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/node_map.cpp @@ -380,9 +380,14 @@ bool NodeMap::load(const std::string & yaml_path) { auto_browse_config_.read_initial_values = parse_bool(ab["read_initial_values"], auto_browse_config_.read_initial_values, "auto_browse.read_initial_values", "auto_browse"); + if (ab["infer_writable"]) { + auto_browse_config_.infer_writable = parse_bool(ab["infer_writable"], auto_browse_config_.infer_writable, + "auto_browse.infer_writable", "auto_browse"); + auto_browse_config_.infer_writable_source = "the node map's auto_browse.infer_writable"; + } warn_unknown_keys(ab, "auto_browse", {"enabled", "root_nodes", "max_depth", "max_nodes", "namespace_allow", "namespace_deny", - "read_initial_values"}); + "read_initial_values", "infer_writable"}); } else { try { auto_browse_config_.enabled = ab.as(); diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp index b99d832b9..4c58b1ce8 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/src/opcua_plugin.cpp @@ -102,6 +102,13 @@ UserAuthMode require_user_auth_mode(const std::string & value) { constexpr const char * kReadOnlyBuildRefusal = "This OPC UA plugin was built read-only (MEDKIT_OPCUA_READ_ONLY=ON) and contains no controller write path; " "rebuild with -DMEDKIT_OPCUA_READ_ONLY=OFF for a write-capable plugin"; + +/// SOVD reserves 403 for a valid token with insufficient permissions, which is +/// the one thing this is not: no credential and no role reaches a write path +/// that is not in the binary. 501 is the status the SOVD catalogue uses for an +/// operation the entity does not support (fault deletion, data lists, +/// subscriptions and triggers all spell it that way), and that is what this is. +constexpr int kReadOnlyBuildStatus = 501; #else /// Parse a JSON "value" field, coerce to the node's declared data_type, and /// validate against the optional min/max range. Shared by handle_plc_operations, @@ -525,6 +532,7 @@ void OpcuaPlugin::configure(const nlohmann::json & config) { if (ab.contains("infer_writable")) { if (ab["infer_writable"].is_boolean()) { ab_cfg.infer_writable = ab["infer_writable"].get(); + ab_cfg.infer_writable_source = "plugins.opcua.auto_browse.infer_writable"; } else { warn("plugins.opcua.auto_browse.infer_writable must be a boolean - keeping current value"); } @@ -593,15 +601,16 @@ void OpcuaPlugin::set_context(PluginContext & context) { log_security_profile(); #if MEDKIT_OPCUA_READ_ONLY - // One line, once, when the operator asked the address-space walk to take its - // writability from the server's CurrentWrite bit. The inference is not in - // this binary, so the setting has no effect and saying so at startup beats - // leaving someone to wonder why every discovered point reads back read-only. - if (node_map_.auto_browse_config().enabled && node_map_.auto_browse_config().infer_writable) { - log_warn( - "auto_browse infer_writable is ignored: this plugin was built with MEDKIT_OPCUA_READ_ONLY=ON and carries " - "no write path, so every discovered data point stays read-only. Rebuild with " - "-DMEDKIT_OPCUA_READ_ONLY=OFF for a write-capable plugin."); + // One line, once, and only when someone asked for the inference: the setting + // defaults to true, so warning on the default would tell every auto_browse + // deployment about a key nobody wrote. An explicit request is different - it + // has no effect here, and the message names the setting so it can be found. + const auto & ab_cfg = node_map_.auto_browse_config(); + if (ab_cfg.enabled && ab_cfg.infer_writable && !ab_cfg.infer_writable_source.empty()) { + log_warn(ab_cfg.infer_writable_source + + " is ignored: this plugin was built with MEDKIT_OPCUA_READ_ONLY=ON and carries no write path, so " + "every discovered data point stays read-only. Rebuild with -DMEDKIT_OPCUA_READ_ONLY=OFF for a " + "write-capable plugin."); } #endif @@ -1709,13 +1718,13 @@ tl::expected OpcuaPlugin::write_dat const nlohmann::json & value) { #if MEDKIT_OPCUA_READ_ONLY // First statement in the function: the refusal precedes every lookup, so no - // request reaches the OPC-UA client, and 403 says the server understood the - // request and will not carry it out. The gateway renders provider errors as + // request reaches the OPC-UA client. The gateway renders provider errors as // the x-medkit-plugin-error vendor code with this message verbatim. (void)entity_id; (void)resource_name; (void)value; - return tl::make_unexpected(DataProviderErrorInfo{DataProviderError::ReadOnly, kReadOnlyBuildRefusal, 403}); + return tl::make_unexpected( + DataProviderErrorInfo{DataProviderError::ReadOnly, kReadOnlyBuildRefusal, kReadOnlyBuildStatus}); #else if (!ctx_ || !poller_) { return tl::make_unexpected(DataProviderErrorInfo{DataProviderError::Internal, "plugin not initialized", 503}); @@ -1858,7 +1867,7 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string // operation id directly. if (operation_name == "acknowledge_fault" || operation_name == "confirm_fault") { return tl::make_unexpected( - OperationProviderErrorInfo{OperationProviderError::Rejected, kReadOnlyBuildRefusal, 403}); + OperationProviderErrorInfo{OperationProviderError::Rejected, kReadOnlyBuildRefusal, kReadOnlyBuildStatus}); } #else // Issue #386: acknowledge_fault and confirm_fault dispatch to OPC-UA @@ -1955,7 +1964,8 @@ OpcuaPlugin::execute_operation(const std::string & entity_id, const std::string // build marks no point writable, so list_operations emits no set_* entry - // and a request aimed straight at one is refused before any node lookup or // client call. - return tl::make_unexpected(OperationProviderErrorInfo{OperationProviderError::Rejected, kReadOnlyBuildRefusal, 403}); + return tl::make_unexpected( + OperationProviderErrorInfo{OperationProviderError::Rejected, kReadOnlyBuildRefusal, kReadOnlyBuildStatus}); #else std::string data_name; if (operation_name.substr(0, 4) == "set_") { diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py index a8fad10ed..75902dac9 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/integration/test_opcua_read_only.test.py @@ -67,6 +67,9 @@ # (primitives.cpp maps every x-medkit-* code into error_code "vendor-specific" # plus this vendor_code). The build property is named in the message. PLUGIN_VENDOR_CODE = 'x-medkit-plugin-error' +# SOVD spells "the entity does not support this" as 501; 403 is reserved for a +# valid token with insufficient permissions, which no credential can fix here. +REFUSAL_STATUS = 501 BUILD_PROPERTY = 'MEDKIT_OPCUA_READ_ONLY' failures = [] @@ -256,8 +259,8 @@ def item_by_name(payload, name): def refusal_is_the_build(status, payload, what): - """Assert one HTTP answer is the read-only build's 403 refusal, naming the build.""" - ok = check(status == 403, f'{what}: 403 (got {status})') + """Assert one HTTP answer is the read-only build's refusal, naming the build.""" + ok = check(status == REFUSAL_STATUS, f'{what}: {REFUSAL_STATUS} (got {status})') if not isinstance(payload, dict): check(False, f'{what}: JSON error body (got {payload!r})') return @@ -497,6 +500,15 @@ def run_auto_browse_leg(workdir, env, plugin, server_bin, server, server_port, check(writable is expected, f'{label}: auto-browsed StatusWord reports writable={expected} (got {writable!r})') + # The startup warning belongs to a key someone set, not to the default. + # infer_writable defaults to true, so warning on the value would warn on + # every auto_browse deployment; the log must name the setting when it is + # written and say nothing when it is not. + warned = wait_log(log, 'plugins.opcua.auto_browse.infer_writable is ignored', deadline=3) + want_warning = read_only and infer_writable == 'true' + check(warned is want_warning, + f'{label}: startup warning about the ignored setting: {want_warning} (got {warned})') + if rebrowse: # CHANGE: the plugin re-walks the address space on a fresh session # (maybe_rebrowse_on_reconnect). Restarting the server is the only diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_address_space_browser.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_address_space_browser.cpp index dbf313e65..9cf19c983 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_address_space_browser.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_address_space_browser.cpp @@ -230,6 +230,51 @@ TEST(AutoBrowserTest, WritableInferredFromServerAccessLevel) { EXPECT_FALSE(find_entry(result.entries, unk.toString())->writable); } +// Whether infer_writable was ASKED FOR is separate from what it evaluates to: +// it defaults to true, so a read-only build that warned on the value would warn +// on every auto_browse deployment. The source string is what tells the two +// apart, and it has to come from either spelling of the key. +TEST(NodeMapAutoBrowseConfigTest, InferWritableSourceRecordsTheNodeMapSpelling) { + NodeMap node_map; + const TempYamlFile yaml_file(R"( +component_id: test_plc +auto_browse: + enabled: true + infer_writable: true +)"); + ASSERT_TRUE(node_map.load(yaml_file.path())); + EXPECT_TRUE(node_map.auto_browse_config().infer_writable); + EXPECT_EQ(node_map.auto_browse_config().infer_writable_source, "the node map's auto_browse.infer_writable"); +} + +TEST(NodeMapAutoBrowseConfigTest, InferWritableSourceStaysEmptyWhenTheKeyIsAbsent) { + NodeMap node_map; + const TempYamlFile yaml_file(R"( +component_id: test_plc +auto_browse: + enabled: true +)"); + ASSERT_TRUE(node_map.load(yaml_file.path())); + // The default is still true - only nobody asked for it. + EXPECT_TRUE(node_map.auto_browse_config().infer_writable); + EXPECT_TRUE(node_map.auto_browse_config().infer_writable_source.empty()); +} + +// The node-map spelling is parsed rather than dropped as an unknown key, so a +// value written there takes effect on a write-capable build. +TEST(NodeMapAutoBrowseConfigTest, InferWritableFalseFromTheNodeMapIsHonoured) { + NodeMap node_map; + const TempYamlFile yaml_file(R"( +component_id: test_plc +auto_browse: + enabled: true + infer_writable: false +)"); + ASSERT_TRUE(node_map.load(yaml_file.path())); + EXPECT_FALSE(node_map.auto_browse_config().infer_writable); + EXPECT_EQ(node_map.auto_browse_config().infer_writable_source, "the node map's auto_browse.infer_writable"); +} + TEST(AutoBrowserTest, InferWritableDisabledKeepsEverythingReadOnly) { FakeAutoBrowseSource source; const auto root = NodeMap::parse_node_id("i=85"); diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp index bb7480135..0c6b58b64 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/test/test_opcua_plugin.cpp @@ -308,7 +308,7 @@ TEST_F(OpcuaPluginTest, ReadDataNotFound) { #if MEDKIT_OPCUA_READ_ONLY // A read-only build refuses every write before it looks anything up, so the // point being writable, read-only or absent, and the body being well formed or -// not, all reach the same answer: 403 and a message naming the build property. +// not, all reach the same answer: 501 and a message naming the build property. TEST_F(OpcuaPluginTest, WriteDataRefusedOnAReadOnlyBuild) { const std::vector resources{"level", "pressure", "nonexistent"}; const std::vector bodies{nlohmann::json{{"value", 5.0}}, nlohmann::json{{"not_value", 42}}, @@ -318,7 +318,7 @@ TEST_F(OpcuaPluginTest, WriteDataRefusedOnAReadOnlyBuild) { auto result = plugin_.write_data("tank", resource, body); ASSERT_FALSE(result.has_value()) << resource << " / " << body.dump(); EXPECT_EQ(result.error().code, DataProviderError::ReadOnly); - EXPECT_EQ(result.error().http_status, 403); + EXPECT_EQ(result.error().http_status, 501); EXPECT_NE(result.error().message.find("MEDKIT_OPCUA_READ_ONLY"), std::string::npos) << "the refusal must name the build property, got: " << result.error().message; } @@ -331,7 +331,7 @@ TEST_F(OpcuaPluginTest, WriteDataRefusedOnAReadOnlyBuild) { TEST_F(OpcuaPluginTest, WriteDataRefusedForUnknownEntityOnAReadOnlyBuild) { auto result = plugin_.write_data("nonexistent", "level", nlohmann::json{{"value", 5.0}}); ASSERT_FALSE(result.has_value()); - EXPECT_EQ(result.error().http_status, 403); + EXPECT_EQ(result.error().http_status, 501); } #else TEST_F(OpcuaPluginTest, WriteDataReadOnly) { @@ -426,7 +426,7 @@ TEST_F(OpcuaPluginTest, ExecuteOperationRefusedOnAReadOnlyBuild) { auto result = plugin_.execute_operation("tank", op, params); ASSERT_FALSE(result.has_value()) << op << " / " << params.dump(); EXPECT_EQ(result.error().code, OperationProviderError::Rejected); - EXPECT_EQ(result.error().http_status, 403); + EXPECT_EQ(result.error().http_status, 501); EXPECT_NE(result.error().message.find("MEDKIT_OPCUA_READ_ONLY"), std::string::npos) << "the refusal must name the build property, got: " << result.error().message; } @@ -448,6 +448,30 @@ TEST_F(OpcuaPluginTest, ExecuteOperationReadOnly) { } #endif +// -- infer_writable: an explicit request is distinguishable from the default -- + +TEST(OpcuaPluginAutoBrowseConfig, InferWritableSourceRecordsTheParameterSpelling) { + OpcuaPlugin plugin; + nlohmann::json config; + config["endpoint_url"] = "opc.tcp://nonexistent:4840"; + config["auto_browse"] = nlohmann::json{{"enabled", true}, {"infer_writable", true}}; + plugin.configure(config); + EXPECT_EQ(plugin.auto_browse_config_for_test().infer_writable_source, "plugins.opcua.auto_browse.infer_writable"); +} + +TEST(OpcuaPluginAutoBrowseConfig, InferWritableSourceStaysEmptyWhenTheParameterIsAbsent) { + OpcuaPlugin plugin; + nlohmann::json config; + config["endpoint_url"] = "opc.tcp://nonexistent:4840"; + config["auto_browse"] = nlohmann::json{{"enabled", true}}; + plugin.configure(config); + // Enabled, and infer_writable still defaults to true - but nobody asked, so a + // read-only build has nothing to warn about. + EXPECT_TRUE(plugin.auto_browse_config_for_test().enabled); + EXPECT_TRUE(plugin.auto_browse_config_for_test().infer_writable); + EXPECT_TRUE(plugin.auto_browse_config_for_test().infer_writable_source.empty()); +} + // -- Condition operations (acknowledge / confirm) per build variant -------- // // Acknowledging an alarm changes condition state on the controller, so a @@ -511,7 +535,7 @@ TEST_F(OpcuaPluginEventAlarmsTest, ConditionOperationsRefusedOnAReadOnlyBuild) { auto result = plugin_.execute_operation("tank", op, nlohmann::json{{"fault_code", "PLC_OVERPRESSURE"}}); ASSERT_FALSE(result.has_value()) << op; EXPECT_EQ(result.error().code, OperationProviderError::Rejected); - EXPECT_EQ(result.error().http_status, 403); + EXPECT_EQ(result.error().http_status, 501); EXPECT_NE(result.error().message.find("MEDKIT_OPCUA_READ_ONLY"), std::string::npos) << "the refusal must name the build property, got: " << result.error().message; } From 21ceac9378d574f1fb94007fad16788505e2100c Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 7 Sep 2026 12:00:30 +0200 Subject: [PATCH 11/12] test(opcua): inspect the object inside the image, and say what is really 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. --- .github/workflows/opcua-plugin.yml | 28 +++++++ .../ros2_medkit_opcua/README.md | 80 ++++++++++++------- .../ros2_medkit_opcua/design/index.rst | 50 ++++++++---- 3 files changed, 114 insertions(+), 44 deletions(-) diff --git a/.github/workflows/opcua-plugin.yml b/.github/workflows/opcua-plugin.yml index fbe8d033e..8455705a4 100644 --- a/.github/workflows/opcua-plugin.yml +++ b/.github/workflows/opcua-plugin.yml @@ -157,6 +157,22 @@ jobs: -f src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway \ -t gateway-opcua . + - name: Inspect the plugin object inside the built image + # The image is what ships, and Dockerfile.gateway builds it with + # BUILD_TESTING=OFF, so the inspection ctest does not exist inside the + # container. Pull the object out and run the same check on the runner: + # without this the image is only proven by its behaviour, and a + # build-arg or Dockerfile drift that shipped the wrong variant would be + # caught by the integration assertions rather than by the inspection. + run: | + cid=$(docker create gateway-opcua) + docker cp "$cid:$(docker run --rm --entrypoint sh gateway-opcua -c \ + "find /root/ws/install -name libros2_medkit_opcua_plugin.so | head -1")" \ + ./plugin-from-image.so + docker rm -v "$cid" >/dev/null + python3 src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py \ + ./plugin-from-image.so --expect ${{ matrix.variant }} + - name: Start OpenPLC timeout-minutes: 3 run: | @@ -277,6 +293,18 @@ jobs: MEDKIT_OPCUA_VARIANT: ${{ matrix.variant }} run: bash src/ros2_medkit_plugins/ros2_medkit_opcua/docker/scripts/run_alarm_tests.sh + - name: Inspect the plugin object inside the built image + # Same reason as the OpenPLC job. This suite builds its own image, so + # the step runs after it; the tag is the one run_alarm_tests.sh builds. + run: | + cid=$(docker create gateway-opcua:alarm-test) + docker cp "$cid:$(docker run --rm --entrypoint sh gateway-opcua:alarm-test -c \ + "find /root/ws/install -name libros2_medkit_opcua_plugin.so | head -1")" \ + ./plugin-from-image.so + docker rm -v "$cid" >/dev/null + python3 src/ros2_medkit_plugins/ros2_medkit_opcua/test/inspect_build_variant.py \ + ./plugin-from-image.so --expect ${{ matrix.variant }} + - name: Dump container logs on failure if: failure() run: | diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md index b2294abb5..a66b58490 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/README.md @@ -11,8 +11,9 @@ Follows the same plugin pattern as `ros2_medkit_graph_provider`: implements `Gat discovers them automatically by browsing the live address space (`auto_browse`, see below) - zero node-map config required - Exposes PLC values as the `x-plc-data` vendor collection -- Ships read-only by default: the shipped binary contains no controller write - path at all (see [Read-only and write-capable builds](#read-only-and-write-capable-builds)) +- Ships read-only by default: the shipped binary contains no compiled path that + writes a value or acknowledges a condition, and exports nothing from the OPC UA + stack (see [Read-only and write-capable builds](#read-only-and-write-capable-builds)) - In a write-capable build, allows writing setpoints via `x-plc-operations` with type-aware coercion and range validation - Reports the connection state and poll metrics via `x-plc-status` @@ -120,27 +121,43 @@ colcon build --packages-select ros2_medkit_opcua \ In the default read-only build: -- `OpcuaClient::write_value`, the vendor route handler, and the open62541pp - Write service functions and templates they reach are **not compiled**. On top - of that the plugin links with `-Wl,--exclude-libs,ALL` and `-Wl,--gc-sections` - (over `-ffunction-sections -fdata-sections`), which drops open62541's own - `UA_Client_write*` primitives from the object as well - nothing references - them once the C++ write path is gone. So `libros2_medkit_opcua_plugin.so` - contains **no code that can issue an OPC-UA Write**, and its dynamic symbol - table exports the six plugin entry points and nothing from the OPC-UA stack, - so `dlsym` has no handle on any of it either. - - Precisely, because a claim of this kind should be exact rather than sweeping: - what does remain inside the object is open62541's generic service dispatcher, - which the read path needs, and the generated type descriptors for the Write - request and response messages, which the library's `UA_TYPES` table keeps - alive whatever the linker does. Neither is exported, neither is reachable from - any route, and no function in the object composes a Write request out of them. - - `test_opcua_build_variant` asserts all of this against the built object with - `nm`, in both variants: absent write symbols and an OPC-UA-free export table - in the read-only build, present write symbols in the write-capable one, and - the six entry points exported in both. +- **What is absent from the object**, each name asserted by `nm` in + `test_opcua_build_variant`: + - every C++ function that composes an OPC UA Write - `OpcuaClient::write_value`, + the vendor route handler `OpcuaPlugin::handle_plc_operations`, the value + coercion they share, and the `open62541pp` templates and service functions + they reach (`Node::writeValueScalar`, `Node::writeValue`, + `services::write`, `services::writeAttribute`); + - the entry point that issues a Part 9 condition method, + `OpcuaClient::call_condition_method`; + - open62541's own `UA_Client_write*` / `UA_Server_write*` primitives, dropped + by `-Wl,--exclude-libs,ALL` and `-Wl,--gc-sections` over + `-ffunction-sections -fdata-sections` because nothing references them once + the C++ write path is gone; + - every OPC UA symbol in the dynamic symbol table. The module exports the six + plugin entry points and C++ vague-linkage symbols, and nothing matching + `UA_*` or `opcua::*`, so `dlsym` reaches none of the machinery below. + +- **What remains inside the object, and why no route reaches it.** open62541 is + one static library, so removing the write path does not remove the transport + it shared. Still present: the generic request dispatcher + `__UA_Client_Service`, which the read, browse and ConditionRefresh paths all + use; the binary encoders (23 `*_encodeBinary` symbols), which serialize every + message type including the ones below; and the generated `UA_TYPES` + descriptors, which the table references as a whole so the linker cannot drop + individual entries - among them `WriteRequest`, `WriteValue`, `WriteResponse`, + `AddNodes`, `DeleteNodes`, `AddReferences`, `SetMonitoringMode`, + `SetPublishingMode` and `TransferSubscriptions` (`HistoryUpdate`: absent). + Descriptors are data, not a code path: no function in the object builds any of + those requests, none of these symbols is exported, and the plugin registers + three GET routes in a read-only build, so nothing in the REST contract + supplies a NodeId, a method id or an attribute id to reach them. + +- Two OPC UA calls that do change server-side state are deliberately **kept** in + both variants, because they change no controller data: `ConditionRefresh`, + which asks the server to replay conditions it already holds, and subscription + and monitored-item creation, which the read path needs to receive values and + alarms at all. - No data point is ever `writable`. A node-map entry that says `writable: true` is ignored with one startup warning naming the build property, and the address-space walk never consults the server's `CurrentWrite` bit whatever @@ -157,9 +174,12 @@ In the default read-only build: stays in both variants: it asks the server to replay conditions it already holds and changes nothing. - `PUT /{type}/{id}/data/{name}` and - `POST /{type}/{id}/operations/{name}/executions` answer **403** before any + `POST /{type}/{id}/operations/{name}/executions` answer **501** before any node lookup, condition lookup or client call, with vendor code - `x-medkit-plugin-error` and a message naming `MEDKIT_OPCUA_READ_ONLY`. + `x-medkit-plugin-error` and a message naming `MEDKIT_OPCUA_READ_ONLY`. 501 is + the status SOVD uses for an operation the entity does not support; 403 is + reserved for a valid token with insufficient permissions, and no credential + reaches a write path that is not in the binary. A write-capable build restores everything above; nothing else differs. @@ -494,10 +514,12 @@ plugins.opcua.auto_browse.enabled: true plugins.opcua.auto_browse.infer_writable: true # write-capable builds only ``` -`infer_writable` is accepted only in this ROS-param form - the node-map YAML's -`auto_browse:` block does not parse it. It defaults to `true` and has no effect -in a read-only build, which logs one warning at startup and leaves every -discovered point read-only. +`infer_writable` is accepted in both forms - the node-map YAML's `auto_browse:` +block and the ROS param above. It defaults to `true`, and in a read-only build +it has no effect: every discovered point stays read-only. Setting it explicitly +there logs one startup warning naming the setting, so it can be found and +removed; leaving the key alone logs nothing, because the default is not a +request anybody made. The JSON/ROS-param form takes precedence over whatever the node-map YAML's `auto_browse:` block set, mirroring how environment variables override the diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst b/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst index a0bcf93ee..8be7812c3 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/design/index.rst @@ -104,7 +104,7 @@ Four of the five protocol plugins cannot write to their device at all. OPC UA ca and config-less discovery would mark a point writable straight from the server's ``CurrentWrite`` bit. A plant asking "can this box change a controller" cannot be answered by a configuration value, because a configuration value can be changed -without rebuilding, reviewing or shipping anything. So the answer is carried by the +without rebuilding or shipping anything. So the answer is carried by the binary. ``MEDKIT_OPCUA_READ_ONLY`` is a CMake cache option, default ``ON``. With it on: @@ -113,12 +113,12 @@ binary. templates it reaches, the vendor route handler ``OpcuaPlugin::handle_plc_operations`` and the value-coercion helper are all outside ``#if`` and are never compiled. -- The link then removes what the compiler alone could not. ``-fvisibility=hidden`` - covers the sources compiled into the module but not the static archives it - links, so open62541's own ``UA_Client_write*`` primitives used to sit in the - object *and* in its dynamic symbol table: no route reached them, but a caller - holding the ``.so`` could ``dlsym`` one and drive a controller with it. - ``-Wl,--exclude-libs,ALL`` takes the archives out of the export table, and +- The link removes what the compiler alone cannot. ``-fvisibility=hidden`` covers + the sources compiled into the module but not the static archives it links, so + without further flags open62541's own ``UA_Client_write*`` primitives sit in the + object *and* in its dynamic symbol table, where a caller holding the ``.so`` + can ``dlsym`` one and drive a controller with it even though no route reaches + it. ``-Wl,--exclude-libs,ALL`` takes the archives out of the export table, and ``-ffunction-sections -fdata-sections`` plus ``-Wl,--gc-sections`` then let the linker drop them from the object entirely, because nothing references them once the C++ write path is gone. The read-only object exports the six plugin entry @@ -131,21 +131,41 @@ binary. local, unexported code. Setting them here makes the property independent of ``CMAKE_BUILD_TYPE``, and ``test_opcua_build_variant`` asserts the absence of that whole family so the difference cannot come back unnoticed. -- What is left of open62541 inside the object is the generic service dispatcher - the read path needs and the generated type descriptors the ``UA_TYPES`` table - pins. They are data and dispatch, not a write path: nothing exports them and no - function in the object composes a Write request from them. The claim the package - makes is therefore the exact one - no code able to issue a Write, and no export - to reach the library through - not a sweeping "no OPC-UA symbols at all". +- What is left of open62541 inside the object is one shared transport, not a + write path. open62541 is a single static library, so removing the write code + does not remove what it shared with the read code: the generic dispatcher + ``__UA_Client_Service`` (used by read, browse and ConditionRefresh), the binary + encoders (23 ``*_encodeBinary`` symbols), and the generated ``UA_TYPES`` + descriptors, which the table references as a whole so the linker cannot drop + individual entries. Measured on the read-only object, those descriptors include + ``WriteRequest``, ``WriteValue``, ``WriteResponse``, ``AddNodes``, + ``DeleteNodes``, ``AddReferences``, ``SetMonitoringMode``, + ``SetPublishingMode`` and ``TransferSubscriptions``; ``HistoryUpdate`` is + absent. + + Descriptors are data. What makes them unreachable is that no function in the + object composes any of those requests, none of these symbols is exported, and a + read-only build registers three GET routes - no route, node-map key or config + key supplies a NodeId, a method id or an attribute id. So the claim the package + makes is the exact one: every C++ path that composes a Write or a condition + method call is absent, and no OPC UA symbol is exported. Not "no OPC UA + machinery at all", which would be false. + +- Subscription and monitored-item creation stay in both variants. They change + server-side session state, which is not controller data, and the read path + cannot receive values or alarms without them. - ``NodeMap::load`` forces ``writable`` to false and warns once when the file asked otherwise; ``AutoBrowser`` does not compile the ``infer_writable`` inference, so the server's ``CurrentWrite`` bit is never read. - No entity registers the ``x-plc-operations`` capability, ``list_operations`` emits no ``set_`` entry, and ``get_routes`` does not register the write route. -- ``write_data`` and the value-write half of ``execute_operation`` return 403 as +- ``write_data`` and the value-write half of ``execute_operation`` return 501 as their first statement, before any node lookup, with a message naming the build - property. They stay declared because the gateway reaches the plugin through the + property. SOVD spells "the entity does not support this" as 501 (fault + deletion, data lists, subscriptions and triggers all use it); 403 is defined + once, for a valid token with insufficient permissions, which is the one reading + that is wrong here - no credential reaches a path that is not in the binary. They stay declared because the gateway reaches the plugin through the ``DataProvider`` / ``OperationProvider`` interfaces; the refusal reaches a client as SOVD vendor code ``x-medkit-plugin-error``, which is the code the gateway assigns to every plugin provider error. From 66cb3a177ae44193617ef0ff05848d95cc2645a1 Mon Sep 17 00:00:00 2001 From: Bartosz Burda Date: Mon, 7 Sep 2026 13:25:43 +0200 Subject: [PATCH 12/12] build(opcua): retry the network fetches the docker images start with 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. --- .../docker/Dockerfile.gateway | 27 ++++++++++++++----- .../docker/openplc/Dockerfile | 12 +++++++-- .../docker/test_alarm_server/Dockerfile | 12 +++++++-- 3 files changed, 40 insertions(+), 11 deletions(-) diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway index 2cfda3238..83cd2aa91 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/Dockerfile.gateway @@ -7,8 +7,16 @@ FROM ros:jazzy-ros-base AS builder ENV DEBIAN_FRONTEND=noninteractive ENV COLCON_WS=/root/ws +# apt-get update fetches package indexes from the distribution mirrors and +# rosdep update fetches the distribution index from raw.githubusercontent.com. +# Both are network reads that a reset connection fails outright, and both run +# before anything of ours compiles, so a blip fails the whole build for a reason +# that has nothing to do with the change under test. Three attempts, five +# seconds apart; each attempt's own error text goes to stderr, so the last +# failure is what a reader sees. # hadolint ignore=DL3008 -RUN apt-get update && apt-get install -y --no-install-recommends \ +RUN retry() { n=1; while :; do "$@" && return 0; [ "$n" -ge 3 ] && return 1; echo "attempt $n of 3 failed: $*" >&2; n=$((n+1)); sleep 5; done; }; \ + retry apt-get update && apt-get install -y --no-install-recommends \ python3-colcon-common-extensions nlohmann-json3-dev libcpp-httplib-dev \ sqlite3 libsqlite3-dev libsystemd-dev libssl-dev libyaml-cpp-dev \ pkg-config git ros-jazzy-ament-cmake-gtest ros-jazzy-yaml-cpp-vendor \ @@ -29,14 +37,18 @@ WORKDIR ${COLCON_WS} # test_depend and which rosdep tries to install even with BUILD_TESTING=OFF). # This was previously masked by Docker layer cache hits on CI; cold builds # always failed. -RUN bash -c "source /opt/ros/jazzy/setup.bash && \ - apt-get update && \ - rosdep update && \ +# Same retry as above, for the same two commands. This is the RUN where a +# reset connection to raw.githubusercontent.com aborts the build. +RUN bash -c 'retry() { n=1; while :; do "$@" && return 0; [ "$n" -ge 3 ] && return 1; \ + echo "attempt $n of 3 failed: $*" >&2; n=$((n+1)); sleep 5; done; }; \ + source /opt/ros/jazzy/setup.bash && \ + retry apt-get update && \ + retry rosdep update && \ rosdep install --from-paths src --ignore-src -r -y \ - --skip-keys='ament_cmake_clang_format ament_cmake_clang_tidy test_msgs sqlite3 ros2_medkit_graph_provider python3-requests launch_testing_ament_cmake launch_testing launch_ros ament_index_python ros2_medkit_param_beacon nav2_msgs' && \ + --skip-keys="ament_cmake_clang_format ament_cmake_clang_tidy test_msgs sqlite3 ros2_medkit_graph_provider python3-requests launch_testing_ament_cmake launch_testing launch_ros ament_index_python ros2_medkit_param_beacon nav2_msgs" && \ rm -rf /var/lib/apt/lists/* && \ colcon build --cmake-args -DBUILD_TESTING=OFF \ - --packages-skip vda5050_agent ros2_medkit_vda5050_msgs ros2_medkit_opcua" + --packages-skip vda5050_agent ros2_medkit_vda5050_msgs ros2_medkit_opcua' # Which write surface the image ships. ON is the default the plugin itself # defaults to: no controller write path in the object. OFF builds the @@ -61,7 +73,8 @@ ENV DEBIAN_FRONTEND=noninteractive ENV COLCON_WS=/root/ws # hadolint ignore=DL3008 -RUN apt-get update && apt-get install -y --no-install-recommends \ +RUN retry() { n=1; while :; do "$@" && return 0; [ "$n" -ge 3 ] && return 1; echo "attempt $n of 3 failed: $*" >&2; n=$((n+1)); sleep 5; done; }; \ + retry apt-get update && apt-get install -y --no-install-recommends \ ros-jazzy-yaml-cpp-vendor ros-jazzy-example-interfaces \ nlohmann-json3-dev libcpp-httplib-dev sqlite3 libsqlite3-dev curl jq \ && rm -rf /var/lib/apt/lists/* diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/openplc/Dockerfile b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/openplc/Dockerfile index 89b3fc8ed..b4d41d55c 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/openplc/Dockerfile +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/openplc/Dockerfile @@ -4,7 +4,13 @@ FROM debian:bookworm-slim AS matiec-builder -RUN apt-get update && apt-get install -y \ +# apt-get update fetches package indexes from the distribution mirrors, a +# network read that a reset connection fails outright and that runs before +# anything of ours is built. Three attempts, five seconds apart; each +# attempt's own error text goes to stderr, so the last failure is what a +# reader sees. +RUN retry() { n=1; while :; do "$@" && return 0; [ "$n" -ge 3 ] && return 1; echo "attempt $n of 3 failed: $*" >&2; n=$((n+1)); sleep 5; done; }; \ + retry apt-get update && apt-get install -y \ git build-essential flex bison autoconf automake \ && rm -rf /var/lib/apt/lists/* @@ -19,7 +25,9 @@ WORKDIR /openplc # Pin to specific commit for reproducible builds ENV OPENPLC_COMMIT=8a22c81ed6ddaba13225caec4a3ff15fd2c92909 -RUN apt-get update && apt-get install -y git ca-certificates curl jq && \ +# Same retry as above, same reason. +RUN retry() { n=1; while :; do "$@" && return 0; [ "$n" -ge 3 ] && return 1; echo "attempt $n of 3 failed: $*" >&2; n=$((n+1)); sleep 5; done; }; \ + retry apt-get update && apt-get install -y git ca-certificates curl jq && \ git clone https://github.com/Autonomy-Logic/openplc-runtime.git /openplc && \ cd /openplc && git checkout ${OPENPLC_COMMIT} && \ rm -rf /var/lib/apt/lists/* diff --git a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/test_alarm_server/Dockerfile b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/test_alarm_server/Dockerfile index e21c6a5ce..e07993407 100644 --- a/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/test_alarm_server/Dockerfile +++ b/src/ros2_medkit_plugins/ros2_medkit_opcua/docker/test_alarm_server/Dockerfile @@ -12,7 +12,13 @@ FROM ubuntu:24.04 AS builder ENV DEBIAN_FRONTEND=noninteractive -RUN apt-get update && apt-get install -y --no-install-recommends \ +# apt-get update fetches package indexes from the distribution mirrors, a +# network read that a reset connection fails outright and that runs before +# anything of ours is built. Three attempts, five seconds apart; each +# attempt's own error text goes to stderr, so the last failure is what a +# reader sees. +RUN retry() { n=1; while :; do "$@" && return 0; [ "$n" -ge 3 ] && return 1; echo "attempt $n of 3 failed: $*" >&2; n=$((n+1)); sleep 5; done; }; \ + retry apt-get update && apt-get install -y --no-install-recommends \ build-essential cmake git python3 ca-certificates libssl-dev \ && rm -rf /var/lib/apt/lists/* @@ -51,7 +57,9 @@ RUN g++ -O2 -std=c++17 -w \ -lssl -lcrypto -lpthread -o /opt/test_alarm_server FROM ubuntu:24.04 -RUN apt-get update && apt-get install -y --no-install-recommends \ +# Same retry as above, same reason. +RUN retry() { n=1; while :; do "$@" && return 0; [ "$n" -ge 3 ] && return 1; echo "attempt $n of 3 failed: $*" >&2; n=$((n+1)); sleep 5; done; }; \ + retry apt-get update && apt-get install -y --no-install-recommends \ libstdc++6 ca-certificates libssl3 openssl \ && rm -rf /var/lib/apt/lists/* COPY --from=builder /opt/test_alarm_server /usr/local/bin/test_alarm_server