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

Support for BGP-MUP SAFI and Extended Community #1142

Merged
merged 3 commits into from Mar 14, 2023

Conversation

takehaya
Copy link
Contributor

@takehaya takehaya commented Mar 13, 2023

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
Copy link
Member

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 takehaya force-pushed the feature/support_srv6_mup branch 2 times, most recently from 93023ed to 97e4039 Compare March 14, 2023 08:37
@takehaya
Copy link
Contributor Author

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
Copy link
Member

Hi @takehaya-san, arigato gozaimashita.

@thomas-mangin
Copy link
Member

thomas-mangin commented Mar 14, 2023

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
Copy link
Contributor Author

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
Copy link
Member

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
Copy link
Contributor Author

takehaya commented Mar 14, 2023

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 thomas-mangin merged commit 1fc8b5e into Exa-Networks:main Mar 14, 2023
4 checks passed
@thomas-mangin
Copy link
Member

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
Copy link
Member

thomas-mangin commented Mar 14, 2023

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
Copy link
Contributor Author

takehaya commented Mar 14, 2023

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 !

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.

None yet

2 participants