(feat) Add support for expanding templates in values

[mparry]

This PR is intended to address issues #2492 / #2133. It's obviously a reasonably large PR, at least at first glance, but I don't think that there's anything extraneous. I'll give an overview of the changes and hopefully that will make it more digestible. (If this looks familiar, that's because it is largely an edited version of a thread that I posted in the helm-dev Slack channel a while ago.)

Firstly, here are some pointers to the main parts of the PR:

• The implementation of the value expansion itself is in template_values.go, and this is the only section of entirely new code other than the corresponding tests that are adjacent.
• The other slightly non-trivial part is the refactoring in engine.go, but really this just boils down to a) breaking up render() so that I could re-use parts of it, and b) parameterise it a bit more.
• Finally this is hooked in in release_server.go (called from release_install.go or release_update.go depending on which command the user is running).

All of the new functionality is controlled by a new --expand-values flag, available on the install, upgrade, and template commands. The intention is obviously that if this is not set then everything will work exactly as it did previously. (I imagine that eventually this might all be enabled by default, but that would be somewhat backwards incompatible, for the edge case where someone has used literal {{ characters in values.)

In release_server.go there is a small hook in ReleaseServer.renderResources() to call a new Engine.ExpandValues() method, if the above flag is set. This method recurses through the .Values tree, expanding values via the standard templating engine as required. I think the implementation should cover all use cases: you can have recursive references to other values, etc.

This expansion occurs after values have been coalesced, operating upon the entire resulting .Values tree.

As I couldn’t see any way of handling general recursive evaluation with the usual .Values.foo syntax, there is a new tval function, specifically for use in values. You would write something like this:

a: foo
b: '{{ tval "a" }}bar' # foobar
c: '{{ tval "b" }}baz' # foobarbaz

As is discussed on the ticket, and as you can see above, it's necessary for these files to be valid YAML, so any templated values need to be wrapped in quotes.

Obviously this looks quite a bit like usage of the existing tpl function. However, the disadvantage of that is that you have to write it every time a value is used, whereas with the above you just write it once. Also, a significant limitation of tpl is that you can’t use it for passing parent chart values into a sub-chart, in the case where you want to rename or modify the value in the process. I think that if my solution is adopted then, longer term, tpl probably isn’t really necessary any longer.

Limitations in my implementation that I’m aware of, but that seem reasonable:

1. The --expand-values flag must be set consistently with the initial install when running upgrades. Failure to do so is likely to just lead to immediate errors, but I'm not sure that's guaranteed.
2. The get command does not currently provide a way of retrieving the resolved values; it just shows you the release as it was sent.

[mparry]

As noted in the commit comment, the changes immediately below address #3054, as it fell quite naturally out of the rearrangement I was doing here. Unlike the change in #3061, which aims to fix the same problem, this simply avoids modifying .Template in the first place (thanks to the new addTplMeta flag).

[bacongobbler]

Hey @mparry! There's definitely a lot going on in this PR so I'll try to break down my comments. I want to make it clear that I'm very happy that someone from the community is taking on this feature, so thank you very much for contributing this!

I think we're going to need to write a lot of documentation around how the template expansion engine works. I know there's a small section you write in the docs around tval, and the comments in the OP is helpful from a developer's perspective, but there needs to be a LOT more on the end-user's side or we're gonna get bombarded with questions like "how to I get this to work"? Perhaps some examples or a tutorial would be helpful. A section under "Chart Template Guide" might be the right place for this. 😄

Secondly, I noticed that there's a few changes being made to the RPC protocol between helm and tiller, as well as for public-facing changes in pkg. For 2.0, we made the promise not to break backwards compatibility for the API so that third party clients using the same RPC endpoints or using functions in pkg should work the same from 2.0 all the way to the end of the 2.x timeline. If this is unavoidable, then this PR may have to sit out a while until we start 3.0 planning (or come up with another solution that works around this). The good news is that we should be kicking off 3.0 discussion at the Helm Dev Summit in late February in Portland, OR. I understand that you're in England, but we'd love to have you there if you could make it!

Finally, I noticed there's a few unit tests in here around pkg/engine/template_values.go which is great, but it'd be awesome if we could have a bit more test coverage around supplementary files like pkg/engine/engine.go. There were a huge whack of changes made in other files, but no tests were added to cover/explain the changes being made to that package. It's especially important that we document every change being made to an open source project like Helm. That way others can understand why certain behaviour of the code base changed over time, why it was changed and its limitations or strong points.

Again, I wanted to say thank you for contributing this! ❤️

[mparry]

documentation

Sure, I can expand this.

I noticed that there's a few changes being made to the RPC protocol between helm and tiller, as well as for public-facing changes in pkg. For 2.0, we made the promise not to break backwards compatibility for the API so that third party clients using the same RPC endpoints or using functions in pkg should work the same from 2.0 all the way to the end of the 2.x timeline.

You may need to elaborate here, I'm afraid. Certainly the protobuf messages have been expanded but isn't this backwards compatible? I think it should be the case that the client->server messages, if sent in the old format, would simply not have the new fields, in which case expand_values should be treated as being false. Similarly, the extra final_values field in the response can just be ignored by clients that don't understand it (and in any case, it won't be of any interest if expand_values isn't set).

I don't think there are any public API changes in pkg API. What did you have in mind?

it'd be awesome if we could have a bit more test coverage around supplementary files like pkg/engine/engine.go. There were a huge whack of changes made in other files, but no tests were added to cover/explain the changes being made to that package.

The idea here was that it was just an internal refactoring. Any code using the existing APIs should not observe any changes, so the point is that the existing tests still pass without modification.

The purpose of the refactoring was to make some of the internal functionality available to the new value expansion code, ie. it's used in ExpandValues(). This is exercised via the tests that cover those code paths.

I could look at adding more, but in light of the above, I'm not sure where exactly.

[mparry]

I should also mention that the conflict that's currently reported is because #3061 was merged. As I mentioned in the opening comment, my refactoring also meant that I could easily fix that bug (#3054). My fix seems preferable, but I appreciate I may be biased! I can unpick this, if the existing fix from #3061 is preferred.

[Chumper]

I also expect the protocol to be backwards compatible and so far I don't see anything that will break that guarantee. So this looks good to be merged into 2.X in my opinion.

I would like to understand why tval is needed and if we can get rid of it. Wouldn't it also be an option to parse the file or expansion again and again until there are no more expansions available?
Sure we would need to protect against endless recursion via depth level o.s, but you are doing that already on the tval code.
I mean can we not assume that every expansion might be recursive and treat it like that?

[mparry]

I would like to understand why tval is needed and if we can get rid of it. Wouldn't it also be an option to parse the file or expansion again and again until there are no more expansions available?
Sure we would need to protect against endless recursion via depth level o.s, but you are doing that already on the tval code. I mean can we not assume that every expansion might be recursive and treat it like that?

I originally explored this approach but one problem was that there didn't seem to be any sensible way of writing literal {{ characters, if you're relying upon the standard expansion code. If you're continuing to recurse until nothing further is expanded, you will always undo any escaping.

[johngmyers]

The --expand-values switch is given at the wrong time. A chart written with templates in values.yaml expects those templates to be expanded and will malfunction if used without that switch. So a chart using this feature would have to document that it has to be installed/upgraded with the --expand-values switch and the chart maintainer would end up getting a stream of support tickets from users who fail to do so.

The templated values might want to live somewhere other than the top-level values.yaml (perhaps templates/_values.yaml) so that they don't appear to be part of the parent chart's external API. That could also address the cyclic references issue.

[mparry]

The --expand-values switch is given at the wrong time.

I agree that having to specify this flag on the command line is not ideal, but something equivalent is necessary for backwards compatibility (at least if this is to go into a 2.x release I think). Do you mean that it would be better to express this in the charts somehow instead (ie. so that the chart declares that it expects its values to be expanded)?

The templated values might want to live somewhere other than the top-level values.yaml

I can see the appeal of this, but I think it's reasonable to view it as a further extension to this feature. That is, I don't think it would be helpful to further expand this (already large) PR by attempting to add it at the same time.

[johngmyers]

Yes, it would be better to express the need for templating in the chart, since it is an attribute of the chart.

I don't think doing the templating in the top-level values.yaml is advisable. Doing it in a newly added place addresses the backwards compatibility (to the extent nobody has created a file in the new location chosen).

[mparry]

The latest update makes the following changes:

1. Rebases against latest master.
2. The feature is now enabled via an expandValues flag in Chart.yaml (rather than requiring that an --expand-values flag is specified in every helm install command line etc.)
3. Additional documentation. (Note that this has tipped the PR over from an XL rating to XXL. The actual code change is slightly smaller than previously, due to point 2.)

@bacongobbler - when you have time, could you let me know if there is anything else that you think is required here and/or where we stand on getting it merged? Thanks.

[Chumper]

The latest changes look good to me, I also think that the declarative way of defining the flag in the Chart.yaml is preferred to a console flag. I like it 😄

Is there any way we can help to bring this PR any further so that it gets merged in the near future?

[johngmyers]

nit: shouldn't change existing tab to spaces

[rajatjindal]

Hi @bacongobbler, @thomastaylor312 can you please look at this PR. We are looking forward to use this feature.

Only if you think this feature will make it to kubernetes/helm is when we would like to use it (we are using a fork for using some other unreleased [but merged] features with intention to start using upstream again with next release).

[thomastaylor312]

@rajatjindal I think this missed 2.9 unfortunately. I am going to let @bacongobbler take a look at this again because he started it. Once he reviews, I'll add an additional review

[johngmyers]

This code fails to properly handle the test case I put in johngmyers/test-helm-expand@02b452e.

The test case has a chart without a expandValues flag in Chart.yaml, but containing a subchart with expandValues. The code in this PR will expand the values in the subchart when it is used stand-alone, but not when used as a subchart of the parent chart.

I believe this PR should not be merged unless this issue is addressed.

[fejta-bot]

Issues go stale after 90d of inactivity.
Mark the issue as fresh with /remove-lifecycle stale.
Stale issues rot after an additional 30d of inactivity and eventually close.

If this issue is safe to close now please do so with /close.

Send feedback to sig-testing, kubernetes/test-infra and/or fejta.
/lifecycle stale

[monotek]

@mparry
Could you please resolve the merge conflicts and fix the issues mentioned here:
#3252 (comment)

/remove-lifecycle stale

[mfilotto]

@mparry any update on this one ?

[mparry]

Yes, we had independently discovered that subchart issue in our usage of helm - assuming I'm understanding the description correctly - but I haven't had chance to address it.

Though that said, I was also awaiting some further feedback from the maintainers; I don't especially want to spend a lot of further time on this if its prospects of being merged are slim.

[dtshepherd]

@bacongobbler Any update on reviewing this PR? It would drastically reduce the tooling we need to use around helm to template image/repos for 3rd-party charts.

[caiobegotti]

Guys, what's blocking this from having a final review or approved or whatever? I hit helm/charts#7114 and apparently this would help.

[allen-servedio]

@bacongobbler and @thomastaylor312 - any thoughts on how we can move this or something similar forward? There appears to be a growing number of tools springing up around this issue (some wrapping helm, some generating the values files, etc...). It would be great if it could be solved within Helm itself. It is fine if you all want a different solution than this - I think we are just looking for something to address it.

[bacongobbler]

For various technical and design reasons behind how Tiller's template rendering engine works, we do not intend to support pre-processing values.yaml as a Go template for either Helm 2 or Helm 3. However, we highly encourage the community to write wrappers or Helm plugins that support this feature for their use case! One such example we've heard from users is gomplate, but we'd love to see other pre-processors that can provide this functionality prior to running helm install.

To re-iterate a previous comment in the original thread: adding support for expanding environment variables in values.yaml would be an easy problem to tackle without introducing much complexity to the rendering engine, but using full-fledged templates would be a major undertaking.

Even if we were able to support this feature, it would result in a sizable increase of bug reports filed when people eventually hit the edge cases. Given the current volume size of the issue queue and the limited number of maintainers and community members chipping in to help weigh in on issues, we simply do not have the human resources to support this undertaking.

We apologize for the inconvenience, and thank you for understanding!

[caiobegotti]

It's absolutely sad that it took you guys that much time (the PR was filed back in december last year!) to simply deny the feature because you fear it may increase the number of new issues on Github. If you had various technicals and design reasons for closing the PR this was the time to describe them. IMHO that is not a healthy response to the comunnity and it ended up looking amateurish.

[dcwangmit01]

Harsh, true, and well said. This was a solid PR.

Perhaps helm needs a plugin system that provides access to the rendering layer. Too bad.

[dtshepherd]

I'm sad this was closed, but I understand @bacongobbler's concerns. Environment variables might be an acceptable solution for some subset of problems, but I wish we had a better solution for encapsulating 3rd-party chart values.yaml inputs. I don't want my master charts to have to know details about a microservice's subcharts (and their inputs).

I looked into using gomplate to pre-process values.yaml, but it caused a few problems for us: 1) we can't easily apply it to 3rd-party subcharts, 2) we would have to maintain two template languages (gomplate does't use Sprig) as well as two input sources (datasources vs values.yaml), 3) we couldn't come up with a good hierarchy for chart/subchart templating, which is already solved by helm.

