Skip to content

BGP CP: Adds Support for Multi-Pool IPAM #27100

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
Merged
merged 1 commit into from
Sep 19, 2023
Merged

BGP CP: Adds Support for Multi-Pool IPAM #27100

merged 1 commit into from
Sep 19, 2023

Conversation

danehans
Copy link
Contributor

@danehans danehans commented Jul 26, 2023 •
edited
Loading

Adds Support for Multi-Pool IPAM:

  • Adds PodIPPoolSelector to CiliumBGPVirtualRouter for defining a label selector for advertising routes from allocated IPAM pools.
  • Adds LocalCiliumNodeResource to the BGP CP Controller to provide a stream of events for changes to the local CiliumNode resource.
  • Adds PodIPPoolAnnouncements to ServerWithConfig for holding all the announced multi-pool IPAM CIDRs.
  • Adds PodIPPoolReconciler to advertise IPAM pool CIDRs of a matching CiliumBGPPeeringPolicy.
  • Updates BGP CP unit and integration tests.

Fixes: #26937

The podIPPoolSelector field has been added to CiliumBGPVirtualRouter for selectively advertising multi-pool IPAM CIDRs.

@danehans danehans requested review from a team as code owners July 26, 2023 19:39
@danehans danehans requested review from youngnick and rastislavs July 26, 2023 19:39
@maintainer-s-little-helper maintainer-s-little-helper bot added the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Jul 26, 2023
rastislavs
rastislavs reviewed Jul 27, 2023
View reviewed changes
@rastislavs rastislavs added area/bgp Impacts the Border Gateway Protocol feature. kind/feature This introduces new functionality. labels Jul 27, 2023
@rastislavs
Copy link
Contributor

Thanks a lot for this contribution! Could you please also take a look at the pkg/bgpv1/test/adverts_test.go? It contains Test_PodCIDRAdvert "component test" that perhaps could be extended (or a similar one can be added) to cover multi-pool IPAM. It does not have to be done in this PR though, we can just open an issue to track it.

@danehans
Copy link
Contributor Author

@rastislavs thanks for the review. I'll update the PR with a component test that covers MP IPAM.

@rastislavs rastislavs added the release-note/minor This PR changes functionality that users may find relevant to operating Cilium. label Aug 2, 2023
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. label Aug 2, 2023
@danehans
Copy link
Contributor Author

danehans commented Aug 3, 2023

@rastislavs integration tests have been added, PTAL.

@rastislavs
Copy link
Contributor

@rastislavs integration tests have been added, PTAL.

Thanks a lot @danehans - looks good to me now! However, we are planing for one more refactor of the BGP CP reconcilers that should land in the next few days, could we ask you to hold on a bit more with this PR and do one more rebase once it is in? I will ping you - and link to the refactoring PR here once it is open. (CC @ldelossa @YutaroHayakawa - FYI)

@danehans
Copy link
Contributor Author

danehans commented Aug 4, 2023

@rastislavs no worries. I noticed the introduction of the Path type in my last rebase. Ping me when you're ready to have me rebase this PR. Feel free to also cc me on your BGP refactor PRs.

@rastislavs
Copy link
Contributor

@danehans This is the PR are we are waiting for here: #27285
On top of that we may have one more follow-up in the next days.

@danehans danehans mentioned this pull request Aug 10, 2023
Merged
@danehans danehans requested review from a team as code owners August 16, 2023 18:09
danehans
danehans commented Aug 16, 2023
View reviewed changes
@danehans danehans force-pushed the issue_26937 branch 3 times, most recently from 6ca15ef to a38e60f Compare August 16, 2023 22:29
@danehans
Copy link
Contributor Author

@christarazi @rastislavs This PR is passing CI and approved by all other reviewers. PTAL when you have a moment.

@bimmlerd bimmlerd removed their request for review September 14, 2023 07:53
rastislavs
rastislavs reviewed Sep 14, 2023
View reviewed changes
rastislavs
rastislavs reviewed Sep 14, 2023
View reviewed changes
@danehans danehans force-pushed the issue_26937 branch 2 times, most recently from 334a81a to 572b94b Compare September 14, 2023 17:20
@danehans
Copy link
Contributor Author

