-
Notifications
You must be signed in to change notification settings - Fork 2.7k
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.
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
daemon: Add option --bpf-lb-external-clusterip #15650
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -325,29 +325,9 @@ bool lb6_svc_is_affinity(const struct lb6_service *svc) | |
return svc->flags & SVC_FLAG_AFFINITY; | ||
} | ||
|
||
static __always_inline | ||
__u8 svc_is_routable_mask(void) | ||
{ | ||
__u8 mask = SVC_FLAG_ROUTABLE; | ||
|
||
#ifdef ENABLE_LOADBALANCER | ||
mask |= SVC_FLAG_LOADBALANCER; | ||
#endif | ||
#ifdef ENABLE_NODEPORT | ||
mask |= SVC_FLAG_NODEPORT; | ||
#endif | ||
#ifdef ENABLE_EXTERNAL_IP | ||
mask |= SVC_FLAG_EXTERNAL_IP; | ||
#endif | ||
#ifdef ENABLE_HOSTPORT | ||
mask |= SVC_FLAG_HOSTPORT; | ||
#endif | ||
return mask; | ||
} | ||
|
||
static __always_inline bool __lb_svc_is_routable(__u8 flags) | ||
{ | ||
return (flags & svc_is_routable_mask()) > SVC_FLAG_ROUTABLE; | ||
return (flags & SVC_FLAG_ROUTABLE) != 0; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Is there anything that could break with this change? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thinking loud about the upgrade path. When cilium-agent starts, services are updated from kube-apiserver before the datapath gets regenerated. So, it means that for some time, the old code will be running with the new service flags. I think this should be fine, as for previously routable services nothing should change. During the downgrade, we will have the old service flags (= pre your changes) with the new datapath (= with your changes) for awhile. This will allow ClusterIP access from outside. But I guess this is tolerable. |
||
} | ||
|
||
static __always_inline | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Shouldn't this be guarded by
EnableClusterIPExternalAccess
config?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not sure what you mean by guarded. The flag is disabled by default in which case
SVC_FLAG_ROUTABLE
is unset for ClusterIP services. EarlierSVC_FLAG_ROUTABLE
was set for ClusterIP services, but higher bits were unset and hence this function returned false forClusterIP
services. Now we only need to check forSVC_FLAG_ROUTABLE
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Speaking of which, I can't find where, in your PR,
SVC_FLAG_ROUTABLE
would be set or unset depending on the value forconfig.ExternalClusterIP
. Shouldn't there be a change to the definition ofSVC_FLAG_ROUTABLE
in bpf/lib/common.h?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change to this logic is in the agent in
updateMasterService
: https://github.com/cilium/cilium/pull/15650/files#diff-8eff0d99dd1ceb7d15ce632811672e7b17a17fb5e984fafa7875d6ad2433b3d8R519. It was already set earlier and in this PR I'm turning it off for ClusterIP services unless the new external access flag is set.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok thank you. On my first read, I somehow understood that you would be changing the value for
SVC_FLAG_ROUTABLE
depending on the value ofExternalClusterIP
, but I understand this is not the case - You're just changing theflags
.