Skip to content
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

feat: ApplicationSet Go template #10026

Merged
merged 9 commits into from
Aug 8, 2022

Conversation

speedfl
Copy link
Contributor

@speedfl speedfl commented Jul 18, 2022

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.go

Non backward compatibility concern

In GitGenerator the following changes occured:

  • path is now a structure, therefore:
    • path representing the full path is now path.path (name could be dicuss)
    • path[n] representing the path segments is now path.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:

  • Either (a) I've created an enhancement proposal and discussed it with the community, (b) this is a bug fix, or (c) this does not need to be in the release notes.
  • The title of the PR states what changed and the related issues number (used for the release note).
  • I've included "Closes [ISSUE #]" or "Fixes [ISSUE #]" in the description to automatically close the associated issue.
  • I've updated both the CLI and UI to expose my feature, or I plan to submit a second PR with them.
  • Does this PR require documentation updates?
  • I've updated documentation as required by this PR.
  • Optional. My organization is added to USERS.md.
  • I have signed off all my commits as required by DCO
  • I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
  • My build is green (troubleshooting builds).

Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link

codecov bot commented Jul 18, 2022

Codecov Report

Attention: Patch coverage is 81.04839% with 47 lines in your changes missing coverage. Please review.

Project coverage is 46.11%. Comparing base (3f858f5) to head (de72194).
Report is 2684 commits behind head on master.

Files Patch % Lines
applicationset/utils/utils.go 75.00% 18 Missing and 6 partials ⚠️
applicationset/generators/matrix.go 57.14% 3 Missing and 3 partials ⚠️
applicationset/generators/merge.go 57.14% 4 Missing and 2 partials ⚠️
applicationset/utils/map.go 58.33% 5 Missing ⚠️
applicationset/generators/list.go 76.47% 2 Missing and 2 partials ⚠️
applicationset/generators/git.go 95.55% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            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     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@speedfl
Copy link
Contributor Author

speedfl commented Jul 18, 2022

@crenshaw-dev I will try this

@speedfl
Copy link
Contributor Author

speedfl commented Jul 18, 2022

@crenshaw-dev thanks a lot for your suggestion.

Here are the outcome of a discussion with a colleague:

path object contains:

  • path: the full path (previous path variable)
  • segments: for the list of segments (ex path[n] variables)
  • basename: the previous path.basename.
  • filename: the previous path.filename
  • basenameNormalized: the previous path.basenameNormalized
  • filenameNormalized: the previous path.filenameNormalized

For path function we unfortunately have to make a choice:

  • returning the full path (the previous path parameter)
  • returning the new object path, making all other parameters working (path.basename, path.filename, path.basenameNormalized, path.filenameNormalized)

Making this choice will break the backward compatibility as some field won't work.

Concerning the path[n], it is unfortunately not possible to have a function name path[n] (example path[0]). We will then have to break the backward compatibility.

I would suggest to add this feature as part of an ApplicationSet v1alpha2.

@crenshaw-dev
Copy link
Member

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 ., add .segments, and preserve the index with a matching group.

This regex should work for path.*:

{{\s*path\.([a-zA-Z]+)\s*}}

Just add a . at the beginning and preserve the member name with the matching group.

The replacements can be done on tmplBytes in RenderTemplateParams before the template string is parsed.

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 crenshaw-dev changed the title ApplicationSet Go template feat: ApplicationSet Go template Jul 18, 2022
@crenshaw-dev
Copy link
Member

crenshaw-dev commented Jul 18, 2022

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 {{ some.dynamic.value }}, making it replaceable with {{ .some.dynamic.value }}.)

@speedfl
Copy link
Contributor Author

speedfl commented Jul 19, 2022

@crenshaw-dev really good idea. Let me try it

@speedfl
Copy link
Contributor Author

speedfl commented Jul 19, 2022

If we replace only complex values (ie: some.value) we will miss all variables with a simple word.

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 cluster) we will have to whitelist all go template functions. Here is a small list (non exhaustive):

https://github.com/golang/go/blob/9ce28b518d9a792d2e3e741bcb38fa046891906e/src/text/template/funcs.go#L42-L62
https://github.com/golang/go/blob/9ce28b518d9a792d2e3e741bcb38fa046891906e/src/text/template/parse/lex.go#L80-L90