danehans commented Sep 14, 2023 •
edited
Loading

CI run failed due to Installation and Conformance Test flake.

@danehans
Copy link
Contributor Author

/test

@danehans
Copy link
Contributor Author

@rastislavs thanks for the review and guidance. Commit 572b94b resolves your review feedback, PTAL.

@danehans danehans requested a review from rastislavs September 14, 2023 18:41
rastislavs
rastislavs approved these changes Sep 14, 2023
View reviewed changes
Copy link
Contributor

@rastislavs rastislavs left a comment

Choose a reason for hiding this comment

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

LGTM now, thanks!

christarazi
christarazi approved these changes Sep 14, 2023
View reviewed changes
Copy link
Member

@christarazi christarazi left a comment

Choose a reason for hiding this comment

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

LGTM for my codeowners

bimmlerd
bimmlerd approved these changes Sep 15, 2023
View reviewed changes
Copy link
Member

@bimmlerd bimmlerd left a comment

Choose a reason for hiding this comment

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

Since maintainers-little-helper insists 🙄
LGTM

@danehans danehans mentioned this pull request Sep 15, 2023
Closed
@danehans
Copy link
Contributor Author

I think we need to have a larger discussion on how these selectors behave differently across resources and wider compared to K8s.

@christarazi I created #28177 to track this issue. I plan to address this in the next community meeting.

@danehans
Copy link
Contributor Author

/ci-clustermesh

@danehans
BGP CP: Adds Support for Multi-Pool IPAM
1471ac1 
- Adds `PodIPPoolSelector` to `CiliumBGPVirtualRouter` for defining
  a label selector for advertising routes from allocated IPAM pools.
- Adds `LocalCiliumNodeResource ` to the BGP CP `Controller` to provide
  a stream of events for changes to the local CiliumNode resource.
- Adds `PodIPPoolAnnouncements` to `ServerWithConfig` for holding all
  the announced multi-pool IPAM CIDRs.
- Adds `PodIPPoolReconciler` to advertise IPAM pool CIDRs of a matching
  CiliumBGPPeeringPolicy.
- Updates BGP CP unit and integration tests.

Signed-off-by: Daneyon Hansen <daneyon.hansen@solo.io>
@danehans
Copy link
Contributor Author

/test

@danehans
Copy link
Contributor Author

/ci-ipsec-e2e

@maintainer-s-little-helper maintainer-s-little-helper bot added the ready-to-merge This PR has passed all tests and received consensus from code owners to merge. label Sep 18, 2023
@danehans
Copy link
Contributor Author

@christarazi @rastislavs @gandro @YutaroHayakawa all reviews are approved and all CI tests are passing. Is anything else needed to merge?

@YutaroHayakawa
Copy link
Member

@danehans You have to do nothing. This PR is already marked as ready-to-merge by the bot. Congrats! And thanks again for your great contribution!

@gandro gandro merged commit 660f2f2 into cilium:main Sep 19, 2023
@danehans danehans deleted the issue_26937 branch September 19, 2023 17:35
This was referenced Sep 20, 2023
Closed
Closed
Closed
@log1cb0mb log1cb0mb mentioned this pull request Sep 30, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gandro gandro gandro left review comments

@ldelossa ldelossa ldelossa left review comments

@YutaroHayakawa YutaroHayakawa YutaroHayakawa approved these changes

@nathanjsweet nathanjsweet nathanjsweet approved these changes

@christarazi christarazi christarazi approved these changes

@bimmlerd bimmlerd bimmlerd approved these changes

@youngnick youngnick youngnick approved these changes

@rastislavs rastislavs rastislavs approved these changes

Assignees
No one assigned
Labels
area/bgp Impacts the Border Gateway Protocol feature. dont-merge/needs-rebase This PR needs to be rebased because it has merge conflicts. kind/feature This introduces new functionality. ready-to-merge This PR has passed all tests and received consensus from code owners to merge. release-note/minor This PR changes functionality that users may find relevant to operating Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

BGP CP: Add Support for Multi-Pool IPAM
9 participants
@danehans @rastislavs @YutaroHayakawa @gandro @christarazi @nathanjsweet @ldelossa @bimmlerd @youngnick