openconfig / ygot

A YANG-centric Go toolkit - Go/Protobuf Code Generation; Validation; Marshaling/Unmarshaling
Apache License 2.0
286 stars 107 forks source link

proto gen + compress paths = invalid openconfig json? #415

Open michaelhenkel opened 4 years ago

michaelhenkel commented 4 years ago

Hello,

for a grpc service I'd like to use openconfig protos with compressed paths.

Now generating the protos with compressed paths using:

go run ${SRCDIR}/proto_generator/protogenerator.go \
  -generate_fakeroot \
  -base_import_path="github.com/openconfig/ygot/demo/protobuf_getting_started/interface" \
  -path=yang -output_dir=interface \
  -compress_paths=true \
  -add_schemapaths=true \
  -exclude_state=true \
  -enum_package_name=enums -package_name=openconfig \
  -exclude_modules=ietf-interfaces \
  yang/interface/openconfig-interfaces.yang yang/interface/openconfig-if-ip.yang

gives me the following (non-compliant?) json:

interface {
  "interface": [
    {
      "name": "eth0",
      "interface": {
        "description": {
          "value": "Another Interface"
        },
        "enabled": {
          "value": true
        },
        "subinterface": [
          {
            "subinterface": {
              "description": {
                "value": "Another Interface"
              },
              "enabled": {
                "value": true
              },
              "ipv4": {
                "address": [
                  {
                    "ip": "10.0.0.1",
                    "address": {
                      "prefixLength": {
                        "value": "24"
                      }
                    }
                  }
                ]
              },
              "name": {
                "value": ".0"
              }
            }
          }
        ]
      }
    }
  ]
}

Doing the same thing with the go generator with compressed path like:

go run ../../generator/generator.go -path=yang \
  -output_file=pkg/ocdemo/oc.go \
  -package_name=ocdemo -generate_fakeroot \
  -fakeroot_name=device \
  -compress_paths=true \
  -shorten_enum_leaf_names \
  -exclude_modules=ietf-interfaces \
  yang/openconfig-interfaces.yang yang/openconfig-if-ip.yang

the resulting json looks more compliant:

{
  "openconfig-interfaces:interfaces": {
    "interface": [
      {
        "config": {
          "description": "An Interface",
          "mtu": 1500,
          "name": "eth0"
        },
        "name": "eth0",
        "state": {
          "admin-status": "UP"
        }
      },
      {
        "config": {
          "description": "Another Interface",
          "enabled": false,
          "name": "eth1",
          "type": "iana-if-type:ethernetCsmacd"
        },
        "name": "eth1",
        "subinterfaces": {
          "subinterface": [
            {
              "config": {
                "index": 0
              },
              "index": 0,
              "openconfig-if-ip:ipv4": {
                "addresses": {
                  "address": [
                    {
                      "config": {
                        "ip": "10.0.42.1",
                        "prefix-length": 8
                      },
                      "ip": "10.0.42.1"
                    },
                    {
                      "config": {
                        "ip": "192.0.2.1",
                        "prefix-length": 24
                      },
                      "ip": "192.0.2.1"
                    }
                  ]
                }
              }
            }
          ]
        }
      }
    ]
  }
}

The yext.schemapath in the proto have the correct annotations:

string name = 1 [(yext.schemapath) = "/interfaces/interface/config/name|/interfaces/interface/name"];

As I don't have access to the ygot helpers with the generated structs, is there any other way to marshal the resulting struct using the schemapath into a compliant openconfig json? If not, is it expected that a struct generated from a proto with compressed paths is not compliant?

Many Thanks, Michael

wenovus commented 4 years ago

@robshakir can probably give a better answer here, but I will try to answer.

I don't think there is support for compliant JSON marshalling from protos. I'm guessing that you used a built-in helper like protojson.Marshal, but compliance seems only to be provided by Go-generated structs, which is intended to be "the primary way that developers work with a YANG schema in Go"

My guess is you would like to use protos as the serialization format instead of JSON. This need has already been identified, and there is ongoing work to make things convertible from proto to Go, but it's not finished yet.

A related previous discussion: https://github.com/openconfig/reference/issues/110

robshakir commented 4 years ago

@wenovus is spot on here.