Other considerations:

  • this won't solve the path => .path.path and path[n] => path.segments[n] for which we will have to add a custom logic.

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 1

Create a v1alpha2 which use gotemplate.

Pro:

  • Backward compatible
  • Rollout deprecation
  • Easy removal (just duplicate temporarly the code and remove the full folder of v1alpha1 when ready)

Cons:

  • Need to introduce new controller + crds
  • Double maintenance ? It is possible to explain that maintenance will be done only on v1alpha2

Solution 2

Add a field (or an annotation) to activate go templating. This will allow us to have a rollout phase:

  • Phase 1: fasttemplate is still the default but deprecated. Field activate go templating
  • Phase 2: fasttemplate removed. Field to activate go templating not needed anymore

Pro:

  • Backward compatible
  • Rollout deprecation
  • Easy removal (just duplicate temporarly the code and remove the full folder leveraging fasttemplate)

Cons:

  • What happen to this field in the future ?
  • Double maintenance ? It is possible to explain that maintenance will be done only gotemplate or shared code

Solution 3

Try 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 path case

Pro:

  • Backward compatible
  • Transparent to user

Cons:

  • Performance issues for big workload
  • Complex maintenance
  • Ugly ?

Solution 4

Detect and hope we don't forget anything

Regexp for complex structure: {{\s*((?!\s*if.*|\s*range.*|\s*define.*|.*-.*|\s*\$|\.).*?\s*(?!-)?)\s*}} (we need to add all protected keywords):
https://regex101.com/r/ghzt0b/3

Regexp for path: {{\s*(path(\[\d+\])?)(\.(.*))?\s*}}: https://regex101.com/r/R0ofRv/1

@crenshaw-dev
Copy link
Member

crenshaw-dev commented Jul 19, 2022

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 goTemplate field alongside the template field.

@speedfl
Copy link
Contributor Author

speedfl commented Jul 19, 2022

Last regexp and corner cases: https://regex101.com/r/ghzt0b/4

{{ hello.hello-world }} Should match and Must be transformed into {{ index .hello "hello-world" }}.

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):

  • all occurence of path[n] to .path.segments[n] with {{\s*path\[(.*)\]\s*}} to {{ .path.segments[${1}] }}
  • all occurence of path to .path.path with {{\s*path\s*}} to {{ .path.path }}
  • all occurence of anything.with.or.without.dot to .anything.with.or.without.dot with {{\s*(?!\.).*?\s*}} to {{ .${1} }}

I maintain it will bring too much complexity and risk. Solution 1 and 2 are:

  • Safer
  • Better in term of performance (regexp can be really costly on big strings)
  • More maintainable (regexp can be really complex to maintain to cover all cases)

Concerning solution 2,based on the choice it will imply crd updates.

Keep me posted

@speedfl
Copy link
Contributor Author

speedfl commented Jul 20, 2022

@crenshaw-dev Here is what I can propose with legacy handling and transformation (with unit tests)

speedfl@29c42bd

If you are ok I can update the documentation + add some integration tests

@crenshaw-dev
Copy link
Member

Extremely well researched, thanks!

I agree with your hesitations about the regex implementation. Option 2 seems very reasonable to me.

Concerning solution 2,based on the choice it will imply crd updates.

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 goTemplate field would leave us stuck maintaining the old template field in v1alpha1. But I think that's a manageable cost.

@speedfl
Copy link
Contributor Author

speedfl commented Jul 21, 2022

@crenshaw-dev I did all the adaptation.

  • I ensured backward compatibility by putting back all unit tests & e2e tests with fasttemplate. There is a bit of test duplication but it was on purpose. When fasttemplate will be decommissioned we will just have to delete the "non go template" tests without doing chirurgical changes.
  • I updated the documentation by adding a GoTemplate section
  • I created all the examples in GoTemplate

@crenshaw-dev
Copy link
Member

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 speedfl force-pushed the feature/go_template branch 3 times, most recently from 318fdda to 4902529 Compare July 22, 2022 14:40
@speedfl
Copy link
Contributor Author

speedfl commented Jul 22, 2022

@crenshaw-dev yes I keep the logic for fast template:
https://github.com/argoproj/argo-cd/blob/master/applicationset/utils/utils.go#L76-L87

