Exa-Networks / exabgp

The BGP swiss army knife of networking
Other
2.06k stars 441 forks source link

Support for BGP-MUP SAFI and Extended Community #1142

Closed takehaya closed 1 year ago

takehaya commented 1 year ago

This PR enables exaBGP to support BGP-MUP SAFI and Extended Community defined in draft-mpmz-bess-mup-safi-02.

SAFI value 85 is assigned by IANA as "BGP-MUP SAFI" https://www.iana.org/assignments/safi-namespace/safi-namespace.xhtml

BGP Transitive Extended Community Types 0x0c is assigned by IANA as "SRv6 MUP Extended Community" https://www.iana.org/assignments/bgp-extended-communities/bgp-extended-communities.xhtml

Usage: https://gist.github.com/takehaya/8170122cb7ff4c710775d02123903e70

thomas-mangin commented 1 year ago

Hello @takehaya - Thank you very much for this patch, it looks pretty good! I will give it a longer review a bit later on.

At first glance, and from previous experience, the main thing I can not change once the code is accepted is the name of the JSON attributes. Right now, I see a mix of spaces, underscores (but no hyphen πŸ‘). I think the user experience would be better with consistent naming and usage of prefixes and suffixes.

Also, I note the use of "parsed: true". I am not sure this is providing much. Was there a good reason for you to add this when checking for anything more than "raw" may achieve the same result?

Once again thank you for this work.

takehaya commented 1 year ago

Hi @thomas-mangin -san Thank you for your quick review! πŸ‘ I confirmed that the json output was strange, so I fixed it. fix json format plz check code πŸ™

thomas-mangin commented 1 year ago

Hi @takehaya-san, arigato gozaimashita.

thomas-mangin commented 1 year ago

I finished the review. All that is needed is a test for the feature, and I can merge! The program used for testing is qa/bin/functional.

You will need to change your configuration example to use 127.0.0.1 as a peer (it is already ibgp so good), define a .ci and .msg file in ./qa/ci - the .ci contains the name of the configuration and the .msg the packet we expect to receive, look at any other file with "raw" line in them.

You can find what is expected by running exabgp with the option -d and then copy the packets generated by ExaBGP (only the UPDATES).

Then run ./qa/bin/functional encoding --list. It will give you a "letter" for the configuration file.

You can then run ./qa/bin/functional encoding <letter> to run the test. If you see an issue, you can run ./qa/bin/functional encoding --server <letter> to run the server and ./qa/bin/functional encoding --client <letter> to run the matching client using your configuration.

That's it. Sorry for not telling you this earlier.

takehaya commented 1 year ago

Oh!! nice japanese! 😲 I am very happy that you spoke to me in JapaneseπŸ‘ Arigato gozaimasu(γ‚γ‚ŠγŒγ¨γ†γ”γ–γ„γΎγ™οΌ)

I got it. Please few a wait that i write by test code...:)

thomas-mangin commented 1 year ago

I was told how to correctly say thank you by an old lady in a Tokyo shop 8 years ago when I visited the country for 10 days. I spend a week in Tokyo and then visited Kyoto, Osaka, the Nara Park and Himeji Castle - everyone was so nice, one of my best holidays :-)

takehaya commented 1 year ago

that "Arigato" is perfect πŸ‘ so nice experience...! Wow. I live in Kyoto :) The cherry blossoms should be beautiful in Kyoto soon🌸(I should see in maybe a week or two... πŸ‘€ )

Actually, I'm planning to bring this code to IETF Hackathon 116 in Japan Yokohama this month. That's why I was doing this.

https://wiki.ietf.org/en/meeting/116/hackathon

BGP-MUP SAFI Implementation and Interop


I tried adding a test. please confirm πŸ™ (and add the case of v6 config)

thomas-mangin commented 1 year ago

Yes, I came at the end of March, and the trees were beautiful.

The code is merged πŸ₯³ 🍾

Thank you for all this work. I was wondering how you found your experience working with the code. Any issues?

thomas-mangin commented 1 year ago

I see that in the log, the routes are presented as "insert mup:isd::100:100:10.0.1.0/24" - do you mind if it is changed to be the same syntax as the annonouce ipv4 section ?

takehaya commented 1 year ago

Please come to Japan again if you have the chance :)

Thank you for your quick and polite review! 🍷

It was pretty well managed and readable code. It was great! (I liked that the documentation was fairly well maintained!) by the way, there were three main problems in doing this work.

  1. the fmt rules for CI lint and make were different. when we applied fmt, we were surprised to see a large number of changes when we applied fmt.
  2. I was worried because I didn't know what kind of syntax to use when adding a new feature.
  3. I'm having trouble finding documentation about test vectors.

I see that in the log, the routes are presented as "insert mup:isd:πŸ’―100:10.0.1.0/24" - do you mind if it is changed to be the same syntax as the annonouce ipv4 section ?

of course !