-
Notifications
You must be signed in to change notification settings - Fork 7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add alternative template delimiter #10299
base: main
Are you sure you want to change the base?
Conversation
Signed-off-by: Neer Friedman <neerfri@gmail.com>
Signed-off-by: Neer Friedman <neerfri@gmail.com>
Signed-off-by: Neer Friedman <neerfri@gmail.com>
@jdolitsky @mattfarina your review will be appreciated, thanks! |
👋 Friendly bump here... |
@neerfri thanks for the pull request and clear description of the need. I've added this to the agenda to discuss in our next meeting. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I had some concerns around the stability of the parser. I left a few comments surrounding those concerns.
I'm curious why you chose to do this per-template instead of a field in the Chart.yaml. It'd be less computationally expensive because you wouldn't have to run a regular expression against each template, and the syntax would be much simpler to understand (it's just another field in a YAML file). With regards to backwards compability concerns... You could attempt to load each dependency's chart metadata so their templates are all rendered based on the specific chart's delimiter. That way users can write charts with their own "custom" delimiter syntax and include charts that use another delimiter.
I do have some concerns that this may make it VERY difficult to port to another template engine in the future. Assuming we do introduce multiple template engine support in the future, this feature implies every template engine should be able to switch the delimiters in use. text/template is the rare exception - many template engines do not provide this ability. See #2577 for some context.
In some ways this makes #2577 more compelling - users could override the default template engine through inheritance, change the default delimiter, then select their custom template engine.
In any case this may be worth going through the HIP process. There are a number of critical design decisions in this PR to consider which may warrant a broader discussion with the community. See https://github.com/helm/helm/blob/main/CONTRIBUTING.md#proposing-an-idea for more details.
Happy to hear your thoughts on this one.
pkg/engine/engine.go
Outdated
if strings.EqualFold(string(match[1]), "delim") { | ||
delim := strings.SplitN(string(match[2]), ",", 2) | ||
templateOpts.delimL = strings.Repeat(delim[0], 2) | ||
templateOpts.delimR = strings.Repeat(delim[1], 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should there be a length check in place? What happens if I write the following?
# helm: delim=[
Wouldn't that result in a panic()
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, good catch!
This makes me wonder if it may be better to use separate modifiers for each delimiter like the text/template` does:
# helm: delimL=[
# helm: delimR=]
I'll give it another thought and play with the code to see the pros/cons
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fixed in new code
pkg/engine/engine.go
Outdated
for _, match := range matches { | ||
if strings.EqualFold(string(match[1]), "delim") { | ||
delim := strings.SplitN(string(match[2]), ",", 2) | ||
templateOpts.delimL = strings.Repeat(delim[0], 2) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't I specify my own delimiter syntax? For example,
# helm: delim={%,%}
Why does it have to be a single character repeated twice? Why not allow single character delimiters?
# helm: delim=$(,)
What happens if I wanted commas as a delimiter?
# helm: delim=,,,
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What happens if I wrap the delimiters in single or double quotes?
# helm: delim="{,}"
or
# helm: "delim={,}"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why can't I specify my own delimiter syntax? For example,
# helm: delim={%,%}
Why does it have to be a single character repeated twice? Why not allow single character delimiters?
# helm: delim=$(,)
I mentioned this in the PR description.
The problem is that when the template is rendered the line # helm: delim={%,%}
is parsed by the engine and it will try to evaluate {%,%}
and panic since ,
is not a valid go-template
expression.
I could remove the magic comment from the template, but I think it adds value to keep it there since you can see the magic comments that were part of the rendering process in the final output which contributes to debugging, not to mention performance (altering the string templates, that are kept in memory)
I could also try to escape it somehow, but just the thought of it gives me a headache considering we are talking about a dynamic delimiter :-D
# helm: delim={%,%}
should become
# helm: delim={%`{%`%},{%`%}`%}
What happens if I wanted commas as a delimiter?
# helm: delim=,,,
😱
Maybe we can even help downstream users and limit the possible delimiters to a set of characters that we know will not collide with other things in the templates.
Imagine if users would choose "" as the delimiter or even
#`...
What do you think about the set: [ { ( %
?
This paves the way to another good option which is to have a closed set of "delimiting style":
# helm: delimiterStyle=curlyBrackets
# helm: delimiterStyle=squareBrackets
# helm: delimiterStyle=roundBrackets
# helm: delimiterStyle=percentageSigns
This will give users less freedom but in return will have a much more simple, stable and stupid-proof UX, as an added bonus this solves the problem of having the explicit delimiter in the template (the one explained in the top of this comment) because it's not using the delimiter itself to describe the delimiter.
Thoughts?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I still think this would be better solved by implementing something similar to what was asked in #9302. We've heard more users ask about whether they can use alternative template engines like jinja2 or jsonnet rather than extending the existing text/template
engine. See also #8290.
If we're going to allow users to extend the existing template engine, IMO it would be more preferable to implement engine swap support rather than just switching existing feature flags through a "magic comments" system. Then you can write your own template engine that extends Helm's current pkg/engine
, swapping out the delimiter with your own choosing. If we find that system to be too clumsy then we can revisit this idea in the future. But we should be able to accomodate both your proposal here as well as what others have asked for in one fell swoop.
I understand this may be more work than you signed up for, but I think it's the right direction for the project overall.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree. I'd be extremely happy if Helm would have a pluggable template engine feature.
I don't see why this is a reason not to accept a tiny feature in the current built-in text/template
engine.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed.
The new code will now parse # helm: delimiter={%,%}
as valid input and will not "double" the delimiters.
It should also not panic for bad input
@bacongobbler Thanks for your reply. You're bringing up important points.
Using the Specifying the delimiter in the
Specifying the delimiter in a magic comment will mean:
Consider this feature a feature of helm's
Sounds good. Now that I see the core team is interested in the idea in general I'd be happy to invest in turning this to a HIP. Thanks again for the thorough review ❤️ ✌️ |
@neerfri thanks for bringing this up. We discussed it in the Helm developer call. One thing that came up in discussion is backwards compatibility. If an older version of Helm picks up a chart with this change it will silently ignore the annotation and render the chart incorrectly. It it could be silently broken. This is different from the situation where a new template function is introduced. An older version of Helm would throw an error in that situation. How would you handle this? I think we need two HIPs. One to introduce annotations that specify behavior. We should think that through so we can make sure it works for other cases that will come up. The second one is for the delimeter functionality. I've labeled this as v4 because I think it would require us to increment the chart version to v3 for compatibility. We might want to create a separate label for that. |
/reply @mattfarina
I guess the answer is embedded in the question :-D in
But in this particular case users want to use this feature so they can use current Helm without the feature:
**Helm with the feature:** (click to expand)
I created the HIP for the alternative delimiters helm/community#222 I don't know how to create the other HIP because I don't have much to say about it that is not directly about the alternative delimiter.
Not sure we need to increment the chart version for this, while this might look "big" it's not because it's not breaking any chart that does not use this feature. Labeling this Given the fact that it's not a breaking change, I would be happy if the helm team can consider putting this into |
Signed-off-by: Neer Friedman <neerfri@gmail.com>
9e861b9
to
4401259
Compare
Signed-off-by: Neer Friedman <neerfri@gmail.com>
To add some context.... Helm has a list of user profiles (roles) in a priority order. The top role is the "Application Operator". This is someone who takes a chart and installs a workload with it. They may not know much about Kubernetes or the application they are trying to run. For example, take PostgreSQL. They may not know much about operating it or StatefulSets. This is conceptually similar to a Linux package manager where someone can install an application without knowing how do work with systemd, where config files need to live, or where to put the binaries. They can install an app and it just works. With that in mind...
This requires the chart maintainer to put something in their chart. There isn't a way to Helm itself to protect the "Application Operator" in older versions that are already released. As we've seen, chart maintainers often fail to include things like this. They often don't realize their impact because it's outside of their personal use or the environments they test it. In Helm we want to protect those end users. Many of whom don't read the contents of a chart or completely understand them. There is a level of trust that Helm and the chart authors will protect them. In Helm, we try to put some level of basic protections in. If a chart author fails to put this sort of statement in and an end user installs something with an older version of Helm it could break their workload. This is the type of situation we try to avoid if possible. We are conservative in how we handle things prioritizing the Application Operator over the Application Distributor. How could we have this level of protection for those top priority users?
In my opinion, the experiment system has failed. This is because some people outside the Helm maintainers told their users that the experiments were production ready and people used it. Without checking on its status. This says a lot about human nature and how it's not logical or researched. Using the experiment the way they did ended up causing problems with turned into support issues for Helm. People didn't treat the experiment as an experiment. So, we are hesitant to perform other experiments. In the case of this feature, an experiment would not protect users of older Helm versions. If they tried to install the chart and didn't have the experiment it would cause a broken situation. The very thing we are trying to avoid. While not ideal, there are other ways to solve this problem. I've helped others on their charts. Have you tried any of the alternative methods to embed one go template in another and keep it as a template? |
@mattfarina Thanks for the explanation, I agree with all of it and I think it's awesome that you guys are thinking about this. As I mentioned any reasonable use-case for this feature will want to use Is this reasonable protection for the Application Operator? |
@mattfarina here's an example of running the test chart this PR adds with a version of helm that does not have the feature:
|
Here's an example that does not result in your error case.
If we use Helm 3.0.0, it will render the following:
If we use this PR, it will render the following:
Many users would argue this could break certain workloads. Both are valid manifests according to Kubernetes, but one of them was rendered incorrectly because they were using an older version of Helm. This reinforces the point that there needs to be some level of protection put in place in Helm to prevent this chart from rendering. We cannot rely on the chart author to do the right thing and put these safeguards in place. In the past we have done that by increasing the Chart.yaml's |
@bacongobbler Given this goes to helm v4 are there still any code changes needed? (your review marked "change requested") @bacongobbler @mattfarina How do you suggest to proceed here? |
@bacongobbler actually the example you posted does fail, which shows how unlikely it is to use the feature in a way that will not fail on an older helm version. Here's an example of a template that will be rendered wrong:
Here's how the previously posted example fails (due to invalid YAML):
|
* use modifier `delimiters=` instead of `delim=` * specify full delimiters instead of duplicating the delimiter (`[,]` -> `[[,]]`) * document the Template Header concept
* use modifier `delimiters=` instead of `delim=` * specify full delimiters instead of duplicating the delimiter (`[,]` -> `[[,]]`) * document the Template Header concept Signed-off-by: Neer Friedman <neerfri@gmail.com>
This is a fantastic feature we'd love to have! espacially to deal with argo-workflow template & prometheus alerting. Any chance to have it merged? |
Is this change still being considered? This would fix all my woes when deploying applications with inbuilt go-templating in config - like the aforementioned prometheus and alertmanager. |
Any updates here? |
As you can see from the discussions above the Helm maintainers still have reservations regarding introducing that feature into Helm. I personally believe the feature is safe enough and improves the UX and safety of charts that need to render resources with go templates in them but I assume I'm biased and respect the maintainers' say regarding this. if a Helm maintainer is interested I am willing to keep pushing changes to make this happen As a reminder there's also a HIP open for this: helm/community#222 |
I'm also in this boat, where using Argo + Helm is really painful. I'd like some fix, and here's maybe more of the possible solution space, if anything is maybe easier or more agreeable.
Given the presence of the delimiter changing |
Just referencing here what personnally helped me on this topic, the "raw string" : # Excerpt from a Prometheus Alertmanager yaml
receivers:
- name: slack-receiver
slack_configs:
- text: |-
{{`{{ range .Alerts }}
*Alert:* {{ .Annotations.summary }}
{{ end }}`}} Notice the backtick on starting and ending curly braces Found thanks to : #2798 (comment) (Still would love to see the feature, but didn't dig enough to have a real opinion :p) |
What this PR does / why we need it:
Some popular kubernetes components (prometheus, argo-workflows, ...) use templating with
{{...}}
syntax.This makes it hard for helm users to template the resources for these workloads (see links below).
The current workarounds render the templates hard to read and hard to maintain.
This PR addresses this by allowing a specific template to change its template delimiters.
The delimiter change is set using "magic comments" which is a wide spread trick used to carry more metadata inlined in a file (see examples below).
To change the template delimiter, a helm user would add a
# helm: delim=[,]
comment to the head of the template file:This will only effect the specific template and so compatibility with other charts and templates is not impacted.
Examples of magic comments in our ecosystems:
refs #4789
refs #2931
https://stackoverflow.com/questions/64802290/how-can-i-use-argo-workflows-templates-in-helm
https://stackoverflow.com/questions/56341558/how-to-escape-and-in-argo-workflow
https://tempered.works/posts/2020-05-21-helm-argo/
Special notes for your reviewer:
Added a unit test to verify rendering of a normal template as well as a template with alternative delimiter side by side.
I'd be happy to add documentation if this is merged.
If applicable:
TODO:
#