Skip to content

Commit 9c34a1f

Browse files
committed
fix(falco): prevent spurious updates with managed fields comparison
Signed-off-by: cannarelladev <cannarella.dev@gmail.com>
1 parent f1dfde1 commit 9c34a1f

File tree

13 files changed

+1152
-523
lines changed

13 files changed

+1152
-523
lines changed

config/manager/kustomization.yaml

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -3,4 +3,3 @@ resources:
33
apiVersion: kustomize.config.k8s.io/v1beta1
44
kind: Kustomization
55
images:
6-

controllers/falco/controller.go

Lines changed: 51 additions & 40 deletions
Original file line numberDiff line numberDiff line change
@@ -231,49 +231,60 @@ func (r *Reconciler) ensureVersion(ctx context.Context, falco *instancev1alpha1.
231231

232232
// handleDeletion handles the deletion of the Falco instance.
233233
func (r *Reconciler) handleDeletion(ctx context.Context, falco *instancev1alpha1.Falco) (bool, error) {
234-
if falco.DeletionTimestamp != nil {
235-
log.FromContext(ctx).Info("Falco instance marked for deletion")
236-
if controllerutil.ContainsFinalizer(falco, finalizer) {
237-
log.FromContext(ctx).Info("Removing finalizer", "finalizer", finalizer)
238-
239-
resourceName := GenerateUniqueName(falco.Name, falco.Namespace)
240-
241-
crb := &unstructured.Unstructured{}
242-
crb.SetGroupVersionKind(schema.GroupVersionKind{
243-
Group: "rbac.authorization.k8s.io",
244-
Version: "v1",
245-
Kind: "ClusterRoleBinding",
246-
})
247-
crb.SetName(resourceName)
248-
if err := r.Delete(ctx, crb); err != nil && !apierrors.IsNotFound(err) {
249-
log.FromContext(ctx).Error(err, "unable to delete clusterrolebinding")
250-
return false, err
251-
}
252-
253-
cr := &unstructured.Unstructured{}
254-
cr.SetGroupVersionKind(schema.GroupVersionKind{
255-
Group: "rbac.authorization.k8s.io",
256-
Version: "v1",
257-
Kind: "ClusterRole",
258-
})
259-
cr.SetName(resourceName)
260-
if err := r.Delete(ctx, cr); err != nil && !apierrors.IsNotFound(err) {
261-
log.FromContext(ctx).Error(err, "unable to delete clusterrole")
262-
return false, err
263-
}
234+
if falco.DeletionTimestamp == nil {
235+
return false, nil
236+
}
264237

265-
if controllerutil.ContainsFinalizer(falco, finalizer) {
266-
patch := client.MergeFrom(falco.DeepCopy())
267-
controllerutil.RemoveFinalizer(falco, finalizer)
268-
if err := r.Patch(ctx, falco, patch); err != nil && !apierrors.IsNotFound(err) {
269-
log.FromContext(ctx).Error(err, "unable to remove finalizer from Falco instance")
270-
return false, err
271-
}
272-
}
273-
}
238+
// Check if finalizer is already removed
239+
if !controllerutil.ContainsFinalizer(falco, finalizer) {
240+
// Finalizer already removed, nothing to do
274241
return true, nil
275242
}
276-
return false, nil
243+
244+
log.FromContext(ctx).Info("Falco instance marked for deletion, removing finalizer", "finalizer", finalizer)
245+
246+
resourceName := GenerateUniqueName(falco.Name, falco.Namespace)
247+
248+
crb := &unstructured.Unstructured{}
249+
crb.SetGroupVersionKind(schema.GroupVersionKind{
250+
Group: "rbac.authorization.k8s.io",
251+
Version: "v1",
252+
Kind: "ClusterRoleBinding",
253+
})
254+
crb.SetName(resourceName)
255+
if err := r.Delete(ctx, crb); err != nil && !apierrors.IsNotFound(err) {
256+
log.FromContext(ctx).Error(err, "unable to delete clusterrolebinding")
257+
return false, err
258+
}
259+
260+
cr := &unstructured.Unstructured{}
261+
cr.SetGroupVersionKind(schema.GroupVersionKind{
262+
Group: "rbac.authorization.k8s.io",
263+
Version: "v1",
264+
Kind: "ClusterRole",
265+
})
266+
cr.SetName(resourceName)
267+
if err := r.Delete(ctx, cr); err != nil && !apierrors.IsNotFound(err) {
268+
log.FromContext(ctx).Error(err, "unable to delete clusterrole")
269+
return false, err
270+
}
271+
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
275+
patch := client.MergeFrom(falco.DeepCopy())
276+
controllerutil.RemoveFinalizer(falco, finalizer)
277+
if err := r.Patch(ctx, falco, patch); err != nil && !apierrors.IsNotFound(err) {
278+
log.FromContext(ctx).Error(err, "unable to remove finalizer from Falco instance")
279+
return false, err
280+
}
281+
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+
}
286+
287+
return true, nil
277288
}
278289

279290
// ensureDeployment ensures the Falco deployment or daemonset is created or updated.

controllers/falco/defaults.go

Lines changed: 6 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -44,13 +44,13 @@ var (
4444
{
4545
Name: "FALCO_HOSTNAME",
4646
ValueFrom: &corev1.EnvVarSource{
47-
FieldRef: &corev1.ObjectFieldSelector{FieldPath: "spec.nodeName"},
47+
FieldRef: &corev1.ObjectFieldSelector{APIVersion: "v1", FieldPath: "spec.nodeName"},
4848
},
4949
},
5050
{
5151
Name: "FALCO_K8S_NODE_NAME",
5252
ValueFrom: &corev1.EnvVarSource{
53-
FieldRef: &corev1.ObjectFieldSelector{FieldPath: "spec.nodeName"},
53+
FieldRef: &corev1.ObjectFieldSelector{APIVersion: "v1", FieldPath: "spec.nodeName"},
5454
},
5555
},
5656
}
@@ -397,15 +397,17 @@ webserver:
397397
Name: "POD_NAMESPACE",
398398
ValueFrom: &corev1.EnvVarSource{
399399
FieldRef: &corev1.ObjectFieldSelector{
400-
FieldPath: "metadata.namespace",
400+
APIVersion: "v1",
401+
FieldPath: "metadata.namespace",
401402
},
402403
},
403404
},
404405
{
405406
Name: "NODE_NAME",
406407
ValueFrom: &corev1.EnvVarSource{
407408
FieldRef: &corev1.ObjectFieldSelector{
408-
FieldPath: "spec.nodeName",
409+
APIVersion: "v1",
410+
FieldPath: "spec.nodeName",
409411
},
410412
},
411413
},

