openconfig / public

Repository for publishing OpenConfig models, documentation, and other material for the community.
Apache License 2.0
894 stars 656 forks source link

openconfig-vlan-types is missing definition of canonical form #663

Open mmiklus opened 2 years ago

mmiklus commented 2 years ago

VLAN trunk-vlans are defined as a union of integer (vlan-id) and string (vlan-range).

This type might be good for human to computer interaction (to mimic CLI behavior), unfortunately it is not so good for machine to machine interactions.

The model is missing description of canonical form of the data, thus the actual behavior of trunk-vlans can vary from vendor to vendor.

For example, when you request a device to attach interface to following VLANs:

{
    "openconfig-vlan:trunk-vlans": [
      1,
      2,
      4,
      5,
      6,
      8
    ]
}

MUST the device normalize the data like this:

{
    "openconfig-vlan:trunk-vlans": [
        "1..2",
        "4..6",
        8
    ]
}

or MUST the data (type) remain intact from the original SetRequest ?

dplore commented 2 years ago

My interpretation of the yang specification is either format is permitted and it is the client's responsibility to attempt to validate the data type in the order specified by the union type.

ref: https://datatracker.ietf.org/doc/html/rfc6020#section-9.12

mmiklus commented 2 years ago

@dplore, that's right. The question is a bit different though: When client set VLANS '[1,2,3]' (type=list of integer), is server allowed to normalize the data to a list of strings '["1..3"]' ? Or when client set VLANs [string(1..5), integer(8)], is the server allowed to change the data type to a list of integers [1,2,3,4,5] ?

dplore commented 2 years ago

I think this is a question of the yang specification and not openconfig. However, I'll still offer my opinion! A device can 'change' the data type as you have described.

My interpretation of the yang specification is it allows expression of a union data type as any of the subtypes and it is irrelevant which type was used by a client or server in the past. The server has the freedom to use any and all of the datatypes present in the union and a client must implement all the types as well (or it won't be compliant with the model).

A gNMI server supporting openconfig is expected to serialize data into and out of a its configuration data store. During that serialization I expect data type conversions to occur which result in compliant yang objects. The server must choose one of the datatypes in a union, but it can be any of them.

mmiklus commented 2 years ago

Usually, if server changes data it is because canonical form of the data is defined... here is a couple of examples: https://github.com/openconfig/public/blob/master/third_party/ietf/ietf-yang-types.yang#L330 https://github.com/openconfig/public/blob/master/third_party/ietf/ietf-yang-types.yang#L394 https://github.com/openconfig/public/blob/master/third_party/ietf/ietf-yang-types.yang#L409 https://github.com/openconfig/public/blob/master/third_party/ietf/ietf-yang-types.yang#L441 https://github.com/openconfig/public/blob/master/third_party/ietf/ietf-yang-types.yang#L452

Unfortunately, openconfig model do not define canonical representation of trunk-vlans, thus this issue is issue of the model.

A few of options how to fix this:

  1. describe canonical form in the "trunk-vlans" description - this is the most easiest & backward compatible "fix"
  2. define additional leaf-list "trunk-vlans-list" (or range) with normalized data and decide which type to use integers[] or string[]
  3. define additional "trunk-vlan" container, with 2 leaf-lists - 1. trunk-vlans-ranges (string[]) 2. trunk-vlan-ids(integer[])
dplore commented 2 years ago

Thanks for the conversation. Those examples of canonical forms applied to a single type make sense to me. But here we are dealing with a union datatype.

For option 1, are you requesting that one of the types in the union be declared as canonical? Which implies that one of those types should be used as the 'default' by a server responding to a read request? If so, is there precedent for this in yang? I do not find a reference for it? All I can find is in rfc7950-section-9.12.3 which I interpret as each member type may have a canonical form, but not the union itself.

For option 2 and 3, it sounds like you are suggesting that we add leafs which do not implement the union data type?

Is the root issue a desire to not support the variability required by a union data type? I can see where option 2 and 3 make it unnecessary to support this union and instead allow the client to specifically request the datatype it wants by using the desired leaf.

While there is precedent to have one than one way to get/set data in OC models, I like to avoid it because it negatively impacts vendor consistency/neutrality. This is because vendor A could support leaf 1 and vendor B could support leaf 2. An operator is then obligated to implement different client behavior depending on the vendor implementation.

mmiklus commented 2 years ago

@dplore, thank you for your time to discuss this issue with me.

trunk-vlans are exactly such a precedent. When you take for example a list of VLANs [1..4] , there are many different ways how to represent them and any of following combinations is valid:

[1,2,3,4]
["1..2",3,4]
[1,"2..3",4]
[1,2,"3..4"]
["1..3",4]
[1,"2..4"]
["1..4"]
[1,"1..4"]
[2,"1..4"]

That affects mostly update and delete operations. For example, there are many ways how to delete a VLAN 2 from a range of "1..4": SetRequest(delete([2]) SetRequest(replace [1,2,3,4] with [1,3,4]) SetRequest(replace ["1..4"] with [1,3,4]) SetRequest(replace ["1..2"] with [1]) SetRequest(replace ["2..3"] with [3]) ...

If you want to implement an external datastore (e.g. configuration cache / backup / restore.. ) you can't simply get the data from a device and compare them with your internal datastore by comparing object to object. As you mentioned in previous comment - you have to write a custom 'trunk-vlan' parser, which is not based on the YANG model, but on a semantic normalization. So instead of O(1) comparison, you're somewhere between O(1) - O(n) complexity, most probably wasting CPU cycles with data normalization.

And you're right - all of the proposed options how to fix this ambiguity are a bit radical for a widely-adopted YANG model.

There might be a less-invasive way how to fix the OC-YANG 'vlan-trunks' ambiguity without changing the data model, by making the leaf-list immutable. Basically extend the description with following details: "Server should not transform data between different data types nor change the size of the list. Following transformations are not allowed: ([1,2,3] -> ["1..3"], ["1..2"] -> [1,2])"

Btw. this is IEEE's way of handling VLAN ranges: 'vid-range-type' String gives you less alternatives how to perform UPDATE operation and when you're some config db (e.g. cache) microservice in between the sever and client(s), then you only need to be YANG-aware, you don't need to be YANG-ieee802-dot1q-aware.

mmiklus commented 2 years ago

Based on RFC7950 7.7 .. https://datatracker.ietf.org/doc/html/rfc7950#section-7.7

 Conceptually, the values in the data tree MUST be in the canonical form

it seems like all of the data in data-tree MUST have canonical form defined, which means that the openconfig-vlan-types YANG model is not RFC7950 compliant.

dplore commented 2 years ago

Thanks for finding this. I think the RFC's requirement is a good idea. I will take your proposal to make this model rfc7950 compliant to the OC operator's meeting next week.

github-actions[bot] commented 5 months ago

This issue is stale because it has been open 180 days with no activity. If you wish to keep this issue active, please remove the stale label or add a comment, otherwise will be closed in 14 days.

mmiklus commented 5 months ago

Hi @dplore, any update on this ?