Skip to content

Commit aa56878

Browse files
committed
fix(controller): handle explicitly diff errors, some other minor changes
Signed-off-by: cannarelladev <cannarella.dev@gmail.com>
1 parent 6db5493 commit aa56878

File tree

3 files changed

+179
-17
lines changed

3 files changed

+179
-17
lines changed

controllers/falco/controller.go

Lines changed: 24 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,7 @@ package falco
1818

1919
import (
2020
"context"
21+
"errors"
2122
"fmt"
2223
"time"
2324

@@ -269,20 +270,14 @@ func (r *Reconciler) handleDeletion(ctx context.Context, falco *instancev1alpha1
269270
return false, err
270271
}
271272

272-
// Remove the finalizer using patch.
273-
// Track ResourceVersion to detect if the patch was a no-op (due to cache race conditions).
274-
oldRV := falco.ResourceVersion
275273
patch := client.MergeFrom(falco.DeepCopy())
276274
controllerutil.RemoveFinalizer(falco, finalizer)
277275
if err := r.Patch(ctx, falco, patch); err != nil && !apierrors.IsNotFound(err) {
278276
log.FromContext(ctx).Error(err, "unable to remove finalizer from Falco instance")
279277
return false, err
280278
}
281279

282-
// Only log deletion if the patch actually removed the finalizer
283-
if falco.ResourceVersion != oldRV {
284-
log.FromContext(ctx).Info("Falco instance deleted")
285-
}
280+
log.FromContext(ctx).Info("Falco instance deleted")
286281

287282
return true, nil
288283
}
@@ -357,24 +352,33 @@ func (r *Reconciler) ensureDeployment(ctx context.Context, falco *instancev1alph
357352
// Check if update is needed to avoid unnecessary API writes.
358353
// This is important for K8s < 1.31 where SSA may cause spurious resourceVersion bumps.
359354
// See: https://github.com/kubernetes/kubernetes/issues/124605
355+
var changedFields string
360356
if resourceExists {
361357
comparison, err := diff(existingResource, applyConfig)
362358
if err != nil {
363-
logger.Error(err, "unable to compare resources")
364-
// On error, proceed with apply to be safe
359+
// ErrNoManagedFields means the resource exists but was never managed by this controller. Proceed with apply to take ownership.
360+
if !errors.Is(err, ErrNoManagedFields) {
361+
reconcileCondition.Status = metav1.ConditionFalse
362+
reconcileCondition.Reason = "ResourceComparisonError"
363+
reconcileCondition.Message = "Unable to compare resources: " + err.Error()
364+
return fmt.Errorf("unable to compare resources: %w", err)
365+
}
366+
logger.V(2).Info("No managed fields found, proceeding with apply to take ownership", "kind", falco.Spec.Type)
365367
} else if comparison.IsSame() {
366368
logger.V(2).Info("Falco resource is up to date, skipping apply", "kind", falco.Spec.Type)
367369
reconcileCondition.Reason = "ResourceUpToDate"
368370
reconcileCondition.Message = "Resource is up to date"
369371
return nil
372+
} else {
373+
changedFields = formatChangedFields(comparison)
370374
}
371375
}
372376

373377
if !resourceExists {
374378
logger.Info("Creating Falco resource", "kind", falco.Spec.Type)
375379
}
376380

377-
applyOpts := []client.ApplyOption{client.ForceOwnership, client.FieldOwner("falco-controller")}
381+
applyOpts := []client.ApplyOption{client.ForceOwnership, client.FieldOwner(fieldManager)}
378382
if err = r.Apply(ctx, client.ApplyConfigurationFromUnstructured(applyConfig), applyOpts...); err != nil {
379383
logger.Error(err, "unable to apply resource", "kind", falco.Spec.Type)
380384
reconcileCondition.Status = metav1.ConditionFalse
@@ -388,7 +392,7 @@ func (r *Reconciler) ensureDeployment(ctx context.Context, falco *instancev1alph
388392
reconcileCondition.Reason = "ResourceCreated"
389393
reconcileCondition.Message = "Resource created successfully"
390394
} else {
391-
logger.Info("Falco resource updated", "kind", falco.Spec.Type)
395+
logger.Info("Falco resource updated", "kind", falco.Spec.Type, "changedFields", changedFields)
392396
reconcileCondition.Reason = "ResourceUpdated"
393397
reconcileCondition.Message = "Resource updated successfully"
394398
}
@@ -551,14 +555,20 @@ func (r *Reconciler) ensureResource(ctx context.Context, falco *instancev1alpha1
551555
// Check if update is needed to avoid unnecessary API writes.
552556
// This is important for K8s < 1.31 where SSA may cause spurious resourceVersion bumps.
553557
// See: https://github.com/kubernetes/kubernetes/issues/124605
558+
var changedFields string
554559
if resourceExists {
555560
comparison, err := diff(existingResource, desiredResource)
556561
if err != nil {
557-
logger.Error(err, "unable to compare resources", "type", resourceType)
558-
// On error, proceed with apply to be safe
562+
// ErrNoManagedFields means the resource exists but was never managed by this controller. Proceed with apply to take ownership.
563+
if !errors.Is(err, ErrNoManagedFields) {
564+
return fmt.Errorf("unable to compare %s resources: %w", resourceType, err)
565+
}
566+
logger.V(3).Info("No managed fields found, proceeding with apply to take ownership", "type", resourceType, "name", desiredResource.GetName())
559567
} else if comparison.IsSame() {
560568
logger.V(3).Info(resourceType+" is up to date, skipping apply", "name", desiredResource.GetName())
561569
return nil
570+
} else {
571+
changedFields = formatChangedFields(comparison)
562572
}
563573
}
564574

@@ -570,7 +580,7 @@ func (r *Reconciler) ensureResource(ctx context.Context, falco *instancev1alpha1
570580
if !resourceExists {
571581
logger.V(3).Info(resourceType+" created", "name", desiredResource.GetName())
572582
} else {
573-
logger.V(3).Info(resourceType+" updated", "name", desiredResource.GetName())
583+
logger.V(3).Info(resourceType+" updated", "name", desiredResource.GetName(), "changedFields", changedFields)
574584
}
575585

576586
return nil

controllers/falco/diff.go

Lines changed: 34 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (C) 2025 The Falco Authors
1+
// Copyright (C) 2026 The Falco Authors
22
//
33
// Licensed under the Apache License, Version 2.0 (the "License");
44
// you may not use this file except in compliance with the License.
@@ -17,7 +17,9 @@
1717
package falco
1818

1919
import (
20+
"errors"
2021
"fmt"
22+
"strings"
2123

2224
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
2325
"k8s.io/apimachinery/pkg/runtime"
@@ -31,6 +33,11 @@ const (
3133
fieldManager = "falco-controller"
3234
)
3335

36+
// ErrNoManagedFields is returned when no managed fields are found for the field manager.
37+
// This is not a fatal error - it indicates the resource was never managed by this controller
38+
// and should be applied.
39+
var ErrNoManagedFields = errors.New("no managed fields found for field manager")
40+
3441
// needsUpdate checks if the current object needs to be updated to match the desired state.
3542
// It extracts only the fields managed by this controller and compares them with the desired config.
3643
//
@@ -75,7 +82,7 @@ func diff(current runtime.Object, desired *unstructured.Unstructured) (*typed.Co
7582
}
7683

7784
if extracted == nil {
78-
return nil, fmt.Errorf("no managed fields found for field manager %s", fieldManager)
85+
return nil, ErrNoManagedFields
7986
}
8087

8188
// Deep copy desired to avoid modifying the original
@@ -87,3 +94,28 @@ func diff(current runtime.Object, desired *unstructured.Unstructured) (*typed.Co
8794

8895
return managedfields.Compare(extracted, desiredCopy)
8996
}
97+
98+
// formatChangedFields returns a human-readable summary of the changed fields from a comparison.
99+
func formatChangedFields(comparison *typed.Comparison) string {
100+
if comparison == nil {
101+
return ""
102+
}
103+
104+
var parts []string
105+
106+
if !comparison.Added.Empty() {
107+
parts = append(parts, fmt.Sprintf("added: %s", comparison.Added.String()))
108+
}
109+
if !comparison.Modified.Empty() {
110+
parts = append(parts, fmt.Sprintf("modified: %s", comparison.Modified.String()))
111+
}
112+
if !comparison.Removed.Empty() {
113+
parts = append(parts, fmt.Sprintf("removed: %s", comparison.Removed.String()))
114+
}
115+
116+
if len(parts) == 0 {
117+
return "no changes"
118+
}
119+
120+
return strings.Join(parts, "; ")
121+
}

controllers/falco/diff_test.go

Lines changed: 121 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (C) 2025 The Falco Authors
1+
// Copyright (C) 2026 The Falco Authors
22
//
33
// Licensed under the Apache License, Version 2.0 (the "License");
44
// you may not use this file except in compliance with the License.
@@ -17,13 +17,16 @@
1717
package falco
1818

1919
import (
20+
"fmt"
2021
"testing"
2122

2223
"github.com/stretchr/testify/assert"
2324
appsv1 "k8s.io/api/apps/v1"
2425
corev1 "k8s.io/api/core/v1"
2526
metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
2627
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
28+
"sigs.k8s.io/structured-merge-diff/v4/fieldpath"
29+
"sigs.k8s.io/structured-merge-diff/v4/typed"
2730
)
2831

2932
func TestNeedsUpdate(t *testing.T) {
@@ -103,4 +106,121 @@ func TestDiff(t *testing.T) {
103106
assert.Error(t, err)
104107
assert.Nil(t, result)
105108
})
109+
110+
t.Run("no managed fields returns ErrNoManagedFields", func(t *testing.T) {
111+
current := &appsv1.DaemonSet{
112+
TypeMeta: metav1.TypeMeta{
113+
APIVersion: "apps/v1",
114+
Kind: "DaemonSet",
115+
},
116+
ObjectMeta: metav1.ObjectMeta{
117+
Name: "test",
118+
// No ManagedFields set
119+
},
120+
}
121+
desired := &unstructured.Unstructured{
122+
Object: map[string]interface{}{
123+
"apiVersion": "apps/v1",
124+
"kind": "DaemonSet",
125+
"metadata": map[string]interface{}{
126+
"name": "test",
127+
},
128+
},
129+
}
130+
result, err := diff(current, desired)
131+
assert.Error(t, err)
132+
assert.ErrorIs(t, err, ErrNoManagedFields)
133+
assert.Nil(t, result)
134+
})
135+
}
136+
137+
func TestErrNoManagedFields(t *testing.T) {
138+
t.Run("error message is descriptive", func(t *testing.T) {
139+
assert.Contains(t, ErrNoManagedFields.Error(), "no managed fields")
140+
})
141+
142+
t.Run("can be wrapped and unwrapped", func(t *testing.T) {
143+
wrapped := fmt.Errorf("failed to compare: %w", ErrNoManagedFields)
144+
assert.ErrorIs(t, wrapped, ErrNoManagedFields)
145+
})
146+
147+
t.Run("is distinguishable from other errors", func(t *testing.T) {
148+
otherErr := fmt.Errorf("some other error")
149+
assert.NotErrorIs(t, otherErr, ErrNoManagedFields)
150+
})
151+
}
152+
153+
func TestFormatChangedFields(t *testing.T) {
154+
t.Run("nil comparison returns empty string", func(t *testing.T) {
155+
result := formatChangedFields(nil)
156+
assert.Equal(t, "", result)
157+
})
158+
159+
t.Run("empty comparison returns no changes", func(t *testing.T) {
160+
comparison := &typed.Comparison{
161+
Added: &fieldpath.Set{},
162+
Modified: &fieldpath.Set{},
163+
Removed: &fieldpath.Set{},
164+
}
165+
result := formatChangedFields(comparison)
166+
assert.Equal(t, "no changes", result)
167+
})
168+
169+
t.Run("only added fields", func(t *testing.T) {
170+
added := fieldpath.NewSet(fieldpath.MakePathOrDie("spec", "replicas"))
171+
comparison := &typed.Comparison{
172+
Added: added,
173+
Modified: &fieldpath.Set{},
174+
Removed: &fieldpath.Set{},
175+
}
176+
result := formatChangedFields(comparison)
177+
assert.Contains(t, result, "added:")
178+
assert.Contains(t, result, "spec")
179+
assert.NotContains(t, result, "modified:")
180+
assert.NotContains(t, result, "removed:")
181+
})
182+
183+
t.Run("only modified fields", func(t *testing.T) {
184+
modified := fieldpath.NewSet(fieldpath.MakePathOrDie("spec", "template", "spec", "containers"))
185+
comparison := &typed.Comparison{
186+
Added: &fieldpath.Set{},
187+
Modified: modified,
188+
Removed: &fieldpath.Set{},
189+
}
190+
result := formatChangedFields(comparison)
191+
assert.Contains(t, result, "modified:")
192+
assert.Contains(t, result, "spec")
193+
assert.NotContains(t, result, "added:")
194+
assert.NotContains(t, result, "removed:")
195+
})
196+
197+
t.Run("only removed fields", func(t *testing.T) {
198+
removed := fieldpath.NewSet(fieldpath.MakePathOrDie("metadata", "labels"))
199+
comparison := &typed.Comparison{
200+
Added: &fieldpath.Set{},
201+
Modified: &fieldpath.Set{},
202+
Removed: removed,
203+
}
204+
result := formatChangedFields(comparison)
205+
assert.Contains(t, result, "removed:")
206+
assert.Contains(t, result, "metadata")
207+
assert.NotContains(t, result, "added:")
208+
assert.NotContains(t, result, "modified:")
209+
})
210+
211+
t.Run("multiple change types", func(t *testing.T) {
212+
added := fieldpath.NewSet(fieldpath.MakePathOrDie("spec", "newField"))
213+
modified := fieldpath.NewSet(fieldpath.MakePathOrDie("spec", "replicas"))
214+
removed := fieldpath.NewSet(fieldpath.MakePathOrDie("metadata", "annotations"))
215+
comparison := &typed.Comparison{
216+
Added: added,
217+
Modified: modified,
218+
Removed: removed,
219+
}
220+
result := formatChangedFields(comparison)
221+
assert.Contains(t, result, "added:")
222+
assert.Contains(t, result, "modified:")
223+
assert.Contains(t, result, "removed:")
224+
assert.Contains(t, result, "; ")
225+
})
106226
}

0 commit comments

Comments
 (0)