feat(teamrolebindings): allow changing namespaces for teamrolebindings (

#813)

* feat(teamrolebindings): allow changing namespaces but deny removing them all from TRB

* feat(teamrolebindings): add cleanup of RoleBindings not matching the namespaces defined in TRB spec

* feat(teamrolebindings): fix removing RoleBindings not in namespaces, add a test

* feat(teamrolebindings): remove all deployed RoleBindings while cleaning up clusters; add tests

* feat(teamrolebindings): deny changing scope of TRB in webhook, add unit tests for webhook

* chore: re-generate manifests

* Automatic generation of CRD API Docs

* feat(teamrolebindings): change webhook message to be more specific

Co-authored-by: Uwe Mayer <115539431+uwe-mayer@users.noreply.github.com>

---------

Co-authored-by: k.zagorski <k.zagorski@accenture.com>
Co-authored-by: Cloud Operator <169066274+cloud-operator@users.noreply.github.com>
Co-authored-by: Uwe Mayer <115539431+uwe-mayer@users.noreply.github.com>

charts/manager/crds/greenhouse.sap_teamrolebindings.yaml

@@ -108,7 +108,7 @@ spec:
                 x-kubernetes-map-type: atomic
               namespaces:
                 description: |-
-                  Namespaces is the immutable list of namespaces in the Greenhouse Clusters to apply the RoleBinding to.
+                  Namespaces is a list of namespaces in the Greenhouse Clusters to apply the RoleBinding to.
                   If empty, a ClusterRoleBinding will be created on the remote cluster, otherwise a RoleBinding per namespace.
                 items:
                   type: string

docs/reference/api/index.html

@@ -3158,7 +3158,7 @@ <h3 id="greenhouse.sap/v1alpha1.TeamRoleBinding">TeamRoleBinding
 </em>
 </td>
 <td>
-<p>Namespaces is the immutable list of namespaces in the Greenhouse Clusters to apply the RoleBinding to.
+<p>Namespaces is a list of namespaces in the Greenhouse Clusters to apply the RoleBinding to.
 If empty, a ClusterRoleBinding will be created on the remote cluster, otherwise a RoleBinding per namespace.</p>
 </td>
 </tr>
@@ -3252,7 +3252,7 @@ <h3 id="greenhouse.sap/v1alpha1.TeamRoleBindingSpec">TeamRoleBindingSpec
 </em>
 </td>
 <td>
-<p>Namespaces is the immutable list of namespaces in the Greenhouse Clusters to apply the RoleBinding to.
+<p>Namespaces is a list of namespaces in the Greenhouse Clusters to apply the RoleBinding to.
 If empty, a ClusterRoleBinding will be created on the remote cluster, otherwise a RoleBinding per namespace.</p>
 </td>
 </tr>

docs/reference/api/openapi.yaml

@@ -988,7 +988,7 @@ components:
               type: object
               x-kubernetes-map-type: atomic
             namespaces:
-              description: Namespaces is the immutable list of namespaces in the Greenhouse Clusters to apply the RoleBinding to.\nIf empty, a ClusterRoleBinding will be created on the remote cluster, otherwise a RoleBinding per namespace.
+              description: Namespaces is a list of namespaces in the Greenhouse Clusters to apply the RoleBinding to.\nIf empty, a ClusterRoleBinding will be created on the remote cluster, otherwise a RoleBinding per namespace.
               items:
                 type: string
               type: array

pkg/admission/teamrolebinding_webhook.go

@@ -5,7 +5,6 @@ package admission
 
 import (
 	"context"
-	"reflect"
 
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -87,8 +86,18 @@ func ValidateUpdateRoleBinding(ctx context.Context, c client.Client, old, cur ru
 				Group:    oldRB.GroupVersionKind().Group,
 				Resource: oldRB.Kind,
 			}, oldRB.Name, field.Forbidden(field.NewPath("spec"), "must contain either spec.clusterName or spec.clusterSelector"))
-	case hasNamespacesChanged(oldRB, curRB):
-		return nil, apierrors.NewForbidden(schema.GroupResource{Group: oldRB.GroupVersionKind().Group, Resource: oldRB.Kind}, oldRB.Name, field.Forbidden(field.NewPath("spec", "namespaces"), "cannot be changed"))
+	case isClusterScoped(oldRB) && !isClusterScoped(curRB):
+		return nil, apierrors.NewForbidden(
+			schema.GroupResource{
+				Group:    oldRB.GroupVersionKind().Group,
+				Resource: oldRB.Kind,
+			}, oldRB.Name, field.Forbidden(field.NewPath("spec", "namespaces"), "cannot change existing TeamRoleBinding from cluster-scoped to namespace-scoped by adding namespaces"))
+	case !isClusterScoped(oldRB) && isClusterScoped(curRB):
+		return nil, apierrors.NewForbidden(
+			schema.GroupResource{
+				Group:    oldRB.GroupVersionKind().Group,
+				Resource: oldRB.Kind,
+			}, oldRB.Name, field.Forbidden(field.NewPath("spec", "namespaces"), "cannot remove all namespaces in existing TeamRoleBinding"))
 	default:
 		return nil, nil
 	}
