router icon indicating copy to clipboard operation
router copied to clipboard

templatizeExtraLabels doesn't stringify typically non-string values

Open gwardwell opened this issue 7 months ago • 1 comments

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:

  1. Create an extraLabels label that has a string value that would otherwise be a non-string value (like "true")
  2. 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 -}}

gwardwell avatar Apr 09 '25 18:04 gwardwell

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.

glasser avatar Apr 11 '25 04:04 glasser