From 34b38a465d428ca507cc87bad5b7058a1886a976 Mon Sep 17 00:00:00 2001 From: Jonathan West Date: Fri, 25 Sep 2026 04:37:10 -0400 Subject: [PATCH] feat: harden image-updater cluster-scoped ArgoCD check Signed-off-by: Jonathan West --- .../controllers/argocd/image_updater.go | 28 +++++++++++++++---- .../controllers/argocd/image_updater_test.go | 3 ++ argocd-operator/controllers/argocd/util.go | 4 ++- 3 files changed, 28 insertions(+), 7 deletions(-) diff --git a/argocd-operator/controllers/argocd/image_updater.go b/argocd-operator/controllers/argocd/image_updater.go index 391cbf42f0f..a35e0c1c516 100644 --- a/argocd-operator/controllers/argocd/image_updater.go +++ b/argocd-operator/controllers/argocd/image_updater.go @@ -888,6 +888,10 @@ func (r *ReconcileArgoCD) reconcileRoleHelper(cr *argoproj.ArgoCD, desiredRole c existingRole := reflect.New(reflect.TypeOf(desiredRole).Elem()).Interface().(client.Object) namespace := cr.Namespace + // Re-check the cluster-config gate here for ClusterRole, rather than trusting the caller. + shouldExist := cr.Spec.ImageUpdater.Enabled + deleteReason := "image updater is disabled" + switch r := desiredRole.(type) { case *rbacv1.Role: if ns := desiredRole.GetNamespace(); ns != "" { @@ -895,6 +899,10 @@ func (r *ReconcileArgoCD) reconcileRoleHelper(cr *argoproj.ArgoCD, desiredRole c } case *rbacv1.ClusterRole: namespace = "" + if shouldExist && !argoutil.IsNamespaceClusterConfigNamespace(cr.Namespace) { + shouldExist = false + deleteReason = fmt.Sprintf("namespace %q is not allowed to host cluster-scoped Argo CD resources", cr.Namespace) + } default: return nil, fmt.Errorf("unsupported type for reconcileRoleResource, got %T", r) } @@ -905,7 +913,7 @@ func (r *ReconcileArgoCD) reconcileRoleHelper(cr *argoproj.ArgoCD, desiredRole c } // role does not exist and shouldn't, nothing to do here - if !cr.Spec.ImageUpdater.Enabled { + if !shouldExist { return nil, nil } @@ -926,11 +934,11 @@ func (r *ReconcileArgoCD) reconcileRoleHelper(cr *argoproj.ArgoCD, desiredRole c } // role exists but shouldn't, so it should be deleted - if !cr.Spec.ImageUpdater.Enabled { + if !shouldExist { if clusterRole, ok := existingRole.(*rbacv1.ClusterRole); ok && !argoutil.CheckClusterRoleOwnership(clusterRole, cr) { return nil, nil } - argoutil.LogResourceDeletion(log, existingRole, "image updater is disabled") + argoutil.LogResourceDeletion(log, existingRole, deleteReason) return nil, r.Delete(context.TODO(), existingRole) } @@ -979,9 +987,17 @@ func (r *ReconcileArgoCD) reconcileRoleBindingHelper(cr *argoproj.ArgoCD, desire return fmt.Errorf("unsupported type for reconcileRoleBindingResource resource, got %T", desiredRoleBinding) } + // Re-check the cluster-config gate here for ClusterRoleBinding, rather than trusting the caller. + shouldExist := cr.Spec.ImageUpdater.Enabled + deleteReason := "image updater is disabled" + namespace := cr.Namespace if _, ok := desiredRoleBinding.(*rbacv1.ClusterRoleBinding); ok { namespace = "" + if shouldExist && !argoutil.IsNamespaceClusterConfigNamespace(cr.Namespace) { + shouldExist = false + deleteReason = fmt.Sprintf("namespace %q is not allowed to host cluster-scoped Argo CD resources", cr.Namespace) + } } else if ns := desiredRoleBinding.GetNamespace(); ns != "" { namespace = ns } @@ -993,7 +1009,7 @@ func (r *ReconcileArgoCD) reconcileRoleBindingHelper(cr *argoproj.ArgoCD, desire } // roleBinding does not exist and shouldn't, nothing to do here - if !cr.Spec.ImageUpdater.Enabled { + if !shouldExist { return nil } @@ -1011,11 +1027,11 @@ func (r *ReconcileArgoCD) reconcileRoleBindingHelper(cr *argoproj.ArgoCD, desire } // roleBinding exists but shouldn't, so it should be deleted - if !cr.Spec.ImageUpdater.Enabled { + if !shouldExist { if clusterRoleBinding, ok := existingRoleBinding.(*rbacv1.ClusterRoleBinding); ok && !argoutil.CheckClusterRoleBindingOwnership(clusterRoleBinding, cr) { return nil } - argoutil.LogResourceDeletion(log, existingRoleBinding, "image updater is disabled") + argoutil.LogResourceDeletion(log, existingRoleBinding, deleteReason) return r.Delete(context.TODO(), existingRoleBinding) } diff --git a/argocd-operator/controllers/argocd/image_updater_test.go b/argocd-operator/controllers/argocd/image_updater_test.go index 5bf50c66c25..c992be532b6 100644 --- a/argocd-operator/controllers/argocd/image_updater_test.go +++ b/argocd-operator/controllers/argocd/image_updater_test.go @@ -95,6 +95,7 @@ func TestReconcileImageUpdater_CreateClusterRoles(t *testing.T) { a := makeTestArgoCD(func(a *argoproj.ArgoCD) { a.Spec.ImageUpdater.Enabled = true }) + allowClusterConfigNamespaces(t, a.Namespace) resObjs := []client.Object{a} subresObjs := []client.Object{a} @@ -207,6 +208,7 @@ func TestReconcileImageUpdater_CreateClusterRoleBinding(t *testing.T) { a := makeTestArgoCD(func(a *argoproj.ArgoCD) { a.Spec.ImageUpdater.Enabled = true }) + allowClusterConfigNamespaces(t, a.Namespace) resObjs := []client.Object{a} subresObjs := []client.Object{a} @@ -507,6 +509,7 @@ func TestDeleteImageUpdaterClusterRBAC(t *testing.T) { a := makeTestArgoCD(func(a *argoproj.ArgoCD) { a.Spec.ImageUpdater.Enabled = true }) + allowClusterConfigNamespaces(t, a.Namespace) resObjs := []client.Object{a} subresObjs := []client.Object{a} diff --git a/argocd-operator/controllers/argocd/util.go b/argocd-operator/controllers/argocd/util.go index 126e54b1f81..939aeb1b806 100644 --- a/argocd-operator/controllers/argocd/util.go +++ b/argocd-operator/controllers/argocd/util.go @@ -2172,8 +2172,10 @@ func (r *ReconcileArgoCD) reconcileDeploymentHelper(cr *argoproj.ArgoCD, desired func (r *ReconcileArgoCD) reconcileGitOpsPromoter(cr *argoproj.ArgoCD) error { log.Info("reconciling GitOps Promoter resources") + // Diagnostic only, not a guard: reconciliation proceeds regardless, since each + // cluster-scoped Promoter resource re-checks IsNamespaceClusterConfigNamespace itself. if cr.Spec.Promoter.IsEnabled() && !argoutil.IsNamespaceClusterConfigNamespace(cr.Namespace) { - log.Info("Warning: will not reconcile GitOps Promoter because namespace is not allowed to deploy cluster scoped ArgoCDs") + log.Info("namespace is not allowed to host cluster-scoped Argo CD resources; GitOps Promoter's cluster-scoped resources will not be created (existing ones owned by this instance will be removed)") } controllerCompName := string(argoproj.PromoterComponentTypeControllerManager)