feat: create cli commands for ApplicationSet #9584

ishitasequeira · 2022-06-05T19:09:46Z

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:

codecov · 2022-06-07T15:43:19Z

Codecov Report

Merging #9584 (b329e4d) into master (9fac0f6) will decrease coverage by 0.38%.
The diff coverage is 17.30%.

@@            Coverage Diff             @@
##           master    #9584      +/-   ##
==========================================
- Coverage   45.69%   45.31%   -0.39%     
==========================================
  Files         234      236       +2     
  Lines       28508    28827     +319     
==========================================
+ Hits        13027    13063      +36     
- Misses      13694    13973     +279     
- Partials     1787     1791       +4

Impacted Files	Coverage Δ
applicationset/controllers/clustereventhandler.go	`69.56% <ø> (ø)`
applicationset/generators/cluster.go	`77.90% <ø> (ø)`
applicationset/generators/duck_type.go	`68.62% <ø> (ø)`
...licationset/generators/generator_spec_processor.go	`61.64% <ø> (ø)`
applicationset/generators/git.go	`85.96% <ø> (ø)`
applicationset/generators/list.go	`62.50% <ø> (ø)`
applicationset/generators/matrix.go	`70.73% <ø> (ø)`
applicationset/generators/merge.go	`59.04% <ø> (ø)`
applicationset/generators/pull_request.go	`42.85% <ø> (ø)`
applicationset/generators/scm_provider.go	`35.23% <ø> (ø)`
... and 19 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

pkg/apis/applicationset/register.go

applicationset/controllers/applicationset_controller.go

cmd/argocd/commands/applicationset.go

server/applicationset/applicationset.go

crenshaw-dev

So I think the big question we have to answer is: should we require all write-type ApplicationSet API requests to have an un-templated spec.template.spec.project field, or can we accept templated Project fields?

I think when @leoluz and @ishitasequeira and @jgwest and I discussed it last, we had settled on disallowing write-type API requests when the AppSet Project field was templated. But I think we can eliminate that requirement.

My proposal:

Before each RBAC check, run fasttemplate over the Project field and replace each template with an asterisk.
Perform the RBAC check for the now-templated project fields.

For example, suppose I have RBAC like this:

