feat: add permitOnlyProjectScopedClusters flag #10237

blakepettersson · 2022-08-08T11:50:02Z

This commit adds a new flag, permitOnlyProjectScopedClusters, which
prevents any application from syncing to clusters which are not a part
of the same project. Fixes #10220.

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:

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).
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.
Optional. My organization is added to USERS.md.
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).

codecov · 2022-08-08T12:04:27Z

Codecov Report

Merging #10237 (35174e4) into master (2fb2c23) will decrease coverage by 0.03%.
The diff coverage is 32.30%.

@@            Coverage Diff             @@
##           master   #10237      +/-   ##
==========================================
- Coverage   45.89%   45.85%   -0.04%     
==========================================
  Files         229      229              
  Lines       28299    28347      +48     
==========================================
+ Hits        12987    12999      +12     
- Misses      13539    13565      +26     
- Partials     1773     1783      +10

Impacted Files	Coverage Δ
controller/state.go	`72.44% <0.00%> (-1.32%)`	⬇️
controller/sync.go	`54.05% <0.00%> (-1.29%)`	⬇️
pkg/apis/application/v1alpha1/types.go	`54.84% <ø> (ø)`
util/argo/argo.go	`65.96% <28.57%> (-0.89%)`	⬇️
controller/appcontroller.go	`51.78% <31.57%> (-0.53%)`	⬇️
server/project/project.go	`51.68% <40.00%> (-0.83%)`	⬇️
pkg/apis/application/v1alpha1/app_project_types.go	`61.16% <69.23%> (+1.62%)`	⬆️
util/settings/settings.go	`51.36% <0.00%> (ø)`

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

blakepettersson · 2022-08-08T13:08:42Z

controller/appcontroller.go

