Add --override-manager flag for server-side apply drift detection

internal/action/diff.go

@@ -27,6 +27,8 @@ import (
 	helmaction "helm.sh/helm/v3/pkg/action"
 	helmrelease "helm.sh/helm/v3/pkg/release"
 	apierrors "k8s.io/apimachinery/pkg/api/errors"
+	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
+	"k8s.io/apimachinery/pkg/apis/meta/v1/unstructured"
 	"k8s.io/apimachinery/pkg/types"
 	apierrutil "k8s.io/apimachinery/pkg/util/errors"
 	"k8s.io/utils/ptr"
@@ -40,12 +42,13 @@ import (
 	ssautil "github.com/fluxcd/pkg/ssa/utils"
 
 	v2 "github.com/fluxcd/helm-controller/api/v2"
+
 	"github.com/fluxcd/helm-controller/internal/diff"
 )
 
 // Diff returns a jsondiff.DiffSet of the changes between the state of the
 // cluster and the Helm release.Release manifest.
-func Diff(ctx context.Context, config *helmaction.Configuration, rls *helmrelease.Release, fieldOwner string, ignore ...v2.IgnoreRule) (jsondiff.DiffSet, error) {
+func Diff(ctx context.Context, config *helmaction.Configuration, rls *helmrelease.Release, fieldOwner string, disallowedFieldManagers []string, ignore ...v2.IgnoreRule) (jsondiff.DiffSet, error) {
 	// Create a dry-run only client to use solely for diffing.
 	cfg, err := config.RESTClientGetter.ToRESTConfig()
 	if err != nil {
@@ -96,6 +99,19 @@ func Diff(ctx context.Context, config *helmaction.Configuration, rls *helmreleas
 		}
 	}
 
+	if len(disallowedFieldManagers) > 0 {
+		c, err := client.New(cfg, client.Options{})
+		if err != nil {
+			return nil, err
+		}
+		for _, obj := range objects {
+			err = replaceDisallowedFieldManagers(ctx, c, fieldOwner, disallowedFieldManagers, obj)
+			if err != nil {
+				return nil, fmt.Errorf("failed to clean-up disallowed field managers: %w", err)
+			}
+		}
+	}
+
 	// Base configuration for the diffing of the object.
 	diffOpts := []jsondiff.ListOption{
 		jsondiff.FieldOwner(fieldOwner),
@@ -278,3 +294,42 @@ func (s sortableDiffs) Less(i, j int) bool {
 
 	return iDiff.GetName() < jDiff.GetName()
 }
+
+func replaceDisallowedFieldManagers(ctx context.Context, c client.Client, fieldOwner string, disallowedFieldManagers []string, obj *unstructured.Unstructured) error {
+	existingObj := &unstructured.Unstructured{}
+	existingObj.SetGroupVersionKind(obj.GroupVersionKind())
+	if err := c.Get(ctx, client.ObjectKeyFromObject(obj), existingObj); err != nil {
+		if apierrors.IsNotFound(err) {
+			return nil
+		}
+		return err
+	}
+
+	fieldManagers := []ssa.FieldManager{}
+	for _, fieldManager := range disallowedFieldManagers {
+		fieldManagers = append(fieldManagers, ssa.FieldManager{
+			Name:          fieldManager,
+			OperationType: metav1.ManagedFieldsOperationApply,
+		})
+		fieldManagers = append(fieldManagers, ssa.FieldManager{
+			Name:          fieldManager,
+			OperationType: metav1.ManagedFieldsOperationUpdate,
+		})
+	}
+
+	patches, err := ssa.PatchReplaceFieldsManagers(existingObj, fieldManagers, fieldOwner)
+	if err != nil {
+		return err
+	}
+	if len(patches) > 0 {
+		rawPatch, err := json.Marshal(patches)
+		if err != nil {
+			return err
+		}
+		err = c.Patch(ctx, existingObj, client.RawPatch(types.JSONPatchType, rawPatch), client.FieldOwner(fieldOwner))
+		if err != nil {
+			return err
+		}
+	}
+	return nil
+}

internal/action/diff_test.go

@@ -72,12 +72,15 @@ func TestDiff(t *testing.T) {
 	}
 
 	const testOwner = "helm-controller"
+	const ownerToOverride = "kubectl"
+	const ownerToKeep = "kube-controller-manager"
 
 	tests := []struct {
 		name          string
 		manifest      string
 		ignoreRules   []v2.IgnoreRule
 		mutateCluster func(objs []*unstructured.Unstructured, namespace string) ([]*unstructured.Unstructured, error)
+		updateCluster func(objs []*unstructured.Unstructured, namespace string) ([]*unstructured.Unstructured, client.FieldOwner, error)
 		want          func(namespace string, desired, cluster []*unstructured.Unstructured) jsondiff.DiffSet
 		wantErr       bool
 	}{
@@ -151,6 +154,91 @@ data:
 				}
 			},
 		},
+		{
+			name: "detects drift after kubectl edit",
+			manifest: `---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: changed
+data:
+  key: value`,
+			mutateCluster: func(objs []*unstructured.Unstructured, namespace string) ([]*unstructured.Unstructured, error) {
+				var clusterObjs []*unstructured.Unstructured
+				for _, obj := range objs {
+					obj := obj.DeepCopy()
+					obj.SetNamespace(namespace)
+					clusterObjs = append(clusterObjs, obj)
+				}
+				return clusterObjs, nil
+			},
+			updateCluster: func(objs []*unstructured.Unstructured, namespace string) ([]*unstructured.Unstructured, client.FieldOwner, error) {
+				var clusterObjs []*unstructured.Unstructured
+				for _, obj := range objs {
+					obj := obj.DeepCopy()
+					if err := unstructured.SetNestedField(obj.Object, "changed", "data", "anotherKey"); err != nil {
+						return nil, "", fmt.Errorf("failed to set nested field: %w", err)
+					}
+					clusterObjs = append(clusterObjs, obj)
+				}
+				return clusterObjs, ownerToOverride, nil
+			},
+			want: func(namespace string, desired, cluster []*unstructured.Unstructured) jsondiff.DiffSet {
+				return jsondiff.DiffSet{
+					{
+						Type:          jsondiff.DiffTypeUpdate,
+						DesiredObject: namespacedUnstructured(desired[0], namespace),
+						ClusterObject: cluster[0],
+						Patch: extjsondiff.Patch{
+							{
+								Type:     extjsondiff.OperationRemove,
+								Path:     "/data/anotherKey",
+								OldValue: "changed",
+							},
+						},
+					},
+				}
+			},
+		},
+		{
+			name: "detect no drift if edited by kube-controller-manager",
+			manifest: `---
+apiVersion: v1
+kind: ConfigMap
+metadata:
+  name: changed
+data:
+  key: value`,
+			mutateCluster: func(objs []*unstructured.Unstructured, namespace string) ([]*unstructured.Unstructured, error) {
+				var clusterObjs []*unstructured.Unstructured
+				for _, obj := range objs {
+					obj := obj.DeepCopy()
+					obj.SetNamespace(namespace)
+					clusterObjs = append(clusterObjs, obj)
+				}
+				return clusterObjs, nil
+			},
+			updateCluster: func(objs []*unstructured.Unstructured, namespace string) ([]*unstructured.Unstructured, client.FieldOwner, error) {
+				var clusterObjs []*unstructured.Unstructured
+				for _, obj := range objs {
+					obj := obj.DeepCopy()
+					if err := unstructured.SetNestedField(obj.Object, "changed", "data", "anotherKey"); err != nil {
+						return nil, "", fmt.Errorf("failed to set nested field: %w", err)
+					}
+					clusterObjs = append(clusterObjs, obj)
+				}
+				return clusterObjs, ownerToKeep, nil
+			},
+			want: func(namespace string, desired, cluster []*unstructured.Unstructured) jsondiff.DiffSet {
+				return jsondiff.DiffSet{
+					{
+						Type:          jsondiff.DiffTypeNone,
+						DesiredObject: namespacedUnstructured(desired[0], namespace),
+						ClusterObject: cluster[0],
+					},
+				}
+			},
+		},
 		{
 			name:     "empty release manifest",
 			manifest: "",
@@ -440,12 +528,34 @@ data:
 				}
 			}
 
-			got, err := Diff(ctx, &helmaction.Configuration{RESTClientGetter: getter}, rls, testOwner, tt.ignoreRules...)
+			if tt.updateCluster != nil {
+				// tt.updateCluster emulates out-of-band modifications like `kubectl edit`
+				var (
+					fieldOwner client.FieldOwner
+				)
+				if clusterObjs, fieldOwner, err = tt.updateCluster(clusterObjs, ns.Name); err != nil {
+					t.Fatalf("Failed to modify cluster resource: %v", err)
+				}
+				for _, obj := range clusterObjs {
+					if err := c.Update(ctx, obj, fieldOwner); err != nil {
+						t.Fatalf("Failed to update object: %v", err)
+					}
+				}
+			}
+
+			got, err := Diff(ctx, &helmaction.Configuration{RESTClientGetter: getter}, rls, testOwner, []string{ownerToOverride}, tt.ignoreRules...)
 			if (err != nil) != tt.wantErr {
 				t.Errorf("Diff() error = %v, wantErr %v", err, tt.wantErr)
 				return
 			}
 
+			// Refresh all objects since Diff might do mutations and this would change resourceVersion
+			for _, obj := range clusterObjs {
+				if err := c.Get(ctx, client.ObjectKeyFromObject(obj), obj); err != nil {
+					t.Fatalf("Failed to create object: %v", err)
+				}
+			}
+
 			var want jsondiff.DiffSet
 			if tt.want != nil {
 				want = tt.want(ns.Name, objs, clusterObjs)

internal/controller/helmrelease_controller.go

@@ -89,13 +89,14 @@ type HelmReleaseReconciler struct {
 
 	// Kubernetes configuration
 
-	FieldManager          string
-	DefaultServiceAccount string
-	GetClusterConfig      func() (*rest.Config, error)
-	ClientOpts            runtimeClient.Options
-	KubeConfigOpts        runtimeClient.KubeConfigOptions
-	APIReader             client.Reader
-	TokenCache            *cache.TokenCache
+	FieldManager            string
+	DisallowedFieldManagers []string
+	DefaultServiceAccount   string
+	GetClusterConfig        func() (*rest.Config, error)
+	ClientOpts              runtimeClient.Options
+	KubeConfigOpts          runtimeClient.KubeConfigOptions
+	APIReader               client.Reader
+	TokenCache              *cache.TokenCache
 
 	// Retry and requeue configuration
 
@@ -393,7 +394,7 @@ func (r *HelmReleaseReconciler) reconcileRelease(ctx context.Context, patchHelpe
 	}
 
 	// Off we go!
-	if err = intreconcile.NewAtomicRelease(patchHelper, cfg, r.EventRecorder, r.FieldManager).Reconcile(ctx, &intreconcile.Request{
+	if err = intreconcile.NewAtomicRelease(patchHelper, cfg, r.EventRecorder, r.FieldManager, r.DisallowedFieldManagers).Reconcile(ctx, &intreconcile.Request{
 		Object: obj,
 		Chart:  loadedChart,
 		Values: values,

internal/reconcile/atomic_release.go

@@ -113,22 +113,24 @@ var (
 // For more information on the individual ActionReconcilers, refer to their
 // documentation.
 type AtomicRelease struct {
-	patchHelper   *patch.SerialPatcher
-	configFactory *action.ConfigFactory
-	eventRecorder record.EventRecorder
-	strategy      releaseStrategy
-	fieldManager  string
+	patchHelper             *patch.SerialPatcher
+	configFactory           *action.ConfigFactory
+	eventRecorder           record.EventRecorder
+	strategy                releaseStrategy
+	fieldManager            string
+	disallowedFieldManagers []string
 }
 
 // NewAtomicRelease returns a new AtomicRelease reconciler configured with the
 // provided values.
-func NewAtomicRelease(patchHelper *patch.SerialPatcher, cfg *action.ConfigFactory, recorder record.EventRecorder, fieldManager string) *AtomicRelease {
+func NewAtomicRelease(patchHelper *patch.SerialPatcher, cfg *action.ConfigFactory, recorder record.EventRecorder, fieldManager string, disallowedFieldManagers []string) *AtomicRelease {
 	return &AtomicRelease{
-		patchHelper:   patchHelper,
-		eventRecorder: recorder,
-		configFactory: cfg,
-		strategy:      &cleanReleaseStrategy{},
-		fieldManager:  fieldManager,
+		patchHelper:             patchHelper,
+		eventRecorder:           recorder,
+		configFactory:           cfg,
+		strategy:                &cleanReleaseStrategy{},
+		fieldManager:            fieldManager,
+		disallowedFieldManagers: disallowedFieldManagers,
 	}
 }
 
@@ -188,7 +190,7 @@ func (r *AtomicRelease) Reconcile(ctx context.Context, req *Request) error {
 		default:
 			// Determine the current state of the Helm release.
 			log.V(logger.DebugLevel).Info("determining current state of Helm release")
-			state, err := DetermineReleaseState(ctx, r.configFactory, req)
+			state, err := DetermineReleaseState(ctx, r.configFactory, req, r.disallowedFieldManagers)
 			if err != nil {
 				conditions.MarkFalse(req.Object, meta.ReadyCondition, "StateError", "Could not determine release state: %s", err)
 				return fmt.Errorf("cannot determine release state: %w", err)

internal/reconcile/atomic_release_test.go

@@ -177,7 +177,7 @@ func TestAtomicRelease_Reconcile(t *testing.T) {
 			Chart:  testutil.BuildChart(testutil.ChartWithTestHook()),
 			Values: nil,
 		}
-		g.Expect(NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager).Reconcile(context.TODO(), req)).ToNot(HaveOccurred())
+		g.Expect(NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil).Reconcile(context.TODO(), req)).ToNot(HaveOccurred())
 
 		g.Expect(obj.Status.Conditions).To(conditions.MatchConditions([]metav1.Condition{
 			{
@@ -206,7 +206,7 @@ func TestAtomicRelease_Reconcile(t *testing.T) {
 		g.Expect(obj.Status.InstallFailures).To(BeZero())
 		g.Expect(obj.Status.UpgradeFailures).To(BeZero())
 
-		endState, err := DetermineReleaseState(ctx, cfg, req)
+		endState, err := DetermineReleaseState(ctx, cfg, req, nil)
 		g.Expect(err).ToNot(HaveOccurred())
 		g.Expect(endState).To(Equal(ReleaseState{Status: ReleaseStatusInSync}))
 	})
@@ -1229,7 +1229,7 @@ func TestAtomicRelease_Reconcile_Scenarios(t *testing.T) {
 				Values: tt.values,
 			}
 
-			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager).Reconcile(context.TODO(), req)
+			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil).Reconcile(context.TODO(), req)
 			wantErr := BeNil()
 			if tt.wantErr != nil {
 				wantErr = MatchError(tt.wantErr)
@@ -1460,7 +1460,7 @@ func TestAtomicRelease_Reconcile_PostRenderers_Scenarios(t *testing.T) {
 				Values: tt.values,
 			}
 
-			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager).Reconcile(context.TODO(), req)
+			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil).Reconcile(context.TODO(), req)
 			g.Expect(err).ToNot(HaveOccurred())
 
 			g.Expect(obj.Status.ObservedPostRenderersDigest).To(Equal(tt.wantDigest))
@@ -2401,7 +2401,7 @@ func TestAtomicRelease_Reconcile_CommonMetadata_Scenarios(t *testing.T) {
 				Values: tt.values,
 			}
 
-			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager).Reconcile(context.TODO(), req)
+			err = NewAtomicRelease(patchHelper, cfg, recorder, testFieldManager, nil).Reconcile(context.TODO(), req)
 			g.Expect(err).ToNot(HaveOccurred())
 
 			g.Expect(obj.Status.ObservedCommonMetadataDigest).To(Equal(tt.wantDigest))

internal/reconcile/state.go

@@ -89,7 +89,7 @@ type ReleaseState struct {
 // DetermineReleaseState determines the state of the Helm release as compared
 // to the v2.HelmRelease object. It returns a ReleaseState that indicates
 // the status of the release, and an error if the state could not be determined.
-func DetermineReleaseState(ctx context.Context, cfg *action.ConfigFactory, req *Request) (ReleaseState, error) {
+func DetermineReleaseState(ctx context.Context, cfg *action.ConfigFactory, req *Request, disallowedFieldManagers []string) (ReleaseState, error) {
 	rls, err := action.LastRelease(cfg.Build(nil), req.Object.GetReleaseName())
 	if err != nil {
 		if errors.Is(err, action.ErrReleaseNotFound) {
@@ -188,7 +188,7 @@ func DetermineReleaseState(ctx context.Context, cfg *action.ConfigFactory, req *
 
 		// Confirm the cluster state matches the desired config.
 		if diffOpts := req.Object.GetDriftDetection(); diffOpts.MustDetectChanges() {
-			diffSet, err := action.Diff(ctx, cfg.Build(nil), rls, kube.ManagedFieldsManager, req.Object.GetDriftDetection().Ignore...)
+			diffSet, err := action.Diff(ctx, cfg.Build(nil), rls, kube.ManagedFieldsManager, disallowedFieldManagers, req.Object.GetDriftDetection().Ignore...)
 			hasChanges := diffSet.HasChanges()
 			if err != nil {
 				if !hasChanges {

internal/reconcile/state_test.go

@@ -636,7 +636,7 @@ func Test_DetermineReleaseState(t *testing.T) {
 				Object: obj,
 				Chart:  tt.chart,
 				Values: tt.values,
-			})
+			}, nil)
 			if tt.wantErr {
 				g.Expect(got).To(BeNil())
 				g.Expect(err).To(HaveOccurred())
@@ -816,7 +816,7 @@ func TestDetermineReleaseState_DriftDetection(t *testing.T) {
 				Object: obj,
 				Chart:  testutil.BuildChart(),
 				Values: rls.Config,
-			})
+			}, nil)
 			g.Expect(err).ToNot(HaveOccurred())
 
 			want := tt.want(releaseNamespace)