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

Use of Any vs OneOf in API protobuff file #2866

Open
crankynetman opened this issue Jan 8, 2025 · 0 comments
Open

Use of Any vs OneOf in API protobuff file #2866

crankynetman opened this issue Jan 8, 2025 · 0 comments

Comments

@crankynetman
Copy link

Hello!

When using the API protobuf and gRPC, I've noticed that the protobuf definition file makes heavy use of the google.protobuf.Any type and then there are comments in the protobuf file saying which type you need to pack to actually send across the wire. To me, this seems like a perfect use case for the Oneof message type, but full disclosure, I'm still very inexperienced with protobuf and auto-generated clients. Is this intended behavior and a conscious choice, and if so, is there any information on why this path was chosen? One of the side-effects of using the Any type seems to be that you can't merge or copy objects into an existing object, at least with python generated code. For example, when creating a Path message to send in an AddPathRequest for the AddPath RPC, you have to generate the objects and pack them, then assemble the final Path message. If you don't pack and just use the object itself, you'll get an error. For example:

path_object = gobgp_pb2.Path(
    nlri=attribute_pb2.IPAddressPrefix(
                prefix_len=32,
                prefix="10.0.0.6",
            ),
)
Traceback (most recent call last):
  File "<string>", line 1, in <module>
TypeError: Parameter to MergeFrom() must be instance of same class: expected google.protobuf.Any got apipb.IPAddressPrefix.

Instead, as shown in the demo documentation, you have to create an empty Any object and then pack that with the appropriate GoBGP object, like so:

nlri = Any()
nlri.Pack(
    attribute_pb2.IPAddressPrefix(
                prefix_len=32,
                prefix="10.0.0.6",
            )
)
path_object = gobgp_pb2.Path(nlri=nlri)

This may not seem like a big deal (and it's not a huge issue) but it seems to ignore a lot of the reasons for using protobufs (specifically around type safety) and it makes it a bit of a pain to modify an object at any point when you're trying to construct payloads.

To me, it seems like converting the API protobuf from the current state of:

message Path {
  // One of the following defined in "api/attribute.proto":
  // - IPAddressPrefix
  // - LabeledIPAddressPrefix
  // - EncapsulationNLRI
  // - VPLSNLRI
  // - EVPNEthernetAutoDiscoveryRoute
  // - EVPNMACIPAdvertisementRoute
  // - EVPNInclusiveMulticastEthernetTagRoute
  // - EVPNEthernetSegmentRoute
  // - EVPNIPPrefixRoute
  // - EVPNIPMSIRoute
  // - LabeledVPNIPAddressPrefix
  // - RouteTargetMembershipNLRI
  // - FlowSpecNLRI
  // - VPNFlowSpecNLRI
  // - OpaqueNLRI
  // - LsAddrPrefix
  // - SRPolicyNLRI
  // - MUPInterworkSegmentDiscoveryRoute
  // - MUPDirectSegmentDiscoveryRoute
  // - MUPType1SessionTransformedRoute
  // - MUPType2SessionTransformedRoute
  google.protobuf.Any nlri = 1;
}

Into something like:

message Path {
  oneof nlri {
    IPAddressPrefix ipprefix = 1;
    LabeledIPAddressPrefix labeledipprefix = 2;
    ...
    MUPType2SessionTransformedRoute muptype2sessiontransformedroute = 100;
  }
}

Would be beneficial, as then we could create complicated paths and use the MergeFrom and CopyFrom functions directly.

Thanks for reading and please let me know if you have any questions (or if I have completely misunderstood how all of this works!)

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

No branches or pull requests

1 participant