Re: [Rift] KV draft published

[Tony Przygienda <[e-mail]>]

Hey Bruno

1. agree with the binary vs string, I read some thrift spec that said
string=binary and string should be preferred that's why we ended like this
2. thrift has no extension concept, we would need to rev the model

value can be encoded model, nothing prevents it

On Fri, Jan 15, 2021 at 10:02 AM Bruno Rijsman <[e-mail]>
wrote:

> Some comments:
>
> *Comment #1*. In draft-ietf-rift-rift-12 both the key type (KeyIDType)
> and the key value (keyvalues) are defined as a Thrift string type. In the
> Thrift spec (https://thrift.apache.org/docs/types) a string is defined as
> "A text string encoded using UTF-8 encoding". We cannot encode arbitrary
> sequences of bytes into a UTF-8 encoded string. All sorts of libraries will
> throw exceptions / return errors when the string is not a valid UTF-8
> encoded string. At the very least we have to change it from a Thrift string
> type to a Thrift binary type (but we should also consider something more
> sophisticated - see #2, #4 and #5 below).
>
> *Comment #2*. It would be a very nice validation of the model-based
> approach if the key-values could be modeled using Thrift. It kind of feels
> like a let-down that we are reverting back to old fashioned ASCII
> byte-layout diagrams. More specific proposals below in #4 and #5.
>
> *Comment #3*. Do we need to introduce a new capability to announce that
> an implementation understands this particular format of key-values?
> Something along the lines of:
>
> struct NodeCapabilities {
>   ...
>   4: optional bool rfcxxxx_key_values;   /* Means we understand the key
> types and values as defined in separate RFC xxxx)
> }
>
> *Comment #4*. Comments #2 and #3 bring up the following question: how do
> we extend the Thrift data model (e.g. introduce new fields and/or structs)
> without having to (a) update the base spec or (b) repeat the entire RIFT
> data model, add something, and bump the version in the spec that defines
> the new thing? Google protobuf2 provides an "extensions" mechanism for
> exactly this purpose (
> https://developers.google.com/protocol-buffers/docs/proto#extensions).
> Similarly, both SNMP (ASN.1) and YANG have "augment" statements. What is
> the mechanism in Thrift for one data model (spec) to augment another data
> model (spec)?
>
> I imagine augmenting the RIFT data model would look something similar to
> the following. Here I am using protobuf2 syntax because I am not aware of
> what the corresponding mechanism in Thrift is. (Maybe I got the protobuf2
> details wrong; it's the idea that counts.)
>
> /*------- RIFT base spec -------*/
>
> message KeyType {
>   extensions 100 to max;
> }
>
> typedef bytes KeyValue;
>
> message NodeCapabilities {
>   required uint32 minor_version = 1;
>   optional bool flood_reduction = 2 [default = false];
>   /* ... other capabilities defined in base spec ... */
>   extensions 100 to max;
> }
>
> /*------- Key-value spec -------*/
>
> extend basespec.KeyType {
>   optional StandardKeyType = 100;
>   optional VendorKeyType = 101;
> }
>
> message StandardKeyType {
>   required bytes standard_assigned_type = 1;
> }
>
> message VendorKeyType {
>   required bytes vendor_oiu = 1;   /* Must be exactly 3 bytes *.
>   required bytes vendor_assigned_type = 2;
> }
>
> extend basespec.KeyValue {
>   optional OUIScopedKeyValue = 100;
> }
>
>
> extend basespec.NodeCapabilities {
>   optional oui_scoped_key_values = 100 [default = false];
> }
>
> PS: Protobuf3 dropped support for extensions and uses the Any type
> instead, which is interesting.
>
> *Comment #5*: In the proposal described in comment #4 the key values are
> still opaque byte strings (same as in the current base spec). The key-value
> internal structure is still not described in the data model. If you follow
> the above line of reasoning to its logical conclusion, you wouldn't need
> seperate key-ids and key-values. You could just have an monilithic
> "expansion" field, along the following lines (using protobuf2 syntax, once
> again):
>
> PS: I carefully picked the term "expansion" to avoid confusion with the
> "extension" GPB keyword.
>
> /*------- Base spec -------*/
>
> message ExpansionTIEElement {
>   repeated optional ExpansionType expansions = 1;
> }
>
> message ExpansionType {
>   /* The format of expansions is defined in other specifications */
>   extensions 100 to max;
> }
>
> /*------- Spec for vendor-defined expansions -------*/
>
> extend basespec.ExpansionType {
>   repeated optional VendorExpansion vendor_expansions = 100;
> }
>
> message VendorExpansion {
>   required bytes vendor_oiu = 1;   /* Must be exactly 3 bytes *.
>   required bytes vendor_assigned_type = 2;
>   optional bytes value = 3;
> }
>
> extend basespec.NodeCapabilities {
>   optional vendor_expansion = 100 [default = false];
> }
>
> /*------- Hypothetical standard spec to flood host names -------*/
>
> extend basespec.ExpansionType {
>   optional HostNameExpansion host_name_expansion = 101;
> }
>
> message HostNameExpansion {
>   required string host_name = 1;
> }
>
> extend basespec.NodeCapabilities {
>   optional host_name_expansion = 101 [default = false];
> }
>
> /*------- Hypothetical standard spec to flood GPS coordinates -------*/
>
> extend basespec.ExpansionType {
>   optional GPSCoordinateExpansion gps_coordinate_expansion = 102;
> }
>
> message GPSCoordinateExpansion {
>   required float longitude = 1;
>   required float lattitude = 2;
>   optional string datum = 3;
> }
>
> extend basespec.NodeCapabilities {
>   optional gps_coordinate_expansion = 102 [default = false];
> }
>
>
>
> — Bruno
>
> On Dec 14, 2020, at 7:59 AM, Tony Przygienda <[e-mail]> wrote:
>
> Co-authorship and discussions around use cases/codepoints encouraged
>
> https://datatracker.ietf.org/doc/draft-przygienda-rift-kv-registry/
>
> _______________________________________________
> RIFT mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/rift
>
>
>