Support native sidecar by default #56428
Conversation
|
|
|
😊 Welcome @irenezhong2861! This is either your first contribution to the Istio istio repo, or it's been You can learn more about the Istio working groups, Code of Conduct, and contribution guidelines Thanks for contributing! Courtesy of your friendly welcome wagon. |
|
Hi @irenezhong2861. Thanks for your PR. I'm waiting for a istio member to verify that this patch is reasonable to test. If it is, they should reply with Once the patch is verified, the new status will be reflected by the I understand the commands that are listed here. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
|
/ok-to-test |
8d311f3 to
08c79ad
Compare
aa7d148 to
d39664b
Compare
manifests/charts/istio-control/istio-discovery/files/injection-template.yaml
Outdated
Show resolved
Hide resolved
| {{ $nativeSidecar := (or (and (not (isset .ObjectMeta.Annotations `sidecar.istio.io/nativeSidecar`)) (eq (env "ENABLE_NATIVE_SIDECARS" "false") "true")) (eq (index .ObjectMeta.Annotations `sidecar.istio.io/nativeSidecar`) "true")) }} | ||
| {{ $nativeSidecar := .NativeSidecars }} | ||
| {{ if (isset .ObjectMeta.Annotations `sidecar.istio.io/nativeSidecar`) }} | ||
| {{ $nativeSidecar = eq (index .ObjectMeta.Annotations `sidecar.istio.io/nativeSidecar`) "true" }} |
There was a problem hiding this comment.
Why is this duplicated in the injection template logic?
There was a problem hiding this comment.
Cleaned up this line. The injection-template is ran here, with params.nativeSidecar being passed in and read in injection-template.yaml via .NativeSidecars. The is to generate the template pod. Later on, the same annotation logic is used to generate the merged pod. Both places need code to overwrite the param.nativeSidecar value with the annotation value.
| Name: "node-1", | ||
| }, | ||
| Status: corev1.NodeStatus{ | ||
| NodeInfo: corev1.NodeSystemInfo{ |
There was a problem hiding this comment.
Can we get more of a range of kubelet versions?
There was a problem hiding this comment.
What about old kubelet versions? We should probably test that they have classic sidecars right?
There was a problem hiding this comment.
added test case below with old version and added golden files. The new test case has one 1.33 node and one 1.28 node so native sidecar will be off without needing to set any flags.
| log.Infof("Resetting the UserID of sideCar proxy as it matches with the app container for Pod %q", params.pod.Name) | ||
| sideCarProxy.SecurityContext.RunAsUser = nil | ||
| sideCarProxy.SecurityContext.RunAsGroup = nil | ||
| } | ||
| } | ||
| } | ||
|
|
||
| var nodes kclient.Client[*corev1.Node] | ||
| if wh.nodes != nil { |
There was a problem hiding this comment.
Should probably log or something if this isn't initialized for some reason
pkg/kube/inject/webhook.go
Outdated
|
|
||
| minVersion := 29 | ||
| allNodesValid := false | ||
| for _, n := range nodes.List(metav1.NamespaceAll, klabels.Everything()) { |
There was a problem hiding this comment.
What happens if someone adds an older node to their k8s cluster? Talk through the process in a code comment.
There was a problem hiding this comment.
add comment around how older nodes will switch the native sidecar value to false since this checks versions of all nodes.
There was a problem hiding this comment.
this needs to take into account Kubernetes version of wait until the minimum supported version is GA. this would be about 3 years so a long wait.
If you look through the issue you'll see I had a WIP PR to do so that we could leverage.
edit: I skimmed quickly on my phone and I think I missed some logic, so I may be wrong - will look closer soon. Sorry!
pkg/kube/inject/webhook.go
Outdated
| return false | ||
| } | ||
|
|
||
| minVersion := 29 |
There was a problem hiding this comment.
should do 1.33 imo. we cannot GA a beta feature
There was a problem hiding this comment.
+1 to this - I missed it in my initial pass. There is also a bug in older versions that doesn't really affect Istio, but still shouldn't expose users to it by default if we can help it IMO
There was a problem hiding this comment.
in my initial PR I had a way for a user to say "automatically detect, minimum beta" as an opt-in to 1.29. but for default should be 1.33 IMO
There was a problem hiding this comment.
if the pod has a node name assigned explicitly (very rare) we can also probably check only that node. maybe niche though
There was a problem hiding this comment.
Updated the version to 1.33, with a branch to check if only the corresponding node if pod.Spec.nodeName is not empty. Currently have a log.warn if the the node name is set but the node can't be found, and not enabling native sidecar in that case, open to suggestions!
There was a problem hiding this comment.
With this kubelet version check though, I don't think we need to wait until 1.33 is the min supported k8s version. It handles the case of nodes on ineligible versions by not defaulting native side cars for them. We might not need a check like this at all once 1.33 is the min version and just enable all by default.
There was a problem hiding this comment.
Yes, once we get past the supported 1.33 version skew, we could remove a lot of these checks, though folks who are running LTS in a cloud provider need to keep them around
| Name: "node-1", | ||
| }, | ||
| Status: corev1.NodeStatus{ | ||
| NodeInfo: corev1.NodeSystemInfo{ |
There was a problem hiding this comment.
What about old kubelet versions? We should probably test that they have classic sidecars right?
b3cc938 to
74b4cce
Compare
|
/test integ-ambient-mc |
* remove finding of pods by IP * fix test * add feature flag and release note
e4fc569 to
553567d
Compare
There was a problem hiding this comment.
I am sorry for this late comment, but i am starting to question whether this is a sane default... the thing I worry about is a user having 1000 nodes and requiring native sidecars, in prod, all good.
Now someone adds some random node pool completely unrelated to istio, where Istio pods cannot even run, and now cluster wide we make a radical change to their (new) pods...
The alternative seems to be to wait ~3 years for this to rollout to everyone which is pretty bad too.
At minimum I would like that users to be able to explicitly say YES to native sidecars. Before we had yes/no, now we have no/auto. We MUST have an explicit YES option
| nodes = wh.nodes.ForCluster(cluster.ID(clusterID)) | ||
| params.nativeSidecar = DetectNativeSidecar(nodes, pod.Spec.NodeName) | ||
| } else { | ||
| // only enable native sidecars if the feature is explicitly enabled |
There was a problem hiding this comment.
IIRC this will warn on each injection request a confusing message for users that disable native sidecars
| @@ -797,6 +801,16 @@ func IntoObject(injector Injector, sidecarTemplate Templates, valuesConfig Value | |||
| warningHandler(warningStr) | |||
| return out, nil | |||
| } | |||
|
|
|||
| var nativeSidecar bool | |||
| if injector != nil && injector.GetKubeClient() != nil { | |||
There was a problem hiding this comment.
This is pretty sketchy since at least for injection we know the Pod is being created at that time. For kube-inject it is checked into CI.
But not a big deal since kube-inject is not very common
|
LGTM aside from that comment though |
So we'd have yes/auto/no as the choices, and
and if the feature is unset, default to |
Yep. I am still not 100% convinced its safe to ever use "auto" (and 'yes' is definitely not able to be default for years) but it may be worth it to do so.. I do also worry users will see the opposite issue when they upgrade their first k8s node |
|
/test integ-cni |
|
PR needs rebase. Instructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
Please provide a description of this PR:
Enables native sidecar by default