-
Notifications
You must be signed in to change notification settings - Fork 208
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
Enable kube-proxy replacement if no kube-proxy is detected in a cluster #1039
Conversation
Hi Team, |
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.
Thanks. Some comment re the kube-apiserver endpoint addr.
Commit 4bc875b does not contain "Signed-off-by". Please follow instructions provided in https://docs.cilium.io/en/stable/contributing/development/contributing_guide/#developer-s-certificate-of-origin |
Previously, Cilium-CLI won’t enable kube-proxy by default automatically if no kube-proxy has been installed in the cluster. This patch will detect kube-proxy installation in the cluster automatically and it’ll set the helm values kubeProxyReplacement=strict, k8sServiceHost, and k8sServicePort if there were no kube-proxy. Fixes: cilium#1004 Signed-off-by: shankeerthan-kasilingam <shankeerthan1995@gmail.com>
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.
Thank you @ksankeerth!
} | ||
|
||
for _, ds := range dsList.Items { | ||
if strings.Contains(ds.Name, "kube-proxy") { |
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.
maybe we should use if ds.Nmae == "kube-proxy"
instead of strings.Contains
?
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.
@cmssczy thanks for the suggestion. Yes, the Kube-proxy auto-detection may take a different DaemonSet as Kube-Proxy if they also contain "kube-proxy". Just for curiosity, have you found such cases like that? @brb wdyt?
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.
I've only seen "kube-proxy", so the suggested change makes sense to me.
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.
Thanks @brb. If you think it's to better to change now, I can create a git Issue for that since we already merged this branch? (I can fix that too)
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.
I think it faster to create a PR than an issue, so I'd go with a fix. Thanks.
Previously, Cilium-CLI won’t enable kube-proxy by default automatically if no kube-proxy has been installed in the cluster. This patch will detect kube-proxy installation in the cluster automatically and it’ll set the helm value kubeProxyReplacement=strict if there were no kube-proxy.
Fixes: #1004