router
router copied to clipboard
templatizeExtraLabels doesn't stringify typically non-string values
Describe the bug
apollographql.templatizeExtraLabels does not properly translate string values to strings when creating labels if they would otherwise be another scalar type.
For example, adding:
router:
extraLabels:
somelabel: "true"
would end up attempting to create this label:
somelabel: true
which would cause an error.
To Reproduce
Steps to reproduce the behavior:
- Create an
extraLabelslabel that has a string value that would otherwise be a non-string value (like"true") - Observe manifest creation failing with an error around labels saying something like
contains non-string key in the map: true is of the type bool, expected string
Expected behavior
Manifest generation would work just fine.
Additional context
Overriding apollographql.templatizeExtraLabels with the following that uses toJson to stringify the label value solved the issue, but it could introduce breaking changes:
{{- define "apollographql.templatizeExtraLabels" -}}
{{- range $key, $value := .Values.extraLabels }}
{{ printf "%s: %s" $key (toJson (include "apollographql.tplvalues.render" ( dict "value" $value "context" $)))}}
{{- end -}}
{{- end -}}
You can work around this with
router:
extraLabels:
somelabel: '"true"'
but it is weird that you have to do this.
I think the fix above for the function (adding toJson) is a good idea. Note that if anybody has actually done a workaround like the above, the fix will make the workaround try to put double quotes in the label — ie, it will result in somelabel: '"true"' just like you wrote it. This is not a valid Kubernetes label value (labels have a restricted set of characters that does not include double quotes). So while this change (which I recommend) is a "breaking" change in that it would require anyone who's done the two-levels-of-quotes workaround to simplify their config, anyone taking the upgrade who'd be affected would immediately find out because Kubernetes would reject their chart — that is, it's an "obviously blatantly breaking change" rather than a "silently subtlely breaking change" which seems better.