argoproj / argo-cd

Declarative Continuous Deployment for Kubernetes
https://argo-cd.readthedocs.io
Apache License 2.0
17.8k stars 5.43k forks source link

Support nested objects in merge generator #12836

Open girstenbrei opened 1 year ago

girstenbrei commented 1 year ago

Summary

Currently, the merge generator relies on the mergeKeys list to select which child generators to merge. This works for goTemplate: false with nested keys, but using Golang templates it only works for root keys.

Motivation

Templating functionality in the values object of a generator allows a user to dynamically assemble and format a specific merge key, especially using Golang templates. It decouples the shape and value of fields generated by a generator into a user-definable one. But, without being able to select a key within the values object, a merge can only be performed with the whole values object. Additionally, this is a feature already working with the non-golang-template engine, making this an inconsistency between the two.

This was discovered as part of implementing #10754 .

Proposal

This issue exists for ApplicationSets with goTemplate: true only. This means, the solution could rely on Golang templates to be available. One option therefore would be to enable Golang templating inside the mergeKeys list values.

Example

# […]
goTemplate: true
spec:
  generators:
    - merge:
        mergeKeys:
          - "{{ .values.merge }}"
        generators:
          - clusters:
            values:
              merge: "{{.metadata.labels.availabilityZone}}/{{.metadata.labels.datacenter}}"
          - git:
            repoURL: https://github.com/argoproj/argo-cd.git
            revision: HEAD
            directories:
              - path: "*"
            values:
              merge: "{{ index .path.segments 0 }}/{{ index .path.segments 1 }}"
# […]

First, the child generators would be evaluated, as they would now. This resolves the templating inside the .values.merge fields into concrete values. Then, the merge generator would evaluate each merge key for each child generator separately. In this example, the merge key {{ .values.merge }} would evaluate to the already expanded values for the git and cluster generator. If the values match, the generated elements merge normally.

Pros

Cons

Additional Info

I think this doesn't break any existing merge keys. The behavior only changes, when goTemplate: true is set. In this case, currently, no nested merging is possible, meaning every valid key in mergeKeys must be a top level key in a generator. As there is no generator with top-level keys which are valid Golang templates, there is no valid merge key now in existence which might be a valid Golang template (and therefore evaluate differently after the change).

There might be other variants of doing this. One option would be to use something like jsonpath. But, this would be an additional, different mechanism which looks visually similar to the existing templating functionality, maybe confusing users when to use what. Another option would be to have something like a mergeObject, which contains the structure of what to merge, but not it's values (which would be in the generators). "Walking" this merge object and the generator values would then merge if the values exist. But this would mean implementing the object walking, which would probably be more work than "template this field with this context". Additionally it would introduce another field next to mergeKeys.

Feel free to let me know about any concerns or better ideas you have regarding this proposal. If you could see this as a viable route, I'd be interested in trying to get a PR ready.

m13t commented 1 year ago

Just hit this same problem after migrating to Go templates as we wanted to use the default function to guard against missing config in the git files generator. Not being able to use the path.basename with Go templates is breaking things for us so it would be great if we can use this with Go templates in the future.

amanfredi commented 1 year ago

Unless I'm missing something, this is preventing all usage of the merge generator with git generators when go templating is enabled, because there is no top level object that can be used as a merge key (what used to be path is now path.path). https://argo-cd.readthedocs.io/en/stable/operator-manual/applicationset/GoTemplate/#git-generators

crenshaw-dev commented 1 year ago

Yep. I think this other PR serves as a good basis for fixing the problem: https://github.com/argoproj/argo-cd/pull/13584#issuecomment-1546967445

m13t commented 1 year ago

@crenshaw-dev slightly more complicated for the keys if I remember rightly. Need to find some time to have a proper deep dive and see if I can come up with a working solution.

amanfredi commented 1 year ago

Just wanted to note that I was able to work around this issue and satisfy my use case by reverting to fasttemplate. The behavior I wanted was to infer most of my parameters from the directory structure (git directory generator), but allow overrides on a per-application basis by using values from a config.yaml (git files generator) without requiring all values to be specified in every config.yaml (provide default values).

The generator structure I ended up with to get this to work is:

merge:

MohammedNoureldin commented 9 months ago

As mentioned, this prevents any usage of GoTemplate when used in a manifest with merge generator that includes different types of generators. In my case no workaround would work and I had to revert back to simple-template.

Is there any estimation when this may be fixed?

Thanks!

girstenbrei commented 6 months ago

Greets everyone, I've mentioned the PR #17145 in Slack but haven't heard back.

If you have some capacity, @crenshaw-dev , maybe you can have a look at it or point me in the right direction towards someone to get some feedback. I'd still like to get this feature, unblocking the whole GoTemplate usage in my AppSet-Generators.

Thanks everyone!

Dominic1DL commented 6 months ago

I would be interested in this feature to. Are there any updates on this?