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

Example VSS layers for defining access control (Discussion material) #137

Closed
wants to merge 4 commits into from

Conversation

gunnarx
Copy link
Contributor

@gunnarx gunnarx commented Feb 11, 2020

DO NOT MERGE. This is discussion material.

The intention was to show access control groups useful for the W3C protocol. Unfortunately an Android-related example was pushed instead. Maybe later I have time to clean up and force push an update.

For now, the concepts should be still possible to derive from reading this.

@gunnarx gunnarx changed the title Example VSS layers (work in progress) Example VSS layers for defining access control (WIP, DO NOT MERGE) Feb 11, 2020
@gunnarx
Copy link
Contributor Author

gunnarx commented Feb 11, 2020

The diff is quite useless, probably better to just view the files here:
https://github.com/gunnarx/vehicle_signal_specification/tree/vss_layers_generic/vss_layers

(*) Oh one more thing. The layer examples are based on totally outdated VSS signals, so be aware of this too -- look at the concept.

@gunnarx gunnarx force-pushed the vss_layers_generic branch from cab567c to 80911c6 Compare February 2, 2021 18:20
@gunnarx gunnarx force-pushed the vss_layers_generic branch 2 times, most recently from 622797d to abcf3f4 Compare February 2, 2021 20:33
@gunnarx
Copy link
Contributor Author

gunnarx commented Feb 2, 2021

A README has been added to define the VSS layers concept.

BUT once it is agreed, the description should probably be placed with documentation and the examples should probably not be merged into the actual repo either. They are just here for viewing.

Therefore, still Do-not-merge, but please give feedback on the definition.

@gunnarx gunnarx force-pushed the vss_layers_generic branch 2 times, most recently from 4fa4694 to 477d341 Compare February 2, 2021 20:58
@erikbosch
Copy link
Collaborator

Would the "layer" concept in some way be possible to use to specify which datapoints that are supported/mandatory/optional. Today we have a lot of datapoints, but no way to specify if the datapoints are mandatory or optional.

Assume e.g. the fleet management system of Hertz require certain data. Then Hertz could define a layer (or profile?) specifying which datapoints that are required, and whether read or write access is necessary. Maybe something like below (ignore the syntax) - all vehicles in the fleet must support a small standardized set of datapoints (e.g. VIN, length, weight) and in addition to this information on whether the vehicle has HVAC and a Hertz specific datapoint on whether the car has StuddedTyres mounted or not.

With this information it would be possible to verify or test that a vehicle fulfils the requirements of the rental company.

/* A rental company like Hertz could add additional requirements that must
   be supported by all vehicles in their fleet  - all */
Hertz.mandatory_read {
  default: 
    cvii.default_passenger_vehicle; /* A small standardized list of datapoints all vehicles must support, like e.g. VIN */
  true: 
    Vehicle.VehicleIdentification.ACRISSCode,
    Vehicle.Cabin.HasHVAC, /* Info if vehicle has hvac or not */
    Hertz.StuddedTyres; 
}

/* Another layer/property can be used to define which datapoints that Hertz must have write access to */
Hertz.mandatory_write {
  default: 
    false;
  true: 
    Hertz.StuddedTyres;
}

@UlfBj
Copy link
Contributor

UlfBj commented Feb 9, 2021

I have the following comments after reading the README file.

The new concept can use namespacing as deep as desired (in this example, the prefix vss.remote-interface.permissions. is used by all definitions). The definitions themselves may also add additional branches (in this case gnss. ), as desired.

I do not understand the above. To keep it understandable I think the Reverse layer example should use the same metadata as used in the Standard layer example (extended with multiple ECUs to show the array concept). Then the above namespacing concept could be explained separately.

The Reverse layer concept is only applicable for bullet 3 "Add new metadata", I believe?

I think the Tool support chapter should describe one design at a level that can be used for implementation, and then leave the door open for alternative designs/implementations. An implementation of the described design should then also be found at the repo.

Copy link
Collaborator

@SebastianSchildt SebastianSchildt left a comment

Choose a reason for hiding this comment

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

Generally speaking (because the individual comments are, of course all nitpicks :) )I think this is largely fine and pretty much describes, what we want.

My only strong feeling is, that the "Reverse layer" is a very bad (and actually redundant) concept. But maybe I didn't get it.

Others will be system, product, or company specific.

### Deployment model

Copy link
Collaborator

Choose a reason for hiding this comment

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

I feel this whole section is a lot of text, without saying much.

I would suggest keeping paragraph two A Deployment Model keeps the information necessary to realize the described interface in a particular environment... and probably adding some more clear examples such as "values" for attributes or min/max metadata

1. Add new metadata, or relationships to new concepts for the purpose of
creating deployment-models, bindings to existing technology, classification
and many other things.
1. Remove signals from the final data model (SYNTAX TO BE DEFINED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I know we discussed this, but I am not super-sure we need it.
After all, an implementation can still give "404" or "40x not implemented" on signals from the "core" spec. I think it would be a more clean design, than letting layers "delete" anything (i.e. I would rather provide a deployment/stack specific layer that tags "not-implemented: true" to relevant leaves, and letting the implementation act accordingly)

Just an opinion though, I won't fight to the death for it

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can realize it with the metadata approach of 1) as you say, but to find a common way how to do it, could be beneficial. Maybe it's just a wording issue, and something like Invalidate unsupported nodes might be more clear than remove.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Remove is really specific, something like omit signals could work if we talk about management of nodes for implementation phase.

names.

Let's call that a `Reverse Layer`.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this section "breaks" the model.

I think it is "legal", and "possible", because it is akin to just extending private data (i.e. creating new branches/subtrees) and filling whatever data you like.

But in this specific example I would assume it is bad practice, so we should not endorse it.

@gunnarx
Copy link
Contributor Author

gunnarx commented Feb 11, 2021

@erikbosch wrote:

Would the "layer" concept in some way be possible to use to specify which datapoints that are supported/mandatory/optional. Today we have a lot of datapoints, but no way to specify if the datapoints are mandatory or optional.

Yes for sure, technically it could of course be done by adding a layer like this:

Vehicle.X.Y.a
   - mandatory : true

Vehicle.X.Y.b
   - mandatory : false

and so on...

But I am not convinced we have a plan or need to do that and therefore I would prefer not using it as an example in the README, even if it would be possible. I have moved the question of whether this should be done or not to #238 so we can keep that away from the more technical discussion here.

I will try to answer the Hertz example after some more thinking.

@gunnarx
Copy link
Contributor Author

gunnarx commented Feb 11, 2021

@UlfBj wrote:

I do not understand the above.

OK, I will look over and try to rewrite that section.

I think the Reverse layer example should use the same metadata as used in the Standard layer example

OK, maybe we can have a simple example that is shown in both ways. The reason it is not using the same now is that (as I see it) some use cases are more convenient with the Reverse format, and others are more convenient with the standard format. Therefore I thought it would actually be more understandable to use different examples to explain why there should be two different methods.

I will try to simplify the explanation first. After that I want to ask again if it is really still hard to understand the examples and concepts.

The Reverse layer concept is only applicable for bullet 3 "Add new metadata", I believe?

That appears true, yes. I haven't thought much about that, but I'm not sure if we have to point that out or if it is simply implied once the concept is understood.

But we could also avoid special cases by allowing in theory, that the same thing is defined using both forward and reverse layers, and if so things could, again theoretically be redefined or removed also with reverse syntax. (N.B. syntax for removal is not specified yet in either forward or reverse ways).

I think the Tool support chapter should describe one design at a level that can be used for implementation, and then leave the door open for alternative designs/implementations. An implementation of the described design should then also be found at the repo.

First I'd say those requirements probably do not belong in a "README" (Actually maybe none of this belongs in a README in the end - I expect it to become part of the combined documentation after we agree). Then, I want to discuss a bit more because I think it is needed that we write down some rules for how tools behave with layers, otherwise the layers concept is not clearly defined. But we can definitely discuss the details more.

@gunnarx
Copy link
Contributor Author

gunnarx commented Feb 11, 2021

@SebastianSchildt wrote:

I feel this whole section is a lot of text, without saying much.

OK, I will look at it again.

(on reverse layers)

I think this section "breaks" the model.

If I understand correctly, you don't see the need for the whole "Reverse layer" description?

