-
Notifications
You must be signed in to change notification settings - Fork 746
apiutil: Add Family argument to NewPath function #3070
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
base: master
Are you sure you want to change the base?
Conversation
Add Family argument to NewPath function in order to avoid getting Family from NLRI. The NLRI type and Family do not have a one-to-one correspondence, we can only obtain an incomplete value for Family. Signed-off-by: FUJITA Tomonori <[email protected]>
@@ -175,7 +175,7 @@ func NewPath(nlri bgp.AddrPrefixInterface, isWithdraw bool, attrs []bgp.PathAttr | |||
Pattrs: a, | |||
Age: tspb.New(age), | |||
IsWithdraw: isWithdraw, | |||
Family: ToApiFamily(nlri.AFI(), nlri.SAFI()), |
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.
in which case getting the family from the nlri/prefix is not the right way ?
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.
RF_IPv4_MC and RF_IPv6_MC.
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.
why not doing it in the prefix itself ? something like
func NewIPAddrPrefix(bits uint8, prefix string) *IPAddrPrefix
p := netip.ParsePrefix("....")
return &IPAddrPrefix{ isMulticast: p.Addr().IsMulticast() , .... }
func (ip4 *IPAddrPrefix) SAFI() {
if ip4.isMulticast {
return RF_IPv4_MC
}
return RF_IPv4_UC
}
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.
We can. The point to discuss is where to store the relationship between Family and NLRI.
Since the number of AddrPrefixInterfaces increases proportionally to both the number of routes and the number of peers, in a full route environment with 10 peers, if NLRI increases by 4 bytes, memory usage would increase by 40MB. Storing Family information in NLRI does not seem ideal.
Note that Paths in the same Destination should share the NLRI. We need to remove PathID info (that is, PrefixDefault) from NLRI before that.
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 think we can do this way #3081
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.
Overall I think the prefix contain the family and need to parse/handle it correctly
via netip.Parse() prefix.IsMulticast() IsUnicast(), may Is4In6() ....
Add Family argument to NewPath function in order to avoid getting Family from NLRI. The NLRI type and Family do not have a one-to-one correspondence, we can only obtain an incomplete value for Family.