Verify DiscoveryURL from OPC Server is resolvable #570

harrison-tin · 2023-03-21T18:21:41Z

What this PR does / why we need it:
This PR solves the issue when OPC server returns a discoveryURL that contains a hostname instead of an IP address. If the broker is not able to resolve the hostname, then it will return an error. The suggested solution is to have the discovery handler try resolving the discoveryURL (by to_socket_addr), if it fails, then we will use the IP address that user passes in instead. Note it should only perform on Server type not DiscoveryServer type.
closes #555

Sample: Using Prosys OPC UA Simulation Server
Before:

harrison@harrison-Virtual-Machine:~/akri-opcua-test$ kubectl logs harrison-virtual-machine-akri-opcua-monitoring-2b9293-pod
.NET Core OPC UA Console Client Start
1 - Create an Application Configuration.
Exception System.IO.DirectoryNotFoundException: Could not find a part of the path '/etc/opcua-certs/client-pki/own/certs'.
   at System.IO.Enumeration.FileSystemEnumerator`1.CreateDirectoryHandle(String path, Boolean ignoreNotFound)
   at System.IO.Enumeration.FileSystemEnumerator`1.Init()
   at System.IO.Enumeration.FileSystemEnumerator`1..ctor(String directory, Boolean isNormalized, EnumerationOptions options)
   at System.IO.Enumeration.FileSystemEnumerable`1..ctor(String directory, FindTransform transform, EnumerationOptions options, Boolean isNormalized)
   at System.IO.Enumeration.FileSystemEnumerableFactory.FileInfos(String directory, String expression, EnumerationOptions options, Boolean isNormalized)
   at System.IO.DirectoryInfo.InternalEnumerateInfos(String path, String searchPattern, SearchTarget searchTarget, EnumerationOptions options)
   at System.IO.DirectoryInfo.GetFiles(String searchPattern, EnumerationOptions enumerationOptions)
   at System.IO.DirectoryInfo.GetFiles(String searchPattern)
   at OpcuaNodeMonitoring.MonitoringClient.GetCertificate(String certificateStorePath) in /src/Program.cs:line 172
   at OpcuaNodeMonitoring.MonitoringClient.CreateApplicationConfiguration() in /src/Program.cs:line 195 thrown when trying to use application certificate mounted at /etc/opcua-certs/client-pki/own/certs/. Using no security.
Application certificates not mounted, using unsecure connection with Security Policy None
Client is using a certificate with subject 
2 - Discover endpoints of opc.tcp://<redacted DESKTOP-HOSTNAME>:53530/OPCUA/SimulationServer.
info: Microsoft.Hosting.Lifetime[0]
      Now listening on: http://[::]:8083
info: Microsoft.Hosting.Lifetime[0]
      Application started. Press Ctrl+C to shut down.
info: Microsoft.Hosting.Lifetime[0]
      Hosting environment: Production
info: Microsoft.Hosting.Lifetime[0]
      Content root path: /app
Exception: One or more errors occurred. (Error establishing a connection: BadNotConnected)
info: Microsoft.Hosting.Lifetime[0]
      Application is shutting down...

After:

.NET Core OPC UA Console Client Start
1 - Create an Application Configuration.
Exception System.IO.DirectoryNotFoundException: Could not find a part of the path '/etc/opcua-certs/client-pki/own/certs'.
   at System.IO.Enumeration.FileSystemEnumerator`1.CreateDirectoryHandle(String path, Boolean ignoreNotFound)
   at System.IO.Enumeration.FileSystemEnumerator`1.Init()
   at System.IO.Enumeration.FileSystemEnumerator`1..ctor(String directory, Boolean isNormalized, EnumerationOptions options)
   at System.IO.Enumeration.FileSystemEnumerable`1..ctor(String directory, FindTransform transform, EnumerationOptions options, Boolean isNormalized)
   at System.IO.Enumeration.FileSystemEnumerableFactory.FileInfos(String directory, String expression, EnumerationOptions options, Boolean isNormalized)
   at System.IO.DirectoryInfo.InternalEnumerateInfos(String path, String searchPattern, SearchTarget searchTarget, EnumerationOptions options)
   at System.IO.DirectoryInfo.GetFiles(String searchPattern, EnumerationOptions enumerationOptions)
   at System.IO.DirectoryInfo.GetFiles(String searchPattern)
   at OpcuaNodeMonitoring.MonitoringClient.GetCertificate(String certificateStorePath) in /src/Program.cs:line 172
   at OpcuaNodeMonitoring.MonitoringClient.CreateApplicationConfiguration() in /src/Program.cs:line 195 thrown when trying to use application certificate mounted at /etc/opcua-certs/client-pki/own/certs/. Using no security.
