Cannot use placeholder in jwtIssuer value

[erikgb]

We are using https://fluxcd.io/flux/components/kustomize/kustomizations/#post-build-variable-substitution to manipulate advanced configurations (per Kubernetes cluster). Configuring Spire is far from simple, and we appreciate the Helm chart provided by this project.

Values reproduce the bug:

global:
  spire:
    jwtIssuer: https://oidc-discovery.apps.${SPIRE_TRUST_DOMAIN}

Expected behavior: Helm chart is templated with the placeholder retained.

Actual behavior:

Error: Error: template: spire/charts/spiffe-oidc-discovery-provider/templates/deployment.yaml:2:19: executing "spire/charts/spiffe-oidc-discovery-provider/templates/deployment.yaml" at <include (print $.Template.BasePath "/configmap.yaml") .>: error calling include: template: spire/charts/spiffe-oidc-discovery-provider/templates/configmap.yaml:61:8: executing "spire/charts/spiffe-oidc-discovery-provider/templates/configmap.yaml" at <include "spiffe-oidc-discovery-provider.yaml-config" (dict "oidcSocket" $oidcSocket "root" .)>: error calling include: template: spire/charts/spiffe-oidc-discovery-provider/templates/configmap.yaml:26:14: executing "spiffe-oidc-discovery-provider.yaml-config" at <urlParse (include "spire-lib.jwt-issuer" .)>: error calling urlParse: unable to parse url: parse "https://oidc-discovery.apps.${SPIRE_TRUST_DOMAIN}": invalid character "{" in host name

I believe the error is caused by the use of a "dumb" Helm function: https://github.com/spiffe/helm-charts-hardened/blob/3bc70255e7aba204dfb22fe84fce598500b67131/charts/spire/charts/spiffe-oidc-discovery-provider/templates/configmap.yaml#L26

I'll be happy to submit a PR to fix this, but let's discuss how to fix it first. It seems like the intention is to include something deduced from the jwtIssuer into the configuration domain array, and I suspect we'll get a breaking change if attempting to fix this....

[kfox1111]

That is a particularly gnarly problem.....

Thanks for bringing it up though.

Some things to help define the problem space:

• The jwt issuer needs to have a protocol on it order for it to work properly with some clients.
• the oidc discovery provider's domains config option has to have the the domain name in it without protocol or path
• We're using the urlParse function to get the pieces parsed out properly
• The urlParse function doesn't like variable substitution, because its not a valid url...
• Some of the config files support ${xxxx} format themselves for substitution, For example, the password when set in mysql or postgresql mode shows up in the rendered config file as ${DBPW} and shouldn't be substituted by flux...

[kfox1111]

Is there a way to have flux build up values to pass rather then try and substitute them via templating via values, and then trying to re-template things that in some cases are also still config file templates?

[erikgb]

Is there a way to have flux build up values to pass rather then try and substitute them via templating via values, and then trying to re-template things that in some cases are also still config file templates?

Well, we could always try workarounds, but let's focus on the issue first. 😄

As an experienced software developer, I have learned that putting things together is always safer than ripping something apart (parse). So if we REALLY need to make these settings interconnected, I prefer building another value from a simpler variant. And not the opposite - as it is now. But changing this will probably break things for users. The question is: can we break this in a "smooth" way?

I think the best solution would be to NOT connect these settings at all. What would be the consequence? We could set/update defaults that might reduce the chance of something breaking.

[kfox1111]

That is something to consider, yeah. But maybe there is another option too before we go too far down that path.

Rather then try and make things work by sliding in another template engine between the helm template engine and the uploading of the objects (complex/fragile), what if we added enough functionality to the chart where you could do what your trying to accomplish with the chart itself?

Like, maybe we could get this to work instead:

global:
  spire:
    jwtIssuer: https://oidc-discovery.apps.{{ .TrustDomain }}

I think that should be possible to implement fairly easily. We'd modify the jwt issuer function to template out the string passed.

