Merge pull request #1862 from viccuad/feat/remove-helm-annot

feat(controller): Strip helm bookkeeping annots on migration

Author: jvanz

internal/constants/constants.go

@@ -44,7 +44,15 @@ const (
 	ComponentLabelKey               = "app.kubernetes.io/component"
 	PartOfLabelKey                  = "app.kubernetes.io/part-of"
 	PartOfLabelValue                = "kubewarden"
-	ManagedByKey                    = "app.kubernetes.io/managed-by"
+
+	ManagedByKey = "app.kubernetes.io/managed-by"
+	// ManagedByKeyLabelValue is set via Helm chart templates.
+	ManagedByKeyLabelValue = "kubewarden-controller"
+
+	// HelmResourcePolicy and others re Helm annotations.
+	HelmResourcePolicy       = "helm.sh/resource-policy"
+	HelmMetaReleaseName      = "meta.helm.sh/release-name"
+	HelmMetaReleaseNamespace = "meta.helm.sh/release-namespace"
 
 	// DefaultsManagedByLabelKey is the label key for resources managed by DefaultsApplier.
 	DefaultsManagedByLabelKey = "kubewarden.io/managed-by"

internal/controller/defaults_applier.go

@@ -114,6 +114,13 @@ func (r *DefaultsApplierReconciler) applyResource(ctx context.Context, desired *
 	// does not end up in our managed field set.
 	unstructured.RemoveNestedField(desired.Object, "status")
 
+	// When adopting a resource that was Helm-managed before the 1.37 chart unification,
+	// strip the Helm bookkeeping annotations from the live object. This must happen
+	// before the server-side Apply below, while the live object still reports Helm ownership.
+	if err := r.stripHelmBookkeeping(ctx, desired); err != nil {
+		return err
+	}
+
 	// Stamp the ownership label used by cleanupStale to identify managed resources.
 	labels := desired.GetLabels()
 	if labels == nil {
@@ -133,6 +140,66 @@ func (r *DefaultsApplierReconciler) applyResource(ctx context.Context, desired *
 	return nil
 }
 
+// stripHelmBookkeeping removes Helm bookkeeping annotations from the live resource when it
+// was previously Helm-managed and the desired state explicitly claims controller ownership.
+//
+// This handles the 1.37 migration where the default PolicyServer and recommended policies
+// moved from Helm ownership to controller ownership. Requiring both conditions avoids
+// stripping annotations from resources that carry managed-by: Helm for unrelated reasons.
+//
+// The Helm annotations cannot be removed by the server-side apply in applyResource: they are
+// owned by Helm's field manager and are absent from our applied configuration,
+// so the API server leaves them untouched. A dedicated merge patch is required.
+func (r *DefaultsApplierReconciler) stripHelmBookkeeping(ctx context.Context, desired *unstructured.Unstructured) error {
+	if desired.GetLabels()[constants.ManagedByKey] != constants.ManagedByKeyLabelValue {
+		// exit if we are not taking ownership
+		return nil
+	}
+
+	live := &unstructured.Unstructured{}
+	live.SetGroupVersionKind(desired.GroupVersionKind())
+	if err := r.Get(ctx, client.ObjectKeyFromObject(desired), live); err != nil {
+		if apierrors.IsNotFound(err) {
+			// fresh resource: nothing to strip
+			return nil
+		}
+		return fmt.Errorf("failed to get live resource: %w", err)
+	}
+
+	if live.GetLabels()[constants.ManagedByKey] != "Helm" {
+		// exit if it wasn't previously owned by Helm
+		return nil
+	}
+
+	orig := live.DeepCopy()
+	annotations := live.GetAnnotations()
+	changed := false
+	helmBookkeepingAnnotations := []string{
+		constants.HelmResourcePolicy,
+		constants.HelmMetaReleaseName,
+		constants.HelmMetaReleaseNamespace,
+	}
+	for _, key := range helmBookkeepingAnnotations {
+		if _, found := annotations[key]; found {
+			delete(annotations, key)
+			changed = true
+		}
+	}
+	if !changed {
+		// exit if there was no annotations to remove
+		return nil
+	}
+	live.SetAnnotations(annotations)
+
+	if err := r.Patch(ctx, live, client.MergeFrom(orig)); err != nil {
+		return fmt.Errorf("failed to strip Helm annotations: %w", err)
+	}
+
+	r.Log.Info("Stripped Helm bookkeeping annotations from adopted resource",
+		"resource", client.ObjectKeyFromObject(live), "kind", live.GetKind())
+	return nil
+}
+
 // isSupportedGVK returns true if the GroupVersionKind is a known Kubewarden resource type.
 func isSupportedGVK(gvk schema.GroupVersionKind) bool {
 	if gvk.Group != constants.KubewardenPoliciesGroup {

internal/controller/defaults_applier_test.go

@@ -279,6 +279,57 @@ var _ = Describe("DefaultsApplierReconciler", func() {
 		})
 	})
 
+	Context("when a previously Helm-managed PolicyServer is adopted by the controller", func() {
+		It("should remove Helm annotations and update the managed-by label on reconcile", func() {
+			// Simulate a PolicyServer that was created by Helm before the 1.37 migration.
+			// It carries Helm bookkeeping annotations and managed-by label.
+			helmPS := policiesv1.NewPolicyServerFactory().WithName(policyServerName).WithoutFinalizers().Build()
+			helmPS.Labels = map[string]string{
+				"app.kubernetes.io/managed-by": "Helm",
+			}
+			helmPS.Annotations = map[string]string{
+				constants.HelmResourcePolicy:       "keep",
+				constants.HelmMetaReleaseName:      "kubewarden-defaults",
+				constants.HelmMetaReleaseNamespace: "kubewarden",
+			}
+			Expect(k8sClient.Create(ctx, helmPS)).To(Succeed())
+
+			// Now the ConfigMap is created, triggering the controller to adopt the resource.
+			// Mirror the behavior of the chart: the ConfigMap YAML sets  `app.kubernetes.io/managed-by`
+			// to `kubewarden-controller`, so when we reconcile, applyManagedKeys overwrites the
+			// Helm value during the same reconcile that strips the Helm annotations.
+			ps := policiesv1.NewPolicyServerFactory().WithName(policyServerName).WithoutFinalizers().Build()
+			ps.Labels = map[string]string{
+				constants.ManagedByKey: constants.ManagedByKeyLabelValue,
+			}
+			policyServerYAML := marshalPolicyServer(ps)
+
+			cm := &corev1.ConfigMap{
+				ObjectMeta: metav1.ObjectMeta{
+					Name:      configMapName,
+					Namespace: deploymentsNamespace,
+				},
+				Data: map[string]string{
+					"policyserver-default": policyServerYAML,
+				},
+			}
+			Expect(k8sClient.Create(ctx, cm)).To(Succeed())
+
+			adoptedPS := &policiesv1.PolicyServer{}
+			Eventually(func(g Gomega) {
+				g.Expect(k8sClient.Get(ctx, types.NamespacedName{Name: policyServerName}, adoptedPS)).To(Succeed())
+				// Helm annotations must be gone.
+				g.Expect(adoptedPS.Annotations).NotTo(HaveKey(constants.HelmResourcePolicy))
+				g.Expect(adoptedPS.Annotations).NotTo(HaveKey(constants.HelmMetaReleaseName))
+				g.Expect(adoptedPS.Annotations).NotTo(HaveKey(constants.HelmMetaReleaseNamespace))
+				// managed-by label must be updated to the controller value.
+				g.Expect(adoptedPS.Labels).To(HaveKeyWithValue(constants.ManagedByKey, constants.ManagedByKeyLabelValue))
+				// Ownership label must be stamped.
+				g.Expect(adoptedPS.Labels).To(HaveKeyWithValue(constants.DefaultsManagedByLabelKey, constants.DefaultsManagedByLabelValue))
+			}, timeout, pollInterval).Should(Succeed())
+		})
+	})
+
 	Context("when ConfigMap has malformed YAML", func() {
 		It("should skip the malformed entry and continue with others", func() {
 			ps := policiesv1.NewPolicyServerFactory().WithName(policyServerName).WithoutFinalizers().Build()