fix(controller): handle explicitly diff errors, some other minor changes

c2ndev · poiana · commit 5fce1c94c6f8 · 2026-02-05T22:16:56.000+01:00
Signed-off-by: cannarelladev &lt;cannarella.dev@gmail.com&gt;
diff --git a/controllers/falco/controller.go b/controllers/falco/controller.go
@@ -18,6 +18,7 @@ package falco
 
 import (
 	"context"
+	"errors"
 	"fmt"
 	"time"
 
@@ -269,20 +270,14 @@ func (r *Reconciler) handleDeletion(ctx context.Context, falco *instancev1alpha1
 		return false, err
 	}
 
-	// Remove the finalizer using patch.
-	// Track ResourceVersion to detect if the patch was a no-op (due to cache race conditions).
-	oldRV := falco.ResourceVersion
 	patch := client.MergeFrom(falco.DeepCopy())
 	controllerutil.RemoveFinalizer(falco, finalizer)
 	if err := r.Patch(ctx, falco, patch); err != nil && !apierrors.IsNotFound(err) {
 		log.FromContext(ctx).Error(err, "unable to remove finalizer from Falco instance")
 		return false, err
 	}
 
-	// Only log deletion if the patch actually removed the finalizer
-	if falco.ResourceVersion != oldRV {
-		log.FromContext(ctx).Info("Falco instance deleted")
-	}
+	log.FromContext(ctx).Info("Falco instance deleted")
 
 	return true, nil
 }
