Fix seeing resources after rolebinding has been deleted for user

pkg/query/collector/reconciler/reconciler.go

@@ -8,6 +8,7 @@ import (
 	"github.com/weaveworks/weave-gitops-enterprise/pkg/query/configuration"
 	"github.com/weaveworks/weave-gitops-enterprise/pkg/query/internal/models"
 	"github.com/weaveworks/weave-gitops/core/logger"
+	"k8s.io/apimachinery/pkg/api/errors"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 )
@@ -22,8 +23,14 @@ type Reconciler interface {
 
 type ProcessFunc func(models.ObjectTransaction) error
 
-func NewReconciler(clusterName string, objectKind configuration.ObjectKind, client client.Client, process ProcessFunc, log logr.Logger) (Reconciler, error) {
+var kindsWithoutFinalizers = map[string]bool{
+	"ClusterRoleBinding": true,
+	"ClusterRole":        true,
+	"RoleBinding":        true,
+	"Role":               true,
+}
 
+func NewReconciler(clusterName string, objectKind configuration.ObjectKind, client client.Client, process ProcessFunc, log logr.Logger) (Reconciler, error) {
 	if client == nil {
 		return nil, fmt.Errorf("invalid client")
 	}
@@ -65,10 +72,35 @@ func (g GenericReconciler) Setup(mgr ctrl.Manager) error {
 }
 
 func (r *GenericReconciler) Reconcile(ctx context.Context, req ctrl.Request) (ctrl.Result, error) {
-
 	clientObject := r.objectKind.NewClientObjectFunc()
 	if err := r.client.Get(ctx, req.NamespacedName, clientObject); err != nil {
-		return ctrl.Result{}, client.IgnoreNotFound(err)
+		// If the object being reconciled has no finalizers, then on deletion we will receive a reconcile request
+		// for this object only after the object was deleted (not before deletion, as in the case of objects with finalizers)
+		// and `client.Get` will return a `NotFound` error.
+		// Thus, if a `NotFound` error is returned for an object whose Kind is included
+		// in the list of known `Kinds` of objects, which do not have finalizers by default,
+		// we can infer that the object was deleted.
+		_, ok := kindsWithoutFinalizers[r.objectKind.Gvk.Kind]
+
+		if !errors.IsNotFound(err) || !ok {
+			return ctrl.Result{}, client.IgnoreNotFound(err)
+		}
+
+		clientObject.SetName(req.Name)
+		clientObject.SetNamespace(req.Namespace)
+		clientObject.GetObjectKind().SetGroupVersionKind(r.objectKind.Gvk)
+
+		tx := transaction{
+			clusterName:     r.clusterName,
+			object:          clientObject,
+			transactionType: models.TransactionTypeDelete,
+			retentionPolicy: r.objectKind.RetentionPolicy,
+			config:          r.objectKind,
+		}
+
+		r.debug.Info("object transaction received", "transaction", tx.String())
+
+		return ctrl.Result{}, r.processFunc(tx)
 	}
 
 	if r.objectKind.FilterFunc != nil {

pkg/query/collector/reconciler/reconciler_test.go

@@ -7,6 +7,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/weaveworks/weave-gitops-enterprise/pkg/query/configuration"
 	"github.com/weaveworks/weave-gitops-enterprise/pkg/query/utils/testutils"
+	rbacv1 "k8s.io/api/rbac/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
 
@@ -116,17 +117,29 @@ func TestReconciler_Reconcile(t *testing.T) {
 	clusterName := "anyCluster"
 	//setup data
 	createdOrUpdatedHelmRelease := testutils.NewHelmRelease("createdOrUpdatedHelmRelease", clusterName)
-	deleteHelmRelease := testutils.NewHelmRelease("deletedHelmRelease", clusterName, func(hr *v2beta1.HelmRelease) {
+	deletedHelmRelease := testutils.NewHelmRelease("deletedHelmRelease", clusterName, func(hr *v2beta1.HelmRelease) {
 		now := metav1.Now()
 		hr.DeletionTimestamp = &now
 	})
-	objects := []runtime.Object{createdOrUpdatedHelmRelease, deleteHelmRelease}
+	deletedClusterRoleWithRules := testutils.NewClusterRole("deletedClusterRoleWithRules", true, func(cr *rbacv1.ClusterRole) {
+		now := metav1.Now()
+		cr.DeletionTimestamp = &now
+	})
+	deletedClusterRoleWithoutRules := testutils.NewClusterRole("deletedClusterRoleWithoutRules", false, func(cr *rbacv1.ClusterRole) {
+		now := metav1.Now()
+		cr.DeletionTimestamp = &now
+	})
+	deletedClusterRoleManuallyConstructed := testutils.NewClusterRole("deletedClusterRoleManuallyConstructed", false)
+	objects := []runtime.Object{createdOrUpdatedHelmRelease, deletedHelmRelease, deletedClusterRoleWithRules, deletedClusterRoleWithoutRules}
 
 	//setup reconciler
 	s := runtime.NewScheme()
 	if err := v2beta1.AddToScheme(s); err != nil {
 		t.Fatalf("could not add v2beta1 to scheme: %v", err)
 	}
+	if err := rbacv1.AddToScheme(s); err != nil {
+		t.Fatalf("could not add rbacv1 to scheme: %v", err)
+	}
 	fakeClient := fake.NewClientBuilder().WithRuntimeObjects(objects...).WithScheme(s).Build()
 
 	tests := []struct {
@@ -159,23 +172,77 @@ func TestReconciler_Reconcile(t *testing.T) {
 		},
 		{
 			name:   "can reconcile delete resource requests",
-			object: deleteHelmRelease,
+			object: deletedHelmRelease,
 			kind:   configuration.HelmReleaseObjectKind,
 			request: ctrl.Request{
 				NamespacedName: types.NamespacedName{
-					Name:      deleteHelmRelease.GetName(),
-					Namespace: deleteHelmRelease.GetNamespace(),
+					Name:      deletedHelmRelease.GetName(),
+					Namespace: deletedHelmRelease.GetNamespace(),
 				},
 			},
 			expectedTx: transaction{
 				clusterName:     "anyCluster",
-				object:          deleteHelmRelease,
+				object:          deletedHelmRelease,
 				transactionType: models.TransactionTypeDelete,
 				config:          configuration.HelmReleaseObjectKind,
 			},
 			errPattern:   "",
 			shouldHaveTX: true,
 		},
+		{
+			name:   "can reconcile delete requests for existing clusterrole with rules",
+			object: deletedClusterRoleWithRules,
+			kind:   configuration.ClusterRoleObjectKind,
+			request: ctrl.Request{
+				NamespacedName: types.NamespacedName{
+					Name: deletedClusterRoleWithRules.GetName(),
+				},
+			},
+			expectedTx: transaction{
+				clusterName:     "anyCluster",
+				object:          deletedClusterRoleWithRules,
+				transactionType: models.TransactionTypeDelete,
+				config:          configuration.ClusterRoleObjectKind,
+			},
+			errPattern:   "",
+			shouldHaveTX: true,
+		},
+		{
+			name:   "can reconcile delete requests for existing clusterrole without rules",
+			object: deletedClusterRoleWithoutRules,
+			kind:   configuration.ClusterRoleObjectKind,
+			request: ctrl.Request{
+				NamespacedName: types.NamespacedName{
+					Name: deletedClusterRoleWithoutRules.GetName(),
+				},
+			},
+			expectedTx: transaction{
+				clusterName:     "anyCluster",
+				object:          deletedClusterRoleWithoutRules,
+				transactionType: models.TransactionTypeDelete,
+				config:          configuration.ClusterRoleObjectKind,
+			},
+			errPattern:   "",
+			shouldHaveTX: true,
+		},
+		{
+			name:   "can reconcile delete requests for already deleted clusterrole",
+			object: deletedClusterRoleManuallyConstructed,
+			kind:   configuration.ClusterRoleObjectKind,
+			request: ctrl.Request{
+				NamespacedName: types.NamespacedName{
+					Name: deletedClusterRoleManuallyConstructed.GetName(),
+				},
+			},
+			expectedTx: transaction{
+				clusterName:     "anyCluster",
+				object:          deletedClusterRoleManuallyConstructed,
+				transactionType: models.TransactionTypeDelete,
+				config:          configuration.ClusterRoleObjectKind,
+			},
+			errPattern:   "",
+			shouldHaveTX: true,
+		},
 		{
 			name:   "does not retain objects that do not pass the filter func",
 			object: createdOrUpdatedHelmRelease,
@@ -239,12 +306,12 @@ func assertObjectTransaction(t *testing.T, actual models.ObjectTransaction, expe
 	assert.Equal(t, expected.TransactionType(), actual.TransactionType(), "different tx type")
 	assert.Equal(t, expected.Object().GetName(), actual.Object().GetName(), "different object")
 
-	cat, err := expected.Object().GetCategory()
+	cat, err := actual.Object().GetCategory()
 	if err != nil {
 		t.Fatalf("could not get category: %v", err)
 	}
 
-	expectedCat, err := actual.Object().GetCategory()
+	expectedCat, err := expected.Object().GetCategory()
 	if err != nil {
 		t.Fatalf("could not get category: %v", err)
 	}

pkg/query/configuration/objectkind.go

@@ -32,6 +32,7 @@ const (
 	CategoryEvent      ObjectCategory = "event"
 	CategoryGitopsSet  ObjectCategory = "gitopsset"
 	CategoryTemplate   ObjectCategory = "template"
+	CategoryRBAC       ObjectCategory = "rbac"
 )
 
 type ObjectKind struct {
@@ -165,27 +166,31 @@ var (
 			return &rbacv1.Role{}
 		},
 		AddToSchemeFunc: rbacv1.AddToScheme,
+		Category:        CategoryRBAC,
 	}
 	ClusterRoleObjectKind = ObjectKind{
 		Gvk: rbacv1.SchemeGroupVersion.WithKind("ClusterRole"),
 		NewClientObjectFunc: func() client.Object {
 			return &rbacv1.ClusterRole{}
 		},
 		AddToSchemeFunc: rbacv1.AddToScheme,
+		Category:        CategoryRBAC,
 	}
 	RoleBindingObjectKind = ObjectKind{
 		Gvk: rbacv1.SchemeGroupVersion.WithKind("RoleBinding"),
 		NewClientObjectFunc: func() client.Object {
 			return &rbacv1.RoleBinding{}
 		},
 		AddToSchemeFunc: rbacv1.AddToScheme,
+		Category:        CategoryRBAC,
 	}
 	ClusterRoleBindingObjectKind = ObjectKind{
 		Gvk: rbacv1.SchemeGroupVersion.WithKind("ClusterRoleBinding"),
 		NewClientObjectFunc: func() client.Object {
 			return &rbacv1.ClusterRoleBinding{}
 		},
 		AddToSchemeFunc: rbacv1.AddToScheme,
+		Category:        CategoryRBAC,
 	}
 
 	PolicyAgentAuditEventObjectKind = ObjectKind{

pkg/query/internal/models/role.go

@@ -32,6 +32,26 @@ func (o Role) GetID() string {
 	return fmt.Sprintf("%s/%s/%s/%s", o.Cluster, o.Namespace, o.Kind, o.Name)
 }
 
+func (o Role) IsValidID() (bool, error) {
+	if o.Cluster == "" {
+		return false, fmt.Errorf("missing cluster field")
+	}
+
+	if o.Name == "" {
+		return false, fmt.Errorf("missing name field")
+	}
+
+	if o.Kind == "Role" && o.Namespace == "" {
+		return false, fmt.Errorf("missing namespace field")
+	}
+
+	if o.Kind == "" {
+		return false, fmt.Errorf("missing kind field")
+	}
+
+	return true, nil
+}
+
 func (o Role) Validate() error {
 	if o.Cluster == "" {
 		return fmt.Errorf("missing cluster field")

pkg/query/rolecollector/rolecollector.go

@@ -80,14 +80,16 @@ func processRecords(objectTransactions []models.ObjectTransaction, store store.S
 				return fmt.Errorf("cannot create role: %w", err)
 			}
 
-			if len(role.GetRules()) == 0 {
-				// Certain roles have no policy rules for some reason.
-				// Possibly related to the rbac.authorization.k8s.io/aggregate-to-gitops-reader label?
+			if obj.TransactionType() == models.TransactionTypeDelete {
+				rolesToDelete = append(rolesToDelete, role.ToModel())
 				continue
 			}
 
-			if obj.TransactionType() == models.TransactionTypeDelete {
-				rolesToDelete = append(rolesToDelete, role.ToModel())
+			// Explorer should support aggregated clusteroles.
+			// Related issue: https://github.com/weaveworks/weave-gitops-enterprise/issues/3443
+			if len(role.GetRules()) == 0 {
+				// Certain roles have no policy rules for some reason.
+				// Possibly related to the rbac.authorization.k8s.io/aggregate-to-gitops-reader label?
 				continue
 			}
 

pkg/query/rolecollector/rolecollector_test.go

@@ -39,14 +39,18 @@ func TestRoleCollector_defaultProcessRecords(t *testing.T) {
 		{
 			name: "can process non-empty roles collection with no errors",
 			objectRecords: []models.ObjectTransaction{
-				testutils.NewObjectTransaction("anyCluster", testutils.NewRole("createdOrUpdatedRole", clusterName), models.TransactionTypeUpsert),
-				testutils.NewObjectTransaction("anyCluster", testutils.NewRole("deletedRole", clusterName, func(hr *rbacv1.Role) {
+				testutils.NewObjectTransaction("anyCluster", testutils.NewRole("createdOrUpdatedRole", clusterName, true), models.TransactionTypeUpsert),
+				testutils.NewObjectTransaction("anyCluster", testutils.NewRole("deletedRole", clusterName, true, func(r *rbacv1.Role) {
 					now := metav1.Now()
-					hr.DeletionTimestamp = &now
+					r.DeletionTimestamp = &now
 				}), models.TransactionTypeDelete),
-				testutils.NewObjectTransaction("anyCluster2", testutils.NewRole("deletedRole2", clusterName, func(hr *rbacv1.Role) {
+				testutils.NewObjectTransaction("anyCluster2", testutils.NewRole("deletedRole2", clusterName, true, func(r *rbacv1.Role) {
 					now := metav1.Now()
-					hr.DeletionTimestamp = &now
+					r.DeletionTimestamp = &now
+				}), models.TransactionTypeDelete),
+				testutils.NewObjectTransaction("anyCluster", testutils.NewRole("deletedRole3", clusterName, true, func(r *rbacv1.Role) {
+					now := metav1.Now()
+					r.DeletionTimestamp = &now
 				}), models.TransactionTypeDelete),
 				testutils.NewObjectTransaction("anyCluster3", nil, models.TransactionTypeDeleteAll),
 			},
@@ -57,6 +61,27 @@ func TestRoleCollector_defaultProcessRecords(t *testing.T) {
 			},
 			errPattern: "",
 		},
+		{
+			name: "can process non-empty cluster roles collection with no errors",
+			objectRecords: []models.ObjectTransaction{
+				testutils.NewObjectTransaction("anyCluster", testutils.NewClusterRole("createdOrUpdatedRole", true), models.TransactionTypeUpsert),
+				testutils.NewObjectTransaction("anyCluster", testutils.NewClusterRole("deletedRole", true, func(cr *rbacv1.ClusterRole) {
+					now := metav1.Now()
+					cr.DeletionTimestamp = &now
+				}), models.TransactionTypeDelete),
+				testutils.NewObjectTransaction("anyCluster2", testutils.NewClusterRole("deletedRole2", false, func(cr *rbacv1.ClusterRole) {
+					now := metav1.Now()
+					cr.DeletionTimestamp = &now
+				}), models.TransactionTypeDelete),
+				testutils.NewObjectTransaction("anyCluster3", nil, models.TransactionTypeDeleteAll),
+			},
+			expectedStoreNumCalls: map[models.TransactionType]int{
+				models.TransactionTypeDelete:    2,
+				models.TransactionTypeUpsert:    2,
+				models.TransactionTypeDeleteAll: 2,
+			},
+			errPattern: "",
+		},
 	}
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {

pkg/query/store/sqlite.go

@@ -365,8 +365,8 @@ func (i *SQLiteStore) DeleteRoles(ctx context.Context, roles []models.Role) (err
 	defer recordMetrics(metrics.DeleteRolesAction, time.Now(), err)
 
 	for _, role := range roles {
-		if err := role.Validate(); err != nil {
-			return fmt.Errorf("invalid role: %w", err)
+		if _, err := role.IsValidID(); err != nil {
+			return fmt.Errorf("invalid role ID: %w", err)
 		}
 
 		where := i.db.Where(