feat: Go template with sprig for application template render (#303)

[vavdoshka]

There might be many interesting use-cases where this feature will come in handy. Just a couple that was in demand for me so far:

• In case of Git File generator centric flow, one can define a default namespace on the ApplciationSet level with {{default "default-name-for-namespace" .namespace}} (Sprig Default Fucntions) while can customize the namespace name if present in the file data. To keep the Git File data DRY, users can also apply this pattern for many other parameters.
• Use reach string formating fucntions (Sprig String Functions). Something like {{trunc 63 (printf "%s-%s" (now | date "2006-01-02") .cluster)}} in order to generate unique namespace name for ephemerial application and make sure it will not run over the limit of 63 characters.

Some ideas over the design for this need:

• The existing template rendering can not be seamlessly extended with the Sprig-like functionality due to the different semantics of the variables in both. In Go Template/Sprig {{ namespace }} will try to call a function namespace, which is different from existing behavior. Aligning the behavior of both seems like a non-trivial and risky venture.
• We could get maximum flexibility in template rendering with Go template and Sprig if the template input would be a raw string rather than ApplicationTemplate type. A change like RawTemplate string `json:"rawTemplate,omitempty"` in ApplicationSetSpec would give full freedom in inserting whatever users wants as Application template. I know at least one use-case where it could be useful described here (controlling syncPolicy section creation). But I believe this option is too complex and might lead to some bad DX, so I refused to POC it.

So I end up integrating Go template / Sprig as seamlessly as possible and letting users decide whether they want to use it or not.

With this POC Pull Request, I want to validate the design/implementation with the maintainers.
If the design is verified, and required changes are done, I want to add some more tests to have some executable spec and add the documentation section about this functionality.
Also, it would be great to get some guidance on how to test the performance of this solution adequately, maybe compare it with the primary templating implementation so that it is also documented and users can make better decisions.

[crenshaw-dev]

Just a couple notes while you continue to work on this:

1. If we can timebox the template evaluations, we probably should.
2. We should probably remove particularly slow Sprig functions (like some of the crypto functions).

[crenshaw-dev]

Another random thought: is it okay to run the template tool across the whole template, or should we recurse through the template and apply templates on a field-by-field basis?

The downsides are speed and complexity.

The upside is we avoid letting params break out of their field. People setting params like "\n targetRevision: my-other-branch\n could make changes to the Application template which weren't anticipated by the AppSet authors.

[vavdoshka]

Another random thought: is it okay to run the template tool across the whole template, or should we recurse through the template and apply templates on a field-by-field basis?

The downsides are speed and complexity.

The upside is we avoid letting params break out of their field. People setting params like "\n targetRevision: my-other-branch\n could make changes to the Application template which weren't anticipated by the AppSet authors.

Hello @crenshaw-dev,
I did some deeper research about how it works with fasttemplate and gotemplate based implementation.

They behave nearly identical, with a tiny difference. With Gotemplate, to begin with, the application structure change is not possible with the current implementation. Here is some demonstration:

https://github.com/vavdoshka/applicationset/blob/b7e9f463628e2424cc4ba6fcc3a9ded94f8a07d2/pkg/utils/util_test.go#L365-L374

The input to GoTemplate renderer is a yaml marshaled Application struct (which is a type-checked user input), so wherever the {{ .Value }} patterns are used these would be the strings enclosed in quotes. And hence the result of the render would be a string too. There are probably some ways to break out of the string with some complex combinations of quotes, though. But I did not manage to do this. So from this standpoint, the behavior of GoTemplate render is identical to fasttemplate based.

The only difference I have found is that with fasttemplate based rendering one can pass in the identation special characters such as \t\n through the paramaters and this will be correctly interpreted in the output, while with GoTemplate in current implementation, these characters would not be interpreted correctly, being repplaced by spaces. But I can not imagine a use-case where it could be usefull, nor I don't thing there are some generators which would produce the parameters with such characters.

Anyway, I think we should probably focus on the option I decided to drop initially. What if we allow Application to be passed down as a raw string from the user to GoTemplate without checking the types and Marshaling it. We would take this raw string and render it as is, then do Unmarshal on the output (as done now with both implementations). So the new thing would be to set a new field in ApplicationSetSpec something like goTemplate with type string, which when provided by the user will be passed down to GoTemplate then. So yes, in this case, the result of this render might be a not valid application or taking to much time to render potentially too. But first it is an advanced templating option for users who knows what they do, second we can validate the correctness of the resulting App with MustUnmarshal and inform the user of the error early, and also we can time box the execution as you suggested. What additional scenarios or options that this would enable:

1. Evaluating the values for non-string value types, such as:

boolean values:
.spec.syncPolicy.automated.*

integers:
.spec.revisionHistoryLimit
Can not be set currently due to type check

and control the whole optional object creation, such as:
.spec.ignoreDifferences
.spec.info
.spec.syncPolicy

Could be very usefull for example to control .spec.syncPolicy.automated creation based on some criteria like branch or directory. Or just to imagine how far it can go one can set dynamically every SCM Provider Generator label value (repository labels) as application metada.label (parse labels csv string in list and do range over it to create as many labels as needed)

2. Resolve the issue with special characters escaping. There will be no need to do so since the user would have complete control of the template structure.

3. Bring more intuitive experience to the users familiar with Helm/etc. Otherwise, like in current implementation, they need to understand that the GoTemplate is something limited to the values of the exisiting structure and can be only strings.

WDYT about this approach?

[crenshaw-dev]

@vavdoshka thanks for the detailed writeup! I'm still not sure where to go with the flexibility/predictability tradeoff. I'm going to spend some time reading the duplicate issues (#518, #447, #351, #303) to see what makes the most sense for their use cases.

I'm also going to look at the use cases for non-string fields (#424, #385, #287) to see if they justify full-text templating.

Finally, I need to look at @jgwest's proposal for an arbitrary-JSON version of template (kind of a middle ground between the current PR and full-text templating) to see if this PR covers any of the use cases mentioned there.

tl;dr - this PR is looking awesome, but I gotta do some reading/thinking before responding to your design questions. :-)

[crenshaw-dev]

Oh! For an out-of-bounds field edit test, try this:

		{
			name:        "the output application structure is imutable",
			fieldVal:    "{{ .yamlPiece }}",
			expectedVal: "some-value\n    chart: some-other-chart",
			templateOptions: &argoprojiov1alpha1.ApplicationSetTemplateOptions{
				GotemplateEnabled: true,
			},
			params: map[string]string{
				"yamlPiece": "some-value'\n    chart: 'some-other-chart",
			},
		},

Given a substitution at the right place, that could change the chart field value.

[vavdoshka]

found also similar need here #168

@crenshaw-dev with tast input the actual value I recieve now is
some-value chart: some-other-chart vs expected some-value\n chart: some-other-chart

so tabulation and line ending are lost during unmarshaling, I've spotted that issue already and not yet exactly sure how to fix this, I will be happy to dig in if needed once we would settle on the overall design.

[crenshaw-dev]

With the above test, I can actually force chart to have the value some-other-chart for some values of fieldName in the test.

        	Error Trace:	util_test.go:398
        	Error:      	Not equal: 
        	            	expected: "some-value\n    chart: some-other-chart"
        	            	actual  : "some-value"

(From the debugger)

In one proposed fix for a similar bug in Argo Workflows, Alex simply traversed through the templated object and only performed templating at the individual-string level. I thought it was a pretty elegant solution, but it was dropped in favor of a less intrusive option at the time. https://github.com/argoproj/argo-workflows/pull/6442/files#diff-f052cf13b02f8aaed79df3589c9f5d0124dc630043901ef3a2c89cc3d829dd90R17

[crenshaw-dev]

Okay, I did my reading. :-)