p, appset-dev-maintainers, applicationsets, update, *-dev/*, allow

If a user tries to update an AppSet like this:

metadata:
  name: apis-appset
spec:
  template:
    spec:
      project: "{{path[0]}}-dev"

Then the following RBAC check would happen:

*-dev/apis-appset # passes because it matches the first rule

I don't think this would be too much additional code, and I think it would make the API support for ApplicationSets much more powerful. Thoughts?

util/argo/argo.go

cmd/argocd/commands/applicationset.go

manifests/crds/applicationset-crd.yaml

pkg/apis/applicationset/register.go

crenshaw-dev · 2022-08-03T21:38:32Z

After chatting with Leo... ignore my last comment, I think that can be introduced in a later PR. For now, let's just reject AppSet API requests where the project field is templated.

crenshaw-dev · 2022-08-03T21:40:56Z

The other comment I had was that maybe we should just skip the Application RBAC checks and instead document very thoroughly that granting write capabilities on applicationsets grants the ability to do a bunch of things with applications by proxy.

crenshaw-dev

Ran into a few issues while testing the CLI.

cmd/argocd/commands/applicationset.go

Going on vacation

jannfis

First round of code review, just looking at the code. I have not checked this out and tested locally.

cmd/argocd/commands/app.go

cmd/argocd/commands/applicationset.go

jannfis · 2022-08-29T16:48:01Z

cmd/util/applicationset.go

+}
+
+func readAppset(yml []byte, appsets *[]*argoprojiov1alpha1.ApplicationSet) error {
+	yamls, _ := kube.SplitYAMLToString(yml)


Not sure if the second return value is of type error, but if it is, is there any reason we ignore it?

As I was checking if the yaml has been sent or not and sending an error before it reaches here would suffice the check. But I guess after re-evaluating I feel return an error makes sense. I will update this.

cmd/util/applicationset.go

jannfis · 2022-08-29T17:46:45Z

server/applicationset/applicationset.go

+	project, err := s.validateAppSet(ctx, appset)
+	if err != nil {
+		return nil, fmt.Errorf("error validating ApplicationSets: %w", err)
+	}


I think this should be slightly modified, because in it's current form, it would allow enumeration of AppProjects without proper permissions.

In s.validateAppSet(), it is tried to retrieve the AppProject from the K8s API referenced in the ApplicationSet resource. If this is not found, it will be returned to the caller without having checked proper privileges before.

I think it would make sense to:

Refactor validateAppSet to return the project name instead of the project

Perform the permissions check using this string, instead of the AppProject resource

After this has been validated, get the AppProject from the API in this function

jannfis · 2022-08-29T19:40:14Z

util/appset/appset.go

+	return fmt.Sprintf("%s/%s", appSet.Spec.Template.Spec.GetProject(), appSet.ObjectMeta.Name)
+}
+
+func RunCommand(command v1alpha1.Command) (string, error) {


What is this used for? And where?

I was using this for my prior approach. I missed removing this part of the code. Will remove it, thanks.

jannfis · 2022-08-29T19:44:10Z

util/argo/argo.go

@@ -90,6 +109,21 @@ func FilterByName(apps []argoappv1.Application, name string) ([]argoappv1.Applic
 	return items, status.Errorf(codes.NotFound, "application '%s' not found", name)
 }

+// FilterAppSetsByName returns an applicationset
+func FilterAppSetsByName(appsets []argoappv1.ApplicationSet, name string) ([]argoappv1.ApplicationSet, error) {


Looking at this function, I'm wondering why it returns a slice, when there can be either one or none results?

I am using the same object newItems everywhere. First in FilterAppSetsByName and then for FilterAppSetsByProjects. FilterAppSetsByName would give one, but if q.name is empty, it would not enter that condition and directly move to FilterAppSetsByProjects

https://github.com/argoproj/argo-cd/blob/23bb5a42d9f14c6d4df0fe3353fbf6b8d028ac3f/server/applicationset/applicationset.go#L120-L128

Yeah, using a list here is convenient... but I think the List method shouldn't even accept a name "filter" (since the user should probably just use the Get endpoint).

server/applicationset/applicationset.go

crenshaw-dev

Since we're introducing a new resource, we should add a note to the 2.4->2.5 upgrade notes warning people that the meaning of * in the resource column of their RBAC CSV is being expanded. The note can follow the model of the one for the new exec resource in 2.4: https://github.com/argoproj/argo-cd/blob/master/docs/operator-manual/upgrading/2.3-2.4.md#configure-rbac-to-account-for-new-exec-resource

crenshaw-dev · 2022-09-09T00:06:35Z

Conditions are pretty verbose for applicationsets. I wonder if we should trim down what we report in the wide format.

$ dist/argocd appset list
NAME       NAMESPACE  PROJECT  SYNCPOLICY  CONDITIONS                                                                                                                                                                                                                                                                                                                                                                       REPO                                     PATH                                                          TARGET
guestbook  argocd     default  nil         [{ErrorOccurred Successfully generated parameters for all Applications 2022-09-08 20:05:18 -0400 EDT False ApplicationSetUpToDate} {ParametersGenerated Successfully generated parameters for all Applications 2022-09-08 20:05:18 -0400 EDT True ParametersGenerated} {ResourcesUpToDate ApplicationSet up to date 2022-09-08 20:05:18 -0400 EDT True ApplicationSetUpToDate}]  https://github.com/argoproj/argo-cd.git  applicationset/examples/list-generator/guestbook/{{cluster}}  HEAD

crenshaw-dev · 2022-09-09T00:07:49Z

The get output is a bit misleading, without the true/false field. Makes it look like an error occurred, when it did not:

$ dist/argocd appset get guestbook
Name:               guestbook
Project:            default
Server:             {{url}}
Namespace:          guestbook
Repo:               https://github.com/argoproj/argo-cd.git
Target:             HEAD
Path:               applicationset/examples/list-generator/guestbook/{{cluster}}
SyncPolicy:         <none>

CONDITION            MESSAGE                                                 LAST TRANSITION
ErrorOccurred        Successfully generated parameters for all Applications  2022-09-08 20:05:18 -0400 EDT
ParametersGenerated  Successfully generated parameters for all Applications  2022-09-08 20:05:18 -0400 EDT
ResourcesUpToDate    ApplicationSet up to date                               2022-09-08 20:05:18 -0400 EDT

crenshaw-dev · 2022-09-09T00:10:20Z

@ishitasequeira I opened a PR with some of the small issues I noticed while testing today. Overall, things are looking very good.

Main thing is adding k8s RBAC so the server deployment can manage appsets on behalf of the user.

https://github.com/ishitasequeira/argo-cd/pull/3

ishitasequeira · 2022-09-09T02:57:20Z

The get output is a bit misleading, without the true/false field. Makes it look like an error occurred, when it did not:

$ dist/argocd appset get guestbook
Name:               guestbook
Project:            default
Server:             {{url}}
Namespace:          guestbook
Repo:               https://github.com/argoproj/argo-cd.git
Target:             HEAD
Path:               applicationset/examples/list-generator/guestbook/{{cluster}}
SyncPolicy:         <none>

CONDITION            MESSAGE                                                 LAST TRANSITION
ErrorOccurred        Successfully generated parameters for all Applications  2022-09-08 20:05:18 -0400 EDT
ParametersGenerated  Successfully generated parameters for all Applications  2022-09-08 20:05:18 -0400 EDT
ResourcesUpToDate    ApplicationSet up to date                               2022-09-08 20:05:18 -0400 EDT

I agree. I think we should add true/false fields as well here. I will add them.

ishitasequeira · 2022-09-09T03:02:36Z

Conditions are pretty verbose for applicationsets. I wonder if we should trim down what we report in the wide format.

$ dist/argocd appset list
NAME       NAMESPACE  PROJECT  SYNCPOLICY  CONDITIONS                                                                                                                                                                                                                                                                                                                                                                       REPO                                     PATH                                                          TARGET
guestbook  argocd     default  nil         [{ErrorOccurred Successfully generated parameters for all Applications 2022-09-08 20:05:18 -0400 EDT False ApplicationSetUpToDate} {ParametersGenerated Successfully generated parameters for all Applications 2022-09-08 20:05:18 -0400 EDT True ParametersGenerated} {ResourcesUpToDate ApplicationSet up to date 2022-09-08 20:05:18 -0400 EDT True ApplicationSetUpToDate}]  https://github.com/argoproj/argo-cd.git  applicationset/examples/list-generator/guestbook/{{cluster}}  HEAD

I was wondering if we could show only conditions having true as value. WDYT?

ishitasequeira · 2022-09-09T03:03:30Z

@ishitasequeira I opened a PR with some of the small issues I noticed while testing today. Overall, things are looking very good.

Main thing is adding k8s RBAC so the server deployment can manage appsets on behalf of the user.

ishitasequeira#3

Thanks @crenshaw-dev for the PR. I will go through it and merge the changes.

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

Signed-off-by: CI <michael@crenshaw.dev> Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

Signed-off-by: CI <michael@crenshaw.dev> Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

crenshaw-dev

lgtm, thanks @ishitasequeira for putting so much work into this!

@jannfis do you want to give it another pass before merging?

renperez-cpi · 2022-09-11T05:08:41Z

@ishitasequeira thank you for this. do you know when this pr will be merged? would love to give this a spin :)

jannfis

LGTM. Thanks @ishitasequeira!

And thanks @crenshaw-dev for giving me opportunity to look over it again!

* feat: add applicationset cli commands Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * squashed commits and rebased with master Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * addressed comments Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * fix lint errors Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * Retrigger CI pipeline Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * Retrigger CI pipeline Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * feat: add applicationset cli commands Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * addressed comments Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * fix lint errors Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * Retrigger CI pipeline Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * Retrigger CI pipeline Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * Addressed PR comments Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * removed duplicate imports Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * fix CI errors Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * address PR comments Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * add k8s RBAC, docs tweaks Signed-off-by: CI <michael@crenshaw.dev> Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * rebase master branch Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * Addressed PR coments Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * remove unnecessary fields, add docs Signed-off-by: CI <michael@crenshaw.dev> Signed-off-by: ishitasequeira <ishiseq29@gmail.com> * fix unit test Signed-off-by: ishitasequeira <ishiseq29@gmail.com> Signed-off-by: ishitasequeira <ishiseq29@gmail.com> Signed-off-by: CI <michael@crenshaw.dev> Co-authored-by: CI <michael@crenshaw.dev>

ishitasequeira changed the title ~~feat: create cli commands for ApplicationSet~~ [NOT READY FOR REVIEW] feat: create cli commands for ApplicationSet Jun 5, 2022

ishitasequeira changed the title ~~[NOT READY FOR REVIEW] feat: create cli commands for ApplicationSet~~ feat: create cli commands for ApplicationSet Jun 5, 2022

ishitasequeira changed the title ~~feat: create cli commands for ApplicationSet~~ feat: create cli commands for ApplicationSet [WIP] Jun 7, 2022

ishitasequeira changed the title ~~feat: create cli commands for ApplicationSet [WIP]~~ [WIP] feat: create cli commands for ApplicationSet Jun 7, 2022

ishitasequeira requested review from crenshaw-dev and jgwest July 21, 2022 13:23

ishitasequeira changed the title ~~[WIP] feat: create cli commands for ApplicationSet~~ feat: create cli commands for ApplicationSet Jul 22, 2022

ishitasequeira marked this pull request as ready for review July 22, 2022 12:59

ishitasequeira commented Jul 25, 2022

View reviewed changes

pkg/apis/applicationset/register.go Outdated Show resolved Hide resolved

ishitasequeira requested review from alexmt and jannfis July 25, 2022 06:40

crenshaw-dev requested changes Jul 27, 2022

View reviewed changes

crenshaw-dev previously requested changes Aug 3, 2022

View reviewed changes

crenshaw-dev reviewed Aug 19, 2022

View reviewed changes

ishitasequeira added this to the v2.5 milestone Aug 25, 2022

jannfis reviewed Aug 29, 2022

View reviewed changes

ishitasequeira requested a review from jannfis September 1, 2022 14:56

crenshaw-dev requested changes Sep 6, 2022

View reviewed changes

ishitasequeira requested a review from crenshaw-dev September 7, 2022 13:47

crenshaw-dev requested changes Sep 8, 2022

View reviewed changes

ishitasequeira and others added 19 commits September 9, 2022 14:17

squashed commits and rebased with master

a006308

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

addressed comments

8c82eb1

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

fix lint errors

a9916ef

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

Retrigger CI pipeline

753c767

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

Retrigger CI pipeline

c91b8fa

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

feat: add applicationset cli commands

55dcd09

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

addressed comments

ac0917a

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

fix lint errors

c829fcf

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

Retrigger CI pipeline

f2f3e11

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

Retrigger CI pipeline

4cac73c

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

Addressed PR comments

45b68fe

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

removed duplicate imports

ae37391

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

fix CI errors

0800fe9

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

address PR comments

c6bdd6f

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

add k8s RBAC, docs tweaks

cc7ffd6

Signed-off-by: CI <michael@crenshaw.dev> Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

rebase master branch

816972e

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

Addressed PR coments

6e47c6f

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

remove unnecessary fields, add docs

a61ce36

Signed-off-by: CI <michael@crenshaw.dev> Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

fix unit test

b329e4d

Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

crenshaw-dev approved these changes Sep 9, 2022

View reviewed changes

jannfis approved these changes Sep 14, 2022

View reviewed changes

jannfis merged commit e53ab49 into argoproj:master Sep 14, 2022

xmath279 mentioned this pull request Oct 20, 2022

Support manage ApplicationSet using Argo CD UI #7352

Open

jcogilvie mentioned this pull request Dec 13, 2022

Revisit support for ApplicationSet argoproj-labs/terraform-provider-argocd#223

Closed

morey-tech mentioned this pull request Jun 6, 2023

the Argo CD API does not currently support creating ApplicationSets with templated project fields" #13665

Open

crenshaw-dev mentioned this pull request Jul 24, 2023

REQUEST: Promotion to Approver for @ishitasequeira argoproj/argoproj#224

Closed

6 tasks

bryantbiggs mentioned this pull request Oct 31, 2023

Cannot generate ApplicationSets clowdhaus/argo-cd-action#23

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: create cli commands for ApplicationSet #9584

feat: create cli commands for ApplicationSet #9584

ishitasequeira commented Jun 5, 2022 •

edited by crenshaw-dev

Loading

codecov bot commented Jun 7, 2022 •

edited

Loading

crenshaw-dev left a comment •

edited

Loading

crenshaw-dev commented Aug 3, 2022

crenshaw-dev commented Aug 3, 2022

crenshaw-dev left a comment

jannfis left a comment

jannfis Aug 29, 2022

ishitasequeira Aug 30, 2022

jannfis Aug 29, 2022

jannfis Aug 29, 2022

ishitasequeira Aug 30, 2022

jannfis Aug 29, 2022

ishitasequeira Aug 30, 2022

ishitasequeira Aug 30, 2022

crenshaw-dev Sep 9, 2022

crenshaw-dev left a comment

crenshaw-dev commented Sep 9, 2022

crenshaw-dev commented Sep 9, 2022

crenshaw-dev commented Sep 9, 2022 •

edited

Loading

ishitasequeira commented Sep 9, 2022

ishitasequeira commented Sep 9, 2022

ishitasequeira commented Sep 9, 2022

crenshaw-dev left a comment

renperez-cpi commented Sep 11, 2022

jannfis left a comment

feat: create cli commands for ApplicationSet #9584

feat: create cli commands for ApplicationSet #9584

Conversation

ishitasequeira commented Jun 5, 2022 • edited by crenshaw-dev Loading

codecov bot commented Jun 7, 2022 • edited Loading

Codecov Report

crenshaw-dev left a comment • edited Loading

Choose a reason for hiding this comment

crenshaw-dev commented Aug 3, 2022

crenshaw-dev commented Aug 3, 2022

crenshaw-dev left a comment

Choose a reason for hiding this comment

jannfis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

crenshaw-dev left a comment

Choose a reason for hiding this comment

crenshaw-dev commented Sep 9, 2022

crenshaw-dev commented Sep 9, 2022

crenshaw-dev commented Sep 9, 2022 • edited Loading

ishitasequeira commented Sep 9, 2022

ishitasequeira commented Sep 9, 2022

ishitasequeira commented Sep 9, 2022

crenshaw-dev left a comment

Choose a reason for hiding this comment

renperez-cpi commented Sep 11, 2022

jannfis left a comment

Choose a reason for hiding this comment

ishitasequeira commented Jun 5, 2022 •

edited by crenshaw-dev

Loading

codecov bot commented Jun 7, 2022 •

edited

Loading

crenshaw-dev left a comment •

edited

Loading

crenshaw-dev commented Sep 9, 2022 •

edited

Loading