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: Local internalTrafficPolicy support for ClusterIP advertisements #31442

Merged
merged 2 commits into from Mar 25, 2024
Merged

bgpv1: Local internalTrafficPolicy support for ClusterIP advertisements #31442

merged 2 commits into from Mar 25, 2024

Conversation

chaunceyjiang
Copy link
Member

@chaunceyjiang chaunceyjiang commented Mar 18, 2024 •
edited by qmonnet

Fixes: #31389

bgpv1: Add Local internalTrafficPolicy support for ClusterIP advertisements

@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 Mar 18, 2024
@chaunceyjiang
Copy link
Member Author

/test

@chaunceyjiang chaunceyjiang marked this pull request as ready for review March 18, 2024 06:00
@chaunceyjiang chaunceyjiang requested a review from a team as a code owner March 18, 2024 06:00
harsimran-pabla
harsimran-pabla requested changes Mar 18, 2024
View reviewed changes
Copy link
Contributor

@harsimran-pabla harsimran-pabla left a comment

Choose a reason for hiding this comment

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

Thanks @chaunceyjiang for taking this up. I don't think change w.r.t calculation of localService endpoints is correct. Please have another look at it.

Also, please add tests with this change.

pkg/bgpv1/manager/reconciler/service.go Show resolved Hide resolved
pkg/bgpv1/manager/reconciler/service.go Outdated Show resolved Hide resolved
@chaunceyjiang
Copy link
Member Author

/test

harsimran-pabla
harsimran-pabla approved these changes Mar 19, 2024
View reviewed changes
Copy link
Contributor

@harsimran-pabla harsimran-pabla left a comment

Choose a reason for hiding this comment

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

Looks good, thank you for this change. I would suggest updating the document as well to clarify usage of iTP vs eTP in various service announements.

@chaunceyjiang chaunceyjiang requested a review from a team as a code owner March 20, 2024 04:20
@chaunceyjiang chaunceyjiang force-pushed the internalTrafficPolicy branch 2 times, most recently from 74c7fe0 to 2096954 Compare March 20, 2024 04:30
@chaunceyjiang
Copy link
Member Author

/test

qmonnet
qmonnet requested changes Mar 20, 2024
View reviewed changes
Copy link
Member

@qmonnet qmonnet 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 this work!

Could you please add the context and motivation for the change to the commit log? It's currently difficult to grasp what the commit is about without opening the related PR, which is not particularly user-friendly when looking at the code locally.

I would also recommend splitting the changes into several commits. You have a doc change that is not strictly related to the changes in the code; variables renaming in the tests; code refactoring in svcDesiredRoutes(); and the addition of the internalTrafficPolicy support in this code. Each of these could be a separate commit with a relevant description, which would make it much easier to review and to understand in the future.

I also have a few style nits, please see inline below.

Documentation/network/bgp-control-plane.rst Outdated Show resolved Hide resolved
Documentation/network/bgp-control-plane.rst Outdated Show resolved Hide resolved
qmonnet
qmonnet requested changes Mar 20, 2024
View reviewed changes
Copy link
Member

@qmonnet qmonnet 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 addressing my feedback on the content 👍.

I stand by my previous comment: splitting the PR into multiple commits remains optional (although recommended), but I really want to see the context and motivation for the change in the commit log before we merge this.

@chaunceyjiang
bgpv1: Local internalTrafficPolicy support for ClusterIP advertisements
e6278be 
Fixes: cilium#31389

As the ClusterIP services can have .spec.internalTrafficPolicy set to Local, advertising a ClusterIP from a node which has no local endpoints may cause unreachability of the advertised service if the traffic is routed to that node.

So if .spec.internalTrafficPolicy of a service is set to Local, advertise its ClusterIP over BGP only if it has non-zero local endpoints.

Signed-off-by: chaunceyjiang <chaunceyjiang@gmail.com>
@chaunceyjiang
bgpv1: Add the documentation for using serviceAdvertisements
eba6b29 
Ideally, the internalTrafficPolicy and externalTrafficPolicy fields are independent. However, since we advertise the ClusterIP to the external world via BGP, it has caused some confusion regarding the meanings of external and internal. Therefore, we need to provide some clarification in the documentation.

For ClusterIP: Only consider the iTP configuration for advertisement, and ignore the eTP configuration.
For ExternalIP and LBIP: Only consider the eTP configuration for advertisement, and ignore the iTP configuration.

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

Hi, @qmonnet I've added some context in the commit log, can you take another look?

qmonnet
qmonnet approved these changes Mar 22, 2024
View reviewed changes
Copy link
Member

@qmonnet qmonnet left a comment

Choose a reason for hiding this comment

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

I like it much better this way, thanks a lot! 🚀

@qmonnet qmonnet added area/bgp release-note/minor This PR changes functionality that users may find relevant to operating Cilium. labels Mar 22, 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 22, 2024
@qmonnet
Copy link
Member

qmonnet commented Mar 22, 2024

/test

@tsacha tsacha mentioned this pull request Mar 22, 2024
Closed
@chaunceyjiang
Copy link
Member Author

/ci-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 Mar 25, 2024
@julianwiedmann julianwiedmann added this pull request to the merge queue Mar 25, 2024
Merged via the queue into cilium:main with commit 33dfaa1 Mar 25, 2024
62 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@qmonnet qmonnet qmonnet approved these changes

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

Assignees
No one assigned
Labels
area/bgp 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: Local internalTrafficPolicy support for ClusterIP advertisements with BGP Control Plane
4 participants
@chaunceyjiang @qmonnet @harsimran-pabla @julianwiedmann