feat: ApplicationSet Go template #10026
Conversation
There was a problem hiding this comment.
This is awesome work, thanks!
I'd like to avoid a breaking syntax change if possible (i.e. requiring . before each value).
What if we augmented the template's function set with functions that simply return the value of the same name? Something like this:
for paramName, paramValue := range params {
template.Functions[paramName] = func() { return paramValue }
}This is pseudo-code, I'm not sure exactly where you add functions to the go template.
If . is not allowed in function names (like path.basename) we might need some kind of pre-processor to convert it to path_dot_basename or something like that before running it through the go templater.
Indexed param names might require something similar, like path[0] -> path_index_0.
These hacks would ensure backwards-compatibility. We could deprecate the dotless syntax and at some point drop support, requiring people to use .param instead of just param.
Codecov Report
@@ Coverage Diff @@
## master #10026 +/- ##
==========================================
+ Coverage 45.93% 46.11% +0.18%
==========================================
Files 227 227
Lines 27419 27550 +131
==========================================
+ Hits 12594 12705 +111
- Misses 13114 13132 +18
- Partials 1711 1713 +2
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
|
@crenshaw-dev I will try this |
|
@crenshaw-dev thanks a lot for your suggestion. Here are the outcome of a discussion with a colleague:
For
Making this choice will break the backward compatibility as some field won't work. Concerning the I would suggest to add this feature as part of an ApplicationSet |
|
I think my earlier suggestion was over-complicated. What if we detected old-style templates and simply upgrade them? This regex should work for path segments: {{\s*path\[(\d+)\]\s*}}You can insert the initial This regex should work for Just add a The replacements can be done on The docs can simply state that old-style params will continue to work as long as they're the only part of the template. If someone wants to properly use Go templating, they have to switch to the new syntax. |
crenshaw-dev
changed the title
|
The git files generator introduces some more complicated cases. I forget how flexible the old flatten logic was, but I believe a regex-based upgrade is still possible. {{\s*(([a-zA-Z\d]+?(\.[a-zA-Z\d]+))+)\s*}}(Matches |
|
@crenshaw-dev really good idea. Let me try it |
|
If we replace only complex values (ie: Example: apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
name: guestbook
spec:
generators:
- list:
elements:
- cluster: engineering-dev
url: https://1.2.3.4
template:
metadata:
name: '{{cluster}}-guestbook'However, if we try to detect all variables, whatever if there is separation or not (ie above https://github.com/golang/go/blob/9ce28b518d9a792d2e3e741bcb38fa046891906e/src/text/template/funcs.go#L42-L62 Other considerations:
Looking deeper I think this may add lot of corner cases, which can result in lot of regressions for basic templating and reduce functionalities for go template (this will introduce more maintenance) I would propose 3 solutions (my order of preference top to bottom) Solution 1Create a Pro:
Cons:
Solution 2Add a field (or an annotation) to activate go templating. This will allow us to have a rollout phase:
Pro:
Cons:
Solution 3Try go template. In case of failure backup to fast template. This would have a cost in term of performance. This will imply flattening + handling the Pro:
Cons:
Solution 4Detect and hope we don't forget anything Regexp for complex structure: Regexp for path: |
|
I still think option 4 is viable. The functions shouldn't be a problem, because as far as I can tell, they all require a space and then at least one argument. They won't match the example regexes. The keyword cases are easily solved by looping over the params and replacing single-word templates only for the params present. It would be up to the user to resolve collisions if they have a param name that overrides a Go template keyword they want to use. But point taken, there are probably a lot of corner cases. I think of the remaining options, I prefer option 2. It could be done by introducing a |
|
Last regexp and corner cases: https://regex101.com/r/ghzt0b/4
We can also do the following logic if s not matches `{{\s*-?(?:if|range|define|index|with|".*?"|\$|.*?\(.*?\)|\.).*?-?}}` # Search go template matchs
s = transformInGoTemplate(s)
fi
templateWithFastTemplate(s)Here is the logic to track Go Template ( https://regex101.com/r/lT8k6I/3) we need to add all protected gotemplate keywords. To tranform in go template it just consist in changing (sequentially):
I maintain it will bring too much complexity and risk. Solution 1 and 2 are:
Concerning solution 2,based on the choice it will imply crd updates. Keep me posted |
|
@crenshaw-dev Here is what I can propose with legacy handling and transformation (with unit tests) If you are ok I can update the documentation + add some integration tests |
|
Extremely well researched, thanks! I agree with your hesitations about the regex implementation. Option 2 seems very reasonable to me.
That's true. Across Argoproj, we've generally been okay with adding fields without versioning the CRD, so long as we don't subtract fields. Adding a |
|
@crenshaw-dev I did all the adaptation.
|
speedfl
force-pushed
the
feature/go_template
branch
from
dd23755
to
84717c3
Compare
|
This is looking awesome! Do we have tests for handling quotation marks in parameter values? I know the fasttemplate implementation had logic to handle escaping quotes (and other JSON control characters) before interpolating output back into the JSON blob. Is that being handled with the go template implementation? |
speedfl
force-pushed
the
feature/go_template
branch
3 times, most recently
from
318fdda
to
4902529
Compare
|
@crenshaw-dev yes I keep the logic for fast template: I just removed the https://github.com/argoproj/argo-cd/blob/master/applicationset/generators/generator_spec_processor.go#L109-L110 Concerning the GoTemplate, special characters are already handled. If you are outside template.Must(template.New("").Parse("{{`{{value}}`}}")) |
speedfl
force-pushed
the
feature/go_template
branch
from
4902529
to
d2daac5
Compare
|
@speedfl I'm more concerned about what happens when values contain quotes, since we're templating over a JSON blob. Consider this case: template.Must(template.New("").Parse(`{"key": "{{.value}}"}`))If The fasttemplate implementation resolved that by doing |
speedfl
force-pushed
the
feature/go_template
branch
from
d2daac5
to
dd21b08
Compare
|
@crenshaw-dev you are right. It is blocking. Well seen. I added a unit test and working on the fix :) |
speedfl
force-pushed
the
feature/go_template
branch
from
dd21b08
to
83d067f
Compare
|
@crenshaw-dev I was able to fix this issue. I added a beautiful unit test in utils_test.go :D {
name: "Index",
fieldVal: `{{ index .admin "admin-ca" }}, \\ "Hello world", {{ index .admin "admin-jks" }}`,
expectedVal: `value "admin" ca with \, \\ "Hello world", value admin jks`,
params: map[string]interface{}{
"admin": map[string]interface{}{
"admin-ca": `value "admin" ca with \`,
"admin-jks": "value admin jks",
},
},
},It tests that:
|
|
@speedfl I'm not sure pre-processing the params will be sufficient. The output of the go expression can still be un-escaped. For example: That test produces this: |
|
I have a question. What is the motivation of doing this json marshall before and not doing on all fields of the Application instead ? |
Honestly this would very much be my preference. Just recurse over the template and run substitution for each string field. I think it might be a bit slower. But so far that hasn't really been a problem. |
speedfl
force-pushed
the
feature/go_template
branch
from
83d067f
to
9cdece2
Compare
|
@crenshaw-dev it seems that there is a problem in integration tests. Could you please check ? |
|
@speedfl they've gotten flakier recently, and I haven't had time to figure out why. Re-triggering. |
Signed-off-by: Geoffrey Muselli <geoffrey.muselli@gmail.com>
speedfl
force-pushed
the
feature/go_template
branch
from
95bc2b7
to
de72194
Compare
There was a problem hiding this comment.
LGTM! Thanks for the persistence @speedfl, this is a huge feature.
Going to see if @jgwest or @rishabh625 have time to provide a second review.
|
LGTM |
|
Hitting merge on this made my day. Very excited for 2.5. 😄 |
* ApplicationSet Go Template Signed-off-by: Geoffrey Muselli <geoffrey.muselli@gmail.com> * go template improvements Signed-off-by: CI <michael@crenshaw.dev> * update go mod Signed-off-by: CI <michael@crenshaw.dev> * fix tests Signed-off-by: CI <michael@crenshaw.dev> * fix tests Signed-off-by: CI <michael@crenshaw.dev> * fix tests Signed-off-by: CI <michael@crenshaw.dev> * Add tests for error handling Signed-off-by: Geoffrey Muselli <geoffrey.muselli@gmail.com> Co-authored-by: CI <michael@crenshaw.dev>
| // 1. contain no more than 253 characters | ||
| // 2. contain only lowercase alphanumeric characters, '-' or '.' | ||
| // 3. start and end with an alphanumeric character | ||
| func sanitizeName(name string) string { |
There was a problem hiding this comment.
I think a sanitizer for hostnames would be even more valueable.
You can rarely just drop the end in case a dns name is too long, but you can shorten hostnames. An example where I expect this function to be used frequently:
- name: ingress.hosts[0].host
value: "{{ .branch | normalize }}.test.organization.example"
This requires truncation to 63 characters instead of 253.
I came here because I am currently trying to solve this very case for my organization. This PR is great by the way and I can't wait for 2.5 to land.
|
@speedfl
If we can't use Can I do something like this? apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
spec:
goTemplate: true
template:
metadata:
annotation:
{{range .parameters}}
{{.key}}: "{{.value}}"
{{end}} |
|
The problem of your approach is that it does not respect the crd model. For example in the application model, you have:
For your idea I would rather see an improvement, bringing the full application template in a string format in the model. apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
spec:
templateString: |
metadata:
annotation:
{{range .parameters}}
{{.key}}: "{{.value}}"
{{end}} This would template the full string and then try to Deserialize as Application. What do you think @crenshaw-dev |
|
this is so cool! Can't wait to test it out! |
|
it seems that a lot of work has been done in this PR I know (from slack forum) that a lot of people wait for the above features. |
|
Just to check — you can't use Go template (or anything) to optionally make an annotation based on app-specific data, or to control the name of the annotation based on app-specific data, right? eg, we're looking into setting the various |
|
@glasser I think the new templatePatch feature may help for your use case (I certainly hope so, as I intend to use it for the exact same case as you). |
|
@kanor1306 Hellow. Have you managed to make a patch for such annotations? |
|
Sorry @zebesh I missed your message. Yes, I did mange to get this working. Obviously this may get more complicated if you have a lot of things to patch, but my use case was pretty simple. |
@zebesh I'm trying templatePatch to add image updater annotation but seens not working. There is something not correct in my config? No annotations are added with this config. I can't figure out where the error is. |
Closes #9961
Closes #9681
Introduction
Purpose of this Pull request is to improve ApplicationSet templating capabilities by using Go text/template.
I am not a Go expert (Java Developer) so don't hesitate if you have remarks on best practices.
Security concerns
billion-laughs attack protection is working:
applicationset/generators/cluster_test.goNon backward compatibility concern
In GitGenerator the following changes occured:
pathis now a structure, therefore:pathrepresenting the full path is nowpath.path(name could be dicuss)path[n]representing the path segments is nowpath.segments[n](name could be discuss)Note on DCO:
If the DCO action in the integration test fails, one or more of your commits are not signed off. Please click on the Details link next to the DCO action for instructions on how to resolve this.
Checklist: