LDS and LDS-ME support

[Pro]

trafficstars

I implemented the LDS (Local Discovery Service) and LDS-ME (Multicast Extenstion) in this PR according to OPC UA Part 12. This means that the full set of Discovery Services is supported now.

[kevinherron]

I'll need some time to familiarize myself with Part 12 again before I can review this in meaningful detail.

After a cursory glance though, a couple things occur to me:

1. These additional discovery services should be opt-in, not enabled by default (when using OpcUaServer, anyway).
2. In addition to optionally supporting these services in OpcUaServer, a new OpcUaDiscoveryServer should probably be introduced. This could be the basis that a standalone discovery server would be built on. We could then implement a simple ExampleDiscoveryServer using this in the milo-examples module.
3. We should figure out some way to make the dependency on jmdns optional as well, some kind of soft dependency mechanism... I'd like to avoid including this dependency unless you're opting into these services.
4. Because of the bureaucracy involved in adding a dependency to the project, especially at this phase as we prepare for a 0.1.0 release, I think this would be best merged post-0.1.0.

edit: and THANK YOU for the contribution

[Pro]

Thanks for the feedback. Here some comments:

1. I would suggest adding a boolean to OpcUaServerConfig to enable/disable the new discovery services. Good to go?
2. What would be the functionality of the OpcUaDiscoveryServer? Only the minimum set of services required for LDS?

• AttributeServiceSet
• MethodServiceSet
• SessionServiceSet
• DiscoveryServiceSet

3. Agree
4. Also agree

[kevinherron]

I would suggest adding a boolean to OpcUaServerConfig to enable/disable the new discovery services. Good to go?

That might work. After thinking about it more it might help if we enumerated the possible scenarios for running a server and possibly discovery server and figure out how the library could best handle it. I've never given much thought to it before - right now I run all my servers on a custom port and just have the server itself implement FindServers and GetEndpoints. But there's obviously more scenarios than that, and I think would be great to support them with the help of this PR.

Without an LDS you have just one basic scenario:

• Running a server without the usage of an LDS at all (just FindServers and GetEndpoints implemented by the server directly). How it works now.

With an LDS the options get more complicated.

• Are we starting the LDS or is it one that will already be present on the system?
• If we will be the LDS, do we simply implement those additional services on the same stack server and port that the server runs on, or do we start up another stack server running on the proper port for discovery (4840)?
• What about registration with the LDS?

I'm sure I'm missing some stuff here.

What would be the functionality of the OpcUaDiscoveryServer? Only the minimum set of services required for LDS?

Isn't it just the DiscoveryServiceSet? Or is there something in the extended discovery services or profile that would require session/attribute/method?

[Pro]

With an LDS the options get more complicated.

I think we should keep it very simple: The developer can/should decide (with the boolean config variable) if the current server supports the DiscoveryServiceSet or not. This answers all the questions:

Are we starting the LDS or is it one that will already be present on the system?

The developer knows that and decides

If we will be the LDS, do we simply implement those additional services on the same stack server and port that the server runs on, or do we start up another stack server running on the proper port for discovery (4840)?

A LDS may at the same time be a "normal" OPC UA server, so we should not start any additional server automatically. E.g. the developer may also decide that the LDS server is running on port 14840, which is also a valid setup.

What about registration with the LDS?

There will be an additional config variable which enables or disables the registration with a specific LDS server.

This is also more or less the implementation how we did it in open62541: https://github.com/open62541/open62541/pull/732

[kevinherron]

Ok all sounds reasonable so far 👍

At some point you might rebase or cherry pick from master, I made the existing FindServers implementation a little better in https://github.com/eclipse/milo/commit/492580186fb730c516fb83f56a4be61ecbe204d1

[Pro]

I already merged master, see https://github.com/eclipse/milo/pull/89/commits/8eb1926f26d189cddea04b65771ac457e3a32c81

I'm currently implementing the stuff we discussed, and will also try to add examples and unit tests.

[kevinherron]

Heads up: sometime this weekend I'm being forced to rewrite history in master. Once that happens you'll need to fetch and reset master in your fork and then rebase this branch onto the "new" master. Make sure you don't merge or the rewritten history may be re-introduced. I'll update this once it happens.

[Pro]

@kevinherron may I ask why you need to rewrite history? That's really bad practice and will break not only my branch but all the other forks. So there should be a really GOOD reason why you want to rewrite the master...

[kevinherron]

Eclipse Foundation is forcing me to do it. I've already lost the appeal. Believe me, this is not my choice.

Legal bureaucracy... 😞

At some point in history I accidentally committed a file from the OPC foundation that is licensed under GPL. Even though I never used this file in any way Eclipse policy is that the file (and its history) must be deleted from the repository. This is all part of the IP validation process in preparation for a release.

[kevinherron]

As you can see by the collateral damage to this PR, the rewrite has taken place. You'll need to rebase your changes and rewrite the history of your fork and branch now. Really sorry for the inconvenience.

