feat(appset): Support more than two child generators for matrix generator

[huynhsontung]

Closes #14182

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).
• [ ] The title of the PR conforms to the Toolchain Guide
• [x] 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.
• [x] Does this PR require documentation updates?
• [x] I've updated documentation as required by this PR.
• [ ] Optional. My organization is added to USERS.md.
• [x] I have signed off all my commits as required by DCO
• [x] I have written unit and/or e2e tests for my change. PRs without these are unlikely to be merged.
• [x] 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.

Please see Contribution FAQs if you have questions about your pull-request.

[crenshaw-dev]

I feel like we should have a controller-level configurable max for number of children in a matrix generator. We're close to having ApplicationSets manageable by non-admins, and it would be good to have a way for admins to mitigate denial of service via bonkers-level matrix generators.

[codecov[bot]]

Codecov Report

Attention: Patch coverage is 68.75000% with 10 lines in your changes missing coverage. Please review.

Project coverage is 44.93%. Comparing base (71e1f30) to head (96fb14e). Report is 29 commits behind head on master.

:exclamation: Current head 96fb14e differs from pull request most recent head 6b83263

Please upload reports for the commit 6b83263 to get more accurate results.

Files	Patch %	Lines
applicationset/generators/matrix.go	78.57%	3 Missing and 3 partials :warning:
...t-controller/commands/applicationset_controller.go	0.00%	4 Missing :warning:

Additional details and impacted files

@@           Coverage Diff           @@
##           master   #14189   +/-   ##
=======================================
  Coverage   44.92%   44.93%           
=======================================
  Files         355      355           
  Lines       47889    47896    +7     
=======================================
+ Hits        21516    21521    +5     
- Misses      23569    23571    +2     
  Partials     2804     2804           

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

[huynhsontung]

@crenshaw-dev Sounds reasonable. There is no precedent for generator configuration at the controller level. How about a controller flag called max-matrix-children with the default of 0 for an unlimited number of children? Unless we want it configurable on the fly through a ConfigMap.

[crenshaw-dev]

I'm wavering between default of 2 (no change of behavior for people upgrading) and default of 0. I think default 2 makes sense if we want people to be able to provide users create/update privileges on appsets as soon as we button down the remaining admin-only features.

We could make it a controller flag/env var and let users set it via argocd-cmd-params-cm.

[huynhsontung]

Do we allow a value of 1? The code does handle this case but intuitively it doesn't really make sense. If not, any value less than 2 will be parsed as unlimited.

Edit: There is already an error when using a matrix generator with only 1 child. Any value less than 2 will be unlimited then.

[huynhsontung]

@crenshaw-dev I have made the max matrix children configurable through NewMatrixGenerator() with the default value of 2. Let me know if there is anything that needs changing before I move on to updating the docs.

[crenshaw-dev]

Sounds reasonable! I'd say carry on with docs.

[huynhsontung]

@crenshaw-dev I updated the manifest to allow setting the limit in the config map and updated the docs.

[huynhsontung]

I had to change those messages a bit to also reflect #14573

[huynhsontung]

@crenshaw-dev Are there any updates on this PR? I can resolve the merge conflicts if it's ready to merge.

[ptr1120]

Hello, is this still expected to be merged for argocd 2.9?

[llavaud]

Hello, is this still expected to be merged for argocd 2.9?

Hello, you mean 2.10 ?

[ehsan310]

any update on this ?

[aburan28]

Any update here?

[jdeus]

What's the ETA on this ?

[ptr1120]

Hello, can we still expect that this support of >2 matrix steps will be reviewed and merged as stated in the roadmap. Since I am having a need for 3 steps and the parameter transfer between nested matrix steps has issues, I am still searching for any solution.

[sathieu]

@crenshaw-dev I have made the max matrix children configurable through NewMatrixGenerator() with the default value of 2. Let me know if there is anything that needs changing before I move on to updating the docs.

Can this default be changed to a higher value? Maybe 100 or 10?

[jdeus]

So are we waiting another year here ?

[huynhsontung]

Can this default be changed to a higher value? Maybe 100 or 10?

@sathieu I chose this default value of 2 to mirror the same limit of the current version of the matrix generator. I'm fine with having a higher value or even unlimited as a default.

[mihaigalos]

Would this not be easier solved using a Plugin Generator? One can then programatically compose the generators the way they're required.

[blakepettersson]

@crenshaw-dev do you still have concerns about this PR?

[crenshaw-dev]

Dismissing my review as stale, since I don't have time to re-review right now. At a glance, no major concerns.

[steve-todorov]

@crenshaw-dev any updates on merging and releasing this feature?