-
Notifications
You must be signed in to change notification settings - Fork 446
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
Implement RFC6514 MCAST-VPN (incomplete) #1234
Conversation
https://datatracker.ietf.org/doc/html/rfc6514 This commit starts support for MCAST-VPN NLRI. Route Types added: + 5 - Source Active A-D route + 6 - Shared Tree Join route + 7 - Source Tree Join route
class SharedJoin(MVPN): | ||
CODE = 6 | ||
NAME = "C-Multicast Shared Tree Join route" | ||
SHORT_NAME = "Shared-Join" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This name may be used when the route is printed and may need to be lower case?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You mean NAME, SHORT_NAME or both? I tried to follow example from other nlri code (like mup and evpn), but didn't notice a pattern regarding lower-case use.
Notice SHORT_NAME is already lower-cased here when printing by _prefix():
exabgp/src/exabgp/bgp/message/update/nlri/mvpn/sharedjoin.py
Lines 51 to 52 in 279a289
def __str__(self): | |
return f'{self._prefix()}:{self.rd._str()}:{str(self.source_as)}:{str(self.source)}:{str(self.group)}' |
exabgp/src/exabgp/bgp/message/update/nlri/mvpn/nlri.py
Lines 60 to 61 in 279a289
def _prefix(self): | |
return "mvpn:%s:" % (self.registered_mvpn.get(self.CODE, self).SHORT_NAME.lower()) |
This is the print output you mean right?
reactor connected to peer-1 with outgoing-1 10.99.12.6-10.99.12.5
processes ipv4 added to neighbor 10.99.12.5 local-ip 10.99.12.6 local-as 65000 peer-as 65000 router-id 32.32.32.32 family-allowed in-open : mvpn:shared-join::65000:99999:65000:10.99.199.1:239.251.255.228 extended-community target:192.168.94.12:5
processes ipv4 added to neighbor 10.99.12.5 local-ip 10.99.12.6 local-as 65000 peer-as 65000 router-id 32.32.32.32 family-allowed in-open : mvpn:source-join::65000:99999:65000:10.99.12.2:239.251.255.228 extended-community target:192.168.94.12:5
processes ipv6 added to neighbor 10.99.12.5 local-ip 10.99.12.6 local-as 65000 peer-as 65000 router-id 32.32.32.32 family-allowed in-open : mvpn:shared-join::65000:99999:65000:fd00::1:ff0e::1 extended-community target:192.168.94.12:5
processes ipv6 added to neighbor 10.99.12.5 local-ip 10.99.12.6 local-as 65000 peer-as 65000 router-id 32.32.32.32 family-allowed in-open : mvpn:source-join::65000:99999:65000:fd12::2:ff0e::1 extended-community target:192.168.94.12:5
processes ipv6 added to neighbor 10.99.12.5 local-ip 10.99.12.6 local-as 65000 peer-as 65000 router-id 32.32.32.32 family-allowed in-open : mvpn:sourcead::65000:99999:fd12::4:ff0e::1 extended-community target:65000:99999
processes ipv4 added to neighbor 10.99.12.5 local-ip 10.99.12.6 local-as 65000 peer-as 65000 router-id 32.32.32.32 family-allowed in-open : mvpn:sourcead::65000:99999:10.99.12.4:239.251.255.228 extended-community target:65000:99999
def __ne__(self, other): | ||
return not self.__eq__(other) | ||
|
||
def __str__(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this represent the route in a way which could be parsed by the configuration code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't get this one. can you share an example?
Hello @jcpvdm - at first glance, it is an excellent commit. Thank you very much. I have two observations/questions above, which I would appreciate if you could consider. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From the readability perspective, I'd rather use a normal (full) name multicast-vpn
, instead of mcast-vpn
.
You mean replace all instances of "mcast-vpn" by "multicast-vpn" in the code? |
At least in configuration mode :) (seeing as an operator). |
I will need to write some tests and may have to perform a non-backwards compatible change to have what I just asked. I will think about it as I have not enforced this consistently. Merging. Happy to accept follow up fixes. |
Thank you very much for this work. It is much appreciated. Thank you, @ton31337, too, for the feedback. |
https://datatracker.ietf.org/doc/html/rfc6514
This commit starts support for MCAST-VPN NLRI. Route Types added:
This came out of my need to perform tests for some MCAST-VPN route types. exabgp fit nicely for the task :)
Note there was some reformatting done on unrelated code by black.