Add (optional) VISSv2 interface #642

argerus · 2023-08-28T00:46:16Z

Add VISSv2 interface to databroker.
Gated by feature flag viss (not built by default).

Supports:

get, set, subscribe and unsubscribe
Authorization (using same jwt scope format as the other interfaces)

Not implemented:

Filters
TLS

lukasmittag · 2023-08-28T06:40:51Z

Isn't it already implemented through https://github.com/eclipse-kuksa/kuksa-viss ?

argerus · 2023-08-28T07:26:51Z

Isn't it already implemented through https://github.com/eclipse-kuksa/kuksa-viss ?

Sure. But that requires running a separate service (written in python). Useful for demonstration purposes, but not very resource efficient or easy to make "production ready".

With that said, I haven't really evaluated the need for a VISSv2 service, just wanted to check the feasibility of doing it and took it up as a fun weekend project :).

Conclusion is that it seems very feasible. And apart from the lower footprint, it would IMHO also be a lot easier to make production ready thanks to the type safety / thread safety provided by Rust.

EDIT:
And if having "feature parity" with kuksa-val-server is a goal. This would basically accomplish that (given a few final touches).

kuksa_databroker/databroker/src/main.rs

erikbosch · 2023-09-06T10:48:35Z

Should we not add a short documentation somewhere. Like how to build it, how to test it. What limitations are there. Shall it for now be considered as "alpha" (see https://github.com/eclipse/kuksa.val/wiki/KUKSA.val-Component-Maturity), i.e. not suitable for production use? I.e. for now we will not test the interface as part of release testing?

How much have been tested? Have we for instance tested with kuksa-client (python) using websocket towards the Databroker VISS interface?

argerus · 2023-09-06T11:32:07Z

How much have been tested? Have we for instance tested with kuksa-client (python) using websocket towards the Databroker VISS interface?

It has been tested with kuksa-client. Feel free to give it a go.

Since kuksa-client isn't fully VISSv2 compliant (or compliant with any VISS version really?), a small change is needed in kuksa-client to support VISSv2 authorization (I have a local branch with that).

Should we not add a short documentation somewhere. Like how to build it, how to test it. What limitations are there.

If anyone is up for that, sure. This was just a "weekend project" which I think would be a useful addition to the project. But it is not a priority at the moment.

With that said, my impression is that it would - even in it's current state - be a better option for "supporting VISS" than the available alternatives.

It implements standards compliant VISSv2 (which I don't think is the case with kuksa-val-server)
It seems to be "more" standards compliant and feature complete than kuksa-viss referenced above.
It would be a more realistic thing to make production ready at some point.

And as the whole thing is gated behind a feature flag, one option would be to have it "hidden" for now, but easily enabled by cargo build --features viss for anyone who wants to. If we decide it would be a useful thing to have going forward, documentation etc can be added.

I do think it would be useful to merge though, as that would prevent the code from rotting.

EDIT:
Branch that adds support for VISSv2 authorization to kuksa-client feature/kuksa_client_viss_v2_auth. I can make it a PR..

erikbosch · 2023-09-07T07:30:48Z

I support merging. One of the problem with VISSv2 is the lack of (commonly used) reference implementations. One which possibly could be considered as the "official one" exists at https://github.com/w3c/automotive-viss2, and if VISSv2 gets higher priority it could be an idea to verify that "their solution" and "our solution" interprets the VISSv2 standard in the same way.

kuksa_databroker/databroker/src/viss/v2/conversions.rs

erikbosch

Looks ok, as it is intended as an experimental/optional feature I think it can be merged without exhaustive testing.

SebastianSchildt · 2023-09-15T06:46:08Z

I am still not sure, what the "correct" way is to move forward here (in terms of maintainability/usage), but letting this rot would also be sad. Moving this behind a feature flag for now is a good first step. I think to merge this, there are few other things we need to do and should not do

Even when compiled (with feature flag), it should need to be enabled explicitly by command line argument and when that is enabled should print a warning along the lines of "You enabled VISS; this is an experimental feature"
- The rationale here is, if we would - at a later point - decide to include this in our default/released builds and something "is wrong", i.e. not secure, or can crash/confuse db, it would not be running by default. And it is a good idea anyway not unnecessarily exposing additional network ports
Write a small doc how to enable it and what VISS is supported: Not a "book", but I would expect something like "VISS get call, no history, no filters, no wildcards, and some example message (like in the VISS transport spec does)
Haven't checked here, but in general: Do NOT extend VISS. Have a ssubset of VISS, not something that somehow "overlaps", i.e. we do not need to be able to do providers (i.e. set sensors) in VISS. Let's keep this an "application" protocol as ~~god~~ ~~W3C~~ Ulf intended it to be

argerus · 2023-09-15T13:22:49Z

Even when compiled (with feature flag), it should need to be enabled explicitly by command line argument and when that is enabled should print a warning along the lines of "You enabled VISS; this is an experimental feature"

That's how it's implemented now (except for the warning). Even if the viss feature is enabled, no "VISSv2" code will run unless --enable-viss is also supplied as an argument at startup.

Haven't checked here, but in general: Do NOT extend VISS.

The code is strictly following the standard (barring bugs). And I agree that it should continue to do so.

Write a small doc how to enable it and what VISS is supported

That is missing, but I can add it.

argerus · 2023-10-19T21:55:36Z

Rebased.
Added documentation.
Included VISSv2 support for kuksa-client (which lived in a separate branch previously).
VISSv2 in kuksa-client is only enabled if the negotiated subprotocol is VISSv2. If no subprotocol is negotiated, it reverts to it's previous behaviour in order to remain backwards compatible with kuksa-val-server which isn't compatible with VISSv2 (and doesn't negotiate any subprotocol).

- Add VISSv2 interface to databroker - Gated by feature flag `viss` (not built by default) - Supports "get", "set", "subscribe" & "unsubscribe" - Supports authorization (same jwt scope format as other interfaces) Not implemented: - Filters (except static-metadata) - TLS

SebastianSchildt · 2023-10-23T07:03:42Z

@erikbosch Can you check this (again). Also I assume we want to merge it fast, but after "release"/tag?

argerus · 2023-10-23T07:20:34Z

Also I assume we want to merge it fast, but after "release"/tag?

Before or after release tag shouldn't matter as it's not included in the release artifacts anyway.

erikbosch · 2023-10-23T14:05:37Z

doc/protocol/support.md

+| Protocol                  | KUKSA.val server | KUKSA.val databroker |
+|---------------------------|:----------------:|:--------------------:|
+| VISS V1                   |         -        |           -          |
+| VISS V2                   |        x/-       |          x/-         |


Isn't it a bit confusing that we in kuksa-client code specifies the two subprotocols:

subprotocols = ["VISSv2", "VISSv1"]

but here we say that both server and databroker supports (parts of) VISSv2

Would it be better to call them (in kuksa-client) something like "VISSv2-std" and "VISSv2-extended" or something similar.

Yes, that is confusing.

When I added it, I assumed that the "not VISSv2" that kuksa-val-server and kuksa-client speaks was VISSv1. So if any server were to return VISSv1, kuksa-client could just use the that.

But reading the documentation here, it seems that it is actually a modified version of VISSv2 they use. Which is also confusing, because I don't think a VISSv2 client would work with kuksa-val-server, and kuksa-client will not work with a VISSv2 server (before this PR). And I guess neither will work with VISSv1 (so having VISSv1 in there doesn't make sense).

Pushed an update which removed "VISSv1" from the requested subprotocols.

Would it be better to call them (in kuksa-client) something like "VISSv2-std" and "VISSv2-extended" or something similar.

The VISSv2 specification explicitly states that it SHALL be "VISSv2", so no.

The sub-protocol name SHALL be 'VISSv2' with the digit 2 being the version number.

And as kuksa-val-server doesn't support negotiating subprotocol in the first place, a prerequisite for using that to identify what protocol to use in kuksa-client would be to add support for that in kuksa-val-server.

erikbosch · 2023-10-23T14:13:30Z

doc/protocol/support.md

+| Feature                       | KUKSA.val server  | KUKSA.val databroker |
+|-------------------------------|:-----------------:|:--------------------:|
+| Read                          |                   |                      |
+|   - Authorized Read           | x<sup>1,2</sup>   |           x          |


Would it be an idea to duplicate the "Authorized Read" and "Authorized Update" lines so that we have lines for:

Authorized Read of current value (supported by both)

Authorized Read of target value (only supported by server)

Authorized Update of current value (only supported by server)

Authorized Update of target value (supported by both)

To make it more clear what we have in server WS protocol

Well, this table - as I understand it - lists what features of VISSv2 are supported.

If we are going to add non-VISSv2 features as well, maybe we should just create a different table for that, which lists the functionality supported by the different protocols in this project. Furthermore, the VISSv2 features that are listed as "partially" supported by KUKSA.val server here, would not work when using any standards compliant VISSv2 client anyway, so maybe a pure VISSv2 feature support table mostly makes sense for databroker.

I.e.

One table that lists what features of VISSv2 are supported (by databroker and maybe kuksa-val-server).

One table that lists the features supported by the different protocols in this project.

erikbosch · 2023-10-23T14:36:53Z

kuksa-client/kuksa_client/cli_backend/ws.py

-        req["action"] = "set"
-        req["path"] = path
-        req["attribute"] = attribute
+        if self.subprotocol == "VISSv2":


Standard VISSv2 only supports setting target value and reading current value, so I suggest we give an error if someone use "setValue" and instead support "setTargetValue". Similar give an error if someone call "getTargetValue"

Pushed an update to this effect.

argerus force-pushed the feature/viss_v2_interface branch 5 times, most recently from 84437c6 to 7cedfec Compare September 5, 2023 06:50

erikbosch reviewed Sep 6, 2023

View reviewed changes

kuksa_databroker/databroker/src/main.rs Outdated Show resolved Hide resolved

argerus force-pushed the feature/viss_v2_interface branch from 7cedfec to a458eeb Compare September 6, 2023 11:48

erikbosch reviewed Sep 7, 2023

View reviewed changes

kuksa_databroker/databroker/src/viss/v2/conversions.rs Outdated Show resolved Hide resolved

erikbosch approved these changes Sep 7, 2023

View reviewed changes

argerus force-pushed the feature/viss_v2_interface branch 4 times, most recently from 708c258 to e400b8d Compare October 19, 2023 21:51

argerus added 2 commits October 19, 2023 23:59

[databroker] Remove type Timestamp & TimestampArray

9424948

argerus force-pushed the feature/viss_v2_interface branch from e400b8d to 4585ad4 Compare October 19, 2023 21:59

argerus force-pushed the feature/viss_v2_interface branch from 4585ad4 to 6670804 Compare October 23, 2023 07:18

argerus force-pushed the feature/viss_v2_interface branch from 6670804 to f0e686f Compare October 23, 2023 07:21

erikbosch reviewed Oct 23, 2023

View reviewed changes

argerus added 3 commits October 23, 2023 17:16

[kuksa-client] Add VISSv2 support

552917a

Add VISSv2 databroker documentation

ed06a05

Add lint action for feature viss

188ec6e

argerus force-pushed the feature/viss_v2_interface branch from cac8cd4 to 188ec6e Compare October 23, 2023 15:16

erikbosch approved these changes Oct 24, 2023

View reviewed changes

erikbosch merged commit 260c87c into eclipse:master Oct 25, 2023
16 checks passed

erikbosch deleted the feature/viss_v2_interface branch October 25, 2023 07:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add (optional) VISSv2 interface #642

Add (optional) VISSv2 interface #642

argerus commented Aug 28, 2023 •

edited

Loading

lukasmittag commented Aug 28, 2023

argerus commented Aug 28, 2023 •

edited

Loading

erikbosch commented Sep 6, 2023

argerus commented Sep 6, 2023 •

edited

Loading

erikbosch commented Sep 7, 2023

erikbosch left a comment

SebastianSchildt commented Sep 15, 2023

argerus commented Sep 15, 2023

argerus commented Oct 19, 2023 •

edited

Loading

SebastianSchildt commented Oct 23, 2023

argerus commented Oct 23, 2023

erikbosch Oct 23, 2023

erikbosch Oct 23, 2023 •

edited

Loading

argerus Oct 23, 2023 •

edited

Loading

argerus Oct 23, 2023

argerus Oct 23, 2023 •

edited

Loading

erikbosch Oct 23, 2023

argerus Oct 23, 2023 •

edited

Loading

erikbosch Oct 23, 2023

argerus Oct 23, 2023

Add (optional) VISSv2 interface #642

Add (optional) VISSv2 interface #642

Conversation

argerus commented Aug 28, 2023 • edited Loading

lukasmittag commented Aug 28, 2023

argerus commented Aug 28, 2023 • edited Loading

erikbosch commented Sep 6, 2023

argerus commented Sep 6, 2023 • edited Loading

erikbosch commented Sep 7, 2023

erikbosch left a comment

Choose a reason for hiding this comment

SebastianSchildt commented Sep 15, 2023

argerus commented Sep 15, 2023

argerus commented Oct 19, 2023 • edited Loading

SebastianSchildt commented Oct 23, 2023

argerus commented Oct 23, 2023

erikbosch Oct 23, 2023

Choose a reason for hiding this comment

erikbosch Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

argerus Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

argerus Oct 23, 2023

Choose a reason for hiding this comment

argerus Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

erikbosch Oct 23, 2023

Choose a reason for hiding this comment

argerus Oct 23, 2023 • edited Loading

Choose a reason for hiding this comment

erikbosch Oct 23, 2023

Choose a reason for hiding this comment

argerus Oct 23, 2023

Choose a reason for hiding this comment

argerus commented Aug 28, 2023 •

edited

Loading

argerus commented Aug 28, 2023 •

edited

Loading

argerus commented Sep 6, 2023 •

edited

Loading

argerus commented Oct 19, 2023 •

edited

Loading

erikbosch Oct 23, 2023 •

edited

Loading

argerus Oct 23, 2023 •

edited

Loading

argerus Oct 23, 2023 •

edited

Loading

argerus Oct 23, 2023 •

edited

Loading