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

huynhsontung · 2023-06-23T19:42:24Z

Checklist:

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

crenshaw-dev · 2023-06-23T19:46:09Z

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 · 2023-06-23T23:29:26Z

Codecov Report

Attention: Patch coverage is 86.04651% with 6 lines in your changes missing coverage. Please review.

Please upload report for BASE (master@40c6077). Learn more about missing BASE report.
Report is 318 commits behind head on master.

Files with missing lines	Patch %	Lines
applicationset/generators/matrix.go	82.35%	3 Missing and 3 partials ⚠️

Additional details and impacted files

@@            Coverage Diff            @@
##             master   #14189   +/-   ##
=========================================
  Coverage          ?   55.80%           
=========================================
  Files             ?      320           
  Lines             ?    44411           
  Branches          ?        0           
=========================================
  Hits              ?    24782           
  Misses            ?    17066           
  Partials          ?     2563

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

huynhsontung · 2023-06-23T23:37:38Z

@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 · 2023-06-23T23:44:35Z

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 · 2023-06-24T00:01:37Z

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 · 2023-06-24T04:11:56Z

@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 · 2023-06-26T13:58:57Z

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

Signed-off-by: Tung Huynh <[email protected]>

huynhsontung · 2023-07-01T19:20:52Z

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

Signed-off-by: Tung Huynh <[email protected]>

crenshaw-dev

Thanks for your patience on the review! Small things. Targeting this for 2.9.

applicationset/generators/matrix.go

Signed-off-by: Tung Huynh <[email protected]>

huynhsontung · 2023-08-24T07:56:21Z

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

huynhsontung · 2023-11-27T21:38:21Z

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

ishitasequeira

One minor comment. Overall code LGTM!!

ishitasequeira · 2023-08-30T02:37:49Z

applicationset/generators/matrix.go

 		return nil, ErrLessThanTwoGenerators
 	}