[Pro]

@kevinherron I now implemented Examples for the discovery and cleaned up the code.

Can you review it and tell me if there are any changes required?

Unfortunately I didn't find any way to make the dependency on jmdns optional. There are optional dependencies in Maven, but then you also need to exclude the .java file from compilation. If you do that, the other depending class does not find the excluded class.

I also did not create a separate OpcUaDiscoveryServer class, because in my opinion it just makes the code more complicated.

Let me know if you have any suggestions for improvement!

[kevinherron]

I'll take a closer look this weekend, but some superficial stuff I see:

• Continuation indents should be 4 spaces if your editor is configurable in that manner. Checkstyle isn't looking for this right now, so that's my fault really.
• I'd suggest the new configuration fields be named discoveryServerEnabled and multicastEnabled, leading to getters/setters named isDiscoveryServerEnabled(), isMulticastEnabled(), setDiscoveryServerEnabled(), setMulticastEnabled().

[Pro]

Done as requested :+1:

[kevinherron]

Just wanted to let you know I haven't forgotten about this! I'm just a little pre-occupied trying to get a bunch of work on DataTypes and encoding done for 0.2.0. I'll be sure to merge this for 0.2.0 as well.

[Pro]

I don't mind if you change things directly here in this PR (you can push to it). Otherwise, maybe end of next week I'll have some time to address your comments

[Pro]

Just as an additional note: I already tested this Milo LDS implementation in combination with the one I wrote for open62451 and they were able to talk to each other, anyways there may be still some minor issues in the code.

[Pro]

@kevinherron I changed the things you mentioned in your review

[Pro]

@kevinherron I now also added unit tests for discovery. Is there anything else I can improve?

[kevinherron]

The only thing I remember seeing was the issue of whether or not it can shutdown and startup again cleanly within a continually running JVM/env (no standalone threads etc...). I'll gladly take a look at that myself once its merged though.

I'm probably going to create a dev/0.2.0 branch this week... if the GitHub UI makes it easy to change your base branch to that once I've created it that would be the last thing to do. Then I'll squash+merge this as well as the work I've been doing on the type system in the type-dictionaries branch and get things moving...

[kevinherron]

Oh, also Travis CI seems to be unhappy about something...

edit: Maybe the certificate is expired? I updated the certificate on the master branch somewhat recently.

[kevinherron]

Branch dev/0.2.x now exists. I'd like to start collecting work for 0.2.0 release there.

[Pro]

I rebased this PR onto dev/0.2.x

The current error in travis is due to the fact that the identity checking test actually never worked before. The KeyStoreLoader always initialized the server certificates with null because your example-certs.pfx did not contain any certificate with identity server-ai.

I changed it to use a valid certificate (https://github.com/eclipse/milo/pull/89/commits/081ea7b6563e70d55e4b9c998f214411682511ef#diff-3ea5a948dc47d2129f93c4929c15fd02R45) and now the identity is actually checked, but can not be validated. Maybe you can check what's wrong here, since I don't have enough insight into the encryption implementation of Milo.

[kevinherron]

I've got this merged into a local copy and am actually looking through the changes in detail in my IDE.

I think aside from modifying it to not use that thread for MdnsHelper, it's mostly nitpicky things left. I can make the changes before merging, or I can fork a copy of your repo, make them, and submit them as PRs to your branch. Let me know which approach you'd prefer.

edit: I'm just gonna do the fork and PR approach.

[Pro]

Maybe it's best if you just commit the changes into this PR's branch, so everything is kept together. See https://help.github.com/articles/committing-changes-to-a-pull-request-branch-created-from-a-fork/

And then merge it. That's probably better for later reference.

[kevinherron]

Assuming the changes I just made to MdnsHelper are what you had in mind originally, the last thing left for me to do is figure out how to give MdnsHelper a lifecycle rather than have it be a thread that lives forever.

[Pro]

I changed the MdnsHelper to not start its own thread, as you suggested. Fortunately it was quite easy. I'm not sure why I added this additional thread in the first place.

[kevinherron]

I have a bunch of changes for this sitting on my home computer. I'll try to push them Sunday evening or Monday.

[kevinherron]

Ugh, pushed the WIP but didn't have my email configured properly on this computer.

You'll notice I split a lot of registration code you had written out into a helper class and moved it to the sdk-client module. The way you had it organized required sdk-server to depend on sdk-client. If somebody wants to do this in their implementation (and then use the RegistrationHelper to register their server) that's fine, but I'm not okay with having either of those modules depend on each other out of the box.

edit: Also the WIP is not compiling... very much WIP. I just wanted to get it pushed before you made any more changes.

[Pro]

Your changes look good! I also like it better that way. Currently I'm not working on this branch/PR so feel free to push/force push if needed.

[Pro]

@kevinherron any update on this PR?