For now, I developed a helm chart mirroring script that patches the existing chart templates with the tpl function, so the values.yaml can contain templates. It has mostly solved my use case, but I kind of hate having to run the pre-processor...

https://gist.github.com/dtshepherd/968d57ac2badea8c1df9daaaa05d8f58#file-mirror_helm_chart-sh-L53-L55

The script is still in development, so use at your own risk.

[nuwang]

@bacongobbler I think the lack of this feature really breaks encapsulation and is a pretty serious design flaw. @mparry has clearly demonstrated a very comprehensive a solution to the issue. If a lot of support issues are anticipated however, having something basic even (e.g. simple variable substitution within the scope of a single values file only, similar to Ansible playbooks), is better than having nothing at all, because having to use third-party tools just for DRY and encapsulation is in unfortunate contrast to the simplicity and reusability that the rest of Helm provides.

For example, our issue is one of simply being able to set the same password across a number of subcharts, while exposing one password value in the top level umbrella chart. To clarify, we have a top-level chart for our app, which includes subcharts for prometheus and grafana. We want the passwords for all three to be the same. Right now, we have to expose the innards of the chart, and set the password three times, individually, which badly breaks encapsulation, DRY and comprehensibility. That's just one simple example, but we've run into several others.

I feel that this is an issue that users are going to run into repeatedly for any non-trivial chart. For us at least, it's our number one issue right now with helm charts.

