Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: add permitOnlyProjectScopedClusters flag #10237

Merged
merged 1 commit into from
Sep 8, 2022
Merged

feat: add permitOnlyProjectScopedClusters flag #10237

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
4 changes: 4 additions & 0 deletions assets/swagger.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4925,6 +4925,10 @@
"orphanedResources": {
"$ref": "#/definitions/v1alpha1OrphanedResourcesMonitorSettings"
},
"permitOnlyProjectScopedClusters": {
"type": "boolean",
"title": "PermitOnlyProjectScopedClusters determines whether destinations can only reference clusters which are project-scoped"
},
"roles": {
"type": "array",
"title": "Roles are user defined RBAC roles associated with this project",
Expand Down
38 changes: 29 additions & 9 deletions controller/appcontroller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -456,7 +456,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) {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

return ctrl.db.GetProjectClusters(context.TODO(), project)
})
if !permitted {
return false
}
nodes = append(nodes, child)
Expand All @@ -478,7 +481,16 @@ func (ctrl *ApplicationController) getResourceTree(a *appv1.Application, managed
belongToAnotherApp = true
}
}
if belongToAnotherApp || !proj.IsResourcePermitted(schema.GroupKind{Group: child.ResourceRef.Group, Kind: child.ResourceRef.Kind}, child.Namespace, a.Spec.Destination) {

if belongToAnotherApp {
return false
}

permitted, _ := proj.IsResourcePermitted(schema.GroupKind{Group: child.ResourceRef.Group, Kind: child.ResourceRef.Kind}, child.Namespace, a.Spec.Destination, func(project string) ([]*appv1.Cluster, error) {
crenshaw-dev marked this conversation as resolved.
Show resolved Hide resolved
return ctrl.db.GetProjectClusters(context.TODO(), project)
})

if !permitted {
return false
}
orphanedNodes = append(orphanedNodes, child)
Expand Down Expand Up @@ -798,7 +810,7 @@ func (ctrl *ApplicationController) processAppOperationQueueItem() (processNext b
app := origApp.DeepCopy()

if app.Operation != nil {
// If we get here, we are about process an operation but we cannot rely on informer since it might has stale data.
// If we get here, we are about to process an operation, but we cannot rely on informer since it might have stale data.
// So always retrieve the latest version to ensure it is not stale to avoid unnecessary syncing.
// We cannot rely on informer since applications might be updated by both application controller and api server.
freshApp, err := ctrl.applicationClientset.ArgoprojV1alpha1().Applications(app.ObjectMeta.Namespace).Get(context.Background(), app.ObjectMeta.Name, metav1.GetOptions{})
Expand All @@ -812,7 +824,9 @@ func (ctrl *ApplicationController) processAppOperationQueueItem() (processNext b
if app.Operation != nil {
ctrl.processRequestedAppOperation(app)
} else if app.DeletionTimestamp != nil && app.CascadedDeletion() {
_, err = ctrl.finalizeApplicationDeletion(app)
_, err = ctrl.finalizeApplicationDeletion(app, func(project string) ([]*appv1.Cluster, error) {
return ctrl.db.GetProjectClusters(context.Background(), project)
})
if err != nil {
ctrl.setAppCondition(app, appv1.ApplicationCondition{
Type: appv1.ApplicationConditionDeletionError,
Expand Down Expand Up @@ -926,21 +940,27 @@ func (ctrl *ApplicationController) shouldBeDeleted(app *appv1.Application, obj *
return !kube.IsCRD(obj) && !isSelfReferencedApp(app, kube.GetObjectRef(obj))
}

func (ctrl *ApplicationController) getPermittedAppLiveObjects(app *appv1.Application, proj *appv1.AppProject) (map[kube.ResourceKey]*unstructured.Unstructured, error) {
func (ctrl *ApplicationController) getPermittedAppLiveObjects(app *appv1.Application, proj *appv1.AppProject, projectClusters func(project string) ([]*appv1.Cluster, error)) (map[kube.ResourceKey]*unstructured.Unstructured, error) {
objsMap, err := ctrl.stateCache.GetManagedLiveObjs(app, []*unstructured.Unstructured{})
if err != nil {
return nil, err
}
// Don't delete live resources which are not permitted in the app project
for k, v := range objsMap {
if !proj.IsLiveResourcePermitted(v, app.Spec.Destination.Server, app.Spec.Destination.Name) {
permitted, err := proj.IsLiveResourcePermitted(v, app.Spec.Destination.Server, app.Spec.Destination.Name, projectClusters)

if err != nil {
return nil, err
}

if !permitted {
delete(objsMap, k)
}
}
return objsMap, nil
}

func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Application) ([]*unstructured.Unstructured, error) {
func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Application, projectClusters func(project string) ([]*appv1.Cluster, error)) ([]*unstructured.Unstructured, error) {
logCtx := log.WithField("application", app.QualifiedName())
logCtx.Infof("Deleting resources")
// Get refreshed application info, since informer app copy might be stale
Expand Down Expand Up @@ -981,7 +1001,7 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic
if validDestination {
// ApplicationDestination points to a valid cluster, so we may clean up the live objects

objsMap, err := ctrl.getPermittedAppLiveObjects(app, proj)
objsMap, err := ctrl.getPermittedAppLiveObjects(app, proj, projectClusters)
if err != nil {
return nil, err
}
Expand Down Expand Up @@ -1016,7 +1036,7 @@ func (ctrl *ApplicationController) finalizeApplicationDeletion(app *appv1.Applic
return objs, err
}

objsMap, err = ctrl.getPermittedAppLiveObjects(app, proj)
objsMap, err = ctrl.getPermittedAppLiveObjects(app, proj, projectClusters)
if err != nil {
return nil, err
}
Expand Down
16 changes: 12 additions & 4 deletions controller/appcontroller_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -554,7 +554,9 @@ func TestFinalizeAppDeletion(t *testing.T) {
patched = true
return true, nil, nil
})
_, err := ctrl.finalizeApplicationDeletion(app)
_, err := ctrl.finalizeApplicationDeletion(app, func(project string) ([]*argoappv1.Cluster, error) {
return []*argoappv1.Cluster{}, nil
})
assert.NoError(t, err)
assert.True(t, patched)
})
Expand Down Expand Up @@ -602,7 +604,9 @@ func TestFinalizeAppDeletion(t *testing.T) {
patched = true
return true, nil, nil
})
objs, err := ctrl.finalizeApplicationDeletion(app)
objs, err := ctrl.finalizeApplicationDeletion(app, func(project string) ([]*argoappv1.Cluster, error) {
return []*argoappv1.Cluster{}, nil
})
assert.NoError(t, err)
assert.True(t, patched)
objsMap, err := ctrl.stateCache.GetManagedLiveObjs(app, []*unstructured.Unstructured{})
Expand Down Expand Up @@ -634,7 +638,9 @@ func TestFinalizeAppDeletion(t *testing.T) {
patched = true
return true, nil, nil
})
_, err := ctrl.finalizeApplicationDeletion(app)
_, err := ctrl.finalizeApplicationDeletion(app, func(project string) ([]*argoappv1.Cluster, error) {
return []*argoappv1.Cluster{}, nil
})
assert.NoError(t, err)
assert.True(t, patched)
})
Expand All @@ -657,7 +663,9 @@ func TestFinalizeAppDeletion(t *testing.T) {
fakeAppCs.AddReactor("get", "*", func(action kubetesting.Action) (handled bool, ret runtime.Object, err error) {
return defaultReactor.React(action)
})
_, err := ctrl.finalizeApplicationDeletion(app)
_, err := ctrl.finalizeApplicationDeletion(app, func(project string) ([]*argoappv1.Cluster, error) {
return []*argoappv1.Cluster{}, nil
})
assert.NoError(t, err)
}

Expand Down
12 changes: 11 additions & 1 deletion controller/state.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -419,7 +419,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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})

if err != nil {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

conditions = append(conditions, v1alpha1.ApplicationCondition{Type: v1alpha1.ApplicationConditionComparisonError, Message: err.Error(), LastTransitionTime: &now})
failedToLoadObjs = true
continue
}

if !permitted {
delete(liveObjByKey, k)
}
}
Expand Down
14 changes: 12 additions & 2 deletions controller/sync.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -226,8 +226,18 @@ func (m *appStateManager) SyncAppState(app *v1alpha1.Application, state *v1alpha
if !proj.IsGroupKindPermitted(un.GroupVersionKind().GroupKind(), res.Namespaced) {
return fmt.Errorf("resource %s:%s is not permitted in project %s", un.GroupVersionKind().Group, un.GroupVersionKind().Kind, proj.Name)
}
if res.Namespaced && !proj.IsDestinationPermitted(v1alpha1.ApplicationDestination{Namespace: un.GetNamespace(), Server: app.Spec.Destination.Server, Name: app.Spec.Destination.Name}) {
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)
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

})