controllers/falco/diff.go

Lines changed: 44 additions & 94 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,4 @@
1-
// Copyright (C) 2026 The Falco Authors
1+
// Copyright (C) 2025 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.
@@ -18,122 +18,72 @@ package falco
1818

1919
import (
2020
"fmt"
21-
"strings"
2221

2322
"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
23+
"k8s.io/apimachinery/pkg/runtime"
2424
"sigs.k8s.io/structured-merge-diff/v4/typed"
2525

26-
"github.com/falcosecurity/falco-operator/internal/pkg/scheme"
26+
"github.com/falcosecurity/falco-operator/internal/pkg/managedfields"
2727
)
2828

29-
// diff calculates the difference between the current and desired objects.
30-
// It accepts either unstructured.Unstructured objects or typed objects that can be converted to unstructured.
29+
const (
30+
// fieldManager is the name used to identify the controller's managed fields.
31+
fieldManager = "falco-controller"
32+
)
33+
34+
// needsUpdate checks if the current object needs to be updated to match the desired state.
35+
// It extracts only the fields managed by this controller and compares them with the desired config.
3136
//
32-
// This is used to avoid unnecessary API writes on Kubernetes versions < 1.31 where
33-
// Server-Side Apply may cause spurious resourceVersion bumps on no-op patches to CRDs.
37+
// This avoids unnecessary API writes on Kubernetes versions < 1.31 where Server-Side Apply
38+
// may cause spurious resourceVersion bumps on no-op patches.
3439
// See: https://github.com/kubernetes/kubernetes/issues/124605
35-
func diff(current, desired interface{}) (*typed.Comparison, error) {
36-
// Convert inputs to unstructured if needed
37-
currentUnstructured, err := toUnstructured(current)
38-
if err != nil {
39-
return nil, fmt.Errorf("failed to convert current object to unstructured: %w", err)
40+
func needsUpdate(current runtime.Object, desired *unstructured.Unstructured) (bool, error) {
41+
if current == nil || desired == nil {
42+
return true, nil
4043
}
4144

42-
desiredUnstructured, err := toUnstructured(desired)
45+
// Extract only the fields managed by our field manager from the current object
46+
extracted, err := managedfields.ExtractAsUnstructured(current, fieldManager)
4347
if err != nil {
44-
return nil, fmt.Errorf("failed to convert desired object to unstructured: %w", err)
48+
return true, fmt.Errorf("failed to extract managed fields: %w", err)
4549
}
4650

47-
// Remove server-managed fields before comparison
48-
removeUnwantedFields(currentUnstructured)
49-
removeUnwantedFields(desiredUnstructured)
50-
51-
// Create a parser to compare the resources
52-
parser := scheme.Parser()
53-
54-
currentTypePath := getTypePath(currentUnstructured)
55-
56-
// Parse the base resource
57-
currentTyped, err := parser.Type(currentTypePath).FromUnstructured(currentUnstructured.Object)
58-
if err != nil {
59-
return nil, err
51+
// If no managed fields found, we need to apply
52+
if extracted == nil {
53+
return true, nil
6054
}
6155

62-
desiredTypePath := getTypePath(desiredUnstructured)
63-
// Parse the user defined resource
64-
desiredTyped, err := parser.Type(desiredTypePath).FromUnstructured(desiredUnstructured.Object)
65-
if err != nil {
66-
return nil, err
67-
}
56+
// Prune empty fields from both objects before comparison
57+
managedfields.PruneEmptyFields(extracted)
58+
managedfields.PruneEmptyFields(desired)
6859

69-
return currentTyped.Compare(desiredTyped)
60+
// Compare the extracted managed fields with the desired state
61+
return managedfields.NeedsUpdate(extracted, desired)
7062
}
7163

72-
// getTypePath returns the schema type path for an unstructured object.
73-
func getTypePath(obj *unstructured.Unstructured) string {
74-
apiVersion := obj.GetAPIVersion()
75-
resourceType := obj.GetKind()
76-
gv := strings.Split(apiVersion, "/")
77-
78-
// Build the schema path based on whether it's a core resource or not
79-
var typePath string
80-
if len(gv) == 1 {
81-
// Core resources like v1 have no group
82-
typePath = fmt.Sprintf("io.k8s.api.core.%s.%s", gv[0], resourceType)
83-
} else {
84-
// Other resources have group and version
85-
typePath = fmt.Sprintf("io.k8s.api.%s.%s.%s", apiGroupToSchemaGroup(gv[0]), gv[1], resourceType)
64+
// diff calculates the difference between the current and desired objects.
65+
// Returns a typed.Comparison that contains Added, Modified, and Removed field sets.
66+
func diff(current runtime.Object, desired *unstructured.Unstructured) (*typed.Comparison, error) {
67+
if current == nil || desired == nil {
68+
return nil, fmt.Errorf("current and desired objects cannot be nil")
8669
}
8770

88-
return typePath
89-
}
90-
91-
func apiGroupToSchemaGroup(apiGroup string) string {
92-
mappings := map[string]string{
93-
"rbac.authorization.k8s.io": "rbac",
94-
"networking.k8s.io": "networking",
95-
"certificates.k8s.io": "certificates",
96-
"storage.k8s.io": "storage",
97-
"admissionregistration.k8s.io": "admissionregistration",
98-
"scheduling.k8s.io": "scheduling",
99-
"coordination.k8s.io": "coordination",
100-
"discovery.k8s.io": "discovery",
71+
// Extract only the fields managed by our field manager
72+
extracted, err := managedfields.ExtractAsUnstructured(current, fieldManager)
73+
if err != nil {
74+
return nil, fmt.Errorf("failed to extract managed fields: %w", err)
10175
}
10276

103-
if mapped, ok := mappings[apiGroup]; ok {
104-
return mapped
77+
if extracted == nil {
78+
return nil, fmt.Errorf("no managed fields found for field manager %s", fieldManager)
10579
}
10680

107-
return apiGroup
108-
}
81+
// Deep copy desired to avoid modifying the original
82+
desiredCopy := desired.DeepCopy()
10983

110-
// removeUnwantedFields removes server-managed fields from the unstructured object
111-
// so they don't affect the comparison.
112-
func removeUnwantedFields(obj *unstructured.Unstructured) {
113-
unstructured.RemoveNestedField(obj.Object, "metadata", "uid")
114-
unstructured.RemoveNestedField(obj.Object, "metadata", "resourceVersion")
115-
unstructured.RemoveNestedField(obj.Object, "metadata", "managedFields")
116-
unstructured.RemoveNestedField(obj.Object, "status")
117-
unstructured.RemoveNestedField(obj.Object, "metadata", "creationTimestamp")
118-
unstructured.RemoveNestedField(obj.Object, "spec", "template", "metadata", "creationTimestamp")
119-
unstructured.RemoveNestedField(obj.Object, "spec", "revisionHistoryLimit")
120-
unstructured.RemoveNestedField(obj.Object, "metadata", "generateName")
121-
unstructured.RemoveNestedField(obj.Object, "metadata", "generation")
122-
// Remove the revision field from the annotations.
123-
unstructured.RemoveNestedField(obj.Object, "metadata", "annotations", "deployment.kubernetes.io/revision")
124-
// Remove the deprecated field from the annotations.
125-
unstructured.RemoveNestedField(obj.Object, "metadata", "annotations", "deprecated.daemonset.template.generation")
126-
// If the annotations field is empty, remove it.
127-
if metadata, ok := obj.Object["metadata"].(map[string]interface{}); ok {
128-
if annotations, ok := metadata["annotations"].(map[string]interface{}); ok {
129-
if len(annotations) == 0 {
130-
unstructured.RemoveNestedField(obj.Object, "metadata", "annotations")
131-
}
132-
}
133-
}
134-
// Only for services, remove the clusterIP and clusterIPs fields.
135-
if obj.GetKind() == "Service" {
136-
unstructured.RemoveNestedField(obj.Object, "spec", "clusterIP")
137-
unstructured.RemoveNestedField(obj.Object, "spec", "clusterIPs")
138-
}
84+
// Prune empty fields before comparison
85+
managedfields.PruneEmptyFields(extracted)
86+
managedfields.PruneEmptyFields(desiredCopy)
87+
88+
return managedfields.Compare(extracted, desiredCopy)
13989
}

0 commit comments

Comments
 (0)