fix: restoring tracking labels for cleaning up of orphaned roles and rolebindings

controllers/argocd/argocd_controller.go

@@ -35,6 +35,10 @@ import (
 	"k8s.io/apimachinery/pkg/types"
 	"k8s.io/client-go/kubernetes"
 
+	errs "errors"
+
+	rbacv1 "k8s.io/api/rbac/v1"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	ctrl "sigs.k8s.io/controller-runtime"
 	"sigs.k8s.io/controller-runtime/pkg/client"
 	logr "sigs.k8s.io/controller-runtime/pkg/log"
@@ -284,7 +288,9 @@ func (r *ReconcileArgoCD) internalReconcile(ctx context.Context, request ctrl.Re
 			return reconcile.Result{}, argocd, argoCDStatus, err
 		}
 	}
-
+	if err = r.restoreTrackingLabelsForOrphanedNamespaces(ctx, argocd); err != nil {
+		return reconcile.Result{}, argocd, argoCDStatus, err
+	}
 	if err = r.setManagedNamespaces(argocd); err != nil {
 		return reconcile.Result{}, argocd, argoCDStatus, err
 	}
@@ -367,3 +373,108 @@ func (r *ReconcileArgoCD) SetupWithManager(mgr ctrl.Manager) error {
 	r.setResourceWatches(bldr, r.clusterResourceMapper, r.tlsSecretMapper, r.namespaceResourceMapper, r.clusterSecretResourceMapper, r.applicationSetSCMTLSConfigMapMapper, r.nmMapper)
 	return bldr.Complete(r)
 }
+
+func (r *ReconcileArgoCD) restoreTrackingLabelsForOrphanedNamespaces(ctx context.Context, cr *argoproj.ArgoCD) error {
+	// List all Roles owned by this ArgoCD CR across all namespaces
+	roles := &rbacv1.RoleList{}
+	if err := r.List(ctx, roles, client.MatchingLabels{common.ArgoCDKeyPartOf: common.ArgoCDAppName, common.ArgoCDKeyManagedBy: cr.Name}, client.InNamespace(metav1.NamespaceAll)); err != nil {
+		return err
+	}
+	var aggregatedErr error
+	for _, role := range roles.Items {
+		// Skip ArgoCD namespace (implicitly tracked)
+		if role.Namespace == cr.Namespace {
+			continue
+		}
+		// Strict orphan validation
+		if !isOrphanedRole(&role, cr) {
+			continue
+		}
+		requiredLabels := requiredTrackingLabelsForRole(&role, cr)
+		if len(requiredLabels) == 0 {
+			continue
+		}
+		// Fetch namespace
+		namespace := &corev1.Namespace{}
+		if err := r.Get(ctx, types.NamespacedName{Name: role.Namespace}, namespace); err != nil {
+			if !errors.IsNotFound(err) {
+				aggregatedErr = errs.Join(aggregatedErr, err)
+			}
+			continue
+		}
+		// Add only missing labels
+		if addMissingLabels(namespace, requiredLabels) {
+			argoutil.LogResourceUpdate(log, namespace, "restoring ArgoCD tracking labels for orphaned namespace")
+			if err := r.Update(ctx, namespace); err != nil {
+				aggregatedErr = errs.Join(aggregatedErr, err)
+			}
+		}
+	}
+	return aggregatedErr
+}
+
+// Orphan validation helpers
+// Core predicate that guarantees convergence and safety
+func isOrphanedRole(role *rbacv1.Role, cr *argoproj.ArgoCD) bool {
+	isAppSetRole := role.Name == getResourceNameForApplicationSetSourceNamespaces(cr)
+	isAppRole := role.Name == getRoleNameForApplicationSourceNamespaces(role.Namespace, cr)
+
+	if !isAppSetRole && !isAppRole {
+		return false
+	}
+	if !hasApplicationScopedRules(role.Rules) {
+		return false
+	}
+	return true
+}
+
+// RBAC scope validation
+func hasApplicationScopedRules(rules []rbacv1.PolicyRule) bool {
+	const argoCDAPIGroup = "argoproj.io"
+	for _, rule := range rules {
+		if !contains(rule.APIGroups, argoCDAPIGroup) {
+			continue
+		}
+		if contains(rule.Resources, "*") {
+			return true
+		}
+		for _, res := range rule.Resources {
+			switch res {
+			case
+				"applications",
+				"applications/status",
+				"applicationsets",
+				"applicationsets/status":
+				return true
+			}
+		}
+	}
+	return false
+}
+
+// Namespace mutation helpers
+func requiredTrackingLabelsForRole(role *rbacv1.Role, cr *argoproj.ArgoCD) map[string]string {
+	labels := map[string]string{}
+	if role.Name == getResourceNameForApplicationSetSourceNamespaces(cr) {
+		labels[common.ArgoCDApplicationSetManagedByClusterArgoCDLabel] = cr.Namespace
+	}
+
+	if role.Name == getRoleNameForApplicationSourceNamespaces(role.Namespace, cr) {
+		labels[common.ArgoCDManagedByClusterArgoCDLabel] = cr.Namespace
+	}
+	return labels
+}
+
+func addMissingLabels(ns *corev1.Namespace, required map[string]string) bool {
+	if ns.Labels == nil {
+		ns.Labels = map[string]string{}
+	}
+	changed := false
+	for k, v := range required {
+		if _, exists := ns.Labels[k]; !exists {
+			ns.Labels[k] = v
+			changed = true
+		}
+	}
+	return changed
+}

