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

bgpv1: ClusterIP advertisement with BGP Control Plane #30963

Merged
merged 1 commit into from Mar 8, 2024
Merged

bgpv1: ClusterIP advertisement with BGP Control Plane #30963

merged 1 commit into from Mar 8, 2024

Conversation

chaunceyjiang
Copy link
Member

@chaunceyjiang chaunceyjiang commented Feb 26, 2024 •
edited by rastislavs

Fixes: #30875

Add support for ClusterIP service advertisement with BGP Control Plane

@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 Feb 26, 2024
@chaunceyjiang chaunceyjiang force-pushed the bgp_svc_advert branch 2 times, most recently from d102537 to 7b65fb4 Compare February 27, 2024 03:19
@borkmann
Copy link
Member

Cc @rastislavs

@chaunceyjiang chaunceyjiang marked this pull request as ready for review March 1, 2024 09:49
@chaunceyjiang chaunceyjiang requested review from a team as code owners March 1, 2024 09:49
@chaunceyjiang
Copy link
Member Author

/test

rastislavs
rastislavs requested changes Mar 1, 2024
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.

Thanks for working on this! I would like to ask you for some additional changes.

pkg/bgpv1/manager/reconciler/service.go Outdated Show resolved Hide resolved
pkg/bgpv1/manager/reconciler/service.go Outdated Show resolved Hide resolved
pkg/bgpv1/manager/reconciler/service.go Show resolved Hide resolved
pkg/bgpv1/manager/reconciler/service_test.go Show resolved Hide resolved
@rastislavs rastislavs changed the title CFP: ClusterIP advertisement with BGP Control Plane bgpv1: ClusterIP advertisement with BGP Control Plane Mar 1, 2024
@rastislavs rastislavs added kind/feature This introduces new functionality. release-note/minor This PR changes functionality that users may find relevant to operating Cilium. area/bgp labels Mar 1, 2024
@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 Mar 1, 2024
YutaroHayakawa
YutaroHayakawa requested changes Mar 4, 2024
View reviewed changes
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 working on this! I did an initial review.

pkg/bgpv1/manager/reconciler/service.go Outdated Show resolved Hide resolved
pkg/bgpv1/manager/reconciler/service.go Outdated Show resolved Hide resolved
pkg/k8s/apis/cilium.io/client/crds/v2alpha1/ciliumbgppeeringpolicies.yaml Outdated Show resolved Hide resolved
@chaunceyjiang chaunceyjiang force-pushed the bgp_svc_advert branch 3 times, most recently from 87ea994 to 9c12558 Compare March 4, 2024 10:22
joamaki
joamaki approved these changes Mar 4, 2024
View reviewed changes
Copy link
Contributor

@joamaki joamaki left a comment

Choose a reason for hiding this comment

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

The k8s/apis/cilium.io/ changes LGTM

@chaunceyjiang chaunceyjiang force-pushed the bgp_svc_advert branch 2 times, most recently from 3e969eb to 88499f4 Compare March 5, 2024 04:26
@chaunceyjiang
Copy link
Member Author

/test

@chaunceyjiang
Copy link
Member Author

/test

YutaroHayakawa
YutaroHayakawa approved these changes Mar 7, 2024
View reviewed changes
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.

Now looks good to me! Thank you very much for working on this!

operator/pkg/bgpv2/bgpp.go Show resolved Hide resolved
@chaunceyjiang
Copy link
Member Author

/test

@harsimran-pabla
Copy link
Contributor

Hi @chaunceyjiang Can you please rebase this branch on top of main. Hitting this issue : https://cilium.slack.com/archives/C7PE7V806/p1709825729364719

@chaunceyjiang
bgpv1: ClusterIP advertisement with BGP Control Plane
18490d3 
Fixes: cilium#30875

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@chaunceyjiang
Copy link
Member Author

Hi @chaunceyjiang Can you please rebase this branch on top of main. Hitting this issue :

Done. @harsimran-pabla

@harsimran-pabla
Copy link
Contributor

/test

@harsimran-pabla
Copy link
Contributor

EKS test failure: #30990

@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 Mar 7, 2024
@joestringer joestringer added this pull request to the merge queue Mar 8, 2024
Merged via the queue into cilium:main with commit 7e4ad4a Mar 8, 2024
61 of 62 checks passed
@chaunceyjiang chaunceyjiang deleted the bgp_svc_advert branch March 8, 2024 01:59
@YutaroHayakawa
Copy link
Member

YutaroHayakawa commented Mar 8, 2024 •
edited

@chaunceyjiang Would you mind working on documenting serviceAdvertisement? It would be a new section under the Advertising Service Virtual IPs section.

https://docs.cilium.io/en/latest/network/bgp-control-plane/#advertising-service-virtual-ips

It would be also nice to have a caveat about using it together with KubeProxyReplacement.

https://docs.cilium.io/en/stable/network/kubernetes/kubeproxy-free/#external-access-to-clusterip-services

@chaunceyjiang
Copy link
Member Author

@YutaroHayakawa Ok.

@YutaroHayakawa YutaroHayakawa mentioned this pull request Mar 11, 2024
Merged
@chaunceyjiang chaunceyjiang mentioned this pull request Mar 12, 2024
Merged
@rastislavs rastislavs mentioned this pull request Mar 14, 2024
Closed
@tsacha tsacha mentioned this pull request Mar 22, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@joamaki joamaki joamaki approved these changes

@YutaroHayakawa YutaroHayakawa YutaroHayakawa approved these changes

@rastislavs rastislavs rastislavs approved these changes

@harsimran-pabla harsimran-pabla harsimran-pabla approved these changes

Assignees
No one assigned
Labels
area/bgp 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.

CFP: ClusterIP advertisement with BGP Control Plane
7 participants
@chaunceyjiang @borkmann @harsimran-pabla @YutaroHayakawa @joamaki @rastislavs @joestringer