Skip to content

fix(appset): do not trigger reconciliation on appsets not part of allowed namespaces when updating a cluster secret #25622

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
agaudreault merged 1 commit into from
Jan 8, 2026
+44 −8
Merged

fix(appset): do not trigger reconciliation on appsets not part of allowed namespaces when updating a cluster secret #25622

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 3 additions & 2 deletions applicationset/controllers/applicationset_controller.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -663,8 +663,9 @@ func (r *ApplicationSetReconciler) SetupWithManager(mgr ctrl.Manager, enableProg
Watches(
&corev1.Secret{},
&clusterSecretEventHandler{
Client: mgr.GetClient(),
Log: log.WithField("type", "createSecretEventHandler"),
Client: mgr.GetClient(),
Log: log.WithField("type", "createSecretEventHandler"),
ApplicationSetNamespaces: r.ApplicationSetNamespaces,
Copy link
Member

@agaudreault agaudreault Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems like the method WithEventFilter(ignoreNotAllowedNamespaces(r.ApplicationSetNamespaces)). should also apply to the Secret type.

// WithEventFilter sets the event filters, to filter which create/update/delete/generic events eventually
// trigger reconciliations. For example, filtering on whether the resource version has changed.
// Given predicate is added for all watched objects and thus must be able to deal with the type
// of all watched objects.

Do you have an explanation why it is not the case?

Copy link
Contributor Author

@OpenGuidou OpenGuidou Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This correctly filters the cluster secrets in the correct namespaces, however the code inside createSecretEventHandler does a list on ApplicationSets at cluster scope without any filter on namespaces.

This PR fixes that

Copy link
Member

@agaudreault agaudreault Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So it should use the object.GetNamespace() for to list on a specific namespace and not all namespace instead.

afaik, Appset cannot use cluster secrets from another namespace than where they are defined right?

Copy link
Contributor Author

@OpenGuidou OpenGuidou Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the case of Appset in any namespaces, they can.
Cluster secrets are in fact not allowed anywhere else than the controller namespace.
So we have this list of allowed namespaces for applicationsets as controller variable, that I'm just reusing here.

Copy link
Member

@agaudreault agaudreault Jan 8, 2026 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Gotcha, but I think what you are doing is not officially supported based on https://argo-cd.readthedocs.io/en/latest/operator-manual/applicationset/Appset-Any-Namespace/#cluster-scoped-argo-cd-installation, most likely because cluster resource is access is required, but the event handling should work correctly.

Your code is good, but a small optimization could be that if the len(ApplicationSetNamespaces) == 1, use a ListOption to only list on the single watched namespace instead of cluster scoped. This would need a new unit test for 1 vs multiple allowedNamespace.

Copy link
Member

@agaudreault agaudreault Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approving this PR still since it is an optimization and this PR by itself fixes the issue.

Copy link
Contributor Author

@OpenGuidou OpenGuidou Jan 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree and it forces to have the list/watch rbac at cluster level even if it's not required. I don't think that's the only place we would have to change that...
Thanks !

}).
Complete(r)
}
Expand Down
10 changes: 8 additions & 2 deletions applicationset/controllers/clustereventhandler.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ import (
"sigs.k8s.io/controller-runtime/pkg/client"
"sigs.k8s.io/controller-runtime/pkg/event"

"github.com/argoproj/argo-cd/v3/applicationset/utils"
"github.com/argoproj/argo-cd/v3/common"
argoprojiov1alpha1 "github.com/argoproj/argo-cd/v3/pkg/apis/application/v1alpha1"
)
Expand All @@ -22,8 +23,9 @@ import (
// requeue any related ApplicationSets.
type clusterSecretEventHandler struct {
// handler.EnqueueRequestForOwner
Log log.FieldLogger
Client client.Client
Log log.FieldLogger
Client client.Client
ApplicationSetNamespaces []string
}

func (h *clusterSecretEventHandler) Create(ctx context.Context, e event.CreateEvent, q workqueue.TypedRateLimitingInterface[reconcile.Request]) {
Expand Down Expand Up @@ -68,6 +70,10 @@ func (h *clusterSecretEventHandler) queueRelatedAppGenerators(ctx context.Contex

h.Log.WithField("count", len(appSetList.Items)).Info("listed ApplicationSets")
for _, appSet := range appSetList.Items {
if !utils.IsNamespaceAllowed(h.ApplicationSetNamespaces, appSet.GetNamespace()) {
// Ignore it as not part of the allowed list of namespaces in which to watch Appsets
continue
}
foundClusterGenerator := false
for _, generator := range appSet.Spec.Generators {
if generator.Clusters != nil {
Expand Down
37 changes: 33 additions & 4 deletions applicationset/controllers/clustereventhandler_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -137,7 +137,7 @@ func TestClusterEventHandler(t *testing.T) {
{
ObjectMeta: metav1.ObjectMeta{
Name: "my-app-set",
Namespace: "another-namespace",
Namespace: "argocd",
},
Spec: argov1alpha1.ApplicationSetSpec{
Generators: []argov1alpha1.ApplicationSetGenerator{
Expand Down Expand Up @@ -171,9 +171,37 @@ func TestClusterEventHandler(t *testing.T) {
},
},
expectedRequests: []reconcile.Request{
{NamespacedName: types.NamespacedName{Namespace: "another-namespace", Name: "my-app-set"}},
{NamespacedName: types.NamespacedName{Namespace: "argocd", Name: "my-app-set"}},
},
},
{
name: "cluster generators in other namespaces should not match",
items: []argov1alpha1.ApplicationSet{
{
ObjectMeta: metav1.ObjectMeta{
Name: "my-app-set",
Namespace: "my-namespace-not-allowed",
},
Spec: argov1alpha1.ApplicationSetSpec{
Generators: []argov1alpha1.ApplicationSetGenerator{
{
Clusters: &argov1alpha1.ClusterGenerator{},
},
},
},
},
},
secret: corev1.Secret{
ObjectMeta: metav1.ObjectMeta{
Namespace: "argocd",
Name: "my-secret",
Labels: map[string]string{
argocommon.LabelKeySecretType: argocommon.LabelValueSecretTypeCluster,
},
},
},
expectedRequests: []reconcile.Request{},
},
{
name: "non-argo cd secret should not match",
items: []argov1alpha1.ApplicationSet{
Expand Down Expand Up @@ -552,8 +580,9 @@ func TestClusterEventHandler(t *testing.T) {
fakeClient := fake.NewClientBuilder().WithScheme(scheme).WithLists(&appSetList).Build()

handler := &clusterSecretEventHandler{
Client: fakeClient,
Log: log.WithField("type", "createSecretEventHandler"),
Client: fakeClient,
Log: log.WithField("type", "createSecretEventHandler"),
ApplicationSetNamespaces: []string{"argocd"},
}

mockAddRateLimitingInterface := mockAddRateLimitingInterface{}
Expand Down
Loading