controllers/argocd/argocd_controller_test.go

@@ -17,6 +17,7 @@ package argocd
 import (
 	"context"
 	"fmt"
+	"maps"
 	"strings"
 	"testing"
 	"time"
@@ -600,3 +601,213 @@ func TestReconcileArgoCD_Cleanup_RBACs_When_NamespaceManagement_Disabled(t *test
 	assert.NoError(t, err)
 	assert.Equal(t, "", string(updatedSecret.Data["namespaces"]))
 }
+
+func Test_restoreTrackingLabelsForOrphanedNamespaces(t *testing.T) {
+	ctx := context.Background()
+	// Test setup
+	argocd := &argoproj.ArgoCD{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "argocd",
+			Namespace: "argocd",
+		},
+	}
+
+	nsWithoutAppsetLabel := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "ns-without-appset-label",
+			Labels: map[string]string{
+				// intentionally missing appset tracking label
+				common.ArgoCDManagedByClusterArgoCDLabel: argocd.Namespace,
+			},
+		},
+	}
+
+	nsWithoutAppsetLabelButWildcard := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "ns-without-appset-label-but-wildcard",
+			Labels: map[string]string{
+				// intentionally missing appset tracking label
+				common.ArgoCDManagedByClusterArgoCDLabel: argocd.Namespace,
+			},
+		},
+	}
+
+	nsWithAppsetLabel := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "ns-with-appset-label",
+			Labels: map[string]string{
+				common.ArgoCDManagedByClusterArgoCDLabel:               argocd.Namespace,
+				common.ArgoCDApplicationSetManagedByClusterArgoCDLabel: argocd.Namespace,
+			},
+		},
+	}
+
+	nsRandom := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: "random-ns",
+			Labels: map[string]string{
+				common.ArgoCDManagedByClusterArgoCDLabel:               argocd.Namespace,
+				common.ArgoCDApplicationSetManagedByClusterArgoCDLabel: argocd.Namespace,
+			},
+		},
+	}
+
+	// ArgoCD instance namespace (must never be mutated)
+	argocdNamespace := &corev1.Namespace{
+		ObjectMeta: metav1.ObjectMeta{
+			Name: argocd.Namespace,
+		},
+	}
+
+	// RBAC rules required for orphan validation
+	appScopedRules := []rbacv1.PolicyRule{
+		{
+			APIGroups: []string{"argoproj.io"},
+			Resources: []string{"applicationsets"},
+			Verbs:     []string{"get", "list"},
+		},
+	}
+
+	// Roles
+	appsetRoleName := getResourceNameForApplicationSetSourceNamespaces(argocd)
+
+	// AppSet role in namespace that already has the label
+	appsetRoleInLabelledNS := &rbacv1.Role{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      appsetRoleName,
+			Namespace: nsWithAppsetLabel.Name,
+			Labels: map[string]string{
+				common.ArgoCDKeyManagedBy: argocd.Name,
+				common.ArgoCDKeyPartOf:    common.ArgoCDAppName,
+			},
+		},
+		Rules: appScopedRules,
+	}
+
+	// AppSet role in namespace missing the label (should trigger restore)
+	appsetRoleInUnlabelledNS := &rbacv1.Role{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      appsetRoleName,
+			Namespace: nsWithoutAppsetLabel.Name,
+			Labels: map[string]string{
+				common.ArgoCDKeyManagedBy: argocd.Name,
+				common.ArgoCDKeyPartOf:    common.ArgoCDAppName,
+			},
+		},
+		Rules: appScopedRules,
+	}
+
+	// AppSet role in namespace missing the label (should trigger restore) but having resources has *
+	appSetRoleInUnlabelledNSWithWildcard := &rbacv1.Role{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      appsetRoleName,
+			Namespace: nsWithoutAppsetLabelButWildcard.Name,
+			Labels: map[string]string{
+				common.ArgoCDKeyManagedBy: argocd.Name,
+				common.ArgoCDKeyPartOf:    common.ArgoCDAppName,
+			},
+		},
+		Rules: []rbacv1.PolicyRule{
+			{
+				APIGroups: []string{"argoproj.io"},
+				Resources: []string{"*"},
+				Verbs:     []string{"get", "list"},
+			},
+		},
+	}
+
+	// Same role name but in ArgoCD namespace (must be ignored)
+	roleInArgoCDNS := &rbacv1.Role{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      appsetRoleName,
+			Namespace: argocd.Namespace,
+			Labels: map[string]string{
+				common.ArgoCDKeyManagedBy: argocd.Name,
+				common.ArgoCDKeyPartOf:    common.ArgoCDAppName,
+			},
+		},
+		Rules: appScopedRules,
+	}
+
+	// Completely unrelated role (no rules, should be ignored)
+	randomRole := &rbacv1.Role{
+		ObjectMeta: metav1.ObjectMeta{
+			Name:      "random-role",
+			Namespace: nsRandom.Name,
+			Labels: map[string]string{
+				common.ArgoCDKeyManagedBy: argocd.Name,
+				common.ArgoCDKeyPartOf:    common.ArgoCDAppName,
+			},
+		},
+	}
+
+	// Fake client + reconciler
+	resObjs := []client.Object{
+		argocd,
+		nsWithoutAppsetLabel,
+		nsWithoutAppsetLabelButWildcard,
+		nsWithAppsetLabel,
+		nsRandom,
+		argocdNamespace,
+		appsetRoleInLabelledNS,
+		appsetRoleInUnlabelledNS,
+		appSetRoleInUnlabelledNSWithWildcard,
+		roleInArgoCDNS,
+		randomRole,
+	}
+
+	subresObjs := append([]client.Object{}, resObjs...)
+	runtimeObjs := []runtime.Object{}
+
+	scheme := makeTestReconcilerScheme(argoproj.AddToScheme)
+	cl := makeTestReconcilerClient(scheme, resObjs, subresObjs, runtimeObjs)
+	kubeClient := testclient.NewSimpleClientset()
+
+	reconciler := makeTestReconciler(cl, scheme, kubeClient)
+
+	// Capture original labels for immutability assertions
+	origWithout := maps.Clone(nsWithoutAppsetLabel.Labels)
+	origWithoutButWildcard := maps.Clone(nsWithoutAppsetLabelButWildcard.Labels)
+	origWith := maps.Clone(nsWithAppsetLabel.Labels)
+	origRandom := maps.Clone(nsRandom.Labels)
+
+	// Execute
+	err := reconciler.restoreTrackingLabelsForOrphanedNamespaces(ctx, argocd)
+	assert.NoError(t, err)
+
+	// Assertions
+	// 1) Namespace with AppSet resources but missing label -> label restored
+	updatedWithout := &corev1.Namespace{}
+	assert.NoError(t, cl.Get(ctx, types.NamespacedName{Name: nsWithoutAppsetLabel.Name}, updatedWithout))
+
+	assert.Equal(t, origWithout[common.ArgoCDManagedByClusterArgoCDLabel], updatedWithout.Labels[common.ArgoCDManagedByClusterArgoCDLabel])
+
+	assert.Equal(t, argocd.Namespace, updatedWithout.Labels[common.ArgoCDApplicationSetManagedByClusterArgoCDLabel], "expected appset tracking label to be restored")
+
+	// 1b) Namespace with wildcard resources but missing label -> label restored
+	updatedWithoutButWildcard := &corev1.Namespace{}
+	assert.NoError(t, cl.Get(ctx, types.NamespacedName{Name: nsWithoutAppsetLabelButWildcard.Name}, updatedWithoutButWildcard))
+
+	assert.Equal(t, origWithoutButWildcard[common.ArgoCDManagedByClusterArgoCDLabel], updatedWithoutButWildcard.Labels[common.ArgoCDManagedByClusterArgoCDLabel])
+
+	assert.Equal(t, argocd.Namespace, updatedWithoutButWildcard.Labels[common.ArgoCDApplicationSetManagedByClusterArgoCDLabel], "expected appset tracking label to be restored")
+
+	// 2) Namespace already labelled -> unchanged
+	updatedWith := &corev1.Namespace{}
+	assert.NoError(t, cl.Get(ctx, types.NamespacedName{Name: nsWithAppsetLabel.Name}, updatedWith))
+	assert.Equal(t, origWith, updatedWith.Labels)
+
+	// 3) Namespace with unrelated role -> unchanged
+	updatedRandom := &corev1.Namespace{}
+	assert.NoError(t, cl.Get(ctx, types.NamespacedName{Name: nsRandom.Name}, updatedRandom))
+	assert.Equal(t, origRandom, updatedRandom.Labels)
+
+	// 4) ArgoCD instance namespace -> never labeled
+	updatedArgoCDNS := &corev1.Namespace{}
+	assert.NoError(t, cl.Get(ctx, types.NamespacedName{Name: argocd.Namespace}, updatedArgoCDNS))
+
+	if updatedArgoCDNS.Labels != nil {
+		assert.NotContains(t, updatedArgoCDNS.Labels, common.ArgoCDApplicationSetManagedByClusterArgoCDLabel)
+		assert.NotContains(t, updatedArgoCDNS.Labels, common.ArgoCDManagedByClusterArgoCDLabel)
+	}
+}