@@ -98,11 +107,6 @@ func ValidateDeleteRoleBinding(_ context.Context, _ client.Client, _ runtime.Obj
 	return nil, nil
 }
 
-// hasNamespacesChanged returns true if the namespaces in the old and current RoleBinding are different.
-func hasNamespacesChanged(old, cur *greenhousev1alpha1.TeamRoleBinding) bool {
-	return !reflect.DeepEqual(old.Spec.Namespaces, cur.Spec.Namespaces)
-}
-
 // validateClusterNameOrSelector checks if the TeamRoleBinding has a valid clusterName or clusterSelector but not both.
 func validateClusterNameOrSelector(rb *greenhousev1alpha1.TeamRoleBinding) error {
 	if rb.Spec.ClusterName != "" && (len(rb.Spec.ClusterSelector.MatchLabels) > 0 || len(rb.Spec.ClusterSelector.MatchExpressions) > 0) {
@@ -114,3 +118,8 @@ func validateClusterNameOrSelector(rb *greenhousev1alpha1.TeamRoleBinding) error
 	}
 	return nil
 }
+
+// isClusterScoped returns true if the TeamRoleBinding will create ClusterRoleBindings.
+func isClusterScoped(trb *greenhousev1alpha1.TeamRoleBinding) bool {
+	return len(trb.Spec.Namespaces) == 0
+}

pkg/admission/teamrolebinding_webhook_test.go

@@ -14,7 +14,6 @@ import (
 )
 
 var _ = Describe("Validate Create RoleBinding", Ordered, func() {
-
 	var (
 		setup    *test.TestSetup
 		teamRole *greenhousev1alpha1.TeamRole
@@ -117,50 +116,124 @@ var _ = Describe("Validate Create RoleBinding", Ordered, func() {
 			Expect(err).To(MatchError(ContainSubstring("cannot specify both spec.clusterName and spec.clusterSelector")))
 		})
 	})
-})
 