@@ -426,7 +426,10 @@ func (ctrl *ApplicationController) getResourceTree(a *appv1.Application, managed
 			})
 		} else {
 			err := ctrl.stateCache.IterateHierarchy(a.Spec.Destination.Server, kube.GetResourceKey(live), func(child appv1.ResourceNode, appName string) bool {
-				if !proj.IsResourcePermitted(schema.GroupKind{Group: child.ResourceRef.Group, Kind: child.ResourceRef.Kind}, child.Namespace, a.Spec.Destination) {
+				permitted, _ := proj.IsResourcePermitted(schema.GroupKind{Group: child.ResourceRef.Group, Kind: child.ResourceRef.Kind}, child.Namespace, a.Spec.Destination, func(project string) ([]*appv1.Cluster, error) {


I'm not a big fan of silencing errors, we should probably change the signature of IterateHierarchy to return an error itself

controller/appcontroller.go

blakepettersson · 2022-08-08T13:09:44Z

controller/state.go

@@ -417,7 +417,17 @@ func (m *appStateManager) CompareAppState(app *v1alpha1.Application, project *ap

 	// filter out all resources which are not permitted in the application project
 	for k, v := range liveObjByKey {
-		if !project.IsLiveResourcePermitted(v, app.Spec.Destination.Server, app.Spec.Destination.Name) {
+		permitted, err := project.IsLiveResourcePermitted(v, app.Spec.Destination.Server, app.Spec.Destination.Name, func(project string) ([]*appv1.Cluster, error) {
+			return m.db.GetProjectClusters(context.TODO(), project)


I added context.TODO() as a placeholder, but should probably be something else

blakepettersson · 2022-08-08T13:11:06Z

controller/sync.go

-				return fmt.Errorf("namespace %v is not permitted in project '%s'", un.GetNamespace(), proj.Name)
+			if res.Namespaced {
+				permitted, err := proj.IsDestinationPermitted(v1alpha1.ApplicationDestination{Namespace: un.GetNamespace(), Server: app.Spec.Destination.Server, Name: app.Spec.Destination.Name}, func(project string) ([]*v1alpha1.Cluster, error) {
+					return m.db.GetProjectClusters(context.TODO(), project)


I added context.TODO() as a placeholder, but should probably be something else

blakepettersson · 2022-08-08T13:25:21Z

controller/state.go

+			return m.db.GetProjectClusters(context.TODO(), project)
+		})
+
+		if err != nil {


Not sure if this block is the right way to do this; I suspect that this also needs tests

crenshaw-dev

Just a couple nitpicks. @alexmt would you have time to glance at this? I'm curious if you have any performance concerns.

pkg/apis/application/v1alpha1/app_project_types.go

crenshaw-dev · 2022-08-09T16:03:09Z

@blakepettersson we discussed this in today's security meeting.

The conclusions were:

This PR does solve a problem 😄
Users might be surprised that this behavior isn't the default (so it might be helpful to add a line in the project-scoped cluster docs saying "this feature enables adding clusters, but it does not restrict which clusters may be used. To do that you need to use the destinations field or the permitOnlyProjectScopedClusters flag"
@jessesuen wanted to ponder the problem a bit more and see if he could think of a solution that doesn't require a new flag - will wait for his conclusion before merging
performance might be a concern, so Jesse and/or @alexmt should take a look

Thanks again for the good work! Assuming we conclude that the change is necessary/performant, I wanna be sure to get this into 2.5, so I'll be watching carefully.

blakepettersson · 2022-08-10T08:39:15Z

Awesome! Looking forward to what you guys come up with! 😄

Users might be surprised that this behavior isn't the default

I was one of them 😄

crenshaw-dev · 2022-08-10T13:14:47Z

Looks like something set you up for a bunch of merge conflicts. Can you resolve? It'll probably be easiest to resolve conflicts for the non-generated files and then just run codegen again.

crenshaw-dev · 2022-08-19T15:43:51Z

Discussed this in the Contributor office hours. @jessesuen and @alexmt are gonna chat about this and review.

This commit adds a new flag, `permitOnlyProjectScopedClusters`, which prevents any application from syncing to clusters which are not a part of the same project. Fixes argoproj#10220. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com>

blakepettersson · 2022-09-07T10:28:56Z

@jessesuen @alexmt @crenshaw-dev do you guys have any further thoughts on this PR? 😄

crenshaw-dev

LGTM - will leave this open in case Jesse or Alex have any thoughts, but I'll merge it by the end of the week if not.

blakepettersson · 2022-09-08T11:38:36Z

Awesome, thanks a lot!! 🤗 🤗

This commit adds a new flag, `permitOnlyProjectScopedClusters`, which prevents any application from syncing to clusters which are not a part of the same project. Fixes argoproj#10220. Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> Signed-off-by: Blake Pettersson <blake.pettersson@gmail.com> docs: remove duplicate word in user-management doc (argoproj#10546) Signed-off-by: Mickaël Canévet <mickael.canevet@jellysmack.com> Signed-off-by: Mickaël Canévet <mickael.canevet@jellysmack.com> fix: hide terminal on the non-pod resource kind (argoproj#9980) (argoproj#10556) Signed-off-by: ashutosh16 <11219262+ashutosh16@users.noreply.github.com> Signed-off-by: ashutosh16 <11219262+ashutosh16@users.noreply.github.com> fix: add more info to creationtime format (argoproj#10286) (argoproj#10493) * fix: add more info to creationtime format Signed-off-by: Ashutosh <mail.ashutosh8@gmail.com> * lint issue Signed-off-by: ashutosh16 <mail.ashutosh8@gmail.com> * fix: add more info to creationtime format Signed-off-by: ashutosh16 <11219262+ashutosh16@users.noreply.github.com> Signed-off-by: Ashutosh <mail.ashutosh8@gmail.com> Signed-off-by: ashutosh16 <mail.ashutosh8@gmail.com> Signed-off-by: ashutosh16 <11219262+ashutosh16@users.noreply.github.com> Co-authored-by: Ashutosh <mail.ashutosh8@gmail.com> feat: support multiple sources for application Signed-off-by: ishitasequeira <ishiseq29@gmail.com> remove debug logging and unwanted code Signed-off-by: ishitasequeira <ishiseq29@gmail.com> fix lint and unit test errors Signed-off-by: ishitasequeira <ishiseq29@gmail.com> fix lint and unit test errors Signed-off-by: ishitasequeira <ishiseq29@gmail.com> Merge branch 'multiple-sources-for-applications' of github.com:ishitasequeira/argo-cd into multiple-sources-for-applications feat: support multiple sources for application Signed-off-by: ishitasequeira <ishiseq29@gmail.com> remove debug logging and unwanted code Signed-off-by: ishitasequeira <ishiseq29@gmail.com> fix lint and unit test errors Signed-off-by: ishitasequeira <ishiseq29@gmail.com> fix lint and unit test errors Signed-off-by: ishitasequeira <ishiseq29@gmail.com> fix bug introduced after rebase Signed-off-by: ishitasequeira <ishiseq29@gmail.com> executed make codegen Signed-off-by: ishitasequeira <ishiseq29@gmail.com>

blakepettersson force-pushed the feature/add-permitonlyprojectscopedclusters-flag branch from 22e7635 to de4fb4d Compare August 8, 2022 11:50

blakepettersson commented Aug 8, 2022

View reviewed changes

controller/appcontroller.go Show resolved Hide resolved

blakepettersson commented Aug 8, 2022

View reviewed changes

crenshaw-dev requested changes Aug 8, 2022

View reviewed changes

pkg/apis/application/v1alpha1/app_project_types.go Outdated Show resolved Hide resolved

pkg/apis/application/v1alpha1/app_project_types.go Outdated Show resolved Hide resolved

blakepettersson force-pushed the feature/add-permitonlyprojectscopedclusters-flag branch from de4fb4d to 345430b Compare August 9, 2022 14:58

todaywasawesome requested a review from jessesuen August 9, 2022 15:25

blakepettersson force-pushed the feature/add-permitonlyprojectscopedclusters-flag branch from 345430b to 1812ba6 Compare August 10, 2022 17:35

crenshaw-dev requested a review from alexmt August 19, 2022 15:44

crenshaw-dev added this to the v2.5 milestone Aug 19, 2022

blakepettersson force-pushed the feature/add-permitonlyprojectscopedclusters-flag branch from 1812ba6 to 35174e4 Compare August 19, 2022 18:18

crenshaw-dev approved these changes Sep 7, 2022

View reviewed changes

crenshaw-dev merged commit 5f3fc68 into argoproj:master Sep 8, 2022

crenshaw-dev mentioned this pull request Sep 8, 2022

Make permitOnlyProjectScopedClusters the default #10549

Open

blakepettersson deleted the feature/add-permitonlyprojectscopedclusters-flag branch December 21, 2022 09:42

crenshaw-dev mentioned this pull request Jul 24, 2023

REQUEST: Promotion to Approver(docs) for @blakepettersson argoproj/argoproj#226

Closed

6 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add permitOnlyProjectScopedClusters flag #10237

feat: add permitOnlyProjectScopedClusters flag #10237

blakepettersson commented Aug 8, 2022 •

edited

codecov bot commented Aug 8, 2022 •

edited

blakepettersson Aug 8, 2022

blakepettersson Aug 8, 2022

blakepettersson Aug 8, 2022

blakepettersson Aug 8, 2022

crenshaw-dev left a comment

crenshaw-dev commented Aug 9, 2022

blakepettersson commented Aug 10, 2022

crenshaw-dev commented Aug 10, 2022

crenshaw-dev commented Aug 19, 2022

blakepettersson commented Sep 7, 2022

crenshaw-dev left a comment

blakepettersson commented Sep 8, 2022

feat: add permitOnlyProjectScopedClusters flag #10237

feat: add permitOnlyProjectScopedClusters flag #10237

Conversation

blakepettersson commented Aug 8, 2022 • edited

codecov bot commented Aug 8, 2022 • edited

Codecov Report

blakepettersson Aug 8, 2022

Choose a reason for hiding this comment

blakepettersson Aug 8, 2022

Choose a reason for hiding this comment

blakepettersson Aug 8, 2022

Choose a reason for hiding this comment

blakepettersson Aug 8, 2022

Choose a reason for hiding this comment

crenshaw-dev left a comment

Choose a reason for hiding this comment

crenshaw-dev commented Aug 9, 2022

blakepettersson commented Aug 10, 2022

crenshaw-dev commented Aug 10, 2022

crenshaw-dev commented Aug 19, 2022

blakepettersson commented Sep 7, 2022

crenshaw-dev left a comment

Choose a reason for hiding this comment

blakepettersson commented Sep 8, 2022

blakepettersson commented Aug 8, 2022 •

edited

codecov bot commented Aug 8, 2022 •

edited