-
Notifications
You must be signed in to change notification settings - Fork 3.4k
bgp auto-discovery default-gateway #39416
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
bgp auto-discovery default-gateway #39416
Conversation
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.
Thanks for the PR! I have some minor requests for changes.
contrib/containerlab/bgpv2/auto-discovery/default-gateway/multi-homing/Makefile
Outdated
Show resolved
Hide resolved
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.
Made an initial review. This time, I focused on the core logic changes and didn't review the lab part much.
Please break this single large commit down into smaller commits.
- k8s resource change
- Core logic change
- service lab
- multi-homing lab
8a3bc26
to
319ae70
Compare
This is fixed. i broke it into smaller commits. |
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.
Made a deeper look at the labs. There are some nits. Overall looks good to me.
Once you addressed the comments, please consolidate the commits to four parts as I mentioned before. We prefer clean history over the incremental change log.
contrib/containerlab/bgpv2/auto-discovery/default-gateway/multi-homing/Makefile
Outdated
Show resolved
Hide resolved
contrib/containerlab/bgpv2/auto-discovery/default-gateway/multi-homing/Makefile
Outdated
Show resolved
Hide resolved
contrib/containerlab/bgpv2/auto-discovery/default-gateway/multi-homing/Makefile
Outdated
Show resolved
Hide resolved
contrib/containerlab/bgpv2/auto-discovery/default-gateway/multi-homing/Makefile
Outdated
Show resolved
Hide resolved
contrib/containerlab/bgpv2/auto-discovery/default-gateway/multi-homing/README.md
Outdated
Show resolved
Hide resolved
contrib/containerlab/bgpv2/auto-discovery/default-gateway/multi-homing/README.md
Outdated
Show resolved
Hide resolved
contrib/containerlab/bgpv2/auto-discovery/default-gateway/service/Makefile
Outdated
Show resolved
Hide resolved
contrib/containerlab/bgpv2/auto-discovery/default-gateway/service/Makefile
Outdated
Show resolved
Hide resolved
contrib/containerlab/bgpv2/auto-discovery/default-gateway/service/Makefile
Outdated
Show resolved
Hide resolved
305dfa8
to
d3ab6c4
Compare
/test |
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.
Now looks good to me. Thanks!
45dbc1b
to
430081c
Compare
Fixes: cilium#37315 (add bgp auto discovery field with default gateway mode) Signed-off-by: Naveen Achyuta <naveen.achyuta22@gmail.com>
Fixes: cilium#37315 (add autodiscovery of bgp peers with default gateway mode) Signed-off-by: Naveen Achyuta <naveen.achyuta22@gmail.com>
Fixes: cilium#37315 (add service lab for testing bgp autodiscovery with default gateway mode) Signed-off-by: Naveen Achyuta <naveen.achyuta22@gmail.com>
Fixes: cilium#37315 (add mutlihoming lab for testing bgp autodiscovery with default gateway mode and update Makefile to run it) Signed-off-by: Naveen Achyuta <naveen.achyuta22@gmail.com>
Fixes: 37315 (update bgpv1 tests with route and device table) Signed-off-by: Naveen Achyuta <naveen.achyuta22@gmail.com>
430081c
to
3c41939
Compare
/test |
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.
LGTM now, thanks for your contribution!
add auto-discovery of bgp peers using default gateway by fetching default routes from statedb
Fixes: #37315
Please ensure your pull request adheres to the following guidelines:
description and a
Fixes: #XXX
line if the commit addresses a particularGitHub issue.
Fixes: <commit-id>
tag, thenplease add the commit author[s] as reviewer[s] to this issue.
This PR adds a new feature called bgp auto-discovery to discover bgp peers dynamically and create bgp sessions with it.
Auto-discovery can use multiple modes to discovery the peers. This change will include default-gateway mode to discovery peers.
Proposal: https://github.com/cilium/design-cfps/blob/main/cilium/CFP-37315-bgp-auto-discovery.md
When a cilium user uses default-gateway mode for auto-discovery for a specific address family, cilium will fetch the routes from route table of statedb and link data from device table of statedb.
It will filter default routes for the given address family and check if the link through which the default route is reachable is UP or not (it will use operational status of the link from the device table for up/down). If its down, we will ignore it.
Finally, it will use the default gateway of the default route with the lowest priority.
What will happen if the default route's attributes like priority or the link status has changed?
cilium will track of route event changes using a separate go-routine. If there is any change in the link or default route, route update will be triggered and we will run bgp reconciliation to find peers again.
Steps for testing this change with multi-homing:-
create kind cluster
sudo make kind-bgpv2-dg-multi-homing
build cilium
sudo KIND_CLUSTER_NAME=bgpv2-cplane-dev-mh make kind-image
Install cilium into the kind cluster
sudo cilium install --chart-directory install/kubernetes/cilium -f contrib/containerlab/bgpv2/auto-discovery/default-gateway/multi-homing/values.yaml --set image.override="localhost:5000/cilium/cilium-dev:local" --set image.pullPolicy=Never --set operator.image.override="localhost:5000/cilium/operator-generic:local" --set operator.image.pullPolicy=Never
apply bgp policy (contrib/containerlab/bgpv2/auto-discovery/default-gateway/bgp.yaml)
sudo make kind-bgpv2-dg-multi-homing-apply-bgp
Fixes: #37315