-var _ = Describe("Validate Update Rolebinding", func() {
-	Context("ensures that changes to the immutable Namespaces are detected", func() {
-		defaultNamespaces := []string{"testNamespace", "demoNamespace"}
-		emptyNamespaces := []string{}
-		editedNamespaces := []string{"editedNamespace", "demoNamespace"}
-		deletedNamespaces := []string{"demoNamespace"}
+	Context("Validate Update Rolebinding", func() {
+		It("Should deny changes to the empty Namespaces", func() {
+			emptyNamespaces := []string{}
+			oldRB := &greenhousev1alpha1.TeamRoleBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "greenhouse",
+					Name:      "testBinding",
+				},
+				Spec: greenhousev1alpha1.TeamRoleBindingSpec{
+					TeamRoleRef: teamRole.Name,
+					TeamRef:     team.Name,
+					ClusterName: cluster.Name,
+					Namespaces:  emptyNamespaces,
+				},
+			}
+
+			editedNamespaces := []string{"demoNamespace"}
+			curRB := &greenhousev1alpha1.TeamRoleBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "greenhouse",
+					Name:      "testBinding",
+				},
+				Spec: greenhousev1alpha1.TeamRoleBindingSpec{
+					TeamRoleRef: teamRole.Name,
+					TeamRef:     team.Name,
+					ClusterName: cluster.Name,
+					Namespaces:  editedNamespaces,
+				},
+			}
+
+			warns, err := ValidateUpdateRoleBinding(test.Ctx, test.K8sClient, oldRB, curRB)
+			Expect(warns).To(BeNil(), "expected no warnings")
+			Expect(err).To(HaveOccurred(), "expected an error")
+			Expect(err).To(MatchError(ContainSubstring("cannot change existing TeamRoleBinding from cluster-scoped to namespace-scoped")))
+		})
 
-		DescribeTable("Validate that adding, removing, or editing Namespaces is detected", func(oldNamespaces, curNamespaces []string, expChange bool) {
+		It("Should deny removing all Namespaces", func() {
+			filledNamespaces := []string{"demoNamespace1", "demoNamespace2"}
 			oldRB := &greenhousev1alpha1.TeamRoleBinding{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "greenhouse",
 					Name:      "testBinding",
 				},
 				Spec: greenhousev1alpha1.TeamRoleBindingSpec{
-					TeamRoleRef: "testRole",
-					Namespaces:  oldNamespaces,
+					TeamRoleRef: teamRole.Name,
+					TeamRef:     team.Name,
+					ClusterName: cluster.Name,
+					Namespaces:  filledNamespaces,
 				},
 			}
 
+			emptyNamespaces := []string{}
 			curRB := &greenhousev1alpha1.TeamRoleBinding{
 				ObjectMeta: metav1.ObjectMeta{
 					Namespace: "greenhouse",
 					Name:      "testBinding",
 				},
 				Spec: greenhousev1alpha1.TeamRoleBindingSpec{
-					TeamRoleRef: "testRole",
-					Namespaces:  curNamespaces,
+					TeamRoleRef: teamRole.Name,
+					TeamRef:     team.Name,
+					ClusterName: cluster.Name,
+					Namespaces:  emptyNamespaces,
 				},
 			}
 
-			switch hasChanged := hasNamespacesChanged(oldRB, curRB); hasChanged {
-			case true:
-				Expect(expChange).To(BeTrue(), "expected Namespaces changes, but none found")
-			default:
-				Expect(expChange).To(BeFalse(), "unexpected Namespaces change detected")
+			warns, err := ValidateUpdateRoleBinding(test.Ctx, test.K8sClient, oldRB, curRB)
+			Expect(warns).To(BeNil(), "expected no warnings")
+			Expect(err).To(HaveOccurred(), "expected an error")
+			Expect(err).To(MatchError(ContainSubstring("cannot remove all namespaces in existing TeamRoleBinding")))
+		})
+
+		It("Should allow changing Namespaces", func() {
+			filledNamespaces := []string{"demoNamespace1", "demoNamespace2"}
+			oldRB := &greenhousev1alpha1.TeamRoleBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "greenhouse",
+					Name:      "testBinding",
+				},
+				Spec: greenhousev1alpha1.TeamRoleBindingSpec{
+					TeamRoleRef: teamRole.Name,
+					TeamRef:     team.Name,
+					ClusterName: cluster.Name,
+					Namespaces:  filledNamespaces,
+				},
 			}
-		},
-			Entry("No Changes, all good", defaultNamespaces, defaultNamespaces, false),
-			Entry("Namespaces added", emptyNamespaces, defaultNamespaces, true),
-			Entry("Namespaces removed", defaultNamespaces, emptyNamespaces, true),
-			Entry("Namespaces edited", defaultNamespaces, editedNamespaces, true),
-			Entry("Namespaces deleted", defaultNamespaces, deletedNamespaces, true),
-		)
+
+			addedNamespaces := []string{"demoNamespace1", "demoNamespace2", "demoNamespace3"}
+			curRB := &greenhousev1alpha1.TeamRoleBinding{
+				ObjectMeta: metav1.ObjectMeta{
+					Namespace: "greenhouse",
+					Name:      "testBinding",
+				},
+				Spec: greenhousev1alpha1.TeamRoleBindingSpec{
+					TeamRoleRef: teamRole.Name,
+					TeamRef:     team.Name,
+					ClusterName: cluster.Name,
+					Namespaces:  addedNamespaces,
+				},
+			}
+
+			warns, err := ValidateUpdateRoleBinding(test.Ctx, test.K8sClient, oldRB, curRB)
+			Expect(warns).To(BeNil(), "expected no warnings")
+			Expect(err).ToNot(HaveOccurred(), "expected no error")
+
+			removedNamespaces := []string{"demoNamespace1"}
+			curRB.Spec.Namespaces = removedNamespaces
+
+			warns, err = ValidateUpdateRoleBinding(test.Ctx, test.K8sClient, oldRB, curRB)
+			Expect(warns).To(BeNil(), "expected no warnings")
+			Expect(err).ToNot(HaveOccurred(), "expected no error")
+
+			differentNamespaces := []string{"differentNamespace1", "differentNamespace2"}
+			curRB.Spec.Namespaces = differentNamespaces
+
+			warns, err = ValidateUpdateRoleBinding(test.Ctx, test.K8sClient, oldRB, curRB)
+			Expect(warns).To(BeNil(), "expected no warnings")
+			Expect(err).ToNot(HaveOccurred(), "expected no error")
+		})
 	})
 })