Break the model? Hmm... for me the "model" (it's perhaps a metamodel really, the underlying model behind the data descriptions) is what it contains in the end, regardless of the input format. There could be more than one way to express it in text and still be the same underlying "model" (in my very broad view). But I do agree that we are also focusing on having primarily one way of expressing (the standard VSS YAML) as far as we can.

Here I am proposing that some data relationships are better expressed in this "Reverse Layer" way (happy to get other name proposals) than the normal way. Instead of "break", we might see it maybe as actually an extension of the model we have, I'm not sure... Consider then if extending could be allowed, since it is under the different name of "VSS layers", compared to the name "VSS"?

(continued)

...I think it is "legal", and "possible", because it is akin to just extending private data (i.e. creating new branches/subtrees) and filling whatever data you like.

I basically don't think of the example as any new branches (or new leaves) at all. The way it is written it is basically defining a new concept (permissions), and then adding leaves (signals) to the new concept. This could alternatively be seen in the standard/forwardd way as adding data to existing leaves (the leaves/signals that are referred to with their full paths).
So the same thing could be transformed into putting new meta-data on existing signals, yes. But the idea is that some mappings are more convenient to define as A->B than with B->A.

I think, just like Ulf's feedback, that the fact that I used a hierarchical names like vss.permissions.whatever here, might be a bit confusing though. It is also possible that we should keep the ability I describe but find another way of explaining it without the "Reverse Layer" name.

(on deletion):

I know we discussed this, but I am not super-sure we need it. After all, an implementation can still give "404" or "40x not implemented" on signals from the "core" spec. I think it would be a more clean design, than letting layers "delete" anything

Consider for example the "fat API" approach where you generate one static function for each item. Then there is no runtime 404, and you probably want to avoid generating for things that should not be there. Even if that's not a common case, I don't want to put in limitations. I think there could be other similar cases. I just think a fundamental model like VSS must very flexible and we should not too much think about the only way it is used right now, today. Also, "downstream" projects (users of VSS) could choose to add more restrictions (for example "deleting is not allowed"), if we start with a flexible definition at first.

(i.e. I would rather provide a deployment/stack specific layer that tags "not-implemented: true" to relevant leaves, and letting the implementation act accordingly)

Sure, that's an alternative approach, with almost exactly the same effect. You're using a different syntax to effectively delete it. Actually we haven't proposed any syntax for deletion yet, so it's all open.

There are more details I think we might get into regarding possible ways to express these things more efficiently. Sorry but not covered in the description yet but I think we may find that wildcards (xxx.*) could be useful in some layer definitions. Let's see what we can come up with.

One key driver, is that while I can see things could be expressed in the standard way I'm concerned not with that a software program is able to read it in that alternative format -- it doesn't care about the format -- but that a human can easily write it.


1. Modify existing metadata and ultimately redefine any signal (i.e. change
its metadata) compared to a previous definition for example the definition
in the standard catalog.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I would add here that you can change existing optional metadata.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Modify/Change. Can we also expand optional metadata?

1. Add new metadata, or relationships to new concepts for the purpose of
creating deployment-models, bindings to existing technology, classification
and many other things.
1. Remove signals from the final data model (SYNTAX TO BE DEFINED)
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think you can realize it with the metadata approach of 1) as you say, but to find a common way how to do it, could be beneficial. Maybe it's just a wording issue, and something like Invalidate unsupported nodes might be more clear than remove.

VSS.

#### Standard Layer

Copy link
Collaborator

Choose a reason for hiding this comment

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

When I read standard layer I would map it to the original vspec. Maybe Extension Layer?

#include Transmission.aperms Drivetrain.Transmission
#include FuelSystem.aperms Drivetrain.FuelSystem
#include ADAS.aperms ADAS
#include Vehicle.aperms Vehicle
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this one intended to be here?

Gunnar Andersson added 3 commits October 12, 2021 17:53
@gunnarx gunnarx force-pushed the vss_layers_generic branch from 477d341 to 8375bbd Compare October 12, 2021 16:00
@gunnarx gunnarx changed the title Example VSS layers for defining access control (WIP, DO NOT MERGE) Example VSS layers for defining access control (Discussion material) Oct 12, 2021
@gunnarx
Copy link
Contributor Author

gunnarx commented Oct 12, 2021

Reverse Layers has been removed from description to simplify the discussion

@gunnarx
Copy link
Contributor Author

gunnarx commented Oct 19, 2021

Latest consensus is being written into this Wiki document:
https://github.com/GENIVI/vehicle_signal_specification/wiki/VSS-Layers-Concept

@gunnarx gunnarx closed this Oct 19, 2021
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.

6 participants