feat: Support nested objects in merge generator using go templates (Issue #12836)

[girstenbrei]

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. (Support nested objects in merge generator #12836)
• The title of the PR states what changed and the related issues number (used for the release note).
• The title of the PR conforms to the Toolchain Guide
• 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.
• 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).
• My new feature complies with the feature status guidelines.
• I have added a brief description of why this PR is necessary and/or what this PR solves.
• Optional. My organization is added to USERS.md.
• Optional. For bug fixes, I've indicated what older releases this fix should be cherry-picked into (this may or may not happen depending on risk/complexity).

Closes #12836

Overview

This PR follows the proposal from the original Issue #12836 . Nothing changes for users with goTemplate: false. Using goTemplate: true makes the mergeKeys valid Go templates. All objects present in a parameter set from a downstream generator can be accessed through this Go Template, as well as all templating functions.

Implementation

Without go templating enabled, nothing changes. The same mechanism is used to index parameter sets:

https://github.com/girstenbrei/argo-cd/blob/master/applicationset/generators/merge.go#L176-L178

With go templating, instead of having the parameter set represented by the value at the merge key for each merge key, the templated value for these keys is used.

https://github.com/girstenbrei/argo-cd/blob/master/applicationset/generators/merge.go#L160-L175

The Good

I think this might just work. No changes in any Files, mergeKeys just support nested objects now the same way golang templates support nested objects.

The Ugly

When is a template a template?

The "heuristic" when a mergeKey is a goTemplate is pretty naive.

https://github.com/girstenbrei/argo-cd/blob/master/applicationset/generators/merge.go#L164-L165

You could set merge Keys containing '{{' && '}}' but they are not go templates, maybe you have a generator generating a template to be filled later and for some reason you need the key to be the template. ATM I'd argue, that deliberate usage of this seems unlikely. Maybe people generate templates in values. No generator in Argo does this in the merge keys.

Map ordering

It might be possible to shoot yourself in the foot with this. Although map ordering is apparently guaranteed in golang templates ¹², this is limited to the case that 'keys are of basic type with a defined order'. Parameter sets are of type map[string]interface{} which should always make them fall into this category, I'm just not sure.

Footnotes

1. https://forum.golangbridge.org/t/text-template-sort-map/5688 ↩

2. https://pkg.go.dev/text/template#hdr-Actions ↩

[girstenbrei]

So after reading the original issues comments a little more carefully, I also tried the suggested flattening from this comment.

I'm not totally against this approach, but it also has it's problems to consider:

Hierarchy Collision

Assume the following parameters generated:

- values:
    name: "a"
- values.name: "a"

The flatten approach generates the same maps for both of these, equal to the second example. The merge generator would then consider those maps to be equal and if used as merge key, merge on them.

This might be a problem, depending on the expected outcome. But especially for setups fetching values from some users, this makes these users now responsible thinking about the equality of those values. I personally would avoid having values in such a structure, it seems confusing, but in a more complex settings this might just be the case. And in a more complex case, I personally would prefer the merging to be strictly correct instead of silently merging those.

The go template version has this problem, too:

- values:
    name: "a"
- values: "map[name: a]"

These equal for the merge key {{ .values }}. Using a simple values this is now solved by prepending the type.

Key Collision

Assume the following parameter generated:

- values:
    name: "one"
  values.name: "two"

Flattening simply starts flattening with the first key, but the second key generating the same flat key overwrites the first value, making the resulting map contain only values.name: "two".

As with hierarchy collisions, IMHO I'd try to avoid these confusing names. But, a more complex scenario might have them from user provided input. And again, I personally would prefer the merging to be strictly correct instead of silently assuming those are equal.

The go template version does not have this problem as it will insert type information.

Complex Value Keys

Assume the following parameter generated:

- cluster: "some-cluster-a"
  values:
    location: "germany"
    gpus: "many"
-  app_name: "my-ai-app"
   values:
     location: "germany"
     gpus: "many"

This might be a use case for a single merge key, values. But after flattening those, the result actually does not contain a key "values", it contains only the two keys "values.location" and "values.gpus". To make merging on complex values work, we'd have to re-run through the parameter set and find all complex values and re-add them back into the map.

I'm aware, that this might also be an error prone use-case. If the cluster gains a label, the app suddenly does not match any more, but with templating the individual fields in the merge key, the ApplicationSet author is in control of that.

The go template version does not have this problem as the source information is not flattened.

[remicode]

Hello everyone, Is there any idea yet of which version this fix might land in?

[girstenbrei]

Hey @remicode ! Thx for the question. I did the initial implementation and then got busy with other stuff, so the rest of communicating this fell along the wayside. I assume, this PR has been under the radar since then.

I still think the implementation idea is ok and I'd love to get feedback for it and bring this issue into a release version.

I'm not a member of any team from ArgoCD, I think I can fit pinging them about this in after easter. Depending on their capacity, it might still take a while, though.

[remicode]

Thanks for the update @girstenbrei. When you do ping someone from the team, if there is anything I can do to help let me know and i'll be happy to pitch in.

[agaudreault]

Related to #13118

[dakr0013]

I am looking forward to have this feature in argocd. when will it be merged?

[iNoahNothing]

@girstenbrei

Any chance of reviving this? I would be happy to put in some work if we have a path forward. Being able to use goTemplate: true with nested objects would really so I can use templatePatch would really make my life a lot easier.

[girstenbrei]

@iNoahNothing Hi there!

Thank you for the interest, the hold up is the following: using nested objects in the merge-keys field technically changes the semantics of this field. Previously, a field with some.thing would try to merge on a key some.thing. Enabling nesting would change this to merge on a key thing inside the value of some. So technically this is not a minor change. There might be more of such 'breaking' cases.

But, on the other hand, one can argue that this is how people actually expect this to work (at least I did) and the number of people who actually have keys in with a dot in them will be low. So the next steps discussed where to write up the exact cases that this change would break to be able to judge the involved risk. Ideally those also would be test-cases.

That's where I stopped due to missing capacity. If you have resources, it would be great if you could jump in for the write up and / or test cases. I also adjusted docs a little, but a second opinion on edge cases would be great, those are easily missed.

Hope I could help,
Christoph

[codecov]

Codecov Report

Attention: Patch coverage is 93.02326% with 3 lines in your changes missing coverage. Please review.

Project coverage is 60.05%. Comparing base (610523b) to head (f98faef).
Report is 40 commits behind head on master.

Files with missing lines	Patch %	Lines
applicationset/generators/merge.go	93.02%	2 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master   #17145      +/-   ##
==========================================
+ Coverage   60.03%   60.05%   +0.01%     
==========================================
  Files         344      343       -1     
  Lines       57781    57881     +100     
==========================================
+ Hits        34689    34758      +69     
- Misses      20333    20351      +18     
- Partials     2759     2772      +13     

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

[girstenbrei]

Greets everyone,
with the upcoming 3.0 release, I would like to try to revive this PR.
Especially to unblock #10858 and #19985, this would be nice.

Blockers

The main blocker has been, that switching to goTemplating for the merge keys
changes semantics in the following way.

Merge keys with dots

Previously, if your merge key contained dots, they would
be interpreted as a single key with a dot in the middle.

mergeKeys:
  - "company.org/config"
generators:
  - list:
      elements:
      - "company.org/config": "marker"
      - company:
          org/config: "A"
  - list:
      elements:
      - "company.org/config": "marker"
      - company:
          org/config: "B"

If we switch this to goTemplates, it will not merge any more. However, currently
the list and plugin generator seem to be the only generators able to generate top
level keys containing dots. As it has been not possible to merge on other keys than
top level keys, only top level keys can have this problem. For people not using
the plugin generator, list-to-list merges the only place this is a real problem.
With the plugin generator, top level keys with dots might be arbitrary important.

Updates

Changes

I updated the implementation, tests and docs for how merge keys are interpreted.
The list elements are interpreted as goTemplate keys only if the regex
match a dot-separated concatenation of valid goTemplate variable names.

This includes simple keys compatible with fasttemplate like:

  - key
  - nested.key

It should enable easier migration from fasttemplate to goTemplates, simple keys
don't need to change at all.

This (AFAIK) should be strictly correct now. Every auto-converted key is a valid
go template and is type safe.

Invariants

The following assumptions need to hold. I tried to verify them as best as I
can, but if you find a counterexample, please let me know, then this whole
PR has a problem.

• No generator except for the list and plugin generator produces top level keys
  containing . (dot) and - (dash).
• Fasttemplate variable names don't allow special characters in them and never start
  with a . (dot).

Next steps

Migration documentation is still missing. Where should I write up this?

It's main component should be to inform users about the need to use the index
function if they need to use top level keys with dots.

[amanfredi]

@crenshaw-dev any chance of getting this in an upcoming release?

[tennengabr]

@crenshaw-dev any chance of getting this in an upcoming release?

this would be awesome.