Skip to content

cilium, bpf: Perform 2nd lookup also on local backends for lb-proto-diff #39360

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
May 7, 2025
Merged

cilium, bpf: Perform 2nd lookup also on local backends for lb-proto-diff #39360

merged 1 commit into from
May 7, 2025

Conversation

pasteley
Copy link
Contributor

@pasteley pasteley commented May 6, 2025 •
edited
Loading

When performing load balancer service lookups for internal scope (LB_LOOKUP_SCOPE_INT), ensure protocol differentiation is considered just like it is for external scope lookups.

Fixes: #39358

Fix a bug where services would fail to match wildcard protocols after switching to Local traffic policy with protocol differentiation enabled.

@pasteley pasteley requested a review from a team as a code owner May 6, 2025 10:54
@pasteley pasteley requested a review from ldelossa May 6, 2025 10:54
@maintainer-s-little-helper
Copy link

Commit fad86af does not match "(?m)^Signed-off-by:".

Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin

@maintainer-s-little-helper maintainer-s-little-helper bot added dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 6, 2025
@github-actions github-actions bot added the kind/community-contribution This was a contribution made by a community member. label May 6, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed the dont-merge/needs-sign-off The author needs to add signoff to their commits before merge. label May 6, 2025
@pasteley
cilium, bpf: Perform 2nd lookup also on local backends for lb-proto-diff
2d1a1a1 
When performing load balancer service lookups for internal scope
(LB_LOOKUP_SCOPE_INT), ensure protocol differentiation is considered
just like it is for external scope lookups.

Fixes: cilium#39358
Signed-off-by: pasteley <ceasebeing@gmail.com>
@julianwiedmann julianwiedmann added kind/bug This is a bug in the Cilium logic. area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. release-note/bug This PR fixes an issue in a previous release of Cilium. area/loadbalancing Impacts load-balancing and Kubernetes service implementations needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch labels May 7, 2025
@maintainer-s-little-helper maintainer-s-little-helper bot removed dont-merge/needs-release-note-label The author needs to describe the release impact of these changes. labels May 7, 2025
@julianwiedmann
Copy link
Member

/test

@julianwiedmann
Copy link
Member

Thanks 🚀 ! That looks as expected, let's see how CI feels.

FYI I looked at what's missing re tests and pushed https://github.com/julianwiedmann/cilium/tree/1.18-bpf-proto-diff, which nicely reproduces this bug at the moment. In case you want to validate your fix locally. I can upstream those patches later, not sure whether they would cleanly backport to v1.17.

@julianwiedmann julianwiedmann self-requested a review May 7, 2025 07:04
@julianwiedmann julianwiedmann enabled auto-merge May 7, 2025 09:07
julianwiedmann
julianwiedmann approved these changes May 7, 2025
View reviewed changes
Copy link
Member

@julianwiedmann julianwiedmann left a comment

Choose a reason for hiding this comment

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

Thanks again!

Please take a moment and complete the release-note section in the PR description. This should describe the user-visible impact - so in your case, changing the Traffic Policy to Local for an existing service.

@julianwiedmann julianwiedmann added this pull request to the merge queue May 7, 2025
Merged via the queue into cilium:main with commit 8cd635f May 7, 2025
68 checks passed
@YutaroHayakawa YutaroHayakawa mentioned this pull request May 7, 2025
Merged
1 task
@YutaroHayakawa YutaroHayakawa added backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. and removed needs-backport/1.17 This PR / issue needs backporting to the v1.17 branch labels May 7, 2025
@github-actions github-actions bot removed the backport-pending/1.17 The backport for Cilium 1.17.x for this PR is in progress. label May 8, 2025
@github-actions github-actions bot added the backport-done/1.17 The backport for Cilium 1.17.x for this PR is done. label May 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@julianwiedmann julianwiedmann julianwiedmann approved these changes

@ldelossa ldelossa Awaiting requested review from ldelossa ldelossa is a code owner automatically assigned from cilium/sig-datapath

Assignees
No one assigned
Labels
area/datapath Impacts bpf/ or low-level forwarding details, including map management and monitor messages. area/loadbalancing Impacts load-balancing and Kubernetes service implementations backport-done/1.17 The backport for Cilium 1.17.x for this PR is done. kind/bug This is a bug in the Cilium logic. kind/community-contribution This was a contribution made by a community member. release-note/bug This PR fixes an issue in a previous release of Cilium.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Perform 2nd lookup also on local backends for lb-proto-diff
3 participants
@pasteley @julianwiedmann @YutaroHayakawa