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

api: Define protobuf structures of BGP NLRI and Path Attributes #1729

Merged
merged 3 commits into from
Jun 13, 2018

Conversation

iwaseyusuke
Copy link
Contributor

Currently, the gRPC client need to serialize the NLRI and Path Attributes into bytes representation when calling AddPath gRPC API, but this requires the client side applications have the implementation to encode/decode the BGP attributes. Then it can be difficult to implement gRPC client with the languages other than Go.

This PR defines the protobuf structures of BGP NLRI and Path Attributes and enable to call AddPath API with these structures.

For the discussion about this improvements and sample code of gRPC client written in Python, see #1457.

@fujita
Copy link
Member

fujita commented Jun 8, 2018

Great, can you add some tests of grpc with this new API? I'm thinking about using gogo/protobuf but concern of Any type a bit.

@cstockton
Copy link

I think this change would be great. One thing to consider would be taking this opportunity to remove the table importing of api. This is because it forces anyone who wants to use the api to download all of the packages the sever requires, which is a lot of dependencies. If more functions like ToPathApiInBin are added to the api package now it will make migrating away more difficult in the future. The change itself would be fairly simple, remove ToNativePath and add table.PathToAPI() table.APIToPath() (or whatever names are preferred). Then in the future other dependencies could slowly be peeled away, which I would be happy to help with if the GoBGP authors would accept the change.

"gobgp/lib" can be independent from "gobgpapi" package, this patch
removes the usage of utility function "ToPathApi()" in this package.

Signed-off-by: IWASE Yusuke <iwase.yusuke0@gmail.com>
Example of protoc command:
$ export PROTOBUF=${HOME}/protobuf/src
$ export GOBGP=${GOPATH}/src/github.com/osrg/gobgp
$ protoc \
    -I ${PROTOBUF} \
    -I ${GOBGP}/api \
    --go_out=plugins=grpc:${GOBGP}/api \
    ${GOBGP}/api/gobgp.proto \
    ${GOBGP}/api/attribute.proto

Signed-off-by: IWASE Yusuke <iwase.yusuke0@gmail.com>
@fujita
Copy link
Member

fujita commented Jun 13, 2018

I guess that we are on the same page. Once I push this pr, I want to make api/ include only code to access to the gRPC APIs and handy functions to handle the protobuf structures. That should remove lots of dependencies. I want to move grpc_server.go and util.go to somewhere else. Then I want to remove client/ and rewrite gobgp/*.go to use only api/.

@iwaseyusuke iwaseyusuke force-pushed the api-Define_protobuf_of_PathAttribute branch from 810d543 to 0ca90f7 Compare June 13, 2018 01:52
Signed-off-by: IWASE Yusuke <iwase.yusuke0@gmail.com>
@iwaseyusuke iwaseyusuke force-pushed the api-Define_protobuf_of_PathAttribute branch from 0ca90f7 to 417f8ba Compare June 13, 2018 01:54
@iwaseyusuke
Copy link
Contributor Author

I've updated to move api/bgp/attribute* into api/ to build protobuf by single protoc command. (Also reworded a commit tile, sorry for disturbing)

@fujita fujita merged commit 417f8ba into osrg:master Jun 13, 2018
@fujita
Copy link
Member

fujita commented Jun 13, 2018

Thanks a lot, pushed. I think that it's better to remove the usage of logrus and just return an error. I'll take care of such minor issues.

@iwaseyusuke iwaseyusuke deleted the api-Define_protobuf_of_PathAttribute branch June 13, 2018 05:03
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

Successfully merging this pull request may close these issues.

3 participants