Re: [Rift] KV draft published

[Bruno Rijsman <[e-mail]>]

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/ <https://datatracker.ietf.org/doc/draft-przygienda-rift-kv-registry/>
> _______________________________________________
> RIFT mailing list
> [e-mail]
> https://www.ietf.org/mailman/listinfo/rift