Please consider updating to new protobuf library

[gibmat]

I'd like to request that you consider updating this program's use of protobuf to utilize the newer version of the library (github.com/protocolbuffers/protobuf-go). My selfish reason for wanting this is that it will allow LXD, which depends on this code, to then eliminate its use of the old protobuf library, which would in turn simplify my packaging of LXD for Debian.

A quick glance through this code shows use of protobuf's ptypes library, which was deprecated within the older version of the library, but has what appears to be easy updates.

[gibmat]

@mikma, I'm not sure where your comment went, although I did get an email notification, so I'll quote it here:

Which protoc version do you want to use? I tried protoc 3.14.0 from Ubuntu 20.04LTS. Some change were needed, but not the ones you mentioned it seems.

Update to protoc 3.14.0

Add go_packages options, required in future protoc versions.
Require grpc v1.32.0, as specified in api/gobgp_grpc.pb.go.
Include api.UnimplementedGobgpApiServer in server struct as required
for forward compatibility.

mikma@c113a6b

I'm not too concerned about the specific version of protoc that's used; Debian has v3.12.4 in unstable. The big changes needed are updating the imported version of the protobuf library itself. I'm not super familiar with Go's protobuf implementations, but basically all imports of github.com/golang/protobuf need to be changed/adjusted to google.golang.org/protobuf, and github.com/golang/protobuf removed from go.mod.

Does that make sense?

[fujita]

This breaks the backward compatibility? So needs to update the major version?

[gibmat]

Honestly, I'm not familiar enough with everything that's involved with the protobuf library transition to be able to answer those questions. I've spent some time trying to understand, but the main takeaway from that for me was a headache. :) I could do a bunch of sed-style updates to use the new library, but I lack the proper insight to understand if the resulting changes would change behavior/break stuff/etc.

[mikma]

This breaks the backward compatibility? So needs to update the major version?

I assume at the very least the Go Native BGP Library is affected. The users will also need to use the new protobuf library, AFAIK.

[aauren]

@fujita Adding a requests here from kube-router as well. We have several libraries with security vulnerabilities that we are unable to update because we need to keep the old version of protobuf that GoBGP uses, as upgrading beyond 1.4.X breaks compatibility with Protobuf v1 which GoBGP uses.

More information about the different versions of Protobuf can be found here: https://go.dev/blog/protobuf-apiv2 (specifically the Versions section towards the bottom of that article).

While I do think that it would be good to change to the v2 API version of Protobuf, this would break backwards compatibility, so I do think that you would need a new major version for this.

As a middle step I believe that you might be able to upgrade to the v1 API at version 1.4.X and achieve forwards and backwards compatibility at least for a time if I read the blog post correctly:

github.com/golang/protobuf@v1.4.0 is a version of APIv1 implemented in terms of APIv2. The API is the same, but the underlying implementation is backed by the new one. This version contains functions to convert between the APIv1 and APIv2 proto.Message interfaces to ease the transition between the two.

Anyway, I can say that the main thing that is blocking us, is that we cannot use anypb instead of the deprecated ptypes functions because the Protobuf objects in GoBGP don't implement the full v2 Protobuf Message interface. Specifically, I believe that they are missing the ProtoReflect() function.

[fujita]

Finished protobuf api v2 migration:
fujita@54eaea8
Please let me know if this works for all.

I'll bump up the major version to 3. I'll also add other changes that break the compatibility. Plans to release few 3.0.0-preX versions.

[aauren]

Thanks @fujita for the quick turn around!

I'll try to test with kube-router sometime this week!

[gibmat]

Thanks @fujita! If you do indeed release a 3.0.0-preX version, I'll build it locally and verify that LXD remains happy (and can drop the old protobuf library itself as well 😄).

[fujita]

I released v3.0.0-rc1. Please try. Update your code to import "v3" like
https://github.com/osrg/gobgp/blob/master/docs/sources/lib.md

[gibmat]

Looks good, at least from a "can I compile?" standpoint. Successfully rebuilt the gobgp Debian package to version 3.0.0-rc1 and then built LXD 4.21 with the few changes required to use v3 of gobgp and update LXD's use of ptypes to anypb. The compiler was happy. I don't really have a way to test LXD's BGP capabilities, but at least this is a big step forward. Thanks!

[fujita]

@gibmat Thanks for the feedback!

[aauren]

@fujita Sorry that it took so long to get back to you on this.

Everything on the Protobuf side looks to be in good order: cloudnativelabs/kube-router#1224

I would say, though that it seems like the WatchEvent() API seems to be a bit slower than MonitorTable() was. I found that after upgrading to 3.X, our BGP unit tests broke. When I looked into it more, I found that after converting to WatchEvent() I had to insert small sleeps between requesting the watch and sending AddPath requests (cloudnativelabs/kube-router@ad38f6d). If I didn't do this, the first AddPath would be sent before the watch was in place and I'd randomly miss the first event.

It's possible that I'm not using the WatchEvent() API correctly here, or that maybe there is a way to check that the watch has been added correctly?

Also, as a general question about the WatchEvent table filters, what is the difference between BEST, ADJIN, and POST_POLICY: https://github.com/osrg/gobgp/blob/master/api/gobgp.pb.go#L362 Is this documented somewhere?

[fujita]

@aauren Thanks a lot for the feedback!

I fixed WatchEvent(). I released rc3 to make it to test easily. I succeeded to run the tests in gobgp_v3.0.0_upgrade branch without sleep hack, but I don't know anything about kube-router so I might overlook something. Please test the branch with rc3 on your env.

Sorry about the lack of documentation. I will add some comments on the photo files. BEST gives that change of best path. ADJIN gives that all the routes that gobgp receives (IOW, you get change of adj-in table). POST_POLICY gives routes after in-policy application.

Any comments on WatchEvent API are appreciated. With the current API, if you don't specify what change of routes (BEST, ADJIN, POST_POLICY) then you don't get any events. But about families, the API gives everything without specifying. Kinda strange to me but I don't have better ideas on API.

[aauren]

Thanks for fixing it in rc3! I can confirm that with the changes you made to rc3 that the tests now work fine for me without the sleep as well.

Also, thanks for the description on the different table filters. That makes sense, I think that BEST is definitely what we were looking for in that list of filters.

In terms of WatchEvent API feedback, in general I would say that what you have is workable.

From a kube-router perspective it would be helpful if I could specify a family filter as well. This would give more 1:1 compatibility with what we had with MonitorTable, however, filtering this ourselves in the callback isn't over burdensome, it would only eliminate ~1 line of code.

I think that it would also be good to default a filter if one isn't given. Probably, most people are interested in BEST route changes, so that would probably be a sensible default. This would allow most users to use something like the following:

server.WatchEvent(context.Background(), &api.WatchEventRequest{
	Table: &api.WatchEventRequest_Table{}
}, tableWatchFunc)

This also makes the syntax very similar to what you get if you add a Peer watch request which doesn't appear to have any filters yet.

The only other piece of feedback I would give is that it would probably be super helpful if, once 3.X became stable, you were to create a doc that talks about the differences between 2.X and 3.X and possible migration examples.

Thanks for your work on GoBGP and your quick turn around on this issue! 🙂