[kraney]

I have a similar use case, a chart that depends on Elasticsearch and Kibana, both third party charts. I just want to point Kibana to the correct Elasticsearch service, but since they are both third party I can’t make them conform to some arbitrary variable at my level.

It’s a ridiculous thing to need a wrapper to solve. In my opinion it makes Helm very vulnerable to any competing solution that comes along. Clearly the maintainers are not spending time building on third party charts or they’d see the issue.

[nuwang]

Since it was suggested elsewhere, I wanted to clarify why it's not possible to use a secret in the example I gave above, although most third-party charts support a secret name as a means of using a shared password. For starters, this is not generalizable to other cases where the simple requirement is to push a single value to multiple subcharts, without exposing the implementation details of the chart.

Secondly, the secret name that's passed to the subcharts cannot be prefixed by the release name, as the release name is a variable, and naturally, needs to be templated. This means that the secret name has to be passed around as a constant, and you end up with only being able to have a single instance of the chart installed per namespace, which breaks the whole idea behind unique releases. That pretty much eliminates the use of secrets as an option, and we fall back into the trap of having to modify a third party chart that we do not control, or individually passing in the password several times per subchart - fully exposing the implementation details of the umbrella chart.

So one way or the other, this either breaks encapsulation, breaks unique releases, breaks DRY, breaks comprehensibility and often, a combination of all of the above.

A few more we ran into that come to mind:

• Persistent volume claims that need to be shared among multiple subcharts, but must also support multiple releases. Same problem as with a secret, without being able to prefix the claim name by the release name, you end up being able to have only one release per namespace.
• A single shared value that a user provides at chart install time, that needs to be pushed to several subcharts. There appears to be no way to do this, other than to individually push the values to each subchart, exposing implementation details, or adding tpl within the subchart, which means modifying a chart that we do not control.

[thomastaylor312]

I wrote up a clarification for why we made this decision on the main proposal:
#2492 (comment)