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/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 f846037f4d4..9dde3ad99bb 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" ) @@ -99,7 +100,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,18 +128,26 @@ 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() == "" { + 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 } + useOwnerRef = useOwnerRef && !helmclient.ContainsResourcePolicyKeep(unstructuredObj.GetAnnotations()) + + 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( @@ -151,17 +164,18 @@ 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 } } 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 new file mode 100644 index 00000000000..f8f3b31937b --- /dev/null +++ b/internal/helm/controller/controller_test.go @@ -0,0 +1,206 @@ +// 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 ( + "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" +) + +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, + }, + { + 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 { + 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 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 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) +} + +func isAnnotationHandler(src source.Source) bool { + h := sourceHandler(src) + _, ok := h.(*libhandler.EnqueueRequestForAnnotation[client.Object]) + return ok +}