pkg/apis/greenhouse/v1alpha1/teamrolebinding_types.go

@@ -21,7 +21,7 @@ type TeamRoleBindingSpec struct {
 	ClusterName string `json:"clusterName,omitempty"`
 	// ClusterSelector is a label selector to select the Clusters the TeamRoleBinding should be deployed to.
 	ClusterSelector metav1.LabelSelector `json:"clusterSelector,omitempty"`
-	// Namespaces is the immutable list of namespaces in the Greenhouse Clusters to apply the RoleBinding to.
+	// Namespaces is a list of namespaces in the Greenhouse Clusters to apply the RoleBinding to.
 	// If empty, a ClusterRoleBinding will be created on the remote cluster, otherwise a RoleBinding per namespace.
 	Namespaces []string `json:"namespaces,omitempty"`
 }

pkg/controllers/teamrbac/teamrolebinding_controller.go

@@ -294,6 +294,61 @@ func (r *TeamRoleBindingReconciler) cleanupResources(ctx context.Context, trb *g
 				return err
 			}
 			trb.RemovePropagationStatus(s.ClusterName)
+			continue
+		}
+
+		// Remove RoleBindings from all namespaces not matching the .spec.namespaces.
+		cluster := &greenhousev1alpha1.Cluster{}
+		err := r.Get(ctx, types.NamespacedName{Namespace: trb.Namespace, Name: s.ClusterName}, cluster)
+		if err != nil {
+			return err
+		}
+		if err = r.cleanupClusterNamespaces(ctx, trb, cluster); err != nil {
+			return err
+		}
+	}
+	return nil
+}
+
+// cleanupClusterNamespaces removes RoleBindings not matching the trb.spec.namespaces from the cluster.
+func (r *TeamRoleBindingReconciler) cleanupClusterNamespaces(ctx context.Context, trb *greenhousev1alpha1.TeamRoleBinding, cluster *greenhousev1alpha1.Cluster) error {
+	if isClusterScoped(trb) {
+		return nil
+	}
+
+	cl, err := clientutil.NewK8sClientFromCluster(ctx, r.Client, cluster)
+	if err != nil {
+		log.FromContext(ctx).Error(err, "Error getting client for cluster", "cluster", cluster.GetName())
+		return err
+	}
+
+	var roleBindings = new(rbacv1.RoleBindingList)
+	err = cl.List(ctx, roleBindings, &client.ListOptions{
+		FieldSelector: fields.OneTermEqualSelector("metadata.name", trb.GetRBACName()),
+	})
+	if apierrors.IsNotFound(err) {
+		return nil
+	} else if err != nil {
+		return err
+	}
+
+	roleBindingsToDelete := slices.DeleteFunc(roleBindings.Items, func(roleBinding rbacv1.RoleBinding) bool {
+		return slices.ContainsFunc(trb.Spec.Namespaces, func(namespace string) bool {
+			return roleBinding.Namespace == namespace
+		})
+	})
+	if len(roleBindingsToDelete) == 0 {
+		return nil
+	}
+
+	for _, roleBinding := range roleBindingsToDelete {
+		result, err := clientutil.Delete(ctx, cl, &roleBinding)
+		switch {
+		case err != nil:
+			log.FromContext(ctx).Error(err, "error deleting RoleBinding", "roleBinding", trb.GetRBACName(), "cluster", cluster.GetName(), "namespace", roleBinding.Namespace)
+			return err
+		case result == clientutil.DeletionResultDeleted:
+			log.FromContext(ctx).Info("deleted RoleBinding successfully", "roleBinding", trb.GetRBACName(), "cluster", cluster.GetName(), "namespace", roleBinding.Namespace)
 		}
 	}
 	return nil
@@ -315,7 +370,7 @@ func (r *TeamRoleBindingReconciler) cleanupCluster(ctx context.Context, trb *gre
 			return err
 		}
 	default:
-		if err := r.deleteRoleBindings(ctx, cl, trb, cluster); err != nil {
+		if err := r.deleteAllDeployedRoleBindings(ctx, cl, trb, cluster); err != nil {
 			return err
 		}
 	}
@@ -511,22 +566,28 @@ func reconcileRoleBinding(ctx context.Context, cl client.Client, c *greenhousev1
 	return nil
 }
 
-func (r TeamRoleBindingReconciler) deleteRoleBindings(ctx context.Context, cl client.Client, trb *greenhousev1alpha1.TeamRoleBinding, cluster *greenhousev1alpha1.Cluster) error {
-	for _, namespace := range trb.Spec.Namespaces {
-		remoteObject := &rbacv1.RoleBinding{
-			ObjectMeta: metav1.ObjectMeta{
-				Name:      trb.GetRBACName(),
-				Namespace: namespace,
-			},
-		}
-		result, err := clientutil.Delete(ctx, cl, remoteObject)
+// deleteAllDeployedRoleBindings deletes all RoleBindings deployed to a remote cluster.
+// Deletes not only those specified in .spec.namespaces, but all by the trb.GetRBACName() name.
+func (r TeamRoleBindingReconciler) deleteAllDeployedRoleBindings(ctx context.Context, cl client.Client, trb *greenhousev1alpha1.TeamRoleBinding, cluster *greenhousev1alpha1.Cluster) error {
+	var roleBindingsToDelete = new(rbacv1.RoleBindingList)
+	err := cl.List(ctx, roleBindingsToDelete, &client.ListOptions{
+		FieldSelector: fields.OneTermEqualSelector("metadata.name", trb.GetRBACName()),
+	})
+	if apierrors.IsNotFound(err) {
+		return nil
+	} else if err != nil {
+		return err
+	}
+
+	for _, roleBinding := range roleBindingsToDelete.Items {
+		result, err := clientutil.Delete(ctx, cl, &roleBinding)
 
 		switch {
 		case err != nil:
-			log.FromContext(ctx).Error(err, "error deleting RoleBinding", "roleBinding", trb.GetRBACName(), "cluster", cluster.GetName(), "namespace", namespace)
+			log.FromContext(ctx).Error(err, "error deleting RoleBinding", "roleBinding", trb.GetRBACName(), "cluster", cluster.GetName(), "namespace", roleBinding.Namespace)
 			return err
 		case result == clientutil.DeletionResultDeleted:
-			log.FromContext(ctx).Info("deleted RoleBinding successfully", "roleBinding", trb.GetRBACName(), "cluster", cluster.GetName(), "namespace", namespace)
+			log.FromContext(ctx).Info("deleted RoleBinding successfully", "roleBinding", trb.GetRBACName(), "cluster", cluster.GetName(), "namespace", roleBinding.Namespace)
 		}
 	}
 	return nil