@@ -357,24 +352,34 @@ func (r *Reconciler) ensureDeployment(ctx context.Context, falco *instancev1alph
 	// Check if update is needed to avoid unnecessary API writes.
 	// This is important for K8s < 1.31 where SSA may cause spurious resourceVersion bumps.
 	// See: https://github.com/kubernetes/kubernetes/issues/124605
+	var changedFields string
 	if resourceExists {
 		comparison, err := diff(existingResource, applyConfig)
 		if err != nil {
-			logger.Error(err, "unable to compare resources")
-			// On error, proceed with apply to be safe
-		} else if comparison.IsSame() {
-			logger.V(2).Info("Falco resource is up to date, skipping apply", "kind", falco.Spec.Type)
-			reconcileCondition.Reason = "ResourceUpToDate"
-			reconcileCondition.Message = "Resource is up to date"
-			return nil
+			if !errors.Is(err, ErrNoManagedFields) {
+				logger.Error(err, "unable to compare existing resource with desired state")
+				reconcileCondition.Status = metav1.ConditionFalse
+				reconcileCondition.Reason = "ResourceComparisonError"
+				reconcileCondition.Message = "Unable to compare existing resource with desired state: " + err.Error()
+				return err
+			}
+			logger.V(2).Info("No managed fields found, proceeding with apply to take ownership", "kind", falco.Spec.Type)
+		} else {
+			if comparison.IsSame() {
+				logger.V(2).Info("Falco resource is up to date, skipping apply", "kind", falco.Spec.Type)
+				reconcileCondition.Reason = "ResourceUpToDate"
+				reconcileCondition.Message = "Resource is up to date"
+				return nil
+			}
+			changedFields = formatChangedFields(comparison)
 		}
 	}
 
 	if !resourceExists {
 		logger.Info("Creating Falco resource", "kind", falco.Spec.Type)
 	}
 
-	applyOpts := []client.ApplyOption{client.ForceOwnership, client.FieldOwner("falco-controller")}
+	applyOpts := []client.ApplyOption{client.ForceOwnership, client.FieldOwner(fieldManager)}
 	if err = r.Apply(ctx, client.ApplyConfigurationFromUnstructured(applyConfig), applyOpts...); err != nil {
 		logger.Error(err, "unable to apply resource", "kind", falco.Spec.Type)
 		reconcileCondition.Status = metav1.ConditionFalse
@@ -388,7 +393,7 @@ func (r *Reconciler) ensureDeployment(ctx context.Context, falco *instancev1alph
 		reconcileCondition.Reason = "ResourceCreated"
 		reconcileCondition.Message = "Resource created successfully"
 	} else {
-		logger.Info("Falco resource updated", "kind", falco.Spec.Type)
+		logger.Info("Falco resource updated", "kind", falco.Spec.Type, "changedFields", changedFields)
 		reconcileCondition.Reason = "ResourceUpdated"
 		reconcileCondition.Message = "Resource updated successfully"
 	}
@@ -551,14 +556,20 @@ func (r *Reconciler) ensureResource(ctx context.Context, falco *instancev1alpha1
 	// Check if update is needed to avoid unnecessary API writes.
 	// This is important for K8s < 1.31 where SSA may cause spurious resourceVersion bumps.
 	// See: https://github.com/kubernetes/kubernetes/issues/124605
+	var changedFields string
 	if resourceExists {
 		comparison, err := diff(existingResource, desiredResource)
 		if err != nil {
-			logger.Error(err, "unable to compare resources", "type", resourceType)
-			// On error, proceed with apply to be safe
-		} else if comparison.IsSame() {
-			logger.V(3).Info(resourceType+" is up to date, skipping apply", "name", desiredResource.GetName())
-			return nil
+			if !errors.Is(err, ErrNoManagedFields) {
+				return fmt.Errorf("unable to compare existing %s with desired state: %w", resourceType, err)
+			}
+			logger.V(3).Info("No managed fields found, proceeding with apply to take ownership", "type", resourceType, "name", desiredResource.GetName())
+		} else {
+			if comparison.IsSame() {
+				logger.V(3).Info(resourceType+" is up to date, skipping apply", "name", desiredResource.GetName())
+				return nil
+			}
+			changedFields = formatChangedFields(comparison)
 		}
 	}
 
@@ -570,7 +581,7 @@ func (r *Reconciler) ensureResource(ctx context.Context, falco *instancev1alpha1
 	if !resourceExists {
 		logger.V(3).Info(resourceType+" created", "name", desiredResource.GetName())
 	} else {
-		logger.V(3).Info(resourceType+" updated", "name", desiredResource.GetName())
+		logger.V(3).Info(resourceType+" updated", "name", desiredResource.GetName(), "changedFields", changedFields)
 	}
 
 	return nil
diff --git a/controllers/falco/diff.go b/controllers/falco/diff.go
@@ -1,4 +1,4 @@
-// Copyright (C) 2025 The Falco Authors
+// Copyright (C) 2026 The Falco Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -17,7 +17,9 @@
 package falco
 
 import (
+	"errors"
 	"fmt"
+	"strings"
 
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/runtime"
@@ -31,35 +33,10 @@ const (
 	fieldManager = "falco-controller"
 )
 
-// needsUpdate checks if the current object needs to be updated to match the desired state.
-// It extracts only the fields managed by this controller and compares them with the desired config.
-//
-// This avoids unnecessary API writes on Kubernetes versions < 1.31 where Server-Side Apply
-// may cause spurious resourceVersion bumps on no-op patches.
-// See: https://github.com/kubernetes/kubernetes/issues/124605
-func needsUpdate(current runtime.Object, desired *unstructured.Unstructured) (bool, error) {
-	if current == nil || desired == nil {
-		return true, nil
-	}
-
-	// Extract only the fields managed by our field manager from the current object
-	extracted, err := managedfields.ExtractAsUnstructured(current, fieldManager)
-	if err != nil {
-		return true, fmt.Errorf("failed to extract managed fields: %w", err)
-	}
-
-	// If no managed fields found, we need to apply
-	if extracted == nil {
-		return true, nil
-	}
-
-	// Prune empty fields from both objects before comparison
-	managedfields.PruneEmptyFields(extracted)
-	managedfields.PruneEmptyFields(desired)
-
-	// Compare the extracted managed fields with the desired state
-	return managedfields.NeedsUpdate(extracted, desired)
-}
+// ErrNoManagedFields is returned when no managed fields are found for the field manager.
+// This is not a fatal error - it indicates the resource was never managed by this controller
+// and should be applied.
+var ErrNoManagedFields = errors.New("no managed fields found for field manager")
 
 // diff calculates the difference between the current and desired objects.
 // Returns a typed.Comparison that contains Added, Modified, and Removed field sets.
@@ -75,7 +52,7 @@ func diff(current runtime.Object, desired *unstructured.Unstructured) (*typed.Co
 	}
 
 	if extracted == nil {
-		return nil, fmt.Errorf("no managed fields found for field manager %s", fieldManager)
+		return nil, ErrNoManagedFields
 	}
 
 	// Deep copy desired to avoid modifying the original
@@ -87,3 +64,28 @@ func diff(current runtime.Object, desired *unstructured.Unstructured) (*typed.Co
 
 	return managedfields.Compare(extracted, desiredCopy)
 }
+
+// formatChangedFields returns a human-readable summary of the changed fields from a comparison.
+func formatChangedFields(comparison *typed.Comparison) string {
+	if comparison == nil {
+		return ""
+	}
+
+	var parts []string
+
+	if !comparison.Added.Empty() {
+		parts = append(parts, fmt.Sprintf("added: %s", comparison.Added.String()))
+	}
+	if !comparison.Modified.Empty() {
+		parts = append(parts, fmt.Sprintf("modified: %s", comparison.Modified.String()))
+	}
+	if !comparison.Removed.Empty() {
+		parts = append(parts, fmt.Sprintf("removed: %s", comparison.Removed.String()))
+	}
+
+	if len(parts) == 0 {
+		return "no changes"
+	}
+
+	return strings.Join(parts, "; ")
+}
diff --git a/controllers/falco/diff_test.go b/controllers/falco/diff_test.go
@@ -1,4 +1,4 @@
-// Copyright (C) 2025 The Falco Authors
+// Copyright (C) 2026 The Falco Authors
 //
 // Licensed under the Apache License, Version 2.0 (the "License");
 // you may not use this file except in compliance with the License.
@@ -17,42 +17,45 @@
 package falco
 
 import (
+	"fmt"
 	"testing"
 
 	"github.com/stretchr/testify/assert"
 	appsv1 "k8s.io/api/apps/v1"
 	corev1 "k8s.io/api/core/v1"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
+	"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
+	"sigs.k8s.io/structured-merge-diff/v4/typed"
 )
 
-func TestNeedsUpdate(t *testing.T) {
-	t.Run("nil current returns needs update", func(t *testing.T) {
+func TestDiff(t *testing.T) {
+	t.Run("nil current returns error", func(t *testing.T) {
 		desired := &unstructured.Unstructured{
 			Object: map[string]interface{}{
 				"apiVersion": "v1",
 				"kind":       "ConfigMap",
 			},
 		}
-		result, err := needsUpdate(nil, desired)
-		assert.NoError(t, err)
-		assert.True(t, result)
+		result, err := diff(nil, desired)
+		assert.Error(t, err)
+		assert.Nil(t, result)
 	})
 
-	t.Run("nil desired returns needs update", func(t *testing.T) {
+	t.Run("nil desired returns error", func(t *testing.T) {
 		current := &corev1.ConfigMap{
 			TypeMeta: metav1.TypeMeta{
 				APIVersion: "v1",
 				Kind:       "ConfigMap",
 			},
 			ObjectMeta: metav1.ObjectMeta{Name: "test"},
 		}
-		result, err := needsUpdate(current, nil)
-		assert.NoError(t, err)
-		assert.True(t, result)
+		result, err := diff(current, nil)
+		assert.Error(t, err)
+		assert.Nil(t, result)
 	})
 
-	t.Run("no managed fields returns needs update", func(t *testing.T) {
+	t.Run("no managed fields returns ErrNoManagedFields", func(t *testing.T) {
 		current := &appsv1.DaemonSet{
 			TypeMeta: metav1.TypeMeta{
 				APIVersion: "apps/v1",
@@ -72,35 +75,100 @@ func TestNeedsUpdate(t *testing.T) {
 				},
 			},
 		}
-		result, err := needsUpdate(current, desired)
-		assert.NoError(t, err)
-		assert.True(t, result) // No managed fields means we need to apply
+		result, err := diff(current, desired)
+		assert.Error(t, err)
+		assert.ErrorIs(t, err, ErrNoManagedFields)
+		assert.Nil(t, result)
 	})
 }
 
-func TestDiff(t *testing.T) {
-	t.Run("nil current returns error", func(t *testing.T) {
-		desired := &unstructured.Unstructured{
-			Object: map[string]interface{}{
-				"apiVersion": "v1",
-				"kind":       "ConfigMap",
-			},
+func TestErrNoManagedFields(t *testing.T) {
+	t.Run("error message is descriptive", func(t *testing.T) {
+		assert.Contains(t, ErrNoManagedFields.Error(), "no managed fields")
+	})
+
+	t.Run("can be wrapped and unwrapped", func(t *testing.T) {
+		wrapped := fmt.Errorf("failed to compare: %w", ErrNoManagedFields)
+		assert.ErrorIs(t, wrapped, ErrNoManagedFields)
+	})
+
+	t.Run("is distinguishable from other errors", func(t *testing.T) {
+		otherErr := fmt.Errorf("some other error")
+		assert.NotErrorIs(t, otherErr, ErrNoManagedFields)
+	})
+}
+
+func TestFormatChangedFields(t *testing.T) {
+	t.Run("nil comparison returns empty string", func(t *testing.T) {
+		result := formatChangedFields(nil)
+		assert.Equal(t, "", result)
+	})
+
+	t.Run("empty comparison returns no changes", func(t *testing.T) {
+		comparison := &typed.Comparison{
+			Added:    &fieldpath.Set{},
+			Modified: &fieldpath.Set{},
+			Removed:  &fieldpath.Set{},
 		}
-		result, err := diff(nil, desired)
-		assert.Error(t, err)
-		assert.Nil(t, result)
+		result := formatChangedFields(comparison)
+		assert.Equal(t, "no changes", result)
 	})
 
-	t.Run("nil desired returns error", func(t *testing.T) {
-		current := &corev1.ConfigMap{
-			TypeMeta: metav1.TypeMeta{
-				APIVersion: "v1",
-				Kind:       "ConfigMap",
-			},
-			ObjectMeta: metav1.ObjectMeta{Name: "test"},
+	t.Run("only added fields", func(t *testing.T) {
+		added := fieldpath.NewSet(fieldpath.MakePathOrDie("spec", "replicas"))
+		comparison := &typed.Comparison{
+			Added:    added,
+			Modified: &fieldpath.Set{},
+			Removed:  &fieldpath.Set{},
 		}
-		result, err := diff(current, nil)
-		assert.Error(t, err)
-		assert.Nil(t, result)
+		result := formatChangedFields(comparison)
+		assert.Contains(t, result, "added:")
+		assert.Contains(t, result, "spec")
+		assert.NotContains(t, result, "modified:")
+		assert.NotContains(t, result, "removed:")
+	})
+
+	t.Run("only modified fields", func(t *testing.T) {
+		modified := fieldpath.NewSet(fieldpath.MakePathOrDie("spec", "template", "spec", "containers"))
+		comparison := &typed.Comparison{
+			Added:    &fieldpath.Set{},
+			Modified: modified,
+			Removed:  &fieldpath.Set{},
+		}
+		result := formatChangedFields(comparison)
+		assert.Contains(t, result, "modified:")
+		assert.Contains(t, result, "spec")
+		assert.NotContains(t, result, "added:")
+		assert.NotContains(t, result, "removed:")
+	})
+
+	t.Run("only removed fields", func(t *testing.T) {
+		removed := fieldpath.NewSet(fieldpath.MakePathOrDie("metadata", "labels"))
+		comparison := &typed.Comparison{
+			Added:    &fieldpath.Set{},
+			Modified: &fieldpath.Set{},
+			Removed:  removed,
+		}
+		result := formatChangedFields(comparison)
+		assert.Contains(t, result, "removed:")
+		assert.Contains(t, result, "metadata")
+		assert.NotContains(t, result, "added:")
+		assert.NotContains(t, result, "modified:")
+	})
+
+	t.Run("multiple change types", func(t *testing.T) {
+		added := fieldpath.NewSet(fieldpath.MakePathOrDie("spec", "newField"))
+		modified := fieldpath.NewSet(fieldpath.MakePathOrDie("spec", "replicas"))
+		removed := fieldpath.NewSet(fieldpath.MakePathOrDie("metadata", "annotations"))
+		comparison := &typed.Comparison{
+			Added:    added,
+			Modified: modified,
+			Removed:  removed,
+		}
+		result := formatChangedFields(comparison)
+		assert.Contains(t, result, "added:")
+		assert.Contains(t, result, "modified:")
+		assert.Contains(t, result, "removed:")
+		assert.Contains(t, result, "; ")
 	})
 }
