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

bgpd: Implement Link-Local Next Hop capability #17871

Merged

Conversation

ton31337
Copy link
Member

@ton31337 ton31337 commented Jan 17, 2025

Implementing https://datatracker.ietf.org/doc/html/draft-white-linklocal-capability.

=> frrbot skipped intentional according to our conventions.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
…er-peer

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
@frrbot frrbot bot added bgp documentation tests Topotests, make check, etc labels Jan 17, 2025
@ton31337 ton31337 force-pushed the feature/bgp_link_local_capability branch from 0d5556e to d69fa2f Compare January 17, 2025 11:52
@donaldsharp
Copy link
Member

I've read the proposed draft and I'm not sure I am happy with the error handling aspect of it and how it works with older versions ( it's not specific enough imo )
I've started a conversation with Jeff/Russ and Donatas in slack to get to the bottom.

@ton31337 ton31337 force-pushed the feature/bgp_link_local_capability branch from d69fa2f to ffc0a8e Compare January 17, 2025 14:36
Related: https://datatracker.ietf.org/doc/html/draft-white-linklocal-capability

TL;DR; use 16 bytes long next-hops for point-to-point (unnumbered) links instead
of sending 32 bytes (::/LL, GUA/LL, LL/LL combinations).

For backward compatiblity we should handle even 32 bytes existing next hops.

Signed-off-by: Donatas Abraitis <donatas@opensourcerouting.org>
@ton31337 ton31337 force-pushed the feature/bgp_link_local_capability branch from ffc0a8e to db853cc Compare January 17, 2025 14:49
@donaldsharp
Copy link
Member

interface based peering is already supposed to do this. Why not just use that for this feature? As I understand it if we peer with a older version of FRR then it would still work. In other words why do we need a new knob when we have one already?

@ton31337
Copy link
Member Author

interface based peering is already supposed to do this. Why not just use that for this feature? As I understand it if we peer with a older version of FRR then it would still work. In other words why do we need a new knob when we have one already?

you mean "neighbor X interface ..."?

@donaldsharp
Copy link
Member

yes exactly. neighbor swp1 interface ....

@ton31337
Copy link
Member Author

ton31337 commented Jan 17, 2025

That would be probably fine. But I'm thinking what if I want to drop this capability (for some backward compatible SNAFU), but keep the session UP?

@ton31337
Copy link
Member Author

@riw777 @jefftant opinions?

@jefftant
Copy link

jefftant commented Jan 21, 2025 via email

@ton31337
Copy link
Member Author

Yes, this is how it's handled now. The question is #17871 (comment).

@ton31337 ton31337 requested a review from riw777 January 23, 2025 13:45
@riw777
Copy link
Member

riw777 commented Feb 4, 2025

Yes, this is how it's handled now. The question is #17871 (comment).

So the question is whether this should be:

neighbor PEER capability link-local

or:

neighbor PEER interface capability link-local

?

Since we'd never use this for multihop, I don't know that it makes much difference? OTOH, I don't know that I'd go looking for this as an interface specific command ... ??

@ton31337
Copy link
Member Author

ton31337 commented Feb 4, 2025

The question was if we need this additional knob at all or we can rely on neighbor PEER interface .... But with neighbor PEER interface ... is a problem that we can't disable LL capability and with a new knob - we could.

@ton31337 ton31337 closed this Feb 4, 2025
@ton31337 ton31337 reopened this Feb 4, 2025
@github-actions github-actions bot added the rebase PR needs rebase label Feb 4, 2025
Copy link
Member

@riw777 riw777 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

looks good

@riw777 riw777 merged commit 2ef76a3 into FRRouting:master Feb 7, 2025
20 checks passed
@ton31337 ton31337 deleted the feature/bgp_link_local_capability branch February 8, 2025 11:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bgp documentation master rebase PR needs rebase size/L tests Topotests, make check, etc
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants