Skip to content

Commit 83d0e6d

Browse files
akhilnittalaakhilnittala
authored
fix: restoring tracking labels for cleaning up of orphaned roles and rolebindings (#2082)
Signed-off-by: akhil nittala <nakhil@redhat.com>
1 parent 99b07bc commit 83d0e6d

2 files changed

Lines changed: 325 additions & 0 deletions

File tree

‎controllers/argocd/argocd_controller.go‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -35,10 +35,15 @@ import (
3535
"k8s.io/apimachinery/pkg/types"
3636
"k8s.io/client-go/kubernetes"
3737

38+
errs "errors"
39+
3840
ctrl "sigs.k8s.io/controller-runtime"
3941
"sigs.k8s.io/controller-runtime/pkg/client"
4042
logr "sigs.k8s.io/controller-runtime/pkg/log"
4143
"sigs.k8s.io/controller-runtime/pkg/reconcile"
44+
45+
rbacv1 "k8s.io/api/rbac/v1"
46+
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
4247
)
4348

4449
type lock struct {
@@ -285,6 +290,10 @@ func (r *ReconcileArgoCD) internalReconcile(ctx context.Context, request ctrl.Re
285290
}
286291
}
287292

293+
if err = r.restoreTrackingLabelsForOrphanedNamespaces(ctx, argocd); err != nil {
294+
return reconcile.Result{}, argocd, argoCDStatus, err
295+
}
296+
288297
if err = r.setManagedNamespaces(argocd); err != nil {
289298
return reconcile.Result{}, argocd, argoCDStatus, err
290299
}
@@ -367,3 +376,108 @@ func (r *ReconcileArgoCD) SetupWithManager(mgr ctrl.Manager) error {
367376
r.setResourceWatches(bldr, r.clusterResourceMapper, r.tlsSecretMapper, r.namespaceResourceMapper, r.clusterSecretResourceMapper, r.applicationSetSCMTLSConfigMapMapper, r.nmMapper)
368377
return bldr.Complete(r)
369378
}
379+
380+
func (r *ReconcileArgoCD) restoreTrackingLabelsForOrphanedNamespaces(ctx context.Context, cr *argoproj.ArgoCD) error {
381+
// List all Roles owned by this ArgoCD CR across all namespaces
382+
roles := &rbacv1.RoleList{}
383+
if err := r.List(ctx, roles, client.MatchingLabels{common.ArgoCDKeyPartOf: common.ArgoCDAppName, common.ArgoCDKeyManagedBy: cr.Name}, client.InNamespace(metav1.NamespaceAll)); err != nil {
384+
return err
385+
}
386+
var aggregatedErr error
387+
for _, role := range roles.Items {
388+
// Skip ArgoCD namespace (implicitly tracked)
389+
if role.Namespace == cr.Namespace {
390+
continue
391+
}
392+
// Strict orphan validation
393+
if !isOrphanedRole(&role, cr) {
394+
continue
395+
}
396+
requiredLabels := requiredTrackingLabelsForRole(&role, cr)
397+
if len(requiredLabels) == 0 {
398+
continue
399+
}
400+
// Fetch namespace
401+
namespace := &corev1.Namespace{}
402+
if err := r.Get(ctx, types.NamespacedName{Name: role.Namespace}, namespace); err != nil {
403+
if !errors.IsNotFound(err) {
404+
aggregatedErr = errs.Join(aggregatedErr, err)
405+
}
406+
continue
407+
}
408+
// Add only missing labels
409+
if addMissingLabels(namespace, requiredLabels) {
410+
argoutil.LogResourceUpdate(log, namespace, "restoring ArgoCD tracking labels for orphaned namespace")
411+
if err := r.Update(ctx, namespace); err != nil {
412+
aggregatedErr = errs.Join(aggregatedErr, err)
413+
}
414+
}
415+
}
416+
return aggregatedErr
417+
}
418+
419+
// Orphan validation helpers
420+
// Core predicate that guarantees convergence and safety
421+
func isOrphanedRole(role *rbacv1.Role, cr *argoproj.ArgoCD) bool {
422+
isAppSetRole := role.Name == getResourceNameForApplicationSetSourceNamespaces(cr)
423+
isAppRole := role.Name == getRoleNameForApplicationSourceNamespaces(role.Namespace, cr)
424+
425+
if !isAppSetRole && !isAppRole {
426+
return false
427+
}
428+
if !hasApplicationScopedRules(role.Rules) {
429+
return false
430+
}
431+
return true
432+
}
433+
434+
// RBAC scope validation
435+
func hasApplicationScopedRules(rules []rbacv1.PolicyRule) bool {
436+
const argoCDAPIGroup = "argoproj.io"
437+
for _, rule := range rules {
438+
if !contains(rule.APIGroups, argoCDAPIGroup) {
439+
continue
440+
}
441+
if contains(rule.Resources, "*") {
442+
return true
443+
}
444+
for _, res := range rule.Resources {
445+
switch res {
446+
case
447+
"applications",
448+
"applications/status",
449+
"applicationsets",
450+
"applicationsets/status":
451+
return true
452+
}
453+
}
454+
}
455+
return false
456+
}
457+
458+
// Namespace mutation helpers
459+
func requiredTrackingLabelsForRole(role *rbacv1.Role, cr *argoproj.ArgoCD) map[string]string {
460+
labels := map[string]string{}
461+
if role.Name == getResourceNameForApplicationSetSourceNamespaces(cr) {
462+
labels[common.ArgoCDApplicationSetManagedByClusterArgoCDLabel] = cr.Namespace
463+
}
464+
465+
if role.Name == getRoleNameForApplicationSourceNamespaces(role.Namespace, cr) {
466+
labels[common.ArgoCDManagedByClusterArgoCDLabel] = cr.Namespace
467+
}
468+
return labels
469+
}
470+
471+
func addMissingLabels(ns *corev1.Namespace, required map[string]string) bool {
472+
if ns.Labels == nil {
473+
ns.Labels = map[string]string{}
474+
}
475+
changed := false
476+
for k, v := range required {
477+
if _, exists := ns.Labels[k]; !exists {
478+
ns.Labels[k] = v
479+
changed = true
480+
}
481+
}
482+
return changed
483+
}

‎controllers/argocd/argocd_controller_test.go‎

Lines changed: 211 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -17,6 +17,7 @@ package argocd
1717
import (
1818
"context"
1919
"fmt"
20+
"maps"
2021
"strings"
2122
"testing"
2223
"time"
@@ -600,3 +601,213 @@ func TestReconcileArgoCD_Cleanup_RBACs_When_NamespaceManagement_Disabled(t *test
600601
assert.NoError(t, err)
601602
assert.Equal(t, "", string(updatedSecret.Data["namespaces"]))
602603
}
604+
605+
func Test_restoreTrackingLabelsForOrphanedNamespaces(t *testing.T) {
606+
ctx := context.Background()
607+
// Test setup
608+
argocd := &argoproj.ArgoCD{
609+
ObjectMeta: metav1.ObjectMeta{
610+
Name: "argocd",
611+
Namespace: "argocd",
612+
},
613+
}
614+
615+
nsWithoutAppsetLabel := &corev1.Namespace{
616+
ObjectMeta: metav1.ObjectMeta{
617+
Name: "ns-without-appset-label",
618+
Labels: map[string]string{
619+
// intentionally missing appset tracking label
620+
common.ArgoCDManagedByClusterArgoCDLabel: argocd.Namespace,
621+
},
622+
},
623+
}
624+
625+
nsWithoutAppsetLabelButWildcard := &corev1.Namespace{
626+
ObjectMeta: metav1.ObjectMeta{
627+
Name: "ns-without-appset-label-but-wildcard",
628+
Labels: map[string]string{
629+
// intentionally missing appset tracking label
630+
common.ArgoCDManagedByClusterArgoCDLabel: argocd.Namespace,
631+
},
632+
},
633+
}
634+
635+
nsWithAppsetLabel := &corev1.Namespace{
636+
ObjectMeta: metav1.ObjectMeta{
637+
Name: "ns-with-appset-label",
638+
Labels: map[string]string{
639+
common.ArgoCDManagedByClusterArgoCDLabel: argocd.Namespace,
640+
common.ArgoCDApplicationSetManagedByClusterArgoCDLabel: argocd.Namespace,
641+
},
642+
},
643+
}
644+
645+
nsRandom := &corev1.Namespace{
646+
ObjectMeta: metav1.ObjectMeta{
647+
Name: "random-ns",
648+
Labels: map[string]string{
649+
common.ArgoCDManagedByClusterArgoCDLabel: argocd.Namespace,
650+
common.ArgoCDApplicationSetManagedByClusterArgoCDLabel: argocd.Namespace,
651+
},
652+
},
653+
}
654+
655+
// ArgoCD instance namespace (must never be mutated)
656+
argocdNamespace := &corev1.Namespace{
657+
ObjectMeta: metav1.ObjectMeta{
658+
Name: argocd.Namespace,
659+
},
660+
}
661+
662+
// RBAC rules required for orphan validation
663+
appScopedRules := []rbacv1.PolicyRule{
664+
{
665+
APIGroups: []string{"argoproj.io"},
666+
Resources: []string{"applicationsets"},
667+
Verbs: []string{"get", "list"},
668+
},
669+
}
670+
671+
// Roles
672+
appsetRoleName := getResourceNameForApplicationSetSourceNamespaces(argocd)
673+
674+
// AppSet role in namespace that already has the label
675+
appsetRoleInLabelledNS := &rbacv1.Role{
676+
ObjectMeta: metav1.ObjectMeta{
677+
Name: appsetRoleName,
678+
Namespace: nsWithAppsetLabel.Name,
679+
Labels: map[string]string{
680+
common.ArgoCDKeyManagedBy: argocd.Name,
681+
common.ArgoCDKeyPartOf: common.ArgoCDAppName,
682+
},
683+
},
684+
Rules: appScopedRules,
685+
}
686+
687+
// AppSet role in namespace missing the label (should trigger restore)
688+
appsetRoleInUnlabelledNS := &rbacv1.Role{
689+
ObjectMeta: metav1.ObjectMeta{
690+
Name: appsetRoleName,
691+
Namespace: nsWithoutAppsetLabel.Name,
692+
Labels: map[string]string{
693+
common.ArgoCDKeyManagedBy: argocd.Name,
694+
common.ArgoCDKeyPartOf: common.ArgoCDAppName,
695+
},
696+
},
697+
Rules: appScopedRules,
698+
}
699+
700+
// AppSet role in namespace missing the label (should trigger restore) but having resources has *
701+
appSetRoleInUnlabelledNSWithWildcard := &rbacv1.Role{
702+
ObjectMeta: metav1.ObjectMeta{
703+
Name: appsetRoleName,
704+
Namespace: nsWithoutAppsetLabelButWildcard.Name,
705+
Labels: map[string]string{
706+
common.ArgoCDKeyManagedBy: argocd.Name,
707+
common.ArgoCDKeyPartOf: common.ArgoCDAppName,
708+
},
709+
},
710+
Rules: []rbacv1.PolicyRule{
711+
{
712+
APIGroups: []string{"argoproj.io"},
713+
Resources: []string{"*"},
714+
Verbs: []string{"get", "list"},
715+
},
716+
},
717+
}
718+
719+
// Same role name but in ArgoCD namespace (must be ignored)
720+
roleInArgoCDNS := &rbacv1.Role{
721+
ObjectMeta: metav1.ObjectMeta{
722+
Name: appsetRoleName,
723+
Namespace: argocd.Namespace,
724+
Labels: map[string]string{
725+
common.ArgoCDKeyManagedBy: argocd.Name,
726+
common.ArgoCDKeyPartOf: common.ArgoCDAppName,
727+
},
728+
},
729+
Rules: appScopedRules,
730+
}
731+
732+
// Completely unrelated role (no rules, should be ignored)
733+
randomRole := &rbacv1.Role{
734+
ObjectMeta: metav1.ObjectMeta{
735+
Name: "random-role",
736+
Namespace: nsRandom.Name,
737+
Labels: map[string]string{
738+
common.ArgoCDKeyManagedBy: argocd.Name,
739+
common.ArgoCDKeyPartOf: common.ArgoCDAppName,
740+
},
741+
},
742+
}
743+
744+
// Fake client + reconciler
745+
resObjs := []client.Object{
746+
argocd,
747+
nsWithoutAppsetLabel,
748+
nsWithoutAppsetLabelButWildcard,
749+
nsWithAppsetLabel,
750+
nsRandom,
751+
argocdNamespace,
752+
appsetRoleInLabelledNS,
753+
appsetRoleInUnlabelledNS,
754+
appSetRoleInUnlabelledNSWithWildcard,
755+
roleInArgoCDNS,
756+
randomRole,
757+
}
758+
759+
subresObjs := append([]client.Object{}, resObjs...)
760+
runtimeObjs := []runtime.Object{}
761+
762+
scheme := makeTestReconcilerScheme(argoproj.AddToScheme)
763+
cl := makeTestReconcilerClient(scheme, resObjs, subresObjs, runtimeObjs)
764+
kubeClient := testclient.NewSimpleClientset()
765+
766+
reconciler := makeTestReconciler(cl, scheme, kubeClient)
767+
768+
// Capture original labels for immutability assertions
769+
origWithout := maps.Clone(nsWithoutAppsetLabel.Labels)
770+
origWithoutButWildcard := maps.Clone(nsWithoutAppsetLabelButWildcard.Labels)
771+
origWith := maps.Clone(nsWithAppsetLabel.Labels)
772+
origRandom := maps.Clone(nsRandom.Labels)
773+
774+
// Execute
775+
err := reconciler.restoreTrackingLabelsForOrphanedNamespaces(ctx, argocd)
776+
assert.NoError(t, err)
777+
778+
// Assertions
779+
// 1) Namespace with AppSet resources but missing label -> label restored
780+
updatedWithout := &corev1.Namespace{}
781+
assert.NoError(t, cl.Get(ctx, types.NamespacedName{Name: nsWithoutAppsetLabel.Name}, updatedWithout))
782+
783+
assert.Equal(t, origWithout[common.ArgoCDManagedByClusterArgoCDLabel], updatedWithout.Labels[common.ArgoCDManagedByClusterArgoCDLabel])
784+
785+
assert.Equal(t, argocd.Namespace, updatedWithout.Labels[common.ArgoCDApplicationSetManagedByClusterArgoCDLabel], "expected appset tracking label to be restored")
786+
787+
// 1b) Namespace with wildcard resources but missing label -> label restored
788+
updatedWithoutButWildcard := &corev1.Namespace{}
789+
assert.NoError(t, cl.Get(ctx, types.NamespacedName{Name: nsWithoutAppsetLabelButWildcard.Name}, updatedWithoutButWildcard))
790+
791+
assert.Equal(t, origWithoutButWildcard[common.ArgoCDManagedByClusterArgoCDLabel], updatedWithoutButWildcard.Labels[common.ArgoCDManagedByClusterArgoCDLabel])
792+
793+
assert.Equal(t, argocd.Namespace, updatedWithoutButWildcard.Labels[common.ArgoCDApplicationSetManagedByClusterArgoCDLabel], "expected appset tracking label to be restored")
794+
795+
// 2) Namespace already labelled -> unchanged
796+
updatedWith := &corev1.Namespace{}
797+
assert.NoError(t, cl.Get(ctx, types.NamespacedName{Name: nsWithAppsetLabel.Name}, updatedWith))
798+
assert.Equal(t, origWith, updatedWith.Labels)
799+
800+
// 3) Namespace with unrelated role -> unchanged
801+
updatedRandom := &corev1.Namespace{}
802+
assert.NoError(t, cl.Get(ctx, types.NamespacedName{Name: nsRandom.Name}, updatedRandom))
803+
assert.Equal(t, origRandom, updatedRandom.Labels)
804+
805+
// 4) ArgoCD instance namespace -> never labeled
806+
updatedArgoCDNS := &corev1.Namespace{}
807+
assert.NoError(t, cl.Get(ctx, types.NamespacedName{Name: argocd.Namespace}, updatedArgoCDNS))
808+
809+
if updatedArgoCDNS.Labels != nil {
810+
assert.NotContains(t, updatedArgoCDNS.Labels, common.ArgoCDApplicationSetManagedByClusterArgoCDLabel)
811+
assert.NotContains(t, updatedArgoCDNS.Labels, common.ArgoCDManagedByClusterArgoCDLabel)
812+
}
813+
}

0 commit comments

Comments
 (0)