From 26c0f79575607bed0eaeac5d12fbaa31b7063f92 Mon Sep 17 00:00:00 2001 From: Abhishek Pathania Date: Thu, 24 Sep 2026 23:53:44 +0530 Subject: [PATCH 1/4] helm: default an omitted dependent namespace to the release namespace The release hook decides per dependent whether to watch it through owner references or through annotations. It reads the stored release manifest, where a template that omits metadata.namespace has an empty namespace, so SupportsOwnerReference sees a namespace mismatch and picks the annotation handler. Helm installs such resources into the release namespace, where ownerRefInjectingClient.Build gives them owner references, so no handler matched them and changes waited for the next periodic reconcile. Use the release namespace when the resource has none. Resources with an explicit namespace keep it, because SupportsOwnerReference uses a non-empty depNamespace in place of the resource's own namespace. Signed-off-by: Abhishek Pathania --- .../fragments/helm-dependent-watches.yaml | 9 ++ internal/helm/controller/controller.go | 8 +- internal/helm/controller/controller_test.go | 117 ++++++++++++++++++ 3 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 changelog/fragments/helm-dependent-watches.yaml create mode 100644 internal/helm/controller/controller_test.go diff --git a/changelog/fragments/helm-dependent-watches.yaml b/changelog/fragments/helm-dependent-watches.yaml new file mode 100644 index 00000000000..e280c1d9082 --- /dev/null +++ b/changelog/fragments/helm-dependent-watches.yaml @@ -0,0 +1,9 @@ +entries: + - description: > + For Helm-based operators, changes to dependent resources now trigger a reconcile + of the owning custom resource for resources that omit `metadata.namespace`, kinds + rendered both inside and outside the release namespace, resources with + `helm.sh/resource-policy: keep`, and cluster-scoped and cross-namespace resources. + Before, these changes waited for the next periodic reconcile. + kind: bugfix + breaking: false diff --git a/internal/helm/controller/controller.go b/internal/helm/controller/controller.go index f846037f4d4..4b5692ee104 100644 --- a/internal/helm/controller/controller.go +++ b/internal/helm/controller/controller.go @@ -130,8 +130,14 @@ func watchDependentResources(mgr manager.Manager, r *HelmOperatorReconciler, c c return nil } + // Helm installs resources with an omitted namespace into the release namespace. + depNamespace := "" + if unstructuredObj.GetNamespace() == "" { + depNamespace = release.Namespace + } + restMapper := mgr.GetRESTMapper() - useOwnerRef, err := k8sutil.SupportsOwnerReference(restMapper, owner, dependent, "") + useOwnerRef, err := k8sutil.SupportsOwnerReference(restMapper, owner, dependent, depNamespace) if err != nil { return err } diff --git a/internal/helm/controller/controller_test.go b/internal/helm/controller/controller_test.go new file mode 100644 index 00000000000..ae79146876c --- /dev/null +++ b/internal/helm/controller/controller_test.go @@ -0,0 +1,117 @@ +// Copyright 2026 The Operator-SDK Authors +// +// Licensed under the Apache License, Version 2.0 (the "License"); +// you may not use this file except in compliance with the License. +// You may obtain a copy of the License at +// +// http://www.apache.org/licenses/LICENSE-2.0 +// +// Unless required by applicable law or agreed to in writing, software +// distributed under the License is distributed on an "AS IS" BASIS, +// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +// See the License for the specific language governing permissions and +// limitations under the License. + +package controller + +import ( + "reflect" + "testing" + + "github.com/stretchr/testify/assert" + rpb "helm.sh/helm/v3/pkg/release" + "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/runtime" + "k8s.io/apimachinery/pkg/runtime/schema" + "sigs.k8s.io/controller-runtime/pkg/cache" + "sigs.k8s.io/controller-runtime/pkg/client" + "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/handler" + "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/source" + + libhandler "github.com/operator-framework/operator-lib/handler" +) + +type fakeManager struct { + manager.Manager + restMapper meta.RESTMapper +} + +func (m *fakeManager) GetRESTMapper() meta.RESTMapper { return m.restMapper } +func (m *fakeManager) GetScheme() *runtime.Scheme { return runtime.NewScheme() } +func (m *fakeManager) GetCache() cache.Cache { return nil } + +type fakeController struct { + controller.Controller + sources []source.Source +} + +func (c *fakeController) Watch(src source.Source) error { + c.sources = append(c.sources, src) + return nil +} + +func TestWatchDependentResources(t *testing.T) { + ownerGVK := schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "Nginx"} + restMapper := meta.NewDefaultRESTMapper(nil) + restMapper.Add(ownerGVK, meta.RESTScopeNamespace) + restMapper.Add(schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, meta.RESTScopeNamespace) + + cases := []struct { + name string + manifest string + expectAnnotations bool + }{ + { + name: "Uses owner references when the dependent omits its namespace", + manifest: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm`, + expectAnnotations: false, + }, + { + name: "Uses owner references when the dependent is in the release namespace", + manifest: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm + namespace: ns`, + expectAnnotations: false, + }, + { + name: "Uses annotations when the dependent is in another namespace", + manifest: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm + namespace: other`, + expectAnnotations: true, + }, + } + + for _, c := range cases { + t.Run(c.name, func(t *testing.T) { + r := &HelmOperatorReconciler{GVK: ownerGVK} + ctr := &fakeController{} + watchDependentResources(&fakeManager{restMapper: restMapper}, r, ctr) + + err := r.releaseHook(&rpb.Release{Namespace: "ns", Manifest: c.manifest}) + assert.NoError(t, err) + if assert.Len(t, ctr.sources, 1) { + assert.Equal(t, c.expectAnnotations, isAnnotationHandler(ctr.sources[0])) + } + }) + } +} + +func sourceHandler(src source.Source) handler.EventHandler { + return reflect.ValueOf(src).Elem().FieldByName("Handler").Interface().(handler.EventHandler) +} + +func isAnnotationHandler(src source.Source) bool { + h := sourceHandler(src) + _, ok := h.(*libhandler.EnqueueRequestForAnnotation[client.Object]) + return ok +} From 381fb0dd77eee8c881969d72fa9d359a8b18a893 Mon Sep 17 00:00:00 2001 From: Abhishek Pathania Date: Thu, 24 Sep 2026 23:53:53 +0530 Subject: [PATCH 2/4] helm: watch a dependent kind with both handlers when it needs both Watches were keyed by GVK, so the first resource of a kind decided the handler for every resource of that kind. A kind rendered both in the release namespace (owner references) and elsewhere (annotations) got only one handler, and because releaseutil.SplitManifests returns a map, which one it got could change on every operator restart. Key watches by GVK and handler type. Both watches share one informer. Signed-off-by: Abhishek Pathania --- internal/helm/controller/controller.go | 26 +++++++++------- internal/helm/controller/controller_test.go | 33 +++++++++++++++++++++ 2 files changed, 49 insertions(+), 10 deletions(-) diff --git a/internal/helm/controller/controller.go b/internal/helm/controller/controller.go index 4b5692ee104..d92c8ce7b79 100644 --- a/internal/helm/controller/controller.go +++ b/internal/helm/controller/controller.go @@ -99,7 +99,11 @@ func Add(mgr manager.Manager, options WatchOptions) error { // that adds watches for resources in released Helm charts. func watchDependentResources(mgr manager.Manager, r *HelmOperatorReconciler, c controller.Controller) { var m sync.RWMutex - watches := map[schema.GroupVersionKind]struct{}{} + type watchKey struct { + gvk schema.GroupVersionKind + useOwnerRef bool + } + watches := map[watchKey]struct{}{} releaseHook := func(release *rpb.Release) error { owner := &unstructured.Unstructured{} owner.SetGroupVersionKind(r.GVK) @@ -123,13 +127,6 @@ func watchDependentResources(mgr manager.Manager, r *HelmOperatorReconciler, c c return nil } - m.RLock() - _, ok := watches[gvkDependent] - m.RUnlock() - if ok { - return nil - } - // Helm installs resources with an omitted namespace into the release namespace. depNamespace := "" if unstructuredObj.GetNamespace() == "" { @@ -142,6 +139,14 @@ func watchDependentResources(mgr manager.Manager, r *HelmOperatorReconciler, c c return err } + key := watchKey{gvkDependent, useOwnerRef} + m.RLock() + _, ok := watches[key] + m.RUnlock() + if ok { + return nil + } + if useOwnerRef { // Setup watch using owner references. err = c.Watch( source.Kind( @@ -164,10 +169,11 @@ func watchDependentResources(mgr manager.Manager, r *HelmOperatorReconciler, c c } } m.Lock() - watches[gvkDependent] = struct{}{} + watches[key] = struct{}{} m.Unlock() log.Info("Watching dependent resource", "ownerApiVersion", r.GVK.GroupVersion(), - "ownerKind", r.GVK.Kind, "apiVersion", gvkDependent.GroupVersion(), "kind", gvkDependent.Kind) + "ownerKind", r.GVK.Kind, "apiVersion", gvkDependent.GroupVersion(), "kind", gvkDependent.Kind, + "useOwnerRef", useOwnerRef) return nil } diff --git a/internal/helm/controller/controller_test.go b/internal/helm/controller/controller_test.go index ae79146876c..df2dea25cdd 100644 --- a/internal/helm/controller/controller_test.go +++ b/internal/helm/controller/controller_test.go @@ -106,6 +106,39 @@ metadata: } } +func TestWatchDependentResourcesMixedNamespaces(t *testing.T) { + ownerGVK := schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "Nginx"} + restMapper := meta.NewDefaultRESTMapper(nil) + restMapper.Add(ownerGVK, meta.RESTScopeNamespace) + restMapper.Add(schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, meta.RESTScopeNamespace) + + sameNamespace := ` + - apiVersion: v1 + kind: ConfigMap + metadata: + name: cm-same + namespace: ns` + otherNamespace := ` + - apiVersion: v1 + kind: ConfigMap + metadata: + name: cm-other + namespace: other` + + // A List keeps item order, so both orders are exercised deterministically. + for _, items := range []string{sameNamespace + otherNamespace, otherNamespace + sameNamespace} { + r := &HelmOperatorReconciler{GVK: ownerGVK} + ctr := &fakeController{} + watchDependentResources(&fakeManager{restMapper: restMapper}, r, ctr) + + manifest := "apiVersion: v1\nkind: List\nitems:" + items + assert.NoError(t, r.releaseHook(&rpb.Release{Namespace: "ns", Manifest: manifest})) + if assert.Len(t, ctr.sources, 2) { + assert.NotEqual(t, isAnnotationHandler(ctr.sources[0]), isAnnotationHandler(ctr.sources[1])) + } + } +} + func sourceHandler(src source.Source) handler.EventHandler { return reflect.ValueOf(src).Elem().FieldByName("Handler").Interface().(handler.EventHandler) } From 8d37e65e5520ea785c3684e339a90b10c0991670 Mon Sep 17 00:00:00 2001 From: Abhishek Pathania Date: Thu, 24 Sep 2026 23:54:02 +0530 Subject: [PATCH 3/4] helm: use the annotation handler for resource-policy keep resources ownerRefInjectingClient.Build annotates resources that carry helm.sh/resource-policy: keep instead of giving them owner references, so that deleting the custom resource does not garbage-collect them. The watch decision did not apply the same rule and watched them through owner references. helm-operator-plugins fixed the same mismatch in operator-framework/helm-operator-plugins#83. Export ContainsResourcePolicyKeep so both paths use the same check. Signed-off-by: Abhishek Pathania --- internal/helm/client/client.go | 5 +++-- internal/helm/client/client_test.go | 2 +- internal/helm/controller/controller.go | 2 ++ internal/helm/controller/controller_test.go | 11 +++++++++++ 4 files changed, 17 insertions(+), 3 deletions(-) diff --git a/internal/helm/client/client.go b/internal/helm/client/client.go index 6ea4d1138ae..f6838e141c3 100644 --- a/internal/helm/client/client.go +++ b/internal/helm/client/client.go @@ -77,7 +77,7 @@ func (c *ownerRefInjectingClient) Build(reader io.Reader, validate bool) (kube.R // If the resource contains the Helm resource-policy keep annotation, then do not add // the owner reference. So when the CR is deleted, Kubernetes won't GCs the resource. - if useOwnerRef && !containsResourcePolicyKeep(u.GetAnnotations()) { + if useOwnerRef && !ContainsResourcePolicyKeep(u.GetAnnotations()) { ownerRef := metav1.NewControllerRef(c.owner, c.owner.GetObjectKind().GroupVersionKind()) u.SetOwnerReferences([]metav1.OwnerReference{*ownerRef}) } else { @@ -94,7 +94,8 @@ func (c *ownerRefInjectingClient) Build(reader io.Reader, validate bool) (kube.R return resourceList, nil } -func containsResourcePolicyKeep(annotations map[string]string) bool { +// ContainsResourcePolicyKeep reports whether annotations carry the Helm resource-policy keep annotation. +func ContainsResourcePolicyKeep(annotations map[string]string) bool { if annotations == nil { return false } diff --git a/internal/helm/client/client_test.go b/internal/helm/client/client_test.go index c223da3339d..20e931c2b16 100644 --- a/internal/helm/client/client_test.go +++ b/internal/helm/client/client_test.go @@ -74,6 +74,6 @@ func TestContainsResourcePolicyKeep(t *testing.T) { } for _, test := range tests { - assert.Equal(t, test.expectedVal, containsResourcePolicyKeep(test.input), test.name) + assert.Equal(t, test.expectedVal, ContainsResourcePolicyKeep(test.input), test.name) } } diff --git a/internal/helm/controller/controller.go b/internal/helm/controller/controller.go index d92c8ce7b79..4ed56a64103 100644 --- a/internal/helm/controller/controller.go +++ b/internal/helm/controller/controller.go @@ -36,6 +36,7 @@ import ( libhandler "github.com/operator-framework/operator-lib/handler" "github.com/operator-framework/operator-lib/predicate" + helmclient "github.com/operator-framework/operator-sdk/internal/helm/client" "github.com/operator-framework/operator-sdk/internal/helm/release" "github.com/operator-framework/operator-sdk/internal/util/k8sutil" ) @@ -138,6 +139,7 @@ func watchDependentResources(mgr manager.Manager, r *HelmOperatorReconciler, c c if err != nil { return err } + useOwnerRef = useOwnerRef && !helmclient.ContainsResourcePolicyKeep(unstructuredObj.GetAnnotations()) key := watchKey{gvkDependent, useOwnerRef} m.RLock() diff --git a/internal/helm/controller/controller_test.go b/internal/helm/controller/controller_test.go index df2dea25cdd..cf5ad73ed4f 100644 --- a/internal/helm/controller/controller_test.go +++ b/internal/helm/controller/controller_test.go @@ -89,6 +89,17 @@ metadata: namespace: other`, expectAnnotations: true, }, + { + name: "Uses annotations when the dependent has the resource-policy keep annotation", + manifest: `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm + namespace: ns + annotations: + helm.sh/resource-policy: keep`, + expectAnnotations: true, + }, } for _, c := range cases { From 2fabd09cdd7b88edb678a1fe059b306c86ad9571 Mon Sep 17 00:00:00 2001 From: Abhishek Pathania Date: Thu, 24 Sep 2026 23:54:11 +0530 Subject: [PATCH 4/4] helm: match the annotation handler on the owner kind SetOwnerAnnotations writes the owner's GroupKind to operator-sdk/primary-resource-type, and EnqueueRequestForAnnotation only enqueues when that annotation equals its Type. Type was set to the dependent's GroupKind, so the handler never enqueued anything and changes to cluster-scoped, cross-namespace and resource-policy keep dependents were never reconciled. This is the behavior reported in #2727, which #2987 set out to fix. Use the owner's GroupKind, as helm-operator-plugins does. Signed-off-by: Abhishek Pathania --- internal/helm/controller/controller.go | 2 +- internal/helm/controller/controller_test.go | 45 +++++++++++++++++++++ 2 files changed, 46 insertions(+), 1 deletion(-) diff --git a/internal/helm/controller/controller.go b/internal/helm/controller/controller.go index 4ed56a64103..9dde3ad99bb 100644 --- a/internal/helm/controller/controller.go +++ b/internal/helm/controller/controller.go @@ -164,7 +164,7 @@ func watchDependentResources(mgr manager.Manager, r *HelmOperatorReconciler, c c source.Kind( mgr.GetCache(), client.Object(unstructuredObj), - &libhandler.EnqueueRequestForAnnotation[client.Object]{Type: gvkDependent.GroupKind()}, + &libhandler.EnqueueRequestForAnnotation[client.Object]{Type: r.GVK.GroupKind()}, predicate.DependentPredicate{})) if err != nil { return err diff --git a/internal/helm/controller/controller_test.go b/internal/helm/controller/controller_test.go index cf5ad73ed4f..f8f3b31937b 100644 --- a/internal/helm/controller/controller_test.go +++ b/internal/helm/controller/controller_test.go @@ -15,19 +15,25 @@ package controller import ( + "context" "reflect" "testing" "github.com/stretchr/testify/assert" rpb "helm.sh/helm/v3/pkg/release" "k8s.io/apimachinery/pkg/api/meta" + "k8s.io/apimachinery/pkg/apis/meta/v1/unstructured" "k8s.io/apimachinery/pkg/runtime" "k8s.io/apimachinery/pkg/runtime/schema" + "k8s.io/apimachinery/pkg/types" + "k8s.io/client-go/util/workqueue" "sigs.k8s.io/controller-runtime/pkg/cache" "sigs.k8s.io/controller-runtime/pkg/client" "sigs.k8s.io/controller-runtime/pkg/controller" + "sigs.k8s.io/controller-runtime/pkg/event" "sigs.k8s.io/controller-runtime/pkg/handler" "sigs.k8s.io/controller-runtime/pkg/manager" + "sigs.k8s.io/controller-runtime/pkg/reconcile" "sigs.k8s.io/controller-runtime/pkg/source" libhandler "github.com/operator-framework/operator-lib/handler" @@ -150,6 +156,45 @@ func TestWatchDependentResourcesMixedNamespaces(t *testing.T) { } } +func TestWatchDependentResourcesAnnotationHandlerEnqueuesOwner(t *testing.T) { + ownerGVK := schema.GroupVersionKind{Group: "example.com", Version: "v1", Kind: "Nginx"} + restMapper := meta.NewDefaultRESTMapper(nil) + restMapper.Add(ownerGVK, meta.RESTScopeNamespace) + restMapper.Add(schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}, meta.RESTScopeNamespace) + + r := &HelmOperatorReconciler{GVK: ownerGVK} + ctr := &fakeController{} + watchDependentResources(&fakeManager{restMapper: restMapper}, r, ctr) + manifest := `apiVersion: v1 +kind: ConfigMap +metadata: + name: cm + namespace: other` + assert.NoError(t, r.releaseHook(&rpb.Release{Namespace: "ns", Manifest: manifest})) + if !assert.Len(t, ctr.sources, 1) || !assert.True(t, isAnnotationHandler(ctr.sources[0])) { + return + } + + owner := &unstructured.Unstructured{} + owner.SetGroupVersionKind(ownerGVK) + owner.SetNamespace("ns") + owner.SetName("nginx") + dependent := &unstructured.Unstructured{} + dependent.SetGroupVersionKind(schema.GroupVersionKind{Version: "v1", Kind: "ConfigMap"}) + dependent.SetNamespace("other") + dependent.SetName("cm") + assert.NoError(t, libhandler.SetOwnerAnnotations(owner, dependent)) + + q := workqueue.NewTypedRateLimitingQueue(workqueue.DefaultTypedControllerRateLimiter[reconcile.Request]()) + defer q.ShutDown() + h := sourceHandler(ctr.sources[0]) + h.Delete(context.TODO(), event.DeleteEvent{Object: dependent}, q) + if assert.Equal(t, 1, q.Len()) { + req, _ := q.Get() + assert.Equal(t, types.NamespacedName{Namespace: "ns", Name: "nginx"}, req.NamespacedName) + } +} + func sourceHandler(src source.Source) handler.EventHandler { return reflect.ValueOf(src).Elem().FieldByName("Handler").Interface().(handler.EventHandler) }