[kfox1111]

Yeah.... that took very little code to get to work.... Please kick the tires with this and see what you think:

diff --git a/charts/spire/charts/spire-lib/templates/_helpers.tpl b/charts/spire/charts/spire-lib/templates/_helpers.tpl
index db33cee..c249330 100644
--- a/charts/spire/charts/spire-lib/templates/_helpers.tpl
+++ b/charts/spire/charts/spire-lib/templates/_helpers.tpl
@@ -15,6 +15,12 @@
 {{- end }}

 {{- define "spire-lib.jwt-issuer" }}
+{{- $values := dict "TrustDomain" (include "spire-lib.trust-domain" .) }}
+{{- $value := include "spire-lib.jwt-issuer-raw" . }}
+{{- tpl $value $values }}
+{{- end }}
+
+{{- define "spire-lib.jwt-issuer-raw" }}
 {{- if ne (len (dig "spire" "jwtIssuer" "" .Values.global)) 0 }}
 {{- .Values.global.spire.jwtIssuer }}
 {{- else if ne (len .Values.jwtIssuer) 0 }}

Can see it working here:

$ helm template charts/spire --set 'global.spire.jwtIssuer=https://foo.bar.{{.TrustDomain}}' | grep jwt_issuer
        "jwt_issuer": "https://foo.bar.example.org",

[erikgb]

Do we really want to complicate the chart even more? How smart/dumb is the Helm tpl function? Will it barf if someone are using Go template syntax in the value - except the supported one: {{ .TrustDomain }}?

I am personally not a big fan of Helm, but it can work if the templates are not too complex. It's worth mentioning that we don't use Helm at all in our clusters. We render the charts to resource manifests client-side and check them into Git. That makes it possible to perform effective governance of the software we install using standard GitOps practices. 😺

[erikgb]

Another problem with the proposed workaround is that we use a placeholder in the trust-domain also: 😆

global:
  spire:
    clusterName: ${SPIRE_CLUSTER_NAME}
    # FIXME: Setting the JWT issuer to an URL with placeholder does not currently work.
    # See https://github.com/spiffe/helm-charts-hardened/issues/455
    # jwtIssuer: https://oidc-discovery.apps.${SPIRE_TRUST_DOMAIN}
    jwtIssuer: oidc-discovery.spire.apps.${CLUSTER_DOMAIN}
    trustDomain: ${SPIRE_TRUST_DOMAIN}

Since the Spire config is embedded into configmaps, we cannot patch in these cluster adjustments - which we normally use. Luckily Flux has it's post-build substitutions that works basically like envsubst on the resource manifests before SSA.

[kfox1111]

The use case isn't specific to you/flux. Having support in the chart for the use case makes sense to me.

The template uses only the values passed in the second arg to tpl. So should only see the one value.

Other charts such as all the bitnami charts support this kind of thing as well. So its fairly commonly done.

I agree, helm is a bit of a mixed bag. Really good at some things, really not so good at others.

[kfox1111]

Another problem with the proposed workaround is that we use a placeholder in the trust-domain also: 😆

global:
  spire:
    clusterName: ${SPIRE_CLUSTER_NAME}
    # FIXME: Setting the JWT issuer to an URL with placeholder does not currently work.
    # See https://github.com/spiffe/helm-charts-hardened/issues/455
    # jwtIssuer: https://oidc-discovery.apps.${SPIRE_TRUST_DOMAIN}
    jwtIssuer: oidc-discovery.spire.apps.${CLUSTER_DOMAIN}
    trustDomain: ${SPIRE_TRUST_DOMAIN}

Since the Spire config is embedded into configmaps, we cannot patch in these cluster adjustments - which we normally use. Luckily Flux has it's post-build substitutions that works basically like envsubst on the resource manifests before SSA.

Heh. so, back to 3 template engines trying to be used at once, along with two that use the same substitution syntax.... :/

Ok, thanks for trying the patch.... Will ponder this some more.