Application certificates not mounted, using unsecure connection with Security Policy None
Client is using a certificate with subject 
2 - Discover endpoints of opc.tcp://<redacted IP address>:53530/OPCUA/SimulationServer.
info: Microsoft.Hosting.Lifetime[0]
      Now listening on: http://[::]:8083
info: Microsoft.Hosting.Lifetime[0]
      Application started. Press Ctrl+C to shut down.
info: Microsoft.Hosting.Lifetime[0]
      Hosting environment: Production
info: Microsoft.Hosting.Lifetime[0]
      Content root path: /app
    Selected endpoint uses: None
3 - Create a session with OPC UA server.
4 - Browse the OPC UA server namespace.
.
.
.

Special notes for your reviewer:

If applicable:

this PR has an associated PR with documentation in akri-docs
this PR contains unit tests
added code adheres to standard Rust formatting (cargo fmt)
code builds properly (cargo build)
code is free of common mistakes (cargo clippy)
all Akri tests succeed (cargo test)
inline documentation builds (cargo doc)
all commits pass the DCO bot check by being signed off -- see the failing DCO check for instructions on how to retroactively sign commits

Signed-off-by: harrisontin <harrisontin@microsoft.com>

harrison-tin · 2023-03-21T18:44:27Z

/version patch

Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

…c-ip

Signed-off-by: harrisontin <harrisontin@microsoft.com>

diconico07

I'm not familiar with OPC UA URLs so I handled this with "generic" URLs in mind, so this functionally looks good to me.

I added some comments about error handling/safety, as I don't think it is likeable that a rogue/faulty discovered device can make the discovery handler panic (this might not be enough as I didn't check the existing code path, but this should make it less likely).

diconico07 · 2023-03-22T09:06:02Z

discovery-handlers/opcua/src/discovery_impl.rs

