fix: apply managed-by label for namespaceManagement tenant namespaces…#2120
Open
nmirasch wants to merge 3 commits intoargoproj-labs:masterfrom
Open
fix: apply managed-by label for namespaceManagement tenant namespaces…#2120nmirasch wants to merge 3 commits intoargoproj-labs:masterfrom
nmirasch wants to merge 3 commits intoargoproj-labs:masterfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughLabels tenant Namespaces with the Argo CD managed-by namespace during role reconciliation, changes RBAC-cleanup skip checks to compare the namespace's managed-by label against the ArgoCD instance namespace, adjusts NamespaceManagement reconcile/delete/update handling, adds tests and e2e start env var to enable namespace-scoped runs. Changes
Sequence DiagramsequenceDiagram
participant ArgoCD as ArgoCD Instance
participant NMCtrl as NamespaceManagement Controller
participant RoleCtrl as Role Reconciler
participant Namespace as Tenant Namespace
participant RBAC as RBAC Resources
ArgoCD->>NMCtrl: spec.namespaceManagement / NamespaceManagement CR
NMCtrl->>NMCtrl: validate NM targeting this ArgoCD instance
NMCtrl->>RoleCtrl: trigger role reconciliation for app-controller
RoleCtrl->>Namespace: Get Namespace object
alt managed-by label missing or differs
RoleCtrl->>Namespace: Patch labels -> set managed-by = ArgoCD namespace
Namespace-->>RoleCtrl: Return updated Namespace
end
RoleCtrl->>RBAC: Create/Update Role & RoleBinding in tenant namespace
RBAC-->>RoleCtrl: Reconciled
NMCtrl->>RBAC: On disable/cleanup, compare namespace managed-by to ArgoCD namespace and skip deletion if mismatch
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
controllers/argocd/role.go (1)
118-133: Consider returning errors from label update failures.The current implementation logs errors but continues reconciliation when:
- The namespace cannot be fetched (line 122-123)
- The label update fails (line 129-131)
While non-fatal error handling can be intentional (e.g., if the namespace is terminating), silently continuing when the update fails means the label won't be applied until the next reconciliation cycle. This could delay Application discovery in the tenant namespace.
If the label is critical for Application discovery (which is the core fix in this PR), consider propagating the update error to trigger a requeue:
♻️ Optional: Propagate label update error
if cr.Namespace != namespace.Name && name == common.ArgoCDApplicationControllerComponent { ns := &corev1.Namespace{} if err := r.Get(context.TODO(), types.NamespacedName{Name: namespace.Name}, ns); err != nil { log.Error(err, "failed to get namespace for managed-by label", "namespace", namespace.Name) + // Continue - namespace might be terminating or temporarily unavailable } else if ns.Labels[common.ArgoCDManagedByLabel] != cr.Namespace { if ns.Labels == nil { ns.Labels = make(map[string]string) } ns.Labels[common.ArgoCDManagedByLabel] = cr.Namespace argoutil.LogResourceUpdate(log, ns, fmt.Sprintf("adding label '%s=%s'", common.ArgoCDManagedByLabel, cr.Namespace)) if err := r.Update(context.TODO(), ns); err != nil { - log.Error(err, fmt.Sprintf("failed to add label to namespace [%s]", namespace.Name)) + return nil, fmt.Errorf("failed to add managed-by label to namespace [%s]: %w", namespace.Name, err) } } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@controllers/argocd/role.go` around lines 118 - 133, The code currently logs failures from r.Get and r.Update when setting the common.ArgoCDManagedByLabel but continues reconciliation; change this to propagate those errors so the caller (Reconcile) will requeue. Specifically, in the block that checks common.ArgoCDApplicationControllerComponent, replace the log-only paths for r.Get(context.TODO(), types.NamespacedName{Name: namespace.Name}, ns) and r.Update(context.TODO(), ns) with returns of the encountered error (or wrap with context using fmt.Errorf) so the reconcile loop sees the failure; keep argoutil.LogResourceUpdate and label mutation logic intact but ensure any Get/Update error is returned from the enclosing reconcile function instead of just logged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@controllers/argocd/role.go`:
- Around line 118-133: The code currently logs failures from r.Get and r.Update
when setting the common.ArgoCDManagedByLabel but continues reconciliation;
change this to propagate those errors so the caller (Reconcile) will requeue.
Specifically, in the block that checks
common.ArgoCDApplicationControllerComponent, replace the log-only paths for
r.Get(context.TODO(), types.NamespacedName{Name: namespace.Name}, ns) and
r.Update(context.TODO(), ns) with returns of the encountered error (or wrap with
context using fmt.Errorf) so the reconcile loop sees the failure; keep
argoutil.LogResourceUpdate and label mutation logic intact but ensure any
Get/Update error is returned from the enclosing reconcile function instead of
just logged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 65ee0521-cb77-48e4-8b45-f915e9761ab7
📒 Files selected for processing (7)
Makefilecontrollers/argocd/argocd_controller.gocontrollers/argocd/namespaceManagement.gocontrollers/argocd/namespaceManagement_test.gocontrollers/argocd/role.gocontrollers/argocd/role_test.gotests/ginkgo/parallel/1-012_validate-managed-by-chain_test.go
akhilnittala
approved these changes
Mar 11, 2026
Mangaal
approved these changes
Mar 12, 2026
Contributor
There was a problem hiding this comment.
@nmirasch, Overall changes look good to me.
I was also testing this out locally.
I found a possible bug within the NamespaceManagement event handler
func (r *ReconcileArgoCD) handleNamespaceManagementUpdate(oldNSMgmt, newNSMgmt *argoproj.NamespaceManagement, k8sClient kubernetes.Interface) bool {
ns := &corev1.Namespace{}
if err := r.Get(context.TODO(), types.NamespacedName{Name: oldNSMgmt.Spec.ManagedBy}, ns); err != nil {
return false
}
// Skip Update if managed-by label exists and matches .spec.managedBy
if labelVal, labelExists := ns.Labels[common.ArgoCDManagedByLabel]; labelExists && labelVal == oldNSMgmt.Spec.ManagedBy {
log.Info(fmt.Sprintf("Namespace %s still managed by same ArgoCD instance via label, skipping update cleanup", oldNSMgmt.Namespace))
return false
}
func (r *ReconcileArgoCD) handleNamespaceManagementDelete(nsMgmt *argoproj.NamespaceManagement, k8sClient kubernetes.Interface) bool {
ns := &corev1.Namespace{}
if err := r.Get(context.TODO(), types.NamespacedName{Name: nsMgmt.Spec.ManagedBy}, ns); err != nil {
return false
}
// Skip cleanup if managed-by label exists and matches .spec.managedBy
if labelVal, labelExists := ns.Labels[common.ArgoCDManagedByLabel]; labelExists && labelVal == nsMgmt.Spec.ManagedBy {
log.Info(fmt.Sprintf("Namespace %s still managed by same ArgoCD instance via label, skipping delete cleanup", nsMgmt.Namespace))
return false
}
In both cases, we fetch the namespace name by .spec.managedBy, which is the Argo CD instance namespace, and then check argocd.argoproj.io/managed-by on that namespace.
I think this should instead fetch the managed tenant namespace:
The managed-by label is expected on the managed namespace, not on the Argo CD instance namespace. This can cause a cleanup that may remove RBAC unexpectedly.
We can include the fix in this pr or have a separate pr for this issue
CC: @svghadi
https://github.com/argoproj-labs/argocd-operator/blob/master/controllers/argocd/util.go#L2080-L2085
… and fix related bugs (argoproj-labs#2039) Signed-off-by: nmirasch <neus.miras@gmail.com>
Signed-off-by: nmirasch <neus.miras@gmail.com>
…handlers Signed-off-by: nmirasch <neus.miras@gmail.com>
nmirasch
force-pushed
the
GITOPS-9071_tenant_namespace_not_labeled
branch
from
3061568 to
2604f18
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
… and fix related bugs (#2039)
What type of PR is this?
/kind bug
What does this PR do / why we need it:
Namespace-scoped Argo CD with spec.namespaceManagement was not labeling the tenant namespace with argocd.argoproj.io/managed-by, so Applications in that namespace were not discovered. This PR fixes that by applying the managed-by label from the role controller when reconciling the application-controller role for managed namespaces
Changes include related fixes (single status update in NamespaceManagement, correct namespace in RBAC skip logic), unit and E2E testing
Which issue(s) this PR fixes:
Fixes #2039?
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores