[go: up one dir, main page]
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.

Sign up for GitHub

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

Jump to bottom

Spike on supporting LLAs #2510

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from
Draft

Spike on supporting LLAs #2510

wants to merge 2 commits into from

Conversation

karampok
Copy link
Contributor
@karampok karampok commented Sep 3, 2024 •
edited
Loading

/kind feature

What this PR does / why we need it:
Fix for #2423

Special notes for your reviewer:

AFAIU there is not need for any API change to support that. We could allow the string fe80::42:acff:fe12:6%eth0 to be properly parsed and if valid (=LLA and %eth0 part which is called zone exists) to handle it (which is if Zone exist then add the extra FRR directive neighbor fe80::a876:4dff:fe77:408 interface eth0

Under the hood:

  1. Moving from net.IP to the netip.Addr helps greatly, and looks a nice improvement
  2. E2E are also possible by finding the LLAs of the interface and use that instead of the global ipv address

Release note:

-->

Support for LLA (LinkLocalAddress) ipv6 addresses (e.g. `fe80::42:acff:fe12:6%eth0` )

@karampok
Use netip.Addr and add zone handling for LLA
5044160 
Signed-off-by: karampok <karampok@gmail.com>
@karampok
E2E: add test for LLA addresses
4157f3f 
"eth0" is hardcoded zone value in the config

Signed-off-by: karampok <karampok@gmail.com>
@karampok
Copy link
Contributor Author
karampok commented Sep 4, 2024

@fedepaol @oribon Please have a fast look here if you do like the direction and if yes then I can further work to undraft it and make it ready for the code review. Thank you

oribon
oribon reviewed Sep 9, 2024
View reviewed changes
Copy link
Member
@oribon oribon left a comment

Choose a reason for hiding this comment

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

a general thought: imo it would be cleaner to just have addr + interface in the api (like proposed in the unnumbered pr) instead of supporting "IPv6 with a scoped addressing zone" in the peer's address - the main reasons being:

  • it seems for frr that format is just an alias for specifying both address and interface
  • when we do have an interface field it might be confusing (do I specify the interface as part of the address or in the interface field).
    wdyt?

@karampok
Copy link
Contributor Author

After some conversation, we will proceed with the proposal from @oribon:
We will have the following API change

type Neighbor struct {
  Address string `json:"address"` //
  Interface string `json:"interface"` 
// 1. if address and no interface as normal
// 2. if address is LLA and interface then LLA support=extra directive `neighbor fe80::a876:4dff:fe77:408 interface eth0`  
// 3. If address is LLA and empty interface, validation is failing
// 4. If address is empty and interface has value then the BGP unnumber peering config will take place= `neighbor PEER interface remote-as <internal|external|auto|ASN> && other && enable RAs on this interface`                                         
}

instead of allowing user to enter fe80::1234%eth0 in the address field and no API change.

That will be reviewed again after the BGP unumber design/implementation

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@oribon oribon oribon left review comments

@fedepaol fedepaol Awaiting requested review from fedepaol fedepaol will be requested when the pull request is marked ready for review fedepaol is a code owner

@russellb russellb Awaiting requested review from russellb russellb will be requested when the pull request is marked ready for review russellb is a code owner

@gclawes gclawes Awaiting requested review from gclawes gclawes will be requested when the pull request is marked ready for review gclawes is a code owner

Assignees
No one assigned
Labels
kind/feature
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@karampok @oribon