-
Notifications
You must be signed in to change notification settings - Fork 18
Description
Description
The helper methods responsible for parsing the image specified in the values files requires the seperation of registry and repository.
This applies to every _helper.tpl method with the name "<PROJECT_NAME>.image" (example shown here with azure-metrics-exporter: charts/azure-metrics-exporter/templates/_helpers.tpl#L69)
Code in question:
_helpers.tpl
...
{{/*
The image to use
*/}}
{{- define "azure-metrics-exporter.image" -}}
{{- if .Values.image.sha -}}
{{- printf "%s/%s:%s@%s" .Values.image.registry .Values.image.repository (default (printf "%s" .Chart.AppVersion) .Values.image.tag) .Values.image.sha }}
{{- else -}}
{{- printf "%s/%s:%s" .Values.image.registry .Values.image.repository (default (printf "%s" .Chart.AppVersion) .Values.image.tag) }}
{{- end }}
{{- end }}
...
deployment.yaml
...
containers:
#######################
# Kube pool manager
#######################
- name: azure-metrics-exporter
image: {{ include "azure-metrics-exporter.image" . | quote }}
imagePullPolicy: {{ .Values.image.pullPolicy | quote }}
...
Thus when specifying the repository as a Fully Qualified url, the Helm templating ends up failing to invalid image url:
values.yaml
image:
registry: ""
repository: "myorg.myregistry.io/azure-metrics-exporter"
tag: "22.9.0-2023-09-25-16-27"
Expected behaviour
deployment.yaml
...
containers:
#######################
# Kube pool manager
#######################
- name: azure-metrics-exporter
image: "myorg.myregistry.io/azure-metrics-exporter:22.9.0-2023-09-25-16-27"
...
Actual behaviour
deployment.yaml
...
containers:
#######################
# Kube pool manager
#######################
- name: azure-metrics-exporter
image: "/myorg.myregistry.io/azure-metrics-exporter:22.9.0-2023-09-25-16-27"
...
Suggestion
I want to submit a Pull Request that can solve this issue without changing the default behaviour.
I have two proposals:
- Check for empty registry.
In this case, the_helpers.tpldefinition of "<PROJECT_NAME>.image" (fx {{- define "azure-metrics-exporter.image" -}}) would allow the registry to be empty, like so:
{{- define "azure-metrics-exporter.image" -}}
{{- if .Values.image.registry -}}
{{- $imageQualifier := (printf "%s/%s" .Values.image.registry .Values.image.repository) -}}
{{- else -}}
{{- $imageQualifier := .Values.image.repository -}}
{{- end }}
{{- if .Values.image.sha -}}
{{- printf "%s:%s@%s" $imageQualifier (default (printf "%s" .Chart.AppVersion) .Values.image.tag) .Values.image.sha }}
{{- else -}}
{{- printf "%s:%s" $imageQualifier (default (printf "%s" .Chart.AppVersion) .Values.image.tag) }}
{{- end }}
{{- end }}
- Allow an overwriting repository qualification
In this case,_helpers.tpldefinition of "<PROJECT_NAME>.image" (fx {{- define "azure-metrics-exporter.image" -}}) would check for a variable invalues.yamlthat will override the returning image, like so:
{{/*
The image to use
*/}}
{{- define "azure-metrics-exporter.image" -}}
{{- if .Values.image.repositoryOverride -}}
{{- .Values.image.registryOverride }}
{{- else if .Values.image.sha -}}
{{- printf "%s/%s:%s@%s" .Values.image.registry .Values.image.repository (default (printf "%s" .Chart.AppVersion) .Values.image.tag) .Values.image.sha }}
{{- else -}}
{{- printf "%s/%s:%s" .Values.image.registry .Values.image.repository (default (printf "%s" .Chart.AppVersion) .Values.image.tag) }}
{{- end }}
{{- end }}
Concerns
I have seen that this implementation of image resolution is very similar to the implementation in prometheus-community/helm-charts/charts/kube-state-metrics/templates/_helpers.tpl#L123.
This was introduced in the kube-state-metrics by this issue: [kube-state-metrics] Image breaks when passing global.imageRegistry #3075 .
And the PR that fixed it mentioned that it was done due to conform to other Helm charts they manage:
We need to split registry and repository values in a proper way as usual in another charts.
I am wondering if the WebDevOps team would like to keep it consistent with such implementations?
If that is the case, then I will perhaps suggest it to the prometheus-community, as they have conflicting implementations.
Sidenote: Why don't I just use the values separately?
I sadly cannot specify these registry and repository values individually.
For my usecase, I do a cached build of every public image, incase I need to patch any vulnerability, and thus I have my own image hosted on a seperate Container Registry. This process is automatic, and results in a central values.yaml file, which contains a list of every image cached in the following format:
global:
images:
azure-metrics-exporter:
repository: "myorg.myregistry.io/azure-metrics-exporter"
tag: "22.9.0-2023-09-25-16-27"
When I then want to use this Helm Chart, I then install it as an Argo CD application pointing to this Chart, and specify Helm values to override:
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
name: azure-metrics-exporter
...
spec:
source:
...
helm:
values: |
image:
repository: {{ .Values.global.images.azure_metrics_exporter.repository | quote }}
tag: {{ .Values.global.images.azure_metrics_exporter.tag | quote }}
However in this case I can't just input my Fully Qualified Repository url.
I hope that this is okay for me to suggest. I will try and create a Pull Request, with the teams permission.
I am leaning more towards my first suggestion, as it would allow specifying both a Fully Qualified Repository url and a tag.