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

bgpv1: Clear announcement vars after server recreation #24304

Conversation

dylandreimerink
Copy link
Member

We maintain a cache/shadow map for all advertisements we have made. During reconciliation we check the desired state against this shadow state which lives in the PodCIDRAnnouncements and ServiceAnnouncements variables.

This works until the preflightReconciler decides to recreate the server, which happens if the router-id or the listen port changes. At this point the GoBGP route server is recreated which also clears the tables. But we forget to clear our shadow state as well. This causes any existing routes to not be added since the reconcilers think they are already there.

Fixes: #24069

Fix bug in BGP CP where changing the route-id of an existing router would cause announcements to disappear

We maintain a cache/shadow map for all advertisements we have made.
During reconciliation we check the desired state against this shadow
state which lives in the `PodCIDRAnnouncements` and
`ServiceAnnouncements` variables.

This works until the `preflightReconciler` decides to recreate the
server, which happens if the router-id or the listen port changes.
At this point the GoBGP route server is recreated which also clears
the tables. But we forget to clear our shadow state as well. This
causes any existing routes to not be added since the reconcilers think
they are already there.

Fixes: cilium#24069

Signed-off-by: Dylan Reimerink <dylan.reimerink@isovalent.com>
@dylandreimerink dylandreimerink added release-note/bug This PR fixes an issue in a previous release of Cilium. area/bgp needs-backport/1.12 labels Mar 10, 2023
@dylandreimerink dylandreimerink requested a review from a team as a code owner March 10, 2023 17:47
@dylandreimerink dylandreimerink added the kind/bug This is a bug in the Cilium logic. label Mar 10, 2023
Copy link
Member

@YutaroHayakawa YutaroHayakawa left a comment

Choose a reason for hiding this comment

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

Thanks for finding out this 🙏 LGTM!

@YutaroHayakawa
Copy link
Member

We don't need to run full e2e tests for this change. This change is trivial and e2e tests don't use BGP CPlane for the tests.

@YutaroHayakawa YutaroHayakawa added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Mar 13, 2023
@borkmann borkmann merged commit 175b2f1 into cilium:master Mar 13, 2023
@NikAleksandrov NikAleksandrov mentioned this pull request Mar 20, 2023
9 tasks
@NikAleksandrov NikAleksandrov mentioned this pull request Mar 23, 2023
29 tasks
@youngnick youngnick added backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. and removed backport-pending/1.12 labels Mar 23, 2023
@jibi jibi added backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. and removed backport-pending/1.13 labels Apr 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/bgp backport-done/1.12 The backport for Cilium 1.12.x for this PR is done. backport-done/1.13 The backport for Cilium 1.13.x for this PR is done. kind/bug This is a bug in the Cilium logic. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
No open projects
Status: Released
Development

Successfully merging this pull request may close these issues.

BGP PodCIDRs are not advertised after Cilium restart
6 participants