I just removed the allowUnresolved as looking at the code non are using allowUnresolved=false. In addition I factorized the code between what we have in RenderTemplateParams and in Replace

https://github.com/argoproj/argo-cd/blob/master/applicationset/generators/generator_spec_processor.go#L109-L110
https://github.com/argoproj/argo-cd/blob/master/applicationset/generators/cluster.go#L175-L176
https://github.com/argoproj/argo-cd/blob/master/applicationset/generators/cluster.go#L175-L176
https://github.com/argoproj/argo-cd/blob/master/applicationset/utils/utils.go#L40-L41

Concerning the GoTemplate, special characters are already handled.

If you are outside {{}} you are good. However if you want to use it inside you need by convention use ` character.

template.Must(template.New("").Parse("{{`{{value}}`}}"))

@crenshaw-dev
Copy link
Member

@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 .value == ", then JSON unmarshaling will fail.

The fasttemplate implementation resolved that by doing strconv.Quote(value)[1:-1] to JSON-escape values before inserting them into the template. I'm not sure if go template provides a hook to transform the evaluated value immediately before interpolation.

@speedfl
Copy link
Contributor Author

speedfl commented Jul 22, 2022

@crenshaw-dev you are right. It is blocking. Well seen. I added a unit test and working on the fix :)

@speedfl
Copy link
Contributor Author

speedfl commented Jul 25, 2022

@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:

  • " between {{}} are not escaped as they are used for templating functions
  • " outside {{}} are escaped (to not break unmarshall)
  • " in params are escaped correctly (to not break unmarshall)

@crenshaw-dev
Copy link
Member

crenshaw-dev commented Jul 25, 2022

@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:

		{
			name:        "quote",
			fieldVal:    `{{ slice .quote 1 }}`,
			expectedVal: ``,
			params: map[string]interface{}{
				"admin": `"`,
			},
		},

That test produces this:

        	Error:      	Received unexpected error:
        	            	unexpected end of JSON input

@speedfl
Copy link
Contributor Author

speedfl commented Jul 25, 2022

I have a question. What is the motivation of doing this json marshall before and not doing on all fields of the Application instead ?
Going through all fields would be more secure I think

@crenshaw-dev
Copy link
Member

Going through all fields would be more secure I think

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.

@crenshaw-dev crenshaw-dev added this to the v2.5 milestone Aug 4, 2022
@speedfl
Copy link
Contributor Author

speedfl commented Aug 4, 2022

@crenshaw-dev it seems that there is a problem in integration tests. Could you please check ?

@crenshaw-dev
Copy link
Member

@speedfl they've gotten flakier recently, and I haven't had time to figure out why. Re-triggering.

Signed-off-by: Geoffrey Muselli <[email protected]>
Copy link
Member

@crenshaw-dev crenshaw-dev left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@wtam2018
Copy link
Contributor

wtam2018 commented Aug 8, 2022

LGTM
Great work @speedfl .

@crenshaw-dev crenshaw-dev merged commit 97471f4 into argoproj:master Aug 8, 2022
@crenshaw-dev crenshaw-dev deleted the feature/go_template branch August 8, 2022 15:50
@crenshaw-dev
Copy link
Member

Hitting merge on this made my day. Very excited for 2.5. 😄

ashutosh16 pushed a commit to ashutosh16/argo-cd that referenced this pull request Aug 11, 2022
* ApplicationSet Go Template

Signed-off-by: Geoffrey Muselli <[email protected]>

* go template improvements

Signed-off-by: CI <[email protected]>

* update go mod

Signed-off-by: CI <[email protected]>

* fix tests

Signed-off-by: CI <[email protected]>

* fix tests

Signed-off-by: CI <[email protected]>

* fix tests

Signed-off-by: CI <[email protected]>

* Add tests for error handling

Signed-off-by: Geoffrey Muselli <[email protected]>

Co-authored-by: CI <[email protected]>
// 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 {
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@yuyuvn
Copy link

yuyuvn commented Sep 5, 2022

@speedfl
Hello, I have a question.
As you wrote in doc:

Go templates are applied on a per-field basis, and only on string fields.

If we can't use {{range ...}} then how can you solve the lack of loop support problem in #9961 ?

Can I do something like this?

apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
spec:
  goTemplate: true
  template:
    metadata:
      annotation:
        {{range .parameters}}
        {{.key}}: "{{.value}}"
        {{end}}

@speedfl
Copy link
Contributor Author

speedfl commented Sep 5, 2022

The problem of your approach is that it does not respect the crd model.
This PR is a per field approach.

For example in the application model, you have:

  • inline helm value: You will be able to do some logic inside with conditions an loops.
  • helm value files list. You will be able to use conditions to select the correct value file (example: customer expected traffic small, medium large)

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

@crenshaw-dev
Copy link
Member

@speedfl I'd be in favor of that. It was attempted, but looks abandoned.

@vavdoshka
Copy link
Contributor

this is so cool! Can't wait to test it out!

@yebolenko
Copy link

it seems that a lot of work has been done in this PR
What about support of combination of 2 (or more) git generators, e.g. git 2 file generators
in such way, /my-path/settings.yaml and /my-path/**/config.yaml
now it doesn't work
using the above, we can put some common values in settings.yaml and some more specific values inside config.yaml
Do you plan to add such support?
it also will be great if it will be possible to combine git directory and git file generators.

I know (from slack forum) that a lot of people wait for the above features.

@glasser
Copy link

glasser commented Oct 5, 2023

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 argocd-image-updater.argoproj.io annotations on Applications based on data we write in the YAML config files, but it doesn't look like it's possible to completely leave the annotations off entirely for apps that don't want to use image updater?

@kanor1306
Copy link

kanor1306 commented Dec 4, 2023

@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).

@zebesh
Copy link

zebesh commented Mar 14, 2024

@kanor1306 Hellow. Have you managed to make a patch for such annotations?

@kanor1306
Copy link

Sorry @zebesh I missed your message. Yes, I did mange to get this working.

  templatePatch: |
    {{- if .useImageUpdater | default false }}
    metadata:
      annotations:
        argocd-image-updater.argoproj.io/write-back-method: argocd
        argocd-image-updater.argoproj.io/image-list: '<image>'
        argocd-image-updater.argoproj.io/{{.appName}}.update-strategy: latest
    {{- end }}

Obviously this may get more complicated if you have a lot of things to patch, but my use case was pretty simple.

@hamadodene
Copy link

hamadodene commented Apr 22, 2024

Sorry @zebesh I missed your message. Yes, I did mange to get this working.

  templatePatch: |
    {{- if .useImageUpdater | default false }}
    metadata:
      annotations:
        argocd-image-updater.argoproj.io/write-back-method: argocd
        argocd-image-updater.argoproj.io/image-list: '<image>'
        argocd-image-updater.argoproj.io/{{.appName}}.update-strategy: latest
    {{- end }}

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?


apiVersion: argoproj.io/v1alpha1
kind: ApplicationSet
metadata:
  name: argocd-application-set
  namespace: argocd
spec:
  goTemplate: true
  generators:
    - matrix:
        generators:
          - git:
              repoURL: https://gitlab/argocd-deployments.git
              revision: HEAD
              directories:
                - path: dev/*/*  
              pathParamPrefix: app
          - git:
              repoURL: https://gitlab/argocd-deployments.git
              revision: HEAD
              files:
                - path: dev/*/*/image-updater-config.json
              pathParamPrefix: app2

  templatePatch: |
    metadata:
      annotations:
        argocd-image-updater.argoproj.io/write-back-method: git:secret:argocd-image-updater/git-creds


  template:
    metadata:
      namespace: argocd
      name: '{{index .app.path.segments 1}}-{{.app.path.basename}}-{{.app2.path.basename}}'
    spec:
      project: test
      source:
        helm:
          valueFiles:
            - values.yaml
        path: '{{.app.path.path}}'
        repoURL: https://gitlab/argocd-deployments.git
        targetRevision: HEAD
      syncPolicy: # auto update deploy in dev enviroment
        automated: {}
          #prune: true   --> If true argocd delete resources that not longer defined in git repository
      destination:
        namespace: '{{.app.path.basename}}'
        server: https://kubernetes.default.svc

No annotations are added with this config. I can't figure out where the error is.
my argocd version is v2.10.7+b060053

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Advanced templating for ApplicationSet Support conditions within ApplicationSet templating