if err != nil {
return err
}

if !permitted {
return fmt.Errorf("namespace %v is not permitted in project '%s'", un.GetNamespace(), proj.Name)
}
}
return nil
}),
Expand Down
27 changes: 27 additions & 0 deletions docs/user-guide/projects.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -293,3 +293,30 @@ stringData:
```

All the examples above talk about Git repositories, but the same principles apply to clusters as well.

With cluster-scoped clusters we can also restrict projects to only allow applications whose destinations belong to the
same project. The default behavior allows for applications to be installed onto clusters which are not a part of the same
project, as the example below demonstrates:

```yaml
apiVersion: argoproj.io/v1alpha1
kind: Application
metadata:
name: "some-ns"
spec:
destination:
# This destination might not actually be a cluster which belongs to `foo-project`
server: https://some-k8s-server/
namespace: "some-ns"
project: foo-project
```

To prevent this behavior, we can set the attribute `permitOnlyProjectScopedClusters` on a project.

```yaml
spec:
permitOnlyProjectScopedClusters: true
```

With this set, the application above would no longer be allowed to be synced to any cluster other than the ones which
are a part of the same project.
4 changes: 4 additions & 0 deletions manifests/core-install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9063,6 +9063,10 @@ spec:
for apps which have orphaned resources
type: boolean
type: object
permitOnlyProjectScopedClusters:
description: PermitOnlyProjectScopedClusters determines whether destinations
can only reference clusters which are project-scoped
type: boolean
roles:
description: Roles are user defined RBAC roles associated with this
project
Expand Down
4 changes: 4 additions & 0 deletions manifests/crds/appproject-crd.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,10 @@ spec:
for apps which have orphaned resources
type: boolean
type: object
permitOnlyProjectScopedClusters:
description: PermitOnlyProjectScopedClusters determines whether destinations
can only reference clusters which are project-scoped
type: boolean
roles:
description: Roles are user defined RBAC roles associated with this
project
Expand Down
4 changes: 4 additions & 0 deletions manifests/ha/install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9063,6 +9063,10 @@ spec:
for apps which have orphaned resources
type: boolean
type: object
permitOnlyProjectScopedClusters:
description: PermitOnlyProjectScopedClusters determines whether destinations
can only reference clusters which are project-scoped
type: boolean
roles:
description: Roles are user defined RBAC roles associated with this
project
Expand Down
4 changes: 4 additions & 0 deletions manifests/install.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9063,6 +9063,10 @@ spec:
for apps which have orphaned resources
type: boolean
type: object
permitOnlyProjectScopedClusters:
description: PermitOnlyProjectScopedClusters determines whether destinations
can only reference clusters which are project-scoped
type: boolean
roles:
description: Roles are user defined RBAC roles associated with this
project
Expand Down
34 changes: 27 additions & 7 deletions pkg/apis/application/v1alpha1/app_project_types.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -336,18 +336,18 @@ func (proj AppProject) IsGroupKindPermitted(gk schema.GroupKind, namespaced bool
}

// IsLiveResourcePermitted returns whether a live resource found in the cluster is permitted by an AppProject
func (proj AppProject) IsLiveResourcePermitted(un *unstructured.Unstructured, server string, name string) bool {
return proj.IsResourcePermitted(un.GroupVersionKind().GroupKind(), un.GetNamespace(), ApplicationDestination{Server: server, Name: name})
func (proj AppProject) IsLiveResourcePermitted(un *unstructured.Unstructured, server string, name string, projectClusters func(project string) ([]*Cluster, error)) (bool, error) {
return proj.IsResourcePermitted(un.GroupVersionKind().GroupKind(), un.GetNamespace(), ApplicationDestination{Server: server, Name: name}, projectClusters)
}

func (proj AppProject) IsResourcePermitted(groupKind schema.GroupKind, namespace string, dest ApplicationDestination) bool {
func (proj AppProject) IsResourcePermitted(groupKind schema.GroupKind, namespace string, dest ApplicationDestination, projectClusters func(project string) ([]*Cluster, error)) (bool, error) {
if !proj.IsGroupKindPermitted(groupKind, namespace != "") {
return false
return false, nil
}
if namespace != "" {
return proj.IsDestinationPermitted(ApplicationDestination{Server: dest.Server, Name: dest.Name, Namespace: namespace})
return proj.IsDestinationPermitted(ApplicationDestination{Server: dest.Server, Name: dest.Name, Namespace: namespace}, projectClusters)
}
return true
return true, nil
}

// HasFinalizer returns true if a resource finalizer is set on an AppProject
Expand Down Expand Up @@ -384,7 +384,27 @@ func (proj AppProject) IsSourcePermitted(src ApplicationSource) bool {
}

// IsDestinationPermitted validates if the provided application's destination is one of the allowed destinations for the project
func (proj AppProject) IsDestinationPermitted(dst ApplicationDestination) bool {
func (proj AppProject) IsDestinationPermitted(dst ApplicationDestination, projectClusters func(project string) ([]*Cluster, error)) (bool, error) {
destinationMatched := proj.isDestinationMatched(dst)
if destinationMatched && proj.Spec.PermitOnlyProjectScopedClusters {
clusters, err := projectClusters(proj.Name)
if err != nil {
return false, fmt.Errorf("could not retrieve project clusters: %s", err)
}

for _, cluster := range clusters {
if cluster.Name == dst.Name || cluster.Server == dst.Server {
return true, nil
}
}

return false, nil
}

return destinationMatched, nil
}

func (proj AppProject) isDestinationMatched(dst ApplicationDestination) bool {
anyDestinationMatched := false
noDenyDestinationsMatched := true

Expand Down