There are a few points:

  1. Path compressed structs are suitable as an internal to an implementation format rather than an interchange format. All marshalling that the ygot libraries do will essentially "undo' this schema transformation and create serialised data that is indistinguishable from data that was created with an uncompressed schema representation. This means that any method that is marshalling a compressed representation version of the schema needs to be aware of how do this reverse transformation.
  2. Language representations need not entirely represent the exact same structure as the YANG model - we rather try to be idiomatic in the language that we're creating. For protobuf, the structure of the messages that we create is not something that matches up entirely with the YANG, for example, for any list we create a repeated message field that represents a list - the message defined consists of solely the keys and a single field that stores the list "member' - as such, this introduces another level of hierarchy. A more "YANG-like" representation would be map<type,message> - but this had some usability concerns in some languages, so we rather chose the pattern above.
  3. The default protojson marshalling code is not aware of either of these things - and hence will produce non-RFC7951 compliant JSON when marshalling.

There are two developments that we have in the backlog:

If your implementation is in Go, then the first of these items will likely help -- and I'm happy to help with contributions to this work. If your implementation is in another language, then there's nothing in our backlog for this right now (the ecosystem that we've developed thus far in ygot is Go-centric).

michaelhenkel commented 4 years ago

Thanks @wenovus & @robshakir! Let me give you some background information on what we are working on and trying to achieve: We are writing a go application which uses a graphdb to store the openconfig data. The protos are the source of truth for the data modeling and we want to use them to generate the graphdb schema, ideally without any homegrown generator. What we currently do is to use protoc to generate the go structs and from the go structs we create the graphdb schema. This seems to work quite nicely with the only caveat that in the graphdb the predicates (fields of the structs) must be of the same type globally. Ie. if I define a field 'name' as string in one struct, the field 'name' in other structs has to be a string as well. Likewise a field defined as a slice has to be a slice in other structs as well. Now the bridge to ygot: With uncompressed paths some of the fields are used as different types:

with state & config:

➜  yang_protos grep -ri peer_group_name .

./openconfig/openconfig_network_instance/openconfig_network_instance.proto:                ywrapper.StringValue peer_group_name = 526508656 [(yext.schemapath) = "/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/config/peer-group-name"];

./openconfig/openconfig_network_instance/openconfig_network_instance.proto:                ywrapper.StringValue peer_group_name = 293930651 [(yext.schemapath) = "/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/state/peer-group-name"];

./openconfig/openconfig_network_instance/openconfig_network_instance.proto:              string peer_group_name = 1 [(yext.schemapath) = "/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/peer-group-name"];

./openconfig/openconfig_bgp/openconfig_bgp.proto:        ywrapper.StringValue peer_group_name = 371592209 [(yext.schemapath) = "/bgp/peer-groups/peer-group/config/peer-group-name"];

./openconfig/openconfig_bgp/openconfig_bgp.proto:        ywrapper.StringValue peer_group_name = 69224716 [(yext.schemapath) = "/bgp/peer-groups/peer-group/state/peer-group-name"];

./openconfig/openconfig_bgp/openconfig_bgp.proto:      string peer_group_name = 1 [(yext.schemapath) = "/bgp/peer-groups/peer-group/peer-group-name"];

peer_group_name is of type ywrapper.StringValue AND of type string (ywrapper.StringValue peer_group_name && string peer_group_name

Using compressed paths it looks like that:

➜  openconfig grep -ri peer_group_name .

./openconfig.proto:    string peer_group_name = 1 [(yext.schemapath) = "/bgp/peer-groups/peer-group/config/peer-group-name|/bgp/peer-groups/peer-group/peer-group-name"];

./openconfig.proto:        string peer_group_name = 1 [(yext.schemapath) = "/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/config/peer-group-name|/network-instances/network-instance/protocols/protocol/bgp/peer-groups/peer-group/peer-group-name"];

peer_group_name is only of type string which satisfies the graphdb requirement of predicates (ie. peer_group_name) being of the same type. Hence the hope that we can use a compressed paths representation as the schema. @robshakir

An implementation mapping ygen-produced protobufs to <path, value> constructs.

Will that allow to unmarshal a compressed, from proto derived struct into an umcompressed GoStruct?

Thanks again!