Use cases

Use cases which only require string templating:

• Regex replacements in variables to make valid name values (Add the ability to use templating on generators values #518, Enhancement: ApplicationSet should support functions in template #447, add ability to modify template variable #351)
• Falling back to default values (Ability to have default templated values or sprig usage in template #303)

Use cases which require non-string primitive templating:

• Setting syncPolicy booleans (Ability to have default templated values or sprig usage in template #303, Cannot template syncPolicy parameters because of applicationset validation #424, Non-string template variables (alt: template overrides from git) #287), revisionHistoryLimit integer

Use cases which require non-primitive templating:

• Looping over lists (Git Generator Files: Ability to loop over keys #385, Non-string template variables (alt: template overrides from git) #287)
• Setting objects like ignoreDifferences and syncPolicy

Proposals

1. Allow go templating, but only within fields (the PR as currently written)
   • Pros:
     • relatively simple
   • Cons:
     • doesn't allow non-string templating
2. Allow go templating over a text field
   • Pros:
     • allows templating all types
   • Cons:
     • allows variable definers relatively-open access to modify the Application
     • might duplicate effort w/ @jgwest's proposal (personally I'd rather see only text or JSON templating implemented, to keep things simple)
3. Allow go templating over a JSON field (Add a new untyped template field, to support passing JSON/YAML objects into ApplicationSet template fields  #362 (comment))
   • Pros:
     • allows templating more types
   • Cons:
     • it's unclear whether/how this will support non-string types (no examples in proposal)

Recommendations

If it were me writing the code, I'd go with proposal 1. It solves a few use cases with relatively little trouble.

But if you're feeling really excited about implementing 2 or 3, I think it would be important to coordinate with @jgwest to make sure that whatever gets written solves the use cases described in his proposal. I know he's been especially busy lately, so that collaboration might be difficult. Hopefully he can chime in here?

[crenshaw-dev]

In the back of my mind, I have this idea of duplicating the Application structure, replacing certain non-string fields with JSON fields and then letting the unmarshaller/templater figure out how to handle each field. So I think proposal 1 could eventually cover all use cases. But I'm not precisely sure how it would work.

[vavdoshka]

Thanks @crenshaw-dev for these inputs

I believe it is worth to spend a bit more time to think of how that new templating feature would satisfy all of these usecases. And I do agree that it can cover #362 need as well.
The rationale is that new templating anyway would provide a new public API and if we know in advance that it is not complete (like would require some new version (rewrite?) or other feature #362 to accommodate the full need) then it does not seem like a good path.

I will dig in this weekend to experiment and hopefully find a better solution.

[crenshaw-dev]

Sounds good!

My main concern about #362 is that, if the template is YAML-formatted, my template must also be valid JSON. For example, I couldn't do this:

spec:
  source:
    helm:
      parameters:
      {{ range .Values.parameters }}
       - name: {{.Name}}
         value: {{.Value}}
      {{ end }}

The output is valid YAML, but the template itself is not valid YAML, so I don't think it would be supported by #362.

[jgwest]

Re: #362, @crenshaw-dev, your lists of pros and cons for each approach looks accurate, great analysis! For 362, it should support non-string types: eg a generator should be able to return a json object as a generator parameter, and it should be serialized into the appropriate field of the template, as a full object (rather than just as a string).

Personally I don't have any insight into which of the use cases we should focus on, like you I can see the pros and cons of each. My motivation for #362 is it seems like a bunch of folks are using Helm, and wanting to insert helm params (JSON objects) into the Application helm field (the Helm example I posted on the issue) from Git generator.

[crenshaw-dev]

@vavdoshka with Jonathan's comments in mind, I'd recommend going ahead with full text-field templating. At some point I'd like to explore per-field templating for users who need more control over their Application resources. But I think full text-field templating would cover all the use cases.

[vavdoshka]

@crenshaw-dev sorry I did not communicate back, I've made some progress -
I have implemented the full-text field templating and tested it against different edge cases, seems working fine, but code is messy, did not refactor it yet.
But then also I realized that we can relatively easily bring a big piece of functionality if utilize {{=expression}} syntax and do per field calculation (similarly to ArgoWorkflows) using a combo of fasttemplate on {{= + }} tokens and then run template/sprig on extract. The advantage is that most users will be able to easily change their existing templates without changing the template type and adding too much complexity there.
And I thought that probably there should be a way also to expand the {{=expression}} idea to support even non-string primitives and objects.

I just need to have a bit more time, I'm sure I will be able to wrap it up by the end of this week.

[crenshaw-dev]

@vavdoshka that's fantastic! My only suggestion would be that we use something besides {{= to avoid confusion with Workflows, which uses expr to evaluate the contents. I'm not particular, something like {{! should be fine.

[stephaneetje]

Hello, any update concerning this PR ? I need to pass a bool value for syncOptions, but can't right now

[crenshaw-dev]

@vavdoshka if you want to just push what you have, I can try to get it across the finish line!

[vavdoshka]

@crenshaw-dev
I appologies, let's get to the finish line indeed, and I think we are almost there. I just pushed a new version where we have two types of templating behavior:

• Support of {{variable}} and ${{expression | .varaible}} with the fast template for only string type. This implementation is backward compatible, so users can add the format templating in exisiting templates by using ${{expression | .varaible}} syntax where needed, old syntax is supported in the same template as well. This covers these usecases: TEXT FORMATTING - issues Enhancement: ApplicationSet should support functions in template #447,add ability to modify template variable #351,Add the ability to use templating on generators values #518, DEFAULT VALUE - issue Ability to have default templated values or sprig usage in template #303

• New field in ApplicationSet spec untypedTemplate (probably dumb name), which has a string type and hence allows to pass raw template and cover the rest of the use-cases: SETTING NON-STRING PRIMITIVES (BOOLEANS) - issues Ability to have default templated values or sprig usage in template #303,Cannot template syncPolicy parameters because of applicationset validation #424,Non-string template variables (alt: template overrides from git) #287, SETTING NON-STRING PRIMITIVES (INTEGER) - no raised issues so far, LOOPING OVER PARAMETERS KEYS - issue Git Generator Files: Ability to loop over keys #385, CONDITIONALLY MOUNT CONFIG SNIPPET, HELM VALUES OR PLUGIN CONFIG - issue Non-string template variables (alt: template overrides from git) #287, Enhancement - Conditional plugin support #168

A consequence of having two different template inputs is that none of them is mandatory now, and I'm not sure if my proposal to handle this is correct, but it works.
And also the generator templates are becoming useless in the case of untypedTemplate usage. This logic logs a warning in that case.

I tried to go with ${{expression | .varaible}} to cover all use-cases but was not able to find a way to change the existing Template to a semi-untyped template that would support both template strings and non-string primitives or objects at the same time. I believe the custom unmarshalling can be used in a normal scenario but not sure how to do this in the case of controller-gen.

Happy to finalize this in short time :)

[rishabh625]

@vavdoshka : Can u please move this PR into argocd as applicationset is moved. If u find it laborious, i can do it on your behalf with your consent

[vavdoshka]

Sure, I agree, please move Пн, 4 апр. 2022 г. в 07:25, rishabh625 ***@***.***>:

@vavdoshka <https://github.com/vavdoshka> : Can u please move this PR into argocd as applicationset is moved <argoproj/argo-cd#8864>. If u find it laborious, i can do it on your behalf with your consent — Reply to this email directly, view it on GitHub <#513 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ACZERGCHXCUSB3RTFD6IBGDVDJ4OHANCNFSM5O47ZCQQ> . You are receiving this because you were mentioned.Message ID: ***@***.***>

-- Cordialement, Vladimir Avdoshka

[SelfhostedPro]

I'm going to take a swing a coming up with some docs for this per: https://cloud-native.slack.com/archives/C01U45M2SVB/p1650554181079489?thread_ts=1650552801.132729&cid=C01U45M2SVB if that'd be helpful.

[zbialik]

Any progress/updates on this PR? Or on where it's been moved within the argo-cd project?

I am looking to use a Git File Generator to define a list parameter in a config.yaml like so:

valueFiles:
- values.yaml
- app-specific-values.yaml
- prod-values.yaml

This would allow me to setup the ApplicationSet to have a templated helm section like:

spec:
  template:
    spec:
      source:
        helm:
          valueFiles: '{{valueFiles}}'

As of right now, we are forced to use helm.values field since it accepts string objects which the parameters automatically render as. But our dev teams like the ability to define separate valueFiles to propagate values updates to all other helm releases which use that given overriding values file.

[crenshaw-dev]

@rishabh625 is this on your TODO list? No worries if not, you've got plenty on your plate. Just want to give @zbialik a status update. :-)

[rishabh625]

I'll do it @crenshaw-dev I forgot about it, since was looking at argo-cd issues only, we should have migrated issues too..