+    // Test that it converts the discovery url to an ip address if the discovery url is a hostname that is not resolvable
+    fn test_get_discovery_url_ip() {
+        let ip_url = "opc.tcp://192.168.0.1:50000/";
+        let discovery_url = "opc.tcp://OPCTest:50000/OPCUA/Simluation";


From a DNS point of view this could one day become resolvable (unlikely but yet), we may want to use the reserved tld .invalid here to ensure this (and make it clearer at first glance this is a not resolvable domain).

Suggested change

let discovery_url = "opc.tcp://OPCTest:50000/OPCUA/Simluation";

let discovery_url = "opc.tcp://OPCTest.invalid:50000/OPCUA/Simluation";

diconico07 · 2023-03-22T09:26:37Z

discovery-handlers/opcua/src/discovery_impl.rs

+    let url = Url::parse(&discovery_url).unwrap();
+    let mut path = url.path().to_string();
+    let host = url.host_str().unwrap();


I'm not really fond of panicking if an external service sends crappy things, I think it is better to directly return an Option<String> here and return None if the URL we got is completely invalid (and log an error so the user can be aware of that).

diconico07 · 2023-03-22T09:28:12Z

discovery-handlers/opcua/src/discovery_impl.rs

+    let url = Url::parse(&discovery_url).unwrap();
+    let mut path = url.path().to_string();
+    let host = url.host_str().unwrap();
+    let port = url.port().unwrap();


I'd rather use unwrap_or here with the default port, as it is valid to omit the port in a URL

is there a standard default port for opcua servers?

Well, there is one defined in the opcua crate (didn't check the specs though): https://docs.rs/opcua/latest/opcua/core/constants/constant.DEFAULT_OPC_UA_SERVER_PORT.html

And IANA confirm that same port number (4840) for "OPC UA Connection Protocol"

thank you! updated

Signed-off-by: harrisontin <harrisontin@microsoft.com>

kate-goldenring

LGTM

discovery-handlers/opcua/src/discovery_impl.rs

Signed-off-by: harrisontin <harrisontin@microsoft.com>

harrison-tin added 4 commits March 21, 2023 11:03

convert to IP

fde18b6

Signed-off-by: harrisontin <harrisontin@microsoft.com>

escape double slash

83926c3

Signed-off-by: harrisontin <harrisontin@microsoft.com>

remove debug statement

65189de

Signed-off-by: harrisontin <harrisontin@microsoft.com>

cargo formatting

2720b51

Signed-off-by: harrisontin <harrisontin@microsoft.com>

harrison-tin requested review from bfjelds, kate-goldenring, jiria, Britel, romoh and adithyaj as code owners March 21, 2023 18:21

harrison-tin changed the title ~~Opc ip~~ Verify DiscoveryURL from OPC Server is resolvable Mar 21, 2023

harrison-tin added 2 commits March 21, 2023 11:24

version patch

54ad601

Signed-off-by: harrisontin <harrisontin@microsoft.com>

cargo clippy suggestion

4a9825b

Signed-off-by: harrisontin <harrisontin@microsoft.com>

harrison-tin force-pushed the opc-ip branch from 6363bc9 to 4a9825b Compare March 21, 2023 18:43

github-actions bot added the version/patch Patch version change is needed label Mar 21, 2023

github-actions bot and others added 4 commits March 21, 2023 18:44

Update patch version

ac27c51

Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>

Merge branch 'main' of https://github.com/project-akri/akri into opc-ip

6e82f43

Merge branch 'opc-ip' of https://github.com/harrison-tin/akri into op…

d813b3a

…c-ip

unit test comment

1425f6d

Signed-off-by: harrisontin <harrisontin@microsoft.com>

diconico07 requested changes Mar 22, 2023

View reviewed changes

harrison-tin added 4 commits March 24, 2023 09:49

better comment in test

254bda0

Signed-off-by: harrisontin <harrisontin@microsoft.com>

revert change on default port

cc7a23e

Signed-off-by: harrisontin <harrisontin@microsoft.com>

error handling

71adf26

Signed-off-by: harrisontin <harrisontin@microsoft.com>

default opcua port: 4840

1c8720c

Signed-off-by: harrisontin <harrisontin@microsoft.com>

diconico07 approved these changes Mar 28, 2023

View reviewed changes

adithyaj approved these changes Mar 31, 2023

View reviewed changes

harrison-tin mentioned this pull request Mar 31, 2023

Discovering Prosys OPC UA simulation server passes in DNS instead of IP address, making the server not discoverable #555

Closed

kate-goldenring approved these changes Apr 7, 2023

View reviewed changes

discovery-handlers/opcua/src/discovery_impl.rs Outdated Show resolved Hide resolved

add discovery url in error msg

1482ee7

Signed-off-by: harrisontin <harrisontin@microsoft.com>

harrison-tin added 3 commits April 10, 2023 18:24

version patch

93ef080

Signed-off-by: harrisontin <harrisontin@microsoft.com>

Merge branch 'main' into opc-ip

a2613b0

version patch

b0bd81f

Signed-off-by: harrisontin <harrisontin@microsoft.com>

yujinkim-msft merged commit 7d043e6 into project-akri:main Apr 11, 2023
51 checks passed

This was referenced Apr 17, 2023

OPC discovery handler duplicates the endpoint path #583

Closed

Fix OPC Discovery Handler resolving IP bug #584

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Verify DiscoveryURL from OPC Server is resolvable #570

Verify DiscoveryURL from OPC Server is resolvable #570

harrison-tin commented Mar 21, 2023

harrison-tin commented Mar 21, 2023

diconico07 left a comment

diconico07 Mar 22, 2023

diconico07 Mar 22, 2023

diconico07 Mar 22, 2023

harrison-tin Mar 24, 2023

diconico07 Mar 27, 2023

diconico07 Mar 27, 2023

harrison-tin Mar 27, 2023

kate-goldenring left a comment

	let discovery_url = "opc.tcp://OPCTest:50000/OPCUA/Simluation";
	let discovery_url = "opc.tcp://OPCTest.invalid:50000/OPCUA/Simluation";

Verify DiscoveryURL from OPC Server is resolvable #570

Verify DiscoveryURL from OPC Server is resolvable #570

Conversation

harrison-tin commented Mar 21, 2023

harrison-tin commented Mar 21, 2023

diconico07 left a comment

Choose a reason for hiding this comment

diconico07 Mar 22, 2023

Choose a reason for hiding this comment

diconico07 Mar 22, 2023

Choose a reason for hiding this comment

diconico07 Mar 22, 2023

Choose a reason for hiding this comment

harrison-tin Mar 24, 2023

Choose a reason for hiding this comment

diconico07 Mar 27, 2023

Choose a reason for hiding this comment

diconico07 Mar 27, 2023

Choose a reason for hiding this comment

harrison-tin Mar 27, 2023

Choose a reason for hiding this comment

kate-goldenring left a comment

Choose a reason for hiding this comment