kubernetes-ingress: add extraObjects templating

[hedgieinsocks]

Allow chart consumers to template arbitrary CRs as part of the helm release.

[oktalz]

hi @hedgieinsocks

I added some comments

[oktalz]

table is missing its header row

[hedgieinsocks]

indeed, my bad, added missing table header

[oktalz]

$value is always coerced to a string
(typeIs "string" .value | ternary .value (.value | toYaml)), but the else
branch passes that string straight to merge, which needs a map:

Error: ... executing "kubernetes-ingress.renderExtraObjects" at <$value>:
wrong type for value; expected map[string]interface {}; got string

[oktalz]

maybe something like

{{- define "kubernetes-ingress.renderExtraObjects" -}}
  {{- $labels := fromYaml (include "kubernetes-ingress.extraRawLabels" .context) -}}
  {{- $value := typeIs "string" .value | ternary .value (.value | toYaml) }}
  {{- if contains "{{" (toString $value) }}
    {{- $value = tpl $value .context }}
  {{- end }}
  {{- toYaml (merge (fromYaml $value) $labels) }}
{{- end -}}

?

[hedgieinsocks]

oh, I decided to complicate the original design that served as an inspiration https://github.com/CloudPirates-io/helm-charts/blob/aff6783b21ce92155849464eb612551d2e8788d5/charts/common/templates/_helpers.tpl#L231-L241 and botched it a bit, but your suggestion seems to be working, so accepted, thanks!

[oktalz]

{{- include ...}}

will merge with previous line

{{ include ...}}

is more correct here

[hedgieinsocks]

gotcha, updated

[hedgieinsocks]

@oktalz hey thank you very much, it should be ready for round 2

[dkorunic]

I've double checked MR and there are a few problems I'd like to highlight:

1. {{- $value = tpl $value .context }} runs always and any object carrying non-Helm Go-template syntax (Grafana dashboards, Prometheus recording/alerting rules, Alertmanager templates) will hard-fail helm install or helm upgrade. There is no way to opt-out a single object out of templating and inherently a part of how tpl works. TL;DR tpl on the whole object breaks any extraObject containing foreign {{ }}.

2. ci/extra-objects-configmap-values.yaml breaks ci naming convention. Files are prefixed with its mode, so they are either deployment-* or daemonset-*, so the file should be named deployment-extraobjects-values.yaml.

[dkorunic]

Maybe this would be a better choice, @oktalz WDYT? I've added comments to explain the code:

  {{- define "kubernetes-ingress.renderExtraObjects" -}}
    {{- $labels := fromYaml (include "kubernetes-ingress.extraRawLabels" .context) -}}
    {{- if typeIs "string" .value }}
      {{- /* string form is an explicit opt-in to templating */ -}}
      {{- toYaml (merge (fromYaml (tpl .value .context)) $labels) }}
    {{- else }}
      {{- /* map form is emitted verbatim; foreign {{ }} left untouched */ -}}
      {{- toYaml (merge (deepCopy .value) $labels) }}
    {{- end }}
  {{- end -}}

[hedgieinsocks]

We can also go for a simpler solution with distinct keys e.g. extra(Raw)Manifests and extra(Raw)Templates. We use this approach in most of our internal charts, but here I tried to be fancy. If you'd like me to refactor it to this approach, I'll gladly do.

[hedgieinsocks]

@dkorunic what about the refactor in the latest commit (001890e)? I've also added a basic CR check to try to ensure the chart fails gracefully when jibberish is pushed into extra objects. I can omit the validation if it does not feel right.