Repository navigation
lbipam: allow sharing IPs across services that select different pods - #45819
Conversation
maintainer-s-little-helper
Bot
added
the
dont-merge/needs-release-note-label
github-actions
Bot
added
the
kind/community-contribution
julianwiedmann
added
release-note/minor
maintainer-s-little-helper
Bot
removed
the
dont-merge/needs-release-note-label
julianwiedmann
added
the
kind/enhancement
dylandreimerink
left a comment
dylandreimerink
left a comment
There was a problem hiding this comment.
Two small nits from me, looks good to me otherwise.
YutaroHayakawa
left a comment
YutaroHayakawa
left a comment
There was a problem hiding this comment.
sig-k8s-owned changes looks good to me.
aanm
added
dont-merge/bad-bot
|
Switching to draft while Dylan's review is addressed. |
ec1cce1 to
6a0b83d
Compare
|
Apologies for the long delay here — this one slipped through the cracks on my side. Both of Dylan's nits are now addressed: the verbose in-code comment is trimmed to a one-liner pointing at the annotation, and the "both"→"all" wording is fixed in the docs and aligned in the commit message and PR description. The PR is out of draft again and ready for another look. One note: the latest commit shows up as Unverified, which is expected here — I couldn't produce a valid GPG signature locally this time due to disk space constraints on my end, so the DCO Signed-off-by trailer is present but the GPG signature isn't. |
|
/test |
Head branch was pushed to by a user without write access
6a0b83d to
4f6b7fc
Compare
|
I've marked this PR as draft. When you have addressed the comments and workflow test results, bring the PR back to the attention of reviewers by scrolling to the bottom of the page and clicking the "Ready for Review" button. Thanks! |
efc2caa to
1e1ad2f
Compare
|
Rebased onto main and applied the wording suggestions. Two more changes on top. The io.cilium/lb-ipam-sharing-permit-different-pods alias is gone: the io.cilium aliases only exist as legacy names for annotations that predate the lbipam.cilium.io prefix, a new annotation has nothing to alias. The upgrade-notes entry is gone too, it described no behaviour change. |
Add an opt-in annotation lbipam.cilium.io/sharing-permit-different-pods that, when set on all sharing services, lets LB IPAM share an IP between services with externalTrafficPolicy=Local that select different sets of pods. The annotation must be present on every service with the same sharing key for the relaxed check to take effect. The default same-selector requirement protects users from a misrouted shared IP, since Cilium's built-in BGP and L2 announcers are not L4 aware. Some deployments steer traffic with mechanisms outside Cilium's view (an L4-aware external load balancer using each service's healthCheckNodePort, or DaemonSets with matching node coverage), and for those the same-selector check is too strict. Documentation explains the trade-off and warns that combining the opt-in with Cilium's built-in announcers can drop traffic. Fixes: cilium#38766 Signed-off-by: Aleksei Sviridkin <[email protected]>
1e1ad2f to
d1c53f7
Compare
ajmmm
left a comment
ajmmm
left a comment
There was a problem hiding this comment.
Thanks, the docs read a lot better to me!
/cc @hemanthmalla
|
/test |
joestringer
added
the
ail:3
maintainer-s-little-helper
Bot
added
the
ready-to-merge
joestringer
added
release-note/major
|
How does this impact the threat model? If we consider that an attacker has access to one namespace, could they disrupt services in other namespaces by adding these annotations onto Pods they control? cc @cilium/sig-lb |
joestringer
added
dont-merge/discussion
I think we are good. This feature works on top of the normal IP sharing. To share an IP across namespaces, all services in a sharing group must explicitly permit the namespaces of the other services in the sharing group. You have to explicitly open yourself up to IP sharing, a bad actor can't just modify annotations and steal arbitrary IPs without also having control over the other service. With regard to this feature, this part is important
So all services in a sharing group have to opt-in to this feature as well, one service can't change the behavior on its own. |
|
I see yep. Seems like this extends further the existing framework documented here, so falls under the same threat model. 👍 Then I think this is ready to merge. Thanks! |
joestringer
removed
the
dont-merge/discussion
maintainer-s-little-helper
Bot
added
the
ready-to-merge
Description of change
LB IPAM rejected services with
externalTrafficPolicy: Localfrom sharing an IP unless they selected the same set of pods. This default is correct for users relying on Cilium's built-in BGP or L2 announcers, which are not L4 aware: announcing the shared IP from a node that lacks a backend for one of the services would drop traffic for that service.Some deployments steer traffic with mechanisms outside Cilium's view — most notably an external L4-aware load balancer that uses each service's
healthCheckNodePortto route only to nodes with a backend, or DaemonSets with matching node coverage so every node hosts every backend. For these the same-selector check is too strict.This PR adds an opt-in annotation
lbipam.cilium.io/sharing-permit-different-pods. When set totrueon all services in a sharing group, LB IPAM relaxes the same-selector requirement. The opt-in is symmetric (every service in the group must opt in), and it only relaxes the same-selector branch — services without a selector (Endpoints-based) are still rejected because LB IPAM cannot read endpoints to determine which nodes host backends.The annotation value is parsed via
strconv.ParseBool. Unparseable values default to disabled and emit a warning so users can debug typos.The need for this opt-in was discussed in the linked CFP, where the maintainer initially preferred a smarter compatibility check but acknowledged that for use cases involving an external L4-aware load balancer there is no reliable way to validate the setup automatically, leaving an explicit opt-in as the practical path.
AI disclosure
This PR was prepared with AIL:3. I personally reviewed each line of the submission prior to opening this PR.
Fixes: #38766