-	if len(appSetGenerator.Matrix.Generators) > 2 {
-		return nil, ErrMoreThanTwoGenerators
+	if m.maxChildren > 1 && numGens > m.maxChildren {


It might be useful to log a separate configuration error if value of maxChildren is 1 or less.

When maxChildren is 1 or less, it's considered no limit. The default value for maxChildren is 2.

Not a showstopper but that sounds un-intuitive IMO - could we not say that no limit only applies if maxChildren < 0?

I would prefer to have maxChildren < 1 to avoid using negative values. The param reposerver.git.lsremote.parallelism.limit has the same logic where any values less than 1 mean unlimited.

plus we don't have to update the docs and tests this way

jdeus · 2024-03-18T17:44:08Z

What's the ETA on this ?

ptr1120 · 2024-03-27T06:55:03Z

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 · 2024-05-16T11:54:45Z

@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 · 2024-05-16T15:06:34Z

So are we waiting another year here ?

huynhsontung · 2024-05-16T17:54:00Z

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 · 2024-05-17T06:06:19Z

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

blakepettersson

Minor comment, but otherwise LGTM

Signed-off-by: Tung Huynh <[email protected]>

blakepettersson · 2024-06-11T20:11:02Z

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

stale

crenshaw-dev · 2024-07-02T15:12:06Z

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

steve-todorov · 2024-08-04T17:04:55Z

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

it2008018 · 2024-09-20T09:06:33Z

Is there any chance of merging this PR? we are looking for this feature to be available asap.

blakepettersson · 2024-09-20T10:02:58Z

@it2008018 for that to happen, the merge conflicts need to be fixed from @huynhsontung.

Once that happens, we can push for someone to review and merge it.

Even if this gets merged tomorrow, this would not be "available asap" since this would come into 2.14, which is slated for next February.

it2008018 · 2024-09-20T10:36:59Z

@blakepettersson will look forward for it to be merged :) @huynhsontung will it be possible for you to resolve conflicts?

blakepettersson · 2024-09-20T10:53:39Z

@huynhsontung if you do not have time to work on it, I can take on the mantle and get the necessary reviews for it to be merged.

Signed-off-by: Tung Huynh <[email protected]>

huynhsontung · 2024-09-21T07:38:19Z

server/applicationset/applicationset.go

-	appSetGenerators := generators.GetGenerators(ctx, s.client, s.k8sClient, namespace, argoCDService, s.dynamicClient, scmConfig)
+	appSetGenerators := generators.GetGenerators(ctx, s.client, s.k8sClient, namespace, argoCDService, s.dynamicClient, scmConfig, matrixConfig)


I don't know how to get the config in this case. This method is used in argocd appset create --dry-run and argocd appset generate. I think not setting a limit here makes sense.

huynhsontung · 2024-09-21T07:40:16Z

@blakepettersson I have resolved all the conflicts

huynhsontung changed the title ~~Support more than two child generators for matrix generator~~ feat: Support more than two child generators for matrix generator Jun 23, 2023

huynhsontung changed the title ~~feat: Support more than two child generators for matrix generator~~ feat(appset): Support more than two child generators for matrix generator Jun 23, 2023

huynhsontung force-pushed the matrix-gen-improve branch from 0621f87 to fdd0e1c Compare June 23, 2023 23:15

huynhsontung added 6 commits July 1, 2023 12:12

support more than two child generators for matrix generator

fc4eac8

Signed-off-by: Tung Huynh <[email protected]>

update matrix generator tests

fb3168c

Signed-off-by: Tung Huynh <[email protected]>

configurable max number of children for matrix generator

0bc1b21

Signed-off-by: Tung Huynh <[email protected]>

make env var configurable in configmap

2018f00

Signed-off-by: Tung Huynh <[email protected]>

update cmd-params-cm example

09a1087

Signed-off-by: Tung Huynh <[email protected]>

update the matrix generator docs

5b77ec7

Signed-off-by: Tung Huynh <[email protected]>

huynhsontung force-pushed the matrix-gen-improve branch from e72dfab to 5b77ec7 Compare July 1, 2023 19:14

huynhsontung marked this pull request as ready for review July 1, 2023 19:16

huynhsontung added 2 commits July 1, 2023 13:34

run codegen

4f741db

Signed-off-by: Tung Huynh <[email protected]>

Merge branch 'master' into matrix-gen-improve

66aadb8

crenshaw-dev previously requested changes Aug 23, 2023

View reviewed changes

applicationset/generators/matrix.go Outdated Show resolved Hide resolved

applicationset/generators/matrix.go Outdated Show resolved Hide resolved

huynhsontung added 2 commits August 24, 2023 00:12

update error messages

0806a38

Signed-off-by: Tung Huynh <[email protected]>

Merge branch 'master' into matrix-gen-improve

a57a87e

ptr1120 mentioned this pull request Nov 27, 2023

Matrix generator should accept more than two child generators #14182

Open

ishitasequeira reviewed Dec 14, 2023

View reviewed changes

Merge branch 'master' into matrix-gen-improve

902a044

huynhsontung requested a review from a team as a code owner December 16, 2023 06:15

Merge branch 'master' into matrix-gen-improve

852d3a3

blakepettersson approved these changes Jun 5, 2024

View reviewed changes

blakepettersson and others added 3 commits June 5, 2024 13:10

Merge branch 'master' into matrix-gen-improve

96fb14e

return config error if max num generators equals to 1

f5e45e2

Signed-off-by: Tung Huynh <[email protected]>

update params comment

6b83263

Signed-off-by: Tung Huynh <[email protected]>

christianh814 approved these changes Jul 2, 2024

View reviewed changes

huynhsontung added 3 commits September 20, 2024 23:40

Merge branch 'master' into matrix-gen-improve

7702f43

Signed-off-by: Tung Huynh <[email protected]>

create MatrixConfig struct instead of passing maxChildren around

74fb0f9

Signed-off-by: Tung Huynh <[email protected]>

run lint fix

05ab025

Signed-off-by: Tung Huynh <[email protected]>

huynhsontung commented Sep 21, 2024

View reviewed changes

christianh814 approved these changes Sep 21, 2024

View reviewed changes

christianh814 requested a review from ishitasequeira September 24, 2024 19:51

christianh814 approved these changes Sep 24, 2024

View reviewed changes

nitishfy approved these changes Sep 25, 2024

View reviewed changes

nitishfy mentioned this pull request Oct 25, 2024

REQUEST: New membership for @nitishfy argoproj/argoproj#334

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

huynhsontung commented Jun 23, 2023 •

edited

Loading

crenshaw-dev commented Jun 23, 2023

codecov bot commented Jun 23, 2023 •

edited

Loading

huynhsontung commented Jun 23, 2023 •

edited

Loading

crenshaw-dev commented Jun 23, 2023

huynhsontung commented Jun 24, 2023 •

edited

Loading

huynhsontung commented Jun 24, 2023

crenshaw-dev commented Jun 26, 2023

huynhsontung commented Jul 1, 2023

crenshaw-dev left a comment

huynhsontung commented Aug 24, 2023

huynhsontung commented Nov 27, 2023

ishitasequeira left a comment

ishitasequeira Aug 30, 2023

huynhsontung Dec 16, 2023

blakepettersson Jun 5, 2024 •

edited

Loading

huynhsontung Jun 5, 2024 •

edited

Loading

jdeus commented Mar 18, 2024

ptr1120 commented Mar 27, 2024

sathieu commented May 16, 2024

jdeus commented May 16, 2024

huynhsontung commented May 16, 2024

mihaigalos commented May 17, 2024

blakepettersson left a comment

blakepettersson commented Jun 11, 2024

crenshaw-dev commented Jul 2, 2024

steve-todorov commented Aug 4, 2024

it2008018 commented Sep 20, 2024

blakepettersson commented Sep 20, 2024

it2008018 commented Sep 20, 2024 •

edited

Loading

blakepettersson commented Sep 20, 2024

huynhsontung Sep 21, 2024 •

edited

Loading

huynhsontung commented Sep 21, 2024

		appSetGenerators := generators.GetGenerators(ctx, s.client, s.k8sClient, namespace, argoCDService, s.dynamicClient, scmConfig)
		appSetGenerators := generators.GetGenerators(ctx, s.client, s.k8sClient, namespace, argoCDService, s.dynamicClient, scmConfig, matrixConfig)

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

Are you sure you want to change the base?

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

Conversation

huynhsontung commented Jun 23, 2023 • edited Loading

crenshaw-dev commented Jun 23, 2023

codecov bot commented Jun 23, 2023 • edited Loading

Codecov Report

huynhsontung commented Jun 23, 2023 • edited Loading

crenshaw-dev commented Jun 23, 2023

huynhsontung commented Jun 24, 2023 • edited Loading

huynhsontung commented Jun 24, 2023

crenshaw-dev commented Jun 26, 2023

huynhsontung commented Jul 1, 2023

crenshaw-dev left a comment

Choose a reason for hiding this comment

huynhsontung commented Aug 24, 2023

huynhsontung commented Nov 27, 2023

ishitasequeira left a comment

Choose a reason for hiding this comment

ishitasequeira Aug 30, 2023

Choose a reason for hiding this comment

huynhsontung Dec 16, 2023

Choose a reason for hiding this comment

blakepettersson Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

huynhsontung Jun 5, 2024 • edited Loading

Choose a reason for hiding this comment

jdeus commented Mar 18, 2024

ptr1120 commented Mar 27, 2024

sathieu commented May 16, 2024

jdeus commented May 16, 2024

huynhsontung commented May 16, 2024

mihaigalos commented May 17, 2024

blakepettersson left a comment

Choose a reason for hiding this comment

blakepettersson commented Jun 11, 2024

crenshaw-dev commented Jul 2, 2024

steve-todorov commented Aug 4, 2024

it2008018 commented Sep 20, 2024

blakepettersson commented Sep 20, 2024

it2008018 commented Sep 20, 2024 • edited Loading

blakepettersson commented Sep 20, 2024

huynhsontung Sep 21, 2024 • edited Loading

Choose a reason for hiding this comment

huynhsontung commented Sep 21, 2024

huynhsontung commented Jun 23, 2023 •

edited

Loading

codecov bot commented Jun 23, 2023 •

edited

Loading

huynhsontung commented Jun 23, 2023 •

edited

Loading

huynhsontung commented Jun 24, 2023 •

edited

Loading

blakepettersson Jun 5, 2024 •

edited

Loading

huynhsontung Jun 5, 2024 •

edited

Loading

it2008018 commented Sep 20, 2024 •

edited

Loading

huynhsontung Sep 21, 2024 •

edited

Loading