Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Enabling OPC UA over WebSockets (again) #1836

Draft
wants to merge 6 commits into
base: master
Choose a base branch
from

Conversation

maxschiffer
Copy link

Proposed changes

Hello all,

some time ago there was an effort to implement the WebSockets transport in the UA.NetStandard SDK and the branch demo/webapi was created.
Since the architecture has changed since then (transport bindings have been moved to a different project) and it could not be moved to the main branch without any additional work.

I've taken the code from the demo/webapi branch and have migrated it into a new project Opc.Ua.Bindings.WebSockets.
Additionaly I've updated the asymmetric/symmetric security implementations, since in the prototypical only parts were implemented.
Since I did not feel comfortable to write this on my own, I've just taken the existing implementation directly from the opc.tcp channel implementation in the core project.

Besides that I've just put the bindings in the core assembly so it is actually loaded and the URL schema is known.

Related Issues

I'm having some trouble with the tests, it was straight forward to enable the client and server tests for opc.wss,
but something seems to be wrong with the test infrastructure.
When executing single tests, they pass in milliseconds, but when executing several tests it gets stuck for an hour or longer before it passes (e.g. the client connect tests for opc.wss). And some other tests just fail because the transport channel is suddenly null and or it just timeouts.

I've been using this custom-built version of the SDK and did not experience any issues (which definitly does not mean that there aren't any).

Types of changes

What types of changes does your code introduce?

  • Bugfix (non-breaking change which fixes an issue)
  • Enhancement (non-breaking change which adds functionality)
  • Test enhancement (non-breaking change to increase test coverage)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected, requires version increase of Nuget packages)
  • Documentation Update (if none of the other choices apply)

Checklist

  • I have read the CONTRIBUTING doc.
  • I have signed the CLA.
  • I ran tests locally with my changes, all passed.
  • I fixed all failing tests in the CI pipelines.
  • I fixed all introduced issues with CodeQL and LGTM.
  • I have added tests that prove my fix is effective or that my feature works and increased code coverage.
  • I have added necessary documentation (if appropriate).
  • Any dependent changes have been merged and published in downstream modules.

Further comments

Since this is a larger contribution, I again want to point out that I actually did not write most of the code but just took it from the prototypical branch demo/webapi and would require some help to get the tests passing.

@CLAassistant
Copy link

CLAassistant commented Jun 1, 2022

CLA assistant check
All committers have signed the CLA.

@lgtm-com
Copy link

lgtm-com bot commented Jun 1, 2022

This pull request introduces 36 alerts when merging 1775a15 into 8cc18b3 - view on LGTM.com

new alerts:

  • 16 for Useless assignment to local variable
  • 6 for Missing Dispose call on local IDisposable
  • 5 for Use of default ToString()
  • 5 for Dereferenced variable may be null
  • 3 for Dereferenced variable is always null
  • 1 for Futile conditional

@mregen
Copy link
Contributor

mregen commented Jun 1, 2022

thanks for driving this .. I see a lot of duplications of crypto code in the wss lib, please try to recycle core functions...

</ItemGroup>

<ItemGroup>
<PackageReference Include="Microsoft.AspNetCore.Server.Kestrel" Version="2.1.3" />
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this kestrel version is end of life -- check the https project how to use latest kestrel...

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done. I just remove all Microsoft.AspNetCore.Server.Kestrel dependecies and add the new Microsoft.AspNetCore.Http dependency.

    <PackageReference Include="Microsoft.AspNetCore.Http" Version="2.2.2" />

No code changes were required at all, the test results remain unchanged (when execting each test on its own):
image

@lgtm-com
Copy link

lgtm-com bot commented Jun 2, 2022

This pull request introduces 36 alerts when merging 7de6922 into e3ac2bf - view on LGTM.com

new alerts:

  • 16 for Useless assignment to local variable
  • 6 for Missing Dispose call on local IDisposable
  • 5 for Use of default ToString()
  • 5 for Dereferenced variable may be null
  • 3 for Dereferenced variable is always null
  • 1 for Futile conditional

@mregen
Copy link
Contributor

mregen commented Jun 3, 2022

Hi @maxschiffer , please check out this implementation for websocket: https://github.com/Azure/Industrial-IoT/tree/main/components/opc-ua/src/Microsoft.Azure.IIoT.OpcUa.Protocol/src/Transport/Http

There is a hook you can use with IMessageSocket, it may be a implementation that requires less duplicate code.

@codecov
Copy link

codecov bot commented Jun 6, 2022

Codecov Report

Merging #1836 (e3ac2bf) into master (88a96a1) will increase coverage by 1.03%.
The diff coverage is 49.30%.

❗ Current head e3ac2bf differs from pull request most recent head 7de6922. Consider uploading reports for the commit 7de6922 to get more accurate results

@@            Coverage Diff             @@
##           master    #1836      +/-   ##
==========================================
+ Coverage   54.61%   55.65%   +1.03%     
==========================================
  Files         319      319              
  Lines       58669    58788     +119     
==========================================
+ Hits        32045    32716     +671     
+ Misses      26624    26072     -552     
Impacted Files Coverage Δ
Libraries/Opc.Ua.Client/DataDictionary.cs 83.33% <ø> (ø)
...es/Opc.Ua.Server/Aggregates/AggregateCalculator.cs 0.00% <0.00%> (ø)
...aries/Opc.Ua.Server/Aggregates/AggregateManager.cs 25.42% <0.00%> (ø)
...c.Ua.Server/Aggregates/CountAggregateCalculator.cs 0.00% <0.00%> (ø)
....Ua.Server/Aggregates/MinMaxAggregateCalculator.cs 0.00% <0.00%> (ø)
...a.Server/Aggregates/StartEndAggregateCalculator.cs 0.00% <0.00%> (ø)
...braries/Opc.Ua.Server/Diagnostics/MonitoredNode.cs 51.54% <0.00%> (-3.40%) ⬇️
...raries/Opc.Ua.Server/Subscription/MonitoredItem.cs 45.02% <0.00%> (-0.96%) ⬇️
...pc.Ua.Core/Stack/Configuration/OAuth2Credential.cs 0.00% <0.00%> (ø)
...ack/Opc.Ua.Core/Stack/State/AlarmConditionState.cs 20.84% <0.00%> (+20.84%) ⬆️
... and 63 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 88a96a1...7de6922. Read the comment docs.

Copy link
Contributor

@mregen mregen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please update the pull request if there is still interest.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants