feat(chart): Provide config secret and use tpl function for providers and extraArgs

[jkroepke]

Description

This wrapper the content of some helm values through the helm tpl function.

This is needed in some helm umbrella charts situation. For example I have a secret named {{ include "external-dns.fullname" . }}, but I can not reference it in extraVolumes. Instead I have to use a static secret name.

Checklist

• Unit tests updated
• End user documentation updated

[stevehipwell]

@jkroepke thanks for the PR but I'm not sure we can accept this, the way the tpl function works makes it unsuitable for use cases where you're calling it on a complex object. This use case has come up a number of times in this repository and others (notable Grafana for loki-distributed config) and I myself have implemented this pattern only to have to rip it all out of a whole repository worth of charts.

In the case of Helm KISS often beats DRY; but an alternative that does work is using a type check and allowing values to either be an object in which case no tpl or a string in which case you can use tpl safely.

[jkroepke]

Do you have an example for such a complex object in that uses cases here? In any case, the toYaml filter function will guarantees that a string is passed to the tpl function.

.Values.provider should be always a string. But I add a toString filter here.

I have the feeling that the approch using tpl (toYaml .) $ works fine. bitnami charts, prometheus-community/kube-prometheus-stack, kubernetes/ingress-nginx or grafana/loki charts are using this at many places.

I agree with your that it should be not used for all variables.

[stevehipwell]

Do you have an example for such a complex object in that uses cases here? In any case, the toYaml filter function will guarantees that a string is passed to the tpl function.

I'd suggest that you give it a try with a local dummy chart as you will see where it falls over.

I have the feeling that the approch using tpl (toYaml .) $ works fine. bitnami charts, prometheus-community/kube-prometheus-stack, kubernetes/ingress-nginx or grafana/loki charts are using this at many places.

I'd be interested where in the KPS this is done as I've personally been part of removing it from that chart a number of times. The Loki charts were the first place where I saw this pattern have a major issue, AFAIK they use the type filter test or YAML object merge patterns. I also wouldn't use the Bitnami charts as an argument to use a pattern.

[jkroepke]

I'd suggest that you give it a try with a local dummy chart as you will see where it falls over.

https://github.com/jkroepke/homelab/tree/main/helm-chart-examples/to-yaml - here we are. No fails over. Feel free to add your sharing as PR here.

I'd be interested where in the KPS

Here is a another PR on loki, which is the same approach as this PR. None of the current maintainers of the loki charts debate this pattern. It feels like acceptable.

From external-dns point of view, define a map on .Values.provider or define any thing except list of string .Values.extraArgs would break the deployment, too. Independently if tpl is used or not.

---

I guess we reach a point now where is more personal related discussion then technically. I would recommend that you or @njuettner do a review and based on the result we may proceed or fully decline the pull request.

[stevehipwell]

@jkroepke this isn't a personal discussion, it's purely technical. The tpl command can be used on simple strings but breaks as soon as someone misunderstands the way the Go type system functions and expects to be be able to template out more than just an individual key or string value. For that reason, IMHO, it's not worth the limited improvement in dryness at the cost of additional, and not always obvious, complexity.

Your examples meet the simple string requirements so they work. The Loki example is the same or similar to ones which have either been removed from other Grafana Loki charts or replaced with similar but different patterns (look at how affinity is dealt with as a string in the same chart). The specific Loki example is made safer by the fact that it isn't replacing the selector labels template but it's still misleading in what it would actually do given anything other than an explicit string map.

[jkroepke]

@stevehipwell

I redesign the PR to introduce the tpl function only on safe string scenarios. It should now meet your requirements. On top, helm validates now, if a string is passed.

[stevehipwell]

I've added a couple of comments.

[stevehipwell]

This is looking good.

For consistency would it be best to have a secretConfiguration.enabled value? That's generally how these are defined once they're more than a single key.

[jkroepke]

secretConfiguration.enabled added.

[stevehipwell]

/lgtm

[stevehipwell]

@Raffo @seanmalloy could one of you approve the GH workflow?

[seanmalloy]

/approve

[k8s-ci-robot]

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: jkroepke, seanmalloy

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

• charts/OWNERS [seanmalloy]

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

[stevehipwell]

/retest

[stevehipwell]

@seanmalloy could you re-run the failing test?

[Raffo]

@stevehipwell the issue is in master, see #3180 (comment) . We can revert sooner if you need this out, the user did not answer yet.

[stevehipwell]

@Raffo I think this PR can wait.

[Raffo]

@jkroepke can you merge master in?

[jkroepke]

@Raffo @stevehipwell PR rebased.

[stevehipwell]

@Raffo could you approve the GH workflows?

[stevehipwell]

/lgtm