istio
istio copied to clipboard
istio
Documentation unclear about sidecar customization (could be bug in injector)
Is this the right place to submit this?
- [X] This is not a security vulnerability or a crashing bug
- [X] This is not a question about how to use Istio
Bug Description
- As you know there are annotations to control the pods memory and cpu requests and limits. We use them
- For the specific purpose of optionally setting up a prestop hook for the proxy, we also embed using the methodology discussed in https://istio.io/latest/docs/setup/additional-setup/sidecar-injection/#customizing-injection
{{- define "mesh-container" }}
{{- if .Values.mesh.enabled -}}
{{- if .Values.mesh.prestop.enabled -}}
- name: istio-proxy
image: auto
lifecycle:
preStop:
exec:
command: [ "{{ join "\",\"" .Values.mesh.prestop.commands }}" ]
{{- end }}
{{- end }}
{{- end }}
- That works great
- Except then we found effectively that memory and cpu annotations are ignored. My guess is the template takes precedence and sees no clause so just uses default values?
Version
istio 1.20.2
Additional Information
i have two concerns
- This isn't very clear in the docs. There's comments on the same page like: "Additionally, certain fields are configurable by annotations on the pod, although it is recommended to use the above approach to customizing settings. Additional care must be taken for certain annotations:
If sidecar.istio.io/proxyCPU is set, make sure to explicitly set sidecar.istio.io/proxyCPULimit. Otherwise the sidecar’s cpu limit will be set as unlimited. If sidecar.istio.io/proxyMemory is set, make sure to explicitly set sidecar.istio.io/proxyMemoryLimit. Otherwise the sidecar’s memory limit will be set as unlimited."
but it doesn't mention they won't even WORK in this case.
- creating a complete alternative template would be pretty hard to do and preserve function. In fact arguably it would be a deployment nightmare.
Given the above, is this
- A bug in istio injector not merging the two correctly (there's no conflict per se)
- A bug in the docs, and the only option is to drop the prestop or try to copy everything into a new template, which is rarely feasible.
I'd be happy to try to submit clearer docs, but it's not obvious to me the current behavior is as designed.
Hey, sorry for the (very long) delay here. I don't really get what you tried.
Where is the define mesh-container specified? Where is mesh-container used?
How are referencing the custom template?
We have a unit test around this I tried to reproduce in, but I couldn't grok how to set it up:
diff --git a/pkg/kube/inject/testdata/inject/custom-template.iop.yaml b/pkg/kube/inject/testdata/inject/custom-template.iop.yaml
diff --git a/pkg/kube/inject/testdata/inject/custom-template.iop.yaml b/pkg/kube/inject/testdata/inject/custom-template.iop.yaml
index f68cf9accb..efb2586a3a 100644
--- a/pkg/kube/inject/testdata/inject/custom-template.iop.yaml
+++ b/pkg/kube/inject/testdata/inject/custom-template.iop.yaml
@@ -8,30 +8,15 @@ spec:
sidecarInjectorWebhook:
templates:
custom: |
- metadata:
- annotations:
- # Disable the built-in transformations. In the future we may want a template-level API
- prometheus.istio.io/merge-metrics: "false"
- sidecar.istio.io/rewriteAppHTTPProbers: "false"
- foo: bar
- spec:
- containers:
- {{- range $index, $container := .Spec.Containers }}
- - name: {{ $container.Name }}
- env:
- - name: SOME_ENV
- value: "true"
- - name: SOME_FILE
- value: /var/lib/data/foo.json
- volumeMounts:
- - mountPath: /var/lib/data/foo.json
- subPath: foo.json
- name: some-injected-file
- {{- end}}
- volumes:
- - name: some-injected-file
- downwardAPI:
- items:
- - path: foo.json
- fieldRef:
- fieldPath: "metadata.annotations['foo']"
+ {{- define "mesh-container" }}
+ {{- if .Values.mesh.enabled -}}
+ {{- if .Values.mesh.prestop.enabled -}}
+ - name: istio-proxy
+ image: auto
+ lifecycle:
+ preStop:
+ exec:
+ command: [ "{{ join "\",\"" .Values.mesh.prestop.commands }}" ]
+ {{- end }}
+ {{- end }}
+ {{- end }}
\ No newline at end of file
diff --git a/pkg/kube/inject/testdata/inject/custom-template.yaml b/pkg/kube/inject/testdata/inject/custom-template.yaml
index 0d1914bacf..5bfbb54e3b 100644
--- a/pkg/kube/inject/testdata/inject/custom-template.yaml
+++ b/pkg/kube/inject/testdata/inject/custom-template.yaml
@@ -9,7 +9,8 @@ spec:
template:
metadata:
annotations:
- inject.istio.io/templates: "custom"
+ inject.istio.io/templates: "sidecar,custom"
+ sidecar.istio.io/proxyCPU: "123m"
labels:
app: hello
spec:
is what I had, if it helps
The "sidecar.istio.io/proxyCPU is set, make sure to explicitly set sidecar.istio.io/proxyCPULimit" part makes sense and I agree could use some improved docs BTW
- We have a chart to deploy our applications.
- It always specifies proxy cpu/memory requests and limits. Similar to
sidecar.istio.io/proxyMemory: {{ .memory | quote }}
sidecar.istio.io/proxyMemoryLimit: {{ .memory | quote }}
sidecar.istio.io/proxyCPU: {{ .cpu | quote }}
sidecar.istio.io/proxyCPULimit: {{ .cpu | quote }}
- In addition if mesh.prestop,enabled is set by the user, this part is injected
{{- define "mesh-container" }}
{{- if .Values.mesh.enabled -}}
{{- if .Values.mesh.prestop.enabled -}}
- name: istio-proxy
image: auto
lifecycle:
preStop:
exec:
command: [ "{{ join "\",\"" .Values.mesh.prestop.commands }}" ]
{{- end }}
{{- end }}
{{- end }}
This follows the advice in https://istio.io/latest/docs/setup/additional-setup/sidecar-injection/#customizing-injection
(I'm open to other methods, the desired outcome is if some triggers exist in the helm values deploy file the proxy container adds a pre-stop hook.)
- This works perfectly!
- Except now the proxy cpu and memory annotations are ignored. You basically set proxy's cpu and memory to unbounded.
- I believe this is an order of merge operations from the default injection template combined with this one provided in the deployment pod template spec
Part of the confusion in this ticket @howardjohn is we have two completely different templates. We have the go-template injection by the injection controller and we have the helm template that we as application deployed use. I hope that corrects the confusion.
To further clarify. Here's fhe first part of our template pod template spec. I've edited it down
template:
metadata:
annotations:
app.kubernetes.io/version: {{ required ".Values.image.tag is required " .Values.image.tag | quote }}
deployed: {{ date "20060102150405" now | quote }}
{{- range $name, $value := .Values.annotations }}
{{ $name }}: "{{ $value }}"
{{- end }}
{{- include "mesh-annotations" . | nindent 8 }}
labels:
{{- range $name, $value := .Values.labels }}
{{ $name }}: "{{ $value }}"
{{- end }}
identity: {{ include "identity.service-account" . | quote }}
version: {{ include "identity.version" . | quote }}
app: {{ .Release.Name }}
{{- end}}
{{- include "mesh-labels" . | nindent 8 }}
This isn't too interesting by itself, but it includes "mesh-labels" and "mesh-annotations:
"mesh-labels" determines sidecar injection
{{- define "mesh-labels" }}
{{- if .Values.mesh.enabled }}
sidecar.istio.io/inject: "{{ .Values.mesh.enabled }}"
{{- end }}
{{- end }}
while "mesh-annotations" is the resource stuff (I'll redact most of it)
sidecar.istio.io/proxyMemory: {{ .memory | quote }}
sidecar.istio.io/proxyMemoryLimit: {{ .memory | quote }}
sidecar.istio.io/proxyCPU: {{ .cpu | quote }}
sidecar.istio.io/proxyCPULimit: {{ .cpu | quote }}
Finally mesh sidecar container definition. This is ONLY injected if mesh.prestop.enabled is on, otherwise we let the injection controller handle this.
{{- define "mesh-container" }}
{{- if .Values.mesh.enabled -}}
{{- if .Values.mesh.prestop.enabled -}}
- name: istio-proxy
image: auto
lifecycle:
preStop:
exec:
command: [ "{{ join "\",\"" .Values.mesh.prestop.commands }}" ]
{{- end }}
{{- end }}
{{- end }}
So IIUC the first part is the Deployment spec, so the fact its templated is invisible to Istio.
Here is what I think the end result you have:
apiVersion: apps/v1
kind: Deployment
metadata:
name: shell
spec:
selector:
matchLabels:
app: shell
template:
metadata:
annotations:
sidecar.istio.io/proxyCPU: 123m
sidecar.istio.io/proxyCPULimit: 200m
sidecar.istio.io/proxyMemory: 123Mi
sidecar.istio.io/proxyMemoryLimit: 250Mi
labels:
app: shell
spec:
containers:
- image: auto
lifecycle:
preStop:
exec:
command:
- sleep
- "12"
name: istio-proxy
- args:
- /bin/sleep
- infinity
image: howardjohn/shell
name: shell
something like that?
In my testing that works fine. Is there a difference for yours?
Archive.zip Yes. Attached are rendered deployments and the final injected pods. Note memory and cpu use the "default" with prestop, and use the annotations without
Do you have some controller mutating Deployment configs to add resourcces? Or maybe even to pods, running before Istio's injector? While the deployment object has no resources, the pod gets:
proxy.istio.io/overrides: |
{
"containers": [
{
"name": "istio-proxy",
"resources": {
"limits": {
"cpu": "100m",
"memory": "256Mi"
},
"requests": {
"cpu": "100m",
"memory": "256Mi"
}
},
"volumeMounts": [
{
"name": "kube-api-access-g9qnt",
"readOnly": true,
"mountPath": "/var/run/secrets/kubernetes.io/serviceaccount"
}
],
"lifecycle": {
"preStop": {
"exec": {
"command": [
"sleep",
"12"
]
}
}
},
"terminationMessagePath": "/dev/termination-log",
"terminationMessagePolicy": "File",
"imagePullPolicy": "Always"
}
]
}
So this is what the Istio webhook actually sees as the container spec by the time it reaches the webhook. This suggests something is actually adding these defaults.
I see kubernetes.io/limit-ranger: 'LimitRanger plugin set: memory request for container, which suggests its the limit ranger adding these.
So the order of operations is:
Base pod < templates (where annotation logic lives) < container overrides
The problem is something mutates the 'container overrides' before the Istio webhook, making it logically:
Base pod < templates (where annotation logic lives) < container overrides < limit-ranger mutation
That makes sense! But why does it only kick in when the skeleton option exists? I'm guessing because the yaml is wholly absen in that case
So without our "template"
deployment -> Limit Ranger - doesn't see "bad" container" so no-op -> istio-injector creates pod yaml -> pod...
with our template
deployment -> Limit Ranger - sees the template skeleton with no request/liimit and mutates-> istio-injector creates pod yaml and sees these take precendence so ignores annotations -> pod...
This makes sense but short of removing limitrange (because it's a lame crd and you can't omit minimums IIRC) what can we do? Is there another way to achieve the prestop desired?
Yep, exactly.
There is probably a few approaches:
- Don't use limit ranger (understandably unacceptable)
- Add the resources into the
istio-proxycontainer spec - Instead of in-lining the config into the pod (like https://istio.io/latest/docs/setup/additional-setup/sidecar-injection/#customizing-injection), make a new template and reference that. See https://github.com/istio/istio/blob/f2f6f265d9c3ffab144fd49df4ec9a287a7fe121/pkg/kube/inject/testdata/inject/custom-template.yaml#L12 (you want
custom,sidecarso they both apply -- else you will have to fully replace it which you don't want). Then define the custom template like so https://github.com/istio/istio/blob/f2f6f265d9c3ffab144fd49df4ec9a287a7fe121/pkg/kube/inject/testdata/inject/custom-template.iop.yaml#L10. Note you will need to read some values from the pod, so you would probably have your deployment templating insert something like an annotation, etc to program it
Those are all good ways. But seem inflexible. I'm not complaining per se - the design issues is fundamental to order of operations of admissions controllers, but very much affects this. Lets go through this list
- Don't use limit ranger (understandably unacceptable)
Yeah we need this.
- Create your own limit range controller as a replacement
It might be feasible to fork our own controller or use the CEL based as a replacement, but ultimately the issue is if you use istios "pod skeleton" feature, some mutator is is likely to have issues
- Repeat the literal cpu/memory settings in the pod skeleton, instead of just as annotation.
You allude to this and its actually the easiest thing - controlled in the same chart. The catch is if someone else changes something via annotation that mutating controller goes astray with. So I think this is better than the first option but has some future proofing.
- Instead of in-lining the config into the pod (like https://istio.io/latest/docs/setup/additional-setup/sidecar-injection/#customizing-injection), make a new template and reference that.
This is injected as permanent template in the injector CM I assume from what I read? That seems like that's not terribly flexible since you'd need to have 1 million templates (exaggeration) to cover every permutation of memory and cpu ;)
This is injected as permanent template in the injector CM I assume from what I read? That seems like that's not terribly flexible since you'd need to have 1 million templates (exaggeration) to cover every permutation of memory and cpu ;)
No, your template would look like
containers:
- name: istio-proxy
lifecycle:
preStop:
exec:
command: [ "sleep", "{{index .ObjectMeta.Annotations `some-annotation/sleep-duration`}}" ]
then when you want to use it, annotate on your pod inject.istio.io/templates: "custom,sidecar" and some-annotation/sleep-duration: 12s
Ok, that clarifies things and I've tested it and it works. Small correction below
prestop: |
{{ if (isset .ObjectMeta.Annotations `mesh.foo.com/sleep-duration`) -}}
spec:
containers:
- name: istio-proxy
lifecycle:
preStop:
exec:
command: [ "sleep", "{{index .ObjectMeta.Annotations `mesh.foo.com/sleep-duration`}}" ]
{{- end}}
Anyway now it's just finding the time to redeploy everywhere. Thanks @howardjohn